diff --git a/database/migrations.go b/database/migrations.go index a9ceb75..82241cf 100644 --- a/database/migrations.go +++ b/database/migrations.go @@ -1660,19 +1660,76 @@ func addRegionToAddressUniqueness() error { 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) + // A second pass for points that sit just outside a boundary. + // + // TIGER state polygons are land only, so a legitimate lakefront or + // coastal address -- Lake Erie shoreline, an island, a pier -- can fall + // outside every one of them by a few metres while being unambiguously + // in that state. Falling back to the nearest boundary within a short + // distance attributes those correctly; anything further away is a real + // coordinate error and is left for the check below. + res, err = tx.Exec(` + UPDATE ohio_addresses a + SET region = nearest.state_abbr + FROM ( + SELECT blank.id, s.state_abbr + FROM ohio_addresses blank + CROSS JOIN LATERAL ( + SELECT s2.state_abbr, s2.geometry + FROM us_states s2 + WHERE s2.geometry IS NOT NULL + ORDER BY s2.geometry <-> blank.geom + LIMIT 1 + ) s + WHERE (blank.region IS NULL OR blank.region = '') + AND ST_DWithin(blank.geom::geography, s.geometry::geography, $1) + ) nearest + WHERE a.id = nearest.id + `, shorelineToleranceMeters) + if err != nil { + return fmt.Errorf("failed to attribute blank regions from the nearest boundary: %w", err) + } + if n, _ := res.RowsAffected(); n > 0 { + log.Printf("Migration 23: attributed %d further row(s) from the nearest boundary within %.0fm", + n, shorelineToleranceMeters) + } + + // Whatever is left is a genuine coordinate error, not a missing state. + // The details go in the error rather than only the log, because that + // error is surfaced on /health -- so whoever has to decide what to do + // with these rows can see what they are without shell access. + rows, err := tx.Query(` + SELECT id, COALESCE(NULLIF(county, ''), '?'), + round(ST_X(geom)::numeric, 4), round(ST_Y(geom)::numeric, 4) + FROM ohio_addresses + WHERE region IS NULL OR region = '' + ORDER BY id + LIMIT 10 + `) + if err != nil { + return fmt.Errorf("failed to inspect rows with no region: %w", err) + } + var stranded []string + for rows.Next() { + var id int64 + var county string + var lng, lat float64 + if err := rows.Scan(&id, &county, &lng, &lat); err != nil { + rows.Close() + return fmt.Errorf("failed to read rows with no region: %w", err) + } + stranded = append(stranded, fmt.Sprintf("id=%d county=%s at (%.4f, %.4f)", id, county, lng, lat)) } - 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) + rows.Close() + if err := rows.Err(); err != nil { + return fmt.Errorf("failed to read rows with no region: %w", err) + } + + if len(stranded) > 0 { + return fmt.Errorf("%d row(s) still have no region: their coordinates are not within %.0fm of any US "+ + "state boundary, so the location is wrong rather than the state missing. Fix or delete them, "+ + "then re-run. Offending rows: %s", + len(stranded), shorelineToleranceMeters, strings.Join(stranded, "; ")) } } @@ -1819,6 +1876,15 @@ func RequireSchemaVersion(db *sql.DB, version int) error { "migrations are still running -- check /health and retry", applied.Int64, version) } +// shorelineToleranceMeters is how far outside a state boundary a point may sit +// and still be attributed to it. +// +// TIGER polygons stop at the waterline, so lakefront and island addresses are +// legitimately outside them by a small margin. 500m is comfortably more than +// that margin and far less than the distance to a neighbouring state, so it +// cannot silently move an address across a border. +const shorelineToleranceMeters = 500.0 + // SchemaVersionRegionUniqueness is migration 23, which keys address uniqueness // on (hash, region). Every ingest path's ON CONFLICT depends on the index it // creates. diff --git a/services/multistate_integration_test.go b/services/multistate_integration_test.go index 271dc6f..aa7ea63 100644 --- a/services/multistate_integration_test.go +++ b/services/multistate_integration_test.go @@ -529,3 +529,88 @@ func TestBlankRegionsAreAttributedFromCoordinates(t *testing.T) { t.Errorf("the Fort Wayne row was attributed %q, want IN -- stamping every blank OH is the bug this replaces", got["blank-in"]) } } + +// TIGER state polygons stop at the waterline, so a legitimate lakefront or +// island address can sit outside every state boundary by a few metres while +// being unambiguously in that state. Production had exactly three rows that +// ST_Contains could not place. +// +// The tolerance has to be wide enough for the waterline and far narrower than +// the distance to another state, so it can never move an address across a +// border. +func TestShorelineAddressesAttributeToTheNearestState(t *testing.T) { + 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) + } + // Ohio's northern edge at 41.7; Lake Erie is above it. + 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,41.7,4326))) + `); err != nil { + t.Fatalf("seed boundary: %v", err) + } + + cases := []struct { + hash string + lng, lat float64 + wantState string + why string + }{ + {"on-land", -83.0, 40.0, "OH", "well inside the boundary"}, + {"shoreline", -82.7, 41.7020, "OH", "about 220m offshore, still Ohio"}, + {"at-sea", -40.0, 35.0, "", "mid-Atlantic, a real coordinate error"}, + } + for _, c := range cases { + 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','','Somewhere','','','','Unknown', + ST_SetSRID(ST_MakePoint($2,$3),4326),'1 Main Street') + `, c.hash, c.lng, c.lat); err != nil { + t.Fatalf("seed %s: %v", c.hash, err) + } + } + + // Both attribution passes, in the order the migration runs them. + 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("contains pass: %v", err) + } + if _, err := db.Exec(` + UPDATE ohio_addresses a SET region = nearest.state_abbr + FROM ( + SELECT blank.id, s.state_abbr + FROM ohio_addresses blank + CROSS JOIN LATERAL ( + SELECT s2.state_abbr, s2.geometry FROM us_states s2 + WHERE s2.geometry IS NOT NULL + ORDER BY s2.geometry <-> blank.geom LIMIT 1 + ) s + WHERE (blank.region IS NULL OR blank.region = '') + AND ST_DWithin(blank.geom::geography, s.geometry::geography, 500) + ) nearest + WHERE a.id = nearest.id`); err != nil { + t.Fatalf("nearest pass: %v", err) + } + + for _, c := range cases { + var got string + if err := db.QueryRow(`SELECT region FROM ohio_addresses WHERE hash = $1`, c.hash).Scan(&got); err != nil { + t.Fatalf("read %s: %v", c.hash, err) + } + if got != c.wantState { + t.Errorf("%s (%s): region = %q, want %q", c.hash, c.why, got, c.wantState) + } + } +}