Support request-scoped, global enhancers

[kaihaase]

I'm submitting a...

[ ] 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

The transform method of global pipes is not called.

Expected behavior

The transform method of global pipes should be called before calling a resolver method.

Minimal reproduction of the problem with instructions

core.module.ts

providers: [
      {
        provide: APP_PIPE,
        scope: Scope.REQUEST,
        useClass: CheckInputPipe,
      }
]

check-input.pipe.ts

@Injectable({ scope: Scope.REQUEST })
export class CheckInputPipe implements PipeTransform<any> {

  constructor(@Inject(CONTEXT) private readonly context) {}

  async transform(value: any, { metatype }: ArgumentMetadata) {
    console.log('Transform method is called!'); 
  }

Console log does not occur.

To test this, you can use the following repository:
https://github.com/yostools/nest-example

clone https://github.com/yostools/nest-example.git
cd nest-example
npm i
npm run test:e2e

If everything would work, the console log Current context should appear.

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

I would like to use a global pipe that compares the input with the rights of the current user and only passes on the inputs to which the user has authorization.

check-input.pipe.ts

import { ArgumentMetadata, BadRequestException, Inject, Injectable, PipeTransform, Scope } from '@nestjs/common';
import { CONTEXT } from '@nestjs/graphql';
import { plainToClass } from 'class-transformer';
import { validate, ValidationError } from 'class-validator';
import { checkRestricted } from '../decorators/restricted.decorator';
import { Context } from '../helpers/context.helper';

/**
 * The CheckInputPipe checks the permissibility of individual properties of inputs for the resolvers
 * in relation to the current user
 */
@Injectable({ scope: Scope.REQUEST })
export class CheckInputPipe implements PipeTransform<any> {

  /**
   * Constructor to inject context
   */
  constructor(@Inject(CONTEXT) private readonly context) {}

  /**
   * Check input
   */
  async transform(value: any, { metatype }: ArgumentMetadata) {

    // Return value if it is only a basic type
    if (!metatype || this.isBasicType(metatype)) {
      return value;
    }

    // Remove restricted values if roles are missing
    const { user }: any = Context.getData(this.context);
    value = checkRestricted(value, user);

    // Validate value
    const plainValue = JSON.parse(JSON.stringify(value));
    const object = plainToClass(metatype, plainValue);
    const errors: ValidationError[] = await validate(object);

    // Check errors
    if (errors.length > 0) {
      throw new BadRequestException('Validation failed');
    }

    // Everything is ok
    return value;
  }

  /**
   * Checks if it is a basic type
   */
  private isBasicType(metatype: any): boolean {
    const types = [String, Boolean, Number, Array, Object, Buffer, ArrayBuffer];
    return types.includes(metatype);
  }
}

Environment

Nest version: 6.5.2
 
For Tooling issues:
- Node version: 12.4.0
- Platform:  Mac

Others:
The error has occurred since version 6.5.0.

[lnmunhoz]

The intercept method is also not being called in case of an Interceptor. I think for the graphql package in general the request scoped providers are not being invoked.

[chrisregnier]

I'm new to nest and I've been trying to write a db transaction interceptor for my graphql mutations, but these issues with no injections are causing me problems.
Is there some sort of work around here, or another way to wrap mutations with a request scoped function that will inject services?

[kamilmysliwiec]

@lnmunhoz @chrisregnier simply use controller-scoped or method-scoped interceptors instead of global ones.

[chrisregnier]

Actually I am using method scoped interceptors already and that's what's not working for me. I haven't tried the other scopes though so I'll give those a try. Thanks.

[kamilmysliwiec]

@chrisregnier could you please create a separate issue + provide a reproduction repository?

[chrisregnier]

I've been trying all day to reproduce my problem in a small test case, but everything seems to be working. So I've clearly got unrelated problems in my own code. Thanks.

[robbyemmert]

I'm also getting the same issue as @kaihaase. Is there any timeline for fixing this issue?

[elimenko]

I'm having similar issue as @kaihaase and @lnmunhoz . I need global interceptor to do some manipulations for all incoming requests before and after response. On nestjs 6.5.0 Request scoped global interceptor works great for regular rest requests, but does not work for graphql requests

[rich-w-lee]

I just tested this w/ Nest V7 and was unable to get it to work with a request scoped Global Interceptor (via provider syntax) or w/ a request scope Global Guard. I'm able to get it to work w/ method level guards/interceptors but that's not an optimal solution. Is there any update w/ adding this feature?

[jmcdo29]

For anyone coming to this issue at the moment, request scoping an interceptor or guard doesn't make much sense because they already have access to the request object. What can be done, is inject the ModuleRef class and get the request scoped instance of the class. A sample of this would look something like

@Injectable()
export class InterceptorUsingReqScopedProvider implements NestInterceptor {
  constructor(private readonly modRef: ModuleRef) {}

  async intercept(context: ExecutionContext, next: CallHandler): Promise<Observable<unknown>> {
    const req = this.getRequest(context);
    const contextId = ContextIdFactory.getByRequest(req);
    const service = await modRef.resolve(ServiceName, contextId);
    // do something with the service
    return next.handle();
    // can also work in the `.pipe` of the observable
  }

  getRequest(context: ExecutionContext) {
    // this can have added logic to take care of REST and other contexts 
    const gql = GqlExecutionContext.create(context);
    return gql.getContext().req;
  }
}

To make sure this works, you should also make sure to set up the context properly from the GraphqlModule like so

GraphqlModule.forRoot({
  context: ({ req }) => ({ req }),
  // other options
})

[kamilmysliwiec]

Just a small note:

For anyone coming to this issue at the moment, request scoping an interceptor or guard doesn't make much sense because they already have access to the request object.

Request scoping an interceptor or guard may make sense in case they depend on request-scoped providers which factory functions perform some complex, async operations.

Using ModuleRef (as shown above) is a good workaround for now.

[jmcdo29]

Request scoping an interceptor or guard may make sense in case they depend on request-scoped providers which factory functions perform some complex, async operations.

You're right. I think I was trying to say making these enhancers explicitly request scoped isn't necessary due to being able to use the ModuleRef class, and resolving the dependency, but didn't make the point clear.

[mrnateriver]

As @jmcdo29 pointed out, it's not a big issue for interceptors or guards, because they already have access to execution context.

However, I'm also experiencing the same issue with global pipes, and they don't have that access.
It should be also noted, that not only transform method is not called, but the pipe is not constructed at all (constructor is not called).

I'm using NestJS 7.6.13. Use case for request-scoped global pipes - automatic validation of entities existence (by IDs) in databases split by tenants (SaaS).

[kaihaase]

@kamilmysliwiec The ticket has been open for a while now and we are working with the latest nestjs packages (see below), but unfortunately I still can't get the context into the pipe. No matter if I call it globally or include it in @Args. Do you have a tip for me how to make it work somehow?

Current example for CheckInputPipe:

@Mutation((returns) => User, { description: 'Update existing user' })
  async updateUser(
    @Args('input', CheckInputPipe) input: UserInput,
    @Args('id') id: string,
    @GraphQLUser() user: User
  ): Promise<User> {
      // User is logged
      console.log(user);
  }

Here I get the context and can read out the user:

graphql-user.decorator.ts:

import { createParamDecorator, ExecutionContext } from '@nestjs/common';
import { GqlExecutionContext } from '@nestjs/graphql';

/**
 * User decorator for GraphQL request
 */
export const GraphQLUser = createParamDecorator((data: unknown, ctx: ExecutionContext) => {
  const context = GqlExecutionContext.create(ctx)?.getContext();
  return context?.user || context?.req?.user;
});

But this is not possible via the pipe:
check-input.pipe.ts

import { ArgumentMetadata, Inject, Injectable, PipeTransform } from '@nestjs/common';
import { CONTEXT } from '@nestjs/graphql';

@Injectable()
export class CheckInputPipe implements PipeTransform {
  
  constructor(@Inject(CONTEXT) protected readonly context) {}

  async transform(value: any, metadata: ArgumentMetadata) {
    // this.context is null
    console.log(this.context)
  }
}

Package versions used:

"@nestjs/common": "8.2.6"
"@nestjs/core": "8.2.6"
"@nestjs/graphql": "9.1.2"

[kaihaase]

@kamilmysliwiec Is there any news on the issue / feature? Is something planned in this direction?

[tsugitta]

@jmcdo29
the code works, but it looks like const contextId = ContextIdFactory.getByRequest(req); always generates new ID because req[REQUEST_CONTEXT_ID] isn't set.
ref:

• https://github.com/nestjs/nest/blob/d1d1a16eb4ce2e30151fe5bdefa0a3e2c084b43e/packages/core/helpers/context-id-factory.ts#L28-L42

• graphql/packages/graphql/lib/services/resolvers-explorer.service.ts

  Line 186 in 90ae4e8

  Object.defineProperty(gqlContext, REQUEST_CONTEXT_ID, {

therefore, the request-scoped service may be created more than once.

It seems to be able to use the same instance used in request scope with:

• get contextId from not req but GraphQL context, like GqlExecutionContext.create(context).getContext()[REQUEST_CONTEXT_ID]
• use strict: false option, like this.moduleRef.resolve(SomeService, contextId, { strict: false });

[ElMatella]

I don't know if this is still relevant or especially smart but here is what I did:

1. I created an Apollo Plugin where I assign a context id to the request as soon as the request gets into the application.:

import {ContextIdFactory, ModuleRef} from '@nestjs/core';
import { Plugin } from '@nestjs/apollo';
import {REQUEST_CONTEXT_ID} from '@nestjs/core/router/request/request-constants';

@Plugin()
export class LoggerPlugin {
  constructor(
    private readonly moduleRef: ModuleRef,
  ) {}

  async requestDidStart(requestContext) {
    if (!requestContext.context.req[REQUEST_CONTEXT_ID]) {
      const newContextId = ContextIdFactory.create();
      Object.defineProperty(requestContext.context.req, REQUEST_CONTEXT_ID, {
        value: newContextId,
        enumerable: false,
        configurable: false,
        writable: false,
      });
      // Here, I bind the request object so it can be accessed in services using @Inject(REQUEST)
      this.moduleRef.registerRequestByContextId(requestContext.context.req, newContextId);
    }

    return {};
  }
}

2. In my interceptor, I can now use:

import {
  CallHandler,
  ExecutionContext,
  Injectable,
  NestInterceptor,
} from '@nestjs/common';
import { Observable, throwError } from 'rxjs';
import { catchError } from 'rxjs/operators';
import { LOGGER } from '../constants';
import { Logger } from 'winston';
import { GqlContextType, GqlExecutionContext } from '@nestjs/graphql';
import { ContextIdFactory, ModuleRef } from '@nestjs/core';

@Injectable()
export class ErrorInterceptor implements NestInterceptor {
  constructor(
    private readonly moduleRef: ModuleRef,
  ) {}

  async intercept(context: ExecutionContext, next: CallHandler): Promise<Observable<any>> {
    // Getting the request object
    const req = this.getRequest(context);
    // The contextId should be set on the request by now
    const contextId = ContextIdFactory.getByRequest(req);
    const logger = await this.moduleRef.resolve<string, Logger>(LOGGER.PROVIDERS.REQUEST_LOGGER, contextId);
    return next.handle().pipe(
      catchError((err) => {
        logger.error(err);
        return throwError(err);
      }),
    );
  }

  getRequest(context: ExecutionContext) {
    if (context.getType<GqlContextType>() === 'graphql') {
      // do something that is only important in the context of GraphQL requests
      const gql = GqlExecutionContext.create(context);
      return gql.getContext().req;
    }

    return context.switchToHttp().getRequest();
  }
}

It seems to work for my usecase

PS: Well, it does not work very well either, I get some { req: Request } object when I inject REQUEST in a request scoped provider. What I have to do is inject both REQUEST and CONTEXT and say something like const req = context?.req || request to make it work in both HTTP and GRAPHQL mode

[kamilmysliwiec]

This will be finally fixed as part of the next release #2636