From 5bac41b3fcf9b80dda759b6d06f6098260670d1f Mon Sep 17 00:00:00 2001 From: Jordan Brough Date: Fri, 18 Dec 2015 23:52:01 -0700 Subject: [PATCH 1/3] Add StockItem#fill_status to enable perf improvements Add a `fill_status` method to StockItem and update `StockLocation#fill_status` to use it. This allows us to avoid N+1 queries by loading multiple stock items at once and querying them directly. While using this as a perf test: https://gist.github.com/jordan-brough/d614babd1202a890b7da I saw this performance difference: old: 4270ms new: 772ms --- core/app/models/spree/stock/packer.rb | 20 +++++++++++++++++-- core/app/models/spree/stock_item.rb | 13 ++++++++++++ core/app/models/spree/stock_location.rb | 12 +---------- core/spec/models/spree/stock/packer_spec.rb | 5 ++--- core/spec/models/spree/stock_location_spec.rb | 14 +++++-------- 5 files changed, 39 insertions(+), 25 deletions(-) diff --git a/core/app/models/spree/stock/packer.rb b/core/app/models/spree/stock/packer.rb index 6820f1601ae..0fe39506cb2 100644 --- a/core/app/models/spree/stock/packer.rb +++ b/core/app/models/spree/stock/packer.rb @@ -22,9 +22,10 @@ def default_package inventory_units.group_by(&:variant).each do |variant, variant_inventory_units| units = variant_inventory_units.clone if variant.should_track_inventory? - next unless stock_location.stock_item(variant) + stock_item = stock_item_lookup[variant.id] + next unless stock_item - on_hand, backordered = stock_location.fill_status(variant, units.count) + on_hand, backordered = stock_item.fill_status(units.count) raise Spree::Order::InsufficientStock unless on_hand > 0 || backordered > 0 package.add_multiple units.slice!(0, on_hand), :on_hand if on_hand > 0 package.add_multiple units.slice!(0, backordered), :backordered if backordered > 0 @@ -37,6 +38,21 @@ def default_package end private + + # Returns a lookup table in the form of: + # { => , ...} + def stock_item_lookup + @stock_item_lookup ||= begin + Spree::StockItem. + where(variant_id: inventory_units.map(&:variant_id).uniq). + where(stock_location_id: stock_location.id). + order(:id). + each_with_object({}) do |stock_item, hash| + hash[stock_item.variant_id] ||= stock_item + end + end + end + def build_splitter splitter = nil splitters.reverse.each do |klass| diff --git a/core/app/models/spree/stock_item.rb b/core/app/models/spree/stock_item.rb index 6cb0b1f1e24..30656a6d764 100644 --- a/core/app/models/spree/stock_item.rb +++ b/core/app/models/spree/stock_item.rb @@ -70,6 +70,19 @@ def reduce_count_on_hand_to_zero self.set_count_on_hand(0) if count_on_hand > 0 end + def fill_status(quantity) + if count_on_hand >= quantity + on_hand = quantity + backordered = 0 + else + on_hand = count_on_hand + on_hand = 0 if on_hand < 0 + backordered = backorderable? ? (quantity - on_hand) : 0 + end + + [on_hand, backordered] + end + private def verify_count_on_hand? count_on_hand_changed? && !backorderable? && (count_on_hand < count_on_hand_was) && (count_on_hand < 0) diff --git a/core/app/models/spree/stock_location.rb b/core/app/models/spree/stock_location.rb index bc075ffd5d9..1e794263590 100644 --- a/core/app/models/spree/stock_location.rb +++ b/core/app/models/spree/stock_location.rb @@ -95,17 +95,7 @@ def move(variant, quantity, originator = nil) def fill_status(variant, quantity) if item = stock_item(variant) - - if item.count_on_hand >= quantity - on_hand = quantity - backordered = 0 - else - on_hand = item.count_on_hand - on_hand = 0 if on_hand < 0 - backordered = item.backorderable? ? (quantity - on_hand) : 0 - end - - [on_hand, backordered] + item.fill_status(quantity) else [0, 0] end diff --git a/core/spec/models/spree/stock/packer_spec.rb b/core/spec/models/spree/stock/packer_spec.rb index 94d1fdefd75..e8c78ae18ab 100644 --- a/core/spec/models/spree/stock/packer_spec.rb +++ b/core/spec/models/spree/stock/packer_spec.rb @@ -28,9 +28,8 @@ module Stock end it 'variants are added as backordered without enough on_hand' do - expect(stock_location).to receive(:fill_status).exactly(5).times.and_return( - *(Array.new(3, [1,0]) + Array.new(2, [0,1])) - ) + inventory_units[0..2].each { |iu| stock_location.stock_item(iu.variant_id).set_count_on_hand(1) } + inventory_units[3..4].each { |iu| stock_location.stock_item(iu.variant_id).set_count_on_hand(0) } package = subject.default_package expect(package.on_hand.size).to eq 3 diff --git a/core/spec/models/spree/stock_location_spec.rb b/core/spec/models/spree/stock_location_spec.rb index 09cbaec6a3a..d5850e16ccd 100644 --- a/core/spec/models/spree/stock_location_spec.rb +++ b/core/spec/models/spree/stock_location_spec.rb @@ -170,10 +170,7 @@ module Spree end it 'zero on_hand with all backordered' do - zero_stock_item = mock_model(StockItem, - count_on_hand: 0, - backorderable?: true) - expect(subject).to receive(:stock_item).with(variant).and_return(zero_stock_item) + stock_item.set_count_on_hand(0) on_hand, backordered = subject.fill_status(variant, 20) expect(on_hand).to eq 0 @@ -182,12 +179,11 @@ module Spree context 'when backordering is not allowed' do before do - @stock_item = mock_model(StockItem, backorderable?: false) - expect(subject).to receive(:stock_item).with(variant).and_return(@stock_item) + stock_item.update!(backorderable: false) end it 'all on_hand' do - allow(@stock_item).to receive_messages(count_on_hand: 10) + stock_item.set_count_on_hand(10) on_hand, backordered = subject.fill_status(variant, 5) expect(on_hand).to eq 5 @@ -195,7 +191,7 @@ module Spree end it 'some on_hand' do - allow(@stock_item).to receive_messages(count_on_hand: 10) + stock_item.set_count_on_hand(10) on_hand, backordered = subject.fill_status(variant, 20) expect(on_hand).to eq 10 @@ -203,7 +199,7 @@ module Spree end it 'zero on_hand' do - allow(@stock_item).to receive_messages(count_on_hand: 0) + stock_item.set_count_on_hand(0) on_hand, backordered = subject.fill_status(variant, 20) expect(on_hand).to eq 0 From a833c2524fcc117455488219f2e83350531b9134 Mon Sep 17 00:00:00 2001 From: Jordan Brough Date: Sat, 19 Dec 2015 00:16:54 -0700 Subject: [PATCH 2/3] Add DB-level uniqueness constraint for stock items For databases that support it (postgres & sqlite). Since application-level uniqueness constraints are vulnerable to race conditions. --- .../20151219020209_add_stock_item_unique_index.rb | 9 +++++++++ 1 file changed, 9 insertions(+) create mode 100644 core/db/migrate/20151219020209_add_stock_item_unique_index.rb diff --git a/core/db/migrate/20151219020209_add_stock_item_unique_index.rb b/core/db/migrate/20151219020209_add_stock_item_unique_index.rb new file mode 100644 index 00000000000..abdc8617267 --- /dev/null +++ b/core/db/migrate/20151219020209_add_stock_item_unique_index.rb @@ -0,0 +1,9 @@ +class AddStockItemUniqueIndex < ActiveRecord::Migration + def change + # Add a database-level uniqueness constraint for databases that support it + # (postgres & sqlite) + if connection.adapter_name =~ /postgres|sqlite/i + add_index 'spree_stock_items', ['variant_id', 'stock_location_id'], where: 'deleted_at is null', unique: true + end + end +end From bc6a0ba10ab1528323c8dfd51fca9433a9a126d0 Mon Sep 17 00:00:00 2001 From: Jordan Brough Date: Sat, 19 Dec 2015 00:17:41 -0700 Subject: [PATCH 3/3] Refactor stock item lookup a bit - add stock_item_for - use to_h instead of each_with_object - remove the order(:id) since we have db-level uniqueness now --- core/app/models/spree/stock/packer.rb | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/core/app/models/spree/stock/packer.rb b/core/app/models/spree/stock/packer.rb index 0fe39506cb2..1a24b4fd9ca 100644 --- a/core/app/models/spree/stock/packer.rb +++ b/core/app/models/spree/stock/packer.rb @@ -22,7 +22,7 @@ def default_package inventory_units.group_by(&:variant).each do |variant, variant_inventory_units| units = variant_inventory_units.clone if variant.should_track_inventory? - stock_item = stock_item_lookup[variant.id] + stock_item = stock_item_for(variant.id) next unless stock_item on_hand, backordered = stock_item.fill_status(units.count) @@ -39,18 +39,18 @@ def default_package private + def stock_item_for(variant_id) + stock_item_lookup[variant_id] + end + # Returns a lookup table in the form of: # { => , ...} def stock_item_lookup - @stock_item_lookup ||= begin - Spree::StockItem. - where(variant_id: inventory_units.map(&:variant_id).uniq). - where(stock_location_id: stock_location.id). - order(:id). - each_with_object({}) do |stock_item, hash| - hash[stock_item.variant_id] ||= stock_item - end - end + @stock_item_lookup ||= Spree::StockItem. + where(variant_id: inventory_units.map(&:variant_id).uniq). + where(stock_location_id: stock_location.id). + map { |stock_item| [stock_item.variant_id, stock_item] }. + to_h # there is only one stock item per variant in a stock location end def build_splitter