Problem/Motivation

The page cache is now enabled per default in Drupal so we should add proper cache tags on REST HTTP responses. Otherwise a configuration change will not trigger a page cache clear correctly resulting in stale 404 responses for example.

Similar for entities exposed via REST: we should add the entity cache tags on GET requests.

Proposed resolution

Add a rest config cache tag to response obejct we are creating. Add a page cache web test to make sure the cache tags are there and the cache is cleared correctly when the config changes. Add the entity cache tags in the EntityResource implementation.

Remaining tasks

Review patch.

User interface changes

none.

API changes

none.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

klausi’s picture

klausi’s picture

Issue summary: View changes
klausi’s picture

Title: REST responses should have peoper cache tags » REST responses should have proper cache tags
pwolanin’s picture

Priority: Normal » Critical
Issue tags: +Needs tests

I think this is critical, since changes to REST config will not appear in cached responses. Needs both the fix and tests

The simple fix from Wim & Berdir:

$response->headers->set('X-Drupal-Cache-Tags', implode(' ', $this->container->get('config.factory')->get('rest.settings')->getCacheTags()));
klausi’s picture

Priority: Critical » Major

This is inconvenient, but a simple cache clear will solve the problem for users, so no need to call this critical.

pwolanin’s picture

Status: Active » Needs review
Issue tags: +D8 Accelerate Dev Days
FileSize
677 bytes

Let's see if this breaks anything. still needs tests.

klausi’s picture

Assigned: Unassigned » klausi

Going to work on a test case.

klausi’s picture

FileSize
3.3 KB

klausi opened a new pull request for this issue.

klausi’s picture

Just posting an intermediary patch here for Wim to see.

Status: Needs review » Needs work

The last submitted patch, 8: 2471473.patch, failed testing.

klausi’s picture

Status: Needs work » Needs review
FileSize
3.76 KB

klausi pushed some commits to the pull request.

For an interdiff please see the list of recent commits.

klausi’s picture

Issue summary: View changes

Found the problem: we use a custom HTTP request method RESTTestBase, which does not have the $this->refreshVariables(); that drupalGet() does in the usual web tests. So the static cache of the cache tags invalidation service was never cleared, which means that the page cache entry for the response was never invalidated.

catch’s picture

Priority: Major » Critical

Looks like this is blocking #2364011: [meta] External caches mix up response formats on URLs where content negotiation is in use so bumping status back up to critical. Agreed it probably wouldn't be critical in its own right though.

pwolanin’s picture

Looks ok, but the wording of this comment is a little unclear:

+    // Trigger a config save which should clear the page cache, so we should
+    // still get a cache miss for the same request.

So, the save doesn't clear the page cache per se as much as invalidating the cache tag and all caches that use it including the page cache.

Also we *now* not *still* get a cache miss? the last one was a hit.

klausi’s picture

FileSize
3.76 KB

klausi pushed some commits to the pull request.

For an interdiff please see the list of recent commits.

klausi’s picture

Right, that comment was outdated, fixed now.

klausi’s picture

FileSize
5.09 KB

klausi pushed some commits to the pull request.

For an interdiff please see the list of recent commits.

klausi’s picture

Issue summary: View changes

And while we are at it we should also add the entity cache tags.

pwolanin’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

Looks good. Entity cache tags is an important fix.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

This issue addresses a critical bug and is allowed per https://www.drupal.org/core/beta-changes. Committed bf9a2f3 and pushed to 8.0.x. Thanks!

diff --git a/core/modules/rest/src/Tests/PageCacheTest.php b/core/modules/rest/src/Tests/PageCacheTest.php
index 70888a1..9cf2859 100644
--- a/core/modules/rest/src/Tests/PageCacheTest.php
+++ b/core/modules/rest/src/Tests/PageCacheTest.php
@@ -7,9 +7,6 @@
 
 namespace Drupal\rest\Tests;
 
-use Drupal\Component\Serialization\Json;
-use Drupal\rest\Tests\RESTTestBase;
-
 /**
  * Tests page caching for REST GET requests.
  *

Remove the unused use.

  • alexpott committed bf9a2f3 on 8.0.x
    Issue #2471473 by klausi, pwolanin: REST responses should have proper...
Wim Leers’s picture

Issue tags: +D8 cacheability

Awesome!

Wim Leers’s picture

Status: Fixed » Closed (fixed)

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