Skip to content

Deduplicate surrogate keys when concatenating with manually-set header#176

Open
koriym wants to merge 1 commit into
1.xfrom
fix-surrogate-key-deduplication
Open

Deduplicate surrogate keys when concatenating with manually-set header#176
koriym wants to merge 1 commit into
1.xfrom
fix-surrogate-key-deduplication

Conversation

@koriym
Copy link
Copy Markdown
Member

@koriym koriym commented May 29, 2026

Problem

SurrogateKeys::setSurrogateHeader() applied array_unique() only to the internally generated keys. When the ResourceObject already had a manually-set Surrogate-Key header, the keys were simply concatenated with .=, leaving duplicates between the manual set and the internal set.

This happens when a parent resource and its embedded child share a common tag (e.g. a site-wide purge tag): the child's tag is merged into the internal collection via addTag(), then concatenated with the parent's manually-set header, producing duplicates.

The result is functionally correct — Surrogate-Key is an unordered token set and purging is idempotent, so duplicates never cause wrong purges. But it is an inconsistency (internal keys are deduplicated, the concatenation is not), and the bloat is a latent risk on CDNs that cap header length, where truncation could drop tags and cause purge misses. The more embeds, the larger the header.

Fixes #175

Change

Build the full key set first, then apply array_unique() once, so manual and internal keys are deduplicated consistently. The manual-key-first ordering is preserved (existing testSetSurrogateHeader expectation is unchanged).

Also

Add /.phpunit.cache/ to .gitignore. phpunit.xml.dist uses cacheDirectory=".phpunit.cache", which did not match the stale /.phpunit-cache/ (hyphen) entry, so the cache directory was left untracked.

Checks

  • composer cs-fix
  • PHPStan + Psalm
  • composer test

setSurrogateHeader() applied array_unique() only to the internally
generated keys. When the ResourceObject already had a manually-set
Surrogate-Key header, the keys were simply concatenated, leaving
duplicates between the manual and internal sets.

This happens when a parent and its embedded child share a common tag:
the child's tag is merged into the internal collection and then
concatenated with the parent's manual header, producing duplicates.

Build the full key set first and apply array_unique() once, so manual
and internal keys are deduplicated consistently. Duplicates are harmless
to purging but bloat the header, risking purge misses on CDNs that cap
header length.

Also add /.phpunit.cache/ to .gitignore (PHPUnit's cacheDirectory),
which did not match the stale /.phpunit-cache/ entry.

Fixes #175
@koriym
Copy link
Copy Markdown
Member Author

koriym commented May 29, 2026

@coderabbitai review

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 29, 2026

Review Change Stack

📝 Walkthrough

Walkthrough

This PR fixes surrogate key deduplication in SurrogateKeys::setSurrogateHeader(). The method now combines existing manually-set header keys with computed keys, removes duplicates across the merged set, and writes back a normalized result. A new test verifies deduplication works when embedded resources and parent resources share common surrogate tokens. The .gitignore is updated to ignore PHPUnit cache.

Changes

Surrogate Key Deduplication

Layer / File(s) Summary
Core deduplication logic
src/SurrogateKeys.php
setSurrogateHeader() refactored to merge existing header keys and computed surrogate keys, split on spaces, deduplicate the combined set, and write back the normalized space-separated result.
Deduplication test coverage
tests/SurrogateKeysTest.php
New test testSetSurrogateHeaderDeduplicatesWithManualKeys() verifies the method does not duplicate shared tokens when combining manually-set headers with child-derived surrogate keys.
Build configuration
.gitignore
Added /.phpunit.cache/ to ignore PHPUnit cache artifacts.

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 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
Title check ✅ Passed Title accurately describes the main change: deduplicating surrogate keys when concatenating with manually-set headers, which is the primary objective.
Description check ✅ Passed Description is comprehensive and directly relates to the changeset, explaining the problem, solution, and additional changes to .gitignore.
Linked Issues check ✅ Passed The PR correctly addresses issue #175 by deduplicating the full surrogate key set (manual + internal) instead of only internal keys, and preserves manual-key-first ordering as expected.
Out of Scope Changes check ✅ Passed All changes are within scope: surrogate key deduplication logic aligns with #175, test validates the deduplication fix, and .gitignore update matches the phpunit.xml.dist configuration.

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

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix-surrogate-key-deduplication

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

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

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 29, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@codecov
Copy link
Copy Markdown

codecov Bot commented May 29, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (41a9b84) to head (4c1c51f).

Additional details and impacted files
@@             Coverage Diff             @@
##                 1.x      #176   +/-   ##
===========================================
  Coverage     100.00%   100.00%           
  Complexity       246       246           
===========================================
  Files             53        53           
  Lines            750       748    -2     
===========================================
- Hits             750       748    -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
.gitignore (1)

10-10: 💤 Low value

Consider removing the stale hyphenated pattern.

Since the PR objectives note that the hyphenated version was "stale" and line 11 now provides the correct pattern matching phpunit.xml.dist, you may want to remove this obsolete entry to avoid confusion.

🧹 Proposed cleanup
-/.phpunit-cache/
 /.phpunit.cache/
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.gitignore at line 10, Remove the stale hyphenated .gitignore entry
"/.phpunit-cache/" from the file; locate the obsolete pattern "/.phpunit-cache/"
and delete it so only the corrected pattern (the phpunit.xml.dist-related entry
mentioned in the PR) remains, avoiding redundant/obsolete ignore rules.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In @.gitignore:
- Line 10: Remove the stale hyphenated .gitignore entry "/.phpunit-cache/" from
the file; locate the obsolete pattern "/.phpunit-cache/" and delete it so only
the corrected pattern (the phpunit.xml.dist-related entry mentioned in the PR)
remains, avoiding redundant/obsolete ignore rules.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: a8fe44eb-1b8d-48f1-9e5e-62b72f72908b

📥 Commits

Reviewing files that changed from the base of the PR and between 41a9b84 and 4c1c51f.

📒 Files selected for processing (3)
  • .gitignore
  • src/SurrogateKeys.php
  • tests/SurrogateKeysTest.php

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.

setSurrogateHeader() が手動設定キーとの連結時に重複排除されない

1 participant