Also validate request bodies against the OpenAPI spec

📌 Create an Open API spec for the current mock HTTP responses Fixed is in.

I had previously assigned 📌 Create an Open API spec for the current mock HTTP responses Fixed to @TravisCarden. Now that that foundation is in, I bet this will be right up his alley 😊

@lauriii just opened 🐛 [PP-1] Occasional PHP warning when refreshing XB Postponed this morning. This issue:

1. would at minimum make writing a test for that trivial
2. likely will surface that same problem through our existing E2E tests

See #3469850-2: [PP-1] Occasional PHP warning when refreshing XB → for details.

I'm making progress here. I'm not certain we've been using the validation library quite properly, so I'm looking into that. I also haven't looked much into automated tests for this yet. It don't see anything for the existing ApiResponseValidator.

Okay, we weren't actually misusing the validation library, but the code be a little clearer--a little more readable and intention-revealing. I've factored it out accordingly and made it reusable.

It's essentially working at this point, except that I need to add query strings to the OpenAPI spec. I also have a couple of TODO's in the code. One is just adding a description to the OpenAPI path. The other is how to throw exceptions in such a way as to produce meaningful UI error messages instead of JSON errors. The issue fork is up-to-date. The remaining tasks are small and shouldn't take long.

#7: totally possible!

Note that 📌 Create an Open API spec for the current mock HTTP responses Fixed introduced the use of that validation library, and it tried to follow the examples set by @larowlan's decoupled_lb_api module:

• https://git.drupalcode.org/project/decoupled_lb_api/-/blob/1.x/tests/src...
• https://git.drupalcode.org/project/decoupled_lb_api/-/blob/e991d78949988...
• https://git.drupalcode.org/search?search=OpenApiSchemaTrait&nav_source=n...

#8: Yay! 😄 I created a MR for your branch, so that CI runs and reviewing the code becomes possible. Posted an initial high-level review, and this is looking superb! 👏 For one of the @todos, I talked to @balintbrews who's worked on 📌 Improve client side error handling Fixed that ties in to your observation 👍

Also, zooming out — I bet that by the time you're done with this, you'll have an idea/opinion of how to:

• expand /openapi.yml, because it is incomplete still
• maximally use OpenAPI validation to our advantage — both for making sure the front-end and back-end end up in a more consistent place, as well as preparing the XB codebase for swift evolution after 🌱 Milestone 0.1.0: Experience Builder Demo Active — because all XB API routes are currently very ad-hoc (in terms of URL structure, logic, etc.)
• document where the exact cut-off lies of where OpenAPI validation ends (i.e. request/response shape validation) and "real" validation (i.e. content/config entity validation that inspects the actual values used, the "foreign keys" if you will)

Would you be interested in not only documenting what you believe should be the best practices, but also pushing forward XB's use of OpenAPI? (Totally fine if the answer is negative!)

@TravisCarden Sure! Can you please create the issue and add details from the backend's perspective? Then I can define what changes we need in the client-side error handling.

Done! ✨ [PP-1] Add the ability to surface more helpful error details for display in the UI Postponed Thanks.

Thanks, @balintbrews. Rather than increasing the scope on this issue, let's move the whole discussion to ✨ [PP-1] Add the ability to surface more helpful error details for display in the UI Postponed .

@Wim Leers, I've stubbed out the rest of the paths I knew about this afternoon. I realized I had been tracking the wrong upstream on my 0.x branch, so I was out-of-date and had to do some rebasing. Tomorrow I'll see if there are any new paths I need to take into account and what's missing in terms of specification. Then I'll see what other validation can be done on the OpenAPI side versus Drupal backend validation and document the difference.

At some point, I'll need someone to poke around the UI with me to make sure I've exercised it fully, since I've only been shown a fraction of it. There appear to be some things that are already broken on 0.x*, making it difficult to know when I've broken something.

* I'm getting an error on a fresh install on 0.x at /xb-component/static-static-card1ab. I've mentioned it in Slack. I'll bring it up at scrum tomorrow.

AssertionError: assert(!empty($components)) in assert() (line 185 of modules/contrib/experience_builder/src/Controller/SdcController.php).

📌 Alphabetize `openapi.yml` fields Fixed is in 👍

@traviscarden I bet 🐛 OpenAPI validation errors must be provided as a JSON response Active happens to be related to the mysterious bit you asked about yesterday, and which you've currently got this for:

+    // Ignore an empty JSON response.
+    // @todo When/why does this happen? Is it actually an error condition?
+    if ($response->getContent() === '{}') {
       return;
     }

🤞

Okay, @tedbow. As we discussed, the openapi.yml diff is next to impossible to read, so I made a piece-by-piece comparison. It was highly manual and a little bit tedious, so hopefully I didn't miss anything. Here it is:

→

@traviscarden looks good!. Thanks for the diff screenshot and help reviewing

Good to go if tests pass!

#17 Woah! No need for that next time, although the intent is very much appreciated! 😄

Several CI jobs are failing, so this cannot be RTBC … 😅 See https://git.drupalcode.org/project/experience_builder/-/pipelines/268587.

I'll push this across the finish line.

This represents a solid unit of work, and request bodies now are being validated. If I change the spec for the "preview" route to expect something nonsensical, then I do get the expected error now 👍

@tedbow RTBC'd in #19 but didn't approve the MR. @hooroomoo suggested the JS changes this MR makes, and they're trivial. So: rather than waiting for them clicking stuff in GitLab, going ahead and merging this, because @traviscarden has pointed out this has been painful to keep resolving conflicts for. (Plus, the next MR that touches openapi.yml is already awaiting: ✨ Backend route to allow logging from the UI Active .)

Next steps:

1. We get errors now for invalid request bodies … but not yet in a very friendlyway — see #3470995-2: OpenAPI validation errors must be provided as a JSON response → — I think that issue is a logical next step after this. (That unfriendliness is pre-existing: this MR did not introduce that!)
2. Can you create a follow-up issue to document the routes that do not yet have request bodies documented in openapi.yml? 🙏
3. It's pretty clear that you have opinions about how we should push this area ("OpenAPI" and all adjacent things) forward further. While working on this MR, you've noticed that several times another MR conflicted with this one. I think it'd make sense to have a person overlooking this crucial area to keep the front-end and back-end code + folks in sync. It will undoubtedly also help future contributors onboard. Plus, this API will evolve — the /xb-components controller returns a lot of cruft 💩 right now. It's "good enough" for now, but it won't remain that way: as the number of installed components grows and/or we're adding too much information to it, that route in particular will hit its limits (… and it seems that literally in the last 24 hours we might've hit that point: 🐛 [Performance regression] Loading the components takes >5s Active 😅).

   So … I'd like you to own that/be the overseer. 🤞🙏 If you agree, please create a new issue that contains:

   diff --git a/CODEOWNERS b/CODEOWNERS
   index 56207fd7..0fede359 100644
   --- a/CODEOWNERS
   +++ b/CODEOWNERS
   @@ -17,6 +17,13 @@
    # Hybrid side.
    #
    
   +[OpenAPI] @traviscarden
   +/openapi.yml
   +/src/EventSubscriber/ApiMessageValidator.php
   +/tests/src/Traits/OpenApiSpecTrait.php
   +/tests/src/Unit/OpenApiSpecValidationTest.php
   +/tests/src/Unit/UiFixturesValidationTest.php
   +
    [semi-coupled theme engine] @bnjmnm @hooroomoo
    /src/Theme/XBThemeNegotiator.php
    /themes/engines/semi_coupled/
   

   😊

Very glad to see this land! 🚀😄

Reverting to to signal this landed, but follow-up issues are still needed. Assigning to @traviscarden for that.

#21:

1. @traviscarden has AFAIK started on 🐛 OpenAPI validation errors must be provided as a JSON response Active 👍
2. I cannot find this follow-up yet among https://www.drupal.org/project/issues/search/experience_builder?issue_ta... →
3. @traviscarden did this: 📌 Add TravisCarden to `CODEOWNERS` for OpenAPI Fixed 🚀

Can you create a follow-up issue to document the routes that do not yet have request bodies documented in openapi.yml? 🙏 (i.e. for the @todo that @tedbow commented on over at https://git.drupalcode.org/project/experience_builder/-/merge_requests/2...)

I believe you're looking for 🐛 OpenAPI validation errors must be provided as a JSON response Active .

I've also created 📌 Lay out a strategy for OpenAPI integration Active and 📌 Add missing API paths + request bodies to `openapi.yml` Active . I think that wraps up this issue.

No, I wasn't looking for that.

The follow-up I requested is for documenting the expected request bodies for routes that do not have that defined yet. That's independent of generating JSON responses instead of plain text PHP stack traces, which is what 🐛 OpenAPI validation errors must be provided as a JSON response Active is for.

But I do think it makes sense to cover "missing request bodies" as part of 📌 Add missing API paths + request bodies to `openapi.yml` Active . Clarified that at #3472584-5: Add missing API paths + request bodies to `openapi.yml` → .

Oops. That was just a copy/paste error on my part. 📌 Add missing API paths + request bodies to `openapi.yml` Active is, of course, the one I meant.

Ah, great! 😄 You definitely made me scratch my head! 😜