Skip to content

bugfix: encoded query parameter (web) #18

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.

Sign up for GitHub

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 12 commits into from
Jun 29, 2023
Merged

bugfix: encoded query parameter (web) #18

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
d22dc27
Fix problem with encoded comma separated list in query string
pboos Jun 28, 2023
a74634e
Improve handling of query params for some edge cases
pboos Jun 28, 2023
6b5255a
Updated test
pboos Jun 28, 2023
f064a2c
Sample rate for examples to 1 (100%)
pboos Jun 28, 2023
28147dd
Do urldecode on the params when building the "SimpleRequest"
pboos Jun 29, 2023
3b22f8a
Revert also in spring2.7
pboos Jun 29, 2023
1ea62e8
Update comment in build.gradle
pboos Jun 29, 2023
3dfb542
Add test
pboos Jun 29, 2023
cdb7e81
Remove unused import
pboos Jun 29, 2023
f22fb52
Remove unused import
pboos Jun 29, 2023
3629173
Rename method & pass in only value
pboos Jun 29, 2023
4c323f3
Test more special characters
pboos Jun 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion examples/example-spring-boot-starter-web/src/main/resources/application.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
openapi.validation.sample-rate=0.5
openapi.validation.sample-rate=1
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ Unrelated to fix. Increasing to 1 for better example.

openapi.validation.specification-file-path=openapi.yaml
openapi.validation.excluded-paths=/_readiness,/_liveness,/_metrics
openapi.validation.validation-report-throttle-wait-seconds=10
Expand Down
2 changes: 1 addition & 1 deletion examples/example-spring-boot-starter-webflux/src/main/resources/application.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
openapi.validation.sample-rate=0.5
openapi.validation.sample-rate=1
openapi.validation.specification-file-path=openapi.yaml
openapi.validation.excluded-paths=/_readiness,/_liveness,/_metrics
openapi.validation.validation-report-throttle-wait-seconds=10
Expand Down
17 changes: 12 additions & 5 deletions ...-core/src/main/java/com/getyourguide/openapi/validation/core/OpenApiRequestValidator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@
import com.getyourguide.openapi.validation.api.model.RequestMetaData;
import com.getyourguide.openapi.validation.api.model.ResponseMetaData;
import com.getyourguide.openapi.validation.api.model.ValidationResult;
import com.getyourguide.openapi.validation.api.model.ValidatorConfiguration;
import com.getyourguide.openapi.validation.core.validator.OpenApiInteractionValidatorWrapper;
import java.net.URLDecoder;
import java.nio.charset.StandardCharsets;
import java.util.concurrent.RejectedExecutionException;
import java.util.concurrent.ThreadPoolExecutor;
Expand All @@ -32,11 +32,10 @@ public OpenApiRequestValidator(
ThreadPoolExecutor threadPoolExecutor,
ValidationReportHandler validationReportHandler,
MetricsReporter metricsReporter,
String specificationFilePath,
ValidatorConfiguration configuration
OpenApiInteractionValidatorWrapper validator
) {
this.threadPoolExecutor = threadPoolExecutor;
this.validator = new OpenApiInteractionValidatorFactory().build(specificationFilePath, configuration);
this.validator = validator;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ To mock the validator this needs to be injected. So I improved the architecture here (should have done this from the beginning).

this.validationReportHandler = validationReportHandler;
this.metricsReporter = metricsReporter;

Expand Down Expand Up @@ -79,14 +78,22 @@ public ValidationResult validateRequestObject(final RequestMetaData request, Str
private static SimpleRequest buildSimpleRequest(RequestMetaData request, String requestBody) {
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(), nullSafeUrlDecode(p.getValue())));
if (requestBody != null) {
requestBuilder.withBody(requestBody);
}
request.getHeaders().forEach(requestBuilder::withHeader);
return requestBuilder.build();
}

private static String nullSafeUrlDecode(String value) {
if (value == null) {
return null;
}

return URLDecoder.decode(value, StandardCharsets.UTF_8);
}

public ValidationResult validateResponseObject(
final RequestMetaData request,
ResponseMetaData response,
Expand Down
30 changes: 26 additions & 4 deletions ...e/src/test/java/com/getyourguide/openapi/validation/core/OpenApiRequestValidatorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,40 +1,62 @@
package com.getyourguide.openapi.validation.core;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.verify;

import com.atlassian.oai.validator.model.SimpleRequest;
import com.getyourguide.openapi.validation.api.metrics.MetricsReporter;
import com.getyourguide.openapi.validation.api.model.ValidatorConfiguration;
import com.getyourguide.openapi.validation.api.model.RequestMetaData;
import com.getyourguide.openapi.validation.core.validator.OpenApiInteractionValidatorWrapper;
import java.net.URI;
import java.util.HashMap;
import java.util.concurrent.RejectedExecutionException;
import java.util.concurrent.ThreadPoolExecutor;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.mockito.ArgumentCaptor;
import org.mockito.Mockito;

public class OpenApiRequestValidatorTest {

private ThreadPoolExecutor threadPoolExecutor;
private OpenApiInteractionValidatorWrapper validator;

private OpenApiRequestValidator openApiRequestValidator;

@BeforeEach
public void setup() {
threadPoolExecutor = mock();
validator = mock();
ValidationReportHandler validationReportHandler = mock();
MetricsReporter metricsReporter = mock();

openApiRequestValidator = new OpenApiRequestValidator(
threadPoolExecutor,
validationReportHandler,
metricsReporter,
"",
new ValidatorConfiguration(null, null, null)
validator
);
}

@Test
public void testWhenThreadPoolExecutorRejectsExecutionThenItShouldNotThrow() {
Mockito.doThrow(new RejectedExecutionException()).when(threadPoolExecutor).execute(Mockito.any());
Mockito.doThrow(new RejectedExecutionException()).when(threadPoolExecutor).execute(any());

openApiRequestValidator.validateRequestObjectAsync(mock(), null);
}

@Test
public void testWhenEncodedQueryParamIsPassedThenValidationShouldHappenWithQueryParamDecoded() {
var uri = URI.create("https://api.example.com?ids=1%2C2%2C3");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, added more tests for these cases you mentioned (and also for spaces encoded with +). Hope happy you are 😉 .

var request = new RequestMetaData("GET", uri, new HashMap<>());

openApiRequestValidator.validateRequestObject(request, null);

var simpleRequestArgumentCaptor = ArgumentCaptor.forClass(SimpleRequest.class);
verify(validator).validateRequest(simpleRequestArgumentCaptor.capture());
var ids = simpleRequestArgumentCaptor.getValue().getQueryParameterValues("ids").iterator().next();
assertEquals("1,2,3", ids);
}
}
5 changes: 3 additions & 2 deletions ...main/java/com/getyourguide/openapi/validation/autoconfigure/LibraryAutoConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
import com.getyourguide.openapi.validation.api.model.ValidatorConfiguration;
import com.getyourguide.openapi.validation.api.model.ValidatorConfigurationBuilder;
import com.getyourguide.openapi.validation.core.DefaultViolationLogger;
import com.getyourguide.openapi.validation.core.OpenApiInteractionValidatorFactory;
import com.getyourguide.openapi.validation.core.OpenApiRequestValidator;
import com.getyourguide.openapi.validation.core.ValidationReportHandler;
import com.getyourguide.openapi.validation.core.throttle.RequestBasedValidationReportThrottler;
Expand Down Expand Up @@ -112,8 +113,8 @@ public OpenApiRequestValidator openApiRequestValidator(
threadPoolExecutor,
validationReportHandler,
metricsReporter,
properties.getSpecificationFilePath(),
validatorConfiguration
new OpenApiInteractionValidatorFactory()
.build(properties.getSpecificationFilePath(), validatorConfiguration)
);
}
}
3 changes: 1 addition & 2 deletions ...2.7/src/main/java/com/getyourguide/openapi/validation/factory/ServletMetaDataFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@

import com.getyourguide.openapi.validation.api.model.RequestMetaData;
import com.getyourguide.openapi.validation.api.model.ResponseMetaData;
import java.util.Map;
import java.util.TreeMap;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
Expand All @@ -13,7 +12,7 @@ public class ServletMetaDataFactory {
public static final String HEADER_CONTENT_TYPE = "Content-Type";

public RequestMetaData buildRequestMetaData(HttpServletRequest request) {
var uri = ServletUriComponentsBuilder.fromRequest(request).build(Map.of());
var uri = ServletUriComponentsBuilder.fromRequest(request).build().toUri();
Copy link
Contributor Author

@pboos pboos Jun 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ Unrelated to fix. Not strictly needed, but this simplifies the code as internally the old call did this + additional unnecessary logic.

return new RequestMetaData(request.getMethod(), uri, getHeaders(request));
}

Expand Down
1 change: 1 addition & 0 deletions spring-boot-starter/spring-boot-starter-web/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,5 +22,6 @@ dependencies {

testImplementation 'org.springframework.boot:spring-boot-starter-test'
testImplementation 'org.springframework:spring-web'
testImplementation 'org.springframework:spring-webmvc'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ 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 :)

testImplementation 'org.apache.tomcat.embed:tomcat-embed-core' // For jakarta.servlet.ServletContext
}
3 changes: 1 addition & 2 deletions ...web/src/main/java/com/getyourguide/openapi/validation/factory/ServletMetaDataFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
import com.getyourguide.openapi.validation.api.model.ResponseMetaData;
import jakarta.servlet.http.HttpServletRequest;
import jakarta.servlet.http.HttpServletResponse;
import java.util.Map;
import java.util.TreeMap;
import org.springframework.web.servlet.support.ServletUriComponentsBuilder;

Expand All @@ -13,7 +12,7 @@ public class ServletMetaDataFactory {
public static final String HEADER_CONTENT_TYPE = "Content-Type";

public RequestMetaData buildRequestMetaData(HttpServletRequest request) {
var uri = ServletUriComponentsBuilder.fromRequest(request).build(Map.of());
var uri = ServletUriComponentsBuilder.fromRequest(request).build().toUri();
return new RequestMetaData(request.getMethod(), uri, getHeaders(request));
}

Expand Down