fix: use schema versioning for import/export features #1475
Conversation
WalkthroughThis pull request enhances schema and version handling across the project. Key changes include improved version validation during file uploads and backup restorations, the introduction of utilities for parsing and comparing version strings, and the addition of a custom exception (VersionFormatError) for managing version formatting errors. The update removes regex-based checks in favor of streamlined logic using the Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant View as API Handler
participant Util as Version Utils
participant Logger as Logger
Client->>View: Upload file with (optional) schema_version
alt schema_version present
View->>Logger: Log uploaded schema_version
View->>Util: compare_schema_versions(uploaded, current)
alt Incompatible version
Util-->>View: Raise ValidationError
View->>Client: Return error response
else Compatible version
Util-->>View: Return success
View->>Client: Process file normally
end
else Legacy file
View->>Util: Use legacy version comparison logic
View->>Client: Process file normally
end
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
nas-tabchiche
requested review from
ab-smith,
eric-intuitem and
Mohamed-Hacene
![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
🧹 Nitpick comments (2)
backend/core/utils.py (2)
6-6: Remove unused import.The
remodule is imported but never used in the code.-import re🧰 Tools
🪛 Ruff (0.8.2)
6-6:
reimported but unusedRemove unused import:
re(F401)
91-103: Improve exception handling by usingraise ... from err.The function is well-implemented, but the exception handling could be improved to preserve the exception chain.
try: return [int(part) for part in parts] except ValueError as err: - raise VersionFormatError(f"Non-numeric version component in {version}") + raise VersionFormatError(f"Non-numeric version component in {version}") from err🧰 Tools
🪛 Ruff (0.8.2)
103-103: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/core/tests/test_helpers.py(2 hunks)backend/core/utils.py(2 hunks)backend/core/views.py(2 hunks)backend/serdes/views.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/serdes/views.py
🧰 Additional context used
🪛 Ruff (0.8.2)
backend/core/utils.py
6-6: re imported but unused
Remove unused import: re
(F401)
103-103: 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: functional-tests (3.12, chromium)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: build (3.12)
- GitHub Check: Analyze (python)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (4)
backend/core/utils.py (1)
106-157: LGTM! Well-documented and robust implementation.The compare_versions function is well-implemented with:
- Clear docstring and examples
- Proper error handling
- Flexible comparison levels (major, minor, patch)
- Correct padding of version components
backend/core/tests/test_helpers.py (2)
159-180: LGTM! Comprehensive test coverage for parse_version.The tests cover all important scenarios:
- Single, two, and three component versions
- Invalid version format (missing 'v')
- Non-numeric components
187-240: LGTM! Excellent test coverage for compare_versions.The tests are well-structured with:
- Parameterized tests for multiple scenarios
- Coverage of all comparison levels (major, minor, patch)
- Edge cases with version padding
- Invalid inputs
backend/core/views.py (1)
2270-2279: LGTM! Version comparison logic improved.The changes correctly implement minor-level version comparison:
- Uses the new compare_versions function
- Clear error handling for incompatible versions
- Aligns with PR objective to check only major and minor versions
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
backend/core/utils.py (1)
106-157: Add return type hint to compare_versions function.The function has comprehensive documentation and examples but could benefit from an explicit return type hint.
def compare_versions( version_a: str, version_b: str, level: Literal["major", "minor", "patch"] = "patch" -) -> int: +) -> Literal[-1, 0, 1]:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
backend/ciso_assistant/meta.json(1 hunks)backend/ciso_assistant/settings.py(2 hunks)backend/core/utils.py(2 hunks)backend/core/views.py(4 hunks)backend/serdes/serializers.py(2 hunks)backend/serdes/views.py(2 hunks)enterprise/backend/enterprise_core/settings.py(2 hunks)frontend/src/routes/(app)/(internal)/backup-restore/+page.server.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- backend/ciso_assistant/meta.json
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/serdes/views.py
- backend/core/views.py
⏰ 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: 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)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (8)
frontend/src/routes/(app)/(internal)/backup-restore/+page.server.ts (1)
36-36: LGTM! Enhanced error logging.The addition of error logging improves debugging capabilities for backup file upload issues.
backend/serdes/serializers.py (2)
40-40: LGTM! Added schema version support.The schema_version field is properly integrated into the MetaSerializer.
121-121: LGTM! Schema version included in backup metadata.The schema_version is correctly retrieved from settings and included in the backup metadata.
backend/core/utils.py (2)
85-88: LGTM! Well-defined custom exception.The VersionFormatError exception is properly defined with a clear docstring.
91-103: LGTM! Robust version parsing.The parse_version function includes proper error handling and clear docstring.
backend/ciso_assistant/settings.py (1)
110-110: LGTM! Added schema version logging.The schema version is properly logged along with other configuration values.
enterprise/backend/enterprise_core/settings.py (2)
113-113: LGTM!The logging statement is consistent with the existing logging pattern for version information.
14-14: Verify the path resolution logic.The path resolution for
COMMUNITY_BACKEND_DIRuses multiple.parentcalls which can be fragile. Consider using a more robust path resolution method.Run this script to verify the path resolution:
Also applies to: 22-22
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Verify that the path resolution leads to the expected directory containing meta.json # Test: Check if meta.json exists at the resolved path python3 -c ' from pathlib import Path import os path = Path(__file__).resolve().parent.parent.parent.parent / "backend" / "ciso_assistant" / "meta.json" print(f"Resolved path: {path}") print(f"Path exists: {path.exists()}") 'Length of output: 370
Verify the path resolution using a simulated file context.
The inline Python command previously failed because file isn’t defined when using Python’s “-c” flag. To mimic the behavior as if we were running the code from enterprise/backend/enterprise_core/settings.py, please run the following shell script. This script manually sets a fake file value to simulate the file’s location and then computes the resolved path for meta.json. This should help verify that the path resolution logic leads to the expected directory.
Please run this script and verify the output. If the resolved path is correct and points to the location where meta.json is expected, then the path resolution logic is working as intended. If not, further adjustments in the path construction may be required.
🏁 Script executed:
#!/bin/bash # Adjusted script: simulate __file__ based on the known location of settings.py python3 -c ' from pathlib import Path # Simulate the location of "enterprise/backend/enterprise_core/settings.py" fake_file = "enterprise/backend/enterprise_core/settings.py" # Emulate the same parent traversal as in the production code resolved_path = (Path(fake_file).resolve()).parent.parent.parent.parent / "backend" / "ciso_assistant" / "meta.json" print(f"Simulated __file__ based path: {resolved_path}") print("Path exists:", resolved_path.exists()) 'Length of output: 561
Path resolution verified.
The simulation confirms that the resolved path:
/home/jailuser/git/backend/ciso_assistant/meta.json
correctly points to an existing file. This indicates that the path traversal logic in the file is working as expected.
backend/ciso_assistant/settings.py
Outdated
| META = {} | ||
| with open(BASE_DIR / "ciso_assistant" / "meta.json") as f: | ||
| META = json.load(f) | ||
| SCHEMA_VERSION = META.get("schema_version", "unset") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for meta.json loading.
The code should handle cases where meta.json is missing or invalid.
META = {}
-with open(BASE_DIR / "ciso_assistant" / "meta.json") as f:
- META = json.load(f)
+meta_file = BASE_DIR / "ciso_assistant" / "meta.json"
+try:
+ with open(meta_file) as f:
+ META = json.load(f)
+except (FileNotFoundError, json.JSONDecodeError) as e:
+ logger.warning("Failed to load meta.json: %s", e)
SCHEMA_VERSION = META.get("schema_version", "unset")📝 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.
| META = {} | |
| with open(BASE_DIR / "ciso_assistant" / "meta.json") as f: | |
| META = json.load(f) | |
| SCHEMA_VERSION = META.get("schema_version", "unset") | |
| META = {} | |
| meta_file = BASE_DIR / "ciso_assistant" / "meta.json" | |
| try: | |
| with open(meta_file) as f: | |
| META = json.load(f) | |
| except (FileNotFoundError, json.JSONDecodeError) as e: | |
| logger.warning("Failed to load meta.json: %s", e) | |
| SCHEMA_VERSION = META.get("schema_version", "unset") |
| META = {} | ||
| with open(COMMUNITY_BACKEND_DIR / "backend" / "ciso_assistant" / "meta.json") as f: | ||
| META = json.load(f) | ||
| SCHEMA_VERSION = META.get("schema_version", "unset") |
There was a problem hiding this comment.
Add error handling for meta.json loading.
The code should handle potential file access and JSON parsing errors gracefully.
Apply this diff to add error handling:
META = {}
-with open(COMMUNITY_BACKEND_DIR / "backend" / "ciso_assistant" / "meta.json") as f:
- META = json.load(f)
+try:
+ with open(COMMUNITY_BACKEND_DIR / "backend" / "ciso_assistant" / "meta.json") as f:
+ META = json.load(f)
+except (FileNotFoundError, json.JSONDecodeError, OSError) as e:
+ logger.error("Failed to load meta.json", error=str(e))
SCHEMA_VERSION = META.get("schema_version", "unset")📝 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.
| META = {} | |
| with open(COMMUNITY_BACKEND_DIR / "backend" / "ciso_assistant" / "meta.json") as f: | |
| META = json.load(f) | |
| SCHEMA_VERSION = META.get("schema_version", "unset") | |
| META = {} | |
| try: | |
| with open(COMMUNITY_BACKEND_DIR / "backend" / "ciso_assistant" / "meta.json") as f: | |
| META = json.load(f) | |
| except (FileNotFoundError, json.JSONDecodeError, OSError) as e: | |
| logger.error("Failed to load meta.json", error=str(e)) | |
| SCHEMA_VERSION = META.get("schema_version", "unset") |
nas-tabchiche
force-pushed
the
fix/compare-major-minor-on-import
branch
from
ea9cfed to
e397228
Compare
nas-tabchiche
changed the title
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 (3)
backend/ciso_assistant/meta.py(1 hunks)backend/ciso_assistant/settings.py(2 hunks)enterprise/backend/enterprise_core/settings.py(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- backend/ciso_assistant/meta.py
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/ciso_assistant/settings.py
🧰 Additional context used
🪛 Ruff (0.8.2)
enterprise/backend/enterprise_core/settings.py
14-14: json imported but unused
Remove unused import: json
(F401)
⏰ 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: build (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: test (3.12)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
enterprise/backend/enterprise_core/settings.py (2)
20-20: LGTM! Schema version configuration looks good.The schema version is properly configured:
- Imported from the meta module
- Path configuration for community backend is correctly set
- Schema version is retrieved from the meta module
Also applies to: 23-23, 29-29
109-109: LGTM! Schema version logging is properly integrated.The schema version is now logged alongside other version information during startup, which will help with debugging and monitoring.
| @@ -11,18 +11,22 @@ | |||
|
|
|||
| from pathlib import Path | |||
| import os | |||
| import json | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused import.
The json module is imported but not used in this file.
Apply this diff to remove the unused import:
-import json📝 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.
| import json | |
| # (The unused json import has been removed.) |
🧰 Tools
🪛 Ruff (0.8.2)
14-14: json imported but unused
Remove unused import: json
(F401)
|
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
backend/serdes/views.py (1)
11-11: Remove unused import.The
ValidationErrorimport is not directly used in this file as it's raised bycompare_schema_versions.-from rest_framework.exceptions import ValidationError🧰 Tools
🪛 Ruff (0.8.2)
11-11:
rest_framework.exceptions.ValidationErrorimported but unusedRemove unused import:
rest_framework.exceptions.ValidationError(F401)
backend/core/utils.py (1)
218-218: Fix typo in error message.There's a typo in the error message: "schena" should be "schema".
- "Backup schema version greater than current schena version", + "Backup schema version greater than current schema version",backend/core/views.py (1)
3016-3016: Add missing docstrings for functions.The TODO comment indicates that proper docstrings are missing for several functions. This should be addressed to improve code maintainability and documentation.
Would you like me to help generate docstrings for these functions? I can analyze the code and propose appropriate documentation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/core/tests/test_helpers.py(2 hunks)backend/core/utils.py(2 hunks)backend/core/views.py(4 hunks)backend/serdes/views.py(3 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
backend/serdes/views.py
11-11: rest_framework.exceptions.ValidationError imported but unused
Remove unused import: rest_framework.exceptions.ValidationError
(F401)
backend/core/tests/test_helpers.py
334-334: pytest.raises(Exception) should be considered evil
(B017)
⏰ 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-functional-tests (3.12, chromium)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: build (3.12)
- GitHub Check: test (3.12)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (9)
backend/serdes/views.py (2)
38-40: LGTM!The backup metadata now includes both media and schema versions, which aligns with the PR's objective of implementing schema versioning.
123-133: LGTM!The version validation logic has been improved by:
- Extracting both media and schema versions from metadata.
- Delegating version validation to the
compare_schema_versionsutility.backend/core/utils.py (3)
97-109: LGTM!The function is well-implemented with:
- Clear input validation
- Proper error handling
- Comprehensive docstring
112-163: LGTM!The function is well-designed with:
- Input validation
- Efficient tuple comparison
- Proper version padding
- Comprehensive docstring with examples
166-246: LGTM!The function is well-implemented with:
- Comprehensive schema version validation
- Fallback to semantic version comparison
- Proper error handling
- Detailed logging
- Comprehensive docstring with examples
backend/core/tests/test_helpers.py (1)
165-336: LGTM!The test suite is well-designed with:
- Comprehensive test coverage for all version handling utilities
- Effective use of parameterization for multiple test cases
- Clear test descriptions and comments
- Edge case handling
🧰 Tools
🪛 Ruff (0.8.2)
334-334:
pytest.raises(Exception)should be considered evil(B017)
backend/core/views.py (3)
15-18: LGTM! Clean import statement focusing on email settings.The imports have been streamlined to include only the required email-related settings.
84-88: LGTM! Proper import organization for schema version comparison.The addition of compare_schema_versions import supports the new schema version validation functionality.
2306-2307: Verify schema version validation during file uploads.The schema version validation has been added to ensure compatibility during file uploads. However, there are a few points to consider:
- The schema_version is optional (using .get()) but there's no explicit handling for when it's missing.
- The import_version is required but there's no error handling if it's missing from the meta section.
Run this script to check how other parts of the codebase handle missing schema versions:
Also applies to: 2315-2315
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for schema_version usage patterns rg -A 5 'schema_version' # Search for compare_schema_versions error handling ast-grep --pattern 'try { $$$ compare_schema_versions($_, $_) $$$ }'Length of output: 11123
Schema version validation appears to be working as intended.
The tests in backend/core/tests/test_helpers.py confirm that both cases—when the backup schema version is present and when it’s absent—are properly handled by the
compare_schema_versionsfunction. In particular:
- If the extracted
schema_versionis missing (i.e.None), the function uses the semantic version comparison fallback behavior (as tested by the “happy” and “unhappy” semver tests).- The extraction via
json_dump["meta"].get("schema_version")intentionally makes the field optional, and downstream processing incompare_schema_versionshandles theNonecase appropriately.Regarding the concern over a missing
import_versionin the meta section: our search indicates that other parts of the codebase expectimport_versionto be set elsewhere before callingcompare_schema_versions.There is no explicit error handling here because the validation logic incompare_schema_versionsassumes that either a valid semantic version or aNoneis provided. Any missing or invalidimport_versionthat causes a version comparison error is caught by the tests (for instance, raising aValidationErrorwith"importVersionNotCompatibleWithCurrentVersion").This behavior is consistent with the established patterns elsewhere in the codebase. No additional error handling appears necessary based on the current test coverage and usage patterns.
backend/core/tests/test_helpers.py
Outdated
| "invalid_version" # This should trigger a formatting error in compare_versions. | ||
| ) | ||
| version_b = "1.2.3" | ||
| with pytest.raises(Exception): |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use specific exception type in pytest.raises.
Using a generic Exception in pytest.raises is considered bad practice as it can mask unexpected errors.
- with pytest.raises(Exception):
+ with pytest.raises(VersionFormatError):📝 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.
| with pytest.raises(Exception): | |
| with pytest.raises(VersionFormatError): |
🧰 Tools
🪛 Ruff (0.8.2)
334-334: pytest.raises(Exception) should be considered evil
(B017)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
backend/core/utils.py (2)
97-109: Add input validation for None or empty strings.The function should validate that the input is not None or empty before processing.
def parse_version(version: str) -> list[int]: + if not version: + raise VersionFormatError("Version string cannot be None or empty") if not version.startswith("v"):
166-252: Add return type hint.The function should specify its return type for better type safety.
def compare_schema_versions( schema_ver_a: int | None, version_a: str | None, version_b: str = settings.VERSION.split("-")[0], schema_ver_b: int = settings.SCHEMA_VERSION, level: Literal["major", "minor", "patch"] = "patch", -): +) -> None:backend/core/tests/test_helpers.py (2)
165-186: Consider using parameterization for parse_version tests.Consolidate the version parsing tests using pytest's parameterize decorator for better organization and maintainability.
-def test_parse_version_single_component(): - assert parse_version("v1") == [1] - -def test_parse_version_two_components(): - assert parse_version("v1.2") == [1, 2] - -def test_parse_version_three_components(): - assert parse_version("v1.2.3") == [1, 2, 3] - -def test_parse_version_invalid_missing_v(): - with pytest.raises(VersionFormatError) as exc_info: - parse_version("1.2.3") - assert "must start with 'v'" in str(exc_info.value) - -def test_parse_version_invalid_non_numeric(): - with pytest.raises(VersionFormatError) as exc_info: - parse_version("v1.2.a") - assert "Non-numeric version component" in str(exc_info.value) +@pytest.mark.parametrize( + "version, expected_result, expected_error", + [ + ("v1", [1], None), + ("v1.2", [1, 2], None), + ("v1.2.3", [1, 2, 3], None), + ("1.2.3", None, "must start with 'v'"), + ("v1.2.a", None, "Non-numeric version component"), + ], +) +def test_parse_version(version, expected_result, expected_error): + if expected_error: + with pytest.raises(VersionFormatError) as exc_info: + parse_version(version) + assert expected_error in str(exc_info.value) + else: + assert parse_version(version) == expected_result
272-314: Consider parameterizing error test cases.Consolidate the error test cases using pytest's parameterize decorator for better organization.
-def test_schema_version_mismatch(): - schema_ver_a = 3 - schema_ver_b = 2 - version_a = "v1.2.3" - version_b = "v1.2.3" - with pytest.raises(ValidationError) as excinfo: - compare_schema_versions(schema_ver_a, version_a, version_b, schema_ver_b) - assert excinfo.value.args[0]["error"] == "backupGreaterVersionError" - -def test_semver_backup_greater(): - schema_ver_a = None - version_a = "v1.3.0" - version_b = "v1.2.9" - with pytest.raises(ValidationError) as excinfo: - compare_schema_versions(schema_ver_a, version_a, version_b, schema_ver_b=1) - assert excinfo.value.args[0]["error"] == "backupGreaterVersionError" - -def test_semver_backup_less(): - schema_ver_a = None - version_a = "v1.1.0" - version_b = "v1.2.0" - with pytest.raises(ValidationError) as excinfo: - compare_schema_versions(schema_ver_a, version_a, version_b, schema_ver_b=1) - assert excinfo.value.args[0]["error"] == "importVersionNotCompatibleWithCurrentVersion" +@pytest.mark.parametrize( + "schema_ver_a, schema_ver_b, version_a, version_b, expected_error", + [ + (3, 2, "v1.2.3", "v1.2.3", "backupGreaterVersionError"), + (None, 1, "v1.3.0", "v1.2.9", "backupGreaterVersionError"), + (None, 1, "v1.1.0", "v1.2.0", "importVersionNotCompatibleWithCurrentVersion"), + ], +) +def test_schema_version_errors(schema_ver_a, schema_ver_b, version_a, version_b, expected_error): + with pytest.raises(ValidationError) as excinfo: + compare_schema_versions(schema_ver_a, version_a, version_b, schema_ver_b) + assert excinfo.value.args[0]["error"] == expected_error
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/core/tests/test_helpers.py(2 hunks)backend/core/utils.py(2 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: functional-tests (3.12, chromium)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: build (3.12)
- GitHub Check: test (3.12)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (4)
backend/core/utils.py (2)
91-94: LGTM!The exception class is well-defined and properly documented.
112-163: LGTM!The function is well-implemented with:
- Comprehensive documentation
- Clear error handling
- Efficient tuple comparison
- Good examples in docstring
backend/core/tests/test_helpers.py (2)
193-246: LGTM!The tests are well-organized with:
- Effective use of parameterization
- Comprehensive coverage of comparison scenarios
- Clear test cases with good comments
334-334: Use specific exception type in pytest.raises.Using a generic
Exceptioninpytest.raisesis considered bad practice as it can mask unexpected errors.
Summary by CodeRabbit
schema_versionin the metadata structure for serialized backup data.