Commit 962f7748 authored by Mehmet Emin INAC's avatar Mehmet Emin INAC

Return an instance of Feedback from Occurrence#_feedback methods

Currently Occurrence#_feedback methods return an instance of
BatchLoader which mimics almost all the behaviour of proxied object
but it's not perfect.
We can't use safe navigation operator on these return objects because
they are not actually `nil` since the Ruby VM just checks the object to
see if it's `NIL_P` before sending the message.
This causes getting NoMethodError on nil:NilClass which causes tears
on developers' eye.

Here we are explicitly returning an instance of Feedback model and also
loading all the feedback of all occurrences which solves the hidden
N+1 query issue.
parent 1aba995b
...@@ -75,5 +75,13 @@ module Vulnerabilities ...@@ -75,5 +75,13 @@ module Vulnerabilities
def has_comment? def has_comment?
comment.present? && comment_author.present? comment.present? && comment_author.present?
end end
def occurrence_key
{
project_id: project_id,
category: category,
project_fingerprint: project_fingerprint
}
end
end end
end end
...@@ -186,30 +186,26 @@ module Vulnerabilities ...@@ -186,30 +186,26 @@ module Vulnerabilities
end end
def feedback(feedback_type:) def feedback(feedback_type:)
params = { load_feedback.find { |f| f.feedback_type == feedback_type }
project_id: project_id, end
category: report_type,
project_fingerprint: project_fingerprint,
feedback_type: feedback_type
}
BatchLoader.for(params).batch do |items, loader| def load_feedback
project_ids = items.map { |i| i[:project_id] } BatchLoader.for(occurrence_key).batch do |occurrence_keys, loader|
categories = items.map { |i| i[:category] } project_ids = occurrence_keys.map { |key| key[:project_id] }
fingerprints = items.map { |i| i[:project_fingerprint] } categories = occurrence_keys.map { |key| key[:category] }
fingerprints = occurrence_keys.map { |key| key[:project_fingerprint] }
Vulnerabilities::Feedback.all_preloaded.where( feedback = Vulnerabilities::Feedback.all_preloaded.where(
project_id: project_ids.uniq, project_id: project_ids.uniq,
category: categories.uniq, category: categories.uniq,
project_fingerprint: fingerprints.uniq project_fingerprint: fingerprints.uniq
).each do |feedback| ).to_a
loaded_params = {
project_id: feedback.project_id, occurrence_keys.each do |occurrence_key|
category: feedback.category, loader.call(
project_fingerprint: feedback.project_fingerprint, occurrence_key,
feedback_type: feedback.feedback_type feedback.select { |f| occurrence_key = f.occurrence_key }
} )
loader.call(loaded_params, feedback)
end end
end end
end end
...@@ -296,5 +292,15 @@ module Vulnerabilities ...@@ -296,5 +292,15 @@ module Vulnerabilities
def first_fingerprint def first_fingerprint
identifiers.first&.fingerprint identifiers.first&.fingerprint
end end
private
def occurrence_key
{
project_id: project_id,
category: report_type,
project_fingerprint: project_fingerprint
}
end
end end
end end
...@@ -97,7 +97,7 @@ class Vulnerability < ApplicationRecord ...@@ -97,7 +97,7 @@ class Vulnerability < ApplicationRecord
end end
def dismissed_by_id def dismissed_by_id
super || finding&.dismissal_feedback.try(:author_id) super || finding&.dismissal_feedback&.author_id
end end
delegate :scanner_name, :metadata, :message, :cve, delegate :scanner_name, :metadata, :message, :cve,
......
...@@ -14,11 +14,7 @@ module Gitlab ...@@ -14,11 +14,7 @@ module Gitlab
end end
def self.preload_feedback!(findings) def self.preload_feedback!(findings)
findings.each do |finding| findings.each(&:load_feedback)
finding.dismissal_feedback
finding.issue_feedback
finding.merge_request_feedback
end
end end
end end
end end
......
...@@ -220,4 +220,16 @@ describe Vulnerabilities::Feedback do ...@@ -220,4 +220,16 @@ describe Vulnerabilities::Feedback do
end end
end end
end end
describe '#occurrence_key' do
let(:project_id) { 1 }
let(:category) { 'sast' }
let(:project_fingerprint) { Digest::SHA1.hexdigest('foo') }
let(:expected_occurrence_key) { { project_id: project_id, category: category, project_fingerprint: project_fingerprint } }
let(:feedback) { build(:vulnerability_feedback, expected_occurrence_key) }
subject { feedback.occurrence_key }
it { is_expected.to eq(expected_occurrence_key) }
end
end end
...@@ -495,6 +495,32 @@ describe Vulnerabilities::Occurrence do ...@@ -495,6 +495,32 @@ describe Vulnerabilities::Occurrence do
end end
end end
describe '#load_feedback' do
let_it_be(:project) { create(:project) }
let_it_be(:occurrence) do
create(
:vulnerabilities_occurrence,
report_type: :dependency_scanning,
project: project
)
end
let_it_be(:feedback) do
create(
:vulnerability_feedback,
:dependency_scanning,
:dismissal,
project: project,
project_fingerprint: occurrence.project_fingerprint
)
end
let(:expected_feedback) { [feedback] }
subject(:load_feedback) { occurrence.load_feedback.to_a }
it { is_expected.to eq(expected_feedback) }
end
describe '#state' do describe '#state' do
before do before do
create(:vulnerability, :dismissed, project: finding_with_issue.project, findings: [finding_with_issue]) create(:vulnerability, :dismissed, project: finding_with_issue.project, findings: [finding_with_issue])
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment