Avoid JSON:API schema validation by default when assert is enabled

Quoting @larowlan:

FYI if we make this a production requirement, we should add a hook_requirements warning people with jsonapi module and assertions enabled that their performance will go through the floor because of that module's response validator.

I've been asked to audit sites for performance and found that combo in production 😱

— #3469516-4: Declare explicit runtime dependency on `justinrainbow/json-schema`

As a JSON:API maintainer: +1 for this.

Hmm, automatic discovery is kinda unsettling indeed. I agree that needs to go. I wonder if we will make things less stable if we stop validating. Especially in tests thi has merit. Altough, if performance on this is very bad (have not seem any numbers in the parent) we might want to choose and disable in anyways.

I do kinda agree on ussing a flag perhaps as twig, seems reasonable. Wonder though if we need to change ci then also to enable that. Something like parameters.jsonapi.validate_schema comes to mind then.

In order to be able to move forwards relatively quicky, this could first just be a parameter defaulting to false if not passed. Then we can consider (and perhaps implement) developer mode in a follow up.

In other news; I was also thinking if this would mean problems in #3031367: Generate JSON schema for content entity types but i think that is all fine.

Wondering, would this same issue exist for:

Drupal\Core\Theme\Component\ComponentValidator

That kinda does the same.

  /**
   * Sets the validator service if available.
   */
  public function setValidator(?Validator $validator = NULL): void {
    if ($validator) {
      $this->validator = $validator;
      return;
    }
    if (class_exists(Validator::class)) {
      $this->validator = new Validator();
    }
  }

Test run results. Kinda weary to do a full conclusion since variance is so high.

So question then becomes. Do we really want this, or perhaps only enable this in testing for a specific node?

Think this should fail since the validation is off by default now. Lets see if that is the case.

Ok it seems we only have unittests for the code. That is fine, i did a booboo on the unit test arguments though, so lets fix that. Validation should be disbled by default now at least.

Think this is fine, first time doing a pattern like this, so not sure if this is the right way.

Adding the extra contructor argument shoiuld be fine. We could also do this without a default value if preferred, but this way chances of breaking something seem smaller.

Code change looks fine but since it's adding a default service probably needs a change record. Maybe release notes? not 100% on the second.

CR drafted, child issue for #6 created in #3478689: Investigate if ComponentValidator should have soft dependency on justinrainbow/json-schema

Rebased and small tweak to test method order and comment. All ready.

Thanks!

CR reads well to me, clearly defines the need and code example makes it look clean.

Ran test-only feature

1) Drupal\Tests\jsonapi\Unit\EventSubscriber\ResourceResponseValidatorTest::testValidateResponse with data set #5 (Symfony\Component\HttpFoundation\Request Object (...), Drupal\rest\ResourceResponse Object (...), true, 'Response validation disabled ...valid.', false)
Response validation disabled doesn't flags empty array as invalid.
Failed asserting that false is identical to true.
/builds/issue/drupal-3472008/core/modules/jsonapi/tests/src/Unit/EventSubscriber/ResourceResponseValidatorTest.php:81
FAILURES!
Tests: 6, Assertions: 6, Failures: 1.
Exiting with EXIT_CODE=1

Which shows coverage.

Rest of the code looks good and believe this is good to go.

Fair point. Will do

Added changes as per #17. Also, fixed flag name mismatch in files.
For #19, I have tested and confirm that it is possible to override this container parameter in services.local.yml with these changes.

#3365985: Promote justinrainbow/json-schema from dev-dependency to full dependency is in.

@catch made an interesting remark over at #3365985-22: Promote justinrainbow/json-schema from dev-dependency to full dependency:

Asserts should be off in production anyway? Maybe we can add a generic status report warning for them when 'dev mode' isn't enabled?

IMHO we should instead pivot this issue to generating such a status report warning instead.

Which … coincidentally is also what @larowlan wrote, see my quote from him in #4!

So: being bold and proposing a rescope of this issue 😇 WDYT?

Sounds like a plan to rescope. +1

We just ran into a case where it would have been great to have this to disable response validation. I rebased the MR.
The change record looks correct already?

Ugly hotfix attached to entirely disable JSON:API response validation until this is fixed (MR!13739).
I'll now mark that one as draft to delete once we have a working solution here.

I just noticed SDC validation in the theme system, which is behind an assert.

However, the JSON:API validation is not behind an assert at all, so I don't see how this can be fixed with a status report check - bumping to critical.

I'll put this on my list since it got bumped.

Rebased, updated the change record.

I think we should do the implemntation as is. Allow enabling it through the parameter. Confirmed this in slack with Catch.

Tests are now green, think this is all good now :)

Created: #3562208: Add check in status report when "assert" is enabled

MR looks good to me. Only question is do we need to put this in default.services.yml or whatever the recommended dev services.yml docs are?

I was looking around and didnt see many examples of this. But it could be added in developement.services.yml, this could mean it's at least exposed to developers.

It also almost feels like we might as well remove it it this state, who is gonna use this really ever if we hide it away. Also right now it is not even used in testing or anything (which will help speed, but might mean we slip though invalid json).

So i guess if we are to keep it, we need be complete about is and document it, and make sure it is enabled in tests.

(hmm, not making this easier, sorry ;p)

Re-titling now that #3562208: Add check in status report when "assert" is enabled is open separately.

Small addendum;

core/modules/jsonapi/tests/src/Kernel/Normalizer/JsonApiTopLevelResourceNormalizerTest.php

that test does check schema's, but with code in the test. So schema's are tested.

That could be a way also yeah.

Which would be pretty small it seems, just add the event (and the test i guess) and enable it in jsonapi tests.

That would mean BC break probably though right, since we'd move classes around and change the namespace. :(

That would complicate things

seems we can do this withouth needing BC since its a subscriber.

Too bad about the window, but things happen. Made a new MR to move to a submodule and enabled that in the tests.

Isn't it always available after #3365985: Promote justinrainbow/json-schema from dev-dependency to full dependency? That was why I bumped this issue to critical.

Yeah that is the point indeed as catch mentioned. Also; the module would allow you to set a validator to use to validate a request if you use setValidator.

Might as well start to clean up a bit if we are not constraint by BC.

Small fix since docblocks were wrong. Fun.

Fun depreaction notice. We support 5 and 6 in core. So this deprecated call is needed.

 ------ ----------------------------------------------------------------------- 
  Line   core/modules/jsonapi/modules/jsonapi_response_validator/src/EventSubs  
         criber/ResourceResponseValidator.php                                   
 ------ ----------------------------------------------------------------------- 
  133    Call to deprecated method check() of class JsonSchema\Validator:       
         since 6.0.0, use Validator::validate() instead, to be removed in 7.0   
         🪪  method.deprecated 

You can add it to the globally ignored deprecations .deprecation-ignore.txt

Okiedokie. :)

#3562477: Drop support for justinrainbow/json-schema:^5

I think the SDC issue is #3352063: Allow schema references in Single Directory Component prop schemas which is RTBC but not committed yet (I haven't reviewed it yet so don't know how RTBC).

Moved some code around to clean up, and I'm questioning the critical prio, still. Good cleanup with more explicit sideeffects.

Thinking on this, the code was really for testing only since it is wrapped in assert to actually check. So i think you're right and it is testing only.

I've also manually checked if it runs on tests with:

 ../
vendor/bin/phpunit ../core/modules/jsonapi/tests/src/Functional/BlockTest.php

and have confirmed it works. Did change the services yml though.

Unrelated test failure.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Did a quick rebase, there were no conflics so setting back to rtbc.

Committed/pushed to main and 11.x, thanks!