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
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
1 change: 1 addition & 0 deletions CHANGELOG
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
[v#.#.#] ([month] [YYYY])
- Kit Import: Use file name sequencing when a template file with the same name exists
- Node merging: Update associated evidence, notes and child nodes updated_at column on node merge
- Upgraded gems:
- rexml
- Bugs fixes:
Expand Down
104 changes: 56 additions & 48 deletions app/services/nodes/merger.rb
Original file line number Diff line number Diff line change
Expand Up @@ -31,66 +31,74 @@ def call

private

attr_accessor :target_node, :source_node, :copied_attachments

DESCENDENT_RELATIONSHIPS = {
activities: :trackable_id,
children: :parent_id,
evidence: :node_id,
notes: :node_id
}.freeze

def move_descendents
DESCENDENT_RELATIONSHIPS.each do |relation, column|
attr_accessor :target_node, :source_node, :copied_attachments

DESCENDENT_RELATIONSHIPS = {
activities: :trackable_id,
children: :parent_id,
evidence: :node_id,
notes: :node_id
}.freeze

def move_descendents
DESCENDENT_RELATIONSHIPS.each do |relation, column|
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
Comment on lines +45 to 53
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

end
end

def reset_counter_caches
Node.reset_counters target_node.id, :children_count
end
def reset_counter_caches
Node.reset_counters target_node.id, :children_count
end

def update_properties
source_node.properties.each do |key, value|
case key.to_sym
when :services
value.each do |service|
data = service.merge(source: :merge)
target_node.set_service data
end
when :services_extras
value.each do |protocol_port, extras|
protocol, port = protocol_port.split('/')

extras.each do |extra|
data = {
extra[:id] => extra[:output],
source: extra[:source],
port: port.to_i,
protocol: protocol
}

def update_properties
source_node.properties.each do |key, value|
case key.to_sym
when :services
value.each do |service|
data = service.merge(source: :merge)
target_node.set_service data
end
when :services_extras
value.each do |protocol_port, extras|
protocol, port = protocol_port.split('/')

extras.each do |extra|
data = {
extra[:id] => extra[:output],
source: extra[:source],
port: port.to_i,
protocol: protocol
}

target_node.set_service data
end
end
else
target_node.set_property key, value
end
else
target_node.set_property key, value
end

target_node.save
end

def copy_attachments
self.copied_attachments = []
target_node.save
end

source_node.attachments.each do |attachment|
copied_attachments << attachment.copy_to(target_node)
end
end
def copy_attachments
self.copied_attachments = []

def undo_attachments_copy
return unless copied_attachments&.any?
copied_attachments.each(&:delete)
source_node.attachments.each do |attachment|
copied_attachments << attachment.copy_to(target_node)
end
end

def undo_attachments_copy
return unless copied_attachments&.any?
copied_attachments.each(&:delete)
end
end
4 changes: 2 additions & 2 deletions app/views/issues/_table.html.erb
Original file line number Diff line number Diff line change
@@ -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 %>
<% table_attributes = {
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

behavior: 'dradis-datatable',
'default-columns': @default_columns.to_json,
Expand All @@ -21,7 +21,7 @@
</thead>
<tbody>
<% issues.each do |issue| %>
<% cache [issue, @all_columns, issue.try(:affected_count), issue.try(:state), 'issues-table', @tags] do %>
<% cache [issue, @all_columns, issue.try(:affected_count), issue.affected_ids, issue.try(:state), 'issues-table', @tags] do %>
<tr id="issue-<%= issue.id %>" data-tag-url="<%= project_issue_path(current_project, issue) %>">
<td class="select-checkbox" data-behavior="select-checkbox"></td>
<% @all_columns.each do |column| %>
Expand Down
Loading