diff --git a/app/controllers/api/v1/failed_events_controller.rb b/app/controllers/api/v1/failed_events_controller.rb index 20769bd..433079a 100644 --- a/app/controllers/api/v1/failed_events_controller.rb +++ b/app/controllers/api/v1/failed_events_controller.rb @@ -28,15 +28,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/shipments_controller.rb b/app/controllers/api/v1/shipments_controller.rb index 009402a..e6a7f5e 100644 --- a/app/controllers/api/v1/shipments_controller.rb +++ b/app/controllers/api/v1/shipments_controller.rb @@ -79,10 +79,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/app/controllers/api/v1/stock_transfers_controller.rb b/app/controllers/api/v1/stock_transfers_controller.rb index 28d4dc5..3fdfc8b 100644 --- a/app/controllers/api/v1/stock_transfers_controller.rb +++ b/app/controllers/api/v1/stock_transfers_controller.rb @@ -44,8 +44,14 @@ def filtered_transfers 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 + # (TESIS-124). + 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? transfers end diff --git a/spec/requests/api/v1/failed_events_spec.rb b/spec/requests/api/v1/failed_events_spec.rb index 6a97738..5347df9 100644 --- a/spec/requests/api/v1/failed_events_spec.rb +++ b/spec/requests/api/v1/failed_events_spec.rb @@ -68,6 +68,39 @@ expect(response.parsed_body['data'].pluck('id')).to contain_exactly(dead.id) 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/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'), diff --git a/spec/requests/api/v1/stock_transfers_spec.rb b/spec/requests/api/v1/stock_transfers_spec.rb index a5870cd..017dc05 100644 --- a/spec/requests/api/v1/stock_transfers_spec.rb +++ b/spec/requests/api/v1/stock_transfers_spec.rb @@ -124,6 +124,34 @@ def dispatch_another_product 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