diff --git a/app/models/application_record.rb b/app/models/application_record.rb index c7e4d64d3d5..78ca2c02174 100644 --- a/app/models/application_record.rb +++ b/app/models/application_record.rb @@ -13,6 +13,10 @@ class ApplicationRecord < ActiveRecord::Base where(id: ids) end + def self.iid_in(iids) + where(iid: iids) + end + def self.id_not_in(ids) where.not(id: ids) end diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index c6437f66148..09f600926c9 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -265,6 +265,7 @@ class MergeRequest < ApplicationRecord *PROJECT_ROUTE_AND_NAMESPACE_ROUTE, metrics: [:latest_closed_by, :merged_by]) } + scope :by_target_branch_wildcard, ->(wildcard_branch_name) do where("target_branch LIKE ?", ApplicationRecord.sanitize_sql_like(wildcard_branch_name).tr('*', '%')) end diff --git a/app/models/namespace.rb b/app/models/namespace.rb index 9a829c41f6e..feac98fc72f 100644 --- a/app/models/namespace.rb +++ b/app/models/namespace.rb @@ -287,6 +287,8 @@ class Namespace < ApplicationRecord end def root_ancestor + return self if persisted? && parent_id.nil? + strong_memoize(:root_ancestor) do self_and_ancestors.reorder(nil).find_by(parent_id: nil) end diff --git a/app/models/project.rb b/app/models/project.rb index f60b5490757..d2259ba61f9 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -524,7 +524,7 @@ class Project < ApplicationRecord scope :with_api_entity_associations, -> { preload(:project_feature, :route, :tags, - group: :ip_restrictions, namespace: [:route, :owner]) + group: [:ip_restrictions, :saml_provider], namespace: [:route, :owner]) } scope :with_api_commit_entity_associations, -> { @@ -602,6 +602,14 @@ class Project < ApplicationRecord end end + def self.projects_user_can(projects, user, action) + projects = where(id: projects) + + DeclarativePolicy.user_scope do + projects.select { |project| Ability.allowed?(user, action, project) } + end + end + # This scope returns projects where user has access to both the project and the feature. def self.filter_by_feature_visibility(feature, user) with_feature_available_for_user(feature, user) diff --git a/changelogs/unreleased/214905-measure-package-adoption.yml b/changelogs/unreleased/214905-measure-package-adoption.yml new file mode 100644 index 00000000000..a502d067712 --- /dev/null +++ b/changelogs/unreleased/214905-measure-package-adoption.yml @@ -0,0 +1,5 @@ +--- +title: Measure adoption of package registry +merge_request: 36514 +author: +type: added diff --git a/changelogs/unreleased/218703-fix-api-projects-search-n-plus-1.yml b/changelogs/unreleased/218703-fix-api-projects-search-n-plus-1.yml new file mode 100644 index 00000000000..c5587a8be8d --- /dev/null +++ b/changelogs/unreleased/218703-fix-api-projects-search-n-plus-1.yml @@ -0,0 +1,5 @@ +--- +title: Resolve N+1 in Search API projects scope +merge_request: 35833 +author: +type: performance diff --git a/changelogs/unreleased/221211-improve-performance-of-group-search-api-advanced-merge_requests-sc.yml b/changelogs/unreleased/221211-improve-performance-of-group-search-api-advanced-merge_requests-sc.yml new file mode 100644 index 00000000000..f951f88320e --- /dev/null +++ b/changelogs/unreleased/221211-improve-performance-of-group-search-api-advanced-merge_requests-sc.yml @@ -0,0 +1,5 @@ +--- +title: Improve the search performance for merge requests +merge_request: 36072 +author: +type: performance diff --git a/doc/api/graphql/reference/gitlab_schema.graphql b/doc/api/graphql/reference/gitlab_schema.graphql index c77e5ecc185..087559f204c 100644 --- a/doc/api/graphql/reference/gitlab_schema.graphql +++ b/doc/api/graphql/reference/gitlab_schema.graphql @@ -5094,6 +5094,11 @@ type Group { """ id: ID + """ + The internal ID of the Iteration to look up + """ + iid: ID + """ Returns the last _n_ elements from the list. """ @@ -6301,6 +6306,11 @@ type Iteration { """ id: ID! + """ + Internal ID of the iteration + """ + iid: ID! + """ Timestamp of the iteration start date """ diff --git a/doc/api/graphql/reference/gitlab_schema.json b/doc/api/graphql/reference/gitlab_schema.json index 3f96d1235cd..6c0c81852a4 100644 --- a/doc/api/graphql/reference/gitlab_schema.json +++ b/doc/api/graphql/reference/gitlab_schema.json @@ -14100,6 +14100,16 @@ }, "defaultValue": null }, + { + "name": "iid", + "description": "The internal ID of the Iteration to look up", + "type": { + "kind": "SCALAR", + "name": "ID", + "ofType": null + }, + "defaultValue": null + }, { "name": "after", "description": "Returns the elements in the list that come after the specified cursor.", @@ -17329,6 +17339,24 @@ "isDeprecated": false, "deprecationReason": null }, + { + "name": "iid", + "description": "Internal ID of the iteration", + "args": [ + + ], + "type": { + "kind": "NON_NULL", + "name": null, + "ofType": { + "kind": "SCALAR", + "name": "ID", + "ofType": null + } + }, + "isDeprecated": false, + "deprecationReason": null + }, { "name": "startDate", "description": "Timestamp of the iteration start date", diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index c8b8136c294..4327c13d775 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -946,6 +946,7 @@ Represents an iteration object. | `description` | String | Description of the iteration | | `dueDate` | Time | Timestamp of the iteration due date | | `id` | ID! | ID of the iteration | +| `iid` | ID! | Internal ID of the iteration | | `startDate` | Time | Timestamp of the iteration start date | | `state` | IterationState! | State of the iteration | | `title` | String! | Title of the iteration | diff --git a/doc/ci/examples/README.md b/doc/ci/examples/README.md index e915f28c05c..7213151dcd8 100644 --- a/doc/ci/examples/README.md +++ b/doc/ci/examples/README.md @@ -61,6 +61,7 @@ choose one of these templates: - [C++ (`C++.gitlab-ci.yml`)](https://gitlab.com/gitlab-org/gitlab/-/blob/master/lib/gitlab/ci/templates/C++.gitlab-ci.yml) - [Chef (`Chef.gitlab-ci.yml`)](https://gitlab.com/gitlab-org/gitlab/-/blob/master/lib/gitlab/ci/templates/Chef.gitlab-ci.yml) - [Clojure (`Clojure.gitlab-ci.yml`)](https://gitlab.com/gitlab-org/gitlab/-/blob/master/lib/gitlab/ci/templates/Clojure.gitlab-ci.yml) +- [Composer `Composer.gitlab-ci.yml`](https://gitlab.com/gitlab-org/gitlab/-/blob/master/lib/gitlab/ci/templates/Composer.gitlab-ci.yml) - [Crystal (`Crystal.gitlab-ci.yml`)](https://gitlab.com/gitlab-org/gitlab/-/blob/master/lib/gitlab/ci/templates/Crystal.gitlab-ci.yml) - [Django (`Django.gitlab-ci.yml`)](https://gitlab.com/gitlab-org/gitlab/-/blob/master/lib/gitlab/ci/templates/Django.gitlab-ci.yml) - [Docker (`Docker.gitlab-ci.yml`)](https://gitlab.com/gitlab-org/gitlab/-/blob/master/lib/gitlab/ci/templates/Docker.gitlab-ci.yml) @@ -76,6 +77,7 @@ choose one of these templates: - [LaTeX (`LaTeX.gitlab-ci.yml`)](https://gitlab.com/gitlab-org/gitlab/-/blob/master/lib/gitlab/ci/templates/LaTeX.gitlab-ci.yml) - [Maven (`Maven.gitlab-ci.yml`)](https://gitlab.com/gitlab-org/gitlab/-/blob/master/lib/gitlab/ci/templates/Maven.gitlab-ci.yml) - [Mono (`Mono.gitlab-ci.yml`)](https://gitlab.com/gitlab-org/gitlab/-/blob/master/lib/gitlab/ci/templates/Mono.gitlab-ci.yml) +- [NPM (`npm.gitlab-ci.yml`)](https://gitlab.com/gitlab-org/gitlab/-/blob/master/lib/gitlab/ci/templates/npm.gitlab-ci.yml) - [Node.js (`Nodejs.gitlab-ci.yml`)](https://gitlab.com/gitlab-org/gitlab/-/blob/master/lib/gitlab/ci/templates/Nodejs.gitlab-ci.yml) - [OpenShift (`OpenShift.gitlab-ci.yml`)](https://gitlab.com/gitlab-org/gitlab/-/blob/master/lib/gitlab/ci/templates/OpenShift.gitlab-ci.yml) - [Packer (`Packer.gitlab-ci.yml`)](https://gitlab.com/gitlab-org/gitlab/-/blob/master/lib/gitlab/ci/templates/Packer.gitlab-ci.yml) diff --git a/lib/api/entities/issuable_entity.rb b/lib/api/entities/issuable_entity.rb index 784bb8d57ed..e2c674c0b8b 100644 --- a/lib/api/entities/issuable_entity.rb +++ b/lib/api/entities/issuable_entity.rb @@ -8,16 +8,39 @@ module API expose :title, :description expose :state, :created_at, :updated_at - # Avoids an N+1 query when metadata is included - def issuable_metadata(subject, options, method, args = nil) - cached_subject = options.dig(:issuable_metadata, subject.id) + def presented + lazy_issuable_metadata - if cached_subject - cached_subject[method] - else - subject.public_send(method, *args) # rubocop: disable GitlabSecurity/PublicSend + super + end + + def issuable_metadata + options.dig(:issuable_metadata, object.id) || lazy_issuable_metadata + end + + protected + + # This method will preload the `issuable_metadata` for the current + # entity according to the current top-level entity options, such + # as the current_user. + def lazy_issuable_metadata + BatchLoader.for(object).batch(key: [current_user, :issuable_metadata]) do |models, loader, args| + current_user = args[:key].first + + issuable_metadata = Gitlab::IssuableMetadata.new(current_user, models) + metadata_by_id = issuable_metadata.data + + models.each do |issuable| + loader.call(issuable, metadata_by_id[issuable.id]) + end end end + + private + + def current_user + options[:current_user] + end end end end diff --git a/lib/api/entities/issue_basic.rb b/lib/api/entities/issue_basic.rb index af92f4124f1..cf96c6556ec 100644 --- a/lib/api/entities/issue_basic.rb +++ b/lib/api/entities/issue_basic.rb @@ -21,10 +21,10 @@ module API issue.assignees.first end - expose(:user_notes_count) { |issue, options| issuable_metadata(issue, options, :user_notes_count) } - expose(:merge_requests_count) { |issue, options| issuable_metadata(issue, options, :merge_requests_count, options[:current_user]) } - expose(:upvotes) { |issue, options| issuable_metadata(issue, options, :upvotes) } - expose(:downvotes) { |issue, options| issuable_metadata(issue, options, :downvotes) } + expose(:user_notes_count) { |issue, options| issuable_metadata.user_notes_count } + expose(:merge_requests_count) { |issue, options| issuable_metadata.merge_requests_count } + expose(:upvotes) { |issue, options| issuable_metadata.upvotes } + expose(:downvotes) { |issue, options| issuable_metadata.downvotes } expose :due_date expose :confidential expose :discussion_locked diff --git a/lib/api/entities/merge_request_basic.rb b/lib/api/entities/merge_request_basic.rb index 1643f267938..69523e3637b 100644 --- a/lib/api/entities/merge_request_basic.rb +++ b/lib/api/entities/merge_request_basic.rb @@ -22,13 +22,11 @@ module API MarkupHelper.markdown_field(entity, :description) end expose :target_branch, :source_branch - expose(:user_notes_count) { |merge_request, options| issuable_metadata(merge_request, options, :user_notes_count) } - expose(:upvotes) { |merge_request, options| issuable_metadata(merge_request, options, :upvotes) } - expose(:downvotes) { |merge_request, options| issuable_metadata(merge_request, options, :downvotes) } - expose :assignee, using: ::API::Entities::UserBasic do |merge_request| - merge_request.assignee - end - expose :author, :assignees, using: Entities::UserBasic + expose(:user_notes_count) { |merge_request, options| issuable_metadata.user_notes_count } + expose(:upvotes) { |merge_request, options| issuable_metadata.upvotes } + expose(:downvotes) { |merge_request, options| issuable_metadata.downvotes } + + expose :author, :assignees, :assignee, using: Entities::UserBasic expose :source_project_id, :target_project_id expose :labels do |merge_request, options| if options[:with_labels_details] @@ -57,9 +55,12 @@ module API expose :discussion_locked expose :should_remove_source_branch?, as: :should_remove_source_branch expose :force_remove_source_branch?, as: :force_remove_source_branch - expose :allow_collaboration, if: -> (merge_request, _) { merge_request.for_fork? } - # Deprecated - expose :allow_collaboration, as: :allow_maintainer_to_push, if: -> (merge_request, _) { merge_request.for_fork? } + + with_options if: -> (merge_request, _) { merge_request.for_fork? } do + expose :allow_collaboration + # Deprecated + expose :allow_collaboration, as: :allow_maintainer_to_push + end # reference is deprecated in favour of references # Introduced [Gitlab 12.6](https://gitlab.com/gitlab-org/gitlab/merge_requests/20354) diff --git a/lib/api/issues.rb b/lib/api/issues.rb index de24de291ec..455511caabb 100644 --- a/lib/api/issues.rb +++ b/lib/api/issues.rb @@ -107,7 +107,6 @@ module API with: Entities::Issue, with_labels_details: declared_params[:with_labels_details], current_user: current_user, - issuable_metadata: Gitlab::IssuableMetadata.new(current_user, issues).data, include_subscribed: false } @@ -133,7 +132,6 @@ module API with: Entities::Issue, with_labels_details: declared_params[:with_labels_details], current_user: current_user, - issuable_metadata: Gitlab::IssuableMetadata.new(current_user, issues).data, include_subscribed: false, group: user_group } @@ -170,7 +168,6 @@ module API with_labels_details: declared_params[:with_labels_details], current_user: current_user, project: user_project, - issuable_metadata: Gitlab::IssuableMetadata.new(current_user, issues).data, include_subscribed: false } diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb index 49493b36d10..ba42d9f206d 100644 --- a/lib/api/merge_requests.rb +++ b/lib/api/merge_requests.rb @@ -93,7 +93,6 @@ module API if params[:view] == 'simple' options[:with] = Entities::MergeRequestSimple else - options[:issuable_metadata] = Gitlab::IssuableMetadata.new(current_user, merge_requests).data options[:skip_merge_status_recheck] = !declared_params[:with_merge_status_recheck] end diff --git a/lib/gitlab/search_results.rb b/lib/gitlab/search_results.rb index 6239158ef06..3d5f64ce05b 100644 --- a/lib/gitlab/search_results.rb +++ b/lib/gitlab/search_results.rb @@ -27,6 +27,7 @@ module Gitlab end def objects(scope, page: nil, per_page: DEFAULT_PER_PAGE, without_count: true, preload_method: nil) + should_preload = preload_method.present? collection = case scope when 'projects' projects @@ -39,9 +40,11 @@ module Gitlab when 'users' users else + should_preload = false Kaminari.paginate_array([]) end + collection = collection.public_send(preload_method) if should_preload # rubocop:disable GitlabSecurity/PublicSend collection = collection.page(page).per(per_page) without_count ? collection.without_count : collection diff --git a/spec/lib/api/entities/merge_request_basic_spec.rb b/spec/lib/api/entities/merge_request_basic_spec.rb new file mode 100644 index 00000000000..715fcf4bcdb --- /dev/null +++ b/spec/lib/api/entities/merge_request_basic_spec.rb @@ -0,0 +1,43 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ::API::Entities::MergeRequestBasic do + let_it_be(:user) { create(:user) } + let_it_be(:project) { create(:project, :public) } + let_it_be(:merge_request) { create(:merge_request) } + let_it_be(:labels) { create_list(:label, 3) } + let_it_be(:merge_requests) { create_list(:labeled_merge_request, 10, :unique_branches, :with_diffs, labels: labels) } + + # This mimics the behavior of the `Grape::Entity` serializer + def present(obj) + described_class.new(obj).presented + end + + context "with :with_api_entity_associations scope" do + let(:scope) { MergeRequest.with_api_entity_associations } + + it "avoids N+1 queries" do + query = scope.find(merge_request.id) + + control = ActiveRecord::QueryRecorder.new do + present(query).to_json + end + + # stub the `head_commit_sha` as it will trigger a + # backward compatibility query that is out-of-scope + # for this test whenever it is `nil` + allow_any_instance_of(MergeRequestDiff).to receive(:head_commit_sha).and_return(Gitlab::Git::BLANK_SHA) + + query = scope.all + batch = ActiveRecord::QueryRecorder.new do + entities = query.map(&method(:present)) + + entities.to_json + end + + # The current threshold is 3 query per entity maximum. + expect(batch.count).to be_within(3 * query.count).of(control.count) + end + end +end diff --git a/spec/models/namespace_spec.rb b/spec/models/namespace_spec.rb index 7ba21b1aa46..4d137f60f9a 100644 --- a/spec/models/namespace_spec.rb +++ b/spec/models/namespace_spec.rb @@ -884,8 +884,13 @@ RSpec.describe Namespace do end describe '#root_ancestor' do + let!(:root_group) { create(:group) } + + it 'returns root_ancestor for root group without a query' do + expect { root_group.root_ancestor }.not_to exceed_query_limit(0) + end + it 'returns the top most ancestor' do - root_group = create(:group) nested_group = create(:group, parent: root_group) deep_nested_group = create(:group, parent: nested_group) very_deep_nested_group = create(:group, parent: deep_nested_group)