🔒 fix!: refuse names that no longer reach their property

A rename did not actually take effect. @JsonProperty stopped the old name
from being *mapped*, but not from being *accepted*: unlike @JsonReadOnly,
whose JSON name goes into the blocked set, the old key fell through to the
unknown-key policy, and the default `allow` copied it onto the instance
untouched.

The value therefore landed on a declared property having skipped everything
declared for it:

  @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

A payload aimed at the previous version of a class was accepted in part, in
silence. Three routes led to the same hole, and all three are now closed:

- the property key of a field renamed with @JsonProperty
- the raw key of a field a naming strategy renders differently
  (`firstName` under snake_case)
- the property key of a field that is both renamed and @JsonReadOnly, which
  was still settable under its own key

Refused, not swallowed. A stale name is a mismatch with whatever produced
the payload, not a deliberate refusal like @JsonReadOnly, so it is kept in
its own map rather than lumped into `blocked`: under unknownKeys: 'error'
it is still reported, and the report now names the property it was reaching
for and what that property is called now.

  "ref" is not a JSON name for Order: property "ref" is mapped to
  "order_ref". Send that name, or add @JsonAlias("ref") to keep
  accepting this one.

@JsonAlias still keeps an old name working, and a key that some *other*
property legitimately answers to is still mapped to that property — both
asserted. The resolution happens once when the name map is built, which is
memoized per class and naming strategy, so deserialization is unchanged at
~45µs for 50 nested orders.

The behaviour this replaces was pinned by tests two commits ago, pending
this decision; those tests now assert the fix, and six more cover the
naming-strategy, read-only, alias and key-collision cases.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SAcqrz3FcadkYr3xG32CjK

src/decorators.ts

@@ -68,9 +68,8 @@ 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 *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`.
+ * original name from reaching it: the old name is refused rather than copied onto the instance
+ * behind the rename's back. Add `@JsonAlias` to keep older clients working.
  */
 export function JsonProperty(name: string): FieldDecorator<unknown> {
   return ((_t: undefined, context: ClassFieldDecoratorContext) => {

src/mapping.test.ts

@@ -465,34 +465,30 @@ describe('renaming and the unknown-key policy', () => {
   }
 
   /**
-   * A sharp edge, pinned here rather than fixed, because the fix is a judgement call the
-   * library has not made yet.
+   * A rename has to actually take effect.
    *
-   * `@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.
+   * The old key used to fall through to the unknown-key policy, and the default `allow`
+   * copied it onto the instance untouched — landing a value on a declared property having
+   * skipped the `@JsonType` declared for it, so `@ValidateNested` then inspected a plain
+   * object with no model and reported nothing. A payload aimed at the previous version of
+   * this class was accepted in part, in silence.
    */
-  it('leaves the old key writable after a rename, under the default policy', async () => {
+  it('does not write the old key after a rename', 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([]);
+    expect(order.ref).toBeUndefined();
+    expect(order.address).toBeUndefined();
+  });
+
+  it('lets validation report the fields the stale payload failed to fill', async () => {
+    const order = await toInstance(Order, { ref: 'A-1' }, { validate: false });
+    const errors = flattenErrors(await validate(order));
+
+    expect(Object.keys(errors)).toContain('ref');
   });
 
   it('maps the declared names properly, producing real instances', async () => {
@@ -506,24 +502,23 @@ describe('renaming and the unknown-key policy', () => {
     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 () => {
+  // A stale name is a mismatch with whatever produced the payload, not a deliberate refusal
+  // like @JsonReadOnly, so a caller who asked to hear about unrecognised keys hears about it —
+  // and is told which property it was reaching for and what that property is called now.
+  it('names the property and its current JSON name under unknownKeys: error', async () => {
     await expect(
       toInstance(Order, { ref: 'A-1' }, { validate: false, unknownKeys: 'error' })
-    ).rejects.toThrow(/Unknown property "ref"/);
+    ).rejects.toThrow(/"ref" is not a JSON name for Order.*mapped to "order_ref".*@JsonAlias\("ref"\)/s);
   });
 
-  it('keeps the old name working properly when @JsonAlias declares it', async () => {
+  it('drops the old key silently under strip and allow alike', async () => {
+    for (const unknownKeys of ['strip', 'allow'] as const) {
+      const order = await toInstance(Order, { ref: 'A-1' }, { validate: false, unknownKeys });
+      expect(order.ref, unknownKeys).toBeUndefined();
+    }
+  });
+
+  it('keeps the old name working when @JsonAlias declares it', async () => {
     class Kept {
       @JsonProperty('order_ref')
       @JsonAlias('ref')
@@ -535,4 +530,53 @@ describe('renaming and the unknown-key policy', () => {
     expect(kept.ref).toBe('A-1');
     expect(await validate(kept)).toEqual([]);
   });
+
+  it('refuses a property key that a naming strategy renders differently', async () => {
+    class Account {
+      @IsString() firstName!: string;
+    }
+
+    const snake = { namingStrategy: 'snake_case' as const, validate: false };
+    expect((await toInstance(Account, { firstName: 'Ada' }, snake)).firstName).toBeUndefined();
+    expect((await toInstance(Account, { first_name: 'Ada' }, snake)).firstName).toBe('Ada');
+  });
+
+  // The same protection by a different route: a read-only property that was also renamed was
+  // still settable under its own key.
+  it('blocks the property key of a renamed read-only field', async () => {
+    class Server {
+      @JsonProperty('server_id')
+      @JsonReadOnly()
+      @IsString()
+      id!: string;
+
+      @IsString() name!: string;
+    }
+
+    const server = await toInstance(
+      Server,
+      { id: 'client-supplied', server_id: 'also-client-supplied', name: 'x' },
+      { validate: false }
+    );
+
+    expect(server.id).toBeUndefined();
+    expect(server.name).toBe('x');
+  });
+
+  // A key one property no longer answers to may be exactly what another property is called.
+  it('still maps a key that another property legitimately claims', async () => {
+    class Shuffled {
+      @JsonProperty('other')
+      @IsString()
+      a!: string;
+
+      @JsonAlias('a')
+      @IsString()
+      b!: string;
+    }
+
+    const shuffled = await toInstance(Shuffled, { other: 'x', a: 'y' }, { validate: false });
+    expect(shuffled.a).toBe('x');
+    expect(shuffled.b).toBe('y');
+  });
 });

src/utils.ts

@@ -146,11 +146,23 @@ interface InboundNames {
   props: Map<string, InboundProperty>;
   /**
    * JSON names that belong to a declared property the payload may NOT set
-   * (`@JsonIgnore` / `@JsonReadOnly`). They are dropped rather than treated as unknown
-   * keys — otherwise the default `unknownKeys: 'allow'` policy would copy them straight
-   * back onto the instance and undo the protection.
+   * (`@JsonIgnore` / `@JsonReadOnly`), plus those properties' own keys. They are dropped
+   * rather than treated as unknown keys — otherwise the default `unknownKeys: 'allow'` policy
+   * would copy them straight back onto the instance and undo the protection.
    */
   blocked: Set<string>;
+  /**
+   * Names that used to reach a declared property but no longer do: the property key of a
+   * field renamed by `@JsonProperty`, or its raw key under a naming strategy that renders it
+   * differently.
+   *
+   * These are kept apart from `blocked` because they mean something different. A blocked name
+   * is a deliberate refusal, so it is dropped in silence. A stale name is a mismatch between
+   * this class and whatever produced the payload, which a caller who asked for
+   * `unknownKeys: 'error'` wants to hear about — and can be told precisely, since we know
+   * which property it was reaching for and what that property is called now.
+   */
+  stale: Map<string, { property: string; accepted: string }>;
 }
 
 // Name maps are derived purely from decorator metadata, which is fixed once a class is
@@ -163,12 +175,13 @@ const inboundCache = new WeakMap<ClassModel, { version: number; byStrategy: Map<
  * 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.
+ * A name that no longer reaches its property must not fall through to the unknown-key policy,
+ * because `allow` would then copy it onto the instance raw — landing a value on a declared
+ * property having skipped the `@JsonType` or `@JsonDeserialize` declared for it, so that
+ * `@ValidateNested` finds a plain object with no model and reports nothing. Renaming a
+ * property has to actually take effect. Those names go into `stale` (reported under `error`,
+ * dropped otherwise), and the keys of properties the payload may not set at all go into
+ * `blocked` (always dropped in silence).
  */
 function inboundNameMap(model: ClassModel, ctx: DeserializeContext): InboundNames {
   let entry = inboundCache.get(model);
@@ -181,8 +194,14 @@ function inboundNameMap(model: ClassModel, ctx: DeserializeContext): InboundName
 
   const accept = new Map<string, string>();
   const blocked = new Set<string>();
+  const stale = new Map<string, { property: string; accepted: string }>();
   const props = new Map<string, InboundProperty>();
 
+  // Whether a property key is also some property's accepted JSON name is only known once
+  // every property has been walked, so these are resolved after the loop.
+  const shadowed: string[] = [];
+  const staleCandidates: { property: string; accepted: string }[] = [];
+
   const claim = (external: string, key: string) => {
     const owner = accept.get(external);
     if (owner && owner !== key) {
@@ -200,10 +219,14 @@ function inboundNameMap(model: ClassModel, ctx: DeserializeContext): InboundName
     const access = accessOf(model, key);
     if (access === 'none' || access === 'readonly') {
       for (const name of names) blocked.add(name);
+      // A renamed read-only property would otherwise still be settable under its own key,
+      // which is the protection undone by a different route.
+      shadowed.push(key);
       continue;
     }
 
     for (const name of names) claim(name, key);
+    if (!names.includes(key)) staleCandidates.push({ property: key, accepted: names[0]! });
 
     if (property.deserializer || property.polymorphic || property.type) {
       props.set(key, {
@@ -214,7 +237,18 @@ function inboundNameMap(model: ClassModel, ctx: DeserializeContext): InboundName
     }
   }
 
-  const result = { accept, blocked, props };
+  // A property key that another property legitimately answers to stays mapped; only keys that
+  // nothing accepts are refused.
+  for (const key of shadowed) {
+    if (!accept.has(key)) blocked.add(key);
+  }
+  for (const candidate of staleCandidates) {
+    if (!accept.has(candidate.property) && !blocked.has(candidate.property)) {
+      stale.set(candidate.property, candidate);
+    }
+  }
+
+  const result = { accept, blocked, stale, props };
   entry.byStrategy.set(ctx.namingKey, result);
   return result;
 }
@@ -473,6 +507,22 @@ function deserialize<T>(clazz: ClassConstructor<T>, plain: any, ctx: Deserialize
     // than quietly refusing to honour it.
     if (inbound.blocked.has(incoming)) continue;
 
+    // A name that used to reach a declared property. Never assigned — doing so would land the
+    // value on that property having skipped every conversion declared for it — but a caller
+    // who asked to hear about unrecognised keys hears about this one by name, because it is a
+    // mismatch with whatever produced the payload rather than a deliberate refusal.
+    const outdated = inbound.stale.get(incoming);
+    if (outdated !== undefined) {
+      if (ctx.unknownKeys === 'error') {
+        throw new JsonMappingError(
+          `${JSON.stringify(incoming)} is not a JSON name for ${clazz.name}: property ` +
+          `"${outdated.property}" is mapped to ${JSON.stringify(outdated.accepted)}. ` +
+          `Send that name, or add @JsonAlias(${JSON.stringify(incoming)}) to keep accepting this one.`
+        );
+      }
+      continue;
+    }
+
     const key = inbound.accept.get(incoming);
     if (key === undefined) {
       // Not a declared property under the active naming strategy.