bugfix: encoded query parameter (web) #18
Conversation
pboos
changed the title
| @@ -1,4 +1,4 @@ | |||
| openapi.validation.sample-rate=0.5 | |||
| openapi.validation.sample-rate=1 | |||
There was a problem hiding this comment.
ℹ️ Unrelated to fix. Increasing to 1 for better example.
| var requestBuilder = new SimpleRequest.Builder(request.getMethod(), request.getUri().getPath()); | ||
| URLEncodedUtils.parse(request.getUri(), StandardCharsets.UTF_8) | ||
| .forEach(p -> requestBuilder.withQueryParam(p.getName(), p.getValue())); | ||
| .forEach(p -> requestBuilder.withQueryParam(p.getName(), getQueryParameterValueWithOptionalDecode(p))); |
There was a problem hiding this comment.
ℹ️ This is the actual fix. All the rest is just other small improvements.
|
|
||
| public RequestMetaData buildRequestMetaData(HttpServletRequest request) { | ||
| var uri = ServletUriComponentsBuilder.fromRequest(request).build(Map.of()); | ||
| var uri = ServletUriComponentsBuilder.fromRequest(request).build().toUri(); |
There was a problem hiding this comment.
ℹ️ Unrelated to fix. Not strictly needed, but this simplifies the code as internally the old call did this + additional unnecessary logic.
|
|
||
| testImplementation 'org.springframework.boot:spring-boot-starter-test' | ||
| testImplementation 'org.springframework:spring-web' | ||
| testImplementation 'org.springframework:spring-webmvc' |
There was a problem hiding this comment.
ℹ️ Unrelated to fix. Not needed right now. But this dependency is useful for writing tests in the future.
Background:
I wrote some tests that were for a previous fix of the problem, but they got reverted as I found a nicer way to fix it. When I wrote those tests this dependency was needed for the tests to run. And it took a while to find this dependency to make the tests work. So better leave them for now so we save time in the future :)
...ion-core/src/main/java/com/getyourguide/openapi/validation/core/OpenApiRequestValidator.java
Outdated
Show resolved
Hide resolved
| ) { | ||
| this.threadPoolExecutor = threadPoolExecutor; | ||
| this.validator = new OpenApiInteractionValidatorFactory().build(specificationFilePath, configuration); | ||
| this.validator = validator; |
There was a problem hiding this comment.
ℹ️ To mock the validator this needs to be injected. So I improved the architecture here (should have done this from the beginning).
| return requestBuilder.build(); | ||
| } | ||
|
|
||
| private static String getQueryParameterValueWithOptionalDecode(NameValuePair p) { |
There was a problem hiding this comment.
I wonder why we pass in the NameValuePair instead of directly the value. Why not pass in p.getValue() and rename the method to nullSafeURLDecode?
|
|
||
| @Test | ||
| public void testWhenEncodedQueryParamIsPassedThenValidationShouldHappenWithQueryParamDecoded() { | ||
| var uri = URI.create("https://api.example.com?ids=1%2C2%2C3"); |
There was a problem hiding this comment.
Any other cases we should test, e.g. when & or = is contained encoded in the value, or when we have multiple name-value pairs being passed?
There was a problem hiding this comment.
So, added more tests for these cases you mentioned (and also for spaces encoded with +). Hope happy you are 😉 .
huguenin
left a comment
huguenin
left a comment
There was a problem hiding this comment.
Nice. Feedback for next time: It would make sense to split up into different test cases, so that a test failure will give you more insight into the specific case that is failing. If your tests fail now, the "minimum failing example" won't be evident. Good for this PR though.
When a query parameter with a comma separated list is sent with
,being encoded (e.g.123%2C2%2C3vs1,2,3) then this would be interpreted as a string instead of a list.This does a urldecode when passing the parameter to the validator.