From 0afcbf1e052d92ac1b3173adaf5d6589a5a7c7b1 Mon Sep 17 00:00:00 2001 From: Stephane CRIVISIER Date: Thu, 20 Dec 2018 16:28:12 +0100 Subject: [PATCH 1/4] Preload taxon's ancestors When asking Api::TaxonsController#index with multiple IDs, we used to generate a new SQL query to find each taxon's ancestors. This generates a query to find all ancestors for all taxons and use `AwesomeNestedSet::associate_parents` to associate all parents --- .../spree/api/taxons_controller.rb | 11 ++++ .../spree/api/taxons_controller_spec.rb | 19 +++++++ api/spec/support/query_count_matcher.rb | 55 +++++++++++++++++++ 3 files changed, 85 insertions(+) create mode 100644 api/spec/support/query_count_matcher.rb diff --git a/api/app/controllers/spree/api/taxons_controller.rb b/api/app/controllers/spree/api/taxons_controller.rb index e45f089c20..e8a8cb01c2 100644 --- a/api/app/controllers/spree/api/taxons_controller.rb +++ b/api/app/controllers/spree/api/taxons_controller.rb @@ -13,6 +13,7 @@ def index end @taxons = paginate(@taxons) + preload_taxon_parents(@taxons) respond_with(@taxons) end @@ -106,6 +107,16 @@ def taxon_params {} end end + + def preload_taxon_parents(taxons) + parents = Spree::Taxon.none + + taxons.map do |taxon| + parents = parents.or(taxon.ancestors) + end + + Spree::Taxon.associate_parents(taxons + parents) + end end end end diff --git a/api/spec/requests/spree/api/taxons_controller_spec.rb b/api/spec/requests/spree/api/taxons_controller_spec.rb index 836aed1515..8dcb733534 100644 --- a/api/spec/requests/spree/api/taxons_controller_spec.rb +++ b/api/spec/requests/spree/api/taxons_controller_spec.rb @@ -85,6 +85,25 @@ module Spree end end + context 'filtering by taxon ids' do + let!(:v2_4) { create(:taxon, name: "2.4", parent: taxon, taxonomy: taxonomy) } + let!(:v2_4_1) { create(:taxon, name: "2.4.1", parent: v2_4, taxonomy: taxonomy) } + + it 'returns only requested ids' do + get spree.api_taxons_path, params: { ids: [v2_4_1.id] } + + expect(json_response['taxons'].size).to eq 1 + end + + it 'avoids N+1 queries retrieving several taxons' do + expect { + get spree.api_taxons_path, params: { ids: [v2_4.id, v2_4_1.id] } + + expect(json_response['taxons'].size).to eq 2 + }.to query_limit_eq(7) + end + end + it "gets a single taxon" do get spree.api_taxonomy_taxon_path(taxonomy, taxon.id) diff --git a/api/spec/support/query_count_matcher.rb b/api/spec/support/query_count_matcher.rb new file mode 100644 index 0000000000..9a712e90a2 --- /dev/null +++ b/api/spec/support/query_count_matcher.rb @@ -0,0 +1,55 @@ +# frozen_string_literal: true + +RSpec::Matchers.define :query_limit_eq do |expected| + match do |block| + query_count(&block) == expected + end + + if respond_to?(:failure_message) + failure_message do |_actual| + failure_text + end + + failure_message_when_negated do |_actual| + failure_text_negated + end + else + failure_message_for_should do |_actual| + failure_text + end + + failure_message_for_should_not do |_actual| + failure_text_negated + end + end + + def query_count(&block) + @counter = 0 + + counter = ->(_name, _started, _finished, _unique_id, payload) { + unless %w[CACHE SCHEMA].include?(payload[:name]) + @counter += 1 + end + } + + ActiveSupport::Notifications.subscribed( + counter, + "sql.active_record", + &block + ) + + @counter + end + + def supports_block_expectations? + true + end + + def failure_text + "Expected to run exactly #{expected} queries, got #{@counter}" + end + + def failure_text_negated + "Expected to run other than #{expected} queries, got #{@counter}" + end +end From 3926961532773ba02dc90115bbbc8372242b7edc Mon Sep 17 00:00:00 2001 From: Stephane CRIVISIER Date: Thu, 20 Dec 2018 16:32:54 +0100 Subject: [PATCH 2/4] Refactor Taxon.pretty_name - use the same "algorithm" than Taxon.permalink - use taxon.parent instead of taxon.ancestors to take advantage of the preloaded parents --- core/app/models/spree/taxon.rb | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/core/app/models/spree/taxon.rb b/core/app/models/spree/taxon.rb index c4d0f1e81f..39fe094ad6 100644 --- a/core/app/models/spree/taxon.rb +++ b/core/app/models/spree/taxon.rb @@ -104,9 +104,14 @@ def all_variants # @return [String] this taxon's ancestors names followed by its own name, # separated by arrows def pretty_name - ancestor_chain = ancestors.map(&:name) - ancestor_chain << name - ancestor_chain.join(" -> ") + if parent.present? + [ + parent.pretty_name, + name + ].compact.join(" -> ") + else + name + end end # @see https://github.com/spree/spree/issues/3390 From a6fc9ef6a63122d18eb437d628689a187ca0db81 Mon Sep 17 00:00:00 2001 From: Stephane CRIVISIER Date: Mon, 14 Jan 2019 19:14:26 +0100 Subject: [PATCH 3/4] Preload taxon's children This endpoint had another N+1 problem when loading the selected taxon's children. This includes them only if the caller ask for the children. --- .../spree/api/taxons_controller.rb | 4 ++++ .../spree/api/taxons_controller_spec.rb | 23 +++++++++---------- 2 files changed, 15 insertions(+), 12 deletions(-) diff --git a/api/app/controllers/spree/api/taxons_controller.rb b/api/app/controllers/spree/api/taxons_controller.rb index e8a8cb01c2..ccd233ce90 100644 --- a/api/app/controllers/spree/api/taxons_controller.rb +++ b/api/app/controllers/spree/api/taxons_controller.rb @@ -12,6 +12,10 @@ def index @taxons = Spree::Taxon.accessible_by(current_ability, :read).order(:taxonomy_id, :lft).ransack(params[:q]).result end + unless params[:without_children] + @taxons = @taxons.includes(:children) + end + @taxons = paginate(@taxons) preload_taxon_parents(@taxons) respond_with(@taxons) diff --git a/api/spec/requests/spree/api/taxons_controller_spec.rb b/api/spec/requests/spree/api/taxons_controller_spec.rb index 8dcb733534..09ab8973ae 100644 --- a/api/spec/requests/spree/api/taxons_controller_spec.rb +++ b/api/spec/requests/spree/api/taxons_controller_spec.rb @@ -4,16 +4,14 @@ module Spree describe Api::TaxonsController, type: :request do - let(:taxonomy) { create(:taxonomy) } - let(:taxon) { create(:taxon, name: "Ruby", taxonomy: taxonomy) } - let(:taxon2) { create(:taxon, name: "Rails", taxonomy: taxonomy) } + let!(:taxonomy) { create(:taxonomy) } + let!(:taxon) { create(:taxon, name: "Ruby", parent: taxonomy.root, taxonomy: taxonomy) } + let!(:taxon2) { create(:taxon, name: "Rails", parent: taxon, taxonomy: taxonomy) } + let!(:rails_v3_2_2) { create(:taxon, name: "3.2.2", parent: taxon2, taxonomy: taxonomy) } let(:attributes) { ["id", "name", "pretty_name", "permalink", "parent_id", "taxonomy_id"] } before do stub_authentication! - taxon2.children << create(:taxon, name: "3.2.2", taxonomy: taxonomy) - taxon.children << taxon2 - taxonomy.root.children << taxon end context "as a normal user" do @@ -86,21 +84,22 @@ module Spree end context 'filtering by taxon ids' do - let!(:v2_4) { create(:taxon, name: "2.4", parent: taxon, taxonomy: taxonomy) } - let!(:v2_4_1) { create(:taxon, name: "2.4.1", parent: v2_4, taxonomy: taxonomy) } - it 'returns only requested ids' do - get spree.api_taxons_path, params: { ids: [v2_4_1.id] } + get spree.api_taxons_path, params: { ids: [rails_v3_2_2.id] } expect(json_response['taxons'].size).to eq 1 end it 'avoids N+1 queries retrieving several taxons' do + # We need a completly new branch to avoid having parent that can be preloaded from the rails ancestors + python = create(:taxon, name: "Python", parent: taxonomy.root, taxonomy: taxonomy) + python_3 = create(:taxon, name: "3.0", parent: python, taxonomy: taxonomy) + expect { - get spree.api_taxons_path, params: { ids: [v2_4.id, v2_4_1.id] } + get spree.api_taxons_path, params: { ids: [rails_v3_2_2.id, python_3.id] } expect(json_response['taxons'].size).to eq 2 - }.to query_limit_eq(7) + }.to query_limit_eq(5) end end From f403266cafee5a1563651123586a599df62d25b0 Mon Sep 17 00:00:00 2001 From: Stephane CRIVISIER Date: Thu, 24 Jan 2019 08:35:32 +0100 Subject: [PATCH 4/4] Remove the query counter check As pointed out by the core team, this spec might be too hazardous in the future (ex: an update of acts_as_nested_set that cause the code to make 1 additional query). I kept the spec to check that the API is correctly handling multiple IDs as it's an existing feature. --- .../spree/api/taxons_controller_spec.rb | 10 ++-- api/spec/support/query_count_matcher.rb | 55 ------------------- 2 files changed, 4 insertions(+), 61 deletions(-) delete mode 100644 api/spec/support/query_count_matcher.rb diff --git a/api/spec/requests/spree/api/taxons_controller_spec.rb b/api/spec/requests/spree/api/taxons_controller_spec.rb index 09ab8973ae..d1ebd71bb0 100644 --- a/api/spec/requests/spree/api/taxons_controller_spec.rb +++ b/api/spec/requests/spree/api/taxons_controller_spec.rb @@ -84,22 +84,20 @@ module Spree end context 'filtering by taxon ids' do - it 'returns only requested ids' do + it 'returns only requested id' do get spree.api_taxons_path, params: { ids: [rails_v3_2_2.id] } expect(json_response['taxons'].size).to eq 1 end - it 'avoids N+1 queries retrieving several taxons' do + it 'returns only requested ids' do # We need a completly new branch to avoid having parent that can be preloaded from the rails ancestors python = create(:taxon, name: "Python", parent: taxonomy.root, taxonomy: taxonomy) python_3 = create(:taxon, name: "3.0", parent: python, taxonomy: taxonomy) - expect { - get spree.api_taxons_path, params: { ids: [rails_v3_2_2.id, python_3.id] } + get spree.api_taxons_path, params: { ids: [rails_v3_2_2.id, python_3.id] } - expect(json_response['taxons'].size).to eq 2 - }.to query_limit_eq(5) + expect(json_response['taxons'].size).to eq 2 end end diff --git a/api/spec/support/query_count_matcher.rb b/api/spec/support/query_count_matcher.rb deleted file mode 100644 index 9a712e90a2..0000000000 --- a/api/spec/support/query_count_matcher.rb +++ /dev/null @@ -1,55 +0,0 @@ -# frozen_string_literal: true - -RSpec::Matchers.define :query_limit_eq do |expected| - match do |block| - query_count(&block) == expected - end - - if respond_to?(:failure_message) - failure_message do |_actual| - failure_text - end - - failure_message_when_negated do |_actual| - failure_text_negated - end - else - failure_message_for_should do |_actual| - failure_text - end - - failure_message_for_should_not do |_actual| - failure_text_negated - end - end - - def query_count(&block) - @counter = 0 - - counter = ->(_name, _started, _finished, _unique_id, payload) { - unless %w[CACHE SCHEMA].include?(payload[:name]) - @counter += 1 - end - } - - ActiveSupport::Notifications.subscribed( - counter, - "sql.active_record", - &block - ) - - @counter - end - - def supports_block_expectations? - true - end - - def failure_text - "Expected to run exactly #{expected} queries, got #{@counter}" - end - - def failure_text_negated - "Expected to run other than #{expected} queries, got #{@counter}" - end -end