Skip to content

fix(plugin): rename header keys instead of overwriting values in modify-response - #7418

Open
Sean-Walker0 wants to merge 1 commit into
apache:masterfrom
Sean-Walker0:fix/6510-modify-response-rename-header-keys
Open

Sean-Walker0 wants to merge 1 commit into
apache:masterfrom
Sean-Walker0:fix/6510-modify-response-rename-header-keys

Conversation

@Sean-Walker0

Copy link
Copy Markdown
Contributor

Fixes #6510

Modifications

ModifyResponsePlugin applied the replaceHeaderKeys map with httpHeaders.replace(key, Collections.singletonList(value)). HttpHeaders.replace keeps the key and replaces the values, so a rule {"X-Old": "X-New"} against an upstream X-Old: abc returned X-Old: X-New instead of renaming the header to X-New: abc — contradicting the documented contract (key: oldHeaderKey, value: newHeaderKey) and the sibling RequestPlugin#replaceHeaderKey, which renames the key while preserving the values.

The fix renames the header key and carries the original values over, mirroring the request-side implementation:

replaceHeaderMap.forEach((key, value) -> {
    List<String> values = httpHeaders.get(key);
    if (Objects.nonNull(values)) {
        httpHeaders.addAll(value, values);
        httpHeaders.remove(key);
    }
});

Verifying this change

  • New ModifyResponsePluginTest#testReplaceHeaderKeysRenamesKeyAndKeepsValues — fails on the pre-fix code (expected: <false> but was: <true>, the old key survived and the values were overwritten) and passes after the fix; it also asserts multiple header values are preserved under the new key.
  • New ModifyResponsePluginTest#testReplaceHeaderKeysLeavesMissingSourceKeyUntouched — a configured source key absent from the response stays a no-op.
  • ./mvnw -pl shenyu-plugin/shenyu-plugin-modify-response -am test -B — 13/13 module tests green, checkstyle clean.

Notes

  • Behavior change (the bug fix itself): responses configured with replaceHeaderKeys now return the header under the new name with the original values, instead of the old name with the new name as the value. This matches the rule-handle javadoc and the request-plugin sibling.
  • Side finding (not addressed here): addHeaders uses httpHeaders::add, which appends to existing values rather than replacing them; that is a separate semantic question for maintainers.
  • Orthogonality: no open PR touches shenyu-plugin-modify-response (verified against the file lists of all 92 open PRs).

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 (module-scoped: shenyu-plugin/shenyu-plugin-modify-response with -am, tests + checkstyle green).

…fy-response

ModifyResponsePlugin applied the replaceHeaderKeys map with
HttpHeaders.replace(key, singletonList(value)), which keeps the old
header name and overwrites its values: a rule {"X-Old": "X-New"} on
an upstream response X-Old: abc produced X-Old: X-New instead of
renaming the header to X-New: abc. This contradicts the documented
contract of ModifyResponseRuleHandle.replaceHeaderKeys (key:
oldHeaderKey, value: newHeaderKey) and the sibling
RequestPlugin#replaceHeaderKey, which renames the key and preserves
the values.

Rename the header key and carry the original values over, mirroring
the request-side implementation. Regression tests cover the rename
(multiple values preserved, old key dropped) and the missing
source key no-op.

Fixes apache#6510

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] ModifyResponse replaceHeaderKeys changes header value instead of header name

1 participant