Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions app/controllers/api/v1/failed_events_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
13 changes: 11 additions & 2 deletions app/controllers/api/v1/shipments_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
10 changes: 8 additions & 2 deletions app/controllers/api/v1/stock_transfers_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
33 changes: 33 additions & 0 deletions spec/requests/api/v1/failed_events_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
56 changes: 56 additions & 0 deletions spec/requests/api/v1/shipments_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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'),
Expand Down
28 changes: 28 additions & 0 deletions spec/requests/api/v1/stock_transfers_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading