From 6c8a4ac7ad735846803d38d30337500fb3ed843e Mon Sep 17 00:00:00 2001 From: Tomas Martin Date: Tue, 25 Aug 2026 19:18:35 -0300 Subject: [PATCH 1/3] feat: [TESIS-103] add stock transfers between warehouses MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Builds the domain TESIS-62 was waiting on. The "In Transit" tab and the "+N Incoming" hint of the Master Catalog were never a missing column: there was no way to say that units left one warehouse and have not reached another yet. `stocks` answers "how many units are in this warehouse" and cannot hold units that are in neither. One derived number, not two. At product level in-transit and incoming are the same figure — units in flight are simultaneously travelling and arriving somewhere. They only diverge per warehouse, and the catalog row is per product. So the API exposes `in_transit_quantity` once and it feeds both the tab filter and the badge. Supplier purchase orders stay out. TESIS-62 says "products moving between nodes", which is internal movement, not external replenishment. That is a different domain and a different card. No draft state, deliberately. A transfer created but not dispatched leaves the units in the origin, so it adds nothing to the two numbers this model exists to produce, and would add a state nobody queries. Creating a transfer is dispatching it: the units come off the origin at that moment, land in the destination on receive, and go back to the origin on cancel. Units in flight are excluded from total_stock on purpose — they are not available in any node, which is the whole reason for exposing them apart. The catalog aggregates them with a scalar subquery rather than a second left_joins. with_total_stock already joins stocks and groups by products.id; joining a second child table would produce a cartesian product between them and multiply the stock SUM. There is a spec that dispatches twice from the same product and asserts total_stock stays correct, plus one asserting the subquery adds zero queries per row. Every transition runs inside the product's advisory lock (ADR-009), and the read of the balance sits inside the same lock as the write — which is why AdjustWarehouseStock does not take the lock itself: taking it there would close it before the transfer row is written and leave open exactly the window the lock exists to close. Settling re-checks in-flight status inside the lock too, so two concurrent receives cannot add the units to the destination twice. The DB carries the guarantees the model cannot: CHECK constraints for a positive quantity, the status vocabulary, and distinct endpoints, with specs that prove update_all cannot bypass them. Foreign keys are RESTRICT rather than cascade — a transfer records real units already deducted from an origin, so deleting the product or a warehouse would strand them with no trace. Same reasoning as order_items to products. Not verified: `bin/rails db:seed` still aborts on a clean run, but the cause predates this branch — `ml_integration` is referenced in the TESIS-40 seed block without ever being assigned. PR #56 fixes it and is not merged yet, so the fix is deliberately not duplicated here. The seeded transfer itself was verified through a runner: NOR-002 reports in_transit_quantity 5 with total_stock 20. Co-Authored-By: Claude Opus 5 --- .../api/v1/stock_transfers_controller.rb | 79 +++++++++++ app/models/product.rb | 29 ++++- app/models/stock_transfer.rb | 58 +++++++++ app/policies/stock_transfer_policy.rb | 25 ++++ app/poros/catalog/adjust_warehouse_stock.rb | 46 +++++++ app/poros/catalog/dispatch_transfer.rb | 41 ++++++ app/poros/catalog/insufficient_stock_error.rb | 18 +++ app/poros/catalog/settle_transfer.rb | 54 ++++++++ app/serializers/product_list_serializer.rb | 2 +- app/serializers/product_serializer.rb | 2 +- app/serializers/stock_transfer_serializer.rb | 22 ++++ config/routes.rb | 7 + .../20260825120000_create_stock_transfers.rb | 41 ++++++ db/schema.rb | 27 +++- db/seeds.rb | 15 +++ docs/guidelines/architecture.md | 15 ++- spec/models/stock_transfer_spec.rb | 88 +++++++++++++ spec/poros/catalog/dispatch_transfer_spec.rb | 64 +++++++++ spec/poros/catalog/settle_transfer_spec.rb | 87 +++++++++++++ spec/requests/api/v1/products_spec.rb | 57 ++++++++ spec/requests/api/v1/stock_transfers_spec.rb | 123 ++++++++++++++++++ 21 files changed, 895 insertions(+), 5 deletions(-) create mode 100644 app/controllers/api/v1/stock_transfers_controller.rb create mode 100644 app/models/stock_transfer.rb create mode 100644 app/policies/stock_transfer_policy.rb create mode 100644 app/poros/catalog/adjust_warehouse_stock.rb create mode 100644 app/poros/catalog/dispatch_transfer.rb create mode 100644 app/poros/catalog/insufficient_stock_error.rb create mode 100644 app/poros/catalog/settle_transfer.rb create mode 100644 app/serializers/stock_transfer_serializer.rb create mode 100644 db/migrate/20260825120000_create_stock_transfers.rb create mode 100644 spec/models/stock_transfer_spec.rb create mode 100644 spec/poros/catalog/dispatch_transfer_spec.rb create mode 100644 spec/poros/catalog/settle_transfer_spec.rb create mode 100644 spec/requests/api/v1/stock_transfers_spec.rb diff --git a/app/controllers/api/v1/stock_transfers_controller.rb b/app/controllers/api/v1/stock_transfers_controller.rb new file mode 100644 index 0000000..96fbfad --- /dev/null +++ b/app/controllers/api/v1/stock_transfers_controller.rb @@ -0,0 +1,79 @@ +# frozen_string_literal: true + +module Api + module V1 + class StockTransfersController < ApplicationController + before_action :set_transfer, only: %i[receive cancel] + rescue_from Catalog::InsufficientStockError, with: :render_unprocessable + rescue_from Catalog::SettleTransfer::NotInFlightError, with: :render_conflict + + def index + transfers = policy_scope(StockTransfer) + .includes(:product, :origin_warehouse, :destination_warehouse) + .order(dispatched_at: :desc) + transfers = transfers.where(status: params[:status]) if params[:status].present? + transfers = transfers.where(product_id: params[:product_id]) if params[:product_id].present? + + render json: { data: StockTransferSerializer.render_as_hash(transfers) } + end + + def create + authorize StockTransfer + + transfer = Catalog::DispatchTransfer.new( + company: current_company, product: product_for_create, + origin_warehouse: warehouse_for(:origin_warehouse_id), + destination_warehouse: warehouse_for(:destination_warehouse_id), + quantity: transfer_params[:quantity] + ).call + + render json: StockTransferSerializer.render(transfer), status: :created + end + + def receive + settle(:received) + end + + def cancel + settle(:cancelled) + end + + private + + def settle(outcome) + transfer = Catalog::SettleTransfer.new(transfer: @transfer, outcome: outcome).call + + render json: StockTransferSerializer.render(transfer), status: :ok + end + + def set_transfer + @transfer = StockTransfer.find(params.expect(:id)) + authorize @transfer, :"#{action_name}?" + end + + # find y no find_by en el scope del tenant: el default_scope de + # CompanyScoped ya acota, así que un id de otra empresa levanta + # RecordNotFound -> 404, que es lo que corresponde (no revelar que existe). + def product_for_create + Product.find(transfer_params[:product_id]) + end + + def warehouse_for(key) + Warehouse.find(transfer_params[key]) + end + + def transfer_params + # permit y no expect, igual que en productos: un body con company_id se + # ignora en lugar de devolver 400. + # rubocop:disable Rails/StrongParametersExpect + params.require(:stock_transfer) + .permit(:product_id, :origin_warehouse_id, :destination_warehouse_id, :quantity) + # rubocop:enable Rails/StrongParametersExpect + end + + def render_conflict(exception) + render json: { error: exception.message }, status: :conflict + end + end + end +end diff --git a/app/models/product.rb b/app/models/product.rb index c4fab73..fedb811 100644 --- a/app/models/product.rb +++ b/app/models/product.rb @@ -11,8 +11,23 @@ class Product < ApplicationRecord # Sumar una categoría tiene que ser una línea acá, no una migración. CATEGORIES = %w[Electronics Machinery Cabling Power].freeze + # Unidades en vuelo hacia/desde depósitos, como subconsulta escalar. + # + # Subconsulta y no un segundo left_joins: `with_total_stock` ya hace join con + # `stocks` y agrupa por products.id. Sumar un join a `stock_transfers` daría + # producto cartesiano entre las dos tablas hijas y el SUM de stocks quedaría + # multiplicado por la cantidad de transferencias. Es una query igual —no N+1— + # pero sin contaminar la agregación existente. + IN_TRANSIT_SUBQUERY = <<~SQL.squish + SELECT COALESCE(SUM(st.quantity), 0) FROM stock_transfers st + WHERE st.product_id = products.id AND st.status = 'in_transit' + SQL + belongs_to :company has_many :stocks, dependent: :destroy + # restrict_with_error: una transferencia en vuelo son unidades reales ya + # descontadas del origen. Borrar el producto las haría desaparecer sin rastro. + has_many :stock_transfers, dependent: :restrict_with_error has_many :product_mappings, dependent: :destroy # Bloquea el borrado si hay ítems de órdenes: son registros financieros y no # deben evaporarse por un DELETE. destroy! levanta RecordNotDestroyed -> 409 (API). @@ -28,7 +43,8 @@ class Product < ApplicationRecord scope :with_total_stock, lambda { left_joins(:stocks) .group(:id) - .select('products.*', 'COALESCE(SUM(stocks.quantity), 0) AS total_stock') + .select('products.*', 'COALESCE(SUM(stocks.quantity), 0) AS total_stock', + "(#{IN_TRANSIT_SUBQUERY}) AS in_transit_quantity") } # Retorna el stock total consolidado. Si la fila fue cargada con el scope @@ -40,6 +56,17 @@ def total_stock has_attribute?(:total_stock) ? self[:total_stock].to_i : stocks.sum(:quantity) end + # Unidades que salieron de un depósito y todavía no llegaron a otro. No están + # en `total_stock` a propósito: no son stock disponible en ningún nodo. + # + # Misma mecánica que total_stock: si la fila vino del scope, el alias del + # SELECT ya trae el agregado; si no, se suma por asociación (detalle, alta). + def in_transit_quantity + return self[:in_transit_quantity].to_i if has_attribute?(:in_transit_quantity) + + stock_transfers.in_flight.sum(:quantity) + end + # Depósito donde está el grueso de las unidades. Lo consume la columna # "Location Node" del listado, que muestra un nodo y no el desglose. # diff --git a/app/models/stock_transfer.rb b/app/models/stock_transfer.rb new file mode 100644 index 0000000..10c6f47 --- /dev/null +++ b/app/models/stock_transfer.rb @@ -0,0 +1,58 @@ +# frozen_string_literal: true + +# Unidades de un producto que salieron de un depósito y todavía no llegaron a +# otro. Existe para poder expresar algo que `stocks` no puede: mientras viajan, +# las unidades no están en ningún nodo. +# +# Por eso NO se cuentan en `Product#total_stock`. El catálogo las expone aparte +# (`in_transit_quantity`), que es lo que alimenta el tab "In Transit" y el +# "+N Incoming" de TESIS-62. +# +# No hay estado borrador a propósito: una transferencia creada y no despachada +# deja las unidades en el origen, así que no aporta a los números que este +# modelo existe para producir. Crear una transferencia ES despacharla. +class StockTransfer < ApplicationRecord + include CompanyScoped + + STATUSES = { in_transit: 'in_transit', received: 'received', cancelled: 'cancelled' }.freeze + + belongs_to :company + belongs_to :product + belongs_to :origin_warehouse, class_name: 'Warehouse' + belongs_to :destination_warehouse, class_name: 'Warehouse' + + # Mismo criterio que WebhookLog y FailedEvent: el enum da predicados y scopes, + # y `validate: true` invalida un estado desconocido en vez de explotar al + # asignarlo. + enum :status, STATUSES, validate: true + + validates :quantity, numericality: { only_integer: true, greater_than: 0 } + validate :warehouses_are_distinct + validate :product_and_warehouses_belong_to_company + + # Unidades en vuelo por producto. Se usa como subconsulta agregada desde el + # listado del catálogo: una sola query para toda la página, no una por fila. + scope :in_flight, -> { where(status: STATUSES[:in_transit]) } + + private + + def warehouses_are_distinct + return if origin_warehouse_id.blank? || destination_warehouse_id.blank? + return if origin_warehouse_id != destination_warehouse_id + + errors.add(:destination_warehouse, 'must be different from the origin warehouse') + end + + # El producto y los dos depósitos tienen que ser de la misma empresa que la + # transferencia: evita mover unidades entre tenants. Mismo criterio que Stock. + def product_and_warehouses_belong_to_company + return if company_id.blank? + + { product: product, origin_warehouse: origin_warehouse, + destination_warehouse: destination_warehouse }.each do |name, record| + next if record.blank? || record.company_id == company_id + + errors.add(name, 'must belong to the same company as the transfer') + end + end +end diff --git a/app/policies/stock_transfer_policy.rb b/app/policies/stock_transfer_policy.rb new file mode 100644 index 0000000..7ce239b --- /dev/null +++ b/app/policies/stock_transfer_policy.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +class StockTransferPolicy < ApplicationPolicy + def index? + user.present? + end + + def create? + user.present? + end + + def receive? + record.company_id == user.company_id + end + + def cancel? + record.company_id == user.company_id + end + + class Scope < ApplicationPolicy::Scope + def resolve + scope.where(company_id: user.company_id) + end + end +end diff --git a/app/poros/catalog/adjust_warehouse_stock.rb b/app/poros/catalog/adjust_warehouse_stock.rb new file mode 100644 index 0000000..6e8ddeb --- /dev/null +++ b/app/poros/catalog/adjust_warehouse_stock.rb @@ -0,0 +1,46 @@ +# frozen_string_literal: true + +module Catalog + # Suma o resta unidades de un producto en un depósito concreto. + # + # Es el único lugar que escribe `stocks.quantity` para las transferencias, y + # las tres transiciones (despachar, recibir, cancelar) pasan por acá — por eso + # existe como pieza aparte y no repetida en cada una. + # + # NO toma el advisory lock: lo toma quien la invoca, porque una transferencia + # necesita que la lectura del saldo y la escritura estén dentro del MISMO lock + # que la creación de la fila de transferencia. Tomarlo acá lo cerraría antes de + # tiempo y dejaría la ventana que el lock existe para cerrar (ADR-009). + class AdjustWarehouseStock < ApplicationPoro + def initialize(product:, warehouse:, delta:) + super() + @product = product + @warehouse = warehouse + @delta = delta.to_i + end + + def call + stock = Stock.find_or_initialize_by(product_id: @product.id, warehouse_id: @warehouse.id) + resulting = stock.quantity.to_i + @delta + ensure_available!(stock, resulting) + + stock.quantity = resulting + stock.save! + stock + end + + private + + # El CHECK `stocks_quantity_non_negative` es la última línea de defensa, pero + # llegaría como CheckViolation genérica. Cortar acá deja un error que nombra + # el depósito, lo disponible y lo pedido. + def ensure_available!(stock, resulting) + return unless resulting.negative? + + raise InsufficientStockError.new( + product_id: @product.id, warehouse_id: @warehouse.id, + available: stock.quantity.to_i, requested: @delta.abs + ) + end + end +end diff --git a/app/poros/catalog/dispatch_transfer.rb b/app/poros/catalog/dispatch_transfer.rb new file mode 100644 index 0000000..c7bf176 --- /dev/null +++ b/app/poros/catalog/dispatch_transfer.rb @@ -0,0 +1,41 @@ +# frozen_string_literal: true + +module Catalog + # Despacha unidades de un depósito a otro: descuenta del origen y deja la + # transferencia en vuelo. + # + # Crear la transferencia y mover el stock es una sola operación atómica, bajo + # el advisory lock del producto (ADR-009): la lectura del saldo del origen y su + # escritura tienen que estar dentro del mismo lock, o dos despachos + # simultáneos del mismo producto podrían descontar sobre el mismo saldo. + class DispatchTransfer < ApplicationPoro + def initialize(company:, product:, origin_warehouse:, destination_warehouse:, quantity:) + super() + @company = company + @product = product + @origin = origin_warehouse + @destination = destination_warehouse + @quantity = quantity + end + + def call + WithStockLock.new(product_id: @product.id, wait: false).call do + transfer = build_transfer + # Se valida antes de tocar stock: un origen igual al destino o una + # cantidad inválida no deben dejar unidades descontadas. + transfer.save! + AdjustWarehouseStock.new(product: @product, warehouse: @origin, + delta: -transfer.quantity).call + transfer + end + end + + private + + def build_transfer + StockTransfer.new(company: @company, product: @product, origin_warehouse: @origin, + destination_warehouse: @destination, quantity: @quantity, + status: :in_transit, dispatched_at: Time.current) + end + end +end diff --git a/app/poros/catalog/insufficient_stock_error.rb b/app/poros/catalog/insufficient_stock_error.rb new file mode 100644 index 0000000..f05b922 --- /dev/null +++ b/app/poros/catalog/insufficient_stock_error.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +module Catalog + # No hay unidades suficientes en el depósito para el movimiento pedido. + # El controller lo mapea a 422: es un dato del request, no un fallo del sistema. + class InsufficientStockError < StandardError + attr_reader :product_id, :warehouse_id, :available, :requested + + def initialize(product_id:, warehouse_id:, available:, requested:) + @product_id = product_id + @warehouse_id = warehouse_id + @available = available + @requested = requested + super("warehouse #{warehouse_id} holds #{available} units of product " \ + "#{product_id}, cannot move #{requested}") + end + end +end diff --git a/app/poros/catalog/settle_transfer.rb b/app/poros/catalog/settle_transfer.rb new file mode 100644 index 0000000..2a6949b --- /dev/null +++ b/app/poros/catalog/settle_transfer.rb @@ -0,0 +1,54 @@ +# frozen_string_literal: true + +module Catalog + # Cierra una transferencia en vuelo: la recibe en el destino o la cancela + # devolviendo las unidades al origen. + # + # Las dos transiciones comparten todo salvo a qué depósito vuelven las + # unidades y con qué estado queda la fila, así que viven en una sola pieza en + # lugar de dos casi idénticas. + class SettleTransfer < ApplicationPoro + class NotInFlightError < StandardError; end + + OUTCOMES = { + received: :destination_warehouse, + cancelled: :origin_warehouse + }.freeze + + def initialize(transfer:, outcome:) + super() + @transfer = transfer + @outcome = outcome.to_sym + end + + def call + warehouse_method = OUTCOMES.fetch(@outcome) do + raise ArgumentError, "unknown outcome '#{@outcome}'" + end + + WithStockLock.new(product_id: @transfer.product_id, wait: false).call do + # Se revisa adentro del lock: dos requests simultáneos sobre la misma + # transferencia podrían pasar los dos el chequeo si estuviera afuera, y + # las unidades entrarían al destino dos veces. + ensure_in_flight! + settle(@transfer.public_send(warehouse_method)) + end + end + + private + + def ensure_in_flight! + return if @transfer.reload.in_transit? + + raise NotInFlightError, + "transfer #{@transfer.id} is already #{@transfer.status}" + end + + def settle(warehouse) + AdjustWarehouseStock.new(product: @transfer.product, warehouse: warehouse, + delta: @transfer.quantity).call + @transfer.update!(status: @outcome, settled_at: Time.current) + @transfer + end + end +end diff --git a/app/serializers/product_list_serializer.rb b/app/serializers/product_list_serializer.rb index 5494747..e032fb7 100644 --- a/app/serializers/product_list_serializer.rb +++ b/app/serializers/product_list_serializer.rb @@ -4,7 +4,7 @@ class ProductListSerializer < ApplicationSerializer identifier :id fields :sku, :name, :description, :category, :dimensions, :total_stock, - :created_at, :updated_at + :in_transit_quantity, :created_at, :updated_at # weight es decimal en la DB y BigDecimal se serializa como string por # defecto; exponerlo como número evita que el front tenga que parsear. diff --git a/app/serializers/product_serializer.rb b/app/serializers/product_serializer.rb index 07da6fd..694a8c3 100644 --- a/app/serializers/product_serializer.rb +++ b/app/serializers/product_serializer.rb @@ -4,7 +4,7 @@ class ProductSerializer < ApplicationSerializer identifier :id fields :sku, :name, :description, :category, :dimensions, :total_stock, - :created_at, :updated_at + :in_transit_quantity, :created_at, :updated_at # weight es decimal en la DB y BigDecimal se serializa como string por # defecto; exponerlo como número evita que el front tenga que parsear. diff --git a/app/serializers/stock_transfer_serializer.rb b/app/serializers/stock_transfer_serializer.rb new file mode 100644 index 0000000..8199f1b --- /dev/null +++ b/app/serializers/stock_transfer_serializer.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +class StockTransferSerializer < ApplicationSerializer + identifier :id + + fields :quantity, :status, :dispatched_at, :settled_at, :created_at, :updated_at + + # Sólo la identidad del producto: quien lista transferencias ya tiene el + # catálogo, y anidar el serializer completo traería total_stock —una agregación + # por fila— para nada. + field :product do |transfer| + { id: transfer.product_id, sku: transfer.product.sku, name: transfer.product.name } + end + + field :origin_warehouse do |transfer| + { id: transfer.origin_warehouse_id, name: transfer.origin_warehouse.name } + end + + field :destination_warehouse do |transfer| + { id: transfer.destination_warehouse_id, name: transfer.destination_warehouse.name } + end +end diff --git a/config/routes.rb b/config/routes.rb index 07d7115..76dc1e8 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -19,6 +19,13 @@ resources :mappings, only: %i[index create destroy], controller: 'product_mappings' end + resources :stock_transfers, path: 'stock-transfers', only: %i[index create] do + member do + post :receive + post :cancel + end + end + resources :failed_events, path: 'failed-events', only: %i[index] do member do post :retry, action: :requeue diff --git a/db/migrate/20260825120000_create_stock_transfers.rb b/db/migrate/20260825120000_create_stock_transfers.rb new file mode 100644 index 0000000..48f76f2 --- /dev/null +++ b/db/migrate/20260825120000_create_stock_transfers.rb @@ -0,0 +1,41 @@ +# frozen_string_literal: true + +class CreateStockTransfers < ActiveRecord::Migration[8.1] + def change + create_table :stock_transfers do |t| + t.references :company, null: false, foreign_key: { on_delete: :cascade } + # RESTRICT y no cascade: una transferencia es el registro de un movimiento + # de unidades reales. Si se pudiera borrar el producto o el depósito con + # transferencias vivas, quedarían unidades descontadas del origen sin + # rastro de a dónde iban. Mismo criterio que order_items -> products. + t.references :product, null: false, foreign_key: { on_delete: :restrict } + t.references :origin_warehouse, null: false, + foreign_key: { to_table: :warehouses, on_delete: :restrict } + t.references :destination_warehouse, null: false, + foreign_key: { to_table: :warehouses, + on_delete: :restrict } + t.integer :quantity, null: false + t.string :status, null: false, default: 'in_transit' + # Cuándo salieron las unidades del origen. NOT NULL porque crear la + # transferencia ES despacharla: no existe el estado borrador. + t.datetime :dispatched_at, null: false + # Cuándo se resolvió (recibida o cancelada). Nulo mientras está en vuelo. + t.datetime :settled_at + + t.timestamps + end + + add_check_constraint :stock_transfers, 'quantity > 0', + name: 'stock_transfers_quantity_positive' + add_check_constraint :stock_transfers, + "status IN ('in_transit', 'received', 'cancelled')", + name: 'stock_transfers_status_check' + add_check_constraint :stock_transfers, + 'origin_warehouse_id <> destination_warehouse_id', + name: 'stock_transfers_distinct_warehouses' + + # El listado del catálogo suma las unidades en vuelo por producto, siempre + # dentro de una empresa y siempre filtrando por estado. + add_index :stock_transfers, %i[company_id status product_id] + end +end diff --git a/db/schema.rb b/db/schema.rb index 943095c..4747867 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[8.1].define(version: 2026_08_23_120000) do +ActiveRecord::Schema[8.1].define(version: 2026_08_25_120000) do # These are extensions that must be enabled in order to support this database enable_extension "pg_catalog.plpgsql" @@ -140,6 +140,27 @@ t.check_constraint "type::text = ANY (ARRAY['ecommerce'::character varying::text, 'courier'::character varying::text])", name: "services_type_check" end + create_table "stock_transfers", force: :cascade do |t| + t.bigint "company_id", null: false + t.datetime "created_at", null: false + t.bigint "destination_warehouse_id", null: false + t.datetime "dispatched_at", null: false + t.bigint "origin_warehouse_id", null: false + t.bigint "product_id", null: false + t.integer "quantity", null: false + t.datetime "settled_at" + t.string "status", default: "in_transit", null: false + t.datetime "updated_at", null: false + t.index ["company_id", "status", "product_id"], name: "index_stock_transfers_on_company_id_and_status_and_product_id" + t.index ["company_id"], name: "index_stock_transfers_on_company_id" + t.index ["destination_warehouse_id"], name: "index_stock_transfers_on_destination_warehouse_id" + t.index ["origin_warehouse_id"], name: "index_stock_transfers_on_origin_warehouse_id" + t.index ["product_id"], name: "index_stock_transfers_on_product_id" + t.check_constraint "origin_warehouse_id <> destination_warehouse_id", name: "stock_transfers_distinct_warehouses" + t.check_constraint "quantity > 0", name: "stock_transfers_quantity_positive" + t.check_constraint "status::text = ANY (ARRAY['in_transit'::character varying, 'received'::character varying, 'cancelled'::character varying]::text[])", name: "stock_transfers_status_check" + end + create_table "stocks", force: :cascade do |t| t.datetime "created_at", null: false t.bigint "product_id", null: false @@ -202,6 +223,10 @@ add_foreign_key "product_mappings", "company_integrations", on_delete: :cascade add_foreign_key "product_mappings", "products", on_delete: :cascade add_foreign_key "products", "companies", on_delete: :cascade + add_foreign_key "stock_transfers", "companies", on_delete: :cascade + add_foreign_key "stock_transfers", "products", on_delete: :restrict + add_foreign_key "stock_transfers", "warehouses", column: "destination_warehouse_id", on_delete: :restrict + add_foreign_key "stock_transfers", "warehouses", column: "origin_warehouse_id", on_delete: :restrict add_foreign_key "stocks", "products", on_delete: :cascade add_foreign_key "stocks", "warehouses", on_delete: :restrict add_foreign_key "users", "companies", on_delete: :cascade diff --git a/db/seeds.rb b/db/seeds.rb index f51177f..9d3845e 100644 --- a/db/seeds.rb +++ b/db/seeds.rb @@ -210,6 +210,21 @@ Stock.find_or_create_by!(product: mouse, warehouse: satelite) { |s| s.quantity = 30 } end + # Transferencia en vuelo (TESIS-103): unidades que ya salieron del Central y + # todavía no llegaron al Satélite. No se usa DispatchTransfer porque el stock + # sembrado arriba ya refleja el saldo posterior al despacho; acá sólo se + # registra el movimiento para que el catálogo tenga un producto con + # `in_transit_quantity > 0` y el tab "In Transit" muestre algo real. + if central && satelite + StockTransfer.find_or_create_by!(product: notebook, origin_warehouse: central, + destination_warehouse: satelite, + status: 'in_transit') do |t| + t.company = norte_company + t.quantity = 5 + t.dispatched_at = 2.days.ago + end + end + # Identity Mapping: vincula productos de Norte con Mercado Libre, usando la # integración de la plantilla de stock (la de órdenes no transmite stock). ml_stock_service = Service.find_by(service_name: 'Mercado Libre - Stock') diff --git a/docs/guidelines/architecture.md b/docs/guidelines/architecture.md index 6b8bc38..18151cc 100644 --- a/docs/guidelines/architecture.md +++ b/docs/guidelines/architecture.md @@ -224,11 +224,24 @@ El proyecto está organizado en 6 dominios correspondientes a los epics de Jira: | ------------ | ---------- | -------------------------------------------------------- | | `auth` | TESIS-19 | Empresas, usuarios, depósitos, autenticación JWT | | `integrations` | TESIS-20 | Plantillas de APIs externas, credenciales por empresa | -| `catalog` | TESIS-21 | Productos, stock por depósito, sincronización multicanal | +| `catalog` | TESIS-21 | Productos, stock por depósito, transferencias entre depósitos, sincronización multicanal | | `webhooks` | TESIS-22 | Gateway de webhooks, cola de mensajes, reintentos | | `orders` | TESIS-23 | Órdenes de compra, ítems, consolidación multicanal | | `shipments` | TESIS-24 | Envíos, cotización de couriers, tracking | +> **Unidades en vuelo.** `stocks` responde "cuántas unidades hay en este +> depósito", y no puede expresar unidades que salieron de uno y todavía no +> llegaron a otro. Eso vive en `stock_transfers` (TESIS-103): al despachar se +> descuentan del origen, al recibir se suman al destino, y mientras viajan no +> pertenecen a ningún nodo — por eso **no** entran en `Product#total_stock` y se +> exponen aparte como `in_transit_quantity`. +> +> Las tres transiciones mueven stock real bajo el advisory lock del producto +> (§9), y el listado del catálogo agrega las unidades en vuelo con una +> **subconsulta escalar**, no con un segundo `left_joins`: `with_total_stock` ya +> hace join con `stocks` y agrupa, así que un segundo join a una tabla hija daría +> producto cartesiano y multiplicaría el `SUM` de stock. + Los POROs y Jobs se organizan por dominio: ``` diff --git a/spec/models/stock_transfer_spec.rb b/spec/models/stock_transfer_spec.rb new file mode 100644 index 0000000..f5cd1d8 --- /dev/null +++ b/spec/models/stock_transfer_spec.rb @@ -0,0 +1,88 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe StockTransfer, type: :model do + subject(:transfer) do + described_class.new(company: company, product: product, origin_warehouse: origin, + destination_warehouse: destination, quantity: 5, + dispatched_at: Time.current) + end + + let(:company) { Company.create!(name: 'Acme', tax_id: '20-12345678-9') } + let(:product) { Product.create!(company: company, sku: 'SKU-1', name: 'Widget') } + let(:origin) { warehouse('Central', '1900') } + let(:destination) { warehouse('North', '1901') } + + def warehouse(name, zip, owner: company) + Warehouse.create!(company: owner, name: name, zip_code: zip, address: "Calle #{zip}") + end + + def other_company + @other_company ||= Company.create!(name: 'Other', tax_id: '30-22222222-2') + end + + it 'is valid with the required attributes' do + expect(transfer).to be_valid + end + + it 'defaults to in_transit' do + transfer.save! + expect(transfer.status).to eq('in_transit') + end + + it 'rejects a non-positive quantity' do + transfer.quantity = 0 + expect(transfer).not_to be_valid + end + + it 'rejects the same warehouse on both ends', :aggregate_failures do + transfer.destination_warehouse = origin + expect(transfer).not_to be_valid + expect(transfer.errors[:destination_warehouse]).to be_present + end + + # `validate: true` en el enum existe para que un estado desconocido invalide + # el registro en lugar de explotar en el asignador (mismo criterio que + # WebhookLog y FailedEvent). + it 'rejects an unknown status without raising on assignment' do + transfer.status = 'lost' + expect(transfer).not_to be_valid + end + + describe 'the same-company rule' do + it 'rejects a product from another company' do + transfer.product = Current.set(company_id: nil) do + Product.create!(company: other_company, sku: 'B-1', name: 'Other') + end + expect(transfer).not_to be_valid + end + + it 'rejects a warehouse from another company' do + transfer.destination_warehouse = Current.set(company_id: nil) do + warehouse('Foreign', '3000', owner: other_company) + end + expect(transfer).not_to be_valid + end + end + + describe 'the database constraints' do + # Las validaciones del modelo son la primera línea; el CHECK es la que no se + # puede saltear con update_all / SQL crudo. Mismo criterio que stocks.quantity. + it 'rejects a non-positive quantity at the database level' do + transfer.save! + + expect do + described_class.where(id: transfer.id).update_all(quantity: 0) # rubocop:disable Rails/SkipsModelValidations + end.to raise_error(ActiveRecord::StatementInvalid) + end + + it 'rejects an unknown status at the database level' do + transfer.save! + + expect do + described_class.where(id: transfer.id).update_all(status: 'lost') # rubocop:disable Rails/SkipsModelValidations + end.to raise_error(ActiveRecord::StatementInvalid) + end + end +end diff --git a/spec/poros/catalog/dispatch_transfer_spec.rb b/spec/poros/catalog/dispatch_transfer_spec.rb new file mode 100644 index 0000000..76d4763 --- /dev/null +++ b/spec/poros/catalog/dispatch_transfer_spec.rb @@ -0,0 +1,64 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe Catalog::DispatchTransfer, type: :poro do + subject(:dispatch) do + described_class.new(company: company, product: product, origin_warehouse: origin, + destination_warehouse: destination, quantity: quantity) + end + + let(:company) { Company.create!(name: 'Acme', tax_id: '20-12345678-9') } + let(:product) { Product.create!(company: company, sku: 'SKU-1', name: 'Widget') } + let(:origin) { Warehouse.create!(company: company, name: 'Central', zip_code: '1900', address: 'A') } + let(:destination) { Warehouse.create!(company: company, name: 'North', zip_code: '1901', address: 'B') } + let(:quantity) { 4 } + + before { Stock.create!(product: product, warehouse: origin, quantity: 10) } + + def origin_quantity = Stock.find_by(product: product, warehouse: origin).quantity + + it 'leaves the transfer in flight' do + expect(dispatch.call).to be_in_transit + end + + it 'deducts the units from the origin warehouse' do + dispatch.call + expect(origin_quantity).to eq(6) + end + + it 'does not put the units in the destination yet' do + dispatch.call + expect(Stock.find_by(product: product, warehouse: destination)).to be_nil + end + + it 'keeps the units out of total_stock while they travel' do + dispatch.call + expect(product.reload.total_stock).to eq(6) + end + + it 'reports them as in transit' do + dispatch.call + expect(product.reload.in_transit_quantity).to eq(4) + end + + context 'when the origin does not hold enough units' do + let(:quantity) { 99 } + + it 'raises instead of leaving a negative balance' do + expect { dispatch.call }.to raise_error(Catalog::InsufficientStockError) + end + + context 'when the dispatch has already failed' do + before { suppress(Catalog::InsufficientStockError) { dispatch.call } } + + it 'writes no transfer at all' do + expect(StockTransfer.count).to eq(0) + end + + it 'leaves the origin balance untouched' do + expect(origin_quantity).to eq(10) + end + end + end +end diff --git a/spec/poros/catalog/settle_transfer_spec.rb b/spec/poros/catalog/settle_transfer_spec.rb new file mode 100644 index 0000000..dbb0655 --- /dev/null +++ b/spec/poros/catalog/settle_transfer_spec.rb @@ -0,0 +1,87 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe Catalog::SettleTransfer, type: :poro do + let(:company) { Company.create!(name: 'Acme', tax_id: '20-12345678-9') } + let(:product) { Product.create!(company: company, sku: 'SKU-1', name: 'Widget') } + let(:origin) { Warehouse.create!(company: company, name: 'Central', zip_code: '1900', address: 'A') } + let(:destination) { Warehouse.create!(company: company, name: 'North', zip_code: '1901', address: 'B') } + + let(:transfer) do + Stock.create!(product: product, warehouse: origin, quantity: 10) + Catalog::DispatchTransfer.new(company: company, product: product, origin_warehouse: origin, + destination_warehouse: destination, quantity: 4).call + end + + def quantity_in(warehouse) + Stock.find_by(product: product, warehouse: warehouse)&.quantity || 0 + end + + describe 'receiving' do + before { described_class.new(transfer: transfer, outcome: :received).call } + + it 'marks the transfer as received' do + expect(transfer.reload).to be_received + end + + it 'adds the units to the destination' do + expect(quantity_in(destination)).to eq(4) + end + + it 'does not give them back to the origin' do + expect(quantity_in(origin)).to eq(6) + end + + it 'stops counting them as in transit' do + expect(product.reload.in_transit_quantity).to eq(0) + end + + it 'returns them to total_stock' do + expect(product.reload.total_stock).to eq(10) + end + + it 'stamps when it was settled' do + expect(transfer.reload.settled_at).to be_present + end + end + + describe 'cancelling' do + before { described_class.new(transfer: transfer, outcome: :cancelled).call } + + it 'marks the transfer as cancelled' do + expect(transfer.reload).to be_cancelled + end + + it 'gives the units back to the origin' do + expect(quantity_in(origin)).to eq(10) + end + + it 'leaves nothing in the destination' do + expect(quantity_in(destination)).to eq(0) + end + end + + # Sin este corte, dos requests simultáneos sobre la misma transferencia + # sumarían las unidades al destino dos veces. + it 'refuses to settle a transfer that is no longer in flight' do + described_class.new(transfer: transfer, outcome: :received).call + + expect { described_class.new(transfer: transfer, outcome: :cancelled).call } + .to raise_error(described_class::NotInFlightError) + end + + it 'does not move stock twice when settled again' do + described_class.new(transfer: transfer, outcome: :received).call + suppress(described_class::NotInFlightError) do + described_class.new(transfer: transfer, outcome: :received).call + end + + expect(quantity_in(destination)).to eq(4) + end + + it 'rejects an unknown outcome' do + expect { described_class.new(transfer: transfer, outcome: :lost).call } + .to raise_error(ArgumentError) + end +end diff --git a/spec/requests/api/v1/products_spec.rb b/spec/requests/api/v1/products_spec.rb index 2bffe44..4146b28 100644 --- a/spec/requests/api/v1/products_spec.rb +++ b/spec/requests/api/v1/products_spec.rb @@ -159,6 +159,63 @@ def holding_advisory_lock_for(product) it 'exposes the category of each product' do expect(response.parsed_body['data'].pluck('category')).to all(be_nil) end + + it 'reports no units in transit when there are no transfers' do + expect(response.parsed_body['data'].pluck('in_transit_quantity')).to all(eq(0)) + end + end + + context 'with units travelling between warehouses' do + let(:warehouse) do + Warehouse.create!(company: company, name: 'Central', zip_code: '1900', address: 'Calle 1') + end + + def north + @north ||= Warehouse.create!(company: company, name: 'North', + zip_code: '1901', address: 'Calle 2') + end + + def dispatch(product, quantity) + Catalog::DispatchTransfer.new(company: company, product: product, + origin_warehouse: warehouse, destination_warehouse: north, + quantity: quantity).call + end + + def row + get '/api/v1/products', headers: headers + response.parsed_body['data'].find { |item| item['sku'] == 'A-001' } + end + + def stocked_product + Product.create!(company: company, sku: 'A-001', name: 'Alpha').tap do |product| + Stock.create!(product: product, warehouse: warehouse, quantity: 10) + end + end + + it 'reports the units in flight apart from the stock', :aggregate_failures do + dispatch(stocked_product, 3) + + expect(row['in_transit_quantity']).to eq(3) + expect(row['total_stock']).to eq(7) + end + + # La subconsulta escalar existe para no romper el SUM de total_stock: un + # segundo left_joins daría producto cartesiano entre stocks y transfers. + it 'does not corrupt total_stock when a product has several transfers' do + product = stocked_product + 2.times { dispatch(product, 2) } + + expect(row['total_stock']).to eq(6) + end + + # 0 y no 1: va como subconsulta escalar dentro del SELECT del listado, así + # que no hay ninguna consulta separada contra stock_transfers. + it 'adds no query per row' do + create_products_with_stock(10) + queries = count_queries(matching: /FROM "stock_transfers"/) { get '/api/v1/products', headers: headers } + + expect(queries).to eq(0) + end end context 'with stock spread across warehouses' do diff --git a/spec/requests/api/v1/stock_transfers_spec.rb b/spec/requests/api/v1/stock_transfers_spec.rb new file mode 100644 index 0000000..69ef31f --- /dev/null +++ b/spec/requests/api/v1/stock_transfers_spec.rb @@ -0,0 +1,123 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe 'Stock transfers API', type: :request do + let(:company) { Company.create!(name: 'Tenant A', tax_id: '30-11111111-1') } + let(:user) { User.create!(email: 'a@example.com', password: 'password123', company: company) } + let(:headers) { auth_headers(user) } + let(:product) { Product.create!(company: company, sku: 'A-001', name: 'Alpha') } + + def auth_headers(user) + post '/api/v1/auth/login', params: { email: user.email, password: 'password123' } + { 'Authorization' => "Bearer #{response.parsed_body['token']}" } + end + + def warehouse(name, zip) + Warehouse.create!(company: company, name: name, zip_code: zip, address: "Calle #{zip}") + end + + def origin = @origin ||= warehouse('Central', '1900') + def destination = @destination ||= warehouse('North', '1901') + + def body_for(quantity) + { stock_transfer: { product_id: product.id, origin_warehouse_id: origin.id, + destination_warehouse_id: destination.id, quantity: quantity } } + end + + # El tenant se fuerza a nil para que el fixture nazca SIEMPRE en la otra + # empresa: assign_current_company pisaría el company: manual si Current + # quedó seteado por un request previo. + def foreign_product + @foreign_product ||= Current.set(company_id: nil) do + other = Company.create!(name: 'Tenant B', tax_id: '30-22222222-2') + Product.create!(company: other, sku: 'B-001', name: 'Other') + end + end + + def dispatch_one(quantity: 4) + Stock.find_or_create_by!(product: product, warehouse: origin) { |s| s.quantity = 10 } + Catalog::DispatchTransfer.new(company: company, product: product, origin_warehouse: origin, + destination_warehouse: destination, quantity: quantity).call + end + + describe 'POST /api/v1/stock-transfers' do + before { Stock.create!(product: product, warehouse: origin, quantity: 10) } + + it 'returns 401 without a token' do + post '/api/v1/stock-transfers', params: body_for(4), as: :json + expect(response).to have_http_status(:unauthorized) + end + + it 'creates the transfer', :aggregate_failures do + post '/api/v1/stock-transfers', params: body_for(4), headers: headers, as: :json + + expect(response).to have_http_status(:created) + expect(response.parsed_body['status']).to eq('in_transit') + end + + it 'returns 422 when the origin cannot cover the quantity' do + post '/api/v1/stock-transfers', params: body_for(99), headers: headers, as: :json + + expect(response).to have_http_status(:unprocessable_content) + end + + it 'returns 422 when both ends are the same warehouse' do + params = body_for(1).deep_merge(stock_transfer: { destination_warehouse_id: origin.id }) + post '/api/v1/stock-transfers', params: params, headers: headers, as: :json + + expect(response).to have_http_status(:unprocessable_content) + end + + it 'returns 404 for a product of another company' do + params = body_for(1).deep_merge(stock_transfer: { product_id: foreign_product.id }) + post '/api/v1/stock-transfers', params: params, headers: headers, as: :json + + expect(response).to have_http_status(:not_found) + end + end + + describe 'GET /api/v1/stock-transfers' do + it 'lists the transfers of the company' do + dispatch_one + get '/api/v1/stock-transfers', headers: headers + + expect(response.parsed_body['data'].length).to eq(1) + end + + it 'filters by status' do + dispatch_one + get '/api/v1/stock-transfers', params: { status: 'received' }, headers: headers + + expect(response.parsed_body['data']).to be_empty + end + end + + describe 'POST /api/v1/stock-transfers/:id/receive' do + it 'settles the transfer into the destination', :aggregate_failures do + transfer = dispatch_one + post "/api/v1/stock-transfers/#{transfer.id}/receive", headers: headers + + expect(response).to have_http_status(:ok) + expect(Stock.find_by(product: product, warehouse: destination).quantity).to eq(4) + end + + it 'returns 409 when it is no longer in flight' do + transfer = dispatch_one + Catalog::SettleTransfer.new(transfer: transfer, outcome: :received).call + post "/api/v1/stock-transfers/#{transfer.id}/receive", headers: headers + + expect(response).to have_http_status(:conflict) + end + end + + describe 'POST /api/v1/stock-transfers/:id/cancel' do + it 'gives the units back to the origin', :aggregate_failures do + transfer = dispatch_one + post "/api/v1/stock-transfers/#{transfer.id}/cancel", headers: headers + + expect(response).to have_http_status(:ok) + expect(Stock.find_by(product: product, warehouse: origin).quantity).to eq(10) + end + end +end From b3da34c1402a9b5d6d05b3b86e5c39da38cae71d Mon Sep 17 00:00:00 2001 From: Tomas Martin Date: Fri, 28 Aug 2026 18:13:08 -0300 Subject: [PATCH 2/3] feat: [TESIS-101] detect concurrent edits of a product with If-Match MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the lost update ADR-009 left open. The advisory lock of TESIS-38 orders the writes; it cannot tell that two people edited the same thing, because the modal sends an absolute quantity and serialising does not change which one wins. This detects instead of ordering. GET /products/:id now returns an ETag with a fingerprint of the aggregate, and PUT accepts it back in If-Match. If the product moved in between, the write is rejected and nothing is written. Not lock_version, and the reason is the shape of what is edited. That column versions the products row, while the modal edits an aggregate: name, measurements and the quantity of every warehouse. Covering the stock with lock_version would mean bumping it from Stock's callbacks — writing to products on every sale, which is write amplification and contention on the hottest path in the system. The fingerprint covers the whole aggregate with no migration and no column. The fingerprint includes the stock deliberately. The dangerous case is not two operators, it is a sale: if a webhook deducts 5 units while the modal is open, saving the absolute quantity the user saw would erase that deduction with no trace. There is a spec for exactly that — stock moves underneath, no one edits anything, and the write is still rejected. The check runs inside the transaction and behind a lock! — SELECT ... FOR UPDATE on the product row. Comparing outside it would let through two requests that read the same version, which is the race being closed. With the row held, the second one waits, re-reads what the first left, and its version no longer matches. 412 rather than the 409 the card asked for. It is the code HTTP defines for a failed precondition, and it settles the card's own requirement of telling this apart from the other two 409s this endpoint already returns (duplicate SKU and stock lock busy) — with 412 the status is enough and the frontend does not have to read the body to know which conflict it is. Without If-Match the update proceeds as before. That is HTTP precondition semantics and it keeps the previous contract, at the cost of making the protection opt-in: a client that does not send the header is not protected. Both consequences are written down in the ADR. Renamed set_version_header to expose_version: it is not a writer, and Naming/AccessorMethodName was right to complain. The frontend half — the modal sending the version and handling the 412 without losing what the user typed — goes in its own PR on proyecto-web. Co-Authored-By: Claude Opus 5 --- app/controllers/api/v1/products_controller.rb | 33 +++++- app/poros/catalog/product_version.rb | 47 ++++++++ app/poros/catalog/stale_product_error.rb | 17 +++ app/poros/products/update_product.rb | 25 ++++- docs/adr/ADR-009-bloqueos-distribuidos.md | 51 ++++++++- spec/poros/catalog/product_version_spec.rb | 67 ++++++++++++ spec/requests/api/v1/products_spec.rb | 100 ++++++++++++++++++ 7 files changed, 337 insertions(+), 3 deletions(-) create mode 100644 app/poros/catalog/product_version.rb create mode 100644 app/poros/catalog/stale_product_error.rb create mode 100644 spec/poros/catalog/product_version_spec.rb diff --git a/app/controllers/api/v1/products_controller.rb b/app/controllers/api/v1/products_controller.rb index 3ff8e38..08f2b4f 100644 --- a/app/controllers/api/v1/products_controller.rb +++ b/app/controllers/api/v1/products_controller.rb @@ -6,6 +6,7 @@ class ProductsController < ApplicationController before_action :set_product, only: %i[show update destroy] rescue_from ActiveRecord::RecordNotUnique, with: :render_conflict rescue_from ActiveRecord::RecordNotSaved, with: :render_unprocessable + rescue_from Catalog::StaleProductError, with: :render_precondition_failed def index page = [params[:page].to_i, 1].max @@ -27,6 +28,7 @@ def index end def show + expose_version(@product) render json: ProductSerializer.render(@product) end @@ -46,9 +48,11 @@ def update product = Products::UpdateProduct.new( product: @product, params: product_params, - stocks: stock_params + stocks: stock_params, + expected_version: expected_version ).call + expose_version(product) render json: ProductSerializer.render(product), status: :ok end @@ -59,6 +63,33 @@ def destroy private + # La version del agregado viaja como ETag (TESIS-101). El cliente la + # devuelve en `If-Match` al guardar y el servidor rechaza la escritura si + # ya no es la vigente. + def expose_version(product) + response.set_header('ETag', %("#{Catalog::ProductVersion.new(product: product).call}")) + end + + # `If-Match` puede venir con comillas, con el prefijo debil `W/` o como + # `*`. `*` significa "cualquier version, siempre que exista": el producto + # ya se resolvio en set_product, asi que equivale a no poner precondicion. + def expected_version + raw = request.headers['If-Match'].to_s.strip + return nil if raw.blank? || raw == '*' + + raw.delete_prefix('W/').delete_prefix('"').delete_suffix('"') + end + + # 412 y no 409, apartandose de lo que pedia la card. Es el codigo que HTTP + # define para una precondicion que no se cumple, y de paso resuelve solo el + # requisito de distinguirlo: este endpoint ya devuelve 409 por SKU + # duplicado y por lock de stock ocupado, y un tercer 409 obligaria al front + # a leer el cuerpo para saber cual es. Con 412 alcanza el status. + def render_precondition_failed(exception) + render json: { error: exception.message, current_version: exception.current_version }, + status: :precondition_failed + end + def set_product # Eager load de stocks y sus warehouses para evitar N+1 en el detalle. @product = Product.includes(stocks: :warehouse).find(params.expect(:id)) diff --git a/app/poros/catalog/product_version.rb b/app/poros/catalog/product_version.rb new file mode 100644 index 0000000..cc6a83e --- /dev/null +++ b/app/poros/catalog/product_version.rb @@ -0,0 +1,47 @@ +# frozen_string_literal: true + +require 'digest' + +module Catalog + # Huella del estado del producto tal como lo vio el cliente: los campos que el + # ABM edita, mas el stock de cada deposito. + # + # Es la version que viaja como ETag y vuelve en `If-Match`. Cubre el agregado + # completo y no solo la fila `products` a proposito: el modal edita nombre, + # medidas y cantidades a la vez, y una version que solo mirara `products` no + # detectaria que alguien movio el stock mientras el modal estaba abierto. + # + # Y no solo protege de otro operador: el caso mas peligroso es una venta. Si un + # webhook descuenta 5 unidades entre que el modal abre y guarda, guardar la + # cantidad ABSOLUTA que el usuario vio borraria ese descuento sin dejar rastro. + # Por eso la huella incluye el stock venga de donde venga el cambio. + class ProductVersion < ApplicationPoro + SEPARATOR = '|' + + def initialize(product:) + super() + @product = product + end + + def call + Digest::SHA256.hexdigest(fingerprint) + end + + private + + def fingerprint + (edited_fields + stock_pairs).join(SEPARATOR) + end + + def edited_fields + [@product.name.to_s, @product.description.to_s, + @product.weight.to_s, @product.dimensions.to_s] + end + + # Los depositos se ordenan antes de digerir: `product.stocks` no garantiza + # orden, y sin esto la misma fila daria huellas distintas entre requests. + def stock_pairs + @product.stocks.sort_by(&:warehouse_id).map { |s| "#{s.warehouse_id}:#{s.quantity}" } + end + end +end diff --git a/app/poros/catalog/stale_product_error.rb b/app/poros/catalog/stale_product_error.rb new file mode 100644 index 0000000..9369e39 --- /dev/null +++ b/app/poros/catalog/stale_product_error.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +module Catalog + # El cliente mando `If-Match` con una version que ya no es la vigente: alguien + # toco el producto entre que lo leyo y lo guardo. + # + # Lleva la version actual para que el controller la devuelva en la respuesta: + # el cliente puede recargar y reintentar sin pedir el detalle de nuevo. + class StaleProductError < StandardError + attr_reader :current_version + + def initialize(current_version:) + @current_version = current_version + super('the product changed since it was loaded') + end + end +end diff --git a/app/poros/products/update_product.rb b/app/poros/products/update_product.rb index b168181..ff8d1e5 100644 --- a/app/poros/products/update_product.rb +++ b/app/poros/products/update_product.rb @@ -4,15 +4,17 @@ module Products class UpdateProduct < ApplicationPoro include Concerns::WarehouseValidation - def initialize(product:, params:, stocks:) + def initialize(product:, params:, stocks:, expected_version: nil) super() @product = product @params = params @stocks_params = stocks + @expected_version = expected_version end def call Product.transaction do + verify_version! @product.update!(@params) if @stocks_params.present? @@ -26,6 +28,27 @@ def call private + # Locking optimista (TESIS-101). El chequeo va DENTRO de la transaccion y + # detras de un `lock!` --o sea `SELECT ... FOR UPDATE` sobre la fila del + # producto-- y no antes: comparar afuera dejaria pasar a dos requests que + # leyeron la misma version, que es exactamente la carrera que esto cierra. + # Con la fila tomada, el segundo espera, relee el estado que dejo el primero + # y su version ya no coincide. + # + # Sin `expected_version` no hay precondicion que verificar y el update pasa + # como siempre: es la semantica de `If-Match` en HTTP, y mantiene el + # contrato anterior para cualquier cliente que no lo mande. + def verify_version! + return if @expected_version.blank? + + @product.lock! + @product.stocks.reload + current = Catalog::ProductVersion.new(product: @product).call + return if current == @expected_version + + raise Catalog::StaleProductError.new(current_version: current) + end + # Advisory lock y no sólo FOR UPDATE: el upsert puede crear filas de # stocks que todavía no existen, y ahí FOR UPDATE no tiene nada que # bloquear. Además la operación abarca varias filas del mismo producto, diff --git a/docs/adr/ADR-009-bloqueos-distribuidos.md b/docs/adr/ADR-009-bloqueos-distribuidos.md index 1af3c6e..e88d828 100644 --- a/docs/adr/ADR-009-bloqueos-distribuidos.md +++ b/docs/adr/ADR-009-bloqueos-distribuidos.md @@ -85,7 +85,56 @@ Catalog::WithStockLock.new(product_id: product.id, wait: false).call { ... } - ✅ Sin infraestructura nueva: PostgreSQL ya es parte del stack y ya es la fuente de verdad del stock - ✅ No hay locks huérfanos que limpiar: el motor los libera solo, al terminar la transacción (commit o rollback) - ✅ El PORO es explícito y auditable: clave, timeout y manejo de errores están a la vista en ~40 líneas -- ⚠️ El lock ordena escrituras, no detecta ediciones concurrentes: el lost update del ABM sigue abierto hasta que se implemente el locking optimista +- ✅ El lost update del ABM lo cierra el locking optimista de TESIS-101 (ver sección siguiente): el lock ordena, el ETag detecta - ⚠️ La granularidad por producto serializa de más cuando una empresa escribe stock del mismo producto en depósitos distintos al mismo tiempo - ⚠️ Las escrituras batch (`update_all`, `upsert_all`, SQL crudo) se saltean el PORO por completo — por eso existe el `CHECK` de la base como red de seguridad independiente - ⚠️ Los specs de concurrencia necesitan desactivar el envoltorio transaccional de RSpec (`use_transactional_tests = false`): si no, las dos "conexiones" que se quieren probar en paralelo viven dentro de la misma transacción de test y nunca compiten de verdad por el lock + +--- + +## Complemento: locking optimista del ABM (TESIS-101) + +Este ADR dejaba abierto el *lost update* del ABM. Se cierra con un mecanismo +distinto, porque responde a otra pregunta: el advisory lock **ordena** las +escrituras, y acá hace falta **detectar** que dos personas editaron lo mismo. + +### Decisión: ETag + `If-Match`, no `lock_version` + +`GET /products/:id` devuelve un `ETag` con la huella del agregado, y +`PUT /products/:id` la exige de vuelta en `If-Match`. + +**Por qué no `lock_version`.** La columna versiona la fila `products`, y el modal +edita un agregado: nombre, medidas **y** las cantidades de cada depósito. Para +que `lock_version` cubriera el stock habría que bumpearlo desde los callbacks de +`Stock`, o sea escribir en `products` en cada venta — amplificación de escritura +y contención justo en el camino más caliente del sistema. El ETag cubre el +agregado completo sin tocar la base ni sumar una migración. + +**Qué cubre la huella** (`Catalog::ProductVersion`): los campos que el modal +edita más el par `warehouse_id:quantity` de cada depósito, ordenados. Que incluya +el stock no es un extra: el escenario más peligroso no son dos operadores sino +**una venta**. Si un webhook descuenta 5 unidades mientras el modal está abierto, +guardar la cantidad *absoluta* que el usuario vio borraría ese descuento sin +dejar rastro. + +**Dónde se verifica.** Dentro de la transacción y detrás de un `lock!` +(`SELECT ... FOR UPDATE` sobre la fila del producto). Comparar afuera dejaría +pasar a dos requests que leyeron la misma versión — exactamente la carrera que +esto cierra. + +### Desvío de la card: 412, no 409 + +La card pedía 409 «con un cuerpo que permita distinguirlo de los otros dos 409». +Se devuelve **412 Precondition Failed**, que es el código que HTTP define para +una precondición incumplida — y de paso resuelve solo el requisito: el endpoint +ya devuelve 409 por SKU duplicado y por lock de stock ocupado, y un tercer 409 +obligaría al front a leer el cuerpo para saber cuál es. Con 412 alcanza el +status. + +### Consecuencias + +- ✅ Sin migración y sin columna nueva +- ✅ Cubre cualquier escritor: otro operador, una venta por webhook, un job +- ✅ Sin `If-Match` el update pasa como siempre — semántica de HTTP, y no rompe el contrato anterior +- ⚠️ Esa misma compatibilidad hace que la protección sea **opt-in**: un cliente que no manda el header no está protegido +- ⚠️ La huella se recalcula en cada `show` y en cada `update`; es un SHA-256 sobre unas pocas decenas de bytes, pero no es gratis diff --git a/spec/poros/catalog/product_version_spec.rb b/spec/poros/catalog/product_version_spec.rb new file mode 100644 index 0000000..33977ed --- /dev/null +++ b/spec/poros/catalog/product_version_spec.rb @@ -0,0 +1,67 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe Catalog::ProductVersion, type: :poro do + let(:company) { Company.create!(name: 'Acme', tax_id: '20-12345678-9') } + let(:product) { Product.create!(company: company, sku: 'SKU-1', name: 'Widget', weight: 2) } + let(:central) do + Warehouse.create!(company: company, name: 'Central', zip_code: '1900', address: 'A') + end + let(:north) do + Warehouse.create!(company: company, name: 'North', zip_code: '1901', address: 'B') + end + + # Metodo y no `subject`: RSpec memoiza el subject, y estos ejemplos necesitan + # leer la version dos veces con un cambio en el medio. + def version + described_class.new(product: product.reload).call + end + + it 'is stable when nothing changes' do + expect(version).to eq(described_class.new(product: product.reload).call) + end + + it 'changes when an edited field changes' do + before_change = version + product.update!(name: 'Renamed') + + expect(version).not_to eq(before_change) + end + + # El caso que motiva la card: el modal guarda cantidades absolutas, asi que un + # movimiento de stock ajeno tiene que invalidar lo que el usuario vio. + it 'changes when the stock of a warehouse moves' do + stock = Stock.create!(product: product, warehouse: central, quantity: 10) + before_change = version + stock.update!(quantity: 5) + + expect(version).not_to eq(before_change) + end + + it 'changes when a warehouse is added' do + Stock.create!(product: product, warehouse: central, quantity: 10) + before_change = version + Stock.create!(product: product, warehouse: north, quantity: 3) + + expect(version).not_to eq(before_change) + end + + # Sin ordenar, la misma fila daria huellas distintas segun como Postgres + # devuelva los stocks, y el guardado fallaria al azar. + it 'does not depend on the order the stocks come back in' do + Stock.create!(product: product, warehouse: north, quantity: 3) + Stock.create!(product: product, warehouse: central, quantity: 10) + reversed = Product.find(product.id) + allow(reversed).to receive(:stocks).and_return(reversed.stocks.to_a.reverse) + + expect(described_class.new(product: reversed).call).to eq(version) + end + + it 'is not affected by fields the modal does not edit' do + before_change = version + product.update_column(:sku, 'SKU-CHANGED') # rubocop:disable Rails/SkipsModelValidations + + expect(version).to eq(before_change) + end +end diff --git a/spec/requests/api/v1/products_spec.rb b/spec/requests/api/v1/products_spec.rb index bae7daa..fe1943f 100644 --- a/spec/requests/api/v1/products_spec.rb +++ b/spec/requests/api/v1/products_spec.rb @@ -202,6 +202,106 @@ def holding_advisory_lock_for(product) end end + describe 'optimistic locking with If-Match' do + let(:warehouse) do + Warehouse.create!(company: company, name: 'Central', zip_code: '1900', address: 'Calle 1') + end + let!(:product) do + p = Product.create!(company: company, sku: 'A-001', name: 'Alpha', weight: 1) + Stock.create!(product: p, warehouse: warehouse, quantity: 10) + p + end + + def current_version + get "/api/v1/products/#{product.id}", headers: headers + response.headers['ETag'] + end + + def save(version, quantity: 7, name: 'Alpha') + body = { product: { name: name, weight: 1, + stocks: stocks_for(warehouse.id, quantity: quantity) } } + put "/api/v1/products/#{product.id}", + params: body, headers: headers.merge('If-Match' => version.to_s), as: :json + end + + it 'exposes the version as an ETag on show' do + expect(current_version).to be_present + end + + it 'accepts the write when the version still matches' do + save(current_version) + expect(response).to have_http_status(:ok) + end + + it 'returns the new version after a successful write', :aggregate_failures do + before_write = current_version + save(before_write) + + expect(response.headers['ETag']).to be_present + expect(response.headers['ETag']).not_to eq(before_write) + end + + # La carrera real de la card: dos ediciones que partieron de la misma + # version. La segunda no puede pisar a la primera en silencio. + context 'when someone else already saved' do + # Metodo y no `let` para no pasar el tope de helpers memoizados del grupo. + def stale + @stale ||= current_version + end + + before { save(stale, quantity: 20, name: 'First writer') } + + it 'rejects the second write with 412' do + save(stale, quantity: 3, name: 'Second writer') + expect(response).to have_http_status(:precondition_failed) + end + + it 'does not apply the second write' do + save(stale, quantity: 3, name: 'Second writer') + expect(product.reload.name).to eq('First writer') + end + + it 'does not touch the stock either' do + save(stale, quantity: 3, name: 'Second writer') + expect(Stock.find_by(product: product, warehouse: warehouse).quantity).to eq(20) + end + + it 'hands back the current version so the client can reload' do + save(stale, quantity: 3) + expect(response.parsed_body['current_version']).to be_present + end + end + + # El caso mas peligroso no es otro operador: es una venta descontando stock + # mientras el modal esta abierto. Guardar la cantidad absoluta lo borraria. + it 'rejects the write when stock moved underneath, even if nobody edited' do + stale = current_version + Stock.find_by(product: product, warehouse: warehouse).update!(quantity: 5) + save(stale) + + expect(response).to have_http_status(:precondition_failed) + end + + # Semantica de HTTP: sin precondicion, no hay nada que verificar. Mantiene + # el contrato anterior para un cliente que no manda el header. + it 'writes without If-Match, as before' do + body = { product: { name: 'No header', weight: 1 } } + put "/api/v1/products/#{product.id}", params: body, headers: headers, as: :json + + expect(response).to have_http_status(:ok) + end + + it 'treats If-Match: * as no precondition' do + save('*') + expect(response).to have_http_status(:ok) + end + + it 'accepts a weak or quoted version' do + save("W/#{current_version}") + expect(response).to have_http_status(:ok) + end + end + describe 'POST /api/v1/products' do let(:warehouse) do Warehouse.create!(company: company, name: 'Central', zip_code: '1900', address: 'Calle 1') From ec2df142799ae0d28ed80887b90f2c0d83ba0442 Mon Sep 17 00:00:00 2001 From: Tomas Martin Date: Fri, 28 Aug 2026 18:14:22 -0300 Subject: [PATCH 3/3] fix: [TESIS-101] expose the ETag header through CORS MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The optimistic locking is useless from the browser without this. CORS only lets JavaScript read the simple response headers unless the server lists the rest, and the frontend runs on a different origin (5173 against 3000), so `response.headers.etag` arrives undefined, the modal sends no If-Match and the protection is silently off — nothing fails, the guarantee just is not there. Found while wiring the frontend half, not by reading the diff. Co-Authored-By: Claude Opus 5 --- config/initializers/cors.rb | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/config/initializers/cors.rb b/config/initializers/cors.rb index ff47924..5acc094 100644 --- a/config/initializers/cors.rb +++ b/config/initializers/cors.rb @@ -10,6 +10,13 @@ origins '*' resource '*', headers: :any, - methods: %i[get post put patch delete options head] + methods: %i[get post put patch delete options head], + # Sin `expose`, el browser le oculta el ETag al JavaScript: CORS + # sólo deja leer los headers simples salvo que el servidor los + # liste. El front corre en otro origen (5173 contra 3000), así que + # sin esta línea `response.headers.etag` llega `undefined`, el + # modal no manda `If-Match` y el locking optimista de TESIS-101 + # queda desactivado sin que nada falle a la vista. + expose: %w[ETag] end end