spec: self-review fixes (explicit claim_id arg, defer_foreign_keys)
This commit is contained in:
+61
-18
@@ -67,9 +67,15 @@ No CLI / settings changes.
|
||||
-- The remittances table had a parallel constraint already removed in 0003
|
||||
-- (because that one WAS a named index), so this migration only touches
|
||||
-- claims.
|
||||
--
|
||||
-- The migration runner (db_migrate.py) wraps each .sql file in an
|
||||
-- implicit transaction via engine.begin(), so we MUST NOT use BEGIN/COMMIT
|
||||
-- inside the file (nested transactions fail in SQLite). We defer FK
|
||||
-- enforcement with PRAGMA defer_foreign_keys instead of turning FKs off
|
||||
-- (which is a no-op inside a transaction in SQLite). The deferred
|
||||
-- checks fire at commit and validate against the renamed claims table.
|
||||
|
||||
PRAGMA foreign_keys=OFF;
|
||||
BEGIN;
|
||||
PRAGMA defer_foreign_keys = ON;
|
||||
|
||||
CREATE TABLE claims_new (
|
||||
id TEXT PRIMARY KEY,
|
||||
@@ -111,9 +117,6 @@ CREATE INDEX idx_claims_payer_rejected_at ON claims(payer_rejected_at);
|
||||
CREATE INDEX idx_claims_payer_rejected_unack
|
||||
ON claims(payer_rejected_at)
|
||||
WHERE payer_rejected_acknowledged_at IS NULL;
|
||||
|
||||
COMMIT;
|
||||
PRAGMA foreign_keys=ON;
|
||||
```
|
||||
|
||||
**Note on the `db.py` ORM model:** `backend/src/cyclone/db.py` declares
|
||||
@@ -165,29 +168,65 @@ Both pure reads; no transaction management needed.
|
||||
|
||||
### 837 path (line 394-415)
|
||||
|
||||
After the UNIQUE constraint is dropped in migration 0013, an `IntegrityError`
|
||||
in `store.add` can only originate from the PK on `claims.id` (CLM01) —
|
||||
either two claims in the same file share CLM01 (rare) or the same CLM01
|
||||
exists in a prior batch. The handler picks the first claim from
|
||||
`result.claims` and asks the helper whether a prior batch already holds
|
||||
that CLM01:
|
||||
|
||||
```python
|
||||
except IntegrityError as exc:
|
||||
existing_batch_id = store.find_existing_batch_for_claim(...)
|
||||
first_claim_id = result.claims[0].claim_id if result.claims else None
|
||||
existing_batch_id = (
|
||||
store.find_existing_batch_for_claim(first_claim_id)
|
||||
if first_claim_id else None
|
||||
)
|
||||
body = {
|
||||
"error": "Duplicate claim",
|
||||
"detail": "...",
|
||||
"detail": (
|
||||
"This file (or one previously ingested with the same "
|
||||
"claim control number) collides with an existing record. "
|
||||
"Inspect the file for duplicate CLM01 control numbers, or "
|
||||
"remove the existing batch before retrying."
|
||||
),
|
||||
"batch_id": rec.id,
|
||||
}
|
||||
if existing_batch_id:
|
||||
if existing_batch_id and existing_batch_id != rec.id:
|
||||
body["existing_batch_id"] = existing_batch_id
|
||||
log.warning("Duplicate claim while persisting batch %s: %s", rec.id, exc)
|
||||
return JSONResponse(status_code=409, content=body)
|
||||
```
|
||||
|
||||
The `...` argument is the first `claim.claim_id` from `result.claims`
|
||||
where the collision occurred. We pick the first one because the DB raises
|
||||
on the second insert; iterating through the parser's claim list to find
|
||||
which CLM01 already existed is cheap.
|
||||
The `existing_batch_id != rec.id` guard avoids surfacing the just-failed
|
||||
batch as a "previous" batch (it never persisted).
|
||||
|
||||
### 835 path (line 588-602)
|
||||
|
||||
Same pattern with `find_existing_batch_for_remit` and the existing remit's
|
||||
`payer_claim_control_number`.
|
||||
Same pattern with `find_existing_batch_for_remit` and the first remit's
|
||||
`payer_claim_control_number` (`result.claims[0].payer_claim_control_number`):
|
||||
|
||||
```python
|
||||
except IntegrityError as exc:
|
||||
first_pcn = result.claims[0].payer_claim_control_number if result.claims else None
|
||||
existing_batch_id = (
|
||||
store.find_existing_batch_for_remit(first_pcn)
|
||||
if first_pcn else None
|
||||
)
|
||||
body = {
|
||||
"error": "Duplicate remittance",
|
||||
"detail": (
|
||||
"This 835 file (or one previously ingested with the same "
|
||||
"payer claim control number) collides with an existing record. "
|
||||
"Remove the existing remittance before retrying."
|
||||
),
|
||||
"batch_id": rec.id,
|
||||
}
|
||||
if existing_batch_id and existing_batch_id != rec.id:
|
||||
body["existing_batch_id"] = existing_batch_id
|
||||
log.warning("Duplicate remittance while persisting batch %s: %s", rec.id, exc)
|
||||
return JSONResponse(status_code=409, content=body)
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
@@ -309,12 +348,16 @@ No new dependencies. No config changes.
|
||||
|
||||
## 9. Risk
|
||||
|
||||
* **Migration irreversibility**: SQLite has no `DROP CONSTRAINT`; the
|
||||
* **Migration reversibility**: SQLite has no `DROP CONSTRAINT`; the
|
||||
recreation is destructive to schema (but not to data — `INSERT INTO
|
||||
claims_new SELECT * FROM claims` preserves every row). If the
|
||||
migration fails mid-way, the transaction rolls back. The
|
||||
`PRAGMA foreign_keys=OFF` block is required because SQLite otherwise
|
||||
can't drop and rename a table that other tables reference.
|
||||
migration fails mid-way, the implicit transaction (`engine.begin()`
|
||||
in `db_migrate.py`) rolls back. `PRAGMA defer_foreign_keys = ON` is
|
||||
required because SQLite otherwise can't drop a table that other
|
||||
tables reference (`remittances.claim_id`, `matches.claim_id`,
|
||||
`line_reconciliations.claim_id`, `activity_events.claim_id`). The
|
||||
deferred checks fire at commit and validate against the renamed
|
||||
`claims` table.
|
||||
* **Loss of uniqueness**: after the migration, two claims in one batch
|
||||
*can* share a `patient_control_number`. This is the intended behavior.
|
||||
Claim identity is still unique via `claims.id` (CLM01, PK) and the
|
||||
|
||||
Reference in New Issue
Block a user