[arthanson]

Closes: #23617

Five code paths updated an existing object without calling snapshot() first, so the update was recorded with no prechange data. In netbox-branching, merging these changes overwrote edits made in main, and reverting them failed.

• Cable.save(): snapshot after the initial insert, before the save that records the terminations
• Device.save(): snapshot each child device before copying the parent's site, location and rack
• update_connected_interfaces(): snapshot both interfaces before setting wireless_link
• update_mac_address_interface(): snapshot the MAC address before assigning it
• SyncedDataMixin.sync(save=True): snapshot before syncing

Adds a regression test for each. Once this merges, the four skip-list entries from #23616 can be removed. netbox-branching's test_revert_cable_create will then pass, so its @expectedFailure marker needs to come out.

[arthanson]

@claude /review Only report on issues of potential concern; we don't need commentary on anything that doesn't require further consideration.

[github-actions]

Claude finished @arthanson's task in 7m 59s —— View job

---

Code review

• Read the diff
• Review changed code paths with surrounding context
• Post review findings

One issue of real concern; the other four snapshot additions look correct.

---

🔴 sync(save=True) crashes on models that aren't change-logged

netbox/netbox/models/features.py:706

def sync(self, save=False):
    if save:
        self.snapshot()

snapshot() is defined only on ChangeLoggingMixin (features.py:93) — it's the only definition in the tree. But SyncedDataMixin is also mixed into a model that has no change logging:

# netbox/core/models/files.py:24
class ManagedFile(SyncedDataMixin, models.Model):
    ...
    _netbox_private = True

…and ScriptModule(PythonModuleMixin, JobsMixin, ManagedFile) is a proxy of it. Neither JobsMixin nor PythonModuleMixin supplies snapshot().

Failure scenario: create a script module from a data file with Auto sync enabled (ScriptFileForm → ManagedFileForm exposes auto_sync_enabled). SyncedDataMixin.save() writes an AutoSyncRecord. On the next DataSource sync, core.signals.auto_sync resolves that record and calls:

# netbox/core/signals.py:348
obj.sync(save=True)

→ AttributeError: 'ManagedFile' object has no attribute 'snapshot'. The receiver catches it, rolls back the savepoint, bumps failure_count and re-raises an aggregated error — so the script file is never re-synced and the data source sync job reports a failure.

The existing auto-sync tests only cover ConfigContext (change-logged), and no test calls ManagedFile.sync(save=True), so CI won't catch this. Note ManagedFile sets _netbox_private = True, so suppressing the snapshot there is the correct behavior, not just a workaround.

Suggested fix — mirror the guard already used for deletes at core/signals.py:208:

if save and hasattr(self, 'snapshot'):
    self.snapshot()

That also avoids silently clobbering a _prechange_snapshot a caller may already have taken (the delete handler guards on not getattr(instance, '_prechange_snapshot', None) for the same reason). Worth a regression test asserting ManagedFile.sync(save=True) doesn't raise.

Fix this →

---

Notes on the rest (no action needed)

• Cable.save() — snapshotting after the insert is correct: serialize_object() → get_terminations() reads CableTermination from the DB, so the prechange snapshot genuinely shows empty terminations before update_terminations() runs. Only one UPDATE ObjectChange is produced (CableTermination.save() touches the terminating object, not the cable), so self._updates(cable).get() in the new test is sound. One minor side effect: the snapshot is left on the instance after save() returns, so a script that saves the same cable instance a second time will reuse a stale prechange — consistent with how _prechange_snapshot behaves elsewhere in NetBox, and no in-tree caller does this.
• Device.save() — children are re-fetched from the DB, so the snapshot reflects committed state and precedes the site/rack/location assignment. Correct.
• update_connected_interfaces() / update_mac_address_interface() — snapshot precedes the mutation; the FK objects are loaded from the DB, and create_cablepaths() uses chunked_update() (no signals), so the tests' single-ObjectChange assumption holds.
• The added cable.snapshot() in test_termination_removal_records_disconnect doesn't change what that test asserts (snapshot only fires on insert) — it just makes the caller realistic.
  • branch 23617-snapshot

[arthanson]

Per Claude review:

1. Fixed: guarded with hasattr(self, 'snapshot') and added a ManagedFile regression test.
2. Not changed: callers always load the object fresh, and keeping a stale snapshot would log the wrong "before" state.

[jeremystretch]

The root issue here is that ChangeLoggingMixin can record an update with no pre-change data when snapshot() was not called. Rather than attempting to audit and resolve every potential instance of this (as more will be inevitably be introduced), can we tweak handle_changed_object to generate a pre-change snapshot from the database whenever it sees an existing object being saved without a pre-change snapshot? (It might also be prudent to log a warning.)

[arthanson]

Added the auto-snapshot with logging, added snapshot to several tests as they were generating the logging and the logging would get drowned out (ignored) if they weren't.