Cacheability metadata on an entity fields' properties is lost

👍

#2921257: On Drupal 8.5, JSON API should respect Typed Data's new ::isInternal(), and JSON API Extras should use ::setInternal() is RTBC, so hopefully this can move forward again soon too.

#2926463: [>=8.5] Remove JSON API's "file URL" field work-around now that Drupal core 8.5 fixed it has been unblocked!

Actually, AFAICT this is not blocked on #2926463: [>=8.5] Remove JSON API's "file URL" field work-around now that Drupal core 8.5 fixed it.

Surprisingly, #5 still applied!

Yay, the expected test failures on 8.6! Yay for #2930028: Comprehensive JSON API integration test coverage phase 1: for every entity type, individual resources only and friends :)

+++ b/src/EntityToJsonApi.php
@@ -102,7 +103,7 @@ class EntityToJsonApi {
-      'cacheable_metadata' => new CacheableMetadata(),
+      CacheableNormalizerInterface::SERIALIZATION_CONTEXT_CACHEABILITY => new CacheableMetadata(),

This means this only works on Drupal >=8.5. So let's just hardcode the string rather than using the constant, since the constant won't be available. (Since this bit of the patch was originally written, this was officially marked as an internal implementation detail anyway, per jsonapi.api.php.)

#17 still failed on 8.4 because #2626924: Include processed text in normalizations: "text" field type's "processed" computed property should be non-internal and carry cacheability metadata only landed in 8.5. Fixing. Also fixed CS violations.

I made a mistake in #19 😅

(And apparently introduced two new CS violations 😅)

I'm still working on this. I should've assigned it to myself some time ago. Sorry. More updates soon.

Many of the JSON API integration tests include this:

  // @codingStandardsIgnoreStart
  /**
   * {@inheritdoc}
   */
  protected function getExpectedCacheContexts() {
    // @todo Uncomment first line, remove second line in https://www.drupal.org/project/jsonapi/issues/2940342.
//    return ['user.permissions'];
    return parent::getExpectedCacheContexts();
  }
  // @codingStandardsIgnoreEnd

They obviously refer to this issue. I copied those over from core's REST tests in #2930028: Comprehensive JSON API integration test coverage phase 1: for every entity type, individual resources only and #2932031: Comprehensive JSON API integration test coverage phase 2: for every entity type, complex use-cases. Because they were cacheability related, I pointed to this issue as being the one in which it should be solved. But the changes that this issue made so far are not actually those tests to fail … which means that either this issue should remove those @todos because they're inaccurate, or this issue should update the issue link.

So, let's dig in.

As of #2113345: Define a mechanism for custom link relationships, core's REST tests are using ['url.site', 'user.permissions'] as the default cache context expectation. Because that issue by default is generating Link response headers for all link relations for an entity type. And those headers contain absolute URLs, and hence need to vary by url.site. Okay, great.

Many config entity types only have a single "admin" permission for all possible operations. But some config entity types have per-operation permissions. In the latter case, no Link response headers will appear, because the user does not have the permission to access those other operations. Therefore in those cases, the url.site cache context should not be present.

And that is what those TODOs are about that I gave an example of. But in the land of JSON API, this is no longer true: JSON API does not expose all of an entity type's link relations via headers. But it does include a self link in every response. Therefore every JSON API response varies by the url.site cache context. Therefore it's safe to remove those @todos!

There are two special cases:

1. File: it's the one content entity type that got similar behavior as some config entity types, because it doesn't have any link relation. But in JSON API it too always has a self link, because it's a JSON API-provided/generated link.
2. ContentLanguageSettings: no link relations so no url.site in core REST, but it does always vary by interface language. In JSON API, it still varies by interface language, but there is always a self link and hence the url.site cache context is present

As of this patch there's zero remaining references to this issue in the code base!

So the patches so far have been using the pattern in Drupal core's serialization and rest modules. That pattern is:

1. oh, Symfony's serialization component is COMPLETELY unaware of cacheability, and doesn't care at all
2. So we have to fit in support for cacheability bubbling somehow.
3. The only way we can possibly do this without breaking BC is by using the $context that normalizers receive, and reserve a key in there, and bubble up via that.

By making JSON API use this same pattern, it's easy for the fixed/improved normalizers in Drupal 8.5 and later (for example #2626924: Include processed text in normalizations: "text" field type's "processed" computed property should be non-internal and carry cacheability metadata) to bubble up cacheability of normalizations, for both the REST and the JSON API module. So that's what the patch does so far.

However …

#2946968: Field-level cacheability metadata is lost was reported and unpublished because I needed to investigate that it wasn't a security issue. It isn't. It's the exact same bug as the one reported here. But it proposes a completely different fix.

That completely different fix points to some code that is obviously broken in \Drupal\jsonapi\Normalizer\EntityNormalizer::normalize():

    $output = new EntityNormalizerValue($normalizer_values, $context, $entity, $link_context);
    // Add the entity level cacheability metadata.
    $output->addCacheableDependency($entity);
    $output->addCacheableDependency($output);
    // Add the field level cacheability metadata.
    array_walk($normalizer_values, function ($normalizer_value) {
      if ($normalizer_value instanceof RefinableCacheableDependencyInterface) {
        $normalizer_value->addCacheableDependency($normalizer_value);
      }
    });

They pointed to the loop's logic being nonsensical (which it is). But also that $output->addCacheableDependency($output); makes no sense at all. And honestly, it also makes no sense to have $output->addCacheableDependency($entity); because the constructor of $output actually already received $entity…

More importantly though, this got me thinking about the approach this issue is taking. It's applying the core approach to JSON API's normalizers. But JSON API's normalizers are NOT like core's normalizers. Because JSON API's normalizers don't return arrays, but value objects. We had a big issue specifically about that about a month ago: #2923779: Normalizers violate NormalizerInterface.

Those value objects returned by normalizers are also specifically designed to carry cacheability! Which means that the approach used in this issue is not using one of the key features that the JSON API module's architecture was designed around! This makes no sense.

What we should be doing is:

1. use JSON API's intended architecture instead of using $context['cacheability'] at every layer like core REST
2. only use $context['cacheability'] at the boundary: at the point(s) where we transition from JSON API's normalizers to core's serialization module style normalizers — and don't bubble this up, but merge that data into the JSON API normalizer value object that is capable of carrying cacheability

Theoretically, we can do that in a separate issue. But then we're just further postponing this much-needed clean-up. The JSON API security release from last week was related to incorrect cacheability handling. The lack of clear/consistent handling of that is clearly the root cause here. So let's fix the root cause.

So, per #25, starting a new patch.

Starting with test coverage that we'll need to make pass but that should currently fail: the test coverage expectations changes in #16.

Note that we'll be first testing these patches against 8.6 only, because it's only in 8.6 that we have #2626924: Include processed text in normalizations: "text" field type's "processed" computed property should be non-internal and carry cacheability metadata.

The absolute minimum set of changes to implement #25 is this. This should make the BlockContentTest expectations that were updated in #26 pass tests.

Rather than making it the responsibility of the caller, it should be the responsibility of the value object itself to return the cacheability of the data it contains/depends on. And this is easily achievable!

Including the test coverage @gabesullice wrote at #5: https://www.drupal.org/files/issues/2940342-5.tests_only.patch. But without the CacheableNormalizer:: SERIALIZATION_CONTEXT_CACHEABILITY bits, since that's no longer in this patch.

The best way to prevent problems/regressions like this is by removing the architectural aspects that allow for these mistakes to happen:

1. the use of $context['cacheable_metadata'] — this allows one to not use the value objects they were designed to do: carry values to be absorbed into a larger structure at a later time, plus the cacheability metadata associated with those larger structures
2. the use of RefineableCacheableDependencyInterface — this allows one to go and modify the cacheability of constructed value objects at a later time, i.e. it makes the value objects mutable rather than immutable, making it much harder to figure out where/at what time the cacheability metadata is being specified (it can be modified at any time!)

Point 1 describes the original approach (until before #25). Point 2 is what allowed for the bug/problem being fixed in #27, and being properly fixed in #28.

If we take #28 one step further, then we prevent that problem altogether.

Significantly expanded the "proposed resolution" in the IS.

So the goal is to get rid of $context['cacheable_metadata'], and to get to that point, we must first ensure that every value object carries the cacheability for the data it contains (i.e. the data it depends on).

#28 already made EntityNormalizerValue self-contained. But it still is open for outside change: it is still mutable. Let's make it immutable by switching from RefinableCacheableDependencyInterface to CacheableDependencyInterface.

#31 made EntityNormalizerValue self-contained wrt cacheability.

Let's do the same for Field(Item)NormalizerValue(Interface). This:

1. updates FieldItemNormalizerValue, FieldItemNormalizerValueInterface to implement CacheableDependencyInterface, not RefinableCacheableDependencyInterface
2. moves the "capturing" of out-of-band bubbled cacheability by core's serialization system from FieldNormalizer to FieldItemNormalizer, because that's the actual level where we cross the boundary between JSON API's normalizers and core's
3. … sadly this also updates HttpExceptionNormalizer, because HttpExceptionNormalizerValue surprisingly is basically an alias for FieldNormalizerValue. Changing that is out-of-scope here, so making the minimum changes necessary.

Now we can also finally delete one of the most dangerous bits of code, which was simultaneously the culprit for the problem being fixed here, and a consequence of HEAD's dealing with cacheability-in-value-objects-but-not-really-because-they're-mutable.

So, all of this disappears from \Drupal\jsonapi\Normalizer\EntityNormalizer:

-  protected function serializeField($field, array $context, $format) {
-    /* @var \Drupal\Core\Field\FieldItemListInterface|\Drupal\jsonapi\Normalizer\Relationship $field */
-    // Continue if the current user does not have access to view this field.
-    $access = $field->access('view', $context['account'], TRUE);
-    $context['cacheable_metadata']->addCacheableDependency($access);
-    if ($field instanceof AccessibleInterface && !$access->isAllowed()) {
-      return (new NullFieldNormalizerValue())->addCacheableDependency($access);
-    }
-    /** @var \Drupal\jsonapi\Normalizer\Value\FieldNormalizerValue $output */
-    $output = $this->serializer->normalize($field, $format, $context);
-    if (!$output instanceof FieldNormalizerValueInterface) {
-      return new NullFieldNormalizerValue();
-    }
-    $is_relationship = $this->isRelationship($field);
-    $property_type = $is_relationship ? 'relationships' : 'attributes';
-    $output->setPropertyType($property_type);
-
-    if ($output instanceof RefinableCacheableDependencyInterface) {
-      // Add the cache dependency to the field level object because we want to
-      // allow the field normalizers to add extra cacheability metadata.
-      $output->addCacheableDependency($access);
-    }
-
-    return $output;
-  }

and instead we move that logic into \Drupal\jsonapi\Normalizer\FieldNormalizer:

  public function normalize($field, $format = NULL, array $context = []) {
    /* @var \Drupal\Core\Field\FieldItemListInterface $field */

    $access = $field->access('view', $context['account'], TRUE);
    $property_type = static::isRelationship($field) ? 'relationships' : 'attributes';

    if ($access->isAllowed()) {
      $normalized_field_items = $this->normalizeFieldItems($field, $format, $context);
      assert(Inspector::assertAll(function ($v) {
        return $v instanceof FieldItemNormalizerValue;
      }, $normalized_field_items));

      $cardinality = $field->getFieldDefinition()
        ->getFieldStorageDefinition()
        ->getCardinality();
      return new FieldNormalizerValue($access, $normalized_field_items, $cardinality, $property_type);
    }
    else {
      return new NullFieldNormalizerValue($access, $property_type);
    }
  }

So then then normalizing of fields becomes self-contained!

Now we ahve nearly identical cacheability merging logic in 3 places. We can make that logic shareable by introducing a trait.

And finally, some sad work-around fixes due to the fact that:

1. ConfigEntityNormalizer uses FieldNormalizerValue, even though config entities don't have fields
2. HttpExceptionNormalizer because HttpExceptionNormalizerValue surprisingly is basically an alias for FieldNormalizerValue (as mentioned in #32.3)
3. RelationshipItemNormalizer(Value) because they subclass FieldItemNormalizer(Value)

Fixing either of those more elegantly is definitely out of scope here: this is making the minimal changes necessary to allow those things to work as intended.

I was a bit too eager in #33, by simplifying EntityNormalizer, I also broke ConfigEntityNormalizer. Fixed.

Looks like I might have fixed #2930217: getRelated() not working when related entity field is empty accidentally, because Drupal\Tests\jsonapi\Functional\UserTest::testGetIndividual() is now failing because the roles relationship is now showing up, which was not the case before.

Updating the assertion.

Most of the remaining failures:

Error: Call to undefined method Drupal\jsonapi\Normalizer\Value\RelationshipItemNormalizerValue::addCacheableDependency()

… because \Drupal\jsonapi\Normalizer\RelationshipItemNormalizer::normalize() first constructs a RelationshipItemNormalizerValue value object and then modifies it, to set the includes, if any. By simply shifting that around so that the constructor already receives the includes (if any), we can remove all that complexity:

-      $normalizer_value->addCacheableDependency($entity_and_access['access']);
-      $normalizer_value->addCacheableDependency($included_normalizer_value);
-      // Add the cacheable dependency of the included item directly to the
-      // response cacheable metadata. This is similar to the flatten include
-      // data structure, instead of a content graph.
-      if (!empty($context['cacheable_metadata'])) {
-        $context['cacheable_metadata']->addCacheableDependency($normalizer_value);
-      }

(Again: rather than modifying a value object after-the-fact to add cacheability for data that it already contains, shifting that responsibility to the value object itself prevents an entire class of problems! And in doing so, we even removed two occurrences of $context['cacheable_metadata'].)

Those first two lines are what triggered the error in the tests. I had hoped to not also remove ::setIncludes() in this issue, but this forced me to do so anyway. As you can see, it's all related: all those setters/mutable value objects cause problems wrt cacheability!

In doing so, I noticed that 100% of the logic for includes lives in Field(Item)NormalizerValue, but 0% of the time do those contain any includes. In fact, they can't! The only time an include is even possible, is in Relationship(Item)NormalizerValue, which is a subclass of Field(Item)NormalizerValue. So, moved the little remaining logic in there.

As of #41, there's quite a bit of this:

Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-MISS
+UNCACHEABLE

It's related to:

1) Drupal\Tests\jsonapi\Kernel\Normalizer\JsonApiDocumentTopLevelNormalizerTest::testNormalize with data set #0 ('uid,field_tags,field_image')
Failed asserting that 0 is identical to -1.

/var/www/html/modules/contrib/jsonapi/tests/src/Kernel/Normalizer/JsonApiDocumentTopLevelNormalizerTest.php:285

2) Drupal\Tests\jsonapi\Kernel\Normalizer\JsonApiDocumentTopLevelNormalizerTest::testNormalize with data set #1 ('uid, field_tags, field_image')
Failed asserting that 0 is identical to -1.

Both have the same root cause (using NULL as a cacheable dependency, which results in max-age=0 being set, which makes the whole thing uncacheable). Simple fix.

I love how the immutability/stricter architecture seemingly is already helping us.

So, UserTest was failing because we were inappropriately granting access to the roles relationship. (That also means #39 was wrong — it actually uncovered a bug!)

Since #33, it's no longer \Drupal\jsonapi\Normalizer\EntityNormalizer::serializeField() that checks field access for every field. It's now FieldNormalizer. But … for entity reference fields, FieldNormalizer is never called! Then EntityReferenceFieldNormalizer() gets called. And EntityReferenceFieldNormalizer::normalize() was not yet updated to check field access. Fixing that fixed the last failure in UserTest, and also allowed me to revert #39. Great.

What's less great is that there's no good way to pass the access result's cacheability for a case of allowed access to an entity reference field ("relationship") onto the value object that eventually gets built, because the actual normalization happens in yet another normalizer. (EntityReferenceFieldNormalizer generates a Relationship object that then is normalized.)

Note that this is also already broken in HEAD, because:

-    // Continue if the current user does not have access to view this field.
-    $access = $field->access('view', $context['account'], TRUE);
…
-    /** @var \Drupal\jsonapi\Normalizer\Value\FieldNormalizerValue $output */
-    $output = $this->serializer->normalize($field, $format, $context);
-    if (!$output instanceof FieldNormalizerValueInterface) {
-      return new NullFieldNormalizerValue();
-    }
…
-    if ($output instanceof RefinableCacheableDependencyInterface) {
-      // Add the cache dependency to the field level object because we want to
-      // allow the field normalizers to add extra cacheability metadata.
-      $output->addCacheableDependency($access);
-    } 

The ::normalize() call returned a Relationship value object, and that is not an instance of RefinableCacheableDependencyInterface.

But fortunately there also was:

-    $context['cacheable_metadata']->addCacheableDependency($access);

… which just ignored the self-containedness of the value objects and just bubbled the access result's cacheability straight to the response. So there was no bug in practice, even though the code did not make sense.

Attached patch fixes all that:

1. updates EntityReferenceFieldNormalizer::normalize() to do field access checking, and work like FieldNormalizer::normalize() as much as possible
2. updates Relationship to implement CacheableDependencyInterface and accept the field access result, so that the access result cacheability can be transported over to the RelationshipNormalizerValue value object.
3. consequently, RelationshipNormalizerValue no longer has to hardcode an "allowed" access result in its call to the parent constructor
4. reverts the expectation changes in UserTest made in #33

This fixes the two Relationship(Item)NormalizerValueTests.

I was forced to also remove the testing test coverage that used nested FieldItemNormalizerValues. That failed horribly, because it ended up looking at the protected PHP object properties and interpreted those as values. Since the value object now carries cacheability, cacheability metadata was being included in the rasterization…
Not only does it make little sense, we also have conclusive proof that this is not used anywhere: the integration tests would have caught the problem long ago. The only occurrence of this was in this unit test!

In RelationshipNormalizerValueTest, there was a need to add cacheability to the constructor, which led to me adding explicit unit test coverage for the value object's cacheability reflecting that of its inputs, yay!

And this fixes the remaining unit tests:

1. EntityNormalizerValueTest needed cacheability in constructors, and ended up getting explicit test coverage for it too. Much like RelationshipNormalizerValueTest.
2. Same for FieldNormalizerValueTest.

I can't reproduce those failures locally :/

LOL! I've been rolling my patches wrong, sorry. The patch in #47 was identical to that in #45, the right patch was in #49. I've been creating patches without the last local commit… 😅 The interdiffs were correct though.

Here's what #49 should have been.

GREEN! Finally!

But it's still failling horribly on Drupal 8.4, because CacheableDependencyTrait is new in 8.5. So for now, let JSON API ship with its own equivalent.

Still fails on 8.4 for the same reasons the first patch iteration failed on 8.4. Quoting #19:

#17 still failed on 8.4 because #2626924: Include processed text in normalizations: "text" field type's "processed" computed property should be non-internal and carry cacheability metadata only landed in 8.5. Fixing.

Fixing CS violations.

And finally, lifting #23 from the previous patch iteration:

Many of the JSON API integration tests include this:

[…]

Oops, #52 introduced two new CS violations.

I now see that I didn't yet explain why I started working on this issue: because it's a hard blocker for #2883086: [PP-1] Port RequestHandler + ResourceResponseSubscriber improvements from REST module to JSON API. This is a hard blocker because without the fixes in this issue, it's too easy to introduce regressions in #2883086

Note that #2883086: [PP-1] Port RequestHandler + ResourceResponseSubscriber improvements from REST module to JSON API in turn is a blocker for other issues.

This issue is now ready for review.

Thanks for reviewing this massive patch! I hope that the incremental updates plus comments describing the "why" for each was helpful.

1. Correct.
2. ✔️
3. Correct.
4. Correct.
5. Correct.
6. Correct.
7. Correct.
8. ✔️ (That should've been exits obviously! Went with your suggestion though :))
9. Correct. And important to note: how we prevent it from persisting in the background, from one field item to the next. IOW: the unset() guarantees there can't be any side effects.
10. Correct.
11. Correct.
12. :)
13. That's exactly how I feel! Initially, there was nothing include-related in this issue/patch, but I was forced to deal with that here too. Because the previous setInclude() call was not dealing with cacheability correctly, and the whole point of this issue is that we now do deal with it correctly. So, postponing this issue on #2945093: Comprehensive JSON API integration test coverage phase 3: test JSON API-specific use cases: related/relationship routes, includes and sparse field sets.
14. Thanks! I spent some time wondering how this was supposed to work, glad to see my comments already helping.
15. I tried to change as little as possible. RelationshipItemNormalizerValue extends FieldItemNormalizerValue. The latter used to contain $this->include, and it was set to NULL by default. The old code in RelationshipNormalizer constructed a RelationshipItemNormalizerValue, then modified it. The new code is required to pass in an include immediately. To keep it behave the exact same way, I'm passing NULL. You're right that perhaps it should become NullFieldNormalizerValue. But there's definitely no need to carry cacheability: in theory it should associate url.query_args:include, but \Drupal\jsonapi\Normalizer\Value\JsonApiDocumentTopLevelNormalizerValue::__construct() is in the current design the one to add that. So, A) to keep things as much unchanged as possible, B) because there's no associated cacheability, the current solution of using NULL is fine/correct/preferable.
16. ✔️
17. Correct.
18. Correct.
19. ✔️
20. ✔️
21. Discussed in chat: we already have integration tests for it in \Drupal\Tests\jsonapi\Functional\CommentTest::getExpectedDocument() and \Drupal\Tests\jsonapi\Functional\TermTest::getExpectedDocument(). At which point Gabe said "Oh, nice, nevermind then."
22. Hah :P Yes, this is your test coverage code!

The blocking patch was committed in #2945093-50: Comprehensive JSON API integration test coverage phase 3: test JSON API-specific use cases: related/relationship routes, includes and sparse field sets! Would be great to have @e0ipso's explicit approval.

(#2945093 was only blocking because it helps ensure that this issue does not introduce regressions. #62 was already green, but I queued re-tests against both 8.4 and 8.6 now that #2945093-50 happened. If it still comes back green, that would proves this doesn't cause any regressions!)

After re-testing, it was green only on 8.4 (where there are no fields in core that bubble cacheability). On 8.6, the re-testing is failing 😞 … but that means that #2945093-50: Comprehensive JSON API integration test coverage phase 3: test JSON API-specific use cases: related/relationship routes, includes and sparse field sets is catching potential regressions wrt cacheability in sparse fieldset handling! 😀🎉

This is great!

What exactly is failing? Well, if a normalized field has certain cacheability associated with it, but that field is omitted (because not listed in a sparse fieldset), then of course that cacheability should also be absent. But the current test infrastructure assumes that any normalization of the entity always has the same cacheability. That's of course no longer true when sparse fieldsets come into play!

So, all we need to do, is refine our test infrastructure a little bit, to allow us to indicate we want the cacheability limited to a particular sparse fieldset, if any.

We need to do that for TermTest, CommentTest and BlockContentTest.

PHP 7/my local env lets me get away with the above, but testbot wisely requires me to add the optional parameter to all method overrides.

#68 should be green — and then conclusively proves that JSON API's sparse fieldset handling A) handles cacheability correctly, B) this patch does not cause any regressions in that area!

We can however add a small extra bit of test coverage to be even more certain.

Fixing CS violations.

#70: I contemplated the same thing :) Done!

After reviewing this I have very good impressions. This refocuses some concepts that have been patched several times one on top of the other for a while now. Great work @Wim Leers!

WHEW! So glad to see you say that :) ❤️

I am a little confused with some scope creep regarding includes and relationships. I see that some of the functionality has been altered, and I'm interested to learn the reasons before merging this. […]

I don't have any specific concerns, but a general feeling. Did you keep the related and relationship endpoints (with their different HTTP methods) in mind when doing the refactor?

Ok, let me walk you through that.

Yes, I kept all of JSON API's capabilities in mind. Quoting a bit from the IS:

4. To truly prevent these sorts of problems from reappearing in the future, we should also completely remove JSON API's use of $context['cacheable_metadata'], and rely solely on value objects.

   Sadly, we can't really complete this refactor yet because we lack solid test coverage for e.g. includes (this is being worked on in #2945093: Comprehensive JSON API integration test coverage phase 3: test JSON API-specific use cases: related/relationship routes, includes and sparse field sets). For example \Drupal\jsonapi\Normalizer\RelationshipItemNormalizer::normalize() specifically deals with includes and still uses $context['cacheable_metadata'].

   So, removing $context['cacheable_metadata'] is definitely out of scope for this issue. Created #2948666: Remove JSON API's use of $context['cacheable_metadata'] for that. This issue only refactors those bits that we do have great test coverage for: normalizing of fields and field items.

For example #60.13 and #62.13 also explicitly discussed includes. They explicitly say we should wait for the patch #2945093-50: Comprehensive JSON API integration test coverage phase 3: test JSON API-specific use cases: related/relationship routes, includes and sparse field sets to land, because the first part of that issue (committed by now, hence this was RTBC'd) explicitly adds test coverage for includes!

Up until #39, there was barely any change wrt includes/relationships. Starting in #41, there was an important refactor in RelationshipItemNormalizerValue. I was forced to do that because class RelationshipItemNormalizerValue extends FieldItemNormalizerValue: FieldItemNormalizerValue needed to be modified before, to fix the root problem properly (see e.g. #25). But because the relationship value objects build on top of the field ones, I now was required to fix those too! Doing that does make sense though: those relationship value objects also can carry cacheability, and hence are prone to similar bugs. By also enforcing the intended architecture for them, we can help prevent similar bugs there in the future.

That in turn forced me to do further fixes in #45 and so on.

If you have the chance, starting at #25 and then reading every comment + interdiff towards the end should give you a complete understanding of all the changes!

Long story short: the patches until before #25 were using $context['cacheable_metadata'] to fix the problem. But this meant we were still not yet fixing the root cause of the problem; it still meant we were not using JSON API's intended architecture. That intended architecture is: each level (entity, field, field item) gets normalized not into an array, but into a value object. That value object then allows the JSON API module to rasterize its data into multiple places into the final JSON API document. But that value object also carries the cacheability of the normalized data!
Sadly, the reality was that the value object rarely carried the intended cacheability (or even any at all). In many places, we worked around this by using $context['cacheability'], i.e. out-of-band bubbling of cacheability. In the places where we did put cacheability on the value object, we did so after the fact, i.e. after creating the value object. And this too resulted in severe bugs. Having the value objects receive information once and only once (i.e. making them immutable) makes everything more predictable. Because it forces us to first build child value objects and then pass them into the parent. The parent then can automatically inherit the cacheability of its child value objects … and the need for us to remember to manually set that cacheability on the parent object disappears.
Once I started on this path, I was forced to see it through for all value objects/all normalizers. That's why the patch is so big.

(Hah, that wasn't very short. But it does paint the overall picture!)

Hope this helps clarify things. If it doesn't, then I'm sorry I couldn't explain it better :( I propose we then set up a meeting to walk you through it, either during this weekend, or on Monday?

I was thinking more about the relationship and related endpoints. I am going to merge this after tagging a release for this week so we can have a full week of testing this if necessary before releasing it.

Sounds good!

Thanks again for some epic work here.

<3