Complete JSON:API's handling of field (including computed & empty fields) cache metadata

It might help to know what the cache tags are if you ask for a single article with that field. There are probably some tags missing regarding the related articles in the computed field which could mean cache it not refreshed when needed.

@tivi22 did you make any progress on that?

Need to do a test on instanceof CacheableDependencyInterface similar to how NormalizerBase::addCacheableDependency() does.

Added conditional... needs tests & maintainer review of concept.

Also ran into this question today - how to handle cacheable empty references? Updated PR incoming, which also touches attributes (that is, all other fields than ERs) and introduces an interface for field item lists to provide this data.

Great to have cachability fixed, added a few comments. But this would be great :) I agree on the follow up to fix any fields that would benifit from thsi.

@bradjones1
I have applied the changes from MR to my codebase but it doesnt work for me. Cache metadata is not visible in header.

Cache metadata is not visible in header.

This does not do anything on its own, rather it enables a rather "expert mode" type of manipulation of cache metadata for field types/field properties which require it.

How i can add cache metadata to my computed ER field ?

foreach ($data as $key => $nid) {
    $item = $this->createItem($key, $nid);
    item->get('target_id')->addCacheTags(["node_list:foo_bar"]);
    $this->list[$key] = $item;
}

Am i missing anything here?

Hi,

I have managed to get it working.

@tivi22

Refer https://tech.kinksters.dating/posts/2022-08-13-cacheability-computed-dru...

I did another trick to propagate cache metadata to the JSON:API normalization of a computed reference.

1. Add protected ?CacheableMetadata $cacheMetadata = NULL; to the computed field class.
2. Implement the access method by the computed field class:

       public function access($operation = 'view', AccountInterface $account = NULL, $return_as_object = FALSE): AccessResultInterface|bool {
         $access = parent::access($operation, $account, TRUE);
       
         if ($return_as_object) {
           // Here you witness a pure hack. The thing is that JSON:API
           // normalization does not compute cacheable metadata for
           // computed relations like this one.
           /** @see \Drupal\jsonapi\JsonApiResource\ResourceIdentifier */
           /** @see \Drupal\jsonapi\Normalizer\ResourceIdentifierNormalizer */
           // However, thanks to the access check, its result is added
           // as a cacheable dependency to the normalization.
           /** @see \Drupal\jsonapi\Normalizer\ResourceObjectNormalizer::serializeField() */
           $this->ensureComputedValue();
           \assert($this->cacheMetadata instanceof CacheableMetadata);
           $access->addCacheableDependency($this->cacheMetadata);
       
           return $access;
         }
       
         return $access->isAllowed();
       }
       

3. Collect cache metadata within the computeValue:

       protected function computeValue(): void {
         $entity = $this->getEntity();
         $cache_tags = [
           // The list tag is needed to ensure the JSON:API normalization cache
           // will be rebuilt on adding a new or deleting an existing node.
           /** @see \Drupal\Core\Entity\EntityBase::getListCacheTagsToInvalidate() */
           'node_list:article',
         ];
         $delta = 0;
       
         foreach ([20, 24, 13] as $id) {
           $this->list[] = $this->createItem($delta++, $id);
           $cache_tags[] = 'node:' . $id;
         }
       
         $this->cacheMetadata = new CacheableMetadata();
         $this->cacheMetadata->setCacheTags($cache_tags);
       }
       

As noted in the comments in #31 that's a hack and while it works is not a definitive solution.

This now conflicts with #3100732: Allow specifying metadata on JSON:API objects but is relatively easy to resolve; they are both swimming in the same direction in that they contribute to the cacheability of the normalization.

Another attempt with a slightly different approach which might not need an additional interface for empty field item lists, let's see how it goes.

Would appreciate some review, but leaving at 'needs work' due to missing test coverage.

@jan.stockler - Thanks for the MR, however I'm curious how you handle the empty case? It seems to require special handling, unless I'm missing something obvious here.

@bradjones1

Thanks for the MR, however I'm curious how you handle the empty case? It seems to require special handling, unless I'm missing something obvious here.

Can you elaborate on why empty field item lists need special handling, i seem to be missing or not fully grasping something on that front?

Rebased my MR, no regressions there, this still needs specific test coverage.

Can you elaborate on why empty field item lists need special handling, i seem to be missing or not fully grasping something on that front?

Empty lists are empty for a reason. However, if we can't attach cacheability data to that emptiness, then when the empty condition changes (e.g., due to something that does not otherwise cache-invalidate the entity) then you'll never see the new, populated data.

One thing the previous MR's fail to account for are computed fields with differing cache contexts. This is currently not used in core, but is supported, see Drupal\entity_test\Plugin\Field\ComputedTestCacheableStringItemList. Perhaps my addition can be rebased on the already existing cache bubbling for entity reference fields.

Thanks for taking a look and catching that. Feel free to give a shot at merging the MRs, as it were, or I'll try to loop back around on this "soon."

Coming back to this after a while and trying to condense the different approaches here. I think the confusion between @bradjones1 and @duivw on the one hand and @jan.stoeckler and myself on the other is that as far as I can tell @bradjones1 and @duivw, you are tackling the issue on the field-item level. @jan.stoeckler and I are tackling the issue on the field-item-list level. On the field-item-list level it is unnecessary to differentiate explicitly between empty and non-empty lists. If a field item list specifies its cacheability it should take care to do so even if it is empty. Thus, I think that is the preferable approach versus introducing a new special-cased interface just for this. Also, I think the list level is the more intuitive one when talking about computed fields, because to get a computed field you always have to create an item list class which uses ComputedItemListTrait. It is valid, however, to allow bubbling cacheable metadata on the item level, as well. I would have expected that code to be introduced into FieldItemNormalizer, however, and not into FieldNormalizer as in your branch, @bradjones1, or in ResourceObjectNormalizer, as in your branch, @duivw. Since both of your branches are lacking test coverage as of yet, I will add some test coverage for item-level cacheable metadata to your branch, @jan.stoeckler, and by that verify if adding some handling to FieldItemNormalizer is in fact possible. Hope that we can drive this issue forward and would love to hear some thoughts. Please let me know if I missed anything or misrepresented the status quo.

Edit: Seeing now that all branches do in fact update ResourceObjectNormalizer so that point is moot or at least confusing. Had missed that before, sorry.

Looking at the issue metadata and stumbled over #21. Looked into any existing computed field item lists and found path alias and content moderation state. So I opened #3570814: Attach route_match cache tag to PathFieldItemList instances and #3570817: Attach cacheable metadata to ModerationStateFieldItemList instances for that, so removing the tag.

In rebasing the branch from @jan.stoeckler I found #3304772: Cache tags from Computed fields do not bubble up to Entity render array (because there was a conflict with the commit from that issue). Over there we added test coverage for the render API to properly bubble cache metadata from the item list metadata. That test coverage is something we can re-use now that that's landed to verify the same thing works in JSON:API.

In my opinion that suggests that fixing support for cache metadata on the item level should be a separate issue where we add test for both the render API and JSON:API.

I hope that makes sense and that you agree with that conclusion. I'm really not trying to railroad this issue, but I do think that that is a relevant precedent. In any case would love to hear some thoughts on this.

OK, rebased this issue now and with #3304772: Cache tags from Computed fields do not bubble up to Entity render array providing the basic setup for the test infrastructure this is now a lot smaller and hopefully easy to review.

Added a draft change notice now.

Updated the issue summary.

Yay, this is awesome. I didn't spot issues, only thing i see that might not be adressed is https://www.drupal.org/project/drupal/issues/3252278#comment-16204241

Which mentioned cache contexts. Can we verify that this is not an issue?

Cool, thanks for taking a look. When working on this I was also wondering why the additional cache context doesn't require any changes in the test code and was afraid something was broken. It's not actually and everything works fine, it's just that the field item uses url.query_args:computed_test_cacheable_integer_field as a cache context and JSON:API responses already add url.query_args by default (see \Drupal\Tests\jsonapi\Functional\ResourceTestBase::getExpectedCacheContexts()) so that gets optimized away.

I can look into updating that to use a different cache context, though, so that the test coverage becomes more explicit. Marking needs work for that.

Alrighty, used a different cache context now and that properly requires adding that to the test expectations.

Awesome, thanks! As per #60, setting RTBC.

I had a concern in ResourceTestBase that $cacheability->addCacheableDependency(CacheableMetadata::createFromObject($field_item_list));, when $field_item_list does not implement CacheableDependencyInterface, meant that max-age 0 bubbles. But that's not part of the response, so it seems fine, and a similar thing is already done with the field properties.

Committed 16b3297 and pushed to main and committed 4ba0eff and pushed to 11.x. Thanks!

🤩

Awesome work!