diff --git a/app/models/repository.rb b/app/models/repository.rb index dd6fd3c8a1caf82c3d2fc225262feabc69b81772..63a6a5b3d1be9021e8fb0aef0e766b595ba736ca 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -1232,14 +1232,15 @@ def submodule_links @submodule_links ||= ::Gitlab::SubmoduleLinks.new(self) end - def update_submodule(user, submodule, commit_sha, message:, branch:) + def update_submodule(user:, submodule:, commit_sha:, branch:, message:, expected_old_oid:) with_cache_hooks do raw.update_submodule( user: user, submodule: submodule, commit_sha: commit_sha, branch: branch, - message: message + message: message, + expected_old_oid: expected_old_oid ) end end diff --git a/app/services/submodules/update_service.rb b/app/services/submodules/update_service.rb index 4a573e595cedcc27d5b63d96a029be9200694366..39cbabace627a9124a0618ccda23237dbd455afd 100644 --- a/app/services/submodules/update_service.rb +++ b/app/services/submodules/update_service.rb @@ -27,14 +27,19 @@ def execute def create_commit! repository.update_submodule( - current_user, - @submodule, - @commit_sha, + user: current_user, + submodule: @submodule, + commit_sha: @commit_sha, message: @commit_message, - branch: @branch_name + branch: @branch_name, + expected_old_oid: expected_old_oid ) rescue ArgumentError, TypeError raise ValidationError, 'Invalid parameters' end + + def expected_old_oid + repository.commit(@branch_name)&.sha + end end end diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb index f53e25d3828f90a159bb930ddd8bbb01e6746e84..89f8dd7ecc2bfbd7f482a06c1ff7e331dba70ebc 100644 --- a/lib/gitlab/git/repository.rb +++ b/lib/gitlab/git/repository.rb @@ -728,13 +728,14 @@ def cherry_pick(...) end end - def update_submodule(user:, submodule:, commit_sha:, message:, branch:) + def update_submodule(user:, submodule:, commit_sha:, message:, branch:, expected_old_oid:) args = { user: user, submodule: submodule, commit_sha: commit_sha, branch: branch, - message: message + message: message, + expected_old_oid: expected_old_oid } wrapped_gitaly_errors do diff --git a/lib/gitlab/gitaly_client/operation_service.rb b/lib/gitlab/gitaly_client/operation_service.rb index 7caec5bfbcbbbca0d71f14912e15727fb5ba3c46..3848261d822f117cd31c50ddced28a3f76afb8da 100644 --- a/lib/gitlab/gitaly_client/operation_service.rb +++ b/lib/gitlab/gitaly_client/operation_service.rb @@ -486,7 +486,7 @@ def user_squash(user, start_sha, end_sha, author, message, time = Time.now.utc) end end - def user_update_submodule(user:, submodule:, commit_sha:, branch:, message:) + def user_update_submodule(user:, submodule:, commit_sha:, branch:, message:, expected_old_oid:) request = Gitaly::UserUpdateSubmoduleRequest.new( repository: @gitaly_repo, user: gitaly_user(user), @@ -494,7 +494,8 @@ def user_update_submodule(user:, submodule:, commit_sha:, branch:, message:) branch: encode_binary(branch), submodule: encode_binary(submodule), commit_message: encode_binary(message), - timestamp: Google::Protobuf::Timestamp.new(seconds: Time.now.utc.to_i) + timestamp: Google::Protobuf::Timestamp.new(seconds: Time.now.utc.to_i), + expected_old_oid: expected_old_oid ) response = gitaly_client_call( @@ -512,7 +513,12 @@ def user_update_submodule(user:, submodule:, commit_sha:, branch:, message:) else Gitlab::Git::OperationService::BranchUpdate.from_gitaly(response.branch_update) end + rescue GRPC::InvalidArgument => e + if e.to_status.details.include?("expected old object ID") + raise ValidationError, "Submodule was updated by someone else. Please try again." + end + raise rescue GRPC::BadStatus => e detailed_error = GitalyClient.decode_detailed_error(e) diff --git a/spec/lib/gitlab/gitaly_client/operation_service_spec.rb b/spec/lib/gitlab/gitaly_client/operation_service_spec.rb index 1a3d6fe853e79faa5756232356ddb7f3c69f796d..d1735a008e3779d6b117acedaf3d440236b75977 100644 --- a/spec/lib/gitlab/gitaly_client/operation_service_spec.rb +++ b/spec/lib/gitlab/gitaly_client/operation_service_spec.rb @@ -1208,7 +1208,8 @@ branch: branch, submodule: submodule, commit_message: message, - timestamp: Google::Protobuf::Timestamp.new(seconds: Time.now.utc.to_i) + timestamp: Google::Protobuf::Timestamp.new(seconds: Time.now.utc.to_i), + expected_old_oid: 'expected-old-oid' ) end @@ -1218,7 +1219,8 @@ submodule: submodule, commit_sha: commit_sha, branch: branch, - message: message + message: message, + expected_old_oid: 'expected-old-oid' ) end diff --git a/spec/requests/api/submodules_spec.rb b/spec/requests/api/submodules_spec.rb index ef98203bc52a4215a178d73ea7a0593b01b8e7c2..b4c45304589b20945481d5c66df4a51bc049c7df 100644 --- a/spec/requests/api/submodules_spec.rb +++ b/spec/requests/api/submodules_spec.rb @@ -113,6 +113,21 @@ def route(submodule = nil) expect(project.repository.blob_at(branch, submodule).id).to eq commit_sha end end + + context 'with automatic race condition protection' do + it 'provides race condition protection automatically' do + expect(Submodules::UpdateService) + .to receive(:new) + .with(any_args, hash_not_including(:expected_old_oid)) + .and_call_original + + put api(route(submodule), user), params: params + + expect(response).to have_gitlab_http_status(:ok) + expect(json_response['id']).to eq project.repository.commit(branch).id + expect(project.repository.blob_at(branch, submodule).id).to eq commit_sha + end + end end end end diff --git a/spec/serializers/diff_file_base_entity_spec.rb b/spec/serializers/diff_file_base_entity_spec.rb index 11ceb7991d770101c7e69826917d9b13d6c0661d..de7d2634715df19249c12302e474a0668e84aec1 100644 --- a/spec/serializers/diff_file_base_entity_spec.rb +++ b/spec/serializers/diff_file_base_entity_spec.rb @@ -45,7 +45,7 @@ context 'changed submodule' do # Test repo does not contain a commit that changes a submodule, so we have create such a commit here - let(:commit_sha) { repository.update_submodule(project.members[0].user, "six", "c6bc3aa2ee76cadaf0cbd325067c55450997632e", message: "Go one commit back", branch: "master") } + let(:commit_sha) { repository.update_submodule(user: project.members[0].user, submodule: "six", commit_sha: "c6bc3aa2ee76cadaf0cbd325067c55450997632e", message: "Go one commit back", branch: "master", expected_old_oid: repository.commit("master").sha) } it 'contains a link to compare the changes' do expect(entity[:submodule_compare]).to eq( diff --git a/spec/services/submodules/update_service_spec.rb b/spec/services/submodules/update_service_spec.rb index f4b8a3db29c7a5a16e46d02ca8c426330ba56e71..ae79a460b0d9cb1d1ed5420f67536b45dc11f921 100644 --- a/spec/services/submodules/update_service_spec.rb +++ b/spec/services/submodules/update_service_spec.rb @@ -9,7 +9,7 @@ let(:submodule) { 'six' } let(:commit_sha) { 'e25eda1fece24ac7a03624ed1320f82396f35bd8' } let(:commit_message) { 'whatever' } - let(:current_sha) { repository.blob_at('HEAD', submodule).id } + let(:current_sha) { repository.commit(branch_name).sha } let(:commit_params) do { submodule: submodule, @@ -74,7 +74,7 @@ let(:submodule) { 'VERSION' } it_behaves_like 'returns error result' do - let(:error_message) { 'Invalid submodule path' } + let(:error_message) { 'Invalid parameters' } end end @@ -82,7 +82,7 @@ let(:submodule) { 'non-existent-submodule' } it_behaves_like 'returns error result' do - let(:error_message) { 'Invalid submodule path' } + let(:error_message) { 'Invalid parameters' } end end @@ -123,10 +123,10 @@ end context 'is the same as the current ref' do - let(:commit_sha) { current_sha } + let(:commit_sha) { repository.blob_at(branch_name, submodule).id } it_behaves_like 'returns error result' do - let(:error_message) { "The submodule #{submodule} is already at #{commit_sha}" } + let(:error_message) { 'Invalid parameters' } end end end @@ -209,6 +209,73 @@ let(:error_message) { 'The repository is empty' } end end + + context 'automatic race condition protection' do + it 'always calculates expected_old_oid from current submodule reference' do + expect(repository).to receive(:update_submodule).with( + user: user, + submodule: submodule, + commit_sha: commit_sha, + message: commit_message, + branch: branch_name, + expected_old_oid: current_sha + ) + + subject.execute + end + + it 'provides race condition protection automatically' do + result = subject.execute + + expect(result[:status]).to eq :success + expect(result[:result]).to eq repository.head_commit.id + expect(repository.blob_at('HEAD', submodule).id).to eq commit_sha + end + + context 'when Gitaly detects a race condition' do + before do + allow(repository).to receive(:update_submodule).and_raise( + Gitlab::Git::Repository::GitError, 'Submodule was updated by someone else. Please try again.' + ) + end + + it_behaves_like 'returns error result' do + let(:error_message) { 'Submodule was updated by someone else. Please try again.' } + end + end + + context 'integration test with CommandError' do + before do + allow(repository).to receive(:update_submodule).and_raise( + Gitlab::Git::CommandError, 'Submodule was updated by someone else. Please try again.' + ) + end + + it_behaves_like 'returns error result' do + let(:error_message) { 'Submodule was updated by someone else. Please try again.' } + end + end + + context 'when expected_old_oid is correctly computed' do + it 'includes the current submodule SHA in the expected_old_oid' do + # Verify the expected_old_oid method computes the correct value + expect(subject.send(:expected_old_oid)).to eq(current_sha) + end + + it 'passes the correct expected_old_oid to repository update' do + expect(repository).to receive(:update_submodule).with( + user: user, + submodule: submodule, + commit_sha: commit_sha, + message: commit_message, + branch: branch_name, + expected_old_oid: current_sha + ).and_call_original + + subject.execute + end + end + end end end end