Problem/Motivation

Reported by @effulgentsia at #2827797-55: ResourceResponse(Subscriber) + Dynamic Page Cache: making authenticated ResourceResponses significantly faster:

+++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
@@ -395,13 +409,44 @@ public function testGet() {
+        // Assert that Dynamic Page Cache did not store a ResourceResponse object,
+        // which needs serialization after every cache hit. Instead, it should
+        // contain a flattened response. Otherwise performance suffers.
+        // @see \Drupal\rest\EventSubscriber\ResourceResponseSubscriber::flattenResponse()
+        $cache_items = $this->container->get('database')
+          ->query("SELECT cid, data FROM {cache_dynamic_page_cache} WHERE cid LIKE :pattern", [
+            ':pattern' => '%[route]=rest.%',
+          ])
+          ->fetchAllAssoc('cid');
+        foreach ($cache_items as $cid => $cache_item) {

This test seems a little brittle. If the cid pattern changes in the future, this test will return an empty array and the foreach will get silently skipped, so the test will still pass. Perhaps there should be a followup to improve on this? Do we really have a situation where we don't know the exact $cid to check for?

We need the wildcard, to fetch the multiple relevant cache items: there's a cache redirect cache item, and an "actual" cache item.

If I add:

 .../tests/src/Functional/EntityResource/EntityResourceTestBase.php | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
index 5bcf5fe..04a66f6 100644
--- a/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
+++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
@@ -437,6 +437,7 @@ public function testGet() {
           ->fetchAllAssoc('cid');
         foreach ($cache_items as $cid => $cache_item) {
           $cached_data = unserialize($cache_item->data);
+          var_dump($cached_data);
           if (!isset($cached_data['#cache_redirect'])) {
             $cached_response = $cached_data['#response'];
             $this->assertNotInstanceOf(ResourceResponseInterface::class, $cached_response);

And then run NodeJsonBasicAuthTest, you get this output:

array(2) {
  ["#cache_redirect"]=>
  bool(true)
  ["#cache"]=>
  array(5) {
    ["keys"]=>
    array(1) {
      [0]=>
      string(8) "response"
    }
    ["contexts"]=>
    array(4) {
      [0]=>
      string(14) "request_format"
      [1]=>
      string(5) "route"
      [2]=>
      string(8) "url.site"
      [3]=>
      string(16) "user.permissions"
    }
    ["tags"]=>
    array(2) {
      [0]=>
      string(32) "config:rest.resource.entity.node"
      [1]=>
      string(6) "node:1"
    }
    ["max-age"]=>
    int(-1)
    ["bin"]=>
    string(18) "dynamic_page_cache"
  }
}
array(5) {
  ["#markup"]=>
  string(0) ""
  ["#attached"]=>
  string(0) ""
  ["#cache"]=>
  array(3) {
    ["contexts"]=>
    array(4) {
      [0]=>
      string(14) "request_format"
      [1]=>
      string(5) "route"
      [2]=>
      string(8) "url.site"
      [3]=>
      string(16) "user.permissions"
    }
    ["tags"]=>
    array(2) {
      [0]=>
      string(32) "config:rest.resource.entity.node"
      [1]=>
      string(6) "node:1"
    }
    ["max-age"]=>
    int(-1)
  }
  ["#cache_properties"]=>
  array(1) {
    [0]=>
    string(9) "#response"
  }
  ["#response"]=>
  object(Drupal\Core\Cache\CacheableResponse)#11508 (7) {
    ["headers"]=>
    object(Symfony\Component\HttpFoundation\ResponseHeaderBag)#1223 (5) {
      ["computedCacheControl":protected]=>
      array(2) {
        ["no-cache"]=>
        bool(true)
        ["private"]=>
        bool(true)
      }
      ["cookies":protected]=>
      array(0) {
      }
      ["headerNames":protected]=>
      array(3) {
        ["cache-control"]=>
        string(13) "Cache-Control"
        ["link"]=>
        string(4) "Link"
        ["content-type"]=>
        string(12) "Content-Type"
      }
      ["headers":protected]=>
      array(3) {
        ["cache-control"]=>
        array(1) {
          [0]=>
          string(17) "no-cache, private"
        }
        ["link"]=>
        array(5) {
          [0]=>
          string(35) "<http://d8/node/1>; rel="canonical""
          [1]=>
          string(78) "<http://d8/node/1/delete>; rel="https://drupal.org/link-relations/delete-form""
          [2]=>
          string(40) "<http://d8/node/1/edit>; rel="edit-form""
          [3]=>
          string(51) "<http://d8/node/1/revisions>; rel="version-history""
          [4]=>
          string(68) "<http://d8/node/1>; rel="https://drupal.org/link-relations/revision""
        }
        ["content-type"]=>
        array(1) {
          [0]=>
          string(16) "application/json"
        }
      }
      ["cacheControl":protected]=>
      array(0) {
      }
    }
    ["content":protected]=>
    string(800) "{"nid":[{"value":1}],"uuid":[{"value":"7e0ce3b9-082e-45e6-8ede-489ab77277d8"}],"vid":[{"value":1}],"langcode":[{"value":"en"}],"type":[{"target_id":"camelids","target_type":"node_type","target_uuid":"a4050c00-df34-4d74-92f4-6e08e73c8dfb"}],"revision_timestamp":[{"value":123456789}],"revision_uid":[{"target_id":2,"target_type":"user","target_uuid":"068851de-a726-4984-9d42-8578bd427ac2","url":"\/user\/2"}],"revision_log":[],"status":[{"value":true}],"title":[{"value":"Llama"}],"uid":[{"target_id":2,"target_type":"user","target_uuid":"068851de-a726-4984-9d42-8578bd427ac2","url":"\/user\/2"}],"created":[{"value":123456789}],"changed":[{"value":1494525772}],"promote":[{"value":true}],"sticky":[{"value":false}],"revision_translation_affected":[{"value":true}],"default_langcode":[{"value":true}]}"
    ["version":protected]=>
    string(3) "1.0"
    ["statusCode":protected]=>
    int(200)
    ["statusText":protected]=>
    string(2) "OK"
    ["charset":protected]=>
    NULL
    ["cacheabilityMetadata":protected]=>
    object(Drupal\Core\Cache\CacheableMetadata)#7539 (3) {
      ["cacheContexts":protected]=>
      array(2) {
        [0]=>
        string(8) "url.site"
        [1]=>
        string(16) "user.permissions"
      }
      ["cacheTags":protected]=>
      array(2) {
        [0]=>
        string(32) "config:rest.resource.entity.node"
        [1]=>
        string(6) "node:1"
      }
      ["cacheMaxAge":protected]=>
      int(-1)
    }
  }
}

Proposed resolution

Because there's always a cache redirect, assert the number of cache items.

Remaining tasks

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#5 2877778-5.patch1.42 KBwim leers
#5 interdiff.txt1.49 KBwim leers
#2 2877778-2.patch862 byteswim leers

Comments

Wim Leers created an issue. See original summary.

wim leers’s picture

Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new862 bytes

Updated IS with explanation.

Attached patch makes it more strict.

tedbow’s picture

Ok looking at this as someone who doesn't know the cache system as well this is what I think the intention of the test is.
for this:

    $this->assertSame(2, count($cache_items));
    foreach ($cache_items as $cid => $cache_item) {
      $cached_data = unserialize($cache_item->data);
      if (!isset($cached_data['#cache_redirect'])) {
         $cached_response = $cached_data['#response'];
         $this->assertNotInstanceOf(ResourceResponseInterface::class, $cached_response);
         $this->assertInstanceOf(CacheableResponseInterface::class, $cached_response);
      }
    }

It looks like there should be 2 items in $cache_items and 1 of those items should meet these conditions

  1. !isset($cached_data['#cache_redirect'])
  2. $this->assertNotInstanceOf(ResourceResponseInterface::class, $cached_response);
  3. $this->assertInstanceOf(CacheableResponseInterface::class, $cached_response);

But if neither of the items passes this condition

  • !isset($cached_data['#cache_redirect'])

Then it will the test will also pass. I assuming that would never happen but again that is because I don't know that cache system as well.

Would it make sense to put flag
$cacheable_response_found = FALSE;
Before the foreach and set it to TRUE if we pass the if in the loop.
Then assert cacheable_response_found === TRUE after the loop?

wim leers’s picture

Assigned: Unassigned » wim leers
Status: Needs review » Needs work

#3 good call!

wim leers’s picture

Assigned: wim leers » Unassigned
Status: Needs work » Needs review
StatusFileSize
new1.49 KB
new1.42 KB
tedbow’s picture

Status: Needs review » Reviewed & tested by the community

@wim thanks for the updates.

This looks good now, RTBC!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 14a1f0c and pushed to 8.4.x. Thanks!

Conflcts on 8.3.x so not backported.

  • alexpott committed 14a1f0c on 8.4.x
    Issue #2877778 by Wim Leers, tedbow: Harden test EntityResource +...
wim leers’s picture

Status: Fixed » Closed (fixed)

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