Validate responses against the JSON API specification

Woot! Thanks Chris :) This is possibly the single most important issue in the JSON API issue queue right now!

We have the ability to create JSON Schemas for the resources themselves based on the Typed Data (using https://github.com/e0ipso/data_model). That means that we could test the output against the specialized schema per resource, instead of the generic schema. This would have the benefit of asserting required values, cardinalities, etc. However the generated schema may have bugs and cause false alerts.

Thoughts?

It looks like DrupalCI is not installing the PHP lib. Described the problem at #2597778-78: Install composer dependencies for contrib projects before running tests, and pinged Mixologic on IRC.

@e0ipso: I think that can be a next step. Regardless of that, I think it's critical that we comply with the generic/basic/barebones schema for JSON API responses. If we don't comply with that, it's pointless to generate our own more specific schemas.
In other words: we must ALWAYS comply with the "basic JSON API JSON schema". As a next step, we should have a JSON schema for any JSON API extensions we support. And as a final step, we can have JSON schemas for our specific resources.

This is looking great, and wonderfully simple :)

1. +++ b/src/RequestHandler.php
   @@ -158,6 +159,8 @@ class RequestHandler implements ContainerAwareInterface, ContainerInjectionInter
   +    assert($this->validateResponse($response), 'A JSON API response failed validation. Please report this at https://www.drupal.org/project/issues/jsonapi');
   

   This is currently running 100% of the time on PHP 5.

   On PHP 5, you must wrap the assertion logic in quotes, i.e. make it a string. PHP 5 eval()s these statements.

2. +++ b/src/RequestHandler.php
   @@ -280,4 +283,42 @@ class RequestHandler implements ContainerAwareInterface, ContainerInjectionInter
   +  protected function validateResponse(ResourceResponse $response) {
   

   Should be static, because this doesn't use any data in the RequestHandler class (and shouldn't).

3. +++ b/src/RequestHandler.php
   @@ -280,4 +283,42 @@ class RequestHandler implements ContainerAwareInterface, ContainerInjectionInter
   +    $responseData = json_decode($response->getContent());
   

   s/$responseData/$response_data/

   Or $response_body.

4. +++ b/src/RequestHandler.php
   @@ -280,4 +283,42 @@ class RequestHandler implements ContainerAwareInterface, ContainerInjectionInter
   +    if ($response->headers->get('Content-Type') != 'application/vnd.api+json') {
   +      return TRUE;
   +    }
   

   Is this really necessary?

5. +++ b/src/RequestHandler.php
   @@ -280,4 +283,42 @@ class RequestHandler implements ContainerAwareInterface, ContainerInjectionInter
   +    $schema_path = DRUPAL_ROOT . '/' . drupal_get_path('module', 'jsonapi')
   +      . '/schema.json';
   

   It's okay for this all to be put on a single line.

6. +++ b/src/RequestHandler.php
   --- a/tests/src/Functional/JsonApiFunctionalTest.php
   +++ b/tests/src/Functional/JsonApiFunctionalTest.php
   

   I'd like to see an additional test here where this newly added assertion is explicitly failing.

   Because that would allow us to ensure that the DX of failed JSON API JSON schema validation is solid.

That does seem like a nice thing to explore, but I'd say create a separate issue for it.

Yup, that's what I meant. I was sharing a random thought in there.

I fixed the drupalci issue, and jsonschema is being installed now. Looks like its still failing #14 has different errors now.

Thanks, Mixologic!

So this is blocked on #2841096: Included objects returned for a relationships endpoint have attributes in a "data" key and #2841109: Included entities that have access denied must comply with the spec, and both of those have been discovered thanks to this issue?

Alright, updating issue title to reflect that then. :)

+++ b/composer.json
@@ -0,0 +1,9 @@
+    "require": {

When the blockers are in and we resume work on this issue/patch, we should change this to require-dev.

If that's done, we should also add a check for the existence of the validator class, in case the dependency isn't installed.

I think that's acceptable.

Looking at #26 again, I think it could be argued either way actually.

My preference would be to leave as a require'd dependency. Perhaps we could plan to move it to a require-dev when the module stabilizes?

I don't really care as long as we have the same outcome, which is there is a conscious decision to turn this on (regardless of the composer command needed).

Perhaps we could plan to move it to a require-dev when the module stabilizes?

Sounds good to me. Let's get it stable first.

This is unblocked now.

WOOT!

1. +++ b/src/Controller/RequestHandler.php
   @@ -165,6 +166,8 @@ class RequestHandler implements ContainerAwareInterface, ContainerInjectionInter
   +    assert('$this->validateResponse($response)', 'A JSON API response failed validation. Please report this at https://www.drupal.org/project/issues/jsonapi');
   

   Wouldn't it be better to show the validation errors here, instead of this generic "please report"? Only developers will see this, and they'll be smart enough to report it?

2. +++ b/tests/src/Functional/JsonApiFunctionalTest.php
   @@ -746,4 +747,53 @@ class JsonApiFunctionalTest extends BrowserTestBase {
   +    // Test validation failure: no "type" in "data".
   ...
   +    // Test validation success.
   ...
   +    // Test validation of an empty response passes.
   

   Looks sensible :)

Sorry I'm late to this, but I'd rather remove references to https://www.drupal.org/project/issues/jsonapi since we're proposing this for inclusion into core.

Maybe: A JSON API response failed validation (see /admin/reports/dblog for details). Please report this at the issue queue.'

1. +++ b/composer.json
   @@ -0,0 +1,9 @@
   +    "require-dev": {
   +        "justinrainbow/json-schema": "^4.1"
   +    }
   
   +++ b/src/Controller/RequestHandler.php
   @@ -286,4 +289,41 @@ class RequestHandler implements ContainerAwareInterface, ContainerInjectionInter
   +    if (!class_exists("\\JsonSchema\\Validator")) {
   +      return TRUE;
   +    }
   

   This is a nice trick!

2. +++ b/tests/src/Functional/JsonApiFunctionalTest.php
   @@ -746,4 +747,53 @@ class JsonApiFunctionalTest extends BrowserTestBase {
    
   +  public function testResponseValidation() {
   +    // Expose the protected RequestHandler::validateResponse() method.
   

   Couldn't this be a kernel test?

3. +++ b/tests/src/Functional/JsonApiFunctionalTest.php
   @@ -746,4 +747,53 @@ class JsonApiFunctionalTest extends BrowserTestBase {
   +    if (!$validate_response->invoke(NULL, $response)) {
   +      $this->fail('Response validation flagged a valid response.');
   +    }
   ...
   +    if (!$validate_response->invoke(NULL, $response)) {
   +      $this->fail('Response validation flagged a valid empty response.');
   +    }
   

   Is there a reason we cannot use assertFalse here?

I'm cool to commit this, but it seems that the patch in #46 got some extra unintended stuff attached to it,

I have only nits:

1. +++ b/composer.json
   @@ -0,0 +1,9 @@
   +    "description": "Provides a JSON API format for the REST resources.",
   

   I was going to say "this description is wrong", but it's wrong in the .info.yml too. So, no fault of this patch.

2. +++ b/composer.json
   --- /dev/null
   +++ b/schema.json
   

   Does this file have a particular version? How will we keep this up to date?

3. +++ b/src/Controller/RequestHandler.php
   @@ -2,6 +2,7 @@
   +use Drupal\Component\Serialization\Json;
   

   Unused use.

4. +++ b/src/Controller/RequestHandler.php
   @@ -289,4 +292,41 @@ class RequestHandler implements ContainerAwareInterface, ContainerInjectionInter
   +   * Only responses that are in the api_json format will be validated.
   

   This sentence is unnecessary, and misleading. This function is simply called, it has no knowledge whatsoever about which responses are going through the system. It simply validates the response it is given.

#54.2: let's at least document in this issue which commit (hash) it uses.
#54.3: oops, sorry!

There we go :)

I'm working on resolving conflicts after #2846857: Automated tests broken got merged and some minor follow ups.

No intediff due to reroll. However these are the most important changes I made:

1. +++ b/src/Controller/RequestHandler.php
   @@ -165,6 +166,8 @@ class RequestHandler implements ContainerAwareInterface, ContainerInjectionInter
   +    assert('$this->validateResponse($response)', 'A JSON API response failed validation (see /admin/reports/dblog for details). Please report this in the issue queue on drupal.org');
   

   (see /admin/reports/dblog for details) to (see the logs for details)

2. +++ b/src/Controller/RequestHandler.php
   @@ -289,4 +292,39 @@ class RequestHandler implements ContainerAwareInterface, ContainerInjectionInter
   +    $response_data = json_decode($response->getContent());
   

   Added:
   <?
   // Do not use Json::decode here since it coerces the response into an
   // associative array, which creates validation errors.

3. +++ b/tests/src/Kernel/Controller/RequestHandlerTest.php
   @@ -0,0 +1,76 @@
   +  public function testResponseValidation() {
   

   Added:

       // Check that the validation class is enabled.
       $this->assertTrue(
         class_exists("\\JsonSchema\\Validator"),
         'The JSON Schema validator is not present. Please make sure to install it using composer.'
       );
   

4. +++ b/tests/src/Kernel/Controller/RequestHandlerTest.php
   @@ -0,0 +1,76 @@
   +    if ($validate_response->invoke(NULL, $response)) {
   +      $this->assertFalse(TRUE, 'Response validation failed to flag an invalid response.');
   +    }
   

   Changed to:

       $this->assertFalse(
         $validate_response->invoke(NULL, $response),
         'Response validation failed to flag an invalid response.'
       );
   

   (This applies to all the asserts in this class).

Merging if green.

Woooooot! This is a huge milestone!