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

Touch evidence, children and notes on node merge & add affected_ids to issues#index table cache #1290

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

caitmich
Copy link
Contributor

@caitmich caitmich commented Sep 3, 2024

Summary

After merging nodes,

  • The issues#index table cache doesn't update because we are not including :affected in the cache keys. This leads to broken nodes#show links in the affected column
  • The issues/evidence#index table cache doesn't bust because the evidence updated_at does not get updated on node merge since we are using update_all. This leads to broken #edit links (since they point to the old node)

This PR adds :affected_ids to the issues#index table cache key and adds updated_at: Time.current to the list of columns to update in merger.rb's move_descendents method

I assign all rights, including copyright, to any future Dradis
work by myself to Security Roots.

Check List

  • Added a CHANGELOG entry

Comment on lines +45 to 53
if relation == :activities
source_node.send(relation).update_all(column => target_node.id)
else
# update_all doesn't update timestamps so we need to do it manually
source_node.send(relation).update_all(
column => target_node.id,
:updated_at => Time.current
)
end
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Main changes. The rest is spacing updates

Copy link
Contributor

Choose a reason for hiding this comment

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

What about just using update instead of updated_all? update would update the updated_at attribute as part of the method call

@@ -1,4 +1,4 @@
<% cache ['issues-table', @all_columns, issues.map(&:id), @issues.map(&:updated_at).map(&:to_i).sort.last, @tags] do %>
<% cache ['issues-table', @all_columns, @issues.map(&:affected_ids), issues.map(&:id), @issues.map(&:updated_at).map(&:to_i).sort.last, @tags] do %>
Copy link
Contributor

Choose a reason for hiding this comment

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

Should issues be touched when nodes are updated so we don't have to add this to the cache key?

Copy link
Contributor Author

@caitmich caitmich Oct 7, 2024

Choose a reason for hiding this comment

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

We already have touch: true in the belongs_to association in the evidence model, and since issues and nodes are connected by evidence, this should suffice in most cases. The reason this is needed is specific to the merge action, which uses update_all so no callbacks are run

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In order to fix this we could call update instead like you mentioned above, but I am concerned about performance implications

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants