[RFC] Parse and Joi validate filter #131
Conversation
Extract individual filter validations into helper functions for more modular and readable code.
pmcnr-hx
changed the title
pmcnr-hx
force-pushed
the
parse-and-joi-validate-search-filter
branch
from
6588ec5 to
3d1e565
Compare
|
Thanks for the contribution 👍 I'm sat here thinking if mutating the original |
|
You're sat there thinking very wise thoughts. I concur with you 100% and will adjust accordingly. That will actually the changes less intrusive and I will be able to simplify the patch. Thanks! |
| if (request.route.query) { | ||
| uri += "&" + request.route.query; | ||
| } | ||
| return uri; |
There was a problem hiding this comment.
Is this change designed to try and remove the leading & from the query string? It's quite hard to read by comparison, I'd rather we either live with the leading ampersand or we simply substring it off with a comment saying // trim the leading ampersand.
There was a problem hiding this comment.
It wasn't just that, it was because we weren't passing the filter to the included resources rerouted request (see 54-56). With the new strategy of not altering the original filter and adding the parsed filter as a new property we won't need to touch lib/postProcess.js at all so I am going to remove these changes.
There was a problem hiding this comment.
I've changed it back to how it was before but we still need to pass the query string to the related resources. I am not sure how the test was working before... 😖
We only meant to skip validation for nested filters, but we were actually skipping array values (multiple values for a property) which should not be skipped.
Filters should always be valid when we get to the post-processing stage so remove the redundant check.
Parse and validate using Joi the filter elements, so this work is only done in one place and the parsed filters can be provided to the handlers, avoid the need to redo that work there as is currently needed.
The test was passing previously by accident. Once the filter is corrected to include a valid email address, it breaks because the filter isn't being passed to the related resource rerouted request. Fix the test and pass the query string (including the filter) to related resources.
pmcnr-hx
force-pushed
the
parse-and-joi-validate-search-filter
branch
from
3d1e565 to
94efbb6
Compare
|
@theninj4: implemented the proposed changes. Adding the processed filter as a new property of the request, instead of mutating the existing filter is indeed a cleaner, less invasive and backward compatible approach 😍. Post-processing filtering is now nicely simplified, and the parsed filter is made available to the handlers simplifying their job and avoiding code duplication there. |
lib/filter.js
Outdated
| return { result: helperResult.result }; | ||
| }; | ||
|
|
||
| filter.process = function(request, callback) { |
There was a problem hiding this comment.
Is filter.process the right name for this? Reading further down and seeing it alongside other high level features left me a bit confused as to what processing it was doing - maybe something like filter.validateAndSanitise could be more appropriate?
There was a problem hiding this comment.
Weird, I though I had renamed it to parseAndValidate... Oops, forgot to push those changes.
|
This is looking good 👍 |
|
Feedback actioned @theninj4. Thanks for the +1. 😀 |
|
Taking 1️⃣ |
|
Looking epic 👍 |
The more generic nature of its functionality warrants the rename.
pmcnr-hx
force-pushed
the
parse-and-joi-validate-search-filter
branch
from
c0bc94c to
4357871
Compare
|
🔎 |
|
Happy with the changes and actioned updates. Looks like some nice work here 👍 |
|
Hmm... this merged into the wrong branch :'( |
|
It was! Just fixing it now. |
Parses and Joi validate, setting values to correct types, during Joi validates.
This was a bit more invasive than initially anticipated, but it should improve the validation of filters while providing a fully parsed filter to both post-processing and handlers, while avoiding duplicated functionality for this in each handler.