-
Notifications
You must be signed in to change notification settings - Fork 324
Fix encoding of plus sign in query params #1036
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
Conversation
| mockRequest(InteractionContext.builder() | ||
| .request(TestRequest.builder() | ||
| .method(GET).path("/oauth/authorize?client_id=app&redirect_uri=http://localhost:8080/app/&scope=openid&response_type=code%20id_token") | ||
| .method(GET).path("/oauth/authorize?client_id=app&redirect_uri=http%3A%2F%2Flocalhost%3A8080%2Fapp%2F&scope=openid&response_type=code%20id_token") |
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 don't particularly like how this turned out, but tests like this one already had some encoded characters even before my change. See org.cloudfoundry.reactor.uaa.clients.ReactorClientsTest.list().
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.
Thanks @nictas !
| .filter(Objects::nonNull); | ||
| } | ||
|
|
||
| private static UriQueryParameter processValue(AnnotatedValue<QueryParameter> annotatedValue) { |
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.
Alphabetical order
| .filter(Objects::nonNull); | ||
| } | ||
|
|
||
| private static UriQueryParameter processValue(AnnotatedValue<FilterParameter> annotatedValue) { |
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.
Alphabetical order
| .filter(Objects::nonNull); | ||
| } | ||
|
|
||
| private static UriQueryParameter processValue(AnnotatedValue<FilterParameter> annotatedValue) { |
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.
Alphabetical order
| .filter(Objects::nonNull); | ||
| } | ||
|
|
||
| private static <T extends Annotation> Function<Method, AnnotatedValue<T>> processMethod(Object instance, Class<T> annotationClass) { |
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.
Alphabetical order?
| public class UriQueryParameters { | ||
|
|
||
| public static void set(UriComponentsBuilder builder, Stream<UriQueryParameter> uriQueryParameters) { | ||
| // Replace all literal values with URI variables to apply more strict encoding: |
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.
We rarely include comments as it's often a sign that the code itself is obscure, or a method within it could be named better. Having said that I don't see anything here that should obviously be changed, but if you have any ideas feel free!
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 also prefer to not include comments, but I think this one will be helpful, since it's not immediately obvious why we're replacing literal values with URI variables.
| public UriVariable register(Object value) { | ||
| UriVariable uriVariable = UriVariable.of(getNextUriVariableKey(), value); | ||
| uriVariables.add(uriVariable); | ||
| return uriVariable; |
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.
House style is a blank line above return statements if they're not one-liners.
cloudfoundry-client-reactor/src/main/java/org/cloudfoundry/reactor/util/_UriVariable.java
Show resolved
Hide resolved
This is a preparation for the next commit where we'll start using URI variables, instead of literals, for query param values.
Fixes #1034.