From fbe6dcba7a915d3c812f39263c35b63727e54ea9 Mon Sep 17 00:00:00 2001 From: Cillian O'Ruanaidh Date: Fri, 11 Nov 2022 15:54:22 +0000 Subject: [PATCH 1/3] Remove N+1 query loading tags for each order cycle exchange --- app/controllers/admin/customers_controller.rb | 16 +------ .../admin/subscriptions_controller.rb | 21 ++++---- app/queries/batch_taggable_tags_query.rb | 22 +++++++++ .../api/admin/exchange_serializer.rb | 10 +++- .../api/admin/order_cycle_serializer.rb | 3 +- .../admin/order_cycles_controller_spec.rb | 37 ++++++++++++++ .../queries/batch_taggable_tags_query_spec.rb | 21 ++++++++ spec/support/query_counter.rb | 48 +++++++++++++++++++ 8 files changed, 147 insertions(+), 31 deletions(-) create mode 100644 app/queries/batch_taggable_tags_query.rb create mode 100644 spec/queries/batch_taggable_tags_query_spec.rb create mode 100644 spec/support/query_counter.rb diff --git a/app/controllers/admin/customers_controller.rb b/app/controllers/admin/customers_controller.rb index 28fcf235ba..4e48ac68ea 100644 --- a/app/controllers/admin/customers_controller.rb +++ b/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 diff --git a/app/controllers/admin/subscriptions_controller.rb b/app/controllers/admin/subscriptions_controller.rb index dfc078f500..c715d88b29 100644 --- a/app/controllers/admin/subscriptions_controller.rb +++ b/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 diff --git a/app/queries/batch_taggable_tags_query.rb b/app/queries/batch_taggable_tags_query.rb new file mode 100644 index 0000000000..c8f28ad415 --- /dev/null +++ b/app/queries/batch_taggable_tags_query.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +class BatchTaggableTagsQuery + def self.call(taggables) + tags = ::ActsAsTaggableOn::Tag. + joins(:taggings). + includes(:taggings). + where(taggings: + { + taggable_type: taggables.model.to_s, + taggable_id: taggables, + context: 'tags' + }) + + tags.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 diff --git a/app/serializers/api/admin/exchange_serializer.rb b/app/serializers/api/admin/exchange_serializer.rb index 5649adc48e..2d199b6757 100644 --- a/app/serializers/api/admin/exchange_serializer.rb +++ b/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(",(pre)") end def tags - object.tag_list.map{ |t| { text: t } } + preloaded_tag_list.map { |tag| { text: tag } } end end end diff --git a/app/serializers/api/admin/order_cycle_serializer.rb b/app/serializers/api/admin/order_cycle_serializer.rb index fd15318adf..9370d3f860 100644 --- a/app/serializers/api/admin/order_cycle_serializer.rb +++ b/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 diff --git a/spec/controllers/admin/order_cycles_controller_spec.rb b/spec/controllers/admin/order_cycles_controller_spec.rb index 747eb6107a..e31ab5a0f4 100644 --- a/spec/controllers/admin/order_cycles_controller_spec.rb +++ b/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 diff --git a/spec/queries/batch_taggable_tags_query_spec.rb b/spec/queries/batch_taggable_tags_query_spec.rb new file mode 100644 index 0000000000..771f7aac38 --- /dev/null +++ b/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 => ["volunteer", "member"], + customer_ii.id => ["member"], + } + ) + end +end diff --git a/spec/support/query_counter.rb b/spec/support/query_counter.rb new file mode 100644 index 0000000000..009c648d03 --- /dev/null +++ b/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 From a8816b378893bb2bfa68cf022cb0802442538d46 Mon Sep 17 00:00:00 2001 From: Cillian O'Ruanaidh Date: Fri, 18 Nov 2022 14:40:30 +0000 Subject: [PATCH 2/3] Remove debugging code left in by accident --- app/serializers/api/admin/exchange_serializer.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/serializers/api/admin/exchange_serializer.rb b/app/serializers/api/admin/exchange_serializer.rb index 2d199b6757..90828f0ba1 100644 --- a/app/serializers/api/admin/exchange_serializer.rb +++ b/app/serializers/api/admin/exchange_serializer.rb @@ -49,7 +49,7 @@ module Api end def tag_list - preloaded_tag_list.join(",(pre)") + preloaded_tag_list.join(",") end def tags From b33d38206966d962417dbc793ac2f105ef4b73f4 Mon Sep 17 00:00:00 2001 From: Cillian O'Ruanaidh Date: Fri, 18 Nov 2022 15:05:03 +0000 Subject: [PATCH 3/3] Order BatchTaggableTagsQuery by name for consistency and to avoid flakey test failure --- app/queries/batch_taggable_tags_query.rb | 6 ++---- spec/queries/batch_taggable_tags_query_spec.rb | 2 +- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/app/queries/batch_taggable_tags_query.rb b/app/queries/batch_taggable_tags_query.rb index c8f28ad415..ea9f6d9bc5 100644 --- a/app/queries/batch_taggable_tags_query.rb +++ b/app/queries/batch_taggable_tags_query.rb @@ -2,7 +2,7 @@ class BatchTaggableTagsQuery def self.call(taggables) - tags = ::ActsAsTaggableOn::Tag. + ::ActsAsTaggableOn::Tag. joins(:taggings). includes(:taggings). where(taggings: @@ -10,9 +10,7 @@ class BatchTaggableTagsQuery taggable_type: taggables.model.to_s, taggable_id: taggables, context: 'tags' - }) - - tags.each_with_object({}) do |tag, indexed_hash| + }).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 diff --git a/spec/queries/batch_taggable_tags_query_spec.rb b/spec/queries/batch_taggable_tags_query_spec.rb index 771f7aac38..2bb63926ea 100644 --- a/spec/queries/batch_taggable_tags_query_spec.rb +++ b/spec/queries/batch_taggable_tags_query_spec.rb @@ -13,7 +13,7 @@ describe BatchTaggableTagsQuery do ) expect(tags).to eq( { - customer_i.id => ["volunteer", "member"], + customer_i.id => ["member", "volunteer"], customer_ii.id => ["member"], } )