diff --git a/api-docs.yaml b/api-docs.yaml index 1ef50e0..7aae1f0 100644 --- a/api-docs.yaml +++ b/api-docs.yaml @@ -525,6 +525,17 @@ paths: schema: type: string example: "Columbus" + - name: state + in: query + required: false + description: >- + Two-letter state code. County names collide across states - roughly + two dozen have a Franklin County - so a county filter without a state + is ambiguous once more than one state is loaded. Case-insensitive. + schema: + type: string + maxLength: 2 + example: OH - name: county in: query required: false diff --git a/database/migrations.go b/database/migrations.go index aad2547..362e3c0 100644 --- a/database/migrations.go +++ b/database/migrations.go @@ -155,6 +155,12 @@ func RunMigrations() error { Up: addUsageCounters, Down: removeUsageCounters, }, + { + Version: 23, + Description: "Key address uniqueness on (hash, region) so a second state can be loaded", + Up: addRegionToAddressUniqueness, + Down: revertRegionAddressUniqueness, + }, } // Create migrations table if it doesn't exist if err := createMigrationsTable(); err != nil { return fmt.Errorf("failed to create migrations table: %w", err) @@ -1550,3 +1556,194 @@ func removeUsageCounters() error { } return nil } + +// addRegionToAddressUniqueness makes the address dedup key state-aware. +// +// ohio_addresses.hash is HouseNumber|Street|Unit|City|Postcode and the column +// is UNIQUE. No state anywhere in it. With a postcode present that mostly does +// not matter -- US ZIPs are state-specific, so they disentangle the key -- but +// ingestion only requires a house number and a street, and county GeoJSON +// extracts frequently ship without a ZIP column. For those rows the key +// collapses to house number, street, unit and city, and city names collide +// across states constantly: Springfield, Columbus, Franklin. +// +// The failure is silent and looks like success. A colliding row is rejected by +// the unique constraint, the importer counts it as a duplicate, and a load that +// quietly discarded half a state reports "N records imported, M duplicates +// skipped" and completes green. +// +// The fix is the uniqueness key, not the hash contents. Recomputing every hash +// would rewrite all ~6M rows; keying on (hash, region) is one index build and +// means exactly the same thing. +// +// LOCKING. Statement order here is load-bearing. ALTER TABLE ... SET NOT NULL, +// ADD CONSTRAINT and DROP CONSTRAINT each take ACCESS EXCLUSIVE, which blocks +// reads as well as writes, and a lock once taken is held until the transaction +// commits. Taking one before the index build would block every /search and +// /geocode query for the whole build -- and migrations run asynchronously while +// the server is already serving. So the two index builds (SHARE: reads and +// other readers fine, writers wait) come first, and everything needing ACCESS +// EXCLUSIVE is last, where it holds that lock only for its own validation scan. +func addRegionToAddressUniqueness() error { + tx, err := DB.Begin() + if err != nil { + return fmt.Errorf("failed to begin address uniqueness migration: %w", err) + } + 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. + 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, ', '), '') + 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) + } + 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) + } + + statements := []string{ + // One pass, not two. Each full-table UPDATE rewrites every tuple, + // roughly doubling the table until vacuum and generating WAL to match. + `UPDATE ohio_addresses + SET region = 'OH' + WHERE region IS NULL OR region = '' OR region <> 'OH'`, + + // SHARE lock: readers unaffected. + `CREATE UNIQUE INDEX IF NOT EXISTS idx_ohio_addresses_hash_region + ON ohio_addresses (hash, region)`, + `CREATE INDEX IF NOT EXISTS idx_ohio_addresses_region ON ohio_addresses (region)`, + + // From here on, ACCESS EXCLUSIVE. Kept last and kept short. + `ALTER TABLE ohio_addresses DROP CONSTRAINT IF EXISTS ohio_addresses_hash_key`, + `ALTER TABLE ohio_addresses ALTER COLUMN region SET NOT NULL`, + `ALTER TABLE ohio_addresses DROP CONSTRAINT IF EXISTS ohio_addresses_region_not_blank`, + `ALTER TABLE ohio_addresses ADD CONSTRAINT ohio_addresses_region_not_blank CHECK (region <> '')`, + } + + for _, stmt := range statements { + if _, err := tx.Exec(stmt); err != nil { + return fmt.Errorf("failed to key address uniqueness on region: %w", err) + } + } + + // DROP CONSTRAINT IF EXISTS is a no-op when the constraint carries some + // other name. That would leave a live global unique hash beside the new + // key, the migration would commit, and this function would log that a + // second state can be loaded while the second state was still being + // rejected as duplicates -- the exact silent loss being fixed. Assert. + var remaining string + err = tx.QueryRow(` + SELECT COALESCE(string_agg(conname, ', '), '') + FROM pg_constraint + WHERE conrelid = 'ohio_addresses'::regclass + AND contype = 'u' + AND array_length(conkey, 1) = 1 + AND conkey[1] = ( + SELECT attnum FROM pg_attribute + WHERE attrelid = 'ohio_addresses'::regclass AND attname = 'hash' + ) + `).Scan(&remaining) + if err != nil { + return fmt.Errorf("failed to verify the global unique hash was dropped: %w", err) + } + if remaining != "" { + return fmt.Errorf("a single-column unique constraint on hash still exists (%s); "+ + "a second state would still be rejected as duplicates", remaining) + } + + if err := tx.Commit(); err != nil { + return fmt.Errorf("failed to commit address uniqueness migration: %w", err) + } + + log.Println("Migration 23: address uniqueness is now (hash, region); a second state can be loaded") + return nil +} + +// revertRegionAddressUniqueness restores the global unique hash. +// +// This can fail, legitimately: once a second state is loaded there are rows +// sharing a hash across regions, and no single-column unique constraint can +// hold them. That is the migration telling you the rollback would destroy +// data, and it is the correct outcome -- the fix is to roll forward. +// +// Which is exactly why it runs in a transaction. Without one, the composite +// key and the CHECK are dropped before the ADD CONSTRAINT fails, leaving the +// table with no uniqueness at all -- every ON CONFLICT (hash, region) in the +// ingest paths then errors with 42P10 and deduplication is off entirely. The +// documented "correct outcome" has to be a clean no-op, not a half-dropped +// schema. +func revertRegionAddressUniqueness() error { + tx, err := DB.Begin() + if err != nil { + return fmt.Errorf("failed to begin address uniqueness rollback: %w", err) + } + defer tx.Rollback() + + statements := []string{ + `DROP INDEX IF EXISTS idx_ohio_addresses_region`, + `DROP INDEX IF EXISTS idx_ohio_addresses_hash_region`, + `ALTER TABLE ohio_addresses DROP CONSTRAINT IF EXISTS ohio_addresses_region_not_blank`, + `ALTER TABLE ohio_addresses ALTER COLUMN region DROP NOT NULL`, + `ALTER TABLE ohio_addresses ADD CONSTRAINT ohio_addresses_hash_key UNIQUE (hash)`, + } + + for _, stmt := range statements { + if _, err := tx.Exec(stmt); err != nil { + return fmt.Errorf("failed to restore the global unique hash, rolled back with the "+ + "composite key intact (rows from more than one state may share a hash): %w", err) + } + } + + return tx.Commit() +} + +// RequireSchemaVersion reports an error unless migrations have reached version. +// +// Migrations run asynchronously by default and the server serves immediately, +// so between a deploy and a migration landing there is a window where code that +// needs new schema is live and the schema is not. Read paths can degrade -- the +// radius search falls back to an unindexed query, the rate limiter falls back +// to the aggregate -- but a write path that needs a constraint cannot: an +// import whose ON CONFLICT target does not exist yet fails every batch and +// marks the dataset failed, which looks like corrupt input rather than a +// transient deploy state. +// +// Call this at the top of such a path so the answer is "migrations pending" +// instead. +// The db is passed rather than read from the package global because callers +// hold their own handle -- DatasetService reads its dataset row through s.db, +// and checking the schema on a different connection than the one the work runs +// on is how a service ends up validating one database and writing to another. +func RequireSchemaVersion(db *sql.DB, version int) error { + if db == nil { + db = DB + } + if db == nil { + return fmt.Errorf("no database connection available to check the schema version") + } + + var applied sql.NullInt64 + if err := db.QueryRow(`SELECT MAX(version) FROM schema_migrations`).Scan(&applied); err != nil { + return fmt.Errorf("cannot determine schema version: %w", err) + } + if int(applied.Int64) >= version { + return nil + } + return fmt.Errorf("database schema is at version %d and this operation needs %d; "+ + "migrations are still running -- check /health and retry", applied.Int64, version) +} + +// SchemaVersionRegionUniqueness is migration 23, which keys address uniqueness +// on (hash, region). Every ingest path's ON CONFLICT depends on the index it +// creates. +const SchemaVersionRegionUniqueness = 23 diff --git a/handlers/address_handlers.go b/handlers/address_handlers.go index 0907494..1bb3ca8 100644 --- a/handlers/address_handlers.go +++ b/handlers/address_handlers.go @@ -6,6 +6,7 @@ import ( "math" "net/http" "strconv" + "strings" "geocoding-api/models" "geocoding-api/services" @@ -19,6 +20,19 @@ func SearchOhioAddressesHandler(c echo.Context) error { // Manually parse query parameters (Echo's Bind doesn't always work for query params) params.Query = c.QueryParam("query") + if raw := strings.TrimSpace(c.QueryParam("state")); raw != "" { + // Without this, ?state=Ohio and ?state=%20 both return an empty page + // with no explanation, because neither matches a two-letter region. + code, err := validateStateCode(raw) + if err != nil { + return c.JSON(http.StatusBadRequest, map[string]interface{}{ + "success": false, + "error": err.Error(), + "example": "state=OH", + }) + } + params.State = code + } params.County = c.QueryParam("county") params.City = c.QueryParam("city") params.Postcode = c.QueryParam("postcode") @@ -89,6 +103,9 @@ func SearchOhioAddressesHandler(c echo.Context) error { // Prepare filters for response filters := make(map[string]any) + if params.State != "" { + filters["state"] = params.State + } if params.County != "" { filters["county"] = params.County } diff --git a/handlers/dataset_handlers.go b/handlers/dataset_handlers.go index 3d0e228..7733bd9 100644 --- a/handlers/dataset_handlers.go +++ b/handlers/dataset_handlers.go @@ -3,6 +3,7 @@ package handlers import ( "encoding/json" "fmt" + "geocoding-api/utils" "io" "mime/multipart" "net/http" @@ -56,6 +57,14 @@ func UploadDatasetHandler(c echo.Context) error { // Get form values name := c.FormValue("name") state := c.FormValue("state") + normalizedState, stateErr := validateStateCode(state) + if stateErr != nil { + return c.JSON(http.StatusBadRequest, GeocodeResponse{ + Success: false, + Error: stateErr.Error(), + }) + } + state = normalizedState county := c.FormValue("county") if name == "" || state == "" || county == "" { @@ -149,6 +158,14 @@ func UploadMultipleHandler(c echo.Context) error { // Get form values state := c.FormValue("state") + normalizedState, stateErr := validateStateCode(state) + if stateErr != nil { + return c.JSON(http.StatusBadRequest, GeocodeResponse{ + Success: false, + Error: stateErr.Error(), + }) + } + state = normalizedState fmt.Printf("[BulkUpload] State: %s\n", state) if state == "" { @@ -322,6 +339,14 @@ func UploadMultipleStreamHandler(c echo.Context) error { // Get form values state := c.FormValue("state") + normalizedState, stateErr := validateStateCode(state) + if stateErr != nil { + return c.JSON(http.StatusBadRequest, GeocodeResponse{ + Success: false, + Error: stateErr.Error(), + }) + } + state = normalizedState if state == "" { return c.JSON(http.StatusBadRequest, map[string]interface{}{ "success": false, @@ -832,3 +857,23 @@ func GetDatasetStatsHandler(c echo.Context) error { "data": stats, }) } + +// validateStateCode rejects anything that is not a real two-letter US state +// code, and returns the canonical upper-case form. +// +// region is half the address uniqueness key as of migration 23. An operator +// typing "HO" instead of "OH" now creates a separate dedup bucket: the import +// succeeds green, and re-uploading the same counties under the correct code +// inserts a second full copy of every row rather than being deduplicated. +// Before that migration the global hash absorbed the typo; now it cannot, so +// the code has to be checked rather than merely upper-cased. +func validateStateCode(raw string) (string, error) { + code := strings.ToUpper(strings.TrimSpace(raw)) + if code == "" { + return "", fmt.Errorf("state is required") + } + if !utils.IsUSStateCode(code) { + return "", fmt.Errorf("state must be a two-letter US state code, got %q", raw) + } + return code, nil +} diff --git a/models/address.go b/models/address.go index d88cade..a11484a 100644 --- a/models/address.go +++ b/models/address.go @@ -67,7 +67,11 @@ const ( // AddressSearchParams represents search parameters for address queries type AddressSearchParams struct { - Query string `json:"query" form:"query"` // General search query + Query string `json:"query" form:"query"` // General search query + // State is the two-letter code. Filtering by county alone is unsafe once + // more than one state is loaded: roughly two dozen states have a Franklin + // County, and a county filter without a state silently mixes them. + State string `json:"state" form:"state"` County string `json:"county" form:"county"` // Filter by county City string `json:"city" form:"city"` // Filter by city Postcode string `json:"postcode" form:"postcode"` // Filter by postal code diff --git a/services/address_service.go b/services/address_service.go index b0bd9d4..966e952 100644 --- a/services/address_service.go +++ b/services/address_service.go @@ -168,6 +168,14 @@ func (s *AddressService) searchAddresses(q querier, params models.AddressSearchP } } + // State filter. Exact match on the indexed region column, upper-cased so a + // caller passing "oh" is not silently told there is no data. + if state := strings.ToUpper(strings.TrimSpace(params.State)); state != "" { + conditions = append(conditions, fmt.Sprintf("region = $%d", argIndex)) + args = append(args, state) + argIndex++ + } + // Bounding box. if params.BBox != nil { conditions = append(conditions, fmt.Sprintf(BBoxPredicateSQL, @@ -769,6 +777,18 @@ func (s *AddressService) searchByComponents(parsed *utils.ParsedAddress, limit i argNum++ } + // The parser extracts a state and this path discarded it. Once a second + // state is loaded, geocoding "100 Main St, Springfield, IL" matches the + // Ohio row on street and city and returns it at full confidence -- exactly + // the cross-state collision this schema change exists to prevent, on the + // endpoint that matters most. + stateArg := 0 + if parsed.State != "" { + stateArg = argNum + args = append(args, strings.ToUpper(strings.TrimSpace(parsed.State))) + argNum++ + } + zipArg := 0 if parsed.Zip != "" { zipArg = argNum @@ -795,11 +815,19 @@ func (s *AddressService) searchByComponents(parsed *utils.ParsedAddress, limit i if len(exclusions) > 0 { exclusionClause = " AND " + strings.Join(exclusions, " AND ") } + // State is an anchor on every tier, not a tier of its own. What relaxes + // as the tiers widen is street-level detail; the state a caller named + // never relaxes, or a query for Springfield IL eventually matches + // Springfield OH and reports it as a hit. + stateClause := "" + if stateArg > 0 { + stateClause = fmt.Sprintf(" AND region = $%d", stateArg) + } tierCTEs = append(tierCTEs, fmt.Sprintf(`%s AS ( SELECT %s, %d as tier FROM ohio_addresses - WHERE %s%s + WHERE %s%s%s LIMIT %d - )`, tierName, selectFields, tierNum, whereClause, exclusionClause, limit)) + )`, tierName, selectFields, tierNum, whereClause, stateClause, exclusionClause, limit)) tierSelects = append(tierSelects, fmt.Sprintf("SELECT * FROM %s", tierName)) exclusions = append(exclusions, fmt.Sprintf("id NOT IN (SELECT id FROM %s)", tierName)) if isExact { diff --git a/services/dataset_ingest_integration_test.go b/services/dataset_ingest_integration_test.go index ee45243..d92f3b4 100644 --- a/services/dataset_ingest_integration_test.go +++ b/services/dataset_ingest_integration_test.go @@ -107,19 +107,32 @@ func setupIngestSchema(t *testing.T, db *sql.DB) { stmts := []string{ `CREATE TABLE ohio_addresses ( id BIGSERIAL PRIMARY KEY, - hash VARCHAR(255) UNIQUE NOT NULL, + hash VARCHAR(255) NOT NULL, house_number VARCHAR(50), street VARCHAR(255), unit VARCHAR(50), city VARCHAR(255), district VARCHAR(10), - region VARCHAR(2), + region VARCHAR(2) NOT NULL, postcode VARCHAR(10), county VARCHAR(255), geom GEOMETRY(POINT, 4326) NOT NULL, full_address TEXT, created_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP )`, + // Uniqueness is (hash, region) as of migration 23: the hash carries no + // state, so a global unique constraint silently dropped a second + // state's rows and reported them as duplicates. + "CREATE UNIQUE INDEX ON ohio_addresses (hash, region)", + // NOT NULL does not stop an empty string, and '' would put every + // stateless row in one bucket. Without the CHECK here, that case passes + // in the suite and fails in production. + "ALTER TABLE ohio_addresses ADD CONSTRAINT ohio_addresses_region_not_blank CHECK (region <> '')", + // ProcessGeoJSONDataset refuses to run until migrations have reached + // the version whose index its ON CONFLICT needs, so the fixture has to + // carry the same record production does. + "CREATE TABLE IF NOT EXISTS schema_migrations (version INTEGER PRIMARY KEY, applied_at TIMESTAMP DEFAULT NOW())", + "INSERT INTO schema_migrations (version) VALUES (23) ON CONFLICT DO NOTHING", `CREATE OR REPLACE FUNCTION update_full_address() RETURNS TRIGGER AS $$ BEGIN diff --git a/services/dataset_service.go b/services/dataset_service.go index 8408fd5..ed9798f 100644 --- a/services/dataset_service.go +++ b/services/dataset_service.go @@ -6,6 +6,7 @@ import ( "encoding/json" "errors" "fmt" + "geocoding-api/database" "io" "log" "os" @@ -323,6 +324,14 @@ const addressProgressInterval = 25000 // are inserted in batches. A 500k-address county extract used to mean 500k // round trips and a 500k-element slice resident in memory. func (s *DatasetService) ProcessGeoJSONDataset(datasetID int) error { + // The batched insert below uses ON CONFLICT (hash, region), whose index + // migration 23 creates. Migrations run asynchronously, so without this a + // deploy-window import fails every batch and marks the dataset failed -- + // indistinguishable from a corrupt file. + if err := database.RequireSchemaVersion(s.db, database.SchemaVersionRegionUniqueness); err != nil { + return fmt.Errorf("cannot import addresses yet: %w", err) + } + dataset, err := s.GetDatasetByID(datasetID) if err != nil { return fmt.Errorf("failed to get dataset: %w", err) @@ -553,7 +562,10 @@ func addressFromFeature(feature geoFeature, county, state string) (models.OhioAd // Set county and state from dataset metadata (full names) address.County = county - address.Region = state + // Upper-cased because the uniqueness key is (hash, region): 'oh' and 'OH' + // are distinct values to the index, so a lowercase upload would reintroduce + // the cross-state duplicates migration 23 closed. + address.Region = strings.ToUpper(strings.TrimSpace(state)) if address.HouseNumber == "" || address.Street == "" { return address, false @@ -710,7 +722,7 @@ func (ai *addressImporter) insertBatch(batch []models.OhioAddress) (int, error) INSERT INTO ohio_addresses ( hash, house_number, street, unit, city, district, region, postcode, county, geom ) VALUES ` + strings.Join(values, ", ") + ` - ON CONFLICT (hash) DO NOTHING + ON CONFLICT (hash, region) DO NOTHING ` result, err := ai.db.Exec(query, args...) diff --git a/services/multistate_integration_test.go b/services/multistate_integration_test.go new file mode 100644 index 0000000..0c3a612 --- /dev/null +++ b/services/multistate_integration_test.go @@ -0,0 +1,355 @@ +package services + +import ( + "database/sql" + "fmt" + "os" + "testing" + + "geocoding-api/models" + + _ "github.com/lib/pq" +) + +const multiStateSchema = "multistate_probe" + +// setupMultiStateDB builds the table with the post-migration-23 uniqueness key. +func setupMultiStateDB(t *testing.T, keyedOnRegion bool) *sql.DB { + t.Helper() + + dsn := os.Getenv("PROBE_DSN") + if dsn == "" { + t.Skip("PROBE_DSN not set") + } + + db, err := sql.Open("postgres", dsn) + if err != nil { + t.Fatalf("open: %v", err) + } + db.SetMaxOpenConns(1) + if err := db.Ping(); err != nil { + t.Skipf("probe database unreachable: %v", err) + } + + unique := "hash VARCHAR(255) UNIQUE NOT NULL" + if keyedOnRegion { + unique = "hash VARCHAR(255) NOT NULL" + } + + stmts := []string{ + "CREATE EXTENSION IF NOT EXISTS postgis", + "CREATE EXTENSION IF NOT EXISTS pg_trgm", + fmt.Sprintf("DROP SCHEMA IF EXISTS %s CASCADE", multiStateSchema), + fmt.Sprintf("CREATE SCHEMA %s", multiStateSchema), + fmt.Sprintf("SET search_path TO %s, public", multiStateSchema), + fmt.Sprintf(`CREATE TABLE ohio_addresses ( + id BIGSERIAL PRIMARY KEY, + %s, + house_number VARCHAR(50), street VARCHAR(255), unit VARCHAR(50), + city VARCHAR(255), district VARCHAR(10), region VARCHAR(2) NOT NULL, + postcode VARCHAR(10), county VARCHAR(255), + geom GEOMETRY(POINT, 4326) NOT NULL, + created_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP, + full_address TEXT + )`, unique), + } + if keyedOnRegion { + stmts = append(stmts, "CREATE UNIQUE INDEX ON ohio_addresses (hash, region)", + "ALTER TABLE ohio_addresses ADD CONSTRAINT ohio_addresses_region_not_blank CHECK (region <> '')") + } + stmts = append(stmts, + "CREATE INDEX ON ohio_addresses (region)", + // Text search runs against the generated tsvector, so a fixture without + // it cannot exercise a query combined with the territory filters. + `ALTER TABLE ohio_addresses ADD COLUMN fts tsvector + GENERATED ALWAYS AS (to_tsvector('simple', coalesce(full_address, ''))) STORED`, + "CREATE INDEX ON ohio_addresses USING gin (fts)", + "CREATE INDEX ON ohio_addresses USING gin (full_address gin_trgm_ops)", + ) + + for _, stmt := range stmts { + if _, err := db.Exec(stmt); err != nil { + t.Fatalf("setup failed on %.60q: %v", stmt, err) + } + } + + t.Cleanup(func() { + if _, err := db.Exec(fmt.Sprintf("DROP SCHEMA IF EXISTS %s CASCADE", multiStateSchema)); err != nil { + t.Logf("cleanup: %v", err) + } + db.Close() + }) + + return db +} + +// insertAddress mirrors what the importer writes, including the hash it builds. +func insertAddress(db *sql.DB, house, street, unit, city, postcode, county, region string) (int64, error) { + hash := fmt.Sprintf("%s|%s|%s|%s|%s", house, street, unit, city, postcode) + var id int64 + err := db.QueryRow(` + INSERT INTO ohio_addresses (hash, house_number, street, unit, city, district, region, postcode, county, geom, full_address) + VALUES ($1,$2,$3,$4,$5,'',$6,$7,$8, ST_SetSRID(ST_MakePoint(-83.0, 40.0), 4326), $9) + ON CONFLICT (hash, region) DO NOTHING + RETURNING id + `, hash, house, street, unit, city, region, postcode, county, + fmt.Sprintf("%s %s, %s, %s %s", house, street, city, region, postcode)).Scan(&id) + return id, err +} + +// The blocker, demonstrated. Two states, the same street address, no ZIP -- +// which ingestion permits, since it requires only a house number and a street, +// and county GeoJSON extracts frequently ship without a ZIP column. +func TestSecondStateSurvivesWithoutAPostcode(t *testing.T) { + db := setupMultiStateDB(t, true) + + if _, err := insertAddress(db, "100", "Main Street", "", "Springfield", "", "Clark", "OH"); err != nil { + t.Fatalf("insert Ohio row: %v", err) + } + if _, err := insertAddress(db, "100", "Main Street", "", "Springfield", "", "Sangamon", "IL"); err != nil { + if err == sql.ErrNoRows { + t.Fatal("the Illinois row was silently discarded as a duplicate of the Ohio one") + } + t.Fatalf("insert Illinois row: %v", err) + } + + var n int + if err := db.QueryRow(`SELECT COUNT(*) FROM ohio_addresses`).Scan(&n); err != nil { + t.Fatalf("count: %v", err) + } + if n != 2 { + t.Errorf("%d row(s) survived, want 2 -- a state's data is being dropped", n) + } +} + +// The old key, so the regression this closes is on the record. Under a global +// unique hash the second state's row vanishes and the importer reports it as a +// duplicate, so a load that lost half a state completes green. +func TestGlobalHashDropsTheSecondState(t *testing.T) { + db := setupMultiStateDB(t, false) + + hash := "100|Main Street||Springfield|" + insert := func(region, county string) error { + _, err := db.Exec(` + INSERT INTO ohio_addresses (hash, house_number, street, unit, city, district, region, postcode, county, geom) + VALUES ($1,'100','Main Street','','Springfield','',$2,'',$3, ST_SetSRID(ST_MakePoint(-83.0,40.0),4326)) + ON CONFLICT (hash) DO NOTHING + `, hash, region, county) + return err + } + + if err := insert("OH", "Clark"); err != nil { + t.Fatalf("ohio: %v", err) + } + if err := insert("IL", "Sangamon"); err != nil { + t.Fatalf("illinois: %v", err) + } + + var n int + if err := db.QueryRow(`SELECT COUNT(*) FROM ohio_addresses`).Scan(&n); err != nil { + t.Fatalf("count: %v", err) + } + if n != 1 { + t.Fatalf("expected the old key to drop the second state, got %d rows", n) + } + + var region string + if err := db.QueryRow(`SELECT region FROM ohio_addresses`).Scan(®ion); err != nil { + t.Fatalf("survivor: %v", err) + } + t.Logf("under the old key only %s survived; Illinois was counted as a duplicate", region) +} + +// Duplicates within one state must still be rejected -- widening the key must +// not turn off deduplication. +func TestDuplicatesWithinAStateAreStillRejected(t *testing.T) { + db := setupMultiStateDB(t, true) + + if _, err := insertAddress(db, "100", "Main Street", "", "Springfield", "", "Clark", "OH"); err != nil { + t.Fatalf("first insert: %v", err) + } + _, err := insertAddress(db, "100", "Main Street", "", "Springfield", "", "Clark", "OH") + if err != sql.ErrNoRows { + t.Errorf("a genuine duplicate was accepted (err=%v); dedup is off", err) + } + + var n int + db.QueryRow(`SELECT COUNT(*) FROM ohio_addresses`).Scan(&n) + if n != 1 { + t.Errorf("%d rows after inserting the same address twice, want 1", n) + } +} + +// County names collide across states, so a county filter without a state is +// ambiguous the moment a second state exists. +func TestStateFilterSeparatesCollidingCounties(t *testing.T) { + db := setupMultiStateDB(t, true) + svc := NewAddressService(db) + + for _, row := range []struct{ house, city, county, region string }{ + {"1", "Columbus", "Franklin", "OH"}, + {"2", "Columbus", "Franklin", "OH"}, + {"3", "Winchester", "Franklin", "IN"}, + } { + if _, err := insertAddress(db, row.house, "Main Street", "", row.city, "", row.county, row.region); err != nil { + t.Fatalf("seed %s/%s: %v", row.region, row.county, err) + } + } + + _, bothStates, err := svc.SearchAddresses(models.AddressSearchParams{County: "Franklin", Limit: 50}) + if err != nil { + t.Fatalf("county only: %v", err) + } + if bothStates != 3 { + t.Errorf("county filter alone returned %d, want 3 -- it should still match across states", bothStates) + } + + rows, ohioOnly, err := svc.SearchAddresses(models.AddressSearchParams{ + County: "Franklin", State: "OH", Limit: 50, + }) + if err != nil { + t.Fatalf("county + state: %v", err) + } + if ohioOnly != 2 { + t.Errorf("Franklin County, OH returned %d, want 2", ohioOnly) + } + for _, r := range rows { + if r.Region != "OH" { + t.Errorf("%s is in %s but the filter asked for OH", r.FullAddress, r.Region) + } + } + t.Logf("Franklin County: %d across states, %d in Ohio", bothStates, ohioOnly) +} + +// A caller passing a lowercase code should not be told there is no data. +func TestStateFilterIsCaseInsensitive(t *testing.T) { + db := setupMultiStateDB(t, true) + svc := NewAddressService(db) + + if _, err := insertAddress(db, "1", "Main Street", "", "Columbus", "", "Franklin", "OH"); err != nil { + t.Fatalf("seed: %v", err) + } + + for _, code := range []string{"OH", "oh", " oh "} { + _, total, err := svc.SearchAddresses(models.AddressSearchParams{State: code, Limit: 10}) + if err != nil { + t.Fatalf("state %q: %v", code, err) + } + if total != 1 { + t.Errorf("state %q returned %d rows, want 1", code, total) + } + } +} + +// The geocoding path parsed a state out of the query and threw it away. Once a +// second state is loaded, "100 Main St, Springfield, IL" matched the Ohio row +// on street and city and came back at full confidence -- the exact collision +// this whole change exists to prevent, on the endpoint that matters most. +func TestGeocodingRespectsTheStateInTheQuery(t *testing.T) { + db := setupMultiStateDB(t, true) + svc := NewAddressService(db) + + if _, err := insertAddress(db, "100", "Main Street", "", "Springfield", "45503", "Clark", "OH"); err != nil { + t.Fatalf("seed OH: %v", err) + } + if _, err := insertAddress(db, "100", "Main Street", "", "Springfield", "62701", "Sangamon", "IL"); err != nil { + t.Fatalf("seed IL: %v", err) + } + + result, err := svc.FullTextSearchAddresses("100 Main St, Springfield, IL", 10) + if err != nil { + t.Fatalf("geocode: %v", err) + } + if len(result.Addresses) == 0 { + t.Fatal("no match for an address that exists") + } + for _, a := range result.Addresses { + if a.Region != "IL" { + t.Errorf("query named IL but %s in %s came back", a.FullAddress, a.Region) + } + } + t.Logf("query named IL, got %d row(s), all in IL", len(result.Addresses)) + + // And the same query for Ohio must return the Ohio row, not the Illinois + // one -- the filter has to select, not merely exclude. + result, err = svc.FullTextSearchAddresses("100 Main St, Springfield, OH", 10) + if err != nil { + t.Fatalf("geocode OH: %v", err) + } + for _, a := range result.Addresses { + if a.Region != "OH" { + t.Errorf("query named OH but %s in %s came back", a.FullAddress, a.Region) + } + } +} + +// A blank region would put every stateless row into one uniqueness bucket, +// reintroducing the collision for any dataset uploaded without a state. +func TestBlankRegionIsRejected(t *testing.T) { + db := setupMultiStateDB(t, true) + + _, err := db.Exec(` + INSERT INTO ohio_addresses (hash, house_number, street, unit, city, district, region, postcode, county, geom) + VALUES ('blank','1','Main Street','','Columbus','','','43004','Franklin', ST_SetSRID(ST_MakePoint(-83.0,40.0),4326)) + `) + if err == nil { + t.Error("an empty region was accepted; every stateless row would share one uniqueness bucket") + } +} + +// State and territory filters arrived on separate branches and first met in a +// merge. They share the hand-numbered placeholder sequence -- the bbox consumes +// four positions -- so a mistake there binds the wrong value to the wrong +// column and returns plausible rows from the wrong place. +func TestStateAndBBoxComposeCorrectly(t *testing.T) { + db := setupMultiStateDB(t, true) + svc := NewAddressService(db) + + // Same coordinates, two states, so only the state filter can separate them. + for _, row := range []struct{ house, county, region string }{ + {"1", "Franklin", "OH"}, + {"2", "Franklin", "OH"}, + {"3", "Sangamon", "IL"}, + } { + if _, err := insertAddress(db, row.house, "Main Street", "", "Springfield", "", row.county, row.region); err != nil { + t.Fatalf("seed %s: %v", row.region, err) + } + } + + box := &models.BoundingBox{MinLng: -83.1, MinLat: 39.9, MaxLng: -82.9, MaxLat: 40.1} + + _, boxOnly, err := svc.SearchAddresses(models.AddressSearchParams{BBox: box, Limit: 50}) + if err != nil { + t.Fatalf("bbox only: %v", err) + } + if boxOnly != 3 { + t.Fatalf("bbox alone returned %d, want all 3 seeded rows", boxOnly) + } + + rows, combined, err := svc.SearchAddresses(models.AddressSearchParams{ + BBox: box, State: "OH", Limit: 50, + }) + if err != nil { + t.Fatalf("bbox + state: %v", err) + } + if combined != 2 { + t.Errorf("bbox + state=OH returned %d, want 2; the placeholders may be misaligned", combined) + } + for _, r := range rows { + if r.Region != "OH" { + t.Errorf("%s is in %s despite state=OH", r.FullAddress, r.Region) + } + } + + // And with a text query on top, which adds placeholders of its own. + _, withQuery, err := svc.SearchAddresses(models.AddressSearchParams{ + BBox: box, State: "OH", Query: "Main", Limit: 50, + }) + if err != nil { + t.Fatalf("bbox + state + query: %v", err) + } + if withQuery != 2 { + t.Errorf("bbox + state + query returned %d, want 2", withQuery) + } + t.Logf("bbox %d, +state %d, +query %d", boxOnly, combined, withQuery) +} diff --git a/services/ohio_address_service.go b/services/ohio_address_service.go index 15f7fc9..5256978 100644 --- a/services/ohio_address_service.go +++ b/services/ohio_address_service.go @@ -314,7 +314,7 @@ func loadCountyAddresses(county, filePath string) (int, error) { INSERT INTO ohio_addresses ( hash, house_number, street, unit, city, district, region, postcode, county, geom ) VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, ST_SetSRID(ST_MakePoint($10, $11), 4326)) - ON CONFLICT (hash) DO NOTHING + ON CONFLICT (hash, region) DO NOTHING `) if err != nil { return 0, fmt.Errorf("failed to prepare statement: %w", err) @@ -340,10 +340,18 @@ func loadCountyAddresses(county, filePath string) (int, error) { streetName := getStringProperty(props, "street", "ST_NAME", "StreetName", "street_name", "STREETNAME", "LSN") unit := getStringProperty(props, "unit", "UNITNUM", "Unit", "UNIT") city := getStringProperty(props, "city", "USPS_CITY", "City", "CITY", "MUNI") - state := getStringProperty(props, "region", "STATE", "State", "state", "REGION") - // Truncate state to 2 characters to match database schema VARCHAR(2) - if len(state) > 2 { - state = state[:2] + // region is the dedup key's second column as of migration 23, so it has + // to be normalised the same way the dataset importer does. Truncating + // blindly was wrong on both counts: "Ohio" became "Oh", a different + // uniqueness bucket from "OH" and invisible to a region = UPPER($1) + // filter, and an extract with no state property yielded "", which the + // region_not_blank CHECK now rejects on every row -- and since that + // error text is not "duplicate key", the loop below would log a warning + // per row and return 0 inserted with a nil error, reporting a total + // failure as success. + state := normalizeStateCode(getStringProperty(props, "region", "STATE", "State", "state", "REGION")) + if state == "" { + state = fallbackState } zipCode := getStringProperty(props, "postcode", "ZIPCODE", "ZipCode", "zip_code", "POSTCODE") // Use existing hash if available (OpenAddresses format), otherwise generate one @@ -449,3 +457,36 @@ func decompressIfNeeded(geojsonPath string) error { log.Printf("Successfully decompressed %s", filepath.Base(geojsonPath)) return nil } + +// fallbackState is used when an extract carries no state property at all. +// +// Every dataset this loader has ever been pointed at is Ohio, and the +// alternative is an empty region, which the region_not_blank CHECK rejects on +// every row. Named rather than inlined so the assumption is findable when a +// second state is loaded through this path. +const fallbackState = "OH" + +// normalizeStateCode upper-cases a state value and accepts it only if it is a +// real two-letter code. +// +// "Ohio" must not become "Oh": region is half the uniqueness key, so a mangled +// value is a separate dedup bucket and a silent duplicate set. +func normalizeStateCode(raw string) string { + trimmed := strings.ToUpper(strings.TrimSpace(raw)) + if utils.IsUSStateCode(trimmed) { + return trimmed + } + if full, ok := stateNameToCode[trimmed]; ok { + return full + } + return "" +} + +// stateNameToCode covers the full-name spellings OpenAddresses extracts use. +// Deliberately small: an unrecognised value returns "" and falls back rather +// than being truncated into something that looks plausible and is not. +var stateNameToCode = map[string]string{ + "OHIO": "OH", "INDIANA": "IN", "MICHIGAN": "MI", + "KENTUCKY": "KY", "PENNSYLVANIA": "PA", "WEST VIRGINIA": "WV", + "ILLINOIS": "IL", +}