Resolve embed dependency before HAL rendering strips Request from body

[koriym]

Problem

#[Cacheable] parent + #[Embed] #[Cacheable] child under the HAL renderer never gets the child's URI tag merged into the parent's Surrogate-Key. A PUT on the child therefore does not invalidate the parent cache. The miss is silent — no error, no log line — and only affects the HAL representation, which is exactly the recommended representation for BEAR resources.

The JSON renderer path is unaffected because JsonRenderer does not mutate $ro->body.

Root cause

HalRenderer::valuateElements() (in bearsunday/BEAR.Resource) unsets every AbstractRequest entry from $ro->body during $ro->toString() and moves the rendered result under $ro->body['_embedded']:

unset($ro->body[$key]);
$view = (string) $embeded;
$ro->body['_embedded'][$key] = json_decode($view, null, 512, JSON_THROW_ON_ERROR);

EtagSetter::setCacheDependency() ran after toString() and walked $ro->body for Request instances — by then every Request had been stripped, so depends() was never called and the parent's Surrogate-Key never gained the child URI tag.

Fix

Move the dependency walk into QueryRepository::put() and run it before $ro->toString(), while the original Request instances are still in $ro->body.

The new walk:

• Bails if the resource already has a manually-set SURROGATE_KEY (preserves CacheDependency::depends()'s assertion contract).
• Casts each Request to string before reading its child ETag so lazy decorators (e.g. bear/async's AsyncRequest) flush their batch.

Constructor changes

• EtagSetter / DevEtagSetter: drop unused CacheDependencyInterface argument.
• QueryRepository: receive CacheDependencyInterface as a required constructor argument.

Test

tests/CacheDependencyTest::testHalEmbeddedChildAddsChildSurrogateKeyToParent renders a #[Cacheable] parent that #[Embed]s a #[Cacheable] child under HalModule, then asserts the parent's stored Surrogate-Key contains the child URI tag. Without the fix this assertion fails with an empty Surrogate-Key.

Affected pattern surface

Any HAL+JSON resource using #[Cacheable] (or #[CacheableResponse]) on a parent with #[Embed] children. Reproduced and pinned in bearsunday/MyVendor.Cms app://self/cache/* showcase.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 3dc6b04f-09f6-4304-bb9a-7a949f6b5faa

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Cache dependency resolution logic moves from EtagSetter and DevEtagSetter into QueryRepository. ETag setters retain backward-compatible constructor parameters but no longer store or use them; QueryRepository gains optional cache dependency injection and scans request bodies to register child resource dependencies during put() for HTTP 200 responses. A new HAL integration test validates the behavior with embedded resource fixtures.

Changes

Cache dependency resolution refactoring

Layer / File(s)	Summary
Remove cache dependency from ETag setters src/DevEtagSetter.php, src/EtagSetter.php	DevEtagSetter and EtagSetter drop internal cache dependency wiring. Constructors retain CacheDependencyInterface parameters for backward compatibility but immediately unset them. Internal setCacheDependency() methods and Request imports are removed. __invoke methods no longer trigger dependency registration.
Add cache dependency wiring to QueryRepository src/QueryRepository.php	QueryRepository constructor accepts an optional CacheDependencyInterface parameter, defaulting to new CacheDependency(new UriTag()). A new private setCacheDependency() method inspects ResourceObject request bodies, registers dependencies for child resources with ETAG headers, and is invoked in put() only for HTTP 200 responses before other persistence logic.
Test HAL cache dependency with new fixtures tests/Fake/fake-app/src/Resource/Page/Hal/Child.php, tests/Fake/fake-app/src/Resource/Page/Hal/ParentResource.php, tests/CacheDependencyTest.php	New HAL resource fixture classes provide test infrastructure: ParentResource embeds Child via #[Embed] annotation. A new test case overrides the DI module with HalModule, fetches the parent resource, derives the child UriTag, and asserts the parent's SURROGATE_KEY header contains the child tag.

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 53.85% 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	The title accurately describes the main fix: moving dependency resolution before HAL rendering strips Request instances from the body.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description check	✅ Passed	The pull request description is directly related to the changeset, clearly explaining the problem with HAL-embedded cache dependencies, the root cause, and the fix implemented.

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

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.}

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (41468fa) to head (649eb0d).

Additional details and impacted files

@@             Coverage Diff             @@
##                 1.x      #174   +/-   ##
===========================================
  Coverage     100.00%   100.00%           
  Complexity       246       246           
===========================================
  Files             53        53           
  Lines            746       750    +4     
===========================================
+ Hits             746       750    +4     

☔ 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.

[koriym]

@coderabbitai review

[coderabbitai]

✅ 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.

[koriym]

レビュアーガイド

読む順序

1. src/QueryRepository.php — 修正の核心。put() の処理順と新規 setCacheDependency() メソッドを見る
2. src/EtagSetter.php / src/DevEtagSetter.php — walk が消えたことを確認
3. tests/CacheDependencyTest.php — 新規テスト testHalEmbeddedChildAddsChildSurrogateKeyToParent
4. テスト fixture とテスト側コンストラクタ呼び出しの修正

核心 1 行

\$ro->toString() (HAL renderer が body から Request を unset する) より前に依存解決 walk を走らせる、それだけ。

検証ポイント

順序が正しいか

QueryRepository::put() で、setCacheDependency() が toString() より前にあること。これが逆転すると bug が再現する。

code === 200 ガード

\$ro->code === 200 の時だけ walk する。旧 EtagSetter::__invoke の早期 return と等価。CacheInterceptor が既に code === 200 ? put() : purge() でガードしているので二重ガードに見えるが、put() を別経路で呼んだ時の防御として残してある。

SURROGATE_KEY 早期 return

手動で SURROGATE_KEY を設定したリソースで自動依存収集を上書きしないため。CacheDependency::depends() の assert(! isset(...)) 契約と整合する。

(string) \$body の意図

child Request を強制評価して body->resourceObject->headers[Header::ETAG] を materialize させる。bear/async の AsyncRequest のような lazy decorator を flush する役割もある。

見落としやすい点

EtagSetter / DevEtagSetter のコンストラクタ削除

CacheDependencyInterface 引数を完全に削除。DI バインディングは QueryRepositoryModule 側で QueryRepository に直接渡るので、EtagSetterInterface の binding 経由で wire する必要がなくなった。

テスト側の手動 new

tests/EtagSetterTest.php と tests/ResourceRepositoryTest.php で:

• new EtagSetter(...) → new EtagSetter()
• new QueryRepository(...) 呼び出しに new CacheDependency(new UriTag()) を追加

DI 経由の Module ユーザは影響を受けない。

Fake fixtures

tests/Fake/fake-app/src/Resource/Page/Hal/Child.php と ParentResource.php は最小実装。周辺 fakes と違いライセンス docblock がないが揃えるかどうかは好み。

確認 SHOULD (任意)

• HAL レンダリングを使う既存テスト (tests/CACHE_DEPENDENCY_TESTS.md) が回帰していないか
• Request::__toString() の 2 回呼び出しコストが許容できるか (walk 内 + HalRenderer 内)
• 子が #[Cacheable] でない (= ETag なし) ケースの continue パスが期待通り
• CDN purger 連携 (Fastly/Akamai) で Surrogate-Key 経由の invalidation が動くか

質問されたら

Q: なぜ EtagSetter から walk を消した?
A: HAL renderer が body を mutate してから walk が走るタイミングの問題なので、ETag 計算とは別関心事。QueryRepository::put() のレンダリング前段に置くのが自然。

Q: BC は?
A: Interface 経由 (DI) で利用していれば影響なし。EtagSetter を直接 `new` している外部コードがあれば引数を削除する必要がある (本パッケージ内テスト 2 ファイルのみ該当)。

Q: なぜ `(string) $body` で結果を捨てる?
A: 副作用 (child の評価と `resourceObject` の materialization) が目的。値は使わない。

[koriym]

@coderabbitai Two follow-up commits pushed since the approval:

• `7323844` — Drop BC scaffolding. Constructors are not part of the public contract (interfaces are, DI resolves), so the unused `CacheDependencyInterface` argument is removed from `EtagSetter` / `DevEtagSetter`, and made a required constructor argument on `QueryRepository` (no null fallback, no `new CacheDependency(new UriTag())` default). Two internal test files updated for the manual `new` paths.
• `bd5a792` — Walk by `AbstractRequest` instead of the concrete `Request`. The previous check (`$body instanceof Request`) silently skipped `AbstractRequest` siblings such as `bear/async`'s `AsyncRequest`, so async embed children never reached the dependency walk despite the PR description claiming AsyncRequest support. Matches the abstraction level `HalRenderer` itself uses.

Please re-review.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

Let me fetch the latest changes from the PR to properly review both commits.

---

🏁 Script executed:

#!/bin/bash
# Fetch PR details and recent commits
gh pr view 174 --repo bearsunday/BEAR.QueryRepository --json commits,files,headRefName

# Get the diff for the PR
gh pr diff 174 --repo bearsunday/BEAR.QueryRepository

Length of output: 13822

---

Let me check the DI module wiring before finalizing the review.

---

🏁 Script executed:

#!/bin/bash
# Check QueryRepositoryModule for CacheDependencyInterface binding
cat src/QueryRepositoryModule.php

echo "---"
# Check if CacheDependencyInterface is bound anywhere
rg -n "CacheDependencyInterface" src/ --type=php

echo "---"
# Verify AbstractRequest import in QueryRepository
rg -n "AbstractRequest" src/QueryRepository.php

echo "---"
# Check EtagSetter and DevEtagSetter are clean readonly classes with no constructor
cat src/EtagSetter.php
echo "---"
cat src/DevEtagSetter.php

echo "---"
# Check the full setCacheDependency method in QueryRepository
cat src/QueryRepository.php

Length of output: 10724

---

@koriym: Both follow-up commits look excellent. Here's the breakdown:

---

7323844 — Drop BC scaffolding ✅

EtagSetter / DevEtagSetter: Now clean final readonly classes with no properties and no constructor — perfectly valid PHP and the right shape for stateless collaborators. No dead code remains.

QueryRepository: CacheDependencyInterface $cacheDependency is now a required constructor parameter. The DI wiring in QueryRepositoryModule is fully correct:

$this->bind(CacheDependencyInterface::class)->to(CacheDependency::class);     // line 64
$this->bind(QueryRepositoryInterface::class)->to(QueryRepository::class)->... // line 63

Ray.Di resolves CacheDependency automatically (and UriTag as a concrete, dependency-free class), so module consumers are unaffected. The two internal test files that used new EtagSetter(...) / new QueryRepository(...) are correctly updated.

---

bd5a792 — Walk by AbstractRequest ✅

Using instanceof AbstractRequest instead of instanceof Request is the right fix. It:

• Matches the abstraction level HalRenderer itself uses for the same body walk
• Covers AsyncRequest from bear/async and any future AbstractRequest decorator
• Closes the silent-skip bug for async embed children described in the commit message

---

One minor observation

The (string) $body in setCacheDependency() evaluates each child request twice — once here (to materialize $body->resourceObject->headers), and again inside HalRenderer::valuateElements() during $ro->toString(). Since Request::__toString() in BEAR.Resource is idempotent (memoized after first evaluation), this is safe and the performance cost is negligible. The intent (flush lazy decorators like AsyncRequest before checking the ETAG header) is clear from the comment.

The fixture files (Child.php, ParentResource.php) still lack license docblocks, but as you noted in the reviewer guide that's a style preference — not a blocker.

Both commits are clean and production-ready. 🐇

[approve]

[koriym]

@coderabbitai

「確認 SHOULD」フォローアップ

レビュアーガイドの 4 項目について調査結果を共有します。

✅ Request::__toString() の 2 回呼び出しコスト

実コストゼロです。AbstractRequest::invoke() が $this->result でメモ化済み (AbstractRequest.php:272):

private function invoke(): ResourceObject
{
    if ($this->result === null) {
        $this->result = ($this)();
    }
    return $this->result;
}

walk 内の (string) $body で $this->result が埋まり、HalRenderer 側の 2 回目は (string) $this->result だけ。リソース再呼び出しは起きません。

AsyncRequest 側も同じ性質: PendingRequests が URI で結果をキャッシュするので、初回 (string) で全 pending を parallel 実行、以降は cached lookup (PendingRequests.php:69-76)。

✅ 子が #[Cacheable] でない (ETag なし) ケースの continue パス

テストを追加しました (commit 1e0e175)。ParentOfNonCacheable (Cacheable) が NonCacheableChild (非 Cacheable) を #[Embed] するケースで、子の URI tag が親の Surrogate-Key に 混入しない ことを assert。

❌ HAL レンダリングを使う既存テスト の回帰チェック

レビュアーガイドのこの項目は前提が誤りでした (作成時の私の早とちり)。確認したところ tests/CacheDependencyTest.php の既存テスト (testDestroyByChild など) は HalModule を install しておらず、HAL レンダリングを経由しません。HAL 経路を踏むのは本 PR で追加した testHalEmbeddedChildAddsChildSurrogateKeyToParent のみです。撤回します。

⏭ CDN purger 連携 (Fastly/Akamai)

実環境を要するため CI 不可。本 PR のスコープ外として保留します (Surrogate-Key 自体の生成は単体テストで検証済み)。

---

追加コミット (このコメント以降の関連):

• e5e5082 — (string) $body コメントを Async 挙動込みに訂正
• 1e0e175 — 非 Cacheable 子 embed の continue パスをテストカバー

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

@koriym: Excellent follow-up — all four items are now fully closed out.

AbstractRequest::invoke() memoization — The source reference to $this->result confirms what I noted as "safe and negligible cost" is actually zero cost. The (string) $body in the walk only triggers real work on the first call; the HalRenderer's second call hits the cached result. The updated comment in e5e5082 makes this explicit for future readers, which is a nice touch.

Non-Cacheable child continue path (1e0e175) — The negative assertion (child URI tag must not appear in parent's Surrogate-Key) is exactly the right shape for this test. It guards against a regression where the if (!isset($childRo->headers[Header::ETAG])) continue; guard gets accidentally removed.

HAL regression check (self-corrected) — The correction is accurate. The scope boundary (existing tests = no HalModule, new test = only HAL path) is now clearly delineated, and the concern was indeed a false premise.

CDN purger deferral — Correct call. Surrogate-Key header correctness is the contract being tested; downstream CDN behavior is an integration concern that belongs in an E2E or staging suite, not here.

The PR is in excellent shape with all outstanding items resolved. 🐇