Allowing request body to be coerced #468
Conversation
| @@ -42,6 +42,7 @@ export interface RequestValidatorOptions | |||
|
|
|||
| export type ValidateRequestOpts = { | |||
| allowUnknownQueryParameters?: boolean; | |||
| coerceTypes?: boolean | 'array'; | |||
There was a problem hiding this comment.
There are two cases for request coercion, i.e. query parameters and body
response coercion has only one type, body
in order to satisfy both request coercion cases, there could be two properties under coerceTypes. this would give complete control, but it results in a lot of options (which i don't like).
all that said, i wonder if it reasonable to go with what you have...
i don't ever see a reason why one wouldn't coerce query params. its essentially required since they all come in as strings. that said, there are some edge cases e.g. a complex json object passed as query param value. in such cases, coercion applies. thinking on this, i think it makes sense to apply the same coercion rules. if so, this change works
thoughts?
There was a problem hiding this comment.
Are you saying this setting controls both query params and body? I only meant to allow body to be coerced.
There was a problem hiding this comment.
I just had a look and it doesn't look like that to me, I may be wrong though! I think it only uses that option to validate body: https://github.com/MadMango/express-openapi-validator/blob/master/src/middlewares/openapi.request.validator.ts#L49
There was a problem hiding this comment.
Ya. I was wondering wether this should be an option for query params, path params, headers, cookies
Ultimately, it should not as those inputs can only come in as strings hence, they must be coerced.
All in all, I posed the question becasuse query params, path params, headers, and cookies will behave differently than bodies. I imagine this may be confusing to folks.
That says, I believe this is the correct behavior and just needs documenting in the README
thoughts?
There was a problem hiding this comment.
Yeah I think all of those should be coerced and I believe they are at the moment and we're only leaving the option for coercing body. Should I make it very clear in the readme that his option only applied to the request body?
There was a problem hiding this comment.
Ya. let's add that to the readme. i'll get this merged in soon.
Thanks for your work, @MadMango
There was a problem hiding this comment.
I've updated the README, please have a look and see if you're happy with the wording, tried to keep it consistent
* allowing to coerce body * changing readme to more informative tone * fix readme * add missing bit of readme * fixed formatting * README - made it clear that validateRequests.coerceTypes only applies to body
|
@ex1st this last commit didn't make it into the merge, can you put up a new PR with the change. we can review it via a fresh PR. thanks! |
We talked about implementing it in #387 and #396
Hope the code is fine, let me know if it needs any tweaks, it's still false as default so it shouldn't affect current behaviour in anyone's projects.
Added tests to cover new features.
I've also made a change to the global
coerceTypessetting, it wasn't affecting anything apart from setting it to true on response validation which seems counterintuitive, especially that now we have both response and request validation so I just removed the code that seemed redundant on these lines