Merge pull request #5406 from kristinalim/fix/5300-optimistic_locking_in_stock_items

5300 Avoid race conditions in Spree::StockItem

.rubocop_todo.yml

@@ -363,6 +363,13 @@ Rails/UniqueValidationWithoutIndex:
     - 'app/models/customer.rb'
     - 'app/models/exchange.rb'
 
+# Offense count: 2
+# Configuration parameters: Include.
+# Include: app/models/**/*.rb
+Rails/UniqueValidationWithoutIndex:
+  Exclude:
+    - 'app/models/spree/stock_item.rb'
+
 # Offense count: 1
 # Configuration parameters: Environments.
 # Environments: development, test, production

app/models/spree/stock_item.rb

@@ -0,0 +1,58 @@
+# frozen_string_literal: true
+
+module Spree
+  class StockItem < ActiveRecord::Base
+    acts_as_paranoid
+
+    belongs_to :stock_location, class_name: 'Spree::StockLocation'
+    belongs_to :variant, class_name: 'Spree::Variant'
+    has_many :stock_movements, dependent: :destroy
+
+    validates :stock_location, :variant, presence: true
+    validates :variant_id, uniqueness: { scope: [:stock_location_id, :deleted_at] }
+    validates :count_on_hand, numericality: { greater_than_or_equal_to: 0, unless: :backorderable? }
+
+    delegate :weight, to: :variant
+    delegate :name, to: :variant, prefix: true
+
+    def backordered_inventory_units
+      Spree::InventoryUnit.backordered_for_stock_item(self)
+    end
+
+    def adjust_count_on_hand(value)
+      with_lock do
+        self.count_on_hand = count_on_hand + value
+        process_backorders if in_stock?
+
+        save!
+      end
+    end
+
+    def in_stock?
+      count_on_hand.positive?
+    end
+
+    # Tells whether it's available to be included in a shipment
+    def available?
+      in_stock? || backorderable?
+    end
+
+    def variant
+      Spree::Variant.unscoped { super }
+    end
+
+    private
+
+    def count_on_hand=(value)
+      self[:count_on_hand] = value
+    end
+
+    def process_backorders
+      backordered_inventory_units.each do |unit|
+        break unless in_stock?
+
+        unit.fill_backorder
+      end
+    end
+  end
+end

db/migrate/20200512070717_add_lock_version_to_stock_items.rb

@@ -0,0 +1,5 @@
+class AddLockVersionToStockItems < ActiveRecord::Migration
+  def change
+    add_column :spree_stock_items, :lock_version, :integer, default: 0
+  end
+end

db/migrate/20200514174526_reset_negative_nonbackorderable_count_on_hand_in_stock_items.rb

@@ -0,0 +1,19 @@
+# frozen_string_literal: true
+
+class ResetNegativeNonbackorderableCountOnHandInStockItems < ActiveRecord::Migration
+  module Spree
+    class StockItem < ActiveRecord::Base
+      self.table_name = "spree_stock_items"
+    end
+  end
+
+  def up
+    Spree::StockItem.where(backorderable: false)
+      .where("count_on_hand < 0")
+      .update_all(count_on_hand: 0)
+  end
+
+  def down
+    raise ActiveRecord::IrreversibleMigration
+  end
+end

db/schema.rb

@@ -901,6 +901,7 @@ ActiveRecord::Schema.define(version: 20200702112157) do
     t.datetime "updated_at",                        null: false
     t.boolean  "backorderable",     default: false
     t.datetime "deleted_at"
+    t.integer  "lock_version",      default: 0
   end
 
   add_index "spree_stock_items", ["stock_location_id", "variant_id"], name: "stock_item_by_loc_and_var_id", using: :btree

spec/controllers/checkout_controller_concurrency_spec.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 require 'spec_helper'
 
 # This is the first example of testing concurrency in the Open Food Network.
@@ -15,6 +17,20 @@ describe CheckoutController, concurrency: true, type: :controller do
   let(:payment_method) { create(:payment_method, distributors: [distributor]) }
   let(:breakpoint) { Mutex.new }
 
+  let(:address_params) { address.attributes.except("id") }
+  let(:order_params) {
+    {
+      "payments_attributes" => [
+        {
+          "payment_method_id" => payment_method.id,
+          "amount" => order.total
+        }
+      ],
+      "bill_address_attributes" => address_params,
+      "ship_address_attributes" => address_params,
+    }
+  }
+
   before do
     # Create a valid order ready for checkout:
     create(:shipping_method, distributors: [distributor])
@@ -26,7 +42,9 @@ describe CheckoutController, concurrency: true, type: :controller do
     allow(controller).to receive(:spree_current_user).and_return(order.user)
     allow(controller).to receive(:current_distributor).and_return(order.distributor)
     allow(controller).to receive(:current_order_cycle).and_return(order.order_cycle)
+  end
 
+  it "handles two concurrent orders successfully" do
     # New threads start running straight away. The breakpoint is after loading
     # the order and before advancing the order's state and making payments.
     breakpoint.lock
@@ -36,21 +54,6 @@ describe CheckoutController, concurrency: true, type: :controller do
       # I did not find out how to call the original code otherwise.
       ActiveSupport::Notifications.instrument("spree.checkout.update")
     end
-  end
-
-  it "waits for concurrent checkouts" do
-    # Basic data the user submits during checkout:
-    address_params = address.attributes.except("id")
-    order_params = {
-      "payments_attributes" => [
-        {
-          "payment_method_id" => payment_method.id,
-          "amount" => order.total
-        }
-      ],
-      "bill_address_attributes" => address_params,
-      "ship_address_attributes" => address_params,
-    }
 
     # Starting two checkout threads. The controller code will determine if
     # these two threads are synchronised correctly or run into a race condition.

spec/models/spree/line_item_spec.rb

@@ -97,7 +97,7 @@ module Spree
       end
 
       it "caps at zero when stock is negative" do
-        v.update! on_hand: -2
+        v.__send__(:stock_item).update_column(:count_on_hand, -2)
         li.cap_quantity_at_stock!
         expect(li.reload.quantity).to eq 0
       end
@@ -123,7 +123,7 @@ module Spree
           before { vo.update(count_on_hand: -3) }
 
           it "caps at zero" do
-            v.update(on_hand: -2)
+            v.__send__(:stock_item).update_column(:count_on_hand, -2)
             li.cap_quantity_at_stock!
             expect(li.reload.quantity).to eq 0
           end

spec/models/spree/stock_item_spec.rb

@@ -0,0 +1,106 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Spree::StockItem do
+  let(:stock_location) { create(:stock_location_with_items) }
+
+  subject { stock_location.stock_items.order(:id).first }
+
+  describe "validation" do
+    let(:stock_item) { stock_location.stock_items.first }
+
+    it "requires count_on_hand to be positive if not backorderable" do
+      stock_item.backorderable = false
+
+      stock_item.__send__(:count_on_hand=, 1)
+      expect(stock_item.valid?).to eq(true)
+
+      stock_item.__send__(:count_on_hand=, 0)
+      expect(stock_item.valid?).to eq(true)
+
+      stock_item.__send__(:count_on_hand=, -1)
+      expect(stock_item.valid?).to eq(false)
+    end
+
+    it "allows count_on_hand to be negative if backorderable" do
+      stock_item.backorderable = true
+
+      stock_item.__send__(:count_on_hand=, 1)
+      expect(stock_item.valid?).to eq(true)
+
+      stock_item.__send__(:count_on_hand=, -1)
+      expect(stock_item.valid?).to eq(true)
+    end
+  end
+
+  it 'maintains the count on hand for a variant' do
+    expect(subject.count_on_hand).to eq 15
+  end
+
+  it "can return the stock item's variant's name" do
+    expect(subject.variant_name).to eq(subject.variant.name)
+  end
+
+  context "available to be included in shipment" do
+    context "has stock" do
+      it { expect(subject).to be_available }
+    end
+
+    context "backorderable" do
+      before { subject.backorderable = true }
+      it { expect(subject).to be_available }
+    end
+
+    context "no stock and not backorderable" do
+      before do
+        subject.backorderable = false
+        allow(subject).to receive_messages(count_on_hand: 0)
+      end
+
+      it { expect(subject).not_to be_available }
+    end
+  end
+
+  context "adjust count_on_hand" do
+    let!(:current_on_hand) { subject.count_on_hand }
+
+    it 'is updated pessimistically' do
+      copy = Spree::StockItem.find(subject.id)
+
+      subject.adjust_count_on_hand(5)
+      expect(subject.count_on_hand).to eq(current_on_hand + 5)
+
+      expect(copy.count_on_hand).to eq(current_on_hand)
+      copy.adjust_count_on_hand(5)
+      expect(copy.count_on_hand).to eq(current_on_hand + 10)
+    end
+
+    context "item out of stock (by two items)" do
+      let(:inventory_unit) { double('InventoryUnit') }
+      let(:inventory_unit_2) { double('InventoryUnit2') }
+
+      before do
+        allow(subject).to receive(:backorderable?).and_return(true)
+        subject.adjust_count_on_hand(- (current_on_hand + 2))
+      end
+
+      it "doesn't process backorders" do
+        expect(subject).not_to receive(:backordered_inventory_units)
+        subject.adjust_count_on_hand(1)
+      end
+
+      context "adds new items" do
+        before { allow(subject).to receive_messages(backordered_inventory_units: [inventory_unit, inventory_unit_2]) }
+
+        it "fills existing backorders" do
+          expect(inventory_unit).to receive(:fill_backorder)
+          expect(inventory_unit_2).to receive(:fill_backorder)
+
+          subject.adjust_count_on_hand(3)
+          expect(subject.count_on_hand).to eq(1)
+        end
+      end
+    end
+  end
+end