feat(serializer): collect cache tags using a TagCollector #5758

usu · 2023-08-19T12:18:28Z

Q	A
Branch?	main
Tickets	Supersedes #5753
License	MIT
Doc PR

Alternative implementation of #5753 but using a service instead of events

soyuka · 2023-08-22T09:31:32Z

src/HttpCache/TagCollector.php

+ *
+ * @author Urban Suppiger <urban@suppiger.net>
+ */
+class TagCollector implements TagCollectorInterface


can you explain to me the difference between all these methods? I think that we should be able to simplify this and reduce the number of arguments. I'd suggest:

use ApiPlatform\Serializer; // we want to avoid creating a dependency between Serializer and HttpCache, the opposite is fine. interface IriCollectorInterface { public function collect(string $iri, mixed $object, array $context) {} }

Not even sure if object matters in the signature and it could be added to the context? It'd be interesting to add the attribute that's being normalized if needed although that I'd start without it and if someone needs that we add it afterwards? Or do you already have a use case where that matters?

If that's the case, we can put everything we need inside our context. I'd not have a childContext though, only give the current context it introduces less complexity and the use cases are often not good ones.

The various methods (extension points) are needed in order to implement the cache logic proposed under #5650 in userland.

In order to implement this logic, the tag collector service on user side would look something like below (no guarantee for correctness, typing in a text editor only).

<?php class CustomTagCollector implements TagCollectorInterface { public function collectTagsFromNormalize(mixed $object, string $format = null, array $context = [], string $iri = null): void { $this->addCacheTagForRessource($context, $iri); } public function collectTagsFromNormalizeRelation(mixed $object, string $format = null, array $context = [], string $iri = null): void { } public function collectTagsFromGetAttribute(mixed $object, string $format = null, array $context = [], string $iri = null, string $attribute = null, ApiProperty $propertyMetadata = null, Type $type = null, array $childContext = []): void { $this->addCacheTagsForRelation(array $context, $propertyMetadata) } private function addCacheTagForRessource(array $context, ?string $iri): void { if (isset($context['resources']) && isset($iri)) { $context['resources'][$iri] = $iri; } } private function addCacheTagsForRelation(array $context, ApiProperty $propertyMetadata): void { // Add cache tag for related collection if (isset($context['resources'])) { if (isset($propertyMetadata->getExtraProperties()['cacheDependencies'])) { foreach ($propertyMetadata->getExtraProperties()['cacheDependencies'] as $dependency) { $cacheTag = $context['iri'].self::IRI_RELATION_DELIMITER.$dependency; $context['resources'][$cacheTag] = $cacheTag; } } else { $cacheTag = $context['iri'].self::IRI_RELATION_DELIMITER.$context['api_attribute']; $context['resources'][$cacheTag] = $cacheTag; } } } }

If there's only one collect method, I'd need to figure out from which point in the normalization process the collect method was called. Might be possible based on the content of the attributes, but not sure whether this leads to robust and understandable code.

With regards to parameters, in order to implement the original behavior of api-platform as well as the logic from #5650 I'd need the following parameters as a minimum: $context, $iri, $propertyMetadata

All the rest ($object, $format, $childContext, $attribute, $type) can also be removed and we could add in case there's a specific usecase coming up for it. $type would be needed though, if anyone wants to differentiate relations to collections (*ToMany) and *ToOne relations.

looking at the $context['operation'] you should be able to find out where you're at. I'd really like to see what code you need to handle these. Understand that if we add an interface we really need to work on it because it'll last a while after publishing it and we did the mistake in the past, making an interface evolve with our current BC layer is quite hard afterwards.

No prob, fully understand design of the interface is crucial.

For implementation of the current api-platform logic, I could use $attribute to ignore calls coming from getAttribute, because this parameter is not sent from normalize or normalizeRelation. I cannot use $context['api_attribute'], because this is set for nested/embedded objects also for calls from normalize or normalizeRelation.

class TagCollector implements TagCollectorInterface { public function collect(mixed $object, string $format = null, array $context = [], string $iri = null, string $attribute = null, ApiProperty $propertyMetadata = null, Type $type = null, array $childContext = []): void { if(!$attribute){ $this->addResourceToContext($context, $iri); } } private function addResourceToContext(array $context, ?string $iri): void { if (isset($context['resources']) && isset($iri)) { $context['resources'][$iri] = $iri; } } }

Implementing the logic above for #5650 is a bit more tricky. With the current code, I don't see any good means to differentiate between calls from normalize and normalizeRelation. I could add an additional string attribute to disclose, where the collect call was made from. But honestly, that seems a bit like a disguised event system and I'd rather have a proper event system implemented as in the initial PR #5753.

Alternatively I think I could add the normalized $data back as an attribute and then check within collect, if data is an object (fully normalized embedded resource) or only a string (resource normalized as IRI only).

You need to find out whether the call is made in a normalizeRelation as opposed to a normalize? you have api_sub_level for that IIRC.

You need to find out whether the call is made in a normalizeRelation as opposed to a normalize?

Yeah, correct. #5650 gives more background on the motivation. But in a nutshell, I'd want to treat fully embedded resources (resource embedded in the response as object) differently from linked resources (only IRI of resource included in response).

For the former, the cache tag is triggered from normalize. For latter, the cache tag is triggered from normalizeRelation (at least that's how it's implemented now).

you have api_sub_level for that IIRC.

Not sure, this works. When running tags.feature tests, api_sub_level seems always to be true.

Alternatively I think I could add the normalized $data back as an attribute and then check within collect, if data is an object (fully normalized embedded resource) or only a string (resource normalized as IRI only).

I just pushed a proposal implementing this. When passing $data, I can check on userland if $data is an array/object or a string (IRI) only and hence differentiate embedded objects from linked IRIs.

For JsonAPI this is a bit "trickier", because even for linked data, the return value is an array and not only a string. However, as we pass $format as well, this could still be differentiated from a normal fully embedded object.

I had to change the tests, because now the order of tags has changed when waiting for the normalized data. The tags are still the same though.

Please resolve #5758 (comment) so that we don't change any tests

src/JsonApi/Serializer/ItemNormalizer.php

soyuka · 2023-08-28T12:46:04Z

src/HttpCache/TagCollector.php

+ */
+class TagCollector implements TagCollectorInterface
+{
+    public function collect(mixed $object = null, string $format = null, array $context = [], string $iri = null, mixed $data = null, string $attribute = null, ApiProperty $propertyMetadata = null, Type $type = null): void


I think we're closed to something nice, a few things still bothers me. Do we really need data and object ? I think that this works using only object.
If I think about an IriCollector for cache tags, I don't see the format in its signature, we can put this inside our $context no? I also think that $propertyMetadata alone is fine as it should hold the $type information. So my signature looks like this:

public function collect(mixed $object = null, string $iri = null, array $context = [], string $attribute = null, ApiProperty $propertyMetadata = null): void

Last but not least, I also think that the iri can actually be computed by the tag collector if needed, so we'd end up with something like this ($context['iri'] should be defined for performances if available):

public function collect(mixed $object = null, array $context = [], string $attribute = null, ApiProperty $propertyMetadata = null): void

Not even sure if we couldn't remove $attribute and $propertyMetadata, are these really useful?

You could probably add your use case as functional test so that I see how you use it? You can add your service in our config_common.yml inside the TestBundle.

You could probably add your use case as functional test so that I see how you use it? You can add your service in our config_common.yml inside the TestBundle.

I tried to integrate this into our own project yesterday. That might help a bit to evaluate some of the use cases:
https://github.com/ecamp/ecamp3/pull/3610/files#diff-92df153f2ab4331ccfd55e92699e78c72a3223ff10a38cb870ad59f769308fe0

Can also try to integrate some of this into the test suit.

For your other comments, will look at them and respond later.

I added a functional test with commit a404dd7

Adding the custom TagCollector to config_common.yml obviously breaks the existing tests in features/http_cache/tags.feature. Not sure what the best way is to isolate this test and couldn't find a good example to copy from. Do I need to set up a separate behat suite which only adds the tag collector service?

#5758 (comment) should resolve the tests issue no?

soyuka · 2023-09-23T10:42:35Z

src/Serializer/TagCollectorInterface.php

+ */
+interface TagCollectorInterface
+{
+    public function collect(mixed $object = null, string $format = null, array $context = [], string $iri = null, mixed $data = null, string $attribute = null, ApiProperty $propertyMetadata = null, Type $type = null): void;


I need to find time to improve that interface, your tests helps a lot thanks!

usu · 2023-10-17T11:00:08Z

I was struggling a bit to ensure Behat is using the custom tag collector service only for a subset of the tests. The best I could come up with is the latest commit: 889a614

It seems not that easy to override a service for a specific Behat scenario. It kind of works, when the kernel is not rebooted between scenarios (there's a whole discussion on this under FriendsOfBehat/SymfonyExtension#149).

With this latest commit, it can at least switch between TagCollectorDefault and TagCollectorCustom. I couldn't figure out how to remove the service completely (manually setting api_platform.http_cache.tag_collector to null doesn't work).

The caveat of this is that the old tests had to be adjusted slightly. TagCollectorDefault replicates the legacy behavior almost identically, but the sequence of the tags is a bit different.

@soyuka Do you mind reviewing this one more time. Do you know of any better method to enable/disable services for specific scenarios?

soyuka · 2023-10-17T12:59:07Z

tests/Behat/HttpCacheContext.php

    {
+        $this->disableReboot($scope);
+        $this->driverContainer->set('api_platform.http_cache.tag_collector', new TagCollectorCustom());


this is quite awesome I wasn't aware we could do that

soyuka · 2023-10-17T13:07:44Z

I like this, if you could:

rebase onto main
reword the commit (I changed the title)

Also about this interface, I suggest to reduce it to:

interface TagCollectorInterface
{
    public function collect(string $iri, array $context = []): void;

Can $iri be actually null but we'd like to collect it anyway?

I definitely think that all these can go to our TagCollector context:

mixed $data = null, string $attribute = null, ApiProperty $propertyMetadata = null, Type $type = null
mixed $object = null, string $format = null,

I'm also okay to add an immutable option object if we don't want an array as $context, but all these are noisy in the function signature no?

LMK what you think

usu · 2023-10-20T17:10:24Z

I will give a try to push everything into $context.

Would you also accept the PR against 3.2 instead of main? Would be nice to start using it right away, as it kind of blocks anything around caching on our application side.

usu · 2023-10-24T13:01:05Z

@soyuka I pushed all the parameters into $context, so nothing else needed for the function signature. Let me know if you believe this looks clean.

I was a bit woried about any side effects at different places, when touching the $context. But tests seem successful.

usu · 2023-11-10T21:16:55Z

hey @soyuka, just wanted to check if you had the chance to look into this?

soyuka · 2023-12-18T08:17:14Z

I was quite busy with the SymfonyCon + related I'm looking at this asap. It'll be merged onto main and out by Feb v3.3 as we can't add features to patch versions. Maybe that I can tag an early beta though!

soyuka

Really nice, this will be a quite nice extension point thanks @usu !! some last few changes and please target the main branch :).

src/Serializer/AbstractItemNormalizer.php

src/Serializer/TagCollectorInterface.php

soyuka · 2023-12-18T08:24:59Z

tests/Fixtures/TestBundle/HttpCache/TagCollectorCustom.php

+
+    public function collect(array $context = []): void
+    {
+        $iri = $context['iri'];


IRI and object can be null here no? (we probably don't care as it's a test fixture) I'd probably add a comment for readers

Yeah, iri and object could potentially both be null. I think this should be clearer now with the interface signature documentation.

In this specific implementation (TagCollectorCustom), I'm checking further below for this (e.g. isset($iri)).

src/Symfony/Bundle/Resources/config/http_cache_purger.xml

src/Serializer/Tests/AbstractItemNormalizerTest.php

usu · 2023-12-25T09:14:09Z

Really nice, this will be a quite nice extension point thanks @usu !! some last few changes and please target the main branch :).

All implemented as per comments and rebased against main.

MongoDB tests and cs-fixer is failing. But doesn't seem related to any code I touched.

Also, I had to disable the new tests tag_collector_service.feature for Symfony lowest. It seemed the method to disable reboot and override services during boot doesn't play nicely with Symfony 6.1.

usu · 2023-12-25T09:16:00Z

Maybe that I can tag an early beta though!

That would actually be really nice, then we could already start test & integration with our own application. 😃

tests/Fixtures/app/config/config_common.yml

…rm#5758) * feat(serializer): collect cache tags using a TagCollector * simplify function signature * fix bug in JsonApi normalizer * minor changes as per latest review * disable new tests for Symfony lowest * cs --------- Co-authored-by: Antoine Bluchet <soyuka@users.noreply.github.com>

soyuka · 2024-01-19T19:22:06Z

@usu would it be possible to document this feature? BTW I released an alpha version today

usu · 2024-01-20T06:34:24Z

@usu would it be possible to document this feature? BTW I released an alpha version today

Sure. I can add a chapter to https://github.com/api-platform/docs/blob/main/core/performance.md.
Thanks for releasing the alpha 🚀

soyuka · 2024-04-30T07:42:10Z

@usu I released would love some docs if you can

usu · 2024-05-02T18:20:47Z

Just opened a PR at #6348.
Do you think a guide would be sufficient? Or would you like to see this being mentioned in the normal docs as well?

usu mentioned this pull request Aug 19, 2023

chore(HttpCache): outsource cache tag collection into normalizer events #5753

Closed

soyuka reviewed Aug 22, 2023

View reviewed changes

src/JsonApi/Serializer/ItemNormalizer.php Outdated Show resolved Hide resolved

soyuka reviewed Aug 28, 2023

View reviewed changes

src/JsonApi/Serializer/ItemNormalizer.php Show resolved Hide resolved

soyuka reviewed Aug 28, 2023

View reviewed changes

soyuka reviewed Sep 23, 2023

View reviewed changes

soyuka added the enhancement label Oct 4, 2023

soyuka mentioned this pull request Oct 17, 2023

Varnish cache tags with a big number of entities #3168

Closed

soyuka reviewed Oct 17, 2023

View reviewed changes

soyuka changed the title ~~chore(serializer): outsource cache tag collection into separate service~~ feat(serializer): collect cache tags using a TagCollector Oct 17, 2023

usu force-pushed the chore/service-for-cache-tags branch from 889a614 to 159245f Compare October 20, 2023 19:04

usu changed the base branch from 3.1 to 3.2 October 20, 2023 19:04

BacLuc mentioned this pull request Nov 18, 2023

Performance optimization big plan ecamp/ecamp3#3717

Closed

soyuka requested changes Dec 18, 2023

View reviewed changes

usu added 3 commits December 23, 2023 10:55

feat(serializer): collect cache tags using a TagCollector

74f1259

simplify function signature

6550208

fix bug in JsonApi normalizer

c80a479

usu changed the base branch from 3.2 to main December 23, 2023 10:01

usu force-pushed the chore/service-for-cache-tags branch 2 times, most recently from 0116a0d to 140e796 Compare December 23, 2023 11:26

minor changes as per latest review

828ecb6

usu force-pushed the chore/service-for-cache-tags branch 2 times, most recently from c1d7196 to 2428cac Compare December 23, 2023 15:09

disable new tests for Symfony lowest

2e2c2c2

usu force-pushed the chore/service-for-cache-tags branch from 2428cac to 2e2c2c2 Compare December 25, 2023 08:46

soyuka reviewed Dec 26, 2023

View reviewed changes

tests/Fixtures/app/config/config_common.yml Outdated Show resolved Hide resolved

cs

1be617d

soyuka approved these changes Dec 26, 2023

View reviewed changes

soyuka merged commit 670e7fb into api-platform:main Dec 26, 2023
49 of 53 checks passed

usu mentioned this pull request Jan 6, 2024

POC: HTTP reverse proxy cache ecamp/ecamp3#3610

Merged

9 tasks

usu mentioned this pull request May 2, 2024

docs: add guide for custom TagCollector service #6348

Merged

soyuka added a commit to soyuka/core that referenced this pull request Sep 13, 2024

perf: skip context data added in api-platform#5758

9563175

soyuka mentioned this pull request Sep 13, 2024

perf: skip context data added in #5758 #6612

Merged

soyuka added a commit to soyuka/core that referenced this pull request Sep 15, 2024

perf: skip context data added in api-platform#5758

9bcffa8

soyuka added a commit to soyuka/core that referenced this pull request Sep 15, 2024

perf: skip context data added in api-platform#5758

2e532bc

soyuka added a commit that referenced this pull request Sep 15, 2024

perf: skip context data added in #5758

04b5ee9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(serializer): collect cache tags using a TagCollector #5758

feat(serializer): collect cache tags using a TagCollector #5758

usu commented Aug 19, 2023 •

edited by soyuka

Loading

soyuka Aug 22, 2023

usu Aug 22, 2023

soyuka Aug 22, 2023

usu Aug 24, 2023

soyuka Aug 24, 2023

usu Aug 26, 2023

usu Aug 26, 2023

soyuka Sep 23, 2023

soyuka Aug 28, 2023

usu Aug 28, 2023

usu Sep 23, 2023

soyuka Sep 23, 2023

soyuka Sep 23, 2023

usu commented Oct 17, 2023

soyuka Oct 17, 2023

soyuka commented Oct 17, 2023

usu commented Oct 20, 2023

usu commented Oct 24, 2023

usu commented Nov 10, 2023

soyuka commented Dec 18, 2023

soyuka left a comment

soyuka Dec 18, 2023

usu Dec 25, 2023

usu commented Dec 25, 2023

usu commented Dec 25, 2023

soyuka commented Jan 19, 2024

usu commented Jan 20, 2024

soyuka commented Apr 30, 2024

usu commented May 2, 2024

feat(serializer): collect cache tags using a TagCollector #5758

feat(serializer): collect cache tags using a TagCollector #5758

Conversation

usu commented Aug 19, 2023 • edited by soyuka Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

usu commented Oct 17, 2023

Choose a reason for hiding this comment

soyuka commented Oct 17, 2023

usu commented Oct 20, 2023

usu commented Oct 24, 2023

usu commented Nov 10, 2023

soyuka commented Dec 18, 2023

soyuka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

usu commented Dec 25, 2023

usu commented Dec 25, 2023

soyuka commented Jan 19, 2024

usu commented Jan 20, 2024

soyuka commented Apr 30, 2024

usu commented May 2, 2024

usu commented Aug 19, 2023 •

edited by soyuka

Loading