Commit 0f3f2792 authored by alexpott's avatar alexpott
Browse files

Issue #2807501 by Wim Leers, dawehner, tstoeckler:...

Issue #2807501 by Wim Leers, dawehner, tstoeckler: ResourceResponse::$responseData is serialized, but is never used: unserializing it on every Page Cache hit is a huge performance penalty
parent 5d973454
......@@ -28,3 +28,8 @@ services:
logger.channel.rest:
parent: logger.channel_base
arguments: ['rest']
rest.resource_response.subscriber:
class: Drupal\rest\EventSubscriber\ResourceResponseSubscriber
tags:
- { name: event_subscriber }
arguments: ['@serializer', '@renderer', '@current_route_match']
<?php
namespace Drupal\rest\EventSubscriber;
use Drupal\Core\Cache\CacheableResponse;
use Drupal\Core\Cache\CacheableResponseInterface;
use Drupal\Core\Render\RenderContext;
use Drupal\Core\Render\RendererInterface;
use Drupal\Core\Routing\RouteMatchInterface;
use Drupal\rest\ResourceResponseInterface;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpFoundation\Response;
use Symfony\Component\HttpKernel\Event\FilterResponseEvent;
use Symfony\Component\HttpKernel\KernelEvents;
use Symfony\Component\EventDispatcher\EventSubscriberInterface;
use Symfony\Component\Serializer\SerializerInterface;
/**
* Response subscriber that serializes and removes ResourceResponses' data.
*/
class ResourceResponseSubscriber implements EventSubscriberInterface {
/**
* The serializer.
*
* @var \Symfony\Component\Serializer\SerializerInterface
*/
protected $serializer;
/**
* The renderer.
*
* @var \Drupal\Core\Render\RendererInterface
*/
protected $renderer;
/**
* The current route match.
*
* @var \Drupal\Core\Routing\RouteMatchInterface
*/
protected $routeMatch;
/**
* Constructs a ResourceResponseSubscriber object.
*
* @param \Symfony\Component\Serializer\SerializerInterface $serializer
* The serializer.
* @param \Drupal\Core\Render\RendererInterface $renderer
* The renderer.
* @param \Drupal\Core\Routing\RouteMatchInterface $route_match
* The current route match.
*/
public function __construct(SerializerInterface $serializer, RendererInterface $renderer, RouteMatchInterface $route_match) {
$this->serializer = $serializer;
$this->renderer = $renderer;
$this->routeMatch = $route_match;
}
/**
* Serializes ResourceResponse responses' data, and removes that data.
*
* @param \Symfony\Component\HttpKernel\Event\FilterResponseEvent $event
* The event to process.
*/
public function onResponse(FilterResponseEvent $event) {
$response = $event->getResponse();
if (!$response instanceof ResourceResponseInterface) {
return;
}
$request = $event->getRequest();
$format = $this->getResponseFormat($this->routeMatch, $request);
$this->renderResponseBody($request, $response, $this->serializer, $format);
$event->setResponse($this->flattenResponse($response));
}
/**
* Determines the format to respond in.
*
* Respects the requested format if one is specified. However, it is common to
* forget to specify a request format in case of a POST or PATCH. Rather than
* simply throwing an error, we apply the robustness principle: when POSTing
* or PATCHing using a certain format, you probably expect a response in that
* same format.
*
* @param \Drupal\Core\Routing\RouteMatchInterface $route_match
* The current route match.
* @param \Symfony\Component\HttpFoundation\Request $request
* The current request.
*
* @return string
* The response format.
*/
public function getResponseFormat(RouteMatchInterface $route_match, Request $request) {
$route = $route_match->getRouteObject();
$acceptable_request_formats = $route->hasRequirement('_format') ? explode('|', $route->getRequirement('_format')) : [];
$acceptable_content_type_formats = $route->hasRequirement('_content_type_format') ? explode('|', $route->getRequirement('_content_type_format')) : [];
$acceptable_formats = $request->isMethodSafe() ? $acceptable_request_formats : $acceptable_content_type_formats;
$requested_format = $request->getRequestFormat();
$content_type_format = $request->getContentType();
// If an acceptable format is requested, then use that. Otherwise, including
// and particularly when the client forgot to specify a format, then use
// heuristics to select the format that is most likely expected.
if (in_array($requested_format, $acceptable_formats)) {
return $requested_format;
}
// If a request body is present, then use the format corresponding to the
// request body's Content-Type for the response, if it's an acceptable
// format for the request.
elseif (!empty($request->getContent()) && in_array($content_type_format, $acceptable_content_type_formats)) {
return $content_type_format;
}
// Otherwise, use the first acceptable format.
elseif (!empty($acceptable_formats)) {
return $acceptable_formats[0];
}
// Sometimes, there are no acceptable formats, e.g. DELETE routes.
else {
return NULL;
}
}
/**
* Renders a resource response body.
*
* Serialization can invoke rendering (e.g., generating URLs), but the
* serialization API does not provide a mechanism to collect the
* bubbleable metadata associated with that (e.g., language and other
* contexts), so instead, allow those to "leak" and collect them here in
* a render context.
*
* @param \Symfony\Component\HttpFoundation\Request $request
* The request object.
* @param \Drupal\rest\ResourceResponseInterface $response
* The response from the REST resource.
* @param \Symfony\Component\Serializer\SerializerInterface $serializer
* The serializer to use.
* @param string|null $format
* The response format, or NULL in case the response does not need a format,
* for example for the response to a DELETE request.
*
* @todo Add test coverage for language negotiation contexts in
* https://www.drupal.org/node/2135829.
*/
protected function renderResponseBody(Request $request, ResourceResponseInterface $response, SerializerInterface $serializer, $format) {
$data = $response->getResponseData();
// If there is data to send, serialize and set it as the response body.
if ($data !== NULL) {
$context = new RenderContext();
$output = $this->renderer
->executeInRenderContext($context, function () use ($serializer, $data, $format) {
return $serializer->serialize($data, $format);
});
if ($response instanceof CacheableResponseInterface && !$context->isEmpty()) {
$response->addCacheableDependency($context->pop());
}
$response->setContent($output);
$response->headers->set('Content-Type', $request->getMimeType($format));
}
}
/**
* Flattens a fully rendered resource response.
*
* Ensures that complex data structures in ResourceResponse::getResponseData()
* are not serialized. Not doing this means that caching this response object
* requires unserializing the PHP data when reading this response object from
* cache, which can be very costly, and is unnecessary.
*
* @param \Drupal\rest\ResourceResponseInterface $response
* A fully rendered resource response.
*
* @return \Drupal\Core\Cache\CacheableResponse|\Symfony\Component\HttpFoundation\Response
* The flattened response.
*/
protected function flattenResponse(ResourceResponseInterface $response) {
$final_response = ($response instanceof CacheableResponseInterface) ? new CacheableResponse() : new Response();
$final_response->setContent($response->getContent());
$final_response->setStatusCode($response->getStatusCode());
$final_response->setProtocolVersion($response->getProtocolVersion());
$final_response->setCharset($response->getCharset());
$final_response->headers->add($response->headers->all());
if ($final_response instanceof CacheableResponseInterface) {
$final_response->addCacheableDependency($response->getCacheableMetadata());
}
return $final_response;
}
/**
* {@inheritdoc}
*/
public static function getSubscribedEvents() {
$events[KernelEvents::RESPONSE][] = ['onResponse'];
return $events;
}
}
......@@ -2,10 +2,9 @@
namespace Drupal\rest;
use Drupal\Core\Cache\CacheableResponseInterface;
use Drupal\Core\DependencyInjection\ContainerInjectionInterface;
use Drupal\Core\Entity\EntityStorageInterface;
use Drupal\Core\Cache\CacheableResponseInterface;
use Drupal\Core\Render\RenderContext;
use Drupal\Core\Routing\RouteMatchInterface;
use Symfony\Component\DependencyInjection\ContainerAwareInterface;
use Symfony\Component\DependencyInjection\ContainerAwareTrait;
......@@ -14,10 +13,11 @@
use Symfony\Component\HttpFoundation\Response;
use Symfony\Component\HttpKernel\Exception\UnsupportedMediaTypeHttpException;
use Symfony\Component\Serializer\Exception\UnexpectedValueException;
use Symfony\Component\Serializer\SerializerInterface;
/**
* Acts as intermediate request forwarder for resource plugins.
*
* @see \Drupal\rest\EventSubscriber\ResourceResponseSubscriber
*/
class RequestHandler implements ContainerAwareInterface, ContainerInjectionInterface {
......@@ -129,118 +129,13 @@ public function handle(RouteMatchInterface $route_match, Request $request) {
}
// Invoke the operation on the resource plugin.
$format = $this->getResponseFormat($route_match, $request);
$response = call_user_func_array(array($resource, $method), array_merge($parameters, array($unserialized, $request)));
return $response instanceof ResourceResponseInterface ?
$this->renderResponse($request, $response, $serializer, $format, $resource_config) :
$response;
}
/**
* Determines the format to respond in.
*
* Respects the requested format if one is specified. However, it is common to
* forget to specify a request format in case of a POST or PATCH. Rather than
* simply throwing an error, we apply the robustness principle: when POSTing
* or PATCHing using a certain format, you probably expect a response in that
* same format.
*
* @param \Drupal\Core\Routing\RouteMatchInterface $route_match
* The current route match.
* @param \Symfony\Component\HttpFoundation\Request $request
* The current request.
*
* @return string
* The response format.
*/
protected function getResponseFormat(RouteMatchInterface $route_match, Request $request) {
$route = $route_match->getRouteObject();
$acceptable_request_formats = $route->hasRequirement('_format') ? explode('|', $route->getRequirement('_format')) : [];
$acceptable_content_type_formats = $route->hasRequirement('_content_type_format') ? explode('|', $route->getRequirement('_content_type_format')) : [];
$acceptable_formats = $request->isMethodSafe() ? $acceptable_request_formats : $acceptable_content_type_formats;
$requested_format = $request->getRequestFormat();
$content_type_format = $request->getContentType();
// If an acceptable format is requested, then use that. Otherwise, including
// and particularly when the client forgot to specify a format, then use
// heuristics to select the format that is most likely expected.
if (in_array($requested_format, $acceptable_formats)) {
return $requested_format;
}
// If a request body is present, then use the format corresponding to the
// request body's Content-Type for the response, if it's an acceptable
// format for the request.
elseif (!empty($request->getContent()) && in_array($content_type_format, $acceptable_content_type_formats)) {
return $content_type_format;
}
// Otherwise, use the first acceptable format.
elseif (!empty($acceptable_formats)) {
return $acceptable_formats[0];
}
// Sometimes, there are no acceptable formats, e.g. DELETE routes.
else {
return NULL;
}
}
/**
* Renders a resource response.
*
* Serialization can invoke rendering (e.g., generating URLs), but the
* serialization API does not provide a mechanism to collect the
* bubbleable metadata associated with that (e.g., language and other
* contexts), so instead, allow those to "leak" and collect them here in
* a render context.
*
* @param \Symfony\Component\HttpFoundation\Request $request
* The request object.
* @param \Drupal\rest\ResourceResponseInterface $response
* The response from the REST resource.
* @param \Symfony\Component\Serializer\SerializerInterface $serializer
* The serializer to use.
* @param string|null $format
* The response format, or NULL in case the response does not need a format,
* for example for the response to a DELETE request.
* @param \Drupal\rest\RestResourceConfigInterface $resource_config
* The resource config.
*
* @return \Drupal\rest\ResourceResponse
* The altered response.
*
* @todo Add test coverage for language negotiation contexts in
* https://www.drupal.org/node/2135829.
*/
protected function renderResponse(Request $request, ResourceResponseInterface $response, SerializerInterface $serializer, $format, RestResourceConfigInterface $resource_config) {
$data = $response->getResponseData();
if ($response instanceof CacheableResponseInterface) {
// Add rest config's cache tags.
$response->addCacheableDependency($resource_config);
}
// If there is data to send, serialize and set it as the response body.
if ($data !== NULL) {
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());
}
}
else {
$output = $serializer->serialize($data, $format);
}
$response->setContent($output);
$response->headers->set('Content-Type', $request->getMimeType($format));
}
return $response;
}
......
......@@ -2,7 +2,6 @@
namespace Drupal\Tests\rest\Kernel;
use Drupal\Component\Serialization\Json;
use Drupal\Core\Entity\EntityStorageInterface;
use Drupal\Core\Routing\RouteMatch;
use Drupal\KernelTests\KernelTestBase;
......@@ -10,8 +9,6 @@
use Drupal\rest\RequestHandler;
use Drupal\rest\ResourceResponse;
use Drupal\rest\RestResourceConfigInterface;
use Prophecy\Argument;
use Prophecy\Prophecy\MethodProphecy;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\Routing\Route;
......@@ -48,11 +45,9 @@ public function setUp() {
}
/**
* Assert some basic handler method logic.
*
* @covers ::handle
*/
public function testBaseHandler() {
public function testHandle() {
$request = new Request();
$route_match = new RouteMatch('test', new Route('/rest/test', ['_rest_resource_config' => 'restplugin'], ['_format' => 'json']));
......@@ -91,249 +86,6 @@ public function testBaseHandler() {
$this->assertEquals($response, $handler_response);
}
/**
* Test that given structured data, the request handler will serialize it.
*
* @dataProvider providerTestSerialization
* @covers ::handle
*/
public function testSerialization($data, $expected_response = FALSE) {
$request = new Request();
$route_match = new RouteMatch('test', new Route('/rest/test', ['_rest_resource_config' => 'restplugin'], ['_format' => 'json']));
$resource = $this->prophesize(StubRequestHandlerResourcePlugin::class);
// Mock the configuration.
$config = $this->prophesize(RestResourceConfigInterface::class);
$config->getResourcePlugin()->willReturn($resource->reveal());
$config->getCacheContexts()->willReturn([]);
$config->getCacheTags()->willReturn([]);
$config->getCacheMaxAge()->willReturn(12);
$this->entityStorage->load('restplugin')->willReturn($config->reveal());
$response = new ResourceResponse($data);
$resource->get(NULL, $request)
->willReturn($response);
$handler_response = $this->requestHandler->handle($route_match, $request);
// Content is a serialized version of the data we provided.
$this->assertEquals($expected_response !== FALSE ? $expected_response : json_encode($data), $handler_response->getContent());
}
public function providerTestSerialization() {
return [
// The default data for \Drupal\rest\ResourceResponse.
[NULL, ''],
[''],
['string'],
['Complex \ string $%^&@ with unicode ΑΒΓΔΕΖΗΘΙΚΛΜΝΞΟΣὨ'],
[[]],
[['test']],
[['test' => 'foobar']],
[TRUE],
[FALSE],
// @todo Not supported. https://www.drupal.org/node/2427811
// [new \stdClass()],
// [(object) ['test' => 'foobar']],
];
}
/**
* @covers ::getResponseFormat
*
* Note this does *not* need to test formats being requested that are not
* accepted by the server, because the routing system would have already
* prevented those from reaching RequestHandler.
*
* @param string[] $methods
* The HTTP methods to test.
* @param string[] $supported_formats
* The supported formats for the REST route to be tested.
* @param string|false $request_format
* The value for the ?_format URL query argument, if any.
* @param string[] $request_headers
* The request headers to send, if any.
* @param string|null $request_body
* The request body to send, if any.
* @param string|null $expected_response_content_type
* The expected MIME type of the response, if any.
* @param string $expected_response_content
* The expected content of the response.
*
* @dataProvider providerTestResponseFormat
*/
public function testResponseFormat($methods, array $supported_formats, $request_format, array $request_headers, $request_body, $expected_response_content_type, $expected_response_content) {
$rest_config_name = $this->randomMachineName();
$parameters = [];
if ($request_format !== FALSE) {
$parameters['_format'] = $request_format;
}
foreach ($request_headers as $key => $value) {
unset($request_headers[$key]);
$key = strtoupper(str_replace('-', '_', $key));
$request_headers[$key] = $value;
}
foreach ($methods as $method) {
$request = Request::create('/rest/test', $method, $parameters, [], [], $request_headers, $request_body);
$route_requirement_key_format = $request->isMethodSafe() ? '_format' : '_content_type_format';
$route_match = new RouteMatch('test', new Route('/rest/test', ['_rest_resource_config' => $rest_config_name], [$route_requirement_key_format => implode('|', $supported_formats)]));
$resource = $this->prophesize(StubRequestHandlerResourcePlugin::class);
// Mock the configuration.
$config = $this->prophesize(RestResourceConfigInterface::class);
$config->getFormats($method)->willReturn($supported_formats);
$config->getResourcePlugin()->willReturn($resource->reveal());
$config->getCacheContexts()->willReturn([]);
$config->getCacheTags()->willReturn([]);
$config->getCacheMaxAge()->willReturn(12);
$this->entityStorage->load($rest_config_name)->willReturn($config->reveal());
// Mock the resource plugin.
$response = new ResourceResponse($method !== 'DELETE' ? ['REST' => 'Drupal'] : NULL);
$resource->getPluginDefinition()->willReturn([]);
$method_prophecy = new MethodProphecy($resource, strtolower($method), [Argument::any(), $request]);
$method_prophecy->willReturn($response);
$resource->addMethodProphecy($method_prophecy);
// Test the request handler.
$handler_response = $this->requestHandler->handle($route_match, $request);
$this->assertSame($expected_response_content_type, $handler_response->headers->get('Content-Type'));
$this->assertEquals($expected_response_content, $handler_response->getContent());
}
}
/**
* @return array
* 0. methods to test
* 1. supported formats for route requirements
* 2. request format
* 3. request headers
* 4. request body
* 5. expected response content type
* 6. expected response body
*/
public function providerTestResponseFormat() {
$json_encoded = Json::encode(['REST' => 'Drupal']);
$xml_encoded = "<?xml version=\"1.0\"?>\n<response><REST>Drupal</REST></response>\n";
$safe_method_test_cases = [
'safe methods: client requested format (JSON)' => [
// @todo add 'HEAD' in https://www.drupal.org/node/2752325
['GET'],
['xml', 'json'],
'json',
[],
NULL,
'application/json',
$json_encoded,
],
'safe methods: client requested format (XML)' => [
// @todo add 'HEAD' in https://www.drupal.org/node/2752325
['GET'],
['xml', 'json'],
'xml',
[],
NULL,
'text/xml',
$xml_encoded,
],
'safe methods: client requested no format: response should use the first configured format (JSON)' => [
// @todo add 'HEAD' in https://www.drupal.org/node/2752325
['GET'],
['json', 'xml'],
FALSE,
[],
NULL,
'application/json',
$json_encoded,
],
'safe methods: client requested no format: response should use the first configured format (XML)' => [
// @todo add 'HEAD' in https://www.drupal.org/node/2752325
['GET'],
['xml', 'json'],
FALSE,
[],
NULL,
'text/xml',
$xml_encoded,
],
];
$unsafe_method_bodied_test_cases = [
'unsafe methods with response (POST, PATCH): client requested no format, response should use request body format (JSON)' => [
['POST', 'PATCH'],
['xml', 'json'],
FALSE,
['Content-Type' => 'application/json'],
$json_encoded,
'application/json',
$json_encoded,
],
'unsafe methods with response (POST, PATCH): client requested no format, response should use request body format (XML)' => [
['POST', 'PATCH'],
['xml', 'json'],
FALSE,
['Content-Type' => 'text/xml'],
$xml_encoded,
'text/xml',
$xml_encoded,
],
'unsafe methods with response (POST, PATCH): client requested format other than request body format (JSON): response format should use requested format (XML)' => [
['POST', 'PATCH'],
['xml', 'json'],
'xml',
['Content-Type' => 'application/json'],
$json_encoded,
'text/xml',
$xml_encoded,
],
'unsafe methods with response (POST, PATCH): client requested format other than request body format (XML), but is allowed for the request body (JSON)' => [
['POST', 'PATCH'],
['xml', 'json'],
'json',
['Content-Type' => 'text/xml'],
$xml_encoded,
'application/json',
$json_encoded,
],
];
$unsafe_method_bodyless_test_cases = [
'unsafe methods with response bodies (DELETE): client requested no format, response should have no format' => [
['DELETE'],