Problem/Motivation

When responding to a GET request for a comment entity with a parent comment Drupal detects leaked cache metadata. Causing the following error:

The controller result claims to be providing relevant cache metadata, but leaked metadata was detected. Please ensure you are not rendering content too early. Returned object class: Drupal\\jsonapi\\ResourceResponse.

This is caused by the rdf module in core/modules/rdf/rdf.module:L244-246:

if ($comment->hasParentComment()) {
  $comment->rdf_data['pid_uri'] = $comment->getParentComment()->toUrl()->toString();
}

The lines above it have already been corrected in #2631774: Impossible to update Comment entity with REST (HTTP PATCH): bundle field not allowed to be updated, but EntityNormalizer::denormalize() requires it to:

// The current function is a storage level hook, so avoid to bubble
// bubbleable metadata, because it can be outside of a render context.
$comment->rdf_data['entity_uri'] = $entity->toUrl()->toString(TRUE)->getGeneratedUrl();

Presumably, this was simply overlooked and the same error is also present in the REST module.

Proposed resolution

Use the same fix as #2631774 did for the remaining lines in rdf_comment_storage_load.

Remaining tasks

Regression test
Patch
Review
Commit

User interface changes

None.

API changes

None.

Data model changes

None.

Release notes snippet

None.

CommentFileSizeAuthor
#9 3053827-9.patch2.56 KBgabesullice
#2 3053827-2-combined.patch2.52 KBgabesullice
#2 3053827-2-fix.patch670 bytesgabesullice
#2 3053827-2-test-only.patch1.87 KBgabesullice

Comments

gabesullice created an issue. See original summary.

gabesullice’s picture

gabesullice
Primary language English
commented
StatusFileSize
new 3053827-2-test-only.patch1.87 KB
new 3053827-2-fix.patch670 bytes
new 3053827-2-combined.patch2.52 KB
gabesullice’s picture

gabesullice
Primary language English
commented
Status: Active » Needs review
gabesullice’s picture

gabesullice
Primary language English
commented
Issue summary: View changes

The last submitted patch, 2: 3053827-2-test-only.patch, failed testing. View results

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: +D8 cacheability

Wow, nice catch! 👏

+++ b/core/modules/jsonapi/tests/src/Functional/JsonApiRegressionTest.php
@@ -905,4 +905,43 @@ public function testMapFieldTypeNormalizationFromIssue3040590() {
+  public function testLeakedCacheMetadataViaRdfFromIssue3053827() {

Nit: "viaRdf" should be changed to something else :) Nope, this is correct, because it's caused by the RDF module 😮

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: Leaked cache metadata detected when using JSON:API to GET a threaded comment » Leaked cache metadata detected when using JSON:API to GET a threaded comment when RDF module is installed
Related issues: +#2631774: Impossible to update Comment entity with REST (HTTP PATCH): bundle field not allowed to be updated, but EntityNormalizer::denormalize() requires it
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

Needs a reroll.

gabesullice’s picture

gabesullice
Primary language English
commented
Status: Needs work » Reviewed & tested by the community
StatusFileSize
new 3053827-9.patch2.56 KB

Rerolled.

  • larowlan committed 21b1b01 on 8.8.x 
    Issue #3053827 by gabesullice: Leaked cache metadata detected when using...
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Reviewed & tested by the community » Fixed 
+++ b/core/modules/jsonapi/tests/src/Functional/JsonApiRegressionTest.php
@@ -953,4 +953,43 @@ public function testRecursionDetectedWhenResponseContainsViolationsFrom3042124()
+   * @see https://www.drupal.org/project/drupal/issues/3053827

nit:I don't think we need this comment, git blame will tell us the issue, we don't normally reference closed issues in the code - so I removed on commit

diff --git a/core/modules/jsonapi/tests/src/Functional/JsonApiRegressionTest.php b/core/modules/jsonapi/tests/src/Functional/JsonApiRegressionTest.php
index 4f5fb99c14..fde4ab22b6 100644
--- a/core/modules/jsonapi/tests/src/Functional/JsonApiRegressionTest.php
+++ b/core/modules/jsonapi/tests/src/Functional/JsonApiRegressionTest.php
@@ -955,8 +955,6 @@ public function testRecursionDetectedWhenResponseContainsViolationsFrom3042124()

   /**
    * Ensure that child comments can be retrieved via JSON:API.
-   *
-   * @see https://www.drupal.org/project/drupal/issues/3053827
    */
   public function testLeakedCacheMetadataViaRdfFromIssue3053827() {
     $this->assertTrue($this->container->get('module_installer')->install(['comment', 'rdf'], TRUE), 'Installed modules.');

There are a lot of existing ones in that test class from before the module came to core, can you open a follow-up to remove them?

Thanks

Committed 21b1b01 and pushed to 8.8.x. Thanks!

c/p as b25d3ca0a6 and pushed to 8.7.x

  • larowlan committed b25d3ca on 8.7.x 
    Issue #3053827 by gabesullice: Leaked cache metadata detected when using...
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

That’s a pattern I established to make it easier to correlate regressions with d.o issues. I know it’s a new pattern, but core actually does this too in BigPipeRegressionTest.

Note that removing those lines you asked to be removed won’t have git histories pointing to issues, since JSON:API commit history wasn’t brought into Drupal Core.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.