From 491799f46e7d02061bf4ab5cd6155d87177a482e Mon Sep 17 00:00:00 2001 From: Akihito Koriyama Date: Mon, 7 Sep 2026 18:37:19 +0900 Subject: [PATCH 1/4] Bind command methods on the donut path to the command interceptor A write annotated #[RefreshCache], or carrying a method-level #[CacheableResponse], was woven with DonutCacheInterceptor. That interceptor answers from the donut store and returns before proceed(), so once the page was cached the write never ran: onPut / onPost / onDelete answered 200 with the cached representation instead of their own. Measured on a 36-cell weaving matrix - 9 cells lost the write, and #[DonutCache] escaped only because it stores no entire-content entry to hit. Both now bind DonutCommandInterceptor, which proceeds first and then purges and refreshes - what #[RefreshCache]'s own docblock promises. The class-level write matcher also missed onPost, the gap #212 fixed on the value-cache side and this one did not. DonutCommandInterceptorTest asserted the wrong binding, which is why 368 tests stayed green over a write that did nothing. --- CHANGELOG.md | 1 + src/DonutCacheModule.php | 45 +++++++-- tests/DonutCommandInterceptorTest.php | 2 +- .../fake-app/src/Resource/Page/Mx/CANone.php | 50 ++++++++++ .../fake-app/src/Resource/Page/Mx/CAPurge.php | 53 ++++++++++ .../src/Resource/Page/Mx/CARefresh.php | 53 ++++++++++ .../fake-app/src/Resource/Page/Mx/CRNone.php | 50 ++++++++++ .../fake-app/src/Resource/Page/Mx/CRPurge.php | 53 ++++++++++ .../src/Resource/Page/Mx/CRRefresh.php | 53 ++++++++++ .../fake-app/src/Resource/Page/Mx/DCNone.php | 50 ++++++++++ .../fake-app/src/Resource/Page/Mx/DCPurge.php | 53 ++++++++++ .../src/Resource/Page/Mx/DCRefresh.php | 53 ++++++++++ .../fake-app/src/Resource/Page/Mx/MRNone.php | 50 ++++++++++ .../fake-app/src/Resource/Page/Mx/MRPurge.php | 53 ++++++++++ .../src/Resource/Page/Mx/MRRefresh.php | 53 ++++++++++ tests/WeavingMatrixTest.php | 96 +++++++++++++++++++ 16 files changed, 759 insertions(+), 9 deletions(-) create mode 100644 tests/Fake/fake-app/src/Resource/Page/Mx/CANone.php create mode 100644 tests/Fake/fake-app/src/Resource/Page/Mx/CAPurge.php create mode 100644 tests/Fake/fake-app/src/Resource/Page/Mx/CARefresh.php create mode 100644 tests/Fake/fake-app/src/Resource/Page/Mx/CRNone.php create mode 100644 tests/Fake/fake-app/src/Resource/Page/Mx/CRPurge.php create mode 100644 tests/Fake/fake-app/src/Resource/Page/Mx/CRRefresh.php create mode 100644 tests/Fake/fake-app/src/Resource/Page/Mx/DCNone.php create mode 100644 tests/Fake/fake-app/src/Resource/Page/Mx/DCPurge.php create mode 100644 tests/Fake/fake-app/src/Resource/Page/Mx/DCRefresh.php create mode 100644 tests/Fake/fake-app/src/Resource/Page/Mx/MRNone.php create mode 100644 tests/Fake/fake-app/src/Resource/Page/Mx/MRPurge.php create mode 100644 tests/Fake/fake-app/src/Resource/Page/Mx/MRRefresh.php create mode 100644 tests/WeavingMatrixTest.php diff --git a/CHANGELOG.md b/CHANGELOG.md index e839f464..c7af04a8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -72,6 +72,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed - Donut caching could not handle a non-200 response, in two ways (#206). The save gate skips 4xx and above, so a `301` or a `204` is stored, but `saveView()` required a validator that `EtagSetter` issues for 200 only: under `#[CacheableResponse]` every such request died on that assertion (a `TypeError` converted to a 400 with `zend.assertions=-1`), and a `204` leaving its body null died earlier still, in `ResourceDonut::create()`. Under `#[DonutCache]` nothing threw and the page answered 200 with the `Location` header attached from the second request on, because neither `DonutRepository::get()` nor `ResourceDonut::refresh()` restored the code the entry was saved with. The validator is now saved only when the response has one, `ResourceDonut` carries the code through the refresh (entries serialized before it leave the response's own code alone), and a null body is composed as an empty one. Behaviour change: a `#[CacheableResponse]` or `#[DonutCache]` page that answers 2xx or 3xx is now served with that status instead of crashing or degrading to 200 - a redirect that was reachable only once now persists until its cache entry is invalidated, so such a page needs the surrogate keys that invalidate it. - `onPost` on a `#[Cacheable]` class ran with no interceptor at all (#212): `CommandInterceptor` was bound to `onPut`/`onPatch`/`onDelete`, and `RefreshInterceptor` only to classes that are not `#[Cacheable]`. A POST - the method a form submits - left the representation it had just made stale being served, and a `#[Refresh]`/`#[Purge]` written on it was dropped with no error and no log event. +- A write on the donut path could be answered from the cache without running: `#[RefreshCache]`, and a method-level `#[CacheableResponse]` on a command method, bound the query-side `DonutCacheInterceptor`, which returns the stored representation and never calls `proceed()`. Once the page was cached, `onPut`/`onPost`/`onDelete` did nothing and answered 200 with the cached page instead of their own response; both now bind `DonutCommandInterceptor`, which runs the write and then purges and refreshes. `DonutCacheModule` also missed `onPost` in its class-level write matcher, the same gap #212 fixed on the value-cache side. `tests/WeavingMatrixTest.php` asserts that a write runs for every declaration shape, and that it announces the change where a declaration names one - a shape whose declaration sits on `onGet` alone still needs `#[Purge]`/`#[Refresh]` on the write, which no matcher can supply. - `DevEtagSetter` and `MobileEtagSetter` set a validator regardless of the response code, where `EtagSetter` has always skipped a non-200. With a stored `301` now reaching them through the donut refresh, an `If-None-Match` match would have let `ConditionalResponse::isModified()` answer `304` in place of the redirect; both now skip a non-200. `CdnCacheControlHeaderSetterInterface` is likewise called for 200 only, so a stored non-200 is no longer handed the Fastly/Akamai default shared-cache lifetime of a year. - A client-chosen `If-None-Match` token containing a PSR-6 reserved character (`{}()/\@:`) reached the ETag pool as a cache key and threw, turning a request header into a 500 that logged like a pool outage. Such a token can never have been issued by this server, so `EntityTags` drops it and the request is answered in full; `*` is likewise dropped (RFC 9110 ยง13.1.2 gives it existence semantics this package does not implement). - An embedded child was materialized twice per parent store: the storage re-invoked the request instead of reusing the execution `setCacheDependency()` and the renderer already paid for, so a `type: "view"` entry could hold a body and a view from two different runs of a non-idempotent child. diff --git a/src/DonutCacheModule.php b/src/DonutCacheModule.php index 54ed8c65..46255185 100644 --- a/src/DonutCacheModule.php +++ b/src/DonutCacheModule.php @@ -8,6 +8,8 @@ use BEAR\RepositoryModule\Annotation\DonutCache; use BEAR\RepositoryModule\Annotation\RefreshCache; use Override; +use Ray\Aop\AbstractMatcher; +use Ray\Aop\MatcherInterface; use Ray\Di\AbstractModule; use Ray\Di\Scope; @@ -68,14 +70,27 @@ private function installAopClassModule(): void $this->bindInterceptor( $this->matcher->annotatedWith(CacheableResponse::class), - $this->matcher->logicalOr( - $this->matcher->startsWith('onPut'), - $this->matcher->logicalOr( - $this->matcher->startsWith('onPatch'), - $this->matcher->startsWith('onDelete'), + self::commandMethods($this->matcher), + [DonutCommandInterceptor::class], + ); + } + + /** + * onPost writes too: a POST that changes state must refresh the donut it invalidates + * + * @see \BEAR\QueryRepository\CacheableModule::installAopModule() same set on the value-cache side + */ + private static function commandMethods(MatcherInterface $matcher): AbstractMatcher + { + return $matcher->logicalOr( + $matcher->startsWith('onPut'), + $matcher->logicalOr( + $matcher->startsWith('onPost'), + $matcher->logicalOr( + $matcher->startsWith('onPatch'), + $matcher->startsWith('onDelete'), ), ), - [DonutCommandInterceptor::class], ); } @@ -83,13 +98,27 @@ private function installAopMethodModule(): void { $this->bindInterceptor( $this->matcher->any(), - $this->matcher->annotatedWith(CacheableResponse::class), + $this->matcher->logicalAnd( + $this->matcher->annotatedWith(CacheableResponse::class), + $this->matcher->startsWith('onGet'), + ), [DonutCacheInterceptor::class], ); + + // A write is not a query: the donut interceptor answers from the store, so binding it to + // a command method makes the write return the cached representation without running. + $this->bindInterceptor( + $this->matcher->any(), + $this->matcher->logicalAnd( + $this->matcher->annotatedWith(CacheableResponse::class), + self::commandMethods($this->matcher), + ), + [DonutCommandInterceptor::class], + ); $this->bindInterceptor( $this->matcher->any(), $this->matcher->annotatedWith(RefreshCache::class), - [DonutCacheInterceptor::class], + [DonutCommandInterceptor::class], ); } } diff --git a/tests/DonutCommandInterceptorTest.php b/tests/DonutCommandInterceptorTest.php index 503b7a8d..6822c5b0 100644 --- a/tests/DonutCommandInterceptorTest.php +++ b/tests/DonutCommandInterceptorTest.php @@ -177,6 +177,6 @@ public function testCacheableResponse(): void assert(is_array($ro->bindings['onDelete'])); assert(isset($ro->bindings['onGet'][0])); assert(isset($ro->bindings['onDelete'][0])); - $this->assertInstanceOf(DonutCacheInterceptor::class, $ro->bindings['onDelete'][0]); + $this->assertInstanceOf(DonutCommandInterceptor::class, $ro->bindings['onDelete'][0]); } } diff --git a/tests/Fake/fake-app/src/Resource/Page/Mx/CANone.php b/tests/Fake/fake-app/src/Resource/Page/Mx/CANone.php new file mode 100644 index 00000000..5929ae8d --- /dev/null +++ b/tests/Fake/fake-app/src/Resource/Page/Mx/CANone.php @@ -0,0 +1,50 @@ +body = ['v' => $id]; + + return $this; + } + + public function onPut(int $id = 0): static + { + self::$ran++; + $this->code = 204; + + return $this; + } + + public function onPost(int $id = 0): static + { + self::$ran++; + $this->code = 204; + + return $this; + } + + public function onDelete(int $id = 0): static + { + self::$ran++; + $this->code = 204; + + return $this; + } + +} diff --git a/tests/Fake/fake-app/src/Resource/Page/Mx/CAPurge.php b/tests/Fake/fake-app/src/Resource/Page/Mx/CAPurge.php new file mode 100644 index 00000000..a931f7fa --- /dev/null +++ b/tests/Fake/fake-app/src/Resource/Page/Mx/CAPurge.php @@ -0,0 +1,53 @@ +body = ['v' => $id]; + + return $this; + } + + #[Purge(uri: 'page://self/mx/CAPurge?id=1')] + public function onPut(int $id = 0): static + { + self::$ran++; + $this->code = 204; + + return $this; + } + + #[Purge(uri: 'page://self/mx/CAPurge?id=1')] + public function onPost(int $id = 0): static + { + self::$ran++; + $this->code = 204; + + return $this; + } + + #[Purge(uri: 'page://self/mx/CAPurge?id=1')] + public function onDelete(int $id = 0): static + { + self::$ran++; + $this->code = 204; + + return $this; + } + +} diff --git a/tests/Fake/fake-app/src/Resource/Page/Mx/CARefresh.php b/tests/Fake/fake-app/src/Resource/Page/Mx/CARefresh.php new file mode 100644 index 00000000..6473ba16 --- /dev/null +++ b/tests/Fake/fake-app/src/Resource/Page/Mx/CARefresh.php @@ -0,0 +1,53 @@ +body = ['v' => $id]; + + return $this; + } + + #[RefreshCache] + public function onPut(int $id = 0): static + { + self::$ran++; + $this->code = 204; + + return $this; + } + + #[RefreshCache] + public function onPost(int $id = 0): static + { + self::$ran++; + $this->code = 204; + + return $this; + } + + #[RefreshCache] + public function onDelete(int $id = 0): static + { + self::$ran++; + $this->code = 204; + + return $this; + } + +} diff --git a/tests/Fake/fake-app/src/Resource/Page/Mx/CRNone.php b/tests/Fake/fake-app/src/Resource/Page/Mx/CRNone.php new file mode 100644 index 00000000..c81a71a8 --- /dev/null +++ b/tests/Fake/fake-app/src/Resource/Page/Mx/CRNone.php @@ -0,0 +1,50 @@ +body = ['v' => $id]; + + return $this; + } + + public function onPut(int $id = 0): static + { + self::$ran++; + $this->code = 204; + + return $this; + } + + public function onPost(int $id = 0): static + { + self::$ran++; + $this->code = 204; + + return $this; + } + + public function onDelete(int $id = 0): static + { + self::$ran++; + $this->code = 204; + + return $this; + } + +} diff --git a/tests/Fake/fake-app/src/Resource/Page/Mx/CRPurge.php b/tests/Fake/fake-app/src/Resource/Page/Mx/CRPurge.php new file mode 100644 index 00000000..eed2a49c --- /dev/null +++ b/tests/Fake/fake-app/src/Resource/Page/Mx/CRPurge.php @@ -0,0 +1,53 @@ +body = ['v' => $id]; + + return $this; + } + + #[Purge(uri: 'page://self/mx/CRPurge?id=1')] + public function onPut(int $id = 0): static + { + self::$ran++; + $this->code = 204; + + return $this; + } + + #[Purge(uri: 'page://self/mx/CRPurge?id=1')] + public function onPost(int $id = 0): static + { + self::$ran++; + $this->code = 204; + + return $this; + } + + #[Purge(uri: 'page://self/mx/CRPurge?id=1')] + public function onDelete(int $id = 0): static + { + self::$ran++; + $this->code = 204; + + return $this; + } + +} diff --git a/tests/Fake/fake-app/src/Resource/Page/Mx/CRRefresh.php b/tests/Fake/fake-app/src/Resource/Page/Mx/CRRefresh.php new file mode 100644 index 00000000..973c93a8 --- /dev/null +++ b/tests/Fake/fake-app/src/Resource/Page/Mx/CRRefresh.php @@ -0,0 +1,53 @@ +body = ['v' => $id]; + + return $this; + } + + #[RefreshCache] + public function onPut(int $id = 0): static + { + self::$ran++; + $this->code = 204; + + return $this; + } + + #[RefreshCache] + public function onPost(int $id = 0): static + { + self::$ran++; + $this->code = 204; + + return $this; + } + + #[RefreshCache] + public function onDelete(int $id = 0): static + { + self::$ran++; + $this->code = 204; + + return $this; + } + +} diff --git a/tests/Fake/fake-app/src/Resource/Page/Mx/DCNone.php b/tests/Fake/fake-app/src/Resource/Page/Mx/DCNone.php new file mode 100644 index 00000000..f6e86d3a --- /dev/null +++ b/tests/Fake/fake-app/src/Resource/Page/Mx/DCNone.php @@ -0,0 +1,50 @@ +body = ['v' => $id]; + + return $this; + } + + public function onPut(int $id = 0): static + { + self::$ran++; + $this->code = 204; + + return $this; + } + + public function onPost(int $id = 0): static + { + self::$ran++; + $this->code = 204; + + return $this; + } + + public function onDelete(int $id = 0): static + { + self::$ran++; + $this->code = 204; + + return $this; + } + +} diff --git a/tests/Fake/fake-app/src/Resource/Page/Mx/DCPurge.php b/tests/Fake/fake-app/src/Resource/Page/Mx/DCPurge.php new file mode 100644 index 00000000..7f7137a0 --- /dev/null +++ b/tests/Fake/fake-app/src/Resource/Page/Mx/DCPurge.php @@ -0,0 +1,53 @@ +body = ['v' => $id]; + + return $this; + } + + #[Purge(uri: 'page://self/mx/DCPurge?id=1')] + public function onPut(int $id = 0): static + { + self::$ran++; + $this->code = 204; + + return $this; + } + + #[Purge(uri: 'page://self/mx/DCPurge?id=1')] + public function onPost(int $id = 0): static + { + self::$ran++; + $this->code = 204; + + return $this; + } + + #[Purge(uri: 'page://self/mx/DCPurge?id=1')] + public function onDelete(int $id = 0): static + { + self::$ran++; + $this->code = 204; + + return $this; + } + +} diff --git a/tests/Fake/fake-app/src/Resource/Page/Mx/DCRefresh.php b/tests/Fake/fake-app/src/Resource/Page/Mx/DCRefresh.php new file mode 100644 index 00000000..1d3638ba --- /dev/null +++ b/tests/Fake/fake-app/src/Resource/Page/Mx/DCRefresh.php @@ -0,0 +1,53 @@ +body = ['v' => $id]; + + return $this; + } + + #[RefreshCache] + public function onPut(int $id = 0): static + { + self::$ran++; + $this->code = 204; + + return $this; + } + + #[RefreshCache] + public function onPost(int $id = 0): static + { + self::$ran++; + $this->code = 204; + + return $this; + } + + #[RefreshCache] + public function onDelete(int $id = 0): static + { + self::$ran++; + $this->code = 204; + + return $this; + } + +} diff --git a/tests/Fake/fake-app/src/Resource/Page/Mx/MRNone.php b/tests/Fake/fake-app/src/Resource/Page/Mx/MRNone.php new file mode 100644 index 00000000..86086e35 --- /dev/null +++ b/tests/Fake/fake-app/src/Resource/Page/Mx/MRNone.php @@ -0,0 +1,50 @@ +body = ['v' => $id]; + + return $this; + } + + public function onPut(int $id = 0): static + { + self::$ran++; + $this->code = 204; + + return $this; + } + + public function onPost(int $id = 0): static + { + self::$ran++; + $this->code = 204; + + return $this; + } + + public function onDelete(int $id = 0): static + { + self::$ran++; + $this->code = 204; + + return $this; + } + +} diff --git a/tests/Fake/fake-app/src/Resource/Page/Mx/MRPurge.php b/tests/Fake/fake-app/src/Resource/Page/Mx/MRPurge.php new file mode 100644 index 00000000..b277569f --- /dev/null +++ b/tests/Fake/fake-app/src/Resource/Page/Mx/MRPurge.php @@ -0,0 +1,53 @@ +body = ['v' => $id]; + + return $this; + } + + #[Purge(uri: 'page://self/mx/MRPurge?id=1')] + public function onPut(int $id = 0): static + { + self::$ran++; + $this->code = 204; + + return $this; + } + + #[Purge(uri: 'page://self/mx/MRPurge?id=1')] + public function onPost(int $id = 0): static + { + self::$ran++; + $this->code = 204; + + return $this; + } + + #[Purge(uri: 'page://self/mx/MRPurge?id=1')] + public function onDelete(int $id = 0): static + { + self::$ran++; + $this->code = 204; + + return $this; + } + +} diff --git a/tests/Fake/fake-app/src/Resource/Page/Mx/MRRefresh.php b/tests/Fake/fake-app/src/Resource/Page/Mx/MRRefresh.php new file mode 100644 index 00000000..ece0f3ef --- /dev/null +++ b/tests/Fake/fake-app/src/Resource/Page/Mx/MRRefresh.php @@ -0,0 +1,53 @@ +body = ['v' => $id]; + + return $this; + } + + #[RefreshCache] + public function onPut(int $id = 0): static + { + self::$ran++; + $this->code = 204; + + return $this; + } + + #[RefreshCache] + public function onPost(int $id = 0): static + { + self::$ran++; + $this->code = 204; + + return $this; + } + + #[RefreshCache] + public function onDelete(int $id = 0): static + { + self::$ran++; + $this->code = 204; + + return $this; + } + +} diff --git a/tests/WeavingMatrixTest.php b/tests/WeavingMatrixTest.php new file mode 100644 index 00000000..2e56774e --- /dev/null +++ b/tests/WeavingMatrixTest.php @@ -0,0 +1,96 @@ + + */ + public static function writeProvider(): array + { + $announces = [ + 'CANone' => true, + 'CAPurge' => true, + 'CARefresh' => true, + 'CRNone' => true, + 'CRPurge' => true, + 'CRRefresh' => true, + 'MRNone' => false, + 'MRPurge' => true, + 'MRRefresh' => true, + 'DCNone' => false, + 'DCPurge' => true, + 'DCRefresh' => true, + ]; + $cases = []; + foreach ($announces as $shape => $announce) { + foreach (['onPut', 'onPost', 'onDelete'] as $method) { + $cases[] = [$shape, $method, $announce]; + } + } + + return $cases; + } + + /** + * A write is never answered from the cache, and it announces what it changed + * + * @param bool $announces whether the shape carries a declaration that must produce a command scope + */ + #[DataProvider('writeProvider')] + public function testWriteRunsAndAnnounces(string $shape, string $method, bool $announces): void + { + $class = 'FakeVendor\HelloWorld\Resource\Page\Mx\\' . $shape; + $class::$ran = 0; + $injector = new Injector( + new FakeEtagPoolModule(ModuleFactory::getInstance('FakeVendor\HelloWorld')), + __DIR__ . '/tmp', + ); + $resource = $injector->getInstance(ResourceInterface::class); + $uri = 'page://self/mx/' . $shape . '?id=1'; + $resource->get($uri); // warm the cache + $ro = $resource->{strtolower(substr($method, 2))}($uri); // then write + assert($ro instanceof ResourceObject); + + $this->assertSame(1, $class::$ran, $shape . '::' . $method . ' was answered from the cache instead of running'); + $this->assertSame(204, $ro->code, $shape . '::' . $method . ' returned the cached representation, not its own'); + + if (! $announces) { + return; + } + + $log = (string) json_encode($injector->getInstance(SemanticLoggerInterface::class, CacheLog::class)->flush()); + $this->assertGreaterThan(0, substr_count($log, '"command"'), $shape . '::' . $method . ' left no command scope: nothing was told that the state changed'); + } +} From a9712fde86a4e64367436d5ca6f314f2ac3fd970 Mon Sep 17 00:00:00 2001 From: Akihito Koriyama Date: Mon, 7 Sep 2026 21:24:04 +0900 Subject: [PATCH 2/4] Exclude classes the class-level binding already covers CodeRabbit caught it on #215: a write on a #[CacheableResponse] class that also carries #[RefreshCache] matched both new bindings, and Ray.Aop merges overlapping bindings without deduplicating. Measured before: CRRefresh::onPost carried DonutCommandInterceptor twice and answered with command=2 purge=2. After: one of each. WeavingMatrixTest now asserts the woven chain holds no duplicated interceptor class, so the next overlapping matcher fails here rather than doubling a CDN purge in production. The remaining two-interceptor case is not a duplicate: #[Purge] on a #[CacheableResponse] class weaves RefreshInterceptor (purges the URI the attribute names) alongside DonutCommandInterceptor (refreshes the resource itself) - two jobs that coincide only when the attribute points at its own URI, as the fixture does. --- src/DonutCacheModule.php | 9 ++++---- tests/WeavingMatrixTest.php | 46 +++++++++++++++++++++++++++++++------ 2 files changed, 44 insertions(+), 11 deletions(-) diff --git a/src/DonutCacheModule.php b/src/DonutCacheModule.php index 46255185..f32d4aed 100644 --- a/src/DonutCacheModule.php +++ b/src/DonutCacheModule.php @@ -96,6 +96,9 @@ private static function commandMethods(MatcherInterface $matcher): AbstractMatch private function installAopMethodModule(): void { + // Ray.Aop merges overlapping bindings without deduplicating, so a class the class-level + // binding already covers is excluded here rather than carrying the interceptor twice. + $notCacheableResponse = $this->matcher->logicalNot($this->matcher->annotatedWith(CacheableResponse::class)); $this->bindInterceptor( $this->matcher->any(), $this->matcher->logicalAnd( @@ -105,10 +108,8 @@ private function installAopMethodModule(): void [DonutCacheInterceptor::class], ); - // A write is not a query: the donut interceptor answers from the store, so binding it to - // a command method makes the write return the cached representation without running. $this->bindInterceptor( - $this->matcher->any(), + $notCacheableResponse, $this->matcher->logicalAnd( $this->matcher->annotatedWith(CacheableResponse::class), self::commandMethods($this->matcher), @@ -116,7 +117,7 @@ private function installAopMethodModule(): void [DonutCommandInterceptor::class], ); $this->bindInterceptor( - $this->matcher->any(), + $notCacheableResponse, $this->matcher->annotatedWith(RefreshCache::class), [DonutCommandInterceptor::class], ); diff --git a/tests/WeavingMatrixTest.php b/tests/WeavingMatrixTest.php index 2e56774e..b85bc73c 100644 --- a/tests/WeavingMatrixTest.php +++ b/tests/WeavingMatrixTest.php @@ -12,8 +12,15 @@ use PHPUnit\Framework\TestCase; use Ray\Di\Injector; +use function array_unique; +use function array_values; use function assert; +use function gettype; +use function implode; +use function is_array; +use function is_object; use function json_encode; +use function property_exists; use function strtolower; use function substr; use function substr_count; @@ -63,11 +70,7 @@ public static function writeProvider(): array return $cases; } - /** - * A write is never answered from the cache, and it announces what it changed - * - * @param bool $announces whether the shape carries a declaration that must produce a command scope - */ + /** A write is never answered from the cache, and it announces what it changed */ #[DataProvider('writeProvider')] public function testWriteRunsAndAnnounces(string $shape, string $method, bool $announces): void { @@ -79,13 +82,16 @@ public function testWriteRunsAndAnnounces(string $shape, string $method, bool $a ); $resource = $injector->getInstance(ResourceInterface::class); $uri = 'page://self/mx/' . $shape . '?id=1'; - $resource->get($uri); // warm the cache - $ro = $resource->{strtolower(substr($method, 2))}($uri); // then write + $resource->get($uri); + $ro = $resource->{strtolower(substr($method, 2))}($uri); assert($ro instanceof ResourceObject); $this->assertSame(1, $class::$ran, $shape . '::' . $method . ' was answered from the cache instead of running'); $this->assertSame(204, $ro->code, $shape . '::' . $method . ' returned the cached representation, not its own'); + $woven = self::wovenOn($ro, $method); + $this->assertSame(array_values(array_unique($woven)), $woven, $shape . '::' . $method . ' carries a duplicated interceptor: ' . implode(', ', $woven)); + if (! $announces) { return; } @@ -93,4 +99,30 @@ public function testWriteRunsAndAnnounces(string $shape, string $method, bool $a $log = (string) json_encode($injector->getInstance(SemanticLoggerInterface::class, CacheLog::class)->flush()); $this->assertGreaterThan(0, substr_count($log, '"command"'), $shape . '::' . $method . ' left no command scope: nothing was told that the state changed'); } + + /** + * Interceptor class names woven onto one method, in chain order + * + * @return list + */ + private static function wovenOn(ResourceObject $ro, string $method): array + { + if (! property_exists($ro, 'bindings') || ! is_array($ro->bindings)) { + return []; + } + + /** @var mixed $onMethod */ + $onMethod = $ro->bindings[$method] ?? []; + if (! is_array($onMethod)) { + return []; + } + + $names = []; + /** @var mixed $interceptor */ + foreach ($onMethod as $interceptor) { + $names[] = is_object($interceptor) ? $interceptor::class : (string) gettype($interceptor); + } + + return $names; + } } From 1dd0e298f8172ee23c6d8daf111b24c826f2c3d1 Mon Sep 17 00:00:00 2001 From: Akihito Koriyama Date: Mon, 7 Sep 2026 21:31:48 +0900 Subject: [PATCH 3/4] Assert an exact command-scope count, and close the remaining overlaps The duplicate check on interceptor classes could not see two different interceptors doing one job twice, so the matrix now asserts the exact number of command scopes a write opens. Two overlaps that count made visible: - a #[Cacheable] class with #[RefreshCache] on a write wove DonutCommandInterceptor beside CommandInterceptor: both purge the resource and regenerate it, measured as command=2 purge=4 - a class declaring #[CacheableResponse] and repeating it on onGet wove DonutCacheableResponseInterceptor beside DonutCacheInterceptor, and a #[DonutCache] class repeating #[CacheableResponse] on onGet wove the same donut interceptor twice The method-level bindings now skip a class whose own declaration governs the method. The read set excludes #[DonutCache] as well as the write set does not: #[DonutCache] binds onGet only, so excluding it from the write bindings leaves a #[RefreshCache] write with no interceptor at all - the defect this branch exists to fix, which one measurement caught. CRPurge keeps two scopes on purpose: RefreshInterceptor purges the URI the attribute names, DonutCommandInterceptor refreshes the resource. Fixtures CRBoth and DCMR cover the repeated declaration; DC* now declare #[DonutCache] on the class, which is the only form any binding reads - a method-level #[DonutCache] is accepted by the attribute and implemented by nothing. --- src/DonutCacheModule.php | 29 +++++++-- .../fake-app/src/Resource/Page/Mx/CRBoth.php | 46 ++++++++++++++ .../fake-app/src/Resource/Page/Mx/DCMR.php | 47 +++++++++++++++ .../fake-app/src/Resource/Page/Mx/DCNone.php | 2 +- .../fake-app/src/Resource/Page/Mx/DCPurge.php | 2 +- .../src/Resource/Page/Mx/DCRefresh.php | 2 +- tests/WeavingMatrixTest.php | 60 +++++++++---------- 7 files changed, 149 insertions(+), 39 deletions(-) create mode 100644 tests/Fake/fake-app/src/Resource/Page/Mx/CRBoth.php create mode 100644 tests/Fake/fake-app/src/Resource/Page/Mx/DCMR.php diff --git a/src/DonutCacheModule.php b/src/DonutCacheModule.php index f32d4aed..4afb072f 100644 --- a/src/DonutCacheModule.php +++ b/src/DonutCacheModule.php @@ -4,6 +4,7 @@ namespace BEAR\QueryRepository; +use BEAR\RepositoryModule\Annotation\Cacheable; use BEAR\RepositoryModule\Annotation\CacheableResponse; use BEAR\RepositoryModule\Annotation\DonutCache; use BEAR\RepositoryModule\Annotation\RefreshCache; @@ -96,11 +97,27 @@ private static function commandMethods(MatcherInterface $matcher): AbstractMatch private function installAopMethodModule(): void { - // Ray.Aop merges overlapping bindings without deduplicating, so a class the class-level - // binding already covers is excluded here rather than carrying the interceptor twice. - $notCacheableResponse = $this->matcher->logicalNot($this->matcher->annotatedWith(CacheableResponse::class)); + // Ray.Aop merges overlapping bindings without deduplicating, so a class whose own + // declaration already governs the method is excluded. The two sets differ because + // #[DonutCache] governs onGet only: excluding it from the write bindings too would + // leave a #[RefreshCache] write on such a class with no interceptor at all. + $readNotDeclared = $this->matcher->logicalNot( + $this->matcher->logicalOr( + $this->matcher->annotatedWith(CacheableResponse::class), + $this->matcher->logicalOr( + $this->matcher->annotatedWith(Cacheable::class), + $this->matcher->annotatedWith(DonutCache::class), + ), + ), + ); + $writeNotDeclared = $this->matcher->logicalNot( + $this->matcher->logicalOr( + $this->matcher->annotatedWith(CacheableResponse::class), + $this->matcher->annotatedWith(Cacheable::class), + ), + ); $this->bindInterceptor( - $this->matcher->any(), + $readNotDeclared, $this->matcher->logicalAnd( $this->matcher->annotatedWith(CacheableResponse::class), $this->matcher->startsWith('onGet'), @@ -109,7 +126,7 @@ private function installAopMethodModule(): void ); $this->bindInterceptor( - $notCacheableResponse, + $writeNotDeclared, $this->matcher->logicalAnd( $this->matcher->annotatedWith(CacheableResponse::class), self::commandMethods($this->matcher), @@ -117,7 +134,7 @@ private function installAopMethodModule(): void [DonutCommandInterceptor::class], ); $this->bindInterceptor( - $notCacheableResponse, + $writeNotDeclared, $this->matcher->annotatedWith(RefreshCache::class), [DonutCommandInterceptor::class], ); diff --git a/tests/Fake/fake-app/src/Resource/Page/Mx/CRBoth.php b/tests/Fake/fake-app/src/Resource/Page/Mx/CRBoth.php new file mode 100644 index 00000000..454a6990 --- /dev/null +++ b/tests/Fake/fake-app/src/Resource/Page/Mx/CRBoth.php @@ -0,0 +1,46 @@ +body = ['v' => $id]; + + return $this; + } + + public function onPut(int $id = 0): static + { + self::$ran++; + $this->code = 204; + + return $this; + } + + public function onPost(int $id = 0): static + { + self::$ran++; + $this->code = 204; + + return $this; + } + + public function onDelete(int $id = 0): static + { + self::$ran++; + $this->code = 204; + + return $this; + } +} diff --git a/tests/Fake/fake-app/src/Resource/Page/Mx/DCMR.php b/tests/Fake/fake-app/src/Resource/Page/Mx/DCMR.php new file mode 100644 index 00000000..3ddad6a8 --- /dev/null +++ b/tests/Fake/fake-app/src/Resource/Page/Mx/DCMR.php @@ -0,0 +1,47 @@ +body = ['v' => $id]; + + return $this; + } + + public function onPut(int $id = 0): static + { + self::$ran++; + $this->code = 204; + + return $this; + } + + public function onPost(int $id = 0): static + { + self::$ran++; + $this->code = 204; + + return $this; + } + + public function onDelete(int $id = 0): static + { + self::$ran++; + $this->code = 204; + + return $this; + } +} diff --git a/tests/Fake/fake-app/src/Resource/Page/Mx/DCNone.php b/tests/Fake/fake-app/src/Resource/Page/Mx/DCNone.php index f6e86d3a..3636ee4f 100644 --- a/tests/Fake/fake-app/src/Resource/Page/Mx/DCNone.php +++ b/tests/Fake/fake-app/src/Resource/Page/Mx/DCNone.php @@ -10,12 +10,12 @@ use BEAR\Resource\ResourceObject; /** Weaving matrix fixture: DCNone */ +#[DonutCache] class DCNone extends ResourceObject { /** Number of times a write body really ran */ public static int $ran = 0; - #[DonutCache] public function onGet(int $id = 0): static { $this->body = ['v' => $id]; diff --git a/tests/Fake/fake-app/src/Resource/Page/Mx/DCPurge.php b/tests/Fake/fake-app/src/Resource/Page/Mx/DCPurge.php index 7f7137a0..b6f5f608 100644 --- a/tests/Fake/fake-app/src/Resource/Page/Mx/DCPurge.php +++ b/tests/Fake/fake-app/src/Resource/Page/Mx/DCPurge.php @@ -10,12 +10,12 @@ use BEAR\Resource\ResourceObject; /** Weaving matrix fixture: DCPurge */ +#[DonutCache] class DCPurge extends ResourceObject { /** Number of times a write body really ran */ public static int $ran = 0; - #[DonutCache] public function onGet(int $id = 0): static { $this->body = ['v' => $id]; diff --git a/tests/Fake/fake-app/src/Resource/Page/Mx/DCRefresh.php b/tests/Fake/fake-app/src/Resource/Page/Mx/DCRefresh.php index 1d3638ba..bbca3064 100644 --- a/tests/Fake/fake-app/src/Resource/Page/Mx/DCRefresh.php +++ b/tests/Fake/fake-app/src/Resource/Page/Mx/DCRefresh.php @@ -10,12 +10,12 @@ use BEAR\Resource\ResourceObject; /** Weaving matrix fixture: DCRefresh */ +#[DonutCache] class DCRefresh extends ResourceObject { /** Number of times a write body really ran */ public static int $ran = 0; - #[DonutCache] public function onGet(int $id = 0): static { $this->body = ['v' => $id]; diff --git a/tests/WeavingMatrixTest.php b/tests/WeavingMatrixTest.php index b85bc73c..42895608 100644 --- a/tests/WeavingMatrixTest.php +++ b/tests/WeavingMatrixTest.php @@ -29,50 +29,54 @@ * Which interceptor is woven onto which method, for every cache declaration shape * * A declaration that does not weave is silent: the resource answers correctly and the suite stays - * green. The pairs below are the ones an application can actually write, and each is asserted on - * two observable facts - the write body ran, and the write announced an invalidation. + * green. Each shape is asserted on what the log and the response show - the write body ran, it + * answered with its own code, no interceptor was woven twice, and the change was announced + * exactly as many times as there are interceptors with something to announce. */ class WeavingMatrixTest extends TestCase { /** - * Every shape an application can write, and whether its write must announce the change + * Every shape an application can write, and how many command scopes its write must open * - * `MRNone` and `DCNone` carry the cache declaration on `onGet` only, so nothing names their - * write: the binding matches the attribute *on* the method, and no matcher can express "a - * class holding this attribute somewhere". Their writes run, and invalidating what they - * changed needs `#[Purge]` or `#[Refresh]` written on the write. + * 0: the cache declaration sits on `onGet` alone, so nothing names the write - invalidating + * what it changed needs `#[Purge]`/`#[Refresh]` on the write itself, which no matcher can + * supply. 2: `CRPurge` weaves two interceptors with different jobs - `RefreshInterceptor` + * purges the URI the attribute names, `DonutCommandInterceptor` refreshes the resource - so + * two scopes are correct there and a duplicate anywhere else is not. * - * @return list + * @return list */ public static function writeProvider(): array { - $announces = [ - 'CANone' => true, - 'CAPurge' => true, - 'CARefresh' => true, - 'CRNone' => true, - 'CRPurge' => true, - 'CRRefresh' => true, - 'MRNone' => false, - 'MRPurge' => true, - 'MRRefresh' => true, - 'DCNone' => false, - 'DCPurge' => true, - 'DCRefresh' => true, + $commandScopes = [ + 'CANone' => 1, + 'CAPurge' => 1, + 'CARefresh' => 1, + 'CRNone' => 1, + 'CRPurge' => 2, + 'CRRefresh' => 1, + 'CRBoth' => 1, + 'MRNone' => 0, + 'MRPurge' => 1, + 'MRRefresh' => 1, + 'DCNone' => 0, + 'DCPurge' => 1, + 'DCRefresh' => 1, + 'DCMR' => 0, ]; $cases = []; - foreach ($announces as $shape => $announce) { + foreach ($commandScopes as $shape => $expected) { foreach (['onPut', 'onPost', 'onDelete'] as $method) { - $cases[] = [$shape, $method, $announce]; + $cases[] = [$shape, $method, $expected]; } } return $cases; } - /** A write is never answered from the cache, and it announces what it changed */ + /** A write is never answered from the cache, and it announces its change exactly once per announcer */ #[DataProvider('writeProvider')] - public function testWriteRunsAndAnnounces(string $shape, string $method, bool $announces): void + public function testWriteRunsAndAnnounces(string $shape, string $method, int $commandScopes): void { $class = 'FakeVendor\HelloWorld\Resource\Page\Mx\\' . $shape; $class::$ran = 0; @@ -92,12 +96,8 @@ public function testWriteRunsAndAnnounces(string $shape, string $method, bool $a $woven = self::wovenOn($ro, $method); $this->assertSame(array_values(array_unique($woven)), $woven, $shape . '::' . $method . ' carries a duplicated interceptor: ' . implode(', ', $woven)); - if (! $announces) { - return; - } - $log = (string) json_encode($injector->getInstance(SemanticLoggerInterface::class, CacheLog::class)->flush()); - $this->assertGreaterThan(0, substr_count($log, '"command"'), $shape . '::' . $method . ' left no command scope: nothing was told that the state changed'); + $this->assertSame($commandScopes, substr_count($log, '"command"'), $shape . '::' . $method . ' opened ' . substr_count($log, '"command"') . ' command scopes, expected ' . $commandScopes . ' - woven: ' . (implode(', ', $woven) ?: 'nothing')); } /** From 978c69ecab3a731459a53ca3c0d3fb526ddc4a0d Mon Sep 17 00:00:00 2001 From: Akihito Koriyama Date: Mon, 7 Sep 2026 21:35:20 +0900 Subject: [PATCH 4/4] Pin the woven chain, not a count The defect this branch fixes was an ordering fact: the query interceptor sat first and answered the write from the store. A count cannot see order, and counting purges would break on any change to how many URIs a command touches. The provider now holds the exact chain per shape, by ::class, and the scope count beside it - the chain is the weaving contract, the count is what the chain did at runtime. --- tests/WeavingMatrixTest.php | 66 ++++++++++++++++++++----------------- 1 file changed, 36 insertions(+), 30 deletions(-) diff --git a/tests/WeavingMatrixTest.php b/tests/WeavingMatrixTest.php index 42895608..0e30e115 100644 --- a/tests/WeavingMatrixTest.php +++ b/tests/WeavingMatrixTest.php @@ -12,8 +12,6 @@ use PHPUnit\Framework\TestCase; use Ray\Di\Injector; -use function array_unique; -use function array_values; use function assert; use function gettype; use function implode; @@ -36,47 +34,55 @@ class WeavingMatrixTest extends TestCase { /** - * Every shape an application can write, and how many command scopes its write must open + * Every shape an application can write: the chain its write carries, and the scopes it opens * - * 0: the cache declaration sits on `onGet` alone, so nothing names the write - invalidating - * what it changed needs `#[Purge]`/`#[Refresh]` on the write itself, which no matcher can - * supply. 2: `CRPurge` weaves two interceptors with different jobs - `RefreshInterceptor` - * purges the URI the attribute names, `DonutCommandInterceptor` refreshes the resource - so - * two scopes are correct there and a duplicate anywhere else is not. + * The chain is the contract, order included - the defect this test exists for was a query + * interceptor sitting first, answering the write from the store. An empty chain means the + * declaration sits on `onGet` alone, so nothing names the write: invalidating what it changed + * needs `#[Purge]`/`#[Refresh]` on the write itself, which no matcher can supply. + * `CRPurge` carries two interceptors with different jobs - `RefreshInterceptor` purges the URI + * the attribute names, `DonutCommandInterceptor` refreshes the resource - so two scopes are + * correct there and nowhere else. `CARefresh` shows `#[RefreshCache]` adding nothing to a + * `#[Cacheable]` class: `CommandInterceptor` already purges and regenerates, and weaving the + * donut command interceptor beside it did the same work twice. * - * @return list + * @return list, 3: int}> */ public static function writeProvider(): array { - $commandScopes = [ - 'CANone' => 1, - 'CAPurge' => 1, - 'CARefresh' => 1, - 'CRNone' => 1, - 'CRPurge' => 2, - 'CRRefresh' => 1, - 'CRBoth' => 1, - 'MRNone' => 0, - 'MRPurge' => 1, - 'MRRefresh' => 1, - 'DCNone' => 0, - 'DCPurge' => 1, - 'DCRefresh' => 1, - 'DCMR' => 0, + $expected = [ + 'CANone' => [[CommandInterceptor::class], 1], + 'CAPurge' => [[CommandInterceptor::class], 1], + 'CARefresh' => [[CommandInterceptor::class], 1], + 'CRNone' => [[DonutCommandInterceptor::class], 1], + 'CRPurge' => [[RefreshInterceptor::class, DonutCommandInterceptor::class], 2], + 'CRRefresh' => [[DonutCommandInterceptor::class], 1], + 'CRBoth' => [[DonutCommandInterceptor::class], 1], + 'MRNone' => [[], 0], + 'MRPurge' => [[RefreshInterceptor::class], 1], + 'MRRefresh' => [[DonutCommandInterceptor::class], 1], + 'DCNone' => [[], 0], + 'DCPurge' => [[RefreshInterceptor::class], 1], + 'DCRefresh' => [[DonutCommandInterceptor::class], 1], + 'DCMR' => [[], 0], ]; $cases = []; - foreach ($commandScopes as $shape => $expected) { + foreach ($expected as $shape => [$chain, $scopes]) { foreach (['onPut', 'onPost', 'onDelete'] as $method) { - $cases[] = [$shape, $method, $expected]; + $cases[] = [$shape, $method, $chain, $scopes]; } } return $cases; } - /** A write is never answered from the cache, and it announces its change exactly once per announcer */ + /** + * A write runs, answers with its own code, and announces its change once per announcer + * + * @param list $chain interceptor short class names, in the order they are woven + */ #[DataProvider('writeProvider')] - public function testWriteRunsAndAnnounces(string $shape, string $method, int $commandScopes): void + public function testWriteRunsAndAnnounces(string $shape, string $method, array $chain, int $commandScopes): void { $class = 'FakeVendor\HelloWorld\Resource\Page\Mx\\' . $shape; $class::$ran = 0; @@ -94,10 +100,10 @@ public function testWriteRunsAndAnnounces(string $shape, string $method, int $co $this->assertSame(204, $ro->code, $shape . '::' . $method . ' returned the cached representation, not its own'); $woven = self::wovenOn($ro, $method); - $this->assertSame(array_values(array_unique($woven)), $woven, $shape . '::' . $method . ' carries a duplicated interceptor: ' . implode(', ', $woven)); + $this->assertSame($chain, $woven, $shape . '::' . $method . ' carries ' . (implode(', ', $woven) ?: 'nothing')); $log = (string) json_encode($injector->getInstance(SemanticLoggerInterface::class, CacheLog::class)->flush()); - $this->assertSame($commandScopes, substr_count($log, '"command"'), $shape . '::' . $method . ' opened ' . substr_count($log, '"command"') . ' command scopes, expected ' . $commandScopes . ' - woven: ' . (implode(', ', $woven) ?: 'nothing')); + $this->assertSame($commandScopes, substr_count($log, '"command"'), $shape . '::' . $method . ' opened ' . substr_count($log, '"command"') . ' command scopes, expected ' . $commandScopes); } /**