fix(router): deduplicate Vary response header during subgraph propagation

[vickyshaw29]

Summary

The gzhttp compression middleware (klauspost/compress) unconditionally adds Vary: Accept-Encoding to every response. When subgraphs also return Vary: Accept-Encoding, the headerPropagationWriter.Write() method in header_rule_engine.go blindly appends it via wh.Add(), resulting in duplicate Vary: Accept-Encoding headers in the client response.

Root Cause

1. gzhttp.NewWrapper() in graph_server.go wraps the handler and adds Vary: Accept-Encoding via w.Header().Add(vary, acceptEncoding) for every compressed response
2. headerPropagationWriter.Write() in header_rule_engine.go propagates all collected subgraph response headers using wh.Add(k, el)
3. Vary is not in the SkippedHeaders map in internal/headers/headers.go, so subgraph Vary values get blindly appended
4. Result: Vary: Accept-Encoding, Vary: Accept-Encoding in the final response

Changes

• router/core/header_rule_engine.go: Modified headerPropagationWriter.Write() to deduplicate Vary header values before propagating. Builds a set of existing Vary directives (handling comma-separated values and case-insensitive comparison), then only adds new directives that aren't already present.
• router/core/header_rule_engine_test.go: Added TestHeaderPropagationWriterDeduplicatesVary with 5 subtests covering exact duplicate prevention, new value propagation, comma-separated value handling, case-insensitive deduplication, and non-Vary header passthrough.

Checklist

• I have discussed my proposed changes in an issue and have received approval to proceed.
• I have followed the coding standards of the project.
• Tests or benchmarks have been added or updated.
• Documentation has been updated on [https://github.com/wundergraph/docs-website]

Fixes #2614

Summary by CodeRabbit

• Bug Fixes
  • Improved Vary header handling to eliminate duplicate values when propagating HTTP response headers. Deduplication is now case-insensitive and performed automatically during header propagation.

[coderabbitai]

Walkthrough

Modified the Vary header propagation logic in the router's header rule engine to prevent duplication by parsing existing Vary values, deduplicating tokens case-insensitively, and merging newly propagated values. Other headers maintain their original append behavior. Added comprehensive test coverage for the deduplication logic.

Changes

Cohort / File(s)	Summary
Header Rule Engine Implementation router/core/header_rule_engine.go	Updated headerPropagationWriter.Write to parse comma-separated Vary header tokens, canonicalize and trim them, merge without duplicates, and append only missing tokens. Other headers continue using standard Add behavior.
Header Rule Engine Tests router/core/header_rule_engine_test.go	Added TestHeaderPropagationWriterDeduplicatesVary test to validate Vary header deduplication behavior: no duplication when already present, appending missing values, handling comma-separated values, case-insensitive matching, and preserving standard behavior for non-Vary headers. Updated imports for test infrastructure.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

• fix(router): fix singleflight deduplication bugs and propagate response headers #2522: Modified router header propagation logic (header_rule_engine.go) and response header propagation tests—directly related to this PR's Vary header deduplication implementation.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% 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 clearly and accurately summarizes the main change: deduplicating the Vary response header during subgraph propagation, which is the core fix in this pull request.
Linked Issues check	✅ Passed	The code changes directly address issue #2614 by implementing deduplication logic for Vary headers in headerPropagationWriter.Write(), preventing duplicate Vary: Accept-Encoding headers.
Out of Scope Changes check	✅ Passed	All changes are scoped to the Vary header deduplication fix: header_rule_engine.go implements the deduplication logic, and header_rule_engine_test.go adds comprehensive test coverage.

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

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

🧹 Nitpick comments (1)

router/core/header_rule_engine_test.go (1)

446-454: Consider simplifying the assertion for clarity.

The assertion on line 453 is convoluted:

assert.False(t, found["Accept-Encoding"] && len(varyValues) > 2)

Since you're already asserting that Accept-Encoding and Origin are both found, a direct length check would be clearer:

♻️ Suggested simplification

 varyValues := rec.Header().Values("Vary")
 found := make(map[string]bool)
 for _, v := range varyValues {
     found[http.CanonicalHeaderKey(v)] = true
 }
 assert.True(t, found["Accept-Encoding"])
 assert.True(t, found["Origin"])
-assert.False(t, found["Accept-Encoding"] && len(varyValues) > 2)
+assert.Len(t, varyValues, 2, "should have exactly 2 Vary values after deduplication")

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@router/core/header_rule_engine_test.go` around lines 446 - 454, The final
assertion is convoluted: instead of assert.False(t, found["Accept-Encoding"] &&
len(varyValues) > 2), replace it with a direct length check on varyValues (or
use assert.Len) to ensure there are exactly two Vary entries since you already
assert Accept-Encoding and Origin exist; update the test around the variables
varyValues and found (populated from rec.Header().Values("Vary")) to assert
len(varyValues) == 2 (or assert.Len(t, varyValues, 2) / assert.Equal(t, 2,
len(varyValues))) for clarity.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@router/core/header_rule_engine_test.go`:
- Around line 446-454: The final assertion is convoluted: instead of
assert.False(t, found["Accept-Encoding"] && len(varyValues) > 2), replace it
with a direct length check on varyValues (or use assert.Len) to ensure there are
exactly two Vary entries since you already assert Accept-Encoding and Origin
exist; update the test around the variables varyValues and found (populated from
rec.Header().Values("Vary")) to assert len(varyValues) == 2 (or assert.Len(t,
varyValues, 2) / assert.Equal(t, 2, len(varyValues))) for clarity.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 3afeced0-62fc-47ed-a793-ac71e0ad4924

📥 Commits

Reviewing files that changed from the base of the PR and between 1a182a7 and 8d558c1.

📒 Files selected for processing (2)

• router/core/header_rule_engine.go
• router/core/header_rule_engine_test.go

[endigma]

Hi @vickyshaw29, is there an accompanying issue for this PR?

It seems like Claude has done most of the work here, and while the solution may function I'd suggest just adding Vary to SkippedHeaders as propagating this doesn't make much sense to me.

We normally ask that there be an issue in advance to discuss what the best solution would be.

I'd also encourage you to read our guidance on using AI in contributions: https://human-oss.dev

[vickyshaw29]

Hey, thanks for the feedback. The accompanying issue is #2614. You are right, adding Vary to SkippedHeaders is the cleaner approach. Propagating it from subgraphs doesn't really serve a purpose since gzhttp already sets it on every compressed response. I overcomplicated this.

I'll close this PR and open a fresh one with the simpler fix. Thanks for pointing me to the human-oss guidelines too, noted for future contributions.

[endigma]

Hi @vickyshaw29, sounds good, remember to link the new PR to the issue in the description.