summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
Diffstat
-rw-r--r--app/models/concerns/protected_ref.rb4
-rw-r--r--app/models/project.rb5
-rw-r--r--app/models/protected_branch.rb2
-rw-r--r--changelogs/unreleased/osw-remove-n-plus-ones-from-branches-api-call.yml5
-rw-r--r--changelogs/unreleased/retrigger-license-compliance.yml5
-rw-r--r--lib/api/branches.rb2
-rw-r--r--lib/api/entities.rb4
-rw-r--r--lib/api/group_container_repositories.rb2
-rw-r--r--lib/api/helpers.rb11
-rw-r--r--lib/api/project_container_repositories.rb9
-rw-r--r--locale/gitlab.pot15
-rw-r--r--spec/lib/api/helpers_spec.rb14
-rw-r--r--spec/requests/api/branches_spec.rb19
-rw-r--r--spec/requests/api/group_container_repositories_spec.rb2
-rw-r--r--spec/requests/api/project_container_repositories_spec.rb7
-rw-r--r--spec/serializers/pipeline_serializer_spec.rb2
-rw-r--r--spec/support/shared_examples/container_repositories_shared_examples.rb8
17 files changed, 109 insertions, 7 deletions
diff --git a/app/models/concerns/protected_ref.rb b/app/models/concerns/protected_ref.rb
index ebacc459cb5..d9a7f0a96dc 100644
--- a/app/models/concerns/protected_ref.rb
+++ b/app/models/concerns/protected_ref.rb
@@ -39,8 +39,8 @@ module ProtectedRef
end
end
- def developers_can?(action, ref)
- access_levels_for_ref(ref, action: action).any? do |access_level|
+ def developers_can?(action, ref, protected_refs: nil)
+ access_levels_for_ref(ref, action: action, protected_refs: protected_refs).any? do |access_level|
access_level.access_level == Gitlab::Access::DEVELOPER
end
end
diff --git a/app/models/project.rb b/app/models/project.rb
index b44c8d1a655..bafde342157 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -663,6 +663,11 @@ class Project < ApplicationRecord
end
end
+ def preload_protected_branches
+ preloader = ActiveRecord::Associations::Preloader.new
+ preloader.preload(self, protected_branches: [:push_access_levels, :merge_access_levels])
+ end
+
# returns all ancestor-groups upto but excluding the given namespace
# when no namespace is given, all ancestors upto the top are returned
def ancestors_upto(top = nil, hierarchy_order: nil)
diff --git a/app/models/protected_branch.rb b/app/models/protected_branch.rb
index 1857a59e01c..735e2bdea81 100644
--- a/app/models/protected_branch.rb
+++ b/app/models/protected_branch.rb
@@ -38,7 +38,7 @@ class ProtectedBranch < ApplicationRecord
end
def self.protected_refs(project)
- project.protected_branches.select(:name)
+ project.protected_branches
end
def self.branch_requires_code_owner_approval?(project, branch_name)
diff --git a/changelogs/unreleased/osw-remove-n-plus-ones-from-branches-api-call.yml b/changelogs/unreleased/osw-remove-n-plus-ones-from-branches-api-call.yml
new file mode 100644
index 00000000000..5e491c6847e
--- /dev/null
+++ b/changelogs/unreleased/osw-remove-n-plus-ones-from-branches-api-call.yml
@@ -0,0 +1,5 @@
+---
+title: Remove N+1 DB calls from branches API
+merge_request: 19661
+author:
+type: performance
diff --git a/changelogs/unreleased/retrigger-license-compliance.yml b/changelogs/unreleased/retrigger-license-compliance.yml
new file mode 100644
index 00000000000..de02b0417ed
--- /dev/null
+++ b/changelogs/unreleased/retrigger-license-compliance.yml
@@ -0,0 +1,5 @@
+---
+title: Triggers the correct endpoint on licence approval
+merge_request: 19078
+author:
+type: fixed
diff --git a/lib/api/branches.rb b/lib/api/branches.rb
index f8f79ab6f5a..054242dca4c 100644
--- a/lib/api/branches.rb
+++ b/lib/api/branches.rb
@@ -32,7 +32,7 @@ module API
use :filter_params
end
get ':id/repository/branches' do
- Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-foss/issues/42329')
+ user_project.preload_protected_branches
repository = user_project.repository
diff --git a/lib/api/entities.rb b/lib/api/entities.rb
index e6429725757..5819b366f7f 100644
--- a/lib/api/entities.rb
+++ b/lib/api/entities.rb
@@ -489,11 +489,11 @@ module API
end
expose :developers_can_push do |repo_branch, options|
- options[:project].protected_branches.developers_can?(:push, repo_branch.name)
+ ::ProtectedBranch.developers_can?(:push, repo_branch.name, protected_refs: options[:project].protected_branches)
end
expose :developers_can_merge do |repo_branch, options|
- options[:project].protected_branches.developers_can?(:merge, repo_branch.name)
+ ::ProtectedBranch.developers_can?(:merge, repo_branch.name, protected_refs: options[:project].protected_branches)
end
expose :can_push do |repo_branch, options|
diff --git a/lib/api/group_container_repositories.rb b/lib/api/group_container_repositories.rb
index d1aaafec8aa..7f95b411b36 100644
--- a/lib/api/group_container_repositories.rb
+++ b/lib/api/group_container_repositories.rb
@@ -26,6 +26,8 @@ module API
user: current_user, subject: user_group
).execute
+ track_event('list_repositories')
+
present paginate(repositories), with: Entities::ContainerRegistry::Repository, tags: params[:tags]
end
end
diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb
index e740134f85f..ca49687081d 100644
--- a/lib/api/helpers.rb
+++ b/lib/api/helpers.rb
@@ -455,6 +455,17 @@ module API
end
end
+ def track_event(action = action_name, **args)
+ category = args.delete(:category) || self.options[:for].name
+ raise "invalid category" unless category
+
+ ::Gitlab::Tracking.event(category, action.to_s, **args)
+ rescue => error
+ Rails.logger.warn( # rubocop:disable Gitlab/RailsLogger
+ "Tracking event failed for action: #{action}, category: #{category}, message: #{error.message}"
+ )
+ end
+
protected
def project_finder_params_ce
diff --git a/lib/api/project_container_repositories.rb b/lib/api/project_container_repositories.rb
index c7731b70f39..2b33069e324 100644
--- a/lib/api/project_container_repositories.rb
+++ b/lib/api/project_container_repositories.rb
@@ -27,6 +27,8 @@ module API
user: current_user, subject: user_project
).execute
+ track_event( 'list_repositories')
+
present paginate(repositories), with: Entities::ContainerRegistry::Repository, tags: params[:tags]
end
@@ -40,6 +42,7 @@ module API
authorize_admin_container_image!
DeleteContainerRepositoryWorker.perform_async(current_user.id, repository.id)
+ track_event('delete_repository')
status :accepted
end
@@ -56,6 +59,8 @@ module API
authorize_read_container_image!
tags = Kaminari.paginate_array(repository.tags)
+ track_event('list_tags')
+
present paginate(tags), with: Entities::ContainerRegistry::Tag
end
@@ -77,6 +82,8 @@ module API
CleanupContainerRepositoryWorker.perform_async(current_user.id, repository.id,
declared_params.except(:repository_id))
+ track_event('delete_tag_bulk')
+
status :accepted
end
@@ -111,6 +118,8 @@ module API
.execute(repository)
if result[:status] == :success
+ track_event('delete_tag')
+
status :ok
else
status :bad_request
diff --git a/locale/gitlab.pot b/locale/gitlab.pot
index 67e9627c7c8..4501366a318 100644
--- a/locale/gitlab.pot
+++ b/locale/gitlab.pot
@@ -11842,15 +11842,30 @@ msgstr ""
msgid "Package was removed"
msgstr ""
+msgid "PackageRegistry|Delete Package"
+msgstr ""
+
msgid "PackageRegistry|Delete Package Version"
msgstr ""
+msgid "PackageRegistry|Learn how to %{noPackagesLinkStart}publish and share your packages%{noPackagesLinkEnd} with GitLab."
+msgstr ""
+
+msgid "PackageRegistry|Remove package"
+msgstr ""
+
+msgid "PackageRegistry|There are no packages yet"
+msgstr ""
+
msgid "PackageRegistry|There was a problem fetching the details for this package."
msgstr ""
msgid "PackageRegistry|Unable to load package"
msgstr ""
+msgid "PackageRegistry|You are about to delete <b>%{packageName}</b>, this operation is irreversible, are you sure?"
+msgstr ""
+
msgid "PackageRegistry|You are about to delete version %{boldStart}%{version}%{boldEnd} of %{boldStart}%{name}%{boldEnd}. Are you sure?"
msgstr ""
diff --git a/spec/lib/api/helpers_spec.rb b/spec/lib/api/helpers_spec.rb
index 0624c25e734..81c4563feb6 100644
--- a/spec/lib/api/helpers_spec.rb
+++ b/spec/lib/api/helpers_spec.rb
@@ -174,4 +174,18 @@ describe API::Helpers do
end
end
end
+
+ describe '#track_event' do
+ it "creates a gitlab tracking event" do
+ expect(Gitlab::Tracking).to receive(:event).with('foo', 'my_event', {})
+
+ subject.track_event('my_event', category: 'foo')
+ end
+
+ it "logs an exception" do
+ expect(Rails.logger).to receive(:warn).with(/Tracking event failed/)
+
+ subject.track_event('my_event', category: nil)
+ end
+ end
end
diff --git a/spec/requests/api/branches_spec.rb b/spec/requests/api/branches_spec.rb
index aecd1d0e71f..675b06b057c 100644
--- a/spec/requests/api/branches_spec.rb
+++ b/spec/requests/api/branches_spec.rb
@@ -119,6 +119,25 @@ describe API::Branches do
it_behaves_like 'repository branches'
end
+
+ it 'does not submit N+1 DB queries', :request_store do
+ create(:protected_branch, name: 'master', project: project)
+
+ # Make sure no setup step query is recorded.
+ get api(route, current_user), params: { per_page: 100 }
+
+ control = ActiveRecord::QueryRecorder.new do
+ get api(route, current_user), params: { per_page: 100 }
+ end
+
+ new_branch_name = 'protected-branch'
+ CreateBranchService.new(project, current_user).execute(new_branch_name, 'master')
+ create(:protected_branch, name: new_branch_name, project: project)
+
+ expect do
+ get api(route, current_user), params: { per_page: 100 }
+ end.not_to exceed_query_limit(control)
+ end
end
context 'when authenticated', 'as a guest' do
diff --git a/spec/requests/api/group_container_repositories_spec.rb b/spec/requests/api/group_container_repositories_spec.rb
index 0b75b056fb7..785006253d8 100644
--- a/spec/requests/api/group_container_repositories_spec.rb
+++ b/spec/requests/api/group_container_repositories_spec.rb
@@ -44,6 +44,8 @@ describe API::GroupContainerRepositories do
let(:object) { group }
end
+ it_behaves_like 'a gitlab tracking event', described_class.name, 'list_repositories'
+
context 'with invalid group id' do
let(:url) { '/groups/123412341234/registry/repositories' }
diff --git a/spec/requests/api/project_container_repositories_spec.rb b/spec/requests/api/project_container_repositories_spec.rb
index 66fb3c6b2d6..d04db134db0 100644
--- a/spec/requests/api/project_container_repositories_spec.rb
+++ b/spec/requests/api/project_container_repositories_spec.rb
@@ -46,6 +46,7 @@ describe API::ProjectContainerRepositories do
it_behaves_like 'rejected container repository access', :guest, :forbidden
it_behaves_like 'rejected container repository access', :anonymous, :not_found
+ it_behaves_like 'a gitlab tracking event', described_class.name, 'list_repositories'
it_behaves_like 'returns repositories for allowed users', :reporter, 'project' do
let(:object) { project }
@@ -57,6 +58,7 @@ describe API::ProjectContainerRepositories do
it_behaves_like 'rejected container repository access', :developer, :forbidden
it_behaves_like 'rejected container repository access', :anonymous, :not_found
+ it_behaves_like 'a gitlab tracking event', described_class.name, 'delete_repository'
context 'for maintainer' do
let(:api_user) { maintainer }
@@ -85,6 +87,8 @@ describe API::ProjectContainerRepositories do
stub_container_registry_tags(repository: root_repository.path, tags: %w(rootA latest))
end
+ it_behaves_like 'a gitlab tracking event', described_class.name, 'list_tags'
+
it 'returns a list of tags' do
subject
@@ -111,6 +115,7 @@ describe API::ProjectContainerRepositories do
it_behaves_like 'rejected container repository access', :developer, :forbidden
it_behaves_like 'rejected container repository access', :anonymous, :not_found
+ it_behaves_like 'a gitlab tracking event', described_class.name, 'delete_tag_bulk'
end
context 'for maintainer' do
@@ -222,6 +227,7 @@ describe API::ProjectContainerRepositories do
it 'properly removes tag' do
expect(service).to receive(:execute).with(root_repository) { { status: :success } }
expect(Projects::ContainerRepository::DeleteTagsService).to receive(:new).with(root_repository.project, api_user, tags: %w[rootA]) { service }
+ expect(Gitlab::Tracking).to receive(:event).with(described_class.name, 'delete_tag', {})
subject
@@ -237,6 +243,7 @@ describe API::ProjectContainerRepositories do
it 'properly removes tag' do
expect(service).to receive(:execute).with(root_repository) { { status: :success } }
expect(Projects::ContainerRepository::DeleteTagsService).to receive(:new).with(root_repository.project, api_user, tags: %w[rootA]) { service }
+ expect(Gitlab::Tracking).to receive(:event).with(described_class.name, 'delete_tag', {})
subject
diff --git a/spec/serializers/pipeline_serializer_spec.rb b/spec/serializers/pipeline_serializer_spec.rb
index ce5264ec8bb..7661c8acc13 100644
--- a/spec/serializers/pipeline_serializer_spec.rb
+++ b/spec/serializers/pipeline_serializer_spec.rb
@@ -180,7 +180,7 @@ describe PipelineSerializer do
# pipeline. With the same ref this check is cached but if refs are
# different then there is an extra query per ref
# https://gitlab.com/gitlab-org/gitlab-foss/issues/46368
- expected_queries = Gitlab.ee? ? 44 : 41
+ expected_queries = Gitlab.ee? ? 41 : 38
expect(recorded.count).to be_within(2).of(expected_queries)
expect(recorded.cached_count).to eq(0)
diff --git a/spec/support/shared_examples/container_repositories_shared_examples.rb b/spec/support/shared_examples/container_repositories_shared_examples.rb
index 946b130fca2..b4f45ba9a00 100644
--- a/spec/support/shared_examples/container_repositories_shared_examples.rb
+++ b/spec/support/shared_examples/container_repositories_shared_examples.rb
@@ -56,3 +56,11 @@ shared_examples 'returns repositories for allowed users' do |user_type, scope|
end
end
end
+
+shared_examples 'a gitlab tracking event' do |category, action|
+ it "creates a gitlab tracking event #{action}" do
+ expect(Gitlab::Tracking).to receive(:event).with(category, action, {})
+
+ subject
+ end
+end
View Lorry Controller Status