Skip to content

[fix] Corrected initial field value assignment in AbstractDevice class #1301

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
nemesifier merged 2 commits into from
Mar 27, 2026
+17 −3
Merged

[fix] Corrected initial field value assignment in AbstractDevice class #1301

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fd4f6b3
[fix] Correct handling of deferred fields when tracking changed fields
pandafy Mar 19, 2026
1a5fb89
[fix] Made test more readable
pandafy Mar 27, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions openwisp_controller/config/base/device.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -338,9 +338,9 @@ def _get_initial_values_for_checked_fields(self):
if not present_values:
return
self.refresh_from_db(fields=present_values.keys())
for field in self._changed_checked_fields:
setattr(self, f"_initial_{field}", field)
setattr(self, field, present_values[field])
for field, value in present_values.items():
setattr(self, f"_initial_{field}", getattr(self, field))
setattr(self, field, value)
Comment on lines 340 to +343
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Mar 27, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

This still captures the post-save value on the public update path.

save() does not reach this helper until after super().save() on Line 290 via _check_changed_fields() on Line 299. For a deferred field changed through device.save(), refresh_from_db() now reads the just-written value, so _initial_<field> becomes the new value and _check_*_changed() / _check_last_ip() no-op.

Please populate _initial_* before the write (and before _check_last_ip() on Line 292), otherwise deferred updates are still missed.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@openwisp_controller/config/base/device.py` around lines 340 - 343, The helper
currently sets _initial_<field> after the DB write (via refresh_from_db()),
causing deferred updates to appear as already-applied; move the logic that
populates _initial_<field> so it runs before the write and before
_check_last_ip()/_check_changed_fields(): call
self.refresh_from_db(fields=present_values.keys()) and then run the loop that
does setattr(self, f"_initial_{field}", getattr(self, field)) prior to invoking
super().save() (or before the call site that triggers _check_last_ip()/
_check_changed_fields()), ensuring _initial_* holds the pre-save values for
present_values.

Copy link
Copy Markdown
Member Author

@pandafy pandafy Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For a deferred field changed through device.save(), refresh_from_db() now reads the just-written value, so initial becomes the new value and check*_changed() / _check_last_ip() no-op.

If a deferred field is updated in the database, the the intial_<field> shall reflect the latest value from the database. We are not interested in the original value of the field.

@coderabbitai is my assumption correct?

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pandafy, yes, your assumption is correct. I need to retract my earlier concern.

Tracing the execution order in save():

  1. super().save() — writes the new value to the DB
  2. _check_last_ip() — for a deferred field, _initial_last_ip == models.DEFERRED, so it returns early (pre-existing guard)
  3. _check_changed_fields() → _get_initial_values_for_checked_fields() — this is where the fix applies

After step 3, _initial_<field> is set to the just-saved DB value. This is intentionally consistent with how the non-deferred path works: _check_last_ip() and _check_management_ip_changed() both end with self._initial_<field> = self.<field>, i.e., the post-save current value, which becomes the baseline for the next comparison.

The fix in this PR correctly addresses the bug (the field name string being stored instead of the actual value), and the behavior of _initial_<field> reflecting the latest DB value after a save is the right design.

My original comment was mistaken in treating the "missed signal for deferred field on first save" as a problem introduced here — that's governed by the pre-existing if self._initial_last_ip == models.DEFERRED: return guard and is separate from the bug this PR fixes.

🧠 Learnings used 
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/base/models.py:63-93
Timestamp: 2026-01-15T14:06:53.460Z
Learning: In openwisp_controller/geo/base/models.py, the BaseLocation.save() method protects the is_estimated field from manual changes when the estimated location feature is disabled by reverting it to its initial database value using the elif branch. This is intentional behavior to ensure the field can only be modified when the feature is properly configured.

Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.

Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.

Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.


def _check_name_changed(self):
if self._initial_name == models.DEFERRED:
Expand Down
14 changes: 14 additions & 0 deletions openwisp_controller/config/tests/test_device.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -541,6 +541,20 @@ def test_changed_checked_fields_no_duplicates(self):
device.__init__()
self.assertEqual(device._changed_checked_fields.count("last_ip"), 1)

def test_deferred_fields_populated_correctly(self):
device = self._create_device(
name="deferred-test",
management_ip="10.0.0.1",
)
# Load the instance with deferred fields omitted
device = Device.objects.only("id").get(pk=device.pk)
device.management_ip = "10.0.0.55"
# Saving the device object will populate the deferred fields
device.save()
# Ensure `_initial_<field>` contains the actual value, not the field name
self.assertEqual(getattr(device, "_initial_management_ip"), "10.0.0.55")
self.assertNotEqual(getattr(device, "_initial_management_ip"), "management_ip")

def test_exceed_organization_device_limit(self):
org = self._get_org()
org.config_limits.device_limit = 1
Expand Down
Loading