Nest: @Inject(REQUEST) is null for mixin pipes used in GraphQL @Args()

[gempain]

Bug Report

Current behavior

When I create a mixin pipe which uses @Inject(REQUEST), the request is null when the pipe is used in GraphQL query method parameter annotated @Args() but is not when the pipe is used in @Param() of a controller parameter method. This happens at runtime and I can reproduce when testing.

Input Code

The test below breaks:

let val: any;

function TestPipe() {
  class TestClass implements PipeTransform {
    constructor(@Inject(REQUEST) private readonly request: IncomingMessage,) {
    }

    transform(value: any, metadata: ArgumentMetadata): any {
      val = this.request;
      return undefined;
    }
  }

  return mixin(TestClass);
}

@Resolver('Test')
class MyResolver {
  @Query(() => String)
  get(@Args('id', TestPipe()) id: string) {
    return '';
  }
}

describe('MyResolver', () => {

  let app: INestApplication;
  let server: any;

  beforeEach(async () => {
    const module: TestingModule = await Test.createTestingModule({
      imports: [GraphQLModule.forRoot({autoSchemaFile: true})],
      providers: [MyResolver]
    }).compile();

    app = module.createNestApplication();
    server = app.getHttpServer();

    await app.init();
  });

  afterEach(() => jest.restoreAllMocks());
  afterAll(async () => await app.close());

  it('should get', async () => {
    await supertest(server)
      .post('/graphql')
      .send({query: 'query { get(id: "1") }'})
      .expect(200)
      .expect({data: {get: ''}});
    expect(val).not.toBeNull();
  });

});

but this one doesn't (same TestPipe as above, just didn't copy/paste to save space):

@Controller()
class MyController {
  @Get('/:id')
  get(@Param('id', TestPipe()) id: string) {
  }
}

describe('MyController', () => {

  let app: INestApplication;
  let server: any;

  beforeEach(async () => {
    const module: TestingModule = await Test.createTestingModule({
      controllers: [MyController]
    }).compile();

    app = module.createNestApplication();
    server = app.getHttpServer();

    await app.init();
  });

  afterEach(() => jest.restoreAllMocks());
  afterAll(async () => await app.close());

  it('should get', async () => {
    await supertest(server)
      .get('/1')
      .expect(200);
    expect(val).not.toBeNull();
  });

});

Expected behavior

I expect this.request to be defined in both tests.

Environment

Nest version:
"@nestjs/common": "^7.0.3",
"@nestjs/config": "^0.4.0",
"@nestjs/core": "^7.0.3",
"@nestjs/graphql": "^7.0.10",
"@nestjs/passport": "^7.0.0",
"@nestjs/platform-express": "^7.0.3",
"@nestjs/schedule": "^0.3.0",
"@nestjs/typeorm": "^7.0.0",
"apollo-server-express": "^2.11.0",
"@nestjs/testing": "^7.0.3",

For Tooling issues:
- Node version: 12.13.1
- Platform:  Mac

[kamilmysliwiec]

Your pipe is not request-scoped. What mixin does underneath, is simply generating a unique class identifier + applies @Injectable() decorator (with the default options = Scope.DEFAULT). Instead of using mixin(), you can annotate your class with @Injectable({ scope: Scope.REQUEST }).

[gempain]

@kamilmysliwiec I was aware of the scope annotation, but I've noticed that when using mixin, it usually works even without it (e.g. the test with the @Controller which passes fine). However, I tried your suggestion, but still the request is null. Here's the updated test:

import {ArgumentMetadata, INestApplication, Inject, Injectable, mixin, PipeTransform, Scope} from '@nestjs/common';
import {REQUEST} from '@nestjs/core';
import {IncomingMessage} from 'http';
import {Test, TestingModule} from '@nestjs/testing';
import supertest from 'supertest';
import {Args, GraphQLModule, Query, Resolver} from '@nestjs/graphql';

let val: any;

function TestPipe() {
  @Injectable({scope: Scope.REQUEST})
  class TestClass implements PipeTransform {
    constructor(@Inject(REQUEST) private readonly request: IncomingMessage,) {
    }

    transform(value: any, metadata: ArgumentMetadata): any {
      val = this.request;
      return undefined;
    }
  }

  return mixin(TestClass);
}

@Resolver('Test')
class MyResolver {
  @Query(() => String)
  get(@Args('id', TestPipe()) id: string) {
    return '';
  }
}

describe('Controller', () => {

  let app: INestApplication;
  let server: any;

  beforeEach(async () => {
    const module: TestingModule = await Test.createTestingModule({
      imports: [GraphQLModule.forRoot({autoSchemaFile: true})],
      providers: [MyResolver]
    }).compile();

    app = module.createNestApplication();
    server = app.getHttpServer();

    await app.init();
  });

  afterEach(() => jest.restoreAllMocks());
  afterAll(async () => await app.close());

  it('should get', async () => {
    await supertest(server)
      .post('/graphql')
      .send({query: 'query { get(id: "1") }'})
      .expect(200)
      .expect({data: {get: ''}});
    expect(val).not.toBeNull();
  });

});

[kamilmysliwiec]

@gempain

@kamilmysliwiec I was aware of the scope annotation, but I've noticed that when using mixin, it usually works even without it (e.g. the test with the @Controller which passes fine). However, I tried your suggestion, but still the request is null. Here's the updated test:

mixin() calls Injectable() under the hood without passing any arguments. Simply return a class instead:

return TestClass;

[gempain]

@kamilmysliwiec it works, you're the man ! I had not understood you meant we could just return the class, I though mixin took care of the injection, my mistake. Would it be possible to have this documented somewhere, I think other users would benefit from it.

BTW, thanks for the amazing framework and all the hard work you put into it. That's why I take some of my time to come here and report things I find misleading or not working, hoping to improve this great tool. TBH, we threw away Spring Boot and Java for your framework ;)

[kamilmysliwiec]

@kamilmysliwiec it works, you're the man ! I had not understood you meant we could just return the class, I though mixin took care of the injection, my mistake. Would it be possible to have this documented somewhere, I think other users would benefit from it.

We're actively working on a dedicated chapter for mixins :) Hopefully, we'll ship it soon.

BTW, thanks for the amazing framework and all the hard work you put into it. That's why I take some of my time to come here and report things I find misleading or not working, hoping to improve this great tool. TBH, we threw away Spring Boot and Java for your framework ;)

Thanks for your feedback! And enjoy using Nest! Let me know if you face any further issues

[gempain]

@kamilmysliwiec interesting, when I console.log(this.request) at runtime, when the pipe is used in an @Param(), it prints:

IncomingMessage {
  _readableState: ReadableState {
    objectMode: false,
...

but when used within the GraphQL resolver, I get:

{
  req: IncomingMessage {
    _readableState: ReadableState {
      objectMode: false,
...

so the request seems to be injected in a {req: ...} property. Could this be related to the fact that I configured my GraphQL module with context: ({req, res}) => ({req, res}), ?

[kamilmysliwiec]

Ah, one thing I forgot to mention:

And here: https://docs.nestjs.com/techniques/authentication#graphql is an example of how to configure context.

[gempain]

@kamilmysliwiec actually using @Inject(CONTEXT) behaves exactly the same as using @Inject(REQUEST).

But doing this works fine for my use case:

const user: User = (this.context as any)?.user || (this.context as any)?.req?.user;

[kamilmysliwiec]

CONTEXT is an alias for REQUEST. See the screenshot above ^ ("you access the inbound request slightly differently [...] GraphQL application). Also, check this section https://docs.nestjs.com/fundamentals/execution-context#current-application-context

[gempain]

@kamilmysliwiec I think now I remember why I used the mixin(), because when I use several time TestPipe(), Nest only uses one instance for all endpoints. See the following test which always prints pipe1, where it should print pipe1 and pipe2:

import {ArgumentMetadata, INestApplication, Inject, Injectable, PipeTransform, Scope} from '@nestjs/common';
import {REQUEST} from '@nestjs/core';
import {Test, TestingModule} from '@nestjs/testing';
import supertest from 'supertest';
import {Args, GraphQLModule, Query, Resolver} from '@nestjs/graphql';

let val: any;
let pipeParam: any;

function TestPipe(param: string) {
  @Injectable({scope: Scope.REQUEST})
  class TestClass implements PipeTransform {
    constructor(@Inject(REQUEST) private readonly request: any,) {
    }

    transform(value: any, metadata: ArgumentMetadata): any {
      val = this.request;
      pipeParam = param;
      console.log(param);
      return undefined;
    }
  }

  return TestClass;
}

@Resolver('Test')
class MyResolver {
  @Query(() => String)
  get(@Args('id', TestPipe('pipe1')) id: string) {
    return '';
  }

  @Query(() => String)
  get2(@Args('id', TestPipe('pipe2')) id: string) {
    return '';
  }
}

describe('Resolver', () => {

  let app: INestApplication;
  let server: any;

  beforeEach(async () => {
    const module: TestingModule = await Test.createTestingModule({
      imports: [GraphQLModule.forRoot({autoSchemaFile: true})],
      providers: [MyResolver]
    }).compile();

    app = module.createNestApplication();
    server = app.getHttpServer();

    await app.init();
  });

  afterEach(() => jest.restoreAllMocks());
  afterAll(async () => await app.close());

  it('should get', async () => {
    await supertest(server)
      .post('/graphql')
      .send({query: 'query { get(id: "1") }'})
      .expect(200)
      .expect({data: {get: ''}});
    expect(pipeParam).toEqual('pipe1');
  });

  it('should get2', async () => {
    await supertest(server)
      .post('/graphql')
      .send({query: 'query { get2(id: "1") }'})
      .expect(200)
      .expect({data: {get2: ''}});
    expect(pipeParam).toEqual('pipe2');
  });

});

[kamilmysliwiec]

As a workaround, generate random class name:

Object.defineProperty(TestPipe, 'name', {
    value: RANDOM_VALUE_HERE,
});
return TestPipe;

In the future, mixin() will take another argument which will be passed down to the @Injectable() decorator

[gempain]

Yes that works ! You're the man, thanks for helping out on Saturday 😄

[lock[bot]]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

[lock[bot]]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.