summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
Diffstat
-rw-r--r--app/models/project.rb4
-rw-r--r--changelogs/unreleased/dm-project-system-hooks-in-transaction.yml5
-rw-r--r--lib/gitlab/git/blob.rb22
-rw-r--r--lib/gitlab/git/repository.rb4
-rw-r--r--lib/gitlab/gitaly_client/blob_service.rb20
-rw-r--r--spec/features/issues/spam_issues_spec.rb3
-rw-r--r--spec/lib/gitlab/git/blob_spec.rb43
-rw-r--r--spec/models/project_spec.rb17
-rw-r--r--spec/services/issues/move_service_spec.rb4
9 files changed, 97 insertions, 25 deletions
diff --git a/app/models/project.rb b/app/models/project.rb
index 0570bbc8ee3..e19873f64ce 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -971,9 +971,9 @@ class Project < ActiveRecord::Base
hooks.hooks_for(hooks_scope).each do |hook|
hook.async_execute(data, hooks_scope.to_s)
end
- end
- SystemHooksService.new.execute_hooks(data, hooks_scope)
+ SystemHooksService.new.execute_hooks(data, hooks_scope)
+ end
end
def execute_services(data, hooks_scope = :push_hooks)
diff --git a/changelogs/unreleased/dm-project-system-hooks-in-transaction.yml b/changelogs/unreleased/dm-project-system-hooks-in-transaction.yml
new file mode 100644
index 00000000000..f59021c0ec9
--- /dev/null
+++ b/changelogs/unreleased/dm-project-system-hooks-in-transaction.yml
@@ -0,0 +1,5 @@
+---
+title: Execute system hooks after-commit when executing project hooks
+merge_request:
+author:
+type: fixed
diff --git a/lib/gitlab/git/blob.rb b/lib/gitlab/git/blob.rb
index f421bf69e8f..81e46028752 100644
--- a/lib/gitlab/git/blob.rb
+++ b/lib/gitlab/git/blob.rb
@@ -34,7 +34,7 @@ module Gitlab
def raw(repository, sha)
Gitlab::GitalyClient.migrate(:git_blob_raw) do |is_enabled|
if is_enabled
- Gitlab::GitalyClient::BlobService.new(repository).get_blob(oid: sha, limit: MAX_DATA_DISPLAY_SIZE)
+ repository.gitaly_blob_client.get_blob(oid: sha, limit: MAX_DATA_DISPLAY_SIZE)
else
rugged_raw(repository, sha, limit: MAX_DATA_DISPLAY_SIZE)
end
@@ -70,11 +70,19 @@ module Gitlab
# Returns array of Gitlab::Git::Blob
# Does not guarantee blob data will be set
def batch_lfs_pointers(repository, blob_ids)
- blob_ids.lazy
- .select { |sha| possible_lfs_blob?(repository, sha) }
- .map { |sha| rugged_raw(repository, sha, limit: LFS_POINTER_MAX_SIZE) }
- .select(&:lfs_pointer?)
- .force
+ return [] if blob_ids.empty?
+
+ repository.gitaly_migrate(:batch_lfs_pointers) do |is_enabled|
+ if is_enabled
+ repository.gitaly_blob_client.batch_lfs_pointers(blob_ids)
+ else
+ blob_ids.lazy
+ .select { |sha| possible_lfs_blob?(repository, sha) }
+ .map { |sha| rugged_raw(repository, sha, limit: LFS_POINTER_MAX_SIZE) }
+ .select(&:lfs_pointer?)
+ .force
+ end
+ end
end
def binary?(data)
@@ -258,7 +266,7 @@ module Gitlab
Gitlab::GitalyClient.migrate(:git_blob_load_all_data) do |is_enabled|
@data = begin
if is_enabled
- Gitlab::GitalyClient::BlobService.new(repository).get_blob(oid: id, limit: -1).data
+ repository.gitaly_blob_client.get_blob(oid: id, limit: -1).data
else
repository.lookup(id).content
end
diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb
index 9864e4251f1..d7c712e75c5 100644
--- a/lib/gitlab/git/repository.rb
+++ b/lib/gitlab/git/repository.rb
@@ -1331,6 +1331,10 @@ module Gitlab
@gitaly_remote_client ||= Gitlab::GitalyClient::RemoteService.new(self)
end
+ def gitaly_blob_client
+ @gitaly_blob_client ||= Gitlab::GitalyClient::BlobService.new(self)
+ end
+
def gitaly_conflicts_client(our_commit_oid, their_commit_oid)
Gitlab::GitalyClient::ConflictsService.new(self, our_commit_oid, their_commit_oid)
end
diff --git a/lib/gitlab/gitaly_client/blob_service.rb b/lib/gitlab/gitaly_client/blob_service.rb
index a250eb75bd4..ee36684197b 100644
--- a/lib/gitlab/gitaly_client/blob_service.rb
+++ b/lib/gitlab/gitaly_client/blob_service.rb
@@ -32,6 +32,26 @@ module Gitlab
binary: Gitlab::Git::Blob.binary?(data)
)
end
+
+ def batch_lfs_pointers(blob_ids)
+ request = Gitaly::GetLFSPointersRequest.new(
+ repository: @gitaly_repo,
+ blob_ids: blob_ids
+ )
+
+ response = GitalyClient.call(@gitaly_repo.storage_name, :blob_service, :get_lfs_pointers, request)
+
+ response.flat_map do |message|
+ message.lfs_pointers.map do |lfs_pointer|
+ Gitlab::Git::Blob.new(
+ id: lfs_pointer.oid,
+ size: lfs_pointer.size,
+ data: lfs_pointer.data,
+ binary: Gitlab::Git::Blob.binary?(lfs_pointer.data)
+ )
+ end
+ end
+ end
end
end
end
diff --git a/spec/features/issues/spam_issues_spec.rb b/spec/features/issues/spam_issues_spec.rb
index 53706ef84bc..c7cfd01f588 100644
--- a/spec/features/issues/spam_issues_spec.rb
+++ b/spec/features/issues/spam_issues_spec.rb
@@ -34,6 +34,9 @@ describe 'New issue', :js do
click_button 'Submit issue'
+ # reCAPTCHA alerts when it can't contact the server, so just accept it and move on
+ page.driver.browser.switch_to.alert.accept
+
# it is impossible to test recaptcha automatically and there is no possibility to fill in recaptcha
# recaptcha verification is skipped in test environment and it always returns true
expect(page).not_to have_content('issue title')
diff --git a/spec/lib/gitlab/git/blob_spec.rb b/spec/lib/gitlab/git/blob_spec.rb
index 8706c89c147..168207552ff 100644
--- a/spec/lib/gitlab/git/blob_spec.rb
+++ b/spec/lib/gitlab/git/blob_spec.rb
@@ -260,29 +260,42 @@ describe Gitlab::Git::Blob, seed_helper: true do
)
end
- it 'returns a list of Gitlab::Git::Blob' do
- blobs = described_class.batch_lfs_pointers(repository, [lfs_blob.id])
+ shared_examples 'fetching batch of LFS pointers' do
+ it 'returns a list of Gitlab::Git::Blob' do
+ blobs = described_class.batch_lfs_pointers(repository, [lfs_blob.id])
- expect(blobs.count).to eq(1)
- expect(blobs).to all( be_a(Gitlab::Git::Blob) )
- end
+ expect(blobs.count).to eq(1)
+ expect(blobs).to all( be_a(Gitlab::Git::Blob) )
+ end
- it 'silently ignores tree objects' do
- blobs = described_class.batch_lfs_pointers(repository, [tree_object.oid])
+ it 'silently ignores tree objects' do
+ blobs = described_class.batch_lfs_pointers(repository, [tree_object.oid])
- expect(blobs).to eq([])
- end
+ expect(blobs).to eq([])
+ end
+
+ it 'silently ignores non lfs objects' do
+ blobs = described_class.batch_lfs_pointers(repository, [non_lfs_blob.id])
- it 'silently ignores non lfs objects' do
- blobs = described_class.batch_lfs_pointers(repository, [non_lfs_blob.id])
+ expect(blobs).to eq([])
+ end
+
+ it 'avoids loading large blobs into memory' do
+ # This line could call `lookup` on `repository`, so do here before mocking.
+ non_lfs_blob_id = non_lfs_blob.id
+
+ expect(repository).not_to receive(:lookup)
- expect(blobs).to eq([])
+ described_class.batch_lfs_pointers(repository, [non_lfs_blob_id])
+ end
end
- it 'avoids loading large blobs into memory' do
- expect(repository).not_to receive(:lookup)
+ context 'when Gitaly batch_lfs_pointers is enabled' do
+ it_behaves_like 'fetching batch of LFS pointers'
+ end
- described_class.batch_lfs_pointers(repository, [non_lfs_blob.id])
+ context 'when Gitaly batch_lfs_pointers is disabled', :disable_gitaly do
+ it_behaves_like 'fetching batch of LFS pointers'
end
end
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 4d10df410ab..31dcb543cbd 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -3228,5 +3228,22 @@ describe Project do
project = build(:project)
project.execute_hooks({ data: 'data' }, :merge_request_hooks)
end
+
+ it 'executes the system hooks when inside a transaction' do
+ allow_any_instance_of(WebHookService).to receive(:execute)
+
+ create(:system_hook, merge_requests_events: true)
+
+ project = build(:project)
+
+ # Ideally, we'd test that `WebHookWorker.jobs.size` increased by 1,
+ # but since the entire spec run takes place in a transaction, we never
+ # actually get to the `after_commit` hook that queues these jobs.
+ expect do
+ project.transaction do
+ project.execute_hooks({ data: 'data' }, :merge_request_hooks)
+ end
+ end.not_to raise_error # Sidekiq::Worker::EnqueueFromTransactionError
+ end
end
end
diff --git a/spec/services/issues/move_service_spec.rb b/spec/services/issues/move_service_spec.rb
index f3c98fa5416..388c9d63c7b 100644
--- a/spec/services/issues/move_service_spec.rb
+++ b/spec/services/issues/move_service_spec.rb
@@ -297,9 +297,11 @@ describe Issues::MoveService do
end
context 'project issue hooks' do
- let(:hook) { create(:project_hook, project: old_project, issues_events: true) }
+ let!(:hook) { create(:project_hook, project: old_project, issues_events: true) }
it 'executes project issue hooks' do
+ allow_any_instance_of(WebHookService).to receive(:execute)
+
# Ideally, we'd test that `WebHookWorker.jobs.size` increased by 1,
# but since the entire spec run takes place in a transaction, we never
# actually get to the `after_commit` hook that queues these jobs.
View Lorry Controller Status