[BUGFIX] getRelated is using wrong cache tags

I like where this is going, but we should not construct the cacheable metadata strings ourselves.

1. +++ b/src/Resource/EntityResource.php
   @@ -253,14 +253,24 @@ class EntityResource implements EntityResourceInterface {
   +    $tags = [
   ...
   +    $response->getCacheableMetadata()->setCacheTags($tags);
   

   We should not be limited by tags. What if there is an uncacheable entity in there, we want to detect that as well.

   We want to use something like:

   $response
     ->getCacheableMetadata()
     ->addCacheableDependency($related_entity);
   

2. +++ b/tests/src/Kernel/Resource/EntityResourceTest.php
   @@ -359,7 +366,11 @@ class EntityResourceTest extends JsonapiKernelTestBase {
   -    $this->assertEquals(['config:user_role_list'], $response
   

   Good! Entity list tags should only be present where the lists are dynamic, not editorially controlled.

1. +++ b/src/Resource/EntityResource.php
   @@ -260,7 +260,19 @@ class EntityResource implements EntityResourceInterface {
   +    // When listing related entities to an entity, the list of cachetags should
   +    // include the id of the related entities and the id of the origin entity.
   +    $response->getCacheableMetadata()->addCacheableDependency($entity);
   +    foreach ($field_list as $field_item) {
   +      /* @var \Drupal\Core\Entity\EntityInterface $related_entity */
   +      $related_entity = $field_item->entity;
   +      $response->getCacheableMetadata()->addCacheableDependency($related_entity);
   +    }
   

   Hm, all of this reminds me of \Drupal\Core\Entity\EntityInterface::referencedEntities():

     /**
      * Gets a list of entities referenced by this entity.
      *
      * @return \Drupal\Core\Entity\EntityInterface[]
      *   An array of entities.
      */
     public function referencedEntities();

2. +++ b/tests/src/Kernel/Resource/EntityResourceTest.php
   @@ -359,7 +378,26 @@ class EntityResourceTest extends JsonapiKernelTestBase {
   -    $this->assertEquals(['config:user_role_list'], $response
   +    $this->assertEquals([
   +        'config:user.role.test_role_one',
   +        'config:user.role.test_role_two',
   +        'user:' . $this->user->id(),
   +      ], $response
   +      ->getCacheableMetadata()
   

   The reason this is changing is that EntityResource::getRelated() used to call EntityResource::respondWithCollection(), but no longer does that.

   AFAICT the real bug is in EntityResource::getRelated():

       // When a new change to any entity in the resource happens, we cannot ensure
       // the validity of this cached list. Add the list tag to deal with that.
       $list_tag = $this->entityTypeManager->getDefinition($entity_type_id)
         ->getListCacheTags();
       $response->getCacheableMetadata()->setCacheTags($list_tag);
   

   That setCacheTags() call is overwriting all cache tags with just the list cache tag. That should become a addCacheTags() call, which will add it to the set of cache tags collected so far.

This is blocked by getting tests on #2836409: [BUGFIX] Improve cacheable metadata for collections.

I like the feature in this patch. However this has the potential to yield a VERY long list of cache tags, such long list may have problems with some CDNs. As fubhy highlighted on the GraphQL module.

Re-rolled and changed strategy a bit.

Added a code comment to explain why the individual requests don't have the cache metadata for the requested entity.

Added #2841851: Audit cacheable metadata generation to see if we can be more focused in the metadata generation.

Looking at the patch, I see what you're fixing/changing:

1. +++ b/src/RequestHandler.php
   @@ -145,6 +145,9 @@ class RequestHandler implements ContainerAwareInterface, ContainerInjectionInter
   +        // The serializer modifies the $cacheable_metadata, which is referenced
   +        // by the $response object, effectively adding the metadata to the
   +        // response.
   

   The serializer doesn't modify this. It's normalizers that refine the cacheability metadata.

   Specifically, \Drupal\jsonapi\Normalizer\JsonApiDocumentTopLevelNormalizer::normalize and \Drupal\jsonapi\Normalizer\RelationshipItemNormalizer::normalize refine it.

   So I'd rephrase this to:
   The serializer receives the response's cacheability metadata object as serialization context. Normalizers called by the serializer then refine this cacheability metadata, and thus they are effectively updating the response object's cacheability.

2. +++ b/src/Resource/EntityResource.php
   @@ -252,19 +253,18 @@ class EntityResource implements EntityResourceInterface {
   +      $cacheable_metadata->addCacheableDependency($entity_item);
   

   So this is the thing that was missing!

3. +++ b/src/Resource/EntityResource.php
   @@ -252,19 +253,18 @@ class EntityResource implements EntityResourceInterface {
   -    array_walk($access_info, function ($access) use ($response) {
   -      $response->addCacheableDependency($access);
   -    });
   

   We still need cacheability metadata of access results. We should keep this.

but I'm not convinced it will fix the original reported issue. It would cause correct invalidation (thanks to adding the missing cache tags), but you're not making changes that would cause Dynamic Page Cache to stop growing so much.

Because the issue reported originally is that dynamic_page_cache grows to enormous sizes. I don't understand how that's possible just yet. Unless we're talking hundreds of thousands of nodes and they're all being requested in many different ways.

The explanation in #2 makes no sense to me: the presence or absence of a certain cache tag cannot cause more or less cache items to be created. Cache tags are used only for invalidation. And invalidation happens only at runtime: if a cache item X has a tag that's been invalidated since that cache item X was created, then X will *not* be removed from the cache until it's requested again. Only when it's been requested, we check whether tags have been invalidated. (This was done for scalability reasons. The details behind this are out of scope here.)

When I started looking into this, I noticed that entity access results are not added to the cacheability metadata for a response. That would definitely be an explanation: it could mean that even responses that vary by the user cache context are being cached by Dynamic Page Cache. This is a problem in \Drupal\jsonapi\Controller\EntityResource::getIndividual for example, but \Drupal\jsonapi\Controller\EntityResource::getCollection and \Drupal\jsonapi\Controller\EntityResource::getRelated seem to get it right for example.

So, +1 for the changes in this patch (well, once my feedback is addressed), but I don't see how it would solve the originally reported problem.

But as you said, the caches never expire. #2 does not solve that?

The root cause is that the default cache back-end (the database-powered one) pretends to be unlimited in size. We have #2526150: Database cache bins allow unlimited growth: cache DB tables of gigabytes! to fix that.

It would cause correct invalidation (thanks to adding the missing cache tags)

For the record, this was already being correctly invalidated via the entity list tag. However it was invalidating too much.

I addressed the feedback on #23.

I re-rolled the existing patch.

Added extra test as requested from @dagmar.

Merging if green.

Hi @dagmar

We are using Jsonapi (1.x-dev) with 2828639-cachetags-for-get-related-6.patch and works good but due to https://www.drupal.org/node/2723323 (When deleting an entity, references to the deleted entity remain in entity reference fields) I would like to complement your patch with an additional validation in order to avoid an error when a referenced node is deleted (Error: Call to a member function id() on null en Drupal\jsonapi\Resource\EntityResource->getRelated()).

Note: I will create a new issue to share the equivalent patch for the last version of jsonapi.

Thanks.