Skip to content
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

@nestjs/graphql 7.9.8 breaks retrieving the request from the context #1364

Closed
paulhill opened this issue Feb 6, 2021 · 6 comments
Closed

Comments

@paulhill
Copy link

paulhill commented Feb 6, 2021

I'm submitting a...


[x] Regression 
[x] Bug report
[ ] Feature request
[ ] Documentation issue or request
[ ] Support request => Please do not submit support request here, instead post your question on Stack Overflow.

Current behavior

I have a few Guards and Interceptors that access the Express request.
I added it to the GraphQL context like so:

GraphQLModule.forRootAsync({
      imports: [ConfigModule],
      useFactory: (configService: ConfigService) => {
        return {
          context: ({ req, res }) => ({ req, res }),
          ...

based on a doc I read which for the life of me I can no longer find.
Anyways, I've isolated an issue down to one specific version upgrade from @nestjs/graphql 7.9.7 to @nestjs/graphql 7.9.8.
When I make that upgrade the request is no longer available in the context and my guards break.

export const getRequestFromExecutionContext = (
  context: ExecutionContext,
): Request => {
  if (context.getType<GqlContextType>() === 'graphql') {
    const ctx = GqlExecutionContext.create(context);
    const req = ctx.getContext().req;
    if (isEmpty(req)) {
      console.error(`req is empty`);
    }
  console.error
    req is empty

      22 |     const req = ctx.getContext().req;
      23 |     if (isEmpty(req)) {
    > 24 |       console.error(`req is empty`);
         |               ^
      25 |     }
      26 |     return req;
      27 |   } else if (context.getType<ContextType>() === 'http') {

      at Object.getRequestFromExecutionContext (../../src/modules/utils/request.utils.ts:24:15)
      at IsAuthenticatedGuard.canActivate (../../src/modules/auth/guards/is-authenticated.guard.ts:35:21)
      at GuardsConsumer.tryActivate (../../node_modules/@nestjs/core/guards/guards-consumer.js:15:34)
      at canActivateFn (../../node_modules/@nestjs/core/helpers/external-context-creator.js:162:59)
      at target (../../node_modules/@nestjs/core/helpers/external-context-creator.js:75:37)
      at ../../node_modules/@nestjs/core/helpers/external-proxy.js:9:30
      at completeAbstractValue (../../node_modules/graphql/execution/execute.js:673:21)

Expected behavior

The req was available in the context in @nestjs/graphql 7.9.7.
I've looked through a diff of that version change and can't see anything that could break this.
I'm stumped.

Minimal reproduction of the problem with instructions

Add req to GraphQL context.

GraphQLModule.forRootAsync({
      imports: [ConfigModule],
      useFactory: (configService: ConfigService) => {
        return {
          context: ({ req, res }) => ({ req, res }),
          ...

Try to retrieve it with @nestjs/graphql 7.9.8

createParamDecorator(
  (data: unknown, context: ExecutionContext): Request => {
  if (context.getType<GqlContextType>() === 'graphql') {
    const ctx = GqlExecutionContext.create(context);
    const req = ctx.getContext().req;
    if (isEmpty(req)) {
      console.error(`req is empty`);
    }

What is the motivation / use case for changing the behavior?

Guards that need access to the request context

Environment

Nest version: 7.6.x
  "dependencies": {
    "@nestjs/common": "^7.6.11",
    "@nestjs/config": "^0.6.3",
    "@nestjs/core": "^7.6.11",
    "@nestjs/graphql": "^7.9.8",
    "@nestjs/jwt": "^7.2.0",
    "@nestjs/passport": "^7.1.5",
    "@nestjs/platform-express": "^7.6.11",

For Tooling issues:

  • Node version: v15.6.0
  • Platform: OSX

Others:
NestJS + GraphQL + Express

@paulhill
Copy link
Author

paulhill commented Feb 6, 2021

Seems similar to this issue: #48

@kamilmysliwiec
Copy link
Member

Please provide a minimum reproduction repository.

@paulhill
Copy link
Author

paulhill commented Feb 10, 2021

I'm trying pretty hard to replicate the issue using the schema-first sample repo.
I just can't seem to find the combination of things that triggers it.
I have a hunch it's something to do with the fact that I use interceptors on my field resolvers.
I don't fully understand the changes in that version but they seem to be doing something like adding middleware to the non-root resolvers and that seems to be triggering guards but without the request in the context.
This is really deep in the weeds and I'm struggling to track it down.

This is how it presents in my e2e tests

[Nest] 80539   - 2021-02-10, 2:07:17 p.m.   [ExceptionHandler] Cannot read property 'isAuthenticated' of undefined
TypeError: Cannot read property 'isAuthenticated' of undefined
    at IsAuthenticatedGuard.canActivate (/Users/paul/source/okhelp/api-server/src/modules/auth/guards/is-authenticated.guard.ts:36:37)
    at GuardsConsumer.tryActivate (/Users/paul/source/okhelp/api-server/node_modules/@nestjs/core/guards/guards-consumer.js:15:34)
    at canActivateFn (/Users/paul/source/okhelp/api-server/node_modules/@nestjs/core/helpers/external-context-creator.js:162:59)
    at target (/Users/paul/source/okhelp/api-server/node_modules/@nestjs/core/helpers/external-context-creator.js:75:37)
    at /Users/paul/source/okhelp/api-server/node_modules/@nestjs/core/helpers/external-proxy.js:9:30
    at completeAbstractValue (/Users/paul/source/okhelp/api-server/node_modules/graphql/execution/execute.js:673:21)
    at completeValue (/Users/paul/source/okhelp/api-server/node_modules/graphql/execution/execute.js:586:12)
    at /Users/paul/source/okhelp/api-server/node_modules/graphql/execution/execute.js:626:25
    at from (<anonymous>)
    at completeListValue (/Users/paul/source/okhelp/api-server/node_modules/graphql/execution/execute.js:613:49)

at this point the req is in the context

image

at this point it's gone from the context

image

@paulhill
Copy link
Author

paulhill commented Mar 3, 2021

I've been trying to build a minimum repro but it's taking a lot of time.
@kamilmysliwiec could you give me some clues how it's supposed to work now?
The Passport integration broke when the enhancers for resolve type were disabled in this change.
Is there some way to actually use field middleware that's not yet documented?
I have a global guard

  providers: [
    ...
    {
      provide: APP_GUARD,
      useClass: IsAuthenticatedGuard,
    },
  ],
export class IsAuthenticatedGuard
  extends AuthGuard('local')
  implements CanActivate {
  private readonly logger = new Logger('IsAuthenticatedGuard');

  constructor(private readonly reflector: Reflector) {
    super();
  }

  public canActivate(context: ExecutionContext): boolean {
    const isPublic = this.reflector.get<boolean>(
      'isPublic',
      context.getHandler(),
    );
    if (isPublic) {
      return true;
    }
    const request = getRequestFromExecutionContext(context);
    const isAuthenticated = request.isAuthenticated();
    if (!isAuthenticated) {
      throw new UnauthorizedException();
    }
    return isAuthenticated;
  }
}

It's clear to me now that the error where the request is missing from the context is only happening on field resolvers.

@paulhill
Copy link
Author

paulhill commented Mar 3, 2021

Ok so I worked out roughly what is going on.
This change means the request is not passed down into the context.
I had a global guard that was being executed on every field resolution that was looking for the request so it was failing.
I fixed it by checking in that adding a check for isResolvingGraphQLField(context)
This took me a while to figure out because I have:

fieldResolverEnhancers: ['interceptors'],

So I'm still not sure why my guard is being executed on field resolvers.

@michaelbromley
Copy link

michaelbromley commented Mar 8, 2021

I also ran into what seems to be the same issue after upgrading from v7.6.0 to v7.9.11. Further testing showed that the regression is not present in v7.9.7.

In my case, I have a global guard defined thus:

providers: [
  {
    provide: APP_GUARD,
    useClass: AuthGuard,
  },
]

Then I have a query like this:

union CustomMappings = CustomProductMappings | CustomProductVariantMappings

type SearchResult {
  # a bunch of simple fields omitted

  customMappings: CustomMappings!
}

type Query {
  search: [SearchResult!]!
}

What I observe is that the AuthGuard.canActivate() method is called twice, once for the the search path, and then again for the customMappings field. Prior to v7.9.8, it would only get called once, for the search path.

Furthermore, when it is called the 2nd time for the customMappings field, the ExecutionContext passed to canActivate() has a different order of arguments when I inspect context.args:

for the top-level (working) search field it is:

  1. parent (undefined)
  2. gql query args
  3. express context (req, res)
  4. gql "info" object

then for the second (erroring) call it is

  1. parent (customMappings object)
  2. express context (req, res)
  3. gql "info" object
  4. gql return type (GraphQLUnionType)

I suspect the cause may be related to the fact that I have the following FieldResolver defined:

@Resolver('CustomMappings')
export class CustomMappingsResolver {
    constructor(@Inject(ELASTIC_SEARCH_OPTIONS) private options: DeepRequired<ElasticsearchOptions>) {}

    @ResolveField()
    __resolveType(value: any): string {
        const productPropertyNames = Object.keys(this.options.customProductMappings);
        return Object.keys(value).every(k => productPropertyNames.includes(k))
            ? 'CustomProductMappings'
            : 'CustomProductVariantMappings';
    }
}

and is possibly related to #1340.

Note I am not using fieldResolverEnhancers in this project.

michaelbromley added a commit to vendure-ecommerce/vendure that referenced this issue Mar 8, 2021
The latest version has a regression which affects the Elasticsearch plugin's customMappings field
resolver. See nestjs/graphql#1364
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants