diff --git a/CHANGELOG.md b/CHANGELOG.md index 18a1e57..a5c4368 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -114,6 +114,25 @@ Also corrected in the same pass: the toolchain table is described as executed by the oxc row — the only ✗ — cannot be, because oxc ships inside a native binary with no standalone transform API. The claim now covers the three rows it actually covers. +### A sharp edge, now documented and pinned + +The README, the landing page and two doc comments all said that once a property carries +`@JsonProperty`, its original name "is no longer accepted on input". It is no longer +*mapped* — but it is not rejected either. Unlike `@JsonReadOnly`, whose JSON name goes into +the blocked set, a renamed property's old key falls through to the unknown-key policy, and +the default `allow` copies it onto the instance untouched. The value lands on a declared +property having skipped everything declared for it: no `@JsonType` conversion, so +`@ValidateNested` then inspects a plain object with no model and reports nothing. + +Blocking the old key would fix that, but it would also swallow the `unknownKeys: 'error'` +report a strict caller gets today, which is arguably the more useful signal. That decision +has not been made, so the behaviour is stated accurately everywhere it was previously stated +wrongly, and five tests in `mapping.test.ts` pin it — including the `strip` and `error` +policies and the `@JsonAlias` fix — so it cannot change by accident either way. + +Two reference entries were also imprecise: `@IsNotEmpty()` and `@IsEmpty()` read as +complements but are not (`[]` and `{}` pass both), and `unknownKeys` is deserialization-only. + ### Positioning `zod-alternative` is out of the keywords, and the README leads with the comparison that diff --git a/README.md b/README.md index 855f05b..2b780e6 100644 --- a/README.md +++ b/README.md @@ -507,8 +507,12 @@ it is one `Symbol.toStringTag` read per object. - **`validate()` on a plain object** returns no errors: rules live on the class, so validate the instance you get back from `toInstance`, not the raw payload. - **Renaming is not backwards-compatible by itself.** Once a property carries - `@JsonProperty`, its original name is no longer accepted on input — add `@JsonAlias` to - keep older clients working. + `@JsonProperty`, its original name no longer *maps* to it. Under the default + `unknownKeys: 'allow'` it is not rejected either: it is copied onto the instance raw, + skipping any `@JsonType` or `@JsonDeserialize` conversion declared for that field, so the + property ends up holding a plain object that `@ValidateNested` then finds nothing wrong + with. Add `@JsonAlias` to keep older clients working, or set `unknownKeys` to `strip` or + `error`. ## Contributing diff --git a/docs/index.html b/docs/index.html index e09561e..984e599 100644 --- a/docs/index.html +++ b/docs/index.html @@ -813,8 +813,13 @@ npm install ../cereale/cereale-0.3.0.tgz

Renaming is not backwards-compatible by itself

-

Once a property carries @JsonProperty, its original name is - no longer accepted on input. Add @JsonAlias to keep older clients working.

+

Once a property carries @JsonProperty, its original name + no longer maps to it — but under the default + unknownKeys: 'allow' it is not rejected either. It is + copied onto the instance raw, skipping any + @JsonType or @JsonDeserialize + conversion declared for that field. Add @JsonAlias to keep + older clients working, or unknownKeys: 'strip' to drop them.

abstract and accessor fields cannot be decorated

diff --git a/docs/page.js b/docs/page.js index af08448..b812f1c 100644 --- a/docs/page.js +++ b/docs/page.js @@ -132,8 +132,8 @@ ["@IsDate()", 'must be a valid Date object'], ["@IsObject()", 'must be an object'], ["@IsDefined()", 'must not be null or undefined'], - ["@IsNotEmpty()", 'must not be empty'], - ["@IsEmpty()", 'must be empty'] + ["@IsNotEmpty()", 'must not be null, undefined or an empty string — [] and {} pass'], + ["@IsEmpty()", 'must be null, undefined, an empty string, [] or {}'] ]], ['Numbers', 'Constraints on number fields.', [ ["@Min(n)", 'must be at least n'], @@ -218,7 +218,7 @@ ['Configuration', 'Per call, or once via configure().', [ ["validate: boolean", 'validate while mapping — default true'], ["namingStrategy: strategy", 'identity (default), camelCase, PascalCase, snake_case, SCREAMING_SNAKE_CASE, kebab-case, or your own function'], - ["unknownKeys: policy", 'allow (default), strip, or error'], + ["unknownKeys: policy", 'allow (default), strip, or error — deserialization only'], ["maxDepth: number", 'nesting limit before a JsonMappingError — default 64'], ["configure(options)", 'sets the library-wide defaults'], ["getConfig()", 'reads the defaults currently in force'], diff --git a/src/decorators.ts b/src/decorators.ts index b619a73..be4423f 100644 --- a/src/decorators.ts +++ b/src/decorators.ts @@ -68,7 +68,9 @@ function pattern(name: string, regex: RegExp, message: (property: string) => str * ``` * * An explicit name always wins over the active naming strategy. Note that renaming stops the - * original name from being accepted on input — add `@JsonAlias` to keep older clients working. + * original name from *mapping* to it. Under the default `unknownKeys: 'allow'` that name is + * still copied onto the instance raw, bypassing any `@JsonType` or `@JsonDeserialize` declared + * for the field — add `@JsonAlias` to keep older clients working, or set `unknownKeys`. */ export function JsonProperty(name: string): FieldDecorator { return ((_t: undefined, context: ClassFieldDecoratorContext) => { diff --git a/src/mapping.test.ts b/src/mapping.test.ts index 5c6f331..e498034 100644 --- a/src/mapping.test.ts +++ b/src/mapping.test.ts @@ -447,3 +447,92 @@ describe('a realistic API payload', () => { expect(response).not.toContain('hunter2'); }); }); + +describe('renaming and the unknown-key policy', () => { + class Address { + @IsString() city!: string; + } + + class Order { + @JsonProperty('order_ref') + @IsString() + ref!: string; + + @JsonProperty('home_address') + @JsonType(() => Address) + @ValidateNested() + address!: Address; + } + + /** + * A sharp edge, pinned here rather than fixed, because the fix is a judgement call the + * library has not made yet. + * + * `@JsonReadOnly` puts its JSON name in the blocked set, so a client cannot set it. + * `@JsonProperty` does not do the same for the property's *old* name: that name simply + * stops mapping, falls through to the unknown-key policy, and the default `allow` copies + * it onto the instance untouched. The value therefore lands on a declared property having + * skipped everything declared for it — no `@JsonType` conversion, and `@ValidateNested` + * then finds a plain object with no model and reports nothing. + * + * Blocking the old key would fix that, but it would also swallow the `unknownKeys: 'error'` + * report that a strict caller gets today, which is arguably the more useful signal. Until + * that is decided, the behaviour is documented in the README and on the landing page, and + * asserted here so it cannot change by accident. + */ + it('leaves the old key writable after a rename, under the default policy', async () => { + const order = await toInstance( + Order, + { ref: 'A-1', address: { city: 'Paris' } }, + { validate: false } + ); + + expect(order.ref).toBe('A-1'); + // The conversion declared for the field did not run. + expect(order.address).toEqual({ city: 'Paris' }); + expect(order.address instanceof Address).toBe(false); + // And nothing complains about it. + expect(await validate(order)).toEqual([]); + }); + + it('maps the declared names properly, producing real instances', async () => { + const order = await toInstance( + Order, + { order_ref: 'A-1', home_address: { city: 'Paris' } }, + { validate: false } + ); + + expect(order.ref).toBe('A-1'); + expect(order.address instanceof Address).toBe(true); + }); + + it('drops the old key under unknownKeys: strip', async () => { + const order = await toInstance( + Order, + { ref: 'A-1', address: { city: 'Paris' } }, + { validate: false, unknownKeys: 'strip' } + ); + + expect(order.ref).toBeUndefined(); + expect(order.address).toBeUndefined(); + }); + + it('reports the old key under unknownKeys: error', async () => { + await expect( + toInstance(Order, { ref: 'A-1' }, { validate: false, unknownKeys: 'error' }) + ).rejects.toThrow(/Unknown property "ref"/); + }); + + it('keeps the old name working properly when @JsonAlias declares it', async () => { + class Kept { + @JsonProperty('order_ref') + @JsonAlias('ref') + @IsString() + ref!: string; + } + + const kept = await toInstance(Kept, { ref: 'A-1' }, { validate: false }); + expect(kept.ref).toBe('A-1'); + expect(await validate(kept)).toEqual([]); + }); +}); diff --git a/src/utils.ts b/src/utils.ts index 1381eaf..ef472f5 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -160,10 +160,15 @@ const inboundCache = new WeakMap property-key lookup used when reading a payload. * - * Only names the class actually declares are accepted: the `@JsonProperty` name (or the - * naming strategy's rendering of the property name) plus any `@JsonAlias`. Renaming a - * property therefore stops the old name from being silently accepted — add `@JsonAlias` to - * keep it working for older clients. + * Only names the class actually declares are mapped: the `@JsonProperty` name (or the naming + * strategy's rendering of the property name) plus any `@JsonAlias`. + * + * Note what this does *not* do. A renamed property's old name stops mapping to it, but it is + * not blocked — unlike `@JsonReadOnly`, which is. It falls through to the unknown-key policy, + * and the default `allow` copies it onto the instance raw, bypassing the `@JsonType` or + * `@JsonDeserialize` declared for that field. `renames leave the old key writable` in + * mapping.test.ts pins that behaviour; see the note there for why it has not simply been + * changed. */ function inboundNameMap(model: ClassModel, ctx: DeserializeContext): InboundNames { let entry = inboundCache.get(model);