Commit 3f5d0012 authored by Terri Chu's avatar Terri Chu Committed by Heinrich Lee Yu

Reduce queries on Controller Projects::LabelsController#index [RUN AS-IF-FOSS] [RUN ALL RSPEC]

parent 6af841ba
......@@ -16,8 +16,8 @@ class Groups::LabelsController < Groups::ApplicationController
format.html do
# at group level we do not want to list project labels,
# we only want `only_group_labels = false` when pulling labels for label filter dropdowns, fetched through json
@labels = available_labels(params.merge(only_group_labels: true)).includes(:group).page(params[:page]) # rubocop: disable CodeReuse/ActiveRecord
@labels.each { |label| label.lazy_subscription(current_user) } # preload subscriptions
@labels = available_labels(params.merge(only_group_labels: true)).page(params[:page]) # rubocop: disable CodeReuse/ActiveRecord
Preloaders::LabelsPreloader.new(@labels, current_user).preload_all
end
format.json do
render json: LabelSerializer.new.represent_appearance(available_labels)
......
......@@ -17,11 +17,14 @@ class Projects::LabelsController < Projects::ApplicationController
feature_category :issue_tracking
def index
@prioritized_labels = @available_labels.prioritized(@project)
@labels = @available_labels.unprioritized(@project).page(params[:page])
respond_to do |format|
format.html
format.html do
@prioritized_labels = @available_labels.prioritized(@project)
@labels = @available_labels.unprioritized(@project).page(params[:page])
# preload group, project, and subscription data
Preloaders::LabelsPreloader.new(@prioritized_labels, current_user, @project).preload_all
Preloaders::LabelsPreloader.new(@labels, current_user, @project).preload_all
end
format.json do
render json: LabelSerializer.new.represent_appearance(@available_labels)
end
......
# frozen_string_literal: true
module Preloaders
# This class preloads the `project`, `group`, and subscription associations for the given
# labels, user, and project (if provided). A Label can be of type ProjectLabel or GroupLabel
# and the preloader supports both.
#
# Usage:
# labels = Label.where(...)
# Preloaders::LabelsPreloader.new(labels, current_user, @project).preload_all
# labels.first.project # won't fire any query
class LabelsPreloader
attr_reader :labels, :user, :project
def initialize(labels, user, project = nil)
@labels, @user, @project = labels, user, project
end
def preload_all
preloader = ActiveRecord::Associations::Preloader.new
preloader.preload(labels.select {|l| l.is_a? ProjectLabel }, { project: [:project_feature, namespace: :route] })
preloader.preload(labels.select {|l| l.is_a? GroupLabel }, { group: :route })
labels.each do |label|
label.lazy_subscription(user)
label.lazy_subscription(user, project) if project.present?
end
end
end
end
Preloaders::LabelsPreloader.prepend_if_ee('EE::Preloaders::LabelsPreloader')
---
title: Reduce queries on projects labels controller
merge_request: 57864
author:
type: performance
# frozen_string_literal: true
module EE
module Preloaders
module LabelsPreloader
extend ::Gitlab::Utils::Override
override :preload_all
def preload_all
super
preloader = ActiveRecord::Associations::Preloader.new
preloader.preload(labels.select { |l| l.is_a? GroupLabel }, { group: [:ip_restrictions, :saml_provider] })
end
end
end
end
......@@ -60,7 +60,7 @@ RSpec.describe Groups::LabelsController do
create_list(:group_label, 3, group: group)
# some n+1 queries still exist
expect { get :index, params: { group_id: group.to_param } }.not_to exceed_all_query_limit(control.count).with_threshold(12)
expect { get :index, params: { group_id: group.to_param } }.not_to exceed_all_query_limit(control.count).with_threshold(10)
expect(assigns(:labels).count).to eq(4)
end
end
......
......@@ -93,6 +93,26 @@ RSpec.describe Projects::LabelsController do
end
end
context 'with views rendered' do
render_views
before do
list_labels
end
it 'avoids N+1 queries' do
control = ActiveRecord::QueryRecorder.new(skip_cached: false) { list_labels }
create_list(:label, 3, project: project)
create_list(:group_label, 3, group: group)
# some n+1 queries still exist
# calls to get max project authorization access level
expect { list_labels }.not_to exceed_all_query_limit(control.count).with_threshold(25)
expect(assigns(:labels).count).to eq(10)
end
end
def list_labels
get :index, params: { namespace_id: project.namespace.to_param, project_id: project }
end
......
# frozen_string_literal: true
require 'spec_helper'
RSpec.describe Preloaders::LabelsPreloader do
let_it_be(:user) { create(:user) }
shared_examples 'an efficient database query' do
let(:subscriptions) { labels.each { |l| create(:subscription, subscribable: l, project: l.project, user: user) }}
it 'does not make n+1 queries' do
first_label = labels_with_preloaded_data.first
clean_labels = labels_with_preloaded_data
expect { access_data(clean_labels) }.to issue_same_number_of_queries_as { access_data([first_label]) }
end
end
context 'project labels' do
let_it_be(:projects) { create_list(:project, 3, :public, :repository) }
let_it_be(:labels) { projects.each { |p| create(:label, project: p) } }
it_behaves_like 'an efficient database query'
end
context 'group labels' do
let_it_be(:groups) { create_list(:group, 3) }
let_it_be(:labels) { groups.each { |g| create(:group_label, group: g) } }
it_behaves_like 'an efficient database query'
end
private
def labels_with_preloaded_data
l = Label.where(id: labels.map(&:id))
described_class.new(l, user).preload_all
l
end
def access_data(labels)
labels.each do |label|
if label.is_a?(ProjectLabel)
label.project.project_feature
label.lazy_subscription(user, label.project)
elsif label.is_a?(GroupLabel)
label.group.route
label.lazy_subscription(user)
end
end
end
end
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