spring-projects
Avoid UriComponentsBuilder.fromUri
Closes #15852
By using UriComponentsBuilder.fromUriString we stay in a single semantic context and do not loose information.
@bodograumann Please sign the Contributor License Agreement!
Click here to manually synchronize the status of this Pull Request.
See the FAQ for frequently asked questions.
Thinking about this more, while it may still be valuable for this PR to have these tests, it seems to me that @rstoyanchev is open to updating UriComponentsBuilder. I think it would be better to focus on addressing the issue in Framework first. Have you already considered submitting a PR there?
True, fixing the underlying issue in UriComponentsBuilder would be preferrable, but I focused on what I could get done in limited time. I'll try to look into adding a test, which will require somehow mocking the RestTemplate and maybe I'll also find time to understand the difference between authority and host enough to propose a solution for UriComponentsBuilder.
The challenge with re-parsing the authority is having to extract the parsing logic out of the whole algorithm. Couldn't you re-parse the full URI string in fallback mode? Something like:
URI uri = URI.create(issuer);
if (uri.getHost() == null && getAuthority() != null) {
uri = UriComponentsBuilder.fromUriString(issuer).build().toURI();
}
This would impact only such URI's, and minimize any potential disruption for all others.
That makes sense @rstoyanchev . Also I couldn't find a way to mock the RestTemplate here, so I create a PR for the spring framework: https://github.com/spring-projects/spring-framework/pull/33614
The challenge with re-parsing the authority is having to extract the parsing logic out of the whole algorithm. Couldn't you re-parse the full URI string in fallback mode? Something like:
URI uri = URI.create(issuer); if (uri.getHost() == null && getAuthority() != null) { uri = UriComponentsBuilder.fromUriString(issuer).build().toURI(); }This would impact only such URI's, and minimize any potential disruption for all others.
Given that since https://github.com/spring-projects/spring-framework/commit/bbb53d03c4f9f65b8bfaca92fbd1e88aabf6e347 UriComponentsBuilder uses the RFC 3986 parser by default, we are here only switching from RFC 2396 to RFC 3986 URI parsing, not to WhatWG parsing anymore.
Do you still think the impact would be too great, @rstoyanchev ?
As for testing: Could I make the oidc, oidcRfc8414 and oauth methods package protected and test them directly? Testing anything involving getConfiguration and the RestTemplate seems unfeasible to me, when we want to vary the hostname.
Given that since https://github.com/spring-projects/spring-framework/commit/bbb53d03c4f9f65b8bfaca92fbd1e88aabf6e347 UriComponentsBuilder uses the RFC 3986 parser by default, we are here only switching from RFC 2396 to RFC 3986 URI parsing, not to WhatWG parsing anymore. Do you still think the impact would be too great, @rstoyanchev ?
The main difference is that UriComponentsBuilder supports URI template variable syntax. Apart from that, it's not possible to guarantee identical results, but parsing via UriComponentsBuilder#fromUriString should generally be aligned with java.net.URI. I will also add that java.net.URI was created quite a while back, and hasn't been updated much.
@jzheaux note that this is in reference to a new URI parser that's used in UriComponentsBuilder as of 6.2, see https://github.com/spring-projects/spring-framework/issues/33639.
If you decide to go with this change, why not pass UriComponentsBuilder into the oidc methods, or perhaps just the raw String and call fromUriString within those?
If you decide to go with this change, why not pass
UriComponentsBuilderinto the oidc methods, or perhaps just the raw String and callfromUriStringwithin those?
Now I remember, that I tried doing something like that before, but oidc and the like call getPath(), so only having the builder is not enough unfortunately.
Thanks for the PR, @bodograumann! And I'm sorry for the extended delay.
In my testing, I found another issue with using URI and so I've adjusted these classes and ClientRegistrations to not use it. You can see these efforts in 571135d2ab06b24d30c91249ca8c7d0b0285a346 and 49e5d3796e019d8ba03c68481e7c1a4327cad751