Repository navigation
Fix URI fragment replacement and preserve target escapes - #3633
Conversation
A replacement URI should not retain a fragment from the previous URI. Keep the fragment during template resolution and preserve percent escapes in target fragments so they remain valid URL text. Fixes OpenFeign#3095
velo
left a comment
velo
left a comment
There was a problem hiding this comment.
Checked against master: clearing the fragment on non-append uri() is offset by resolve() restoring it, contracts that call uri() without append are unaffected, and getRawFragment() keeps the target's escapes. The new tests fail on master and pass here. Nits, not blocking: in resolve() the null check is always true right after uri(), so plain resolved.fragment = this.fragment would do; and the comment in explicitlyEmptyFragmentShouldReplacePreviousFragment says a bare '#' drops the fragment, but the test asserts that it is kept as empty.
|
Approved, and auto-merge is enabled. It now conflicts with master, but only in CHANGELOG.md, because master already has a |
Signed-off-by: Marvin Froeder <[email protected]> # Conflicts: # CHANGELOG.md
…ent test comment Signed-off-by: Marvin Froeder <[email protected]>
|
Merged master; CHANGELOG.md was removed from the repo so the entry was dropped. Also applied the review nits: plain assignment for the fragment in resolve() and a test comment that matches the assertion. |
Fixes #3095.
Replacing
/old#stalewith/new?q=vretained#stale, with no API to clear it completely. A target such ashttps://example.com/a#encoded%20value%2Falso lost its percent escapes because the decoded fragment was appended to the URL.This clears the old fragment on URI replacement and preserves raw target fragments. Append behavior, explicit empty fragments, copying, and normal template resolution retain their existing behavior. The internal resolve path carries the separately stored fragment forward. No public setter or dependency is added.
Nine tests cover both failures and those controls; the changelog records the replacement semantics for 13.18.
Validation on macOS ARM64, using a fixed JDK 8 toolchain for main sources and an actual Temurin JDK 25.0.4.1 toolchain for test compilation/execution:
./mvnw -Pdev -pl core clean verify: 781 tests, 0 failures/errors, 4 existing conditional skips. All 58 RequestTemplate tests pass../mvnw -Pdev validate: passes with the required JDK 8/11/25 toolchains.Checks ran in an independent checkout of the same base because the license plugin's JGit failed on a Git worktree. License and formatter checks remained enabled; all three tested file hashes match the submitted files. Local toolchain configuration is not part of the patch.
AI assistance: DeepSeek assisted implementation and first-pass review; Codex coordinated reproduction, actual validation, and final diff review.