Avoid UriComponentsBuilder.fromUri

[bodograumann]

Closes #15852

By using UriComponentsBuilder.fromUriString we stay in a single semantic context and do not loose information.

[pivotal-cla]

@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.

[pivotal-cla]

@bodograumann Thank you for signing the Contributor License Agreement!

[jzheaux]

Thanks for the PR, @bodograumann! WIll you please also add a test for both servlet and reactive that show that this behavior is now repaired?

[jzheaux]

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?

[bodograumann]

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.

[rstoyanchev]

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.

[bodograumann]

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: spring-projects/spring-framework#33614

[bodograumann]

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 spring-projects/spring-framework@bbb53d0 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.

[rstoyanchev]

Given that since spring-projects/spring-framework@bbb53d0 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 spring-projects/spring-framework#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?

[bodograumann]

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?

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.