Add revision ID to "jsonapi_normalizations" cache keys

Woah, good catch!

1. +++ b/core/modules/jsonapi/src/EventSubscriber/ResourceObjectNormalizationCacher.php
   @@ -97,9 +97,7 @@ public function saveOnTerminate(ResourceObject $object, array $normalization_par
   -    $this->toCache[$key] = [$object, $normalization_parts];
   +    $this->toCache[] = [$object, $normalization_parts];
   

   Why is removing the key okay?

   Because the key was not used anyway!

   ✅

2. +++ b/core/modules/jsonapi/src/EventSubscriber/ResourceObjectNormalizationCacher.php
   @@ -110,8 +108,7 @@ public function saveOnTerminate(ResourceObject $object, array $normalization_par
   -      list($object, $normalization_parts) = $value;
   -      $this->set($object, $normalization_parts);
   +      $this->set(...$value);
   

   Woah! First time I'm seeing this 🤓

3. +++ b/core/modules/jsonapi/src/EventSubscriber/ResourceObjectNormalizationCacher.php
   @@ -162,9 +159,16 @@ protected function set(ResourceObject $object, array $normalization_parts) {
   +    if ($resource_type->isVersionable()) {
   +      $keys[] = $object->getVersionIdentifier();
   +    }
   +
        return [
          '#cache' => [
   -        'keys' => [$object->getResourceType()->getTypeName(), $object->getId()],
   +        'keys' => $keys,
   

   👍 This makes total sense! This is the root of the bug described by the issue title.

4. +++ b/core/modules/jsonapi/src/Normalizer/JsonApiDocumentTopLevelNormalizer.php
   @@ -268,6 +269,13 @@ protected function normalizeOmissionsLinks(OmittedData $omissions, $format, arra
   +        // The collection of revisions of a single entity will have the same
   +        // link so we need to make it unique.
   +        $href = $error['links']['via']['href'];
   +        if (!isset($hrefs_counts[$href])) {
   +          $hrefs_counts[$href] = 0;
   +        }
   +        $hrefs_counts[$href]++;
            // JSON:API links cannot be arrays and the spec generally favors link
            // relation types as keys. 'item' is the right link relation type, but
            // we need multiple values. To do that, we generate a meaningless,
   @@ -275,9 +283,9 @@ protected function normalizeOmissionsLinks(OmittedData $omissions, $format, arra
   
   @@ -275,9 +283,9 @@ protected function normalizeOmissionsLinks(OmittedData $omissions, $format, arra
            // random salt and the link href. This ensures that the key is non-
            // deterministic while letting use deduplicate the links by their
            // href. The salt is *not* used for any cryptographic reason.
   -        $link_key = 'item--' . static::getLinkHash($link_hash_salt, $error['links']['via']['href']);
   +        $link_key = 'item--' . static::getLinkHash($link_hash_salt . $hrefs_counts[$href], $href);
   

   🤔 This is over my head right now 😅 Why are we appending a count to a hash salt?

Thanks @BR0keN, i think this is a smart addition and will make life easier later on.

I've gone through the code and there is a few small things that i think would need work. More importantly, there is quite a few test failures that seem to point at some empty array values. We do need to get to the bottom of that.

1. +++ b/core/modules/jsonapi/src/EventSubscriber/ResourceObjectNormalizationCacher.php
   @@ -97,9 +97,7 @@ public function saveOnTerminate(ResourceObject $object, array $normalization_par
   +    $this->toCache[] = [$object, $normalization_parts];
   

   Removing the key here feels a bit scary, but i can't seem to find a way this would create issues.

2. +++ b/core/modules/jsonapi/src/EventSubscriber/ResourceObjectNormalizationCacher.php
   @@ -110,8 +108,7 @@ public function saveOnTerminate(ResourceObject $object, array $normalization_par
   +      $this->set(...$value);
   

   I understand this seems nicer, but i think it is wise to move this change to a folow up issue to minimize the change surface of this patch

3. +++ b/core/modules/jsonapi/src/EventSubscriber/ResourceObjectNormalizationCacher.php
   @@ -162,9 +159,16 @@ protected function set(ResourceObject $object, array $normalization_parts) {
   +        'keys' => $keys,
   

   Another reason for #3252424: Follow-up: ResourceObjectNormalizationCacher should get tags from ResourceObject instead of building them i think.

4. +++ b/core/modules/jsonapi/src/Normalizer/EntityAccessDeniedHttpExceptionNormalizer.php
   @@ -52,8 +53,10 @@ protected function buildErrorObjects(HttpException $exception) {
   +        $errors[0]['links']['via']['meta']['resourceId'] = $entity_uuid;
   
   +++ b/core/modules/jsonapi/src/Normalizer/JsonApiDocumentTopLevelNormalizer.php
   @@ -265,24 +265,32 @@ protected function normalizeOmissionsLinks(OmittedData $omissions, $format, arra
   +        $link_key = 'item--' . static::getLinkHash($via['meta']['resourceVersion'] ?? random_bytes(8), $via['href']);
   

   Shouldn't we keep this outside the loop? Don't think there is a real reason to move it inline and calculate it in the loop instead of outside.

1. Yeah, no concrete worries, just a feeling :)
2. This might come back to bite us, but keep it for now.
3. Yes.
4. Ah yeah, good one sorry.

Wegarding the tests, we are quite explicit in what we expect as output, so if we change the output, the tests will fail. Not sure how we can go around that. I remember #3036593: Add 'drupal_internal__target_id' to JSON:API representation of entity reference fields, because that can't be retrieved from the target resource for target entity types without corresponding resources, was a little bit of effort as seen in the commit.

I've also pinged you onm slack

We had some discussion on slack regarding this issue:

br0ken 1 hour ago
Yes, you’re right. The main problem is with the assertResourceErrorResponse which is called from lots of palces

br0ken 1 hour ago
null because the entity is not revisionable or has no revision

br0ken 1 hour ago
To incorporate the changes to the assertResourceErrorResponse we have to somehow pass revision info to it. It’s relatively possible for individual requests, but I can’t see a way of doing that for collections. (edited)

Björn Brala (bbrala) 1 hour ago
Hmm, that is a good point.

Björn Brala (bbrala) 1 hour ago
I'm trying to think, but need to see the full error for a collection. Another day where those generic test methods are making it harder to solve :x

br0ken 1 hour ago
Would it make sense to not precisely check the output but, let’s say, the count of omissions only?

Björn Brala (bbrala) 1 hour ago
Hmm

Björn Brala (bbrala) 1 hour ago
I might be on to something.

Björn Brala (bbrala) 1 hour ago
When getting resource links it uses ResourceResponseTestTrait::getResourceLink

Björn Brala (bbrala) 1 hour ago
Should't that be aware if versioning was turned on?

Björn Brala (bbrala) 1 hour ago
But that doesnt solve the meta info..

Björn Brala (bbrala) 35 minutes ago
"Would it make sense to not precisely check the output but, let’s say, the count of omissions only?"
That could work, but i have a feeling we can actually build the correct expected response in getExpectedCollectionResponse, we should really have the information needed...

Björn Brala (bbrala) 34 minutes ago
And no wonder these tests are slow when getExpectedCollectionResponse does singe requests for the collection and then merges them xD

Björn Brala (bbrala) 26 minutes ago
I think i found it.

Björn Brala (bbrala) 24 minutes ago
toCollectionResourceResponse is building the response. It recieves the correct reponses with the proper version information. Then i comes to if (!empty($response_document['errors'])) { which adds the error information, calling static::errorsToOmittedObject. This function should be extended with the meta we expect. The current loop is:

Björn Brala (bbrala) 24 minutes ago

foreach ($errors as $error) {
      $omitted['links']['item--' . substr(Crypt::hashBase64($error['links']['via']['href']), 0, 7)] = [
        'href' => $error['links']['via']['href'],
        'meta' => [
          'detail' => $error['detail'],
          'rel' => 'item',
        ],
      ];
    }

Björn Brala (bbrala) 23 minutes ago
Which means missing version info

br0ken 22 minutes ago
It’s never executed for individual requests though

Björn Brala (bbrala) 20 minutes ago
Nope, that is mostly getExpectedDocument, which is manual right? So for example NodeTest::getExpectedDocument. And exactly what i mentioned in the comment on the issue?

Björn Brala (bbrala) 20 minutes ago
Dont think we can really get around defining what we expect as a returning document.

Björn Brala (bbrala) 19 minutes ago
For single's

br0ken 17 minutes ago
I’m lost, to be frank. Will try to re-read the thread

Björn Brala (bbrala) 15 minutes ago
No worries, gonna try and suggest a change for the collections

br0ken 14 minutes ago
Still not getting it, sorry.
I think that the way to make it work is to somehow pass entities during the call assertResourceErrorResponse. That way the expected document can be built. (edited)

Björn Brala (bbrala) 14 minutes ago
For the failing tests that are sourced from the getExpectedDocument in the test classes themselves the array needs to be changed

Björn Brala (bbrala) 12 minutes ago
I'm smoking right now, I'll try and finish a fix for the error information missing the meta information of the version and show the code
:+1:
1

Björn Brala (bbrala) 6 minutes ago
Yay, the collection test is now green locally.

br0ken 4 minutes ago
Hmm, that’s great! Was I over-dramatic estimating the complexity?

Björn Brala (bbrala) 3 minutes ago
git diff modules/jsonapi/tests/src/Functional/ResourceResponseTestTrait.php

diff --git a/core/modules/jsonapi/tests/src/Functional/ResourceResponseTestTrait.php b/core/modules/jsonapi/tests/src/Functional/ResourceResponseTestTrait.php
index 993ff5b57b..11c4080564 100644
--- a/core/modules/jsonapi/tests/src/Functional/ResourceResponseTestTrait.php
+++ b/core/modules/jsonapi/tests/src/Functional/ResourceResponseTestTrait.php
@@ -594,13 +594,17 @@ protected static function errorsToOmittedObject(array $errors) {
       ],
     ];
     foreach ($errors as $error) {
-      $omitted['links']['item--' . substr(Crypt::hashBase64($error['links']['via']['href']), 0, 7)] = [
+      $errorInfo = [
         'href' => $error['links']['via']['href'],
         'meta' => [
           'detail' => $error['detail'],
           'rel' => 'item',
         ],
       ];
+      if(isset($error['links']['via']['meta'])) {
+        $errorInfo['meta'] += $error['links']['via']['meta'];
+      }
+      $omitted['links']['item--' . substr(Crypt::hashBase64($error['links']['via']['href']), 0, 7)] = $errorInfo;
     }
     return $omitted;
   }

Björn Brala (bbrala) 3 minutes ago
I had to use the debugger in phpunit to find out how the hell this was done. So ehh, its just overly complex sometimes how tests are done.

Björn Brala (bbrala) 2 minutes ago
The isset part might be better of being explicit in the keys it copies perhaps. But im not sure.

Björn Brala (bbrala) 1 minute ago
This change should at least reduce the amount of errors. But as you can see in this commit you will still need to go through the tests and adjust the getExpectedDocument functions to include the revision info where it is relevant.

Lets have a look after this run. I think this will probably only be updating getExpectedDocument for a few classes.

I'm going to try some things in a seperate branch to see if i can clean up how tests is configured.

Opening merge request of my changes to have tests run without changing the branch by @BR0ken

Ok, the changes I made to how the tests are run are a lot cleaner I think. The following is needed still:

1. The IS needs updating. There is meta information added, this needs to be added to the issue summary.
2. We also need to make sure the meta tag is added to the right endpoints. Is is added to the correct endpoints now? It seems for example that /jsonapi/user--user/relations/revision_id doesn't provide that information. Is that correct? It really does need to be clear when it is added.

After that i'll have a look if the tests are correct in respect to the proposed change, but for that I really want to know explicitly what is added.

Also; added 'Needs change record' since this change will need a change record.

Another thing i thought if, why should the revisionId be included in the meta if it is NULL, wouldn't this mean, there is no revision and shouldn't it then just be omitted?

Personally I find mr's easier to work with :)

See next comment

Attached an attempt at creating a patch from MR 2417 that is compatible with Drupal 10.3.x

Attached a patch from MR 2417 that is compatible with Drupal 10.4.x