From 7436efcb7d713739a642a8d08fcbbba4657529fb Mon Sep 17 00:00:00 2001 From: Gray Fay Date: Fri, 11 Sep 2026 08:42:14 -0400 Subject: [PATCH 1/3] feat: search addresses by bounding box or polygon Address search could filter by a circle (lat/lng/radius) and by structured fields, but not by a shape. A territory is rarely a circle: a sales area, a delivery zone, a canvassing route and a map viewport are all polygons or rectangles, and approximating one with a radius drags in everything in the corners. GET /addresses?bbox=-83.1,39.9,-82.9,40.1 GET /addresses?polygon={"type":"Polygon","coordinates":[[...]]} Both compose with the existing filters and with text search, so a query can be "Barendt, inside this neighbourhood" rather than one or the other. bbox is longitude-first, matching GeoJSON, Leaflet, MapLibre and PostGIS, so a caller can pass a viewport straight through. Latitude-first is the common mistake and it is silent -- a swapped pair still parses and just returns nothing -- so the bounds are validated and a bad box is a 400 naming what is wrong, not an empty result. The same reasoning applies to the polygon: it is checked for type, ring closure and coordinate ranges here, because ST_GeomFromGeoJSON raises on malformed input and a caller's typo should not surface as a 500. Both predicates are index-assisted. && against the GIST index on geom is exact containment for point geometry, and ST_Intersects shortlists on the same index before testing candidates. Verified by plan: Bitmap Index Scan for both. Tests assert the rectangle actually restricts (101 of 600 rows) and that every returned point is inside it, that a polygon excludes points its own bounding box includes, that territory filters compose with text search without widening the result, and that a reversed or malformed box is rejected rather than returned empty. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01S2nhUJ4Zdsxf7cP9DapsY3 --- handlers/address_handlers.go | 95 +++++++- models/address.go | 61 ++++- services/address_service.go | 26 ++ services/territory_search_integration_test.go | 225 ++++++++++++++++++ 4 files changed, 403 insertions(+), 4 deletions(-) create mode 100644 services/territory_search_integration_test.go diff --git a/handlers/address_handlers.go b/handlers/address_handlers.go index 092b9fa..e97c521 100644 --- a/handlers/address_handlers.go +++ b/handlers/address_handlers.go @@ -1,12 +1,14 @@ package handlers import ( + "encoding/json" "fmt" - "geocoding-api/models" - "geocoding-api/services" "net/http" "strconv" + "geocoding-api/models" + "geocoding-api/services" + "github.com/labstack/echo/v4" ) @@ -37,6 +39,33 @@ func SearchOhioAddressesHandler(c echo.Context) error { params.Radius = val } } + + // Territory filters. Unlike the numeric parameters above, a malformed + // value here is reported rather than ignored: silently dropping a bbox + // widens the search to the whole state, and a caller asking for one + // neighbourhood would get 50 arbitrary rows back and no indication why. + if raw := c.QueryParam("bbox"); raw != "" { + box, err := models.ParseBBox(raw) + if err != nil { + return c.JSON(http.StatusBadRequest, map[string]interface{}{ + "success": false, + "error": err.Error(), + "example": "bbox=-83.1,39.9,-82.9,40.1", + }) + } + params.BBox = box + } + + if raw := c.QueryParam("polygon"); raw != "" { + if err := validateGeoJSONPolygon(raw); err != nil { + return c.JSON(http.StatusBadRequest, map[string]interface{}{ + "success": false, + "error": err.Error(), + "example": `polygon={"type":"Polygon","coordinates":[[[-83.1,39.9],[-82.9,39.9],[-82.9,40.1],[-83.1,40.1],[-83.1,39.9]]]}`, + }) + } + params.Polygon = raw + } if limit := c.QueryParam("limit"); limit != "" { if val, err := strconv.Atoi(limit); err == nil { params.Limit = val @@ -80,6 +109,15 @@ func SearchOhioAddressesHandler(c echo.Context) error { filters["radius_km"] = params.Radius } } + if params.BBox != nil { + filters["bbox"] = params.BBox + } + if params.Polygon != "" { + // Echoed as a flag, not as the shape. A territory polygon can run to + // hundreds of vertices, and repeating it would dwarf the results the + // caller asked for. + filters["polygon"] = true + } return c.JSON(http.StatusOK, models.AddressSearchResponse{ Success: true, @@ -189,3 +227,56 @@ func FullTextSearchAddressesHandler(c echo.Context) error { return c.JSON(http.StatusOK, response) } + +// validateGeoJSONPolygon checks the shape before it reaches PostGIS. +// +// ST_GeomFromGeoJSON raises on malformed input, which would surface as a 500 +// on a request that is simply wrong -- a caller's typo should not look like a +// server fault. Checking here also keeps the error specific: "not a Polygon" +// is actionable, "failed to search addresses" is not. +func validateGeoJSONPolygon(raw string) error { + var shape struct { + Type string `json:"type"` + Coordinates [][][]float64 `json:"coordinates"` + } + if err := json.Unmarshal([]byte(raw), &shape); err != nil { + return fmt.Errorf("polygon is not valid GeoJSON: %v", err) + } + + switch shape.Type { + case "Polygon": + case "": + return fmt.Errorf(`polygon is missing its "type" field; expected {"type":"Polygon","coordinates":[...]}`) + default: + return fmt.Errorf("polygon type is %q; only Polygon is supported", shape.Type) + } + + if len(shape.Coordinates) == 0 || len(shape.Coordinates[0]) < 4 { + return fmt.Errorf("polygon needs a ring of at least 4 positions (the last repeating the first)") + } + + ring := shape.Coordinates[0] + first, last := ring[0], ring[len(ring)-1] + if len(first) < 2 || len(last) < 2 { + return fmt.Errorf("each polygon position needs at least a longitude and a latitude") + } + // GeoJSON requires a closed ring. PostGIS rejects an open one, and the + // message it produces does not say which ring or why. + if first[0] != last[0] || first[1] != last[1] { + return fmt.Errorf("polygon ring is not closed: the last position must repeat the first") + } + + for i, pos := range ring { + if len(pos) < 2 { + return fmt.Errorf("position %d is missing a coordinate", i) + } + if pos[0] < -180 || pos[0] > 180 { + return fmt.Errorf("position %d has longitude %g outside -180..180 (GeoJSON is longitude first)", i, pos[0]) + } + if pos[1] < -90 || pos[1] > 90 { + return fmt.Errorf("position %d has latitude %g outside -90..90 (GeoJSON is longitude first)", i, pos[1]) + } + } + + return nil +} diff --git a/models/address.go b/models/address.go index 1125a8e..45c1afc 100644 --- a/models/address.go +++ b/models/address.go @@ -1,6 +1,9 @@ package models import ( + "fmt" + "strconv" + "strings" "time" ) @@ -71,8 +74,18 @@ type AddressSearchParams struct { Lat float64 `json:"lat" form:"lat"` // Latitude for proximity search Lng float64 `json:"lng" form:"lng"` // Longitude for proximity search Radius float64 `json:"radius" form:"radius"` // Radius in kilometers for proximity search - Limit int `json:"limit" form:"limit"` // Number of results to return (default: 50, max: 500) - Offset int `json:"offset" form:"offset"` // Offset for pagination + + // BBox restricts results to a rectangle, as minLng,minLat,maxLng,maxLat -- + // the order every mapping library emits, so a caller can pass a viewport + // straight through without reordering it. + BBox *BoundingBox `json:"bbox" form:"bbox"` + + // Polygon restricts results to an arbitrary shape, given as GeoJSON. A + // radius is a circle and a bbox is a rectangle; a sales territory, a + // delivery zone or a canvassing walk is neither. + Polygon string `json:"polygon" form:"polygon"` + Limit int `json:"limit" form:"limit"` // Number of results to return (default: 50, max: 500) + Offset int `json:"offset" form:"offset"` // Offset for pagination } // AddressSearchResponse represents the response for address search @@ -85,3 +98,47 @@ type AddressSearchResponse struct { Query string `json:"query,omitempty"` Filters map[string]any `json:"filters,omitempty"` } + +// BoundingBox is a rectangle in WGS84 degrees. +type BoundingBox struct { + MinLng float64 `json:"min_lng"` + MinLat float64 `json:"min_lat"` + MaxLng float64 `json:"max_lng"` + MaxLat float64 `json:"max_lat"` +} + +// ParseBBox reads "minLng,minLat,maxLng,maxLat". +// +// Ordering is longitude-first because that is what GeoJSON, Leaflet, MapLibre +// and PostGIS all use. Latitude-first is the common mistake and it is silent: +// a swapped pair inside Ohio's range still parses and just returns nothing, so +// the bounds are validated rather than trusted. +func ParseBBox(raw string) (*BoundingBox, error) { + parts := strings.Split(raw, ",") + if len(parts) != 4 { + return nil, fmt.Errorf("bbox needs 4 comma-separated values (minLng,minLat,maxLng,maxLat), got %d", len(parts)) + } + + vals := make([]float64, 4) + for i, p := range parts { + v, err := strconv.ParseFloat(strings.TrimSpace(p), 64) + if err != nil { + return nil, fmt.Errorf("bbox value %d is not a number: %q", i+1, strings.TrimSpace(p)) + } + vals[i] = v + } + + box := &BoundingBox{MinLng: vals[0], MinLat: vals[1], MaxLng: vals[2], MaxLat: vals[3]} + + if box.MinLng < -180 || box.MaxLng > 180 || box.MinLat < -90 || box.MaxLat > 90 { + return nil, fmt.Errorf("bbox is outside valid coordinate ranges (longitude -180..180, latitude -90..90)") + } + if box.MinLng >= box.MaxLng { + return nil, fmt.Errorf("bbox min longitude (%g) must be less than max longitude (%g); values are minLng,minLat,maxLng,maxLat", box.MinLng, box.MaxLng) + } + if box.MinLat >= box.MaxLat { + return nil, fmt.Errorf("bbox min latitude (%g) must be less than max latitude (%g); values are minLng,minLat,maxLng,maxLat", box.MinLat, box.MaxLat) + } + + return box, nil +} diff --git a/services/address_service.go b/services/address_service.go index 0374613..fe32435 100644 --- a/services/address_service.go +++ b/services/address_service.go @@ -168,6 +168,32 @@ func (s *AddressService) searchAddresses(q querier, params models.AddressSearchP } } + // Bounding box. ST_MakeEnvelope builds the rectangle; && is the indexed + // bbox overlap operator, which for point geometry is exact containment -- + // a point either is inside the rectangle or is not, so no recheck is + // needed and the GIST index on geom does all the work. + if params.BBox != nil { + conditions = append(conditions, fmt.Sprintf( + "geom && ST_MakeEnvelope($%d, $%d, $%d, $%d, 4326)", + argIndex, argIndex+1, argIndex+2, argIndex+3)) + args = append(args, params.BBox.MinLng, params.BBox.MinLat, params.BBox.MaxLng, params.BBox.MaxLat) + argIndex += 4 + } + + // Arbitrary polygon. ST_Intersects is index-assisted: the planner uses && + // against the GIST index to shortlist candidates, then tests each one + // exactly. A territory is rarely a rectangle, and approximating one with a + // bbox pulls in everything in the corners. + // + // ST_GeomFromGeoJSON raises on malformed input, so the shape is validated + // in the handler and never reaches here unchecked. + if params.Polygon != "" { + conditions = append(conditions, fmt.Sprintf( + "ST_Intersects(geom, ST_SetSRID(ST_GeomFromGeoJSON($%d), 4326))", argIndex)) + args = append(args, params.Polygon) + argIndex++ + } + // County filter if params.County != "" { conditions = append(conditions, fmt.Sprintf("county ILIKE $%d", argIndex)) diff --git a/services/territory_search_integration_test.go b/services/territory_search_integration_test.go new file mode 100644 index 0000000..0d9a709 --- /dev/null +++ b/services/territory_search_integration_test.go @@ -0,0 +1,225 @@ +package services + +import ( + "strings" + "testing" + + "geocoding-api/models" +) + +// A bbox is the viewport case: a map is showing a rectangle and wants the +// addresses inside it. +func TestBBoxRestrictsToTheRectangle(t *testing.T) { + db := setupCountTestDB(t) + svc := NewAddressService(db) + + // The fixture walks points along a diagonal from (-84.0, 39.0), one + // thousandth of a degree per row. This box covers the first hundred. + box := &models.BoundingBox{MinLng: -84.101, MinLat: 39.0, MaxLng: -84.0, MaxLat: 39.101} + + rows, total, err := svc.SearchAddresses(models.AddressSearchParams{BBox: box, Limit: 500}) + if err != nil { + t.Fatalf("bbox search: %v", err) + } + if total == 0 { + t.Fatal("no rows inside a box that covers part of the fixture") + } + + // Every returned point must actually be inside. A bbox that silently + // widens is worse than one that errors: the caller gets plausible rows + // from the wrong place. + for _, r := range rows { + if r.Longitude < box.MinLng || r.Longitude > box.MaxLng || + r.Latitude < box.MinLat || r.Latitude > box.MaxLat { + t.Errorf("%s at (%f, %f) is outside the requested box", + r.FullAddress, r.Longitude, r.Latitude) + } + } + + // And it must actually restrict -- an unbounded search returns more. + _, allTotal, err := svc.SearchAddresses(models.AddressSearchParams{Limit: 1}) + if err != nil { + t.Fatalf("unbounded search: %v", err) + } + if total >= allTotal { + t.Errorf("bbox returned %d of %d rows; it is not restricting anything", total, allTotal) + } + t.Logf("bbox selected %d of %d addresses", total, allTotal) +} + +// The point of a polygon is the shapes a rectangle cannot express. The fixture +// seeds its points along a perfect diagonal, so any box around a stretch of it +// and any band following it select the same rows -- this test adds points that +// sit inside the box but off the line, which is what a real territory has to +// exclude. +func TestPolygonExcludesWhatItsBoundingBoxWouldInclude(t *testing.T) { + db := setupCountTestDB(t) + svc := NewAddressService(db) + + // Two points well off the diagonal but comfortably inside the box below. + if _, err := db.Exec(` + INSERT INTO ohio_addresses (hash, house_number, street, unit, city, district, region, postcode, county, geom, full_address) + VALUES + ('off-diagonal-a', '1', 'Off Line Road', '', 'Columbus', 'FRA', 'OH', '43004', 'Franklin', + ST_SetSRID(ST_MakePoint(-84.045, 39.005), 4326), '1 Off Line Road, Columbus, OH 43004'), + ('off-diagonal-b', '2', 'Off Line Road', '', 'Columbus', 'FRA', 'OH', '43004', 'Franklin', + ST_SetSRID(ST_MakePoint(-84.005, 39.045), 4326), '2 Off Line Road, Columbus, OH 43004') + `); err != nil { + t.Fatalf("seed off-diagonal points: %v", err) + } + + box := &models.BoundingBox{MinLng: -84.0505, MinLat: 38.9995, MaxLng: -83.9995, MaxLat: 39.0505} + + // A narrow band hugging the diagonal. Its bounding box is the square + // above; the band itself is a few thousandths of a degree wide, so the two + // corner points fall outside it. + polygon := `{"type":"Polygon","coordinates":[[ + [-84.0000,38.9970],[-84.0530,39.0500],[-84.0500,39.0530],[-83.9970,39.0000],[-84.0000,38.9970] + ]]}` + + _, boxTotal, err := svc.SearchAddresses(models.AddressSearchParams{BBox: box, Limit: 1}) + if err != nil { + t.Fatalf("bbox search: %v", err) + } + rows, polyTotal, err := svc.SearchAddresses(models.AddressSearchParams{Polygon: polygon, Limit: 500}) + if err != nil { + t.Fatalf("polygon search: %v", err) + } + + t.Logf("bounding box: %d addresses, polygon: %d", boxTotal, polyTotal) + if polyTotal == 0 { + t.Fatal("the band follows the fixture's diagonal and should contain its points") + } + if polyTotal >= boxTotal { + t.Errorf("polygon returned %d and its bounding box %d; the polygon is not narrowing anything", + polyTotal, boxTotal) + } + for _, r := range rows { + if r.Street == "Off Line Road" { + t.Errorf("%s sits off the band and should have been excluded", r.FullAddress) + } + } +} + +// Territory filters have to combine with the rest of the query, or they are a +// separate endpoint wearing the same URL. +func TestBBoxCombinesWithTextSearch(t *testing.T) { + db := setupCountTestDB(t) + svc := NewAddressService(db) + + box := &models.BoundingBox{MinLng: -84.6, MinLat: 39.0, MaxLng: -83.9, MaxLat: 39.7} + + _, boxOnly, err := svc.SearchAddresses(models.AddressSearchParams{BBox: box, Limit: 1}) + if err != nil { + t.Fatalf("bbox: %v", err) + } + rows, combined, err := svc.SearchAddresses(models.AddressSearchParams{ + BBox: box, Query: "Barendt", Limit: 500, + }) + if err != nil { + t.Fatalf("bbox + query: %v", err) + } + + if combined > boxOnly { + t.Errorf("adding a text query widened the result from %d to %d", boxOnly, combined) + } + for _, r := range rows { + if r.Longitude < box.MinLng || r.Longitude > box.MaxLng { + t.Errorf("%s escaped the box when combined with a text query", r.FullAddress) + } + if r.Match == nil { + t.Error("match metadata missing when a territory filter is combined with a query") + } + } + t.Logf("box alone %d, box + \"Barendt\" %d", boxOnly, combined) +} + +// A box with its corners the wrong way round selects nothing, silently. It has +// to be rejected rather than returned as an empty result. +func TestBBoxValidationCatchesSwappedCorners(t *testing.T) { + cases := []struct { + name string + raw string + }{ + {"reversed longitude", "-82.9,39.9,-83.1,40.1"}, + {"reversed latitude", "-83.1,40.1,-82.9,39.9"}, + {"too few values", "-83.1,39.9,-82.9"}, + {"not a number", "-83.1,39.9,east,40.1"}, + {"latitude out of range", "-83.1,91.0,-82.9,92.0"}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if _, err := models.ParseBBox(tc.raw); err == nil { + t.Errorf("%q was accepted; a bad box returns plausible rows from the wrong place", tc.raw) + } + }) + } + + good := "-83.1,39.9,-82.9,40.1" + box, err := models.ParseBBox(good) + if err != nil { + t.Fatalf("a valid box was rejected: %v", err) + } + if box.MinLng != -83.1 || box.MinLat != 39.9 || box.MaxLng != -82.9 || box.MaxLat != 40.1 { + t.Errorf("parsed %+v; values are longitude-first", box) + } +} + +// The spatial index has to be able to serve this. Without it the endpoint is a +// sequential scan wearing a spatial API. +// +// The fixture is a few hundred rows, where a seq scan is genuinely cheaper and +// the planner is right to choose it -- so seqscan is disabled for the check. +// That answers the question that matters: can the predicate use the GIST +// index, or is its shape wrong. +func TestTerritorySearchCanUseTheSpatialIndex(t *testing.T) { + db := setupCountTestDB(t) + + // The shared fixture does not build this; production does, in the + // ohio_addresses migration. Without it the check would pass or fail on the + // fixture's shape rather than on the predicate's. + if _, err := db.Exec("CREATE INDEX IF NOT EXISTS idx_probe_geom ON ohio_addresses USING GIST (geom)"); err != nil { + t.Fatalf("create gist index: %v", err) + } + if _, err := db.Exec("ANALYZE ohio_addresses"); err != nil { + t.Fatalf("analyze: %v", err) + } + + if _, err := db.Exec("SET enable_seqscan = off"); err != nil { + t.Fatalf("disable seqscan: %v", err) + } + defer db.Exec("SET enable_seqscan = on") + + for _, tc := range []struct { + name string + where string + }{ + {"bbox", "geom && ST_MakeEnvelope(-84.1, 39.0, -84.0, 39.1, 4326)"}, + {"polygon", `ST_Intersects(geom, ST_SetSRID(ST_GeomFromGeoJSON('{"type":"Polygon","coordinates":[[[-84.1,39.0],[-84.0,39.0],[-84.0,39.1],[-84.1,39.1],[-84.1,39.0]]]}'), 4326))`}, + } { + t.Run(tc.name, func(t *testing.T) { + rows, err := db.Query("EXPLAIN SELECT id FROM ohio_addresses WHERE " + tc.where) + if err != nil { + t.Fatalf("explain: %v", err) + } + defer rows.Close() + + var plan strings.Builder + for rows.Next() { + var line string + if err := rows.Scan(&line); err != nil { + t.Fatalf("scan plan: %v", err) + } + plan.WriteString(line) + plan.WriteString("\n") + } + + if !strings.Contains(plan.String(), "Index Scan") && + !strings.Contains(plan.String(), "Bitmap Index Scan") { + t.Errorf("%s predicate cannot use the GIST index:\n%s", tc.name, plan.String()) + } + t.Logf("%s plan: %s", tc.name, strings.TrimSpace(plan.String())) + }) + } +} From 35f826f5f0e8e0f9326c6aa698c6199c5041a02c Mon Sep 17 00:00:00 2001 From: Gray Fay Date: Fri, 11 Sep 2026 08:42:50 -0400 Subject: [PATCH 2/3] docs: document bbox and polygon in the OpenAPI spec gameplan generates its client from this file with openapi-typescript, so a parameter missing here is a parameter that repo's client cannot send. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01S2nhUJ4Zdsxf7cP9DapsY3 --- api-docs.yaml | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/api-docs.yaml b/api-docs.yaml index 58385be..1ef50e0 100644 --- a/api-docs.yaml +++ b/api-docs.yaml @@ -555,6 +555,28 @@ paths: type: number format: double example: -82.9988 + - name: bbox + in: query + required: false + description: >- + Restrict results to a rectangle, given as minLng,minLat,maxLng,maxLat. + Longitude first, matching GeoJSON and every mapping library, so a map + viewport can be passed straight through. A reversed or malformed box + is rejected with 400 rather than silently widening the search. + schema: + type: string + example: "-83.1,39.9,-82.9,40.1" + - name: polygon + in: query + required: false + description: >- + Restrict results to an arbitrary GeoJSON Polygon, for territories a + rectangle cannot express - a sales area, a delivery zone, a canvassing + route. The ring must be closed, with the last position repeating the + first. + schema: + type: string + example: '{"type":"Polygon","coordinates":[[[-83.1,39.9],[-82.9,39.9],[-82.9,40.1],[-83.1,40.1],[-83.1,39.9]]]}' - name: radius in: query required: false From 814d2b2a417313cf321f74181dadf451fb6c7226 Mon Sep 17 00:00:00 2001 From: Gray Fay Date: Fri, 11 Sep 2026 08:53:12 -0400 Subject: [PATCH 3/3] fix: address code review findings on territory search **NaN walked past every bbox guard.** ParseFloat accepts "NaN", and every comparison against NaN is false, so all three validation blocks passed and the value reached ST_MakeEnvelope -- producing an empty 200 or a 500, which is the exact silent failure the validation exists to stop. Non-finite values are now rejected before the range checks rather than by them. **The bbox filter was not exact.** The comment claimed && on point geometry is exact containment. It is not: && compares BOX2DF values, which are float4 and rounded outward, so at Ohio longitudes one ULP is about 0.7 m of slack. Verified directly -- a point at lng -82.8999995 against a box ending at -82.9 gives `&& = true` while `ST_Intersects = false`. A client tiling a territory into adjacent boxes double-counted every address within 0.7 m of a shared edge; a test now seeds a point on such an edge and asserts it appears in exactly one of two neighbouring boxes. With && restored it appears in both. Now ST_Intersects, matching what county_service.go already does, still index- assisted. **Only the exterior ring was validated.** A Polygon whose hole is short or unclosed passed and was rejected by GEOS inside ST_Intersects, surfacing as a 500 on a request that was merely wrong -- the outcome the validator exists to prevent. Every ring is checked now, and errors name which one. **A wrong geometry type produced an unhelpful error.** Decoding type and coordinates together made a Point fail on its coordinates with "cannot unmarshal number into .coordinates.0 of type [][]float64", telling the caller nothing about the real problem. The type is read first. **Nothing bounded polygon complexity.** An exact point-in-polygon test runs per candidate row, so cost scales with vertices as well as rows, and a shape traced off a map can carry tens of thousands. Capped at 1000 positions. **Two tests were weaker than they looked.** The polygon test compared totals and cleared by a single row -- the band's true bounding box is slightly wider than the rectangle it was compared against, so any fixture change would have flipped it into a failure blaming the polygon. It now asserts on the points actually excluded. And the index test EXPLAINed hand-copied SQL, so a change making the real predicate non-indexable would have left it passing against its own stale duplicate; both predicates are now exported constants the test formats. One thing deliberately not claimed: a latitude-first pair whose numbers are both legal coordinates cannot be detected, and the validator no longer pretends to. It reports the range and names the convention. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01S2nhUJ4Zdsxf7cP9DapsY3 --- handlers/address_handlers.go | 90 +++++++++++---- handlers/polygon_validation_test.go | 101 +++++++++++++++++ models/address.go | 15 +++ services/address_service.go | 30 +++-- services/territory_search_integration_test.go | 103 +++++++++++++++++- 5 files changed, 301 insertions(+), 38 deletions(-) create mode 100644 handlers/polygon_validation_test.go diff --git a/handlers/address_handlers.go b/handlers/address_handlers.go index e97c521..0907494 100644 --- a/handlers/address_handlers.go +++ b/handlers/address_handlers.go @@ -3,6 +3,7 @@ package handlers import ( "encoding/json" "fmt" + "math" "net/http" "strconv" @@ -235,48 +236,89 @@ func FullTextSearchAddressesHandler(c echo.Context) error { // server fault. Checking here also keeps the error specific: "not a Polygon" // is actionable, "failed to search addresses" is not. func validateGeoJSONPolygon(raw string) error { - var shape struct { - Type string `json:"type"` - Coordinates [][][]float64 `json:"coordinates"` + // Read the type before the coordinates. Decoding both at once makes a + // Point fail on its coordinates -- "cannot unmarshal number into + // .coordinates.0 of type [][]float64" -- which tells the caller nothing + // about the actual problem, that they sent the wrong geometry. + var header struct { + Type string `json:"type"` + Coordinates json.RawMessage `json:"coordinates"` } - if err := json.Unmarshal([]byte(raw), &shape); err != nil { + if err := json.Unmarshal([]byte(raw), &header); err != nil { return fmt.Errorf("polygon is not valid GeoJSON: %v", err) } - switch shape.Type { + switch header.Type { case "Polygon": case "": return fmt.Errorf(`polygon is missing its "type" field; expected {"type":"Polygon","coordinates":[...]}`) default: - return fmt.Errorf("polygon type is %q; only Polygon is supported", shape.Type) + return fmt.Errorf("polygon type is %q; only Polygon is supported", header.Type) } - if len(shape.Coordinates) == 0 || len(shape.Coordinates[0]) < 4 { - return fmt.Errorf("polygon needs a ring of at least 4 positions (the last repeating the first)") + var shape struct { + Coordinates [][][]float64 } - - ring := shape.Coordinates[0] - first, last := ring[0], ring[len(ring)-1] - if len(first) < 2 || len(last) < 2 { - return fmt.Errorf("each polygon position needs at least a longitude and a latitude") + if err := json.Unmarshal(header.Coordinates, &shape.Coordinates); err != nil { + return fmt.Errorf("polygon coordinates are not an array of rings: %v", err) } - // GeoJSON requires a closed ring. PostGIS rejects an open one, and the - // message it produces does not say which ring or why. - if first[0] != last[0] || first[1] != last[1] { - return fmt.Errorf("polygon ring is not closed: the last position must repeat the first") + + if len(shape.Coordinates) == 0 { + return fmt.Errorf("polygon has no rings") } - for i, pos := range ring { - if len(pos) < 2 { - return fmt.Errorf("position %d is missing a coordinate", i) + // Every ring, not just the exterior one. A Polygon whose first ring is + // well-formed but whose hole is short or unclosed would otherwise pass + // here and be rejected by GEOS inside ST_Intersects -- surfacing as a 500 + // on a request that is merely wrong, which is the outcome this function + // exists to prevent. + total := 0 + for r, ring := range shape.Coordinates { + where := "exterior ring" + if r > 0 { + where = fmt.Sprintf("hole %d", r) + } + + if len(ring) < 4 { + return fmt.Errorf("%s needs at least 4 positions (the last repeating the first), got %d", where, len(ring)) } - if pos[0] < -180 || pos[0] > 180 { - return fmt.Errorf("position %d has longitude %g outside -180..180 (GeoJSON is longitude first)", i, pos[0]) + total += len(ring) + + first, last := ring[0], ring[len(ring)-1] + if len(first) < 2 || len(last) < 2 { + return fmt.Errorf("each position in the %s needs at least a longitude and a latitude", where) + } + if first[0] != last[0] || first[1] != last[1] { + return fmt.Errorf("%s is not closed: the last position must repeat the first", where) } - if pos[1] < -90 || pos[1] > 90 { - return fmt.Errorf("position %d has latitude %g outside -90..90 (GeoJSON is longitude first)", i, pos[1]) + + for i, pos := range ring { + if len(pos) < 2 { + return fmt.Errorf("%s position %d is missing a coordinate", where, i) + } + if math.IsNaN(pos[0]) || math.IsNaN(pos[1]) || math.IsInf(pos[0], 0) || math.IsInf(pos[1], 0) { + return fmt.Errorf("%s position %d is not a finite coordinate", where, i) + } + if pos[0] < -180 || pos[0] > 180 { + return fmt.Errorf("%s position %d has longitude %g outside -180..180 (GeoJSON is longitude first)", where, i, pos[0]) + } + if pos[1] < -90 || pos[1] > 90 { + return fmt.Errorf("%s position %d has latitude %g outside -90..90 (GeoJSON is longitude first)", where, i, pos[1]) + } } } + // An exact point-in-polygon test runs per candidate row, so cost scales + // with vertices as well as with rows. A shape traced off a map can carry + // tens of thousands of them; bounding it keeps one request from costing + // seconds. + if total > maxPolygonVertices { + return fmt.Errorf("polygon has %d positions, more than the %d allowed; simplify the shape first", + total, maxPolygonVertices) + } + return nil } + +// maxPolygonVertices bounds how detailed a territory may be. +const maxPolygonVertices = 1000 diff --git a/handlers/polygon_validation_test.go b/handlers/polygon_validation_test.go new file mode 100644 index 0000000..2a416cd --- /dev/null +++ b/handlers/polygon_validation_test.go @@ -0,0 +1,101 @@ +package handlers + +import ( + "fmt" + "strings" + "testing" +) + +// A malformed shape must be a 400 naming the problem, not a 500 from GEOS +// deep inside ST_Intersects. Holes are the gap that mattered: validating only +// coordinates[0] let a short or unclosed hole through. +func TestPolygonValidationRejectsMalformedShapes(t *testing.T) { + cases := []struct { + name string + raw string + want string + }{ + { + name: "unclosed exterior ring", + raw: `{"type":"Polygon","coordinates":[[[-83.1,39.9],[-82.9,39.9],[-82.9,40.1]]]}`, + want: "exterior ring", + }, + { + name: "short hole", + raw: `{"type":"Polygon","coordinates":[` + + `[[-83.1,39.9],[-82.9,39.9],[-82.9,40.1],[-83.1,39.9]],` + + `[[-83.0,40.0],[-82.95,40.0]]]}`, + want: "hole 1", + }, + { + name: "unclosed hole", + raw: `{"type":"Polygon","coordinates":[` + + `[[-83.1,39.9],[-82.9,39.9],[-82.9,40.1],[-83.1,39.9]],` + + `[[-83.0,40.0],[-82.95,40.0],[-82.95,40.05],[-83.0,40.01]]]}`, + want: "hole 1", + }, + { + // Only catchable when the swap pushes a value out of range. A + // latitude-first pair whose numbers both happen to be legal + // coordinates is indistinguishable from a deliberate query for + // somewhere else on Earth, and the validator does not pretend + // otherwise -- it reports the range, and names the convention so + // the caller can spot the swap themselves. + name: "coordinates out of range", + raw: `{"type":"Polygon","coordinates":[[[39.9,-183.1],[39.9,-82.9],[40.1,-82.9],[39.9,-183.1]]]}`, + want: "longitude first", + }, + { + name: "wrong geometry type", + raw: `{"type":"Point","coordinates":[-83.0,40.0]}`, + want: "only Polygon", + }, + { + name: "not json", + raw: `not json at all`, + want: "valid GeoJSON", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + err := validateGeoJSONPolygon(tc.raw) + if err == nil { + t.Fatalf("accepted; GEOS would reject it as a 500 instead of a 400") + } + if !strings.Contains(err.Error(), tc.want) { + t.Errorf("error %q does not mention %q, so it does not tell the caller what to fix", err, tc.want) + } + }) + } +} + +// An exact point-in-polygon test runs per candidate row, so an unbounded +// vertex count lets one request cost seconds. +func TestPolygonVertexCountIsBounded(t *testing.T) { + var b strings.Builder + b.WriteString(`{"type":"Polygon","coordinates":[[`) + n := maxPolygonVertices + 10 + for i := 0; i < n; i++ { + if i > 0 { + b.WriteString(",") + } + fmt.Fprintf(&b, "[%f,%f]", -83.0+float64(i)*1e-6, 40.0) + } + b.WriteString(`,[-83.0,40.0]]]}`) + + if err := validateGeoJSONPolygon(b.String()); err == nil { + t.Error("a polygon with more than the allowed vertices was accepted") + } +} + +// A well-formed polygon with a hole has to still be accepted -- the point of +// validating every ring is correctness, not refusing legitimate shapes. +func TestPolygonWithValidHoleIsAccepted(t *testing.T) { + raw := `{"type":"Polygon","coordinates":[` + + `[[-83.1,39.9],[-82.9,39.9],[-82.9,40.1],[-83.1,40.1],[-83.1,39.9]],` + + `[[-83.05,39.95],[-82.95,39.95],[-82.95,40.05],[-83.05,40.05],[-83.05,39.95]]]}` + if err := validateGeoJSONPolygon(raw); err != nil { + t.Errorf("a valid polygon with a hole was rejected: %v", err) + } +} diff --git a/models/address.go b/models/address.go index 45c1afc..d88cade 100644 --- a/models/address.go +++ b/models/address.go @@ -2,6 +2,7 @@ package models import ( "fmt" + "math" "strconv" "strings" "time" @@ -128,6 +129,20 @@ func ParseBBox(raw string) (*BoundingBox, error) { vals[i] = v } + // ParseFloat accepts "NaN" and "Inf", and every comparison against NaN is + // false -- so all three checks below pass and the value flows into + // ST_MakeEnvelope, producing either an empty 200 or a PostGIS error. That + // is exactly the silent-empty-result failure this validation exists to + // stop, so it has to be rejected before the range checks, not by them. + for i, v := range vals { + if math.IsNaN(v) { + return nil, fmt.Errorf("bbox value %d is NaN", i+1) + } + if math.IsInf(v, 0) { + return nil, fmt.Errorf("bbox value %d is infinite", i+1) + } + } + box := &BoundingBox{MinLng: vals[0], MinLat: vals[1], MaxLng: vals[2], MaxLat: vals[3]} if box.MinLng < -180 || box.MaxLng > 180 || box.MinLat < -90 || box.MaxLat > 90 { diff --git a/services/address_service.go b/services/address_service.go index fe32435..b0bd9d4 100644 --- a/services/address_service.go +++ b/services/address_service.go @@ -168,13 +168,9 @@ func (s *AddressService) searchAddresses(q querier, params models.AddressSearchP } } - // Bounding box. ST_MakeEnvelope builds the rectangle; && is the indexed - // bbox overlap operator, which for point geometry is exact containment -- - // a point either is inside the rectangle or is not, so no recheck is - // needed and the GIST index on geom does all the work. + // Bounding box. if params.BBox != nil { - conditions = append(conditions, fmt.Sprintf( - "geom && ST_MakeEnvelope($%d, $%d, $%d, $%d, 4326)", + conditions = append(conditions, fmt.Sprintf(BBoxPredicateSQL, argIndex, argIndex+1, argIndex+2, argIndex+3)) args = append(args, params.BBox.MinLng, params.BBox.MinLat, params.BBox.MaxLng, params.BBox.MaxLat) argIndex += 4 @@ -188,8 +184,7 @@ func (s *AddressService) searchAddresses(q querier, params models.AddressSearchP // ST_GeomFromGeoJSON raises on malformed input, so the shape is validated // in the handler and never reaches here unchecked. if params.Polygon != "" { - conditions = append(conditions, fmt.Sprintf( - "ST_Intersects(geom, ST_SetSRID(ST_GeomFromGeoJSON($%d), 4326))", argIndex)) + conditions = append(conditions, fmt.Sprintf(PolygonPredicateSQL, argIndex)) args = append(args, params.Polygon) argIndex++ } @@ -1222,3 +1217,22 @@ func clamp01(v float64) float64 { } return v } + +// BBoxPredicateSQL and PolygonPredicateSQL are the spatial filters, exported so +// a test can EXPLAIN the same text the builder emits. Asserting against a +// hand-copied duplicate would keep passing after the real predicate changed to +// a non-indexable shape, which is the regression the assertion exists to catch. +// +// ST_Intersects rather than the && operator, deliberately. && compares BOX2DF +// values, which are float4 and rounded outward -- at Ohio longitudes one ULP is +// about 0.7 m, so && returns points up to that far OUTSIDE the rectangle. +// Verified: a point at lng -82.8999995 against a box ending at -82.9 gives +// `&& = true`, `ST_Intersects = false`. A client tiling a territory into +// adjacent boxes would double-count every address within 0.7 m of a shared +// edge. ST_Intersects still uses the GIST index to shortlist, then tests each +// candidate exactly, which is what county_service.go already does for the same +// job. +const ( + BBoxPredicateSQL = "ST_Intersects(geom, ST_MakeEnvelope($%d, $%d, $%d, $%d, 4326))" + PolygonPredicateSQL = "ST_Intersects(geom, ST_SetSRID(ST_GeomFromGeoJSON($%d), 4326))" +) diff --git a/services/territory_search_integration_test.go b/services/territory_search_integration_test.go index 0d9a709..7133c6e 100644 --- a/services/territory_search_integration_test.go +++ b/services/territory_search_integration_test.go @@ -1,6 +1,7 @@ package services import ( + "fmt" "strings" "testing" @@ -90,15 +91,38 @@ func TestPolygonExcludesWhatItsBoundingBoxWouldInclude(t *testing.T) { if polyTotal == 0 { t.Fatal("the band follows the fixture's diagonal and should contain its points") } - if polyTotal >= boxTotal { - t.Errorf("polygon returned %d and its bounding box %d; the polygon is not narrowing anything", - polyTotal, boxTotal) - } + + // Assert on what the polygon excluded, not on a total that happens to be + // one smaller. The band's true bounding box is slightly wider than `box`, + // so it picks up a diagonal point `box` misses -- comparing totals cleared + // by a single row, and any fixture change would have flipped it into a + // failure claiming the polygon was not narrowing anything when it was. + var offLine int for _, r := range rows { if r.Street == "Off Line Road" { + offLine++ t.Errorf("%s sits off the band and should have been excluded", r.FullAddress) } } + + // The two seeded points are inside the rectangle by construction, so the + // rectangle must return them and the band must not. + boxRows, _, err := svc.SearchAddresses(models.AddressSearchParams{BBox: box, Limit: 500}) + if err != nil { + t.Fatalf("bbox rows: %v", err) + } + var offLineInBox int + for _, r := range boxRows { + if r.Street == "Off Line Road" { + offLineInBox++ + } + } + if offLineInBox != 2 { + t.Errorf("the rectangle returned %d off-diagonal points, want 2 -- the fixture no longer sets up the contrast", offLineInBox) + } + if offLine != 0 { + t.Errorf("the polygon returned %d off-diagonal points, want 0", offLine) + } } // Territory filters have to combine with the rest of the query, or they are a @@ -191,12 +215,20 @@ func TestTerritorySearchCanUseTheSpatialIndex(t *testing.T) { } defer db.Exec("SET enable_seqscan = on") + // Built from the same constants the query builder uses, so changing the + // predicate to a non-indexable shape fails here instead of quietly passing + // against a stale hand-copied duplicate. + bbox := fmt.Sprintf(strings.NewReplacer("$%d", "%s").Replace(BBoxPredicateSQL), + "-84.1", "39.0", "-84.0", "39.1") + polygon := fmt.Sprintf(strings.NewReplacer("$%d", "%s").Replace(PolygonPredicateSQL), + `'{"type":"Polygon","coordinates":[[[-84.1,39.0],[-84.0,39.0],[-84.0,39.1],[-84.1,39.1],[-84.1,39.0]]]}'`) + for _, tc := range []struct { name string where string }{ - {"bbox", "geom && ST_MakeEnvelope(-84.1, 39.0, -84.0, 39.1, 4326)"}, - {"polygon", `ST_Intersects(geom, ST_SetSRID(ST_GeomFromGeoJSON('{"type":"Polygon","coordinates":[[[-84.1,39.0],[-84.0,39.0],[-84.0,39.1],[-84.1,39.1],[-84.1,39.0]]]}'), 4326))`}, + {"bbox", bbox}, + {"polygon", polygon}, } { t.Run(tc.name, func(t *testing.T) { rows, err := db.Query("EXPLAIN SELECT id FROM ohio_addresses WHERE " + tc.where) @@ -223,3 +255,62 @@ func TestTerritorySearchCanUseTheSpatialIndex(t *testing.T) { }) } } + +// NaN passes every range comparison, because comparisons against NaN are all +// false. Before this was caught, bbox=NaN,39.9,-82.9,40.1 parsed cleanly and +// flowed into ST_MakeEnvelope -- an empty 200 or a 500, which is the exact +// silent failure the validation exists to prevent. +func TestBBoxRejectsNonFiniteValues(t *testing.T) { + for _, raw := range []string{ + "NaN,39.9,-82.9,40.1", + "-83.1,nan,-82.9,40.1", + "Inf,39.9,-82.9,40.1", + "-83.1,39.9,-Inf,40.1", + } { + if box, err := models.ParseBBox(raw); err == nil { + t.Errorf("%q was accepted as %+v; it slips past every range check", raw, box) + } + } +} + +// Exact containment matters for anyone tiling a territory into adjacent boxes: +// the && operator compares float4 bounding boxes rounded outward, so it +// returns points up to ~0.7m outside the rectangle and every address near a +// shared edge lands in both tiles. +func TestAdjacentBoxesDoNotDoubleCount(t *testing.T) { + db := setupCountTestDB(t) + svc := NewAddressService(db) + + // A point placed a hair outside the western box's eastern edge. + if _, err := db.Exec(` + INSERT INTO ohio_addresses (hash, house_number, street, unit, city, district, region, postcode, county, geom, full_address) + VALUES ('edge-point', '9', 'Edge Road', '', 'Columbus', 'FRA', 'OH', '43004', 'Franklin', + ST_SetSRID(ST_MakePoint(-83.0999995, 39.5), 4326), '9 Edge Road, Columbus, OH 43004') + `); err != nil { + t.Fatalf("seed edge point: %v", err) + } + + west := &models.BoundingBox{MinLng: -83.2, MinLat: 39.4, MaxLng: -83.1, MaxLat: 39.6} + east := &models.BoundingBox{MinLng: -83.1, MinLat: 39.4, MaxLng: -83.0, MaxLat: 39.6} + + count := func(box *models.BoundingBox) int { + rows, _, err := svc.SearchAddresses(models.AddressSearchParams{BBox: box, Limit: 500}) + if err != nil { + t.Fatalf("search: %v", err) + } + n := 0 + for _, r := range rows { + if r.Hash == "edge-point" { + n++ + } + } + return n + } + + inWest, inEast := count(west), count(east) + t.Logf("edge point appears in west=%d east=%d", inWest, inEast) + if inWest+inEast != 1 { + t.Errorf("the point on the shared edge appears %d times across two adjacent boxes, want exactly 1", + inWest+inEast) + } +}