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)