diff --git a/app/poros/products/update_product.rb b/app/poros/products/update_product.rb index ff8d1e59..cd0d399f 100644 --- a/app/poros/products/update_product.rb +++ b/app/poros/products/update_product.rb @@ -14,14 +14,13 @@ def initialize(product:, params:, stocks:, expected_version: nil) def call Product.transaction do - verify_version! - @product.update!(@params) - - if @stocks_params.present? - write_stocks! - reload_for_render! + within_stock_lock do + verify_version! + @product.update!(@params) + write_stocks! if @stocks_params.present? end + reload_for_render! if @stocks_params.present? @product end end @@ -54,27 +53,33 @@ def verify_version! # bloquear. Además la operación abarca varias filas del mismo producto, # así que se serializa por producto y no fila por fila. # - # Ojo con el alcance: esto ordena las escrituras, no detecta ediciones - # concurrentes. Como la cantidad llega absoluta desde el request, dos - # operadores que editan el mismo producto siguen pisándose — el último - # gana. Detectar eso pide locking optimista (lock_version / If-Match) y - # un cambio de contrato; ver ADR-009. + # Con stocks en el request, el lock se toma ANTES de verificar la versión y + # cubre las dos cosas. Antes sólo envolvía la escritura: el `lock!` de la + # versión bloquea la fila de `products`, pero las ventas (`DeductStock`) y + # las transferencias no la tocan, sólo toman este advisory lock. Entre el + # chequeo y el lock entraba una venta, la versión ya validada no la veía y + # la cantidad absoluta del request la borraba sin rastro (hallazgo de la + # auditoría de TESIS-89). Bajo el mismo lock, la venta espera o el PUT + # responde 409. # - # El lock envuelve sólo la escritura de stocks y no el update! del - # producto: editar el nombre no compite por stock con nadie, y con - # wait: false envolver todo el #call devolvería 409 espurios mientras un - # job de sincronización toca los stocks en paralelo. + # Sin stocks, nada: editar el nombre no compite por stock con nadie, y con + # wait: false envolver ese caso devolvería 409 espurios mientras un job de + # sincronización toca los stocks en paralelo. # # wait: false porque esto corre en el ciclo de un request HTTP: conviene # devolver 409 enseguida (ApplicationController mapea el # Catalog::LockTimeoutError) antes que colgar un thread de Puma esperando. # El modo wait: true queda para los jobs de background. + def within_stock_lock(&) + return yield if @stocks_params.blank? + + Catalog::WithStockLock.new(product_id: @product.id, wait: false).call(&) + end + + # Corre ya dentro del advisory lock (ver `within_stock_lock`). def write_stocks! validate_warehouses_belong_to_company! - - Catalog::WithStockLock.new(product_id: @product.id, wait: false).call do - upsert_stocks_for_product! - end + upsert_stocks_for_product! end def upsert_stocks_for_product! diff --git a/spec/poros/products/update_product_spec.rb b/spec/poros/products/update_product_spec.rb new file mode 100644 index 00000000..6a9b1b3b --- /dev/null +++ b/spec/poros/products/update_product_spec.rb @@ -0,0 +1,85 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe Products::UpdateProduct, type: :poro do + def company = @company ||= Company.create!(name: 'Acme', tax_id: '20-12345678-9') + def product = @product ||= Product.create!(company: company, sku: 'CEL-1', name: 'Celular') + + def warehouse + @warehouse ||= Warehouse.create!(company: company, name: 'Central', zip_code: '1900', + address: 'Av 1') + end + + before do + Current.company_id = company.id + Stock.create!(product: product, warehouse: warehouse, quantity: 10) + end + + def current_version = Catalog::ProductVersion.new(product: product.reload).call + + def update(stocks:, expected_version: current_version) + described_class.new(product: product, params: { name: 'Celular X' }, stocks: stocks, + expected_version: expected_version).call + end + + # Desde otra sesión de Postgres, como lo vería una venta: el advisory lock es + # reentrante dentro de la misma conexión, así que preguntarlo desde la del + # test diría que está libre aunque no lo esté. + def stock_lock_free_for_others? + key = Catalog::WithStockLock.new(product_id: product.id).lock_key + config = ActiveRecord::Base.connection_db_config.configuration_hash + other = PG.connect(host: config[:host], port: config[:port], dbname: config[:database], + user: config[:username], password: config[:password]) + other.exec('BEGIN') + other.exec("SELECT pg_try_advisory_xact_lock(#{key})").getvalue(0, 0) == 't' + ensure + other&.exec('ROLLBACK') + other&.close + end + + # Qué tan libre estaba el lock de stock en el momento en que se calculó la + # versión del producto para compararla con el If-Match. + def lock_free_while_checking_the_version + observed = nil + allow(Catalog::ProductVersion).to receive(:new).and_wrap_original do |original, **kwargs| + observed = stock_lock_free_for_others? + original.call(**kwargs) + end + yield + observed + end + + # Hallazgo de auditoría (TESIS-89). Si el lock se tomaba recién para escribir, + # una venta entraba entre el chequeo y la escritura, la versión ya validada no + # la veía y la cantidad absoluta del request la borraba sin rastro. + it 'holds the stock lock while it checks the version, so no sale slips in between' do + version = current_version + + free = lock_free_while_checking_the_version do + update(stocks: [{ warehouse_id: warehouse.id, quantity: 3 }], expected_version: version) + end + + expect(free).to be(false) + end + + it 'takes no stock lock for a change that does not touch stocks' do + version = current_version + + free = lock_free_while_checking_the_version { update(stocks: [], expected_version: version) } + + expect(free).to be(true) + end + + it 'writes the stock it was sent' do + update(stocks: [{ warehouse_id: warehouse.id, quantity: 3 }]) + + expect(Stock.find_by(product: product, warehouse: warehouse).quantity).to eq(3) + end + + it 'refuses a stale version and writes nothing', :aggregate_failures do + expect { update(stocks: [{ warehouse_id: warehouse.id, quantity: 3 }], expected_version: 'stale') } + .to raise_error(Catalog::StaleProductError) + expect(Stock.find_by(product: product, warehouse: warehouse).quantity).to eq(10) + end +end diff --git a/spec/requests/api/v1/products_spec.rb b/spec/requests/api/v1/products_spec.rb index 2c1a67a5..32d30c80 100644 --- a/spec/requests/api/v1/products_spec.rb +++ b/spec/requests/api/v1/products_spec.rb @@ -869,8 +869,8 @@ def put_stock(quantity) expect(stock.reload.quantity).to eq(5) end - # El lock sólo envuelve la escritura de stocks (ver comentario en - # Products::UpdateProduct#write_stocks!): un PATCH que sólo cambia + # El lock sólo se toma cuando el request trae stocks (ver comentario en + # Products::UpdateProduct#within_stock_lock): un PATCH que sólo cambia # `name` no compite por él. Este ejemplo documenta esa decisión de # diseño de no envolver todo el update en el advisory lock. it 'still returns 200 for an update that does not touch stocks' do