Commit 17d45dee authored by Michał Zając's avatar Michał Zając

Fix N+1 query in VulnerabilitiesResolver

Use LooksAhead module to preload finding unconditionally and preload
remediations when has_solutions is requested.

Changelog: performance
EE: true
parent 6665b71e
......@@ -45,6 +45,7 @@ module EE
field :vulnerabilities,
::Types::VulnerabilityType.connection_type,
null: true,
extras: [:lookahead],
description: 'Vulnerabilities reported on the projects in the group and its subgroups.',
resolver: ::Resolvers::VulnerabilitiesResolver
......
......@@ -13,6 +13,7 @@ module EE
field :vulnerabilities,
::Types::VulnerabilityType.connection_type,
null: true,
extras: [:lookahead],
description: 'Vulnerabilities reported on the project.',
resolver: ::Resolvers::VulnerabilitiesResolver
......
......@@ -17,6 +17,7 @@ module EE
field :vulnerabilities,
::Types::VulnerabilityType.connection_type,
null: true,
extras: [:lookahead],
description: "Vulnerabilities reported on projects on the current user's instance security dashboard.",
resolver: ::Resolvers::VulnerabilitiesResolver
......
......@@ -3,6 +3,7 @@
module Resolvers
class VulnerabilitiesResolver < VulnerabilitiesBaseResolver
include Gitlab::Utils::StrongMemoize
include LooksAhead
type Types::VulnerabilityType, null: true
......@@ -49,7 +50,7 @@ module Resolvers
"the response only matches entries for a `reportType` "\
"that includes #{::Vulnerabilities::Finding::REPORT_TYPES_WITH_LOCATION_IMAGE.map { |type| "`#{type}`" }.join(', ')}."
def resolve(**args)
def resolve_with_lookahead(**args)
return Vulnerability.none unless vulnerable
args[:scanner_id] = resolve_gids(args[:scanner_id], ::Vulnerabilities::Scanner) if args[:scanner_id]
......@@ -59,10 +60,20 @@ module Resolvers
.with_created_issue_links_and_issues
end
def unconditional_includes
[:findings]
end
def preloads
{
has_solutions: [{ findings: [:remediations] }]
}
end
private
def vulnerabilities(params)
Security::VulnerabilitiesFinder.new(vulnerable, params).execute
apply_lookahead(Security::VulnerabilitiesFinder.new(vulnerable, params).execute)
end
end
end
......@@ -5,7 +5,7 @@ require 'spec_helper'
RSpec.describe GitlabSchema.types['Vulnerability'] do
let_it_be(:project) { create(:project) }
let_it_be(:user) { create(:user) }
let_it_be(:vulnerability) { create(:vulnerability, project: project) }
let_it_be(:vulnerability) { create(:vulnerability, :with_remediation, project: project) }
let_it_be(:fields) do
%i[userPermissions
id
......@@ -95,17 +95,37 @@ RSpec.describe GitlabSchema.types['Vulnerability'] do
context 'N+1 queries' do
it 'avoids N+1 database queries' do
pending('See: https://gitlab.com/gitlab-org/gitlab/-/issues/292993')
# Execute the query once so we don't count selecting Projects and Namespaces
GitlabSchema.execute(query, context: { current_user: user })
# Count queries for the baseline which is a single Vulnerability with a remediation
# Should be 10
control_count = ActiveRecord::QueryRecorder.new { GitlabSchema.execute(query, context: { current_user: user }) }.count
expect(control_count).to eq(10)
create(:vulnerability, :with_remediation, project: project)
create(:vulnerability, :with_remediation, project: project)
create(:vulnerability, :with_remediation, project: project)
expect { GitlabSchema.execute(query, context: { current_user: user }) }.not_to exceed_query_limit(control_count)
# Every additional Vulnerability seems to add TWO more database calls similar to
# SELECT
# MAX("project_authorizations"."access_level") AS maximum_access_level,
# "project_authorizations"."user_id" AS project_authorizations_user_id
# FROM "project_authorizations"
# WHERE
# "project_authorizations"."project_id" = 315
# AND
# "project_authorizations"."user_id" = 409
# GROUP BY
# "project_authorizations"."user_id"
#
# I have no idea where do they come from or if we could batch them
# so we have to increase the control_count by 2 * number of Vulnerabilities
expect { GitlabSchema.execute(query, context: { current_user: user }) }.not_to exceed_query_limit(control_count + (3 * 2))
result = GitlabSchema.execute(query, context: { current_user: user }).to_h
vulnerability = result.dig('data', 'project', 'vulnerabilities', 'nodes').first
expect(vulnerability['hasSolution']).to be_truthy
expect(vulnerability['hasSolutions']).to be_truthy
end
end
end
......
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