Skip to content

fix(core): correct misunderstanding of optional metadata #15137

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

sukjuhong
Copy link

@sukjuhong sukjuhong commented May 14, 2025

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • 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:

public reflectConstructorParams<T>(type: Type<T>): any[] {
const paramtypes = [
...(Reflect.getMetadata(PARAMTYPES_METADATA, type) || []),
];
const selfParams = this.reflectSelfParams<T>(type);
selfParams.forEach(({ index, param }) => (paramtypes[index] = param));
return paramtypes;
}
public reflectOptionalParams<T>(type: Type<T>): any[] {
return Reflect.getMetadata(OPTIONAL_DEPS_METADATA, type) || [];
}

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.

} catch (err) {
const isOptional = optionalDependenciesIds.includes(index);
if (!isOptional) {
throw err;
}
return undefined;
}

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
  • No

Other information

@sukjuhong sukjuhong changed the title feat(core): Get optional parameter metadata with getOwnMetadata fix(core): Corrected misunderstanding of optional metadata May 14, 2025
@coveralls
Copy link

coveralls commented May 14, 2025

Pull Request Test Coverage Report for Build 9fc99eb4-8b02-4641-9ecf-f82ba207d960

Details

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 88.94%

Totals Coverage Status
Change from base Build 5ca63861-46c2-436b-ac30-fe4e26f81547: 0.0%
Covered Lines: 7197
Relevant Lines: 8092

💛 - Coveralls

@sukjuhong sukjuhong changed the title fix(core): Corrected misunderstanding of optional metadata fix(core): correct misunderstanding of optional metadata May 18, 2025
@sukjuhong
Copy link
Author

To help illustrate the issue more clearly, I’ve uploaded debugging screenshots from the process of running the new test I added.

image

Here we can see that NeededService is correctly included in the paramtypes.

image

And here we see that index 0 appears in the optional parameters metadata.

Even though @Optional() is defined in the parent class, the index 0 is inherited, which causes it to be misinterpreted in the child class.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove optional dependency metadata inherited from base class
3 participants