Merge pull request #9983 from cillian/remove-exchange-tags-n1-query

Remove N+1 query loading tags for each order cycle exchange

app/controllers/admin/customers_controller.rb

@@ -116,21 +116,7 @@ module Admin
 
     # Fetches tags for all customers of the enterprise and returns a hash indexed by customer_id
     def customer_tags_by_id
-      customer_tags = ::ActsAsTaggableOn::Tag.
-        joins(:taggings).
-        includes(:taggings).
-        where(taggings:
-                { taggable_type: 'Customer',
-                  taggable_id: Customer.of(managed_enterprise_id),
-                  context: 'tags' })
-
-      customer_tags.each_with_object({}) do |tag, indexed_hash|
-        tag.taggings.each do |tagging|
-          customer_id = tagging.taggable_id
-          indexed_hash[customer_id] ||= []
-          indexed_hash[customer_id] << tag.name
-        end
-      end
+      BatchTaggableTagsQuery.call(Customer.of(managed_enterprise_id))
     end
   end
 end

app/controllers/admin/subscriptions_controller.rb

@@ -162,25 +162,20 @@ module Admin
       [:index]
     end
 
+    def managed_enterprise_id
+      Enterprise.managed_by(spree_current_user).select('enterprises.id').
+        find_by(id: params[:enterprise_id])
+    end
+
     def subscription_params
       @subscription_params ||= PermittedAttributes::Subscription.new(params).call.
         to_h.with_indifferent_access
     end
 
     def payment_method_tags_by_id
-      payment_method_tags = ::ActsAsTaggableOn::Tag.
-        joins(:taggings).
-        includes(:taggings).
-        where(taggings: { taggable_type: "Spree::PaymentMethod",
-                          taggable_id: Spree::PaymentMethod.from(Enterprise.managed_by(spree_current_user).
-                          select('enterprises.id').find_by(id: params[:enterprise_id])),
-                          context: 'tags' })
-
-      payment_method_tags.each_with_object({}) do |tag, hash|
-        payment_method_id = tag.taggings.first.taggable_id
-        hash[payment_method_id] ||= []
-        hash[payment_method_id] << tag.name
-      end
+      @payment_method_tags_by_id ||= BatchTaggableTagsQuery.call(
+        Spree::PaymentMethod.from(managed_enterprise_id)
+      )
     end
   end
 end

app/queries/batch_taggable_tags_query.rb

@@ -0,0 +1,20 @@
+# frozen_string_literal: true
+
+class BatchTaggableTagsQuery
+  def self.call(taggables)
+    ::ActsAsTaggableOn::Tag.
+      joins(:taggings).
+      includes(:taggings).
+      where(taggings:
+        {
+          taggable_type: taggables.model.to_s,
+          taggable_id: taggables,
+          context: 'tags'
+        }).order("tags.name").each_with_object({}) do |tag, indexed_hash|
+      tag.taggings.each do |tagging|
+        indexed_hash[tagging.taggable_id] ||= []
+        indexed_hash[tagging.taggable_id] << tag.name
+      end
+    end
+  end
+end

app/serializers/api/admin/exchange_serializer.rb

@@ -42,12 +42,18 @@ module Api
           .visible_variants_for_outgoing_exchanges_to(object.receiver)
       end
 
+      def preloaded_tag_list
+        return object.tag_list unless options[:preloaded_tags]
+
+        options.dig(:preloaded_tags, object.id) || []
+      end
+
       def tag_list
-        object.tag_list.join(",")
+        preloaded_tag_list.join(",")
       end
 
       def tags
-        object.tag_list.map{ |t| { text: t } }
+        preloaded_tag_list.map { |tag| { text: tag } }
       end
     end
   end

app/serializers/api/admin/order_cycle_serializer.rb

@@ -34,7 +34,8 @@ module Api
 
         ActiveModel::ArraySerializer.
           new(scoped_exchanges, each_serializer: Api::Admin::ExchangeSerializer,
-                                current_user: options[:current_user])
+                                current_user: options[:current_user],
+                                preloaded_tags: BatchTaggableTagsQuery.call(scoped_exchanges))
       end
 
       def editable_variants_for_incoming_exchanges

spec/controllers/admin/order_cycles_controller_spec.rb

@@ -116,6 +116,43 @@ module Admin
           end
         end
       end
+
+      describe "queries" do
+        context "as manager, when order cycle has multiple exchanges" do
+          let!(:distributor) { create(:distributor_enterprise) }
+          let(:order_cycle) { create(:simple_order_cycle, coordinator: distributor) }
+          before do
+            order_cycle.exchanges.create! sender: distributor, receiver: distributor, incoming: true,
+                                          receival_instructions: 'A', tag_list: "A"
+            order_cycle.exchanges.create! sender: distributor, receiver: distributor, incoming: false,
+                                          pickup_instructions: 'B', tag_list: "B"
+            controller_login_as_enterprise_user([distributor])
+          end
+
+          it do
+            query_counter = QueryCounter.new
+            get :show, params: { id: order_cycle.id }, as: :json
+            expect(query_counter.queries).to eq(
+              {
+                select: {
+                  enterprise_fees: 3,
+                  enterprise_groups: 1,
+                  enterprises: 22,
+                  exchanges: 7,
+                  order_cycles: 6,
+                  proxy_orders: 1,
+                  schedules: 1,
+                  spree_roles: 9,
+                  spree_variants: 8,
+                  tags: 1
+                },
+                update: { spree_users: 1 }
+              }
+            )
+            query_counter.stop
+          end
+        end
+      end
     end
 
     describe "create" do

spec/queries/batch_taggable_tags_query_spec.rb

@@ -0,0 +1,21 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe BatchTaggableTagsQuery do
+  it "fetches tags for multiple models in one query" do
+    customer_i = create(:customer, tag_list: "member,volunteer")
+    customer_ii = create(:customer, tag_list: "member")
+    customer_iii = create(:customer, tag_list: nil)
+
+    tags = BatchTaggableTagsQuery.call(
+      Customer.where(id: [customer_i, customer_ii, customer_iii])
+    )
+    expect(tags).to eq(
+      {
+        customer_i.id => ["member", "volunteer"],
+        customer_ii.id => ["member"],
+      }
+    )
+  end
+end

spec/support/query_counter.rb

@@ -0,0 +1,48 @@
+# frozen_string_literal: true
+
+class QueryCounter
+  QUERY_TYPES = [:delete, :insert, :select, :update].freeze
+
+  attr_reader :queries
+
+  def initialize
+    @queries = {}
+    @subscriber = ActiveSupport::Notifications.
+      subscribe("sql.active_record") do |_name, _started, _finished, _unique_id, payload|
+      type = get_type(payload[:sql])
+      next if QUERY_TYPES.exclude?(type) || pg_query?(payload[:sql])
+
+      table = get_table(payload[:sql])
+      @queries[type] ||= {}
+      @queries[type][table] ||= 0
+      @queries[type][table] += 1
+    end
+  end
+
+  def stop
+    ActiveSupport::Notifications.unsubscribe("sql.active_record")
+  end
+
+  private
+
+  def get_table(sql)
+    sql_parts = sql.split(" ")
+    case get_type(sql)
+    when :insert
+      sql_parts[3]
+    when :update
+      sql_parts[1]
+    else
+      table_index = sql_parts.index("FROM")
+      sql_parts[table_index + 1]
+    end.gsub(/(\\|")/, "").to_sym
+  end
+
+  def get_type(sql)
+    sql.split(" ")[0].downcase.to_sym
+  end
+
+  def pg_query?(sql)
+    sql.include?("SELECT a.attname") || sql.include?("pg_attribute")
+  end
+end