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 diff --git a/handlers/address_handlers.go b/handlers/address_handlers.go index 092b9fa..0907494 100644 --- a/handlers/address_handlers.go +++ b/handlers/address_handlers.go @@ -1,12 +1,15 @@ package handlers import ( + "encoding/json" "fmt" - "geocoding-api/models" - "geocoding-api/services" + "math" "net/http" "strconv" + "geocoding-api/models" + "geocoding-api/services" + "github.com/labstack/echo/v4" ) @@ -37,6 +40,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 +110,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 +228,97 @@ 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 { + // 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), &header); err != nil { + return fmt.Errorf("polygon is not valid GeoJSON: %v", err) + } + + 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", header.Type) + } + + var shape struct { + Coordinates [][][]float64 + } + if err := json.Unmarshal(header.Coordinates, &shape.Coordinates); err != nil { + return fmt.Errorf("polygon coordinates are not an array of rings: %v", err) + } + + if len(shape.Coordinates) == 0 { + return fmt.Errorf("polygon has no rings") + } + + // 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)) + } + 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) + } + + 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 1125a8e..d88cade 100644 --- a/models/address.go +++ b/models/address.go @@ -1,6 +1,10 @@ package models import ( + "fmt" + "math" + "strconv" + "strings" "time" ) @@ -71,8 +75,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 +99,61 @@ 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 + } + + // 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 { + 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..b0bd9d4 100644 --- a/services/address_service.go +++ b/services/address_service.go @@ -168,6 +168,27 @@ func (s *AddressService) searchAddresses(q querier, params models.AddressSearchP } } + // Bounding box. + if params.BBox != nil { + 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 + } + + // 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(PolygonPredicateSQL, argIndex)) + args = append(args, params.Polygon) + argIndex++ + } + // County filter if params.County != "" { conditions = append(conditions, fmt.Sprintf("county ILIKE $%d", argIndex)) @@ -1196,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 new file mode 100644 index 0000000..7133c6e --- /dev/null +++ b/services/territory_search_integration_test.go @@ -0,0 +1,316 @@ +package services + +import ( + "fmt" + "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") + } + + // 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 +// 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") + + // 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", bbox}, + {"polygon", polygon}, + } { + 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())) + }) + } +} + +// 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) + } +}