From 984f74f0f5e2a38a05600bef449c0ddf00abd647 Mon Sep 17 00:00:00 2001 From: Gray Fay Date: Mon, 14 Sep 2026 00:37:28 -0400 Subject: [PATCH] feat: report the data problems nothing else surfaces Everything this endpoint reports is wrong in a way that raises no error. A search still returns rows, an import still reports success, and nothing in the logs says otherwise. Production carried eleven distinct region codes -- ON (Ontario), BE, IH, PJ, a bare 0, a lowercase oh -- and 985,634 addresses with no state at all. None of it was visible anywhere. It surfaced only because /coverage happened to group by region while being used for something else. GET /api/v1/admin/data-quality reports invalid region codes with a reason for each, blank region, county, city and postcode counts, and coordinates outside a loose envelope around the United States. Admin-only, and cached for thirty minutes: every check is an aggregate over the whole address table, and an operator refreshing a dashboard should not re-scan 5.8M rows each time. An import invalidates it, as it does coverage. Regions are classified in Go rather than SQL because the set of valid codes already lives in utils.IsUSStateCode, and a second copy inside a query is how the two drift apart. Note IsUSStateCode upper-cases before checking, so it accepts "oh" -- a value only counts as correct if it is a real code AND already canonical, since 'oh' and 'OH' are distinct keys to the uniqueness index. The first version of this missed that and reported the lowercase region as fine; the test caught it. Also fixes /coverage, which folded blank regions into the Ohio bucket with COALESCE(NULLIF(region,''),'OH'). That is the specific reason a million stateless rows went unseen and the Ohio count read about a million higher than it was. Blanks are now reported as (none), and the per-county drill-down no longer absorbs them either. Co-Authored-By: Claude Opus 5 --- handlers/data_quality_handlers.go | 36 +++++ main.go | 1 + services/coverage_service.go | 10 +- services/data_quality_integration_test.go | 163 +++++++++++++++++++ services/data_quality_service.go | 189 ++++++++++++++++++++++ services/dataset_service.go | 1 + 6 files changed, 397 insertions(+), 3 deletions(-) create mode 100644 handlers/data_quality_handlers.go create mode 100644 services/data_quality_integration_test.go create mode 100644 services/data_quality_service.go diff --git a/handlers/data_quality_handlers.go b/handlers/data_quality_handlers.go new file mode 100644 index 0000000..6bbde7f --- /dev/null +++ b/handlers/data_quality_handlers.go @@ -0,0 +1,36 @@ +package handlers + +import ( + "net/http" + + "geocoding-api/services" + + "github.com/labstack/echo/v4" +) + +// GetDataQualityHandler reports the silent correctness problems in the address +// data. +// +// Everything it surfaces is wrong in a way that raises no error: a search still +// returns rows, an import still reports success, and nothing in the logs says +// otherwise. Production held eleven distinct region codes -- including ON +// (Ontario), BE, IH, PJ and a bare 0 -- plus 985,634 addresses with no state at +// all, and none of it was visible anywhere. It surfaced only because an +// unrelated endpoint happened to group by region. +// +// Admin-only: it is an operational view of data health, and the scan is +// expensive enough to be worth restricting. +func GetDataQualityHandler(c echo.Context) error { + report, err := services.GetDataQuality(services.GetDB()) + if err != nil { + return c.JSON(http.StatusInternalServerError, GeocodeResponse{ + Success: false, + Error: "Failed to scan address data", + }) + } + + return c.JSON(http.StatusOK, GeocodeResponse{ + Success: true, + Data: report, + }) +} diff --git a/main.go b/main.go index bf034d3..5057f52 100644 --- a/main.go +++ b/main.go @@ -344,6 +344,7 @@ func main() { admin.PUT("/users/:id/admin", handlers.UpdateUserAdminHandler) admin.GET("/api-keys", handlers.GetAllAPIKeysHandler) admin.GET("/system-status", handlers.GetSystemStatusHandler) + admin.GET("/data-quality", handlers.GetDataQualityHandler) admin.POST("/usage-counters/rebuild", handlers.RebuildUsageCountersHandler) admin.GET("/counties", handlers.GetCountyStatsHandler) admin.POST("/counties/load", handlers.LoadCountyBoundariesHandler) diff --git a/services/coverage_service.go b/services/coverage_service.go index 0f759d6..d916d75 100644 --- a/services/coverage_service.go +++ b/services/coverage_service.go @@ -86,12 +86,16 @@ func buildCoverage(db *sql.DB) (*Coverage, error) { db = database.DB } + // No COALESCE to 'OH'. An earlier version folded every blank region into + // the Ohio bucket, which is precisely why 985,634 stateless rows went + // unnoticed and why the Ohio count read ~1M higher than it was. A blank is + // reported as what it is. rows, err := db.Query(` - SELECT COALESCE(NULLIF(region, ''), 'OH') AS state, + SELECT COALESCE(NULLIF(region, ''), '(none)') AS state, COUNT(DISTINCT county) AS counties, COUNT(*) AS addresses FROM ohio_addresses - GROUP BY COALESCE(NULLIF(region, ''), 'OH') + GROUP BY COALESCE(NULLIF(region, ''), '(none)') ORDER BY COUNT(*) DESC `) if err != nil { @@ -132,7 +136,7 @@ func GetStateCoverage(db *sql.DB, state string) ([]CountyCoverage, error) { rows, err := db.Query(` SELECT county, COUNT(*) AS addresses FROM ohio_addresses - WHERE COALESCE(NULLIF(region, ''), 'OH') = UPPER($1) + WHERE region = UPPER($1) AND county <> '' GROUP BY county ORDER BY county diff --git a/services/data_quality_integration_test.go b/services/data_quality_integration_test.go new file mode 100644 index 0000000..c9b0894 --- /dev/null +++ b/services/data_quality_integration_test.go @@ -0,0 +1,163 @@ +package services + +import ( + "testing" + + "geocoding-api/models" +) + +// Every problem below is silent: a search still returns rows, an import still +// reports success, nothing errors. Production carried eleven distinct region +// codes and 985,634 stateless addresses, and the only reason anyone found out +// was an unrelated endpoint happening to group by region. +func TestDataQualityFindsTheProblemsProductionHad(t *testing.T) { + db := setupCountTestDB(t) + ResetDataQualityCache() + + // The exact shapes production held. + if _, err := db.Exec(` + INSERT INTO ohio_addresses (hash, house_number, street, unit, city, district, region, postcode, county, geom, full_address) + VALUES + ('dq-ontario','1','Main','','Toronto','','ON','','York', + ST_SetSRID(ST_MakePoint(-79.4,43.7),4326),'1 Main, Toronto'), + ('dq-truncated','2','Main','','Somewhere','','BE','','Unknown', + ST_SetSRID(ST_MakePoint(-83.0,40.0),4326),'2 Main'), + ('dq-single','3','Main','','Somewhere','','O','','Unknown', + ST_SetSRID(ST_MakePoint(-83.0,40.0),4326),'3 Main'), + ('dq-lowercase','4','Main','','Columbus','','oh','43004','Franklin', + ST_SetSRID(ST_MakePoint(-83.0,40.0),4326),'4 Main'), + ('dq-nocounty','5','Main','','Columbus','','OH','43004','', + ST_SetSRID(ST_MakePoint(-83.0,40.0),4326),'5 Main'), + ('dq-nozip','6','Main','','Columbus','','OH','','Franklin', + ST_SetSRID(ST_MakePoint(-83.0,40.0),4326),'6 Main'), + ('dq-atsea','7','Main','','Nowhere','','OH','43004','Franklin', + ST_SetSRID(ST_MakePoint(-40.0,35.0),4326),'7 Main') + `); err != nil { + t.Fatalf("seed: %v", err) + } + + report, err := GetDataQuality(db) + if err != nil { + t.Fatalf("scan: %v", err) + } + + byRegion := map[string]RegionIssue{} + for _, r := range report.InvalidRegions { + byRegion[r.Region] = r + } + for _, want := range []string{"ON", "BE", "O", "oh"} { + if _, ok := byRegion[want]; !ok { + t.Errorf("region %q was not reported as invalid; it is its own uniqueness bucket", want) + } + } + // A valid code must not be flagged. + if _, flagged := byRegion["OH"]; flagged { + t.Error("OH was reported as invalid") + } + // The reason has to say what is wrong, or the report is just a list. + if r, ok := byRegion["oh"]; ok && r.Reason == "" { + t.Error("the lowercase region carries no explanation") + } + + if report.BlankCounty < 1 { + t.Error("a row with no county was not counted") + } + if report.BlankPostcode < 1 { + t.Error("a row with no postcode was not counted") + } + if report.OutsideUS < 1 { + t.Error("a row in the mid-Atlantic was not counted as outside US bounds") + } + if report.TotalAddresses < 7 { + t.Errorf("total = %d, want at least the 7 seeded rows", report.TotalAddresses) + } + + t.Logf("invalid regions: %d, blank county: %d, blank postcode: %d, outside US: %d", + len(report.InvalidRegions), report.BlankCounty, report.BlankPostcode, report.OutsideUS) +} + +// The scan is aggregates over the whole table, so it is cached -- the same +// reasoning as coverage, and the same mistake the address search count query +// was making before it was fixed. +func TestDataQualityIsCached(t *testing.T) { + db := setupCountTestDB(t) + ResetDataQualityCache() + + first, err := GetDataQuality(db) + if err != nil { + t.Fatalf("first: %v", err) + } + second, err := GetDataQuality(db) + if err != nil { + t.Fatalf("second: %v", err) + } + if !first.GeneratedAt.Equal(second.GeneratedAt) { + t.Error("the second call rescanned instead of serving the cached report") + } +} + +// Coverage folded every blank region into the Ohio bucket, which is why nobody +// saw 985,634 stateless rows and why the Ohio count read about a million higher +// than it was. +func TestCoverageReportsBlankRegionsSeparately(t *testing.T) { + db := setupCountTestDB(t) + ResetCoverageCache() + + if _, err := db.Exec(` + INSERT INTO ohio_addresses (hash, house_number, street, unit, city, district, region, postcode, county, geom, full_address) + VALUES ('cov-blank','1','Main','','Columbus','','','43004','Franklin', + ST_SetSRID(ST_MakePoint(-83.0,40.0),4326),'1 Main') + `); err != nil { + t.Fatalf("seed blank region: %v", err) + } + + snapshot, err := GetCoverage(db) + if err != nil { + t.Fatalf("coverage: %v", err) + } + + var blank, ohio int + for _, s := range snapshot.States { + switch s.State { + case "(none)": + blank = s.Addresses + case "OH": + ohio = s.Addresses + } + } + + if blank != 1 { + t.Errorf("blank regions reported as %d rows under '(none)', want 1 -- they are being hidden in another bucket", blank) + } + if ohio == 0 { + t.Error("no Ohio rows reported at all") + } + t.Logf("coverage: OH %d, (none) %d", ohio, blank) +} + +// The drill-down must not inherit the same fold, or it disagrees with the +// summary it sits under. +func TestStateCoverageDoesNotAbsorbBlankRegions(t *testing.T) { + db := setupCountTestDB(t) + ResetCoverageCache() + + if _, err := db.Exec(` + INSERT INTO ohio_addresses (hash, house_number, street, unit, city, district, region, postcode, county, geom, full_address) + VALUES ('cov-blank2','2','Main','','Columbus','','','43004','Blankshire', + ST_SetSRID(ST_MakePoint(-83.0,40.0),4326),'2 Main') + `); err != nil { + t.Fatalf("seed: %v", err) + } + + counties, err := GetStateCoverage(db, "OH") + if err != nil { + t.Fatalf("state coverage: %v", err) + } + for _, c := range counties { + if c.County == "Blankshire" { + t.Error("a blank-region row was counted under OH in the drill-down") + } + } +} + +var _ = models.AddressSearchParams{} diff --git a/services/data_quality_service.go b/services/data_quality_service.go new file mode 100644 index 0000000..ea0c905 --- /dev/null +++ b/services/data_quality_service.go @@ -0,0 +1,189 @@ +package services + +import ( + "database/sql" + "strings" + "sync" + "time" + + "geocoding-api/database" + "geocoding-api/utils" +) + +// dataQualityTTL bounds how often the scan runs. +// +// Every check below is an aggregate over the whole address table, which at ~5.8M +// rows is seconds of work. Nothing here changes except when a dataset is +// imported, so a cached answer is never far wrong and an operator refreshing a +// dashboard does not re-scan the table each time. +const dataQualityTTL = 30 * time.Minute + +// usBounds is a generous envelope around the United States, used only to catch +// coordinates that are obviously wrong -- a zero pair, a sign flip, a +// transposed lat/lng. It is deliberately loose: the point is to find nonsense, +// not to police borders. +const ( + usMinLng, usMinLat = -180.0, 15.0 + usMaxLng, usMaxLat = -60.0, 72.0 +) + +// RegionIssue is one region code that is not a valid two-letter US state. +type RegionIssue struct { + Region string `json:"region"` + Addresses int `json:"addresses"` + Reason string `json:"reason"` +} + +// DataQuality reports the silent correctness problems in the address table. +// +// Every field here describes something that is wrong but does not raise an +// error anywhere: a search still returns rows, an import still reports success, +// and nothing in the logs says otherwise. Production held eleven distinct +// region codes -- including ON (Ontario), BE, IH, PJ and a bare 0 -- and the +// only reason anyone noticed was an unrelated endpoint happening to group by +// region. That is the gap this closes. +type DataQuality struct { + TotalAddresses int `json:"total_addresses"` + + // InvalidRegions are region codes that are not real US states. They come + // from the legacy loader truncating a state name to two characters, and + // each one is its own address-uniqueness bucket. + InvalidRegions []RegionIssue `json:"invalid_regions"` + + // BlankRegion counts rows with no state at all. These share a single + // uniqueness bucket, so two identical addresses in different states + // silently collapse into one. + BlankRegion int `json:"blank_region"` + + BlankCounty int `json:"blank_county"` + BlankCity int `json:"blank_city"` + BlankPostcode int `json:"blank_postcode"` + + // OutsideUS counts coordinates outside a loose envelope around the country + // -- a zero pair, a dropped minus sign, a transposed lat/lng. + OutsideUS int `json:"outside_us_bounds"` + + GeneratedAt time.Time `json:"generated_at"` + MaxAgeSecs int `json:"max_age_seconds"` +} + +type dataQualityCache struct { + mu sync.Mutex + snapshot *DataQuality + builtAt time.Time +} + +var dataQuality = &dataQualityCache{} + +// GetDataQuality returns the current report, rescanning when stale. +func GetDataQuality(db *sql.DB) (*DataQuality, error) { + dataQuality.mu.Lock() + defer dataQuality.mu.Unlock() + + if dataQuality.snapshot != nil && time.Since(dataQuality.builtAt) < dataQualityTTL { + return dataQuality.snapshot, nil + } + + report, err := buildDataQuality(db) + if err != nil { + return nil, err + } + + dataQuality.snapshot = report + dataQuality.builtAt = time.Now() + return report, nil +} + +func buildDataQuality(db *sql.DB) (*DataQuality, error) { + if db == nil { + db = database.DB + } + + report := &DataQuality{ + InvalidRegions: []RegionIssue{}, + GeneratedAt: time.Now(), + MaxAgeSecs: int(dataQualityTTL.Seconds()), + } + + // One pass for the counts. FILTER lets every check share a single scan + // rather than each walking the table on its own. + err := db.QueryRow(` + SELECT + COUNT(*), + COUNT(*) FILTER (WHERE region IS NULL OR region = ''), + COUNT(*) FILTER (WHERE county IS NULL OR county = ''), + COUNT(*) FILTER (WHERE city IS NULL OR city = ''), + COUNT(*) FILTER (WHERE postcode IS NULL OR postcode = ''), + COUNT(*) FILTER (WHERE NOT ST_Intersects( + geom, ST_MakeEnvelope($1, $2, $3, $4, 4326))) + FROM ohio_addresses + `, usMinLng, usMinLat, usMaxLng, usMaxLat).Scan( + &report.TotalAddresses, + &report.BlankRegion, + &report.BlankCounty, + &report.BlankCity, + &report.BlankPostcode, + &report.OutsideUS, + ) + if err != nil { + return nil, err + } + + // Regions are classified in Go rather than SQL. The set of valid state + // codes already lives in utils.IsUSStateCode, and embedding a second copy + // in a query is how the two drift apart. + rows, err := db.Query(` + SELECT region, COUNT(*) + FROM ohio_addresses + WHERE region IS NOT NULL AND region <> '' + GROUP BY region + ORDER BY COUNT(*) DESC + `) + if err != nil { + return nil, err + } + defer rows.Close() + + for rows.Next() { + var region string + var n int + if err := rows.Scan(®ion, &n); err != nil { + return nil, err + } + + // IsUSStateCode upper-cases before checking, so it accepts "oh" -- which + // is the point of the first arm below: a value is only correct if it is + // a real code AND already in the canonical case. 'oh' and 'OH' are + // distinct keys to the uniqueness index, so a case difference is a real + // defect, not a cosmetic one. + switch { + case region == strings.ToUpper(region) && utils.IsUSStateCode(region): + // Correct, nothing to report. + case utils.IsUSStateCode(region): + report.InvalidRegions = append(report.InvalidRegions, RegionIssue{ + Region: region, Addresses: n, + Reason: "valid state code in the wrong case; it is a separate uniqueness bucket from the upper-case form", + }) + case len(region) < 2: + report.InvalidRegions = append(report.InvalidRegions, RegionIssue{ + Region: region, Addresses: n, + Reason: "too short to be a state code, most likely a truncated state name", + }) + default: + report.InvalidRegions = append(report.InvalidRegions, RegionIssue{ + Region: region, Addresses: n, + Reason: "not a US state code; the legacy loader truncated state names to two characters", + }) + } + } + + return report, rows.Err() +} + +// ResetDataQualityCache drops the cached report, so a fresh scan runs after an +// import rather than serving a pre-import picture. +func ResetDataQualityCache() { + dataQuality.mu.Lock() + defer dataQuality.mu.Unlock() + dataQuality.snapshot = nil +} diff --git a/services/dataset_service.go b/services/dataset_service.go index ed9798f..e67aec2 100644 --- a/services/dataset_service.go +++ b/services/dataset_service.go @@ -438,6 +438,7 @@ func (s *DatasetService) ProcessGeoJSONDataset(datasetID int) error { // An import is the only thing that changes coverage, so drop the cached // snapshot instead of serving a stale one for the rest of the TTL. ResetCoverageCache() + ResetDataQualityCache() log.Printf("Successfully processed dataset %d: %d records imported, %d duplicates skipped, %d failed", datasetID, recordCount, skippedDuplicates, importer.failed)