JSON API spec compliance: Throw error if relationships are not stored under data

@kostajh it's probably due to D8.4.x-dev, not PHP7. Can you test with Drupal core on 8.4 locally?

This patch includes a fix to the prior work, but it also solves #2888889: JSON API spec compliance: Throw error if the "type" attribute is missing in write operations, cleans up some tests and fixes invalid request payloads... in our own tests :S

Derp, fixed some copy pasta.

1. +++ b/tests/src/Functional/JsonApiFunctionalTest.php
   @@ -577,10 +577,43 @@ class JsonApiFunctionalTest extends JsonApiFunctionalTestBase {
   -    $created_response = Json::decode($response->getBody()->__toString());
   +    $created_response = Json::decode((string) $response->getBody());
   

   I see this in many places. What's the motivation?

2. +++ b/tests/src/Kernel/Normalizer/JsonApiDocumentTopLevelNormalizerTest.php
   @@ -444,7 +444,7 @@ class JsonApiDocumentTopLevelNormalizerTest extends JsonapiKernelTestBase {
   -    $payload = '{"type":"article", "data":{"attributes":{"title":"Testing article"}}}';
   +    $payload = '{"data":{"type":"article","attributes":{"title":"Testing article"}}}';
   

   😮

@gabesullice one clarification about this issue. These compliance issues were created before we had the schema validation built in. This validation here is redundant with the schema validation (which is run in "dev" mode and during functional tests).

What are these (limited) manual tests adding?

I see this in many places. What's the motivation?

Just taste. PHP let's you make a mess with characters like this ->__. Elsewhere in core, I've seen type conversion used more often than the __toString magic method.

What are these (limited) manual tests adding?

As far as I can tell, that validation is only for our responses. That is, we're not validating the incoming request payloads. This adds a bit of validation to those as a DX improvement. I'd love to hear your thoughts about validating incoming payloads with Schemata (my assumption was that we were not doing it because of performance concerns).

1. As far as I can tell, that validation is only for our responses. That is, we're not validating the incoming request payloads. This adds a bit of validation to those as a DX improvement.

   Indeed.

   Validating request bodies with Schemata too would be very interesting. Why haven't we thought about that before? :)

2. +++ b/src/Normalizer/JsonApiDocumentTopLevelNormalizer.php
   @@ -75,10 +75,6 @@
   -    // Check if relationships was placed outside of data.
   -    if (!empty($data['relationships'])) {
   -      throw new BadRequestHttpException("The 'relationships' key belongs under 'data'.");
   -    }
   
   @@ -87,9 +83,14 @@
   +    // Validate a few common errors in document formatting.
   +    $this->validateDocument($data);
   
   +++ b/tests/src/Functional/JsonApiFunctionalTest.php
   @@ -392,20 +392,6 @@
   -    // 6.1 Relationships are not included in "data".
   -    $malformed_body = $body;
   

   Because this logic is moved, we also had to move the test coverage? (i.e. because its' validated slightly later)?

   Also, why isn't that $this->validateDocument() call in the place that the original if-test was?

3. +++ b/src/Normalizer/JsonApiDocumentTopLevelNormalizer.php
   @@ -279,2 +280,16 @@
   +  public function validateDocument($document) {
   

   This looks like it could apply to the normalized result. But it's really only intended for incoming data: "request body document".

   So I'd rename this to something like validateRequestBody() or validateReceivedDocument() or something like that.

4. +++ b/tests/src/Functional/JsonApiFunctionalTest.php
   @@ -392,20 +392,6 @@
   -    $this->assertEquals('The \'relationships\' key belongs under \'data\'.', $created_response['errors'][0]['detail']);
   
   @@ -591,10 +577,43 @@
   +    $this->assertEquals("Found \"relationships\" within the document's top level. The \"relationships\" key must be within resource object.", $created_response['errors'][0]['detail']);
   

   Interesting. So we had some validation for this already?

5. +++ b/tests/src/Functional/JsonApiFunctionalTest.php
   @@ -554,7 +540,7 @@
   -    $created_response = Json::decode($response->getBody()->__toString());
   +    $created_response = Json::decode((string) $response->getBody());
   

   I'd +1 these changes, but they feel out of scope. Let's move them to a separate issue, so we can get that consistent everywhere first.

   (And of course, you'd need to convince @e0ipso.)

6. +++ b/tests/src/Kernel/Normalizer/JsonApiDocumentTopLevelNormalizerTest.php
   @@ -444,7 +444,7 @@ class JsonApiDocumentTopLevelNormalizerTest extends JsonapiKernelTestBase {
   -    $payload = '{"type":"article", "data":{"attributes":{"title":"Testing article"}}}';
   +    $payload = '{"data":{"type":"article","attributes":{"title":"Testing article"}}}';
   
   @@ -554,8 +554,8 @@ class JsonApiDocumentTopLevelNormalizerTest extends JsonapiKernelTestBase {
        $payload_data = [
   -      'type' => 'node--article',
          'data' => [
   +        'type' => 'node--article',
            'attributes' => [
              'title' => 'Testing article',
              'id' => '33095485-70D2-4E51-A309-535CC5BC0115',
   @@ -631,8 +631,8 @@ class JsonApiDocumentTopLevelNormalizerTest extends JsonapiKernelTestBase {
   
   @@ -631,8 +631,8 @@ class JsonApiDocumentTopLevelNormalizerTest extends JsonapiKernelTestBase {
    
        $node = [
          [
   -        'type' => 'node--article',
            'data' => [
   +          'type' => 'node--article',
              'attributes' => [
   

   Woah!

Validating request bodies with Schemata too would be very interesting. Why haven't we thought about that before? :)

It's an interesting thought. I'm not sure how I feel about it. Would it only be useful for internal application development when assert could be turned on? I don't think a public, production API would want to have schemata validating every incoming request, but I'm not sure.

Because this logic is moved, we also had to move the test coverage? (i.e. because its' validated slightly later)?

The test coverage didn't really "move". I think that's an artifact from #2 being 6 months old. I didn't reroll the patch before creating the interdiff.

Also, why isn't that $this->validateDocument() call in the place that the original if-test was?

Because it makes more sense to do the validation before extracting values from the document. I went even further with this and moved it to the first line of the method.

So I'd rename this to something like validateRequestBody() or validateReceivedDocument() or something like that.

Done.

Interesting. So we had some validation for this already?

Nope, you're just looking at the interdiff. I reworded this to more closely align to the language of the spec.

I'd +1 these changes, but they feel out of scope. Let's move them to a separate issue, so we can get that consistent everywhere first.

Removed (while pinching my nose).

The test coverage didn't really "move"

Nope, you're just looking at the interdiff.

Hah, oops!

1. +++ b/src/Normalizer/JsonApiDocumentTopLevelNormalizer.php
   @@ -273,4 +278,18 @@ class JsonApiDocumentTopLevelNormalizer extends NormalizerBase implements Denorm
   +  public function validateRequestBody($document) {
   

   Should be protected and typehinted to array.

   Also: should be static IMHO (see http://www.drupal4hu.com/node/416.html).

2. +++ b/tests/src/Functional/JsonApiFunctionalTest.php
   @@ -581,6 +581,39 @@ class JsonApiFunctionalTest extends JsonApiFunctionalTestBase {
   +    $this->assertEquals("Bad Request", $created_response['errors'][0]['title']);
   

   Why not assertSame()?

1. Good catch.

2. Because I'm a 🐑 and copied the rest of the tests (except for __toString apparently).

Can you also upload a test-only patch? That one should fail.

Sure, but note that this is a weird change where the tests were broken and the code change caught it.

Also, because the first assertion fails for the "relationship" case, the "type" case is never hit.

Hm, right.

Alright, then this is ready for final review & commit by @e0ipso :)

I'm a progress junkie! This feels right. I am now wondering if we should validate input documents against the JSON API schema instead of doing manual validations. If you feel that way, please open a new issue becase this one goes in!

I am now wondering if we should validate input documents against the JSON API schema instead of doing manual validations.

I think we should at least try that. It could save us a lot of trouble in the future!

I'm sorry it took so long @kostajh, but we made it there!

I am now wondering if we should validate input documents against the JSON API schema instead of doing manual validations.

I've rewritten this reply three different times, each with a different conclusion :P

I'm gonna settle on: "we should give this a shot, but not make it a high priority."

If we do put this in, it should be implemented to prove the validity of our implementation-not as a development tool.

More technically, this might mean we run this validation in our exception handler instead of the request handler. We can then decorate the error log with any validation failures (but not change the error from a 5xx to a 4xx!).

This means we should probably revert some of the validation that we did here.

I'm really wary of doing anything that encourages people to be making all their requests by hand, rather than using a jsonapi compliant client-side library (what's the point of a spec if you are?).

To clarify, I'm okay with leaving these two "manual" validations in because:

1. Because we documented this incorrectly
2. Because the spec specifically says we must throw a 409 if the type parameter is incorrect (we should be sure that we're checking that it's correct for the route too)

Alright, so let's open a follow-up issue for that? :)