diff --git a/CHANGELOG.md b/CHANGELOG.md index 4fcf6dfac..3768b9f74 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,9 @@ +### Version 13.18 + +* Replacing a request URI without a fragment clears the previous fragment; appending a URI and + resolving template variables preserve it. Target URL fragments retain their percent escapes + instead of being decoded (#3095). + ### Version 13.17 * `Request` now accepts a `null` `Request.Body` and treats it as "no body", instead of throwing a diff --git a/core/src/main/java/feign/RequestTemplate.java b/core/src/main/java/feign/RequestTemplate.java index 6f376c4ae..bab7e0d5b 100644 --- a/core/src/main/java/feign/RequestTemplate.java +++ b/core/src/main/java/feign/RequestTemplate.java @@ -234,6 +234,10 @@ public RequestTemplate resolve(Map variables) { /* add the uri to result */ resolved.uri(uri.toString()); + // Fragment state is separate from the path and query expanded above. + if (resolved.fragment == null) { + resolved.fragment = this.fragment; + } /* headers */ if (!this.headers.isEmpty()) { @@ -430,6 +434,9 @@ public RequestTemplate insert(int pos, CharSequence value) { /** * Set the Uri for the request, replacing the existing uri if set. * + *

A fragment in the given uri replaces any existing fragment, even an empty one. Replacing the + * uri with one that has no fragment clears the existing fragment. + * * @param uri to use, must be a relative uri. * @return a RequestTemplate for chaining. */ @@ -463,8 +470,12 @@ public RequestTemplate uri(String uri, boolean append) { int fragmentIndex = uri.indexOf('#'); if (fragmentIndex > -1) { + /* an incoming fragment, even an empty one, replaces any existing fragment */ fragment = uri.substring(fragmentIndex); uri = uri.substring(0, fragmentIndex); + } else if (!append) { + /* replacing the uri without a fragment clears the existing fragment */ + fragment = null; } /* @@ -524,8 +535,9 @@ public RequestTemplate target(String target) { /* strip the query string */ this.target = targetUri.getScheme() + "://" + targetUri.getRawAuthority() + targetUri.getRawPath(); - if (targetUri.getFragment() != null) { - this.fragment = "#" + targetUri.getFragment(); + /* keep the fragment as it was written, the value is never decoded */ + if (targetUri.getRawFragment() != null) { + this.fragment = "#" + targetUri.getRawFragment(); } } catch (IllegalArgumentException iae) { /* the uri provided is not a valid one, we can't continue */ diff --git a/core/src/test/java/feign/RequestTemplateTest.java b/core/src/test/java/feign/RequestTemplateTest.java index b2afd1cea..7b1d53f2a 100644 --- a/core/src/test/java/feign/RequestTemplateTest.java +++ b/core/src/test/java/feign/RequestTemplateTest.java @@ -23,6 +23,7 @@ import feign.Request.HttpMethod; import feign.template.UriUtils; +import java.net.URI; import java.util.Arrays; import java.util.Collection; import java.util.Collections; @@ -579,6 +580,114 @@ void fragmentShouldBeExtractedWhenQueryParamsExist() { assertThat(template.queryLine()).isEqualTo("?query=queryValue"); } + @Test + void resolvingUriShouldPreserveFragment() { + RequestTemplate template = + new RequestTemplate().method(HttpMethod.GET).uri("/path/{id}#anchor"); + + RequestTemplate resolved = template.resolve(Collections.singletonMap("id", "123")); + + assertThat(resolved.url()).isEqualTo("/path/123#anchor"); + assertThat(resolved.request().url()).isEqualTo("/path/123#anchor"); + assertThat(template.url()).isEqualTo("/path/{id}#anchor"); + } + + @Test + void replacingUriShouldClearFragmentFromPreviousUri() { + RequestTemplate template = new RequestTemplate().method(HttpMethod.GET).uri("/old#stale"); + assertThat(template.url()).isEqualTo("/old#stale"); + + /* cannot set a fragment on the replacement uri, the previous one must not survive */ + template.uri("/new?q=v"); + + assertThat(template.url()).isEqualTo("/new?q=v"); + assertThat(template.queryLine()).isEqualTo("?q=v"); + } + + @Test + void explicitlyEmptyFragmentShouldReplacePreviousFragment() { + RequestTemplate template = new RequestTemplate().method(HttpMethod.GET).uri("/old#stale"); + + /* a bare '#' is how a caller explicitly drops the fragment, without a fragment value */ + template.uri("/new#"); + + /* + * RFC 3986 allows an empty fragment and java.net.URI reports it as "" rather than null, so an + * empty fragment is not asserted as stripped, only as no longer being the stale one. + */ + String url = template.url(); + assertThat(url).doesNotContain("stale"); + assertThat(url).isEqualTo("/new#"); + assertThat(URI.create(url).getRawFragment()).isNotNull().isEmpty(); + } + + @Test + void appendingUriShouldPreserveExistingFragment() { + RequestTemplate template = new RequestTemplate().method(HttpMethod.GET).uri("/base#anchor"); + + template.uri("/suffix", true); + + assertThat(template.url()).isEqualTo("/base/suffix#anchor"); + } + + @Test + void incomingFragmentShouldReplaceExistingFragment() { + RequestTemplate replaced = + new RequestTemplate().method(HttpMethod.GET).uri("/old#stale").uri("/new#fresh"); + assertThat(replaced.url()).isEqualTo("/new#fresh"); + + RequestTemplate appended = + new RequestTemplate().method(HttpMethod.GET).uri("/base#stale").uri("/suffix#fresh", true); + assertThat(appended.url()).isEqualTo("/base/suffix#fresh"); + } + + @Test + void targetShouldPreserveRawFragment() { + RequestTemplate template = + new RequestTemplate() + .method(HttpMethod.GET) + .target("https://example.com/a#encoded%20value%2F"); + + /* the raw fragment is re-appended verbatim, not decoded into " " or "/" */ + assertThat(template.url()).isEqualTo("https://example.com/a#encoded%20value%2F"); + assertThat(template.headers()).isEmpty(); + assertThat(template.queries()).isEmpty(); + } + + @Test + void targetShouldPreserveRawFragmentWhenQueryIsPresent() { + RequestTemplate template = + new RequestTemplate() + .method(HttpMethod.GET) + .target("https://example.com/a?x=1#encoded%20fragment"); + + assertThat(template.url()).isEqualTo("https://example.com/a?x=1#encoded%20fragment"); + assertThat(template.queries()).containsKey("x"); + } + + @Test + void targetWithoutFragmentShouldNotClearExistingFragment() { + RequestTemplate template = + new RequestTemplate() + .method(HttpMethod.GET) + .uri("/path#fragment") + .target("https://example.com"); + + assertThat(template.url()).isEqualTo("https://example.com/path#fragment"); + } + + @Test + void copyShouldPreserveAndIsolateFragment() { + RequestTemplate original = new RequestTemplate().method(HttpMethod.GET).uri("/path#fragment"); + + RequestTemplate copy = RequestTemplate.from(original); + assertThat(copy.url()).isEqualTo("/path#fragment"); + + copy.uri("/other#other-fragment"); + assertThat(copy.url()).isEqualTo("/other#other-fragment"); + assertThat(original.url()).isEqualTo("/path#fragment"); + } + @Test void urlEncodingRemainsInPlace() { RequestTemplate template =