feat: Handle multiple requirement mapping sets in libraries #1943
Conversation
WalkthroughThe changes introduce improved validation, error handling, and backward compatibility for library importers and updaters, enabling support for multiple frameworks, risk matrices, and requirement mapping sets. Typos in keys and documentation are corrected, and requirement mapping sets now support reversible mappings. Filtering and API responses are updated for deprecated field handling, and documentation is aligned with code changes. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant LibraryImporter
participant LibraryUpdater
participant FrameworkImporter
participant RequirementMappingSetImporter
User->>LibraryImporter: Import library (with multiple objects)
LibraryImporter->>FrameworkImporter: init_framework (list of frameworks)
FrameworkImporter-->>LibraryImporter: error string or None
LibraryImporter->>RequirementMappingSetImporter: init_requirement_mapping_set (list of sets)
RequirementMappingSetImporter-->>LibraryImporter: error string or None
LibraryImporter->>LibraryUpdater: check_and_import_dependencies()
LibraryUpdater-->>LibraryImporter: error string or None
LibraryImporter-->>User: Success or error message
sequenceDiagram
participant Script
participant convert_library_v2.py
Script->>convert_library_v2.py: create_library(input_file, output_file)
convert_library_v2.py->>convert_library_v2.py: Process original mapping set
convert_library_v2.py->>convert_library_v2.py: Generate reversed mapping set
convert_library_v2.py->>convert_library_v2.py: Store both sets in output
convert_library_v2.py-->>Script: Output file with original and reversed mappings
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (9)
🔇 Additional comments (1)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/library/utils.py(14 hunks)documentation/architecture/data-model.md(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: test (3.12)
- GitHub Check: build (3.12)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
documentation/architecture/data-model.md (1)
95-95: Typo fixed – looks goodThe ER diagram now uses the correct entity name
REQUIREMENT_MAPPING_SET. ✅
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
backend/core/models.py (1)
351-355: 💡 Verification agent🧩 Analysis chain
Investigate pipeline failure in
load()methodThere's a pipeline failure related to this method:
Error in library.load() method: AttributeError due to missing '_library_data' attribute in LibraryImporter.
This suggests that the LibraryImporter interface might have changed and needs to be addressed.
🏁 Script executed:
#!/bin/bash # Look for LibraryImporter implementation to check its interface rg -A 10 "class LibraryImporter" backend/Length of output: 762
🏁 Script executed:
#!/bin/bash # Show class definition and methods of LibraryImporter rg -n -C5 "class LibraryImporter" backend/library/utils.py rg -n -C3 "def import_library" backend/library/utils.py rg -n -C3 "def load" backend/library/utils.pyLength of output: 1330
🏁 Script executed:
#!/bin/bash # Find the init method in LibraryImporter rg -n -C5 "def init" backend/library/utils.py # Search for references to _library_data within LibraryImporter rg -n "_library_data" -C5 backend/library/utils.pyLength of output: 5469
Fix LibraryImporter’s missing
_library_dataattribute and align method namesThere’s no
_library_dataset onLibraryImporter, yet its finalinit()refers to it—this causes theAttributeErroryou’re seeing inlibrary.load(). Also, per the file-level directive the importer should useloadrather thanimport.Please update as follows:
• backend/library/utils.py:
- In
class LibraryImporter, theinit()at lines ~605–610 should useself.data(which is already initialized) instead ofself._library_data, or else add an__init__that sets_library_data = data.- Rename
import_library()toload()to match the file comment, and adjust its callers accordingly.Example diff for the
initmethod inLibraryImporter:- def init(self) -> Union[str, None]: - missing_fields = self.REQUIRED_FIELDS - set(self._library_data.keys()) + def init(self) -> Union[str, None]: + # Use the already-set `self.data` rather than the unset `_library_data` + missing_fields = self.REQUIRED_FIELDS - set(self.data.keys()) if missing_fields: return "The following fields are missing in the library: {}".format( ", ".join(missing_fields) )• backend/core/models.py (around lines 351–355):
- If you rename
import_library()toload(), update the call accordingly:- error_msg = library_importer.import_library() + error_msg = library_importer.load() if error_msg is None: self.is_loaded = True self.save() return error_msg🧰 Tools
🪛 GitHub Actions: Backend code coverage
[error] 351-351: Error in library.load() method: AttributeError due to missing '_library_data' attribute in LibraryImporter.
🧹 Nitpick comments (8)
backend/core/models.py (1)
7-7: Remove unused importTypeAccording to static analysis, this import is unused in the file.
-from typing import Self, Type, Union, List +from typing import Self, Union, List🧰 Tools
🪛 Ruff (0.8.2)
7-7:
typing.Typeimported but unusedRemove unused import:
typing.Type(F401)
backend/library/utils.py (7)
132-134: Improve exception handling with explicit exception chainingThe static analysis tool suggests improving exception handling to preserve the original exception context.
- error_msg = f"ERROR: target requirement with URN {self.data['target_requirement_urn']} does not exist" - print(error_msg) - raise Http404(error_msg) + error_msg = f"ERROR: target requirement with URN {self.data['target_requirement_urn']} does not exist" + print(error_msg) + raise Http404(error_msg) from None🧰 Tools
🪛 Ruff (0.8.2)
134-134: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
140-142: Improve exception handling with explicit exception chainingSimilar to the previous comment, add explicit exception chaining here as well.
- error_msg = f"ERROR: source requirement with URN {self.data['source_requirement_urn']} does not exist" - print(error_msg) - raise Http404(error_msg) + error_msg = f"ERROR: source requirement with URN {self.data['source_requirement_urn']} does not exist" + print(error_msg) + raise Http404(error_msg) from None🧰 Tools
🪛 Ruff (0.8.2)
142-142: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
597-603: Fix typo and inconsistent index usage in error messageThere's a typo in the error message and the index is used inconsistently.
- return "[FRAMEWORK_ERROR] {} invaldi framework{} detected, the {}{} framework has the following error : {}".format( + return "[FRAMEWORK_ERROR] {} invalid framework{} detected, the {}{} framework has the following error : {}".format( len(import_errors), "s" if len(import_errors) > 1 else "", - invalid_framework_index, + invalid_framework_index + 1, {1: "st", 2: "nd", 3: "rd"}.get(invalid_framework_index + 1, "th"), invalid_framework_error, )
629-639: Add logging instead of using print statementsConsider using the logger instead of print statements for consistency with the rest of the codebase.
if ( framework_import_error := self.init_framework(framework_data) ) is not None: - print( - "framework_import_error", - framework_import_error, - ) + logger.error("Framework import error", error=framework_import_error) return framework_import_error
660-664: Replace print statements with loggingSimilar to the previous comment, use the logger instead of print statements for consistency.
if ( requirement_mapping_set_import_error := self.init_requirement_mapping_set(requirement_mapping_set_data) ) is not None: - print( - "requirement_mapping_set_import_error", - requirement_mapping_set_import_error, - ) + logger.error("Requirement mapping set import error", error=requirement_mapping_set_import_error) return requirement_mapping_set_import_error
669-670: Replace print statement with loggingFor consistency, use the logger here as well.
if (threat_import_error := self.init_threats(threat_data)) is not None: - print("threat errors", threat_import_error) + logger.error("Threat import error", error=threat_import_error) return threat_import_error
785-786: Replace print statement with loggingFor better debugging and consistency, use the logger instead of print statements.
- print("::: Getting Dependencies :::") + logger.info("Getting dependencies for library import") error_msg = self.check_and_import_dependencies() - print("::: Dependencies are ok :::") + logger.info("Dependencies verified successfully")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/core/models.py(10 hunks)backend/library/utils.py(15 hunks)documentation/architecture/data-model.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- documentation/architecture/data-model.md
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/library/utils.py (1)
backend/core/models.py (7)
RequirementNode(1187-1257)StoredLibrary(232-355)frameworks(942-943)frameworks(1008-1009)LoadedLibrary(798-908)load(344-355)update(804-816)
🪛 Ruff (0.8.2)
backend/core/models.py
7-7: typing.Type imported but unused
Remove unused import: typing.Type
(F401)
backend/library/utils.py
134-134: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
142-142: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🪛 GitHub Actions: Backend code coverage
backend/core/models.py
[error] 351-351: Error in library.load() method: AttributeError due to missing '_library_data' attribute in LibraryImporter.
backend/library/utils.py
[error] 606-606: AttributeError: 'LibraryImporter' object has no attribute '_library_data' during library import initialization.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
🔇 Additional comments (12)
backend/core/models.py (7)
302-304: LGTM: Unified handling of object counts inobjects_metaThe change now consistently handles the object count for all object types, removing special case handling for "requirement_mapping_set" which aligns with the PR's goal of unifying singular/plural object handling.
363-385: Good implementation of normalized object field handlingThis change properly standardizes the handling of singular and plural forms for frameworks, risk matrices, and requirement mapping sets, providing backward compatibility for deprecated singular field names. The implementation correctly handles both single objects (as dictionaries) and multiple objects (as lists).
464-473: Good refactoring of object dictionariesSeparating i18n_object_dict and referential_object_dict improves code organization and makes it clearer what properties are being applied to new/updated objects.
498-696: Improved framework update logic for multiple frameworksThe updated logic now properly handles multiple frameworks. The code has been restructured to iterate over each framework in the list and process its requirement nodes correctly, supporting the PR's objective of multiple objects of the same type.
725-796: Good implementation of RequirementMappingSet update logicThe new code for handling requirement mapping sets correctly:
- Supports multiple sets
- Skips creation of new sets during updates (only updates existing ones)
- Prevents changing source/target frameworks of existing sets
- Processes mappings within each set
405-407: Consistent error variable namingThis change from
err_msgtoerror_msgimproves variable naming consistency across the codebase.
415-416: Consistent error variable namingThis change from
err_msgtoerror_msgimproves variable naming consistency across the codebase.backend/library/utils.py (5)
678-682: Approve handling of singular risk_matrix objectsGood implementation of backward compatibility for risk matrices. This properly handles the case when a singular risk_matrix object is provided as a dictionary instead of a list.
652-656: Approve backward compatibility for requirement mapping setsGood implementation of backward compatibility for requirement mapping sets. This properly handles the case when a singular requirement_mapping_set object is provided as a dictionary instead of a list.
630-632: Approve backward compatibility for frameworksGood implementation of backward compatibility for frameworks. This properly handles the case when a singular framework object is provided as a dictionary instead of a list.
162-164: Approve addition of is_empty methodGood addition of the
is_emptymethod to check for empty requirement mapping sets. This improves error handling and validation.
754-771: Approve update to import_objects methodGood update to the
import_objectsmethod to work with lists of importers instead of single importers. This aligns with the changes to support multiple frameworks and requirement mapping sets.
backend/core/models.py
Outdated
| "stregth_of_relationship", | ||
| ] | ||
| } | ||
| requirement_mapping_dict["strength_of_relationship"] = ( | ||
| requirement_mapping["stregth_of_relationship"] | ||
| ) # # Fix the typo caused by the convert_library.py code. | ||
| source_requirement = RequirementNode.objects.get( |
There was a problem hiding this comment.
Fix inconsistency with "stregth_of_relationship" typo
There's a comment about fixing a typo in the field name, but the code is inconsistent. The dictionary excludes "stregth_of_relationship" from being copied directly, but then uses this misspelled key to get the value. This might cause issues with future library updates if the format is fixed.
- requirement_mapping_dict["strength_of_relationship"] = (
- requirement_mapping["stregth_of_relationship"]
- ) # # Fix the typo caused by the convert_library.py code.
+ # Handle both spellings for backward compatibility
+ requirement_mapping_dict["strength_of_relationship"] = (
+ requirement_mapping.get("strength_of_relationship") or
+ requirement_mapping.get("stregth_of_relationship")
+ ) # Fix the typo caused by the convert_library.py code.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "stregth_of_relationship", | |
| ] | |
| } | |
| requirement_mapping_dict["strength_of_relationship"] = ( | |
| requirement_mapping["stregth_of_relationship"] | |
| ) # # Fix the typo caused by the convert_library.py code. | |
| source_requirement = RequirementNode.objects.get( | |
| "stregth_of_relationship", | |
| ] | |
| } | |
| # Handle both spellings for backward compatibility | |
| requirement_mapping_dict["strength_of_relationship"] = ( | |
| requirement_mapping.get("strength_of_relationship") | |
| or requirement_mapping.get("stregth_of_relationship") | |
| ) # Fix the typo caused by the convert_library.py code. | |
| source_requirement = RequirementNode.objects.get( |
backend/library/utils.py
Outdated
|
|
||
| def init(self) -> Union[str, None]: | ||
| """missing_fields = self.REQUIRED_FIELDS - set(self._library_data.keys()) | ||
| missing_fields = self.REQUIRED_FIELDS - set(self._library_data.keys()) |
There was a problem hiding this comment.
Fix AttributeError by using the correct attribute name
The pipeline is failing with AttributeError: 'LibraryImporter' object has no attribute '_library_data'. You should use self._library.content instead, which is correctly used elsewhere in the code (e.g., line 612).
- missing_fields = self.REQUIRED_FIELDS - set(self._library_data.keys())
+ missing_fields = self.REQUIRED_FIELDS - set(self._library.content.keys())📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| missing_fields = self.REQUIRED_FIELDS - set(self._library_data.keys()) | |
| missing_fields = self.REQUIRED_FIELDS - set(self._library.content.keys()) |
🧰 Tools
🪛 GitHub Actions: Backend code coverage
[error] 606-606: AttributeError: 'LibraryImporter' object has no attribute '_library_data' during library import initialization.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
backend/core/models.py (1)
713-728: Fix persistent issue with misspelled relationship field.The code still doesn't handle both spellings of "strength_of_relationship" for backward compatibility as previously suggested. This could cause data loss if both field versions exist.
requirement_mapping_dict = { key: value for key, value in requirement_mapping.items() if key not in [ "source_requirement_urn", "target_requirement_urn", "stregth_of_relationship", ] } -requirement_mapping_dict["strength_of_relationship"] = ( - requirement_mapping["stregth_of_relationship"] -) # # Fix the typo caused by the convert_library.py code. +# Handle both spellings for backward compatibility +requirement_mapping_dict["strength_of_relationship"] = ( + requirement_mapping.get("strength_of_relationship") or + requirement_mapping.get("stregth_of_relationship") +) # Fix the typo caused by the convert_library.py code.
🧹 Nitpick comments (9)
backend/core/models.py (4)
7-7: Remove unused import.The
Typeimport from typing module is not used in the code.-from typing import Self, Type, Union, List +from typing import Self, Union, List🧰 Tools
🪛 Ruff (0.11.9)
7-7:
typing.Typeimported but unusedRemove unused import:
typing.Type(F401)
672-672: Fix typo in method name.Method name has a typo - "udpate" should be "update".
-def udpate_requirement_mapping_sets(self): +def update_requirement_mapping_sets(self):This typo should also be corrected in the caller at line 809.
789-797: Remove redundant dictionary definitions.These dictionaries are already defined at initialization and available as instance attributes. The redefinitions here are unnecessary.
- i18n_object_dict = { - "locale": self.old_library.locale, - "default_locale": self.old_library.default_locale, - } - - referential_object_dict = { - "provider": self.new_library.provider, - "is_published": True, - }🧰 Tools
🪛 Ruff (0.11.9)
789-789: Local variable
i18n_object_dictis assigned to but never usedRemove assignment to unused variable
i18n_object_dict(F841)
794-794: Local variable
referential_object_dictis assigned to but never usedRemove assignment to unused variable
referential_object_dict(F841)
808-810: Fix method name in update_library.Update the method call to use the correct spelling.
if self.new_requirement_mapping_sets is not None: - self.udpate_requirement_mapping_sets() + self.update_requirement_mapping_sets()backend/library/utils.py (5)
132-134: Consider using proper exception chainingWhen re-raising exceptions within an exception handler, it's best practice to use
raise ... from errsyntax to maintain the exception context.- raise Http404(error_msg) + raise Http404(error_msg) from None🧰 Tools
🪛 Ruff (0.11.9)
134-134: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
140-142: Consider using proper exception chainingSimilarly here, use
raise ... from Nonefor proper exception chaining.- raise Http404(error_msg) + raise Http404(error_msg) from None🧰 Tools
🪛 Ruff (0.11.9)
142-142: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
585-604: Fix typo in error messageThere's a typo in the error message that could cause confusion.
- return "[FRAMEWORK_ERROR] {} invaldi framework{} detected, the {}{} framework has the following error : {}".format( + return "[FRAMEWORK_ERROR] {} invalid framework{} detected, the {}{} framework has the following error : {}".format(Also, there appears to be an inconsistency in the index formatting:
- invalid_framework_index, + invalid_framework_index + 1,
673-682: LGTM: Consistent handling of risk matricesThe implementation follows the same pattern as other object types, with proper mutual exclusivity checks and backward compatibility handling.
There's a small typo in the comment on line 682: "requiremnt_mapping_set" should be "requirement_mapping_set".
606-612: Clean up commented out code using wrong attributeThere's commented out code that references
self._library_datawhich doesn't exist in the class.Either remove the commented code or update it to use
self._library.contentas in line 613.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
backend/core/models.py(9 hunks)backend/library/utils.py(15 hunks)
🧰 Additional context used
🪛 Ruff (0.11.9)
backend/core/models.py
7-7: typing.Type imported but unused
Remove unused import: typing.Type
(F401)
763-763: Do not use bare except
(E722)
789-789: Local variable i18n_object_dict is assigned to but never used
Remove assignment to unused variable i18n_object_dict
(F841)
794-794: Local variable referential_object_dict is assigned to but never used
Remove assignment to unused variable referential_object_dict
(F841)
backend/library/utils.py
134-134: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
142-142: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
🔇 Additional comments (17)
backend/core/models.py (5)
363-371: Good code refactoring with common dictionaries.Moving common field dictionaries for i18n and referential objects to class initialization improves code reusability across update methods.
373-393: Approve pluralization support implementation.This change efficiently handles both singular and plural forms (e.g., framework/frameworks) and normalizes single objects into lists for consistent processing. The approach provides good backward compatibility while enabling the new multi-object support.
302-304: Approve objects_meta calculation update.The simplified objects count calculation now consistently handles singular/plural forms, correctly counting only "framework" as 1 while using actual length for other object collections.
799-810: Approve modular update method sequence.Good code organization with sequential invocation of specialized update methods for each object type, improving maintainability and readability.
701-707: 🛠️ Refactor suggestionPrevent early return from causing incomplete updates.
Early return in the loop could prevent processing of other valid requirement mapping sets if one has changed frameworks. Consider skipping the current item instead of terminating the entire method.
if ( requirement_mapping_set_obj.source_framework.urn != source_framework_urn or requirement_mapping_set_obj.target_framework.urn != target_framework_urn ): # We don't allow an update to modify the "source_framework" and "target_framework" of a RequirementMappingSet. - return "invalidLibraryUpdate" + # Skip this mapping set but continue processing others + continueLikely an incorrect or invalid review comment.
backend/library/utils.py (12)
119-121: Good update in error handling approachThe change from potentially raising exceptions to returning error strings is a good practice, as it allows for better error accumulation and reporting throughout the validation process.
155-155: LGTM: Updated required fields for RequirementMappingSetImporterAdding
source_framework_urnandtarget_framework_urnas required fields ensures proper validation for requirement mapping sets.
162-164: LGTM: Added is_empty methodAdding this helper method clarifies the validation logic and makes the code more maintainable.
169-177: LGTM: Improved error collection in mapping validationThe updated initialization logic properly collects errors from all mapping importers, allowing for better error reporting.
259-266: LGTM: Added consistent is_empty method to FrameworkImporterThis is a good addition for consistency with other importers and helps with validation logic.
457-468: LGTM: Clearly separated deprecated and non-deprecated fieldsGood practice to indicate which fields are deprecated and provide a clear reference to the preferred fields to use.
553-583: LGTM: Clean implementation of requirement mapping set initializationThis method follows the established error collection pattern, making the code consistent and maintainable.
622-625: LGTM: Proper mutual exclusivity check for framework fieldsThis validation ensures that the library doesn't contain both singular and plural forms of the same object type.
642-647: LGTM: Proper mutual exclusivity check for requirement mapping set fieldsThis validation is consistent with the framework validation pattern.
654-656: LGTM: Good backward compatibility handlingConverting a single dictionary to a list for backward compatibility is a clean approach.
697-716: LGTM: Improved dependency checkingThe updated error handling in dependency checking is more consistent with the overall error handling approach.
755-771: LGTM: Updated import_objects to handle multiple objectsThe method has been properly updated to iterate through multiple frameworks and requirement mapping sets.
backend/core/models.py
Outdated
| except: | ||
| return "dependencyNotFound" | ||
| new_dependencies.append(new_dependency) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use specific exception handling instead of bare except.
Bare except clauses can catch unexpected exceptions and hide bugs. Catch specific exceptions that might occur during this operation.
try:
new_dependency = LoadedLibrary.objects.filter(
urn=new_dependency_urn
).first() # The locale is not handled by this code
-except:
+except Exception as e:
+ logger.error(f"Error finding dependency with URN {new_dependency_urn}: {e}")
return "dependencyNotFound"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| except: | |
| return "dependencyNotFound" | |
| new_dependencies.append(new_dependency) | |
| try: | |
| new_dependency = LoadedLibrary.objects.filter( | |
| urn=new_dependency_urn | |
| ).first() # The locale is not handled by this code | |
| except Exception as e: | |
| logger.error(f"Error finding dependency with URN {new_dependency_urn}: {e}") | |
| return "dependencyNotFound" | |
| new_dependencies.append(new_dependency) |
🧰 Tools
🪛 Ruff (0.11.9)
763-763: Do not use bare except
(E722)
🤖 Prompt for AI Agents
In backend/core/models.py around lines 763 to 765, replace the bare except
clause with specific exception handling by identifying the exact exceptions that
can occur in the try block. Catch only those exceptions instead of using a
general except to avoid hiding unexpected errors and improve code clarity.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
backend/core/models.py (2)
724-728: Fix inconsistency with "stregth_of_relationship" typo.There's a comment about fixing a typo in the field name, but the code is inconsistent. The dictionary excludes "stregth_of_relationship" from being copied directly, but then uses this misspelled key to get the value. This might cause issues with future library updates if the format is fixed.
- requirement_mapping_dict["strength_of_relationship"] = ( - requirement_mapping["stregth_of_relationship"] - ) # # Fix the typo caused by the convert_library.py code. + # Handle both spellings for backward compatibility + requirement_mapping_dict["strength_of_relationship"] = ( + requirement_mapping.get("strength_of_relationship") or + requirement_mapping.get("stregth_of_relationship") + ) # Fix the typo caused by the convert_library.py code.
763-765: Use specific exception handling instead of bare except.Bare except clauses can catch unexpected exceptions and hide bugs. Catch specific exceptions that might occur during this operation.
try: new_dependency = LoadedLibrary.objects.filter( urn=new_dependency_urn ).first() # The locale is not handled by this code -except: +except Exception as e: + logger.error(f"Error finding dependency with URN {new_dependency_urn}: {e}") return "dependencyNotFound"🧰 Tools
🪛 Ruff (0.11.9)
763-763: Do not use bare
except(E722)
🧹 Nitpick comments (3)
backend/core/models.py (3)
7-7: Remove unused import Type.The
Typeimport from typing is not used in the code and should be removed.-from typing import Self, Type, Union, List +from typing import Self, Union, List🧰 Tools
🪛 Ruff (0.11.9)
7-7:
typing.Typeimported but unusedRemove unused import:
typing.Type(F401)
672-672: Fix typo in method name.The method name has a spelling error: "udpate" instead of "update".
- def udpate_requirement_mapping_sets(self): + def update_requirement_mapping_sets(self):Make sure to update any references to this method name as well (like in line 809).
809-809: Update method name reference to match the fixed typo.If you fix the typo in the method name from
udpate_requirement_mapping_setstoupdate_requirement_mapping_sets, you'll need to update this reference as well.- self.udpate_requirement_mapping_sets() + self.update_requirement_mapping_sets()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
backend/core/models.py(9 hunks)documentation/architecture/data-model.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- documentation/architecture/data-model.md
🧰 Additional context used
🪛 Ruff (0.11.9)
backend/core/models.py
7-7: typing.Type imported but unused
Remove unused import: typing.Type
(F401)
763-763: Do not use bare except
(E722)
789-789: Local variable i18n_object_dict is assigned to but never used
Remove assignment to unused variable i18n_object_dict
(F841)
794-794: Local variable referential_object_dict is assigned to but never used
Remove assignment to unused variable referential_object_dict
(F841)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: build (3.12)
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: startup-docker-compose-test
- GitHub Check: test (3.12)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
backend/core/models.py (2)
373-393: Good approach for handling both singular and plural field names.The implementation elegantly handles backward compatibility by supporting both singular and plural field names (framework/frameworks, risk_matrix/risk_matrices, requirement_mapping_set/requirement_mapping_sets) while normalizing both single dictionary and list formats to a common list representation.
426-451: Excellent modularization of update methods.The refactoring of update logic into separate methods for each object type improves code maintainability and readability. The consistent use of common dictionaries (
referential_object_dictandi18n_object_dict) across different methods also reduces code duplication.Also applies to: 452-645, 646-671, 672-742, 743-811
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
backend/library/utils.py (3)
133-134: Missing best practice for exception chainingWhen raising exceptions within an except block, you should use
from errorfrom Noneto preserve the exception context.- error_msg = f"ERROR: target requirement with URN {self.data['target_requirement_urn']} does not exist" - print(error_msg) - raise Http404(error_msg) + error_msg = f"ERROR: target requirement with URN {self.data['target_requirement_urn']} does not exist" + print(error_msg) + raise Http404(error_msg) from None🧰 Tools
🪛 Ruff (0.11.9)
134-134: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
141-142: Missing best practice for exception chainingWhen raising exceptions within an except block, you should use
from errorfrom Noneto preserve the exception context.- error_msg = f"ERROR: source requirement with URN {self.data['source_requirement_urn']} does not exist" - print(error_msg) - raise Http404(error_msg) + error_msg = f"ERROR: source requirement with URN {self.data['source_requirement_urn']} does not exist" + print(error_msg) + raise Http404(error_msg) from None🧰 Tools
🪛 Ruff (0.11.9)
142-142: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
697-716: Update return type docstring for consistencyThe method signature has been updated to return
Union[str, None], but the docstring doesn't reflect this change.- def check_and_import_dependencies(self) -> Union[str, None]: - """Check and import library dependencies.""" + def check_and_import_dependencies(self) -> Union[str, None]: + """Check and import library dependencies. + + Returns: + Union[str, None]: Error message if dependencies cannot be loaded or updated, None otherwise. + """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
backend/library/utils.py(15 hunks)backend/library/views.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/library/views.py
🧰 Additional context used
🪛 Ruff (0.11.9)
backend/library/utils.py
134-134: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
142-142: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: startup-docker-compose-test
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: test (3.12)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: build (3.12)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (9)
backend/library/utils.py (9)
119-121: Improved error handling approach!The change from returning a boolean to returning a string or None is a good practice for providing more descriptive error handling across the codebase.
162-164: Good addition for validation!This method helps validate empty requirement mapping sets, which is important for preventing empty objects from being processed.
165-192: Effective error collection pattern!This implementation properly collects all errors and provides meaningful feedback to the user. The ordinal suffix handling is also correctly implemented.
215-222: LGTM! Comprehensive validationThis initialization method properly checks for missing fields and empty requirement mappings before proceeding.
553-583: Good implementation for multiple requirement mapping sets!This new method properly handles multiple requirement mapping sets and provides detailed error reporting, which aligns with the PR's objective.
622-647: Good mutual exclusivity checks for frameworks and mapping setsThe implementation correctly prevents simultaneous use of both singular and plural field names, ensuring data consistency.
648-666: Good backward compatibility for singular mapping setsThe implementation properly handles both singular and plural field names, and converts single objects to lists for consistent processing.
673-683: Good backward compatibility for risk matricesSimilar to the frameworks and mapping sets, this implementation properly handles both singular and plural field names for risk matrices.
755-772: Good update to support multiple frameworks and mapping setsThe implementation now properly imports multiple frameworks and requirement mapping sets, which is the primary goal of this PR.
|
@eric-intuitem shoud be fine now for mapping updates |
There was a problem hiding this comment.
The "create_library.py" script seems to work fine on my end too. I tried it on another mapping.
I couldn't test the generated mapping by importing it into CISO Assistant as I need to apply the changes made to the backend code and couldn't figure out how to do it.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
tools/create_library.py (1)
793-793:⚠️ Potential issueBug: Overwriting of multiple requirement mapping sets
The direct assignment to
library["objects"]["requirement_mapping_sets"]will overwrite any previously processed mapping sets if this function handles multiple requirement mapping sets.Apply this fix to append to the list instead of overwriting it:
-library["objects"]["requirement_mapping_sets"] = [requirement_mapping_set, requirement_mapping_set_revert] +if "requirement_mapping_sets" not in library["objects"]: + library["objects"]["requirement_mapping_sets"] = [] +library["objects"]["requirement_mapping_sets"].extend([requirement_mapping_set, requirement_mapping_set_revert])backend/core/models.py (2)
770-772:⚠️ Potential issueIncomplete fix for field name typo
The code only retrieves the correctly spelled field without handling backward compatibility for the misspelled version. This might cause issues with existing data that uses the typo.
Apply the previously suggested fix for backward compatibility:
- requirement_mapping_dict["strength_of_relationship"] = ( - requirement_mapping.get("strength_of_relationship") - ) # # Fix the typo caused by the convert_library.py code. + # Handle both spellings for backward compatibility + requirement_mapping_dict["strength_of_relationship"] = ( + requirement_mapping.get("strength_of_relationship") or + requirement_mapping.get("stregth_of_relationship") + ) # Fix the typo caused by the convert_library.py code.
808-809:⚠️ Potential issueUse specific exception handling
Bare except clauses can catch unexpected exceptions and hide bugs.
- except: + except LoadedLibrary.DoesNotExist: + logger.error(f"Dependency with URN {new_dependency_urn} not found") return "dependencyNotFound"🧰 Tools
🪛 Ruff (0.11.9)
808-808: Do not use bare
except(E722)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
backend/core/models.py(8 hunks)backend/library/libraries/mapping-adobe-ccf-v5-to-iso27001-2022.yaml(2 hunks)backend/library/libraries/mapping-iso27001-2013-to-iso27001-2022.yaml(2 hunks)tools/README.md(1 hunks)tools/convert_library.py(3 hunks)tools/create_library.py(3 hunks)
✅ Files skipped from review due to trivial changes (2)
- tools/README.md
- backend/library/libraries/mapping-adobe-ccf-v5-to-iso27001-2022.yaml
🧰 Additional context used
🪛 Ruff (0.11.9)
backend/core/models.py
7-7: typing.Type imported but unused
Remove unused import: typing.Type
(F401)
808-808: Do not use bare except
(E722)
834-834: Local variable i18n_object_dict is assigned to but never used
Remove assignment to unused variable i18n_object_dict
(F841)
839-839: Local variable referential_object_dict is assigned to but never used
Remove assignment to unused variable referential_object_dict
(F841)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- 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: test (3.12)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
🔇 Additional comments (5)
tools/convert_library.py (1)
99-99: LGTM! Typo fix for consistent key naming.The correction from
stregth_of_relationshiptostrength_of_relationshipensures consistency across the codebase and aligns with the fixes in the YAML mapping files.Also applies to: 902-905, 913-913
backend/library/libraries/mapping-iso27001-2013-to-iso27001-2022.yaml (1)
8-8: LGTM! Consistent typo fix and version update.The YAML file has been properly updated:
- Version incremented to 2 to reflect the schema change
- All instances of the misspelled
stregth_of_relationshiphave been corrected tostrength_of_relationshipThis ensures data consistency across all requirement mappings.
Also applies to: 29-29, 34-34, 39-39, 44-44, 49-49, 54-54, 59-59, 64-64, 69-69, 74-74, 79-79, 84-84, 89-89, 94-94, 99-99, 104-104, 109-109, 114-114, 119-119, 124-124, 129-129, 134-134, 139-139, 144-144, 149-149, 154-154, 159-159, 164-164, 169-169, 174-174, 179-179, 184-184, 189-189, 194-194, 199-199, 204-204, 209-209, 214-214, 219-219, 224-224, 229-229, 234-234, 239-239, 244-244, 249-249, 254-254, 259-259, 264-264, 269-269, 274-274, 279-279, 284-284, 289-289, 294-294, 299-299, 304-304, 309-309, 314-314, 319-319, 324-324, 329-329, 334-334, 339-339, 344-344, 349-349, 354-354, 359-359, 364-364, 369-369, 374-374, 379-379, 384-384, 389-389, 394-394, 399-399, 404-404, 409-409, 414-414, 419-419, 424-424, 429-429, 434-434, 439-439, 444-444, 449-449, 454-454, 459-459, 464-464, 469-469, 474-474, 479-479, 484-484, 489-489, 494-494, 499-499, 504-504, 509-509, 514-514, 519-519, 524-524, 529-529, 534-534, 539-539, 544-544, 549-549, 554-554, 559-559, 564-564, 569-569, 574-574, 579-579, 584-584, 589-589, 594-594, 599-599, 604-604, 609-609, 614-614, 619-619, 624-624, 629-629, 634-634, 639-639, 644-644, 649-649, 654-654, 659-659, 664-664, 669-669, 674-674, 679-679, 684-684, 689-689, 694-694
tools/create_library.py (1)
331-337: LGTM! Well-implemented relationship reversal logic.The
revert_relationshipfunction correctly handles the bidirectional nature of subset/superset relationships while preserving other relationship types unchanged.backend/core/models.py (2)
374-393: Good implementation of backward compatibilityThe approach of accepting both singular and plural forms (framework/frameworks, risk_matrix/risk_matrices, requirement_mapping_set/requirement_mapping_sets) and normalizing them to lists is well-implemented. This ensures smooth migration as mentioned in the PR objectives.
303-303:⚠️ Potential issueInconsistent with PR objectives - framework count forced to 1
This change contradicts the PR objective of supporting multiple frameworks. The code forces the framework count to always be 1, even when multiple frameworks are provided as a list. This defeats the purpose of supporting multiple objects.
- key: (1 if key == "framework" else len(value)) + key: len(value)Likely an incorrect or invalid review comment.
backend/core/models.py
Outdated
| i18n_object_dict = { | ||
| "locale": self.old_library.locale, | ||
| "default_locale": self.old_library.default_locale, | ||
| } | ||
|
|
||
| referential_object_dict = { | ||
| "provider": self.new_library.provider, | ||
| "is_published": True, | ||
| } |
There was a problem hiding this comment.
Remove redundant variable declarations
These local variables duplicate the instance variables created in __init__ and are never used. The update methods correctly use self.i18n_object_dict and self.referential_object_dict.
- i18n_object_dict = {
- "locale": self.old_library.locale,
- "default_locale": self.old_library.default_locale,
- }
-
- referential_object_dict = {
- "provider": self.new_library.provider,
- "is_published": True,
- }
-
self.update_threats()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| i18n_object_dict = { | |
| "locale": self.old_library.locale, | |
| "default_locale": self.old_library.default_locale, | |
| } | |
| referential_object_dict = { | |
| "provider": self.new_library.provider, | |
| "is_published": True, | |
| } | |
| self.update_threats() |
🧰 Tools
🪛 Ruff (0.11.9)
834-834: Local variable i18n_object_dict is assigned to but never used
Remove assignment to unused variable i18n_object_dict
(F841)
839-839: Local variable referential_object_dict is assigned to but never used
Remove assignment to unused variable referential_object_dict
(F841)
🤖 Prompt for AI Agents
In backend/core/models.py between lines 834 and 842, the local variables
i18n_object_dict and referential_object_dict are declared but never used, as the
class already has instance variables with the same names. Remove these redundant
variable declarations to clean up the code and avoid confusion, since the update
methods correctly reference the instance variables.
backend/core/models.py
Outdated
| @@ -4,7 +4,7 @@ | |||
| import hashlib | |||
| from datetime import date, datetime | |||
| from pathlib import Path | |||
| from typing import Self, Type, Union | |||
| from typing import Self, Type, Union, List | |||
There was a problem hiding this comment.
Remove unused import
The Type import from typing is not used anywhere in the code.
-from typing import Self, Type, Union, List
+from typing import Self, Union, List📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| from typing import Self, Type, Union, List | |
| from typing import Self, Union, List |
🧰 Tools
🪛 Ruff (0.11.9)
7-7: typing.Type imported but unused
Remove unused import: typing.Type
(F401)
🤖 Prompt for AI Agents
In backend/core/models.py on line 7, the import statement includes `Type` from
`typing`, but it is not used anywhere in the file. Remove `Type` from the import
statement to clean up the code and avoid unnecessary imports.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
backend/library/utils.py (2)
670-670: Remove debug print statementReplace with proper logging:
- print("threat errors", threat_import_error) + logger.error("Threat import error", error=threat_import_error)
681-681: Fix typo in comment- # Handle risk matrix as dict for consistency (it would be bad for "risk_matrix" to not accept a dict but allowing it for "framework" and "requiremnt_mapping_set") + # Handle risk matrix as dict for consistency (it would be bad for "risk_matrix" to not accept a dict but allowing it for "framework" and "requirement_mapping_set")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/library/utils.py(15 hunks)
🧰 Additional context used
🪛 Ruff (0.11.9)
backend/library/utils.py
134-134: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
142-142: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: Analyze (python)
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: build (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: test (3.12)
🔇 Additional comments (5)
backend/library/utils.py (5)
162-193: Well-implemented error accumulation pattern!The refactored
is_emptymethod andinit_requirement_mappingserror handling provide clear, actionable error messages while maintaining backward compatibility. The ordinal suffix calculation is correctly implemented.
255-255: Correct ordinal suffix calculationGood fix! The
+1ensures the ordinal suffix matches the 1-based index shown in the error message.
622-683: Excellent backward compatibility implementation!The mutual exclusion checks and automatic conversion of single dictionaries to lists ensure smooth migration from deprecated singular fields (
framework,requirement_mapping_set,risk_matrix) to their plural counterparts. This implementation maintains API compatibility while encouraging the use of the new plural fields.
697-716: Consistent error handling in dependency managementThe refactored method properly returns error messages instead of raising exceptions, maintaining consistency with the new error handling pattern throughout the codebase.
755-771: Proper implementation of multiple object supportThe
import_objectsmethod correctly iterates over lists of frameworks and requirement mapping sets, aligning with the PR's objective to support multiple objects of different types within libraries.
backend/library/utils.py
Outdated
| error_msg = f"ERROR: target requirement with URN {self.data['target_requirement_urn']} does not exist" | ||
| print(error_msg) | ||
| raise Http404(error_msg) |
There was a problem hiding this comment.
Use explicit exception chaining with raise ... from
When re-raising exceptions within an except clause, use explicit exception chaining to preserve the original traceback and distinguish between errors in exception handling.
Apply this diff to fix the exception chaining:
- raise Http404(error_msg)
+ raise Http404(error_msg) from NoneThis applies to both occurrences (lines 134 and 142).
Also applies to: 140-142
🧰 Tools
🪛 Ruff (0.11.9)
134-134: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🤖 Prompt for AI Agents
In backend/library/utils.py around lines 132 to 134, the code raises an Http404
exception without chaining the original exception, which can obscure the
traceback. Modify the raise statement to include 'from e' (or the original
exception variable) to explicitly chain the exception, preserving the traceback
and improving debugging clarity. Apply this change to both relevant occurrences
at lines 134 and 142, including the block from lines 140 to 142.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
backend/core/models.py (3)
7-7: Remove unused import as previously flagged.The
Typeimport fromtypingis still present but remains unused in the codebase.-from typing import Self, Union, List +from typing import Self, Union, List
770-772: Fix inconsistency with strength_of_relationship field handling.The current implementation only handles the corrected spelling "strength_of_relationship" but doesn't provide backward compatibility for the typo mentioned in the comment. This could cause issues when processing older library data.
- requirement_mapping_dict["strength_of_relationship"] = ( - requirement_mapping.get("strength_of_relationship") - ) # # Fix the typo caused by the convert_library.py code. + # Handle both spellings for backward compatibility + requirement_mapping_dict["strength_of_relationship"] = ( + requirement_mapping.get("strength_of_relationship") or + requirement_mapping.get("stregth_of_relationship") + ) # Fix the typo caused by the convert_library.py code.
804-810: Use specific exception handling instead of bare except.The bare
exceptclause can catch unexpected exceptions and hide bugs. Although this is flagged in past comments, it still needs to be addressed.try: new_dependency = LoadedLibrary.objects.filter( urn=new_dependency_urn ).first() # The locale is not handled by this code -except ValueError: +except (ValueError, LoadedLibrary.DoesNotExist) as e: + logger.error(f"Error finding dependency with URN {new_dependency_urn}: {e}") return "dependencyNotFound"
🧹 Nitpick comments (2)
backend/library/utils.py (2)
680-681: Fix typo in comment- # Handle risk matrix as dict for consistency (it would be bad for "risk_matrix" to not accept a dict but allowing it for "framework" and "requiremnt_mapping_set") + # Handle risk matrix as dict for consistency (it would be bad for "risk_matrix" to not accept a dict but allowing it for "framework" and "requirement_mapping_set")
707-707: Improve error message consistencyFor consistency with other error messages in the file:
- return f"ERROR: Stored Library with URN {dependency_urn} does not exist" + return "ERROR: Stored Library with URN {} does not exist".format(dependency_urn)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/core/models.py(8 hunks)backend/library/utils.py(15 hunks)
⏰ 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: enterprise-startup-functional-test (3.12)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: Analyze (python)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: test (3.12)
- GitHub Check: build (3.12)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (12)
backend/core/models.py (8)
303-303: Good implementation for framework object handling.The special case handling for "framework" objects (setting count to 1) aligns with the PR objectives to support single objects as dictionaries in addition to lists.
360-394: Excellent refactoring to support multiple object formats.The LibraryUpdater constructor now properly handles both single objects (as dict) and multiple objects (as list) for frameworks, risk matrices, and requirement mapping sets. The fallback logic using
oroperators correctly prioritizes the plural forms over singular forms, which aligns perfectly with the PR objectives.The normalization logic that converts single dictionaries to lists is well-implemented and will facilitate easier migration of existing data.
414-416: Clean use of walrus operator for error handling.The walrus operator usage here makes the code more concise while maintaining readability for the dependency loading logic.
424-426: Appropriate walrus operator usage in dependency updates.The conditional assignment pattern is used correctly here for handling dependency update results.
455-470: Well-structured framework update logic.The framework update method correctly handles the new list-based approach for frameworks while maintaining proper error handling and database operations. The use of
self.referential_object_dictandself.i18n_object_dictis consistent with the constructor changes.
686-711: Solid implementation of risk matrices update method.The method correctly processes multiple risk matrices, properly separating JSON definition fields from other metadata. The bulk update approach is efficient for handling multiple matrices.
712-787: Comprehensive requirement mapping sets update implementation.The method provides good validation by checking framework URNs and preventing unauthorized changes to source/target frameworks. The bulk deletion and recreation approach for requirement mappings ensures data consistency.
789-845: Robust library update orchestration.The main update method properly coordinates all the individual update operations while respecting the conditional logic for optional components (frameworks, matrices, requirement mapping sets). The dependency validation logic ensures data integrity.
backend/library/utils.py (4)
119-121: Good improvement in error handling approachThe change from boolean return values to detailed error strings provides better debugging information and aligns with the consistent error handling pattern throughout the file.
Also applies to: 217-224
257-257: Clean implementation of ordinal suffixesGood use of dictionary lookup for ordinal suffixes (
1st,2nd,3rd,nth). This is more maintainable than if/else chains.Also applies to: 498-498, 528-530, 551-551, 604-604
587-606: Well-structured implementation for multiple objects supportThe implementation properly handles:
- Multiple frameworks and requirement mapping sets
- Detailed error reporting with proper indexing
- Consistent error message formatting
- Clean separation of initialization and import logic
Also applies to: 555-585, 754-770
164-166: Good addition of empty validation methodsThe
is_empty()methods provide a clean way to check if objects contain any data, improving validation logic.Also applies to: 261-268
| logger.error( | ||
| "Target requirement does not exist", | ||
| error=self.data["target_requirement_urn"], | ||
| ) | ||
| try: | ||
| source_requirement = RequirementNode.objects.get( | ||
| urn=self.data["source_requirement_urn"].lower(), default_locale=True | ||
| ) | ||
| except RequirementNode.DoesNotExist: | ||
| err_msg = f"ERROR: source requirement with URN {self.data['source_requirement_urn']} does not exist" | ||
| print(err_msg) | ||
| raise Http404(err_msg) | ||
| logger.error( | ||
| "Source requirement does not exist", | ||
| error=self.data["source_requirement_urn"], | ||
| ) |
There was a problem hiding this comment.
Critical: Undefined variables will cause NameError
The code logs errors when requirements don't exist but continues execution, leading to NameError when trying to create RequirementMapping with undefined target_requirement or source_requirement variables.
Either return early when requirements don't exist or assign None and handle it:
def load(
self,
mapping_set: RequirementMappingSet,
):
+ target_requirement = None
+ source_requirement = None
try:
target_requirement = RequirementNode.objects.get(
urn=self.data["target_requirement_urn"].lower(), default_locale=True
)
except RequirementNode.DoesNotExist:
logger.error(
"Target requirement does not exist",
error=self.data["target_requirement_urn"],
)
+ return None
try:
source_requirement = RequirementNode.objects.get(
urn=self.data["source_requirement_urn"].lower(), default_locale=True
)
except RequirementNode.DoesNotExist:
logger.error(
"Source requirement does not exist",
error=self.data["source_requirement_urn"],
)
+ return None
return RequirementMapping.objects.create(
mapping_set=mapping_set,
target_requirement=target_requirement,
source_requirement=source_requirement,
relationship=self.data["relationship"],
annotation=self.data.get("annotation"),
strength_of_relationship=self.data.get("strength_of_relationship"),
rationale=self.data.get("rationale"),
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| logger.error( | |
| "Target requirement does not exist", | |
| error=self.data["target_requirement_urn"], | |
| ) | |
| try: | |
| source_requirement = RequirementNode.objects.get( | |
| urn=self.data["source_requirement_urn"].lower(), default_locale=True | |
| ) | |
| except RequirementNode.DoesNotExist: | |
| err_msg = f"ERROR: source requirement with URN {self.data['source_requirement_urn']} does not exist" | |
| print(err_msg) | |
| raise Http404(err_msg) | |
| logger.error( | |
| "Source requirement does not exist", | |
| error=self.data["source_requirement_urn"], | |
| ) | |
| def load( | |
| self, | |
| mapping_set: RequirementMappingSet, | |
| ): | |
| # Initialize to avoid NameError if lookup fails | |
| target_requirement = None | |
| source_requirement = None | |
| try: | |
| target_requirement = RequirementNode.objects.get( | |
| urn=self.data["target_requirement_urn"].lower(), | |
| default_locale=True | |
| ) | |
| except RequirementNode.DoesNotExist: | |
| logger.error( | |
| "Target requirement does not exist", | |
| error=self.data["target_requirement_urn"], | |
| ) | |
| return None | |
| try: | |
| source_requirement = RequirementNode.objects.get( | |
| urn=self.data["source_requirement_urn"].lower(), | |
| default_locale=True | |
| ) | |
| except RequirementNode.DoesNotExist: | |
| logger.error( | |
| "Source requirement does not exist", | |
| error=self.data["source_requirement_urn"], | |
| ) | |
| return None | |
| return RequirementMapping.objects.create( | |
| mapping_set=mapping_set, | |
| target_requirement=target_requirement, | |
| source_requirement=source_requirement, | |
| relationship=self.data["relationship"], | |
| annotation=self.data.get("annotation"), | |
| strength_of_relationship=self.data.get("strength_of_relationship"), | |
| rationale=self.data.get("rationale"), | |
| ) |
🤖 Prompt for AI Agents
In backend/library/utils.py between lines 132 and 144, the code logs errors when
target or source requirements do not exist but does not stop execution or assign
these variables, which will cause a NameError later when attempting to use
`target_requirement` or `source_requirement`. Fix this by returning early after
logging the error or assigning None to these variables if they don't exist,
ensuring the code does not proceed with undefined variables.
All previously created mappings are now in v2 (including Excel & YAML files). I used the updated "create_library.py" script from PR #1943 to do so.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
tools/convert_library_v2.py (1)
746-793: 💡 Verification agent🧩 Analysis chain
Verify URN uniqueness and consistent plural key usage.
The implementation correctly creates bidirectional mappings. However, there are a few concerns:
- The "-revert" suffix could potentially conflict with existing URNs
- The plural key "requirement_mapping_sets" should be verified for consistency
Consider adding validation to ensure the generated revert URNs don't conflict:
+ # Validate that the revert URN doesn't already exist + revert_urn = meta.get("urn") + "-revert" + if revert_urn in [obj.get("urn") for obj in library.get("objects", {}).get("requirement_mapping_sets", [])]: + raise ValueError(f"Revert URN '{revert_urn}' would create a conflict") + requirement_mapping_set_revert = { - "urn": meta.get("urn") + "-revert", + "urn": revert_urn, "ref_id": meta.get("ref_id") + "-revert",
🏁 Script executed:
#!/bin/bash # Description: Verify the usage of plural forms and check for potential URN conflicts # Check if other object types use plural forms in library storage echo "Checking plural key usage in library objects..." rg "library\[\"objects\"\]\[" --type py -A 1 # Search for existing URN patterns that might conflict with "-revert" suffix echo -e "\nChecking for existing URNs with '-revert' pattern..." fd -e yaml -e yml --exec grep -h "urn:.*-revert" {} \; || echo "No existing '-revert' URNs found"Length of output: 3366
Validate uniqueness of “-revert” URNs to prevent collisions
After reviewing the plural key usage and existing configurations:
- The key
requirement_mapping_setsis correctly pluralized for a list of mapping‐set objects (consistent with other list keys likethreatsandreference_controls).- A search of your YAML definitions uncovered an existing URN ending in
-revert(urn:intuitem:risk:req_mapping_set:ccb-cff-2023-03-01-revert), which indicates a real collision risk.Please add a guard to ensure that newly generated revert URNs cannot collide with existing ones:
# generate the revert URN - requirement_mapping_set_revert = { - "urn": meta.get("urn") + "-revert", + revert_urn = meta.get("urn") + "-revert" + existing_urns = [ + obj.get("urn") + for obj in library + .get("objects", {}) + .get("requirement_mapping_sets", []) + ] + if revert_urn in existing_urns: + raise ValueError(f"Revert URN '{revert_urn}' already exists and would collide") + requirement_mapping_set_revert = { + "urn": revert_urn, "ref_id": meta.get("ref_id") + "-revert", "name": meta.get("name"), "description": meta.get("description"), "target_framework_urn": meta.get("source_framework_urn"), "source_framework_urn": meta.get("target_framework_urn"), }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tools/convert_library_v1.py(3 hunks)tools/convert_library_v2.py(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- tools/convert_library_v1.py
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: startup-docker-compose-test
- GitHub Check: build (3.12)
- 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: test (3.12)
- GitHub Check: startup-functional-test (3.12)
🔇 Additional comments (1)
tools/convert_library_v2.py (1)
2-2: LGTM!The documentation updates correctly reflect the script rename from
create_library.pytoconvert_library_v2.py.Also applies to: 8-8
| def revert_relationship(relation: str): | ||
| if relation == "subset": | ||
| return "superset" | ||
| elif relation == "superset": | ||
| return "subset" | ||
| else: | ||
| return relation |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Add documentation and verify completeness of relationship types.
The function correctly inverts subset/superset relationships. However, consider adding a docstring and verifying all possible relationship types are handled appropriately.
Add documentation to clarify the function's purpose:
def revert_relationship(relation: str):
+ """
+ Inverts the direction of a relationship for bidirectional mapping.
+
+ Args:
+ relation: The relationship type (e.g., "subset", "superset")
+
+ Returns:
+ The inverted relationship, or the original if no inversion is defined
+ """
if relation == "subset":
return "superset"
elif relation == "superset":
return "subset"
else:
return relation🏁 Script executed:
#!/bin/bash
# Description: Find all relationship types used in requirement mappings to ensure complete handling
# Search for relationship values in YAML files
echo "Searching for relationship types in YAML files..."
fd -e yaml -e yml --exec grep -h "relationship:" {} \; | sort -u | sed 's/.*relationship: *//'
# Search for relationship mentions in Python code
echo -e "\nSearching for relationship types in Python code..."
rg -A 3 -B 3 "relationship" --type pyLength of output: 34102
Add docstring to revert_relationship and document symmetric types
The only relationship values found in your mapping YAML files are:
- equal
- intersect
- subset
- superset
- null
Only subset and superset require inversion; all others are symmetric and should be returned unchanged. Please add a docstring to clarify this behaviour:
def revert_relationship(relation: str):
+ """
+ Invert directed relationships for bidirectional mappings.
+
+ Only 'subset' ↔ 'superset' are flipped; symmetric relationships
+ ('equal', 'intersect', 'null') are returned unchanged.
+
+ Args:
+ relation: one of 'subset', 'superset', 'equal', 'intersect', 'null'
+ Returns:
+ The inverted relationship or the original for symmetric types.
+ """
if relation == "subset":
return "superset"
elif relation == "superset":
return "subset"
else:
return relation📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def revert_relationship(relation: str): | |
| if relation == "subset": | |
| return "superset" | |
| elif relation == "superset": | |
| return "subset" | |
| else: | |
| return relation | |
| def revert_relationship(relation: str): | |
| """ | |
| Invert directed relationships for bidirectional mappings. | |
| Only 'subset' ↔ 'superset' are flipped; symmetric relationships | |
| ('equal', 'intersect', 'null') are returned unchanged. | |
| Args: | |
| relation: one of 'subset', 'superset', 'equal', 'intersect', 'null' | |
| Returns: | |
| The inverted relationship or the original for symmetric types. | |
| """ | |
| if relation == "subset": | |
| return "superset" | |
| elif relation == "superset": | |
| return "subset" | |
| else: | |
| return relation |
🤖 Prompt for AI Agents
In tools/convert_library_v2.py around lines 331 to 337, add a docstring to the
revert_relationship function explaining that it inverts 'subset' and 'superset'
relationships while returning all other relationship types unchanged because
they are symmetric. Also verify that all relationship types found in the YAML
mappings ('equal', 'intersect', 'subset', 'superset', 'null') are handled
correctly by returning them unchanged except for 'subset' and 'superset' which
are inverted.
eric-intuitem
changed the title
We want to be able to put any number of any type of objects in a library. Currently this is not the case for mapping_set and framework. It is the case for matrix, but the word is incorrect (should be matrices).
The change done is the following:
This approach make migration easy.
Summary by CodeRabbit
Bug Fixes
New Features
Documentation
Refactor
Chores
Frontend
Tests