From 03dab3baa746aaa2bb6b0b974389594b4cec30df Mon Sep 17 00:00:00 2001 From: Tomas Martin Date: Tue, 22 Sep 2026 23:30:23 -0300 Subject: [PATCH 1/2] fix: [TESIS-124] read the shipments listing filters as single values MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `shipments#index` was the last listing reading `params[...]` straight. `?page[]=1` and `?per_page[]=1` answered 500, because `Array#to_i` does not exist, and `?status[foo]=bar` did too, from putting an `ActionController::Parameters` inside a `where`. `?order_id[]=1` was the one worth finding: it did not fail. `where` translated the array into an `IN`, so the listing filtered by several orders at once and answered 200 — an undeclared capability hidden behind a successful response. The card expected a 500 there; it was quieter than that. The unknown-versus-malformed distinction stays: `?status=inventado` still answers an empty list, which is the honest answer for a filter that matches nothing, and only a malformed shape is a 400. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_012xAtddb53LRNVLuyTXyELp --- .../api/v1/shipments_controller.rb | 20 +++++-- spec/requests/api/v1/shipments_spec.rb | 56 +++++++++++++++++++ 2 files changed, 72 insertions(+), 4 deletions(-) diff --git a/app/controllers/api/v1/shipments_controller.rb b/app/controllers/api/v1/shipments_controller.rb index 17b36b1..8898095 100644 --- a/app/controllers/api/v1/shipments_controller.rb +++ b/app/controllers/api/v1/shipments_controller.rb @@ -23,8 +23,11 @@ class ShipmentsController < ApplicationController COURIER_ERROR_LIMIT = 300 def index - page = [params[:page].to_i, 1].max - per_page = params.fetch(:per_page, 20).to_i.clamp(1, 100) + # `scalar_param` y no `params[...]` directo: una query con `?page[]=1` + # entrega un Array, y `Array#to_i` no existe — el listado moría con un + # 500. Mismo criterio que orders#index y products#index (TESIS-124). + page = [scalar_param(:page).to_i, 1].max + per_page = (scalar_param(:per_page) || 20).to_i.clamp(1, 100) # La precarga es load-bearing: ShipmentListSerializer lee el nombre del # courier a través de la plantilla del Service, y sin ella son dos @@ -82,10 +85,19 @@ def confirm # Un status desconocido no se filtra ni se rechaza: `where` lo busca igual # y devuelve la lista vacía, que es la respuesta honesta para un filtro que # no matchea nada (status es un string plano, no un enum: no rompe). + # + # Desconocido no es lo mismo que mal formado, y por eso los dos filtros + # pasan por `scalar_param`. `?status[foo]=bar` reventaba con un TypeError; + # `?order_id[]=1` era peor porque NO reventaba: `where` recibía el Array y + # lo traducía a un `IN`, así que la query filtraba por varias órdenes a la + # vez —una capacidad que nadie declaró ni documentó— y el 200 lo tapaba. def filtered_shipments + status = scalar_param(:status) + order_id = scalar_param(:order_id) + shipments = policy_scope(Shipment) - shipments = shipments.where(status: params[:status]) if params[:status].present? - shipments = shipments.where(order_id: params[:order_id]) if params[:order_id].present? + shipments = shipments.where(status: status) if status.present? + shipments = shipments.where(order_id: order_id) if order_id.present? shipments end diff --git a/spec/requests/api/v1/shipments_spec.rb b/spec/requests/api/v1/shipments_spec.rb index fa1ea9e..c9621b2 100644 --- a/spec/requests/api/v1/shipments_spec.rb +++ b/spec/requests/api/v1/shipments_spec.rb @@ -51,6 +51,62 @@ def foreign_shipment expect(response).to have_http_status(:unauthorized) end + # Una query mal armada es un error del cliente, no del servidor (TESIS-124). + # Antes `?page[]=1` y `?per_page[]=1` salían 500 —`Array#to_i` no existe— y + # `?status[foo]=bar` también, por el TypeError de meter un + # ActionController::Parameters en un `where`. + # + # `?order_id[]=1` era el más engañoso porque NO fallaba: `where` traducía el + # Array a un `IN`, así que el listado filtraba por varias órdenes a la vez y + # contestaba 200. Una capacidad que nadie declaró, escondida detrás de una + # respuesta exitosa. + # + # Se responde 400 y no «se ignora el filtro»: descartarlo en silencio + # devolvería el listado entero, que es una respuesta plausible y equivocada. + context 'when a query parameter is malformed' do + it 'returns 400 for a page that is not a single value' do + get '/api/v1/shipments', params: { page: ['2'] }, headers: headers + + expect(response).to have_http_status(:bad_request) + end + + it 'returns 400 for a per_page that is not a single value' do + get '/api/v1/shipments', params: { per_page: ['1'] }, headers: headers + + expect(response).to have_http_status(:bad_request) + end + + it 'returns 400 for a status that is not a single value' do + get '/api/v1/shipments', params: { status: { foo: 'bar' } }, headers: headers + + expect(response).to have_http_status(:bad_request) + end + + it 'returns 400 for an order_id that is not a single value' do + get '/api/v1/shipments', params: { order_id: %w[1 2] }, headers: headers + + expect(response).to have_http_status(:bad_request) + end + + it 'says which parameter is wrong' do + get '/api/v1/shipments', params: { order_id: %w[1 2] }, headers: headers + + expect(response.parsed_body['error']).to include('order_id') + end + end + + # La distinción que la card pide conservar: mal formado es 400, pero un valor + # desconocido se filtra igual y devuelve la lista vacía, que es la respuesta + # honesta para un filtro que no matchea nada. + it 'answers an empty list for a status that simply does not exist', :aggregate_failures do + shipment_for('Juan') + + get '/api/v1/shipments', params: { status: 'inventado' }, headers: headers + + expect(response).to have_http_status(:ok) + expect(response.parsed_body['data']).to be_empty + end + context 'when authenticated' do before do shipment_for('Ana', status: 'in_transit', integration: courier('Andreani'), From d7c9d30f3d0e4288ea23bd0a4dead035bf9acba6 Mon Sep 17 00:00:00 2001 From: Tomas Martin Date: Thu, 24 Sep 2026 21:41:01 -0300 Subject: [PATCH 2/2] fix: [TESIS-124] read the filters of failed events and transfers too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The review probed the listings the card did not name and found the same two symptoms this PR fixes for shipments: `?event_type[]=x` and `?status[]=x` answer 200 while filtering with an IN, and a hash-shaped filter raises TypeError as a 500. The card's criterion is general — no listing of api/v1 answers 500 to a malformed query parameter — so `failed_events#index` and `stock_transfers#index` now read their filters with `scalar_param`, with a 400 spec per parameter. The status of failed events was not crashing, since it is validated against the enum, but a list-shaped status was silently ignored and the whole listing came back as if no filter had been sent. `page` and `per_page` of failed events stay untouched here: TESIS-108 moves them into the Paginatable concern, which already uses `scalar_param`. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_012xAtddb53LRNVLuyTXyELp --- .../api/v1/failed_events_controller.rb | 11 +++++-- .../api/v1/stock_transfers_controller.rb | 9 +++-- spec/requests/api/v1/failed_events_spec.rb | 33 +++++++++++++++++++ spec/requests/api/v1/stock_transfers_spec.rb | 28 ++++++++++++++++ 4 files changed, 76 insertions(+), 5 deletions(-) diff --git a/app/controllers/api/v1/failed_events_controller.rb b/app/controllers/api/v1/failed_events_controller.rb index 93d113c..ecf7327 100644 --- a/app/controllers/api/v1/failed_events_controller.rb +++ b/app/controllers/api/v1/failed_events_controller.rb @@ -34,15 +34,20 @@ def discard private + # `scalar_param` y no `params[...]`: `?event_type[]=x` entrega un Array, y + # `where` lo traduce a un `IN` —filtra por otra cosa que lo pedido, con un + # 200 que no delata nada— mientras que `?event_type[foo]=x` levanta + # TypeError y sale como 500. Mismo criterio que envíos y órdenes. def filtered_events events = policy_scope(FailedEvent) - events = events.where(status: params[:status]) if valid_status? - events = events.where(event_type: params[:event_type]) if params[:event_type].present? + events = events.where(status: scalar_param(:status)) if valid_status? + event_type = scalar_param(:event_type) + events = events.where(event_type: event_type) if event_type.present? events end def valid_status? - FailedEvent.statuses.key?(params[:status]) + FailedEvent.statuses.key?(scalar_param(:status)) end def set_failed_event diff --git a/app/controllers/api/v1/stock_transfers_controller.rb b/app/controllers/api/v1/stock_transfers_controller.rb index b1b21f1..08d0e36 100644 --- a/app/controllers/api/v1/stock_transfers_controller.rb +++ b/app/controllers/api/v1/stock_transfers_controller.rb @@ -11,8 +11,13 @@ 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? + # `scalar_param` y no `params[...]`: un `?status[]=received` entrega un + # Array que `where` convierte en un `IN`, y un `?product_id[foo]=1` + # levanta TypeError → 500. Mismo criterio que envíos y órdenes. + status = scalar_param(:status) + product_id = scalar_param(:product_id) + transfers = transfers.where(status: status) if status.present? + transfers = transfers.where(product_id: product_id) if product_id.present? render json: { data: StockTransferSerializer.render_as_hash(transfers) } end diff --git a/spec/requests/api/v1/failed_events_spec.rb b/spec/requests/api/v1/failed_events_spec.rb index d61dc40..e4d6f5a 100644 --- a/spec/requests/api/v1/failed_events_spec.rb +++ b/spec/requests/api/v1/failed_events_spec.rb @@ -45,6 +45,39 @@ expect(response.parsed_body['meta']).to eq('page' => 1, 'per_page' => 2, 'total' => 4) end + # Un filtro con forma de Array o de Hash es un error de contrato del + # cliente, no del servidor: `where` lo traduciría a un `IN` —200 filtrando + # por otra cosa— o levantaría TypeError → 500 (TESIS-124). + context 'when a filter is not a single value' do + it 'returns 400 for an event type sent as a list' do + get '/api/v1/failed-events', params: { event_type: ['integrations.http_request'] }, + headers: headers + + expect(response).to have_http_status(:bad_request) + end + + it 'says which parameter is wrong' do + get '/api/v1/failed-events', params: { event_type: ['x'] }, headers: headers + + expect(response.parsed_body['error']).to include('event_type') + end + + it 'returns 400 for a status sent as a hash instead of ignoring it' do + get '/api/v1/failed-events', params: { status: { foo: 'dead' } }, headers: headers + + expect(response).to have_http_status(:bad_request) + end + + it 'does not answer 200 with the wrong rows' do + create_event(company, event_type: 'orders.ingestion') + + get '/api/v1/failed-events', params: { event_type: ['integrations.http_request'] }, + headers: headers + + expect(response.parsed_body).not_to have_key('data') + end + end + it 'ignores an unknown status filter' do get '/api/v1/failed-events', params: { status: 'exploded' }, headers: headers diff --git a/spec/requests/api/v1/stock_transfers_spec.rb b/spec/requests/api/v1/stock_transfers_spec.rb index 8fb2427..9043c9f 100644 --- a/spec/requests/api/v1/stock_transfers_spec.rb +++ b/spec/requests/api/v1/stock_transfers_spec.rb @@ -93,6 +93,34 @@ def dispatch_one(quantity: 4) end end + # Los dos filtros del listado, con una forma que el endpoint no espera: 400 y + # no un `IN` silencioso ni un 500 (TESIS-124). + describe 'GET /api/v1/stock-transfers with a malformed filter' do + it 'returns 400 for a status sent as a list' do + get '/api/v1/stock-transfers', params: { status: ['in_transit'] }, headers: headers + + expect(response).to have_http_status(:bad_request) + end + + it 'returns 400 for a product id sent as a list' do + get '/api/v1/stock-transfers', params: { product_id: [1, 2] }, headers: headers + + expect(response).to have_http_status(:bad_request) + end + + it 'returns 400 for a status sent as a hash' do + get '/api/v1/stock-transfers', params: { status: { foo: 'bar' } }, headers: headers + + expect(response).to have_http_status(:bad_request) + end + + it 'says which parameter is wrong' do + get '/api/v1/stock-transfers', params: { product_id: [1] }, headers: headers + + expect(response.parsed_body['error']).to include('product_id') + end + end + describe 'POST /api/v1/stock-transfers/:id/receive' do it 'settles the transfer into the destination', :aggregate_failures do transfer = dispatch_one