Issue #2626298 by marthinal, borisson_, Wim Leers, dawehner: REST module must...

Issue #2626298 by marthinal, borisson_, Wim Leers, dawehner: REST module must cache only responses to GET requests

core/modules/rest/src/ModifiedResourceResponse.php

+<?php
+
+namespace Drupal\rest;
+
+use Symfony\Component\HttpFoundation\Response;
+
+/**
+ * A response that does not contain cacheability metadata.
+ *
+ * Used when resources are modified by a request: responses to unsafe requests
+ * (POST/PATCH/DELETE) can never be cached.
+ *
+ * @see \Drupal\rest\ResourceResponse
+ */
+class ModifiedResourceResponse extends Response implements ResourceResponseInterface {
+
+  use ResourceResponseTrait;
+
+  /**
+   * Constructor for ModifiedResourceResponse objects.
+   *
+   * @param mixed $data
+   *   Response data that should be serialized.
+   * @param int $status
+   *   The response status code.
+   * @param array $headers
+   *   An array of response headers.
+   */
+  public function __construct($data = NULL, $status = 200, $headers = []) {
+    $this->responseData = $data;
+    parent::__construct('', $status, $headers);
+  }
+
+}

core/modules/rest/src/Plugin/rest/resource/EntityResource.php

@@ -6,6 +6,7 @@
 use Drupal\Core\Entity\EntityStorageException;
 use Drupal\rest\Plugin\ResourceBase;
 use Drupal\rest\ResourceResponse;
+use Drupal\rest\ModifiedResourceResponse;
 use Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException;
 use Symfony\Component\HttpKernel\Exception\BadRequestHttpException;
 use Symfony\Component\HttpKernel\Exception\HttpException;
@@ -34,7 +35,7 @@ class EntityResource extends ResourceBase {
    * @param \Drupal\Core\Entity\EntityInterface $entity
    *   The entity object.
    *
-   * @return \Drupal\rest\ResourceResponse
+   * @return \Drupal\rest\ModifiedResourceResponse
    *   The response containing the entity with its accessible fields.
    *
    * @throws \Symfony\Component\HttpKernel\Exception\HttpException
@@ -108,11 +109,10 @@ public function post(EntityInterface $entity = NULL) {
       $this->logger->notice('Created entity %type with ID %id.', array('%type' => $entity->getEntityTypeId(), '%id' => $entity->id()));
 
       // 201 Created responses return the newly created entity in the response
-      // body.
+      // body. These responses are not cacheable, so we add no cacheability
+      // metadata here.
       $url = $entity->urlInfo('canonical', ['absolute' => TRUE])->toString(TRUE);
-      $response = new ResourceResponse($entity, 201, ['Location' => $url->getGeneratedUrl()]);
-      // Responses after creating an entity are not cacheable, so we add no
-      // cacheability metadata here.
+      $response = new ModifiedResourceResponse($entity, 201, ['Location' => $url->getGeneratedUrl()]);
       return $response;
     }
     catch (EntityStorageException $e) {
@@ -168,7 +168,7 @@ public function patch(EntityInterface $original_entity, EntityInterface $entity
       $this->logger->notice('Updated entity %type with ID %id.', array('%type' => $original_entity->getEntityTypeId(), '%id' => $original_entity->id()));
 
       // Update responses have an empty body.
-      return new ResourceResponse(NULL, 204);
+      return new ModifiedResourceResponse(NULL, 204);
     }
     catch (EntityStorageException $e) {
       throw new HttpException(500, 'Internal Server Error', $e);
@@ -195,7 +195,7 @@ public function delete(EntityInterface $entity) {
       $this->logger->notice('Deleted entity %type with ID %id.', array('%type' => $entity->getEntityTypeId(), '%id' => $entity->id()));
 
       // Delete responses have an empty body.
-      return new ResourceResponse(NULL, 204);
+      return new ModifiedResourceResponse(NULL, 204);
     }
     catch (EntityStorageException $e) {
       throw new HttpException(500, 'Internal Server Error', $e);

core/modules/rest/src/RequestHandler.php

@@ -2,6 +2,7 @@
 
 namespace Drupal\rest;
 
+use Drupal\Core\Cache\CacheableResponseInterface;
 use Drupal\Core\Render\RenderContext;
 use Drupal\Core\Routing\RouteMatchInterface;
 use Symfony\Component\DependencyInjection\ContainerAwareInterface;
@@ -98,7 +99,7 @@ public function handle(RouteMatchInterface $route_match, Request $request) {
       return new Response($content, $e->getStatusCode(), $headers);
     }
 
-    return $response instanceof ResourceResponse ?
+    return $response instanceof ResourceResponseInterface ?
       $this->renderResponse($request, $response, $serializer, $format) :
       $response;
   }
@@ -124,7 +125,7 @@ public function csrfToken() {
    *
    * @param \Symfony\Component\HttpFoundation\Request $request
    *   The request object.
-   * @param \Drupal\rest\ResourceResponse $response
+   * @param \Drupal\rest\ResourceResponseInterface $response
    *   The response from the REST resource.
    * @param \Symfony\Component\Serializer\SerializerInterface $serializer
    *   The serializer to use.
@@ -137,22 +138,29 @@ public function csrfToken() {
    * @todo Add test coverage for language negotiation contexts in
    *   https://www.drupal.org/node/2135829.
    */
-  protected function renderResponse(Request $request, ResourceResponse $response, SerializerInterface $serializer, $format) {
+  protected function renderResponse(Request $request, ResourceResponseInterface $response, SerializerInterface $serializer, $format) {
     $data = $response->getResponseData();
-    $context = new RenderContext();
-    $output = $this->container->get('renderer')
-      ->executeInRenderContext($context, function () use ($serializer, $data, $format) {
-        return $serializer->serialize($data, $format);
-      });
-    $response->setContent($output);
-    if (!$context->isEmpty()) {
-      $response->addCacheableDependency($context->pop());
-    }
 
+    if ($response instanceof CacheableResponseInterface) {
+      $context = new RenderContext();
+      $output = $this->container->get('renderer')
+        ->executeInRenderContext($context, function () use ($serializer, $data, $format) {
+          return $serializer->serialize($data, $format);
+        });
+
+      if (!$context->isEmpty()) {
+        $response->addCacheableDependency($context->pop());
+      }
+
+      // Add rest settings config's cache tags.
+      $response->addCacheableDependency($this->container->get('config.factory')
+        ->get('rest.settings'));
+    }
+    else {
+      $output = $serializer->serialize($data, $format);
+    }
+    $response->setContent($output);
     $response->headers->set('Content-Type', $request->getMimeType($format));
-    // Add rest settings config's cache tags.
-    $response->addCacheableDependency($this->container->get('config.factory')
-      ->get('rest.settings'));
 
     return $response;
   }

core/modules/rest/src/ResourceResponse.php

@@ -13,17 +13,13 @@
  * our response data. $content implies that the provided data must either be a
  * string or an object with a __toString() method, which is not a requirement
  * for data used here.
+ *
+ * @see \Drupal\rest\ModifiedResourceResponse
  */
-class ResourceResponse extends Response implements CacheableResponseInterface {
+class ResourceResponse extends Response implements CacheableResponseInterface, ResourceResponseInterface {
 
   use CacheableResponseTrait;
-
-  /**
-   * Response data that should be serialized.
-   *
-   * @var mixed
-   */
-  protected $responseData;
+  use ResourceResponseTrait;
 
   /**
    * Constructor for ResourceResponse objects.
@@ -40,14 +36,4 @@ public function __construct($data = NULL, $status = 200, $headers = array()) {
     parent::__construct('', $status, $headers);
   }
 
-  /**
-   * Returns response data that should be serialized.
-   *
-   * @return mixed
-   *   Response data that should be serialized.
-   */
-  public function getResponseData() {
-    return $this->responseData;
-  }
-
 }

core/modules/rest/src/ResourceResponseInterface.php

+<?php
+
+namespace Drupal\rest;
+
+/**
+ * Defines a common interface for resource responses.
+ */
+interface ResourceResponseInterface {
+
+  /**
+   * Returns response data that should be serialized.
+   *
+   * @return mixed
+   *   Response data that should be serialized.
+   */
+  public function getResponseData();
+
+}

core/modules/rest/src/ResourceResponseTrait.php

+<?php
+
+namespace Drupal\rest;
+
+
+trait ResourceResponseTrait {
+
+  /**
+   * Response data that should be serialized.
+   *
+   * @var mixed
+   */
+  protected $responseData;
+
+  /**
+   * Returns response data that should be serialized.
+   *
+   * @return mixed
+   *   Response data that should be serialized.
+   */
+  public function getResponseData() {
+    return $this->responseData;
+  }
+
+}

core/modules/rest/src/Tests/PageCacheTest.php

@@ -2,6 +2,9 @@
 
 namespace Drupal\rest\Tests;
 
+use Drupal\Core\Url;
+use Drupal\system\Tests\Cache\AssertPageCacheContextsAndTagsTrait;
+
 /**
  * Tests page caching for REST GET requests.
  *
@@ -9,6 +12,8 @@
  */
 class PageCacheTest extends RESTTestBase {
 
+  use AssertPageCacheContextsAndTagsTrait;
+
   /**
    * Modules to install.
    *
@@ -16,22 +21,59 @@ class PageCacheTest extends RESTTestBase {
    */
   public static $modules = array('hal', 'rest', 'entity_test');
 
+  /**
+   * The 'serializer' service.
+   *
+   * @var \Symfony\Component\Serializer\Serializer
+   */
+  protected $serializer;
+
+  /**
+   * {@inheritdoc}
+   */
+  protected function setUp() {
+    parent::setUp();
+    // Get the 'serializer' service.
+    $this->serializer = $this->container->get('serializer');
+  }
+
   /**
    * Tests that configuration changes also clear the page cache.
    */
   public function testConfigChangePageCache() {
-    $this->enableService('entity:entity_test', 'GET');
     // Allow anonymous users to issue GET requests.
-    $permissions = $this->entityPermissions('entity_test', 'view');
-    $permissions[] = 'restful get entity:entity_test';
-    user_role_grant_permissions('anonymous', $permissions);
+    user_role_grant_permissions('anonymous', ['view test entity', 'restful get entity:entity_test']);
+
+    $this->enableService('entity:entity_test', 'POST');
+    $permissions = [
+      'administer entity_test content',
+      'restful post entity:entity_test',
+      'restful get entity:entity_test',
+      'restful patch entity:entity_test',
+      'restful delete entity:entity_test',
+    ];
+    $account = $this->drupalCreateUser($permissions);
 
-    // Create an entity programmatically.
+    // Create an entity and test that the response from a post request is not
+    // cacheable.
     $entity = $this->entityCreate('entity_test');
     $entity->set('field_test_text', 'custom cache tag value');
-    $entity->save();
+    $serialized = $this->serializer->serialize($entity, $this->defaultFormat);
+    // Log in for creating the entity.
+    $this->drupalLogin($account);
+    $this->httpRequest('entity/entity_test', 'POST', $serialized, $this->defaultMimeType);
+    $this->assertResponse(201);
+
+    if ($this->getCacheHeaderValues('x-drupal-cache')) {
+      $this->fail('Post request is cached.');
+    }
+    $this->drupalLogout();
+
+    $url = Url::fromUri('internal:/entity_test/1?_format=' . $this->defaultFormat);
+
     // Read it over the REST API.
-    $this->httpRequest($entity->urlInfo()->setRouteParameter('_format', $this->defaultFormat), 'GET', NULL, $this->defaultMimeType);
+    $this->enableService('entity:entity_test', 'GET');
+    $this->httpRequest($url, 'GET', NULL, $this->defaultMimeType);
     $this->assertResponse(200, 'HTTP response code is correct.');
     $this->assertHeader('x-drupal-cache', 'MISS');
     $this->assertCacheTag('config:rest.settings');
@@ -39,7 +81,7 @@ public function testConfigChangePageCache() {
     $this->assertCacheTag('entity_test_access:field_test_text');
 
     // Read it again, should be page-cached now.
-    $this->httpRequest($entity->urlInfo()->setRouteParameter('_format', $this->defaultFormat), 'GET', NULL, $this->defaultMimeType);
+    $this->httpRequest($url, 'GET', NULL, $this->defaultMimeType);
     $this->assertResponse(200, 'HTTP response code is correct.');
     $this->assertHeader('x-drupal-cache', 'HIT');
     $this->assertCacheTag('config:rest.settings');
@@ -49,12 +91,48 @@ public function testConfigChangePageCache() {
     // Trigger a config save which should clear the page cache, so we should get
     // a cache miss now for the same request.
     $this->config('rest.settings')->save();
-    $this->httpRequest($entity->urlInfo()->setRouteParameter('_format', $this->defaultFormat), 'GET', NULL, $this->defaultMimeType);
+    $this->httpRequest($url, 'GET', NULL, $this->defaultMimeType);
     $this->assertResponse(200, 'HTTP response code is correct.');
     $this->assertHeader('x-drupal-cache', 'MISS');
     $this->assertCacheTag('config:rest.settings');
     $this->assertCacheTag('entity_test:1');
     $this->assertCacheTag('entity_test_access:field_test_text');
+
+    // Log in for deleting / updating entity.
+    $this->drupalLogin($account);
+
+    // Test that updating an entity is not cacheable.
+    $this->enableService('entity:entity_test', 'PATCH');
+
+    // Create a second stub entity for overwriting a field.
+    $patch_values['field_test_text'] = [0 => [
+      'value' => 'patched value',
+      'format' => 'plain_text',
+    ]];
+    $patch_entity = $this->container->get('entity_type.manager')
+      ->getStorage('entity_test')
+      ->create($patch_values);
+    // We don't want to overwrite the UUID.
+    $patch_entity->set('uuid', NULL);
+    $serialized = $this->container->get('serializer')
+      ->serialize($patch_entity, $this->defaultFormat);
+
+    // Update the entity over the REST API.
+    $this->httpRequest($url, 'PATCH', $serialized, $this->defaultMimeType);
+    $this->assertResponse(204);
+
+    if ($this->getCacheHeaderValues('x-drupal-cache')) {
+      $this->fail('Patch request is cached.');
+    }
+
+    // Test that the response from a delete request is not cacheable.
+    $this->enableService('entity:entity_test', 'DELETE');
+    $this->httpRequest($url, 'DELETE');
+    $this->assertResponse(204);
+
+    if ($this->getCacheHeaderValues('x-drupal-cache')) {
+      $this->fail('Patch request is cached.');
+    }
   }
 
 }