refactor(core/inventory): column-scope the inventory upsert's existing-row write

apps/api/test/integration/inventory-stale-prune.int-spec.ts

@@ -159,6 +159,17 @@ describe('Inventory stale-prune (#1478)', () => {
     ]);
     await inventoryService.pruneStaleVariants(productId, [variantKeep]);
 
+    const inventoryRepo = dataSource.getRepository(InventoryItemOrmEntity);
+    const [beforeRow] = await inventoryRepo.find({
+      where: { productId, productVariantId: variantGone },
+    });
+
+    // A master timestamp deliberately far in the past. The column-scoped update
+    // (#2071) excludes `updatedAt` from its SET clause so Postgres stamps
+    // CURRENT_TIMESTAMP; if it were ever written from the item instead, this
+    // value would land in the row and poison the propagation dedupe key.
+    const staleMasterTimestamp = new Date('2020-01-01T00:00:00Z');
+
     // The gone variant reappears at the master — a fresh (live) canonical write.
     await inventoryService.setInventory(
       new InventoryItemEntity(
@@ -168,21 +179,76 @@ describe('Inventory stale-prune (#1478)', () => {
         4,
         0,
         null,
-        new Date(),
+        staleMasterTimestamp,
         false,
       ),
     );
 
-    const inventoryRepo = dataSource.getRepository(InventoryItemOrmEntity);
     const rows = await inventoryRepo.find({ where: { productId, productVariantId: variantGone } });
     expect(rows).toHaveLength(1); // no duplicate created
     expect(rows[0].isStale).toBe(false);
     expect(rows[0].availableQuantity).toBe(4);
+    // The DB stamped it, not the master.
+    // >= not >: getTime() truncates Postgres microseconds to ms, so two writes
+    // in the same millisecond would otherwise flake. The next assertion (now vs
+    // 2020) is the one that actually proves the DB stamped it.
+    expect(rows[0].updatedAt.getTime()).toBeGreaterThanOrEqual(beforeRow.updatedAt.getTime());
+    expect(rows[0].updatedAt.getTime()).toBeGreaterThan(staleMasterTimestamp.getTime());
 
     const availability = await queryService.getAvailabilityByVariantIds([variantGone]);
     expect(availability.find((a) => a.productVariantId === variantGone)?.totalAvailable).toBe(4);
   });
 
+  it('stamps updatedAt from the database on a first insert, not from the master (#2071)', async () => {
+    const dataSource = harness.getDataSource();
+    const suffix = `${Date.now()}_${Math.floor(Math.random() * 100000)}`;
+    const variantSeeded = `ol_variant_seeded_${suffix}`;
+    const variantNew = `ol_variant_new_${suffix}`;
+
+    // Seed one variant so the product exists, then write a SECOND variant that
+    // has no inventory row yet — that takes the INSERT branch, where
+    // `toOrmEntity` used to assign the master's timestamp and suppress
+    // CURRENT_TIMESTAMP exactly as the UPDATE branch did.
+    const { productId } = await seedProduct(dataSource, [
+      { variantId: variantSeeded, availableQuantity: 5 },
+    ]);
+    // The variant exists in the catalogue but has no inventory row yet.
+    const variantRepo = dataSource.getRepository(ProductVariantOrmEntity);
+    await variantRepo.save(
+      variantRepo.create({
+        id: variantNew,
+        productId,
+        sku: null,
+        attributes: null,
+        ean: null,
+        gtin: null,
+      }),
+    );
+
+    const staleMasterTimestamp = new Date('2020-01-01T00:00:00Z');
+
+    await inventoryService.setInventory(
+      new InventoryItemEntity(
+        `ignored-${suffix}`,
+        productId,
+        variantNew,
+        9,
+        0,
+        null,
+        staleMasterTimestamp,
+        false,
+      ),
+    );
+
+    const inventoryRepo = dataSource.getRepository(InventoryItemOrmEntity);
+    const [row] = await inventoryRepo.find({
+      where: { productId, productVariantId: variantNew },
+    });
+
+    expect(row.availableQuantity).toBe(9);
+    expect(row.updatedAt.getTime()).toBeGreaterThan(staleMasterTimestamp.getTime());
+  });
+
   it('marks a product-level (null-variant) row stale when the keep set omits null', async () => {
     const dataSource = harness.getDataSource();
     const suffix = `${Date.now()}_${Math.floor(Math.random() * 100000)}`;

docs/lessons.md

@@ -28,8 +28,9 @@ When a lesson hardens into a rule, **graduate it** to the canonical doc and leav
 **Context**: `order_records` carries denormalized columns that no ingestion payload supplies and that a different context pushes in with a narrow `UPDATE` - `fulfillmentState` (a rollup over the order's shipments, written by `updateFulfillmentState`) and `cancelledAt` (written by `markCancelled`). `OrderRecordRepository.upsert` is a full-object TypeORM `save()`.
 **Problem**: `toOrm` mapped `fulfillmentState` unconditionally while `persistOrder` never populated it (it is the 12th constructor argument and only 11 were passed), so every re-ingestion - a poll re-read, a webhook-triggered sync, a manual re-sync - wrote `null` over a committed `'dispatched'`. A dispatched order silently reappeared as not-shipped in the ship-by SLA buckets and the not-shipped list filter. The same class of defect had already been fixed for `cancelledAt` in the same method (#1984) and the exclusion comment sat three lines below the offending assignment.
 **Rule**: When a column has a dedicated out-of-band writer, that writer must be its **only** writer: leave the ORM entity property unset in `toOrm` so TypeORM omits the column from the generated statement, and say so in a comment next to the columns that *are* mapped. Do not "fix" it by reading the row first and carrying the value onto the new instance - an unlocked upsert racing the out-of-band UPDATE still loses a value that commits between the read and the save. Pin it with a unit test asserting the property is `undefined` on the entity handed to `save()` **and** an integration test proving the committed value survives a second persist (a mocked spec cannot prove TypeORM really omits the column). Note the consequence: the record returned by the upsert reports such a column as `null` whatever the row holds, so a caller needing its live value must re-read. Sweep the **whole method**, not just the column you came for: #2101 excluded `fulfillmentState` and left its exclusion comment sitting directly *below* two more assignments with the identical defect (`syncStatus`, `syncAttempts`), which then needed #2140 - so re-read every remaining assignment and ask which out-of-band writer owns it, and keep the exclusions in one consolidated block rather than interleaved with the assignments, or the next author drops a fresh one into the gap. Two follow-on traps #2140 surfaced: (1) an excluded **array** column needs a `?? []` guard where `toDomain` reads it, because the update path has no `RETURNING` clause and hands the property straight back `undefined` - `null`able scalars were already guarded, so #2101 never hit it; (2) for a `NOT NULL` column, omitting it makes the insert depend on the DB `DEFAULT` (TypeORM emits `DEFAULT` for an `undefined` column on Postgres), so verify that default is actually guaranteed rather than reading it off the creating migration - `1770000000000` wraps its `CREATE TABLE` in `if (!table)`, so a schema first built by `synchronize` skipped it entirely and took the column from the ORM decorator instead. Declare the `default` on the decorator (it is what a synchronize-built schema, including the int-spec harness, uses) *and* assert it on the migration-built schema with an idempotent `ALTER COLUMN ... SET DEFAULT`.
-**Applies to**: any repository whose `upsert`/`save` coexists with a narrow `UPDATE` writer on the same table - today `libs/core/src/orders/infrastructure/persistence/repositories/order-record.repository.ts` (`syncStatus`, `syncAttempts`, `fulfillmentState`, `cancelledAt`).
-**Source**: #2101 (surfaced reviewing #2050 / ADR-040, which adopts the narrow-conditional-UPDATE shape for its own columns); the `cancelledAt` precedent is #1984; #2140 closed the same defect for `syncStatus` / `syncAttempts` in the same method.
+**Two mechanisms, and which to reach for** (#2071): the exclusion above is *omission* - leave the property unset in `toOrm` and let TypeORM drop the column from a full-object `save()`. The second is *allowlisting* - replace the existing-row `save()` with an explicit `createQueryBuilder().update().set({ ...owned })`, naming only the columns this writer owns. Omission is the cheaper edit and the right default when the full-row `save()` is otherwise correct and only a column or two must be withheld. Allowlisting is worth its extra weight when the excluded set is large or load-bearing enough that "did anyone add an assignment?" needs a machine to answer: it pairs with a spec that reads `getMetadataArgsStorage().columns` for the entity and asserts every declared column falls into exactly one of an identity / owned / DB-managed group, so a newly-added column **fails the build** until someone decides who owns it - a guarantee omission cannot give, since omission is the absence of a line and nothing fails when the next author adds one. Do not "DRY" the allowlist's `.set({...})` literal into an object built from the constant: the literal is what gives TypeORM's key type-checking something to check, and the spec already asserts the two agree. Note also that allowlisting reintroduces trap (1) on the *insert* branch - an INSERT still writes every column, so a DB-stamped column omitted from the insert mapping comes back only if the driver returns the row; guard where the branch reads it rather than assuming, exactly as the update branch guards `RETURNING`.
+**Applies to**: any repository whose `upsert`/`save` coexists with a narrow `UPDATE` writer on the same table - today `libs/core/src/orders/infrastructure/persistence/repositories/order-record.repository.ts` (`syncStatus`, `syncAttempts`, `fulfillmentState`, `cancelledAt`, and the six FX columns of #2135, by omission) and `libs/core/src/inventory/infrastructure/persistence/repositories/inventory.repository.ts` (`updatedAt` plus the row's identity columns, by allowlisting).
+**Source**: #2101 (surfaced reviewing #2050 / ADR-040, which adopts the narrow-conditional-UPDATE shape for its own columns); the `cancelledAt` precedent is #1984; #2140 closed the same defect for `syncStatus` / `syncAttempts` in the same method; #2071 is the first use of the allowlisting mechanism, on `inventory_items`, where the excluded `updatedAt` feeds the propagation dedupe key.
 
 ## Claim an ADR number from the "Reserved numbers" note, not from the last row of the index table