Skip to content

fix(router): fix append algorithm producing multiple headers instead of comma-separated#2532

Merged
jensneuse merged 4 commits intomainfrom
jensneuse/normalization-tests
Feb 19, 2026
Merged

fix(router): fix append algorithm producing multiple headers instead of comma-separated#2532
jensneuse merged 4 commits intomainfrom
jensneuse/normalization-tests

Conversation

@jensneuse
Copy link
Copy Markdown
Member

@jensneuse jensneuse commented Feb 19, 2026

Summary by CodeRabbit

  • Bug Fixes

    • Header propagation now preserves Set-Cookie as separate header lines and concatenates other header values when appending; router-level response header rules now behave consistently with propagation and precedence.
  • Tests

    • Extensive test coverage added for header propagation behaviors, append/first/last-write semantics, regex matching, defaults/deduplication, error scenarios, canonicalization, and router rule interactions.

Fixes #2531. The response header algorithm: append was producing multiple separate HTTP header lines instead of a single comma-separated value as documented. The fix joins all values into one comma-separated string in applyResponseRuleKeyValue.

Also adds 30 unit tests for previously uncovered functions (createMostRestrictivePolicy, AddCacheControlPolicyToRules, applyResponseRuleKeyValue, PropagatedHeaders, NewHeaderPropagation), plus 7 new integration tests that explicitly assert the single-header contract and default value fallback behavior.

Checklist

…eader (#2531)

The response header propagation append algorithm was creating multiple
HTTP header lines instead of a single comma-separated value. Fix joins
all values into one comma-separated string before writing to the client.
Also adds unit tests for createMostRestrictivePolicy, AddCacheControlPolicyToRules,
applyResponseRuleKeyValue, PropagatedHeaders, and NewHeaderPropagation, plus
integration tests for the append algorithm's single-header contract.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Feb 19, 2026

Router-nonroot image scan passed

✅ No security vulnerabilities found in image:

ghcr.io/wundergraph/cosmo/router:sha-0af67b03165da9cb3d5cf57bf0bf60a9bbae0d41-nonroot

Comment thread router/core/header_rule_engine.go Outdated
@codecov
Copy link
Copy Markdown

codecov Bot commented Feb 19, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 61.00%. Comparing base (957d92a) to head (8bcc9a6).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #2532       +/-   ##
===========================================
- Coverage   89.99%   61.00%   -28.99%     
===========================================
  Files          17      239      +222     
  Lines        3358    25416    +22058     
  Branches      893        0      -893     
===========================================
+ Hits         3022    15506    +12484     
- Misses        336     8589     +8253     
- Partials        0     1321     +1321     
Files with missing lines Coverage Δ
router/core/header_rule_engine.go 83.33% <100.00%> (ø)

... and 255 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Comment thread router/core/header_rule_engine_test.go Outdated
Comment thread router/core/header_rule_engine_test.go Outdated
Comment thread router/core/header_rule_engine_test.go Outdated
Comment thread router/core/header_rule_engine_test.go Outdated
Comment thread router/core/header_rule_engine.go Outdated
Comment thread router-tests/header_propagation_test.go
Comment thread router-tests/header_propagation_test.go Outdated
Comment thread router-tests/header_propagation_test.go
Set-Cookie cannot be comma-combined per RFC 6265 because commas appear
inside cookie values (e.g. Expires dates). The append algorithm now
falls back to multi-line headers for Set-Cookie while comma-joining
all other headers as intended.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Feb 19, 2026

No actionable comments were generated in the recent review. 🎉


Walkthrough

Adds comprehensive tests for header propagation and updates the response-header Append algorithm to join non-Set-Cookie values with commas while preserving Set-Cookie as separate header lines.

Changes

Cohort / File(s) Summary
Header Rule Engine Core Logic
router/core/header_rule_engine.go
Modified Append algorithm: merge existing and new values and comma-join for all headers except Set-Cookie, which is preserved as separate header lines.
Integration / Behavior Tests
router-tests/header_propagation_test.go
Added extensive tests covering Append behavior (comma-joining), Set-Cookie preservation, regex matching, defaults/first-last-write semantics, repeated headers, router response header interactions, canonicalization, and error cases.
Unit Tests for Rule Engine
router/core/header_rule_engine_test.go
Added many unit tests for cache-control policy creation/merging, response rule key-value application (FirstWrite/LastWrite/Append), propagated header validation, NewHeaderPropagation validation, and nil-receiver guards.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 71.43% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: fixing the append algorithm to produce comma-separated headers instead of multiple headers.
Linked Issues check ✅ Passed The PR directly addresses issue #2531 by fixing the append algorithm to join multiple header values with commas instead of emitting multiple header lines, as documented.
Out of Scope Changes check ✅ Passed All changes are directly related to fixing the append algorithm for response header propagation as specified in issue #2531; no out-of-scope modifications detected.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@StarpTech StarpTech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Simplify append logic, clean up test structure, use require.Len,
and add missing test for duplicated defaults with append algorithm.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Member

@endigma endigma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, nit (also applies to a few other tests)

Comment thread router-tests/header_propagation_test.go Outdated
Address remaining PR review nits: replace require.Len + require.Contains
with require.ElementsMatch, and merge two nil-receiver subtests into one.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@jensneuse jensneuse merged commit 88927f6 into main Feb 19, 2026
47 of 49 checks passed
@jensneuse jensneuse deleted the jensneuse/normalization-tests branch February 19, 2026 13:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

response header algortihm append does not append values comma seperated as described in docs

3 participants