Commit bfff836f authored by David Fernandez's avatar David Fernandez Committed by James Lopez

Avoid an N+1 loop in GroupPackagesFinder

Replace `.select` with a proper ActiveRecord scope.
This way, #group_projects_visible_to_current_user are lazily loaded
instead of being all loaded in memory
parent b8bbf57d
......@@ -25,7 +25,7 @@ module Packages
.including_build_info
.including_project_route
.including_tags
.for_projects(group_projects_visible_to_current_user)
.for_projects(group_projects_visible_to_current_user.select(:id))
.processed
.has_version
.sort_by_attribute("#{params[:order_by]}_#{params[:sort]}")
......@@ -36,11 +36,14 @@ module Packages
end
def group_projects_visible_to_current_user
# according to project_policy.rb
# access to packages is ruled by:
# - project is public or the current user has access to it with at least the reporter level
# - the repository feature is available to the current_user
::Project
.in_namespace(groups)
.public_or_visible_to_user(current_user, Gitlab::Access::REPORTER)
.with_project_feature
.select { |project| Ability.allowed?(current_user, :read_package, project) }
.with_feature_available_for_user(:repository, current_user)
end
def package_type
......
---
title: Fix an N+1 issue in Packages::GroupPackagesFinder
merge_request: 45875
author:
type: fixed
......@@ -2,13 +2,16 @@
require 'spec_helper'
RSpec.describe Packages::GroupPackagesFinder do
let_it_be(:user) { create(:user) }
let_it_be(:group) { create(:group) }
let_it_be(:project) { create(:project, namespace: group) }
let(:another_group) { create(:group) }
using RSpec::Parameterized::TableSyntax
let_it_be(:user) { create(:user) }
let_it_be_with_reload(:group) { create(:group) }
let_it_be_with_reload(:project) { create(:project, namespace: group, builds_access_level: ProjectFeature::PRIVATE, merge_requests_access_level: ProjectFeature::PRIVATE) }
let(:add_user_to_group) { true }
before do
group.add_developer(user)
group.add_developer(user) if add_user_to_group
end
describe '#execute' do
......@@ -27,16 +30,16 @@ RSpec.describe Packages::GroupPackagesFinder do
end
context 'group has packages' do
let!(:package1) { create(:maven_package, project: project) }
let!(:package2) { create(:maven_package, project: project) }
let!(:package3) { create(:maven_package) }
let_it_be(:package1) { create(:maven_package, project: project) }
let_it_be(:package2) { create(:maven_package, project: project) }
let_it_be(:package3) { create(:maven_package) }
it { is_expected.to match_array([package1, package2]) }
context 'subgroup has packages' do
let(:subgroup) { create(:group, parent: group) }
let(:subproject) { create(:project, namespace: subgroup) }
let!(:package4) { create(:npm_package, project: subproject) }
let_it_be_with_reload(:subgroup) { create(:group, parent: group) }
let_it_be_with_reload(:subproject) { create(:project, namespace: subgroup, builds_access_level: ProjectFeature::PRIVATE, merge_requests_access_level: ProjectFeature::PRIVATE) }
let_it_be(:package4) { create(:npm_package, project: subproject) }
it { is_expected.to match_array([package1, package2, package4]) }
......@@ -45,16 +48,87 @@ RSpec.describe Packages::GroupPackagesFinder do
it { is_expected.to match_array([package1, package2]) }
end
context 'permissions' do
let(:add_user_to_group) { false }
where(:role, :project_visibility, :repository_visibility, :packages_returned) do
:anonymous | :public | :enabled | :all
:guest | :public | :enabled | :all
:reporter | :public | :enabled | :all
:developer | :public | :enabled | :all
:maintainer | :public | :enabled | :all
:anonymous | :public | :private | :none
:guest | :public | :private | :all
:reporter | :public | :private | :all
:developer | :public | :private | :all
:maintainer | :public | :private | :all
:anonymous | :private | :enabled | :none
:guest | :private | :enabled | :none
:reporter | :private | :enabled | :all
:developer | :private | :enabled | :all
:maintainer | :private | :enabled | :all
:anonymous | :private | :private | :none
:guest | :private | :private | :none
:reporter | :private | :private | :all
:developer | :private | :private | :all
:maintainer | :private | :private | :all
end
with_them do
let(:expected_packages) do
case packages_returned
when :all
[package1, package2, package4]
when :none
[]
end
end
before do
subgroup.update!(visibility: project_visibility.to_s)
group.update!(visibility: project_visibility.to_s)
project.update!(
visibility: project_visibility.to_s,
repository_access_level: repository_visibility.to_s
)
subproject.update!(
visibility: project_visibility.to_s,
repository_access_level: repository_visibility.to_s
)
unless role == :anonymous
project.add_user(user, role)
subproject.add_user(user, role)
end
end
it { is_expected.to match_array(expected_packages) }
end
end
context 'avoid N+1 query' do
it 'avoids N+1 database queries' do
count = ActiveRecord::QueryRecorder.new { subject }
.count
Packages::Package.package_types.keys.each do |package_type|
create("#{package_type}_package", project: create(:project, namespace: subgroup))
end
expect { described_class.new(user, group, params).execute }.not_to exceed_query_limit(count)
end
end
end
context 'when there are processing packages' do
let!(:package4) { create(:nuget_package, project: project, name: Packages::Nuget::CreatePackageService::TEMPORARY_PACKAGE_NAME) }
let_it_be(:package4) { create(:nuget_package, project: project, name: Packages::Nuget::CreatePackageService::TEMPORARY_PACKAGE_NAME) }
it { is_expected.to match_array([package1, package2]) }
end
context 'does not include packages without version number' do
let!(:package_without_version) { create(:maven_package, project: project, version: nil) }
let_it_be(:package_without_version) { create(:maven_package, project: project, version: nil) }
it { is_expected.not_to include(package_without_version) }
end
......@@ -80,7 +154,7 @@ RSpec.describe Packages::GroupPackagesFinder do
end
context 'group has package of all types' do
package_types.each { |pt| let!("package_#{pt}") { create("#{pt}_package", project: project) } }
package_types.each { |pt| let_it_be("package_#{pt}") { create("#{pt}_package", project: project) } }
package_types.each do |package_type|
it_behaves_like 'with package type', package_type
......@@ -98,7 +172,7 @@ RSpec.describe Packages::GroupPackagesFinder do
end
context 'package type is nil' do
let!(:package1) { create(:maven_package, project: project) }
let_it_be(:package1) { create(:maven_package, project: project) }
subject { described_class.new(user, group, package_type: nil).execute }
......@@ -110,47 +184,5 @@ RSpec.describe Packages::GroupPackagesFinder do
it { expect { subject }.to raise_exception(described_class::InvalidPackageTypeError) }
end
context 'when project is public' do
let_it_be(:other_user) { create(:user) }
let(:finder) { described_class.new(other_user, group) }
before do
project.update!(visibility_level: ProjectFeature::ENABLED)
end
context 'when packages are public' do
before do
project.project_feature.update!(
builds_access_level: ProjectFeature::PRIVATE,
merge_requests_access_level: ProjectFeature::PRIVATE,
repository_access_level: ProjectFeature::ENABLED)
end
it 'returns group packages' do
package1 = create(:maven_package, project: project)
package2 = create(:maven_package, project: project)
create(:maven_package)
expect(finder.execute).to match_array([package1, package2])
end
end
context 'packages are members only' do
before do
project.project_feature.update!(
builds_access_level: ProjectFeature::PRIVATE,
merge_requests_access_level: ProjectFeature::PRIVATE,
repository_access_level: ProjectFeature::PRIVATE)
create(:maven_package, project: project)
create(:maven_package)
end
it 'filters out the project if the user doesn\'t have permission' do
expect(finder.execute).to be_empty
end
end
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