Skip to content
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.

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

Pre-Change-Data missing on cascaded Deletions #6389

Closed
moonrail opened this issue May 11, 2021 · 4 comments
Closed

Pre-Change-Data missing on cascaded Deletions #6389

moonrail opened this issue May 11, 2021 · 4 comments
Assignees
Labels
status: accepted This issue has been accepted for implementation type: bug A confirmed report of unexpected behavior in the application

Comments

@moonrail
Copy link

NetBox version

v2.11.3

Python version

3.8

Steps to Reproduce

  1. Create a Device/VM
  2. Create a Journal-Entry on this Device/VM
  3. Create two Interfaces on this Device/VM
  4. Create a IP on the first Interface
  5. Delete the first Interface
  6. Look into Changelog (or API /extras/object-changes) and find, that the IP-Address-Entry will not have any "Pre-Change"-Data
  7. Delete the Device/VM
  8. Look into Changelog (or API /extras/object-changes) and find, that the Interface- and Journal-Entries will not have any "Pre-Change"-Data

Expected Behavior

Pre-Change-Data should be in the Changelog.

This is the case, if e.g. an IP-Address is deleted directly, but not when its deleted by a cascade.

Observed Behavior

Pre-Change-Data is missing in the Changelog, when an object is deleted by a cascade.

@moonrail moonrail added the type: bug A confirmed report of unexpected behavior in the application label May 11, 2021
@larsux
Copy link

larsux commented May 11, 2021

Hi,

the same bug is triggered, when (de-)selecting an IP as primary for a device (Make this the primary IP for the device/VM).
In the corresponding ChangeLog-Object for the Device/VM only the "Post-Change Data" is filled and "Pre-Change Data" is empty.

It seems that there are some 'snapshot' function calls missing before saving the device object.

@jeremystretch jeremystretch added the status: needs owner This issue is tentatively accepted pending a volunteer committed to its implementation label May 11, 2021
@jweiher
Copy link

jweiher commented May 19, 2021

Hi,

to me the way and locations where the snapshot method of the ChangeLogginMixin is called seems a bit odd. I think the reason why it is not working on related models is simply because it gets called in the views when fetching a single object and it does not know anything about related objects.
In my opinion it would be a better solution to wire the calling of the snapshot method somewhere in the models so all of the changelogging happens under the hood and is not scattered around the codebase.
just my 2 cents

-- Jan

@jeremystretch
Copy link
Member

Opting to put the call to snapshot() in the view was a performance optimization: We only want spend time taking a snapshot if we anticipate making changes to the model. For instance, we wouldn't want to take a snapshot of each instance in a list if we're just viewing a list of objects with no intent to modify them.

That said, maybe it's an unnecessary optimization. We could try calling it inside from_db() for each instance to ensure that a pre-change snapshot is always present, though testing is needed to determine the performance impact.

@jeremystretch
Copy link
Member

A lot has changed since this bug was opened, but this seems solvable now by simply calling snapshot() in the pre_delete receiver.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status: accepted This issue has been accepted for implementation type: bug A confirmed report of unexpected behavior in the application
Projects
None yet
Development

No branches or pull requests

4 participants