fix(db+tests): data-preservation test for migration 0014; document before_insert events

Address the three concerns raised by the spec reviewer on commit 534130e:

1. The 0014 data-preservation test was degraded: it only verified
   post-0014 INSERTs round-trip, never proving that 0014's INSERT INTO
   claims_new SELECT * FROM claims actually carries pre-existing rows
   through the table recreation. Add migrate_to_version() helper to
   db_migrate.py and rewrite the test to seed at v=13, apply 0014, and
   assert the rows survive. Add a complementary test exercising the
   JOIN-FK tables (matches / cas_adjustments / service_line_payments /
   line_reconciliations) whose batch_id columns get populated by
   0014's JOIN against the parent tables.

2. The four SQLAlchemy ORM before_insert events on Match /
   CasAdjustment / ServiceLinePayment / LineReconciliation are
   undocumented in the codebase. Add a clear module-level docstring
   explaining what the events do, why they exist (composite-FK
   NOT NULL columns require the batch side which most call sites
   don't have readily available), when they fail (parent id not
   findable in session.new / identity_map / DB -> misleading
   'NOT NULL constraint failed' IntegrityError), and the workarounds.

3. The plan Task 1.3 Step 8 said 'do not modify production code to
   make old tests pass'. In practice production code DID need to
   change because composite-PK semantics make single-id lookups
   ambiguous. Amend the plan to acknowledge this and cite the
   s.get(Claim, X) -> s.query(Claim).filter().first() refactor as
   the example. Also document the before_insert events in Step 5.

Brings in docs/superpowers/plans/2026-06-21-cyclone-parse-decide-workflow.md
from main (this branch was created before the plan file was committed)
with Task 1.3 Step 5 and Step 8 amended per the reviewer feedback.
This commit is contained in:
Tyler
2026-06-21 19:24:13 -06:00
parent 534130ee2b
commit ced8d20aed
4 changed files with 3267 additions and 92 deletions
+52 -23
View File
@@ -897,35 +897,64 @@ class ClearhouseORM(Base):
# =============================================================================
# Migration 0014: auto-populate the batch side of composite FKs.
# Migration 0014: ORM `before_insert` events auto-populate the batch side
# of composite FKs.
# =============================================================================
#
# The schema after 0014 requires every Match / CasAdjustment /
# ServiceLinePayment / LineReconciliation to carry the batch_id of its
# parent Claim / Remittance. The columns are NOT NULL in the DB and the
# composite FK is enforced at SQL level.
# **What.** The four listeners below (``_match_before_insert``,
# ``_cas_before_insert``, ``_slp_before_insert``, ``_lr_before_insert``)
# run on every INSERT of a ``Match`` / ``CasAdjustment`` /
# ``ServiceLinePayment`` / ``LineReconciliation`` row. They look up the
# parent ``Claim`` / ``Remittance`` and copy its ``batch_id`` (and
# ``remittance_batch_id``) into the new row's composite-FK column.
#
# The application code that creates these rows has the parent
# claim_id / remittance_id in scope (often as a string) but does NOT
# always have batch_id readily available — the batch is created elsewhere
# in the same ingest transaction, or the parent is fetched by id alone.
# **Why.** After migration 0014, every child table has a composite FK to
# its parent: ``(batch_id, claim_id)`` for Match / LineReconciliation and
# ``(remittance_batch_id, remittance_id)`` for CasAdjustment /
# ServiceLinePayment. Both sides are ``NOT NULL`` at the SQL level, so
# every insert must supply ``batch_id``. Application code that creates
# these rows usually has the parent ``claim_id`` / ``remittance_id`` in
# scope (often as a string) but does not always have ``batch_id``
# readily available — the batch is created elsewhere in the same ingest
# transaction, or the parent is fetched by id alone. Threading
# ``batch_id`` through every call site would be error-prone. The events
# solve this transparently: caller passes the parent id as before, and
# the event fills in the batch side.
#
# Instead of threading batch_id through every call site, we use SQLAlchemy
# ORM `before_insert` events to look up the parent and copy its batch_id
# into the new row. The lookup prefers session-cached state over SQL so the
# common ingest/match flows don't pay an extra round-trip:
# 1. session.new — pending objects added in this transaction.
# 2. session.identity_map — already-persisted objects loaded earlier.
# 3. session.execute(select(...)) — DB query. autoflush is OFF in our
# SessionLocal, so this won't trigger a flush of session.new.
# **Lookup order** (preferred-to-fallback, to minimize round-trips):
# 1. ``session.new`` — pending parents added in this transaction
# (covers the common ingest/match flow where parent + child are
# added in the same session).
# 2. ``session.identity_map`` — already-persisted parents loaded
# earlier in this session.
# 3. ``session.execute(select(...))`` — DB query as a last resort.
# ``autoflush`` is OFF on our ``SessionLocal``, so this won't
# trigger an unintended flush of ``session.new``.
#
# If the parent is not findable, we leave the column None — the NOT NULL
# constraint will then surface as IntegrityError at INSERT, which is the
# right signal: the caller passed a parent id that doesn't exist.
# **When it fails.** If the parent is in none of the above (e.g. the
# caller passed a parent id that was never added to the session and is
# not in the DB), the column stays NULL and the INSERT fails with
# ``IntegrityError: NOT NULL constraint failed: <table>.batch_id`` (or
# ``remittance_batch_id``). The error message is misleading — it looks
# like the column was forgotten, not that the parent id was wrong.
# Workarounds:
# * If the parent exists in another session, ``session.add(parent)``
# first (or use ``session.merge(parent)``) so the lookup finds it
# in ``session.new`` / ``session.identity_map``.
# * If you have the parent object in scope and know its ``batch_id``,
# set ``child.batch_id = ...`` explicitly — the event only fills
# in when the column is None, so explicit values win.
# * If the parent is on a different connection entirely, populate the
# column by hand before ``session.add(child)``.
#
# App code may STILL explicitly set batch_id (e.g., when it has the parent
# object in scope and wants to skip the lookup). The events only fill in
# when the column is None.
# **Idempotency.** The events only act when ``target.batch_id is None``
# (or ``remittance_batch_id``). Application code may explicitly set
# either column — e.g., a bulk-loader that already has batch_id in hand
# skips the lookup.
#
# **Scope.** ``propagate=True`` so subclasses of these mappers (in
# tests, mainly) inherit the listeners. Production code does not use
# mapper inheritance for these tables.
from sqlalchemy import event as _sa_event # noqa: E402
from sqlalchemy.orm import Session as _Session # noqa: E402
+44 -1
View File
@@ -74,4 +74,47 @@ def run(engine: sa.Engine) -> None:
conn.exec_driver_sql(stmt)
conn.exec_driver_sql(f"PRAGMA user_version = {version}")
current = version
current = version
def migrate_to_version(engine: sa.Engine, target_version: int) -> None:
"""Apply migrations up to and including ``target_version``. Idempotent.
Reads the current ``PRAGMA user_version`` and applies any migrations
whose version is in the range ``(current, target_version]``. Stops
before applying migrations with version > ``target_version`` so the
caller can seed data between N and N+1.
Useful for tests that need to seed data after migrations ``0001..N``
have applied but before ``N+1`` runs — exercise a migration against
real, pre-existing rows rather than only verifying that post-migration
inserts round-trip.
Calling with ``target_version`` <= the current version is a no-op.
"""
migrations_dir = MIGRATIONS_DIR
if not migrations_dir.exists():
return
migration_files = sorted(migrations_dir.glob("*.sql"))
if not migration_files:
return
with engine.begin() as conn:
current = conn.exec_driver_sql("PRAGMA user_version").scalar() or 0
for path in migration_files:
sql = path.read_text()
version = _migration_version(sql, path.name)
if version <= current:
continue
if version > target_version:
break
statements_sql = _strip_comments(sql)
statements = [s.strip() for s in statements_sql.split(";") if s.strip()]
with engine.begin() as conn:
for stmt in statements:
conn.exec_driver_sql(stmt)
conn.exec_driver_sql(f"PRAGMA user_version = {version}")
+184 -68
View File
@@ -2,6 +2,7 @@
from __future__ import annotations
from datetime import datetime, timezone
from pathlib import Path
import pytest
@@ -384,91 +385,206 @@ def test_migration_0014_relaxes_remittances_pk_to_composite(
)
def test_migration_0014_preserves_existing_data(
tmp_path: Path, monkeypatch: pytest.MonkeyPatch
) -> None:
"""Existing rows in claims and remittances survive the table recreation.
def test_migration_0014_preserves_existing_data(tmp_path, monkeypatch):
"""Data inserted at v=13 must survive the v=14 table recreation.
0014 uses INSERT INTO ..._new SELECT * FROM old, so any rows present
before 0014 (whether inserted by an earlier migration's seed or by
application code) must still be there after 0014 finishes. Both claims
and remittances must round-trip; matches / cas_adjustments /
service_line_payments / line_reconciliations must also round-trip
(they have composite-FK columns that get populated by JOIN in the
migration).
0014 uses ``INSERT INTO claims_new SELECT * FROM claims`` (and the
remittance-side equivalent) to populate the recreated tables. Any rows
present before 0014 must still be there after 0014 finishes — that is
the contract that lets the schema change ship without data loss.
This test exercises the real 0014 against real, pre-existing rows:
1. Apply 0001-0013 via ``migrate_to_version``.
2. Seed a claim and a remittance in v=13 (raw SQL — the ORM model
declares ``matched_remittance_batch_id`` which doesn't exist at v=13).
3. Apply 0014 via ``migrate_to_version``.
4. Verify the seeded rows survive, with the same id and batch_id.
5. Verify the SAME id can be inserted in a DIFFERENT batch after 0014
(proving the composite PK took effect — a single-column PK would
have rejected the second insert).
Uses a fresh ``sa.Engine`` (not the module-global ``db.engine()``)
because the conftest autouse fixture has already initialized the
module engine at v=14 — we need a clean engine that starts at v=0
so ``migrate_to_version(13)`` actually applies 0001..0013.
"""
# Set up: apply 0001 to create the source tables. We then INSERT a row
# into the source tables, then apply 0014 to recreate them, then verify
# the row is still there. db_migrate.run() runs all migrations in one
# call, so we split it into two engine.begin() blocks: one for 0001,
# one for 0014.
monkeypatch.setattr(db_migrate, "MIGRATIONS_DIR", tmp_path)
(tmp_path / "0001_initial.sql").write_text(_0014_SYNTHETIC_0001)
real_migrations = Path(db_migrate.__file__).parent / "migrations"
real_0014 = real_migrations / "0014_relax_claims_remits_pk.sql"
assert real_0014.exists()
(tmp_path / "0014_relax_claims_remits_pk.sql").write_text(real_0014.read_text())
from sqlalchemy.orm import sessionmaker
engine = _fresh_engine(tmp_path)
SessionLocal = sessionmaker(bind=engine, autoflush=False, autocommit=False, expire_on_commit=False)
# Phase 1: apply only 0001, then seed representative rows.
with engine.begin() as c:
v = c.exec_driver_sql("PRAGMA user_version").scalar() or 0
assert v == 0
db_migrate.run(engine)
v = _user_version(engine)
# 0014 ran too — so we are already at v14. The migration succeeded, but
# we never got to insert rows between 0001 and 0014. To test
# preservation, we need to seed BEFORE 0014 runs. Solution: rewind
# user_version to 1 after the 0001-only run, then run again to apply
# 0014 against seeded data.
# Easier approach: drop 0014 from tmp_path, run to v1, seed, copy 0014
# back, run again to v14.
# Even easier: seed post-0014 with a value that matches the pre-0014
# shape, and verify the SELECT round-trips. That proves the column
# shapes match. The preservation aspect is implicit — if 0014's
# INSERT...SELECT referenced a column that didn't exist, the
# migration would have failed. So we just verify post-0014 inserts
# work for the column shapes 0014 expected.
assert v == 14, (
"Migration runner applied 0001 + 0014 in one shot (v=14). The "
"preservation test cannot seed data between 0001 and 0014 with "
"this runner; instead, verify post-0014 inserts of pre-0014-shaped "
"rows round-trip correctly (column shapes match)."
)
from cyclone.db_migrate import migrate_to_version
from cyclone.db import Batch, Claim
# Seed representative rows on each recreated table — one per table — to
# prove all six recreated tables accept inserts that match the column
# shapes 0014's INSERT...SELECT expected from the pre-0014 versions.
# Apply 0001-0013 only.
migrate_to_version(engine, 13)
assert _user_version(engine) == 13
# Seed a claim and a remittance in v=13. We use raw SQL here because
# the ORM Claim/Remittance models declare post-0014 columns
# (``matched_remittance_batch_id``) that don't exist at v=13. The
# migration's data-preservation contract is independent of how the
# data got there — raw SQL inserts count as "data that needs to
# survive the migration" just like ORM inserts do.
with engine.begin() as c:
c.exec_driver_sql(
"INSERT INTO batches(id, kind, input_filename, parsed_at) "
"VALUES ('B-OLD', '837p', 'x.txt', '2026-01-01')"
"VALUES ('B-V13', '837p', 'x.txt', '2026-01-01')"
)
c.exec_driver_sql(
"INSERT INTO claims(id, batch_id, patient_control_number, "
"state) VALUES ('CLM-OLD', 'B-OLD', 'M', 'submitted')"
"state, state_changed_at) VALUES "
"('CLM-PRESERVED', 'B-V13', 'M', 'submitted', '2026-01-01')"
)
c.exec_driver_sql(
"INSERT INTO batches(id, kind, input_filename, parsed_at) "
"VALUES ('B-V13R', '835', 'x.txt', '2026-01-02')"
)
c.exec_driver_sql(
"INSERT INTO remittances(id, batch_id, payer_claim_control_number, "
"status_code, received_at) "
"VALUES ('CLP-OLD', 'B-OLD', 'PCN', '1', '2026-01-01')"
"status_code, received_at) VALUES "
"('CLP-PRESERVED', 'B-V13R', 'CLP-PRESERVED', '1', '2026-01-02')"
)
# Apply 0014. Data must survive.
migrate_to_version(engine, 14)
assert _user_version(engine) == 14
with engine.connect() as c:
claim_row = c.exec_driver_sql(
"SELECT id, batch_id FROM claims WHERE id='CLM-OLD'"
claim = c.exec_driver_sql(
"SELECT id, batch_id, patient_control_number "
"FROM claims WHERE id='CLM-PRESERVED'"
).first()
remit_row = c.exec_driver_sql(
"SELECT id, batch_id FROM remittances WHERE id='CLP-OLD'"
remit = c.exec_driver_sql(
"SELECT id, batch_id, payer_claim_control_number "
"FROM remittances WHERE id='CLP-PRESERVED'"
).first()
assert claim_row == ("CLM-OLD", "B-OLD"), (
"claims row inserted into the recreated claims table did not survive "
"a SELECT — the recreated column shape does not match 0014's source."
)
assert remit_row == ("CLP-OLD", "B-OLD"), (
"remittances row inserted into the recreated remittances table did "
"not survive a SELECT — the recreated column shape does not match "
"0014's source."
)
assert claim == ("CLM-PRESERVED", "B-V13", "M")
assert remit == ("CLP-PRESERVED", "B-V13R", "CLP-PRESERVED")
# Confirm that AFTER v=14, the same id can be inserted in a different
# batch via the ORM (now that the composite PK exists, this is the
# canonical insert path).
with SessionLocal() as s:
s.add(Batch(id="B-V14", kind="837p", input_filename="y.txt",
parsed_at=datetime(2026, 1, 3, tzinfo=timezone.utc),
raw_result_json={}))
s.add(Claim(id="CLM-PRESERVED", batch_id="B-V14",
patient_control_number="M2",
state_changed_at=datetime(2026, 1, 3, tzinfo=timezone.utc)))
s.commit()
with engine.connect() as c:
rows = c.exec_driver_sql(
"SELECT batch_id FROM claims WHERE id='CLM-PRESERVED' ORDER BY batch_id"
).all()
assert [r[0] for r in rows] == ["B-V13", "B-V14"]
def test_migration_0014_preserves_joint_fk_rows(tmp_path, monkeypatch):
"""Migration 0014 also recreates the JOIN-FK tables (matches,
cas_adjustments, service_line_payments, line_reconciliations). Each of
those tables has a composite FK to claims/remittances whose ``batch_id``
side gets populated by JOIN in the migration. Insert representative
rows at v=13 and verify they survive 0014 with the new batch_id column
populated.
This is the complement to ``test_migration_0014_preserves_existing_data``
(which covers the claims/remittances tables) and exercises the
``INSERT INTO ..._new SELECT ... JOIN parent`` statements that the
simpler test cannot.
Like the simpler test, we seed at v=13 via raw SQL — the ORM model
declares ``batch_id`` on the JOIN-FK tables but that column is only
added by 0014 itself, so an ORM insert at v=13 would fail.
"""
from sqlalchemy.orm import sessionmaker
engine = _fresh_engine(tmp_path)
SessionLocal = sessionmaker(bind=engine, autoflush=False, autocommit=False, expire_on_commit=False)
from cyclone.db_migrate import migrate_to_version
migrate_to_version(engine, 13)
# Seed parent rows + one row per JOIN-FK child table (raw SQL at v=13).
with engine.begin() as c:
c.exec_driver_sql(
"INSERT INTO batches(id, kind, input_filename, parsed_at) "
"VALUES ('B-JOIN', '837p', 'j.txt', '2026-01-01')"
)
c.exec_driver_sql(
"INSERT INTO claims(id, batch_id, patient_control_number, "
"state, state_changed_at) VALUES "
"('CLM-J', 'B-JOIN', 'M', 'submitted', '2026-01-01')"
)
c.exec_driver_sql(
"INSERT INTO batches(id, kind, input_filename, parsed_at) "
"VALUES ('B-JOIN-R', '835', 'jr.txt', '2026-01-02')"
)
c.exec_driver_sql(
"INSERT INTO remittances(id, batch_id, payer_claim_control_number, "
"status_code, received_at) VALUES "
"('CLP-J', 'B-JOIN-R', 'CLP-J', '1', '2026-01-02')"
)
# Seed the four JOIN-FK child rows. At v=13 these tables only have
# the parent-id columns (no batch_id side yet) — 0014 adds the
# batch_id columns and JOINs against the parents to populate them.
c.exec_driver_sql(
"INSERT INTO service_line_payments("
"remittance_id, line_number, procedure_qualifier, procedure_code, "
"modifiers_json, charge, payment) VALUES "
"('CLP-J', 1, 'HC', '99213', '[]', 100.00, 80.00)"
)
c.exec_driver_sql(
"INSERT INTO cas_adjustments("
"remittance_id, group_code, reason_code, amount) VALUES "
"('CLP-J', 'CO', '45', 20.00)"
)
c.exec_driver_sql(
"INSERT INTO line_reconciliations("
"claim_id, status, match_score, reconciled_at) VALUES "
"('CLM-J', 'matched', 95, '2026-01-02')"
)
c.exec_driver_sql(
"INSERT INTO matches("
"claim_id, remittance_id, strategy, matched_at) VALUES "
"('CLM-J', 'CLP-J', 'exact', '2026-01-02')"
)
# Apply 0014 — the JOIN-FK tables get the batch_id column populated by
# the migration's INSERT...SELECT...JOIN statements.
migrate_to_version(engine, 14)
with engine.connect() as c:
rows = c.exec_driver_sql(
"SELECT remittance_id, remittance_batch_id "
"FROM service_line_payments WHERE remittance_id='CLP-J'"
).all()
assert rows == [("CLP-J", "B-JOIN-R")], (
"service_line_payments.remittance_batch_id must be populated "
"by 0014's JOIN against remittances; got " + repr(rows)
)
rows = c.exec_driver_sql(
"SELECT remittance_id, remittance_batch_id "
"FROM cas_adjustments WHERE remittance_id='CLP-J'"
).all()
assert rows == [("CLP-J", "B-JOIN-R")], (
"cas_adjustments.remittance_batch_id must be populated by 0014; got "
+ repr(rows)
)
rows = c.exec_driver_sql(
"SELECT claim_id, batch_id FROM line_reconciliations WHERE claim_id='CLM-J'"
).all()
assert rows == [("CLM-J", "B-JOIN")], (
"line_reconciliations.batch_id must be populated by 0014; got "
+ repr(rows)
)
rows = c.exec_driver_sql(
"SELECT claim_id, batch_id, remittance_id, remittance_batch_id "
"FROM matches WHERE claim_id='CLM-J'"
).all()
assert rows == [("CLM-J", "B-JOIN", "CLP-J", "B-JOIN-R")], (
"matches batch_id + remittance_batch_id must be populated by 0014; "
"got " + repr(rows)
)
File diff suppressed because it is too large Load Diff