fix: migration 23 is failing in production and would corrupt data if it ran

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

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(&region, &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")
+	}
+}