Openapi validation broken in HEAD

Can you define "broken"? 🙏

I assume this should go back to @tedbow now.

So the endpoint actually just receives a plain, arbitrary JSON object, then? If so, this looks fine to me.

Really?! 😳🤔

I don't think that's the case.

I think @larowlan was only trying to demonstrate that /openapi.yml was not updated in 📌 Change ApiContentUpdateForDemoController to save from auto-save instead of request data Active , yet still it allowed a seemingly invalid request to be considered acceptable.

@larowlan, is this a duplicate of/a subset of 📌 See why OpenAPI mistake for API error didn't cause test failures Active ?

I closed 📌 See why OpenAPI mistake for API error didn't cause test failures Active and pushed the MR there to here

copying my comment from 📌 See why OpenAPI mistake for API error didn't cause test failures Active

@TravisCarden Why not instead of having isProd at all we just leave the other checks in \Drupal\experience_builder\EventSubscriber\ApiMessageValidatorBase::shouldValidate but remove the isProd() check. Then in we could

1. catch the OpenApi validation errors and save the error info in Drupal state under xb_openapi_errors
2. log the errors in \Drupal\experience_builder\EventSubscriber\ApiMessageValidatorBase::logFailure
3. In \Drupal\Tests\BrowserTestBase::tearDown in our base test class(es?) and check the state there and throw an exception. We could have 1 trait for this across all test types

On advantage of this is that you won't fail on the first Open Validation error but could report on all the validation errors in the test run. Another advantage is it won't stop the test so you would also see another failure that would fail the test even if Open Validation failed.

This would also allow say if you were adding a new route locally but were sure on the return structure yet to not do the changes to openapi.yml or not complete the changes until you were sure on the structure, even if you had league/openapi-psr7-validator installed. It wouldn't stop you from working on the test for the new route. Openapi failures would still cause the test to fail but only at the very end so you would know if the rest of the test would pass.

I guess the con would be errors would not stop requests even if you had league/openapi-psr7-validator installed locally during manual testing, only automated tests. But we would also display the errors on the page.

We do something like this in \Drupal\Tests\package_manager\Kernel\PackageManagerKernelTestBase::tearDown but in that case we check the logs for specific errors.

re #5 I am pretty sure it is duplicate but @larowlan could correct me. But just to have 1 place to discuss this now let reopen this one. I was really getting confused between the 2 issues

So this MR https://git.drupalcode.org/project/experience_builder/-/merge_requests/485 proves at least validation is being triggered for responses.

Opened up https://git.drupalcode.org/project/experience_builder/-/merge_requests/486 to see if the problem is just that requests are not being validated.

📌 Change ApiContentUpdateForDemoController to save from auto-save instead of request data Active should have triggered a request validation error. I can confirm locally it does

looking at the openapi related fails in 3494388-force-fail and the fact that 3494388-debug-request-validation doesn't have any openapi fails I think the problem is just requests

larowlan → changed the visibility of the branch 3494388-see-why-openapi to hidden.

@wim leers ApiContentUpdateForDemoControllerTest is broken in HEAD but it doesn't fail in CI.

📌 Change ApiContentUpdateForDemoController to save from auto-save instead of request data Active changed the endpoint to no longer require a body (it uses the autosave store instead) but the openapi.yml wasn't updated.

Without the change in https://git.drupalcode.org/project/experience_builder/-/merge_requests/479 this causes a fail when run locally

1) Drupal\Tests\experience_builder\Functional\ApiContentUpdateForDemoControllerTest::testSave
Failed asserting that two arrays are identical.
--- Expected
+++ Actual
@@ @@
 Array &0 [
-    'message' => 'Saved successfully.',
+    'message' => 'Missing required header "Content-Type" for Request [patch /xb/api/content-update/{entityTypeId}/{entityId}]',

Updated the issue summary, which I should have done yesterday, but I quickly put this up before I finished for the day

I merged MR 479. Not sure if we want to keep this issue open for further troubleshooting or do that in new issues.

I've opened https://git.drupalcode.org/project/experience_builder/-/merge_requests/487 as another attempt

Credits

CI runs in a subdirectory and that's not taken into account in the validator. Currently invalid paths are skipped:

    catch (NoPath $e) {
      // @todo Temporarily log and ignore missing paths. Once 'openapi.yml' is
      //   is complete, remove this to treat them as failures.
      $this->logger->debug($e->getMessage());
    }

but if we throw this exception in CI, ApiContentUpdateForDemoControllerTest fails with:

-    'message' => 'Saved successfully.',
+    'message' => 'OpenAPI spec contains no such operation [/web/xb/api/content-update/node/1]',
+    'file' => '/builds/project/experience_builder/vendor/league/openapi-psr7-validator/src/PSR7/Exception/NoPath.php',

Maybe we need to do some translation of the request path in ApiRequestValidator::validate()?

MR!498 contains the fix, MR!499 should fail.

longwave → changed the visibility of the branch 3494388-force-fail to hidden.

longwave → changed the visibility of the branch 3494388-debug-request-validation to hidden.

longwave → changed the visibility of the branch 3494388-openapi-validation-broken to hidden.

📌 Implement auto-save of the page template config entity Active introduced another error into the request schema, entity_form_fields is now sent as part of the preview but not declared - fixed here.

Bumping to critical, as this is causing us to introduce test failures that are visible locally but not in CI.

@longwave thanks for figuring this out!

I assigned to @traviscarden because he needs to approve the MR.

Looks right to me!

thanks @traviscarden

longwave → closed merge request !499

So obvious in hindsight, thanks @longwave for #22!

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

wim leers → closed merge request !486