Merge branch...

Merge branch '21078-controller-projects-labelscontroller-index-executes-more-than-100-sql-queries' into 'master'

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

See merge request gitlab-org/gitlab!57864

app/controllers/groups/labels_controller.rb

@@ -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)

app/controllers/projects/labels_controller.rb

@@ -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

app/models/preloaders/labels_preloader.rb

+# 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')

changelogs/unreleased/21078-controller-projects-labelscontroller-index-executes-more-than-100-s.yml

+---
+title: Reduce queries on projects labels controller
+merge_request: 57864
+author:
+type: performance

ee/app/models/ee/preloaders/labels_preloader.rb

+# 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

spec/controllers/groups/labels_controller_spec.rb

@@ -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

spec/controllers/projects/labels_controller_spec.rb

@@ -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

spec/models/preloaders/labels_preloader_spec.rb

+# 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