nest icon indicating copy to clipboard operation
nest copied to clipboard

fix(core): correct misunderstanding of optional metadata

Open sukjuhong opened this issue 6 months ago • 2 comments
trafficstars

PR Checklist

Please check if your PR fulfills the following requirements:

  • [x] The commit message follows our guidelines: https://github.com/nestjs/nest/blob/master/CONTRIBUTING.md
  • [x] Tests for the changes have been added (for bug fixes / features)
  • [ ] Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • [x] Bugfix
  • [ ] Feature
  • [ ] Code style update (formatting, local variables)
  • [ ] Refactoring (no functional changes, no api changes)
  • [ ] Build related changes
  • [ ] CI related changes
  • [ ] Other... Please describe:

What is the current behavior?

Issue Number: resolves #2581

Currently, Nest does not throw an error even when a child class has incomplete dependencies due to an optional parameter declared in a parent class.

To demonstrate the reason for this behavior, here is a simplified reproduction:

function Injectable(): ClassDecorator {
  return (target) => {};
}

function Optional(): ParameterDecorator {
  return (target, key, index) => {
    const args = (Reflect.getMetadata("optional", target) ?? []);
    Reflect.defineMetadata("optional", [...args, index], target);
  };
}

class Foo {}

@Injectable()
class Parent {
  constructor(@Optional() private readonly foo: Foo) {}
}

@Injectable()
class Child extends Parent {
  constructor() {
    super(new Foo());
  }
}

console.log(Reflect.getMetadata("design:paramtypes", Parent)); // [ [class Foo] ]
console.log(Reflect.getMetadata("design:paramtypes", Child)); // [ ]

console.log(Reflect.getMetadata("optional", Parent)); // [ 0 ]
console.log(Reflect.getMetadata("optional", Child)); // [ 0 ]

In this example, I created a decorator that behaves the same way as in NestJS to test the behavior.

Looking at the current state, design:paramtypes reflects only the parameters of the respective class. So for Parent, it contains class Foo, whereas for Child, it returns an empty array. (This is because the constructor overrides the metadata. If Child did not define its own constructor, it would inherit class Foo just like Parent.)

However, in the case of the @Optional() decorator, the metadata is not overridden and is inherited by Child as is.

This difference in how metadata is inherited leads to the core issue.

At first, I suspected that this might be caused by the mixin mechanism, but it turns out the actual issue was a misunderstanding of how Optional metadata behaves.


Let’s look into what actually happens, based on the reproduction:

https://github.com/nestjs/nest/blob/eab3910d0d60afe466829aa6ea89270c6682b6b0/packages/core/injector/injector.ts#L387-L399

The Injector retrieves both paramtypes and optional metadata. In this case, the constructor parameters reflect the new JwtAuthGuard, so paramtypes would contain [ [class JwtAuthGuard] ]. However, due to the @Optional decorator declared in the parent class (e.g., AuthGuard), [0] ends up in the optional metadata.

@Optional stores the index of the parameter in the metadata.

https://github.com/nestjs/nest/blob/eab3910d0d60afe466829aa6ea89270c6682b6b0/packages/core/injector/injector.ts#L325-L331

As a result, the first parameter of JwtAuthGuard is mistakenly treated as optional. This causes Nest to silently ignore the missing dependency—even though it’s not imported by the module—because it assumes the parameter is optional.

Ultimately, this does not appear to be an issue exclusive to the use of mixins.

What is the new behavior?

Changed from getMetadata to getOwnMetadata when retrieving optional parameters in a simpler way.

Now, the optional status of a parameter is determined solely by its own @Optional, without mistakenly inheriting the @Optional from its parent.

Does this PR introduce a breaking change?

  • [ ] Yes
  • [x] No

Other information

sukjuhong avatar May 14 '25 12:05 sukjuhong