-
Notifications
You must be signed in to change notification settings - Fork 150
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
feat: validateBooleanMiddleware for controllers #1023
Conversation
typeof req.query[queryParam] === 'string' | ||
? (req.query[queryParam] as string).toLowerCase() | ||
: ''; | ||
if (!(queryParamVal === 'true' || queryParamVal === 'false')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this is the idiomatic way of checking the queryParams, a bit annoying but fair enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it's because all query params are strings.
Maybe worth doing queryParamVal.toLowerCase()
to support eg TRUE or True? Not too bothered though; nothing wrong with being strict and accepting only true
or false
.
In some apps, the mere presence or absense of a query param sets it to true or false, eg ?foo&bar
leads to foo and bar being true. Again, I'm happy if we just want to be stirct and support only =true
or =false
; just throwing it out there :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe worth doing queryParamVal.toLowerCase() to support eg TRUE or True? Not too bothered though; nothing wrong with being strict and accepting only true or false.
I actually already set queryParamVal
to lower case above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yeah; apologies; I somehow missed that entirely :)
* @param queryParams An array of queryParams to check for. These are passed in at the controller level. | ||
*/ | ||
export const validateBooleanMiddleware = ( | ||
queryParams: string[] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you wanted to save a few characters you could do ...queryParams: string[]
here (if i recall the spread syntax properly) to avoid needing array parens anywhere... since it's just a manually built list anyway each time.
But not important at all!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about this at first too actually and I think I settled with the explicit array because it makes it more clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love it; a huge improvement from the global list :)
Summary
This adds validation for boolean middleware inputs. It checks specifically for
true
, andfalse
for known query params that expect booleans. It will error with a 400 BadRequest. For query params that are not part of the API, as normal we just ignore them as the API should not handle those query params.Extra
I cleaned up the validation tests and utils for both validateBoolean and validateAddress by creating a util folder that handles that functionality.
closes: #398