ResourceResponse(Subscriber) + Dynamic Page Cache: making authenticated ResourceResponses significantly faster

ResourceResponseSubscriber this let me thinking ... isnt' that what \Symfony\Component\HttpKernel\KernelEvents::VIEW is designed to do?

* The VIEW event occurs when the return value of a controller
* is not a Response instance
.

Could this convert the response over?

+++ b/core/modules/rest/src/EventSubscriber/RestRouteSubscriber.php
@@ -0,0 +1,40 @@
+      if (strpos($name, 'rest.') === 0 && $route->hasOption('_admin_route')) {

Is there any there anything we can check for besides strpos($name, 'rest.') === 0? I guess modules are suppose to use the machine name for module as the start to all their routes?

What if we ever wanted to add an non-resource route to the REST module. Like some sort of config form. What if a custom module did this and didn't follow the rule about machine names?

What about $route->getDefault('_controller') == 'Drupal\rest\RequestHandler::handle'? I wish there was a better way we could tell if a route was REST resource.

Think this is close to RTBC. I TEST_ONLY patch would be good prove the ResourceResponse object is stored currently in the Dynamic Page Cache.

(Of course, ideally, AdminRouteSubscriber would only be touching _format: html routes! This again shows that Drupal 8 is not really API-first yet: many things are still HTML-centric.)

Right, but what is the reason we cannot do that? Can't we check whether format is set, and in case it doesn't support HTML we would not set the option ...

My point in #22 is no longer a concern because #24 removed RestRouteSubscriber.

The patch looks good to me but will let @dawehner RTBC if thinks it is good.

1. +++ b/core/modules/rest/src/EventSubscriber/ResourceResponseSubscriber.php
   @@ -196,8 +196,9 @@ protected function flattenResponse(ResourceResponseInterface $response) {
       * {@inheritdoc}
       */
      public static function getSubscribedEvents() {
   -    // Run shortly before \Drupal\Core\EventSubscriber\FinishResponseSubscriber.
   -    $events[KernelEvents::RESPONSE][] = ['onResponse', 5];
   +    // Run before \Drupal\dynamic_page_cache\EventSubscriber\DynamicPageCacheSubscriber
   +    // (priority 100), so that Dynamic Page Cache can cache flattened responses.
   +    $events[KernelEvents::RESPONSE][] = ['onResponse', 128];
        return $events;
   

   I agree for now just changing the priority is the right thing to do. Moving to the view event would be certainly way more of an effort.

2. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
   @@ -395,13 +399,34 @@ public function testGet() {
   +      $this->assertSame(['HIT'], $response->getHeader('X-Drupal-Dynamic-Cache'));
   +      // 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) {
   +        $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);
   +        }
   

   Nice test coverage

3. +++ b/core/modules/system/src/EventSubscriber/AdminRouteSubscriber.php
   @@ -15,14 +16,27 @@ class AdminRouteSubscriber extends RouteSubscriberBase {
       */
      protected function alterRoutes(RouteCollection $collection) {
   -    foreach ($collection->all() as $route) {
   -      if (strpos($route->getPath(), '/admin') === 0 && !$route->hasOption('_admin_route')) {
   +    foreach ($collection->all() as $name => $route) {
   +      if (strpos($route->getPath(), '/admin') === 0 && !$route->hasOption('_admin_route') && static::isHtmlRoute($route)) {
            $route->setOption('_admin_route', TRUE);
          }
        }
      }
    
      /**
   +   * Whether the given route is a HTML route.
   +   *
   +   * @param \Symfony\Component\Routing\Route $route
   +   *   The route to inspect.
   +   *
   +   * @return bool
   +   *   Whether the given route is a HTML route.
   +   */
   +  protected static function isHtmlRoute(Route $route) {
   +    return !$route->hasRequirement('_format') || in_array('html', explode('|', $route->getRequirement('_format')), TRUE);
   +  }
   +
   

   I was about to say: it'd be nice to have test coverage, but you know, here is an updated patch with it.

It works on my system :P

New test coverage for AdminRouteSubscriber looks good.

Woot! Thanks @Wim Leers and @dawehner for your work on this! RTBC

+++ b/core/modules/system/src/EventSubscriber/AdminRouteSubscriber.php
@@ -15,14 +16,27 @@ class AdminRouteSubscriber extends RouteSubscriberBase {
-      if (strpos($route->getPath(), '/admin') === 0 && !$route->hasOption('_admin_route')) {
+      if (strpos($route->getPath(), '/admin') === 0 && !$route->hasOption('_admin_route') && static::isHtmlRoute($route)) {

Can this be moved to a separate issue (even if it means postponing this one on that). I'd like it to have a dedicated commit in the commit history and a dedicated change record. Because this will introduce some (potentially desirable) side effects. For example, it will restrict AdminPathConfigEntityConverter and LanguageNegotiationUserAdmin to only operate on HTML routes. As well as similar code in contrib. It's not entirely obvious to me whether for those 2 examples if that's a desirable change or not: is the concept of "administration" and changing config overrides behavior and/or language negotiation based on whether you're "administering" a strictly HTML concept?

is the concept of "administration" and changing config overrides behavior and/or language negotiation based on whether you're "administering" a strictly HTML concept?

If the concept of non-HTML "administration" is still something that's meaningful to preserve, then the CR for the hunk in #34 could just indicate that the behavior of automatically registering routes with /admin/* paths as administrative routes is limited to only HTML routes. And that if someone wants a non-HTML route to be administrative, then they need to explicitly add the _admin_route option to the route themselves, regardless of that route's path. However, that kind of begs the question of why we want to make /admin/* paths have different defaults based on the format.

The reason #13 failed for MenuLinkContent and Shortcut is that their canonical URLs are admin routes because their URL path starts with /admin/…

Maybe that's a bug with those 2 entity classes? Does it make any sense that their HTML routes are administrative? Furthermore, the MenuLinkContent one ends in /edit, which is maybe also a bug?

+++ b/core/modules/rest/src/EventSubscriber/RestRouteSubscriber.php
@@ -0,0 +1,40 @@
+    foreach ($collection->all() as $name => $route) {
+      // Removes the '_admin_route' route option from each REST route that had
+      // it added by \Drupal\system\EventSubscriber\AdminRouteSubscriber.
+      if (strpos($name, 'rest.') === 0 && $route->hasOption('_admin_route')) {
+        $route->setOption('_admin_route', FALSE);
+      }
+    }

This documentation just tells what is happening, but it doesn't tell why this is done.

+++ b/core/modules/dynamic_page_cache/src/PageCache/ResponsePolicy/DenyAdminRoutes.php
@@ -6,9 +6,10 @@
- * Cache policy for routes with the '_admin_route' option set.
+ * Cache policy for HTML routes with the '_admin_route' option set.
  *
  * This policy rule denies caching of responses generated for admin routes,
  * because admin routes have very low cache hit ratios due to low traffic and

If the reason this policy rule exists is for the reason stated in the above PHPDoc, then I don't see why it makes sense to couple it to the HTML format. If we don't want to cache low hit ratio HTML responses, then why do we want to cache low hit ratio non-HTML responses? And if a particular resource has a low hit ratio in HTML, then why would it have a high hit ratio in non-HTML? Maybe there are logical answers to that, but if so, they need to be documented.

However, I don't think the problem is with this policy, but rather with the entity types that define incorrect canonical link templates. Therefore, I opened #2873529: BlockContent, MenuLinkContent, and Shortcut define incorrect canonical links. If we think that analysis is correct, but don't want to hold up this issue on resolving that one, then perhaps it makes sense to meanwhile write this patch's tests to not expect caching for the entity types with canonical resources at admin routes?

The PHPDoc you quoted is specifically for the admin area of a Drupal site, i.e. where site builders go to configure things. Clearly, retrieving entities via REST is something different entirely.

I agree that REST GET requests for entities don't make sense to treat as administrative routes. A way to express that would be to change EntityResource::getBaseRoute() to add the '_admin_route' => FALSE route option. I don't know whether that should be just for GET methods or if it should be added to all methods. I also don't know if it should be broadened to all REST resources by default, not just entities. If so, then that could be moved to ResourceBase::getBaseRoute(). I suggest opening a separate issue to figure out the scope we want for that.

I also think that if we want to change DenyAdminRoutes to be scoped to just HTML routes, we should do it in a separate issue. But I'm not convinced of the logic for doing so, other than to work around the bug that some routes are incorrectly marked as administrative.

I still think that the scope of this issue should be to change our tests to not expect caching for the problematic entity types. And then change those tests again when one of the other issues is correspondingly fixed.

I still think that the scope of this issue should be to change our tests to not expect caching for the problematic entity types. And then change those tests again when one of the other issues is correspondingly fixed.

I think I agree, mostly because caching of those is less important than most of content entities, which are on not admin routes. So even if we don't support caching for those, we still have a major benefit.

I opened #2877528: Change Dynamic Page Cache's response policy from "deny admin routes" to "deny html admin routes", and changed the @todos in this patch to reference that issue in addition to #2874938: AdminRouteSubscriber must only mark HTML routes as administrative.

As a side-note, #2877136: dynamic_page_cache.module has an incorrect and misleading @file doc tripped me up while digging into all that.

As this is only a comment change, leaving at RTBC. I'll commit this tomorrow unless objections are raised before then.

Ticking the credit box for @tedbow for his reviews.

+++ b/core/modules/rest/src/EventSubscriber/ResourceResponseSubscriber.php
@@ -196,8 +196,9 @@ protected function flattenResponse(ResourceResponseInterface $response) {
-    // Run shortly before \Drupal\Core\EventSubscriber\FinishResponseSubscriber.
-    $events[KernelEvents::RESPONSE][] = ['onResponse', 5];
+    // Run before \Drupal\dynamic_page_cache\EventSubscriber\DynamicPageCacheSubscriber
+    // (priority 100), so that Dynamic Page Cache can cache flattened responses.
+    $events[KernelEvents::RESPONSE][] = ['onResponse', 128];

Do we need a CR for this? Normally, I don't think changing the priority of a subscriber automatically requires a CR, but I'm curious if any of you know of a reason to provide one in this case.

Pushed to 8.4.x. #2807501: ResourceResponse::$responseData is serialized, but is never used: unserializing it on every Page Cache hit is a huge performance penalty was tagged for release notes mention, so tagging this one too.

+++ 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?