Review follow-ups: PR #607 — hotfix(imports): flush new Products before staging items (FK violation from #606) #857

New Issue

2026-09-26T21:00:18Z

gambit-admin commented

2026-09-26 21:00:18 +00:00

Non-blocking findings from the auto code review of PR #607 (hotfix(imports): flush new Products before staging items (FK violation from #606)) — captured for batch cleanup rather than blocking the PR.
Commit 46fdcb1 · https://github.com/Gambit-Inc/gambit/pull/607

SHOULD-FIX:

backend/app/services/import_stage.py:271: the fix relies on a bare db.flush() before the items are added. That works only because the new Product rows are already pending in the session when stage_rows reaches the end. If _find_product_for_batch_item or BatchProductCache ever flushes or commits internally, or if a Product insert fails, the failure surfaces at this line. The ordering is undocumented and easy to break. A regression test that asserts the insert order and the FK enforcement also exists (the SQLite FK pragma), but only in the three test files touched here. Other test modules that call stage_rows on SQLite still run with FKs off, so they cannot catch this class of bug.
backend/app/services/import_stage.py:263: items grows to hold every IntakeBatchItem for the whole call. This is an 8k-row commit, so the objects are held either way and there is no real regression. But if stage_rows is ever called with far larger row lists, you now hold both the list and the session identity map. This is only worth noting; no action is needed now.

NITS:
none

Filed from a Claude Code session

Non-blocking findings from the auto code review of **PR #607** (hotfix(imports): flush new Products before staging items (FK violation from #606)) — captured for batch cleanup rather than blocking the PR. Commit `46fdcb1` · https://github.com/Gambit-Inc/gambit/pull/607 SHOULD-FIX: - `backend/app/services/import_stage.py:271`: the fix relies on a bare `db.flush()` before the items are added. That works only because the new `Product` rows are already pending in the session when `stage_rows` reaches the end. If `_find_product_for_batch_item` or `BatchProductCache` ever flushes or commits internally, or if a `Product` insert fails, the failure surfaces at this line. The ordering is undocumented and easy to break. A regression test that asserts the insert order and the FK enforcement also exists (the SQLite FK pragma), but only in the three test files touched here. Other test modules that call `stage_rows` on SQLite still run with FKs off, so they cannot catch this class of bug. - `backend/app/services/import_stage.py:263`: `items` grows to hold every `IntakeBatchItem` for the whole call. This is an 8k-row commit, so the objects are held either way and there is no real regression. But if `stage_rows` is ever called with far larger row lists, you now hold both the list and the session identity map. This is only worth noting; no action is needed now. NITS: none --- Filed from a Claude Code session

gambit-admin added the enhancement label 2026-09-26 21:00:18 +00:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: gambit/gambit#857