diff --git a/.gitignore b/.gitignore index 7868101a..f80656a9 100644 --- a/.gitignore +++ b/.gitignore @@ -31,3 +31,6 @@ apps/api/uploads/ # Entorno local personal (scripts, seeds, docker extra) — no subir al repo .local/ + +# Local git worktrees for parallel agent work — never commit +.worktrees/ diff --git a/apps/api/drizzle/0055_resource_inventory_version.sql b/apps/api/drizzle/0055_resource_inventory_version.sql new file mode 100644 index 00000000..68b8c814 --- /dev/null +++ b/apps/api/drizzle/0055_resource_inventory_version.sql @@ -0,0 +1,11 @@ +-- #294: PUT /resources/:id/inventory replaced the declared inventory with an +-- unconditional delete+reinsert (no concurrency control), so a line merged +-- concurrently by POST /resources/:id/inventory-entries or the donation intake +-- worker (both via Resource.receiveInventory) between the owner's form load +-- and their save was silently discarded (lost update). This counter backs an +-- optimistic-concurrency check: the owner's PUT must send back the version it +-- read (`expectedVersion`), bumped on every inventory change; a mismatch means +-- someone else changed the inventory in the meantime and the write is +-- rejected with 409 instead of overwriting it. +ALTER TABLE "resources" + ADD COLUMN IF NOT EXISTS "inventory_version" integer NOT NULL DEFAULT 0; diff --git a/apps/api/openapi.json b/apps/api/openapi.json index 6b4cb567..e92e816b 100644 --- a/apps/api/openapi.json +++ b/apps/api/openapi.json @@ -1124,6 +1124,7 @@ }, "/resources/{resourceId}/inventory": { "get": { + "description": "Returns the declared lines plus the optimistic-concurrency `version` (#294) — send it back as `expectedVersion` on the PUT below.", "operationId": "ResourcesController_getMyInventoryAction", "parameters": [ { @@ -1143,10 +1144,7 @@ "content": { "application/json": { "schema": { - "type": "array", - "items": { - "$ref": "#/components/schemas/SupplyLineResponseDto" - } + "$ref": "#/components/schemas/InventoryViewDto" } } } @@ -1172,6 +1170,7 @@ ] }, "put": { + "description": "Optimistic concurrency (#294): `expectedVersion` must match the current `version` (read from GET) or the request fails with 409 — someone else changed the inventory (inventory-entries, a donation) since it was loaded.", "operationId": "ResourcesController_updateMyInventoryAction", "parameters": [ { @@ -1210,6 +1209,9 @@ }, "404": { "description": "Resource not found" + }, + "409": { + "description": "expectedVersion is stale — the inventory changed since it was loaded (#294)" } }, "security": [ @@ -10212,6 +10214,26 @@ "category" ] }, + "InventoryViewDto": { + "type": "object", + "properties": { + "items": { + "type": "array", + "items": { + "$ref": "#/components/schemas/SupplyLineResponseDto" + } + }, + "version": { + "type": "number", + "example": 3, + "description": "Optimistic-concurrency version of the declared inventory. Pass back as `expectedVersion` on PUT; a mismatch means it changed since this read (#294)." + } + }, + "required": [ + "items", + "version" + ] + }, "UpdateInventoryDto": { "type": "object", "properties": { @@ -10221,10 +10243,16 @@ "items": { "$ref": "#/components/schemas/SupplyLineDto" } + }, + "expectedVersion": { + "type": "number", + "example": 3, + "description": "The inventory `version` read from GET /resources/:id/inventory. Rejected with 409 if it no longer matches the current version (#294)." } }, "required": [ - "items" + "items", + "expectedVersion" ] }, "VerifyResourceDto": { diff --git a/apps/api/src/contexts/resources/application/get-my-inventory.spec.ts b/apps/api/src/contexts/resources/application/get-my-inventory.spec.ts index 27642e98..c7069d7f 100644 --- a/apps/api/src/contexts/resources/application/get-my-inventory.spec.ts +++ b/apps/api/src/contexts/resources/application/get-my-inventory.spec.ts @@ -59,13 +59,28 @@ describe('GetMyInventory', () => { const bus = new FakeEventBus(); const id = await makeResource(repo, bus); - const lines = await new GetMyInventory(repo, noMembership).execute({ + const { items } = await new GetMyInventory(repo, noMembership).execute({ resourceId: id, requesterUserId: OWNER_ID, }); - expect(lines).toHaveLength(1); - expect(lines[0]).toMatchObject({ name: 'Agua', quantity: 10, unit: 'l' }); + expect(items).toHaveLength(1); + expect(items[0]).toMatchObject({ name: 'Agua', quantity: 10, unit: 'l' }); + }); + + // #294: the caller must be able to send this back as `expectedVersion` on + // PUT /resources/:id/inventory for the optimistic-concurrency check. + it('reports the current inventoryVersion, starting at 0 for a freshly registered resource', async () => { + const repo = new InMemoryResourceRepository(); + const bus = new FakeEventBus(); + const id = await makeResource(repo, bus); + + const { version } = await new GetMyInventory(repo, noMembership).execute({ + resourceId: id, + requesterUserId: OWNER_ID, + }); + + expect(version).toBe(0); }); it('coordinator (not owner) can read', async () => { @@ -73,12 +88,15 @@ describe('GetMyInventory', () => { const bus = new FakeEventBus(); const id = await makeResource(repo, bus); - const lines = await new GetMyInventory(repo, coordOnlyMembership).execute({ + const { items } = await new GetMyInventory( + repo, + coordOnlyMembership, + ).execute({ resourceId: id, requesterUserId: COORD_ID, }); - expect(lines).toHaveLength(1); + expect(items).toHaveLength(1); }); it('point manager (entity-scoped grant, not owner/coordinator) can read (#316)', async () => { @@ -86,7 +104,7 @@ describe('GetMyInventory', () => { const bus = new FakeEventBus(); const id = await makeResource(repo, bus); - const lines = await new GetMyInventory(repo, noMembership).execute({ + const { items } = await new GetMyInventory(repo, noMembership).execute({ resourceId: id, requesterUserId: MANAGER_ID, grants: [ @@ -98,7 +116,7 @@ describe('GetMyInventory', () => { ], }); - expect(lines).toHaveLength(1); + expect(items).toHaveLength(1); }); it('third party → UnauthorizedInventoryChangeError', async () => { diff --git a/apps/api/src/contexts/resources/application/get-my-inventory.ts b/apps/api/src/contexts/resources/application/get-my-inventory.ts index a7d5852a..1fe20cd4 100644 --- a/apps/api/src/contexts/resources/application/get-my-inventory.ts +++ b/apps/api/src/contexts/resources/application/get-my-inventory.ts @@ -12,13 +12,24 @@ export interface GetMyInventoryQuery { grants?: PrincipalGrant[]; } +export interface GetMyInventoryResult { + items: SupplyLineSnapshot[]; + /** + * Optimistic-concurrency version of the declared inventory (#294). The + * caller must send it back as `expectedVersion` on + * `PUT /resources/:id/inventory`; a mismatch there means the inventory + * changed since this read. + */ + version: number; +} + export class GetMyInventory { constructor( private readonly repo: ResourceRepository, private readonly membershipReader: ResourceMembershipReader, ) {} - async execute(q: GetMyInventoryQuery): Promise { + async execute(q: GetMyInventoryQuery): Promise { const resource = await loadResourceForManagement({ repo: this.repo, membershipReader: this.membershipReader, @@ -28,6 +39,9 @@ export class GetMyInventory { makeForbidden: () => new UnauthorizedInventoryChangeError(), }); - return resource.items.map((i) => i.toSnapshot()); + return { + items: resource.items.map((i) => i.toSnapshot()), + version: resource.inventoryVersion, + }; } } diff --git a/apps/api/src/contexts/resources/application/inventory-version-conflict.error.ts b/apps/api/src/contexts/resources/application/inventory-version-conflict.error.ts new file mode 100644 index 00000000..7e0df34d --- /dev/null +++ b/apps/api/src/contexts/resources/application/inventory-version-conflict.error.ts @@ -0,0 +1,16 @@ +/** + * Optimistic-concurrency conflict on `PUT /resources/:id/inventory` (#294): + * the caller's `expectedVersion` no longer matches the resource's current + * `inventoryVersion` — someone else (an inventory-entry, the donation intake + * worker, or another PUT) changed the declared inventory since the caller + * loaded it. Mapped to 409 so the client reloads and retries instead of + * silently overwriting the concurrent change. + */ +export class InventoryVersionConflictError extends Error { + constructor() { + super( + 'The inventory has changed since it was loaded; reload it and try again', + ); + this.name = 'InventoryVersionConflictError'; + } +} diff --git a/apps/api/src/contexts/resources/application/update-my-inventory.spec.ts b/apps/api/src/contexts/resources/application/update-my-inventory.spec.ts index a5ff36f1..05544b2d 100644 --- a/apps/api/src/contexts/resources/application/update-my-inventory.spec.ts +++ b/apps/api/src/contexts/resources/application/update-my-inventory.spec.ts @@ -7,10 +7,14 @@ import { ResourceType } from '../domain/resource-enums'; import { ResourceEmergencyStatusReader } from '../domain/ports/emergency-status-reader'; import { ResourceMembershipReader } from '../domain/ports/membership-reader'; import { Category } from '../../supplies/domain/category'; -import { SupplyLineProps } from '../../supplies/domain/supply-line'; +import { + SupplyLine, + SupplyLineProps, + SupplyLineValidationError, +} from '../../supplies/domain/supply-line'; import { ResourceNotFoundError } from './resource-not-found.error'; import { UnauthorizedInventoryChangeError } from './unauthorized-inventory-change.error'; -import { SupplyLineValidationError } from '../../supplies/domain/supply-line'; +import { InventoryVersionConflictError } from './inventory-version-conflict.error'; const EM = '11111111-1111-4111-8111-111111111111'; const OWNER_ID = 'owner-user-0000-0000-000000000000'; @@ -66,6 +70,7 @@ describe('UpdateMyInventory', () => { resourceId: id, requesterUserId: OWNER_ID, lines: [line('Arroz', 3)], + expectedVersion: 0, }); const found = await repo.findById(ResourceId.fromString(id)); @@ -81,6 +86,7 @@ describe('UpdateMyInventory', () => { resourceId: id, requesterUserId: COORD_ID, lines: [line('Mantas', 5)], + expectedVersion: 0, }); const found = await repo.findById(ResourceId.fromString(id)); @@ -96,6 +102,7 @@ describe('UpdateMyInventory', () => { resourceId: id, requesterUserId: MANAGER_ID, lines: [line('Kits', 7)], + expectedVersion: 0, grants: [ { roleId: 'point_manager', @@ -126,6 +133,7 @@ describe('UpdateMyInventory', () => { resourceId: id, requesterUserId: OWNER_ID, lines: [line('Arroz', 3)], + expectedVersion: 0, }); expect(membershipQueried).toBe(false); @@ -140,12 +148,78 @@ describe('UpdateMyInventory', () => { resourceId: id, requesterUserId: OWNER_ID, lines: [], + expectedVersion: 0, }); const found = await repo.findById(ResourceId.fromString(id)); expect(found?.items).toHaveLength(0); }); + it('a successful replace advances inventoryVersion, so a stale caller is rejected next time (#294)', async () => { + const repo = new InMemoryResourceRepository(); + const bus = new FakeEventBus(); + const id = await makeResource(repo, bus, [line('Agua', 10)]); + + await new UpdateMyInventory(repo, noMembership).execute({ + resourceId: id, + requesterUserId: OWNER_ID, + lines: [line('Arroz', 3)], + expectedVersion: 0, + }); + + const found = await repo.findById(ResourceId.fromString(id)); + expect(found?.inventoryVersion).toBe(1); + + // Retrying with the now-stale version (0) must fail, not overwrite again. + await expect( + new UpdateMyInventory(repo, noMembership).execute({ + resourceId: id, + requesterUserId: OWNER_ID, + lines: [line('Mantas', 1)], + expectedVersion: 0, + }), + ).rejects.toBeInstanceOf(InventoryVersionConflictError); + }); + + it('#294: a concurrent merge (receiveInventory) between the form load and the save is NOT silently discarded', async () => { + // Reproduces the reported lost-update scenario: the owner opens the + // inventory edit form (reads version 0, seeded with line A); while the + // form is open, an operator/worker merges in line B via + // POST /resources/:id/inventory-entries (Resource.receiveInventory), + // bumping the version to 1. The owner then saves the form unchanged + // (still carrying expectedVersion 0) — the PUT must reject the stale + // write with a conflict instead of overwriting B away. + const repo = new InMemoryResourceRepository(); + const bus = new FakeEventBus(); + const id = await makeResource(repo, bus, [line('Agua', 10)]); + const loadedVersion = 0; + + // Concurrent merge lands first (operator records a manual entry). + const resource = await repo.findById(ResourceId.fromString(id)); + resource!.receiveInventory([SupplyLine.create(line('Mantas', 5))]); + await repo.save(resource!); + + const beforeOverwrite = await repo.findById(ResourceId.fromString(id)); + expect(beforeOverwrite?.items.map((i) => i.name).sort()).toEqual([ + 'Agua', + 'Mantas', + ]); + + // Owner's stale form save (still thinks the version is 0) must be rejected. + await expect( + new UpdateMyInventory(repo, noMembership).execute({ + resourceId: id, + requesterUserId: OWNER_ID, + lines: [line('Agua', 10)], + expectedVersion: loadedVersion, + }), + ).rejects.toBeInstanceOf(InventoryVersionConflictError); + + // Mantas must still be there — the concurrent merge was NOT lost. + const after = await repo.findById(ResourceId.fromString(id)); + expect(after?.items.map((i) => i.name).sort()).toEqual(['Agua', 'Mantas']); + }); + it('third party (not owner, not coordinator) → UnauthorizedInventoryChangeError', async () => { const repo = new InMemoryResourceRepository(); const bus = new FakeEventBus(); @@ -156,6 +230,7 @@ describe('UpdateMyInventory', () => { resourceId: id, requesterUserId: THIRD_ID, lines: [line('Agua', 1)], + expectedVersion: 0, }), ).rejects.toBeInstanceOf(UnauthorizedInventoryChangeError); }); @@ -168,6 +243,7 @@ describe('UpdateMyInventory', () => { resourceId: '99999999-9999-4999-8999-999999999999', requesterUserId: OWNER_ID, lines: [], + expectedVersion: 0, }), ).rejects.toBeInstanceOf(ResourceNotFoundError); }); @@ -182,6 +258,7 @@ describe('UpdateMyInventory', () => { resourceId: id, requesterUserId: OWNER_ID, lines: [{ ...line('Agua', 0) }], + expectedVersion: 0, }), ).rejects.toBeInstanceOf(SupplyLineValidationError); }); diff --git a/apps/api/src/contexts/resources/application/update-my-inventory.ts b/apps/api/src/contexts/resources/application/update-my-inventory.ts index cf0d1ea8..97794b36 100644 --- a/apps/api/src/contexts/resources/application/update-my-inventory.ts +++ b/apps/api/src/contexts/resources/application/update-my-inventory.ts @@ -2,6 +2,7 @@ import { ResourceRepository } from '../domain/ports/resource.repository'; import { ResourceMembershipReader } from '../domain/ports/membership-reader'; import { SupplyLine, SupplyLineProps } from '../../supplies/domain/supply-line'; import { UnauthorizedInventoryChangeError } from './unauthorized-inventory-change.error'; +import { InventoryVersionConflictError } from './inventory-version-conflict.error'; import { loadResourceForManagement } from './load-resource-for-management'; import { PrincipalGrant } from './principal-grant'; @@ -9,6 +10,13 @@ export interface UpdateMyInventoryCommand { resourceId: string; requesterUserId: string; lines: SupplyLineProps[]; + /** + * The `inventoryVersion` the caller read via `GET /resources/:id/inventory` + * (#294). Must still match the persisted version or the write is rejected + * with `InventoryVersionConflictError` (409) instead of silently overwriting + * a concurrent merge (inventory-entries #9, donation events #129). + */ + expectedVersion: number; /** Request grants, so an entity-scoped point manager is authorized (#316). */ grants?: PrincipalGrant[]; } @@ -29,12 +37,21 @@ export class UpdateMyInventory { makeForbidden: () => new UnauthorizedInventoryChangeError(), }); - // Last-write-wins: the whole snapshot replaces the persisted lines with no - // version check, so a line merged concurrently via receiveInventory - // (intake entries #9, donation events #129) between the owner's form load - // and this save is overwritten. Accepted for now — revisit with an - // optimistic version/If-Match if it bites in the field. + // Fail fast on the version the caller actually read — avoids a wasted + // write attempt in the (dominant) single-writer case. The repository still + // re-checks atomically at the storage level below: a concurrent writer + // could otherwise slip in between this check and the write itself. + if (resource.inventoryVersion !== cmd.expectedVersion) { + throw new InventoryVersionConflictError(); + } + resource.replaceInventory(cmd.lines.map((l) => SupplyLine.create(l))); - await this.repo.save(resource); + const applied = await this.repo.saveIfInventoryVersionMatches( + resource, + cmd.expectedVersion, + ); + if (!applied) { + throw new InventoryVersionConflictError(); + } } } diff --git a/apps/api/src/contexts/resources/domain/ports/resource.repository.ts b/apps/api/src/contexts/resources/domain/ports/resource.repository.ts index e6bfd6f8..4150e307 100644 --- a/apps/api/src/contexts/resources/domain/ports/resource.repository.ts +++ b/apps/api/src/contexts/resources/domain/ports/resource.repository.ts @@ -41,6 +41,23 @@ export interface ManagedResourceRow { export interface ResourceRepository { save(resource: Resource): Promise; + /** + * Persist a resource whose declared inventory just changed, but ONLY if the + * resource's `inventoryVersion` in storage still equals `expectedVersion` + * (the optimistic-concurrency guard for `PUT /resources/:id/inventory`, + * #294). The caller has already advanced `resource.inventoryVersion` past + * `expectedVersion` (via `replaceInventory`); this must check-and-set the + * version atomically at the storage level — a plain read-then-write from the + * application layer would still race with a concurrent writer between the + * two steps. Returns `false` (nothing written) when the version no longer + * matches — a concurrent merge (receiveInventory) or another PUT already + * changed the inventory since the caller read it; `true` when the write + * committed. + */ + saveIfInventoryVersionMatches( + resource: Resource, + expectedVersion: number, + ): Promise; findById(id: ResourceId): Promise; findPendingByEmergency(emergencyId: EmergencyId): Promise; /** diff --git a/apps/api/src/contexts/resources/domain/resource-receive-inventory.spec.ts b/apps/api/src/contexts/resources/domain/resource-receive-inventory.spec.ts index 7c592bdd..e09b3eaa 100644 --- a/apps/api/src/contexts/resources/domain/resource-receive-inventory.spec.ts +++ b/apps/api/src/contexts/resources/domain/resource-receive-inventory.spec.ts @@ -138,6 +138,19 @@ describe('Resource.receiveInventory', () => { expect(r.items[0].quantity).toBe(10); }); + // #294: a concurrent merge must advance inventoryVersion so a stale + // PUT /resources/:id/inventory overwrite (loaded before the merge) is + // detected instead of silently discarding it. A no-op merge (empty list) + // must NOT bump the version — nothing actually changed. + it('bumps inventoryVersion when it merges lines, but not on a no-op', () => { + const r = make([line('Agua', 10, 'l')]); + expect(r.inventoryVersion).toBe(0); + r.receiveInventory([]); + expect(r.inventoryVersion).toBe(0); + r.receiveInventory([line('Agua', 5, 'l')]); + expect(r.inventoryVersion).toBe(1); + }); + it('adds to an empty inventory', () => { const r = make(); r.receiveInventory([line('Mantas', 20, 'unidades', Category.Shelter)]); diff --git a/apps/api/src/contexts/resources/domain/resource-replace-inventory.spec.ts b/apps/api/src/contexts/resources/domain/resource-replace-inventory.spec.ts index 859df323..ed4313bc 100644 --- a/apps/api/src/contexts/resources/domain/resource-replace-inventory.spec.ts +++ b/apps/api/src/contexts/resources/domain/resource-replace-inventory.spec.ts @@ -42,4 +42,15 @@ describe('Resource.replaceInventory', () => { quantity: 5, }); }); + + // #294: PUT /resources/:id/inventory needs a counter to detect a concurrent + // merge (receiveInventory) that happened between the form load and the save. + it('bumps inventoryVersion on every replace', () => { + const r = make([line('Agua', 10)]); + expect(r.inventoryVersion).toBe(0); + r.replaceInventory([line('Arroz', 3)]); + expect(r.inventoryVersion).toBe(1); + r.replaceInventory([]); + expect(r.inventoryVersion).toBe(2); + }); }); diff --git a/apps/api/src/contexts/resources/domain/resource.ts b/apps/api/src/contexts/resources/domain/resource.ts index 49ba9a4d..0d949566 100644 --- a/apps/api/src/contexts/resources/domain/resource.ts +++ b/apps/api/src/contexts/resources/domain/resource.ts @@ -99,6 +99,12 @@ export interface ResourceSnapshot { disputeDismissedAt?: Date | null; /** Optional (legacy-safe) restricted author attribution (#235). */ author?: AuthorSnapshot | null; + /** + * Optimistic-concurrency counter for the declared inventory (#294): bumped + * every time `replaceInventory`/`receiveInventory` change `items`. Optional + * (defaults to 0) so existing snapshot literals in tests don't need updating. + */ + inventoryVersion?: number; } /** @@ -137,6 +143,7 @@ export class Resource { private _disputedAt: Date | null, private _disputeDismissedAt: Date | null, public readonly author: Author | null, + private _inventoryVersion: number, ) {} static register(props: RegisterResourceProps): Resource { @@ -166,6 +173,7 @@ export class Resource { null, null, props.author ?? null, + 0, ); r.events.push( new ResourceRegistered(r.id.value, { @@ -204,6 +212,7 @@ export class Resource { s.disputedAt ?? null, s.disputeDismissedAt ?? null, s.author ? Author.fromSnapshot(s.author) : null, + s.inventoryVersion ?? 0, ); } @@ -238,6 +247,18 @@ export class Resource { get items(): SupplyLine[] { return this._items; } + /** + * Optimistic-concurrency counter for the declared inventory (#294): bumped by + * every `replaceInventory`/`receiveInventory` call that actually changes + * `items`. `PUT /resources/:id/inventory` must send back the version it read + * (`expectedVersion`) — a mismatch means a concurrent writer (the incremental + * merge endpoint or the donation worker) already changed the inventory, and + * the full-snapshot overwrite must be rejected instead of silently + * discarding that change. + */ + get inventoryVersion(): number { + return this._inventoryVersion; + } verify(level: VerificationLevel, coordinatorId: string): void { if (level === VerificationLevel.Unverified) { @@ -357,12 +378,14 @@ export class Resource { ); } this._items = [...byKey.values()]; + this._inventoryVersion++; } /** Owner/coordinator overwrites the declared inventory (#263). Replaces, not * merges — unlike receiveInventory (donation/intake). Empty list clears it. */ replaceInventory(lines: SupplyLine[]): void { this._items = [...lines]; + this._inventoryVersion++; } /** @@ -469,6 +492,7 @@ export class Resource { disputedAt: this._disputedAt, disputeDismissedAt: this._disputeDismissedAt, author: this.author ? this.author.toSnapshot() : null, + inventoryVersion: this._inventoryVersion, }; } diff --git a/apps/api/src/contexts/resources/infrastructure/drizzle/drizzle-resource.repository.int-spec.ts b/apps/api/src/contexts/resources/infrastructure/drizzle/drizzle-resource.repository.int-spec.ts index 08d9a6c6..c3cf6153 100644 --- a/apps/api/src/contexts/resources/infrastructure/drizzle/drizzle-resource.repository.int-spec.ts +++ b/apps/api/src/contexts/resources/infrastructure/drizzle/drizzle-resource.repository.int-spec.ts @@ -170,6 +170,112 @@ describe('DrizzleResourceRepository (integration)', () => { ]); }); + // #294: PUT /resources/:id/inventory must be rejected — not silently applied + // — when the caller's version is stale. These prove the guard is enforced + // atomically by Postgres itself (a single conditional UPDATE), not just by + // the application-layer pre-check, which is what actually closes the race + // between two concurrent writers. + describe('saveIfInventoryVersionMatches (#294)', () => { + it('persists items and advances inventoryVersion when the expected version matches', async () => { + const id = ResourceId.create(); + const r = Resource.register({ + id, + emergencyId: EmergencyId.fromString(EM), + type: ResourceType.Warehouse, + name: 'Almacén versión', + location: baseLocation, + ownerUserId: OWNER_ID, + items: [ + SupplyLine.create({ + name: 'Agua', + quantity: 10, + unit: 'l', + category: Category.Water, + }), + ], + }); + await repo.save(r); + expect(r.inventoryVersion).toBe(0); + + const loaded = await repo.findById(id); + loaded!.replaceInventory([ + SupplyLine.create({ + name: 'Arroz', + quantity: 3, + unit: 'kg', + category: Category.Food, + }), + ]); + const applied = await repo.saveIfInventoryVersionMatches(loaded!, 0); + expect(applied).toBe(true); + + const found = await repo.findById(id); + expect(found?.items.map((i) => i.name)).toEqual(['Arroz']); + expect(found?.inventoryVersion).toBe(1); + }); + + it('rejects the write and leaves storage untouched when the expected version is stale', async () => { + const id = ResourceId.create(); + const r = Resource.register({ + id, + emergencyId: EmergencyId.fromString(EM), + type: ResourceType.Warehouse, + name: 'Almacén versión conflicto', + location: baseLocation, + ownerUserId: OWNER_ID, + items: [ + SupplyLine.create({ + name: 'Agua', + quantity: 10, + unit: 'l', + category: Category.Water, + }), + ], + }); + await repo.save(r); + + // Simulates a concurrent merge (receiveInventory) landing first: it uses + // the plain save(), so it always succeeds and bumps the version. + const concurrent = await repo.findById(id); + concurrent!.receiveInventory([ + SupplyLine.create({ + name: 'Mantas', + quantity: 5, + unit: 'unidades', + category: Category.Shelter, + }), + ]); + await repo.save(concurrent!); + + // The owner's form was loaded BEFORE the concurrent merge, so it still + // carries the stale version (0). Rebuild that exact in-process shape. + const current = await repo.findById(id); + const staleForm = Resource.fromSnapshot({ + ...current!.toSnapshot(), + inventoryVersion: 0, + }); + staleForm.replaceInventory([ + SupplyLine.create({ + name: 'Agua', + quantity: 10, + unit: 'l', + category: Category.Water, + }), + ]); + + const applied = await repo.saveIfInventoryVersionMatches(staleForm, 0); + expect(applied).toBe(false); + + // The concurrent merge must survive untouched — Mantas is still there. + const found = await repo.findById(id); + expect(found?.items.map((i) => i.name).sort()).toEqual([ + 'Agua', + 'Mantas', + ]); + expect(found?.inventoryVersion).toBe(1); + }); + }); + it('round-trips resource with description and ownerOrganizationId', async () => { const r = Resource.register({ id: ResourceId.create(), diff --git a/apps/api/src/contexts/resources/infrastructure/drizzle/drizzle-resource.repository.ts b/apps/api/src/contexts/resources/infrastructure/drizzle/drizzle-resource.repository.ts index e475d237..386e0937 100644 --- a/apps/api/src/contexts/resources/infrastructure/drizzle/drizzle-resource.repository.ts +++ b/apps/api/src/contexts/resources/infrastructure/drizzle/drizzle-resource.repository.ts @@ -83,6 +83,7 @@ type RawRow = { disputed_at: unknown; dispute_dismissed_at: unknown; author?: unknown; + inventory_version?: unknown; }; /** @@ -161,6 +162,7 @@ function rawRowToSnapshot(row: RawRow): ResourceSnapshot { // Raw SQL paths (nearby) power the map, which does not render inventory — // items are intentionally not hydrated here to keep the payload lean. items: [], + inventoryVersion: Number(row.inventory_version ?? 0), }; } @@ -207,6 +209,7 @@ function rowToSnapshot(row: Row, items: ItemsRow[] = []): ResourceSnapshot { disputeDismissedAt: row.disputeDismissedAt ?? null, author: row.author ?? null, items: itemsToSnapshots(items), + inventoryVersion: row.inventoryVersion, }; } @@ -261,6 +264,7 @@ export class DrizzleResourceRepository implements ResourceRepository { disputedAt: s.disputedAt ?? null, disputeDismissedAt: s.disputeDismissedAt ?? null, author: s.author ?? null, + inventoryVersion: s.inventoryVersion ?? 0, }) .onConflictDoUpdate({ target: resourcesTable.id, @@ -283,6 +287,7 @@ export class DrizzleResourceRepository implements ResourceRepository { disputed: s.disputed ?? false, disputedAt: s.disputedAt ?? null, disputeDismissedAt: s.disputeDismissedAt ?? null, + inventoryVersion: s.inventoryVersion ?? 0, }, }); @@ -304,6 +309,51 @@ export class DrizzleResourceRepository implements ResourceRepository { }); } + /** + * Optimistic-concurrency write for the declared inventory (#294). The + * check-and-set is a single conditional `UPDATE ... WHERE id = $1 AND + * inventory_version = $2`: Postgres row-locks the matched row for the + * duration of the statement, so two concurrent callers racing on the same + * `expectedVersion` cannot both succeed — the loser's WHERE simply matches + * zero rows once the winner commits. Only the `resources` row and its items + * are touched; unrelated resource fields are NOT overwritten here (unlike + * `save()`), since this path only ever runs after `resource.replaceInventory`. + */ + async saveIfInventoryVersionMatches( + resource: Resource, + expectedVersion: number, + ): Promise { + const s = resource.toSnapshot(); + return this.db.transaction(async (tx) => { + const updated = await tx + .update(resourcesTable) + .set({ inventoryVersion: s.inventoryVersion ?? 0 }) + .where( + and( + eq(resourcesTable.id, s.id), + eq(resourcesTable.inventoryVersion, expectedVersion), + ), + ) + .returning({ id: resourcesTable.id }); + if (updated.length === 0) return false; + + await tx + .delete(resourceItemsTable) + .where(eq(resourceItemsTable.resourceId, s.id)); + + if (s.items.length > 0) { + await tx.insert(resourceItemsTable).values( + s.items.map((item) => ({ + id: randomUUID(), + resourceId: s.id, + ...supplyLineToColumns(item), + })), + ); + } + return true; + }); + } + async findById(id: ResourceId): Promise { const rows = await this.db .select() diff --git a/apps/api/src/contexts/resources/infrastructure/drizzle/schema.ts b/apps/api/src/contexts/resources/infrastructure/drizzle/schema.ts index 5eafcbfb..189d8a49 100644 --- a/apps/api/src/contexts/resources/infrastructure/drizzle/schema.ts +++ b/apps/api/src/contexts/resources/infrastructure/drizzle/schema.ts @@ -6,6 +6,7 @@ import { doublePrecision, jsonb, boolean, + integer, } from 'drizzle-orm/pg-core'; import { supplyLineColumns } from '../../../supplies/infrastructure/drizzle/supply-line-columns'; import { suppliesTable } from '../../../supplies/infrastructure/drizzle/schema'; @@ -46,6 +47,9 @@ export const resourcesTable = pgTable('resources', { disputeDismissedAt: timestamp('dispute_dismissed_at', { withTimezone: true }), /** Restricted self-reported author attribution (#235). Never public. */ author: jsonb('author').$type(), + // Optimistic-concurrency counter for the declared inventory + // (0055_resource_inventory_version, #294). See Resource.inventoryVersion. + inventoryVersion: integer('inventory_version').notNull().default(0), }); // Reportes ciudadanos de validez de un punto (0031_resource_validity_reports): diff --git a/apps/api/src/contexts/resources/infrastructure/http/domain-exception.filter.ts b/apps/api/src/contexts/resources/infrastructure/http/domain-exception.filter.ts index 690ae866..ac44d15b 100644 --- a/apps/api/src/contexts/resources/infrastructure/http/domain-exception.filter.ts +++ b/apps/api/src/contexts/resources/infrastructure/http/domain-exception.filter.ts @@ -8,6 +8,7 @@ import { Response } from 'express'; import { ResourceNotFoundError } from '../../application/resource-not-found.error'; import { UnauthorizedStatusChangeError } from '../../application/unauthorized-status-change.error'; import { UnauthorizedInventoryChangeError } from '../../application/unauthorized-inventory-change.error'; +import { InventoryVersionConflictError } from '../../application/inventory-version-conflict.error'; import { ResourceAlreadyPublishedError, ResourceNotVerifiedError, @@ -33,6 +34,7 @@ type DomainError = | EmergencyNotAcceptingIntakeError | UnauthorizedStatusChangeError | UnauthorizedInventoryChangeError + | InventoryVersionConflictError | InvalidPublicStatusTransitionError | ResourceNotPublishedError | ResourceNotPendingError @@ -54,6 +56,7 @@ const STATUS_BY_ERROR: ReadonlyArray = [ [UnauthorizedStatusChangeError, HttpStatus.FORBIDDEN], [UnauthorizedInventoryChangeError, HttpStatus.FORBIDDEN], [OwnerCannotReportValidityError, HttpStatus.FORBIDDEN], + [InventoryVersionConflictError, HttpStatus.CONFLICT], [EmergencyNotAcceptingIntakeError, HttpStatus.CONFLICT], [ResourceAlreadyPublishedError, HttpStatus.CONFLICT], [ResourceNotVerifiedError, HttpStatus.CONFLICT], @@ -73,6 +76,7 @@ const STATUS_BY_ERROR: ReadonlyArray = [ EmergencyNotAcceptingIntakeError, UnauthorizedStatusChangeError, UnauthorizedInventoryChangeError, + InventoryVersionConflictError, InvalidPublicStatusTransitionError, ResourceNotPublishedError, ResourceNotPendingError, diff --git a/apps/api/src/contexts/resources/infrastructure/http/dto.ts b/apps/api/src/contexts/resources/infrastructure/http/dto.ts index 0498dd87..a72200e9 100644 --- a/apps/api/src/contexts/resources/infrastructure/http/dto.ts +++ b/apps/api/src/contexts/resources/infrastructure/http/dto.ts @@ -197,6 +197,12 @@ export class RecordInventoryEntryDto { * Body for the owner/coordinator declared-inventory edit (#263): the FULL set * of lines. Replaces (not merges) the resource inventory; empty list clears it, * so no @ArrayNotEmpty. + * + * `expectedVersion` is the optimistic-concurrency guard (#294): the caller + * must send back the `version` it read from `GET /resources/:id/inventory`. + * A stale value (someone merged inventory-entries or a donation into this + * point since) is rejected with 409 instead of silently overwriting the + * concurrent change. */ export class UpdateInventoryDto { @ApiProperty({ @@ -207,6 +213,16 @@ export class UpdateInventoryDto { @ValidateNested({ each: true }) @Type(() => SupplyLineDto) items!: SupplyLineDto[]; + + @ApiProperty({ + example: 3, + description: + 'The inventory `version` read from GET /resources/:id/inventory. ' + + 'Rejected with 409 if it no longer matches the current version (#294).', + }) + @IsInt() + @Min(0) + expectedVersion!: number; } export class UpdateResourcePublicStatusDto { diff --git a/apps/api/src/contexts/resources/infrastructure/http/resources.controller.ts b/apps/api/src/contexts/resources/infrastructure/http/resources.controller.ts index 7488750c..0630eea5 100644 --- a/apps/api/src/contexts/resources/infrastructure/http/resources.controller.ts +++ b/apps/api/src/contexts/resources/infrastructure/http/resources.controller.ts @@ -62,7 +62,6 @@ import { ResolveResourceDisputeDto, UpdateInventoryDto, } from './dto'; -import { SupplyLineResponseDto } from '../../../supplies/infrastructure/http/supply-line.dto'; import { toSupplyLineProps, toSupplyLineResponse, @@ -74,6 +73,7 @@ import { MyManagedResourceDto, ReportResourceValidityResponseDto, ValidityReportDto, + InventoryViewDto, } from './response.dto'; import { JwtAuthGuard, @@ -217,26 +217,29 @@ export class ResourcesController { @ApiBearerAuth() @ApiOperation({ summary: 'Read a point declared inventory in full (owner or coordinator)', + description: + 'Returns the declared lines plus the optimistic-concurrency `version` ' + + '(#294) — send it back as `expectedVersion` on the PUT below.', }) @ApiParam({ name: 'resourceId', description: 'Resource UUID', format: 'uuid', }) - @ApiOkResponse({ type: SupplyLineResponseDto, isArray: true }) + @ApiOkResponse({ type: InventoryViewDto }) @ApiNotFoundResponse({ description: 'Resource not found' }) @ApiUnauthorizedResponse({ description: 'Missing or invalid token' }) @ApiForbiddenResponse({ description: 'Not owner nor coordinator' }) async getMyInventoryAction( @Param('resourceId', ParseUUIDPipe) resourceId: string, @Req() req: Request & { user?: AuthenticatedUser }, - ): Promise { - const lines = await this.getMyInventory.execute({ + ): Promise { + const { items, version } = await this.getMyInventory.execute({ resourceId, requesterUserId: req.user!.id, grants: this.principalGrants(req.user!), }); - return lines.map(toSupplyLineResponse); + return { items: items.map(toSupplyLineResponse), version }; } @Put('resources/:resourceId/inventory') @@ -245,6 +248,10 @@ export class ResourcesController { @ApiBearerAuth() @ApiOperation({ summary: 'Replace a point declared inventory (owner or coordinator) — #263', + description: + 'Optimistic concurrency (#294): `expectedVersion` must match the current ' + + '`version` (read from GET) or the request fails with 409 — someone else ' + + 'changed the inventory (inventory-entries, a donation) since it was loaded.', }) @ApiParam({ name: 'resourceId', @@ -256,6 +263,10 @@ export class ResourcesController { @ApiBadRequestResponse({ description: 'Invalid request body or UUID' }) @ApiUnauthorizedResponse({ description: 'Missing or invalid token' }) @ApiForbiddenResponse({ description: 'Not owner nor coordinator' }) + @ApiConflictResponse({ + description: + 'expectedVersion is stale — the inventory changed since it was loaded (#294)', + }) async updateMyInventoryAction( @Param('resourceId', ParseUUIDPipe) resourceId: string, @Body() dto: UpdateInventoryDto, @@ -265,6 +276,7 @@ export class ResourcesController { resourceId, requesterUserId: req.user!.id, lines: dto.items.map(toSupplyLineProps), + expectedVersion: dto.expectedVersion, grants: this.principalGrants(req.user!), }); } diff --git a/apps/api/src/contexts/resources/infrastructure/http/response.dto.ts b/apps/api/src/contexts/resources/infrastructure/http/response.dto.ts index f759254f..7da81e48 100644 --- a/apps/api/src/contexts/resources/infrastructure/http/response.dto.ts +++ b/apps/api/src/contexts/resources/infrastructure/http/response.dto.ts @@ -9,6 +9,7 @@ import { ValidityReason, ValidityReportStatus, } from '../../domain/resource-validity-report'; +import { SupplyLineResponseDto } from '../../../supplies/infrastructure/http/supply-line.dto'; export class RegisterResourceResponseDto { @ApiProperty({ @@ -47,6 +48,26 @@ export class MyManagedResourceDto { emergencySlug!: string | null; } +/** + * Response for `GET /resources/:id/inventory` (#294): the declared lines plus + * the optimistic-concurrency `version`, which the client must send back as + * `expectedVersion` on `PUT /resources/:id/inventory` — a mismatch there means + * the inventory changed (inventory-entries #9, donation events #129) since + * this read, and the write is rejected with 409 instead of overwriting it. + */ +export class InventoryViewDto { + @ApiProperty({ type: [SupplyLineResponseDto] }) + items!: SupplyLineResponseDto[]; + + @ApiProperty({ + example: 3, + description: + 'Optimistic-concurrency version of the declared inventory. Pass back as ' + + '`expectedVersion` on PUT; a mismatch means it changed since this read (#294).', + }) + version!: number; +} + export class LocationViewDto { @ApiProperty({ example: 'Calle Mayor 1, Valencia' }) address!: string; diff --git a/apps/api/src/contexts/resources/infrastructure/in-memory-resource.repository.ts b/apps/api/src/contexts/resources/infrastructure/in-memory-resource.repository.ts index 88032c15..0580f362 100644 --- a/apps/api/src/contexts/resources/infrastructure/in-memory-resource.repository.ts +++ b/apps/api/src/contexts/resources/infrastructure/in-memory-resource.repository.ts @@ -46,6 +46,21 @@ export class InMemoryResourceRepository implements ResourceRepository { return Promise.resolve(); } + /** In-memory mirror of the Drizzle conditional UPDATE (#294): a single + * synchronous check-then-set, so it is race-free the same way a real + * `UPDATE ... WHERE inventory_version = $expected` is. */ + saveIfInventoryVersionMatches( + resource: Resource, + expectedVersion: number, + ): Promise { + const current = this.store.get(resource.id.value); + if ((current?.inventoryVersion ?? 0) !== expectedVersion) { + return Promise.resolve(false); + } + this.store.set(resource.id.value, resource.toSnapshot()); + return Promise.resolve(true); + } + findById(id: ResourceId): Promise { const snap = this.store.get(id.value); return Promise.resolve(snap ? Resource.fromSnapshot(snap) : null); diff --git a/apps/api/test/resources-manage-grant.e2e-spec.ts b/apps/api/test/resources-manage-grant.e2e-spec.ts index 280e6309..a1232671 100644 --- a/apps/api/test/resources-manage-grant.e2e-spec.ts +++ b/apps/api/test/resources-manage-grant.e2e-spec.ts @@ -238,17 +238,27 @@ describe('Resource management by entity-scoped grant (e2e, #316)', () => { .set('Authorization', `Bearer ${managerToken}`) .expect(200); - const lines = res.body as Line[]; - expect(lines).toHaveLength(1); - expect(lines[0]).toMatchObject({ name: 'Agua', quantity: 10 }); + const body = res.body as { items: Line[]; version: number }; + expect(body.items).toHaveLength(1); + expect(body.items[0]).toMatchObject({ name: 'Agua', quantity: 10 }); + // Freshly registered point (#294): the optimistic-concurrency counter + // starts at 0 and the PUT below must send it back as expectedVersion. + expect(body.version).toBe(0); }); it('the manager replaces the declared inventory of their point', async () => { + const before = await request(server) + .get(`/resources/${managedResourceId}/inventory`) + .set('Authorization', `Bearer ${managerToken}`) + .expect(200); + const { version } = before.body as { items: Line[]; version: number }; + await request(server) .put(`/resources/${managedResourceId}/inventory`) .set('Authorization', `Bearer ${managerToken}`) .send({ items: [{ name: 'Arroz', quantity: 3, unit: 'kg', category: 'food' }], + expectedVersion: version, }) .expect(204); @@ -256,8 +266,18 @@ describe('Resource management by entity-scoped grant (e2e, #316)', () => { .get(`/resources/${managedResourceId}/inventory`) .set('Authorization', `Bearer ${managerToken}`) .expect(200); - const lines = res.body as Line[]; - expect(lines.map((l) => l.name)).toEqual(['Arroz']); + const body = res.body as { items: Line[]; version: number }; + expect(body.items.map((l) => l.name)).toEqual(['Arroz']); + // The replace advanced the version — reusing the old one must now 409 (#294). + expect(body.version).toBe(version + 1); + await request(server) + .put(`/resources/${managedResourceId}/inventory`) + .set('Authorization', `Bearer ${managerToken}`) + .send({ + items: [{ name: 'Mantas', quantity: 1, unit: 'u', category: 'other' }], + expectedVersion: version, + }) + .expect(409); }); it('the manager changes the public status of their point', async () => { @@ -294,6 +314,7 @@ describe('Resource management by entity-scoped grant (e2e, #316)', () => { .set('Authorization', `Bearer ${strangerToken}`) .send({ items: [{ name: 'Nada', quantity: 1, unit: 'u', category: 'other' }], + expectedVersion: 0, }) .expect(403); }); diff --git a/apps/web/src/app/e/[slug]/mis-puntos/[resourceId]/inventario/actions.ts b/apps/web/src/app/e/[slug]/mis-puntos/[resourceId]/inventario/actions.ts index 502d0cea..54baad33 100644 --- a/apps/web/src/app/e/[slug]/mis-puntos/[resourceId]/inventario/actions.ts +++ b/apps/web/src/app/e/[slug]/mis-puntos/[resourceId]/inventario/actions.ts @@ -9,18 +9,23 @@ import { parseSupplyLines } from '@/lib/supply-lines'; import { getT } from '@/i18n/server'; import { getCategories } from '@/adapters/get-categories'; -type SupplyLineView = components['schemas']['SupplyLineResponseDto']; +type InventoryView = components['schemas']['InventoryViewDto']; export type InventoryState = | { status: 'idle' } | { status: 'success' } | { status: 'error'; message: string }; -/** Owner/coordinator read of the point's full declared lines (null → notFound). */ +/** + * Owner/coordinator read of the point's full declared lines (null → + * notFound). Includes the optimistic-concurrency `version` (#294): the form + * carries it back as `expectedVersion` on save, so a concurrent merge + * (inventory-entries, a donation) is detected instead of silently overwritten. + */ export async function fetchMyInventory( resourceId: string, slug: string, -): Promise { +): Promise { const token = await requireSession(`/e/${slug}/mis-puntos/${resourceId}/inventario`); const { data, response } = await api.GET('/resources/{resourceId}/inventory', { @@ -71,10 +76,17 @@ export async function saveMyInventory( return { status: 'error', message: t.account.inventory_invalid_items }; } + // Optimistic-concurrency guard (#294): the hidden field carries the version + // read on page load. A missing/non-numeric value is treated as "definitely + // stale" (0 never matches a resource whose inventory has been touched more + // than once) so the API rejects it with 409 rather than the write silently + // going through with an undefined expectedVersion. + const expectedVersion = Number(formData.get('expectedVersion') ?? NaN); + const { response } = await api.PUT('/resources/{resourceId}/inventory', { params: { path: { resourceId } }, headers: authHeaders(token), - body: { items }, + body: { items, expectedVersion: Number.isFinite(expectedVersion) ? expectedVersion : 0 }, }); if (response.status === 401) { @@ -83,6 +95,14 @@ export async function saveMyInventory( if (response.status === 403) { return { status: 'error', message: t.account.inventory_update_forbidden }; } + if (response.status === 409) { + // Someone else (an inventory entry, a donation) changed the inventory + // since this form was loaded — revalidate so a page reload shows the + // fresh state, and tell the owner to reload instead of silently + // overwriting the concurrent change (#294). + revalidatePath(`/e/${slug}/mis-puntos/${resourceId}/inventario`); + return { status: 'error', message: t.account.inventory_update_conflict }; + } if (!response.ok) { return { status: 'error', message: t.account.inventory_update_failed }; } diff --git a/apps/web/src/app/e/[slug]/mis-puntos/[resourceId]/inventario/inventory-edit-form.tsx b/apps/web/src/app/e/[slug]/mis-puntos/[resourceId]/inventario/inventory-edit-form.tsx index 4358e382..61c72bec 100644 --- a/apps/web/src/app/e/[slug]/mis-puntos/[resourceId]/inventario/inventory-edit-form.tsx +++ b/apps/web/src/app/e/[slug]/mis-puntos/[resourceId]/inventario/inventory-edit-form.tsx @@ -22,6 +22,13 @@ type BoundAction = ( interface InventoryEditFormProps { action: BoundAction; initial: SupplyLineView[]; + /** + * Optimistic-concurrency version read alongside `initial` (#294). Carried as + * a hidden field and sent back as `expectedVersion` on save — the API + * rejects the write with 409 if it no longer matches (someone merged + * inventory-entries or a donation into this point in the meantime). + */ + expectedVersion: number; t: Messages['registrar']; ta: Messages['account']; locale: 'es' | 'en'; @@ -41,6 +48,7 @@ const toLine = (l: SupplyLineView): SupplyLine => ({ export function InventoryEditForm({ action, initial, + expectedVersion, t, ta, locale, @@ -53,6 +61,7 @@ export function InventoryEditForm({ return (
+ {state.status === 'success' && (

; EditResourceDto: { @@ -6974,7 +6993,7 @@ export interface operations { [name: string]: unknown; }; content: { - "application/json": components["schemas"]["SupplyLineResponseDto"][]; + "application/json": components["schemas"]["InventoryViewDto"]; }; }; /** @description Missing or invalid token */ @@ -7051,6 +7070,13 @@ export interface operations { }; content?: never; }; + /** @description expectedVersion is stale — the inventory changed since it was loaded (#294) */ + 409: { + headers: { + [name: string]: unknown; + }; + content?: never; + }; }; }; ResourcesController_verifyResource: {