Merge branch...

Merge branch '21034-controller-groups-labelscontroller-index-executes-more-than-100-sql-queries' into 'master'

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

See merge request gitlab-org/gitlab!57517

app/controllers/groups/labels_controller.rb

@@ -16,7 +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)).page(params[:page])
+        @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
       end
       format.json do
         render json: LabelSerializer.new.represent_appearance(available_labels)

app/helpers/labels_helper.rb

@@ -164,8 +164,8 @@ module LabelsHelper
   end
 
   def label_subscription_status(label, project)
-    return 'group-level' if label.subscribed?(current_user)
-    return 'project-level' if label.subscribed?(current_user, project)
+    return 'group-level' if label.lazy_subscribed?(current_user)
+    return 'project-level' if label.lazy_subscribed?(current_user, project)
 
     'unsubscribed'
   end
@@ -181,7 +181,7 @@ module LabelsHelper
   end
 
   def label_subscription_toggle_button_text(label, project = nil)
-    label.subscribed?(current_user, project) ? 'Unsubscribe' : 'Subscribe'
+    label.lazy_subscribed?(current_user, project) ? 'Unsubscribe' : 'Subscribe'
   end
 
   def create_label_title(subject)

app/models/concerns/subscribable.rb

@@ -17,13 +17,47 @@ module Subscribable
   def subscribed?(user, project = nil)
     return false unless user
 
-    if subscription = subscriptions.find_by(user: user, project: project)
+    if (subscription = subscriptions.find_by(user: user, project: project))
       subscription.subscribed
     else
       subscribed_without_subscriptions?(user, project)
     end
   end
 
+  def lazy_subscribed?(user, project = nil)
+    return false unless user
+
+    if (subscription = lazy_subscription(user, project)&.itself)
+      subscription.subscribed
+    else
+      subscribed_without_subscriptions?(user, project)
+    end
+  end
+
+  def lazy_subscription(user, project = nil)
+    return unless user
+
+    # handle project and group labels as well as issuable subscriptions
+    subscribable_type = self.class.ancestors.include?(Label) ? 'Label' : self.class.name
+    BatchLoader.for(id: id, subscribable_type: subscribable_type, project_id: project&.id).batch do |items, loader|
+      values = items.each_with_object({ ids: Set.new, subscribable_types: Set.new, project_ids: Set.new }) do |item, result|
+        result[:ids] << item[:id]
+        result[:subscribable_types] << item[:subscribable_type]
+        result[:project_ids] << item[:project_id]
+      end
+
+      subscriptions = Subscription.where(subscribable_id: values[:ids], subscribable_type: values[:subscribable_types], project_id: values[:project_ids], user: user)
+
+      subscriptions.each do |subscription|
+        loader.call({
+          id: subscription.subscribable_id,
+          subscribable_type: subscription.subscribable_type,
+          project_id: subscription.project_id
+          }, subscription)
+      end
+    end
+  end
+
   # Override this method to define custom logic to consider a subscribable as
   # subscribed without an explicit subscription record.
   def subscribed_without_subscriptions?(user, project)

changelogs/unreleased/21034-controller-groups-labelscontroller-index-executes-more-than-100-sql.yml

+---
+title: Reduce queries on group labels controller
+merge_request: 57517
+author:
+type: performance

spec/controllers/groups/labels_controller_spec.rb

@@ -46,6 +46,24 @@ RSpec.describe Groups::LabelsController do
 
       it_behaves_like 'disabled when using an external authorization service'
     end
+
+    context 'with views rendered' do
+      render_views
+
+      before do
+        get :index, params: { group_id: group.to_param }
+      end
+
+      it 'avoids N+1 queries' do
+        control = ActiveRecord::QueryRecorder.new(skip_cached: false) { get :index, params: { group_id: group.to_param } }
+
+        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(assigns(:labels).count).to eq(4)
+      end
+    end
   end
 
   describe 'POST #toggle_subscription' do

spec/factories/subscriptions.rb

@@ -5,5 +5,11 @@ FactoryBot.define do
     project
     user { project.creator }
     subscribable factory: :issue
+
+    trait :group_label do
+      project { nil }
+      user { association(:user) }
+      subscribable factory: :group_label
+    end
   end
 end

spec/models/concerns/subscribable_spec.rb

@@ -7,50 +7,54 @@ RSpec.describe Subscribable, 'Subscribable' do
   let(:resource) { create(:issue, project: project) }
   let(:user_1)   { create(:user) }
 
-  describe '#subscribed?' do
+  shared_examples 'returns expected values' do |method|
     context 'without user' do
       it 'returns false' do
-        expect(resource.subscribed?(nil, project)).to be_falsey
+        expect(resource.public_send(method, nil, project)).to be_falsey
       end
     end
 
     context 'without project' do
       it 'returns false when no subscription exists' do
-        expect(resource.subscribed?(user_1)).to be_falsey
+        expect(resource.public_send(method, user_1)).to be_falsey
       end
 
-      it 'returns true when a subcription exists and subscribed is true' do
+      it 'returns true when a subscription exists and subscribed is true' do
         resource.subscriptions.create!(user: user_1, subscribed: true)
 
-        expect(resource.subscribed?(user_1)).to be_truthy
+        expect(resource.public_send(method, user_1)).to be_truthy
       end
 
-      it 'returns false when a subcription exists and subscribed is false' do
+      it 'returns false when a subscription exists and subscribed is false' do
         resource.subscriptions.create!(user: user_1, subscribed: false)
 
-        expect(resource.subscribed?(user_1)).to be_falsey
+        expect(resource.public_send(method, user_1)).to be_falsey
       end
     end
 
     context 'with project' do
       it 'returns false when no subscription exists' do
-        expect(resource.subscribed?(user_1, project)).to be_falsey
+        expect(resource.public_send(method, user_1, project)).to be_falsey
       end
 
-      it 'returns true when a subcription exists and subscribed is true' do
+      it 'returns true when a subscription exists and subscribed is true' do
         resource.subscriptions.create!(user: user_1, project: project, subscribed: true)
 
-        expect(resource.subscribed?(user_1, project)).to be_truthy
+        expect(resource.public_send(method, user_1, project)).to be_truthy
       end
 
-      it 'returns false when a subcription exists and subscribed is false' do
+      it 'returns false when a subscription exists and subscribed is false' do
         resource.subscriptions.create!(user: user_1, project: project, subscribed: false)
 
-        expect(resource.subscribed?(user_1, project)).to be_falsey
+        expect(resource.public_send(method, user_1, project)).to be_falsey
       end
     end
   end
 
+  describe '#subscribed?' do
+    it_behaves_like 'returns expected values', :subscribed?
+  end
+
   describe '#subscribers' do
     it 'returns [] when no subcribers exists' do
       expect(resource.subscribers(project)).to be_empty
@@ -189,4 +193,31 @@ RSpec.describe Subscribable, 'Subscribable' do
       it_behaves_like 'setting subscriptions'
     end
   end
+
+  describe '#lazy_subscribed?' do
+    it_behaves_like 'returns expected values', :lazy_subscribed?
+  end
+
+  describe '#lazy_subscription' do
+    let(:labels) { create_list(:group_label, 5) }
+
+    before do
+      labels.each do |label|
+        create(:subscription, :group_label, user: user_1, subscribable: label)
+      end
+    end
+
+    it 'executes only one SQL query' do
+      lazy_queries = ActiveRecord::QueryRecorder.new do
+        labels.each { |label| label.lazy_subscription(user_1) }
+      end
+
+      preloaded_queries = ActiveRecord::QueryRecorder.new do
+        labels.each { |label| label.lazy_subscription(user_1)&.subscribed? }
+      end
+
+      expect(lazy_queries.count).to eq(0)
+      expect(preloaded_queries.count).to eq(1)
+    end
+  end
 end