fix: fix path-variable off-by-one and guard placeholder substitution in rewrite plugin (#6882)

[juicewcode]

Fixes #6882

Changes

• Remove the +1 in rewriteUri.substring(regex.indexOf("{")), so the full
  path-variable value is extracted instead of skipping its first character —
  avoids the silent truncated upstream path (/123 → /23).
• Guard the placeholder-substitution branch with rewriteHandle.getRegex().contains("{")
  so it is only entered when both the replace template and the regex contain {;
  otherwise fall back to the plain replaceAll branch — avoids the
  StringIndexOutOfBoundsException / 500 on every matching request.

Tests

• shouldRewritePathVariableWithCompleteValue — verifies the path-variable
  value is rewritten completely (/shenyu/123), not truncated.
• shouldNotThrowWhenOnlyReplaceContainsPlaceholder — verifies no exception is
  thrown when the replace template has { but the regex does not.

Make sure that:

• You have read the contribution guidelines.
• You submit test cases (unit or integration tests) that back your changes.
• Your local test passed ./mvnw clean install -Dmaven.javadoc.skip=true.

[Aias00]

Review: fix: fix path-variable off-by-one and guard placeholder substitution in rewrite plugin (#6882)

Approved.

Verification

• Root cause (truncation): in the placeholder branch, rewriteUri.substring(regex.indexOf("{") + 1) dropped the first character after {, turning /123 into /23 when mapping /shenyu/{id} → /shenyu/123. The fix removes the +1 so the full value is substituted → /shenyu/123.
• Root cause (crash): the branch was entered whenever getReplace().contains("{"), even if getRegex() had no {. Then getRegex().substring(getRegex().indexOf("{")) indexed at -1 → StringIndexOutOfBoundsException on every matching request. The new guard getReplace().contains("{") && getRegex().contains("{") avoids this; non-placeholder templates fall back to the plain replaceAll branch.
• Confirmed indexOf("{") is evaluated against getRegex(), not rewriteUri, so with the guard in place it is always >= 0 — no out-of-bounds.
• Confirmed PathMatchUtils.replaceAll(path, regex, replacement) uses Pattern.quote(regex) + Matcher.quoteReplacement(replacement), so {id} is treated as a literal placeholder, not a regex. {id} → /123 yields /shenyu/123 as the test expects.
• New tests: shouldRewritePathVariableWithCompleteValue (asserts /shenyu/123) and shouldNotThrowWhenOnlyReplaceContainsPlaceholder (regex /shenyu/.*, replace /new/{id} → result /new/{id}, no exception).

Notes

• Both bugs are fixed with clear regression coverage. No further changes needed.

Thanks!

[juicewcode]

Resolved the merge conflict in RewritePlugin. The conflict was positively caused by whitespace.Please review again. Thank you！

[yu199195]

the code is same？

[juicewcode]

Hi, the tab change here is just from resolving a merge conflict between my branch and master. My actual code changes are in the first commit (f4a3193) of this PR.It is likely that the same changes were already merged into master, so they no longer appear in the diff.

[Copilot]

🟡 Changes recommended

Valid Java regex quantifiers are still misclassified as placeholders and can cause 500 errors.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Fixes rewrite-plugin path-variable truncation and adds safer placeholder handling.

Changes:

• Corrects path-variable extraction offset.
• Adds placeholder guards and regression tests.

File summaries

File	Description
RewritePlugin.java	Updates rewrite and placeholder logic.
RewritePluginTest.java	Adds regression coverage for truncation and missing placeholders.

Review details

• Files reviewed: 1/2 changed files
• Comments generated: 1
• Review effort level: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Aias00]

Approved as PMC (Aias00). Coherent fix with regression tests; green CI, mergeable. Reviewed the diff.