Conversation
openedx-webhooks
added
open-source-contribution
|
Thanks for the pull request, @rodmgwgu! 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. |
8eb558d to
3e3f69a
Compare
7039856 to
7828722
Compare
mphilbrick211
added
the
mao-onboarding
7828722 to
ad046ff
Compare
| ERROR = "error" | ||
| WARNING = "warning" |
There was a problem hiding this comment.
nit: Could this be part of a class as we've done before?
|
|
||
| SUPPORTED_SCHEMA_VERSIONS = frozenset({"1.0"}) | ||
|
|
||
| # ---- entry points ----------------------------------------------------- |
There was a problem hiding this comment.
Same question about these inline comments as in other PRs. Do you think we need them?
| return issues | ||
|
|
||
| def validate_compiled(self, schema: CompiledSchema) -> list[ValidationIssue]: | ||
| """Re-check the resolved schema after extensions and priority are applied. |
There was a problem hiding this comment.
If this is a re-check, shouldn't it call checks already implemented instead of redefining its own L102-L137?
|
|
||
| # ---- whole-set -------------------------------------------------------- | ||
|
|
||
| def validate_set(self, documents: list[SchemaDocument]) -> list[ValidationIssue]: |
There was a problem hiding this comment.
I think I have the same question here about reusability. I understand these validations are happening at two levels:
- Per document
- The entire set
In what ways do those validations differ that we have to implement validations for each instead of reusing them? This same question I have about checking before and after compilation.
|
|
||
| def _check_permission_id(self, value: str, context: str, sid: str) -> list[ValidationIssue]: | ||
| """A complete permission id is ``namespace.name`` with both parts valid.""" | ||
| if value.count(".") != 1: |
There was a problem hiding this comment.
Can't we use a regex here instead to be more rigorous?
| return self.level == ERROR | ||
|
|
||
|
|
||
| class SchemaValidator: |
There was a problem hiding this comment.
Can we validate somehow against the current schema? https://github.com/openedx/openedx-authz/blob/main/src/openedx_authz/schema/authz-schema-v1.json, or do you think it's not worth it at this point?
| return issues | ||
|
|
||
| @staticmethod | ||
| def _register(index: dict, key: str, value, kind: str, sid: str) -> list[ValidationIssue]: |
There was a problem hiding this comment.
We're passing several of these structures around by reference and modifying them directly. Could some of this state live on the SchemaValidator instance instead? Maybe issues as well.
My thought process is that documents and the indexes we build from them are used across multiple validation steps, and right now we pass them through several methods and mutate them along the way. I wonder if keeping the state for a validation run on the validator instance would make it a bit easier to manage and reduce the amount of arguments we need to pass around.
Just a thought though, I'm not completely sure how it would look in practice.
| return [] | ||
|
|
||
| @staticmethod | ||
| def _relationship_source_id(schema: CompiledSchema, role_id: str, perm_id: str) -> str | None: |
There was a problem hiding this comment.
At first I didn't understand what _relationship_source_id I wasn't sure what "relationship" was
5 participants
Problem
An invalid schema must stop a deployment before anything is written, and it must report every problem at once rather than failing on the first one — a deployment operator fixing schema files one error per run is the failure mode to avoid (ADR 0018 §1).
Approach
PLEASE NOTE: jsonschema-based validation hasn't been implemented at this point, this means that some cases like validation of required priority and other fields are not complete. The implementation of the jsonschema validation will cover these, that is tracked by this issue: #459
openedx_authz/engine/schema/validation.py—SchemaValidator,ValidationIssueopenedx_authz/tests/schema/test_validation.pyTwo entry points, matching the two things that can only be checked at different stages:
validate()covers each document and the document set (identifier shape, required fields, duplicates), andvalidate_compiled()covers the rules only a resolved schema can answer (a role referencing a permission that does not exist, a permission in an unknown category). Issues are collected and returned with their source, so a caller can report all of them;SchemaValidationErrorcarries the full list.Manual testing instructions
Rollback plan
Revert this PR. Nothing consumes the validator yet, so the revert is inert.
Retro compatibility
No authorization behavior changes. No models, no migration, no automatic code path.
AI Usage
Kiro was used to assist on feature planning and implementation. Implementation was done step by step with human guidance and validation, based on the ADRs.
Stack (4/8) — #446 split into reviewable pieces. Bases chain bottom-up; merge in order.
rod/authz-schema-compiler)rod/authz-schema-models— definition models + migrationrod/authz-schema-renderer— policy rendererrod/authz-schema-applier— schema applierload_authz_schemacommand, version bump and changelogMerge checklist:
authz-schema-v1.jsonis tracked separately in Extend schema loader validation to use schema reference defined in #431 #459