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

E2E tests fail when using exception filter extended from BaseExceptionFilter #1160

Closed
mpontus opened this issue Oct 3, 2018 · 9 comments

Comments

@mpontus
Copy link

commented Oct 3, 2018

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

Setting up Exception Filter by extending from BaseExceptionFilter will fail E2E tests setup using a testing module.

I've been trying to set up an exception filter on the controller to map domain-level errors to their HttpException counterparts.

I set up the exception filter as follows:

@Catch()
export class HttpExceptionFilter extends BaseExceptionFilter {
  constructor(@Inject(HTTP_SERVER_REF) applicationRef: HttpServer) {
    super(applicationRef);
  }

  catch(error: Error, host: ArgumentsHost) {
    switch (error.name) {
      case 'UserNotFoundError':
        return super.catch(new BadRequestException('User not found'), host);

      default:
        return super.catch(error, host);
    }
  }
}

and introduced it into the controller:

  @Get()
  @UseFilters(HttpExceptionFilter)
  root(): string {
    throw new UserNotFoundError();
  }

This solution works on the actual server but fails during E2E tests with the follwing error:

    TypeError: this.applicationRef.reply is not a function

      17 |     switch (error.name) {
      18 |       case 'UserNotFoundError':
    > 19 |         return super.catch(new BadRequestException('User not found'), host);
         |                           ^
      20 | 
      21 |       default:
      22 |         return super.catch(error, host);

Expected behavior

It should be possible to extend BaseExceptionFilter and test its functionality.

Minimal reproduction of the problem with instructions

https://github.com/mpontus/nest-exception-filter-example

  1. Clone the repo
  2. Run yarn install
  3. Run yarn test:e2e to observe the error
  4. (optional) Run yarn start and visit http://localhost:3000 to observe correct behavior

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

My initial thought when coming up with this solution, was to rethrow errors from the custom exception filter. Similar approach was tested by @quezak in this issue, but the error shown there still persists.

My understandign from the linked PR is that subclassing BaseExceptionFilter is the recommended solution for this problem. If I made a mistake somewhere in my implementation, I would appreciate the correction.

Otherwise, I think it would be important to ensure that error handling, achieved through the use of exception filter, can be expressed through the context of E2E tests.

Environment


Nest version: 5.3.10

 
For Tooling issues:
- Node version: 8.11.3  
- Platform: Linux 


Others:

@MIrinkov

This comment has been minimized.

Copy link

commented Oct 5, 2018

I have just encountered this issue. I believe it may have something to do with how superagent sets up the instance, when you do request(app).get('/')....
If you extend BaseExceptionFilter, you need to inject HTTP_SERVER_REF, but it's an empty object (checked by putting breakpoints in). Hence the error.

constructor(@Inject(HTTP_SERVER_REF) appRef: HttpServer) {
    super(appRef); // appRef is {}
}

catch(exception: any, host: ArgumentsHost) {
    if (exception instanceof DomainError) {
        exception = new HttpException(exception.message, 400);
    }
    super.catch(exception, host); // this.appRef is {}
}

I don't know what to make of it, but posting just in case it helps anyone.

@mpontus

This comment has been minimized.

Copy link
Author

commented Oct 5, 2018

I worked around the problem by using NestFactory instead of createTestingModule:

import { INestApplication } from '@nestjs/common';
import { NestFactory } from '@nestjs/core';
import * as request from 'supertest';
import { AppModule } from './../src/app.module';

describe('AppController (e2e)', () => {
  let app: INestApplication;

  beforeAll(async () => {
    app = await NestFactory.create(AppModule, {
      logger: false,
    });

    await app.init();
  });

  it('/ (GET)', () => {
    return request(app.getHttpServer())
      .get('/')
      .expect(400)
      .expect(/User not found/);
  });
});

Although, I think you lose the benefit of being able to substitute modules.

@kamilmysliwiec

This comment has been minimized.

Copy link
Member

commented Nov 30, 2018

Should be fixed in 5.4.1, just ensure to entirely remove this:

constructor(@Inject(HTTP_SERVER_REF) appRef: HttpServer) {
    super(appRef); // appRef is {}
}
@ricardovf

This comment has been minimized.

Copy link

commented Feb 13, 2019

@kamilmysliwiec Confirmed, removing the constructor fixed the problem.

@nelsonec87

This comment has been minimized.

Copy link

commented Feb 28, 2019

I'm not sure why, but in v5.7.3 I'm trying:

`

\@Catch(NotFoundException)
export class NotFoundExceptionFilter extends BaseExceptionFilter {
    catch(exception: HttpException, host: ArgumentsHost) {
        const ctx = host.switchToHttp();
        const res: Response = ctx.getResponse();
        const req: Request = ctx.getRequest();

        if (req.url.indexOf('/api') == 0)
            super.catch(exception, host);
        else
            res.sendFile(join(__dirname, '../../public/index.html'));
    }
}

and applicationRef still undefined in the BaseExceptionFilter, causing UnhandledPromiseRejectionWarning: TypeError: Cannot read property 'reply' of undefined

What am I missing?

@kamilmysliwiec

This comment has been minimized.

Copy link
Member

commented Mar 1, 2019

@nelsonec87 the exception filter has access to the applicationRef only if the class instance has been instantiated by the framework (new NotFoundExceptionFilter() will not work).

@nelsonec87

This comment has been minimized.

Copy link

commented Mar 1, 2019

That was it.
I removed the .useGlobalFilters and added the Filter to the main module's providers with APP_FILTER. It's working, thanks!

@murbanowicz

This comment has been minimized.

Copy link

commented Apr 29, 2019

Hi! I've started getting this error for some reason on all my E2E tests with Nest 6. It used to work until 2-3 weeks ago. I did not change anything...

This is how my code looks like.

describe('E2E - authorization flows', () => {
  let app: INestApplication;
  beforeAll(async () => {
    const moduleFixture = await Test.createTestingModule({
      imports: [AppModule],
    }).compile();
    app = moduleFixture.createNestApplication();
    await app.init();
  }, 15000);

  afterAll(async () => {
    await app.close();
  });

  it('/ (GET)', () => {
    return request(app.getHttpServer())
      .get('/')
      .expect(200)
      .expect('Hello World!');
  });
@boenrobot

This comment has been minimized.

Copy link
Contributor

commented Jul 24, 2019

the exception filter has access to the applicationRef only if the class instance has been instantiated by the framework (new NotFoundExceptionFilter() will not work).

But how should one proceed if they want to fallback to the base exception filter inside a global application filter on certain conditions?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.