-
-
Notifications
You must be signed in to change notification settings - Fork 967
feat(httpcache): introduce PurgeTagProviderInterface extension point #7970
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 1 commit
88bd233
f9dd946
23f166d
7ad1afe
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| <?php | ||
|
|
||
| /* | ||
| * This file is part of the API Platform project. | ||
| * | ||
| * (c) Kévin Dunglas <dunglas@gmail.com> | ||
| * | ||
| * For the full copyright and license information, please view the LICENSE | ||
| * file that was distributed with this source code. | ||
| */ | ||
|
|
||
| declare(strict_types=1); | ||
|
|
||
| namespace ApiPlatform\HttpCache; | ||
|
|
||
| /** | ||
| * Collects extra HTTP cache tags to invalidate for a given entity. | ||
| */ | ||
| interface PurgeTagProviderInterface | ||
| { | ||
| /** | ||
| * @return iterable<string> | ||
| */ | ||
| public function getTagsForResource(object $entity): iterable; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nitpicking : if it's getTagsForResource, it should be called $resource, not $entity, shouldn't it? That said I likely would have split methods for different operations, I agree with @usu that you're possibly not invalidating the same tags when inserting / modifying / deleting...but the caller should know which case they're in so why not just let the caller use the correct method?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed, renaming to Regarding splitting into separate methods: the single-method approach with a nullable
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm probably missing something, but i don't see how having 3 methods in the PurgeTagProviderInterface would change anything for DI.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
You're right, my DI argument was wrong. Apologies.
Agreed, and that also pointed to a deeper issue: splitting the calls between the listener and a processor was the wrong architecture. Moving everything to the processor layer is cleaner.
The listener keeps its existing logic (collection IRI, item IRI, relations). The providers, as an extension point, belong in the processor layer where the context is richer. So the interface becomes: public function getTagsForInsert(object $resource): iterable;
public function getTagsForUpdate(object $resource, object $previousResource): iterable;
public function getTagsForDelete(object $resource): iterable;Called from a |
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,114 @@ | ||
| <?php | ||
|
|
||
| /* | ||
| * This file is part of the API Platform project. | ||
| * | ||
| * (c) Kévin Dunglas <dunglas@gmail.com> | ||
| * | ||
| * For the full copyright and license information, please view the LICENSE | ||
| * file that was distributed with this source code. | ||
| */ | ||
|
|
||
| declare(strict_types=1); | ||
|
|
||
| namespace ApiPlatform\Laravel\Tests\Unit\Listener; | ||
|
|
||
| use ApiPlatform\HttpCache\PurgerInterface; | ||
| use ApiPlatform\HttpCache\PurgeTagProviderInterface; | ||
| use ApiPlatform\Laravel\Eloquent\Listener\PurgeHttpCacheListener; | ||
| use ApiPlatform\Metadata\Exception\InvalidArgumentException; | ||
| use ApiPlatform\Metadata\GetCollection; | ||
| use ApiPlatform\Metadata\IriConverterInterface; | ||
| use ApiPlatform\Metadata\ResourceClassResolverInterface; | ||
| use Illuminate\Database\Eloquent\Model; | ||
| use PHPUnit\Framework\TestCase; | ||
|
|
||
| class PurgeHttpCacheListenerTest extends TestCase | ||
| { | ||
| public function testPurgeTagProviders(): void | ||
| { | ||
| $model = new class extends Model { | ||
| }; | ||
|
|
||
| $purger = $this->createMock(PurgerInterface::class); | ||
| $purger->expects($this->once()) | ||
| ->method('purge') | ||
| ->with(['/models/1', '/models', '/parents/42/children']); | ||
|
|
||
| $iriConverter = $this->createStub(IriConverterInterface::class); | ||
| $iriConverter->method('getIriFromResource') | ||
| ->willReturnCallback(static function (object|string $resource, int $referenceType = 0, ?object $operation = null): string { | ||
| if ($operation instanceof GetCollection) { | ||
| return '/models'; | ||
| } | ||
|
|
||
| return '/models/1'; | ||
| }); | ||
|
|
||
| $resourceClassResolver = $this->createStub(ResourceClassResolverInterface::class); | ||
| $resourceClassResolver->method('isResourceClass')->willReturn(true); | ||
|
|
||
| $provider = $this->createMock(PurgeTagProviderInterface::class); | ||
| $provider->expects($this->once()) | ||
| ->method('getTagsForResource') | ||
| ->with($model) | ||
| ->willReturn(['/parents/42/children']); | ||
|
|
||
| $listener = new PurgeHttpCacheListener($purger, $iriConverter, $resourceClassResolver, [$provider]); | ||
| $listener->handleModelSaved('eloquent.saved: '.$model::class, [$model]); | ||
| $listener->postFlush(); | ||
| } | ||
|
|
||
| public function testPurgeTagProvidersOnDelete(): void | ||
| { | ||
| $model = new class extends Model { | ||
| }; | ||
|
|
||
| $purger = $this->createMock(PurgerInterface::class); | ||
| $purger->expects($this->once()) | ||
| ->method('purge') | ||
| ->with(['/models/1', '/models', '/parents/42/children']); | ||
|
|
||
| $iriConverter = $this->createStub(IriConverterInterface::class); | ||
| $iriConverter->method('getIriFromResource') | ||
| ->willReturnCallback(static function (object|string $resource, int $referenceType = 0, ?object $operation = null): string { | ||
| if ($operation instanceof GetCollection) { | ||
| return '/models'; | ||
| } | ||
|
|
||
| return '/models/1'; | ||
| }); | ||
|
|
||
| $resourceClassResolver = $this->createStub(ResourceClassResolverInterface::class); | ||
| $resourceClassResolver->method('isResourceClass')->willReturn(true); | ||
|
|
||
| $provider = $this->createMock(PurgeTagProviderInterface::class); | ||
| $provider->expects($this->once()) | ||
| ->method('getTagsForResource') | ||
| ->with($model) | ||
| ->willReturn(['/parents/42/children']); | ||
|
|
||
| $listener = new PurgeHttpCacheListener($purger, $iriConverter, $resourceClassResolver, [$provider]); | ||
| $listener->handleModelDeleted('eloquent.deleted: '.$model::class, [$model]); | ||
| $listener->postFlush(); | ||
| } | ||
|
|
||
| public function testNoTagsWhenNoProviders(): void | ||
| { | ||
| $model = new class extends Model { | ||
| }; | ||
|
|
||
| $purger = $this->createMock(PurgerInterface::class); | ||
| $purger->expects($this->never())->method('purge'); | ||
|
|
||
| $iriConverter = $this->createStub(IriConverterInterface::class); | ||
| $iriConverter->method('getIriFromResource')->willThrowException(new InvalidArgumentException()); | ||
|
|
||
| $resourceClassResolver = $this->createStub(ResourceClassResolverInterface::class); | ||
| $resourceClassResolver->method('isResourceClass')->willReturn(true); | ||
|
|
||
| $listener = new PurgeHttpCacheListener($purger, $iriConverter, $resourceClassResolver); | ||
| $listener->handleModelSaved('eloquent.saved: '.$model::class, [$model]); | ||
| $listener->postFlush(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this method better receive both the old entity (before an update) and the new entity (after an update)? In the example provided in the initial issue, it will otherwise be difficult to detect when a child changes it's parent and hence 2 separate tags need to be purged?
Also providing old entity and new entity would provide an easy method for the provider to detect if the method was called from an insertion or deletion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the feedback, you're right that the current signature is insufficient for the parent-change use case.
When a
Childchanges itsparent, two tags need to be purged:/parents/{old_id}/childrenand/parents/{new_id}/children. With the currentgetTagsForResource(object $entity), the implementor only receives the entity in its new state and has no way to compute the old parent's tag.The fix I have in mind is to change the signature to:
Where
previousEntityisnullon insertion and populated on update/deletion. This is BC-safe since the second parameter is nullable with a default value.The key architectural point is where this is called. The current implementation calls providers from inside
PurgeHttpCacheListener(a Doctrine ORM event listener). At that layer there is noprevious_data— Doctrine's changeset only exposes scalar/identifier diffs, not a fully hydrated snapshot of the previous entity.API Platform already solves this at a higher level:
ReadProviderclones the entity before deserialization and stores it in$context['previous_data']. This snapshot is exactly what we need. The right place to call the providers is therefore the processor layer (e.g.PersistProcessoror a dedicated purge processor), where$context['previous_data']is naturally available for PUT/PATCH operations andnullfor POST.I can rework the PR to move the provider calls to that layer and update the interface accordingly. @soyuka what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, good point. We currently have an own implementation of the listener, where we use the Doctrine changeset to manually calculate the state of the previous entity. It works well for us, but cannot guarantee it is bulletproof:
https://github.com/ecamp/ecamp3/blob/6bebc1320d7f92bea8ecda743517632556b807a1/api/src/HttpCache/PurgeHttpCacheListener.php#L140C22-L140C39
Regarding the signature, shouldn't
$entityalso be nullable in case of deletions? In that case the provider could distinguish all 3 cases (updates, deletions, inserts) based on which parameter is null or populated.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's an architectural constraint worth surfacing here. The providers are currently called from inside the Doctrine event listener, which operates at the ORM layer — it has no access to API Platform's request context. That means
$previousResourcecannot be populated on updates from that layer: Doctrine only exposesgetEntityChangeSet()(scalar diffs), not a fully hydrated snapshot of the previous entity.Two options:
Option 1 — keep providers in the listener, add
array $context = []The listener only knows the Doctrine event type, so
$contextwould carry a string at best:$previousResourcewould always benullon updates, defeating the purpose.Option 2 — move provider calls to the processor layer
PersistProcessorhas$context['previous_data']natively (a full hydrated snapshot set byReadProvider), and the actual API PlatformOperationobject as a dedicated parameter. Both can be forwarded to the provider:This is the only way to get a real
$previousResourceon updates, plus a typedOperationinstance (e.g.Put,Delete) instead of a plain string. The catch: IRI isn't available before flush on inserts (no auto-generated ID yet), which the current listener handles via$scheduledInsertions+postFlush()— a complementary post-flush listener would still be needed for that case.Option 2 gives the interface you described, at the cost of a broader refactor. Happy to go that route — wanted to flag the tradeoff first. What do you think?