Skip to content

Commit a4cc449

Browse files
committed
Merge remote-tracking branch 'origin/main' into feat/tech-hardening-29-2-stream-report-export
2 parents ae9d386 + 13e8be2 commit a4cc449

2 files changed

Lines changed: 129 additions & 22 deletions

File tree

backend/src/test/resources/db/README.md

Lines changed: 44 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -22,23 +22,44 @@ two such branches merge:
2222

2323
## Conventions
2424

25-
1. **Version numbers.** Take the next free integer after the highest `V` currently on `main` **plus
26-
any in-flight PRs you know about**. When you rebase/merge `main` and hit a duplicate, bump *your*
27-
(newer) migration to the next free slot — never renumber someone else's merged migration. If we
28-
keep colliding, switch to timestamp versions (`V20260728__…`), which removes the race entirely.
29-
30-
**Timestamp versions do NOT remove the race** — proven 2026-08-13, when three branches all reached
31-
for the same two days (`V20260814` → schedule 5 subpage fixtures, `V20260815` → schedule 9 write
32-
fixtures). Everyone picks today's or tomorrow's date, so a date is just as scarce as an integer.
33-
34-
1a. **Schema changes that must apply LAST: use a repeatable migration (`R__…`).** If your script adds a
35-
constraint or index over data that *other* migrations seed — rather than seeding its own fixtures —
36-
it does not want a version number at all. It wants to run after everything, which is precisely
37-
Flyway's guarantee for repeatable migrations. `R__cost_detail_bridge_culvert_fks.sql` is the worked
38-
example: it declares the delivery FKs on `ILCR_COST_REPORT_DETAIL` after every schedule's fixtures
39-
have populated their per-report column, and it cannot collide with anyone. `FlywayMigrationVersionUniquenessTest`
40-
ignores `R__` files by design. This is NOT a general escape hatch — a migration that inserts its own
41-
fixtures still takes a version, because re-running it on a reused container would duplicate rows.
25+
1. **Seed data goes in a repeatable migration (`R__`), not a versioned one.** Decided 2026-08-20 —
26+
see `docs/decisions/flyway-test-fixture-strategy.md`. Name the file for **what it seeds**, with a
27+
numeric ordering prefix: `R__<10-80>_<what_it_seeds>.sql` for data,
28+
`R__<90+>_<name>.sql` for constraints, indexes and FKs that must land after the data.
29+
30+
Why: a version number is a **shared, sequential** resource, and two branches claiming the same one
31+
produce two *differently named files* — so **git merges them cleanly and the failure only appears
32+
when Flyway loads**, taking the whole `*IT` suite down at boot. Five collisions on record. With a
33+
content-derived `R__` name, a shared prefix is harmless (both files run, ordered by the rest of the
34+
name) and an identical name is the same path, which git reports as an ordinary conflict.
35+
36+
Verified on Flyway 12.4.0: repeatables apply **after every versioned migration**, in
37+
**lexicographic order of description** — so the numeric prefix is what fixes FK ordering, and
38+
digits sort before letters.
39+
40+
1a. **Only DDL keeps a version.** `V<next>__<name>.sql` for adding or altering tables. Take the next
41+
free integer after the highest `V` on `main` plus any in-flight PR you know about; on a duplicate,
42+
bump *your* (newer) migration and never renumber someone else's merged one.
43+
`FlywayMigrationVersionUniquenessTest` catches a clash at PR time rather than at IT boot. **Keep
44+
`INSERT`s out of these files** — putting seed rows in a new `V__` reopens the collision this
45+
convention exists to close.
46+
47+
*(Historical note, kept because it is the reason for the rule above: timestamp versions were tried
48+
and did NOT remove the race. Proven 2026-08-13, when three branches reached for the same two days
49+
(`V20260814`, `V20260815`), and again on 2026-08-19 with `V20260819`. Everyone hand-picks today's
50+
date, so a date is exactly as scarce as an integer.)*
51+
52+
1b. **`R__` files are safe to re-run here, and this is not the escape hatch it once looked like.** An
53+
earlier revision of this README said a fixture-inserting migration "still takes a version, because
54+
re-running it on a reused container would duplicate rows." That was wrong on both halves. Flyway
55+
re-runs a repeatable migration **only when its checksum changes**, and `AbstractOracleIT` creates
56+
the container **fresh per JVM** (no `withReuse`), so every repeatable applies exactly once per run.
57+
The only residual case is editing an `R__` file against a container you are deliberately reusing —
58+
which needs a clean container, the same caveat every DDL fixture here already carries.
59+
`R__cost_detail_bridge_culvert_fks.sql` is the worked example of the `90+` band: it declares the
60+
delivery FKs on `ILCR_COST_REPORT_DETAIL` after every schedule's fixtures have populated their
61+
per-report column. It predates the prefix convention and still carries no number; it should gain
62+
`90_` when the first prefixed seed lands beside it.
4263
2. **Fixture ID ranges.** Namespace seed entities by track so PKs can't overlap:
4364

4465
| Track | `MILL_ID` block | Notes |
@@ -54,14 +75,14 @@ two such branches merge:
5475
| Schedule 9 | **700–706** | `V20260815` |
5576
| Schedule 10 | **710–716** | `V20260817` |
5677

57-
**Schedule 5 sub-pages (`V20260814`, Story 7.4)** — a **timestamp version**, per convention 1 and
58-
the `V20260807` precedent. Seeds the first item-62 / item-68 rows the suite has ever held, on its
78+
**Schedule 5 sub-pages (`V20260814`, Story 7.4)** — a **timestamp version**, per the historical
79+
note in convention 1a and the `V20260807` precedent. Seeds the first item-62 / item-68 rows the suite has ever held, on its
5980
own mills so no destructive test can touch Story 7.2's `670–676`: `690` the write playground
6081
(Draft 2016–2023, one destructive concern per year), `691` Submitted → the write-gate 409, `692`
6182
check-status against real sub-page rows, `693` owned solely by the authorization IT. The block was
6283
`680–683` until Schedule 7B's `V20260811` landed on `main` claiming `680–681`; both migrations
6384
`INSERT INTO THE.MILL` those ids, so the merge would have failed Flyway outright on ORA-00001.
64-
Per convention 1 the newer (unmerged) claim moved. PK ranges are
85+
Per convention 1a the newer (unmerged) claim moved. PK ranges are
6586
a **new block**, verified above every value in use (the previous high-water mark was `8438`):
6687
`CAMP_REPORT_ID` **`8700–8719`** and `ILCR_COST_REPORT_DETAIL_ID` **`8720–8799`** — both below the
6788
sequence starts. It adds NO cost item (62/68/141/142 already exist via `V34`/`V31`).
@@ -212,5 +233,6 @@ snapshot they seed into, with the `V3x` references in the `schedule6` `*IT`s and
212233
lockstep. Version numbers only; no seed-ID clash (schedule 6 owns mills `660–666`).
213234

214235
This is the third version collision on this convention (schedule 2, schedule 11, schedule 6), and
215-
each one was caught only after CI went red on a branch that was otherwise green. The
216-
timestamp-version escape hatch in convention 1 above is worth taking.
236+
each one was caught only after CI went red on a branch that was otherwise green. Under the 2026-08-20 decision this class of
237+
clash no longer arises for seed data at all: it goes in an `R__` file, which has no version to claim
238+
(convention 1 above).
Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,85 @@
1+
# Decision: seed data moves to repeatable migrations (`R__`); Flyway keeps only the schema
2+
3+
**Status:** **ACCEPTED 2026-08-20.** Option 2 selected. Superseded the original proposal of Option 3, which is retained below as a stretch goal.
4+
**Scope:** backend test fixtures only (`backend/src/test/resources/db`). Not production — the runtime ships no application DDL (AD-2), and ITs run against a throwaway Testcontainers Oracle with **no persisted Flyway history**.
5+
6+
## Problem
7+
8+
Every schedule's integration tests seed their data through one **flat, shared, versioned Flyway namespace** (`V<n>__…sql`). With several schedules being built on parallel branches, two branches routinely claim the same version, and the clash only surfaces on merge. **Five occurrences on record:**
9+
10+
- **`V20260814`** and **`V20260815`** — claimed twice in one day, both lost by the bridge/culvert FK migration (to schedule 5 subpage fixtures, then schedule 9 write fixtures). #277 worked around it by switching to a repeatable `R__` migration.
11+
- **`V20260815`** — also claimed by both Story 9.2's `seed_schedule9_write_fixtures` and #277.
12+
- The `V33`/`V34` "timestamp escape hatch" note, and the mill-id renumbering (690–696 → 700–706) done to dodge cross-seed `ORA-00001` collisions.
13+
- **`V20260819`** — 2026-08-19, claimed by both the Schedule 10 TSA/TSB code-table migration and Story 24.1's reporting-year fixtures. Flyway answered `Found more than one migration with version 20260819` and the whole IT suite died at boot.
14+
15+
### The root cause, stated precisely
16+
17+
The original framing here was "a flat, shared, sequential namespace." That is close, but the operative property is sharper: **the collision is invisible to git.**
18+
19+
Two branches picking `V20260819` produce two *differently named files*. Git merges them cleanly — no conflict, nothing to review, nothing for a PR to catch — and the failure appears only when Flyway loads. As `R__cost_detail_bridge_culvert_fks.sql` puts it: *"a version number is a scarce shared resource and 'highest' is only knowable at merge time."*
20+
21+
That property is what selects the fix: **an identifier derived from content rather than sequence puts the collision back onto a single file path, where git catches it as an ordinary conflict.**
22+
23+
Two distinct problems are worth separating, because they have different answers:
24+
25+
- **P1 — version-number collision.** Flyway refuses to load; the whole IT suite dies at boot. This is what breaks builds.
26+
- **P2 — one shared mutable dataset.** PK collisions, cross-class write leakage, order-dependence. Real, but currently managed by hand (`Schedule2WriteIT:120` explicitly undoes a summary it created so it "must not leak across IT classes").
27+
28+
Because these are **test-only** seeds (fresh DB per run, no checksum/immutability constraints, no prod chain to preserve), we can reorganize freely.
29+
30+
## Options considered
31+
32+
1. **Second-granularity timestamps** (`Vyyyymmddhhmmss__…`). **Rejected — already tried and failed.** Day-granularity timestamps were adopted for exactly this reason and collided twice on 2026-08-13 and again on 2026-08-19, because everyone hand-picks *today's* date, making a date exactly as scarce as an integer. Second granularity would work only if machine-generated, and it still leaves the shared namespace and P2 untouched.
33+
2. **Repeatable migrations (`R__`) — SELECTED.** No version, so nothing to collide with; Flyway applies them after all versioned migrations. The original objection here was that "most of our seeds are stateful ordered inserts" — true, but ordering is controllable by name (see below), so the objection does not hold.
34+
3. **Split schema baseline from per-test data via `@Sql`.** Flyway owns only the schema; per-test fixture data moves onto the tests. Solves P1 *and* P2 in principle. **Deferred — see "Why not Option 3 now."**
35+
- **`flyway.outOfOrder=true` — rejected.** It permits a lower-version migration to apply after higher ones, which helps the "my branch's version is now below main's" case, but does **nothing** for the duplicate-version collisions we actually hit.
36+
37+
## Decision: Option 2
38+
39+
**Seed data becomes `R__`. Only DDL keeps a version.**
40+
41+
- **`V__` = schema only.** Table and constraint DDL. Few files, rarely touched, and `FlywayMigrationVersionUniquenessTest` already catches a collision there at PR time.
42+
- **`R__` = all seed/fixture data**, with a **content-derived name and a numeric ordering prefix**: seeds `10``80`, constraints/FKs/indexes `90`+.
43+
- **The existing chain freezes as the baseline.** Existing files are *not* converted wholesale; the convention governs new work, and an old file converts only when someone is already editing it. This is deliberately incremental — no stop-the-world refactor.
44+
45+
### Verified behaviour, not assumed
46+
47+
Probed against the real Oracle container on Flyway 12.4.0 (four scratch `R__` files plus a `flyway_schema_history` dump, since removed):
48+
49+
- All 46 versioned migrations applied at ranks **1–46**; all repeatables at **47–51**. So **`R__`-after-`V__` is a Flyway guarantee** — the *tool* enforces schema-before-data, which is the separation Option 3 sought by convention.
50+
- Repeatables apply in **lexicographic order of description** (observed: `05``50``90``bbb`). Digits sort before letters, so a numeric prefix yields fully deterministic FK ordering.
51+
52+
### Why this ends P1 structurally
53+
54+
| Two branches pick… | Git sees | Flyway sees |
55+
| --- | --- | --- |
56+
| the same `V` version *(before this decision)* | **nothing** — two filenames, clean merge | **fatal** — refuses to load, suite dies at boot |
57+
| the same `R__` numeric prefix | nothing | **harmless** — both run, order decided by the rest of the name |
58+
| the same `R__` full name | **merge conflict on one path** | n/a — never reaches Flyway |
59+
60+
The failure mode moves from *invisible and fatal* to *visible or harmless*. A shared prefix is no longer an error at all; only an identical filename collides, and that is the same path, which git reports normally.
61+
62+
### Consequences
63+
64+
- **No test changes and no file conversions** are required to adopt this. Cost is the convention, the README, and one rename.
65+
- **`R__cost_detail_bridge_culvert_fks.sql` should gain a `90_` prefix** when the first prefixed seed lands beside it. It sorts last among repeatables today only by ASCII accident (`c` sorts after any digit), which happens to be the order it needs; the prefix makes that explicit rather than incidental. Not renamed in this PR — it changes no behaviour until a prefixed `R__` constraint exists to sort against, and this PR is deliberately docs-only.
66+
- **P1 is closed. P2 is not.** Cross-schedule data collisions remain governed by the mill-ID range registry in `backend/src/test/resources/db/README.md`.
67+
- **The convention needs a machine check to hold.** The README convention alone did not prevent any of the five collisions, whereas `FlywayMigrationVersionUniquenessTest` caught the fifth at PR time. Extending it to reject a *new* `V__` file containing `INSERT`s — so seed data cannot drift back into the versioned namespace — is the follow-up that makes this self-enforcing. **Not done in this PR and not yet ticketed** — it needs an owner, or the convention rests on the same README discipline that failed five times.
68+
69+
## Why not Option 3 now
70+
71+
Option 3 remains the more thorough shape and is **retained as a stretch goal**, to revisit only if test pollution (P2) becomes untamable. Measurements taken against the tree on 2026-08-20 argue against doing it now:
72+
73+
- **74% of fixture data is shared backbone.** Of 1,232 `INSERT` statements, **912** target `MILL`, `ILCR_MILL_STATUS_XREF`, `ILCR_MILL_REPORT_STATUS`, `ILCR_COST_REPORT_DETAIL`, `ILCR_REPORT_COST_ITEM` and `ILCR_REPORT_SUMMARY`; two of those are each written by **29 of 47 files**. So Option 3 would relocate ~26% of the inserts and leave 74% in a shared baseline — it adds a *second* mechanism rather than replacing the shared dataset. (Statement counts, not row counts, so approximate; the direction is not.)
74+
- **Rollback is not available.** There are **115 IT classes and exactly 2 use `@Transactional`**; the other 113 commit real writes through MockMvc, so per-class `@Sql` needs idempotent load *plus* teardown. `@Sql` appears in **zero** test files today. Separately, ~16 of the 46 files mix DDL and `INSERT`s, so "freeze the chain as the baseline" is not a clean cut without splitting them first.
75+
- **Performance runs the other way.** Flyway applies all 51 files in **8.57s, once per JVM** (slowest single file 846ms) against a ~65s Oracle container start — about 12% of setup. Reloading subsets across 115 classes multiplies that and grows with every class added.
76+
- **It can silently weaken IDOR coverage — the decisive risk.** Several cross-tenant tests depend on *another mill's rows existing*: `update_foreignMillBridgeId_notFound`, `delete_foreignMillBridgeId_notFound`, `foreignMillCulvertReturns404`, `foreignMillCamp_404`, `sameNameOtherMillYear_succeeds`. Each asserts "someone else's row → 404." Under per-class fixtures, if the foreign mill is not deliberately provisioned the test still passes — because the row does not exist at all. The assertion survives while its meaning evaporates, and CI stays green.
77+
78+
Option 2 is not a lesser Option 3; it is the **first step Option 3 needs anyway**, since it separates schema from data with Flyway enforcing the boundary. Adopting it now forfeits nothing if the team later wants full per-test isolation.
79+
80+
## Authoring a new fixture — the rule
81+
82+
1. **Seeding data?** `R__<prefix>_<what_it_seeds>.sql`, prefix `10``80`. Pick the prefix by dependency order (parents before children); a tie is harmless.
83+
2. **Adding a constraint, index or FK over data others seed?** `R__9x_<name>.sql`.
84+
3. **Adding or altering a table?** `V<next>__<name>.sql`, and keep `INSERT`s out of it.
85+
4. **Never** put seed rows in a new `V__` file — that reopens the collision.

0 commit comments

Comments
 (0)