Skip to content
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

WebClient's UriBuilder option should encode path parameters passed into build [SPR-17465] #21997

Closed
spring-projects-issues opened this issue Nov 5, 2018 · 7 comments
Assignees
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) status: backported An issue that has been backported to maintenance branches type: bug A general bug
Milestone

Comments

@spring-projects-issues
Copy link
Collaborator

spring-projects-issues commented Nov 5, 2018

Benjamin Conlan opened SPR-17465 and commented

When using the reactive web WebClient's uribuilder building a Uri path value doesn't get encoded as expected:

(Excuse code here... Composed from phone)

webClient.get().uri(builder -> builder.path("/{test}").build("encode/this"))

Behaves differently to:

webClient.get().uri("/{test}", "encode/this")

I would expect the url path segment in the 1st case to also be encoded.


Affects: 5.0.8

Issue Links:

Referenced from: commits 2405161, e4c84ec

Backported to: 5.0.11

@spring-projects-issues
Copy link
Collaborator Author

spring-projects-issues commented Nov 7, 2018

Rossen Stoyanchev commented

Indeed there is an issue.

The explanation is made a little more complicated by the fact that in 5.1, the encoding mode for DefaultUriBuilderFactory changed as part of the work for #21577. Prior to 5.1 the encoding mode was URI_COMPONENTS where if you use path(..), as opposed to pathSegment(..), then "/" is allowed and therefore not encoded. So technically this example is not a regression, but using pathSegment in 5.1 does not work either because which is a regression.

After the change to encoding mode TEMPLATE_AND_VALUES in 5.1, URI variables should be strictly encoded, regardless of whether you use path or pathSegment, due to a bug when using the builder option. After the fix both of the above examples should encode as expected.

For more background on those options see the reference docs.

@spring-projects-issues
Copy link
Collaborator Author

spring-projects-issues commented Nov 9, 2018

Rossen Stoyanchev commented

Benjamin Conlan I updated my previous comment to say 5.1 (instead of 5.0.8), as it should have been.

For 5.0.x the example you gave is expected behavior I'm afraid. You would have to use pathSegment("{test}") instead of path("/{test}"), or as of 5.0.11 (i.e. after the fix) switch the encoding mode from URI_COMPONENT to TEMPLATE_AND_VALUES.

For 5.1.x after the fix in 5.1.3 the example should just work since in 5.1.x the default encoding mode is TEMPLATE_AND_VALUES by default.

@spring-projects-issues spring-projects-issues added type: bug A general bug status: backported An issue that has been backported to maintenance branches in: web Issues in web modules (web, webmvc, webflux, websocket) labels Jan 11, 2019
@spring-projects-issues spring-projects-issues added this to the 5.1.3 milestone Jan 11, 2019
@marceloverdijk
Copy link

I'm upgrading from 5.1.2 to 5.1.7 and facing this issue.

        String query = "{foo}";

        client.get().uri(uriBuilder -> uriBuilder.path("/graphql")
                .queryParam("query", query)
                .build())
                .accept(MediaType.APPLICATION_JSON_UTF8)
                .exchange()
                .expectStatus().isOk();

now returns java.lang.IllegalArgumentException: Not enough variable values available to expand 'foo'.

When using 5.1.2 we encoded the query string manually like:

        String query = "{foo}";
        String queryString = URLEncoder.encode(query, "UTF-8");

        client.get().uri(uriBuilder -> uriBuilder.path("/graphql")
                .queryParam("query", queryString)
                .build())
                .accept(MediaType.APPLICATION_JSON_UTF8)
                .exchange()
                .expectStatus().isOk();

but also this won't work anymore > 5.1.2.

@rstoyanchev what should be the approach from 5.1.3 onwards?

@rstoyanchev
Copy link
Contributor

@marceloverdijk, in 5.1.x TEMPLATE_AND_VALUES encoding applies by default, which is why pre-encoding any part of the template leads to double encoding. In 5.1.2 there was a bug that prevented this.

This encoding mode however applies full encoding to URI variables so you can do:

String query = "{foo}";

client.get().uri(uriBuilder -> uriBuilder.path("/graphql")
		.queryParam("query", "{query}")
		.build(query))
		.accept(MediaType.APPLICATION_JSON_UTF8)
		.exchange()
		.expectStatus().isOk();

@marceloverdijk
Copy link

Thx @rstoyanchev that worked!

@leonard84
Copy link

@rstoyanchev this works for fixed parameters, but how would you suggest handling dynamic query parameters. Having to create an array to hand over to build() seems cumbersome and most of all really error prone.

Here is a short example:

List<String> rels = ... // list of strings also containing special chars

webClient.get().uri(baseUri, uriBuilder -> {
         uriBuilder.queryParam("email", "{email}"); 
         rels.forEach(rel -> uriBuilder.queryParam(REL_PARAM, rel)); // how should we handle this?
         return uriBuilder.build(email);
 }

@rstoyanchev
Copy link
Contributor

UriUtils#encodeQueryParams perhaps, assuming the rest of the URI template is already encoded.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) status: backported An issue that has been backported to maintenance branches type: bug A general bug
Projects
None yet
Development

No branches or pull requests

4 participants