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

add error management for failed Validation Pipes #169

Merged
merged 9 commits into from
Mar 21, 2024
Merged

Conversation

Ptroger
Copy link
Contributor

@Ptroger Ptroger commented Mar 13, 2024

No description provided.

Comment on lines 58 to 68
useValue: new ValidationPipe({
exceptionFactory: (errors) => {
const logger = new Logger('ValidationPipe')
const errorMessages = errors.map((error) => ({
property: error.property,
constraints: error.constraints
}))
logger.error(JSON.stringify(errorMessages))
return new BadRequestException('Validation failed')
}
})
Copy link
Collaborator

@wcalderipe wcalderipe Mar 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a better way to solve that problem than adding a custom handler to the validation pipe. Moreover, the validation pipe already throws a BadRequestException, by rethrowing the error you're losing information. Never catch an error and throw a new one without its origin; otherwise you lose the stack trace – that's crucial for debugging.

The NestJS architecture calls this component Exception Filter.

Please reference the application-exception.filter.ts from the Armory application. However, instead of catch ApplicationException you want to filter by Nest's HttpException.

See also Catch Everything section in the documentation.

Copy link
Collaborator

@wcalderipe wcalderipe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, port the ApplicationExceptionFilter test from the Armory to the Policy Engine project. Don't mind by the duplication. It's fine.

@@ -0,0 +1,52 @@
import { ArgumentsHost, Catch, ExceptionFilter, LogLevel, Logger } from '@nestjs/common'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, port the test file from the Armory to the Policy Engine project as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

const withGlobalFilters =
(configService: ConfigService<Config, true>) =>
(app: INestApplication): INestApplication => {
app.useGlobalFilters(new HttpExceptionFilter(configService), new ApplicationExceptionFilter(configService))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Not an action]

When setting global filters in your bootstrap, think of them as a framework version of Node's unhandledRejection. They act as a final catch before things blow up.

The side effect of including global filters in the bootstrap is that if you assemble an application using a module, it won't have the filter registered. An example of assembling the application by module is in tests where we don't need the server running. In other words, none of the E2E tests will have this filter, which can lead to confusion.

What you're essentially saying is: "I want my SERVER to have these filters," but you're not adding them to your business logic unless its public interface is a server.

That's why some modules, like the OrchestrationModule, set the same filters as a dependency.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, good lesson here ! Ty

Ptroger and others added 2 commits March 14, 2024 17:16
@Ptroger Ptroger self-assigned this Mar 20, 2024
Copy link
Collaborator

@wcalderipe wcalderipe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding the tests.

You missed the tests for the HttpExceptionFilter, but it's fine.

@Ptroger Ptroger merged commit 4703b62 into main Mar 21, 2024
2 checks passed
@Ptroger Ptroger deleted the ValidationPipeLogging branch March 21, 2024 11:53
mattschoch pushed a commit that referenced this pull request Jun 19, 2024
* add error management for failed Validation Pipes

* format

* test exception on ping controller

* registered exception filters to policy-engine

* format

* Update apps/armory/src/shared/filter/application-exception.filter.ts

Co-authored-by: William Calderipe <wcalderipe@gmail.com>

* ported filter test file

---------

Co-authored-by: William Calderipe <wcalderipe@gmail.com>
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

Successfully merging this pull request may close these issues.

2 participants