-
Notifications
You must be signed in to change notification settings - Fork 114
[RFC] Parse and Joi validate filter #131
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
Merged
Merged
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
8c81b30
Move filter validation into its own module.
pmcnr-hx b95828e
More accurate error reporting and testing.
pmcnr-hx 1407d6e
Extract filter validations into helper functions.
pmcnr-hx acae198
Don't skip validating multiple values for a key.
pmcnr-hx f1d0983
Filters should always be valid in post-processing.
pmcnr-hx 8fdf375
Parse and Joi validate filters.
pmcnr-hx a948a33
Pass query string to related resources.
pmcnr-hx 74710d8
Rename `filter.validate` to `parseAndValidate`.
pmcnr-hx 4357871
`filter.parseAndvalidate` is actually synchronous.
pmcnr-hx File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,141 @@ | ||
| /* @flow weak */ | ||
| "use strict"; | ||
| var filter = module.exports = { }; | ||
|
|
||
|
|
||
| var FILTER_OPERATORS = ["<", ">", "~", ":"]; | ||
| var STRING_ONLY_OPERATORS = ["~", ":"]; | ||
|
|
||
|
|
||
| filter._resourceDoesNotHaveProperty = function(resourceConfig, key) { | ||
| if (resourceConfig.attributes[key]) return null; | ||
| return { | ||
| status: "403", | ||
| code: "EFORBIDDEN", | ||
| title: "Invalid filter", | ||
| detail: resourceConfig.resource + " do not have attribute or relationship '" + key + "'" | ||
| }; | ||
| }; | ||
|
|
||
| filter._relationshipIsForeign = function(resourceConfig, key) { | ||
| var relationSettings = resourceConfig.attributes[key]._settings; | ||
| if (!relationSettings || !relationSettings.__as) return null; | ||
| return { | ||
| status: "403", | ||
| code: "EFORBIDDEN", | ||
| title: "Invalid filter", | ||
| detail: "Filter relationship '" + key + "' is a foreign reference and does not exist on " + resourceConfig.resource | ||
| }; | ||
| }; | ||
|
|
||
| filter._splitElement = function(element) { | ||
| if (!element) return null; | ||
| if (FILTER_OPERATORS.indexOf(element[0]) !== -1) { | ||
| return { operator: element[0], value: element.substring(1) }; | ||
| } | ||
| return { operator: null, value: element }; | ||
| }; | ||
|
|
||
| filter._stringOnlyOperator = function(operator, attributeConfig) { | ||
| if (!operator || !attributeConfig) return null; | ||
| if (STRING_ONLY_OPERATORS.indexOf(operator) !== -1 && attributeConfig._type !== "string") { | ||
| return "operator " + operator + " can only be applied to string attributes"; | ||
| } | ||
| return null; | ||
| }; | ||
|
|
||
| filter._parseScalarFilterElement = function(attributeConfig, scalarElement) { | ||
| if (!scalarElement) return { error: "invalid or empty filter element" }; | ||
|
|
||
| var splitElement = filter._splitElement(scalarElement); | ||
| if (!splitElement) return { error: "empty filter" }; | ||
|
|
||
| var error = filter._stringOnlyOperator(splitElement.operator, attributeConfig); | ||
| if (error) return { error: error }; | ||
|
|
||
| if (attributeConfig._settings) { // relationship attribute: no further validation | ||
| return { result: splitElement }; | ||
| } | ||
|
|
||
| var validateResult = attributeConfig.validate(splitElement.value); | ||
| if (validateResult.error) { | ||
| return { error: validateResult.error.message }; | ||
| } | ||
|
|
||
| var validatedElement = { operator: splitElement.operator, value: validateResult.value }; | ||
| return { result: validatedElement }; | ||
| }; | ||
|
|
||
| filter._parseFilterElementHelper = function(attributeConfig, filterElement) { | ||
| if (!filterElement) return { error: "invalid or empty filter element" }; | ||
|
|
||
| var parsedElements = [].concat(filterElement).map(function(scalarElement) { | ||
| return filter._parseScalarFilterElement(attributeConfig, scalarElement); | ||
| }); | ||
|
|
||
| if (parsedElements.length === 1) return parsedElements[0]; | ||
|
|
||
| var errors = parsedElements.reduce(function(combined, element) { | ||
| if (!combined) { | ||
| if (!element.error) return combined; | ||
| return [ element.error ]; | ||
| } | ||
| return combined.concat(element.error); | ||
| }, null); | ||
|
|
||
| if (errors) return { error: errors }; | ||
|
|
||
| var results = parsedElements.map(function(element) { | ||
| return element.result; | ||
| }); | ||
|
|
||
| return { result: results }; | ||
| }; | ||
|
|
||
| filter._parseFilterElement = function(attributeName, attributeConfig, filterElement) { | ||
| var helperResult = filter._parseFilterElementHelper(attributeConfig, filterElement); | ||
|
|
||
| if (helperResult.error) { | ||
| return { | ||
| error: { | ||
| status: "403", | ||
| code: "EFORBIDDEN", | ||
| title: "Invalid filter", | ||
| detail: "Filter value for key '" + attributeName + "' is invalid: " + helperResult.error | ||
| } | ||
| }; | ||
| } | ||
| return { result: helperResult.result }; | ||
| }; | ||
|
|
||
| filter.parseAndValidate = function(request) { | ||
| if (!request.params.filter) return null; | ||
|
|
||
| var resourceConfig = request.resourceConfig; | ||
|
|
||
| var processedFilter = { }; | ||
| var error; | ||
| var filterElement; | ||
| var parsedFilterElement; | ||
|
|
||
| for (var key in request.params.filter) { | ||
| filterElement = request.params.filter[key]; | ||
|
|
||
| if (!Array.isArray(filterElement) && filterElement instanceof Object) continue; // skip deep filters | ||
|
|
||
| error = filter._resourceDoesNotHaveProperty(resourceConfig, key); | ||
| if (error) return error; | ||
|
|
||
| error = filter._relationshipIsForeign(resourceConfig, key); | ||
| if (error) return error; | ||
|
|
||
| parsedFilterElement = filter._parseFilterElement(key, resourceConfig.attributes[key], filterElement); | ||
| if (parsedFilterElement.error) return parsedFilterElement.error; | ||
|
|
||
| processedFilter[key] = [].concat(parsedFilterElement.result); | ||
| } | ||
|
|
||
| request.processedFilter = processedFilter; | ||
|
|
||
| return null; | ||
| }; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 touchlib/postProcess.jsat all so I am going to remove these changes.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.
Ahhh right, cool 👍
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'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... 😖