From 32c57d919bc9324fd671fb94b102b0a0b7ee59a5 Mon Sep 17 00:00:00 2001 From: Gray Fay Date: Fri, 11 Sep 2026 11:16:24 -0400 Subject: [PATCH 1/3] feat: key address uniqueness on (hash, region) so a second state can load ohio_addresses.hash is HouseNumber|Street|Unit|City|Postcode and the column was UNIQUE. No state anywhere in it. With a postcode present that mostly did not matter -- US ZIPs are state-specific, so they disentangle the key -- but ingestion requires only 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 was silent and looked like success. A colliding row hit the unique constraint, the importer counted it as a duplicate, and a load that quietly discarded half a state reported "N records imported, M duplicates skipped" and completed green. A test now pins that old behaviour so the regression stays on the record. Migration 23 changes the uniqueness key rather than the hash contents. Recomputing every hash would rewrite all ~6M rows; keying on (hash, region) is one index build and means the same thing. It also: - backfills region to 'OH' where NULL or empty, since NULLs are distinct in a unique index and those rows could otherwise duplicate without limit - upper-cases existing values, because 'oh' and 'OH' are different keys and a lowercase upload would reintroduce the collision - adds CHECK (region <> ''), since NOT NULL does not stop an empty string and '' would put every stateless row in one shared bucket - indexes region, without which filtering by state is unusable No DEFAULT on region, deliberately: a default would silently label a future non-Ohio import as Ohio, which is the same quiet corruption being fixed. The other half is a state filter. Filtering by county alone is ambiguous once two states exist, so ?state=OH now narrows it, case-insensitively. The down migration can legitimately fail once a second state is loaded -- no single-column unique constraint can hold rows that share a hash across regions. That is the rollback refusing to destroy data. COST: the unique index build takes a SHARE lock, so reads continue and writes wait; on ~6M rows expect tens of seconds. Writes here are imports, which are deliberate admin actions. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01S2nhUJ4Zdsxf7cP9DapsY3 --- api-docs.yaml | 11 + database/migrations.go | 105 +++++++++ handlers/address_handlers.go | 4 + models/address.go | 6 +- services/address_service.go | 8 + services/dataset_ingest_integration_test.go | 8 +- services/dataset_service.go | 7 +- services/multistate_integration_test.go | 232 ++++++++++++++++++++ services/ohio_address_service.go | 2 +- 9 files changed, 377 insertions(+), 6 deletions(-) create mode 100644 services/multistate_integration_test.go diff --git a/api-docs.yaml b/api-docs.yaml index 58385be..82b63ed 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..d4bfe08 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,102 @@ 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. +// +// COST. The unique index build takes a SHARE lock: reads continue, writes wait. +// On ~6M rows expect tens of seconds. Writes here are imports, which are +// deliberate admin actions, so this is scheduled rather than concurrent -- and +// CREATE INDEX CONCURRENTLY cannot run inside the transaction the rest of this +// migration needs. +func addRegionToAddressUniqueness() error { + tx, err := DB.Begin() + if err != nil { + return fmt.Errorf("failed to begin address uniqueness migration: %w", err) + } + defer tx.Rollback() + + statements := []string{ + // Every row currently in this table is Ohio. A NULL or empty region + // would defeat the new key outright: NULLs are distinct in a unique + // index, so those rows could duplicate without limit. + `UPDATE ohio_addresses SET region = 'OH' WHERE region IS NULL OR region = ''`, + + // 'oh' and 'OH' are different values to a unique index, so a lowercase + // upload would reintroduce exactly the duplicates this is closing. + `UPDATE ohio_addresses SET region = UPPER(region) WHERE region <> UPPER(region)`, + + // No DEFAULT on purpose. A default would silently label a future + // non-Ohio import as Ohio, which is the same class of quiet data + // corruption being fixed here -- better that a bad import fails loudly. + `ALTER TABLE ohio_addresses ALTER COLUMN region SET NOT NULL`, + + // NOT NULL does not stop an empty string, and '' would put every + // stateless row into one shared bucket -- reintroducing exactly the + // collision this migration closes, for any dataset uploaded without a + // state. A CHECK makes that import fail loudly instead. + `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 <> '')`, + + `ALTER TABLE ohio_addresses DROP CONSTRAINT IF EXISTS ohio_addresses_hash_key`, + `CREATE UNIQUE INDEX IF NOT EXISTS idx_ohio_addresses_hash_region + ON ohio_addresses (hash, region)`, + + // Searching or filtering by state is unusable without this, and it is + // the other half of what multi-state support needs. + `CREATE INDEX IF NOT EXISTS idx_ohio_addresses_region ON ohio_addresses (region)`, + } + + for _, stmt := range statements { + if _, err := tx.Exec(stmt); err != nil { + return fmt.Errorf("failed to key address uniqueness on region: %w", err) + } + } + + 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. +func revertRegionAddressUniqueness() error { + 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 := DB.Exec(stmt); err != nil { + return fmt.Errorf("failed to restore the global unique hash (rows from more than one state may share a hash): %w", err) + } + } + return nil +} diff --git a/handlers/address_handlers.go b/handlers/address_handlers.go index 092b9fa..066c05c 100644 --- a/handlers/address_handlers.go +++ b/handlers/address_handlers.go @@ -16,6 +16,7 @@ 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") + params.State = c.QueryParam("state") params.County = c.QueryParam("county") params.City = c.QueryParam("city") params.Postcode = c.QueryParam("postcode") @@ -59,6 +60,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/models/address.go b/models/address.go index 1125a8e..f5f4678 100644 --- a/models/address.go +++ b/models/address.go @@ -63,7 +63,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 0374613..d1fded3 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 params.State != "" { + conditions = append(conditions, fmt.Sprintf("region = UPPER($%d)", argIndex)) + args = append(args, strings.TrimSpace(params.State)) + argIndex++ + } + // County filter if params.County != "" { conditions = append(conditions, fmt.Sprintf("county ILIKE $%d", argIndex)) diff --git a/services/dataset_ingest_integration_test.go b/services/dataset_ingest_integration_test.go index ee45243..e442b69 100644 --- a/services/dataset_ingest_integration_test.go +++ b/services/dataset_ingest_integration_test.go @@ -107,19 +107,23 @@ 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)", `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..636e213 100644 --- a/services/dataset_service.go +++ b/services/dataset_service.go @@ -553,7 +553,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 +713,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..4b42918 --- /dev/null +++ b/services/multistate_integration_test.go @@ -0,0 +1,232 @@ +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", + 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)") + } + stmts = append(stmts, "CREATE INDEX ON ohio_addresses (region)") + + 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) + } + } +} diff --git a/services/ohio_address_service.go b/services/ohio_address_service.go index 15f7fc9..76328ff 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) From 8482a59c241b7f3bdf4a2b52bc2663cbce6bc129 Mon Sep 17 00:00:00 2001 From: Gray Fay Date: Fri, 11 Sep 2026 11:28:29 -0400 Subject: [PATCH 2/3] fix: address code review findings on multi-state uniqueness **The migration blocked reads, not just writes.** ALTER TABLE ... SET NOT NULL takes ACCESS EXCLUSIVE -- verified, not assumed -- and a lock once taken is held until commit. Taking it before the index build meant every /search and /geocode query blocked for the whole build, while migrations run asynchronously against a server already serving. The two index builds (SHARE) now come first and everything needing ACCESS EXCLUSIVE is last, holding it only for its own validation scan. The cost note said "reads continue"; it was wrong. **The down migration could leave the table with no uniqueness at all.** Its own comment says the final ADD CONSTRAINT is expected to fail once two states are loaded -- but the composite key and the CHECK were already dropped by then, so every ON CONFLICT (hash, region) would fail with 42P10 and deduplication would be off entirely. Now one transaction, so the documented "correct outcome" is a clean no-op rather than a half-dropped schema. **ON CONFLICT (hash, region) shipped ahead of the index it needs.** Migrations run asynchronously while upload endpoints are live, so a deploy-window import failed every batch and marked the dataset failed -- indistinguishable from a corrupt file. This is the third time this branch family has hit that window, so it is now a named guard: database.RequireSchemaVersion, called at the top of the import path. Read paths can degrade; a write path that needs a constraint cannot. **The legacy loader was updated for the new conflict target but not the new invariants.** It truncated "Ohio" to "Oh" -- a different uniqueness bucket from "OH" and invisible to a region filter -- and produced "" when an extract had no state, which the region CHECK rejects on every row. Since that error text is not "duplicate key", the loop logged per row and returned 0 inserted with a nil error: a total failure reported as success. It now normalises through a real state-code check. **Geocoding ignored the state in the query.** The parser extracts one and searchByComponents discarded it, so "100 Main St, Springfield, IL" matched the Ohio row on street and city and returned it at full confidence -- the exact collision this branch exists to prevent, on the endpoint that matters most. State is now an anchor on every tier: what relaxes as tiers widen is street-level detail, never the state. **An uploaded state code was upper-cased but never validated.** "HO" for "OH" now creates a separate dedup bucket, so the import succeeds green and a re-upload under the correct code inserts a second full copy. Validated at all three upload entry points. **DROP CONSTRAINT IF EXISTS failed open.** If the production constraint carries another name it is a silent no-op, leaving a live global unique hash beside the new key while the migration logs success -- the same silent loss being fixed. It now asserts against pg_constraint and errors. **"Every row is Ohio" was an assumption.** Now self-verifying: the migration refuses to stamp OH onto blank regions if any other state is present. Verified by seeding an IL row and watching it refuse. The two full-table UPDATEs are also one pass now rather than two rewrites of ~6M rows. **?state=%20 and ?state=Ohio returned an empty page with no explanation.** Trimmed and validated at the edge, 400 with an example. Test fixtures carry the region CHECK and schema_migrations, so the cases those guard fail in the suite rather than only in production. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01S2nhUJ4Zdsxf7cP9DapsY3 --- database/migrations.go | 156 ++++++++++++++++---- handlers/address_handlers.go | 15 +- handlers/dataset_handlers.go | 45 ++++++ services/address_service.go | 30 +++- services/dataset_ingest_integration_test.go | 9 ++ services/dataset_service.go | 9 ++ services/multistate_integration_test.go | 59 +++++++- services/ohio_address_service.go | 49 +++++- 8 files changed, 329 insertions(+), 43 deletions(-) diff --git a/database/migrations.go b/database/migrations.go index d4bfe08..362e3c0 100644 --- a/database/migrations.go +++ b/database/migrations.go @@ -1576,11 +1576,14 @@ func removeUsageCounters() error { // would rewrite all ~6M rows; keying on (hash, region) is one index build and // means exactly the same thing. // -// COST. The unique index build takes a SHARE lock: reads continue, writes wait. -// On ~6M rows expect tens of seconds. Writes here are imports, which are -// deliberate admin actions, so this is scheduled rather than concurrent -- and -// CREATE INDEX CONCURRENTLY cannot run inside the transaction the rest of this -// migration needs. +// 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 { @@ -1588,35 +1591,43 @@ func addRegionToAddressUniqueness() error { } defer tx.Rollback() - statements := []string{ - // Every row currently in this table is Ohio. A NULL or empty region - // would defeat the new key outright: NULLs are distinct in a unique - // index, so those rows could duplicate without limit. - `UPDATE ohio_addresses SET region = 'OH' WHERE region IS NULL OR region = ''`, - - // 'oh' and 'OH' are different values to a unique index, so a lowercase - // upload would reintroduce exactly the duplicates this is closing. - `UPDATE ohio_addresses SET region = UPPER(region) WHERE region <> UPPER(region)`, - - // No DEFAULT on purpose. A default would silently label a future - // non-Ohio import as Ohio, which is the same class of quiet data - // corruption being fixed here -- better that a bad import fails loudly. - `ALTER TABLE ohio_addresses ALTER COLUMN region SET NOT NULL`, + // 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) + } - // NOT NULL does not stop an empty string, and '' would put every - // stateless row into one shared bucket -- reintroducing exactly the - // collision this migration closes, for any dataset uploaded without a - // state. A CHECK makes that import fail loudly instead. - `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 <> '')`, + 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'`, - `ALTER TABLE ohio_addresses DROP CONSTRAINT IF EXISTS ohio_addresses_hash_key`, + // SHARE lock: readers unaffected. `CREATE UNIQUE INDEX IF NOT EXISTS idx_ohio_addresses_hash_region ON ohio_addresses (hash, region)`, - - // Searching or filtering by state is unusable without this, and it is - // the other half of what multi-state support needs. `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 { @@ -1625,6 +1636,31 @@ func addRegionToAddressUniqueness() error { } } + // 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) } @@ -1639,7 +1675,20 @@ func addRegionToAddressUniqueness() error { // 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`, @@ -1649,9 +1698,52 @@ func revertRegionAddressUniqueness() error { } for _, stmt := range statements { - if _, err := DB.Exec(stmt); err != nil { - return fmt.Errorf("failed to restore the global unique hash (rows from more than one state may share a hash): %w", err) + 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 nil + + 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 066c05c..0b1c25a 100644 --- a/handlers/address_handlers.go +++ b/handlers/address_handlers.go @@ -6,6 +6,7 @@ import ( "geocoding-api/services" "net/http" "strconv" + "strings" "github.com/labstack/echo/v4" ) @@ -16,7 +17,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") - params.State = c.QueryParam("state") + 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") 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/services/address_service.go b/services/address_service.go index d1fded3..ed83fa2 100644 --- a/services/address_service.go +++ b/services/address_service.go @@ -170,9 +170,9 @@ 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 params.State != "" { - conditions = append(conditions, fmt.Sprintf("region = UPPER($%d)", argIndex)) - args = append(args, strings.TrimSpace(params.State)) + if state := strings.ToUpper(strings.TrimSpace(params.State)); state != "" { + conditions = append(conditions, fmt.Sprintf("region = $%d", argIndex)) + args = append(args, state) argIndex++ } @@ -756,6 +756,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 @@ -782,11 +794,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 e442b69..d92f3b4 100644 --- a/services/dataset_ingest_integration_test.go +++ b/services/dataset_ingest_integration_test.go @@ -124,6 +124,15 @@ func setupIngestSchema(t *testing.T, db *sql.DB) { // 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 636e213..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) diff --git a/services/multistate_integration_test.go b/services/multistate_integration_test.go index 4b42918..2072b45 100644 --- a/services/multistate_integration_test.go +++ b/services/multistate_integration_test.go @@ -53,7 +53,8 @@ func setupMultiStateDB(t *testing.T, keyedOnRegion bool) *sql.DB { )`, unique), } if keyedOnRegion { - stmts = append(stmts, "CREATE UNIQUE INDEX ON ohio_addresses (hash, region)") + 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)") @@ -230,3 +231,59 @@ func TestStateFilterIsCaseInsensitive(t *testing.T) { } } } + +// 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") + } +} diff --git a/services/ohio_address_service.go b/services/ohio_address_service.go index 76328ff..5256978 100644 --- a/services/ohio_address_service.go +++ b/services/ohio_address_service.go @@ -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", +} From 39756e0cd1bfc4ae914f332f68094ed2e761b27f Mon Sep 17 00:00:00 2001 From: Gray Fay Date: Fri, 11 Sep 2026 13:36:38 -0400 Subject: [PATCH 3/3] test: prove the state and territory filters compose after the merge They arrived on separate branches and first met here. Both extend the same hand-numbered placeholder sequence -- the bbox consumes four positions -- and a mistake there binds the wrong value to the wrong column, which returns plausible rows from the wrong place rather than an error. Seeds the same coordinates in two states so only the state filter can separate them, then checks bbox alone, bbox plus state, and bbox plus state plus a text query, each adding placeholders of its own. The fixture also gains the generated fts column and its indexes: without them a text query combined with a territory filter could not run at all, which is why this combination had never been exercised. Co-Authored-By: Claude Opus 5 --- services/multistate_integration_test.go | 68 ++++++++++++++++++++++++- 1 file changed, 67 insertions(+), 1 deletion(-) diff --git a/services/multistate_integration_test.go b/services/multistate_integration_test.go index 2072b45..0c3a612 100644 --- a/services/multistate_integration_test.go +++ b/services/multistate_integration_test.go @@ -38,6 +38,7 @@ func setupMultiStateDB(t *testing.T, keyedOnRegion bool) *sql.DB { 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), @@ -56,7 +57,15 @@ func setupMultiStateDB(t *testing.T, keyedOnRegion bool) *sql.DB { 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)") + 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 { @@ -287,3 +296,60 @@ func TestBlankRegionIsRejected(t *testing.T) { 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) +}