refactor: add option to configure error propagation #67
Conversation
| debug(JSON.stringify(error)); | ||
| res.status(error.status || 500).send(error); | ||
| if (options.propagateError) { |
There was a problem hiding this comment.
Will we always have a non null options object? Otherwise, this line could throw an error instead of jumping to the else.
There was a problem hiding this comment.
I'm passing in the normalizedOptions, which always returns an object according to lib/normalizeOptions.js
| * @see https://github.com/cdimascio/express-openapi-validator#asyncawait | ||
| */ | ||
| const add = app => { | ||
| const add = (app, options) => { |
There was a problem hiding this comment.
| const add = (app, options) => { | |
| const add = (app, { propagatedError }) => { |
I'm not sure if this can make the code more readable.
There was a problem hiding this comment.
Sure. However, if we keep adding more options, it could be a bit cumbersome
| @@ -3,14 +3,19 @@ const debug = require('debug')('swagger-endpoint-validator:custom-error-handler' | |||
| /** | |||
| * Adds a custom error handler for the errors thrown by the validator. | |||
| * @param {Object} app - Express application object. | |||
| * @param {Object} options - Configuration options. | |||
There was a problem hiding this comment.
Again, I'm not sure if adding here the only field expected in this object is better for readibility or not.
There was a problem hiding this comment.
I'm following the same convention as in lib/normalizeOptions.js. It's true that there more options are used. However, if we plan to keep adding new options, it could become a bit difficult to maintain. Anyway, all the options are documented in index.js
|
@neodmy LGTM |
| @@ -0,0 +1,37 @@ | |||
| const request = require('supertest'); | |||
|
@all-contributors please add @neodmy for code, test |
|
I've put up a pull request to add @neodmy! 🎉 |
Description
Provide an additional option to control whether the library should send the response with the validation error or let it get passed to the next error handler.
Related Issue
No issue related so far. I will add the link when created.
Motivation and Context
I'm working on a project in which it is necessary to register errors to a vendor analytic server. One of the approaches using the SDK of that tool is by setting an Express error handler and explicitly executing one of the functions available on their API. In our project, we are using
swagger-endpoint-validation. The problem is that in the current implementation, validation errors are always captured, and therefore they never reach our error handler. We've tried initializingswagger-endpoint-validationafter our own error handlers have been registered in the Express stack, but then the payload and params validation is not performed, so faulty data reaches our business logic making it crash.After checking with the team, we agreed that adding an additional option that controls whether the error handle that this library uses propagates the error, or sends the response with the validation error.
To ensure backward compatibility, the error propagation is off by default. So current users of this library are not forced to set their own error middleware necessarily.
How Has This Been Tested?
I've used Node 16, as specified in the
.nvmrc. Tests consist of 3 test cases:propagateErroristrue, the validation error should reach the next error handlerpropagateErrorisfalseor not defined, the validation error should not reach the next error handlerScreenshots (if appropriate):
Types of changes
Checklist: