From 84f301eb94d215236e465753a6b4d564fe95fdfd Mon Sep 17 00:00:00 2001 From: maddievn Date: Thu, 7 Dec 2023 15:25:16 +0200 Subject: [PATCH 01/18] Cache users from Bitbucket Server Changelog: added --- app/workers/all_queues.yml | 9 +++ .../stage/import_repository_worker.rb | 6 +- .../stage/import_users_worker.rb | 25 ++++++ .../bitbucket_server_cache_users.yml | 8 ++ config/sidekiq_queues.yml | 2 + lib/bitbucket_server/client.rb | 5 ++ lib/bitbucket_server/representation/user.rb | 21 +++++ lib/bitbucket_server/users_mapper.rb | 29 +++++++ .../importers/users_importer.rb | 51 ++++++++++++ spec/lib/bitbucket_server/client_spec.rb | 10 +++ .../representation/user_spec.rb | 19 +++++ .../lib/bitbucket_server/users_mapper_spec.rb | 38 +++++++++ .../importers/users_importer_spec.rb | 50 ++++++++++++ spec/workers/every_sidekiq_worker_spec.rb | 1 + .../stage/import_repository_worker_spec.rb | 15 +++- .../stage/import_users_worker_spec.rb | 77 +++++++++++++++++++ 16 files changed, 364 insertions(+), 2 deletions(-) create mode 100644 app/workers/gitlab/bitbucket_server_import/stage/import_users_worker.rb create mode 100644 config/feature_flags/development/bitbucket_server_cache_users.yml create mode 100644 lib/bitbucket_server/representation/user.rb create mode 100644 lib/bitbucket_server/users_mapper.rb create mode 100644 lib/gitlab/bitbucket_server_import/importers/users_importer.rb create mode 100644 spec/lib/bitbucket_server/representation/user_spec.rb create mode 100644 spec/lib/bitbucket_server/users_mapper_spec.rb create mode 100644 spec/lib/gitlab/bitbucket_server_import/importers/users_importer_spec.rb create mode 100644 spec/workers/gitlab/bitbucket_server_import/stage/import_users_worker_spec.rb diff --git a/app/workers/all_queues.yml b/app/workers/all_queues.yml index 3d8c349910ee96..bfbadc16960da3 100644 --- a/app/workers/all_queues.yml +++ b/app/workers/all_queues.yml @@ -2577,6 +2577,15 @@ :weight: 1 :idempotent: false :tags: [] +- :name: bitbucket_server_import_stage_import_users + :worker_name: Gitlab::BitbucketServerImport::Stage::ImportUsersWorker + :feature_category: :importers + :has_external_dependencies: true + :urgency: :low + :resource_boundary: :unknown + :weight: 1 + :idempotent: false + :tags: [] - :name: bulk_import :worker_name: BulkImportWorker :feature_category: :importers diff --git a/app/workers/gitlab/bitbucket_server_import/stage/import_repository_worker.rb b/app/workers/gitlab/bitbucket_server_import/stage/import_repository_worker.rb index b378d07d59cd60..113f984d573fb4 100644 --- a/app/workers/gitlab/bitbucket_server_import/stage/import_repository_worker.rb +++ b/app/workers/gitlab/bitbucket_server_import/stage/import_repository_worker.rb @@ -14,7 +14,11 @@ def import(project) importer.execute - ImportPullRequestsWorker.perform_async(project.id) + if Feature.enabled?(:bitbucket_server_cache_users, project.creator) + ImportUsersWorker.perform_async(project.id) + else + ImportPullRequestsWorker.perform_async(project.id) + end end def importer_class diff --git a/app/workers/gitlab/bitbucket_server_import/stage/import_users_worker.rb b/app/workers/gitlab/bitbucket_server_import/stage/import_users_worker.rb new file mode 100644 index 00000000000000..203ca50ea0f884 --- /dev/null +++ b/app/workers/gitlab/bitbucket_server_import/stage/import_users_worker.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +module Gitlab + module BitbucketServerImport + module Stage + class ImportUsersWorker # rubocop:disable Scalability/IdempotentWorker -- users should be cached once only + include StageMethods + + private + + def import(project) + importer = importer_class.new(project) + + importer.execute + + ImportPullRequestsWorker.perform_async(project.id) + end + + def importer_class + Importers::UsersImporter + end + end + end + end +end diff --git a/config/feature_flags/development/bitbucket_server_cache_users.yml b/config/feature_flags/development/bitbucket_server_cache_users.yml new file mode 100644 index 00000000000000..1458521151c219 --- /dev/null +++ b/config/feature_flags/development/bitbucket_server_cache_users.yml @@ -0,0 +1,8 @@ +--- +name: bitbucket_server_cache_users +introduced_by_url: +rollout_issue_url: +milestone: '16.7' +type: development +group: group::import and integrate +default_enabled: false diff --git a/config/sidekiq_queues.yml b/config/sidekiq_queues.yml index 77503814158d3b..cb45c09560dd49 100644 --- a/config/sidekiq_queues.yml +++ b/config/sidekiq_queues.yml @@ -127,6 +127,8 @@ - 1 - - bitbucket_server_import_stage_import_repository - 1 +- - bitbucket_server_import_stage_import_users + - 1 - - bulk_import - 1 - - bulk_imports_entity diff --git a/lib/bitbucket_server/client.rb b/lib/bitbucket_server/client.rb index 8e84afe51d771a..5f09f029d18f71 100644 --- a/lib/bitbucket_server/client.rb +++ b/lib/bitbucket_server/client.rb @@ -29,6 +29,11 @@ def repos(page_offset: 0, limit: nil, filter: nil) get_collection(path, :repo, page_offset: page_offset, limit: limit) end + def users(project) + path = "/projects/#{project}/permissions/users" + get_collection(path, :user) + end + def create_branch(project_key, repo, branch_name, sha) payload = { name: branch_name, diff --git a/lib/bitbucket_server/representation/user.rb b/lib/bitbucket_server/representation/user.rb new file mode 100644 index 00000000000000..433baec1c42f96 --- /dev/null +++ b/lib/bitbucket_server/representation/user.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +module BitbucketServer + module Representation + class User < Representation::Base + def email + user['emailAddress'] + end + + def username + user['slug'] + end + + private + + def user + raw['user'] + end + end + end +end diff --git a/lib/bitbucket_server/users_mapper.rb b/lib/bitbucket_server/users_mapper.rb new file mode 100644 index 00000000000000..f24d4465692500 --- /dev/null +++ b/lib/bitbucket_server/users_mapper.rb @@ -0,0 +1,29 @@ +# frozen_string_literal: true + +module BitbucketServer + class UsersMapper + include Gitlab::Utils::StrongMemoize + + SOURCE_USER_CACHE_KEY = 'bitbucket_server/project/%s/source/username/%s' + + def initialize(project) + @project_id = project.id + end + + attr_reader :project_id + + def write_source_email_to_cache(username, email) + ::Gitlab::Cache::Import::Caching.write(cache_key(username), email) + end + + def read_source_email_from_cache(username) + ::Gitlab::Cache::Import::Caching.read(cache_key(username)) + end + + private + + def cache_key(username) + format(SOURCE_USER_CACHE_KEY, project_id, username) + end + end +end diff --git a/lib/gitlab/bitbucket_server_import/importers/users_importer.rb b/lib/gitlab/bitbucket_server_import/importers/users_importer.rb new file mode 100644 index 00000000000000..777b677449d9a4 --- /dev/null +++ b/lib/gitlab/bitbucket_server_import/importers/users_importer.rb @@ -0,0 +1,51 @@ +# frozen_string_literal: true + +module Gitlab + module BitbucketServerImport + module Importers + class UsersImporter + include Loggable + + BATCH_SIZE = 100 + + def initialize(project) + @project = project + end + + attr_reader :project + + def execute + log_info(import_stage: 'import_users', message: 'starting') + + users = client.users(project_key) + + users.each_slice(BATCH_SIZE) do |batch| + log_info(import_stage: 'import_users', message: "caching batch of #{batch.count} users") + + batch.each { |user| cache_source_user(user) } + end + + log_info(import_stage: 'import_users', message: 'finished') + end + + private + + def cache_source_user(user) + users_mapper.write_source_email_to_cache(user.username, user.email) + end + + def users_mapper + @users_mapper ||= BitbucketServer::UsersMapper.new(project) + end + + def client + BitbucketServer::Client.new(project.import_data.credentials) + end + + def project_key + project.import_data.data['project_key'] + end + end + end + end +end diff --git a/spec/lib/bitbucket_server/client_spec.rb b/spec/lib/bitbucket_server/client_spec.rb index 1d6e8492760129..b11367ae01ccb4 100644 --- a/spec/lib/bitbucket_server/client_spec.rb +++ b/spec/lib/bitbucket_server/client_spec.rb @@ -80,6 +80,16 @@ end end + describe '#users' do + let(:path) { "/projects/#{project}/permissions/users" } + + it 'requests a collection' do + expect(BitbucketServer::Paginator).to receive(:new).with(anything, path, :user, page_offset: 0, limit: nil) + + subject.users(project) + end + end + describe '#create_branch' do let(:branch) { 'test-branch' } let(:sha) { '12345678' } diff --git a/spec/lib/bitbucket_server/representation/user_spec.rb b/spec/lib/bitbucket_server/representation/user_spec.rb new file mode 100644 index 00000000000000..32470e3a12ffc2 --- /dev/null +++ b/spec/lib/bitbucket_server/representation/user_spec.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe BitbucketServer::Representation::User, feature_category: :importers do + let(:email) { 'test@email.com' } + let(:username) { 'test_user' } + let(:sample_data) { { 'user' => { 'emailAddress' => email, 'slug' => username } } } + + subject(:user) { described_class.new(sample_data) } + + describe '#email' do + it { expect(user.email).to eq(email) } + end + + describe '#username' do + it { expect(user.username).to eq(username) } + end +end diff --git a/spec/lib/bitbucket_server/users_mapper_spec.rb b/spec/lib/bitbucket_server/users_mapper_spec.rb new file mode 100644 index 00000000000000..79bb3f343d7089 --- /dev/null +++ b/spec/lib/bitbucket_server/users_mapper_spec.rb @@ -0,0 +1,38 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe BitbucketServer::UsersMapper, feature_category: :importers do + let_it_be(:project) { create(:project) } + let(:username) { 'test_user' } + let(:email) { 'test@email.com' } + + subject(:users_mapper) { described_class.new(project) } + + describe '#write_source_email_to_cache' do + it 'caches provided source username and email' do + expect(Gitlab::Cache::Import::Caching).to receive(:write) + .with("bitbucket_server/project/#{project.id}/source/username/#{username}", email) + + users_mapper.write_source_email_to_cache(username, email) + end + end + + describe '#read_source_email_from_cache', :clean_gitlab_redis_cache do + context 'when it has been cached' do + before do + users_mapper.write_source_email_to_cache(username, email) + end + + it 'returns the matching email' do + expect(users_mapper.read_source_email_from_cache(username)).to eq(email) + end + end + + context 'when it has not been cached' do + it 'returns the matching email' do + expect(users_mapper.read_source_email_from_cache(username)).to be_nil + end + end + end +end diff --git a/spec/lib/gitlab/bitbucket_server_import/importers/users_importer_spec.rb b/spec/lib/gitlab/bitbucket_server_import/importers/users_importer_spec.rb new file mode 100644 index 00000000000000..33803718ffd6bb --- /dev/null +++ b/spec/lib/gitlab/bitbucket_server_import/importers/users_importer_spec.rb @@ -0,0 +1,50 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::BitbucketServerImport::Importers::UsersImporter, feature_category: :importers do + let(:users_mapper) { BitbucketServer::UsersMapper.new(project) } + + let_it_be(:project) do + create(:project, :with_import_url, :import_started, :empty_repo, + import_data_attributes: { + data: { 'project_key' => 'key', 'repo_slug' => 'slug' }, + credentials: { 'base_uri' => 'http://bitbucket.org/', 'user' => 'bitbucket', 'password' => 'password' } + } + ) + end + + before do + allow(BitbucketServer::UsersMapper).to receive(:new).and_return(users_mapper) + stub_const("#{described_class}::BATCH_SIZE", 1) + allow(Gitlab::BitbucketServerImport::Logger).to receive(:info).and_call_original + + allow_next_instance_of(BitbucketServer::Client) do |client| + allow(client).to receive(:users).with('key').and_return( + [ + BitbucketServer::Representation::User.new( + { 'user' => { 'emailAddress' => 'email1', 'slug' => 'username1' } } + ), + BitbucketServer::Representation::User.new( + { 'user' => { 'emailAddress' => 'email2', 'slug' => 'username2' } } + ) + ] + ) + end + end + + subject(:importer) { described_class.new(project) } + + describe '#execute' do + it 'writes the username and email to cache for every user in batches' do + expect(Gitlab::BitbucketServerImport::Logger).to receive(:info).exactly(4).times + + expect(users_mapper).to receive(:write_source_email_to_cache).and_call_original.twice + + importer.execute + + expect(users_mapper.read_source_email_from_cache('username1')).to eq('email1') + expect(users_mapper.read_source_email_from_cache('username2')).to eq('email2') + end + end +end diff --git a/spec/workers/every_sidekiq_worker_spec.rb b/spec/workers/every_sidekiq_worker_spec.rb index 136fa6b4aa0f6c..ecc2774afc4df9 100644 --- a/spec/workers/every_sidekiq_worker_spec.rb +++ b/spec/workers/every_sidekiq_worker_spec.rb @@ -262,6 +262,7 @@ 'Gitlab::BitbucketServerImport::Stage::ImportNotesWorker' => 3, 'Gitlab::BitbucketServerImport::Stage::ImportPullRequestsWorker' => 3, 'Gitlab::BitbucketServerImport::Stage::ImportRepositoryWorker' => 3, + 'Gitlab::BitbucketServerImport::Stage::ImportUsersWorker' => 3, 'Gitlab::GithubImport::AdvanceStageWorker' => 3, 'Gitlab::GithubImport::Attachments::ImportReleaseWorker' => 5, 'Gitlab::GithubImport::Attachments::ImportNoteWorker' => 5, diff --git a/spec/workers/gitlab/bitbucket_server_import/stage/import_repository_worker_spec.rb b/spec/workers/gitlab/bitbucket_server_import/stage/import_repository_worker_spec.rb index 7ea23041e791b3..619202a1c46f75 100644 --- a/spec/workers/gitlab/bitbucket_server_import/stage/import_repository_worker_spec.rb +++ b/spec/workers/gitlab/bitbucket_server_import/stage/import_repository_worker_spec.rb @@ -18,12 +18,25 @@ end it 'schedules the next stage' do - expect(Gitlab::BitbucketServerImport::Stage::ImportPullRequestsWorker).to receive(:perform_async) + expect(Gitlab::BitbucketServerImport::Stage::ImportUsersWorker).to receive(:perform_async) .with(project.id) worker.perform(project.id) end + context 'when the bitbucket_server_cache_users flag is disabled' do + before do + stub_feature_flags(bitbucket_server_cache_users: false) + end + + it 'skips the user import and schedules the next stage' do + expect(Gitlab::BitbucketServerImport::Stage::ImportPullRequestsWorker).to receive(:perform_async) + .with(project.id) + + worker.perform(project.id) + end + end + it 'logs stage start and finish' do expect(Gitlab::BitbucketServerImport::Logger) .to receive(:info).with(hash_including(message: 'starting stage', project_id: project.id)) diff --git a/spec/workers/gitlab/bitbucket_server_import/stage/import_users_worker_spec.rb b/spec/workers/gitlab/bitbucket_server_import/stage/import_users_worker_spec.rb new file mode 100644 index 00000000000000..d4cd1b82349529 --- /dev/null +++ b/spec/workers/gitlab/bitbucket_server_import/stage/import_users_worker_spec.rb @@ -0,0 +1,77 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::BitbucketServerImport::Stage::ImportUsersWorker, feature_category: :importers do + let_it_be(:project) { create(:project, :import_started) } + + let(:worker) { described_class.new } + + it_behaves_like Gitlab::BitbucketServerImport::StageMethods + + describe '#perform' do + context 'when the import succeeds' do + before do + allow_next_instance_of(Gitlab::BitbucketServerImport::Importers::UsersImporter) do |importer| + allow(importer).to receive(:execute) + end + end + + it 'schedules the next stage' do + expect(Gitlab::BitbucketServerImport::Stage::ImportPullRequestsWorker).to receive(:perform_async) + .with(project.id) + + worker.perform(project.id) + end + + it 'logs stage start and finish' do + expect(Gitlab::BitbucketServerImport::Logger) + .to receive(:info).with(hash_including(message: 'starting stage', project_id: project.id)) + expect(Gitlab::BitbucketServerImport::Logger) + .to receive(:info).with(hash_including(message: 'stage finished', project_id: project.id)) + + worker.perform(project.id) + end + end + + context 'when project does not exists' do + it 'does not call importer' do + expect(Gitlab::BitbucketServerImport::Importers::UsersImporter).not_to receive(:new) + + worker.perform(-1) + end + end + + context 'when project import state is not `started`' do + it 'does not call importer' do + project = create(:project, :import_canceled) + + expect(Gitlab::BitbucketServerImport::Importers::UsersImporter).not_to receive(:new) + + worker.perform(project.id) + end + end + + context 'when the importer fails' do + it 'does not schedule the next stage and raises error' do + exception = StandardError.new('Error') + + allow_next_instance_of(Gitlab::BitbucketServerImport::Importers::UsersImporter) do |importer| + allow(importer).to receive(:execute).and_raise(exception) + end + + expect(Gitlab::Import::ImportFailureService) + .to receive(:track).with( + project_id: project.id, + exception: exception, + error_source: described_class.name, + fail_import: false + ).and_call_original + + expect { worker.perform(project.id) } + .to change { Gitlab::BitbucketServerImport::Stage::ImportUsersWorker.jobs.size }.by(0) + .and raise_error(exception) + end + end + end +end -- GitLab From 42703afe242fe6cca57da47f78587ec646a4fb73 Mon Sep 17 00:00:00 2001 From: maddievn Date: Fri, 8 Dec 2023 10:13:08 +0200 Subject: [PATCH 02/18] Add details for feature flag --- .../stage/import_repository_worker.rb | 2 +- .../development/bitbucket_server_cache_users.yml | 8 -------- .../bitbucket_server_convert_mentions_to_users.yml | 8 ++++++++ .../stage/import_repository_worker_spec.rb | 4 ++-- 4 files changed, 11 insertions(+), 11 deletions(-) delete mode 100644 config/feature_flags/development/bitbucket_server_cache_users.yml create mode 100644 config/feature_flags/development/bitbucket_server_convert_mentions_to_users.yml diff --git a/app/workers/gitlab/bitbucket_server_import/stage/import_repository_worker.rb b/app/workers/gitlab/bitbucket_server_import/stage/import_repository_worker.rb index 113f984d573fb4..573c73cd7dfe66 100644 --- a/app/workers/gitlab/bitbucket_server_import/stage/import_repository_worker.rb +++ b/app/workers/gitlab/bitbucket_server_import/stage/import_repository_worker.rb @@ -14,7 +14,7 @@ def import(project) importer.execute - if Feature.enabled?(:bitbucket_server_cache_users, project.creator) + if Feature.enabled?(:bitbucket_server_convert_mentions_to_users, project.creator) ImportUsersWorker.perform_async(project.id) else ImportPullRequestsWorker.perform_async(project.id) diff --git a/config/feature_flags/development/bitbucket_server_cache_users.yml b/config/feature_flags/development/bitbucket_server_cache_users.yml deleted file mode 100644 index 1458521151c219..00000000000000 --- a/config/feature_flags/development/bitbucket_server_cache_users.yml +++ /dev/null @@ -1,8 +0,0 @@ ---- -name: bitbucket_server_cache_users -introduced_by_url: -rollout_issue_url: -milestone: '16.7' -type: development -group: group::import and integrate -default_enabled: false diff --git a/config/feature_flags/development/bitbucket_server_convert_mentions_to_users.yml b/config/feature_flags/development/bitbucket_server_convert_mentions_to_users.yml new file mode 100644 index 00000000000000..90ea8d56c0acca --- /dev/null +++ b/config/feature_flags/development/bitbucket_server_convert_mentions_to_users.yml @@ -0,0 +1,8 @@ +--- +name: bitbucket_server_convert_mentions_to_users +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/139097 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/434453 +milestone: '16.7' +type: development +group: group::import and integrate +default_enabled: false diff --git a/spec/workers/gitlab/bitbucket_server_import/stage/import_repository_worker_spec.rb b/spec/workers/gitlab/bitbucket_server_import/stage/import_repository_worker_spec.rb index 619202a1c46f75..1531b30089c9c2 100644 --- a/spec/workers/gitlab/bitbucket_server_import/stage/import_repository_worker_spec.rb +++ b/spec/workers/gitlab/bitbucket_server_import/stage/import_repository_worker_spec.rb @@ -24,9 +24,9 @@ worker.perform(project.id) end - context 'when the bitbucket_server_cache_users flag is disabled' do + context 'when the bitbucket_server_convert_mentions_to_users flag is disabled' do before do - stub_feature_flags(bitbucket_server_cache_users: false) + stub_feature_flags(bitbucket_server_convert_mentions_to_users: false) end it 'skips the user import and schedules the next stage' do -- GitLab From 7c6fe4831c2fc24253b82ef37160b2c42c9b5e69 Mon Sep 17 00:00:00 2001 From: maddievn Date: Fri, 8 Dec 2023 16:17:08 +0200 Subject: [PATCH 03/18] Add a limit --- lib/bitbucket_server/client.rb | 2 +- spec/lib/bitbucket_server/client_spec.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/bitbucket_server/client.rb b/lib/bitbucket_server/client.rb index 5f09f029d18f71..a7f30eed8a6a7d 100644 --- a/lib/bitbucket_server/client.rb +++ b/lib/bitbucket_server/client.rb @@ -31,7 +31,7 @@ def repos(page_offset: 0, limit: nil, filter: nil) def users(project) path = "/projects/#{project}/permissions/users" - get_collection(path, :user) + get_collection(path, :user, limit: 10_000) end def create_branch(project_key, repo, branch_name, sha) diff --git a/spec/lib/bitbucket_server/client_spec.rb b/spec/lib/bitbucket_server/client_spec.rb index b11367ae01ccb4..f345718b830a1f 100644 --- a/spec/lib/bitbucket_server/client_spec.rb +++ b/spec/lib/bitbucket_server/client_spec.rb @@ -84,7 +84,7 @@ let(:path) { "/projects/#{project}/permissions/users" } it 'requests a collection' do - expect(BitbucketServer::Paginator).to receive(:new).with(anything, path, :user, page_offset: 0, limit: nil) + expect(BitbucketServer::Paginator).to receive(:new).with(anything, path, :user, page_offset: 0, limit: 10_000) subject.users(project) end -- GitLab From 807216954431643481d8ed5f0fc9698e2ef179a8 Mon Sep 17 00:00:00 2001 From: maddievn Date: Mon, 11 Dec 2023 09:54:19 +0200 Subject: [PATCH 04/18] Loop through all users --- lib/bitbucket_server/client.rb | 4 +-- .../importers/users_importer.rb | 17 +++++++--- spec/lib/bitbucket_server/client_spec.rb | 11 ++++++- .../importers/users_importer_spec.rb | 33 ++++++++++++------- 4 files changed, 46 insertions(+), 19 deletions(-) diff --git a/lib/bitbucket_server/client.rb b/lib/bitbucket_server/client.rb index a7f30eed8a6a7d..d27719390ea432 100644 --- a/lib/bitbucket_server/client.rb +++ b/lib/bitbucket_server/client.rb @@ -29,9 +29,9 @@ def repos(page_offset: 0, limit: nil, filter: nil) get_collection(path, :repo, page_offset: page_offset, limit: limit) end - def users(project) + def users(project, page_offset: 0, limit: nil) path = "/projects/#{project}/permissions/users" - get_collection(path, :user, limit: 10_000) + get_collection(path, :user, page_offset: page_offset, limit: limit) end def create_branch(project_key, repo, branch_name, sha) diff --git a/lib/gitlab/bitbucket_server_import/importers/users_importer.rb b/lib/gitlab/bitbucket_server_import/importers/users_importer.rb index 777b677449d9a4..6298151e91b5ef 100644 --- a/lib/gitlab/bitbucket_server_import/importers/users_importer.rb +++ b/lib/gitlab/bitbucket_server_import/importers/users_importer.rb @@ -17,12 +17,21 @@ def initialize(project) def execute log_info(import_stage: 'import_users', message: 'starting') - users = client.users(project_key) + page = 1 - users.each_slice(BATCH_SIZE) do |batch| - log_info(import_stage: 'import_users', message: "caching batch of #{batch.count} users") + loop do + log_info( + import_stage: 'import_users', + message: "importing page #{page} using batch size #{BATCH_SIZE}" + ) - batch.each { |user| cache_source_user(user) } + users = client.users(project_key, page_offset: page, limit: BATCH_SIZE).to_a + + break if users.empty? + + users.each { |user| cache_source_user(user) } + + page += 1 end log_info(import_stage: 'import_users', message: 'finished') diff --git a/spec/lib/bitbucket_server/client_spec.rb b/spec/lib/bitbucket_server/client_spec.rb index f345718b830a1f..0d027234a0d393 100644 --- a/spec/lib/bitbucket_server/client_spec.rb +++ b/spec/lib/bitbucket_server/client_spec.rb @@ -84,10 +84,19 @@ let(:path) { "/projects/#{project}/permissions/users" } it 'requests a collection' do - expect(BitbucketServer::Paginator).to receive(:new).with(anything, path, :user, page_offset: 0, limit: 10_000) + expect(BitbucketServer::Paginator).to receive(:new).with(anything, path, :user, page_offset: 0, limit: nil) subject.users(project) end + + it 'requests a collection with offset and limit' do + offset = 10 + limit = 100 + + expect(BitbucketServer::Paginator).to receive(:new).with(anything, path, :user, page_offset: offset, limit: limit) + + subject.users(project, page_offset: offset, limit: limit) + end end describe '#create_branch' do diff --git a/spec/lib/gitlab/bitbucket_server_import/importers/users_importer_spec.rb b/spec/lib/gitlab/bitbucket_server_import/importers/users_importer_spec.rb index 33803718ffd6bb..6bee47ea819a19 100644 --- a/spec/lib/gitlab/bitbucket_server_import/importers/users_importer_spec.rb +++ b/spec/lib/gitlab/bitbucket_server_import/importers/users_importer_spec.rb @@ -4,6 +4,7 @@ RSpec.describe Gitlab::BitbucketServerImport::Importers::UsersImporter, feature_category: :importers do let(:users_mapper) { BitbucketServer::UsersMapper.new(project) } + let(:logger) { Gitlab::BitbucketServerImport::Logger } let_it_be(:project) do create(:project, :with_import_url, :import_started, :empty_repo, @@ -14,22 +15,26 @@ ) end + let(:user_1) do + BitbucketServer::Representation::User.new( + { 'user' => { 'emailAddress' => 'email1', 'slug' => 'username1' } } + ) + end + + let(:user_2) do + BitbucketServer::Representation::User.new( + { 'user' => { 'emailAddress' => 'email2', 'slug' => 'username2' } } + ) + end + before do allow(BitbucketServer::UsersMapper).to receive(:new).and_return(users_mapper) stub_const("#{described_class}::BATCH_SIZE", 1) - allow(Gitlab::BitbucketServerImport::Logger).to receive(:info).and_call_original allow_next_instance_of(BitbucketServer::Client) do |client| - allow(client).to receive(:users).with('key').and_return( - [ - BitbucketServer::Representation::User.new( - { 'user' => { 'emailAddress' => 'email1', 'slug' => 'username1' } } - ), - BitbucketServer::Representation::User.new( - { 'user' => { 'emailAddress' => 'email2', 'slug' => 'username2' } } - ) - ] - ) + allow(client).to receive(:users).with('key', limit: 1, page_offset: 1).and_return([user_1]) + allow(client).to receive(:users).with('key', limit: 1, page_offset: 2).and_return([user_2]) + allow(client).to receive(:users).with('key', limit: 1, page_offset: 3).and_return([]) end end @@ -37,7 +42,11 @@ describe '#execute' do it 'writes the username and email to cache for every user in batches' do - expect(Gitlab::BitbucketServerImport::Logger).to receive(:info).exactly(4).times + expect(logger).to receive(:info).with(hash_including(message: 'starting')) + expect(logger).to receive(:info).with(hash_including(message: 'importing page 1 using batch size 1')) + expect(logger).to receive(:info).with(hash_including(message: 'importing page 2 using batch size 1')) + expect(logger).to receive(:info).with(hash_including(message: 'importing page 3 using batch size 1')) + expect(logger).to receive(:info).with(hash_including(message: 'finished')) expect(users_mapper).to receive(:write_source_email_to_cache).and_call_original.twice -- GitLab From b56fec5d50601b693d0a500b95f4c17907631179 Mon Sep 17 00:00:00 2001 From: maddievn Date: Tue, 12 Dec 2023 12:19:06 +0200 Subject: [PATCH 05/18] Use write_multiple instead of writing each user --- .../stage/import_users_worker.rb | 2 +- lib/bitbucket_server/client.rb | 4 +- lib/bitbucket_server/users_mapper.rb | 29 -------------- .../importers/users_importer.rb | 17 +++++---- .../bitbucket_server_import/user_caching.rb | 13 +++++++ .../lib/bitbucket_server/users_mapper_spec.rb | 38 ------------------- .../importers/users_importer_spec.rb | 30 +++++++++------ 7 files changed, 44 insertions(+), 89 deletions(-) delete mode 100644 lib/bitbucket_server/users_mapper.rb create mode 100644 lib/gitlab/bitbucket_server_import/user_caching.rb delete mode 100644 spec/lib/bitbucket_server/users_mapper_spec.rb diff --git a/app/workers/gitlab/bitbucket_server_import/stage/import_users_worker.rb b/app/workers/gitlab/bitbucket_server_import/stage/import_users_worker.rb index 203ca50ea0f884..dd18139fc9e3dc 100644 --- a/app/workers/gitlab/bitbucket_server_import/stage/import_users_worker.rb +++ b/app/workers/gitlab/bitbucket_server_import/stage/import_users_worker.rb @@ -3,7 +3,7 @@ module Gitlab module BitbucketServerImport module Stage - class ImportUsersWorker # rubocop:disable Scalability/IdempotentWorker -- users should be cached once only + class ImportUsersWorker # rubocop:disable Scalability/IdempotentWorker -- ImportPullRequestsWorker is not idempotent include StageMethods private diff --git a/lib/bitbucket_server/client.rb b/lib/bitbucket_server/client.rb index d27719390ea432..432928b0591c4f 100644 --- a/lib/bitbucket_server/client.rb +++ b/lib/bitbucket_server/client.rb @@ -29,8 +29,8 @@ def repos(page_offset: 0, limit: nil, filter: nil) get_collection(path, :repo, page_offset: page_offset, limit: limit) end - def users(project, page_offset: 0, limit: nil) - path = "/projects/#{project}/permissions/users" + def users(project_key, page_offset: 0, limit: nil) + path = "/projects/#{project_key}/permissions/users" get_collection(path, :user, page_offset: page_offset, limit: limit) end diff --git a/lib/bitbucket_server/users_mapper.rb b/lib/bitbucket_server/users_mapper.rb deleted file mode 100644 index f24d4465692500..00000000000000 --- a/lib/bitbucket_server/users_mapper.rb +++ /dev/null @@ -1,29 +0,0 @@ -# frozen_string_literal: true - -module BitbucketServer - class UsersMapper - include Gitlab::Utils::StrongMemoize - - SOURCE_USER_CACHE_KEY = 'bitbucket_server/project/%s/source/username/%s' - - def initialize(project) - @project_id = project.id - end - - attr_reader :project_id - - def write_source_email_to_cache(username, email) - ::Gitlab::Cache::Import::Caching.write(cache_key(username), email) - end - - def read_source_email_from_cache(username) - ::Gitlab::Cache::Import::Caching.read(cache_key(username)) - end - - private - - def cache_key(username) - format(SOURCE_USER_CACHE_KEY, project_id, username) - end - end -end diff --git a/lib/gitlab/bitbucket_server_import/importers/users_importer.rb b/lib/gitlab/bitbucket_server_import/importers/users_importer.rb index 6298151e91b5ef..bd7e84a3b0c28e 100644 --- a/lib/gitlab/bitbucket_server_import/importers/users_importer.rb +++ b/lib/gitlab/bitbucket_server_import/importers/users_importer.rb @@ -5,14 +5,16 @@ module BitbucketServerImport module Importers class UsersImporter include Loggable + include UserCaching BATCH_SIZE = 100 def initialize(project) @project = project + @project_id = project.id end - attr_reader :project + attr_reader :project, :project_id def execute log_info(import_stage: 'import_users', message: 'starting') @@ -29,7 +31,7 @@ def execute break if users.empty? - users.each { |user| cache_source_user(user) } + cache_users(users) page += 1 end @@ -39,12 +41,13 @@ def execute private - def cache_source_user(user) - users_mapper.write_source_email_to_cache(user.username, user.email) - end + def cache_users(users) + users_hash = users.each_with_object({}) do |user, hash| + cache_key = source_user_cache_key(project_id, user.username) + hash[cache_key] = user.email + end - def users_mapper - @users_mapper ||= BitbucketServer::UsersMapper.new(project) + ::Gitlab::Cache::Import::Caching.write_multiple(users_hash) end def client diff --git a/lib/gitlab/bitbucket_server_import/user_caching.rb b/lib/gitlab/bitbucket_server_import/user_caching.rb new file mode 100644 index 00000000000000..0f0169122c513d --- /dev/null +++ b/lib/gitlab/bitbucket_server_import/user_caching.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +module Gitlab + module BitbucketServerImport + module UserCaching + SOURCE_USER_CACHE_KEY = 'bitbucket_server/project/%s/source/username/%s' + + def source_user_cache_key(project_id, username) + format(SOURCE_USER_CACHE_KEY, project_id, username) + end + end + end +end diff --git a/spec/lib/bitbucket_server/users_mapper_spec.rb b/spec/lib/bitbucket_server/users_mapper_spec.rb deleted file mode 100644 index 79bb3f343d7089..00000000000000 --- a/spec/lib/bitbucket_server/users_mapper_spec.rb +++ /dev/null @@ -1,38 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe BitbucketServer::UsersMapper, feature_category: :importers do - let_it_be(:project) { create(:project) } - let(:username) { 'test_user' } - let(:email) { 'test@email.com' } - - subject(:users_mapper) { described_class.new(project) } - - describe '#write_source_email_to_cache' do - it 'caches provided source username and email' do - expect(Gitlab::Cache::Import::Caching).to receive(:write) - .with("bitbucket_server/project/#{project.id}/source/username/#{username}", email) - - users_mapper.write_source_email_to_cache(username, email) - end - end - - describe '#read_source_email_from_cache', :clean_gitlab_redis_cache do - context 'when it has been cached' do - before do - users_mapper.write_source_email_to_cache(username, email) - end - - it 'returns the matching email' do - expect(users_mapper.read_source_email_from_cache(username)).to eq(email) - end - end - - context 'when it has not been cached' do - it 'returns the matching email' do - expect(users_mapper.read_source_email_from_cache(username)).to be_nil - end - end - end -end diff --git a/spec/lib/gitlab/bitbucket_server_import/importers/users_importer_spec.rb b/spec/lib/gitlab/bitbucket_server_import/importers/users_importer_spec.rb index 6bee47ea819a19..33d6ab94513640 100644 --- a/spec/lib/gitlab/bitbucket_server_import/importers/users_importer_spec.rb +++ b/spec/lib/gitlab/bitbucket_server_import/importers/users_importer_spec.rb @@ -3,7 +3,6 @@ require 'spec_helper' RSpec.describe Gitlab::BitbucketServerImport::Importers::UsersImporter, feature_category: :importers do - let(:users_mapper) { BitbucketServer::UsersMapper.new(project) } let(:logger) { Gitlab::BitbucketServerImport::Logger } let_it_be(:project) do @@ -27,14 +26,19 @@ ) end + let(:user_3) do + BitbucketServer::Representation::User.new( + { 'user' => { 'emailAddress' => 'email3', 'slug' => 'username3' } } + ) + end + before do - allow(BitbucketServer::UsersMapper).to receive(:new).and_return(users_mapper) - stub_const("#{described_class}::BATCH_SIZE", 1) + stub_const("#{described_class}::BATCH_SIZE", 2) allow_next_instance_of(BitbucketServer::Client) do |client| - allow(client).to receive(:users).with('key', limit: 1, page_offset: 1).and_return([user_1]) - allow(client).to receive(:users).with('key', limit: 1, page_offset: 2).and_return([user_2]) - allow(client).to receive(:users).with('key', limit: 1, page_offset: 3).and_return([]) + allow(client).to receive(:users).with('key', limit: 2, page_offset: 1).and_return([user_1, user_2]) + allow(client).to receive(:users).with('key', limit: 2, page_offset: 2).and_return([user_3]) + allow(client).to receive(:users).with('key', limit: 2, page_offset: 3).and_return([]) end end @@ -43,17 +47,19 @@ describe '#execute' do it 'writes the username and email to cache for every user in batches' do expect(logger).to receive(:info).with(hash_including(message: 'starting')) - expect(logger).to receive(:info).with(hash_including(message: 'importing page 1 using batch size 1')) - expect(logger).to receive(:info).with(hash_including(message: 'importing page 2 using batch size 1')) - expect(logger).to receive(:info).with(hash_including(message: 'importing page 3 using batch size 1')) + expect(logger).to receive(:info).with(hash_including(message: 'importing page 1 using batch size 2')) + expect(logger).to receive(:info).with(hash_including(message: 'importing page 2 using batch size 2')) + expect(logger).to receive(:info).with(hash_including(message: 'importing page 3 using batch size 2')) expect(logger).to receive(:info).with(hash_including(message: 'finished')) - expect(users_mapper).to receive(:write_source_email_to_cache).and_call_original.twice + expect(Gitlab::Cache::Import::Caching).to receive(:write_multiple).and_call_original.twice importer.execute - expect(users_mapper.read_source_email_from_cache('username1')).to eq('email1') - expect(users_mapper.read_source_email_from_cache('username2')).to eq('email2') + cache_key_prefix = "bitbucket_server/project/#{project.id}/source/username" + expect(Gitlab::Cache::Import::Caching.read("#{cache_key_prefix}/username1")).to eq('email1') + expect(Gitlab::Cache::Import::Caching.read("#{cache_key_prefix}/username2")).to eq('email2') + expect(Gitlab::Cache::Import::Caching.read("#{cache_key_prefix}/username3")).to eq('email3') end end end -- GitLab From 07934ca886185b5402f3bc64830c55a82c10e1ce Mon Sep 17 00:00:00 2001 From: maddievn Date: Thu, 7 Dec 2023 15:25:16 +0200 Subject: [PATCH 06/18] Cache users from Bitbucket Server Changelog: added --- .../bitbucket_server_cache_users.yml | 8 ++++ lib/bitbucket_server/users_mapper.rb | 29 ++++++++++++++ .../lib/bitbucket_server/users_mapper_spec.rb | 38 +++++++++++++++++++ 3 files changed, 75 insertions(+) create mode 100644 config/feature_flags/development/bitbucket_server_cache_users.yml create mode 100644 lib/bitbucket_server/users_mapper.rb create mode 100644 spec/lib/bitbucket_server/users_mapper_spec.rb diff --git a/config/feature_flags/development/bitbucket_server_cache_users.yml b/config/feature_flags/development/bitbucket_server_cache_users.yml new file mode 100644 index 00000000000000..1458521151c219 --- /dev/null +++ b/config/feature_flags/development/bitbucket_server_cache_users.yml @@ -0,0 +1,8 @@ +--- +name: bitbucket_server_cache_users +introduced_by_url: +rollout_issue_url: +milestone: '16.7' +type: development +group: group::import and integrate +default_enabled: false diff --git a/lib/bitbucket_server/users_mapper.rb b/lib/bitbucket_server/users_mapper.rb new file mode 100644 index 00000000000000..f24d4465692500 --- /dev/null +++ b/lib/bitbucket_server/users_mapper.rb @@ -0,0 +1,29 @@ +# frozen_string_literal: true + +module BitbucketServer + class UsersMapper + include Gitlab::Utils::StrongMemoize + + SOURCE_USER_CACHE_KEY = 'bitbucket_server/project/%s/source/username/%s' + + def initialize(project) + @project_id = project.id + end + + attr_reader :project_id + + def write_source_email_to_cache(username, email) + ::Gitlab::Cache::Import::Caching.write(cache_key(username), email) + end + + def read_source_email_from_cache(username) + ::Gitlab::Cache::Import::Caching.read(cache_key(username)) + end + + private + + def cache_key(username) + format(SOURCE_USER_CACHE_KEY, project_id, username) + end + end +end diff --git a/spec/lib/bitbucket_server/users_mapper_spec.rb b/spec/lib/bitbucket_server/users_mapper_spec.rb new file mode 100644 index 00000000000000..79bb3f343d7089 --- /dev/null +++ b/spec/lib/bitbucket_server/users_mapper_spec.rb @@ -0,0 +1,38 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe BitbucketServer::UsersMapper, feature_category: :importers do + let_it_be(:project) { create(:project) } + let(:username) { 'test_user' } + let(:email) { 'test@email.com' } + + subject(:users_mapper) { described_class.new(project) } + + describe '#write_source_email_to_cache' do + it 'caches provided source username and email' do + expect(Gitlab::Cache::Import::Caching).to receive(:write) + .with("bitbucket_server/project/#{project.id}/source/username/#{username}", email) + + users_mapper.write_source_email_to_cache(username, email) + end + end + + describe '#read_source_email_from_cache', :clean_gitlab_redis_cache do + context 'when it has been cached' do + before do + users_mapper.write_source_email_to_cache(username, email) + end + + it 'returns the matching email' do + expect(users_mapper.read_source_email_from_cache(username)).to eq(email) + end + end + + context 'when it has not been cached' do + it 'returns the matching email' do + expect(users_mapper.read_source_email_from_cache(username)).to be_nil + end + end + end +end -- GitLab From f449e5acf6e898243b7f7730763499d255edae05 Mon Sep 17 00:00:00 2001 From: maddievn Date: Fri, 8 Dec 2023 10:13:08 +0200 Subject: [PATCH 07/18] Add details for feature flag --- .../development/bitbucket_server_cache_users.yml | 8 -------- 1 file changed, 8 deletions(-) delete mode 100644 config/feature_flags/development/bitbucket_server_cache_users.yml diff --git a/config/feature_flags/development/bitbucket_server_cache_users.yml b/config/feature_flags/development/bitbucket_server_cache_users.yml deleted file mode 100644 index 1458521151c219..00000000000000 --- a/config/feature_flags/development/bitbucket_server_cache_users.yml +++ /dev/null @@ -1,8 +0,0 @@ ---- -name: bitbucket_server_cache_users -introduced_by_url: -rollout_issue_url: -milestone: '16.7' -type: development -group: group::import and integrate -default_enabled: false -- GitLab From 32a8f0e5c5c24676a6750d2c6c0ea5ad020f6d86 Mon Sep 17 00:00:00 2001 From: maddievn Date: Fri, 8 Dec 2023 10:53:41 +0200 Subject: [PATCH 08/18] Convert mentions on pull request description and notes Changelog: changed --- .../importers/pull_request_importer.rb | 19 ++- .../importers/pull_request_notes_importer.rb | 14 ++- .../mentions_converter.rb | 47 ++++++++ .../importers/pull_request_importer_spec.rb | 14 +++ .../pull_request_notes_importer_spec.rb | 33 ++++++ .../mentions_converter_spec.rb | 110 ++++++++++++++++++ 6 files changed, 230 insertions(+), 7 deletions(-) create mode 100644 lib/gitlab/bitbucket_server_import/mentions_converter.rb create mode 100644 spec/lib/gitlab/bitbucket_server_import/mentions_converter_spec.rb diff --git a/lib/gitlab/bitbucket_server_import/importers/pull_request_importer.rb b/lib/gitlab/bitbucket_server_import/importers/pull_request_importer.rb index 0d4de385f5eee9..419598c90e8ca3 100644 --- a/lib/gitlab/bitbucket_server_import/importers/pull_request_importer.rb +++ b/lib/gitlab/bitbucket_server_import/importers/pull_request_importer.rb @@ -10,6 +10,7 @@ def initialize(project, hash) @project = project @formatter = Gitlab::ImportFormatter.new @user_finder = UserFinder.new(project) + @mentions_converter = Gitlab::BitbucketServerImport::MentionsConverter.new(project) # Object should behave as a object so we can remove object.is_a?(Hash) check # This will be fixed in https://gitlab.com/gitlab-org/gitlab/-/issues/412328 @@ -19,10 +20,6 @@ def initialize(project, hash) def execute log_info(import_stage: 'import_pull_request', message: 'starting', iid: object[:iid]) - description = '' - description += author_line - description += object[:description] if object[:description] - attributes = { iid: object[:iid], title: object[:title], @@ -49,7 +46,19 @@ def execute private - attr_reader :object, :project, :formatter, :user_finder + attr_reader :object, :project, :formatter, :user_finder, :mentions_converter + + def description + description = '' + description += author_line + description += object[:description] if object[:description] + + if Feature.enabled?(:bitbucket_server_convert_mentions_to_users, project.creator) + description = mentions_converter.convert(description) + end + + description + end def author_line return '' if user_finder.uid(object) diff --git a/lib/gitlab/bitbucket_server_import/importers/pull_request_notes_importer.rb b/lib/gitlab/bitbucket_server_import/importers/pull_request_notes_importer.rb index aa54f19315ae87..6e3287e0f1e810 100644 --- a/lib/gitlab/bitbucket_server_import/importers/pull_request_notes_importer.rb +++ b/lib/gitlab/bitbucket_server_import/importers/pull_request_notes_importer.rb @@ -188,12 +188,18 @@ def pull_request_comment_attributes(comment) note = "*By #{comment.author_username} (#{comment.author_email})*\n\n" end + comment_note = if Feature.enabled?(:bitbucket_server_convert_mentions_to_users, project.creator) + mentions_converter.convert(comment.note) + else + comment.note + end + note += # Provide some context for replying if comment.parent_comment - "> #{comment.parent_comment.note.truncate(80)}\n\n#{comment.note}" + "> #{comment.parent_comment.note.truncate(80)}\n\n#{comment_note}" else - comment.note + comment_note end { @@ -216,6 +222,10 @@ def project_key def repository_slug project.import_data.data['repo_slug'] end + + def mentions_converter + Gitlab::BitbucketServerImport::MentionsConverter.new(project) + end end end end diff --git a/lib/gitlab/bitbucket_server_import/mentions_converter.rb b/lib/gitlab/bitbucket_server_import/mentions_converter.rb new file mode 100644 index 00000000000000..580104ca4873e1 --- /dev/null +++ b/lib/gitlab/bitbucket_server_import/mentions_converter.rb @@ -0,0 +1,47 @@ +# frozen_string_literal: true + +module Gitlab + module BitbucketServerImport + class MentionsConverter + MENTIONS_REGEX = /@([\w\d_-]*)/ + PLACEHOLDER = 'PLACEHOLDER_TEXT-' + + attr_reader :project, :users_mapper + + def initialize(project) + @project = project + @users_mapper = BitbucketServer::UsersMapper.new(project) + end + + def convert(text) + mentions = text.scan(MENTIONS_REGEX).flatten + + return text if mentions.blank? + + text = text.dup + altered_mentions = [] + + mentions.each do |mention| + cached_email = users_mapper.read_source_email_from_cache(mention) + + if cached_email + user = User.find_by_any_email(cached_email, confirmed: true) + + if user + altered_mentions << ["@#{mention}", "#{PLACEHOLDER}#{user.username}"] + next + end + end + + altered_mentions << ["@#{mention}", "`#{mention}`"] + end + + altered_mentions.each do |original_mention, altered_mention| + text.sub!(original_mention, altered_mention) + end + + text.gsub(PLACEHOLDER, '@') + end + end + end +end diff --git a/spec/lib/gitlab/bitbucket_server_import/importers/pull_request_importer_spec.rb b/spec/lib/gitlab/bitbucket_server_import/importers/pull_request_importer_spec.rb index 1ae68f9efb8610..1ea93dd8e0b298 100644 --- a/spec/lib/gitlab/bitbucket_server_import/importers/pull_request_importer_spec.rb +++ b/spec/lib/gitlab/bitbucket_server_import/importers/pull_request_importer_spec.rb @@ -18,6 +18,8 @@ it 'imports the merge request correctly' do expect_next(Gitlab::Import::MergeRequestCreator, project).to receive(:execute).and_call_original expect_next(Gitlab::BitbucketServerImport::UserFinder, project).to receive(:author_id).and_call_original + expect_next(Gitlab::BitbucketServerImport::MentionsConverter, project).to receive(:convert).and_call_original + expect { importer.execute }.to change { MergeRequest.count }.by(1) merge_request = project.merge_requests.find_by_iid(pull_request.iid) @@ -34,6 +36,18 @@ ) end + context 'when the `bitbucket_server_convert_mentions_to_users` flag is disabled' do + before do + stub_feature_flags(bitbucket_server_convert_mentions_to_users: false) + end + + it 'does not convert mentions' do + expect_next(Gitlab::BitbucketServerImport::MentionsConverter, project).not_to receive(:convert) + + importer.execute + end + end + context 'when the `bitbucket_server_user_mapping_by_username` flag is disabled' do before do stub_feature_flags(bitbucket_server_user_mapping_by_username: false) diff --git a/spec/lib/gitlab/bitbucket_server_import/importers/pull_request_notes_importer_spec.rb b/spec/lib/gitlab/bitbucket_server_import/importers/pull_request_notes_importer_spec.rb index f063545a44c3b4..9c98347543d316 100644 --- a/spec/lib/gitlab/bitbucket_server_import/importers/pull_request_notes_importer_spec.rb +++ b/spec/lib/gitlab/bitbucket_server_import/importers/pull_request_notes_importer_spec.rb @@ -17,6 +17,7 @@ let_it_be(:pull_request_data) { Gitlab::Json.parse(fixture_file('importers/bitbucket_server/pull_request.json')) } let_it_be(:pull_request) { BitbucketServer::Representation::PullRequest.new(pull_request_data) } let_it_be(:note_author) { create(:user, username: 'note_author', email: 'note_author@example.org') } + let(:mentions_converter) { Gitlab::BitbucketServerImport::MentionsConverter.new(project) } let!(:pull_request_author) do create(:user, username: 'pull_request_author', email: 'pull_request_author@example.org') @@ -79,6 +80,10 @@ def expect_log(stage:, message:) .to receive(:info).with(include(import_stage: stage, message: message)) end + before do + allow(Gitlab::BitbucketServerImport::MentionsConverter).to receive(:new).and_return(mentions_converter) + end + subject(:importer) { described_class.new(project.reload, pull_request.to_hash) } describe '#execute' do @@ -126,6 +131,8 @@ def expect_log(stage:, message:) end it 'imports the stand alone comments' do + expect(mentions_converter).to receive(:convert).and_call_original + expect { subject.execute }.to change { Note.count }.by(1) expect(merge_request.notes.count).to eq(1) @@ -137,6 +144,18 @@ def expect_log(stage:, message:) ) end + context 'when the `bitbucket_server_convert_mentions_to_users` flag is disabled' do + before do + stub_feature_flags(bitbucket_server_convert_mentions_to_users: false) + end + + it 'does not convert mentions' do + expect(mentions_converter).not_to receive(:convert).and_call_original + + subject.execute + end + end + it 'logs its progress' do expect_log(stage: 'import_standalone_pr_comments', message: 'starting') expect_log(stage: 'import_standalone_pr_comments', message: 'finished') @@ -194,6 +213,8 @@ def expect_log(stage:, message:) end it 'imports the threaded discussion' do + expect(mentions_converter).to receive(:convert).and_call_original.twice + expect { subject.execute }.to change { Note.count }.by(2) expect(merge_request.discussions.count).to eq(1) @@ -217,6 +238,18 @@ def expect_log(stage:, message:) expect(reply_note.position.new_line).to eq(pr_inline_note.new_pos) end + context 'when the `bitbucket_server_convert_mentions_to_users` flag is disabled' do + before do + stub_feature_flags(bitbucket_server_convert_mentions_to_users: false) + end + + it 'does not convert mentions' do + expect(mentions_converter).not_to receive(:convert) + + subject.execute + end + end + it 'logs its progress' do expect_log(stage: 'import_inline_comments', message: 'starting') expect_log(stage: 'import_inline_comments', message: 'finished') diff --git a/spec/lib/gitlab/bitbucket_server_import/mentions_converter_spec.rb b/spec/lib/gitlab/bitbucket_server_import/mentions_converter_spec.rb new file mode 100644 index 00000000000000..a990bd4690b338 --- /dev/null +++ b/spec/lib/gitlab/bitbucket_server_import/mentions_converter_spec.rb @@ -0,0 +1,110 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::BitbucketServerImport::MentionsConverter, :clean_gitlab_redis_cache, feature_category: :importers do + let_it_be(:project) { create(:project) } + let(:users_mapper) { BitbucketServer::UsersMapper.new(project) } + let(:text) { 'text without mentions' } + + subject(:converted_text) { described_class.new(project).convert(text) } + + describe '#convert' do + context 'when the text has no mentions' do + it 'does not change the text' do + expect(converted_text).to eq(text) + end + end + + context 'when the text has a mention' do + let(:text) { 'mentioning @john' } + + context 'when the mention has matching cached email' do + before do + users_mapper.write_source_email_to_cache('john', 'john@example.com') + end + + context 'when a user with the email does not exist on gitlab' do + it 'puts the mention in backticks' do + expect(converted_text).to eq('mentioning `john`') + end + end + + context 'when a user with the same email exists on gitlab' do + let_it_be(:user) { create(:user, username: 'johndoe', email: 'john@example.com') } + + it "replaces the mention with the user's username" do + expect(converted_text).to eq('mentioning @johndoe') + end + end + + context 'when a user with the same username but not email exists on gitlab' do + let_it_be(:user) { create(:user, username: 'john') } + + it 'puts the mention in backticks' do + expect(converted_text).to eq('mentioning `john`') + end + end + end + + context 'when there is cached email but not for the mentioned username' do + before do + users_mapper.write_source_email_to_cache('jane', 'jane@example.com') + end + + it 'puts the mention in backticks' do + expect(converted_text).to eq('mentioning `john`') + end + + context 'when a user with the same email exists on gitlab' do + let_it_be(:user) { create(:user, username: 'jane', email: 'jane@example.com') } + + it 'puts the mention in backticks' do + expect(converted_text).to eq('mentioning `john`') + end + end + end + + context 'when the mention has digits, underscores, uppercase and hyphens' do + let(:text) { '@john_DOE-123' } + let_it_be(:user) { create(:user, username: 'johndoe', email: 'john@example.com') } + + before do + users_mapper.write_source_email_to_cache('john_DOE-123', 'john@example.com') + end + + it "replaces the mention with the user's username" do + expect(converted_text).to eq('@johndoe') + end + end + + context 'when no emails are cached' do + it 'puts the mention in backticks' do + expect(converted_text).to eq('mentioning `john`') + end + end + end + + context 'when the text has multiple mentions' do + let(:text) { "@john, @jane-doe and @johndoe123 with \n@john again on a newline" } + + context 'if none of the mentions have matching cached emails and users' do + it 'puts every mention in backticks' do + expect(converted_text).to eq("`john`, `jane-doe` and `johndoe123` with \n`john` again on a newline") + end + end + + context 'if one of the mentions have matching user' do + let_it_be(:user) { create(:user, username: 'johndoe', email: 'john@example.com') } + + before do + users_mapper.write_source_email_to_cache('john', 'john@example.com') + end + + it 'replaces all mentions with the username and puts rest of mentions in backticks' do + expect(converted_text).to eq("@johndoe, `jane-doe` and `johndoe123` with \n@johndoe again on a newline") + end + end + end + end +end -- GitLab From 8f93e4135d24f9392025a7012f917dfd519b868c Mon Sep 17 00:00:00 2001 From: maddievn Date: Fri, 8 Dec 2023 11:57:41 +0200 Subject: [PATCH 09/18] Handle emails --- .../mentions_converter.rb | 19 ++++++++++++++++--- .../mentions_converter_spec.rb | 8 ++++++++ 2 files changed, 24 insertions(+), 3 deletions(-) diff --git a/lib/gitlab/bitbucket_server_import/mentions_converter.rb b/lib/gitlab/bitbucket_server_import/mentions_converter.rb index 580104ca4873e1..9dd553e116816f 100644 --- a/lib/gitlab/bitbucket_server_import/mentions_converter.rb +++ b/lib/gitlab/bitbucket_server_import/mentions_converter.rb @@ -4,7 +4,9 @@ module Gitlab module BitbucketServerImport class MentionsConverter MENTIONS_REGEX = /@([\w\d_-]*)/ + EMAIL_REGEX = /\b[A-Z0-9._%+-]+@[A-Z0-9.-]+\.[A-Z]{2,4}\b/i PLACEHOLDER = 'PLACEHOLDER_TEXT-' + EMAIL_PLACEHOLDER = '~PLACEHOLDER~' attr_reader :project, :users_mapper @@ -14,11 +16,22 @@ def initialize(project) end def convert(text) - mentions = text.scan(MENTIONS_REGEX).flatten + text = text.dup + emails = text.scan(EMAIL_REGEX) - return text if mentions.blank? + emails.each_with_index { |email, index| text.gsub!(email, "~PLACEHOLDER-#{index}-EMAIL~") } - text = text.dup + text = replace_mentions(text) + + emails.each_with_index { |email, index| text.gsub!("~PLACEHOLDER-#{index}-EMAIL~", email) } + + text + end + + private + + def replace_mentions(text) + mentions = text.scan(MENTIONS_REGEX).flatten altered_mentions = [] mentions.each do |mention| diff --git a/spec/lib/gitlab/bitbucket_server_import/mentions_converter_spec.rb b/spec/lib/gitlab/bitbucket_server_import/mentions_converter_spec.rb index a990bd4690b338..1d304af6d2c4ac 100644 --- a/spec/lib/gitlab/bitbucket_server_import/mentions_converter_spec.rb +++ b/spec/lib/gitlab/bitbucket_server_import/mentions_converter_spec.rb @@ -78,6 +78,14 @@ end end + context 'when the mention has emails' do + let(:text) { "@john's email is john@gmail.com and @jane's email is jane@example.com." } + + it 'does not alter the emails' do + expect(converted_text).to eq("`john`'s email is john@gmail.com and `jane`'s email is jane@example.com.") + end + end + context 'when no emails are cached' do it 'puts the mention in backticks' do expect(converted_text).to eq('mentioning `john`') -- GitLab From 563a77f1f1547bcdfe0ee1d695775774a6ff0e39 Mon Sep 17 00:00:00 2001 From: maddievn Date: Fri, 8 Dec 2023 16:05:23 +0200 Subject: [PATCH 10/18] Handle single @s --- lib/gitlab/bitbucket_server_import/mentions_converter.rb | 5 ++--- .../bitbucket_server_import/mentions_converter_spec.rb | 2 +- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/lib/gitlab/bitbucket_server_import/mentions_converter.rb b/lib/gitlab/bitbucket_server_import/mentions_converter.rb index 9dd553e116816f..fd50899893546d 100644 --- a/lib/gitlab/bitbucket_server_import/mentions_converter.rb +++ b/lib/gitlab/bitbucket_server_import/mentions_converter.rb @@ -3,10 +3,9 @@ module Gitlab module BitbucketServerImport class MentionsConverter - MENTIONS_REGEX = /@([\w\d_-]*)/ + MENTIONS_REGEX = /@([\w\d_-]+)/ EMAIL_REGEX = /\b[A-Z0-9._%+-]+@[A-Z0-9.-]+\.[A-Z]{2,4}\b/i - PLACEHOLDER = 'PLACEHOLDER_TEXT-' - EMAIL_PLACEHOLDER = '~PLACEHOLDER~' + PLACEHOLDER = '~PLACEHOLDER_SYMBOL~' attr_reader :project, :users_mapper diff --git a/spec/lib/gitlab/bitbucket_server_import/mentions_converter_spec.rb b/spec/lib/gitlab/bitbucket_server_import/mentions_converter_spec.rb index 1d304af6d2c4ac..404e706a2dc58f 100644 --- a/spec/lib/gitlab/bitbucket_server_import/mentions_converter_spec.rb +++ b/spec/lib/gitlab/bitbucket_server_import/mentions_converter_spec.rb @@ -5,7 +5,7 @@ RSpec.describe Gitlab::BitbucketServerImport::MentionsConverter, :clean_gitlab_redis_cache, feature_category: :importers do let_it_be(:project) { create(:project) } let(:users_mapper) { BitbucketServer::UsersMapper.new(project) } - let(:text) { 'text without mentions' } + let(:text) { 'text without @ mentions' } subject(:converted_text) { described_class.new(project).convert(text) } -- GitLab From 0236041db72f8826c900c9dfb46fc1db847f4410 Mon Sep 17 00:00:00 2001 From: maddievn Date: Tue, 12 Dec 2023 12:33:06 +0200 Subject: [PATCH 11/18] Apply reviewer feedback --- lib/gitlab/bitbucket_server_import/mentions_converter.rb | 6 +++--- .../importers/pull_request_notes_importer_spec.rb | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/gitlab/bitbucket_server_import/mentions_converter.rb b/lib/gitlab/bitbucket_server_import/mentions_converter.rb index fd50899893546d..9b35665969a695 100644 --- a/lib/gitlab/bitbucket_server_import/mentions_converter.rb +++ b/lib/gitlab/bitbucket_server_import/mentions_converter.rb @@ -4,7 +4,7 @@ module Gitlab module BitbucketServerImport class MentionsConverter MENTIONS_REGEX = /@([\w\d_-]+)/ - EMAIL_REGEX = /\b[A-Z0-9._%+-]+@[A-Z0-9.-]+\.[A-Z]{2,4}\b/i + EMAIL_REGEX = /\b[A-Z0-9._%+-]+@[A-Z0-9.-]+\.[A-Z]{2,63}\b/i PLACEHOLDER = '~PLACEHOLDER_SYMBOL~' attr_reader :project, :users_mapper @@ -14,8 +14,8 @@ def initialize(project) @users_mapper = BitbucketServer::UsersMapper.new(project) end - def convert(text) - text = text.dup + def convert(original_text) + text = original_text.dup emails = text.scan(EMAIL_REGEX) emails.each_with_index { |email, index| text.gsub!(email, "~PLACEHOLDER-#{index}-EMAIL~") } diff --git a/spec/lib/gitlab/bitbucket_server_import/importers/pull_request_notes_importer_spec.rb b/spec/lib/gitlab/bitbucket_server_import/importers/pull_request_notes_importer_spec.rb index 9c98347543d316..9a535261890109 100644 --- a/spec/lib/gitlab/bitbucket_server_import/importers/pull_request_notes_importer_spec.rb +++ b/spec/lib/gitlab/bitbucket_server_import/importers/pull_request_notes_importer_spec.rb @@ -150,7 +150,7 @@ def expect_log(stage:, message:) end it 'does not convert mentions' do - expect(mentions_converter).not_to receive(:convert).and_call_original + expect(mentions_converter).not_to receive(:convert) subject.execute end -- GitLab From 7002e7f5aad1462239253e9634ae07cbb17ca66e Mon Sep 17 00:00:00 2001 From: maddievn Date: Tue, 12 Dec 2023 13:03:49 +0200 Subject: [PATCH 12/18] Remove uses mapper --- .../mentions_converter.rb | 16 +++++--- .../lib/bitbucket_server/users_mapper_spec.rb | 38 ------------------- .../mentions_converter_spec.rb | 14 +++---- 3 files changed, 18 insertions(+), 50 deletions(-) delete mode 100644 spec/lib/bitbucket_server/users_mapper_spec.rb diff --git a/lib/gitlab/bitbucket_server_import/mentions_converter.rb b/lib/gitlab/bitbucket_server_import/mentions_converter.rb index 9b35665969a695..a206d962ea90a6 100644 --- a/lib/gitlab/bitbucket_server_import/mentions_converter.rb +++ b/lib/gitlab/bitbucket_server_import/mentions_converter.rb @@ -3,15 +3,16 @@ module Gitlab module BitbucketServerImport class MentionsConverter + include UserCaching + MENTIONS_REGEX = /@([\w\d_-]+)/ EMAIL_REGEX = /\b[A-Z0-9._%+-]+@[A-Z0-9.-]+\.[A-Z]{2,63}\b/i PLACEHOLDER = '~PLACEHOLDER_SYMBOL~' - attr_reader :project, :users_mapper + attr_reader :project_id - def initialize(project) - @project = project - @users_mapper = BitbucketServer::UsersMapper.new(project) + def initialize(project_id) + @project_id = project_id end def convert(original_text) @@ -34,7 +35,7 @@ def replace_mentions(text) altered_mentions = [] mentions.each do |mention| - cached_email = users_mapper.read_source_email_from_cache(mention) + cached_email = read_email_from_cache(mention) if cached_email user = User.find_by_any_email(cached_email, confirmed: true) @@ -54,6 +55,11 @@ def replace_mentions(text) text.gsub(PLACEHOLDER, '@') end + + def read_email_from_cache(mention) + cache_key = source_user_cache_key(project_id, mention) + ::Gitlab::Cache::Import::Caching.read(cache_key) + end end end end diff --git a/spec/lib/bitbucket_server/users_mapper_spec.rb b/spec/lib/bitbucket_server/users_mapper_spec.rb deleted file mode 100644 index 79bb3f343d7089..00000000000000 --- a/spec/lib/bitbucket_server/users_mapper_spec.rb +++ /dev/null @@ -1,38 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe BitbucketServer::UsersMapper, feature_category: :importers do - let_it_be(:project) { create(:project) } - let(:username) { 'test_user' } - let(:email) { 'test@email.com' } - - subject(:users_mapper) { described_class.new(project) } - - describe '#write_source_email_to_cache' do - it 'caches provided source username and email' do - expect(Gitlab::Cache::Import::Caching).to receive(:write) - .with("bitbucket_server/project/#{project.id}/source/username/#{username}", email) - - users_mapper.write_source_email_to_cache(username, email) - end - end - - describe '#read_source_email_from_cache', :clean_gitlab_redis_cache do - context 'when it has been cached' do - before do - users_mapper.write_source_email_to_cache(username, email) - end - - it 'returns the matching email' do - expect(users_mapper.read_source_email_from_cache(username)).to eq(email) - end - end - - context 'when it has not been cached' do - it 'returns the matching email' do - expect(users_mapper.read_source_email_from_cache(username)).to be_nil - end - end - end -end diff --git a/spec/lib/gitlab/bitbucket_server_import/mentions_converter_spec.rb b/spec/lib/gitlab/bitbucket_server_import/mentions_converter_spec.rb index 404e706a2dc58f..28d2084039f361 100644 --- a/spec/lib/gitlab/bitbucket_server_import/mentions_converter_spec.rb +++ b/spec/lib/gitlab/bitbucket_server_import/mentions_converter_spec.rb @@ -3,11 +3,11 @@ require 'spec_helper' RSpec.describe Gitlab::BitbucketServerImport::MentionsConverter, :clean_gitlab_redis_cache, feature_category: :importers do - let_it_be(:project) { create(:project) } - let(:users_mapper) { BitbucketServer::UsersMapper.new(project) } + let(:project_id) { 12 } let(:text) { 'text without @ mentions' } + let(:source_user_cache_prefix) { "bitbucket_server/project/#{project_id}/source/username" } - subject(:converted_text) { described_class.new(project).convert(text) } + subject(:converted_text) { described_class.new(project_id).convert(text) } describe '#convert' do context 'when the text has no mentions' do @@ -21,7 +21,7 @@ context 'when the mention has matching cached email' do before do - users_mapper.write_source_email_to_cache('john', 'john@example.com') + ::Gitlab::Cache::Import::Caching.write("#{source_user_cache_prefix}/john", 'john@example.com') end context 'when a user with the email does not exist on gitlab' do @@ -49,7 +49,7 @@ context 'when there is cached email but not for the mentioned username' do before do - users_mapper.write_source_email_to_cache('jane', 'jane@example.com') + ::Gitlab::Cache::Import::Caching.write("#{source_user_cache_prefix}/jane", 'jane@example.com') end it 'puts the mention in backticks' do @@ -70,7 +70,7 @@ let_it_be(:user) { create(:user, username: 'johndoe', email: 'john@example.com') } before do - users_mapper.write_source_email_to_cache('john_DOE-123', 'john@example.com') + ::Gitlab::Cache::Import::Caching.write("#{source_user_cache_prefix}/john_DOE-123", 'john@example.com') end it "replaces the mention with the user's username" do @@ -106,7 +106,7 @@ let_it_be(:user) { create(:user, username: 'johndoe', email: 'john@example.com') } before do - users_mapper.write_source_email_to_cache('john', 'john@example.com') + ::Gitlab::Cache::Import::Caching.write("#{source_user_cache_prefix}/john", 'john@example.com') end it 'replaces all mentions with the username and puts rest of mentions in backticks' do -- GitLab From f86d2c4634be6ef0da7ec5fe91abd0030266c63a Mon Sep 17 00:00:00 2001 From: maddievn Date: Tue, 12 Dec 2023 13:09:17 +0200 Subject: [PATCH 13/18] Fix specs --- lib/bitbucket_server/users_mapper.rb | 29 ------------------- .../importers/pull_request_importer.rb | 2 +- .../importers/pull_request_notes_importer.rb | 2 +- .../importers/pull_request_importer_spec.rb | 4 +-- 4 files changed, 4 insertions(+), 33 deletions(-) delete mode 100644 lib/bitbucket_server/users_mapper.rb diff --git a/lib/bitbucket_server/users_mapper.rb b/lib/bitbucket_server/users_mapper.rb deleted file mode 100644 index f24d4465692500..00000000000000 --- a/lib/bitbucket_server/users_mapper.rb +++ /dev/null @@ -1,29 +0,0 @@ -# frozen_string_literal: true - -module BitbucketServer - class UsersMapper - include Gitlab::Utils::StrongMemoize - - SOURCE_USER_CACHE_KEY = 'bitbucket_server/project/%s/source/username/%s' - - def initialize(project) - @project_id = project.id - end - - attr_reader :project_id - - def write_source_email_to_cache(username, email) - ::Gitlab::Cache::Import::Caching.write(cache_key(username), email) - end - - def read_source_email_from_cache(username) - ::Gitlab::Cache::Import::Caching.read(cache_key(username)) - end - - private - - def cache_key(username) - format(SOURCE_USER_CACHE_KEY, project_id, username) - end - end -end diff --git a/lib/gitlab/bitbucket_server_import/importers/pull_request_importer.rb b/lib/gitlab/bitbucket_server_import/importers/pull_request_importer.rb index 419598c90e8ca3..99f4adbe317477 100644 --- a/lib/gitlab/bitbucket_server_import/importers/pull_request_importer.rb +++ b/lib/gitlab/bitbucket_server_import/importers/pull_request_importer.rb @@ -10,7 +10,7 @@ def initialize(project, hash) @project = project @formatter = Gitlab::ImportFormatter.new @user_finder = UserFinder.new(project) - @mentions_converter = Gitlab::BitbucketServerImport::MentionsConverter.new(project) + @mentions_converter = Gitlab::BitbucketServerImport::MentionsConverter.new(project.id) # Object should behave as a object so we can remove object.is_a?(Hash) check # This will be fixed in https://gitlab.com/gitlab-org/gitlab/-/issues/412328 diff --git a/lib/gitlab/bitbucket_server_import/importers/pull_request_notes_importer.rb b/lib/gitlab/bitbucket_server_import/importers/pull_request_notes_importer.rb index 6e3287e0f1e810..b314e8913c3ff8 100644 --- a/lib/gitlab/bitbucket_server_import/importers/pull_request_notes_importer.rb +++ b/lib/gitlab/bitbucket_server_import/importers/pull_request_notes_importer.rb @@ -224,7 +224,7 @@ def repository_slug end def mentions_converter - Gitlab::BitbucketServerImport::MentionsConverter.new(project) + Gitlab::BitbucketServerImport::MentionsConverter.new(project.id) end end end diff --git a/spec/lib/gitlab/bitbucket_server_import/importers/pull_request_importer_spec.rb b/spec/lib/gitlab/bitbucket_server_import/importers/pull_request_importer_spec.rb index 1ea93dd8e0b298..eeb2f9c8000d42 100644 --- a/spec/lib/gitlab/bitbucket_server_import/importers/pull_request_importer_spec.rb +++ b/spec/lib/gitlab/bitbucket_server_import/importers/pull_request_importer_spec.rb @@ -18,7 +18,7 @@ it 'imports the merge request correctly' do expect_next(Gitlab::Import::MergeRequestCreator, project).to receive(:execute).and_call_original expect_next(Gitlab::BitbucketServerImport::UserFinder, project).to receive(:author_id).and_call_original - expect_next(Gitlab::BitbucketServerImport::MentionsConverter, project).to receive(:convert).and_call_original + expect_next(Gitlab::BitbucketServerImport::MentionsConverter, project.id).to receive(:convert).and_call_original expect { importer.execute }.to change { MergeRequest.count }.by(1) @@ -42,7 +42,7 @@ end it 'does not convert mentions' do - expect_next(Gitlab::BitbucketServerImport::MentionsConverter, project).not_to receive(:convert) + expect_next(Gitlab::BitbucketServerImport::MentionsConverter, project.id).not_to receive(:convert) importer.execute end -- GitLab From e906f68cc9c45a49d9d0befbaa9a540130c4d7a4 Mon Sep 17 00:00:00 2001 From: maddievn Date: Thu, 14 Dec 2023 13:03:58 +0200 Subject: [PATCH 14/18] Keep the @ between backticks --- .../mentions_converter.rb | 2 +- .../mentions_converter_spec.rb | 16 ++++++++-------- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/lib/gitlab/bitbucket_server_import/mentions_converter.rb b/lib/gitlab/bitbucket_server_import/mentions_converter.rb index a206d962ea90a6..750f3c1cb5256d 100644 --- a/lib/gitlab/bitbucket_server_import/mentions_converter.rb +++ b/lib/gitlab/bitbucket_server_import/mentions_converter.rb @@ -46,7 +46,7 @@ def replace_mentions(text) end end - altered_mentions << ["@#{mention}", "`#{mention}`"] + altered_mentions << ["@#{mention}", "`#{PLACEHOLDER}#{mention}`"] end altered_mentions.each do |original_mention, altered_mention| diff --git a/spec/lib/gitlab/bitbucket_server_import/mentions_converter_spec.rb b/spec/lib/gitlab/bitbucket_server_import/mentions_converter_spec.rb index 28d2084039f361..05c5d948c61849 100644 --- a/spec/lib/gitlab/bitbucket_server_import/mentions_converter_spec.rb +++ b/spec/lib/gitlab/bitbucket_server_import/mentions_converter_spec.rb @@ -26,7 +26,7 @@ context 'when a user with the email does not exist on gitlab' do it 'puts the mention in backticks' do - expect(converted_text).to eq('mentioning `john`') + expect(converted_text).to eq('mentioning `@john`') end end @@ -42,7 +42,7 @@ let_it_be(:user) { create(:user, username: 'john') } it 'puts the mention in backticks' do - expect(converted_text).to eq('mentioning `john`') + expect(converted_text).to eq('mentioning `@john`') end end end @@ -53,14 +53,14 @@ end it 'puts the mention in backticks' do - expect(converted_text).to eq('mentioning `john`') + expect(converted_text).to eq('mentioning `@john`') end context 'when a user with the same email exists on gitlab' do let_it_be(:user) { create(:user, username: 'jane', email: 'jane@example.com') } it 'puts the mention in backticks' do - expect(converted_text).to eq('mentioning `john`') + expect(converted_text).to eq('mentioning `@john`') end end end @@ -82,13 +82,13 @@ let(:text) { "@john's email is john@gmail.com and @jane's email is jane@example.com." } it 'does not alter the emails' do - expect(converted_text).to eq("`john`'s email is john@gmail.com and `jane`'s email is jane@example.com.") + expect(converted_text).to eq("`@john`'s email is john@gmail.com and `@jane`'s email is jane@example.com.") end end context 'when no emails are cached' do it 'puts the mention in backticks' do - expect(converted_text).to eq('mentioning `john`') + expect(converted_text).to eq('mentioning `@john`') end end end @@ -98,7 +98,7 @@ context 'if none of the mentions have matching cached emails and users' do it 'puts every mention in backticks' do - expect(converted_text).to eq("`john`, `jane-doe` and `johndoe123` with \n`john` again on a newline") + expect(converted_text).to eq("`@john`, `@jane-doe` and `@johndoe123` with \n`@john` again on a newline") end end @@ -110,7 +110,7 @@ end it 'replaces all mentions with the username and puts rest of mentions in backticks' do - expect(converted_text).to eq("@johndoe, `jane-doe` and `johndoe123` with \n@johndoe again on a newline") + expect(converted_text).to eq("@johndoe, `@jane-doe` and `@johndoe123` with \n@johndoe again on a newline") end end end -- GitLab From 94d2939b9e4b39fd4dfa6f71437ce033c6003ab7 Mon Sep 17 00:00:00 2001 From: maddievn Date: Thu, 14 Dec 2023 15:03:15 +0200 Subject: [PATCH 15/18] Add specs per undercov --- .../pull_request_notes_importer_spec.rb | 48 +++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/spec/lib/gitlab/bitbucket_server_import/importers/pull_request_notes_importer_spec.rb b/spec/lib/gitlab/bitbucket_server_import/importers/pull_request_notes_importer_spec.rb index 9a535261890109..07aad3cc42a5b0 100644 --- a/spec/lib/gitlab/bitbucket_server_import/importers/pull_request_notes_importer_spec.rb +++ b/spec/lib/gitlab/bitbucket_server_import/importers/pull_request_notes_importer_spec.rb @@ -144,6 +144,54 @@ def expect_log(stage:, message:) ) end + context 'when the author is not found' do + before do + allow_next_instance_of(Gitlab::BitbucketServerImport::UserFinder) do |user_finder| + allow(user_finder).to receive(:uid).and_return(nil) + end + end + + it 'adds a note with the author username and email' do + subject.execute + + expect(Note.first.note).to include("*By #{note_author.username} (#{note_author.email})") + end + end + + context 'when the note has a parent note' do + let(:pr_note) do + instance_double( + BitbucketServer::Representation::Comment, + note: 'Note', + author_email: note_author.email, + author_username: note_author.username, + comments: [], + created_at: now, + updated_at: now, + parent_comment: pr_parent_note + ) + end + + let(:pr_parent_note) do + instance_double( + BitbucketServer::Representation::Comment, + note: 'Parent note', + author_email: note_author.email, + author_username: note_author.username, + comments: [], + created_at: now, + updated_at: now, + parent_comment: nil + ) + end + + it 'adds the parent note before the actual note' do + subject.execute + + expect(Note.first.note).to include("> #{pr_parent_note.note}\n\n") + end + end + context 'when the `bitbucket_server_convert_mentions_to_users` flag is disabled' do before do stub_feature_flags(bitbucket_server_convert_mentions_to_users: false) -- GitLab From 2bccdaa83be07794c6f1364249f7d165e32f3885 Mon Sep 17 00:00:00 2001 From: maddievn Date: Tue, 19 Dec 2023 11:39:30 +0200 Subject: [PATCH 16/18] Apply reviewer comments --- .../importers/pull_request_notes_importer.rb | 7 +- .../importers/users_importer.rb | 2 +- .../mentions_converter.rb | 34 ++++------ .../bitbucket_server_import/user_caching.rb | 13 ---- .../user_from_mention.rb | 35 ++++++++++ .../user_from_mention_spec.rb | 67 +++++++++++++++++++ 6 files changed, 118 insertions(+), 40 deletions(-) delete mode 100644 lib/gitlab/bitbucket_server_import/user_caching.rb create mode 100644 lib/gitlab/bitbucket_server_import/user_from_mention.rb create mode 100644 spec/lib/gitlab/bitbucket_server_import/user_from_mention_spec.rb diff --git a/lib/gitlab/bitbucket_server_import/importers/pull_request_notes_importer.rb b/lib/gitlab/bitbucket_server_import/importers/pull_request_notes_importer.rb index 2d6270f8417064..19e5cdcbdc27a4 100644 --- a/lib/gitlab/bitbucket_server_import/importers/pull_request_notes_importer.rb +++ b/lib/gitlab/bitbucket_server_import/importers/pull_request_notes_importer.rb @@ -11,6 +11,7 @@ def initialize(project, hash) @project = project @user_finder = UserFinder.new(project) @formatter = Gitlab::ImportFormatter.new + @mentions_converter = Gitlab::BitbucketServerImport::MentionsConverter.new(project.id) @object = hash.with_indifferent_access end @@ -43,7 +44,7 @@ def execute private - attr_reader :object, :project, :formatter, :user_finder + attr_reader :object, :project, :formatter, :user_finder, :mentions_converter def import_data_valid? project.import_data&.credentials && project.import_data&.data @@ -226,10 +227,6 @@ def project_key def repository_slug project.import_data.data['repo_slug'] end - - def mentions_converter - Gitlab::BitbucketServerImport::MentionsConverter.new(project.id) - end end end end diff --git a/lib/gitlab/bitbucket_server_import/importers/users_importer.rb b/lib/gitlab/bitbucket_server_import/importers/users_importer.rb index f8d0521afb2611..835b389c806ef0 100644 --- a/lib/gitlab/bitbucket_server_import/importers/users_importer.rb +++ b/lib/gitlab/bitbucket_server_import/importers/users_importer.rb @@ -47,7 +47,7 @@ def cache_users(users) hash[cache_key] = user.email end - ::Gitlab::Cache::Import::Caching.write_multiple(users_hash) + cache_multiple(users_hash) end def client diff --git a/lib/gitlab/bitbucket_server_import/mentions_converter.rb b/lib/gitlab/bitbucket_server_import/mentions_converter.rb index 750f3c1cb5256d..6fd14495b829b8 100644 --- a/lib/gitlab/bitbucket_server_import/mentions_converter.rb +++ b/lib/gitlab/bitbucket_server_import/mentions_converter.rb @@ -3,11 +3,12 @@ module Gitlab module BitbucketServerImport class MentionsConverter - include UserCaching + include UserFromMention - MENTIONS_REGEX = /@([\w\d_-]+)/ - EMAIL_REGEX = /\b[A-Z0-9._%+-]+@[A-Z0-9.-]+\.[A-Z]{2,63}\b/i - PLACEHOLDER = '~PLACEHOLDER_SYMBOL~' + MENTIONS_REGEX = User.reference_pattern + EMAIL_REGEX = /[^@\s]+@[^@\s]+/ + MENTION_PLACEHOLDER = '~MENTION_PLACEHOLDER~' + EMAIL_PLACEHOLDER = '~EMAIL_PLACEHOLDER~' attr_reader :project_id @@ -19,11 +20,11 @@ def convert(original_text) text = original_text.dup emails = text.scan(EMAIL_REGEX) - emails.each_with_index { |email, index| text.gsub!(email, "~PLACEHOLDER-#{index}-EMAIL~") } + emails.each_with_index { |email, index| text.gsub!(email, "#{EMAIL_PLACEHOLDER}#{index}") } text = replace_mentions(text) - emails.each_with_index { |email, index| text.gsub!("~PLACEHOLDER-#{index}-EMAIL~", email) } + emails.each_with_index { |email, index| text.gsub!("#{EMAIL_PLACEHOLDER}#{index}", email) } text end @@ -35,30 +36,21 @@ def replace_mentions(text) altered_mentions = [] mentions.each do |mention| - cached_email = read_email_from_cache(mention) + user = user_from_cache(mention) - if cached_email - user = User.find_by_any_email(cached_email, confirmed: true) - - if user - altered_mentions << ["@#{mention}", "#{PLACEHOLDER}#{user.username}"] - next - end + if user + altered_mentions << ["@#{mention}", "#{MENTION_PLACEHOLDER}#{user.username}"] + next end - altered_mentions << ["@#{mention}", "`#{PLACEHOLDER}#{mention}`"] + altered_mentions << ["@#{mention}", "`#{MENTION_PLACEHOLDER}#{mention}`"] end altered_mentions.each do |original_mention, altered_mention| text.sub!(original_mention, altered_mention) end - text.gsub(PLACEHOLDER, '@') - end - - def read_email_from_cache(mention) - cache_key = source_user_cache_key(project_id, mention) - ::Gitlab::Cache::Import::Caching.read(cache_key) + text.gsub(MENTION_PLACEHOLDER, '@') end end end diff --git a/lib/gitlab/bitbucket_server_import/user_caching.rb b/lib/gitlab/bitbucket_server_import/user_caching.rb deleted file mode 100644 index 0f0169122c513d..00000000000000 --- a/lib/gitlab/bitbucket_server_import/user_caching.rb +++ /dev/null @@ -1,13 +0,0 @@ -# frozen_string_literal: true - -module Gitlab - module BitbucketServerImport - module UserCaching - SOURCE_USER_CACHE_KEY = 'bitbucket_server/project/%s/source/username/%s' - - def source_user_cache_key(project_id, username) - format(SOURCE_USER_CACHE_KEY, project_id, username) - end - end - end -end diff --git a/lib/gitlab/bitbucket_server_import/user_from_mention.rb b/lib/gitlab/bitbucket_server_import/user_from_mention.rb new file mode 100644 index 00000000000000..03fba7088acd73 --- /dev/null +++ b/lib/gitlab/bitbucket_server_import/user_from_mention.rb @@ -0,0 +1,35 @@ +# frozen_string_literal: true + +module Gitlab + module BitbucketServerImport + module UserFromMention + SOURCE_USER_CACHE_KEY = 'bitbucket_server/project/%s/source/username/%s' + + def user_from_cache(mention) + cached_email = read(mention) + + return unless cached_email + + find_user(cached_email) + end + + def cache_multiple(hash) + ::Gitlab::Cache::Import::Caching.write_multiple(hash) + end + + def source_user_cache_key(project_id, username) + format(SOURCE_USER_CACHE_KEY, project_id, username) + end + + private + + def read(mention) + ::Gitlab::Cache::Import::Caching.read(source_user_cache_key(project_id, mention)) + end + + def find_user(email) + User.find_by_any_email(email, confirmed: true) + end + end + end +end diff --git a/spec/lib/gitlab/bitbucket_server_import/user_from_mention_spec.rb b/spec/lib/gitlab/bitbucket_server_import/user_from_mention_spec.rb new file mode 100644 index 00000000000000..a09e5bb265a75b --- /dev/null +++ b/spec/lib/gitlab/bitbucket_server_import/user_from_mention_spec.rb @@ -0,0 +1,67 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::BitbucketServerImport::UserFromMention, :clean_gitlab_redis_cache, feature_category: :importers do + let(:project_id) { 11 } + let(:username) { '@johndoe' } + let(:email) { 'john@gmail.com' } + let(:hash) { { key: 'value' } } + let(:cache_key) { "bitbucket_server/project/#{project_id}/source/username/#{username}" } + + let(:example) do + Class.new do + include Gitlab::BitbucketServerImport::UserFromMention + + def initialize(project_id) + @project_id = project_id + end + + attr_reader :project_id + + def foo(mention) + user_from_cache(mention) + end + + def bar(hash) + cache_multiple(hash) + end + end + end + + subject(:example_class) { example.new(project_id) } + + describe '#user_from_cache' do + it 'returns nil if the cache is empty' do + expect(example_class.foo(username)).to be_nil + end + + context 'when the username and email is cached' do + before do + ::Gitlab::Cache::Import::Caching.write(cache_key, email) + end + + context 'if a user with the email does not exist' do + it 'returns nil' do + expect(example_class.foo(username)).to be_nil + end + end + + context 'if a user with the email exists' do + let!(:user) { create(:user, email: email) } + + it 'returns the user' do + expect(example_class.foo(username)).to eq(user) + end + end + end + end + + describe '#cache_multiple' do + it 'calls write_multiple with the hash' do + expect(Gitlab::Cache::Import::Caching).to receive(:write_multiple).with(hash) + + example_class.bar(hash) + end + end +end -- GitLab From d5bc73ade53e24ba920f85c274841f0f6c1b70c4 Mon Sep 17 00:00:00 2001 From: maddievn Date: Tue, 19 Dec 2023 12:30:04 +0200 Subject: [PATCH 17/18] Set expiry to 72 hours --- .../bitbucket_server_import/importers/users_importer.rb | 2 +- lib/gitlab/bitbucket_server_import/user_from_mention.rb | 6 +++++- .../bitbucket_server_import/user_from_mention_spec.rb | 2 +- 3 files changed, 7 insertions(+), 3 deletions(-) diff --git a/lib/gitlab/bitbucket_server_import/importers/users_importer.rb b/lib/gitlab/bitbucket_server_import/importers/users_importer.rb index 835b389c806ef0..32bea5942d4310 100644 --- a/lib/gitlab/bitbucket_server_import/importers/users_importer.rb +++ b/lib/gitlab/bitbucket_server_import/importers/users_importer.rb @@ -5,7 +5,7 @@ module BitbucketServerImport module Importers class UsersImporter include Loggable - include UserCaching + include UserFromMention BATCH_SIZE = 100 diff --git a/lib/gitlab/bitbucket_server_import/user_from_mention.rb b/lib/gitlab/bitbucket_server_import/user_from_mention.rb index 03fba7088acd73..907db24576014c 100644 --- a/lib/gitlab/bitbucket_server_import/user_from_mention.rb +++ b/lib/gitlab/bitbucket_server_import/user_from_mention.rb @@ -14,7 +14,7 @@ def user_from_cache(mention) end def cache_multiple(hash) - ::Gitlab::Cache::Import::Caching.write_multiple(hash) + ::Gitlab::Cache::Import::Caching.write_multiple(hash, timeout: timeout) end def source_user_cache_key(project_id, username) @@ -30,6 +30,10 @@ def read(mention) def find_user(email) User.find_by_any_email(email, confirmed: true) end + + def timeout + ::Gitlab::Cache::Import::Caching::LONGER_TIMEOUT + end end end end diff --git a/spec/lib/gitlab/bitbucket_server_import/user_from_mention_spec.rb b/spec/lib/gitlab/bitbucket_server_import/user_from_mention_spec.rb index a09e5bb265a75b..73f9cde832286e 100644 --- a/spec/lib/gitlab/bitbucket_server_import/user_from_mention_spec.rb +++ b/spec/lib/gitlab/bitbucket_server_import/user_from_mention_spec.rb @@ -59,7 +59,7 @@ def bar(hash) describe '#cache_multiple' do it 'calls write_multiple with the hash' do - expect(Gitlab::Cache::Import::Caching).to receive(:write_multiple).with(hash) + expect(Gitlab::Cache::Import::Caching).to receive(:write_multiple).with(hash, timeout: 72.hours) example_class.bar(hash) end -- GitLab From c8d138415540cfe70a629bcce97bf6d160535ec4 Mon Sep 17 00:00:00 2001 From: maddievn Date: Thu, 21 Dec 2023 10:59:52 +0200 Subject: [PATCH 18/18] Remove email handling --- .../mentions_converter.rb | 17 +++-------------- .../mentions_converter_spec.rb | 4 ++-- 2 files changed, 5 insertions(+), 16 deletions(-) diff --git a/lib/gitlab/bitbucket_server_import/mentions_converter.rb b/lib/gitlab/bitbucket_server_import/mentions_converter.rb index 6fd14495b829b8..8b1eeb6e007873 100644 --- a/lib/gitlab/bitbucket_server_import/mentions_converter.rb +++ b/lib/gitlab/bitbucket_server_import/mentions_converter.rb @@ -6,9 +6,7 @@ class MentionsConverter include UserFromMention MENTIONS_REGEX = User.reference_pattern - EMAIL_REGEX = /[^@\s]+@[^@\s]+/ - MENTION_PLACEHOLDER = '~MENTION_PLACEHOLDER~' - EMAIL_PLACEHOLDER = '~EMAIL_PLACEHOLDER~' + MENTION_PLACEHOLDER = '~GITLAB_MENTION_PLACEHOLDER~' attr_reader :project_id @@ -16,17 +14,8 @@ def initialize(project_id) @project_id = project_id end - def convert(original_text) - text = original_text.dup - emails = text.scan(EMAIL_REGEX) - - emails.each_with_index { |email, index| text.gsub!(email, "#{EMAIL_PLACEHOLDER}#{index}") } - - text = replace_mentions(text) - - emails.each_with_index { |email, index| text.gsub!("#{EMAIL_PLACEHOLDER}#{index}", email) } - - text + def convert(text) + replace_mentions(text.dup) end private diff --git a/spec/lib/gitlab/bitbucket_server_import/mentions_converter_spec.rb b/spec/lib/gitlab/bitbucket_server_import/mentions_converter_spec.rb index 05c5d948c61849..46800c924c9e19 100644 --- a/spec/lib/gitlab/bitbucket_server_import/mentions_converter_spec.rb +++ b/spec/lib/gitlab/bitbucket_server_import/mentions_converter_spec.rb @@ -79,10 +79,10 @@ end context 'when the mention has emails' do - let(:text) { "@john's email is john@gmail.com and @jane's email is jane@example.com." } + let(:text) { "@john's email is john@gmail.com and @jane's email is info@jane." } it 'does not alter the emails' do - expect(converted_text).to eq("`@john`'s email is john@gmail.com and `@jane`'s email is jane@example.com.") + expect(converted_text).to eq("`@john`'s email is john@gmail.com and `@jane`'s email is info@jane.") end end -- GitLab