From 899dd7c825fd30cde41c38c64b300849731426e6 Mon Sep 17 00:00:00 2001 From: Tomas Martin Date: Sun, 27 Sep 2026 21:38:46 -0300 Subject: [PATCH 1/2] fix: [TESIS-133] answer 400 instead of 500 to a malformed body `params.require(:x)` returns whatever sits under the key, so `permit` on a String raised NoMethodError and the API answered 500 to a body like `{"product": "x"}`. TESIS-82 fixed it in WarehousesController with `params.expect`; the same shape was still live in products, stock transfers, product mappings and orders. Stock transfers and product mappings move to `params.expect`, which rejects anything that is not an object. Products and orders keep `permit`, behind a new `body_of` guard in ApplicationController. `expect` treats an all-filtered-out hash as missing, and a PUT that only carries `stocks` or `items` is a valid request: those two parts are walked line by line, each with its own 422 and message, so they never reach the permit list and the filtered hash comes back empty. `body_of` does the same shape check without the filtering. The order wrapper used to answer 422 through a hand-rolled RecordNotSaved. Its shape is contract, not business, so it is now a 400 like the rest. ParameterMissing is rescued once in ApplicationController so the 400 carries `{"error": "..."}` (ADR-015) instead of Rails' own body; the three controllers that declared it themselves drop their copy. A company_id in the body is still dropped in silence: `expect` filters unpermitted keys exactly like `permit` does. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_012xAtddb53LRNVLuyTXyELp --- CLAUDE.md | 5 ++- .../api/v1/draft_quotes_controller.rb | 4 -- app/controllers/api/v1/orders_controller.rb | 35 +++++++++-------- .../api/v1/product_mappings_controller.rb | 9 +++-- app/controllers/api/v1/products_controller.rb | 23 ++++++++--- .../api/v1/shipments_controller.rb | 2 - .../api/v1/stock_transfers_controller.rb | 11 +++--- app/controllers/application_controller.rb | 23 +++++++++++ spec/requests/api/v1/order_updates_spec.rb | 17 ++++++++ spec/requests/api/v1/orders_spec.rb | 23 ++++++++--- spec/requests/api/v1/product_mappings_spec.rb | 20 ++++++++++ spec/requests/api/v1/products_spec.rb | 39 +++++++++++++++++++ spec/requests/api/v1/stock_transfers_spec.rb | 20 ++++++++++ 13 files changed, 189 insertions(+), 42 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 18867c0..c292b1e 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -251,8 +251,11 @@ module Api authorize @product end + # expect y no require + permit: require devuelve lo que haya bajo la + # clave, y permit sobre un String levanta NoMethodError -> 500. expect + # responde 400 a cualquier cosa que no sea un objeto (TESIS-133). def product_params - params.require(:product).permit(:sku, :name, :stock) + params.expect(product: %i[sku name stock]) end end end diff --git a/app/controllers/api/v1/draft_quotes_controller.rb b/app/controllers/api/v1/draft_quotes_controller.rb index 2c31313..81cec4e 100644 --- a/app/controllers/api/v1/draft_quotes_controller.rb +++ b/app/controllers/api/v1/draft_quotes_controller.rb @@ -10,10 +10,6 @@ module V1 # que el asistente ya juntó: depósito de origen, destino y líneas. La orden se # crea una sola vez, cuando el operador elige y confirma. class DraftQuotesController < ApplicationController - # El parámetro que falta es un 400 de contrato, con el mismo cuerpo - # `{ error }` que el resto de la API. - rescue_from ActionController::ParameterMissing, with: :render_bad_request - # Es el primer endpoint autenticado que sale a los couriers sin dejar nada # en la base: antes cotizar exigía crear la orden, que era un freno natural. # Sin tope, un cliente en loop —un `useEffect` mal puesto que cotiza en diff --git a/app/controllers/api/v1/orders_controller.rb b/app/controllers/api/v1/orders_controller.rb index f160174..d6161fb 100644 --- a/app/controllers/api/v1/orders_controller.rb +++ b/app/controllers/api/v1/orders_controller.rb @@ -8,10 +8,6 @@ class OrdersController < ApplicationController rescue_from ActiveRecord::RecordNotSaved, with: :render_unprocessable rescue_from Catalog::InsufficientStockError, with: :render_insufficient_stock - # ParameterMissing no es 422 de negocio: es un 400 de contrato. Rescatarlo - # acá mantiene la forma del body ({error: ...}) consistente con el resto - # de la API en vez del default de Rails. - rescue_from ActionController::ParameterMissing, with: :render_bad_request # Una orden cancelada o con el envío ya despachado: no hay body que haga # pasar el mismo PUT, así que es 409 y no 422 (TESIS-126). rescue_from Orders::OrderNotEditableError, with: :render_conflict @@ -19,6 +15,11 @@ class OrdersController < ApplicationController MAX_ITEMS = 100 + # Los datos del cliente que acepta el body. `items` queda afuera: se arma + # aparte, línea por línea, en `items_params`. + ORDER_FIELDS = %i[customer_name customer_document customer_address + customer_zip_code customer_city customer_province].freeze + # Campos sobre los que corre el buscador del listado (TESIS-52). Son las # formas en que un operador nombra una venta: el id con el que la conoce el # canal externo, el nombre con el que la cargó, y a dónde va. @@ -161,34 +162,36 @@ def apply_search(orders) orders.where(condition, pattern: pattern) end - def order_params(*extra_keys) - order = params.require(:order) - unless order.is_a?(ActionController::Parameters) - raise ActiveRecord::RecordNotSaved, 'order must be an object' - end - - order.permit(:customer_name, :customer_document, - :customer_address, :customer_zip_code, - :customer_city, :customer_province, *extra_keys) + def order_params + order_body.permit(*ORDER_FIELDS) end # Lo mismo que el alta más el estado. Qué valores de estado se aceptan lo # decide Orders::UpdateOrder (sólo pending y paid). def update_params - order_params(:status) + order_body.permit(*ORDER_FIELDS, :status) + end + + # El envoltorio, validado como objeto por `body_of`. La forma es contrato, + # así que un `order` String o Array es 400 y no el 422 de negocio que + # devolvía el RecordNotSaved que había acá (TESIS-133). Las `items` se leen + # de este mismo objeto: cada línea tiene sus propios 422 con mensaje —y el + # tope de MAX_ITEMS—, que un filtrado de strong params no sabe dar. + def order_body + body_of(:order) end # Sin `items` en el body, las líneas no se tocan: un PUT que sólo corrige la # dirección no tiene por qué mandar la orden entera. `id` identifica una # línea que ya existe; sin él, la línea es nueva. def update_items_params - return nil unless params[:order].key?(:items) + return nil unless order_body.key?(:items) items_params(:id) end def items_params(*extra_keys) - raw = params[:order][:items] + raw = order_body[:items] raise ActiveRecord::RecordNotSaved, 'items must be an array' unless raw.is_a?(Array) if raw.size > MAX_ITEMS raise ActiveRecord::RecordNotSaved, diff --git a/app/controllers/api/v1/product_mappings_controller.rb b/app/controllers/api/v1/product_mappings_controller.rb index d4a572c..63b3a22 100644 --- a/app/controllers/api/v1/product_mappings_controller.rb +++ b/app/controllers/api/v1/product_mappings_controller.rb @@ -74,10 +74,13 @@ def company_integration # El body va anidado bajo `product_mapping`, igual que `product` en # ProductsController: los dos endpoints del mismo árbol comparten contrato. + # + # `expect` y no `require` + `permit`: un `product_mapping` que no sea un + # objeto responde 400 en vez de reventar con 500 (TESIS-133). def mapping_params - # rubocop:disable-next Rails/StrongParametersExpect - params.require(:product_mapping) - .permit(:company_integration_id, :external_product_id, :external_price) + params.expect( + product_mapping: %i[company_integration_id external_product_id external_price] + ) end # El índice único (company_integration_id, external_product_id) no tiene diff --git a/app/controllers/api/v1/products_controller.rb b/app/controllers/api/v1/products_controller.rb index 5ed6b69..c8faae6 100644 --- a/app/controllers/api/v1/products_controller.rb +++ b/app/controllers/api/v1/products_controller.rb @@ -6,6 +6,10 @@ class ProductsController < ApplicationController include OptimisticLocking include Paginatable + # Qué acepta el body del producto. `stocks` no entra: se arma aparte en + # `stock_params`, línea por línea. + PRODUCT_FIELDS = %i[sku name description category weight dimensions].freeze + before_action :set_product, only: %i[show update destroy] rescue_from ActiveRecord::RecordNotUnique, with: :render_conflict rescue_from ActiveRecord::RecordNotSaved, with: :render_unprocessable @@ -110,16 +114,23 @@ def set_product authorize @product end + # Un company_id en el body se sigue descartando en silencio: `permit` sólo + # deja pasar lo que lista. def product_params - # permit (no expect) es intencional y load-bearing: expect usa - # on_unpermitted: :raise, así que un body con company_id daría 400 en - # vez de ignorarlo — rompiendo el requisito de la card. - # rubocop:disable-next Rails/StrongParametersExpect - params.require(:product).permit(:sku, :name, :description, :category, :weight, :dimensions) + product_body.permit(*PRODUCT_FIELDS) + end + + # El envoltorio, validado como objeto por `body_of` (TESIS-133). Antes esto + # era `params.require(:product)`, que devolvía el String tal cual y hacía + # reventar al `permit` de abajo con un 500. + def product_body + body_of(:product) end + # `stocks` queda afuera de `expect`: cada línea necesita sus propios 422 + # con mensaje, que `expect` no sabe dar. def stock_params - raw_stocks = params[:product][:stocks] + raw_stocks = product_body[:stocks] return [] if raw_stocks.nil? # Si stocks viene presente pero no es un array (ej. un objeto), es un diff --git a/app/controllers/api/v1/shipments_controller.rb b/app/controllers/api/v1/shipments_controller.rb index 8a4c841..6cba8a1 100644 --- a/app/controllers/api/v1/shipments_controller.rb +++ b/app/controllers/api/v1/shipments_controller.rb @@ -17,8 +17,6 @@ class ShipmentsController < ApplicationController rescue_from Shipments::DispatchResponseError, with: :render_bad_gateway rescue_from Shipments::InvalidShippingCostError, with: :render_bad_request rescue_from Integrations::AdapterExecutionError, with: :render_courier_failure - # El parámetro que falta es un 400 de contrato, no un 422 de negocio. - rescue_from ActionController::ParameterMissing, with: :render_bad_request include Paginatable diff --git a/app/controllers/api/v1/stock_transfers_controller.rb b/app/controllers/api/v1/stock_transfers_controller.rb index 3fdfc8b..c117669 100644 --- a/app/controllers/api/v1/stock_transfers_controller.rb +++ b/app/controllers/api/v1/stock_transfers_controller.rb @@ -77,12 +77,13 @@ def warehouse_for(key) Warehouse.find(transfer_params[key]) end + # `expect` y no `require` + `permit`, igual que en productos: un + # `stock_transfer` que no sea un objeto es 400 y no un 500 (TESIS-133). + # Un company_id en el body se sigue descartando en silencio. 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-next Rails/StrongParametersExpect - params.require(:stock_transfer) - .permit(:product_id, :origin_warehouse_id, :destination_warehouse_id, :quantity) + params.expect( + stock_transfer: %i[product_id origin_warehouse_id destination_warehouse_id quantity] + ) end def render_conflict(exception) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index be17628..a5fddb0 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -23,6 +23,12 @@ class MalformedParameterError < StandardError; end rescue_from Catalog::LockTimeoutError, with: :render_lock_conflict rescue_from ActiveRecord::CheckViolation, with: :render_constraint_violation rescue_from MalformedParameterError, with: :render_bad_request + # Lo que levanta `params.expect` cuando el body no trae el recurso o no es un + # objeto. Rails ya lo mapea a 400, pero con su propio cuerpo; acá se rescata + # para que el error viaje como {"error": "..."} igual que todos los demás + # (ADR-015). Vive en el padre porque los cuatro controllers que migraron a + # `expect` en TESIS-133 lo necesitan por igual. + rescue_from ActionController::ParameterMissing, with: :render_bad_request # Las acciones index usan policy_scope; el resto deben llamar authorize. # Si una acción futura olvida el authorize, falla en vez de pasar sin ruido. @@ -91,6 +97,23 @@ def scalar_param(name) raise MalformedParameterError, "#{name} must be a single value" end + # El envoltorio del body (`params[:product]`, `params[:order]`), validado como + # objeto y no vacío. `params.expect` hace esto mismo y además filtra, y es lo + # que usan los controllers cuyo body no tiene partes que se arman a mano. + # + # Acá no alcanza: `expect` considera faltante un filtrado que queda vacío, y + # un PUT que sólo manda `stocks` o `items` —las dos partes que se recorren + # línea por línea, cada una con sus propios 422— es un request válido que no + # puede terminar en 400. El chequeo de forma, que es lo que evitaba el 500, + # es el mismo: `permit` sobre un String levanta NoMethodError (TESIS-133). + def body_of(key) + body = params[key] + raise ActionController::ParameterMissing, key unless body.is_a?(ActionController::Parameters) + raise ActionController::ParameterMissing, key if body.empty? + + body + end + def render_bad_request(exception) render json: { error: exception.message }, status: :bad_request end diff --git a/spec/requests/api/v1/order_updates_spec.rb b/spec/requests/api/v1/order_updates_spec.rb index 9fe7fc4..089ad88 100644 --- a/spec/requests/api/v1/order_updates_spec.rb +++ b/spec/requests/api/v1/order_updates_spec.rb @@ -265,5 +265,22 @@ def foreign_order expect(response).to have_http_status(:bad_request) end + + # `expect` corta por la forma del envoltorio antes de tocar la orden + # (TESIS-133); antes de eso, `permit` sobre un String era un 500. + it 'returns 400 and changes nothing when order is not an object', :aggregate_failures do + put "/api/v1/orders/#{order.id}", params: { order: 'Otro' }, headers: headers, as: :json + + expect(response).to have_http_status(:bad_request) + expect(order.reload.customer_name).to eq('Juan Pérez') + end + + it 'returns 400 and changes nothing when order is a list', :aggregate_failures do + put "/api/v1/orders/#{order.id}", params: { order: [{ customer_name: 'Otro' }] }, + headers: headers, as: :json + + expect(response).to have_http_status(:bad_request) + expect(order.reload.customer_name).to eq('Juan Pérez') + end end end diff --git a/spec/requests/api/v1/orders_spec.rb b/spec/requests/api/v1/orders_spec.rb index 9acd2ca..63f3ddb 100644 --- a/spec/requests/api/v1/orders_spec.rb +++ b/spec/requests/api/v1/orders_spec.rb @@ -402,11 +402,24 @@ def order_of_another_company expect(response).to have_http_status(:unprocessable_content) end - it 'rejects when order is not an object' do - post '/api/v1/orders', - params: { order: 'not_an_object' }, - headers: headers, as: :json - expect(response).to have_http_status(:unprocessable_content) + # 400 y no 422: la forma del envoltorio es contrato, no negocio. Lo da + # `params.expect` desde TESIS-133; antes era un RecordNotSaved a mano. + it 'rejects when order is not an object with a 400', :aggregate_failures do + expect do + post '/api/v1/orders', params: { order: 'not_an_object' }, + headers: headers, as: :json + end.not_to change(Order, :count) + + expect(response).to have_http_status(:bad_request) + end + + it 'rejects when order is a list with a 400', :aggregate_failures do + expect do + post '/api/v1/orders', params: { order: [{ customer_name: 'X' }] }, + headers: headers, as: :json + end.not_to change(Order, :count) + + expect(response).to have_http_status(:bad_request) end it 'rejects when items is not an array' do diff --git a/spec/requests/api/v1/product_mappings_spec.rb b/spec/requests/api/v1/product_mappings_spec.rb index 0780d45..3baad6b 100644 --- a/spec/requests/api/v1/product_mappings_spec.rb +++ b/spec/requests/api/v1/product_mappings_spec.rb @@ -197,6 +197,26 @@ def post_mapping(product_id: product.id, **attrs) expect(response).to have_http_status(:not_found) end + # Mismo contrato que POST /products: un envoltorio que no es un objeto es + # 400 y no el 500 que daba `permit` sobre un String (TESIS-133). + it 'returns 400 when product_mapping is not an object', :aggregate_failures do + expect do + post mappings_url(product.id), params: { product_mapping: 'MLA-123' }, + headers: headers, as: :json + end.not_to change(ProductMapping, :count) + + expect(response).to have_http_status(:bad_request) + end + + it 'returns 400 when product_mapping is a list', :aggregate_failures do + expect do + post mappings_url(product.id), params: { product_mapping: [valid_params[:product_mapping]] }, + headers: headers, as: :json + end.not_to change(ProductMapping, :count) + + expect(response).to have_http_status(:bad_request) + end + it 'returns 409 when the external id is already linked in that integration', :aggregate_failures do link_external_id_to_another_product('MLA-123') diff --git a/spec/requests/api/v1/products_spec.rb b/spec/requests/api/v1/products_spec.rb index 2b6682c..2c1a67a 100644 --- a/spec/requests/api/v1/products_spec.rb +++ b/spec/requests/api/v1/products_spec.rb @@ -702,6 +702,29 @@ def stale post '/api/v1/products', params: { product: product_attrs }, headers: headers, as: :json expect(response).to have_http_status(:unprocessable_content) end + + # Hallazgo de la QA de TESIS-82, que acá seguía vivo: `require` devolvía el + # String tal cual y `permit` reventaba sobre él, con un 500 (TESIS-133). + it 'returns 400 when product is not an object', :aggregate_failures do + expect do + post '/api/v1/products', params: { product: 'Teclado' }, headers: headers, as: :json + end.not_to change(Product, :count) + + expect(response).to have_http_status(:bad_request) + end + + it 'returns 400 when product is a list', :aggregate_failures do + expect do + post '/api/v1/products', params: { product: [product_attrs] }, headers: headers, as: :json + end.not_to change(Product, :count) + + expect(response).to have_http_status(:bad_request) + end + + it 'returns 400 when the product key is missing' do + post '/api/v1/products', params: {}, headers: headers, as: :json + expect(response).to have_http_status(:bad_request) + end end describe 'PUT /api/v1/products/:id' do @@ -732,6 +755,22 @@ def stale expect(response).to have_http_status(:not_found) end + it 'returns 400 and changes nothing when product is not an object', :aggregate_failures do + put "/api/v1/products/#{product.id}", params: { product: 'Updated' }, + headers: headers, as: :json + + expect(response).to have_http_status(:bad_request) + expect(product.reload.name).to eq('Original') + end + + it 'returns 400 and changes nothing when product is a list', :aggregate_failures do + put "/api/v1/products/#{product.id}", params: { product: [{ name: 'Updated' }] }, + headers: headers, as: :json + + expect(response).to have_http_status(:bad_request) + expect(product.reload.name).to eq('Original') + end + it 'rejects stocks with a warehouse from another company' do params = { product: { stocks: stocks_for(other_warehouse.id, quantity: 5) } } put "/api/v1/products/#{product.id}", params: params, headers: headers, as: :json diff --git a/spec/requests/api/v1/stock_transfers_spec.rb b/spec/requests/api/v1/stock_transfers_spec.rb index 017dc05..a712028 100644 --- a/spec/requests/api/v1/stock_transfers_spec.rb +++ b/spec/requests/api/v1/stock_transfers_spec.rb @@ -69,6 +69,26 @@ def dispatch_one(quantity: 4) expect(response).to have_http_status(:unprocessable_content) end + # Mismo hallazgo que en warehouses y productos: `permit` sobre un String + # era un 500; con `expect` es un 400 de contrato (TESIS-133). + it 'returns 400 when stock_transfer is not an object', :aggregate_failures do + expect do + post '/api/v1/stock-transfers', params: { stock_transfer: 'PROD-1' }, + headers: headers, as: :json + end.not_to change(StockTransfer, :count) + + expect(response).to have_http_status(:bad_request) + end + + it 'returns 400 when stock_transfer is a list', :aggregate_failures do + expect do + post '/api/v1/stock-transfers', params: { stock_transfer: [body_for(4)[:stock_transfer]] }, + headers: headers, as: :json + end.not_to change(StockTransfer, :count) + + expect(response).to have_http_status(:bad_request) + 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 From 8a8d4a13e7759775be91b668684a6c5db890762b Mon Sep 17 00:00:00 2001 From: Tomas Martin Date: Tue, 29 Sep 2026 18:59:12 -0300 Subject: [PATCH 2/2] docs: [TESIS-133] say when the guard replaces expect in the sample controller The sample controller in CLAUDE.md is named after products_controller.rb, which is exactly the one this card does not move to `expect`. As written, the example invites the next reader to "finish the migration" and undo the reason it was left out. Reported by Santiago in the review of #93. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_012xAtddb53LRNVLuyTXyELp --- CLAUDE.md | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/CLAUDE.md b/CLAUDE.md index c292b1e..384e112 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -254,6 +254,13 @@ module Api # expect y no require + permit: require devuelve lo que haya bajo la # clave, y permit sobre un String levanta NoMethodError -> 500. expect # responde 400 a cualquier cosa que no sea un objeto (TESIS-133). + # + # Ojo con el ProductsController real, que NO usa expect: cuando el body + # trae una parte que se recorre a mano —las `stocks` de un producto, las + # `items` de una orden—, esa clave no entra en la lista, y expect toma + # como faltante un filtrado que queda vacío. Un PUT que sólo manda stocks + # terminaba en 400. Ahí el envoltorio se valida con `body_of` + # (ApplicationController) y el permit va sobre lo que devuelve. def product_params params.expect(product: %i[sku name stock]) end