feat: allow automatic loading of required libraries when performing a domain import #1409
Conversation
WalkthroughThis pull request introduces enhancements to the library import functionality across multiple files. The core change is the addition of a new Changes
Possibly related PRs
Suggested Reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🪛 Ruff (0.8.2)backend/core/views.py2290-2290: (F405) 2294-2294: (F405) 2299-2299: (F405) ⏰ Context from checks skipped due to timeout of 90000ms (10)
🔇 Additional comments (3)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
…ibraries-when-performing-a-domain-import
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
backend/core/views.py (1)
2140-2143: Improve boolean parsing ofload_missing_librariesparameterThe current parsing of
load_missing_librariescompares the lowercase string directly to"true". To handle more truthy values and improve robustness, consider using Django's built-in utilities or a function likestrtobool.Apply this diff to enhance the parsing:
+from distutils.util import strtobool load_missing_libraries = ( - request.query_params.get("load_missing_libraries", "false").lower() + strtobool(request.query_params.get("load_missing_libraries", "false")) - == "true" )frontend/src/routes/(app)/(internal)/[model=urlmodel]/+page.server.ts (1)
104-106: Refactor endpoint URL construction for clarityWhen constructing the endpoint URL, consider separating the base URL and query parameters for better readability and maintainability.
Apply this diff to refactor the URL construction:
const endpoint = - `${BASE_API_URL}/folders/import/` + - (form.data.load_missing_libraries ? `?load_missing_libraries=true` : ''); + `${BASE_API_URL}/folders/import/${form.data.load_missing_libraries ? '?load_missing_libraries=true' : ''}`;Or use URLSearchParams for handling multiple query parameters:
+import { URLSearchParams } from 'url'; +const params = new URLSearchParams(); +if (form.data.load_missing_libraries) { + params.append('load_missing_libraries', 'true'); +} const endpoint = `${BASE_API_URL}/folders/import/${params.toString() ? '?' + params.toString() : ''}`;backend/serdes/utils.py (1)
519-530: LGTM! The library loading logic has been improved to include dependencies.The changes correctly implement a two-step process to load libraries:
- First, load directly related libraries
- Then, include their dependencies through a union operation
This ensures comprehensive library coverage during domain export.
Consider using a single query with
Qobjects to improve performance:- initial_loaded_libraries = LoadedLibrary.objects.filter( - Q(folder__in=folders) - | Q(threats__in=threats) - | Q(reference_controls__in=reference_controls) - | Q(risk_matrices__in=risk_matrices) - | Q(frameworks__in=frameworks) - ).distinct() - loaded_libraries = initial_loaded_libraries.union( - LoadedLibrary.objects.filter( - pk__in=initial_loaded_libraries.values_list("dependencies", flat=True) - ).distinct() - ) + loaded_libraries = LoadedLibrary.objects.filter( + Q(folder__in=folders) + | Q(threats__in=threats) + | Q(reference_controls__in=reference_controls) + | Q(risk_matrices__in=risk_matrices) + | Q(frameworks__in=frameworks) + | Q(pk__in=LoadedLibrary.objects.filter( + Q(folder__in=folders) + | Q(threats__in=threats) + | Q(reference_controls__in=reference_controls) + | Q(risk_matrices__in=risk_matrices) + | Q(frameworks__in=frameworks) + ).values_list("dependencies", flat=True)) + ).distinct()frontend/src/lib/utils/schemas.ts (1)
72-74: LGTM! The schema has been updated to support library loading control.The addition of
load_missing_librarieswithz.coerce.boolean().default(false)is well-implemented and safely defaults to false.Consider expanding the comment to explain why the coerce workaround is needed:
- //NOTE: coerce is a workaround because without we get an inconsistent error in form validation + //NOTE: coerce is used to handle checkbox form values which can be strings ('true'/'false') + //or booleans (true/false). Without coerce, form validation fails inconsistently when + //string values are received.frontend/messages/en.json (1)
1129-1130: LGTM! Clear and helpful messages for the new feature.The new messages provide clear UI text for the automatic library loading feature:
- A concise label for the control
- Helpful explanatory text that guides users on when to use this option
Consider adding a period at the end of the help text for consistency with other help texts in the file:
- "loadMissingLibrariesHelpText": "Load the missing libraries to complete the import if they are available", + "loadMissingLibrariesHelpText": "Load the missing libraries to complete the import if they are available.",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
backend/core/views.py(4 hunks)backend/library/utils.py(0 hunks)backend/serdes/utils.py(1 hunks)frontend/messages/en.json(1 hunks)frontend/src/lib/components/Forms/ModelForm.svelte(1 hunks)frontend/src/lib/components/Forms/ModelForm/FolderForm.svelte(2 hunks)frontend/src/lib/utils/schemas.ts(1 hunks)frontend/src/routes/(app)/(internal)/[model=urlmodel]/+page.server.ts(3 hunks)frontend/src/routes/(app)/(internal)/[model=urlmodel]/+page.svelte(1 hunks)
💤 Files with no reviewable changes (1)
- backend/library/utils.py
🧰 Additional context used
🪛 Ruff (0.8.2)
backend/core/views.py
2292-2292: LoadedLibrary may be undefined, or defined from star imports
(F405)
2296-2296: StoredLibrary may be undefined, or defined from star imports
(F405)
2301-2301: StoredLibrary may be undefined, or defined from star imports
(F405)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: startup-docker-compose-test
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: test (3.12)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: build (3.12)
🔇 Additional comments (8)
backend/core/views.py (2)
2292-2292: Address possible undefined variables reported by static analysisStatic analysis indicates that
LoadedLibrarymay be undefined. Ensure thatLoadedLibraryis properly imported.🧰 Tools
🪛 Ruff (0.8.2)
2292-2292:
LoadedLibrarymay be undefined, or defined from star imports(F405)
2296-2296: Address possible undefined variables reported by static analysisStatic analysis indicates that
StoredLibrarymay be undefined. Ensure thatStoredLibraryis properly imported.🧰 Tools
🪛 Ruff (0.8.2)
2296-2296:
StoredLibrarymay be undefined, or defined from star imports(F405)
frontend/src/lib/components/Forms/ModelForm/FolderForm.svelte (2)
5-5: Import ofCheckboxcomponent is necessaryThe import of the
Checkboxcomponent is correctly added and used in the form, ensuring the new checkbox functionality works as intended.
20-25: Implementation ofload_missing_librariescheckbox is correctThe
Checkboxcomponent forload_missing_librariesis properly implemented with correct bindings to the form. Labels and help texts are correctly sourced from the messages module.frontend/src/routes/(app)/(internal)/[model=urlmodel]/+page.server.ts (1)
74-74: EnsurefolderImportModelis utilized correctlyThe
folderImportModelis added to themodelobject. Verify that this new property is correctly referenced in the frontend components to prevent any undefined property issues.frontend/src/routes/(app)/(internal)/[model=urlmodel]/+page.svelte (1)
57-57: LGTM! The model property access has been updated correctly.The change to access
folderImportModelfromdata.modelaligns with the new data structure supporting the library loading feature.frontend/src/lib/components/Forms/ModelForm.svelte (1)
192-193: LGTM! The form rendering logic has been enhanced.The changes correctly:
- Support both 'folders' and 'folders-import' URL models
- Simplify the props passed to FolderForm
frontend/messages/en.json (1)
1128-1128: LGTM! Improved message clarity.The updated message now clearly indicates that libraries can be either missing or outdated, which better reflects the actual state of dependencies during import.
backend/core/views.py
Outdated
| if not LoadedLibrary.objects.filter( | ||
| urn=library["urn"], version=library["version"] | ||
| ).exists(): | ||
| if ( | ||
| StoredLibrary.objects.filter( | ||
| urn=library["urn"], version=library["version"] | ||
| ).exists() | ||
| and load_missing_libraries | ||
| ): | ||
| StoredLibrary.objects.get( | ||
| urn=library["urn"], version=library["version"] | ||
| ).load() | ||
| else: | ||
| missing_libraries.append(library) |
There was a problem hiding this comment.
Import missing models LoadedLibrary and StoredLibrary
The models LoadedLibrary and StoredLibrary are used but not imported in this module, which will result in a NameError at runtime.
Add the necessary imports at the top of the file:
+from core.models import LoadedLibrary, StoredLibraryCommittable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.8.2)
2292-2292: LoadedLibrary may be undefined, or defined from star imports
(F405)
2296-2296: StoredLibrary may be undefined, or defined from star imports
(F405)
2301-2301: StoredLibrary may be undefined, or defined from star imports
(F405)
Mohamed-Hacene
requested review from
ab-smith,
nas-tabchiche and
eric-intuitem
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
enterprise/frontend/src/lib/components/Forms/ModelForm/FolderForm.svelte(2 hunks)frontend/src/lib/components/Forms/ModelForm.svelte(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/lib/components/Forms/ModelForm.svelte
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: build (3.12)
- GitHub Check: Analyze (python)
- GitHub Check: test (3.12)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
enterprise/frontend/src/lib/components/Forms/ModelForm/FolderForm.svelte (1)
Line range hint
7-16: LGTM! Clean component setup with proper type definitions.The import statements and component setup are well-structured. The addition of the Checkbox component aligns with the PR objective of allowing control over library loading during import.
| <FileInput | ||
| {form} | ||
| field="name" | ||
| label={m.name()} | ||
| cacheLock={cacheLocks['name']} | ||
| bind:cachedValue={formDataCache['name']} | ||
| data-focusindex="0" | ||
| allowPaste={true} | ||
| field="file" | ||
| label={m.file()} | ||
| allowedExtensions={['bak', 'zip']} | ||
| helpText={m.importFolderHelpText()} | ||
| /> | ||
| <Checkbox | ||
| {form} | ||
| field="load_missing_libraries" | ||
| label={m.loadMissingLibraries()} | ||
| helpText={m.loadMissingLibrariesHelpText()} |
There was a problem hiding this comment.
💡 Codebase verification
Frontend file extension validation doesn't match backend requirements
The frontend allows both .bak and .zip files, but the backend only processes ZIP files containing a data.json. Remove .bak from the allowed extensions in FolderForm.svelte to prevent users from uploading unsupported file types.
enterprise/frontend/src/lib/components/Forms/ModelForm/FolderForm.svelte: UpdateallowedExtensionsto only include['zip']
🔗 Analysis chain
LGTM! Well-structured form with clear user guidance.
The form implementation provides a good user experience with:
- Clear file input restrictions (.bak, .zip)
- Explicit control over library loading through the checkbox
- Helpful guidance text for both inputs
Consider adding frontend validation to ensure the file extension check is consistent with backend expectations.
Let's verify the file extension validation consistency:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for file extension validation in backend views
rg -A 5 "allowed.*extension" backend/
Length of output: 1091
Script:
#!/bin/bash
# Search for folder import related views and validation
rg -l "folder.*import|import.*folder" backend/
rg -A 5 "\.bak|\.zip" backend/
Length of output: 3552
Script:
#!/bin/bash
# Search for import views, serializers, and models
rg -A 5 "class.*Import.*View|class.*Upload.*View" backend/
rg -A 5 "@action.*import|def.*import" backend/
rg -A 5 "class.*ImportSerializer" backend/
Length of output: 14448
Script:
#!/bin/bash
# Search for folder serializer and import validation
rg -A 10 "class FolderImportExportSerializer" backend/
rg -B 5 -A 10 "def import_domain" backend/
Length of output: 1688
Script:
#!/bin/bash
# Search for file processing implementation
rg -B 2 -A 10 "_process_uploaded_file" backend/
Length of output: 1798
ab-smith
changed the title
nas-tabchiche
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
backend/core/views.py (2)
16-16: Consider alternatives todistutils.util.strtobool.
distutilsis deprecated in Python 3.12. For forward compatibility, a recommended approach is to define a helper function or use an available library function (such as fromdjango.utils) to parse boolean-like strings.
2242-2244: Document the new parameter in_import_objects.
Enhance the method docstring or inline comments to clarify howload_missing_librariesinfluences the import process.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/core/views.py(5 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
backend/core/views.py
2290-2290: LoadedLibrary may be undefined, or defined from star imports
(F405)
2294-2294: StoredLibrary may be undefined, or defined from star imports
(F405)
2299-2299: StoredLibrary may be undefined, or defined from star imports
(F405)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: startup-docker-compose-test
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: build (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: test (3.12)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
backend/core/views.py (3)
2139-2141: Potential exception for invalid query param.
If theload_missing_librariesquery parameter doesn't match valid strings liketrueorfalse,strtoboolcan raise aValueError. Consider handling malformed inputs gracefully.
2147-2149: Implementation looks correct.
This call to_import_objectsproperly threads the newload_missing_librariesparameter.
2290-2303: Handle missing imports and verify library version check.
- Ensure
LoadedLibraryandStoredLibraryare properly imported to avoid NameErrors.- Double-check whether
version__gteis intentional. If only exact matches are valid, you may wantversion=library["version"].🧰 Tools
🪛 Ruff (0.8.2)
2290-2290:
LoadedLibrarymay be undefined, or defined from star imports(F405)
2294-2294:
StoredLibrarymay be undefined, or defined from star imports(F405)
2299-2299:
StoredLibrarymay be undefined, or defined from star imports(F405)
| required_libraries.append( | ||
| {"urn": fields["urn"], "version": fields["version"]} | ||
| ) | ||
| logger.info( | ||
| "Adding library to required libraries", urn=fields["library"] | ||
| "Adding library to required libraries", urn=fields["urn"] | ||
| ) | ||
| continue | ||
| if fields.get("library"): |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Confirm completeness of the library creation logic.
Currently, objects of type LoadedLibrary are skipped rather than created. Validate usage scenarios to ensure that libraries needed during import are properly persisted.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor