From 2709db0ee7c2444f920933b41da2ae19952a18ae Mon Sep 17 00:00:00 2001 From: Tomas Martin Date: Wed, 23 Sep 2026 18:09:41 -0300 Subject: [PATCH 1/4] feat: [TESIS-107] settle the response convention and wrap the last bare list MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The API answered in four shapes because every card picked its own and nobody wrote the rule down. The frontend already carried the symptom as a comment: "index wraps in { data }, but show and update return the object bare". A note like that is the tell that no rule exists. The rule is now one line, in ADR-015: a collection travels wrapped, a single resource travels bare, an error is always `{ "error": ... }`. Only `integrations#index` had to change — it returned an array at the root. That one mattered beyond consistency: an array at the root has nowhere to put `meta`, so that endpoint could never start paginating without breaking whoever reads it. TESIS-108 needs every collection to have that room. Wrapping single resources too was the tidier rule, and it is written down in the ADR as the alternative with the reason it was not taken: ten call sites across four frontier files in the frontend, two of which TESIS-58 and TESIS-61 are editing in live branches right now. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_012xAtddb53LRNVLuyTXyELp --- .../api/v1/integrations_controller.rb | 12 +++- ...R-015-convencion-de-respuesta-de-la-api.md | 71 +++++++++++++++++++ docs/guidelines/architecture.md | 16 +++++ spec/requests/api/v1/api_contract_spec.rb | 20 +++--- spec/requests/api/v1/integrations_spec.rb | 18 ++++- 5 files changed, 123 insertions(+), 14 deletions(-) create mode 100644 docs/adr/ADR-015-convencion-de-respuesta-de-la-api.md diff --git a/app/controllers/api/v1/integrations_controller.rb b/app/controllers/api/v1/integrations_controller.rb index 8b84bd4..292245e 100644 --- a/app/controllers/api/v1/integrations_controller.rb +++ b/app/controllers/api/v1/integrations_controller.rb @@ -5,11 +5,17 @@ module V1 class IntegrationsController < ApplicationController skip_after_action :verify_authorized, :verify_policy_scoped + # Envuelto en `data` como el resto de las colecciones (ADR-015). Era el + # único listado que devolvía un array pelado, y un array en la raíz no + # deja lugar para agregarle `meta` el día que pagine sin romper a quien + # lo consume. def index integrations = current_company.company_integrations.index_by(&:service_id) - render json: IntegrationStatusSerializer.render( - Service.order(:id), integrations_by_service_id: integrations - ) + render json: { + data: IntegrationStatusSerializer.render_as_hash( + Service.order(:id), integrations_by_service_id: integrations + ) + } end def update diff --git a/docs/adr/ADR-015-convencion-de-respuesta-de-la-api.md b/docs/adr/ADR-015-convencion-de-respuesta-de-la-api.md new file mode 100644 index 0000000..d380385 --- /dev/null +++ b/docs/adr/ADR-015-convencion-de-respuesta-de-la-api.md @@ -0,0 +1,71 @@ +# ADR-015: Convención de respuesta de la API + +**Fecha:** 2026-09-23 +**Estado:** Aceptado + +--- + +## Contexto + +La API creció una card por vez y cada una eligió cómo devolver su respuesta. Nadie escribió la regla, así que no había una: había cuatro formas conviviendo. + +| Forma | Endpoints | +| --------------------------- | -------------------------------------------------------------------- | +| `{ "data": [...], "meta" }` | `GET /products`, `/orders`, `/shipments`, `/failed-events` | +| `{ "data": [...] }` | `/warehouses`, `/stock-transfers`, `/products/:id/mappings`, `/quotes` | +| `[...]` — array pelado | `GET /integrations` | +| objeto pelado | todos los `show`, `create` y `update` | + +El costo no lo paga el backend, lo paga el consumidor. El frontend ya lo tenía anotado como trampa en `src/features/inventory/api.ts`: «`index` envuelve en `{ data: [...] }`, pero `show` y `update` devuelven el objeto pelado». Un comentario así es la señal de que la regla no existe: si existiera, no haría falta recordarla por endpoint. + +Mientras hubo una sola pantalla consumiendo la API la inconsistencia era barata. Con el panel, el catálogo, órdenes, envíos y reportes leyendo de acá, cada pantalla nueva tiene que redescubrir de qué forma contesta cada endpoint. + +## Decisión + +**Una colección viaja envuelta. Un recurso solo viaja pelado.** + +``` +GET /api/v1/products → { "data": [ {...}, {...} ], "meta": { page, per_page, total } } +GET /api/v1/warehouses → { "data": [ {...}, {...} ] } +GET /api/v1/integrations → { "data": [ {...}, {...} ] } + +GET /api/v1/products/:id → { "id": 1, "sku": "...", ... } +POST /api/v1/products → { "id": 1, "sku": "...", ... } +PUT /api/v1/products/:id → { "id": 1, "sku": "...", ... } + +cualquier error → { "error": "..." } +``` + +`meta` aparece sólo si el listado pagina, y es siempre `page`, `per_page` y `total`, contando el scope **ya filtrado**. + +El único endpoint que hubo que cambiar fue `integrations#index`, que devolvía un array en la raíz. + +## Alternativas consideradas + +**Envolver también los recursos solos** (`{ "data": { ... } }` en `show`, `create` y `update`). Es la regla más simple de enunciar —una sola, sin excepciones— y deja lugar para agregarle `meta` a un recurso individual el día que haga falta. + +Se descartó por lo que costaba **ahora**, no por lo que vale: son diez lugares del frontend, en cuatro archivos de frontera, y dos de esos archivos son los que TESIS-58 y TESIS-61 están editando en ramas vivas. Cambiar el contrato debajo de dos PRs abiertos, para ganar uniformidad en endpoints que hoy nadie confunde, no se paga. + +La puerta queda abierta: pasar de esta convención a la otra es aditivo del lado del backend —envolver lo que hoy va pelado— y el día que se haga, este ADR se reemplaza en vez de discutirse de nuevo. + +**Dejar la inconsistencia documentada** en vez de corregirla. Se descartó porque un array en la raíz no es sólo una forma distinta: no admite agregarle `meta` sin romper a quien lo consume. Si `integrations` pagina algún día —y TESIS-108 lo evalúa— habría que romperlo igual, con más consumidores encima. + +## Consecuencias + +**A favor** + +- La regla se enuncia en una línea y no tiene excepciones que justificar. +- Ninguna colección queda con un array en la raíz, así que cualquiera puede empezar a paginar sin romper su contrato. Es la precondición de TESIS-108. +- El comentario-trampa del frontend se borra: lo que explicaba ya no pasa. +- `spec/requests/api/v1/api_contract_spec.rb` (TESIS-90) fija las tres formas, así que un endpoint nuevo que invente una cuarta rompe la suite. + +**En contra** + +- El consumidor sigue teniendo que saber si lo que pidió es una colección o un recurso. No es gratis, pero es una distinción que ya existe en la URL: `/products` contra `/products/:id`. +- `GET /api/v1/integrations` cambia de forma. Es un cambio que rompe, y va con su lado del frontend en el mismo momento. + +## Referencias + +- TESIS-107 — la card que pedía definir la convención +- TESIS-108 — paginación consistente, que necesita que ninguna colección esté pelada +- TESIS-90 — `api_contract_spec.rb`, donde la regla queda fijada de forma ejecutable diff --git a/docs/guidelines/architecture.md b/docs/guidelines/architecture.md index cddf155..644497b 100644 --- a/docs/guidelines/architecture.md +++ b/docs/guidelines/architecture.md @@ -133,6 +133,22 @@ Serializer (Blueprinter) render json: ... ``` +### 4.0 Forma de la respuesta + +Una sola regla, fijada en [ADR-015](../adr/ADR-015-convencion-de-respuesta-de-la-api.md): **una colección viaja envuelta, un recurso solo viaja pelado.** + +``` +GET /api/v1/products → { "data": [ ... ], "meta": { page, per_page, total } } +GET /api/v1/warehouses → { "data": [ ... ] } +GET /api/v1/products/:id → { "id": 1, "sku": "...", ... } +POST /api/v1/products → { "id": 1, "sku": "...", ... } +cualquier error → { "error": "..." } +``` + +`meta` aparece sólo si el listado pagina, y cuenta el scope **ya filtrado**, no la tabla entera. + +Ninguna colección devuelve un array en la raíz: un array pelado no admite agregarle `meta` sin romper a quien lo consume. `spec/requests/api/v1/api_contract_spec.rb` fija las tres formas, así que un endpoint nuevo que invente una cuarta rompe la suite. + ### 4.1 Flujo de un webhook entrante Los eventos de las plataformas externas no siguen el flujo de arriba: no hay JWT, no hay usuario y el proveedor no espera detrás de la lógica de negocio. El gateway persiste y suelta la conexión; el procesamiento corre en un worker (ver [ADR-010](../adr/ADR-010-ingesta-de-ordenes-de-webhooks.md)). diff --git a/spec/requests/api/v1/api_contract_spec.rb b/spec/requests/api/v1/api_contract_spec.rb index e032d83..17838ce 100644 --- a/spec/requests/api/v1/api_contract_spec.rb +++ b/spec/requests/api/v1/api_contract_spec.rb @@ -104,13 +104,13 @@ def shipment # ───────────────────────────────────────────────────────── forma del sobre # - # Hoy la API responde con CUATRO formas distintas. No es una decisión: cada - # card eligió la suya y nadie la escribió. TESIS-107 las unifica. + # La regla la fija ADR-015 (TESIS-107) y es una sola: **una colección viaja + # envuelta en `data`** —más `meta` si pagina— y **un recurso solo viaja + # pelado**. Los errores, siempre `{ "error": "..." }`. # - # Se fijan igual, y a propósito: mientras la inconsistencia exista, el front - # tiene que saber cuál le toca a cada endpoint, y este archivo es el único - # lugar donde eso está dicho. Cuando entre TESIS-107, estos cuatro ejemplos - # son la lista de lo que hay que cambiar. + # Antes eran cuatro formas distintas, porque cada card eligió la suya y nadie + # la escribió. Estos ejemplos son lo que impide que vuelva a pasar: agregar + # un endpoint con otra forma tiene que romper acá. describe 'the shape of the envelope, endpoint by endpoint' do it 'wraps a paginated collection in data plus meta', :aggregate_failures do product @@ -136,11 +136,13 @@ def shipment expect(response.parsed_body.keys).to include('sku') end - # El único endpoint que devuelve un array pelado, sin objeto que lo envuelva. - it 'returns a bare array for the integrations listing' do + # Era el único que devolvía un array pelado. Un array en la raíz no admite + # `meta` sin romper a quien lo consume, así que ninguna colección puede + # quedar así. + it 'wraps the integrations listing too, with no exception' do get '/api/v1/integrations', headers: headers - expect(response.parsed_body).to be_an(Array) + expect(response.parsed_body.keys).to eq(['data']) end # Los errores sí son consistentes en toda la API, y conviene que siga así. diff --git a/spec/requests/api/v1/integrations_spec.rb b/spec/requests/api/v1/integrations_spec.rb index 0bd1ae2..7dc1c7c 100644 --- a/spec/requests/api/v1/integrations_spec.rb +++ b/spec/requests/api/v1/integrations_spec.rb @@ -11,12 +11,26 @@ uri: 'https://api.mercadolibre.com', http_method: 'GET') end + # El listado viaja envuelto en `data`, como todas las colecciones (ADR-015). + def listed + response.parsed_body['data'] + end + describe 'GET /api/v1/integrations' do it 'returns 401 without a token' do get '/api/v1/integrations' expect(response).to have_http_status(:unauthorized) end + # Era el único listado que contestaba un array pelado. Un array en la raíz + # no deja lugar para `meta` sin romper a quien lo consume, y obligaba al + # front a recordar que éste es la excepción (ADR-015, TESIS-107). + it 'wraps the collection in data, like every other listing' do + get '/api/v1/integrations', headers: headers + + expect(response.parsed_body.keys).to eq(['data']) + end + context 'when the company has the service configured' do before do CompanyIntegration.create!(company: company, service: service, @@ -25,7 +39,7 @@ end it 'marks the service as configured and active', :aggregate_failures do - row = response.parsed_body.find { |r| r['service_id'] == service.id } + row = listed.find { |r| r['service_id'] == service.id } expect(row['configured']).to be(true) expect(row['is_active']).to be(true) end @@ -45,7 +59,7 @@ let(:other_company) { Company.create!(name: 'Tenant B', tax_id: '30-22222222-2') } it 'shows the service as not configured for the current tenant', :aggregate_failures do - row = response.parsed_body.find { |r| r['service_id'] == service.id } + row = listed.find { |r| r['service_id'] == service.id } expect(row['configured']).to be(false) expect(row['is_active']).to be(false) end From 4a57fd2e1857ee383aecb4c843cd886bb1df8736 Mon Sep 17 00:00:00 2001 From: Tomas Martin Date: Wed, 23 Sep 2026 20:23:46 -0300 Subject: [PATCH 2/4] feat: [TESIS-108] paginate every listing from one place MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The page calculation was copied verbatim in four controllers and missing in three others, which answered with the whole table. `Api::V1::Paginatable` now owns both halves: how many rows come back and how `meta` is built. `warehouses`, `mappings` and `integrations` start paginating. Those three are read whole by the frontend — they fill selects, not tables with a pager — so they take a default of 100 rather than the 20 of a paged screen. The ceiling is the same for everyone, which is the point of the card: no listing can answer an unbounded number of rows. A company past the ceiling finds out through `meta.total`, which is better than a default of 20 hiding warehouses in silence. `integrations` paginates too. It is a global table with few rows and leaving it out was defensible, but an exception has to be justified every time someone reads it, and the rule is worth more than the saving. With this, every collection carries `meta`, so the envelope rule from ADR-015 loses its "only if it paginates" clause. The ADR and the guideline say the simpler thing now. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_012xAtddb53LRNVLuyTXyELp --- .../api/v1/failed_events_controller.rb | 14 +- .../api/v1/integrations_controller.rb | 16 +- .../api/v1/product_mappings_controller.rb | 23 +-- app/controllers/api/v1/products_controller.rb | 23 ++- .../api/v1/warehouses_controller.rb | 16 +- .../concerns/api/v1/paginatable.rb | 67 ++++++++ ...R-015-convencion-de-respuesta-de-la-api.md | 8 +- docs/guidelines/architecture.md | 8 +- spec/requests/api/v1/api_contract_spec.rb | 21 ++- spec/requests/api/v1/integrations_spec.rb | 4 +- spec/requests/api/v1/pagination_spec.rb | 147 ++++++++++++++++++ spec/requests/api/v1/product_mappings_spec.rb | 4 +- 12 files changed, 295 insertions(+), 56 deletions(-) create mode 100644 app/controllers/concerns/api/v1/paginatable.rb create mode 100644 spec/requests/api/v1/pagination_spec.rb diff --git a/app/controllers/api/v1/failed_events_controller.rb b/app/controllers/api/v1/failed_events_controller.rb index 93d113c..20769bd 100644 --- a/app/controllers/api/v1/failed_events_controller.rb +++ b/app/controllers/api/v1/failed_events_controller.rb @@ -3,20 +3,14 @@ module Api module V1 class FailedEventsController < ApplicationController + include Paginatable + before_action :set_failed_event, only: %i[requeue discard] def index - page = [params[:page].to_i, 1].max - per_page = params.fetch(:per_page, 20).to_i.clamp(1, 100) - - events = filtered_events.order(created_at: :desc) - .offset((page - 1) * per_page) - .limit(per_page) + events, meta = paginate(filtered_events.order(created_at: :desc)) - render json: { - data: FailedEventSerializer.render_as_hash(events), - meta: { page: page, per_page: per_page, total: filtered_events.count } - } + render json: { data: FailedEventSerializer.render_as_hash(events), meta: meta } end # POST /api/v1/failed-events/:id/retry (`retry` es palabra reservada en Ruby) diff --git a/app/controllers/api/v1/integrations_controller.rb b/app/controllers/api/v1/integrations_controller.rb index 292245e..dc26392 100644 --- a/app/controllers/api/v1/integrations_controller.rb +++ b/app/controllers/api/v1/integrations_controller.rb @@ -3,18 +3,30 @@ module Api module V1 class IntegrationsController < ApplicationController + include Paginatable + skip_after_action :verify_authorized, :verify_policy_scoped # Envuelto en `data` como el resto de las colecciones (ADR-015). Era el # único listado que devolvía un array pelado, y un array en la raíz no # deja lugar para agregarle `meta` el día que pagine sin romper a quien # lo consume. + # Pagina como todo listado (TESIS-108), aunque hoy `services` tenga pocas + # filas: es una tabla global que sólo crece cuando el administrador carga + # una plantilla nueva. Dejarla afuera sería una excepción que habría que + # justificar, y la regla vale más que el ahorro. + # + # `WHOLE_LIST_PER_PAGE` porque el panel la lee entera para dibujar sus + # nodos, no de a páginas. def index integrations = current_company.company_integrations.index_by(&:service_id) + services, meta = paginate(Service.order(:id), per_page: WHOLE_LIST_PER_PAGE) + render json: { data: IntegrationStatusSerializer.render_as_hash( - Service.order(:id), integrations_by_service_id: integrations - ) + services, integrations_by_service_id: integrations + ), + meta: meta } end diff --git a/app/controllers/api/v1/product_mappings_controller.rb b/app/controllers/api/v1/product_mappings_controller.rb index 63c404d..d4a572c 100644 --- a/app/controllers/api/v1/product_mappings_controller.rb +++ b/app/controllers/api/v1/product_mappings_controller.rb @@ -3,6 +3,8 @@ module Api module V1 class ProductMappingsController < ApplicationController + include Paginatable + MISSING_INTEGRATION = 'company_integration_id is required' ALREADY_LINKED = 'external product already linked to another product in this integration' @@ -12,15 +14,18 @@ class ProductMappingsController < ApplicationController rescue_from ActiveRecord::RecordNotUnique, with: :render_conflict def index - mappings = policy_scope(ProductMapping) - .where(product_id: @product.id) - .includes(company_integration: :service) - .order(:created_at) - - # Se envuelve en `data` para que el front trate una sola shape en todo - # el árbol de /products. No lleva `meta` como el index de productos: - # los mappings son tantos como canales de venta y no se paginan. - render json: { data: ProductMappingSerializer.render_as_hash(mappings) } + scope = policy_scope(ProductMapping) + .where(product_id: @product.id) + .includes(company_integration: :service) + .order(:created_at) + + # Los mapeos de un producto son tantos como canales de venta tenga la + # empresa: se leen enteros, así que van con `WHOLE_LIST_PER_PAGE`. Lo + # que cambia respecto de antes es que ahora hay un techo, que es el + # punto de TESIS-108: ningún listado devuelve una cantidad ilimitada. + mappings, meta = paginate(scope, per_page: WHOLE_LIST_PER_PAGE) + + render json: { data: ProductMappingSerializer.render_as_hash(mappings), meta: meta } end def create diff --git a/app/controllers/api/v1/products_controller.rb b/app/controllers/api/v1/products_controller.rb index 46cb77c..5ed6b69 100644 --- a/app/controllers/api/v1/products_controller.rb +++ b/app/controllers/api/v1/products_controller.rb @@ -4,6 +4,7 @@ module Api module V1 class ProductsController < ApplicationController include OptimisticLocking + include Paginatable before_action :set_product, only: %i[show update destroy] rescue_from ActiveRecord::RecordNotUnique, with: :render_conflict @@ -11,9 +12,6 @@ class ProductsController < ApplicationController rescue_from Catalog::StaleProductError, with: :render_precondition_failed def index - 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: ProductListSerializer lee el depósito # principal de cada fila, y sin ella son dos queries por producto # (stocks + warehouse) en vez de dos para toda la página. @@ -24,17 +22,14 @@ def index # como eager_load, sumaría las columnas de stocks y warehouses a ese # SELECT y Postgres rechazaría la consulta por columnas fuera del # GROUP BY. preload garantiza las consultas separadas. - products = filtered_products.preload(stocks: :warehouse) - .order(created_at: :desc) - .offset((page - 1) * per_page) - .limit(per_page) - - total = count_of(filtered_products) - - render json: { - data: ProductListSerializer.render_as_hash(products), - meta: { page: page, per_page: per_page, total: total } - } + # `total:` explícito: el scope viene agrupado por products.id, así que + # su `.count` devolvería un Hash y no un entero (ver `count_of`). + products, meta = paginate( + filtered_products.preload(stocks: :warehouse).order(created_at: :desc), + total: count_of(filtered_products) + ) + + render json: { data: ProductListSerializer.render_as_hash(products), meta: meta } end def show diff --git a/app/controllers/api/v1/warehouses_controller.rb b/app/controllers/api/v1/warehouses_controller.rb index 0b482fd..a6ff6f6 100644 --- a/app/controllers/api/v1/warehouses_controller.rb +++ b/app/controllers/api/v1/warehouses_controller.rb @@ -3,15 +3,27 @@ module Api module V1 class WarehousesController < ApplicationController + include Paginatable + before_action :set_warehouse, only: %i[show update destroy] rescue_from ActiveRecord::RecordNotDestroyed, with: :render_conflict def index # with_stored_units agrega la suma de stocks en la misma consulta: sin # el scope, el serializer pediria las unidades deposito por deposito. - warehouses = policy_scope(Warehouse).with_stored_units.order(created_at: :desc) + # + # `WHOLE_LIST_PER_PAGE` y no el default: el front usa este listado para + # llenar selects —el picker de origen del alta manual, el de los modales + # de producto—, no una tabla con paginador. Con 20 le faltarían depósitos + # sin que nada se lo diga; el techo sigue existiendo y `meta.total` le + # avisa si alguna vez lo pasa. + # + # `total:` explícito: with_stored_units agrupa por warehouses.id. + scope = policy_scope(Warehouse).with_stored_units.order(created_at: :desc) + warehouses, meta = paginate(scope, per_page: WHOLE_LIST_PER_PAGE, + total: policy_scope(Warehouse).count) - render json: { data: WarehouseSerializer.render_as_hash(warehouses) } + render json: { data: WarehouseSerializer.render_as_hash(warehouses), meta: meta } end def show diff --git a/app/controllers/concerns/api/v1/paginatable.rb b/app/controllers/concerns/api/v1/paginatable.rb new file mode 100644 index 0000000..18cf8b9 --- /dev/null +++ b/app/controllers/concerns/api/v1/paginatable.rb @@ -0,0 +1,67 @@ +# frozen_string_literal: true + +module Api + module V1 + # Paginación de los listados de la API (TESIS-108). + # + # Existía copiada literal en cuatro controllers —el mismo `[page.to_i, 1].max` + # y el mismo `clamp(1, 100)`— y ausente en otros tres, que devolvían la tabla + # entera. Esto es la única definición de las dos cosas: cuántas filas se + # devuelven y cómo se arma el `meta` que las acompaña. + # + # La forma de la respuesta la fija ADR-015: la colección va en `data` y el + # `meta` al lado, con `page`, `per_page` y `total`. El `total` cuenta el + # scope **ya filtrado**, no la tabla: de ahí salen los contadores de las + # pestañas y los KPIs del panel. + module Paginatable + extend ActiveSupport::Concern + + # Techo duro. Nadie puede pedir más, venga el número de donde venga: es lo + # que impide que un listado devuelva una cantidad ilimitada de filas. + MAX_PER_PAGE = 100 + + # Cuántas filas devuelve un listado que nadie acotó. Es el tamaño de una + # pantalla paginada. + DEFAULT_PER_PAGE = 20 + + # Para los listados que el consumidor lee enteros —depósitos, mapeos, + # integraciones: los usa para llenar un select, no una tabla con + # paginador—. Siguen teniendo techo; lo que cambia es que el default no + # los recorta antes de tiempo. + # + # Si alguna empresa pasa de acá, el consumidor se entera por `meta.total`, + # que va a ser mayor que las filas recibidas, y ahí le toca paginar. Es + # preferible a que el default de 20 le esconda depósitos en silencio. + WHOLE_LIST_PER_PAGE = MAX_PER_PAGE + + private + + # Devuelve `[filas, meta]`. + # + # `total:` se puede pasar cuando contar el scope no es directo: el catálogo + # viene agrupado por `products.id`, así que su `.count` devuelve un Hash y + # el controller ya sabe cómo contarlo (ver `count_of`). + def paginate(scope, per_page: DEFAULT_PER_PAGE, total: nil) + page = page_number + size = page_size(per_page) + + [scope.offset((page - 1) * size).limit(size), + { page: page, per_page: size, total: total || scope.count }] + end + + # Página pedida, nunca menor que 1. `page=0` y `page=-3` se acotan en vez + # de romper: un offset negativo es un error de SQL, y un 400 por un número + # que se puede interpretar sería antipático. + def page_number + [scalar_param(:page).to_i, 1].max + end + + # `scalar_param` y no `params[...]`: `?per_page[]=1` entrega un Array y + # `Array#to_i` no existe (TESIS-124). Un valor no numérico cae en `to_i` + # a 0 y el `clamp` lo lleva al mínimo. + def page_size(default) + (scalar_param(:per_page) || default).to_i.clamp(1, MAX_PER_PAGE) + end + end + end +end diff --git a/docs/adr/ADR-015-convencion-de-respuesta-de-la-api.md b/docs/adr/ADR-015-convencion-de-respuesta-de-la-api.md index d380385..461b002 100644 --- a/docs/adr/ADR-015-convencion-de-respuesta-de-la-api.md +++ b/docs/adr/ADR-015-convencion-de-respuesta-de-la-api.md @@ -26,8 +26,8 @@ Mientras hubo una sola pantalla consumiendo la API la inconsistencia era barata. ``` GET /api/v1/products → { "data": [ {...}, {...} ], "meta": { page, per_page, total } } -GET /api/v1/warehouses → { "data": [ {...}, {...} ] } -GET /api/v1/integrations → { "data": [ {...}, {...} ] } +GET /api/v1/warehouses → { "data": [ {...}, {...} ], "meta": { ... } } +GET /api/v1/integrations → { "data": [ {...}, {...} ], "meta": { ... } } GET /api/v1/products/:id → { "id": 1, "sku": "...", ... } POST /api/v1/products → { "id": 1, "sku": "...", ... } @@ -36,7 +36,7 @@ PUT /api/v1/products/:id → { "id": 1, "sku": "...", ... } cualquier error → { "error": "..." } ``` -`meta` aparece sólo si el listado pagina, y es siempre `page`, `per_page` y `total`, contando el scope **ya filtrado**. +`meta` es siempre `page`, `per_page` y `total`, contando el scope **ya filtrado**. No hay colección sin `meta`: desde TESIS-108 todas paginan, así que el consumidor puede leer `total` en cualquiera sin preguntarse cuál lo trae. El único endpoint que hubo que cambiar fue `integrations#index`, que devolvía un array en la raíz. @@ -55,7 +55,7 @@ La puerta queda abierta: pasar de esta convención a la otra es aditivo del lado **A favor** - La regla se enuncia en una línea y no tiene excepciones que justificar. -- Ninguna colección queda con un array en la raíz, así que cualquiera puede empezar a paginar sin romper su contrato. Es la precondición de TESIS-108. +- Ninguna colección queda con un array en la raíz, así que todas pudieron empezar a paginar sin romper su contrato. Fue la precondición de TESIS-108, que se hizo justo encima. - El comentario-trampa del frontend se borra: lo que explicaba ya no pasa. - `spec/requests/api/v1/api_contract_spec.rb` (TESIS-90) fija las tres formas, así que un endpoint nuevo que invente una cuarta rompe la suite. diff --git a/docs/guidelines/architecture.md b/docs/guidelines/architecture.md index 644497b..e4f9801 100644 --- a/docs/guidelines/architecture.md +++ b/docs/guidelines/architecture.md @@ -139,15 +139,17 @@ Una sola regla, fijada en [ADR-015](../adr/ADR-015-convencion-de-respuesta-de-la ``` GET /api/v1/products → { "data": [ ... ], "meta": { page, per_page, total } } -GET /api/v1/warehouses → { "data": [ ... ] } +GET /api/v1/warehouses → { "data": [ ... ], "meta": { page, per_page, total } } GET /api/v1/products/:id → { "id": 1, "sku": "...", ... } POST /api/v1/products → { "id": 1, "sku": "...", ... } cualquier error → { "error": "..." } ``` -`meta` aparece sólo si el listado pagina, y cuenta el scope **ya filtrado**, no la tabla entera. +`meta` cuenta el scope **ya filtrado**, no la tabla entera, y lo lleva toda colección: ningún listado devuelve una cantidad ilimitada de filas. -Ninguna colección devuelve un array en la raíz: un array pelado no admite agregarle `meta` sin romper a quien lo consume. `spec/requests/api/v1/api_contract_spec.rb` fija las tres formas, así que un endpoint nuevo que invente una cuarta rompe la suite. +El cálculo vive en un solo lugar, el concern `Api::V1::Paginatable`, con el techo (`MAX_PER_PAGE = 100`) y los dos defaults: 20 para una pantalla paginada y 100 para los listados que el consumidor lee enteros —depósitos, mapeos, integraciones— y usa para llenar un select. `page` y `per_page` fuera de rango se acotan en vez de romper. + +`spec/requests/api/v1/pagination_spec.rb` fija los bordes una vez y verifica que **todos** los listados traigan `meta` y respeten el techo; `api_contract_spec.rb` fija las formas. Un endpoint nuevo que no pagine rompe la suite. ### 4.1 Flujo de un webhook entrante diff --git a/spec/requests/api/v1/api_contract_spec.rb b/spec/requests/api/v1/api_contract_spec.rb index 17838ce..90860fa 100644 --- a/spec/requests/api/v1/api_contract_spec.rb +++ b/spec/requests/api/v1/api_contract_spec.rb @@ -104,9 +104,12 @@ def shipment # ───────────────────────────────────────────────────────── forma del sobre # - # La regla la fija ADR-015 (TESIS-107) y es una sola: **una colección viaja - # envuelta en `data`** —más `meta` si pagina— y **un recurso solo viaja - # pelado**. Los errores, siempre `{ "error": "..." }`. + # La regla la fija ADR-015 (TESIS-107, TESIS-108) y es una sola: **una + # colección viaja en `data` + `meta`** y **un recurso solo viaja pelado**. + # Los errores, siempre `{ "error": "..." }`. + # + # No hay colección sin `meta`: desde TESIS-108 todas paginan, así que el + # consumidor puede leer `total` en cualquiera sin preguntarse cuál lo trae. # # Antes eran cuatro formas distintas, porque cada card eligió la suya y nadie # la escribió. Estos ejemplos son lo que impide que vuelva a pasar: agregar @@ -121,13 +124,15 @@ def shipment expect(response.parsed_body['meta'].keys).to match_array(claves[:meta]) end - # Sin `meta`: el listado no pagina. Es la mitad de TESIS-108. - it 'wraps an unpaginated collection in data alone' do + # Antes devolvía `data` sola porque no paginaba. Desde TESIS-108 no queda + # ninguna así: un listado sin techo puede devolver la tabla entera. + it 'wraps every collection in data plus meta, with no exception', :aggregate_failures do warehouse get '/api/v1/warehouses', headers: headers - expect(response.parsed_body.keys).to eq(['data']) + expect(response.parsed_body.keys).to match_array(%w[data meta]) + expect(response.parsed_body['meta'].keys).to match_array(claves[:meta]) end it 'returns a single resource with no envelope at all' do @@ -139,10 +144,10 @@ def shipment # Era el único que devolvía un array pelado. Un array en la raíz no admite # `meta` sin romper a quien lo consume, así que ninguna colección puede # quedar así. - it 'wraps the integrations listing too, with no exception' do + it 'wraps the integrations listing too, which used to be a bare array' do get '/api/v1/integrations', headers: headers - expect(response.parsed_body.keys).to eq(['data']) + expect(response.parsed_body.keys).to match_array(%w[data meta]) end # Los errores sí son consistentes en toda la API, y conviene que siga así. diff --git a/spec/requests/api/v1/integrations_spec.rb b/spec/requests/api/v1/integrations_spec.rb index 7dc1c7c..bd13f11 100644 --- a/spec/requests/api/v1/integrations_spec.rb +++ b/spec/requests/api/v1/integrations_spec.rb @@ -25,10 +25,10 @@ def listed # Era el único listado que contestaba un array pelado. Un array en la raíz # no deja lugar para `meta` sin romper a quien lo consume, y obligaba al # front a recordar que éste es la excepción (ADR-015, TESIS-107). - it 'wraps the collection in data, like every other listing' do + it 'wraps the collection in data and meta, like every other listing' do get '/api/v1/integrations', headers: headers - expect(response.parsed_body.keys).to eq(['data']) + expect(response.parsed_body.keys).to match_array(%w[data meta]) end context 'when the company has the service configured' do diff --git a/spec/requests/api/v1/pagination_spec.rb b/spec/requests/api/v1/pagination_spec.rb new file mode 100644 index 0000000..3ec4eb8 --- /dev/null +++ b/spec/requests/api/v1/pagination_spec.rb @@ -0,0 +1,147 @@ +# frozen_string_literal: true + +require 'rails_helper' + +# Los bordes de la paginación, una vez y no por endpoint (TESIS-108). +# +# El cálculo vive en `Api::V1::Paginatable` y lo comparten todos los listados, +# así que se prueba sobre uno —productos, que es el que más filas tiene en los +# ejemplos— y aparte se verifica que los demás lo usen. +# +# Lo que estos ejemplos protegen es que un número raro se **acote** en vez de +# romper: un `page=0` se traduce a un offset negativo, que es un error de SQL, y +# un `per_page=9999` es una respuesta que nadie pidió. +RSpec.describe 'Pagination', type: :request do + let(:company) { Company.create!(name: 'Norte', tax_id: '30-11111111-1') } + let(:user) { User.create!(email: 'n@example.com', password: 'password123', company: company) } + let(:headers) { auth_headers(user) } + + def auth_headers(for_user) + post '/api/v1/auth/login', + params: { email: for_user.email, password: 'password123' }, + headers: { 'X-Tenant-Slug' => for_user.company.slug } + { 'Authorization' => "Bearer #{response.parsed_body['token']}" } + end + + def create_products(count) + count.times { |i| Product.create!(company: company, sku: "NOR-#{i}", name: "Producto #{i}") } + end + + def meta_for(params) + get '/api/v1/products', params: params, headers: headers + response.parsed_body['meta'] + end + + def rows_for(params) + get '/api/v1/products', params: params, headers: headers + response.parsed_body['data'] + end + + describe 'the page number' do + it 'defaults to the first page' do + expect(meta_for({})['page']).to eq(1) + end + + # Un offset negativo es un error de SQL: se acota en vez de romper. + it 'clamps page zero to the first page' do + expect(meta_for(page: 0)['page']).to eq(1) + end + + it 'clamps a negative page to the first page' do + expect(meta_for(page: -3)['page']).to eq(1) + end + + it 'reads a page that is not a number as the first one' do + expect(meta_for(page: 'dos')['page']).to eq(1) + end + + # Una página más allá del final no es un error: es una página vacía. + it 'answers an empty page past the end, with the real total', :aggregate_failures do + create_products(3) + + expect(rows_for(page: 99)).to be_empty + expect(meta_for(page: 99)['total']).to eq(3) + end + end + + describe 'the page size' do + it 'defaults to twenty rows' do + expect(meta_for({})['per_page']).to eq(Api::V1::Paginatable::DEFAULT_PER_PAGE) + end + + it 'honours a size the caller asked for' do + expect(meta_for(per_page: 5)['per_page']).to eq(5) + end + + # El techo es lo que impide que un listado devuelva la tabla entera. + it 'never goes above the ceiling, however large the request' do + expect(meta_for(per_page: 9999)['per_page']).to eq(Api::V1::Paginatable::MAX_PER_PAGE) + end + + it 'clamps a size of zero to one row' do + expect(meta_for(per_page: 0)['per_page']).to eq(1) + end + + it 'clamps a negative size to one row' do + expect(meta_for(per_page: -5)['per_page']).to eq(1) + end + + it 'reads a size that is not a number as one row' do + expect(meta_for(per_page: 'todas')['per_page']).to eq(1) + end + + it 'returns exactly the rows it says it returns' do + create_products(7) + + expect(rows_for(per_page: 3).length).to eq(3) + end + end + + describe 'the total' do + it 'counts the whole scope and not the page', :aggregate_failures do + create_products(7) + + get '/api/v1/products', params: { per_page: 3 }, headers: headers + + expect(response.parsed_body['data'].length).to eq(3) + expect(response.parsed_body['meta']['total']).to eq(7) + end + end + + # Antes esto era el cálculo copiado en cuatro controllers y ausente en otros + # tres. Si alguien agrega un listado que no pagina, o vuelve a copiar el + # cálculo, este bloque lo delata. + describe 'every listing of the API' do + def warehouse + Warehouse.create!(company: company, name: 'CD Norte', address: 'Av. 1', zip_code: '1900') + end + + def listings + product = Product.create!(company: company, sku: 'NOR-X', name: 'Producto') + warehouse + ['/api/v1/products', '/api/v1/warehouses', '/api/v1/orders', '/api/v1/shipments', + '/api/v1/failed-events', '/api/v1/integrations', "/api/v1/products/#{product.id}/mappings"] + end + + def meta_of(path) + get path, headers: headers + response.parsed_body['meta'] + end + + it 'answers with a meta that carries page, per_page and total', :aggregate_failures do + listings.each do |path| + expect(meta_of(path)&.keys).to match_array(%w[page per_page total]), + "#{path} no trae el meta esperado" + end + end + + it 'respects the ceiling everywhere', :aggregate_failures do + listings.each do |path| + get path, params: { per_page: 9999 }, headers: headers + + expect(response.parsed_body['meta']['per_page']).to eq(Api::V1::Paginatable::MAX_PER_PAGE), + "#{path} se pasa del techo" + end + end + end +end diff --git a/spec/requests/api/v1/product_mappings_spec.rb b/spec/requests/api/v1/product_mappings_spec.rb index fbfe289..0780d45 100644 --- a/spec/requests/api/v1/product_mappings_spec.rb +++ b/spec/requests/api/v1/product_mappings_spec.rb @@ -72,12 +72,12 @@ def link_external_id_to_another_product(external_id) # Misma envoltura que GET /api/v1/products (TESIS-33): el front no tiene que # tratar dos shapes distintas en endpoints vecinos del mismo árbol. - it 'wraps the collection in a data key', :aggregate_failures do + it 'wraps the collection in data and meta', :aggregate_failures do mapping_for(product, meli_integration, 'MLA-123') get mappings_url(product.id), headers: headers expect(response.parsed_body).to be_a(Hash) - expect(response.parsed_body.keys).to eq(['data']) + expect(response.parsed_body.keys).to match_array(%w[data meta]) expect(response.parsed_body['data']).to be_an(Array) end From df0ce50901353f82b073cb55bde3667fac2074c9 Mon Sep 17 00:00:00 2001 From: Tomas Martin Date: Thu, 24 Sep 2026 21:37:56 -0300 Subject: [PATCH 3/4] feat: [TESIS-108] paginate the three listings the first pass missed The review found that two of the four card criteria were not met while the docs said they were. - `stock_transfers#index` returned the whole scope: no ceiling, no meta. It was missing from the list the sweeping spec walks, which is why nothing caught it. Both are fixed. - `orders#index` and `shipments#index` kept their own copy of the page calculation, including the loose `clamp(1, 100)` the card asked to turn into a constant. They go through the concern now. The rule in the ADR and the guide now names what does not paginate and why: fixed vocabularies (provinces, categories) and the result of an action (quotes) travel in `data` without `meta`, because their length is decided by the code and not by the company's data. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_012xAtddb53LRNVLuyTXyELp --- app/controllers/api/v1/orders_controller.rb | 31 +++++++------------ .../api/v1/shipments_controller.rb | 25 +++++++-------- .../api/v1/stock_transfers_controller.rb | 21 +++++++++---- .../concerns/api/v1/paginatable.rb | 10 ++++-- ...R-015-convencion-de-respuesta-de-la-api.md | 11 ++++++- docs/guidelines/architecture.md | 4 +-- spec/requests/api/v1/pagination_spec.rb | 3 +- 7 files changed, 59 insertions(+), 46 deletions(-) diff --git a/app/controllers/api/v1/orders_controller.rb b/app/controllers/api/v1/orders_controller.rb index 69dff9d..f160174 100644 --- a/app/controllers/api/v1/orders_controller.rb +++ b/app/controllers/api/v1/orders_controller.rb @@ -4,6 +4,7 @@ module Api module V1 class OrdersController < ApplicationController include OptimisticLocking + include Paginatable rescue_from ActiveRecord::RecordNotSaved, with: :render_unprocessable rescue_from Catalog::InsufficientStockError, with: :render_insufficient_stock @@ -31,28 +32,20 @@ class OrdersController < ApplicationController ].freeze def index - # `scalar_param` y no `params[...]` directo: una query con `?page[]=1` - # entrega un Array y `to_i` sale con NoMethodError → 500. Ver - # ApplicationController. - page = [scalar_param(:page).to_i, 1].max - per_page = (scalar_param(:per_page) || 20).to_i.clamp(1, 100) - # La precarga alimenta dos columnas del serializer: `item_count` sale de # order_items y `courier` de la cadena envío → integración → servicio. # Sin ella, cada fila de la página dispara sus propias consultas. - orders = filtered_orders.preload(:order_items, shipment: { company_integration: :service }) - .order(created_at: :desc, id: :desc) - .offset((page - 1) * per_page) - .limit(per_page) - - render json: { - data: OrderListSerializer.render_as_hash(orders), - # El total se cuenta sobre el scope YA FILTRADO, no sobre la tabla de - # la empresa: de este número salen los KPIs de TESIS-53, que los pide - # con `?status=pending&per_page=1` y lee sólo el meta. Si contara de - # más, los KPIs mentirían. - meta: { page: page, per_page: per_page, total: filtered_orders.count } - } + # + # El `total` del meta lo cuenta el concern sobre el scope YA FILTRADO, + # no sobre la tabla de la empresa: de ese número salen los KPIs de + # TESIS-53, que los piden con `?status=pending&per_page=1` y leen sólo + # el meta. Si contara de más, los KPIs mentirían. + orders, meta = paginate( + filtered_orders.preload(:order_items, shipment: { company_integration: :service }) + .order(created_at: :desc, id: :desc) + ) + + render json: { data: OrderListSerializer.render_as_hash(orders), meta: meta } end # Vocabulario del select de provincia del alta manual (TESIS-58). Mismo diff --git a/app/controllers/api/v1/shipments_controller.rb b/app/controllers/api/v1/shipments_controller.rb index 17b36b1..009402a 100644 --- a/app/controllers/api/v1/shipments_controller.rb +++ b/app/controllers/api/v1/shipments_controller.rb @@ -19,27 +19,24 @@ class ShipmentsController < ApplicationController # 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 + # Cuánto del cuerpo del courier se propaga en el mensaje de error. 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) - # La precarga es load-bearing: ShipmentListSerializer lee el nombre del # courier a través de la plantilla del Service, y sin ella son dos # queries por fila (company_integrations + services). - shipments = filtered_shipments.preload(company_integration: :service) - .order(created_at: :desc, id: :desc) - .offset((page - 1) * per_page) - .limit(per_page) - - render json: { - data: ShipmentListSerializer.render_as_hash(shipments), - # El total se cuenta sobre el scope filtrado, no sobre el total de la - # empresa: de acá sale el KPI de envíos activos (TESIS-53). - meta: { page: page, per_page: per_page, total: filtered_shipments.count } - } + # + # El total lo cuenta el concern sobre el scope filtrado, no sobre el + # total de la empresa: de acá sale el KPI de envíos activos (TESIS-53). + shipments, meta = paginate( + filtered_shipments.preload(company_integration: :service) + .order(created_at: :desc, id: :desc) + ) + + render json: { data: ShipmentListSerializer.render_as_hash(shipments), meta: meta } end def show diff --git a/app/controllers/api/v1/stock_transfers_controller.rb b/app/controllers/api/v1/stock_transfers_controller.rb index b1b21f1..28d4dc5 100644 --- a/app/controllers/api/v1/stock_transfers_controller.rb +++ b/app/controllers/api/v1/stock_transfers_controller.rb @@ -3,18 +3,16 @@ module Api module V1 class StockTransfersController < ApplicationController + include Paginatable + before_action :set_transfer, only: %i[receive cancel] rescue_from Catalog::InsufficientWarehouseStockError, with: :render_unprocessable rescue_from Catalog::SettleTransfer::NotInFlightError, with: :render_conflict 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? + transfers, meta = paginate(filtered_transfers) - render json: { data: StockTransferSerializer.render_as_hash(transfers) } + render json: { data: StockTransferSerializer.render_as_hash(transfers), meta: meta } end def create @@ -40,6 +38,17 @@ def cancel private + # Las unidades en vuelo, filtrables por estado y por producto. El listado + # del catálogo pide las de un producto para el «+N Incoming» de su ficha. + 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? + transfers + end + def settle(outcome) transfer = Catalog::SettleTransfer.new(transfer: @transfer, outcome: outcome).call diff --git a/app/controllers/concerns/api/v1/paginatable.rb b/app/controllers/concerns/api/v1/paginatable.rb index 18cf8b9..79f89e8 100644 --- a/app/controllers/concerns/api/v1/paginatable.rb +++ b/app/controllers/concerns/api/v1/paginatable.rb @@ -5,9 +5,13 @@ module V1 # Paginación de los listados de la API (TESIS-108). # # Existía copiada literal en cuatro controllers —el mismo `[page.to_i, 1].max` - # y el mismo `clamp(1, 100)`— y ausente en otros tres, que devolvían la tabla - # entera. Esto es la única definición de las dos cosas: cuántas filas se - # devuelven y cómo se arma el `meta` que las acompaña. + # y el mismo `clamp(1, 100)`— y ausente en otros cuatro, que devolvían la + # tabla entera. Esto es la única definición de las dos cosas: cuántas filas + # se devuelven y cómo se arma el `meta` que las acompaña. + # + # Todo listado de registros pasa por acá. Los vocabularios fijos + # (`/orders/provinces`, `/products/categories`) y el resultado de una acción + # (`/orders/:id/quotes`) no: su largo lo decide el código, no la empresa. # # La forma de la respuesta la fija ADR-015: la colección va en `data` y el # `meta` al lado, con `page`, `per_page` y `total`. El `total` cuenta el diff --git a/docs/adr/ADR-015-convencion-de-respuesta-de-la-api.md b/docs/adr/ADR-015-convencion-de-respuesta-de-la-api.md index 461b002..dd79f91 100644 --- a/docs/adr/ADR-015-convencion-de-respuesta-de-la-api.md +++ b/docs/adr/ADR-015-convencion-de-respuesta-de-la-api.md @@ -36,7 +36,16 @@ PUT /api/v1/products/:id → { "id": 1, "sku": "...", ... } cualquier error → { "error": "..." } ``` -`meta` es siempre `page`, `per_page` y `total`, contando el scope **ya filtrado**. No hay colección sin `meta`: desde TESIS-108 todas paginan, así que el consumidor puede leer `total` en cualquiera sin preguntarse cuál lo trae. +`meta` es siempre `page`, `per_page` y `total`, contando el scope **ya filtrado**. + +**Qué lleva `meta` y qué no.** Desde TESIS-108 pagina todo **listado de registros** —productos, depósitos, órdenes, envíos, transferencias, eventos fallidos, mapeos, integraciones—, así que ahí el consumidor puede leer `total` sin preguntarse cuál lo trae. Van envueltos en `data` **sin** `meta`, en cambio, los que no son listados de registros: + +| Respuesta | Por qué no pagina | +| --- | --- | +| `GET /orders/provinces`, `GET /products/categories` | Vocabularios fijos del dominio, no filas de una tabla: su tamaño lo fija el código, no los datos de la empresa | +| `POST /orders/:id/quotes` | El resultado de una acción —una cotización por courier configurado—, no una consulta | + +La distinción importa para el consumidor: leer `meta.total` en cualquiera de esas tres devuelve `undefined`. La regla corta es **si el largo lo decide la empresa, pagina; si lo decide el código, no**. El único endpoint que hubo que cambiar fue `integrations#index`, que devolvía un array en la raíz. diff --git a/docs/guidelines/architecture.md b/docs/guidelines/architecture.md index e4f9801..977827b 100644 --- a/docs/guidelines/architecture.md +++ b/docs/guidelines/architecture.md @@ -145,11 +145,11 @@ POST /api/v1/products → { "id": 1, "sku": "...", ... } cualquier error → { "error": "..." } ``` -`meta` cuenta el scope **ya filtrado**, no la tabla entera, y lo lleva toda colección: ningún listado devuelve una cantidad ilimitada de filas. +`meta` cuenta el scope **ya filtrado**, no la tabla entera, y lo lleva todo **listado de registros**: ninguno devuelve una cantidad ilimitada de filas. Los vocabularios fijos (`/orders/provinces`, `/products/categories`) y el resultado de una acción (`/orders/:id/quotes`) viajan en `data` sin `meta`, porque su largo lo decide el código y no los datos de la empresa (ver ADR-015). El cálculo vive en un solo lugar, el concern `Api::V1::Paginatable`, con el techo (`MAX_PER_PAGE = 100`) y los dos defaults: 20 para una pantalla paginada y 100 para los listados que el consumidor lee enteros —depósitos, mapeos, integraciones— y usa para llenar un select. `page` y `per_page` fuera de rango se acotan en vez de romper. -`spec/requests/api/v1/pagination_spec.rb` fija los bordes una vez y verifica que **todos** los listados traigan `meta` y respeten el techo; `api_contract_spec.rb` fija las formas. Un endpoint nuevo que no pagine rompe la suite. +`spec/requests/api/v1/pagination_spec.rb` fija los bordes una vez y recorre los listados que enumera, verificando que cada uno traiga `meta` y respete el techo; `api_contract_spec.rb` fija las formas de los endpoints que enumera. Las dos listas están escritas a mano: **un listado nuevo hay que sumarlo ahí**, o la suite no se entera de que existe. ### 4.1 Flujo de un webhook entrante diff --git a/spec/requests/api/v1/pagination_spec.rb b/spec/requests/api/v1/pagination_spec.rb index 3ec4eb8..7534973 100644 --- a/spec/requests/api/v1/pagination_spec.rb +++ b/spec/requests/api/v1/pagination_spec.rb @@ -120,7 +120,8 @@ def listings product = Product.create!(company: company, sku: 'NOR-X', name: 'Producto') warehouse ['/api/v1/products', '/api/v1/warehouses', '/api/v1/orders', '/api/v1/shipments', - '/api/v1/failed-events', '/api/v1/integrations', "/api/v1/products/#{product.id}/mappings"] + '/api/v1/failed-events', '/api/v1/integrations', '/api/v1/stock-transfers', + "/api/v1/products/#{product.id}/mappings"] end def meta_of(path) From ad7db50f5464469d629129a351a5e346ec6bc7fe Mon Sep 17 00:00:00 2001 From: Tomas Martin Date: Thu, 24 Sep 2026 21:44:12 -0300 Subject: [PATCH 4/4] fix: [TESIS-107] make the registration errors follow the convention MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The review found the fourth shape the ADR says no longer exists: `auth/register` answered both of its failures as `{ errors: [...] }` — plural, array — with a comment claiming it did so to spare the frontend two formats. It is a single `error` string now, and the contract spec covers both paths, which is why nothing caught it before. The ADR also said three things that were not true: that `api_contract_spec.rb` breaks on any new shape (it only covers the endpoints it lists), that integrations was the only endpoint to change, and that two files were being edited in live branches — TESIS-58 and TESIS-61 merged today. The rule now also states that `error` may carry extra data, as the 409 of optimistic locking does with `current_version`. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_012xAtddb53LRNVLuyTXyELp --- .../api/v1/auth/registrations_controller.rb | 12 +++++++---- ...R-015-convencion-de-respuesta-de-la-api.md | 13 +++++++++--- spec/requests/api/v1/api_contract_spec.rb | 20 +++++++++++++++++++ 3 files changed, 38 insertions(+), 7 deletions(-) diff --git a/app/controllers/api/v1/auth/registrations_controller.rb b/app/controllers/api/v1/auth/registrations_controller.rb index f28c290..7a4085a 100644 --- a/app/controllers/api/v1/auth/registrations_controller.rb +++ b/app/controllers/api/v1/auth/registrations_controller.rb @@ -16,7 +16,11 @@ def create user = ::Auth::RegisterUser.new(params: user_params, company: company).call render json: UserSerializer.render(user), status: :created rescue ActiveRecord::RecordInvalid => e - render json: { errors: e.record.errors.full_messages }, status: :unprocessable_content + # `error` en singular y con un string, como el resto de la API + # (ADR-015). Los mensajes se unen en una oración: el consumidor de un + # alta que falla muestra el motivo, no arma una lista. + render json: { error: e.record.errors.full_messages.to_sentence }, + status: :unprocessable_content end private @@ -29,10 +33,10 @@ def user_params end # Mismo cuerpo para slug ausente, inexistente e inactivo: la respuesta no - # dice si el tenant existe. Se usa el shape `errors` que ya devuelve el - # 422 de validación, para que el frontend no distinga dos formatos. + # dice si el tenant existe. Misma forma que el 422 de validación de acá + # arriba y que la de toda la API, así el frontend no distingue formatos. def render_unknown_tenant - render json: { errors: ['Unable to complete registration'] }, + render json: { error: 'Unable to complete registration' }, status: :unprocessable_content end end diff --git a/docs/adr/ADR-015-convencion-de-respuesta-de-la-api.md b/docs/adr/ADR-015-convencion-de-respuesta-de-la-api.md index d380385..2ed0242 100644 --- a/docs/adr/ADR-015-convencion-de-respuesta-de-la-api.md +++ b/docs/adr/ADR-015-convencion-de-respuesta-de-la-api.md @@ -36,15 +36,22 @@ PUT /api/v1/products/:id → { "id": 1, "sku": "...", ... } cualquier error → { "error": "..." } ``` +`error` es una sola clave y un solo string, y **siempre está**. Puede venir acompañado de datos para recuperarse: el 409 del locking optimista agrega `current_version`, que es lo que el frontend necesita para reintentar. Lo que no se admite es otra clave en su lugar —`errors` en plural, un array, un objeto por campo—, porque entonces el consumidor tiene que probar dos formas. + +``` +``` + `meta` aparece sólo si el listado pagina, y es siempre `page`, `per_page` y `total`, contando el scope **ya filtrado**. -El único endpoint que hubo que cambiar fue `integrations#index`, que devolvía un array en la raíz. +Hubo que cambiar dos endpoints. `integrations#index`, que devolvía un array en la raíz, y `auth/register`, que respondía sus dos errores como `{ "errors": [...] }` —plural y array—. El registro no lo detectó la primera pasada porque el spec de contrato no lo cubría; ahora sí. ## Alternativas consideradas **Envolver también los recursos solos** (`{ "data": { ... } }` en `show`, `create` y `update`). Es la regla más simple de enunciar —una sola, sin excepciones— y deja lugar para agregarle `meta` a un recurso individual el día que haga falta. -Se descartó por lo que costaba **ahora**, no por lo que vale: son diez lugares del frontend, en cuatro archivos de frontera, y dos de esos archivos son los que TESIS-58 y TESIS-61 están editando en ramas vivas. Cambiar el contrato debajo de dos PRs abiertos, para ganar uniformidad en endpoints que hoy nadie confunde, no se paga. +Se descartó por lo que costaba **ahora**, no por lo que vale: son diez lugares del frontend, repartidos en cuatro archivos de frontera, y cada uno hay que cambiarlo y volver a probarlo para ganar uniformidad en endpoints que hoy nadie confunde. La URL ya separa los dos casos —`/products` contra `/products/:id`—, así que lo que se compra con esos diez cambios es que la regla se enuncie sin la segunda mitad. + +_(La primera versión de este ADR agregaba que dos de esos archivos estaban siendo editados en las ramas vivas de TESIS-58 y TESIS-61. Las dos se mergearon el 24/09, así que ese motivo ya no corre; el costo de los diez lugares, sí.)_ La puerta queda abierta: pasar de esta convención a la otra es aditivo del lado del backend —envolver lo que hoy va pelado— y el día que se haga, este ADR se reemplaza en vez de discutirse de nuevo. @@ -57,7 +64,7 @@ La puerta queda abierta: pasar de esta convención a la otra es aditivo del lado - La regla se enuncia en una línea y no tiene excepciones que justificar. - Ninguna colección queda con un array en la raíz, así que cualquiera puede empezar a paginar sin romper su contrato. Es la precondición de TESIS-108. - El comentario-trampa del frontend se borra: lo que explicaba ya no pasa. -- `spec/requests/api/v1/api_contract_spec.rb` (TESIS-90) fija las tres formas, así que un endpoint nuevo que invente una cuarta rompe la suite. +- `spec/requests/api/v1/api_contract_spec.rb` (TESIS-90) fija las tres formas **sobre los endpoints que enumera**, hoy incluido el registro. La lista está escrita a mano: un endpoint nuevo con otra forma no rompe nada hasta que se lo agrega ahí. Recorrer todas las rutas sería otra card; mientras tanto, sumar el endpoint al spec es parte de agregarlo. **En contra** diff --git a/spec/requests/api/v1/api_contract_spec.rb b/spec/requests/api/v1/api_contract_spec.rb index 17838ce..8a7f86c 100644 --- a/spec/requests/api/v1/api_contract_spec.rb +++ b/spec/requests/api/v1/api_contract_spec.rb @@ -151,6 +151,26 @@ def shipment expect(response.parsed_body.keys).to eq(['error']) end + + # El registro respondía `errors` en plural y con un array: era la cuarta + # forma que este ADR vino a sacar, y no la veía nadie porque el endpoint no + # estaba acá. Los dos caminos que fallan, fijados. + it 'reports a failed registration with the same single error key' do + post '/api/v1/auth/register', + params: { email: 'no-es-un-mail', password: '123' }, + headers: { 'X-Tenant-Slug' => company.slug } + + expect(response.parsed_body.keys).to eq(['error']) + end + + it 'reports an unknown tenant with the same single error key', :aggregate_failures do + post '/api/v1/auth/register', + params: { email: 'nuevo@example.com', password: 'password123' }, + headers: { 'X-Tenant-Slug' => 'no-existe' } + + expect(response.parsed_body.keys).to eq(['error']) + expect(response.parsed_body['error']).to be_a(String) + end end # ──────────────────────────────────────────────── los campos que el front lee