Problem/Motivation

While working on #2930028-50: Comprehensive JSON API integration test coverage phase 1: for every entity type, individual resources only, I noticed that I was able to POST this:

    [
      'data' => [
        'type' => 'comment--comment',
        'attributes' => [
          'entity_id' => 345,
          'entity_type' => 'entity_test',
          'field_name' => 'comment',
          'subject' => 'Dramallama',
          'comment_body' => [
            'value' => 'Llamas are awesome.',
            'format' => 'plain_text',
          ],
        ],
      ],
    ]

But that should not have been allowed: entity_id is a relationship field and thus should have been under the relationships key! Why is evident from both the JSON API spec and the response: what you get back is of the form 

    [
      'data' => [
        'type' => 'comment--comment',
        'attributes' => [
          'entity_type' => 'entity_test',
          'field_name' => 'comment',
          'subject' => 'Dramallama',
          'comment_body' => [
            'value' => 'Llamas are awesome.',
            'format' => 'plain_text',
          ],
        ],
        'relationships' => [
          'entity_id' => [
            'data' => [
              'type' => 'entity_test--bar',
              'id' => '<some UUID>',
            ],
          ],
        ],
      ],
    ]

Proposed resolution

Add validation in \Drupal\jsonapi\Normalizer\JsonApiDocumentTopLevelNormalizer::denormalize().

See http://jsonapi.org/format/#crud-creating.

Remaining tasks

TBD

User interface changes

None.

API changes

None. (Well, requests getting this wrong may now get HTTP 4xx responses, but this is per the JSON API spec and per the JSON API Drupal module's documentation: we reserve the right to fix spec compliance bugs without introducing BC layers for invalid calls.)

Data model changes

None.

CommentFileSizeAuthor
#16 2942549-16.patch6.93 KBgabesullice
#16 interdiff-2942549-11-16.txt2.09 KBgabesullice
#11 2942549-11.patch5.61 KBgabesullice
#11 interdiff-2942549-9-11.txt2.23 KBgabesullice
#9 2942549-9.patch5.94 KBgabesullice
#4 2942549-4.patch3.62 KBwim leers

Comments

Wim Leers created an issue. See original summary.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: Spec Compliance: JSON API allows posting relationship fields in 'data' rather than in 'relationships' » Spec Compliance: JSON API allows POSTing relationship fields in 'data' rather than in 'relationships'
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Version: 8.x-1.x-dev » 8.x-2.x-dev

Per #2952293-27: Branch next major: version 2, requiring Drupal core >=8.5 and #2952293-28: Branch next major: version 2, requiring Drupal core >=8.5, moving this to v2.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Active » Needs review
StatusFileSize
new 2942549-4.patch3.62 KB

This still needs explicit test coverage, but if I modify CommentTest to do what's described in the IS, I can simulate a test that should fail.

This then at least has some initial patch!

An explicit test probably belongs in \Drupal\Tests\jsonapi\Functional\JsonApiRegressionTest, because not every entity type has a relationship.

gabesullice’s picture

gabesullice
Primary language English
commented
Status: Needs review » Needs work

still NW

gabesullice’s picture

gabesullice
Primary language English
commented
Title: Spec Compliance: JSON API allows POSTing relationship fields in 'data' rather than in 'relationships' » Spec Compliance: JSON API allows POSTing relationship fields in 'attributes' rather than in 'relationships'

I think @Wim Leers just set this to NR to get a testbot run, but maybe it was for a +1 on the approach.

In that case, +1!

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

#6: exactly :)

gabesullice’s picture

gabesullice
Primary language English
commented
Assigned: Unassigned » gabesullice
gabesullice’s picture

gabesullice
Primary language English
commented
Assigned: gabesullice » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs tests +API-First Initiative
StatusFileSize
new 2942549-9.patch5.94 KB

Rerolled. Added explicit tests. Moved the check to validateRequestBody and made that a static call, since it's a static method.

Still need to handle internal vs public fields.

gabesullice’s picture

gabesullice
Primary language English
commented 
  1. +++ b/src/Normalizer/JsonApiDocumentTopLevelNormalizer.php
@@ -271,6 +273,16 @@ class JsonApiDocumentTopLevelNormalizer extends NormalizerBase implements Denorm
+    // Ensure that no relationship fields are being set via that attributes

    s/that/the 

  2. +++ b/src/Normalizer/JsonApiDocumentTopLevelNormalizer.php
@@ -271,6 +273,16 @@ class JsonApiDocumentTopLevelNormalizer extends NormalizerBase implements Denorm
+    // Ensure that no relationship fields are being set via that attributes

    s/that/the 

  3. +++ b/tests/src/Functional/ResourceTestBase.php
@@ -2203,6 +2207,17 @@ abstract class ResourceTestBase extends BrowserTestBase {
+    // DX: 422 when request document contains non-existent field.
+    $response = $this->request('PATCH', $url, $request_options);
+    $this->assertResourceErrorResponse(422, sprintf("The attribute field_nonexistent does not exist on the %s resource type.", static::$resourceTypeName), $response);

    This is a copy/paste mistake.

gabesullice’s picture

gabesullice
Primary language English
commented
StatusFileSize
new interdiff-2942549-9-11.txt2.23 KB
new 2942549-11.patch5.61 KB

Turns out that there is no need to deal with internal/public fields. The received values should already be internal and array_keys($resource_type->getRelatableResourceTypes()) also returns public fields.

Status: Needs review » Needs work

The last submitted patch, 11: 2942549-11.patch, failed testing. View results

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

The received values should already be internal and array_keys($resource_type->getRelatableResourceTypes()) also returns public fields.

🤔

  1. +++ b/src/Normalizer/JsonApiDocumentTopLevelNormalizer.php
@@ -75,8 +76,10 @@ class JsonApiDocumentTopLevelNormalizer extends NormalizerBase implements Denorm
+    $resource_type = $this->resourceTypeRepository->getByTypeName($data['data']['type']);

    We're now accessing this first, then calling ::validateRequestBody(). But ::validateRequestBody() is also validating the presence of this… 

  2. +++ b/src/Normalizer/JsonApiDocumentTopLevelNormalizer.php
@@ -75,8 +76,10 @@ class JsonApiDocumentTopLevelNormalizer extends NormalizerBase implements Denorm
-    $this->validateRequestBody($data);
+    static::validateRequestBody($data, $resource_type);

    ❤️

gabesullice’s picture

gabesullice
Primary language English
commented

The received values should already be internal and array_keys($resource_type->getRelatableResourceTypes()) also returns public fields.

Epically confusing typo. They both are dealing with public field names.

We're now accessing this first, then calling ::validateRequestBody(). But ::validateRequestBody() is also validating the presence of this…

Yup... silly mistake. Let's just use $context['resource_type'] rather than dynamically loading the type from the document value.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

👍

gabesullice’s picture

gabesullice
Primary language English
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-2942549-11-16.txt2.09 KB
new 2942549-16.patch6.93 KB

Done.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Reviewed & tested by the community

  • gabesullice committed 329a222 on 8.x-2.x 
    Issue #2942549 by gabesullice, Wim Leers: Spec Compliance: JSON API...
gabesullice’s picture

gabesullice
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

🎉

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

👏

Status: Fixed » Closed (fixed)

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