In #3455975-11: HTTP API: update /xb-component/{component_id} to list possible prop sources for current entity context we discovered the patternProperties was not supported.
I put in a fix that used \Drupal\experience_builder\EventSubscriber\ApiResponseValidator::performXbValidation and \Drupal\experience_builder\EventSubscriber\ApiResponseValidator::validateKeys and an addition to the openapi.yml file
# Add a custom validation method to enforce the key pattern via an OpenAPI extension which are
# designated with 'x-' prefix.
# @see https://swagger.io/docs/specification/openapi-extensions
# @see \Drupal\experience_builder\EventSubscriber\ApiResponseValidator::performXbValidation
x-xb-validation:
method: 'validateKeys'
arguments:
# @see \Drupal\Core\Plugin\Component::$machineName
- '^[a-zA-Z0-9_-]+:[a-zA-Z0-9_-]+$'
but in fixing another problem with the openapi.yml file this addition was removed
here is the version with the fix https://git.drupalcode.org/project/experience_builder/-/blob/f649d4bbaea...
patternPropertiespatternPropertiesBasically we want to make sure we can't keep adding parts of the openapi schema that is not actually being enforce, and enforce that parts we can now. We just can't enforce the property keys.
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #3
tedbowComment #4
tedbowI pushed up a rough start to test.
Putting this down to work on other issues for now
Comment #5
wim leersComment #6
wim leersComment #8
longwaveMerged 0.x and pushed some fixes, but there are multiple deprecations that I don't understand yet and a test fail that I am not sure what to do with.
Comment #9
longwaveSaving attribution.
Comment #10
tedbowJust merged #3478830: Remove dead `ApiResponseValidator::performXbValidation()` we may have to add this back here if we need it
Comment #11
wim leersThe removal of
x-xb-validationin/openapi.ymlseems to have been accidental in this commit: https://git.drupalcode.org/project/experience_builder/-/merge_requests/1... ? 🤔Comment #12
wim leersFollowing up on non-draft MRs that have been open for a long time. That led me here. What's next here? Is this still relevant?
Comment #13
longwaveWe should remove
patternPropertiesfrom openapi.yml if it doesn't work, but otherwise I am not sure we need to strictly validate here, Drupal will do it for us anyway?Comment #15
traviscarden commentedThis MR is quite old and out of sync with
0.x. Let's have a rebase, shall we? 👆Comment #17
tedbowre #12 yes, still relevant. I think any time we have `patternProperties` in openapi that part is not getting validated. example #3505224: XbConfigEntityHttpApiTest missing test coverage for individual GET requests for JavaScriptComponent + AssetLibrary
Although I think my approach in https://git.drupalcode.org/project/experience_builder/-/merge_requests/226 probably could work, I think it is probably overly complicated for what it gets us.
I think
addtionalPropertiesgets us everything except key checking. Although not ideal I don't think we have had actual problems with invalid keys and it seem like an edge case of what we are likely to hitSo added new MR that simply adds a test that make sure we don't add any new uses
addtionalProperties, which I think has happened since we first opened this issues. We would have to make a few changes to get the new test to passComment #18
tedbowTests are failing I think because the validation is now starting be enforced for sections that were being skipped because of our use of
patternPropertiesComment #19
tedbowComment #20
tedbowComment #22
tedbowI think we should get this MR in ASAP. We have 3 response that are not being validated but appear to be. People would waste time in other issue updating the response spec thinking it is actually being enforced.
1 of the responses already fails the validation that is there and the other 2 could be broken at any point and no one would know. This is really just to stop the bleeding and #3472632: Decide on an approach for writing tests for OpenAPI integration can figure the way forward
I created #3506543: Enforce response keys in openapi if possible to figure out if we can actually validate keys.
Comment #23
tedbowComment #24
tedbowNeed to bring back more changes after merging with 0.x
Comment #25
tedbowanother follow-up #3471064: OpenApi validation is not enforced because openapi.yml uses `patternProperties` which is not supported by our dependencies
Comment #26
tedbowComment #27
tedbowComment #28
longwaveAgree with the rationale, and the test is helpful until we get an upgrade to the OpenAPI spec validator - which is somewhat outside the scope of this project.
Comment #29
wim leersThis adds onto #3499703: Make all XB HTTP API routes consistently prefixed, ensure they all have OpenAPI specs, and tests to keep it so, in making XB's use of OpenAPI less brittle! 🚀
Comment #30
wim leersLanded #3504494: OpenAPI spec insufficiently precise for `LayoutComponent` while waiting for this to be green.
Comment #32
tedbow🎉
Comment #33
wim leersWell done!