Avoid opaque to hierarchical reset in UriComponentsBuilder when input is null

[kganesh]

Affects: 4.2.x

Execute below test case and verify the result:

public void testURIComponentBuilder(){
        String uri = "urn:ietf:wg:oauth:2.0:oob";
	UriComponentsBuilder redirectUriBuilder =  UriComponentsBuilder.fromUriString(uri);
	redirectUriBuilder.replaceQuery(null);
	redirectUriBuilder.fragment(null);
	redirectUriBuilder.build().toUriString();
	assertThat(redirectUriBuilder.toUriString()).isEqualTo(uri);
}

Expected result: urn:ietf:wg:oauth:2.0:oob

Actual result: urn:

[sbrannen]

I've edited your comment to improve the formatting. You might want to check out this Mastering Markdown guide for future reference.

[sbrannen]

I have verified that the following test fails against master.

@Test
public void replaceQueryWithNull() {
	String uri = "urn:ietf:wg:oauth:2.0:oob";
	UriComponentsBuilder redirectUriBuilder = UriComponentsBuilder.fromUriString(uri);
	redirectUriBuilder.replaceQuery(null);
	assertThat(redirectUriBuilder.toUriString()).isEqualTo(uri);
}

Commenting out redirectUriBuilder.replaceQuery(null); causes the test to pass.

The cause appears to be the fact that replaceQuery(String) invokes resetSchemeSpecificPart() as a side effect. Commenting out that invocation also causes the test to pass.

@rstoyanchev, what are your thoughts on the matter?

[rstoyanchev]

UriComponentsBuilder can create either opaque or hierarchical URIs. The opaque variant has only a scheme, scheme specific part, and possibly a fragment. All other methods on UriComponentsBuilder are for hierarchical URIs, so when you use one of those we reset the internal state (see the reset prefixed private methods).

We can try to improve the behavior in some way. We could for example avoid or defer resetting if what you're setting a component to is null. Would that work for your case and can you explain the reason for the above code in the first place?

[kganesh]

I've edited your comment to improve the formatting. You might want to check out this Mastering Markdown guide for future reference.

Thank you!

[kganesh]

UriComponentsBuilder can create either opaque or hierarchical URIs. The opaque variant has only a scheme, scheme specific part, and possibly a fragment. All other methods on UriComponentsBuilder are for hierarchical URIs, so when you use one of those we reset the internal state (see the reset prefixed private methods).

We can try to improve the behavior in some way. We could for example avoid or defer resetting if what you're setting a component to is null. Would that work for your case and can you explain the reason for the above code in the first place?

The test case is motivated by the behavior of Spring Oauth2 DefaultRedirectResolver. Please see below code excerpt from the class -

private String obtainMatchingRedirect(Set<String> redirectUris, String requestedRedirect) {
	Assert.notEmpty(redirectUris, "Redirect URIs cannot be empty");

	if (redirectUris.size() == 1 && requestedRedirect == null) {
		return redirectUris.iterator().next();
	}

	for (String redirectUri : redirectUris) {
		if (requestedRedirect != null && redirectMatches(requestedRedirect, redirectUri)) {
			// Initialize with the registered redirect-uri
			UriComponentsBuilder redirectUriBuilder = UriComponentsBuilder.fromUriString(redirectUri);

			UriComponents requestedRedirectUri = UriComponentsBuilder.fromUriString(requestedRedirect).build();

			if (this.matchSubdomains) {
				redirectUriBuilder.host(requestedRedirectUri.getHost());
			}
			if (!this.matchPorts) {
				redirectUriBuilder.port(requestedRedirectUri.getPort());
			}
			redirectUriBuilder.replaceQuery(requestedRedirectUri.getQuery());		// retain additional params (if any)
			redirectUriBuilder.fragment(null);
			return redirectUriBuilder.build().toUriString();
		}
	}

Note below lines from above code snippet -

        redirectUriBuilder.replaceQuery(requestedRedirectUri.getQuery());		
	redirectUriBuilder.fragment(null);
	return redirectUriBuilder.build().toUriString();

For opaque URI - requestedRedirectUri.getQuery() is null which creates above test case. Invocation of method void resetSchemeSpecificPart() in UriComponentsBuilder query(String query) and UriComponentsBuilder replaceQuery(String query) sets UriComponentsBuilder member ssp to null. This results opaque URI built as HierarchicalUriComponents. Code below:

public UriComponents build(boolean encoded) {
	if (this.ssp != null) {
		return new OpaqueUriComponents(this.scheme, this.ssp, this.fragment);
	}
	else {
		return new HierarchicalUriComponents(this.scheme, this.userInfo, this.host, this.port,
					this.pathBuilder.build(), this.queryParams, this.fragment, encoded, true);
	}
}

This breaks the upstream DefaultRedirectResolver obtainMatchingRedirect() code handling redirects for Opaque URIs.

[rstoyanchev]

Thanks for the extra details. Based on this, I will make changes to avoid switching from opaque to hierarchical if the input is null (or -1 for port).

[kganesh]

Could the fix be ported to 4.2.x? Thank you!

[jhoeller]

Note that 4.2.x has reached its end-of-life (EOL) several years ago, and even its immediate successor 4.3.x is only supported until the end of 2020. We can consider a backport all the way down to 5.1.x, 5.0.x and 4.3.x but I'm afraid this is as far as we could possibly take it, and even this only if the fix is not expected to have any side effects. Please upgrade at your earliest convenience - at least to 4.3.x.

[kganesh]

Note that 4.2.x has reached its end-of-life (EOL) several years ago, and even its immediate successor 4.3.x is only supported until the end of 2020. We can consider a backport all the way down to 5.1.x, 5.0.x and 4.3.x but I'm afraid this is as far as we could possibly take it, and even this only if the fix is not expected to have any side effects. Please upgrade at your earliest convenience - at least to 4.3.x.

@jhoeller Thank you for the update.