diff --git a/CHANGELOG.md b/CHANGELOG.md index ddd596f3..fe6417a1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -74,6 +74,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. +- `onPost` on a `#[Cacheable]` class whose parameters did not cover `onGet`'s required ones threw `UnmatchedQuery`, uncaught (#219, a regression from #214 above): `RefreshSameCommand`'s automatic same-URI refresh runs `MatchQuery` against the write's own URI query, and a create posting to a collection URI routinely lacks a parameter `onGet` needs. The write itself already succeeded; the refresh had nothing to target. The same gap existed on the donut path: `DonutCommandInterceptor::refreshDonutAndState()` called `MatchQuery` with no catch either, so a `#[CacheableResponse]`/`#[DonutCache]` collection `onPost` with the same shape 500ed too. Both interceptors now catch `UnmatchedQuery` for `onPost` specifically and record it as a `cache_error{operation: write}` instead; `RefreshSameCommand` returning rather than throwing also lets `CommandsProvider`'s next command run, so an explicit `#[Refresh]`/`#[Purge]` on the same method still fires. `onPut`/`onPatch`/`onDelete` keep throwing on both interceptors: those act on an entity `onGet` already addresses, so a missing required parameter there is a real signature mismatch (`BehaviorTest::testUnMatchQuery`). - 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). diff --git a/src/CommandInterceptor.php b/src/CommandInterceptor.php index 4b33096a..5ee51c9b 100644 --- a/src/CommandInterceptor.php +++ b/src/CommandInterceptor.php @@ -19,7 +19,7 @@ /** * Interceptor for cache invalidation on CQRS commands with #[Purge] or #[Refresh] * - * Automatically bound to all command methods (onPut/onPatch/onDelete) of #[Cacheable] classes. + * Automatically bound to all command methods (onPost/onPut/onPatch/onDelete) of #[Cacheable] classes. * Processes #[Purge] and #[Refresh] annotations on these methods and executes cache * invalidation after successful write operations. * diff --git a/src/DonutCommandInterceptor.php b/src/DonutCommandInterceptor.php index c1cc5430..bdd2b0ff 100644 --- a/src/DonutCommandInterceptor.php +++ b/src/DonutCommandInterceptor.php @@ -5,6 +5,8 @@ namespace BEAR\QueryRepository; +use BEAR\QueryRepository\Exception\UnmatchedQuery; +use BEAR\QueryRepository\Log\Context\CacheErrorContext; use BEAR\QueryRepository\Log\Context\CommandResultContext; use BEAR\RepositoryModule\Annotation\CacheLog; use BEAR\Resource\Code; @@ -18,11 +20,12 @@ use function assert; use function call_user_func_array; use function is_callable; +use function str_starts_with; /** * Interceptor for donut cache invalidation on CQRS commands * - * Bound to command methods (onPut/onPatch/onDelete) of classes marked with #[CacheableResponse]. + * Bound to command methods (onPost/onPut/onPatch/onDelete) of classes marked with #[CacheableResponse]. * Refreshes donut cache and resource state after successful write operations. * * @see \BEAR\RepositoryModule\Annotation\CacheableResponse @@ -53,7 +56,21 @@ public function invoke(MethodInvocation $invocation): ResourceObject $openId = $this->logger->open(($this->commandContextFactory)($invocation, 'DonutCommandInterceptor')); try { if ($ro->code < Code::BAD_REQUEST) { - $this->refreshDonutAndState($ro); + try { + $this->refreshDonutAndState($ro); + } catch (UnmatchedQuery $e) { + // Same shape as RefreshSameCommand::command() on the value-cache side (#219): + // an onPost-bound class that creates rather than addresses an entity routinely + // lacks a parameter onGet requires. The write already ran; there is no entry + // to refresh, so the purge/refresh is skipped and recorded instead of thrown. + // onPut/onPatch/onDelete keep throwing - a mismatch there addresses an entity + // onGet already does, so it is a real signature mismatch, not this case. + if (! str_starts_with($invocation->getMethod()->getName(), 'onPost')) { + throw $e; + } + + $this->logger->event(new CacheErrorContext((string) $ro->uri, 'write', $e->getMessage(), $e::class)); + } } } finally { $this->logger->close(new CommandResultContext($ro->code), $openId); diff --git a/src/RefreshSameCommand.php b/src/RefreshSameCommand.php index dc9d6fe5..c666adf8 100644 --- a/src/RefreshSameCommand.php +++ b/src/RefreshSameCommand.php @@ -5,27 +5,56 @@ namespace BEAR\QueryRepository; +use BEAR\QueryRepository\Exception\UnmatchedQuery; +use BEAR\QueryRepository\Log\Context\CacheErrorContext; +use BEAR\RepositoryModule\Annotation\CacheLog; use BEAR\Resource\ResourceObject; +use Koriym\SemanticLogger\NullSemanticLogger; +use Koriym\SemanticLogger\SemanticLoggerInterface; use Override; use Ray\Aop\MethodInvocation; use ReflectionException; use function call_user_func_array; use function is_callable; +use function str_starts_with; final readonly class RefreshSameCommand implements CommandInterface { public function __construct( private QueryRepositoryInterface $repository, private MatchQueryInterface $matchQuery, + #[CacheLog] + private SemanticLoggerInterface $logger = new NullSemanticLogger(), ) { } #[Override] public function command(MethodInvocation $invocation, ResourceObject $ro): void { - unset($invocation); - $getQuery = $this->getQuery($ro); + try { + $getQuery = $this->getQuery($ro); + } catch (UnmatchedQuery $e) { + // #214 bound this command to onPost too, on classes that create rather than address + // an existing entity - onGet's required parameters (an id assigned on write) are + // routinely absent from a POST's own query. Pre-#214, onPost reached no command at + // all, so this was silently a no-op; skipping here restores that shape instead of a + // regression, and an explicit #[Refresh]/#[Purge] on the method still runs next + // (CommandsProvider orders this command first, but returning - not throwing - lets + // CommandInterceptor's loop reach the next command). + // + // onPut/onPatch/onDelete keep throwing: those act on an entity onGet already + // addresses, so a required parameter missing there is a real signature mismatch, not + // this case (BehaviorTest::testUnMatchQuery pins that). + if (! str_starts_with($invocation->getMethod()->getName(), 'onPost')) { + throw $e; + } + + $this->logger->event(new CacheErrorContext((string) $ro->uri, 'write', $e->getMessage(), $e::class)); + + return; + } + $delUri = clone $ro->uri; $delUri->query = $getQuery; @@ -46,6 +75,7 @@ public function command(MethodInvocation $invocation, ResourceObject $ro): void * @return array * * @throws ReflectionException + * @throws UnmatchedQuery */ private function getQuery(ResourceObject $ro): array { diff --git a/tests/DonutUnmatchedQueryRefreshTest.php b/tests/DonutUnmatchedQueryRefreshTest.php new file mode 100644 index 00000000..6d23de02 --- /dev/null +++ b/tests/DonutUnmatchedQueryRefreshTest.php @@ -0,0 +1,67 @@ +override(new TwigModule([dirname(__DIR__) . '/tests/Fake/fake-app/var/templates'])); + $injector = new Injector($module, __DIR__ . '/tmp'); + $this->resource = $injector->getInstance(ResourceInterface::class); + $this->logger = $injector->getInstance(SemanticLoggerInterface::class, CacheLog::class); + + parent::setUp(); + } + + public function testPostSkipsTheRefreshInsteadOfThrowing(): void + { + $ro = $this->resource->post('page://self/html/mismatched-donut-writer', ['title' => 'new']); + + $this->assertSame(200, $ro->code, 'the write itself still succeeds; only the automatic refresh cannot apply'); + } + + public function testTheSkipIsRecordedAsACacheError(): void + { + $this->resource->post('page://self/html/mismatched-donut-writer', ['title' => 'new']); + $tree = $this->flushAndValidate($this->logger); + + $error = self::eventContextJsonOf($tree, 'cache_error'); + $this->assertNotNull($error, 'the skip is a recorded event, not a silent one'); + $this->assertStringContainsString('"operation":"write"', $error); + $this->assertStringContainsString('"exceptionClass":"BEAR\\\\QueryRepository\\\\Exception\\\\UnmatchedQuery"', $error); + } + + public function testPutStillThrowsForAGenuineMismatch(): void + { + $this->expectException(UnmatchedQuery::class); + $this->resource->put('page://self/html/mismatched-donut-writer', ['title' => 'new']); + } +} diff --git a/tests/Fake/fake-app/src/Resource/App/MismatchedWriter.php b/tests/Fake/fake-app/src/Resource/App/MismatchedWriter.php new file mode 100644 index 00000000..b52a10de --- /dev/null +++ b/tests/Fake/fake-app/src/Resource/App/MismatchedWriter.php @@ -0,0 +1,35 @@ +body = ['id' => $id]; + + return $this; + } + + #[Purge(uri: 'app://self/refresh-dest?id=1')] + public function onPost(string $title): static + { + $this->body = ['title' => $title]; + + return $this; + } +} diff --git a/tests/Fake/fake-app/src/Resource/Page/Html/MismatchedDonutWriter.php b/tests/Fake/fake-app/src/Resource/Page/Html/MismatchedDonutWriter.php new file mode 100644 index 00000000..ecf2bf39 --- /dev/null +++ b/tests/Fake/fake-app/src/Resource/Page/Html/MismatchedDonutWriter.php @@ -0,0 +1,39 @@ +body = ['id' => $id]; + + return $this; + } + + public function onPost(string $title): static + { + $this->body = ['title' => $title]; + + return $this; + } + + public function onPut(string $title): static + { + $this->body = ['title' => $title]; + + return $this; + } +} diff --git a/tests/Fake/fake-app/var/templates/Page/Html/MismatchedDonutWriter.html.twig b/tests/Fake/fake-app/var/templates/Page/Html/MismatchedDonutWriter.html.twig new file mode 100644 index 00000000..94c34951 --- /dev/null +++ b/tests/Fake/fake-app/var/templates/Page/Html/MismatchedDonutWriter.html.twig @@ -0,0 +1 @@ +mismatched:{{ id }} \ No newline at end of file diff --git a/tests/UnmatchedQueryRefreshTest.php b/tests/UnmatchedQueryRefreshTest.php new file mode 100644 index 00000000..7658fcc5 --- /dev/null +++ b/tests/UnmatchedQueryRefreshTest.php @@ -0,0 +1,72 @@ +resource = $injector->getInstance(ResourceInterface::class); + $this->repository = $injector->getInstance(QueryRepositoryInterface::class); + $this->logger = $injector->getInstance(SemanticLoggerInterface::class, CacheLog::class); + + RefreshDest::$id = 0; + + parent::setUp(); + } + + public function testPostSkipsTheRefreshInsteadOfThrowing(): void + { + $ro = $this->resource->post('app://self/mismatched-writer', ['title' => 'new']); + + $this->assertSame(200, $ro->code, 'the write itself still succeeds; only the automatic refresh cannot apply'); + } + + public function testPostStillRunsTheExplicitPurgeWrittenOnIt(): void + { + $dest = new Uri('app://self/refresh-dest?id=1'); + $this->repository->put($this->resource->get('app://self/refresh-dest', ['id' => '1'])); + $this->assertInstanceOf(ResourceState::class, $this->repository->get($dest)); + + $this->resource->post('app://self/mismatched-writer', ['title' => 'new']); + + $this->assertNull($this->repository->get($dest), 'CommandsProvider runs RefreshAnnotatedCommand next - the skip must not stop it'); + } + + public function testTheSkipIsRecordedAsACacheError(): void + { + $this->resource->post('app://self/mismatched-writer', ['title' => 'new']); + $tree = $this->flushAndValidate($this->logger); + + $error = self::eventContextJsonOf($tree, 'cache_error'); + $this->assertNotNull($error, 'the skip is a recorded event, not a silent one'); + $this->assertStringContainsString('"operation":"write"', $error); + $this->assertStringContainsString('"exceptionClass":"BEAR\\\\QueryRepository\\\\Exception\\\\UnmatchedQuery"', $error); + } +}