EntityNormalizer::denormalize should not throw UnexpectedValueException, but \Symfony\Component\HttpKernel\Exception\UnprocessableEntityHttpException, so we get a 422 response instead of 400

Oops.

It's still a 4xx response. So this will still end up in the general "error response handling" bucket.

We've changed many of our error responses (status codes, but especially the bodies), and need to, to provide sensible feedback. I think there's no way around that.

#7: ahhh!

@dawehner Well, @larowlan is right that this change would break BC for code that does this:

try {
  $this->serializer->denormalize($data, $class, 'hal_json');
} 
catch (UnexpectedValueException $e) {
  // Do something.
}

I think this means we want to wrap every (de)normalize() call and every (de)serialize() call in a try/catch and convert any exception to a \Symfony\Component\HttpKernel\Exception\UnprocessableEntityHttpException?

+++ b/core/modules/rest/src/RequestHandler.php
@@ -107,9 +108,10 @@ public function handle(RouteMatchInterface $route_match, Request $request) {
         catch (UnexpectedValueException $e) {
+          $status_code = $e instanceof UnprocessableEntityException ? 422 : 400;
           $error['error'] = $e->getMessage();
           $content = $serializer->serialize($error, $format);
-          return new Response($content, 400, array('Content-Type' => $request->getMimeType($format)));
+          return new Response($content, $status_code, array('Content-Type' => $request->getMimeType($format)));
         }
       }

This will change again in #2813853: RequestHandler has its own error handling rather than leaving it to KernelEvents::EXCEPTION event subscribers, but that's not really a problem.

My question is simply this: what if we just always convert a caught UnexpectedValueException here to a UnprocessableEntityHttpException?

(That's what I'm getting at in #11.)

In other words:

1. what's the value of changing all these normalizers and adding this slight variation of UnexpectedValueException? Especially if every single normalizer out there (including contrib ones) will be using UnexpectedValueException.
2. isn't sending a 422 response for any exception during denormalizing appropriate?

That's my point exactly! What I'm basically proposing is this:

  public function handle(RouteMatchInterface $route_match, Request $request) {
    …
        catch (UnexpectedValueException $e) {
          throw new UnprocessableEntityHttpException($e->getMessage());
        }
    …

That'd be even less disruptive, and would achieve the same AFAICT.

Am I missing something?

OMG, what a design fail in Symfony's serializer :/

Just to confirm what Daniel is saying: see \Symfony\Component\Serializer\Encoder\JsonDecode::decode(), \Symfony\Component\Serializer\Encoder\JsonEncode::encode(), \Symfony\Component\Serializer\Encoder\XmlEncoder::buildXml() and \Symfony\Component\Serializer\Encoder\XmlEncoder::decode(): they all throw \Symfony\Component\Serializer\Exception\UnexpectedValueException.

This means that the Symfony serializer's default exceptions do not allow us to distinguish between exceptions that should cause 400 exceptions and 422 exceptions.

Hence the approach in #10 presents the only possible solution: a new exception that extends the existing one, and defines additional semantic meaning. That way, all existing code continues to work. And code that wants to be aware of exceptions with the semantic meaning of a HTTP 422 response, can do so.

This is that new exception:

+++ b/core/modules/serialization/src/Exception/UnprocessableEntityException.php
@@ -0,0 +1,16 @@
+/**
+ * Exception to indicate a syntactic right but semantically wrong request.
+ *
+ * This exception might be converted to a HTTP 422 response.
+ *
+ * @see https://tools.ietf.org/html/rfc4918#page-78
+ */
+class UnprocessableEntityException extends UnexpectedValueException {

Sorry for holding this up, and thanks for the extra clarifications. Now this patch totally makes sense!

OMG, what a design fail in Symfony's serializer :/

The right thing to do is to +1 any relevant issues. But all of these issue searches result in zero matches:

• https://github.com/symfony/symfony/issues?utf8=%E2%9C%93&q=is%3Aissue%20...
• https://github.com/symfony/symfony/issues?utf8=%E2%9C%93&q=is%3Aissue%20...
• https://github.com/symfony/symfony/issues?utf8=%E2%9C%93&q=is%3Aissue%20...
• https://github.com/symfony/symfony/issues?utf8=%E2%9C%93&q=is%3Aissue%20...

And I also browsed Symfony's Serializer component docs. Turns out they're at least as bad, if not worse than Drupal's. No useful information regarding all this there either. That's how I stumbled upon https://api-platform.com/ though, which apparently is a Symfony bundle providing functionality similar to rest.module in Drupal 8. It's about a year old. So rest.module predates it by years. Surprisingly, zero mentions of 422 responses there too:
https://github.com/api-platform/core/search?utf8=%E2%9C%93&q=Unprocessab... or https://github.com/api-platform/core/search?utf8=%E2%9C%93&q=422.

So, created a new issue: https://github.com/symfony/symfony/issues/20534.

While doing the research for #21, found a problem:

1. +++ b/core/modules/hal/src/Normalizer/ContentEntityNormalizer.php
   @@ -234,7 +235,7 @@ protected function getTypedDataIds($types, $context = array()) {
   -        throw new UnexpectedValueException('Type must contain an \'href\' attribute.');
   +        throw new UnprocessableEntityException('Type must contain an \'href\' attribute.');
   
   @@ -247,7 +248,7 @@ protected function getTypedDataIds($types, $context = array()) {
   -      throw new UnexpectedValueException(sprintf('Type %s does not correspond to an entity on this site.', $type_uri));
   +      throw new UnprocessableEntityException(sprintf('Type %s does not correspond to an entity on this site.', $type_uri));
   

   These, and many like it, make sense. No BC break. Because the exception is being replaced by a subclassed exception.

2. +++ b/core/modules/hal/src/Normalizer/FieldItemNormalizer.php
   @@ -41,10 +42,10 @@ public function normalize($field_item, $format = NULL, array $context = array())
   -      throw new InvalidArgumentException('$context[\'target_instance\'] must be set to denormalize with the FieldItemNormalizer');
   +      throw new UnprocessableEntityException('$context[\'target_instance\'] must be set to denormalize with the FieldItemNormalizer');
   ...
   -      throw new InvalidArgumentException('The field item passed in via $context[\'target_instance\'] must have a parent set.');
   +      throw new UnprocessableEntityException('The field item passed in via $context[\'target_instance\'] must have a parent set.');
   
   +++ b/core/modules/hal/src/Normalizer/FieldNormalizer.php
   @@ -57,10 +58,10 @@ public function normalize($field, $format = NULL, array $context = array()) {
   -      throw new InvalidArgumentException('$context[\'target_instance\'] must be set to denormalize with the FieldNormalizer');
   +      throw new UnprocessableEntityException('$context[\'target_instance\'] must be set to denormalize with the FieldNormalizer');
   ...
   -      throw new InvalidArgumentException('The field passed in via $context[\'target_instance\'] must have a parent set.');
   +      throw new UnprocessableEntityException('The field passed in via $context[\'target_instance\'] must have a parent set.');
   

   But these do break BC… :(

I first had reservations about putting more logic in RequestHandler. But it's done for a good reason: to convert an exception to a 400 error response when decoding doesn't work, and to convert to a 422 response when denormalizing doesn't work. This makes a lot of sense.

Great insight, @damiankloip! :)

+++ b/core/modules/rest/src/RequestHandler.php
@@ -96,20 +97,39 @@ public function handle(RouteMatchInterface $route_match, Request $request) {
+        // First decode the request data. We can then determine if the
+        // serialized data was malformed.
         try {
-          if (!empty($definition['serialization_class'])) {
-            $unserialized = $serializer->deserialize($received, $definition['serialization_class'], $format, array('request_method' => $method));
-          }
-          // If the plugin does not specify a serialization class just decode
-          // the received data.
-          else {
-            $unserialized = $serializer->decode($received, $format, array('request_method' => $method));
-          }
+          $unserialized = $serializer->decode($received, $format, ['request_method' => $method]);
         }
         catch (UnexpectedValueException $e) {
-          $error['error'] = $e->getMessage();
+          // If an exception was thrown at this stage, there was a problem
+          // decoding the data. Return a 400 response.
+          $error = ['error' => $e->getMessage()];
           $content = $serializer->serialize($error, $format);
-          return new Response($content, 400, array('Content-Type' => $request->getMimeType($format)));
+
+          return new Response($content, 400, ['Content-Type' => $request->getMimeType($format)]);
+        }
+
+        // Then attempt to denormalize if there is a serialization class.
+        if (!empty($definition['serialization_class'])) {
+          try {
+            $unserialized = $serializer->denormalize($unserialized, $definition['serialization_class'], $format, ['request_method' => $method]);
+          }
+          catch (\Exception $e) {
+            if ($e instanceof UnexpectedValueException || $e instanceof InvalidArgumentException) {
+              // If one of the above exceptions was thrown at this stage, there
+              // was a problem with the structure of the decoded data and it's
+              // not valid.
+              $error = ['error' => $e->getMessage()];
+              $content = $serializer->serialize($error, $format);
+              return new Response($content, 422, ['Content-Type' => $request->getMimeType($format)]);
+            }
+
+            // Otherwise, re-throw the exception.
+            throw $e;
+          }
         }

Let's put the exception handling logic in a helper method that we pass either 400 or 422. That will make this code less repetitive and more legible.

1. +++ b/core/modules/rest/src/RequestHandler.php
   @@ -96,20 +97,34 @@ public function handle(RouteMatchInterface $route_match, Request $request) {
            catch (UnexpectedValueException $e) {
   ...
   +          catch (\Exception $e) {
   +            if ($e instanceof UnexpectedValueException || $e instanceof InvalidArgumentException) {
   ...
   +            // Otherwise, re-throw the exception.
   +            throw $e;
   

   Why should these not also have a similar structure?

2. +++ b/core/modules/rest/src/RequestHandler.php
   @@ -139,4 +154,23 @@ public function handle(RouteMatchInterface $route_match, Request $request) {
   +   * Creates a format serialized error response.
   

   Something's wrong in this sentence.

3. +++ b/core/modules/rest/src/RequestHandler.php
   @@ -139,4 +154,23 @@ public function handle(RouteMatchInterface $route_match, Request $request) {
   +   * @param \Exception $e
   +   * @param int $status_code
   +   * @param \Symfony\Component\HttpFoundation\Request $request
   +   * @param string $format
   +   *
   +   * @return \Symfony\Component\HttpFoundation\Response
   

   Missing doc lines.

4. +++ b/core/modules/rest/src/RequestHandler.php
   @@ -139,4 +154,23 @@ public function handle(RouteMatchInterface $route_match, Request $request) {
   +    $error = ['error' => $e->getMessage()];
   

   This is consistent with HEAD, but is also a problem, because everywhere else we have ['messsage' => …].

   However, we have #2813853: RequestHandler has its own error handling rather than leaving it to KernelEvents::EXCEPTION event subscribers to fix this, and AFAICT doing that will stay equally simple. In fact, in #2813853 we should be able to remove this helper method altogether and throw a 400/422 exception in those catch statements above.

1. I'm not sure I fully understand this sentence/reasoning. Put more simply: why do we need to catch everything for denormalizing, but not for decoding? Why can't we just not do that? Oh wait, because we have two types of exceptions we need to catch for the denormalizing case, and only one for the decoding case! Gah, this is messy. Because… isn't it entirely possible that there are yet other exceptions (other than UnexpectedValueException and InvalidArgumentException) that denormalizers might throw?

4. Yes, leave it for now. This was just an "FYI", but also a "please confirm that you think it's A) still possible, B) that that makes sense".
   

   I guess we would just throw the appropriate HttpException and be done. It can then we converted to the correct format etc... in the exception subscriber.

   Exactly. That's what #2813853: RequestHandler has its own error handling rather than leaving it to KernelEvents::EXCEPTION event subscribers is all about :)

Agreed! But how do we know there's not additional exceptions that may be thrown that should signify a 422?

AFAICT this is a huge oversight on Symfony's side: the serializer component should have its own exception base interface for this reason.

The failing tests here look like this:

1) Drupal\Tests\hal\Functional\EntityResource\Comment\CommentHalJsonAnonTest::testPost
Failed asserting that Array &0 (
    0 => 'application/json'
) is identical to Array &0 (
    0 => 'application/hal+json'
).

/var/www/html/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php:536

2) Drupal\Tests\hal\Functional\EntityResource\Comment\CommentHalJsonAnonTest::testPatch
Failed asserting that Array &0 (
    0 => 'application/json'
) is identical to Array &0 (
    0 => 'application/hal+json'
).

That's being fixed in #2805281: ?_format=hal_json error responses are application/json, yet should be application/hal+json, which is blocked on #2739617: Make it easier to write on4xx() exception subscribers, which is RTBC :)

It's blocked on two issues, not one.

And #2739617: Make it easier to write on4xx() exception subscribers, so now it's only blocked on one issue anymore.

#2805281: ?_format=hal_json error responses are application/json, yet should be application/hal+json was committed, this is now unblocked!

Oh, darn, I see what you mean, @damiankloip!

• Both this patch and the one at #2813853 indeed touch all the hunks that contain // @todo … https://www.drupal.org/node/2813853.
• Both this patch and the one at #2813853 touch RequestHandler::handle()

But…

1. this patch doesn't fix the key thing that #2813853 fixes: changing the error key to message
2. this patch would be a lot simpler of #2813853 landed first: then it could fix exactly what it was meant to fix: the 400 and 422 responses. It'd only need to make changes to RequestHandler::handle() and then do s/400/422/ in a few places in the tests.

So in other words: the concern of #2813853 is error -> message, the concern of this issue is 400 -> 422.

Which made me realize:

1. that this issue summary needs a massive overhaul: it says almost nothing right now! A committer would have a very hard time committing this. I can take that on.
2. that we perhaps want explicit test coverage for this? I'm not sure though. What do you think?

This is a straight reroll of #59. Let's see how it does now.

Per #75.

#2813853: RequestHandler has its own error handling rather than leaving it to KernelEvents::EXCEPTION event subscribers landed, this is now unblocked! :)

Straight reroll of #73, with conflicts resolved.

Back to RTBC, all of the actual work here was done by @damiankloip — see #59 and earlier.

Comparing #73 and #77 makes it clear that it was a good idea to wait for #2813853: RequestHandler has its own error handling rather than leaving it to KernelEvents::EXCEPTION event subscribers: the patch now is trivial. :)

Conflicted with #2838144: Update "bundle missing" error message in entity denormalization to indicate which field is actually missing. Simple conflict to resolve.

Ahh there it is :)

Per #28, this didn't make it into 8.2.x. But sadly, this also didn't make 8.3.x. Alas.