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

UriComponentsBuilder.toUriString() is broken [SPR-17630] #22161

Closed
spring-projects-issues opened this issue Dec 28, 2018 · 5 comments
Closed

UriComponentsBuilder.toUriString() is broken [SPR-17630] #22161

spring-projects-issues opened this issue Dec 28, 2018 · 5 comments

Comments

@spring-projects-issues
Copy link
Collaborator

@spring-projects-issues spring-projects-issues commented Dec 28, 2018

rougou opened SPR-17630 and commented

UriComponentsBuilder.toUriString() (and possibly UriComponentsBuilder.encode()) is broken as of 5.0.8.

The code was changed from 

return build(false).encode().toUriString();

 to

return encode().build().toUriString();

without any change to the javadoc.

There are 2 problems with this (other than the javadoc issue)

  1. UriComponentsBuilder itself gets mutated by calling toUriString(), which is unexpected behavior.
  2. If you have a query param value with curly braces such as "{{{{", an IllegalStateException will now be thrown. If you have a closing brace such as "{{{}", no exception will be thrown but it won't be url-encoded, either.

On top of that, the javadoc for UriComponentsBuilder.encode() basically says it will give the expected result while UriComponents.encode() will not, but not encoding braces and throwing IllegalStateExceptions is obviously not the expected result. It seems the variable expansion mechanism introduced is affecting behavior even when this feature is not used.

 

 


Affects: 5.0.8, 5.1.1

Issue Links:

  • #21565 HtmlUnitRequestBuilder decodes plus sign in query parameter
  • #21577 Support stricter encoding of URI variables in UriComponents

Referenced from: commits b219c6c, 5aa131a, 124e817, dc3f953, c6e5008, 6398480

Backported to: 5.0.12

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jan 2, 2019

Rossen Stoyanchev commented

This should be fixed now, on both points. For the second, the code is now more lenient about mismatched curly braces, treating them as if it is just part of the literal parts of the URI template, and not any kind of URI variable. However anything that looks like a URI variable, i.e. with properly matching curly braces, is left out of the encoding. It'd be useful if you could give the changes a try.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jan 8, 2019

rougou commented

Rossen Stoyanchev,

It works as you say. However, I'm still not sure why "{}" should be left out of encoding when I'm not trying to use variables at all. Is there any way to turn this feature off? As it is, the only viable solution is to call builder.build().encode().toUriString()

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jan 8, 2019

Rossen Stoyanchev commented

Yes, you're right. The behavior was introduced with support for pre-configuring URI variables at the UriComponentsBuilder level. If that's not used, it makes more sense to use the original behavior, which is to encode the full template. I've restored adjusted the behavior accordingly. Thanks for the issue and feedback.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jan 9, 2019

rougou commented

Rossen Stoyanchev,

Thanks, that should work in theory. Seems you are still calling encode().build() instead of build().encode() though.

c6e5008#diff-73d13991926d9c9b00031a62c7e39c95R461

 

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jan 9, 2019

Rossen Stoyanchev commented

That should be fixed now. I've added a test as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants