feat: [TESIS-101] detect concurrent edits of a product with If-Match - #61
Open
TomasMartin2004 wants to merge 2 commits into
Open
feat: [TESIS-101] detect concurrent edits of a product with If-Match#61TomasMartin2004 wants to merge 2 commits into
TomasMartin2004 wants to merge 2 commits into
Conversation
Closes the lost update ADR-009 left open. The advisory lock of TESIS-38 orders the writes; it cannot tell that two people edited the same thing, because the modal sends an absolute quantity and serialising does not change which one wins. This detects instead of ordering. GET /products/:id now returns an ETag with a fingerprint of the aggregate, and PUT accepts it back in If-Match. If the product moved in between, the write is rejected and nothing is written. Not lock_version, and the reason is the shape of what is edited. That column versions the products row, while the modal edits an aggregate: name, measurements and the quantity of every warehouse. Covering the stock with lock_version would mean bumping it from Stock's callbacks — writing to products on every sale, which is write amplification and contention on the hottest path in the system. The fingerprint covers the whole aggregate with no migration and no column. The fingerprint includes the stock deliberately. The dangerous case is not two operators, it is a sale: if a webhook deducts 5 units while the modal is open, saving the absolute quantity the user saw would erase that deduction with no trace. There is a spec for exactly that — stock moves underneath, no one edits anything, and the write is still rejected. The check runs inside the transaction and behind a lock! — SELECT ... FOR UPDATE on the product row. Comparing outside it would let through two requests that read the same version, which is the race being closed. With the row held, the second one waits, re-reads what the first left, and its version no longer matches. 412 rather than the 409 the card asked for. It is the code HTTP defines for a failed precondition, and it settles the card's own requirement of telling this apart from the other two 409s this endpoint already returns (duplicate SKU and stock lock busy) — with 412 the status is enough and the frontend does not have to read the body to know which conflict it is. Without If-Match the update proceeds as before. That is HTTP precondition semantics and it keeps the previous contract, at the cost of making the protection opt-in: a client that does not send the header is not protected. Both consequences are written down in the ADR. Renamed set_version_header to expose_version: it is not a writer, and Naming/AccessorMethodName was right to complain. The frontend half — the modal sending the version and handling the 412 without losing what the user typed — goes in its own PR on proyecto-web. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The optimistic locking is useless from the browser without this. CORS only lets JavaScript read the simple response headers unless the server lists the rest, and the frontend runs on a different origin (5173 against 3000), so `response.headers.etag` arrives undefined, the modal sends no If-Match and the protection is silently off — nothing fails, the guarantee just is not there. Found while wiring the frontend half, not by reading the diff. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🔗 Ticket de Jira
TESIS-101 — mitad backend. La del frontend va en
proyecto-weby las dos tienen que mergear juntas.📝 Descripción
Cierra el lost update que ADR-009 dejó abierto. El advisory lock de TESIS-38 ordena las escrituras; no puede saber que dos personas editaron lo mismo, porque el modal manda la cantidad absoluta y serializar no cambia cuál gana.
GET /products/:iddevuelve ahora unETagcon la huella del agregado, yPUTla acepta de vuelta enIf-Match. Si el producto se movió en el medio, la escritura se rechaza y no se escribe nada.Por qué no
lock_versionEsa columna versiona la fila
products, y el modal edita un agregado: nombre, medidas y la cantidad de cada depósito. Para que cubriera el stock habría que bumpearla desde los callbacks deStock— o sea escribir enproductsen cada venta: amplificación de escritura y contención justo en el camino más caliente del sistema. La huella cubre todo el agregado sin migración y sin columna.La huella incluye el stock, y ese es el punto
El caso peligroso no son dos operadores: es una venta. Si un webhook descuenta 5 unidades mientras el modal está abierto, guardar la cantidad absoluta que el usuario vio borraría ese descuento sin dejar rastro. Hay un spec exactamente para eso: el stock se mueve, nadie edita nada, y la escritura se rechaza igual.
Dónde se verifica
Dentro de la transacción y detrás de un
lock!—SELECT ... FOR UPDATEsobre la fila del producto. Comparar afuera dejaría pasar a dos requests que leyeron la misma versión, que es la carrera que esto cierra. Con la fila tomada, el segundo espera, relee lo que dejó el primero y su versión ya no coincide.La card pedía 409 "con un cuerpo que permita distinguirlo de los otros dos 409". Se devuelve 412 Precondition Failed: es el código que HTTP define para una precondición incumplida, y resuelve solo el requisito de la card — este endpoint ya devuelve 409 por SKU duplicado y por lock de stock ocupado, y un tercer 409 obligaría al front a leer el cuerpo para saber cuál es. Con 412 alcanza el status.
🛠️ Cambios realizados
Catalog::ProductVersion: SHA-256 sobre los campos que el modal edita máswarehouse_id:quantityde cada depósito, ordenados — sin ordenar, la misma fila daría huellas distintas entre requests y el guardado fallaría al azar.Catalog::StaleProductError, que lleva la versión vigente para que el cliente pueda recargar sin pedir el detalle de nuevo.Products::UpdateProduct:expected_versionopcional, verificado traslock!dentro de la transacción.ProductsController:ETagenshowy en la respuesta delupdate, lectura deIf-Match(tolera comillas,W/y*), y412.config/initializers/cors.rb:expose: %w[ETag]— ver abajo.El bug que apareció cableando el frontend
Sin
expose, CORS le oculta el ETag al JavaScript: sólo deja leer los headers simples salvo que el servidor los liste. El front corre en otro origen (5173 contra 3000), así queresponse.headers.etagllegabaundefined, el modal no mandabaIf-Matchy el locking optimista quedaba desactivado sin que nada fallara a la vista.No lo encontré leyendo el diff sino armando la otra mitad. Es el tipo de cosa que hace que "está implementado" y "funciona" no sean lo mismo.
🧪 Cómo probar
GET /api/v1/products/:id→ headerETagPUTcon eseIf-Match→ 200, y unETagnuevo en la respuestaPUTcon elIf-Matchviejo → 412, y el producto sin tocarStock#update!) y guardar con la versión previa → 412PUTsinIf-Match→ 200, como antes📸 Evidencia
N/A (backend).
bundle exec rspec→ 585 examples, 0 failures (17 nuevos)rubocop --force-exclusion→ 145 archivos, sin ofensasbrakeman -q→ No warnings foundPUTsinIf-Matchsigue funcionando igual. Es la semántica de HTTP y no rompe a ningún cliente.proyecto-webes el que hace que el modal la use.showy porupdate.