[firmware] Fix HTTP 500 when saving device after credentials are removed#364
Conversation
…ng firmware validation openwisp#250 Convert DeviceConnection.DoesNotExist raised during firmware upgrade validation into a proper ValidationError so the admin form can display a recoverable error instead of crashing. This allows users to reassign credentials and save the device configuration without encountering an internal server error. Fixes openwisp#250 .
komaleswarreddy
force-pushed
the
issues/250-handle-missing-credentials-validation
branch
from
4f271f8 to
b3dc829
Compare
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
In the current state this PR adds more problems than it solves.
If the CI build is failing for some reason, let it fail so we can debug it instead of trying to cover it up with bloat.
See my comments below.
Please remove the extra bloat.
| @@ -0,0 +1,10 @@ | |||
| # Minimal stub migration for openwisp_notifications | |||
There was a problem hiding this comment.
Adding openwisp_notifications here is most likely wrong, what is prompting you to do this?
| "openwisp_controller.config", | ||
| "openwisp_controller.connection", | ||
| "openwisp_controller.geo", | ||
| # "openwisp_controller.geo", # Temporarily disabled - requires GDAL |
There was a problem hiding this comment.
This is working fine on master, it seems wrong to do this.
|
|
||
| if __name__ == "__main__": | ||
| # Add project root to path so Django can find openwisp_notifications stub | ||
| project_root = os.path.dirname(os.path.abspath(__file__)) |
There was a problem hiding this comment.
same here, this is unacceptable
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughAdds guarded retrieval of the upgrader class and structured validation of upgrade options to prevent AttributeError when device connections/credentials are missing; converts FirmwareUpgradeOptionsException payloads to picklable ValidationError messages; adds regression tests and updates test settings and test requirements. Changes
Sequence Diagram(s)sequenceDiagram
%%{init: {"themeVariables":{"actorBorder":"#2b6cb0","actorBackground":"#bee3f8","noteBorder":"#9ae6b4","noteBackground":"#f0fff4"}}}%%
actor Admin
participant DeviceModel as DeviceFirmware / UpgradeOperation
participant Lookup as Upgrader lookup
participant Upgrader as UpgraderClass
participant Validator as JSON Schema / upgrader.validate_options
Admin->>DeviceModel: save / full_clean()
DeviceModel->>Lookup: get upgrader_class (guarded)
alt upgrader_class missing
Lookup-->>DeviceModel: ValidationError("assign credentials/connection first")
DeviceModel-->>Admin: raise ValidationError
else upgrader_class present
Lookup-->>DeviceModel: upgrader_class
DeviceModel->>Upgrader: inspect SCHEMA
Upgrader->>Validator: validate options
alt validation fails with FirmwareUpgradeOptionsException
Validator-->>DeviceModel: convert args to picklable strings/dict -> ValidationError
DeviceModel-->>Admin: raise ValidationError
else success
DeviceModel-->>Admin: save succeeds
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
🔇 Additional comments (1)
Comment |
komaleswarreddy
force-pushed
the
issues/250-handle-missing-credentials-validation
branch
from
b3dc829 to
683ef02
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
openwisp_firmware_upgrader/base/models.py (2)
58-65: Good fix for the missing credentials scenario.This correctly catches
ObjectDoesNotExistwhen the device has no valid connection and raises a user-friendlyValidationErrorinstead of crashing with HTTP 500.Consider adding exception chaining for better debugging:
🔎 Proposed improvement
try: upgrader_class = self.upgrader_class except ObjectDoesNotExist: - raise ValidationError( + raise ValidationError( "Cannot validate firmware upgrade options because the device " "has no valid credentials or connection assigned. " "Please assign credentials to the device first." - ) + ) from NoneUsing
from Noneexplicitly suppresses the exception chain, which is appropriate here since the user doesn't need to see the internalDoesNotExisttraceback.
70-87: Good defensive handling ofFirmwareUpgradeOptionsException.The conversion of lazy translation values to strings ensures picklability for Celery tasks, and the fallback handling covers edge cases appropriately.
For consistency with Django best practices, consider adding exception chaining to preserve context during debugging:
🔎 Optional: Add exception chaining
except jsonschema.ValidationError: - raise ValidationError("The upgrade options are invalid") + raise ValidationError("The upgrade options are invalid") from None except FirmwareUpgradeOptionsException as error: # Extract only picklable string messages from error.args # error.args[0] is typically a dict with lazy translation values if error.args and isinstance(error.args[0], dict): # Convert dict with lazy translations to dict with plain strings clean_message_dict = { key: str(value) for key, value in error.args[0].items() } - raise ValidationError(clean_message_dict) + raise ValidationError(clean_message_dict) from None elif error.args: # If it's a plain string or other type, convert to string - raise ValidationError(str(error.args[0])) + raise ValidationError(str(error.args[0])) from None else: - raise ValidationError("Invalid upgrade options") + raise ValidationError("Invalid upgrade options") from Noneopenwisp_firmware_upgrader/tests/test_models.py (2)
176-224: Well-structured regression test for issue #250.The test properly validates that
ValidationErroris raised instead ofDoesNotExistwhen credentials are removed after DeviceFirmware assignment. The helper function pattern effectively captures the error message while avoiding potential pickling issues.Optional: Address static analysis style suggestion (TRY300)
The static analysis tool suggests moving the
return Noneinto anelseblock for better code organization:def get_validation_error_message(): try: device_fw.full_clean() - return None except ValidationError as e: return " ".join(str(msg) for msg in e.messages) + else: + return NoneThis is a minor style improvement and not required for functionality.
292-344: Comprehensive test coverage for UpgradeOperation validation.This test mirrors the DeviceFirmware test but validates the UpgradeOperation code path, ensuring both scenarios from issue #250 are covered. The test correctly verifies that credential removal results in
ValidationErrorrather than an HTTP 500 error.Optional: Address static analysis style suggestion (TRY300)
Similar to the previous test, consider moving the
return Noneinto anelseblock:def get_validation_error_message(): try: uo.full_clean() - return None except ValidationError as e: return " ".join(str(msg) for msg in e.messages) + else: + return None
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
openwisp_firmware_upgrader/base/models.pyopenwisp_firmware_upgrader/tests/test_models.pyopenwisp_notifications/__init__.pyopenwisp_notifications/apps.pyopenwisp_notifications/context_processors.pyopenwisp_notifications/handlers.pyopenwisp_notifications/signals.pyopenwisp_notifications/types.pyopenwisp_notifications/utils.pytests/openwisp2/settings.py
🧰 Additional context used
🧬 Code graph analysis (1)
openwisp_firmware_upgrader/base/models.py (2)
openwisp_firmware_upgrader/upgraders/openwrt.py (1)
validate_upgrade_options(108-126)openwisp_firmware_upgrader/exceptions.py (1)
FirmwareUpgradeOptionsException(5-6)
🪛 Ruff (0.14.10)
openwisp_notifications/utils.py
3-3: Unused function argument: args
(ARG001)
3-3: Unused function argument: kwargs
(ARG001)
openwisp_firmware_upgrader/base/models.py
61-65: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
61-65: Avoid specifying long messages outside the exception class
(TRY003)
66-66: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
67-69: Avoid specifying long messages outside the exception class
(TRY003)
73-73: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
73-73: Avoid specifying long messages outside the exception class
(TRY003)
82-82: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
85-85: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
87-87: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
87-87: Avoid specifying long messages outside the exception class
(TRY003)
openwisp_firmware_upgrader/tests/test_models.py
207-207: Consider moving this statement to an else block
(TRY300)
327-327: Consider moving this statement to an else block
(TRY300)
openwisp_notifications/context_processors.py
3-3: Unused function argument: request
(ARG001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.1.0
🔇 Additional comments (7)
openwisp_notifications/utils.py (1)
1-6: LGTM!This stub correctly provides a placeholder implementation for the
_get_object_linkfunction. The unused*args, **kwargsis intentional for maintaining signature compatibility with the real implementation.openwisp_notifications/__init__.py (1)
1-2: LGTM!Minimal package stub to work around Windows path length issues.
openwisp_notifications/signals.py (1)
1-6: LGTM!Appropriate stub for the
notifysignal function.openwisp_notifications/handlers.py (1)
1-6: LGTM!Appropriate stub for the notification cache update handler.
openwisp_notifications/context_processors.py (1)
1-6: LGTM!The context processor stub correctly maintains the required Django signature
(request)and returns an empty dictionary. The unusedrequestparameter is expected for maintaining compatibility.openwisp_notifications/apps.py (1)
1-7: LGTM!Standard Django app configuration with appropriate naming.
openwisp_notifications/types.py (1)
1-18: LGTM!Appropriate stubs for notification type registration with an empty
NOTIFICATION_TYPESlist.
| if not getattr(upgrader_class, "SCHEMA"): | ||
| raise ValidationError( | ||
| "Using upgrade options is not allowed with this upgrader." | ||
| ) |
There was a problem hiding this comment.
Potential AttributeError if SCHEMA attribute doesn't exist.
The getattr(upgrader_class, "SCHEMA") without a default value will raise AttributeError if the SCHEMA attribute is not defined on the class, rather than returning a falsy value.
🔎 Proposed fix
- if not getattr(upgrader_class, "SCHEMA"):
+ if not getattr(upgrader_class, "SCHEMA", None):
raise ValidationError(
"Using upgrade options is not allowed with this upgrader."
- )
+ ) from None📝 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.
| if not getattr(upgrader_class, "SCHEMA"): | |
| raise ValidationError( | |
| "Using upgrade options is not allowed with this upgrader." | |
| ) | |
| if not getattr(upgrader_class, "SCHEMA", None): | |
| raise ValidationError( | |
| "Using upgrade options is not allowed with this upgrader." | |
| ) from None |
🧰 Tools
🪛 Ruff (0.14.10)
66-66: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
67-69: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In openwisp_firmware_upgrader/base/models.py around lines 66 to 69, the code
uses getattr(upgrader_class, "SCHEMA") without a default which can raise
AttributeError if SCHEMA is missing; update the check to use
getattr(upgrader_class, "SCHEMA", None) (or use hasattr(upgrader_class,
"SCHEMA")) and then test the returned value for truthiness, raising
ValidationError when the attribute is missing or falsy so the check never raises
an AttributeError.
| DATABASES = { | ||
| "default": { | ||
| "ENGINE": "openwisp_utils.db.backends.spatialite", | ||
| "ENGINE": "django.db.backends.sqlite3", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n tests/openwisp2/settings.py | head -150Repository: openwisp/openwisp-firmware-upgrader
Length of output: 5372
🏁 Script executed:
git diff tests/openwisp2/settings.pyRepository: openwisp/openwisp-firmware-upgrader
Length of output: 61
🏁 Script executed:
git log --oneline -10 -- tests/openwisp2/settings.pyRepository: openwisp/openwisp-firmware-upgrader
Length of output: 160
🏁 Script executed:
git diff HEAD~1 tests/openwisp2/settings.pyRepository: openwisp/openwisp-firmware-upgrader
Length of output: 267
🏁 Script executed:
git show HEAD:tests/openwisp2/settings.py | head -150Repository: openwisp/openwisp-firmware-upgrader
Length of output: 4322
🏁 Script executed:
git log --all --oneline | grep -i "credential\|#250" | head -5Repository: openwisp/openwisp-firmware-upgrader
Length of output: 160
🏁 Script executed:
git show 683ef02 --statRepository: openwisp/openwisp-firmware-upgrader
Length of output: 8078
🏁 Script executed:
git log --oneline | head -20Repository: openwisp/openwisp-firmware-upgrader
Length of output: 160
🏁 Script executed:
git log --all --oneline | wc -lRepository: openwisp/openwisp-firmware-upgrader
Length of output: 80
🏁 Script executed:
# Check if settings were intentionally disabled for this PR or are pre-existing
git show 683ef02:tests/openwisp2/settings.py | grep -A2 "openwisp_controller.geo\|openwisp_notifications" | head -20Repository: openwisp/openwisp-firmware-upgrader
Length of output: 487
🏁 Script executed:
# Check the actual changes introduced in this commit for the credential fix
git show 683ef02 -- openwisp_firmware_upgrader/ | head -200Repository: openwisp/openwisp-firmware-upgrader
Length of output: 7189
🏁 Script executed:
# Look for the specific change mentioned in the commit message
git show 683ef02 | grep -A5 -B5 "DeviceConnection.DoesNotExist\|ValidationError" | head -50Repository: openwisp/openwisp-firmware-upgrader
Length of output: 1788
🏁 Script executed:
# Check if there are any test failures related to the disabled modules
git show 683ef02:tests/openwisp2/local_settings.example.py 2>/dev/null || echo "File not found"Repository: openwisp/openwisp-firmware-upgrader
Length of output: 421
Remove or properly justify the test infrastructure workarounds from the committed code.
The disabled modules in settings.py (lines 42, 45, 131) appear to be environment-specific workarounds rather than necessary changes for the credential validation fix. Comments indicate they were disabled due to missing GDAL dependencies and Windows path length issues—issues that should be documented in setup instructions or handled via local configuration, not committed as "Temporarily disabled" code.
Three options:
- Remove these disabled imports entirely if they are not required for tests
- Move them to
local_settings.pyor environment-specific configuration - Document why they must remain disabled and what the plan is for re-enabling them
Test settings should be reproducible across environments without temporary workarounds.
🤖 Prompt for AI Agents
In tests/openwisp2/settings.py (referencing lines ~13, 42, 45, 131) the file
contains committed "Temporarily disabled" module imports/workarounds for GDAL
and Windows path-length problems; remove these environment-specific disables
from the shared test settings and either (a) delete the disabled imports
entirely if tests pass without them, or (b) move them into a local_settings.py
(or an environment-specific config) that is gitignored and imported optionally,
and add a short entry in README/CONTRIBUTING describing how to reproduce the
environment and how to enable those modules locally; ensure the main test
settings file contains no commented-out workarounds and fails fast if required
modules are missing so CI and other developers use consistent setup.
683ef02 to
c21dc26
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
openwisp_firmware_upgrader/base/models.py (1)
66-69: Past review comment still applies:getattrneeds a default value.As noted in the previous review,
getattr(upgrader_class, "SCHEMA")without a default will raiseAttributeErrorif theSCHEMAattribute doesn't exist, rather than allowing the truthiness check to work as intended.Based on past review feedback, apply:
- if not getattr(upgrader_class, "SCHEMA"): + if not getattr(upgrader_class, "SCHEMA", None): raise ValidationError( "Using upgrade options is not allowed with this upgrader." - ) + ) from None
🧹 Nitpick comments (2)
openwisp_firmware_upgrader/base/models.py (2)
58-65: Core fix looks good – handles missing credentials gracefully.This correctly catches
ObjectDoesNotExistwhenself.upgrader_classis accessed without valid device credentials and raises a user-friendlyValidationErrorinstead of causing an HTTP 500.Optional: Suppress exception chain with
from NoneTo make it clear this is an intentional conversion from an internal exception to a user-facing error, consider adding
from None:except ObjectDoesNotExist: raise ValidationError( "Cannot validate firmware upgrade options because the device " "has no valid credentials or connection assigned. " "Please assign credentials to the device first." - ) + ) from None
74-87: Good handling of lazy translation pickling issues.The conversion of
FirmwareUpgradeOptionsExceptionerror messages to plain strings correctly addresses pickling issues with Django's lazy translations. The three-branch handling (dict, other types, empty args) appropriately covers all cases.Optional: Add
from Noneto clarify intentional exception conversionConsider adding
from Noneto suppress the exception chain when converting exception types:if error.args and isinstance(error.args[0], dict): # Convert dict with lazy translations to dict with plain strings clean_message_dict = { key: str(value) for key, value in error.args[0].items() } - raise ValidationError(clean_message_dict) + raise ValidationError(clean_message_dict) from None elif error.args: # If it's a plain string or other type, convert to string - raise ValidationError(str(error.args[0])) + raise ValidationError(str(error.args[0])) from None else: - raise ValidationError("Invalid upgrade options") + raise ValidationError("Invalid upgrade options") from None
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
openwisp_firmware_upgrader/base/models.pyopenwisp_firmware_upgrader/tests/test_models.py
🧰 Additional context used
🧬 Code graph analysis (2)
openwisp_firmware_upgrader/base/models.py (2)
openwisp_firmware_upgrader/upgraders/openwrt.py (1)
validate_upgrade_options(108-126)openwisp_firmware_upgrader/exceptions.py (1)
FirmwareUpgradeOptionsException(5-6)
openwisp_firmware_upgrader/tests/test_models.py (3)
openwisp_firmware_upgrader/tests/base.py (1)
_create_device_firmware(104-118)openwisp_firmware_upgrader/admin.py (1)
full_clean(355-366)openwisp_firmware_upgrader/models.py (1)
UpgradeOperation(43-46)
🪛 Ruff (0.14.10)
openwisp_firmware_upgrader/base/models.py
61-65: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
61-65: Avoid specifying long messages outside the exception class
(TRY003)
66-66: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
67-69: Avoid specifying long messages outside the exception class
(TRY003)
73-73: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
73-73: Avoid specifying long messages outside the exception class
(TRY003)
82-82: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
85-85: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
87-87: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
87-87: Avoid specifying long messages outside the exception class
(TRY003)
openwisp_firmware_upgrader/tests/test_models.py
207-207: Consider moving this statement to an else block
(TRY300)
327-327: Consider moving this statement to an else block
(TRY300)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
🔇 Additional comments (2)
openwisp_firmware_upgrader/tests/test_models.py (2)
176-224: Excellent regression test for the credentials-removal scenario.This test correctly validates that
ValidationErroris raised (notDoesNotExist) when credentials are removed afterDeviceFirmwareis created. The helper function pattern effectively avoids pickling issues withValidationErrorin test assertions, and the keyword checks ensure the error message is appropriate.
292-344: Good parallel test for UpgradeOperation validation path.This test mirrors the DeviceFirmware test but validates the
UpgradeOperationvalidation path. Together, these two tests provide comprehensive regression coverage for the credentials-removal scenario across both validation entry points.
komaleswarreddy
force-pushed
the
issues/250-handle-missing-credentials-validation
branch
from
c21dc26 to
8bf022e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
openwisp_firmware_upgrader/base/models.py (1)
66-69: PotentialAttributeErrorifSCHEMAattribute doesn't exist.The
getattr(upgrader_class, "SCHEMA")call without a default value will raiseAttributeErrorif theSCHEMAattribute is not defined on the upgrader class, rather than returning a falsy value that can be checked. This was flagged in a previous review and should be addressed.🔎 Proposed fix
- if not getattr(upgrader_class, "SCHEMA"): - raise ValidationError( - "Using upgrade options is not allowed with this upgrader." - ) + if not getattr(upgrader_class, "SCHEMA", None): + raise ValidationError( + "Using upgrade options is not allowed with this upgrader." + ) from NoneBased on past review comments.
🧹 Nitpick comments (2)
openwisp_firmware_upgrader/base/models.py (2)
58-65: Good fix for missing credentials, but add exception chaining.The try-except block properly catches
ObjectDoesNotExistand converts it to aValidationError, which fixes the HTTP 500 issue described in the PR objectives. However, theraisestatement should usefrom Noneto suppress the original exception traceback since this is an expected condition.🔎 Suggested refinement
try: upgrader_class = self.upgrader_class except ObjectDoesNotExist: raise ValidationError( "Cannot validate firmware upgrade options because the device " "has no valid credentials or connection assigned. " "Please assign credentials to the device first." - ) + ) from None
74-87: Excellent enhancement for picklability, consider exception chaining.The conversion of
FirmwareUpgradeOptionsExceptionerror arguments to picklable strings is a well-thought-out solution that handles lazy translation objects properly. The logic correctly handles different types of error arguments (dict, other types, empty).Consider adding
from errorto theraisestatements to preserve the exception chain for debugging purposes.🔎 Optional enhancement for exception chaining
if error.args and isinstance(error.args[0], dict): # Convert dict with lazy translations to dict with plain strings clean_message_dict = { key: str(value) for key, value in error.args[0].items() } - raise ValidationError(clean_message_dict) + raise ValidationError(clean_message_dict) from error elif error.args: # If it's a plain string or other type, convert to string - raise ValidationError(str(error.args[0])) + raise ValidationError(str(error.args[0])) from error else: - raise ValidationError("Invalid upgrade options") + raise ValidationError("Invalid upgrade options") from error
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
openwisp_firmware_upgrader/base/models.pyopenwisp_firmware_upgrader/tests/test_models.py
🧰 Additional context used
🧬 Code graph analysis (2)
openwisp_firmware_upgrader/tests/test_models.py (2)
openwisp_firmware_upgrader/admin.py (1)
full_clean(355-366)openwisp_firmware_upgrader/models.py (1)
UpgradeOperation(43-46)
openwisp_firmware_upgrader/base/models.py (2)
openwisp_firmware_upgrader/upgraders/openwrt.py (1)
validate_upgrade_options(108-126)openwisp_firmware_upgrader/exceptions.py (1)
FirmwareUpgradeOptionsException(5-6)
🪛 Ruff (0.14.10)
openwisp_firmware_upgrader/tests/test_models.py
207-207: Consider moving this statement to an else block
(TRY300)
327-327: Consider moving this statement to an else block
(TRY300)
openwisp_firmware_upgrader/base/models.py
61-65: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
61-65: Avoid specifying long messages outside the exception class
(TRY003)
66-66: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
67-69: Avoid specifying long messages outside the exception class
(TRY003)
73-73: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
73-73: Avoid specifying long messages outside the exception class
(TRY003)
82-82: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
85-85: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
87-87: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
87-87: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
🔇 Additional comments (2)
openwisp_firmware_upgrader/tests/test_models.py (2)
176-224: LGTM! Well-structured regression test.The test properly validates that
ValidationErroris raised (notDoesNotExist) when credentials are removed after firmware assignment. The helper function pattern effectively isolates exception handling and avoids pickling issues in the test environment.
292-344: LGTM! Comprehensive regression test coverage.This test mirrors the DeviceFirmware test for UpgradeOperation, ensuring consistent behavior across both models when credentials are missing. The validation logic and error message assertions are appropriate.
…penwisp#250 Convert DeviceConnection.DoesNotExist raised during firmware upgrade validation into ValidationError. This prevents HTTP 500 errors in Django admin when device credentials are removed and allows users to recover. Also removes openwisp_notifications stub that was causing migration dependency issues. Fixes openwisp#250
komaleswarreddy
force-pushed
the
issues/250-handle-missing-credentials-validation
branch
from
8bf022e to
3818fdc
Compare
|
@pandafy @nemesifier I’ve cleaned up the PR so it now only contains the firmware upgrader validation fix and the related regression tests for #250. All unrelated changes (stubs, settings tweaks, test runner changes) have been removed. The remaining issue I was seeing locally turned out to be an environment problem (missing openwisp-notifications migrations), not a code issue, and does not require changes in this PR. Please let me know if the current scope looks correct now |
| @@ -1,4 +1,5 @@ | |||
| openwisp-utils[qa,selenium] @ https://github.com/openwisp/openwisp-utils/archive/refs/heads/1.3.tar.gz | |||
| openwisp-notifications @ https://github.com/openwisp/openwisp-notifications/archive/refs/heads/1.3.tar.gz | |||
There was a problem hiding this comment.
Thanks for the contribution! However, we cannot accept changes to dependencies or core settings at this time as they complicate our CI/CD pipeline. Please revert those specific changes so we can focus on reviewing the core logic. Since we’re currently handling a high volume of PRs, we have to prioritize "clean" submissions that don't introduce build risks. If this task feels a bit complex, I’d be happy to point you toward some "Good First Issues" that are easier to get through the pipeline!
|
Hi @komaleswarreddy 👋, This pull request has been automatically closed due to 127 days of inactivity. After changes were requested, the PR remained inactive. We understand that life gets busy, and we appreciate your initial contribution! 💙 The door is always open for you to come back:
If you have any questions or need help, don't hesitate to reach out. We're here to support you! Thank you for your interest in contributing to OpenWISP! 🙏 |
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (4 files)
OverviewThis PR successfully fixes the HTTP 500 error described in issue #250. The fix correctly catches the DoesNotExist exception raised when device credentials are deleted after firmware assignment and converts it to a proper ValidationError, allowing the Django admin to display a user-friendly error message instead of crashing. The implementation correctly:
Reviewed by kimi-k2.5 · 306,501 tokens |
|
Hi @komaleswarreddy 👋, This pull request has been automatically closed due to 128 days of inactivity. After changes were requested, the PR remained inactive. We understand that life gets busy, and we appreciate your initial contribution! 💙 The door is always open for you to come back:
If you have any questions or need help, don't hesitate to reach out. We're here to support you! Thank you for your interest in contributing to OpenWISP! 🙏 |
|
Hi @komaleswarreddy 👋, This pull request has been marked as stale due to 133 days of inactivity after changes were requested. As a result, any linked issues are being unassigned from you so other contributors can pick them up. However, you can still continue working on this PR! If you push new commits or respond to the review feedback:
If you need more time or have questions about the requested changes, please let us know. We're happy to help! 🤝 |
|
Hi @komaleswarreddy 👋, This PR has been inactive for 134 days since changes were requested. Are you still working on it? If yes, push new commits or reply to let us know. If you've moved on, please close the PR or comment so another contributor can pick it up. |
3 participants
This PR fixes an issue where deleting device credentials after assigning
a firmware image causes Django admin to crash with HTTP 500.
The crash was caused by an uncaught DoesNotExist exception raised during
firmware upgrade validation when no valid device connection was found.
This PR converts the exception into a ValidationError so the admin shows
a proper, recoverable error message.
Changes
Fixes #250
Summary by CodeRabbit
Bug Fixes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.