From 9bac03169f4f3046b95442bd5e549eeccea7a377 Mon Sep 17 00:00:00 2001 From: Dries Date: Sat, 16 Feb 2013 18:48:20 -0500 Subject: [PATCH] Issue #1865594 by klausi: Better REST error messages in case of exceptions. --- .../Plugin/rest/resource/DBLogResource.php | 2 +- .../Plugin/rest/resource/EntityResource.php | 16 +++++++------- .../rest/lib/Drupal/rest/RequestHandler.php | 21 +++++++++++++++---- .../rest/lib/Drupal/rest/Tests/CreateTest.php | 7 ++++++- .../rest/lib/Drupal/rest/Tests/DBLogTest.php | 3 ++- .../rest/lib/Drupal/rest/Tests/DeleteTest.php | 3 ++- .../rest/lib/Drupal/rest/Tests/ReadTest.php | 3 ++- 7 files changed, 38 insertions(+), 17 deletions(-) diff --git a/core/modules/rest/lib/Drupal/rest/Plugin/rest/resource/DBLogResource.php b/core/modules/rest/lib/Drupal/rest/Plugin/rest/resource/DBLogResource.php index 60a16b9b5f0..cc06e1a5126 100644 --- a/core/modules/rest/lib/Drupal/rest/Plugin/rest/resource/DBLogResource.php +++ b/core/modules/rest/lib/Drupal/rest/Plugin/rest/resource/DBLogResource.php @@ -58,6 +58,6 @@ class DBLogResource extends ResourceBase { return $response; } } - throw new NotFoundHttpException('Not Found'); + throw new NotFoundHttpException(t('Log entry with ID @id was not found', array('@id' => $id))); } } diff --git a/core/modules/rest/lib/Drupal/rest/Plugin/rest/resource/EntityResource.php b/core/modules/rest/lib/Drupal/rest/Plugin/rest/resource/EntityResource.php index 4c973e1f428..541c80b6327 100644 --- a/core/modules/rest/lib/Drupal/rest/Plugin/rest/resource/EntityResource.php +++ b/core/modules/rest/lib/Drupal/rest/Plugin/rest/resource/EntityResource.php @@ -63,16 +63,16 @@ class EntityResource extends ResourceBase { * @throws \Symfony\Component\HttpKernel\Exception\HttpException */ public function post($id, EntityInterface $entity) { + $definition = $this->getDefinition(); // Verify that the deserialized entity is of the type that we expect to // prevent security issues. - $definition = $this->getDefinition(); if ($entity->entityType() != $definition['entity_type']) { - throw new BadRequestHttpException(); + throw new BadRequestHttpException(t('Invalid entity type')); } // POSTed entities must not have an ID set, because we always want to create // new entities here. if (!$entity->isNew()) { - throw new BadRequestHttpException(); + throw new BadRequestHttpException(t('Only new entities can be created')); } try { $entity->save(); @@ -83,7 +83,7 @@ class EntityResource extends ResourceBase { return new ResourceResponse(NULL, 201, array('Location' => $url)); } catch (EntityStorageException $e) { - throw new HttpException(500, 'Internal Server Error', $e); + throw new HttpException(500, t('Internal Server Error'), $e); } } @@ -122,7 +122,7 @@ class EntityResource extends ResourceBase { return new ResourceResponse(NULL, 204); } catch (EntityStorageException $e) { - throw new HttpException(500, 'Internal Server Error', $e); + throw new HttpException(500, t('Internal Server Error'), $e); } } @@ -145,7 +145,7 @@ class EntityResource extends ResourceBase { } $definition = $this->getDefinition(); if ($entity->entityType() != $definition['entity_type']) { - throw new BadRequestHttpException('Invalid entity type'); + throw new BadRequestHttpException(t('Invalid entity type')); } $original_entity = entity_load($definition['entity_type'], $id); // We don't support creating entities with PATCH, so we throw an error if @@ -167,7 +167,7 @@ class EntityResource extends ResourceBase { return new ResourceResponse(NULL, 204); } catch (EntityStorageException $e) { - throw new HttpException(500, 'Internal Server Error', $e); + throw new HttpException(500, t('Internal Server Error'), $e); } } @@ -194,7 +194,7 @@ class EntityResource extends ResourceBase { return new ResourceResponse(NULL, 204); } catch (EntityStorageException $e) { - throw new HttpException(500, 'Internal Server Error', $e); + throw new HttpException(500, t('Internal Server Error'), $e); } } throw new NotFoundHttpException(t('Entity with ID @id not found', array('@id' => $id))); diff --git a/core/modules/rest/lib/Drupal/rest/RequestHandler.php b/core/modules/rest/lib/Drupal/rest/RequestHandler.php index fe74eec3aae..3922e9e1397 100644 --- a/core/modules/rest/lib/Drupal/rest/RequestHandler.php +++ b/core/modules/rest/lib/Drupal/rest/RequestHandler.php @@ -12,6 +12,7 @@ use Symfony\Component\DependencyInjection\ContainerAware; use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\Response; use Symfony\Component\HttpKernel\Exception\HttpException; +use Symfony\Component\Serializer\Exception\UnexpectedValueException; /** * Acts as intermediate request forwarder for resource plugins. @@ -44,9 +45,16 @@ class RequestHandler extends ContainerAware { if (!empty($received)) { $definition = $resource->getDefinition(); $class = $definition['serialization_class']; - // @todo Replace the format here with something we get from the HTTP - // Content-type header. See http://drupal.org/node/1850704 - $unserialized = $serializer->deserialize($received, $class, 'drupal_jsonld'); + try { + // @todo Replace the format here with something we get from the HTTP + // Content-type header. See http://drupal.org/node/1850704 + $unserialized = $serializer->deserialize($received, $class, 'drupal_jsonld'); + } + catch (UnexpectedValueException $e) { + $error['error'] = $e->getMessage(); + $content = $serializer->serialize($error, 'drupal_jsonld'); + return new Response($content, 400, array('Content-Type' => 'application/vnd.drupal.ld+json')); + } } // Invoke the operation on the resource plugin. @@ -54,7 +62,12 @@ class RequestHandler extends ContainerAware { $response = $resource->{$method}($id, $unserialized, $request); } catch (HttpException $e) { - return new Response($e->getMessage(), $e->getStatusCode(), $e->getHeaders()); + $error['error'] = $e->getMessage(); + $content = $serializer->serialize($error, 'drupal_jsonld'); + // Add the default content type, but only if the headers from the + // exception have not specified it already. + $headers = $e->getHeaders() + array('Content-Type' => 'application/vnd.drupal.ld+json'); + return new Response($content, $e->getStatusCode(), $headers); } // Serialize the outgoing data for the response, if available. diff --git a/core/modules/rest/lib/Drupal/rest/Tests/CreateTest.php b/core/modules/rest/lib/Drupal/rest/Tests/CreateTest.php index c5b298a1a47..df77ec3efaa 100644 --- a/core/modules/rest/lib/Drupal/rest/Tests/CreateTest.php +++ b/core/modules/rest/lib/Drupal/rest/Tests/CreateTest.php @@ -49,7 +49,7 @@ class CreateTest extends RESTTestBase { $serialized = $serializer->serialize($entity, 'drupal_jsonld'); // Create the entity over the web API. $this->httpRequest('entity/' . $entity_type, 'POST', $serialized, 'application/vnd.drupal.ld+json'); - $this->assertResponse('201', 'HTTP response code is correct.'); + $this->assertResponse(201); // Get the new entity ID from the location header and try to read it from // the database. @@ -69,6 +69,11 @@ class CreateTest extends RESTTestBase { } $loaded_entity->delete(); + + // Try to send invalid data that cannot be correctly deserialized. + $this->httpRequest('entity/' . $entity_type, 'POST', 'kaboom!', 'application/vnd.drupal.ld+json'); + $this->assertResponse(400); + // Try to create an entity without the CSRF token. $this->curlExec(array( CURLOPT_HTTPGET => FALSE, diff --git a/core/modules/rest/lib/Drupal/rest/Tests/DBLogTest.php b/core/modules/rest/lib/Drupal/rest/Tests/DBLogTest.php index 2595735fedd..d4b9cc23f76 100644 --- a/core/modules/rest/lib/Drupal/rest/Tests/DBLogTest.php +++ b/core/modules/rest/lib/Drupal/rest/Tests/DBLogTest.php @@ -61,6 +61,7 @@ class DBLogTest extends RESTTestBase { // Request an unknown log entry. $response = $this->httpRequest("dblog/9999", 'GET', NULL, 'application/vnd.drupal.ld+json'); $this->assertResponse(404); - $this->assertEqual($response, 'Not Found', 'Response message is correct.'); + $decoded = drupal_json_decode($response); + $this->assertEqual($decoded['error'], 'Log entry with ID 9999 was not found', 'Response message is correct.'); } } diff --git a/core/modules/rest/lib/Drupal/rest/Tests/DeleteTest.php b/core/modules/rest/lib/Drupal/rest/Tests/DeleteTest.php index ed2d04d469e..e39675e1ef6 100644 --- a/core/modules/rest/lib/Drupal/rest/Tests/DeleteTest.php +++ b/core/modules/rest/lib/Drupal/rest/Tests/DeleteTest.php @@ -57,7 +57,8 @@ class DeleteTest extends RESTTestBase { // Try to delete an entity that does not exist. $response = $this->httpRequest('entity/' . $entity_type . '/9999', 'DELETE'); $this->assertResponse(404); - $this->assertEqual($response, 'Entity with ID 9999 not found', 'Response message is correct.'); + $decoded = drupal_json_decode($response); + $this->assertEqual($decoded['error'], 'Entity with ID 9999 not found', 'Response message is correct.'); // Try to delete an entity without proper permissions. $this->drupalLogout(); diff --git a/core/modules/rest/lib/Drupal/rest/Tests/ReadTest.php b/core/modules/rest/lib/Drupal/rest/Tests/ReadTest.php index 0578fc358a6..29fb1befc4e 100644 --- a/core/modules/rest/lib/Drupal/rest/Tests/ReadTest.php +++ b/core/modules/rest/lib/Drupal/rest/Tests/ReadTest.php @@ -67,7 +67,8 @@ class ReadTest extends RESTTestBase { // Try to read an entity that does not exist. $response = $this->httpRequest('entity/' . $entity_type . '/9999', 'GET', NULL, 'application/vnd.drupal.ld+json'); $this->assertResponse(404); - $this->assertEqual($response, 'Entity with ID 9999 not found', 'Response message is correct.'); + $decoded = drupal_json_decode($response); + $this->assertEqual($decoded['error'], 'Entity with ID 9999 not found', 'Response message is correct.'); // Try to read an entity without proper permissions. $this->drupalLogout();