Repository navigation
feat: add filesystem-path schema discovery and multi-document YAML loading - #534
Conversation
|
Thanks for the pull request, @carlos-marquez-wgu! 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. |
dbddb51 to
3cb134e
Compare
…ading Implement ADR 0029: Tutor integration for authorization schema discovery. - Add OPENEDX_AUTHZ_SCHEMA_FILES Django setting for discovering schema files by absolute filesystem path (discovery.py) - Introduce DiscoveredFileResource and SourceFileRecord with sentinel values for non-package sources - Switch YAML parsing from safe_load to safe_load_all so each schema resource is treated as a multi-document stream (loading.py) - Add tests for filesystem discovery, multi-document streams, and filesystem source identity - Add tutor plugin
3135324 to
1c20ec7
Compare
1c20ec7 to
3953580
Compare
|
Hi @openedx/committers-openedx-authz, this PR is ready for reviews |
Description
Implements ADR 0029: Tutor integration for authorization schema discovery.
OPENEDX_AUTHZ_SCHEMA_FILESDjango setting for discovering schema files by absolute filesystem path (discovery.py)DiscoveredFileResourceandSourceFileRecordwith sentinel values for non-package sourcessafe_loadtosafe_load_allso each schema resource is treated as a multi-document stream (loading.py)openedx-authz-schemapatch. Most of it is the output ofcookiecutter, leaving it as is for future tutor related changes regardingopenedx-authzTesting instructions
tutor plugins enable authztouch $(tutor config printroot)-plugins/my_authz_schema.pymy_authz_schema.pytutor plugins enable my_authz_schemado inittasks or run theload_authz_schemamanagement command directlytutor dev do initMerge checklist:
Check off if complete or not applicable:
Concerns
As per the conversation in this PR, we are taking the approach of setting
moduleasnullfor file system contributions, what I noted while implementing the changes is that given the tableopenedx_authz_authzschemasourcehas this unique constraint:And for file system contributions, the identifying source is always
(<tutor-patch>, NULL), which means all file system contributions collapse into a single source row. MySQL treatsNULL!=NULLin unique constraints, so the constraint does not prevent duplicate(<tutor-patch>, NULL)rows. The renderer cache handles the single-row case correctly and doesn't break, but if for any reason we get two or more rows with(<tutor-patch>, NULL)(concurrent deployments or something else), renderer.py#L563 will throw aMultipleObjectsReturnederror.Additionally, the single-row constraint means per-file provenance is lost — if
OPENEDX_AUTHZ_SCHEMA_FILESlists multiple files, you can't distinguish which file contributed which definition at the database level.It is probably unlikely to hit this path, but I wanted to note it as an existing risk, I can add additional code to handle that situation gracefully but wanted to bring this on before I keep going through the rabbit hole (what are we going to do if we hit that situation?)
Alternatively, if it's acceptable, I wanted to bring the other option we discussed in the linked PR comment, and that one being using the file system absolute path as the module, I understand this breaks the pattern of
module=python module, but we are already breaking a convention by allowingdistributionbeing just a sentinel value. With the sentinel value approach, this is clearer (just drop the last commit in this PR).I went ahead and implemented it with
module=nullin mind, but wanted to bring this up so we can choose to take one trade off or another (more solutions are welcome, thanks!)AI Usage
Kiro + Claude were used to assist with the creation of the tests and code implementations while thoroughly and carefully guided, everything was reviewed and corrected manually by an actual person and ensured the changes were up to standard and met the closing issue requirements.
Closes: