From 3792d8095a7c28f9ea861c9639e9d08e65208350 Mon Sep 17 00:00:00 2001 From: Dzmitry Meshcharakou <12459192-dmeshcharakou@users.noreply.gitlab.com> Date: Tue, 16 Sep 2025 12:58:43 +0200 Subject: [PATCH] Read entries from packages_composer_packages table In REST API and GraphQL read composer packages from the detached `packages_composer_packages` table. --- app/finders/packages/group_packages_finder.rb | 1 + app/finders/packages/package_finder.rb | 13 +++++- app/finders/packages/packages_finder.rb | 22 +++++++++- app/models/packages/package.rb | 34 ++++++++++---- app/models/packages/unified/package.rb | 30 +++++++++++++ .../packages_unified_packages.yml | 10 +++++ db/docs/packages_packages.yml | 1 + lib/api/entities/package.rb | 6 ++- lib/api/project_packages.rb | 2 + spec/finders/packages/package_finder_spec.rb | 26 ++++++++++- spec/finders/packages/packages_finder_spec.rb | 36 +++++++++------ spec/models/packages/package_spec.rb | 44 ++++++++++++++++++- spec/models/packages/unified/package_spec.rb | 18 ++++++++ .../finders/packages_shared_examples.rb | 6 +-- 14 files changed, 216 insertions(+), 33 deletions(-) create mode 100644 app/models/packages/unified/package.rb create mode 100644 config/feature_flags/gitlab_com_derisk/packages_unified_packages.yml create mode 100644 spec/models/packages/unified/package_spec.rb diff --git a/app/finders/packages/group_packages_finder.rb b/app/finders/packages/group_packages_finder.rb index 9ce175e7bee73a..479057e745c277 100644 --- a/app/finders/packages/group_packages_finder.rb +++ b/app/finders/packages/group_packages_finder.rb @@ -89,6 +89,7 @@ def preload_pipelines params.fetch(:preload_pipelines, true) end + # TODO: use unified packages def packages_class if group return ::Packages::Package unless package_type diff --git a/app/finders/packages/package_finder.rb b/app/finders/packages/package_finder.rb index 0b1e700a6fd588..bb587562e2345a 100644 --- a/app/finders/packages/package_finder.rb +++ b/app/finders/packages/package_finder.rb @@ -8,13 +8,22 @@ def initialize(project, package_id) end def execute - @project - .packages + base .preload_pipelines .including_project_namespace_route .including_tags .displayable .find(@package_id) end + + private + + def base + if Feature.enabled?(:packages_unified_packages, Feature.current_request) + ::Packages::Unified::Package.all_packages.for_projects(@project) + else + @project.packages + end + end end end diff --git a/app/finders/packages/packages_finder.rb b/app/finders/packages/packages_finder.rb index 58408e053d5cac..c7fa17f65dd617 100644 --- a/app/finders/packages/packages_finder.rb +++ b/app/finders/packages/packages_finder.rb @@ -3,6 +3,7 @@ module Packages class PackagesFinder include ::Packages::FinderHelper + include Gitlab::Utils::StrongMemoize def initialize(project, params = {}) @project = project @@ -37,13 +38,30 @@ def preload_pipelines end def base - return ::Packages::Package.for_projects(project).without_package_type(:terraform_module) unless package_type + return packages_without_terraform_modules unless package_type - package_class = ::Packages::Package.inheritance_column_to_class_map[package_type.to_sym] + package_class = if unified_packages_enabled? + ::Packages::Package.package_type_to_class_map[package_type.to_sym] + else + ::Packages::Package.inheritance_column_to_class_map[package_type.to_sym] + end raise ArgumentError, "'#{package_type}' is not a valid package_type" unless package_class package_class.constantize.for_projects(project) end + + def packages_without_terraform_modules + if unified_packages_enabled? + ::Packages::Unified::Package.all_packages.for_projects(project).without_package_type(:terraform_module) + else + ::Packages::Package.for_projects(project).without_package_type(:terraform_module) + end + end + + def unified_packages_enabled? + Feature.enabled?(:packages_unified_packages, Feature.current_request) + end + strong_memoize_attr :unified_packages_enabled? end end diff --git a/app/models/packages/package.rb b/app/models/packages/package.rb index 18ada0453bd54d..030311e7c2884e 100644 --- a/app/models/packages/package.rb +++ b/app/models/packages/package.rb @@ -152,6 +152,12 @@ def self.inheritance_column_to_class_map }.freeze end + def self.package_type_to_class_map + inheritance_column_to_class_map.merge( + composer: 'Packages::Composer::Package' + ).freeze + end + def self.by_name_and_file_name(name, file_name) with_name(name) .joins(:package_files) @@ -198,15 +204,21 @@ def self.installable_statuses end def versions - self.class.inheritance_column_to_class_map[package_type.to_sym] - .constantize - .for_projects(project) - .preload_pipelines - .including_tags - .displayable - .with_name(name) - .where.not(version: version) - .order(:version) + package_class = if Feature.enabled?(:packages_unified_packages, Feature.current_request) + self.class.package_type_to_class_map[package_type.to_sym] + else + self.class.inheritance_column_to_class_map[package_type.to_sym] + end + + package_class + .constantize + .for_projects(project) + .preload_pipelines + .including_tags + .displayable + .with_name(name) + .where.not(version: version) + .order(:version) end # Technical debt: to be removed in https://gitlab.com/gitlab-org/gitlab/-/issues/281937 @@ -257,6 +269,10 @@ def publish_creation_event def detailed_info? DETAILED_INFO_STATUSES.include?(status.to_sym) end + + def to_unified_package + becomes(::Packages::Unified::Package) + end end end diff --git a/app/models/packages/unified/package.rb b/app/models/packages/unified/package.rb new file mode 100644 index 00000000000000..7c2aa4ceb91daa --- /dev/null +++ b/app/models/packages/unified/package.rb @@ -0,0 +1,30 @@ +# frozen_string_literal: true + +module Packages + module Unified + class Package < ::Packages::Package + include FromUnion + + self.inheritance_column = nil # rubocop:disable Database/AvoidInheritanceColumn -- suppress single table inheritance + + def self.all_packages + # TODO: shall I use cached_column_list from ApplicationRecord? + common_columns = column_names.without('package_type') + + from_union( + [ + ::Packages::Package + .without_package_type(:composer) + .select(*common_columns, 'package_type'), + ::Packages::Composer::Package + .select(*common_columns, "#{package_types[:composer]} AS package_type") + ] + ) + end + + def to_typed_package + becomes(self.class.package_type_to_class_map[package_type.to_sym].constantize) + end + end + end +end diff --git a/config/feature_flags/gitlab_com_derisk/packages_unified_packages.yml b/config/feature_flags/gitlab_com_derisk/packages_unified_packages.yml new file mode 100644 index 00000000000000..7ee7fcd1a10079 --- /dev/null +++ b/config/feature_flags/gitlab_com_derisk/packages_unified_packages.yml @@ -0,0 +1,10 @@ +--- +name: packages_unified_packages +description: +feature_issue_url: +introduced_by_url: +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/577827 +milestone: '18.6' +group: group::package registry +type: gitlab_com_derisk +default_enabled: false diff --git a/db/docs/packages_packages.yml b/db/docs/packages_packages.yml index 2d46eea8a27225..4043f3fb410e2c 100644 --- a/db/docs/packages_packages.yml +++ b/db/docs/packages_packages.yml @@ -17,6 +17,7 @@ classes: - Packages::Rpm::Package - Packages::Rubygems::Package - Packages::TerraformModule::Package +- Packages::Unified::Package feature_categories: - package_registry description: Information for individual packages in the package registry diff --git a/lib/api/entities/package.rb b/lib/api/entities/package.rb index bfde770a83177c..a851c68ebf4f7d 100644 --- a/lib/api/entities/package.rb +++ b/lib/api/entities/package.rb @@ -13,7 +13,11 @@ class Package < Grape::Entity expose :name, documentation: { type: 'string', example: '@foo/bar' } do |package| if package.conan? - package.conan_recipe + if Feature.enabled?(:packages_unified_packages, Feature.current_request) + package.to_typed_package.conan_recipe + else + package.conan_recipe + end else package.name end diff --git a/lib/api/project_packages.rb b/lib/api/project_packages.rb index 36d0bbb0bc4b1d..1b9506f034e2e0 100644 --- a/lib/api/project_packages.rb +++ b/lib/api/project_packages.rb @@ -150,6 +150,8 @@ def package forbidden!('Package is deletion protected.') if service_response[:protection_rule_exists?] end + package = package.to_typed_package if Feature.enabled?(:packages_unified_packages, Feature.current_request) + destroy_conditionally!(package) do |package| ::Packages::MarkPackageForDestructionService.new(container: package, current_user: current_user).execute end diff --git a/spec/finders/packages/package_finder_spec.rb b/spec/finders/packages/package_finder_spec.rb index 1b0c88a4771496..dd135c777276fb 100644 --- a/spec/finders/packages/package_finder_spec.rb +++ b/spec/finders/packages/package_finder_spec.rb @@ -11,7 +11,15 @@ subject { described_class.new(project, package_id).execute } - it { is_expected.to eq(maven_package) } + it { is_expected.to eq(maven_package.to_unified_package) } + + context 'when packages_unified_packages is disabled' do + before do + stub_feature_flags(packages_unified_packages: false) + end + + it { is_expected.to eq(maven_package) } + end context 'with non-displayable package' do before do @@ -32,5 +40,21 @@ expect { subject }.to raise_exception(ActiveRecord::RecordNotFound) end end + + context 'when composer package' do + let_it_be(:composer_package) { create(:composer_package_sti, project: project) } + + let(:package_id) { composer_package.id } + + it { is_expected.to eq(composer_package.to_unified_package) } + + context 'when packages_unified_packages is disabled' do + before do + stub_feature_flags(packages_unified_packages: false) + end + + it { is_expected.to eq(composer_package) } + end + end end end diff --git a/spec/finders/packages/packages_finder_spec.rb b/spec/finders/packages/packages_finder_spec.rb index eb5ba18ef3d60c..5211b9861c0cef 100644 --- a/spec/finders/packages/packages_finder_spec.rb +++ b/spec/finders/packages/packages_finder_spec.rb @@ -6,6 +6,8 @@ let_it_be(:project) { create(:project) } let_it_be(:maven_package) { create(:maven_package, project: project, created_at: 2.days.ago, name: 'maven', version: '2.0.0') } let_it_be(:conan_package) { create(:conan_package, project: project, created_at: 1.day.ago, name: 'conan', version: '1.0.0') } + let_it_be(:maven_package_unified) { maven_package.to_unified_package } + let_it_be(:conan_package_unified) { conan_package.to_unified_package } describe '#execute' do let(:params) { {} } @@ -20,6 +22,14 @@ let(:params) { { package_type: 'conan' } } it { is_expected.to eq([conan_package]) } + + context 'when packages_unified_packages is disabled' do + before do + stub_feature_flags(packages_composer_read_from_detached_table: false) + end + + it { is_expected.to eq([conan_package]) } + end end context 'npm packages' do @@ -40,68 +50,68 @@ context 'with order_by' do context 'by default is created_at' do - it { is_expected.to eq([maven_package, conan_package]) } + it { is_expected.to eq([maven_package_unified, conan_package_unified]) } end context 'order by name' do let(:params) { { order_by: 'name' } } - it { is_expected.to eq([conan_package, maven_package]) } + it { is_expected.to eq([conan_package_unified, maven_package_unified]) } end context 'order by version' do let(:params) { { order_by: 'version' } } - it { is_expected.to eq([conan_package, maven_package]) } + it { is_expected.to eq([conan_package_unified, maven_package_unified]) } end context 'order by type' do let(:params) { { order_by: 'type' } } - it { is_expected.to eq([maven_package, conan_package]) } + it { is_expected.to eq([maven_package_unified, conan_package_unified]) } end end context 'with sort' do context 'by default is ascending' do - it { is_expected.to eq([maven_package, conan_package]) } + it { is_expected.to eq([maven_package_unified, conan_package_unified]) } end context 'can sort descended' do let(:params) { { sort: 'desc' } } - it { is_expected.to eq([conan_package, maven_package]) } + it { is_expected.to eq([conan_package_unified, maven_package_unified]) } end end context 'with package_name' do let(:params) { { package_name: 'maven' } } - it { is_expected.to eq([maven_package]) } + it { is_expected.to eq([maven_package_unified]) } end context 'with nil params' do - it { is_expected.to match_array([conan_package, maven_package]) } + it { is_expected.to match_array([conan_package_unified, maven_package_unified]) } end context 'with processing packages' do let_it_be(:nuget_package) { create(:nuget_package, :processing, project: project) } - it { is_expected.to match_array([conan_package, maven_package]) } + it { is_expected.to match_array([conan_package_unified, maven_package_unified]) } end context 'preload_pipelines' do it 'preloads pipelines by default' do - expect(Packages::Package).to receive(:preload_pipelines).and_call_original - expect(subject).to match_array([maven_package, conan_package]) + expect(Packages::Unified::Package).to receive(:preload_pipelines).and_call_original + expect(subject).to match_array([maven_package_unified, conan_package_unified]) end context 'set to false' do let(:params) { { preload_pipelines: false } } it 'does not preload pipelines' do - expect(Packages::Package).not_to receive(:preload_pipelines) - expect(subject).to match_array([maven_package, conan_package]) + expect(Packages::Unified::Package).not_to receive(:preload_pipelines) + expect(subject).to match_array([maven_package_unified, conan_package_unified]) end end end diff --git a/spec/models/packages/package_spec.rb b/spec/models/packages/package_spec.rb index b97f7184204e2f..092c2ac5c7fe36 100644 --- a/spec/models/packages/package_spec.rb +++ b/spec/models/packages/package_spec.rb @@ -480,8 +480,14 @@ expect(package.versions).to contain_exactly(package2, package3) end - it 'does not return different packages' do - expect(package.versions).not_to include(package4) + context 'when packages_unified_packages is disabled' do + before do + stub_feature_flags(packages_unified_packages: false) + end + + it 'returns other package versions of the same package name belonging to the project' do + expect(package.versions).to contain_exactly(package2, package3) + end end end @@ -760,4 +766,38 @@ it { is_expected.to eq(result) } end end + + describe '.package_type_to_class_map' do + let(:result) do + { + ml_model: 'Packages::MlModel::Package', + golang: 'Packages::Go::Package', + rubygems: 'Packages::Rubygems::Package', + conan: 'Packages::Conan::Package', + rpm: 'Packages::Rpm::Package', + debian: 'Packages::Debian::Package', + composer: 'Packages::Composer::Package', + helm: 'Packages::Helm::Package', + generic: 'Packages::Generic::Package', + pypi: 'Packages::Pypi::Package', + terraform_module: 'Packages::TerraformModule::Package', + nuget: 'Packages::Nuget::Package', + npm: 'Packages::Npm::Package', + maven: 'Packages::Maven::Package', + cargo: 'Packages::Cargo::Package' + } + end + + subject { described_class.package_type_to_class_map } + + it { is_expected.to eq(result) } + end + + describe '#to_unified' do + let(:package) { build(:generic_package) } + + subject { package.to_unified } + + it { is_expected.to be_a(::Packages::Unified::Package) } + end end diff --git a/spec/models/packages/unified/package_spec.rb b/spec/models/packages/unified/package_spec.rb new file mode 100644 index 00000000000000..a2d4992ddeb032 --- /dev/null +++ b/spec/models/packages/unified/package_spec.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Packages::Unified::Package, feature_category: :package_registry do + it { is_expected.to be_a FromUnion } + + describe '.all_packages' do + let_it_be(:npm_package) { create(:npm_package) } + let_it_be(:composer_package) { create(:composer_package_sti) } + + subject(:result) { described_class.all_packages } + + it 'returns a union of packages from different database tables' do + expect(result).to contain_exactly(npm_package.to_unified, composer_package.to_unified) + end + end +end diff --git a/spec/support/shared_examples/finders/packages_shared_examples.rb b/spec/support/shared_examples/finders/packages_shared_examples.rb index b3ec2336ccaa31..4c5d430d76139c 100644 --- a/spec/support/shared_examples/finders/packages_shared_examples.rb +++ b/spec/support/shared_examples/finders/packages_shared_examples.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true RSpec.shared_examples 'concerning versionless param' do - let_it_be(:versionless_package) { create(:maven_package, project: project, version: nil) } + let_it_be(:versionless_package) { create(:maven_package, project: project, version: nil).to_unified_package } it { is_expected.not_to include(versionless_package) } @@ -19,8 +19,8 @@ end RSpec.shared_examples 'concerning package statuses' do - let_it_be(:hidden_package) { create(:maven_package, :hidden, project: project) } - let_it_be(:error_package) { create(:maven_package, :error, project: project) } + let_it_be(:hidden_package) { create(:maven_package, :hidden, project: project).to_unified_package } + let_it_be(:error_package) { create(:maven_package, :error, project: project).to_unified_package } context 'displayable packages' do it { is_expected.not_to include(hidden_package) } -- GitLab