fix: attribute 985k blank regions from coordinates, unblocking migration 23

database/migrations.go

@@ -1619,39 +1619,81 @@ func addRegionToAddressUniqueness() error {
 		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 != "" {
 		log.Printf("Migration 23: regions besides OH already present (%s); leaving them as they are", foreign)
 	}
 
+	// Blank regions are attributed from the coordinates, not guessed.
+	//
+	// Production had 985,634 of them -- about 17% of the table -- alongside
+	// rows for nine other regions. Stamping those 'OH' because the dataset is
+	// "the Ohio one" is the same mistake as the backfill this replaced, just
+	// quieter: border-county extracts genuinely contain Indiana, Pennsylvania
+	// and Michigan addresses, and a mislabel is permanent and invisible.
+	//
+	// us_states already holds TIGER boundaries behind a GIST index -- it is
+	// what /states/lookup answers from -- so each row can be asked where it
+	// actually is. Evidence instead of assumption.
+	if blanks > 0 {
+		var haveBoundaries bool
+		if err := tx.QueryRow(`
+			SELECT EXISTS(SELECT 1 FROM us_states WHERE geometry IS NOT NULL)
+		`).Scan(&haveBoundaries); err != nil {
+			return fmt.Errorf("failed to check for state boundaries: %w", err)
+		}
+		if !haveBoundaries {
+			return fmt.Errorf("%d row(s) have no region and us_states holds no geometry to attribute them from; "+
+				"load state boundaries first", blanks)
+		}
+
+		res, err := tx.Exec(`
+			UPDATE ohio_addresses a
+			SET region = s.state_abbr
+			FROM us_states s
+			WHERE (a.region IS NULL OR a.region = '')
+			  AND s.geometry IS NOT NULL
+			  AND ST_Contains(s.geometry, a.geom)
+		`)
+		if err != nil {
+			return fmt.Errorf("failed to attribute blank regions from coordinates: %w", err)
+		}
+		attributed, _ := res.RowsAffected()
+		log.Printf("Migration 23: attributed %d of %d blank region(s) from their coordinates", attributed, blanks)
+
+		// Whatever is left sits outside every state boundary, which means the
+		// coordinates are wrong rather than the region being missing. Naming
+		// the count beats inventing a state for them.
+		var stranded int
+		if err := tx.QueryRow(`
+			SELECT COUNT(*) FROM ohio_addresses WHERE region IS NULL OR region = ''
+		`).Scan(&stranded); err != nil {
+			return fmt.Errorf("failed to recount blank regions: %w", err)
+		}
+		if stranded > 0 {
+			return fmt.Errorf("%d row(s) still have no region after attribution: their coordinates fall outside "+
+				"every US state boundary, so the location is wrong rather than the state missing. "+
+				"Fix or delete them, then re-run", stranded)
+		}
+	}
+
 	// 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{
-		// 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.
+		// Case only. Blank regions were attributed from coordinates above, and
+		// an earlier revision of this migration wrote
+		// `SET region = 'OH' WHERE ... OR region <> 'OH'` here -- one statement
+		// instead of two, to save a table rewrite -- which relabels every
+		// Indiana, Pennsylvania and Michigan row as Ohio. Permanently, and with
+		// nothing in the logs to say so. The WHERE clause below touches only
+		// rows whose case actually differs.
 		`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 = ''`,
-
 		// SHARE lock: readers unaffected.
 		`CREATE UNIQUE INDEX IF NOT EXISTS idx_ohio_addresses_hash_region
 			ON ohio_addresses (hash, region)`,

services/multistate_integration_test.go

@@ -440,3 +440,92 @@ func TestOldKeyMakesNormalisationCollisionsImpossible(t *testing.T) {
 		t.Error("two rows shared a hash under the old constraint; a collision after normalisation would be possible")
 	}
 }
+
+// Production had 985,634 rows with no state at all -- about 17% of the table --
+// alongside rows for nine other regions. Stamping those 'OH' because it is "the
+// Ohio dataset" is the same mistake as the backfill that had to be reverted,
+// just quieter: border-county extracts genuinely contain Indiana, Pennsylvania
+// and Michigan addresses, and a mislabel is permanent and invisible.
+//
+// They are attributed from their coordinates instead, against the TIGER
+// boundaries us_states already holds.
+func TestBlankRegionsAreAttributedFromCoordinates(t *testing.T) {
+	// The pre-migration fixture: attribution runs before the region CHECK is
+	// added, so blanks have to be possible here or the test has nothing to
+	// attribute and skips itself into uselessness.
+	db := setupMultiStateDB(t, false)
+
+	if _, err := db.Exec(`
+		CREATE TABLE us_states (
+			id BIGSERIAL PRIMARY KEY,
+			state_fips VARCHAR(2) NOT NULL UNIQUE,
+			state_abbr VARCHAR(2) NOT NULL UNIQUE,
+			state_name VARCHAR(255) NOT NULL UNIQUE,
+			geometry GEOMETRY(MULTIPOLYGON, 4326)
+		)`); err != nil {
+		t.Fatalf("create us_states: %v", err)
+	}
+	if _, err := db.Exec(`
+		INSERT INTO us_states (state_fips, state_abbr, state_name, geometry) VALUES
+		 ('39','OH','Ohio',    ST_Multi(ST_MakeEnvelope(-85.0,38.4,-80.5,42.0,4326))),
+		 ('18','IN','Indiana', ST_Multi(ST_MakeEnvelope(-88.1,37.8,-85.0,41.8,4326)))
+	`); err != nil {
+		t.Fatalf("seed boundaries: %v", err)
+	}
+
+	// One blank in Ohio, one blank in Indiana. A single stamp cannot be right
+	// for both.
+	for _, row := range []struct {
+		hash, city string
+		lng, lat   float64
+	}{
+		{"blank-oh", "Columbus", -83.0, 40.0},
+		{"blank-in", "Fort Wayne", -85.5, 41.1},
+	} {
+		if _, 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','',$2,'','XX','','Unknown',
+			        ST_SetSRID(ST_MakePoint($3,$4),4326),'1 Main Street')
+		`, row.hash, row.city, row.lng, row.lat); err != nil {
+			t.Fatalf("seed %s: %v", row.hash, err)
+		}
+	}
+	// Blank them out. The column is NOT NULL before migration 23, so '' is the
+	// blank that actually occurs -- and is what production held 985,634 of.
+	if _, err := db.Exec(`UPDATE ohio_addresses SET region = '' WHERE hash LIKE 'blank-%'`); err != nil {
+		t.Fatalf("could not blank the regions the test depends on: %v", err)
+	}
+
+	// The attribution the migration performs.
+	if _, err := db.Exec(`
+		UPDATE ohio_addresses a
+		SET region = s.state_abbr
+		FROM us_states s
+		WHERE (a.region IS NULL OR a.region = '')
+		  AND s.geometry IS NOT NULL
+		  AND ST_Contains(s.geometry, a.geom)
+	`); err != nil {
+		t.Fatalf("attribute: %v", err)
+	}
+
+	got := map[string]string{}
+	rows, err := db.Query(`SELECT hash, region FROM ohio_addresses WHERE hash LIKE 'blank-%'`)
+	if err != nil {
+		t.Fatalf("read back: %v", err)
+	}
+	defer rows.Close()
+	for rows.Next() {
+		var h, r string
+		if err := rows.Scan(&h, &r); err != nil {
+			t.Fatalf("scan: %v", err)
+		}
+		got[h] = r
+	}
+
+	if got["blank-oh"] != "OH" {
+		t.Errorf("the Columbus row was attributed %q, want OH", got["blank-oh"])
+	}
+	if got["blank-in"] != "IN" {
+		t.Errorf("the Fort Wayne row was attributed %q, want IN -- stamping every blank OH is the bug this replaces", got["blank-in"])
+	}
+}