-
Notifications
You must be signed in to change notification settings - Fork 108
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
Add REST API query normalization #9190
base: main
Are you sure you want to change the base?
Add REST API query normalization #9190
Conversation
Signed-off-by: Edwin Greene <[email protected]>
Signed-off-by: Edwin Greene <[email protected]>
Signed-off-by: Edwin Greene <[email protected]>
Signed-off-by: Edwin Greene <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #9190 +/- ##
============================================
+ Coverage 92.33% 92.55% +0.22%
+ Complexity 7600 6997 -603
============================================
Files 915 903 -12
Lines 30503 29643 -860
Branches 3727 3745 +18
============================================
- Hits 28165 27437 -728
+ Misses 1506 1442 -64
+ Partials 832 764 -68 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Edwin Greene <[email protected]>
Signed-off-by: Edwin Greene <[email protected]>
Quality Gate passedIssues Measures |
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 really like this. I can import normalizeRequestQueryParams()
into the new response caching middleware and use it to form most of the cache key, along with Accept-Encoding
header value.
As far as this PR goes, it can be merged independently as no code is invoked as configured Express middleware or as a query processor.
input: { | ||
path: '/api/v1/blocks', | ||
query: { | ||
order: 'asc', | ||
unknown: '3', | ||
limit: '5', | ||
}, | ||
}, | ||
expected: '/api/v1/blocks?limit=5&order=asc&unknown=3', |
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.
For clarity and brevity, it would be better if input was simply a string even if you have to parse out the query params from that string in the test. Also, should provide a description that's relayed to jest.
input: { | |
path: '/api/v1/blocks', | |
query: { | |
order: 'asc', | |
unknown: '3', | |
limit: '5', | |
}, | |
}, | |
expected: '/api/v1/blocks?limit=5&order=asc&unknown=3', | |
description: 'Query parameters are sorted', | |
input: '/api/v1/blocks?order=asc&unknown=3&limit=5', | |
expected: '/api/v1/blocks?limit=5&order=asc&unknown=3', |
if (_.isUndefined(openApiMap)) { | ||
const openApiObject = getV1OpenApiObject(); | ||
const patternMap = getPathParametersPatterns(openApiObject); | ||
openApiMap = new Map(); |
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.
Should not assign openApiMap until after it's populated. Use a temporary variable.
* @param patternMap | ||
* @returns {RegExp} | ||
*/ | ||
const pathToRegexConverter = (path, patternMap) => { |
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.
Instead of searching 20+ regexes to map it to OpenAPI, we should enable the OpenAPI middleware which already attaches an OpenAPI object to each request. This should be faster than our approach. We should take care to disable request and response validation outside tests though.
Description:
Adds a means to normalize REST API queries.
Related issue(s):
Fixes #9113
Notes for reviewer:
Checklist