[fix] Prevent HTTP 500 on missing device credentials#404
[fix] Prevent HTTP 500 on missing device credentials#404Eeshu-Yadav wants to merge 4 commits intoopenwisp:masterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughValidation for upgrade options was made more robust: resolving the upgrader class is wrapped to convert missing related connection/credentials into a ValidationError with a device-specific message; the SCHEMA check uses the resolved class via Sequence Diagram(s)sequenceDiagram
participant AdminForm as Admin form
participant Model as DeviceFirmware / UpgradeOperation
participant UpgradeMixin as UpgradeOptionsMixin.validate_upgrade_options
participant DB as Database (DeviceConnection)
participant Upgrader as Resolved upgrader class
AdminForm->>Model: full_clean() / save()
Model->>UpgradeMixin: validate_upgrade_options()
UpgradeMixin->>DB: lookup related connection/credentials
DB-->>UpgradeMixin: connection data or ObjectDoesNotExist
alt ObjectDoesNotExist
UpgradeMixin-->>Model: raise ValidationError("No related connection or credentials found for this device.")
Model-->>AdminForm: propagate form error
else Connection found
UpgradeMixin->>Upgrader: resolve upgrader_class
UpgradeMixin->>Upgrader: check getattr(upgrader_class, "SCHEMA", None)
alt SCHEMA absent/falsey
UpgradeMixin-->>Model: raise ValidationError("Using upgrade options is not allowed with this upgrader.")
Model-->>AdminForm: propagate form error
else SCHEMA present
UpgradeMixin-->>Model: validate upgrade options (continue)
Model-->>AdminForm: save succeeds
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Fixes an admin-side crash (HTTP 500) when saving models that use upgrade_options after device connection/credentials have been removed, by converting the underlying DoesNotExist into a user-facing ValidationError.
Changes:
- Catch
ObjectDoesNotExistwhen resolvingself.upgrader_classduringvalidate_upgrade_options()and raise aValidationErrorinstead. - Use a safe
getattr(..., None)when checkingSCHEMAto avoidAttributeErrorwhen no upgrader class is available. - Add regression tests covering both
DeviceFirmwareandUpgradeOperationvalidation when credentials/connections are removed.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| openwisp_firmware_upgrader/base/models.py | Prevents unhandled exceptions in validate_upgrade_options() by converting missing-connection scenarios into a ValidationError. |
| openwisp_firmware_upgrader/tests/test_models.py | Adds regression coverage ensuring missing credentials/connections produce validation errors instead of server errors. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pandafy
left a comment
There was a problem hiding this comment.
@Eeshu-Yadav Thanks for the patch. I’ve tested the changes and can now see an error message on the existing DeviceFirmware object:
However, the issue still cannot be resolved unless the DeviceFirmwareImage object is deleted and recreated.
Maybe, we can execute the following check only when the image field of the DeviceFirmware object changes or when a new DeviceFirmware object is created.
openwisp-firmware-upgrader/openwisp_firmware_upgrader/base/models.py
Lines 412 to 419 in 10e5aa3
4ad6f3b to
efccfe0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openwisp_firmware_upgrader/base/models.py (1)
412-419:⚠️ Potential issue | 🟠 MajorDon't skip the connection prerequisite for pending firmware.
This now lets an existing
DeviceFirmwarewithinstalled=Falsepassfull_clean()as long as the image is unchanged, butsave()still creates a new upgrade operation whennot self.installed(Lines 427-433). With no connection left, that operation immediately hits the"No device connection available"branch inupgrade()and never transitions out ofin-progress(Lines 916-922). Please keep this guard aligned with the save trigger, not justimage_has_changed.Suggested fix
- if self.image_has_changed and self.device.deviceconnection_set.count() < 1: + if ( + (self.image_has_changed or not self.installed) + and self.device.deviceconnection_set.count() < 1 + ): raise ValidationError( _( "This device does not have a related connection object defined "🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_firmware_upgrader/base/models.py` around lines 412 - 419, The current validation in DeviceFirmware.full_clean only blocks missing device connections when image_has_changed, but save() will still create an upgrade when installed is False and later upgrade() fails due to no connection; update the guard in the model (the block referencing image_has_changed and device.deviceconnection_set.count()) to also enforce the prerequisite when the firmware is pending install (i.e. when self.installed is False) so it reads logically like "if self.image_has_changed or not self.installed" then raise ValidationError; ensure this touches the same DeviceFirmware attributes/methods referenced (image_has_changed, installed, device.deviceconnection_set.count()) so full_clean() prevents save() from creating an upgrade with no connections.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openwisp_firmware_upgrader/tests/test_models.py`:
- Around line 180-184: Update the test_device_fw_save_after_credentials_removed
to cover the unchanged-image save path: create a DeviceFirmware instance with
installed=False (pending), record the current count of UpgradeOperation for that
device/firmware, remove device connections via
device_fw.device.deviceconnection_set.all().delete(), call device_fw.save() (not
just full_clean()), and assert that no new UpgradeOperation was queued (i.e.,
the UpgradeOperation count for that device/firmware remains unchanged). Repeat
the same pattern for the related test block around lines 252-264 to ensure
save() behavior is covered in both cases and reference
AbstractDeviceFirmware.clean() and save() logic when writing the assertions.
---
Outside diff comments:
In `@openwisp_firmware_upgrader/base/models.py`:
- Around line 412-419: The current validation in DeviceFirmware.full_clean only
blocks missing device connections when image_has_changed, but save() will still
create an upgrade when installed is False and later upgrade() fails due to no
connection; update the guard in the model (the block referencing
image_has_changed and device.deviceconnection_set.count()) to also enforce the
prerequisite when the firmware is pending install (i.e. when self.installed is
False) so it reads logically like "if self.image_has_changed or not
self.installed" then raise ValidationError; ensure this touches the same
DeviceFirmware attributes/methods referenced (image_has_changed, installed,
device.deviceconnection_set.count()) so full_clean() prevents save() from
creating an upgrade with no connections.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 199cd677-d840-4cc2-b038-7ebb7a51204f
📒 Files selected for processing (2)
openwisp_firmware_upgrader/base/models.pyopenwisp_firmware_upgrader/tests/test_models.py
📜 Review details
⏰ 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.10 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
openwisp_firmware_upgrader/tests/test_models.pyopenwisp_firmware_upgrader/base/models.py
🧠 Learnings (9)
📚 Learning: 2026-03-26T20:36:45.314Z
Learnt from: CR
Repo: openwisp/openwisp-firmware-upgrader PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-26T20:36:45.314Z
Learning: Update tests to cover non-trivial behavior changes
Applied to files:
openwisp_firmware_upgrader/tests/test_models.py
📚 Learning: 2026-02-25T11:20:13.903Z
Learnt from: pandafy
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: runtests.py:27-38
Timestamp: 2026-02-25T11:20:13.903Z
Learning: In openwisp-firmware-upgrader, runtests.py now uses `execute_from_command_line()` to run Django tests in-process rather than via subprocess, so the `patch = subprocess` coverage directive is not needed in pyproject.toml.
Applied to files:
openwisp_firmware_upgrader/tests/test_models.pyopenwisp_firmware_upgrader/base/models.py
📚 Learning: 2026-02-27T19:08:56.218Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 383
File: openwisp_firmware_upgrader/tests/test_admin.py:50-50
Timestamp: 2026-02-27T19:08:56.218Z
Learning: In tests for openwisp-firmware-upgrader, derive app_label values from model meta instead of hard-coding. Specifically, let BaseTestAdmin compute app_label from Build._meta.app_label and config_app_label from Device._meta.app_label to ensure tests remain correct if models are swapped. This pattern should apply to all test files under openwisp_firmware_upgrader/tests.
Applied to files:
openwisp_firmware_upgrader/tests/test_models.py
📚 Learning: 2026-02-27T19:09:01.705Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 383
File: openwisp_firmware_upgrader/tests/test_admin.py:50-50
Timestamp: 2026-02-27T19:09:01.705Z
Learning: In openwisp-firmware-upgrader tests, the BaseTestAdmin class should derive app_label from Build._meta.app_label and config_app_label from Device._meta.app_label instead of hard-coding them. This ensures tests work correctly when models are swapped.
Applied to files:
openwisp_firmware_upgrader/base/models.py
📚 Learning: 2026-02-23T21:36:30.581Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: tests/openwisp2/sample_firmware_upgrader/migrations/0001_initial.py:61-67
Timestamp: 2026-02-23T21:36:30.581Z
Learning: In the openwisp/openwisp-firmware-upgrader repository, the sample app (tests/openwisp2/sample_firmware_upgrader) can have a different migration path from the main app. New migration files are generally not created for small changes in the sample app since it's only used for demonstration and testing purposes. The sample app's initial migration may include fields or choices that are added incrementally in the main app's migration history.
Applied to files:
openwisp_firmware_upgrader/base/models.py
📚 Learning: 2026-03-07T01:16:24.447Z
Learnt from: atif09
Repo: openwisp/openwisp-firmware-upgrader PR: 387
File: openwisp_firmware_upgrader/admin.py:420-423
Timestamp: 2026-03-07T01:16:24.447Z
Learning: In openwisp-firmware-upgrader, UpgradeOperation and BatchUpgradeOperation always share the same organization (BatchUpgradeOperation.build.category.organization). A user who can view an UpgradeOperation can always view its related BatchUpgradeOperation. Model-level has_view_permission(request) is sufficient for batch breadcrumb visibility in UpgradeOperationAdmin.change_view; object-level permission checks are not needed.
Applied to files:
openwisp_firmware_upgrader/base/models.py
📚 Learning: 2026-02-21T20:21:04.228Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: openwisp_firmware_upgrader/migrations/0015_add_group_to_batchupgradeoperation.py:16-26
Timestamp: 2026-02-21T20:21:04.228Z
Learning: In openwisp-firmware-upgrader project, the team prefers not to add explicit `related_name` to ForeignKey fields in migrations when the default reverse accessor (`<model>_set`) is acceptable. They are comfortable with Django's default naming convention.
Applied to files:
openwisp_firmware_upgrader/base/models.py
📚 Learning: 2026-02-25T00:17:23.479Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: openwisp_firmware_upgrader/static/firmware-upgrader/js/upgrade-progress.js:458-463
Timestamp: 2026-02-25T00:17:23.479Z
Learning: In the openwisp/openwisp-firmware-upgrader repository, focus review comments on real, actionable issues rather than theoretical edge cases, trivialities, or potential future problems. Avoid flagging issues that are functionally correct but could theoretically become problems only if the code were significantly refactored or used in different contexts.
Applied to files:
openwisp_firmware_upgrader/base/models.py
📚 Learning: 2026-02-23T21:44:41.470Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: openwisp_firmware_upgrader/websockets.py:233-246
Timestamp: 2026-02-23T21:44:41.470Z
Learning: In openwisp-firmware-upgrader, the `upgrade_operations` attribute on `BatchUpgradeOperation` is a cached property that returns a queryset, so it should be accessed as `batch_operation.upgrade_operations.all()` rather than being called as a method.
Applied to files:
openwisp_firmware_upgrader/base/models.py
🔇 Additional comments (1)
openwisp_firmware_upgrader/base/models.py (1)
64-75: Nice hardening around missing credentials.Catching
ObjectDoesNotExisthere turns the reported crash into a normal validation error, and the new message is correctly wrapped for translation.
efccfe0 to
872d992
Compare
Test Failures in Firmware UpgraderHello @Eeshu-Yadav, There is one test failure:
Fix: Review the test case |
872d992 to
c21ce16
Compare
|
I see the validation for UpgradeOperation also triggers this error. Can you propose a solution to mitigate this? As long as the error is solvable, e.g. the user can add back the credentials object it should be fine. |
Yes, the error is recoverable. I've added a fix: existing UpgradeOperation objects skip upgrade_options validation, so the user can add credentials back and save without being blocked by the old UpgradeOperation inline. |
pandafy
left a comment
There was a problem hiding this comment.
Great work @Eeshu-Yadav — I verified the initial fix, and the validation error no longer appears when saving a Device without a DeviceConnection, which is a good improvement.
However, the issue still reoccurs in a specific workflow when updating the firmware image on a device that no longer has an associated DeviceConnection.
Steps to reproduce:
- Create a Device with both
DeviceConnectionandDeviceFirmwareImage. - Delete the associated
DeviceConnection. - Update/change the Firmware Image on the Device.
- Observe the validation error:
This device does not have a related connection object defined yet and therefore it would not be possible to upgrade it, please add one in the section named "Credentials"
- Navigate to the Credentials section to add a new
DeviceConnection.
Problem:
At this point, the user is effectively blocked. Because the firmware image change is already staged in the form, they cannot add credentials without first saving. This forces a non-intuitive, multi-step workaround:
- Save the device with new credentials first
- Then go back and update the firmware image
Why this matters:
This creates a UX inconsistency and introduces friction in a fairly common workflow. Users are required to understand an implicit ordering constraint that isn’t communicated by the UI.
Can you add a test in test_admin.py which simulates submitting the complete admin form? The current tests do not cover the original scenario in which the bug appeared. You can take inspiration from test_using_upgrade_options_with_unsupported_upgrader on how to craft the payload for the device change form.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge pending maintainer approval The PR effectively fixes issue #250 by converting unhandled Changes Reviewed
Key Implementation Details
Files Reviewed (5 files)
Reviewed by kimi-k2.5-0127 · 192,903 tokens |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openwisp_firmware_upgrader/admin.py`:
- Around line 671-681: The helper _has_credentials_in_form currently treats any
submitted "...-credentials" value as active even when that inline is being
deleted; update _has_credentials_in_form to ignore credentials entries whose
corresponding inline delete flag is set (i.e. for keys like
"deviceconnection_set-<index>-credentials" skip if
"deviceconnection_set-<index>-DELETE" is truthy). Keep the logic in
_has_credentials_in_form and leave full_clean setting
instance._skip_connection_check unchanged, but ensure you only return True for
credentials on inlines that are not marked for deletion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5b8700d4-3dc5-443d-8e05-f46432e353c4
📒 Files selected for processing (3)
openwisp_firmware_upgrader/admin.pyopenwisp_firmware_upgrader/base/models.pyopenwisp_firmware_upgrader/tests/test_admin.py
📜 Review details
⏰ 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.13 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
openwisp_firmware_upgrader/admin.pyopenwisp_firmware_upgrader/tests/test_admin.pyopenwisp_firmware_upgrader/base/models.py
🧠 Learnings (7)
📓 Common learnings
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: openwisp_firmware_upgrader/static/firmware-upgrader/js/upgrade-progress.js:458-463
Timestamp: 2026-02-25T00:17:23.479Z
Learning: In the openwisp/openwisp-firmware-upgrader repository, focus review comments on real, actionable issues rather than theoretical edge cases, trivialities, or potential future problems. Avoid flagging issues that are functionally correct but could theoretically become problems only if the code were significantly refactored or used in different contexts.
📚 Learning: 2026-03-07T01:16:15.471Z
Learnt from: atif09
Repo: openwisp/openwisp-firmware-upgrader PR: 387
File: openwisp_firmware_upgrader/admin.py:420-423
Timestamp: 2026-03-07T01:16:15.471Z
Learning: In openwisp-firmware-upgrader, UpgradeOperation and BatchUpgradeOperation always share the same organization (BatchUpgradeOperation.build.category.organization). A user who can view an UpgradeOperation can always view its related BatchUpgradeOperation. For the UpgradeOperationAdmin.change_view, model-level has_view_permission(request) suffices to show breadcrumbs for batch, and object-level permission checks are not required.
Applied to files:
openwisp_firmware_upgrader/admin.py
📚 Learning: 2026-02-27T19:08:56.218Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 383
File: openwisp_firmware_upgrader/tests/test_admin.py:50-50
Timestamp: 2026-02-27T19:08:56.218Z
Learning: In tests for openwisp-firmware-upgrader, derive app_label values from model meta instead of hard-coding. Specifically, let BaseTestAdmin compute app_label from Build._meta.app_label and config_app_label from Device._meta.app_label to ensure tests remain correct if models are swapped. This pattern should apply to all test files under openwisp_firmware_upgrader/tests.
Applied to files:
openwisp_firmware_upgrader/tests/test_admin.py
📚 Learning: 2026-03-26T20:36:45.314Z
Learnt from: CR
Repo: openwisp/openwisp-firmware-upgrader PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-26T20:36:45.314Z
Learning: Update tests to cover non-trivial behavior changes
Applied to files:
openwisp_firmware_upgrader/tests/test_admin.py
📚 Learning: 2026-02-25T11:20:13.903Z
Learnt from: pandafy
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: runtests.py:27-38
Timestamp: 2026-02-25T11:20:13.903Z
Learning: In openwisp-firmware-upgrader, runtests.py now uses `execute_from_command_line()` to run Django tests in-process rather than via subprocess, so the `patch = subprocess` coverage directive is not needed in pyproject.toml.
Applied to files:
openwisp_firmware_upgrader/tests/test_admin.py
📚 Learning: 2026-02-23T21:44:41.470Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: openwisp_firmware_upgrader/websockets.py:233-246
Timestamp: 2026-02-23T21:44:41.470Z
Learning: In openwisp-firmware-upgrader, the `upgrade_operations` attribute on `BatchUpgradeOperation` is a cached property that returns a queryset, so it should be accessed as `batch_operation.upgrade_operations.all()` rather than being called as a method.
Applied to files:
openwisp_firmware_upgrader/base/models.py
📚 Learning: 2026-03-07T01:16:24.447Z
Learnt from: atif09
Repo: openwisp/openwisp-firmware-upgrader PR: 387
File: openwisp_firmware_upgrader/admin.py:420-423
Timestamp: 2026-03-07T01:16:24.447Z
Learning: In openwisp-firmware-upgrader, UpgradeOperation and BatchUpgradeOperation always share the same organization (BatchUpgradeOperation.build.category.organization). A user who can view an UpgradeOperation can always view its related BatchUpgradeOperation. Model-level has_view_permission(request) is sufficient for batch breadcrumb visibility in UpgradeOperationAdmin.change_view; object-level permission checks are not needed.
Applied to files:
openwisp_firmware_upgrader/base/models.py
c39eaf5 to
5b39b9f
Compare


Checklist
Reference to Existing Issue
Closes #250
Description of Changes
When device credentials are deleted after a firmware image is assigned, saving the device form crashes with HTTP 500 because
validate_upgrade_options()callsself.upgrader_classwhich raisesDoesNotExist(unhandled).This PR catches
ObjectDoesNotExistand raises aValidationErrorinstead, so the admin shows a proper error message.Also addsgetattrdefault to prevent potentialAttributeErroronSCHEMA.