graphql
graphql copied to clipboard
nestjs
Resolver metadata cache returns wrong results
Is there an existing issue for this?
- [X] 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 - [X]
@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
- [X] Windows
- [ ] Linux
Other
It comes down to this commit https://github.com/nestjs/nest/commit/035348e07d22897b83b1ff20dcc6ea24094c49c3 which fixes this https://github.com/nestjs/nest/issues/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.
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!
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
@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.
