feat: [TESIS-46] add the concurrent shipping rating engine - #60
feat: [TESIS-46] add the concurrent shipping rating engine#60TomasMartin2004 wants to merge 2 commits into
Conversation
Quotes a shipment against every active courier of the company at once and returns a normalized list to choose from. One courier failing does not sink the quote: each call is isolated and the operator that does not answer simply does not appear among the options. Two things the card left open, decided here. The origin warehouse travels in the request instead of being derived. Shipments do not record an origin, and the items of an order can sit in several warehouses, so any automatic rule would be invented. The design already has the user picking it — TESIS-58, "Wizard Orden Manual (Paso 2): Origen y Destino". The route is POST /api/v1/orders/:order_id/quotes. The card names a service but its acceptance criteria end at "ready for the user to pick an option", so an endpoint is needed. It hangs off the order because that is where the quote inputs live: destination from the customer, weight from the items. `orders` is declared with `only: []` — the CRUD is TESIS-42 and defines its own actions. Which template can quote is declared by the template. A courier has separate endpoints for quoting and for dispatching, and by this project's convention that means two Services (as with 'Mercado Libre' and 'Mercado Libre - Stock'). Rather than a new column, the one that quotes is the one mapping the cost in its response_mapper — the same data-driven principle as the rest of the integrations, and no migration per new capability. Asking a dispatch template for a rate would call the wrong endpoint of the provider; there is a spec for that. The adapter's timeouts become parameters. They were fixed at 10s, which is fine for a background sync but not for a quote running inside a request while the user waits. Quoting uses 4s, within the 3-5s the card asks for. Three fixes that came out of building this. Threads that run application code need Rails.application.executor.wrap. Without it the first thread to touch a not-yet-autoloaded constant deadlocks against Zeitwerk's load interlock — the process hangs rather than slows. ApplicationController named :index in `only:`/`except:` for the Pundit verification callbacks. Since Rails 7.1 that raises ActionNotFound when the controller does not define the action, and index is defined by the subclasses — so the first controller without one died with a misleading 404 before running its action. This is that controller. The condition now goes through a predicate: identical behaviour, no longer a trap for the next single-action controller, and the Rails/LexicallyScopedActionFilter silencing is no longer needed. It is a shared file, so it is worth a look in review. An empty list is a 200, not an error. "No operator answered in time" and "the quote failed" are different things and the frontend needs to tell them apart. Parallelism is not covered by a spec, and that is deliberate: WebMock is not thread safe, and three concurrent delayed responses hang the process instead of failing. It was measured against a real HTTP server instead — three couriers delayed 0.4s each, which sequentially would be 1.2s, resolved in 0.46s with all three quotes returned. The script is in the PR. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
LoLoo03
left a comment
LoLoo03
left a comment
There was a problem hiding this comment.
Resumen de la revisión — TESIS-46-shipping-rating-engine
El diseño central del PR está sólido: aislar cada courier con rescue dentro del hilo, resolver payload/integrations con ActiveRecord antes de abrir threads, envolver en Rails.application.executor.wrap, y el fix de ApplicationController (que sí es un bug real y reproducible en Rails 7.1+ con raise_on_missing_callback_actions = true, activo en config/environments/test.rb:52). No encontré bugs de concurrencia ni de multi-tenancy en el flujo feliz.
Los 5 hallazgos que sobrevivieron la verificación, de más a menos importante:
[Alta] Llamada HTTP externa síncrona dentro de un request de Puma (shipping_quotes_controller.rb:11) — contradice literalmente architecture.md §7.4 ("siempre ejecutado desde un job"). El diseño mitiga bastante el riesgo (timeout de 4s, aislamiento por hilo), pero bajo carga puede agotar el pool de threads de Puma. Vale documentar la excepción o discutirla, no necesariamente cambiarla.
[Media] Dominio equivocado: Shipping en vez de shipments (quote_shipping.rb:3) — feature-structure.md da como ejemplo textual app/poros/shipments/quote_shipment.rb para esta misma feature. Esta PR es la primera en tocar el dominio de envíos y arranca con el namespace incorrecto; TESIS-47 puede terminar fragmentando el dominio entre Shipping:: y Shipments::.
[Baja] quote: {} sin origin_warehouse_id da 404 en vez de 400 (shipping_quotes_controller.rb:36) — el spec sólo cubre params: {} (clave quote ausente), no el caso de clave presente pero vacía.
[Baja] OrderPolicy::Scope sin uso (order_policy.rb:14) — código anticipado para el ABM de TESIS-42, que CLAUDE.md pide evitar.
[Baja] Filtro de couriers en Ruby, no en SQL (quote_shipping.rb:48) — trae todas las integraciones activas (e-commerce incluido) antes de descartar por tipo.
No pude correr bundle exec rspec/rubocop/brakeman localmente (no hay Ruby nativo en esta máquina, ver docker-verification-setup.md en memoria) — las cifras del PR (586 examples, rubocop limpio, brakeman limpio) no están verificadas por mí.
Five findings from the review of PR #60. Four applied, one documented. The domain namespace was wrong. feature-structure.md lists app/poros/shipments/quote_shipment.rb — for this exact feature, by name — and this PR is the first to touch shipments, so starting on Shipping:: would have split the domain in two before TESIS-47 even lands. Renamed the PORO, its spec, the controller and the route to the documented vocabulary. A blank origin_warehouse_id returned 404. The reproduction in the review was `quote: {}`, which actually returns 400 — params.expect does raise on a missing key. The real trigger is a key present with an empty value: that reaches Warehouse.find('') and comes out as "not found", telling the client the warehouse does not exist when what is missing is the parameter. Now a blank value raises ParameterMissing like an absent one, and there is a spec for it. OrderPolicy::Scope was anticipated code: no action lists orders yet. Removed, with a note that TESIS-42 defines its own when the listing arrives. The courier filter moves the type check into SQL so the e-commerce integrations, which will never quote, are not loaded to be discarded in Ruby. The capability check stays in Ruby on purpose: it inspects the values of a jsonb mapper, and expressing that in SQL needs a jsonb_each_text inside an EXISTS — harder to read than what it saves over the handful of rows the type filter already leaves. The synchronous HTTP call is deliberate and now documented where the rule lives, in architecture.md §7.4, rather than left to be discovered in the code. The user is waiting for the rates to pick one, so returning them from a job would mean polling or a realtime channel for a value consumed on the spot. The section states what bounds the risk — a shorter timeout of its own, real concurrency so the ceiling is the slowest courier and not the sum, per-courier isolation — and, more useful, when the exception stops being justified and the call has to go back to a job. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Aplicados los cinco. Cuatro corregidos, uno documentado. Commit 🟠 [Media] Namespace del dominio — corregidoTenías razón y es más grave de lo que parece por ser el primer PR que toca el dominio. Renombrado en bloque, con
No queda ninguna referencia a 🔵 [Baja] 400 vs 404 — corregido, pero la reproducción no era esaEl hallazgo es real; el caso que lo dispara es otro. Lo medí antes de tocar nada: O sea que 🔵 [Baja]
|
2 participants
🔗 Ticket de Jira
TESIS-46 — desbloquea a TESIS-47
📝 Descripción
Cotiza un envío contra todos los operadores logísticos activos de la empresa en paralelo y devuelve una lista normalizada para que el usuario elija. El fallo de un courier no voltea la cotización: cada llamada se aísla y el operador que no contesta simplemente no aparece entre las opciones.
Es la diferencia entre "no pudimos cotizar" y "no pudimos cotizar con Andreani".
Dos definiciones que la card dejaba abiertas
El depósito de origen viaja en el request, no se deduce.
shipmentsno guarda origen y los ítems de una orden pueden estar en varios depósitos, así que cualquier regla automática sería una invención. Además el diseño ya tiene al usuario eligiéndolo: TESIS-58, "Wizard Orden Manual (Paso 2): Origen y Destino".La ruta es
POST /api/v1/orders/:order_id/quotes. La card nombra un servicio, pero su criterio de finalización termina en "listo para que el usuario pueda seleccionar una opción" — hace falta endpoint. Cuelga de la orden porque ahí viven los datos de entrada: el destino sale del cliente y el peso de los ítems.ordersse declara cononly: []; el ABM es TESIS-42 y define ahí sus acciones.Qué plantilla sabe cotizar lo declara la plantilla
Un courier tiene endpoints distintos para cotizar y para despachar, y por convención del proyecto eso son dos
Service(igual queMercado LibreyMercado Libre - Stock).En vez de sumar una columna, la que cotiza es la que mapea el costo en su
response_mapper. Es el mismo principio data-driven del resto de las integraciones —el template declara qué sabe contestar— y evita una migración por cada capacidad nueva. Pedirle una tarifa a una plantilla de despacho sería llamar al endpoint equivocado del proveedor; hay un spec para eso.🛠️ Cambios realizados
Shipping::QuoteShipping: busca las integraciones activas cotizables, arma el contexto una sola vez, dispara un hilo por operador y normaliza. Ordena por precio, así que la más barata queda primera.Service#courier?yService#quotes_shipping?.Integrations::HttpAdapter: timeouts por parámetro. Estaban fijos en 10s — está bien para un sync en background, no para una cotización que corre dentro de un request con el usuario esperando. Cotizar usa 4s, dentro de los 3-5 que pide la card.Api::V1::ShippingQuotesController+OrderPolicy, ruta anidada, y la plantilla de cotización de Andreani en los seeds.ApplicationController— ver abajo, es lo único que toca código compartido.ApplicationControllernombraba:indexen elonly:/except:de los callbacks de verificación de Pundit:Desde Rails 7.1 eso levanta
AbstractController::ActionNotFoundcuando el controller no define esa acción — yindexlo definen las subclases. Todos los controllers de hoy tienen uno, así que nadie lo había pisado: el primero que no lo tenga se cae con un 404 engañoso antes de ejecutar su acción. Este es ese controller, y me costó tres iteraciones encontrarlo justamente porque el síntoma es un 404 y no un error de callback.Ahora la condición va por predicado:
Comportamiento idéntico, deja de ser una trampa para el próximo controller de acción única, y de paso ya no hace falta silenciar
Rails/LexicallyScopedActionFilter.Un detalle de concurrencia que no se ve
Los hilos van envueltos en
Rails.application.executor.wrap. Un hilo que corre código de la aplicación tiene que declararlo: es lo que permite cargar constantes todavía no autocargadas desde fuera del hilo principal. Sin eso, el primer hilo que toca una constante nueva deadlockea contra el load interlock de Zeitwerk y el proceso queda colgado, no lento.Y el payload y las integraciones se resuelven antes de abrir los hilos, a propósito: adentro no puede haber ActiveRecord. Tomarían otra conexión del pool —que en test no ve la transacción del ejemplo— y
Current.company_idno cruza el límite del hilo, así que eldefault_scopedeCompanyScopedquedaría sin tenant.🧪 Cómo probar
bin/rails db:seed→ crea la plantillaAndreani - Cotizacióny su integración para Distribuidora NortePOST /api/v1/orders/:order_id/quotescon{ "quote": { "origin_warehouse_id": N } }200con[{ company_integration_id, provider_name, shipping_cost, estimated_days }]200condata: [], no un 500order_ido unorigin_warehouse_idde otra empresa →404📸 Evidencia
N/A (backend).
bundle exec rspec→ 586 examples, 0 failures (18 nuevos)rubocop --force-exclusion→ 147 archivos, sin ofensasbrakeman -q→ No warnings foundEl criterio de paralelismo, medido
No hay spec de paralelismo, y es deliberado: WebMock no es thread-safe y tres respuestas concurrentes con retardo cuelgan el proceso en vez de fallar — lo comprobé. Un spec que se cuelga es peor que no tenerlo.
Se midió contra un servidor HTTP real (
TCPServer, datos creados y rolleados en una transacción):El tiempo total es el del courier más lento, no la suma — que es exactamente el criterio de finalización de la card. El script queda en el comentario de abajo por si lo quieren correr.
ApplicationControllerpreserva el comportamiento.