🔍 fix: correct the last three findings from the page review
Of 26 findings raised across five auditors, 23 were refuted on a second
pass. These three survived.
**A renamed property's old key is still writable.** 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, the old key falls through to the
unknown-key policy, and the default `allow` copies it onto the instance
untouched. Reproduced against dist:
@JsonProperty('home_address') @JsonType(() => Addr) @ValidateNested()
address!: Addr;
toInstanceSync(Order, { address: { city: 'Paris' } })
-> address is a plain object, instanceof Addr === false
-> validateSync() returns [] <- nothing complains
-> round-trips out as home_address <- silently accepted
Blocking the old key would fix it, but would also swallow the
`unknownKeys: 'error'` report a strict caller gets today, which is arguably
the more useful signal. That is a judgement call the library has not made,
so this commit states the behaviour accurately everywhere it was stated
wrongly and pins it with five tests covering the default, `strip`, `error`
and the @JsonAlias fix — so it cannot drift either way while the question
is open.
**@IsNotEmpty and @IsEmpty are not complements.** `[]` and `{}` pass BOTH:
isNotEmpty checks only null/undefined/'' while isEmpty also treats empty
arrays and objects as empty. Listed one line apart as "must not be empty" /
"must be empty", they invited exactly the wrong inference.
**unknownKeys is deserialization-only**, in a group whose blurb says these
apply per call or via configure().
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SAcqrz3FcadkYr3xG32CjK
This commit is contained in:
@@ -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
|
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.
|
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
|
### Positioning
|
||||||
|
|
||||||
`zod-alternative` is out of the keywords, and the README leads with the comparison that
|
`zod-alternative` is out of the keywords, and the README leads with the comparison that
|
||||||
|
|||||||
@@ -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
|
- **`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.
|
the instance you get back from `toInstance`, not the raw payload.
|
||||||
- **Renaming is not backwards-compatible by itself.** Once a property carries
|
- **Renaming is not backwards-compatible by itself.** Once a property carries
|
||||||
`@JsonProperty`, its original name is no longer accepted on input — add `@JsonAlias` to
|
`@JsonProperty`, its original name no longer *maps* to it. Under the default
|
||||||
keep older clients working.
|
`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
|
## Contributing
|
||||||
|
|
||||||
|
|||||||
+7
-2
@@ -813,8 +813,13 @@ npm install ../cereale/cereale-0.3.0.tgz</code></pre>
|
|||||||
<div class="notes">
|
<div class="notes">
|
||||||
<div class="note-item">
|
<div class="note-item">
|
||||||
<h3>Renaming is not backwards-compatible by itself</h3>
|
<h3>Renaming is not backwards-compatible by itself</h3>
|
||||||
<p>Once a property carries <code class="inline-code">@JsonProperty</code>, its original name is
|
<p>Once a property carries <code class="inline-code">@JsonProperty</code>, its original name
|
||||||
no longer accepted on input. Add <code class="inline-code">@JsonAlias</code> to keep older clients working.</p>
|
no longer <em>maps</em> to it — but under the default
|
||||||
|
<code class="inline-code">unknownKeys: 'allow'</code> it is not rejected either. It is
|
||||||
|
copied onto the instance raw, skipping any
|
||||||
|
<code class="inline-code">@JsonType</code> or <code class="inline-code">@JsonDeserialize</code>
|
||||||
|
conversion declared for that field. Add <code class="inline-code">@JsonAlias</code> to keep
|
||||||
|
older clients working, or <code class="inline-code">unknownKeys: 'strip'</code> to drop them.</p>
|
||||||
</div>
|
</div>
|
||||||
<div class="note-item">
|
<div class="note-item">
|
||||||
<h3><code class="inline-code">abstract</code> and <code class="inline-code">accessor</code> fields cannot be decorated</h3>
|
<h3><code class="inline-code">abstract</code> and <code class="inline-code">accessor</code> fields cannot be decorated</h3>
|
||||||
|
|||||||
+3
-3
@@ -132,8 +132,8 @@
|
|||||||
["@IsDate()", 'must be a valid Date object'],
|
["@IsDate()", 'must be a valid Date object'],
|
||||||
["@IsObject()", 'must be an object'],
|
["@IsObject()", 'must be an object'],
|
||||||
["@IsDefined()", 'must not be null or undefined'],
|
["@IsDefined()", 'must not be null or undefined'],
|
||||||
["@IsNotEmpty()", 'must not be empty'],
|
["@IsNotEmpty()", 'must not be null, undefined or an empty string — [] and {} pass'],
|
||||||
["@IsEmpty()", 'must be empty']
|
["@IsEmpty()", 'must be null, undefined, an empty string, [] or {}']
|
||||||
]],
|
]],
|
||||||
['Numbers', 'Constraints on number fields.', [
|
['Numbers', 'Constraints on number fields.', [
|
||||||
["@Min(n)", 'must be at least n'],
|
["@Min(n)", 'must be at least n'],
|
||||||
@@ -218,7 +218,7 @@
|
|||||||
['Configuration', 'Per call, or once via configure().', [
|
['Configuration', 'Per call, or once via configure().', [
|
||||||
["validate: boolean", 'validate while mapping — default true'],
|
["validate: boolean", 'validate while mapping — default true'],
|
||||||
["namingStrategy: strategy", 'identity (default), camelCase, PascalCase, snake_case, SCREAMING_SNAKE_CASE, kebab-case, or your own function'],
|
["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'],
|
["maxDepth: number", 'nesting limit before a JsonMappingError — default 64'],
|
||||||
["configure(options)", 'sets the library-wide defaults'],
|
["configure(options)", 'sets the library-wide defaults'],
|
||||||
["getConfig()", 'reads the defaults currently in force'],
|
["getConfig()", 'reads the defaults currently in force'],
|
||||||
|
|||||||
+3
-1
@@ -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
|
* 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<unknown> {
|
export function JsonProperty(name: string): FieldDecorator<unknown> {
|
||||||
return ((_t: undefined, context: ClassFieldDecoratorContext) => {
|
return ((_t: undefined, context: ClassFieldDecoratorContext) => {
|
||||||
|
|||||||
@@ -447,3 +447,92 @@ describe('a realistic API payload', () => {
|
|||||||
expect(response).not.toContain('hunter2');
|
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([]);
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|||||||
+9
-4
@@ -160,10 +160,15 @@ const inboundCache = new WeakMap<ClassModel, { version: number; byStrategy: Map<
|
|||||||
/**
|
/**
|
||||||
* Builds the JSON-name -> property-key lookup used when reading a payload.
|
* Builds the JSON-name -> property-key lookup used when reading a payload.
|
||||||
*
|
*
|
||||||
* Only names the class actually declares are accepted: the `@JsonProperty` name (or the
|
* Only names the class actually declares are mapped: the `@JsonProperty` name (or the naming
|
||||||
* naming strategy's rendering of the property name) plus any `@JsonAlias`. Renaming a
|
* strategy's rendering of the property name) plus any `@JsonAlias`.
|
||||||
* property therefore stops the old name from being silently accepted — add `@JsonAlias` to
|
*
|
||||||
* keep it working for older clients.
|
* 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 {
|
function inboundNameMap(model: ClassModel, ctx: DeserializeContext): InboundNames {
|
||||||
let entry = inboundCache.get(model);
|
let entry = inboundCache.get(model);
|
||||||
|
|||||||
Reference in New Issue
Block a user