add onError to validateRequestOptions#1101
add onError to validateRequestOptions#1101srsudar wants to merge 10 commits intocdimascio:masterfrom
Conversation
|
@cdimascio -- any thoughts on this? I'd be happy to try some cleanup if it seems like something you're interested in merging. If not, I might maintain a fork. I think I'll need this to be able to safely begin integrating |
| coerceTypes, | ||
| removeAdditional, | ||
| onError, | ||
| } = <ValidateRequestOpts>this.options.validateRequests; |
There was a problem hiding this comment.
Is this change necessary? Does the list include the complete set of user specified options? If not, it will change/ break existing behavior
There was a problem hiding this comment.
I thought that I did need it, yes, b/c otherwise the onError property isn't actually passed from the constructor.
Confirmed that removing it breaks the tests I added.
I thought this would be a no-op if you weren't defining this new property. I didn't break any existing tests, so I thought this was safe.
What am I missing?
| allowUnknownQueryParameters?: boolean; | ||
| coerceTypes?: boolean | 'array'; | ||
| removeAdditional?: boolean | 'all' | 'failing'; | ||
| onError?: (err: InternalServerError, req: Request) => void; |
There was a problem hiding this comment.
Is the intend of this change to introduce onError to requests, mirroring what exists currently (but only) for responses?
There was a problem hiding this comment.
Yes that's right. My thinking is that we currently have three platforms hitting our APIs. I believe I've captured the the real-world usage of our APIs, but there are enough edge cases that I'd believe I've missed something. I'd like to be able to introduce validation slowly, basically making sure that we are correctly extracting the openapi interface before I begin actually serving 400s using the validator.
| throw error; | ||
| if (this.requestOpts.onError) { | ||
| this.requestOpts.onError(error, req); | ||
| next(); |
There was a problem hiding this comment.
do we need to call next() here? we should expect onError handlers to always throw, correct? or is the intent to also allow clients to eat errors?
There was a problem hiding this comment.
My thinking was that we should let clients eat the error, yeah.
In the case I'm implementing it for, eg, I want to basically know when express-openapi-validator says there's an error, log it, but not actually cause the client request to fail.
If the client wants an error, they can re-throw. This is how I've been using the response validator, and as far as I can tell it is working.
2 participants
I would like to be able to slowly and safely introduce this to an existing project. It's possible that we have some misbehaving clients, an I'd like to be able to identify cases where we aren't correctly describing types, without necessarily breaking the clients.
It seems like to do this, and be aware of bad requests, I want an
onErroroption inValidateRequestOptions, the same way it exists inValidateResponseOptions.This PR is an attempt to accomplish that. I cribbed pretty liberally from the same tests for response validation, and I'd believe of course that I'm missing something. I welcome comments!