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") + } +}