Fix URI fragment replacement and preserve target escapes

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

core/src/main/java/feign/RequestTemplate.java

@@ -234,6 +234,10 @@ public RequestTemplate resolve(Map<String, ?> 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.
    *
+   * <p>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 */

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 =