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..4afb072f 100644 --- a/src/DonutCacheModule.php +++ b/src/DonutCacheModule.php @@ -4,10 +4,13 @@ namespace BEAR\QueryRepository; +use BEAR\RepositoryModule\Annotation\Cacheable; use BEAR\RepositoryModule\Annotation\CacheableResponse; 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,28 +71,72 @@ 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], ); } private function installAopMethodModule(): void { + // 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(), - $this->matcher->annotatedWith(CacheableResponse::class), + $readNotDeclared, + $this->matcher->logicalAnd( + $this->matcher->annotatedWith(CacheableResponse::class), + $this->matcher->startsWith('onGet'), + ), [DonutCacheInterceptor::class], ); + $this->bindInterceptor( - $this->matcher->any(), + $writeNotDeclared, + $this->matcher->logicalAnd( + $this->matcher->annotatedWith(CacheableResponse::class), + self::commandMethods($this->matcher), + ), + [DonutCommandInterceptor::class], + ); + $this->bindInterceptor( + $writeNotDeclared, $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/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/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/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 new file mode 100644 index 00000000..3636ee4f --- /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..b6f5f608 --- /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..bbca3064 --- /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..0e30e115 --- /dev/null +++ b/tests/WeavingMatrixTest.php @@ -0,0 +1,134 @@ +, 3: int}> + */ + public static function writeProvider(): array + { + $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 ($expected as $shape => [$chain, $scopes]) { + foreach (['onPut', 'onPost', 'onDelete'] as $method) { + $cases[] = [$shape, $method, $chain, $scopes]; + } + } + + return $cases; + } + + /** + * 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, array $chain, int $commandScopes): 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); + $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($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); + } + + /** + * 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; + } +}