The EntityResource controller class has a deserialize() method which takes care of building the entity object from the submitted request data.

At some point, if either UnexpectedValueException or InvalidArgumentException are thrown by the underlying serialized, it is catched and an UnprocessableHttpEntityException is thrown by the method itself.

    try {
      ...
    }
    // These two serialization exception types mean there was a problem with
    // the structure of the decoded data and it's not valid.
    catch (UnexpectedValueException $e) {
      throw new UnprocessableHttpEntityException($e->getMessage());
    }
    catch (InvalidArgumentException $e) {
      throw new UnprocessableHttpEntityException($e->getMessage());
    }

The issue here is that, per its signature, UnprocessableHttpEntityException first argument should be an Exception object, not a string. When one of this situation occurs, a TypeError is thrown by PHP, breaking the execution flow and leading to a 5xx error instead of a JSON:API managed 4xx error.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

garphy created an issue. See original summary.

garphy’s picture

This patch provides a new test case that exhibit the issue : We expect a UnprocessableHttpEntityException but we get a PHP error instead.

garphy’s picture

Status: Active » Needs review

Firing testbot.

garphy’s picture

Issue tags: +API-First Initiative

Status: Needs review » Needs work

The last submitted patch, 2: jsonapi-3052954-2-test-only.patch, failed testing. View results

gabesullice’s picture

Status: Needs work » Needs review
FileSize
2.81 KB
4.01 KB

Thanks @garphy!

Wim Leers’s picture

+++ b/core/modules/jsonapi/src/Controller/EntityResource.php
@@ -822,10 +822,10 @@ protected function deserialize(ResourceType $resource_type, Request $request, $c
     catch (UnexpectedValueException $e) {
...
+      throw new UnprocessableHttpEntityException($e);
     }
     catch (InvalidArgumentException $e) {
-      throw new UnprocessableHttpEntityException($e->getMessage());
+      throw new UnprocessableHttpEntityException($e);
     }

I'd swear I have a patch for this already…

Yep, found it, over at #3042124-23: [regression] Empty response body when user is an administrator and an exception is thrown; some traces cannot be encoded because of recursion detection.. I think that's the correct solution.

If we want to go with this solution, then we'll need to update the @throws entry on the \Drupal\jsonapi\Controller\EntityResource::deserialize() docs.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
1.86 KB

Per #7.

Wim Leers’s picture

Version: 8.7.x-dev » 8.8.x-dev
gabesullice’s picture

Status: Needs review » Reviewed & tested by the community

Thanks @Wim Leers! LGTM. Nope, see below.

gabesullice’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

I lied, we need to reroll the tests in #2. They should probably be moved to JsonapiRegressionTest too.

gabesullice’s picture

Assigned: Unassigned » Wim Leers
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
1.68 KB
3.42 KB

The actual fix in #8 looks good to me; back to @Wim Leers to review the reroll of @garphy's tests.

The test only patch is the interdiff.

The last submitted patch, 12: 3052954-12--tests-only.patch, failed testing. View results

Wim Leers’s picture

Assigned: Wim Leers » Unassigned
Status: Needs review » Reviewed & tested by the community
FileSize
1.86 KB
1.54 KB
3.28 KB
  1. +++ b/core/modules/jsonapi/tests/src/Functional/JsonApiRegressionTest.php
    @@ -989,4 +989,43 @@ public function testLeakedCacheMetadataViaRdfFromIssue3053827() {
    +   * Ensure that a 422 Unprocessable Entity for invalid POST data.
    

    🤓 Missing a verb. Fixed. ✅

  2. +++ b/core/modules/jsonapi/tests/src/Functional/JsonApiRegressionTest.php
    @@ -989,4 +989,43 @@ public function testLeakedCacheMetadataViaRdfFromIssue3053827() {
    +    // Set up some invalid data.
    ...
    +    // Set up a test user.
    ...
    +    // Send the invalid request.
    

    🤔This is quite different compared to the other test methods in this class. Let's follow the same pattern. Done. ✅

All of that is just adding docs and reordering logic that @garphy wrote, so that doesn't preclude me from RTBC'ing :)

catch’s picture

Status: Reviewed & tested by the community » Needs work

Needs a re-roll. Not sure what happened to DrupalCI marking issues CNW.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
1.58 KB
3.32 KB

Rebased.

The last submitted patch, 16: 3052954-16--tests-only-FAIL.patch, failed testing. View results

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed e0a8d33 and pushed to 8.8.x. Thanks!

  • catch committed e0a8d33 on 8.8.x
    Issue #3052954 by Wim Leers, gabesullice, garphy: Incorrect use of...
catch’s picture

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.