From 3d816b02a16196990a1ea9fdbc3feaa30bc2b243 Mon Sep 17 00:00:00 2001 From: Sam Word Date: Tue, 14 Oct 2025 19:56:06 -0400 Subject: [PATCH 1/3] Added Import::Offline::Export model and associations Introduced Import::Offline::Export, its table import_offline_exports and added optional belong_to :offline_export association to to BulkImports::Exports. Changelog: changed --- app/models/bulk_imports/export.rb | 5 + app/models/import/offline/export.rb | 50 +++++ .../import/offline/exports/create_service.rb | 91 +++++++++ .../wip/offline_transfer_exports.yml | 9 + db/docs/import_offline_exports.yml | 12 ++ ...010195318_create_import_offline_exports.rb | 17 ++ ...r_foreign_key_to_import_offline_exports.rb | 16 ++ ...n_foreign_key_to_import_offline_exports.rb | 16 ++ ...port_foreign_key_to_bulk_import_exports.rb | 20 ++ db/schema_migrations/20251010195318 | 1 + db/schema_migrations/20251014192903 | 1 + db/schema_migrations/20251014193254 | 1 + db/schema_migrations/20251014202556 | 1 + db/structure.sql | 42 ++++ lib/import/offline/error.rb | 29 +++ locale/gitlab.pot | 12 ++ spec/factories/bulk_import/exports.rb | 4 + spec/factories/import/offline/exports.rb | 26 +++ spec/models/bulk_imports/export_spec.rb | 19 ++ spec/models/import/offline/export_spec.rb | 31 +++ .../offline/exports/create_service_spec.rb | 186 ++++++++++++++++++ 21 files changed, 589 insertions(+) create mode 100644 app/models/import/offline/export.rb create mode 100644 app/services/import/offline/exports/create_service.rb create mode 100644 config/feature_flags/wip/offline_transfer_exports.yml create mode 100644 db/docs/import_offline_exports.yml create mode 100644 db/migrate/20251010195318_create_import_offline_exports.rb create mode 100644 db/migrate/20251014192903_add_user_foreign_key_to_import_offline_exports.rb create mode 100644 db/migrate/20251014193254_add_organization_foreign_key_to_import_offline_exports.rb create mode 100644 db/migrate/20251014202556_add_offline_export_foreign_key_to_bulk_import_exports.rb create mode 100644 db/schema_migrations/20251010195318 create mode 100644 db/schema_migrations/20251014192903 create mode 100644 db/schema_migrations/20251014193254 create mode 100644 db/schema_migrations/20251014202556 create mode 100644 lib/import/offline/error.rb create mode 100644 spec/factories/import/offline/exports.rb create mode 100644 spec/models/import/offline/export_spec.rb create mode 100644 spec/services/import/offline/exports/create_service_spec.rb diff --git a/app/models/bulk_imports/export.rb b/app/models/bulk_imports/export.rb index 5a9975695876a3..7edd068c8f633c 100644 --- a/app/models/bulk_imports/export.rb +++ b/app/models/bulk_imports/export.rb @@ -13,6 +13,7 @@ class Export < ApplicationRecord belongs_to :project, optional: true belongs_to :group, optional: true belongs_to :user, optional: true + belongs_to :offline_export, optional: true, class_name: 'Import::Offline::Export' has_one :upload, class_name: 'BulkImports::ExportUpload' has_many :batches, class_name: 'BulkImports::ExportBatch' @@ -83,5 +84,9 @@ def remove_existing_upload! def relation_has_user_contributions? config.relation_has_user_contributions?(relation) end + + def offline? + offline_export.present? + end end end diff --git a/app/models/import/offline/export.rb b/app/models/import/offline/export.rb new file mode 100644 index 00000000000000..0eee8632b24e14 --- /dev/null +++ b/app/models/import/offline/export.rb @@ -0,0 +1,50 @@ +# frozen_string_literal: true + +module Import + module Offline + class Export < ApplicationRecord + self.table_name = 'import_offline_exports' + + KNOWN_IMPORT_HOSTS = %w[github.com bitbucket.org gitea.com].freeze + + belongs_to :user + belongs_to :organization, class_name: 'Organizations::Organization' + + has_many :bulk_import_exports, class_name: 'BulkImports::Export', inverse_of: :offline_export + + validates :source_hostname, :status, presence: true + validate :validate_source_hostname + + state_machine :status, initial: :created do + state :created, value: 0 + state :started, value: 1 + state :finished, value: 2 + state :failed, value: -1 + + event :start do + transition created: :started + end + + event :finish do + transition started: :finished + end + + event :fail_op do + transition any => :failed + end + end + + def validate_source_hostname + uri = Gitlab::Utils.parse_url(source_hostname) + + if KNOWN_IMPORT_HOSTS.include?(uri&.host) + return errors.add(:source_hostname, :invalid, message: 'must not be the host of a known import source') + end + + return if uri && uri.scheme && uri.host && uri.path.blank? && uri.query.blank? + + errors.add(:source_hostname, :invalid, message: 'must contain scheme and host, and not path or query') + end + end + end +end diff --git a/app/services/import/offline/exports/create_service.rb b/app/services/import/offline/exports/create_service.rb new file mode 100644 index 00000000000000..0aa58ae67065f6 --- /dev/null +++ b/app/services/import/offline/exports/create_service.rb @@ -0,0 +1,91 @@ +# frozen_string_literal: true + +module Import + module Offline + module Exports + class CreateService + include ::Gitlab::Utils::StrongMemoize + + ServiceError = Class.new(StandardError) + + def initialize(current_user, source_hostname, portable_params) + @current_user = current_user + @source_hostname = source_hostname + @portable_params = portable_params + end + + def execute + unless Feature.enabled?(:offline_transfer_exports, current_user) + raise Import::Offline::Error.feature_flag_not_enabled + end + + validate_user_permissions! + validate_portables_exist! + + offline_export = Import::Offline::Export.create!( + user: current_user, + organization_id: current_user.organization_id, + source_hostname: source_hostname + ) + + ServiceResponse.success(payload: offline_export) + rescue ActiveRecord::RecordInvalid, Import::Offline::Error => e + ServiceResponse.error( + message: e.message, + reason: :unprocessable_entity + ) + end + + private + + attr_reader :current_user, :source_hostname, :portable_params + + def validate_portables_exist! + missing_params = portable_params.any? { |params| params[:type].blank? || params[:full_path].blank? } + + raise Import::Offline::Error.invalid_portable_params if missing_params + + missing_group_paths = full_paths_for_type(:group) - groups.map(&:full_path) + missing_project_paths = full_paths_for_type(:project) - projects.map(&:full_path) + + return if missing_group_paths.blank? && missing_project_paths.blank? + + raise Import::Offline::Error.export_paths_not_found([missing_group_paths, missing_project_paths].flatten) + end + + def validate_user_permissions! + portables = [groups, projects].flatten + invalid_paths = portables.filter_map do |portable| + portable.full_path unless user_can_admin_portable?(portable) + end + + return unless invalid_paths.present? + + raise Import::Offline::Error.invalid_export_permissions(invalid_paths) + end + + def user_can_admin_portable?(portable) + ability = "admin_#{portable.to_ability_name}" + + current_user.can?(ability, portable) + end + + def groups + Group.where_full_path_in(full_paths_for_type(:group)) + end + strong_memoize_attr :groups + + def projects + Project.where_full_path_in(full_paths_for_type(:project)) + end + strong_memoize_attr :projects + + def full_paths_for_type(type) + portable_params.filter_map do |params| + params[:full_path] if params[:type] == type.to_s + end.uniq + end + end + end + end +end diff --git a/config/feature_flags/wip/offline_transfer_exports.yml b/config/feature_flags/wip/offline_transfer_exports.yml new file mode 100644 index 00000000000000..a42ba68a4b3caa --- /dev/null +++ b/config/feature_flags/wip/offline_transfer_exports.yml @@ -0,0 +1,9 @@ +--- +name: offline_transfer_exports +feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/538941 +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/209344 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/577715 +milestone: '18.6' +group: group::import +type: wip +default_enabled: false diff --git a/db/docs/import_offline_exports.yml b/db/docs/import_offline_exports.yml new file mode 100644 index 00000000000000..288b9ebcc79b76 --- /dev/null +++ b/db/docs/import_offline_exports.yml @@ -0,0 +1,12 @@ +--- +table_name: import_offline_exports +classes: +- Import::Offline::Export +feature_categories: +- importers +description: Used to define which exported relations belong to a single export +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/209344 +milestone: '18.6' +gitlab_schema: gitlab_main_org +sharding_key: + organization_id: organizations diff --git a/db/migrate/20251010195318_create_import_offline_exports.rb b/db/migrate/20251010195318_create_import_offline_exports.rb new file mode 100644 index 00000000000000..52997c1e68c71f --- /dev/null +++ b/db/migrate/20251010195318_create_import_offline_exports.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +class CreateImportOfflineExports < Gitlab::Database::Migration[2.3] + milestone '18.6' + + def change + create_table :import_offline_exports do |t| + t.bigint :user_id, index: true, null: false + t.bigint :organization_id, index: true, null: false + t.integer :status, null: false, limit: 2, default: 0 + t.text :source_hostname, null: false, limit: 255 + t.boolean :has_failures, default: false, null: false + + t.timestamps_with_timezone null: false + end + end +end diff --git a/db/migrate/20251014192903_add_user_foreign_key_to_import_offline_exports.rb b/db/migrate/20251014192903_add_user_foreign_key_to_import_offline_exports.rb new file mode 100644 index 00000000000000..dec7a56fa4e9cb --- /dev/null +++ b/db/migrate/20251014192903_add_user_foreign_key_to_import_offline_exports.rb @@ -0,0 +1,16 @@ +# frozen_string_literal: true + +class AddUserForeignKeyToImportOfflineExports < Gitlab::Database::Migration[2.3] + disable_ddl_transaction! + milestone '18.6' + + def up + add_concurrent_foreign_key :import_offline_exports, :users, column: :user_id, on_delete: :cascade + end + + def down + with_lock_retries do + remove_foreign_key :import_offline_exports, column: :user_id + end + end +end diff --git a/db/migrate/20251014193254_add_organization_foreign_key_to_import_offline_exports.rb b/db/migrate/20251014193254_add_organization_foreign_key_to_import_offline_exports.rb new file mode 100644 index 00000000000000..295357dace5ced --- /dev/null +++ b/db/migrate/20251014193254_add_organization_foreign_key_to_import_offline_exports.rb @@ -0,0 +1,16 @@ +# frozen_string_literal: true + +class AddOrganizationForeignKeyToImportOfflineExports < Gitlab::Database::Migration[2.3] + disable_ddl_transaction! + milestone '18.6' + + def up + add_concurrent_foreign_key :import_offline_exports, :organizations, column: :organization_id, on_delete: :cascade + end + + def down + with_lock_retries do + remove_foreign_key :import_offline_exports, column: :organization_id + end + end +end diff --git a/db/migrate/20251014202556_add_offline_export_foreign_key_to_bulk_import_exports.rb b/db/migrate/20251014202556_add_offline_export_foreign_key_to_bulk_import_exports.rb new file mode 100644 index 00000000000000..9028bb3d635a81 --- /dev/null +++ b/db/migrate/20251014202556_add_offline_export_foreign_key_to_bulk_import_exports.rb @@ -0,0 +1,20 @@ +# frozen_string_literal: true + +class AddOfflineExportForeignKeyToBulkImportExports < Gitlab::Database::Migration[2.3] + disable_ddl_transaction! + milestone '18.6' + + TABLE_NAME = :bulk_import_exports + COLUMN_NAME = :offline_export_id + + def up + add_column TABLE_NAME, COLUMN_NAME, :bigint, if_not_exists: true + + add_concurrent_index TABLE_NAME, COLUMN_NAME, name: "index_#{TABLE_NAME}_on_#{COLUMN_NAME}" + add_concurrent_foreign_key TABLE_NAME, :import_offline_exports, column: COLUMN_NAME, on_delete: :cascade + end + + def down + remove_column TABLE_NAME, COLUMN_NAME, if_exists: true + end +end diff --git a/db/schema_migrations/20251010195318 b/db/schema_migrations/20251010195318 new file mode 100644 index 00000000000000..f909e54c3adbb0 --- /dev/null +++ b/db/schema_migrations/20251010195318 @@ -0,0 +1 @@ +8839e0bbb498dd99342916381ea33c90e6833fd484eff67d7faa507d3281ba2e \ No newline at end of file diff --git a/db/schema_migrations/20251014192903 b/db/schema_migrations/20251014192903 new file mode 100644 index 00000000000000..ecd2e7b6eed00d --- /dev/null +++ b/db/schema_migrations/20251014192903 @@ -0,0 +1 @@ +76f45fbfaae548e904b00b9a5bb6a70571b2fc099db445a5727c78f9ec0568d7 \ No newline at end of file diff --git a/db/schema_migrations/20251014193254 b/db/schema_migrations/20251014193254 new file mode 100644 index 00000000000000..e04df450ce1a25 --- /dev/null +++ b/db/schema_migrations/20251014193254 @@ -0,0 +1 @@ +60170e63657be5ca7049f674cfb4cf457541070792682039c75c141b8ffa0747 \ No newline at end of file diff --git a/db/schema_migrations/20251014202556 b/db/schema_migrations/20251014202556 new file mode 100644 index 00000000000000..3f2b2d6e9a7dda --- /dev/null +++ b/db/schema_migrations/20251014202556 @@ -0,0 +1 @@ +20ac7e742c7e63fc8c40f30886dfba0e0cdce793077a91defb70515a585cfffd \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index ed81bde46a9ffa..c6eb56cea4c0be 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -13082,6 +13082,7 @@ CREATE TABLE bulk_import_exports ( batches_count integer DEFAULT 0 NOT NULL, total_objects_count integer DEFAULT 0 NOT NULL, user_id bigint, + offline_export_id bigint, CONSTRAINT check_24cb010672 CHECK ((char_length(relation) <= 255)), CONSTRAINT check_8f0f357334 CHECK ((char_length(error) <= 255)), CONSTRAINT check_9ee6d14d33 CHECK ((char_length(jid) <= 255)), @@ -17806,6 +17807,27 @@ CREATE SEQUENCE import_failures_id_seq ALTER SEQUENCE import_failures_id_seq OWNED BY import_failures.id; +CREATE TABLE import_offline_exports ( + id bigint NOT NULL, + user_id bigint NOT NULL, + organization_id bigint NOT NULL, + status smallint DEFAULT 0 NOT NULL, + source_hostname text NOT NULL, + has_failures boolean DEFAULT false NOT NULL, + created_at timestamp with time zone NOT NULL, + updated_at timestamp with time zone NOT NULL, + CONSTRAINT check_dcd47fbc18 CHECK ((char_length(source_hostname) <= 255)) +); + +CREATE SEQUENCE import_offline_exports_id_seq + START WITH 1 + INCREMENT BY 1 + NO MINVALUE + NO MAXVALUE + CACHE 1; + +ALTER SEQUENCE import_offline_exports_id_seq OWNED BY import_offline_exports.id; + CREATE TABLE import_placeholder_memberships ( id bigint NOT NULL, source_user_id bigint NOT NULL, @@ -30987,6 +31009,8 @@ ALTER TABLE ONLY import_export_uploads ALTER COLUMN id SET DEFAULT nextval('impo ALTER TABLE ONLY import_failures ALTER COLUMN id SET DEFAULT nextval('import_failures_id_seq'::regclass); +ALTER TABLE ONLY import_offline_exports ALTER COLUMN id SET DEFAULT nextval('import_offline_exports_id_seq'::regclass); + ALTER TABLE ONLY import_placeholder_memberships ALTER COLUMN id SET DEFAULT nextval('import_placeholder_memberships_id_seq'::regclass); ALTER TABLE ONLY import_placeholder_user_details ALTER COLUMN id SET DEFAULT nextval('import_placeholder_user_details_id_seq'::regclass); @@ -33952,6 +33976,9 @@ ALTER TABLE ONLY import_export_uploads ALTER TABLE ONLY import_failures ADD CONSTRAINT import_failures_pkey PRIMARY KEY (id); +ALTER TABLE ONLY import_offline_exports + ADD CONSTRAINT import_offline_exports_pkey PRIMARY KEY (id); + ALTER TABLE ONLY import_placeholder_memberships ADD CONSTRAINT import_placeholder_memberships_pkey PRIMARY KEY (id); @@ -39015,6 +39042,8 @@ CREATE INDEX index_bulk_import_export_uploads_on_project_id ON bulk_import_expor CREATE INDEX index_bulk_import_exports_on_group_id ON bulk_import_exports USING btree (group_id); +CREATE INDEX index_bulk_import_exports_on_offline_export_id ON bulk_import_exports USING btree (offline_export_id); + CREATE INDEX index_bulk_import_exports_on_project_id ON bulk_import_exports USING btree (project_id); CREATE INDEX index_bulk_import_exports_on_user_id ON bulk_import_exports USING btree (user_id); @@ -40199,6 +40228,10 @@ CREATE INDEX index_import_failures_on_project_id_not_null ON import_failures USI CREATE INDEX index_import_failures_on_user_id_not_null ON import_failures USING btree (user_id) WHERE (user_id IS NOT NULL); +CREATE INDEX index_import_offline_exports_on_organization_id ON import_offline_exports USING btree (organization_id); + +CREATE INDEX index_import_offline_exports_on_user_id ON import_offline_exports USING btree (user_id); + CREATE INDEX index_import_placeholder_memberships_on_group_id ON import_placeholder_memberships USING btree (group_id); CREATE INDEX index_import_placeholder_memberships_on_namespace_id ON import_placeholder_memberships USING btree (namespace_id); @@ -47722,6 +47755,9 @@ ALTER TABLE ONLY incident_management_timeline_events ALTER TABLE ONLY terraform_state_versions ADD CONSTRAINT fk_180cde327a FOREIGN KEY (project_id) REFERENCES projects(id) ON DELETE CASCADE; +ALTER TABLE ONLY bulk_import_exports + ADD CONSTRAINT fk_18441f89c5 FOREIGN KEY (offline_export_id) REFERENCES import_offline_exports(id) ON DELETE CASCADE; + ALTER TABLE ONLY project_features ADD CONSTRAINT fk_18513d9b92 FOREIGN KEY (project_id) REFERENCES projects(id) ON DELETE CASCADE; @@ -48022,6 +48058,9 @@ ALTER TABLE ONLY user_project_callouts ALTER TABLE ONLY projects_branch_rules_squash_options ADD CONSTRAINT fk_33b614a558 FOREIGN KEY (protected_branch_id) REFERENCES protected_branches(id) ON DELETE CASCADE; +ALTER TABLE ONLY import_offline_exports + ADD CONSTRAINT fk_34265d27dc FOREIGN KEY (organization_id) REFERENCES organizations(id) ON DELETE CASCADE; + ALTER TABLE ONLY namespaces ADD CONSTRAINT fk_3448c97865 FOREIGN KEY (push_rule_id) REFERENCES push_rules(id) ON DELETE SET NULL; @@ -48685,6 +48724,9 @@ ALTER TABLE ONLY merge_requests_approval_rules ALTER TABLE ONLY organization_cluster_agent_mappings ADD CONSTRAINT fk_7b441007e5 FOREIGN KEY (creator_id) REFERENCES users(id) ON DELETE SET NULL; +ALTER TABLE ONLY import_offline_exports + ADD CONSTRAINT fk_7b730f0df3 FOREIGN KEY (user_id) REFERENCES users(id) ON DELETE CASCADE; + ALTER TABLE ONLY issue_customer_relations_contacts ADD CONSTRAINT fk_7b92f835bb FOREIGN KEY (contact_id) REFERENCES customer_relations_contacts(id) ON DELETE CASCADE; diff --git a/lib/import/offline/error.rb b/lib/import/offline/error.rb new file mode 100644 index 00000000000000..9f178d7b48002a --- /dev/null +++ b/lib/import/offline/error.rb @@ -0,0 +1,29 @@ +# frozen_string_literal: true + +module Import + module Offline + class Error < StandardError + def self.feature_flag_not_enabled + new(s_('OfflineTransfer|offline_transfer_exports feature flag must be enabled.')) + end + + def self.invalid_export_permissions(full_paths) + new(format( + s_('OfflineTransfer|Export failed. You do not have permission to export the following resources: %{paths}'), + paths: full_paths.join(', ') + )) + end + + def self.export_paths_not_found(full_paths) + new(format( + s_('OfflineTransfer|Export failed. The following resources do not exist: %{paths}'), + paths: full_paths.join(', ') + )) + end + + def self.invalid_portable_params + new(s_('OfflineTransfer|Export failed. Entity types and full paths must be provided.')) + end + end + end +end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index e64cd38d06f5f1..08fa79121f14bd 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -45309,6 +45309,18 @@ msgstr "" msgid "Offline nodes automatically deleted after" msgstr "" +msgid "OfflineTransfer|Export failed. Entity types and full paths must be provided." +msgstr "" + +msgid "OfflineTransfer|Export failed. The following resources do not exist: %{paths}" +msgstr "" + +msgid "OfflineTransfer|Export failed. You do not have permission to export the following resources: %{paths}" +msgstr "" + +msgid "OfflineTransfer|offline_transfer_exports feature flag must be enabled." +msgstr "" + msgid "Oh no!" msgstr "" diff --git a/spec/factories/bulk_import/exports.rb b/spec/factories/bulk_import/exports.rb index abe73a9314def2..298772e28aadb3 100644 --- a/spec/factories/bulk_import/exports.rb +++ b/spec/factories/bulk_import/exports.rb @@ -24,5 +24,9 @@ trait :batched do batched { true } end + + trait :offline do + offline_export + end end end diff --git a/spec/factories/import/offline/exports.rb b/spec/factories/import/offline/exports.rb new file mode 100644 index 00000000000000..61b7b77cfc93b8 --- /dev/null +++ b/spec/factories/import/offline/exports.rb @@ -0,0 +1,26 @@ +# frozen_string_literal: true + +FactoryBot.define do + factory :offline_export, class: 'Import::Offline::Export' do + user + organization + + source_hostname { 'https://offline-environment-gitlab.example.com' } + + trait :created do + status { 0 } + end + + trait :started do + status { 1 } + end + + trait :finished do + status { 2 } + end + + trait :failed do + status { -1 } + end + end +end diff --git a/spec/models/bulk_imports/export_spec.rb b/spec/models/bulk_imports/export_spec.rb index 0732305802b7fb..7bafdd91fbe48b 100644 --- a/spec/models/bulk_imports/export_spec.rb +++ b/spec/models/bulk_imports/export_spec.rb @@ -6,6 +6,7 @@ describe 'associations' do it { is_expected.to belong_to(:group) } it { is_expected.to belong_to(:project) } + it { is_expected.to belong_to(:offline_export).class_name('Import::Offline::Export') } it { is_expected.to have_one(:upload) } it { is_expected.to have_many(:batches) } end @@ -176,4 +177,22 @@ it { is_expected.to eq(false) } end end + + describe '#offline?' do + context 'when associated to an offline export' do + subject(:export) { build(:bulk_import_export, :offline) } + + it 'returns true' do + expect(export.offline?).to be(true) + end + end + + context 'when not associated to an offline export' do + subject(:export) { build(:bulk_import_export) } + + it 'returns false' do + expect(export.offline?).to be(false) + end + end + end end diff --git a/spec/models/import/offline/export_spec.rb b/spec/models/import/offline/export_spec.rb new file mode 100644 index 00000000000000..07df573b5ef240 --- /dev/null +++ b/spec/models/import/offline/export_spec.rb @@ -0,0 +1,31 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Import::Offline::Export, feature_category: :importers do + describe 'associations' do + it { is_expected.to belong_to(:user) } + it { is_expected.to belong_to(:organization) } + it { is_expected.to have_many(:bulk_import_exports).class_name('BulkImports::Export') } + end + + describe 'validations' do + it { is_expected.to validate_presence_of(:source_hostname) } + it { is_expected.to validate_presence_of(:status) } + + describe '#source_hostname' do + it { is_expected.to allow_value('http://example.com:8080').for(:source_hostname) } + it { is_expected.to allow_value('https://example.com:8080').for(:source_hostname) } + it { is_expected.to allow_value('http://example.com').for(:source_hostname) } + it { is_expected.to allow_value('https://example.com').for(:source_hostname) } + it { is_expected.not_to allow_value('http://').for(:source_hostname) } + it { is_expected.not_to allow_value('example.com').for(:source_hostname) } + it { is_expected.not_to allow_value('https://example.com/dir').for(:source_hostname) } + it { is_expected.not_to allow_value('https://example.com?param=1').for(:source_hostname) } + it { is_expected.not_to allow_value('https://example.com/dir?param=1').for(:source_hostname) } + it { is_expected.not_to allow_value('https://github.com').for(:source_hostname) } + it { is_expected.not_to allow_value('https://bitbucket.org').for(:source_hostname) } + it { is_expected.not_to allow_value('https://gitea.com').for(:source_hostname) } + end + end +end diff --git a/spec/services/import/offline/exports/create_service_spec.rb b/spec/services/import/offline/exports/create_service_spec.rb new file mode 100644 index 00000000000000..1cab163ebaa789 --- /dev/null +++ b/spec/services/import/offline/exports/create_service_spec.rb @@ -0,0 +1,186 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Import::Offline::Exports::CreateService, feature_category: :importers do + describe '#execute' do + let_it_be(:current_user) { create(:user) } + let_it_be(:groups) { create_list(:group, 2) } + let_it_be(:projects) { create_list(:project, 2) } + + let(:source_hostname) { 'https://offline-environment-gitlab.example.com' } + let(:portable_params) do + [ + { type: 'group', full_path: groups[0].full_path }, + { type: 'group', full_path: groups[1].full_path }, + { type: 'project', full_path: projects[0].full_path }, + { type: 'project', full_path: projects[1].full_path } + ] + end + + subject(:service) { described_class.new(current_user, source_hostname, portable_params) } + + before do + groups.each { |group| group.add_owner(current_user) } + projects.each { |project| project.add_owner(current_user) } + end + + shared_examples 'successfully creates an offline export' do + it 'creates an offline export object' do + result = service.execute + + expect(result).to be_success + expect(result.payload).to be_a(Import::Offline::Export) + expect(result.payload.user).to eq(current_user) + expect(result.payload.source_hostname).to eq(source_hostname) + end + end + + it_behaves_like 'successfully creates an offline export' + + context 'when only groups are exported' do + let(:entites) { [{ type: 'group', full_path: groups[0].full_path }] } + + it_behaves_like 'successfully creates an offline export' + end + + context 'when only projects are exported' do + let(:entites) { [{ type: 'project', full_path: projects[0].full_path }] } + + it_behaves_like 'successfully creates an offline export' + end + + context 'when portables contain duplicate paths' do + let(:entites) do + [ + { type: 'group', full_path: groups[0].full_path }, + { type: 'group', full_path: groups[0].full_path }, + { type: 'group', full_path: groups[1].full_path }, + { type: 'project', full_path: projects[0].full_path }, + { type: 'project', full_path: projects[1].full_path }, + { type: 'project', full_path: projects[1].full_path } + ] + end + + it_behaves_like 'successfully creates an offline export' + end + + context 'when the user does not have permission to export all portables' do + let_it_be(:unauthorized_group) { create(:group) } + let_it_be(:unauthorized_project) { create(:project) } + + let(:portable_params) do + [ + { type: 'group', full_path: groups[0].full_path }, + { type: 'group', full_path: unauthorized_group.full_path }, + { type: 'project', full_path: projects[0].full_path }, + { type: 'project', full_path: unauthorized_project.full_path } + ] + end + + it 'returns a service error with full paths the user does not have access to' do + result = service.execute + + expect(result).to be_a(ServiceResponse) + expect(result).to be_error + expect(result.message).to include('You do not have permission to export the following resources') + expect(result.message).to include(unauthorized_group.full_path) + expect(result.message).to include(unauthorized_project.full_path) + end + + it 'returns a service error with full paths the user has insufficient access to' do + unauthorized_group.add_maintainer(current_user) + unauthorized_project.add_developer(current_user) + + result = service.execute + + expect(result).to be_a(ServiceResponse) + expect(result).to be_error + expect(result.message).to include('You do not have permission to export the following resources') + expect(result.message).to include(unauthorized_group.full_path) + expect(result.message).to include(unauthorized_project.full_path) + end + end + + context 'when some portables do not exist' do + let(:portable_params) do + [ + { type: 'group', full_path: groups[0].full_path }, + { type: 'group', full_path: 'nonexistent/group' }, + { type: 'project', full_path: projects[0].full_path }, + { type: 'project', full_path: 'nonexistent/project' } + ] + end + + it 'returns a service error with nonexistent full paths' do + result = service.execute + + expect(result).to be_a(ServiceResponse) + expect(result).to be_error + expect(result.message).to include('The following resources do not exist') + expect(result.message).to include('nonexistent/group') + expect(result.message).to include('nonexistent/project') + end + end + + context 'when a portable type is not provided' do + let(:portable_params) do + [ + { type: '', full_path: groups[0].full_path }, + { type: 'project', full_path: projects[0].full_path } + ] + end + + it 'returns a service error' do + result = service.execute + + expect(result).to be_a(ServiceResponse) + expect(result).to be_error + expect(result.message).to include('Entity types and full paths must be provided') + end + end + + context 'when a portable full path is not provided' do + let(:portable_params) do + [ + { type: 'group', full_path: '' }, + { type: 'project', full_path: projects[0].full_path } + ] + end + + it 'returns a service error' do + result = service.execute + + expect(result).to be_a(ServiceResponse) + expect(result).to be_error + expect(result.message).to include('Entity types and full paths must be provided') + end + end + + context 'when the offline export fails validations' do + let(:source_hostname) { 'invalid-hostname' } + + it 'returns a service error' do + result = service.execute + + expect(result).to be_a(ServiceResponse) + expect(result).to be_error + expect(result.message).to include('must contain scheme and host') + end + end + + context 'when offline_transfer_exports is disabled' do + before do + stub_feature_flags(offline_transfer_exports: false) + end + + it 'returns a service response error' do + result = service.execute + + expect(result).to be_a(ServiceResponse) + expect(result).to be_error + expect(result.message).to eq('offline_transfer_exports feature flag must be enabled.') + end + end + end +end -- GitLab From 7338fe603f76be2c340deba4b70a8c3457def5a7 Mon Sep 17 00:00:00 2001 From: Sam Word Date: Wed, 22 Oct 2025 01:12:26 -0400 Subject: [PATCH 2/3] Refactored based on feedback --- app/models/bulk_imports/export.rb | 2 +- .../import/offline/exports/create_service.rb | 87 +++++++++++-------- lib/import/offline/error.rb | 29 ------- locale/gitlab.pot | 5 +- spec/models/bulk_imports/export_spec.rb | 4 +- .../offline/exports/create_service_spec.rb | 74 ++++++---------- 6 files changed, 82 insertions(+), 119 deletions(-) delete mode 100644 lib/import/offline/error.rb diff --git a/app/models/bulk_imports/export.rb b/app/models/bulk_imports/export.rb index 7edd068c8f633c..c532dbafbdf881 100644 --- a/app/models/bulk_imports/export.rb +++ b/app/models/bulk_imports/export.rb @@ -86,7 +86,7 @@ def relation_has_user_contributions? end def offline? - offline_export.present? + offline_export_id.present? end end end diff --git a/app/services/import/offline/exports/create_service.rb b/app/services/import/offline/exports/create_service.rb index 0aa58ae67065f6..4135068a6756b4 100644 --- a/app/services/import/offline/exports/create_service.rb +++ b/app/services/import/offline/exports/create_service.rb @@ -6,8 +6,11 @@ module Exports class CreateService include ::Gitlab::Utils::StrongMemoize - ServiceError = Class.new(StandardError) - + # @param current_user [User] current user object + # @param source_hostname [String] source hostname or alias hostname + # @param portable_params [Array] list of portables to export. + # Each portable hash must have at least its type and full path. E.g.: + # { type: 'project', full_path: 'gitlab-org/gitlab' } def initialize(current_user, source_hostname, portable_params) @current_user = current_user @source_hostname = source_hostname @@ -15,12 +18,9 @@ def initialize(current_user, source_hostname, portable_params) end def execute - unless Feature.enabled?(:offline_transfer_exports, current_user) - raise Import::Offline::Error.feature_flag_not_enabled - end - - validate_user_permissions! - validate_portables_exist! + return feature_flag_disabled_error unless Feature.enabled?(:offline_transfer_exports, current_user) + return invalid_params_error unless portable_params_valid? + return insufficient_permissions_error unless user_can_export_all_portables? offline_export = Import::Offline::Export.create!( user: current_user, @@ -29,39 +29,32 @@ def execute ) ServiceResponse.success(payload: offline_export) - rescue ActiveRecord::RecordInvalid, Import::Offline::Error => e - ServiceResponse.error( - message: e.message, - reason: :unprocessable_entity - ) + rescue ActiveRecord::RecordInvalid => e + service_error(e.message) end private - attr_reader :current_user, :source_hostname, :portable_params + attr_reader :current_user, :source_hostname, :portable_params, :invalid_paths - def validate_portables_exist! - missing_params = portable_params.any? { |params| params[:type].blank? || params[:full_path].blank? } + def user_can_export_all_portables? + full_path_params = portable_full_paths + found_full_paths = groups.map(&:full_path) + projects.map(&:full_path) - raise Import::Offline::Error.invalid_portable_params if missing_params + @invalid_paths = full_path_params - found_full_paths - missing_group_paths = full_paths_for_type(:group) - groups.map(&:full_path) - missing_project_paths = full_paths_for_type(:project) - projects.map(&:full_path) - - return if missing_group_paths.blank? && missing_project_paths.blank? - - raise Import::Offline::Error.export_paths_not_found([missing_group_paths, missing_project_paths].flatten) - end - - def validate_user_permissions! - portables = [groups, projects].flatten - invalid_paths = portables.filter_map do |portable| + @invalid_paths += [groups, projects].flatten.filter_map do |portable| portable.full_path unless user_can_admin_portable?(portable) end - return unless invalid_paths.present? + @invalid_paths.blank? + end + + def portable_params_valid? + return false if portable_params.blank? + return false if portable_params.any? { |h| !h.is_a?(Hash) || h[:type].blank? || h[:full_path].blank? } - raise Import::Offline::Error.invalid_export_permissions(invalid_paths) + true end def user_can_admin_portable?(portable) @@ -71,19 +64,41 @@ def user_can_admin_portable?(portable) end def groups - Group.where_full_path_in(full_paths_for_type(:group)) + Group.where_full_path_in(portable_full_paths) end strong_memoize_attr :groups def projects - Project.where_full_path_in(full_paths_for_type(:project)) + Project.where_full_path_in(portable_full_paths) end strong_memoize_attr :projects - def full_paths_for_type(type) - portable_params.filter_map do |params| - params[:full_path] if params[:type] == type.to_s - end.uniq + def portable_full_paths + portable_params.map { |params| params[:full_path] }.uniq # rubocop:disable Rails/Pluck -- Not an ActiveRecord object + end + strong_memoize_attr :portable_full_paths + + def feature_flag_disabled_error + service_error(s_('OfflineTransfer|offline_transfer_exports feature flag must be enabled.')) + end + + def invalid_params_error + service_error(s_('OfflineTransfer|Export failed. Entity types and full paths must be provided.')) + end + + def insufficient_permissions_error + service_error(format( + s_('OfflineTransfer|Export failed. You do not have permission to ' \ + 'export the following resources or they do not exist: %{paths}'), + paths: invalid_paths.join(', ') + )) + end + + def service_error(message) + ServiceResponse.error( + message: message, + reason: :unprocessable_entity + ) end end end diff --git a/lib/import/offline/error.rb b/lib/import/offline/error.rb deleted file mode 100644 index 9f178d7b48002a..00000000000000 --- a/lib/import/offline/error.rb +++ /dev/null @@ -1,29 +0,0 @@ -# frozen_string_literal: true - -module Import - module Offline - class Error < StandardError - def self.feature_flag_not_enabled - new(s_('OfflineTransfer|offline_transfer_exports feature flag must be enabled.')) - end - - def self.invalid_export_permissions(full_paths) - new(format( - s_('OfflineTransfer|Export failed. You do not have permission to export the following resources: %{paths}'), - paths: full_paths.join(', ') - )) - end - - def self.export_paths_not_found(full_paths) - new(format( - s_('OfflineTransfer|Export failed. The following resources do not exist: %{paths}'), - paths: full_paths.join(', ') - )) - end - - def self.invalid_portable_params - new(s_('OfflineTransfer|Export failed. Entity types and full paths must be provided.')) - end - end - end -end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 08fa79121f14bd..520a92b22a7038 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -45312,10 +45312,7 @@ msgstr "" msgid "OfflineTransfer|Export failed. Entity types and full paths must be provided." msgstr "" -msgid "OfflineTransfer|Export failed. The following resources do not exist: %{paths}" -msgstr "" - -msgid "OfflineTransfer|Export failed. You do not have permission to export the following resources: %{paths}" +msgid "OfflineTransfer|Export failed. You do not have permission to export the following resources or they do not exist: %{paths}" msgstr "" msgid "OfflineTransfer|offline_transfer_exports feature flag must be enabled." diff --git a/spec/models/bulk_imports/export_spec.rb b/spec/models/bulk_imports/export_spec.rb index 7bafdd91fbe48b..76a099c539170a 100644 --- a/spec/models/bulk_imports/export_spec.rb +++ b/spec/models/bulk_imports/export_spec.rb @@ -180,7 +180,7 @@ describe '#offline?' do context 'when associated to an offline export' do - subject(:export) { build(:bulk_import_export, :offline) } + subject(:export) { create(:bulk_import_export, :offline) } it 'returns true' do expect(export.offline?).to be(true) @@ -188,7 +188,7 @@ end context 'when not associated to an offline export' do - subject(:export) { build(:bulk_import_export) } + subject(:export) { create(:bulk_import_export) } it 'returns false' do expect(export.offline?).to be(false) diff --git a/spec/services/import/offline/exports/create_service_spec.rb b/spec/services/import/offline/exports/create_service_spec.rb index 1cab163ebaa789..ed0811a8451974 100644 --- a/spec/services/import/offline/exports/create_service_spec.rb +++ b/spec/services/import/offline/exports/create_service_spec.rb @@ -2,11 +2,11 @@ require 'spec_helper' -RSpec.describe Import::Offline::Exports::CreateService, feature_category: :importers do +RSpec.describe Import::Offline::Exports::CreateService, :aggregate_failures, feature_category: :importers do describe '#execute' do let_it_be(:current_user) { create(:user) } - let_it_be(:groups) { create_list(:group, 2) } - let_it_be(:projects) { create_list(:project, 2) } + let_it_be(:groups) { create_list(:group, 2, owners: current_user) } + let_it_be(:projects) { create_list(:project, 2, maintainers: current_user) } let(:source_hostname) { 'https://offline-environment-gitlab.example.com' } let(:portable_params) do @@ -20,11 +20,6 @@ subject(:service) { described_class.new(current_user, source_hostname, portable_params) } - before do - groups.each { |group| group.add_owner(current_user) } - projects.each { |project| project.add_owner(current_user) } - end - shared_examples 'successfully creates an offline export' do it 'creates an offline export object' do result = service.execute @@ -39,19 +34,19 @@ it_behaves_like 'successfully creates an offline export' context 'when only groups are exported' do - let(:entites) { [{ type: 'group', full_path: groups[0].full_path }] } + let(:portable_params) { [{ type: 'group', full_path: groups[0].full_path }] } it_behaves_like 'successfully creates an offline export' end context 'when only projects are exported' do - let(:entites) { [{ type: 'project', full_path: projects[0].full_path }] } + let(:portable_params) { [{ type: 'project', full_path: projects[0].full_path }] } it_behaves_like 'successfully creates an offline export' end context 'when portables contain duplicate paths' do - let(:entites) do + let(:portable_params) do [ { type: 'group', full_path: groups[0].full_path }, { type: 'group', full_path: groups[0].full_path }, @@ -65,61 +60,46 @@ it_behaves_like 'successfully creates an offline export' end - context 'when the user does not have permission to export all portables' do + context 'when portables are invalid' do let_it_be(:unauthorized_group) { create(:group) } let_it_be(:unauthorized_project) { create(:project) } + let_it_be(:low_access_group) { create(:group) } + let_it_be(:low_access_project) { create(:project) } + + let(:invalid_portable_error) do + 'You do not have permission to export the following resources or they do not exist' + end let(:portable_params) do [ { type: 'group', full_path: groups[0].full_path }, { type: 'group', full_path: unauthorized_group.full_path }, + { type: 'group', full_path: low_access_group.full_path }, + { type: 'group', full_path: 'nonexistent/group' }, { type: 'project', full_path: projects[0].full_path }, - { type: 'project', full_path: unauthorized_project.full_path } + { type: 'project', full_path: unauthorized_project.full_path }, + { type: 'project', full_path: low_access_project.full_path }, + { type: 'project', full_path: 'nonexistent/project' } ] end - it 'returns a service error with full paths the user does not have access to' do - result = service.execute - - expect(result).to be_a(ServiceResponse) - expect(result).to be_error - expect(result.message).to include('You do not have permission to export the following resources') - expect(result.message).to include(unauthorized_group.full_path) - expect(result.message).to include(unauthorized_project.full_path) - end - - it 'returns a service error with full paths the user has insufficient access to' do - unauthorized_group.add_maintainer(current_user) - unauthorized_project.add_developer(current_user) - - result = service.execute - - expect(result).to be_a(ServiceResponse) - expect(result).to be_error - expect(result.message).to include('You do not have permission to export the following resources') - expect(result.message).to include(unauthorized_group.full_path) - expect(result.message).to include(unauthorized_project.full_path) + before_all do + low_access_group.add_maintainer(current_user) + low_access_project.add_developer(current_user) end - end - context 'when some portables do not exist' do - let(:portable_params) do - [ - { type: 'group', full_path: groups[0].full_path }, - { type: 'group', full_path: 'nonexistent/group' }, - { type: 'project', full_path: projects[0].full_path }, - { type: 'project', full_path: 'nonexistent/project' } + it 'returns a service error without differentiating nonexistant and unauthorized portables' do + invalid_paths = [ + unauthorized_group.full_path, low_access_group.full_path, 'nonexistent/group', + unauthorized_project.full_path, low_access_project.full_path, 'nonexistent/project' ] - end - it 'returns a service error with nonexistent full paths' do result = service.execute expect(result).to be_a(ServiceResponse) expect(result).to be_error - expect(result.message).to include('The following resources do not exist') - expect(result.message).to include('nonexistent/group') - expect(result.message).to include('nonexistent/project') + expect(result.message).to include(invalid_portable_error) + invalid_paths.each { |path| expect(result.message).to include(path) } end end -- GitLab From db38158db230ed90ad548c34a5f6630c2305fe8a Mon Sep 17 00:00:00 2001 From: Sam Word Date: Wed, 22 Oct 2025 10:59:33 -0400 Subject: [PATCH 3/3] Added more refactors from feedback --- app/services/import/offline/exports/create_service.rb | 2 +- locale/gitlab.pot | 3 --- spec/models/bulk_imports/export_spec.rb | 8 ++------ .../import/offline/exports/create_service_spec.rb | 9 ++------- 4 files changed, 5 insertions(+), 17 deletions(-) diff --git a/app/services/import/offline/exports/create_service.rb b/app/services/import/offline/exports/create_service.rb index 4135068a6756b4..7d6ff589d4696e 100644 --- a/app/services/import/offline/exports/create_service.rb +++ b/app/services/import/offline/exports/create_service.rb @@ -79,7 +79,7 @@ def portable_full_paths strong_memoize_attr :portable_full_paths def feature_flag_disabled_error - service_error(s_('OfflineTransfer|offline_transfer_exports feature flag must be enabled.')) + service_error('offline_transfer_exports feature flag must be enabled.') end def invalid_params_error diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 520a92b22a7038..cb287ab23e17f2 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -45315,9 +45315,6 @@ msgstr "" msgid "OfflineTransfer|Export failed. You do not have permission to export the following resources or they do not exist: %{paths}" msgstr "" -msgid "OfflineTransfer|offline_transfer_exports feature flag must be enabled." -msgstr "" - msgid "Oh no!" msgstr "" diff --git a/spec/models/bulk_imports/export_spec.rb b/spec/models/bulk_imports/export_spec.rb index 76a099c539170a..09bbf3a291e71e 100644 --- a/spec/models/bulk_imports/export_spec.rb +++ b/spec/models/bulk_imports/export_spec.rb @@ -182,17 +182,13 @@ context 'when associated to an offline export' do subject(:export) { create(:bulk_import_export, :offline) } - it 'returns true' do - expect(export.offline?).to be(true) - end + it { is_expected.to be_offline } end context 'when not associated to an offline export' do subject(:export) { create(:bulk_import_export) } - it 'returns false' do - expect(export.offline?).to be(false) - end + it { is_expected.not_to be_offline } end end end diff --git a/spec/services/import/offline/exports/create_service_spec.rb b/spec/services/import/offline/exports/create_service_spec.rb index ed0811a8451974..eca06d12ab18c7 100644 --- a/spec/services/import/offline/exports/create_service_spec.rb +++ b/spec/services/import/offline/exports/create_service_spec.rb @@ -63,8 +63,8 @@ context 'when portables are invalid' do let_it_be(:unauthorized_group) { create(:group) } let_it_be(:unauthorized_project) { create(:project) } - let_it_be(:low_access_group) { create(:group) } - let_it_be(:low_access_project) { create(:project) } + let_it_be(:low_access_group) { create(:group, maintainers: current_user) } + let_it_be(:low_access_project) { create(:project, developers: current_user) } let(:invalid_portable_error) do 'You do not have permission to export the following resources or they do not exist' @@ -83,11 +83,6 @@ ] end - before_all do - low_access_group.add_maintainer(current_user) - low_access_project.add_developer(current_user) - end - it 'returns a service error without differentiating nonexistant and unauthorized portables' do invalid_paths = [ unauthorized_group.full_path, low_access_group.full_path, 'nonexistent/group', -- GitLab