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

Only the last one of APP_ providers get registered #812

Closed
autaut03 opened this issue Jun 25, 2018 · 4 comments
Closed

Only the last one of APP_ providers get registered #812

autaut03 opened this issue Jun 25, 2018 · 4 comments

Comments

@autaut03
Copy link
Contributor

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

If more than two APP_ providers are provided within one module, only the last of them actually gets registered.

Expected behavior

Both APP_FILTER providers should get registered. Same goes for 3+ and other types of APP providers.

Minimal reproduction of the problem with instructions

https://gist.github.com/autaut03/15af00e63cb41cad77e35c7cab1d1330
If any exception besides ForbiddenException is thrown anywhere in the app, it won't be catched by InternalServerErrorFilter. The same behaviour will persist even when more filters added, even for specific exceptions (BadRequestFilter for BadRequestException, for example).

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

This is a core functionality. In my case I want to register many exception handlers (filters) globally and still use dependency injection in them. As a workaround currently I use this:

filters.forEach(FilterType => {
  const instance = app.select(AppModule).get(FilterType as any);

  app.useGlobalFilters(instance as ExceptionFilter);
});

which isn't exactly the best code and I still have to provide them within my AppModule.

Environment


Nest version: 5.0.0
The code is same on the newer version, so the issue remains.
@autaut03
Copy link
Contributor Author

autaut03 commented Jun 25, 2018

After little investigation I found the problem. NestJS treats provide: APP_FILTER as a provider of APP_FILTER and of name APP_FILTER (it's one variable). I think it's better to put function's name as a value for providerToken key and providerToken variable as a value for providerType key.

For this it would be { moduleToken: '6ac152f5e1bfac81fdc71d049372f7654a4c6837', providerType: 'APP_FILTER', providerToken: 'InternalServerErrorFilter' }
so we are not losing the uniqueness of each APP_FILTER. However components in a module are also parsed incorrectly and in our example would use 'APP_FILTER' as a key, not 'InternalServerErrorFilter'. To fix this, this line would have to also check if .useClass is a function, and if it is - use it as a name for the component.

I can make a PR if needed. Thanks regardless.

@twittwer
Copy link

twittwer commented Jul 2, 2018

We tried to register multiple Exception Filters via the provider token APP_FILTER and get the same problem. Only the last Exception filter catches any error, the other exceptions went to the client.
As a workaround, we were able to register them in different modules, but it would nice to have one module for the global exception filters.

@kamilmysliwiec
Copy link
Member

Fixed in 5.1.0

@lock
Copy link

lock bot commented Sep 24, 2019

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 lock bot locked as resolved and limited conversation to collaborators Sep 24, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants