Resolver metadata cache returns wrong results

[jakubnavratil]

Is there an existing issue for this?

• I have searched the existing issues

Current behavior

When using two equally named resolvers even when they are in diffrent modules, and both have equally named methods, Args decorator doesnt propagate data into the last one.

@Resolver()
export class UserResolver {
  @Mutation(() => String, { name: 'moduleALogin' })
  login(@Args('code') code: string) {
    return code;
  }
}

@Resolver()
export class UserResolver {
  @Mutation(() => String, { name: 'moduleBLogin' })
  login(@Args('username') username: string) {
    return username; // BUG: undefined
  }
}

Minimum reproduction code

https://github.com/jakubnavratil/nestjs-resolvers-bug

Steps to reproduce

npm i
npm run start
open http://localhost:3000/graphql

run both, B fails

mutation A {
  moduleALogin(code:"code")
}
mutation B {
  moduleBLogin(username:"username")
}

Expected behavior

Works without need to make unique methods name across multiple modules.

Package

• I don't know. Or some 3rd-party package
• @nestjs/common
• @nestjs/core
• @nestjs/microservices
• @nestjs/platform-express
• @nestjs/platform-fastify
• @nestjs/platform-socket.io
• @nestjs/platform-ws
• @nestjs/testing
• @nestjs/websockets
• Other (see below)

Other package

No response

NestJS version

No response

Packages versions

"@nestjs/common": "^8.0.0",
"@nestjs/core": "^8.0.0",
"@nestjs/graphql": "^9.1.2",

Node.js version

No response

In which operating systems have you tested?

• macOS
• Windows
• Linux

Other

It comes down to this commit nestjs/nest@035348e
which fixes this nestjs/nest#3231 and introduced uniquely generated ID for controllers but doesnt do that for Resolvers (providers)

This ID is then used here https://github.com/nestjs/nest/blob/master/packages/core/helpers/handler-metadata-storage.ts#L62
but if it is missing, it uses class name to generate keys.
Hence both login methods in my example has same keys.

I believe this should be fixed by adding this line

this.assignControllerUniqueId(controller);

before this line https://github.com/nestjs/nest/blob/master/packages/core/injector/module.ts#L240

At least, this works for me in fairly large app.

[kamilmysliwiec]

Would you like to create a PR to address this issue?

[jakubnavratil]

Yeah I could give it a try

[kamilmysliwiec]

🏓 🙌

[jasonwilson]

Was this issue ever resolved? We're observing similar behavior where resolvers overlap in a nestjs app with multiple graphql endpoints. Renaming the offending resolvers fixed the issue.

Let us know if a fix is planned otherwise we'd be willing to open a PR!

[jakubnavratil]

I'm sorry I can't fix this now. It was side project and I currently don't have time for it. If you can open a PR, please do. Thanks

[koyedele]

@kamilmysliwiec, I started looking into this issue and the fix proposed is too broad, IMHO. What it means is that every single provider will get a CONTROLLER_ID_KEY property written to them. But we only want resolvers to have this since they are the ones that are technically "controllers". If all providers suddenly have the CONTROLLER_ID_KEY property, I worry that it may be confusing for others who are debugging unrelated issues. I'll note that this solution is easier to implement as it only involves a change to NestJS core.

Another way we could approach this is to add a method called, say, assignHandlerUniqueId method to the ExternalContextCreator. The method would accept a handler and would do the work of creating the CONTROLLER_ID_KEY property on the handler. All resolvers have a context created for them by the ExternalContextCreator (here for request-scoped resolvers, and here for others) so we can just call assignHandlerUniqueId on each resolver before we create the external context.

A slightly different take might be to pass an extra argument (say, makeUnique) to the ExternalContextCreator's create method which would tell it to create a unique id for the handler it is being passed. The default value of this extra argument would be false so as to be backward-compatible. Then in the ResolversExplorerService in nestjs/graphql where we create the context for the resolvers, we pass true for makeUnique to the ExternalContextCreator.

These, IMHO, would be cleaner ways of proceeding but would require two sets of changes — one to NestJS core, and the other to the nestjs/graphql library to have the correct interaction with ExternalContextCreator. This would also pave the way for other types of non-Controller handlers to not face the same issue.

I'm happy to make PRs for these but want to check with you if this is a good approach or there's something else I'm missing.

[kamilmysliwiec]

Let's track this here #1907