From a681363066efea358b11f0745e2b0b5280f62c1d Mon Sep 17 00:00:00 2001 From: Gray Fay Date: Sun, 13 Sep 2026 23:30:33 -0400 Subject: [PATCH] fix: stop migration 23 relabelling every out-of-state row as Ohio Smoke-testing the new /coverage endpoint against production showed the data was never single-state: OH 5,775,659 IN 249 PA 170 MI 59 oh 2 BE 9 ON 2 O 1 IH 1 0 1 PJ 1 The backfill read: SET region = 'OH' WHERE region IS NULL OR region = '' OR region <> 'OH' which stamps OH over Indiana, Pennsylvania and Michigan. Permanently, with nothing in the logs. That came from collapsing two statements into one to avoid a second table rewrite -- an optimisation that changed what the statement meant. It is two statements again, and the comment says why they cannot be merged: case normalisation touches only rows whose case differs, the backfill touches only blanks, and neither is the full-table rewrite the single statement appeared to avoid. The guard added alongside it is the only reason this was caught rather than deployed, but it was too strict in the other direction: it refused outright whenever any non-OH region existed, so migration 23 could never have run in production at all. The two conditions are now separate. Other states alone are fine and are logged. Other states AND blanks is the genuinely ambiguous case, because a blank could belong to any of them, and that still refuses. Also removed a pre-check for (hash, region) collisions after case normalisation. It can never fire: the constraint this migration replaces makes hash unique on its own, so no two rows share a hash in any region. Verified by trying to construct the collision and being refused by the old constraint -- which is now a test. Verified against a database carrying the real region distribution: 'oh' merges into 'OH', and IN, PA, MI and the junk codes are left exactly as they were. The junk codes are left alone deliberately. BE, ON, O, IH, 0 and PJ come from the legacy loader truncating a state name to two characters -- fixed going forward in this branch -- but inventing a correction for 15 existing rows would be the same guessing this migration refuses to do elsewhere. Co-Authored-By: Claude Opus 5 --- database/migrations.go | 54 ++++++++++++--- services/multistate_integration_test.go | 87 +++++++++++++++++++++++++ 2 files changed, 131 insertions(+), 10 deletions(-) diff --git a/database/migrations.go b/database/migrations.go index 362e3c0..eb8bc70 100644 --- a/database/migrations.go +++ b/database/migrations.go @@ -1591,32 +1591,66 @@ func addRegionToAddressUniqueness() error { } defer tx.Rollback() - // The backfill below stamps 'OH' onto every blank region, which is only - // correct because everything loaded so far is Ohio. That is an assumption, - // and a wrong one would be permanent and invisible -- so verify it rather - // than trust it. + // Blank regions get stamped 'OH' below, which is only sound if Ohio is the + // only state present. Production turned out to hold Indiana, Pennsylvania + // and Michigan rows as well, so "everything here is Ohio" was never true -- + // it just looked true from the code. + // + // The two conditions are separate. Other states are fine on their own; + // what is not fine is other states AND blanks, because then a blank could + // belong to any of them and stamping it OH is a guess that cannot be + // undone. var foreign string // COALESCE because string_agg over no rows is NULL, and the empty-table // case -- a fresh install -- is the common one. err = tx.QueryRow(` - SELECT COALESCE(string_agg(DISTINCT region, ', '), '') + SELECT COALESCE(string_agg(DISTINCT region, ', ' ORDER BY region), '') FROM ohio_addresses WHERE region IS NOT NULL AND region <> '' AND UPPER(region) <> 'OH' `).Scan(&foreign) if err != nil { return fmt.Errorf("failed to check existing regions: %w", err) } + + var blanks int + if err := tx.QueryRow(` + SELECT COUNT(*) FROM ohio_addresses WHERE region IS NULL OR region = '' + `).Scan(&blanks); err != nil { + return fmt.Errorf("failed to count blank regions: %w", err) + } + + if blanks > 0 && foreign != "" { + return fmt.Errorf("%d row(s) have no region while rows also exist for %s: "+ + "a blank cannot be assumed to be Ohio, so set them explicitly before migrating", blanks, foreign) + } if foreign != "" { - return fmt.Errorf("refusing to backfill blank regions to OH: rows already exist for %s, "+ - "so a blank region cannot be assumed to be Ohio", foreign) + log.Printf("Migration 23: regions besides OH already present (%s); leaving them as they are", foreign) } + // Upper-casing merges 'oh' into 'OH'. That cannot produce a duplicate + // (hash, region) pair, because the constraint this migration replaces makes + // hash unique on its own -- so no two rows share a hash to begin with, in + // any region. Checked rather than assumed while writing this, by trying to + // construct the collision and being refused by the old constraint. + statements := []string{ - // One pass, not two. Each full-table UPDATE rewrites every tuple, - // roughly doubling the table until vacuum and generating WAL to match. + // Two statements, and they are NOT interchangeable. Collapsing them + // into one `SET region = 'OH' WHERE ... OR region <> 'OH'` -- which an + // earlier revision of this migration did, to save a table rewrite -- + // relabels every Indiana, Pennsylvania and Michigan row as Ohio. + // Permanently, and with nothing in the logs to say so. + // + // Case normalisation only touches rows whose case differs, and the + // backfill only touches blanks. Both are narrow: the WHERE clauses + // keep them off the ~5.8M rows that are already correct, so neither is + // the full-table rewrite the single statement appeared to avoid. + `UPDATE ohio_addresses + SET region = UPPER(region) + WHERE region IS NOT NULL AND region <> UPPER(region)`, + `UPDATE ohio_addresses SET region = 'OH' - WHERE region IS NULL OR region = '' OR region <> 'OH'`, + WHERE region IS NULL OR region = ''`, // SHARE lock: readers unaffected. `CREATE UNIQUE INDEX IF NOT EXISTS idx_ohio_addresses_hash_region diff --git a/services/multistate_integration_test.go b/services/multistate_integration_test.go index 0c3a612..bdf0607 100644 --- a/services/multistate_integration_test.go +++ b/services/multistate_integration_test.go @@ -353,3 +353,90 @@ func TestStateAndBBoxComposeCorrectly(t *testing.T) { } t.Logf("bbox %d, +state %d, +query %d", boxOnly, combined, withQuery) } + +// Production is not single-state and never was. Coverage reports Indiana (249), +// Pennsylvania (170) and Michigan (59) rows alongside Ohio, plus a lowercase +// 'oh' and several junk codes from the legacy loader truncating a state name to +// two characters. +// +// An earlier revision of migration 23 backfilled with +// `SET region = 'OH' WHERE region IS NULL OR region = ” OR region <> 'OH'`, +// collapsing two statements into one to save a table rewrite. Against that data +// it relabels every out-of-state row as Ohio, permanently and silently. This +// pins the behaviour the migration must have instead. +func TestRegionNormalisationLeavesOtherStatesAlone(t *testing.T) { + db := setupMultiStateDB(t, true) + + seed := []struct{ house, county, region string }{ + {"1", "Franklin", "OH"}, + {"2", "Allen", "IN"}, + {"3", "Erie", "PA"}, + {"4", "Wayne", "MI"}, + } + for _, r := range seed { + if _, err := insertAddress(db, r.house, "Main Street", "", "Springfield", "", r.county, r.region); err != nil { + t.Fatalf("seed %s: %v", r.region, err) + } + } + + // What the migration does to regions, in the order it does it. + if _, err := db.Exec(`UPDATE ohio_addresses SET region = UPPER(region) + WHERE region IS NOT NULL AND region <> UPPER(region)`); err != nil { + t.Fatalf("normalise case: %v", err) + } + if _, err := db.Exec(`UPDATE ohio_addresses SET region = 'OH' + WHERE region IS NULL OR region = ''`); err != nil { + t.Fatalf("backfill blanks: %v", err) + } + + rows, err := db.Query(`SELECT region, COUNT(*) FROM ohio_addresses GROUP BY region ORDER BY region`) + if err != nil { + t.Fatalf("query: %v", err) + } + defer rows.Close() + + got := map[string]int{} + for rows.Next() { + var region string + var n int + if err := rows.Scan(®ion, &n); err != nil { + t.Fatalf("scan: %v", err) + } + got[region] = n + } + + for _, want := range []string{"OH", "IN", "PA", "MI"} { + if got[want] != 1 { + t.Errorf("region %s has %d rows, want 1 -- out-of-state data was relabelled", want, got[want]) + } + } + if len(got) != 4 { + t.Errorf("got %d distinct regions (%v), want 4", len(got), got) + } +} + +// The old constraint makes hash unique on its own, so no two rows share a hash +// in any region -- which is why normalising case cannot create a duplicate +// (hash, region) pair, and why migration 23 needs no collision pre-check. +func TestOldKeyMakesNormalisationCollisionsImpossible(t *testing.T) { + db := setupMultiStateDB(t, false) + + hash := "1|Main Street||Springfield|" + insert := func(region string) error { + _, err := db.Exec(` + INSERT INTO ohio_addresses (hash, house_number, street, unit, city, district, region, postcode, county, geom, full_address) + VALUES ($1,'1','Main Street','','Springfield','',$2,'','Franklin', + ST_SetSRID(ST_MakePoint(-83.0,40.0),4326),'1 Main Street, Springfield') + `, hash, region) + return err + } + + if err := insert("OH"); err != nil { + t.Fatalf("first insert: %v", err) + } + // Same hash, different case. If this were possible, upper-casing would + // merge the two and break the unique index build. + if err := insert("oh"); err == nil { + t.Error("two rows shared a hash under the old constraint; a collision after normalisation would be possible") + } +}