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

Unnecessary changelog records for tag add/change/remove #5583

Closed
netsandbox opened this issue Jan 6, 2021 · 5 comments
Closed

Unnecessary changelog records for tag add/change/remove #5583

netsandbox opened this issue Jan 6, 2021 · 5 comments
Assignees
Labels
status: accepted This issue has been accepted for implementation type: bug A confirmed report of unexpected behavior in the application

Comments

@netsandbox
Copy link
Contributor

Environment

  • Python version: 3.6.9
  • NetBox version: 2.10.3

Steps to Reproduce

  1. create two tags TagA and TagB
  2. create a virtual-machine (or device)
  3. add TagA to the virtual-machine
  4. remove TagA and add TagB to the virtual-machine (in one request)
  5. remove TagB from the virtual-machine

Expected Behavior

Steps 3., 4. and 5. create only one changelog record

Observed Behavior

Step 3. creates two changelog records (with the same request id), where only one record shows the tag add and one shows no change.

Step 4. creates three changelog records (with the same request id), where one record shows the TagA remove, one the TagB add, and one shows no change.

Step 5. creates two changelog records (with the same request id), where only one record shows the tag remove and one shows no change.

Here are the database extras_objectchange records for the above steps 2.-5.:
https://gist.github.com/cloos/e7dd4c3b1f064098e4250e7cddb29c85

@jeremystretch
Copy link
Member

Multiple change records are created because tags (a many-to-many relationship) are added and removed using separate queries. I agree that it's not optimal but the necessary data is still captured. Attempting to consolidate these changes would likely take quite a bit of effort. If you'd like to volunteer for this work, I'll assign it to you. Otherwise, I don't think there's much to be done.

@netsandbox
Copy link
Contributor Author

I think it is ok for Step 4. to have two change records, one for tag remove and one for tag add.

But the additional change records in Step 3., 4., and 5. which shows no changes should not be created.
It's a little bit annoying when you try to inspect the object changes and always have change records which shows no object changes. Also if you frequently change tags, this unnecessarily increases the extras_objectchange table.

Sadly my knowledge of the NetBox code is not good enough to fix this myself in a reasonable time.

patrickfnielsen added a commit to patrickfnielsen/netbox that referenced this issue Feb 18, 2021
@patrickfnielsen
Copy link

I agree with @netsandbox, there's no reason to create a log entry if there's no changes.
The best solution I could find with my limited knowledge of the system, is to check if there's any diff between the objects before creating a ObjectChanges - This has the drawback of requiring an extra db call.

@jeremystretch Is this something that you would accept a PR for? In my limited testing it seems to fix the problem, but I'm not sure if theres any other drawbacks of doing it this way.

@jeremystretch jeremystretch added status: under review Further discussion is needed to determine this issue's scope and/or implementation type: bug A confirmed report of unexpected behavior in the application labels Apr 13, 2021
@jeremystretch
Copy link
Member

jeremystretch commented Apr 13, 2021

With the improvements made to change logging in v2.11 (see #5913), we should be able to update the first ObjectChange instance on m2m_changed instead of creating a second instance. I'm going to dig into this a bit more.

@jeremystretch jeremystretch added status: accepted This issue has been accepted for implementation and removed status: under review Further discussion is needed to determine this issue's scope and/or implementation labels Apr 13, 2021
@jeremystretch jeremystretch self-assigned this Apr 13, 2021
@jeremystretch
Copy link
Member

This will be fixed in the upcoming v2.11 release.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 13, 2021
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

3 participants