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

Global interceptors for ResolveField #760

Closed
gempain opened this issue Apr 7, 2020 · 9 comments
Closed

Global interceptors for ResolveField #760

gempain opened this issue Apr 7, 2020 · 9 comments

Comments

@gempain
Copy link

gempain commented Apr 7, 2020

I'm submitting a...

[x] Feature request
[x] Documentation issue or request

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

This is an in-depth analysis of #320 and how we could find a way to make serialization more handy for GraphQL requests.

Introduction

Say I have this class:

export class MyClass {
  @Expose({ groups: [...] })
  prop: string;
}

In a perfect world, all I'd have to do is annotate the property with @Expose({...}) and ensure I'm returning a class in controllers / resolvers, and that's it. Behind the scenes, I could register a custom interceptor that does plainToClass(response, {groups: user.permissions}). But the world isn't perfect.

Analysis

For controller methods, this can be achieved easily using an interceptor. But for GraphQL resolvers, that another matter.

i've explored a few ways to do this, but so far, nothing handy.

Custom class serializer interceptor

In practice, interceptors do not access the full GraphQL response. They only get the response of the @Query()/@Mutation() method, but not the complete response with the resolved fields, so that's a dead end.

Custom GraphQL plugin

Apollo Server provides a way to write plugins, and it can be easily implemented in NestJS.

Unfortunately, responseForOperation only allows you to replace the response, and willSendResponse gives you access to the response via requestContext.response.data but it's a plain object, not a class, so that's a dead end as well.

Manually serialize at the resolver level

So far, the only thing we can do is to manually call plainToClass at the @Query() / @Mutation() / @ResolveField() method level, but that's really annoying.

Binding resolvers at the query/mutation/field-resolver level

Interceptors already work with @Query() and @Mutation(), so I'm wondering whether it would be possible to register a global interceptor that wraps values returned by @ResolveField(). I haven't looked at the code yet, but I'm guessing that you're passing the handler to Apollo in some way, so you could wrap that handler with a custom interceptor.

I see several ways to implement this, but I think it should be an opt-in:

  • add an interceptor property to ResolveFieldOptions
  • provide a way to tell app.useGlobalInterceptors(..., { bindToGraphQLResolveFields })

What do you think @kamilmysliwiec ?

@kamilmysliwiec
Copy link
Member

Interceptors already work with @query() and @mutation(), so I'm wondering whether it would be possible to register a global interceptor that wraps values returned by @ResolveField(). I haven't looked at the code yet, but I'm guessing that you're passing the handler to Apollo in some way, so you could wrap that handler with a custom interceptor.

This is already possible, but not yet documented nestjs/docs.nestjs.com#514

You can find an example here #295

@gempain
Copy link
Author

gempain commented Apr 8, 2020

In #295 you advise against using it for @ResolveProperty(), which I'm guessing has been renamed @ResolveField() by now, but I'm wondering if there is a performance difference between calling classToPlain once for the entire response or once for every field. In terms of complexity, I think both cases are in O(n). But in practice, I'm assuming that ALL interceptors are run for the @ResolveProperty() when you set fieldResolverEnhancers: ['interceptors'], which is halfway the battle I'd say. What would be really nice is to be able to specify the interceptors that are run.

@kamilmysliwiec
Copy link
Member

In #295 you advise against using it for @ResolveProperty(), which I'm guessing has been renamed @ResolveField() by now, but I'm wondering if there is a performance difference between calling classToPlain once for the entire response or once for every field.

There's a decent difference when processing large data (e.g, arrays with +1000 items in which for each item we must evaluate the resolve field function). Other than that, you should be fine :)

What would be really nice is to be able to specify the interceptors that are run.

What you can do is to decide whether to run an interceptor or not based on the ExecutionContext :) For example, you can use info GraphQL argument (or Reflector). We don't plan to introduce more filtering features here :(

@gempain
Copy link
Author

gempain commented Apr 8, 2020

There's a decent difference when processing large data (e.g, arrays with +1000 items in which for each item we must evaluate the resolve field function). Other than that, you should be fine :)

If I had to return that many items, I would introduce pagination. It's nonsense to return that many records at once, because classToPlain won't be your first worry in that case, most likely the handling of several Mb of data at once in RAM will, just imagine that in production...

What you can do is to decide whether to run an interceptor or not based on the ExecutionContext :) For example, you can use info GraphQL argument (or Reflector). We don't plan to introduce more filtering features here :(

Sure, here's a working version I came up with:

export function isGraphQlContext(executionContext: ExecutionContext) {
  return executionContext.getType().toString() === 'graphql';
}

/**
 * When you enabled fieldResolverEnhancers, this can be used inside global interceptors/filters/etc to improve performance 
 * by skip execution when called for @ResolveField() methods. More info here: https://github.com/nestjs/graphql/issues/760.
 * @param context
 */
export function isResolvingGraphQLField(context: ExecutionContext): boolean {
  if (isGraphQlContext(context)) {
    const gqlContext = GqlExecutionContext.create(context);
    const info = gqlContext.getInfo<GraphQLContextInfo>();
    const parentType = info.parentType.name;
    return parentType !== 'Query' && parentType !== 'Mutation';
  }
  return false;
}

@kamilmysliwiec do you think this would make it in the docs somewhere ? I'm sure other would benefit from it.

@kamilmysliwiec
Copy link
Member

If I had to return that many items, I would introduce pagination. It's nonsense to return that many records at once, because classToPlain won't be your first worry in that case, most likely the handling of several Mb of data at once in RAM will, just imagine that in production...

I do agree! But as a framework, we have to stay enough flexible to cover some edge cases too :)

@kamilmysliwiec do you think this would make it in the docs somewhere ? I'm sure other would benefit from it.

If you would like to contribute with this + fieldResolverEnhancers, it would be amazing! PRs are more than welcome!

@gempain
Copy link
Author

gempain commented Apr 8, 2020

If you would like to contribute with this + fieldResolverEnhancers, it would be amazing! PRs are more than welcome!

Just tell me where in the docs you'd like to have this, and I'll write something.

@kamilmysliwiec
Copy link
Member

Good question. I'm guessing GraphQL -> Other features would be the right place? (where we show guards and interceptors)

@gempain
Copy link
Author

gempain commented Apr 9, 2020

@kamilmysliwiec done :) should I close this issue ?

@kamilmysliwiec
Copy link
Member

Thanks @gempain! Let's close it :)

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

No branches or pull requests

2 participants