feat: Automatically load/update appropriate libs and improve mapping engine invalidation #2905
Conversation
…engine cache invalidation
WalkthroughThe pull request refactors library storage and autoloading mechanisms. StoredLibrary.store_library_content() now returns tuples indicating newly stored and outdated libraries. Signal handlers are added for cache invalidation on model changes. Library autoload logic is updated with version comparison, and associated views and management commands are adjusted accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant signals as Signal Handlers
participant engine as Engine Cache
participant db as Database
autonumber
Caller->>db: post_save(RequirementMappingSet) or post_delete
db->>signals: Trigger invalidate_requirement_mapping_set_cache
signals->>engine: engine.load_rms_data()
engine->>db: Reload RMS data
engine-->>signals: Cache updated
Caller->>db: post_save(Framework) or post_delete
db->>signals: Trigger invalidate_framework_cache
signals->>engine: engine.load_frameworks()
engine->>db: Reload frameworks
engine-->>signals: Cache updated
sequenceDiagram
participant Caller as Caller/View
participant Models as Models
participant AutoloadCmd as Autoload Command
participant Loader as LoadedLibrary
autonumber
Caller->>Models: store_library_content(data)
Models->>Models: Determine autoload flag from library_data
Models-->>Caller: (new_library, outdated_library)
AutoloadCmd->>Models: Query LoadedLibrary where autoload=True
Models->>Models: Compare version vs max(StoredLibrary.version)
Models-->>AutoloadCmd: libs_to_update
AutoloadCmd->>Loader: lib.update() for each in libs_to_update
Loader->>Models: Update LoadedLibrary with new version
Models-->>Loader: Updated
Loader-->>AutoloadCmd: Confirm/Error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/library/views.py (1)
287-323: Handle(None, None)result fromstore_library_contentand re-examine upload RBACThe new upload flow mostly aligns with the
(library, outdated_library)API, but there are a couple of important edge cases:
- Bug when no new library is stored
StoredLibrary.store_library_contentcan legitimately return(None, None)(e.g., same checksum already stored, same or older version, etc.). In that case:library, outdated_library = StoredLibrary.store_library_content(content) # ... return Response( StoredLibrarySerializer(library).data, status=HTTP_201_CREATED )Here
libraryisNone, soStoredLibrarySerializer(library).datawill assert/raise and be caught only by the outer broadexcept, resulting in an"invalidLibraryFileError"instead of a clear “library already stored / no update” response.You likely want an explicit guard before serializing:
- library, outdated_library = StoredLibrary.store_library_content(content) - - if library is not None: + library, outdated_library = StoredLibrary.store_library_content(content) + + if library is None and outdated_library is None: + # No new stored library; checksum/version already present. + # Adjust status/body to whatever UX you prefer. + return HttpResponse( + json.dumps({"error": "libraryAlreadyStored"}), + status=HTTP_400_BAD_REQUEST, + ) + + if library is not None: if not (_has_update := outdated_library is not None): response = self._import_library(request, library.urn) if response is not None: return response else: if error_msg := outdated_library.update(): return Response( error_msg, status=status.HTTP_400_BAD_REQUEST )This avoids the crash and lets you send a specific, predictable outcome when nothing changes.
- RBAC semantics on upload vs import/update
- The upload endpoint correctly checks
add_storedlibraryup front.- When a new library is created and
outdated_library is None, you unconditionally call_import_library, which requiresadd_loadedlibrary. If the caller lacks that permission, they get a 403 from_import_library, but theStoredLibraryrow has already been created. That’s a bit surprising: a 403 response after a successful write.- When an existing loaded library is updated (
outdated_library is not None), you calloutdated_library.update()without checkingadd_loadedlibrary, effectively allowing any caller withadd_storedlibraryto trigger a load/update.It would be good to confirm whether this mixing of
add_storedlibraryandadd_loadedlibrarysemantics is intentional. If not, options include:
- Only auto‑import/auto‑update when the caller also has
add_loadedlibrary, otherwise just store and return the newStoredLibrary.- Or treat upload as an “admin‑only” pathway that’s allowed to manage both stored and loaded libraries, and adjust permissions accordingly.
backend/core/models.py (1)
264-363: Fixoutdated_loaded_librarylocale handling and align return contract docsThere are a few things worth tightening up in
store_library_content:
Potentially wrong
LoadedLibraryselected (locale missing + arbitrary version)
outdated_loaded_libraryis currently resolved only byurnandversion__lt=version, so if you ever have multiple locales for the same URN you may:
- Pick a
LoadedLibrarywith a differentlocalethan the newly storedStoredLibrary.- Pick an arbitrary older version instead of the latest one below
version.This can cause the wrong loaded library to be treated as “updatable” for this stored content. You almost certainly want to match the same locale and, if several versions exist, prefer the highest one below the new version.
Consider updating the query like this:
outdated_loaded_library = LoadedLibrary.objects.filter(urn=urn, version__lt=version).first()
outdated_loaded_library = (LoadedLibrary.objects.filter(urn=urn,locale=locale,version__lt=version,).order_by("-version").first())
Docstring still mentions
Falseinstead ofNone
The docstring describeslibraryas “otherwise False”, but all early‑exit paths now return(None, None). To avoid surprising callers:
- `library`: The newly created `StoredLibrary` object on success, otherwise False
- `library`: The newly created `StoredLibrary` object on success, otherwise None
Optional readability improvement for return type
Not blocking, but the return annotation could be made easier to read as a single tuple of optionals instead of a union of two Tuple shapes, e.g.:def store_library_content( cls, library_content: bytes, builtin: bool = False ) -> tuple["StoredLibrary | None", "LoadedLibrary | None"]: ...This would still express the
(None, None)/(StoredLibrary, LoadedLibrary | None)contract cleanly.
🧹 Nitpick comments (4)
backend/library/tests/test_store_library_content.py (1)
140-155: Tests correctly adapted to tuple return fromstore_library_contentUnpacking
(stored_library, _)matches the new(library, outdated_library)contract and the autoload assertions remain valid for the provided YAML samples. You might later add a dedicated test that asserts theoutdated_librarybehavior when uploading a higher-version library, but that’s optional.backend/core/signals.py (1)
1-15: Signal-driven cache invalidation works; review transaction semanticsHooking
post_save/post_deleteonRequirementMappingSetandFrameworkto callengine.load_rms_data()/engine.load_frameworks()is a clean way to keep the in‑memory mappings in sync with model changes and satisfies the stated invalidation needs.Be aware though:
- In Django,
post_save/post_deleteare triggered when the ORM executes the operation, not strictly “after commit”. If you perform changes insidetransaction.atomicblocks that later roll back, the engine may have loaded data that no longer exists in the DB. For strict transaction awareness, consider wrapping the engine calls intransaction.on_commit(...).- If you expect bulk updates or high write throughput on these models, repeatedly recomputing the full mappings on every row change could become expensive; batching or debouncing invalidations might be useful in that scenario.
backend/library/management/commands/autoloadlibraries.py (1)
5-8: Autoupdate query is correct; consider scoping by locale and tightening error handlingThe autoload + autoupdate flow is implemented correctly: you first load missing autoloaded libraries, then select
LoadedLibraryrows whoseversionis less than the maxStoredLibrary.versionfor the sameurnwithautoload=True, and calllib.update()with appropriate logging.Two refinements to consider:
- Scope by locale:
libs_to_updateonly constrains onurn. If you have multiple locales for the same URN, you’ll attempt updates for locales that may have no newerStoredLibraryrow, resulting inupdate()returning"libraryHasNoUpdate"and an error log. Addinglocale=OuterRef("locale")to the subquery filter would avoid needless work and clearer logs.- Differentiate expected “no update” from real failures: Right now every non‑
Noneerror_msgis logged as “Failed to automatically update library”. If"libraryHasNoUpdate"is a benign outcome, you might treat it separately from real errors, or avoid logging it at error level to keep logs actionable.Also applies to: 15-44
backend/library/views.py (1)
228-264: Helper_import_librarycan return a plainResponseand be called directlyThe new
_import_libraryhelper cleanly centralizes permission checks, lookup, and load error handling, which is a nice refactor. Given the current implementation:
_import_libraryalways returns aResponse; there is no branch that yieldsNone.import_libraryonly unwraps the helper and returns the value if notNone.You can simplify this to make the flow clearer:
- Change the return annotation to
-> Response.- Have
import_libraryjustreturn self._import_library(request, pk)without theif (response := ...) is not Noneguard.This keeps behavior identical but tightens types and reduces indirection.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
backend/core/apps.py(1 hunks)backend/core/mappings/engine.py(0 hunks)backend/core/models.py(6 hunks)backend/core/signals.py(1 hunks)backend/library/management/commands/autoloadlibraries.py(2 hunks)backend/library/tests/test_store_library_content.py(1 hunks)backend/library/views.py(5 hunks)
💤 Files with no reviewable changes (1)
- backend/core/mappings/engine.py
🧰 Additional context used
🧬 Code graph analysis (5)
backend/core/signals.py (3)
backend/core/models.py (2)
RequirementMappingSet(1754-1774)Framework(1579-1660)backend/core/views.py (1)
mappings(5558-5567)backend/core/mappings/engine.py (2)
load_rms_data(57-92)load_frameworks(94-100)
backend/library/tests/test_store_library_content.py (1)
backend/core/models.py (2)
StoredLibrary(243-385)store_library_content(264-362)
backend/library/management/commands/autoloadlibraries.py (1)
backend/core/models.py (3)
LoadedLibrary(876-992)StoredLibrary(243-385)update(882-894)
backend/core/models.py (1)
backend/core/utils.py (1)
sha256(37-41)
backend/library/views.py (2)
backend/iam/models.py (4)
RoleAssignment(807-1094)is_access_allowed(837-857)Folder(68-316)get_root_folder(75-77)backend/core/models.py (3)
StoredLibrary(243-385)store_library_content(264-362)update(882-894)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: startup-docker-compose-test
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: build_community_frontend
- GitHub Check: build_enterprise_frontend
🔇 Additional comments (3)
backend/core/apps.py (1)
13-18: Signal module import inready()is appropriateImporting
core.signalsinsideCoreConfig.ready()is a standard way to register signal handlers and placing it before theRUN_MAINguard ensures they are wired in all processes while keeping existingpost_migratebehavior intact.backend/core/models.py (2)
7-7: Typing imports match usageThe added
TupleandOptionalimports are used in the newstore_library_contentannotation and look consistent with the rest of the file’s typing style.
371-372: Usage of new tuple return shape looks correct
store_library_filecorrectly unpacks the(new_library, outdated_loaded_library)tuple and returns onlynew_library, preserving the previous public API (StoredLibrary | None) for callers of this helper. Ignoring the second element here makes sense for file‑based loading where any update action is handled elsewhere.
Note for myself: Failing tests has to be fixed:
core/signals.pyfile which imports theFrameworkmodel (which may not be created in the database since it may this file may be imported and therefore executed before the Framework table creation migration was applied).What this PR does:
autoload=Trueat startup.Using signals to invalidate the MappingEngine cache gives the ability to the cache to react to
on_delete=models.CASCADEoriginated deletions.It's also more convenient for transaction support as the post_save and post_delete signals are only fired when a transaction executes successfully.
Summary by CodeRabbit
New Features
Improvements
Changes