Commit a6254f40 authored by Terri Chu's avatar Terri Chu Committed by Mayra Cabrera

Revert "Revert "Merge branch...

Revert "Revert "Merge branch '215708-improve-performance-of-search-api-advanced-commits-scope' into 'master'""

This reverts commit 39e9826e.
parent e27a4b3c
...@@ -525,6 +525,10 @@ class Project < ApplicationRecord ...@@ -525,6 +525,10 @@ class Project < ApplicationRecord
group: :ip_restrictions, namespace: [:route, :owner]) group: :ip_restrictions, namespace: [:route, :owner])
} }
scope :with_api_commit_entity_associations, -> {
preload(:project_feature, :route, namespace: [:route, :owner])
}
enum auto_cancel_pending_pipelines: { disabled: 0, enabled: 1 } enum auto_cancel_pending_pipelines: { disabled: 0, enabled: 1 }
chronic_duration_attr :build_timeout_human_readable, :build_timeout, chronic_duration_attr :build_timeout_human_readable, :build_timeout,
......
---
title: Fix N+1 queries for Elastic Search commits scope.
merge_request: 35449
author:
type: performance
...@@ -181,7 +181,7 @@ module Elastic ...@@ -181,7 +181,7 @@ module Elastic
# Wrap returned results into GitLab model objects and paginate # Wrap returned results into GitLab model objects and paginate
# #
# @return [Kaminari::PaginatableArray] # @return [Kaminari::PaginatableArray]
def elastic_search_and_wrap(query, type:, page: 1, per: 20, options: {}, &blk) def elastic_search_and_wrap(query, type:, page: 1, per: 20, options: {}, preload_method: nil, &blk)
response = elastic_search( response = elastic_search(
query, query,
type: type, type: type,
...@@ -190,17 +190,19 @@ module Elastic ...@@ -190,17 +190,19 @@ module Elastic
options: options options: options
)[type.pluralize.to_sym][:results] )[type.pluralize.to_sym][:results]
items, total_count = yield_each_search_result(response, type, &blk) items, total_count = yield_each_search_result(response, type, preload_method, &blk)
# Before "map" we had a paginated array so we need to recover it # Before "map" we had a paginated array so we need to recover it
offset = per * ((page || 1) - 1) offset = per * ((page || 1) - 1)
Kaminari.paginate_array(items, total_count: total_count, limit: per, offset: offset) Kaminari.paginate_array(items, total_count: total_count, limit: per, offset: offset)
end end
def yield_each_search_result(response, type) def yield_each_search_result(response, type, preload_method)
# Avoid one SELECT per result by loading all projects into a hash # Avoid one SELECT per result by loading all projects into a hash
project_ids = response.map { |result| project_id_for_commit_or_blob(result, type) }.uniq project_ids = response.map { |result| project_id_for_commit_or_blob(result, type) }.uniq
projects = Project.with_route.id_in(project_ids).index_by(&:id) projects = Project.with_route.id_in(project_ids)
projects = projects.public_send(preload_method) if preload_method # rubocop:disable GitlabSecurity/PublicSend
projects = projects.index_by(&:id)
total_count = response.total_count total_count = response.total_count
items = response.map do |result| items = response.map do |result|
......
...@@ -10,8 +10,8 @@ module Elastic ...@@ -10,8 +10,8 @@ module Elastic
end end
# @return [Kaminari::PaginatableArray] # @return [Kaminari::PaginatableArray]
def find_commits_by_message_with_elastic(query, page: 1, per_page: 20, options: {}) def find_commits_by_message_with_elastic(query, page: 1, per_page: 20, options: {}, preload_method: nil)
elastic_search_and_wrap(query, type: 'commit', page: page, per: per_page, options: options) do |result, project| elastic_search_and_wrap(query, type: 'commit', page: page, per: per_page, options: options, preload_method: preload_method) do |result, project|
raw_commit = Gitlab::Git::Commit.new( raw_commit = Gitlab::Git::Commit.new(
project.repository.raw, project.repository.raw,
prepare_commit(result['_source']['commit']), prepare_commit(result['_source']['commit']),
......
...@@ -8,7 +8,7 @@ module Elastic ...@@ -8,7 +8,7 @@ module Elastic
delegate :project, to: :target delegate :project, to: :target
delegate :id, to: :project, prefix: true delegate :id, to: :project, prefix: true
def find_commits_by_message_with_elastic(query, page: 1, per_page: 20) def find_commits_by_message_with_elastic(query, page: 1, per_page: 20, preload_method: nil)
response = elastic_search(query, type: 'commit', page: page, per: per_page)[:commits][:results] response = elastic_search(query, type: 'commit', page: page, per: per_page)[:commits][:results]
commits = response.map do |result| commits = response.map do |result|
......
...@@ -61,7 +61,7 @@ module Gitlab ...@@ -61,7 +61,7 @@ module Gitlab
Note.elastic_search(query, options: opt) Note.elastic_search(query, options: opt)
end end
def commits(page: 1, per_page: DEFAULT_PER_PAGE) def commits(page: 1, per_page: DEFAULT_PER_PAGE, preload_method: nil)
return Kaminari.paginate_array([]) unless Ability.allowed?(@current_user, :download_code, project) return Kaminari.paginate_array([]) unless Ability.allowed?(@current_user, :download_code, project)
if project.empty_repo? || query.blank? if project.empty_repo? || query.blank?
...@@ -72,7 +72,8 @@ module Gitlab ...@@ -72,7 +72,8 @@ module Gitlab
project.repository.find_commits_by_message_with_elastic( project.repository.find_commits_by_message_with_elastic(
query, query,
page: (page || 1).to_i, page: (page || 1).to_i,
per_page: per_page per_page: per_page,
preload_method: preload_method
) )
else else
offset = per_page * ((page || 1) - 1) offset = per_page * ((page || 1) - 1)
......
...@@ -43,7 +43,7 @@ module Gitlab ...@@ -43,7 +43,7 @@ module Gitlab
when 'wiki_blobs' when 'wiki_blobs'
wiki_blobs(page: page, per_page: per_page) wiki_blobs(page: page, per_page: per_page)
when 'commits' when 'commits'
commits(page: page, per_page: per_page) commits(page: page, per_page: per_page, preload_method: preload_method)
when 'users' when 'users'
users.page(page).per(per_page) users.page(page).per(per_page)
else else
...@@ -270,7 +270,7 @@ module Gitlab ...@@ -270,7 +270,7 @@ module Gitlab
# hitting ES twice for any page that's not page 1, and that's something we want to avoid. # hitting ES twice for any page that's not page 1, and that's something we want to avoid.
# #
# It is safe to memoize the page we get here because this method is _always_ called before `#commits_count` # It is safe to memoize the page we get here because this method is _always_ called before `#commits_count`
def commits(page: 1, per_page: DEFAULT_PER_PAGE) def commits(page: 1, per_page: DEFAULT_PER_PAGE, preload_method: nil)
return Kaminari.paginate_array([]) if query.blank? return Kaminari.paginate_array([]) if query.blank?
strong_memoize(:commits) do strong_memoize(:commits) do
...@@ -282,7 +282,8 @@ module Gitlab ...@@ -282,7 +282,8 @@ module Gitlab
query, query,
page: (page || 1).to_i, page: (page || 1).to_i,
per_page: per_page, per_page: per_page,
options: options options: options,
preload_method: preload_method
) )
end end
end end
......
...@@ -102,7 +102,7 @@ RSpec.describe API::Search do ...@@ -102,7 +102,7 @@ RSpec.describe API::Search do
it_behaves_like 'pagination', scope: 'wiki_blobs' it_behaves_like 'pagination', scope: 'wiki_blobs'
end end
context 'for commits scope', :sidekiq_might_not_need_inline do context 'for commits scope', :sidekiq_inline do
before do before do
project.repository.index_commits_and_blobs project.repository.index_commits_and_blobs
ensure_elasticsearch_index! ensure_elasticsearch_index!
...@@ -113,6 +113,20 @@ RSpec.describe API::Search do ...@@ -113,6 +113,20 @@ RSpec.describe API::Search do
it_behaves_like 'response is correct', schema: 'public_api/v4/commits_details', size: 2 it_behaves_like 'response is correct', schema: 'public_api/v4/commits_details', size: 2
it_behaves_like 'pagination', scope: 'commits' it_behaves_like 'pagination', scope: 'commits'
it 'avoids N+1 queries' do
control = ActiveRecord::QueryRecorder.new { get api(endpoint, user), params: { scope: 'commits', search: 'folder' } }
project_2 = create(:project, :public, :repository, :wiki_repo, name: 'awesome project 2')
project_3 = create(:project, :public, :repository, :wiki_repo, name: 'awesome project 3')
project_2.repository.index_commits_and_blobs
project_3.repository.index_commits_and_blobs
ensure_elasticsearch_index!
# Some N+1 queries still exist
expect { get api(endpoint, user), params: { scope: 'commits', search: 'folder' } }.not_to exceed_query_limit(control.count + 9)
end
end end
context 'for blobs scope', :sidekiq_might_not_need_inline do context 'for blobs scope', :sidekiq_might_not_need_inline do
......
...@@ -24,7 +24,8 @@ module API ...@@ -24,7 +24,8 @@ module API
merge_requests: :with_api_entity_associations, merge_requests: :with_api_entity_associations,
projects: :with_api_entity_associations, projects: :with_api_entity_associations,
issues: :with_api_entity_associations, issues: :with_api_entity_associations,
milestones: :with_api_entity_associations milestones: :with_api_entity_associations,
commits: :with_api_commit_entity_associations
}.freeze }.freeze
def search(additional_params = {}) def search(additional_params = {})
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment