diff --git a/database/migrations.go b/database/migrations.go index eb8bc70..a9ceb75 100644 --- a/database/migrations.go +++ b/database/migrations.go @@ -1619,14 +1619,63 @@ 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 @@ -1634,24 +1683,17 @@ func addRegionToAddressUniqueness() error { // 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)`, diff --git a/services/multistate_integration_test.go b/services/multistate_integration_test.go index bdf0607..271dc6f 100644 --- a/services/multistate_integration_test.go +++ b/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"]) + } +}