Repository navigation
feat: separate structural and semantic schema validation - #542
Conversation
openedx-webhooks
added
open-source-contribution
|
Thanks for the pull request, @mariajgrimaldi! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
67d5aec to
6efbdf9
Compare
9bef38e to
06b2c5d
Compare
mariajgrimaldi
changed the title
BryanttV
left a comment
BryanttV
left a comment
There was a problem hiding this comment.
Thanks, @mariajgrimaldi! The code looks great, thanks for your work on this! I tested it locally and it works perfectly. I just have one minor (non-blocking) comment.
| log = logger.error if issue.is_error else logger.warning | ||
| log("authz schema %s: %s [%s]", issue.level, issue.message, issue.source_id or "-") | ||
| if self._validator.has_errors(issues): | ||
| if any(issue.is_error for issue in issues): |
There was a problem hiding this comment.
Why not keep using has_errors here?
There was a problem hiding this comment.
It seems that has_errors was only used here, if we are not using it becomes dead code that should be cleaned up.
There was a problem hiding this comment.
We have two validators now, so the decision to stop belongs in _gate(). This avoids exposing the same error-checking API on both validators. I’ll remove the unused has_errors() method and its tests.
rodmgwgu
left a comment
rodmgwgu
left a comment
There was a problem hiding this comment.
Looks great, thanks! I just have the same question as Bryann on the usage of the has_errors method
There was a problem hiding this comment.
Thanks for your work, @mariajgrimaldi ! I tested this locally and is working as expected
One non-blocking comment
- File
src/openedx_authz/engine/schema/__init__.pystill describes the pipeline lifecycle asdiscover -> load -> validate -> compile, some other places were updated to match the new onediscover -> parse -> validate structure -> load -> validate semantics -> compile
| def test_structural_errors_stop_before_typed_loading(self): | ||
| """All structural findings stop loading and later lifecycle stages.""" | ||
| errors = [ | ||
| ValidationIssue("error", "bad first document", "first"), |
There was a problem hiding this comment.
nit pic: Should these statements (and similar) use IssueLevel.ERROR instead raw string error?
There was a problem hiding this comment.
Fixed. Pipeline test fixtures now use IssueLevel.ERROR and IssueLevel.WARNING.
# Conflicts: # docs/decisions/0017-static-authorization-schema.rst
Description
Each contribution is now validated against the JSON Schema contract from #431, so the pipeline parses YAML, validates each document against the configured JSON Schema, and loads only structurally valid documents, while Python validation keeps semantic checks that require combined or compiled definitions, or the vendored Paragon icon names.
The JSON Schema now covers the structural rules previously implemented in Python (in
validation.py):Tests moved between the loading, parsing, syntax, and semantic suites with the same coverage, and the Python validators in
validation.pystill cover:How to test
From a Tutor LMS development container, at the mounted
openedx-authzcheckout root, run:pytest -q -o addopts='' -p no:randomly --create-db --ds=cms.envs.test \ src/openedx_authz/tests/integration/test_schema_pipeline.pyFixes #459