When Schemata is present, we can generate schemas for our resources. That means that we can validate that all requests coming out of JSON API can be validated against those schemas.

We would like to use the same strategy for that we are using to validate agains the generic JSON API schema.

CommentFileSizeAuthor
#62 2917260--validate-against-specific-schemas--62.patch5.63 KBe0ipso
#59 2917260--validate-against-specific-schemas--59.patch5.64 KBgabesullice
#59 interdiff--2917260--57-59.txt796 bytesgabesullice
#57 interdiff-51-57.txt3.72 KBgabesullice
#57 2917260--validate-against-specific-schemas--57.patch5.63 KBgabesullice
#51 2917260--validate-against-specific-schemas--51.patch5.45 KBe0ipso
#51 2917260--interdiff--49-51.txt1.03 KBe0ipso
#49 2917260--validate-against-specific-schemas--49.patch4.67 KBe0ipso
#49 2917260--interdiff--49-46.txt1.8 KBe0ipso
#46 2917260--validate-against-specific-schemas--46.patch2.87 KBe0ipso
#40 2917260-40.patch23.97 KBgabesullice
#40 interdiff-35-40.txt4.27 KBgabesullice
#35 2917260-35.patch24.34 KBgabesullice
#35 interdiff-34-35.txt12 KBgabesullice
#34 2917260-34.patch17.72 KBgabesullice
#34 interdiff-31-34.txt792 bytesgabesullice
#31 interdiff-19-31.txt15.74 KBgabesullice
#31 2917260-31.patch17.44 KBgabesullice
#19 2917260-19.patch11.05 KBWim Leers
#16 2917260-16.patch11.06 KBWim Leers
#16 interdiff.txt461 bytesWim Leers
#15 2917260-15.patch10.62 KBWim Leers
#15 interdiff.txt903 bytesWim Leers
#13 2917260-13.patch10.61 KBWim Leers
#13 interdiff.txt1.04 KBWim Leers
#12 2917260-12.patch10.26 KBWim Leers
#12 interdiff.txt604 bytesWim Leers
#10 2917260-10.patch9.69 KBWim Leers
#10 interdiff.txt5.41 KBWim Leers
#2 feat--2917260--validate-against-specific-schemas--2.patch4.88 KBe0ipso
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

e0ipso created an issue. See original summary.

e0ipso’s picture

This is the initial pass. Please review.

This already uncovered some issues with the schema generation:
- If JSON API Extras disables some fields that are marked as required, then Schemata should drop them from the required section.
- If JSON API Extras customizes names of fields that are marked as required, then Schemata should add the custom name in the required section.

Status: Needs review » Needs work
Wim Leers’s picture

Issue tags: +API-First Initiative
Wim Leers’s picture

+1 for principle!

+++ b/src/EventSubscriber/ResourceResponseSubscriber.php
@@ -186,12 +189,65 @@ class ResourceResponseSubscriber implements EventSubscriberInterface {
+    if (!\Drupal::moduleHandler()->moduleExists('schemata')) {
+      return TRUE;
+    }
+    // Get the schema for the current resource. For that we will need to
+    // introspect the request to find the entity type and bundle matched by the
+    // router.
+    $route = $request->attributes->get(RouteObjectInterface::ROUTE_OBJECT);
+    // Check if the response is for a collection. We need to validate each
+    // resource object against the schema.

I think all this new logic should be moved to a separate helper method. The code is currently hard to read.

dawehner’s picture

I'm wondering whether we can do some profiling to see how much costs are included on every page response, when you have schemata enabled. I fear people would run those modules on production.

Wim Leers’s picture

But assert() hopefully is not enabled on production servers.

dawehner’s picture

Oh nevermind :) I missed that this function is called from assert().

Wim Leers’s picture

Assigned: Unassigned » Wim Leers

Heh, np :)

First, let's make this pass tests.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
5.41 KB
9.69 KB

Status: Needs review » Needs work

The last submitted patch, 10: 2917260-10.patch, failed testing. View results

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
604 bytes
10.26 KB

Forgot to include the updated service definition hunk.

Wim Leers’s picture

FileSize
1.04 KB
10.61 KB

I get this error:

"detail": "Could not normalize object of type Drupal\\schemata\\Schema\\NodeSchema, no supporting normalizer found.",

… until I install schemata_json_schema. i.e. just the schemata module is not enough.

So let's make that a test dependency.

Status: Needs review » Needs work

The last submitted patch, 13: 2917260-13.patch, failed testing. View results

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
903 bytes
10.62 KB

Forgot to update the test expectation.

Wim Leers’s picture

FileSize
461 bytes
11.06 KB

It looks like testbot doesn't actually respect test_dependencies… so let's also add it to composer.json's require-dev too. Fingers crossed 🤞

Wim Leers’s picture

Looks like that worked:

00:01:03.631 Composer Command: ./bin/composer require 'drupal/schemata:1.x-dev#8325d172e1d6880aa24073f8f751ef089282cf9a' 'justinrainbow/json-schema:^4.1' --ignore-platform-reqs --prefer-stable --no-progress --no-suggest --working-dir /var/lib/drupalci/workspace/jenkins-drupal8_contrib_patches-15256/source

Now I still need to install that module during tests. I know where to continue on Monday now! :)

e0ipso’s picture

That's a nice workaround with the require-dev.

$this->validateSchema($specific_schema, $resource_object) && $valid;

Is it worth to reverse this && so we skip faster when $valid is false?

Wim Leers’s picture

FileSize
11.05 KB
Wim Leers’s picture

#18: I've wondered about that exact same thing, and I even changed that line :D But then I figured you had done it this way to ensure everything would be validated, i.e. that we wouldn't not validate the second entity in a collection just because the first was invalid. Which is why I removed that from my review.

e0ipso’s picture

Status: Needs review » Needs work

We should bail on checking the schema for the related and relationship routes as well.

Wim Leers’s picture

Added this to #2842148: The path for JSON API to "super stability".

#21: why? Because the JSON Schema cannot possibly cover that, right?

Wim Leers’s picture

Wim Leers’s picture

e0ipso’s picture

I agree with @Wim Leers that this is a high priority issue, even though it may not be the most pressing for core's inclusion.

Wim Leers’s picture

Assigned: Wim Leers » Unassigned

It's not pressing for core inclusion directly, but it is indirectly, because it proves there is a certain level of accuracy/robustness. Which means it's easier for core committers to trust that things are working as intended.

In other words: this means there's less of a need for core committers to understand technical implementation details, because they'll be able to read the schema instead, observe that it makes sense, and since tests are passing, that means that all the code in the JSON API module is in fact generating responses matching that schema!

e0ipso’s picture

Yep, nevertheless I think this needs to be worked on soon. I may take a stab at it soon if you or Gabe don't beat me to it.

Wim Leers’s picture

+1. I know Gabe said he was interested in working on this, but I'm sure he wouldn't mind you working on it instead!

e0ipso’s picture

Assigned: Unassigned » gabesullice

I will leave this to Gabe then.

gabesullice’s picture

I checked with @e0ipso (via drupal.slack.com #jsonapi) to find out what was remaining on this issue. His summary of the remaining work was as follows:

  1. We should not check the schema on related or relationship requests. That's because the schema for article won't validate against /node/article/12345/field_tags/related
  2. We should make sure we do not run validation in production, even if the schema validation library is installed in production (other modules may require the lib in prod)
  3. We should make sure that in dev we still validate against the generic schema if the schemata module is not installed
gabesullice’s picture

This patch addresses numbers 2 and 3 from #30.

I think we have to rely on assert being disabled in production. I can't think of any other check to perform. It looks like that was the opinion expressed in #7 as well.

The tests in this patch should show that validation is not performed when asserts are disabled... but only in PHP 7.1 and above.

Unfortunately, these tests will fail in PHP 5.x because validation will still run (see #2920892: Remove all assert('string') calls from JSON API because deprecated in PHP 7.2 for why we can't make these pass).

I haven't addressed number 1 yet.

@e0ipso, I think we still want the generic schema validations to run for related/relationships, correct?

e0ipso’s picture

@e0ipso, I think we still want the generic schema validations to run for related/relationships, correct?

Yes. I would say so.

Unfortunately, these tests will fail in PHP 5.x because validation will still run (see #2920892: Remove all assert('string') calls from JSON API because deprecated in PHP 7.2 for why we can't make these pass).

Isn't there any way to prevent tests from running in 5.5, 5.6 and 7.0?

gabesullice’s picture

Isn't there any way to prevent tests from running in 5.5, 5.6 and 7.0?

Yes. I wasn't very clear. I meant, "the particular tests on this comment" will fail and we can't stop PHP from doing validation.

I plan on doing a PHP version check to ensure we get passing tests. I just wanted it to be clear on the fact that there's difference between 5 & 7.

gabesullice’s picture

gabesullice’s picture

e0ipso’s picture

Status: Needs review » Needs work

Here's the first review. Only some very minor comments.

  1. +++ b/jsonapi.services.yml
    @@ -131,9 +131,12 @@ services:
    +      - [setSchemaFactory, ['@?schemata.schema_factory']] # This is only injected when the service is available.
    

    Pretty cool! 🆒

  2. +++ b/src/EventSubscriber/ResourceResponseSubscriber.php
    @@ -66,6 +87,8 @@ class ResourceResponseSubscriber implements EventSubscriberInterface {
    +   * @param \Drupal\Core\Extension\ModuleHandlerInterface $module_handler
    +   *   The module handler.
    

    This is probably a leftover.

  3. +++ b/src/EventSubscriber/ResourceResponseSubscriber.php
    @@ -73,6 +96,36 @@ class ResourceResponseSubscriber implements EventSubscriberInterface {
    +    if ($validator) {
    +      $this->validator = $validator;
    +    }
    

    Is this code path exercised anywhere?

  4. +++ b/src/EventSubscriber/ResourceResponseSubscriber.php
    @@ -73,6 +96,36 @@ class ResourceResponseSubscriber implements EventSubscriberInterface {
    +    elseif (class_exists(Validator::class)) {
    +      $this->validator = new Validator();
    +    }
    

    Should we make a service out of Validator and inject it instead?

  5. +++ b/src/EventSubscriber/ResourceResponseSubscriber.php
    @@ -186,30 +255,77 @@ class ResourceResponseSubscriber implements EventSubscriberInterface {
    +    if (!$this->schemaFactory) {
    +      // We're done because we're missing schemata.
    +      return $is_valid;
    +    }
    

    This can only be TRUE at this point, right?

    Should we return TRUE instead for clarity?

  6. +++ b/src/EventSubscriber/ResourceResponseSubscriber.php
    @@ -186,30 +255,77 @@ class ResourceResponseSubscriber implements EventSubscriberInterface {
    +    $is_related = strpos($route_name, '.related') !== FALSE;
    +    $is_relationship = strpos($route_name, '.relationship') !== FALSE;
    ...
    +    $is_collection = strpos($route_name, '.collection') !== FALSE;
    

    This doesn't feel great. I don't think we have any better way.

    Does anyone feel that we should add a route option and check for that here instead of string matching on a string?

    @gabesullice can you create a follow up?

  7. +++ b/src/EventSubscriber/ResourceResponseSubscriber.php
    @@ -186,30 +255,77 @@ class ResourceResponseSubscriber implements EventSubscriberInterface {
    +      return $is_valid;
    

    Same comment as above.

  8. +++ b/src/EventSubscriber/ResourceResponseSubscriber.php
    @@ -186,30 +255,77 @@ class ResourceResponseSubscriber implements EventSubscriberInterface {
    +    $specific_schema = json_decode($output);
    

    Maybe Json::decode? Or we have that problem on coercing arrays instead of stdClass objects?

    Just making sure.

e0ipso’s picture

We agreed on #2920892-13: Remove all assert('string') calls from JSON API because deprecated in PHP 7.2 to only execute validation if the assertions are enabled. Even in PHP 5.5 and 5.6. We never ended up creating that issue 😑, but I think this is the moment to implement that. We are harming performance in lower PHP versions (which are already performance impared).

gabesullice’s picture

Should we make a service out of Validator and inject it instead?

I thought about doing this and researched it, but couldn't figure out how to make a service out of it only when the library exists. We could make a service _and_ a factory, but then we'd still end up with similar logic trying to find out if the service was actually injected or not.

TL;DR: it would be a whole lot of work and complexity for the same result, but maybe I'm missing a fancy trick.

This can only be TRUE at this point, right? ... Should we return TRUE instead for clarity?

Heh, I actually switched it to this because I thought this way was more clear. Sure, only one thing is possible, but this communicates that we're "falling back" to the generic result from above.

This doesn't feel great. I don't think we have any better way.

Does anyone feel that we should add a route option and check for that here instead of string matching on a string?

I agree that this isn't ideal, but I don't think there's a better way either. I also don't feel like a route option is any better because we're kind of abusing the routing system with those already. I've love to see something like "route tags" introduced, but for now, appending the name feels like the cleanest way to do that.

Maybe Json::decode? Or we have that problem on coercing arrays instead of stdClass objects?

Hm, not sure. Thats's just how it was. I'll give it a try.

e0ipso’s picture

I actually switched it to this because I […]

Let's switch it back to TRUE and let's add a one line comment for clarity.

I also don't feel like a route option is any better because we're kind of abusing the routing system with those already.

What is your concern about the suggested (and already current) approach?

gabesullice’s picture

Category: Feature request » Task
Priority: Normal » Major
Status: Needs work » Needs review
FileSize
4.27 KB
23.97 KB

This is probably a leftover.

Removed.

Is this code path exercised anywhere?

Yes, that's how the Validator mocks are injected. See testDoValidateResponse.

@gabesullice can you create a follow up?
...
What is your concern about the suggested (and already current) approach?

Will do. Let's discuss there.

Maybe Json::decode?

Done.

Let's switch it back to TRUE and let's add a one line comment for clarity.

Oui Chef! ;)

Re: #37: Only run asserts when asserts are active.

Done. Good suggestion, I like this approach better than skipping tests.

gabesullice’s picture

e0ipso’s picture

Status: Needs review » Reviewed & tested by the community

This issue is pretty awesome! I'm excited to ship this feature!

e0ipso’s picture

Status: Reviewed & tested by the community » Fixed

  • e0ipso committed 0a56ad4 on 8.x-1.x authored by gabesullice
    Issue #2917260 by Wim Leers, gabesullice, e0ipso: Validate against...
gabesullice’s picture

Status: Reviewed & tested by the community » Fixed

\o/

e0ipso’s picture

Assigned: gabesullice » Unassigned
Status: Fixed » Needs work
FileSize
2.87 KB

Reopening due to an interaction with xdebug on PHP 7.1

Attached patch.

e0ipso’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 46: 2917260--validate-against-specific-schemas--46.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

e0ipso’s picture

e0ipso’s picture

Status: Needs work » Needs review
e0ipso’s picture

gabesullice’s picture

Status: Needs review » Needs work
  1. +++ b/src/EventSubscriber/ResourceResponseSubscriber.php
    @@ -5,6 +5,7 @@ namespace Drupal\jsonapi\EventSubscriber;
    +use Drupal\Core\Extension\ModuleHandlerInterface;
    

    Thanks for alphabetizing XD

  2. +++ b/src/EventSubscriber/ResourceResponseSubscriber.php
    @@ -255,8 +266,13 @@ class ResourceResponseSubscriber implements EventSubscriberInterface {
    -    $schema_path = dirname(dirname(__DIR__)) . '/schema.json';
    ...
    +      $this->moduleHandler->getModule('jsonapi')->getPath(),
    

    Nice!

  3. +++ b/tests/src/Unit/EventSubscriber/ResourceResponseSubscriberTest.php
    @@ -15,6 +17,8 @@ use Symfony\Component\HttpFoundation\Request;
    +define('DRUPAL_ROOT', dirname(dirname(dirname(dirname(dirname(__DIR__))))));
    

    Are we sure this won't have unforeseen, negative effects?

If the answer to #3 is "yes," let's get this merged.

e0ipso’s picture

Are we sure this won't have unforeseen, negative effects?

I wondered that myself. But since tests pass and the constant was not being declared inside the test, I'm reasonably confident this should not be an issue. Maybe Daniel can clarify.

dawehner’s picture

In tests there is $this->root you can use directly, which you can inject using $container->get('app.root') .

Does that make sense?

Wim Leers’s picture

#40+#44: wow, I need to be away more often, great stuff like this just is already committed when I get back, and then you trick me by having the issue marked Needs work, leading me to think it's not yet committed 😀

Redeclaring core constants definitely is smelly, and #54 is better. Yes, there still are some core tests where this happens. But not many. Most of those cases were removed. Precisely because doing this caused so much problems :) So, NW++ to implement @dawehner's suggestion.

gabesullice’s picture

Assigned: Unassigned » gabesullice

I'll finish this one up. @e0ipso or @Wim Leers, just ping me in slack if you're already been working on it.

gabesullice’s picture

Status: Needs review » Needs work

The last submitted patch, 57: 2917260--validate-against-specific-schemas--57.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

gabesullice’s picture

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
e0ipso’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/src/EventSubscriber/ResourceResponseSubscriber.php
@@ -78,6 +79,20 @@ class ResourceResponseSubscriber implements EventSubscriberInterface {
+  /**
+   * The application's root file path.
+   *
+   * @var string
+   */
+  protected $app_root;
+

@@ -87,11 +102,17 @@ class ResourceResponseSubscriber implements EventSubscriberInterface {
+    $this->appRoot = $app_root;

This should say: protected $appRoot;

e0ipso’s picture

Status: Needs work » Fixed
FileSize
5.63 KB

  • e0ipso committed 44a780c on 8.x-1.x
    Issue #2917260 by gabesullice, Wim Leers, e0ipso, dawehner: Validate...

  • e0ipso committed 75633ec on 8.x-1.x
    Issue #2917260 by e0ipso: improve path building for schema
    

Status: Fixed » Closed (fixed)

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