feat: add authz schema loading - #475
Conversation
|
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. |
mariajgrimaldi
left a comment
There was a problem hiding this comment.
A few comments for you to review! Thanks a lot for this :))
| except Exception: # noqa: BLE001 - defensive; metadata quirks across envs | ||
| mapping = {} | ||
| candidates = mapping.get(top_level) or [] | ||
| if candidates: |
There was a problem hiding this comment.
Could there be duplicates?
There was a problem hiding this comment.
In theory yes, I'm adding a way to deterministically choosing the candidate:
- First search for the distribution that actually has the discovered resource file
- If more than one matches exists, sort and return the first one as a deterministic fallback (and log a warning about it.)
There was a problem hiding this comment.
I wonder if these decisions should be recorded somehow. When we merge this stack, can we do like a reverse-engineering review to capture the deliberate decisions we're making here?
| ``hidden`` is tri-state: ``None`` leaves the current value untouched. | ||
| """ | ||
|
|
||
| role: str |
There was a problem hiding this comment.
Should we use some kind of role_id to reference the role we're extending?
There was a problem hiding this comment.
This also comes form the yaml definition as specified in the ADRs
There was a problem hiding this comment.
The reference mentions role with a description of role ID, which is very straightforward. By itself not so much. We can update the ADRs and references if needed. Let me know what you think!
c9a67dc to
bc3bd6b
Compare
6bbb22e to
442e19d
Compare
442e19d to
bc50391
Compare
mariajgrimaldi
left a comment
There was a problem hiding this comment.
LGTM! Thanks so much for addressing my comments!
(The comments left to address are only nits so I'll leave my approval! :))
Problem
Discovered resources are still opaque bytes. They need to be parsed into typed schema documents, with the source identity and content digest that later phases and the provenance tables depend on (ADR 0018 §1).
Approach
The load phase, plus the shared vocabulary the remaining phases are written against.
openedx_authz/engine/schema/loading.py—SchemaLoaderopenedx_authz/engine/schema/types.py— the schema dataclassesopenedx_authz/engine/schema/exceptions.py— the pipeline exception hierarchyopenedx_authz/constants/schema.py—SchemaOriginKindopenedx_authz/tests/schema/{factories,test_loading,test_types}.pytypes.pyandexceptions.pyland whole rather than sliced across the stack: they are the shared vocabulary, and splitting them would make every later PR conflict on the same two files. Some names in them (CompiledSchema,CompiledDefinition,RelationshipSource, and the compile/validate/apply errors) are unused until later PRs.Manual testing instructions
Rollback plan
Revert this PR. Nothing consumes the loader 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 (2/8) — #446 split into reviewable pieces. Bases chain bottom-up; merge in order.
rod/authz-schema-discovery)rod/authz-schema-compiler— schema compilationrod/authz-schema-validator— schema validationrod/authz-schema-models— definition models + migrationrod/authz-schema-renderer— policy rendererrod/authz-schema-applier— schema applierload_authz_schemacommand, version bump and changelogMerge checklist: