From aad1b85da4e13e357782bfa1036b1a6c0a53cbba Mon Sep 17 00:00:00 2001 From: David Fernandez <dfernandez@gitlab.com> Date: Thu, 4 Nov 2021 14:38:57 +0100 Subject: [PATCH] Remove the packages_remove_cross_joins_to_pipelines feature flag Removes cross join queries between package tables and the ci_pipelines table. Changelog: other --- app/finders/packages/group_packages_finder.rb | 2 +- app/finders/packages/package_finder.rb | 2 +- app/finders/packages/packages_finder.rb | 2 +- app/models/packages/package.rb | 10 +--- app/models/packages/package_file.rb | 2 +- ...ckages_remove_cross_joins_to_pipelines.yml | 8 --- lib/api/package_files.rb | 2 +- .../packages/group_packages_finder_spec.rb | 24 --------- spec/finders/packages/package_finder_spec.rb | 24 --------- spec/finders/packages/packages_finder_spec.rb | 25 --------- spec/models/packages/package_spec.rb | 51 ------------------- spec/requests/api/package_files_spec.rb | 28 ---------- .../package_details_shared_examples.rb | 19 ------- 13 files changed, 7 insertions(+), 192 deletions(-) delete mode 100644 config/feature_flags/development/packages_remove_cross_joins_to_pipelines.yml diff --git a/app/finders/packages/group_packages_finder.rb b/app/finders/packages/group_packages_finder.rb index 093deb43c41..2a62dd5c0e5 100644 --- a/app/finders/packages/group_packages_finder.rb +++ b/app/finders/packages/group_packages_finder.rb @@ -22,7 +22,7 @@ module Packages def packages_for_group_projects(installable_only: false) packages = ::Packages::Package - .load_pipelines + .preload_pipelines .including_project_route .including_tags .for_projects(group_projects_visible_to_current_user.select(:id)) diff --git a/app/finders/packages/package_finder.rb b/app/finders/packages/package_finder.rb index f4463bf0726..e482a0503f0 100644 --- a/app/finders/packages/package_finder.rb +++ b/app/finders/packages/package_finder.rb @@ -9,7 +9,7 @@ module Packages def execute @project .packages - .load_pipelines + .preload_pipelines .including_project_route .including_tags .displayable diff --git a/app/finders/packages/packages_finder.rb b/app/finders/packages/packages_finder.rb index 29ec3645633..3bc348c8dc8 100644 --- a/app/finders/packages/packages_finder.rb +++ b/app/finders/packages/packages_finder.rb @@ -14,7 +14,7 @@ module Packages def execute packages = project.packages - .load_pipelines + .preload_pipelines .including_project_route .including_tags diff --git a/app/models/packages/package.rb b/app/models/packages/package.rb index ff9e7ef4cf5..dbcda3da2fa 100644 --- a/app/models/packages/package.rb +++ b/app/models/packages/package.rb @@ -40,7 +40,7 @@ class Packages::Package < ApplicationRecord has_one :composer_metadatum, inverse_of: :package, class_name: 'Packages::Composer::Metadatum' has_one :rubygems_metadatum, inverse_of: :package, class_name: 'Packages::Rubygems::Metadatum' has_many :build_infos, inverse_of: :package - has_many :pipelines, through: :build_infos, disable_joins: -> { disable_cross_joins_to_pipelines? } + has_many :pipelines, through: :build_infos, disable_joins: true has_one :debian_publication, inverse_of: :package, class_name: 'Packages::Debian::Publication' has_one :debian_distribution, through: :debian_publication, source: :distribution, inverse_of: :packages, class_name: 'Packages::Debian::ProjectDistribution' @@ -102,7 +102,6 @@ class Packages::Package < ApplicationRecord scope :with_status, ->(status) { where(status: status) } scope :displayable, -> { with_status(DISPLAYABLE_STATUSES) } scope :installable, -> { with_status(INSTALLABLE_STATUSES) } - scope :including_build_info, -> { includes(pipelines: :user) } scope :including_project_route, -> { includes(project: { namespace: :route }) } scope :including_tags, -> { includes(:tags) } scope :including_dependency_links, -> { includes(dependency_links: :dependency) } @@ -135,7 +134,6 @@ class Packages::Package < ApplicationRecord scope :last_of_each_version, -> { where(id: all.select('MAX(id) AS id').group(:version)) } scope :limit_recent, ->(limit) { order_created_desc.limit(limit) } scope :select_distinct_name, -> { select(:name).distinct } - scope :load_pipelines, -> { disable_cross_joins_to_pipelines? ? preload_pipelines : including_build_info } # Sorting scope :order_created, -> { reorder(created_at: :asc) } @@ -162,10 +160,6 @@ class Packages::Package < ApplicationRecord joins(:project).reorder(keyset_order) end - def self.disable_cross_joins_to_pipelines? - ::Feature.enabled?(:packages_remove_cross_joins_to_pipelines, default_enabled: :yaml) - end - def self.only_maven_packages_with_path(path, use_cte: false) if use_cte # This is an optimization fence which assumes that looking up the Metadatum record by path (globally) @@ -251,7 +245,7 @@ class Packages::Package < ApplicationRecord def versions project.packages - .load_pipelines + .preload_pipelines .including_tags .with_name(name) .where.not(version: version) diff --git a/app/models/packages/package_file.rb b/app/models/packages/package_file.rb index 554c5e64f7c..87c9f56cc41 100644 --- a/app/models/packages/package_file.rb +++ b/app/models/packages/package_file.rb @@ -15,7 +15,7 @@ class Packages::PackageFile < ApplicationRecord has_one :conan_file_metadatum, inverse_of: :package_file, class_name: 'Packages::Conan::FileMetadatum' has_many :package_file_build_infos, inverse_of: :package_file, class_name: 'Packages::PackageFileBuildInfo' - has_many :pipelines, through: :package_file_build_infos, disable_joins: -> { Packages::Package.disable_cross_joins_to_pipelines? } + has_many :pipelines, through: :package_file_build_infos, disable_joins: true has_one :debian_file_metadatum, inverse_of: :package_file, class_name: 'Packages::Debian::FileMetadatum' has_one :helm_file_metadatum, inverse_of: :package_file, class_name: 'Packages::Helm::FileMetadatum' diff --git a/config/feature_flags/development/packages_remove_cross_joins_to_pipelines.yml b/config/feature_flags/development/packages_remove_cross_joins_to_pipelines.yml deleted file mode 100644 index 5f3b4490cf7..00000000000 --- a/config/feature_flags/development/packages_remove_cross_joins_to_pipelines.yml +++ /dev/null @@ -1,8 +0,0 @@ ---- -name: packages_remove_cross_joins_to_pipelines -introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/70624 -rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/342921 -milestone: '14.5' -type: development -group: group::package -default_enabled: false diff --git a/lib/api/package_files.rb b/lib/api/package_files.rb index a4d298d94e8..79ebf18ff27 100644 --- a/lib/api/package_files.rb +++ b/lib/api/package_files.rb @@ -29,7 +29,7 @@ module API .new(user_project, params[:package_id]).execute files = package.package_files - files = files.preload_pipelines if Packages::Package.disable_cross_joins_to_pipelines? + .preload_pipelines present paginate(files), with: ::API::Entities::PackageFile end diff --git a/spec/finders/packages/group_packages_finder_spec.rb b/spec/finders/packages/group_packages_finder_spec.rb index 7da1bfa58aa..3254c436674 100644 --- a/spec/finders/packages/group_packages_finder_spec.rb +++ b/spec/finders/packages/group_packages_finder_spec.rb @@ -160,30 +160,6 @@ RSpec.describe Packages::GroupPackagesFinder do end end - context 'with pipelines' do - let_it_be(:build_info_1) { create(:package_build_info, :with_pipeline, package: package1) } - let_it_be(:build_info_2) { create(:package_build_info, :with_pipeline, package: package2) } - - it 'preloads the pipelines' do - expect(::Packages::Package).to receive(:preload_pipelines).and_call_original - expect(::Packages::Package).not_to receive(:including_build_info) - - expect(subject).to match_array([package1, package2]) - end - - context 'with packages_remove_cross_joins_to_pipelines disabled' do - before do - stub_feature_flags(packages_remove_cross_joins_to_pipelines: false) - end - - it 'includes the pipelines' do - expect(::Packages::Package).to receive(:including_build_info).and_call_original - expect(::Packages::Package).not_to receive(:preload_pipelines) - expect(subject).to match_array([package1, package2]) - end - end - end - it_behaves_like 'concerning versionless param' it_behaves_like 'concerning package statuses' end diff --git a/spec/finders/packages/package_finder_spec.rb b/spec/finders/packages/package_finder_spec.rb index dfa1a65b322..1b0c88a4771 100644 --- a/spec/finders/packages/package_finder_spec.rb +++ b/spec/finders/packages/package_finder_spec.rb @@ -32,29 +32,5 @@ RSpec.describe ::Packages::PackageFinder do expect { subject }.to raise_exception(ActiveRecord::RecordNotFound) end end - - context 'with pipelines' do - let_it_be(:build_info) { create(:package_build_info, :with_pipeline, package: maven_package) } - - it 'preloads the pipelines' do - expect(::Packages::Package).to receive(:preload_pipelines).and_call_original - expect(::Packages::Package).not_to receive(:including_build_info) - - expect(subject).to eq(maven_package) - end - - context 'with packages_remove_cross_joins_to_pipelines disabled' do - before do - stub_feature_flags(packages_remove_cross_joins_to_pipelines: false) - end - - it 'includes the pipelines' do - expect(::Packages::Package).to receive(:including_build_info).and_call_original - expect(::Packages::Package).not_to receive(:preload_pipelines) - - expect(subject).to eq(maven_package) - end - end - end end end diff --git a/spec/finders/packages/packages_finder_spec.rb b/spec/finders/packages/packages_finder_spec.rb index 8d67c96e815..b72f4aab3ec 100644 --- a/spec/finders/packages/packages_finder_spec.rb +++ b/spec/finders/packages/packages_finder_spec.rb @@ -81,31 +81,6 @@ RSpec.describe ::Packages::PackagesFinder do it { is_expected.to match_array([conan_package, maven_package]) } end - context 'with pipelines' do - let_it_be(:build_info1) { create(:package_build_info, :with_pipeline, package: conan_package) } - let_it_be(:build_info2) { create(:package_build_info, :with_pipeline, package: maven_package) } - - it 'preloads the pipelines' do - expect(::Packages::Package).to receive(:preload_pipelines).and_call_original - expect(::Packages::Package).not_to receive(:including_build_info) - - expect(subject).to match_array([conan_package, maven_package]) - end - - context 'with packages_remove_cross_joins_to_pipelines disabled' do - before do - stub_feature_flags(packages_remove_cross_joins_to_pipelines: false) - end - - it 'includes the pipelines' do - expect(::Packages::Package).to receive(:including_build_info).and_call_original - expect(::Packages::Package).not_to receive(:preload_pipelines) - - expect(subject).to match_array([conan_package, maven_package]) - end - end - end - it_behaves_like 'concerning versionless param' it_behaves_like 'concerning package statuses' end diff --git a/spec/models/packages/package_spec.rb b/spec/models/packages/package_spec.rb index 12a2a2a94a6..09dc8a3a304 100644 --- a/spec/models/packages/package_spec.rb +++ b/spec/models/packages/package_spec.rb @@ -961,33 +961,6 @@ RSpec.describe Packages::Package, type: :model do end end - describe '.load_pipelines' do - let_it_be(:package) { create(:maven_package) } - let_it_be(:build_info) { create(:package_build_info, :with_pipeline, package: package) } - - subject { described_class.load_pipelines } - - it 'uses preload', :aggregate_failures do - expect(described_class).to receive(:preload_pipelines).and_call_original - expect(described_class).not_to receive(:including_build_info) - - subject - end - - context 'with packages_remove_cross_joins_to_pipelines disabled' do - before do - stub_feature_flags(packages_remove_cross_joins_to_pipelines: false) - end - - it 'uses includes', :aggregate_failures do - expect(described_class).to receive(:including_build_info).and_call_original - expect(described_class).not_to receive(:preload_pipelines) - - subject - end - end - end - describe '#versions' do let_it_be(:project) { create(:project) } let_it_be(:package) { create(:maven_package, project: project) } @@ -1001,30 +974,6 @@ RSpec.describe Packages::Package, type: :model do it 'does not return different packages' do expect(package.versions).not_to include(package3) end - - context 'with pipelines' do - let_it_be(:build_info) { create(:package_build_info, :with_pipeline, package: package2) } - - it 'preloads the pipelines' do - expect(::Packages::Package).to receive(:preload_pipelines).and_call_original - expect(::Packages::Package).not_to receive(:including_build_info) - - expect(package.versions).to contain_exactly(package2) - end - - context 'with packages_remove_cross_joins_to_pipelines disabled' do - before do - stub_feature_flags(packages_remove_cross_joins_to_pipelines: false) - end - - it 'includes the pipelines' do - expect(::Packages::Package).to receive(:including_build_info).and_call_original - expect(::Packages::Package).not_to receive(:preload_pipelines) - - expect(package.versions).to contain_exactly(package2) - end - end - end end describe '#pipeline' do diff --git a/spec/requests/api/package_files_spec.rb b/spec/requests/api/package_files_spec.rb index c170c151057..eb1f04d193e 100644 --- a/spec/requests/api/package_files_spec.rb +++ b/spec/requests/api/package_files_spec.rb @@ -27,34 +27,6 @@ RSpec.describe API::PackageFiles do expect(response).to have_gitlab_http_status(:not_found) end - - context 'with pipelines' do - let(:package_file_build_info) { create(:package_file_build_info, :with_pipeline, package_file: package.package_files.first) } - - it 'preloads the pipelines' do - expect(::Packages::PackageFile).to receive(:preload_pipelines).and_call_original - - get api(url) - - expect(response).to have_gitlab_http_status(:ok) - end - - context 'with packages_remove_cross_joins_to_pipelines disabled' do - before do - stub_feature_flags(packages_remove_cross_joins_to_pipelines: false) - end - - it 'does not preload the pipelines' do - expect(::Packages::PackageFile).not_to receive(:preload_pipelines) - - ::Gitlab::Database.allow_cross_joins_across_databases(url: 'https://gitlab.com/gitlab-org/gitlab/-/issues/342921') do - get api(url) - end - - expect(response).to have_gitlab_http_status(:ok) - end - end - end end context 'project is private' do diff --git a/spec/support/shared_examples/requests/api/graphql/packages/package_details_shared_examples.rb b/spec/support/shared_examples/requests/api/graphql/packages/package_details_shared_examples.rb index 2faeb1a534b..d576a5874fd 100644 --- a/spec/support/shared_examples/requests/api/graphql/packages/package_details_shared_examples.rb +++ b/spec/support/shared_examples/requests/api/graphql/packages/package_details_shared_examples.rb @@ -13,25 +13,6 @@ RSpec.shared_examples 'a package detail' do it_behaves_like 'a working graphql query' do it_behaves_like 'matching the package details schema' end - - context 'with packages_remove_cross_joins_to_pipelines disabled' do - # subject is called in a before callback that is executed before the one below - # the callback below MUST be executed before the subject - # solution: nullify subject and manually call #post_graphql - subject {} - - before do - stub_feature_flags(packages_remove_cross_joins_to_pipelines: false) - - ::Gitlab::Database.allow_cross_joins_across_databases(url: 'https://gitlab.com/gitlab-org/gitlab/-/issues/342921') do - post_graphql(query, current_user: user) - end - end - - it_behaves_like 'a working graphql query' do - it_behaves_like 'matching the package details schema' - end - end end end -- 2.30.9