diff --git a/POI/categories.go b/POI/categories.go index be8364ea..b119c23d 100644 --- a/POI/categories.go +++ b/POI/categories.go @@ -3,6 +3,7 @@ package POI import ( "fmt" "math" + "sort" "strings" ) @@ -33,34 +34,137 @@ type LocationType string const ( // LocationTypeAny leaves the Google Maps place type unset, used by keyword (brand) searches - LocationTypeAny = LocationType("") - LocationTypeCafe = LocationType("cafe") - LocationTypeRestaurant = LocationType("restaurant") - LocationTypeBar = LocationType("bar") - LocationTypeBakery = LocationType("bakery") - LocationTypeMealTakeaway = LocationType("meal_takeaway") - LocationTypeMuseum = LocationType("museum") - LocationTypeGallery = LocationType("art_gallery") - LocationTypeAmusementPark = LocationType("amusement_park") - LocationTypePark = LocationType("park") + LocationTypeAny = LocationType("") + // Eatery place types + LocationTypeCafe = LocationType("cafe") + LocationTypeRestaurant = LocationType("restaurant") + LocationTypeBar = LocationType("bar") + LocationTypeBakery = LocationType("bakery") + LocationTypeMealTakeaway = LocationType("meal_takeaway") + LocationTypeMealDelivery = LocationType("meal_delivery") + LocationTypeNightClub = LocationType("night_club") + // Visit place types + LocationTypeMuseum = LocationType("museum") + LocationTypeGallery = LocationType("art_gallery") + LocationTypeAmusementPark = LocationType("amusement_park") + LocationTypePark = LocationType("park") + LocationTypeTouristAttraction = LocationType("tourist_attraction") + LocationTypeZoo = LocationType("zoo") + LocationTypeAquarium = LocationType("aquarium") + LocationTypeMovieTheater = LocationType("movie_theater") + LocationTypeStadium = LocationType("stadium") + LocationTypeBowlingAlley = LocationType("bowling_alley") // Shopping place types LocationTypeShoppingMall = LocationType("shopping_mall") LocationTypeDepartmentStore = LocationType("department_store") LocationTypeSupermarket = LocationType("supermarket") LocationTypeClothingStore = LocationType("clothing_store") LocationTypeStore = LocationType("store") + // LocationTypeGroceryOrSupermarket is a types[]-only value: it appears in a place's + // Types list but is NOT a legal ?type= value for the legacy Nearby Search + // (maps.ParsePlaceType rejects it). Never pass it to CreateMapSearchRequest or add it to + // GetPlaceTypes; it is matched only via PrimaryLocationType/GetPlaceCategory. + LocationTypeGroceryOrSupermarket = LocationType("grocery_or_supermarket") + // Any type that is a strict specialization of the already-mapped `store` goes to Shopping. + LocationTypeConvenienceStore = LocationType("convenience_store") + LocationTypeHardwareStore = LocationType("hardware_store") + LocationTypeHomeGoodsStore = LocationType("home_goods_store") + LocationTypeElectronicsStore = LocationType("electronics_store") + LocationTypeFurnitureStore = LocationType("furniture_store") + LocationTypeBookStore = LocationType("book_store") + LocationTypeShoeStore = LocationType("shoe_store") + LocationTypeJewelryStore = LocationType("jewelry_store") + LocationTypePetStore = LocationType("pet_store") + LocationTypeBicycleStore = LocationType("bicycle_store") + LocationTypeFlorist = LocationType("florist") + LocationTypeLiquorStore = LocationType("liquor_store") + LocationTypeGasStation = LocationType("gas_station") // Lodging place types LocationTypeLodging = LocationType("lodging") // Wellness place types - LocationTypeGym = LocationType("gym") - LocationTypeSpa = LocationType("spa") - LocationTypePharmacy = LocationType("pharmacy") + LocationTypeGym = LocationType("gym") + LocationTypeSpa = LocationType("spa") + LocationTypePharmacy = LocationType("pharmacy") + LocationTypeDrugstore = LocationType("drugstore") + LocationTypeBeautySalon = LocationType("beauty_salon") + LocationTypeHairCare = LocationType("hair_care") ) +// placeTypeToCategory is the reverse map from a Google place type to its category. It backs +// both GetPlaceCategory (the write path / classification rule) and ReclassifyForCategory (the +// read filter) below, so there is exactly one table that decides "what category does this +// Google type belong to" anywhere in the service. +// +// It is a SUPERSET of GetPlaceTypes' inverse: it covers every Google primary type (see +// PrimaryLocationType) this service knows how to classify, not only the types the nearby-search +// endpoints actively query for (GetPlaceTypes' 18 searched types are all present here too). +// Widening this map only ever makes ReclassifyForCategory keep MORE places, never fewer — see +// TestReclassifyForCategoryKeepsAllFormerlySearchedTypes. +var placeTypeToCategory = map[LocationType]PlaceCategory{ + // Eatery + LocationTypeCafe: PlaceCategoryEatery, + LocationTypeRestaurant: PlaceCategoryEatery, + LocationTypeBar: PlaceCategoryEatery, + LocationTypeBakery: PlaceCategoryEatery, + LocationTypeMealTakeaway: PlaceCategoryEatery, + LocationTypeMealDelivery: PlaceCategoryEatery, + LocationTypeNightClub: PlaceCategoryEatery, + + // Visit + LocationTypePark: PlaceCategoryVisit, + LocationTypeAmusementPark: PlaceCategoryVisit, + LocationTypeGallery: PlaceCategoryVisit, + LocationTypeMuseum: PlaceCategoryVisit, + LocationTypeTouristAttraction: PlaceCategoryVisit, + LocationTypeZoo: PlaceCategoryVisit, + LocationTypeAquarium: PlaceCategoryVisit, + LocationTypeMovieTheater: PlaceCategoryVisit, + LocationTypeStadium: PlaceCategoryVisit, + LocationTypeBowlingAlley: PlaceCategoryVisit, + + // Shopping. Any type that is a strict specialization of the already-mapped `store` goes + // to Shopping. + LocationTypeShoppingMall: PlaceCategoryShopping, + LocationTypeDepartmentStore: PlaceCategoryShopping, + LocationTypeSupermarket: PlaceCategoryShopping, + LocationTypeClothingStore: PlaceCategoryShopping, + LocationTypeStore: PlaceCategoryShopping, + LocationTypeGroceryOrSupermarket: PlaceCategoryShopping, + LocationTypeConvenienceStore: PlaceCategoryShopping, + LocationTypeHardwareStore: PlaceCategoryShopping, + LocationTypeHomeGoodsStore: PlaceCategoryShopping, + LocationTypeElectronicsStore: PlaceCategoryShopping, + LocationTypeFurnitureStore: PlaceCategoryShopping, + LocationTypeBookStore: PlaceCategoryShopping, + LocationTypeShoeStore: PlaceCategoryShopping, + LocationTypeJewelryStore: PlaceCategoryShopping, + LocationTypePetStore: PlaceCategoryShopping, + LocationTypeBicycleStore: PlaceCategoryShopping, + LocationTypeFlorist: PlaceCategoryShopping, + LocationTypeLiquorStore: PlaceCategoryShopping, + LocationTypeGasStation: PlaceCategoryShopping, + + // Lodging + LocationTypeLodging: PlaceCategoryLodging, + + // Wellness + LocationTypeGym: PlaceCategoryWellness, + LocationTypeSpa: PlaceCategoryWellness, + LocationTypePharmacy: PlaceCategoryWellness, + LocationTypeDrugstore: PlaceCategoryWellness, + LocationTypeBeautySalon: PlaceCategoryWellness, + LocationTypeHairCare: PlaceCategoryWellness, + + // LocationTypeAny ("") is deliberately NOT a key: GetPlaceCategory("") must stay + // ("", false), the same as any other unmapped type. +} + // GetPlaceCategory maps a Google Maps place type back to its category, reporting whether -// the type is mapped at all. It is the inverse of GetPlaceTypes and MUST stay consistent -// with it: the nearby-search cache writes each place under -// EncodeNearbySearchRedisKey(GetPlaceCategory(place.LocationType), ...), so a type that +// the type is mapped at all. placeTypeToCategory (above) is now a SUPERSET of GetPlaceTypes' +// inverse, not its exact inverse — it classifies every primary type this service recognizes, +// while GetPlaceTypes still only lists the subset each category's Nearby Search issues as +// ?type=. The write path relies on the shared subset: the nearby-search cache writes each place +// under EncodeNearbySearchRedisKey(GetPlaceCategory(place.LocationType), ...), so a type that // resolves to a different category than the one it was searched under would never cache-hit. // // It deliberately has NO default category. An earlier version defaulted to Eatery, which @@ -69,24 +173,34 @@ const ( // GetPlaceTypes(Eatery), Google ignored the unenforceable filter, and prominence-ranked // hotels were written into the eatery geo buckets. Returning ok=false forces every caller // to decide what an unmapped type means, and makes TestPlaceCategoryRoundTrip able to fail. +// +// DO NOT ADD: fast_food_restaurant, food_court. Both are Places API (New)-only values that +// never appear in a legacy Nearby Search result's types[], so adding them re-opens the exact +// guard class TestGetPlaceCategoryRejectsUnknownTypes exists for. func GetPlaceCategory(placeType LocationType) (PlaceCategory, bool) { - switch placeType { - case LocationTypePark, LocationTypeAmusementPark, LocationTypeGallery, LocationTypeMuseum: - return PlaceCategoryVisit, true - case LocationTypeCafe, LocationTypeRestaurant, LocationTypeBar, LocationTypeBakery, LocationTypeMealTakeaway: - return PlaceCategoryEatery, true - case LocationTypeShoppingMall, LocationTypeDepartmentStore, LocationTypeSupermarket, LocationTypeClothingStore, LocationTypeStore: - return PlaceCategoryShopping, true - case LocationTypeLodging: - return PlaceCategoryLodging, true - case LocationTypeGym, LocationTypeSpa, LocationTypePharmacy: - return PlaceCategoryWellness, true - default: - return PlaceCategory(""), false + cat, ok := placeTypeToCategory[placeType] + return cat, ok +} + +// MappedLocationTypes returns every LocationType classified by GetPlaceCategory, sorted for +// deterministic iteration. Exported for tests (e.g. TestGetPlaceCategoryKeysAreGoogleTypes), +// which need to walk the full map without depending on Go's randomized map iteration order. +func MappedLocationTypes() []LocationType { + keys := make([]LocationType, 0, len(placeTypeToCategory)) + for t := range placeTypeToCategory { + keys = append(keys, t) } + sort.Slice(keys, func(i, j int) bool { return keys[i] < keys[j] }) + return keys } -// GetPlaceTypes returns a set of types defined in Google Maps API given a location type +// GetPlaceTypes returns a set of types defined in Google Maps API given a location type. This +// is the SEARCHED subset: the exact place types each category's Nearby Search issues as ?type=. +// It is intentionally unchanged by the classification-map expansion above — widening it changes +// the outbound Google query for every existing search, not just how a result gets classified, +// and is how the fast_food_restaurant incident happened (a type the legacy API doesn't +// understand, silently ignored by Google, poisoning the eatery cache). Add new types to +// placeTypeToCategory / GetPlaceCategory instead of here. func GetPlaceTypes(placeCat PlaceCategory) (placeTypes []LocationType) { switch placeCat { case PlaceCategoryVisit: @@ -161,25 +275,35 @@ func PrimaryLocationType(types []string) LocationType { return LocationType("") } -// ReclassifyForCategory decides whether a place belongs in cat based on its -// PRIMARY function, and returns the place re-tagged with that primary type. +// ReclassifyForCategory decides whether a place belongs in cat based on its PRIMARY function, +// and returns the place re-tagged with that primary type. It now keys on the same +// placeTypeToCategory map as GetPlaceCategory — the same map the nearby-search write path +// (SetPlacesAddGeoLocations) and the bucket-cleanup migration +// (RemoveMisclassifiedPlacesFromCategoryBuckets) key on — so there is one rule everywhere for +// "does this place belong in this category": // -// - primary type is one of cat's search types → keep, LocationType := primary +// - primary type maps to cat → keep, LocationType := primary // (e.g. a "cafe"-searched result that is really a restaurant is re-tagged). -// - primary type is known but NOT in cat → drop (keep=false): its main +// - primary type maps to a DIFFERENT category → drop (keep=false): its main // function is something else (a supermarket the food search returned). -// - no Types on the place (older cached records) → keep unchanged, so coverage -// never regresses on data written before Types was captured. +// - no Types at all (empty primary) → keep unchanged (older cached records), so +// coverage never regresses on data written before Types was captured. +// - primary type is present but unmapped → drop (keep=false), same as the old rule: +// an unmapped primary was never a member of GetPlaceTypes(cat) either, so this is not a +// behavior change from before the placeTypeToCategory unification. +// +// Because placeTypeToCategory is a strict superset of GetPlaceTypes' searched types (see +// GetPlaceCategory's docstring), this keeps every place the old primary-in-GetPlaceTypes(cat) +// rule kept, plus more — never fewer. TestReclassifyForCategoryKeepsAllFormerlySearchedTypes +// pins that monotonicity. func ReclassifyForCategory(place Place, cat PlaceCategory) (Place, bool) { primary := PrimaryLocationType(place.Types) if primary == LocationType("") { - return place, true + return place, true // records with no Types stay kept (older cache entries) } - for _, t := range GetPlaceTypes(cat) { - if t == primary { - place.LocationType = primary - return place, true - } + if c, ok := GetPlaceCategory(primary); ok && c == cat { + place.LocationType = primary // re-tag with the true type + return place, true } return place, false } diff --git a/README.md b/README.md index 50b500c2..3efd2f5f 100644 --- a/README.md +++ b/README.md @@ -20,6 +20,128 @@ will incorporate personalized recommendations. * View trip details * Make a plan yourself by creating a template +## Place Search API + +Two endpoints let a caller add a place to the shared cache by name, as an alternative to the +category-based nearby-places search: search by free text, then confirm one result into the +cache. + +### `POST /v1/place-search` + +Runs a free-text Google Places search around a coordinate and returns every result as a +confirmable candidate; nothing is written to the shared cache by this call. Every result +(insertable or not) is stashed server-side under its Google place ID for 30 minutes so a +subsequent confirm call can resolve it by ID alone rather than trusting place data an HTTP +caller might send back. + +Request: + +```json +{ + "query": "Joe's Pizza", + "location": {"latitude": 40.7309, "longitude": -74.0021}, + "radius": 5000, + "limit": 10 +} +``` + +`query` is required (2-120 characters). `location` (`latitude`/`longitude`) is required, with +no default — same as the nearby-places endpoints' own zero-location rejection — because an +unbiased text query like "konjoe" can resolve to the wrong continent without a coordinate to +anchor it. `radius` is in meters and is clamped to the service's max search radius (16,000 m / +~10 miles) when zero or larger than that maximum. `limit` defaults to 10 and is capped at 20. + +Response (`200`, fields elided for brevity): + +```json +{ + "results": [ + { + "place": { + "ID": "ChIJd8BlQ2BZwokRAFUEcm_qrcA", + "Name": "Joe's Pizza", + "Status": "OPERATIONAL", + "LocationType": "restaurant", + "Types": ["restaurant", "food", "point_of_interest", "establishment"], + "FormattedAddress": "7 Carmine St, New York, NY 10014", + "Location": {"latitude": 40.7309, "longitude": -74.0021, "city": "", "adminAreaLevelOne": "", "country": ""}, + "PriceLevel": 1, + "Rating": 4.5, + "UserRatingsTotal": 3200 + }, + "category": "Eatery", + "insertable": true + } + ] +} +``` + +`category` and `insertable` are always derived server-side from the place's own Google types. +A candidate whose primary type does not map to a known category is still returned (so the +caller can see it), but with `category: ""` and `insertable: false`. + +### `POST /v1/place-search/confirm` + +Inserts a previously returned candidate into the shared cache — `placeIDs:` plus a +`place_details:place_ID:*` record — making one Place Details call to fill in hours, address, +URL, summary, and (as a gap-fill only, never overwriting an existing photo) photo before +writing. + +Request: + +```json +{"placeId": "ChIJd8BlQ2BZwokRAFUEcm_qrcA"} +``` + +Response (`200`): + +```json +{ + "place": { "ID": "ChIJd8BlQ2BZwokRAFUEcm_qrcA", "...": "same shape as place-search's place object, now enriched with hours/URL/summary" }, + "category": "Eatery", + "alreadyCached": false +} +``` + +Error responses: + +* `404` `{"error": "...", "code": "candidate_expired"}` — the place ID was never searched, or + its 30-minute stash entry expired. +* `422` `{"error": "...", "code": "unsupported_place_type", "placeType": ""}` — + the candidate's primary Google type does not map to any category; nothing is written. + +#### Visibility + +Confirming a place writes it into the shared Redis cache immediately — you can verify this +directly, without waiting on any other endpoint: the confirm response itself echoes the +written place, and `ZSCORE placeIDs: ` against Redis returns a score right +away. + +Whether the place then shows up in `/v1/nearby-places-by-category` depends on that cell's +search freshness, not on the write above. In a **warm** cell (one whose `MapsLastSearchTime` +marker is still fresh), the confirmed place appears on the very next read. In a **cold or +stale** cell, the next read triggers a background Google search whose results replace — not +merge with — the cached bucket in that response, so the newly confirmed place is briefly +missing from that one response and only appears from the following read onward. When +verifying a confirm in a cell you are not sure is warm, check `ZSCORE` first and expect to +need up to two reads of `/v1/nearby-places-by-category` before the place shows up. + +Never manually stamp a cell's `MapsLastSearchTime` to force this — doing so marks the cell +"searched" for 14 days and would suppress a real cold search the cell still needs. + +### Authentication + +Both endpoints require the same authentication as the other `/v1` endpoints: a Personal +Access Token via `Authorization: Bearer `, or a JWT session cookie as a browser +fallback. + +### Safety design + +The category a place lands under is always computed server-side from Google's own primary +type on the place, never accepted from the caller, and a primary type that maps to no known +category is refused outright rather than defaulted into some bucket — so nothing +client-supplied ever reaches the shared place cache unclassified. + ## Installation (Mac) * git clone the repository diff --git a/docs/migrations/reclassify-buckets.md b/docs/migrations/reclassify-buckets.md index c22e9148..f64154b5 100644 --- a/docs/migrations/reclassify-buckets.md +++ b/docs/migrations/reclassify-buckets.md @@ -32,12 +32,54 @@ evidence of misclassification, because the write path would legitimately place i | `lodging` | Lodging | removed | | `supermarket`, `department_store` | Shopping | removed | | `cafe`, `restaurant` | Eatery | kept | -| `meal_delivery`, `night_club` | unmapped | kept | +| `meal_delivery`, `night_club` | Eatery (positively mapped) | kept | +| `stadium` (found in the Eatery bucket, per the production audit below) | Visit | removed | +| `hardware_store`, `shoe_store`, `furniture_store` (found in the Eatery bucket, per the production audit below) | Shopping | removed | +| `movie_theater` (rule example only — pinned by the test truth table; never observed in the production Eatery bucket, see Known residue) | Visit | removed | +| `university`, `airport`, `real_estate_agency`, `doctor`, `finance`, `local_government_office`, `general_contractor`, `veterinary_care` | unmapped | kept | | no `types[]` at all | unmapped | kept | -⚠️ This rule and the write rule (`SetPlacesAddGeoLocations`, which keys on the stamped -`LocationType`) deliberately disagree. A refactor that "unifies" them reintroduces the -incident. `TestRemoveMisclassifiedPlacesPrimaryTypeTruthTable` pins the dangerous direction. +`meal_delivery` and `night_club` used to fall in the "unmapped, kept" row above: the map had +no entry for them, so `GetPlaceCategory` returned `ok=false` and the migration kept them for +lack of any evidence of misclassification. The place-type map expansion (`POI/categories.go`, +`placeTypeToCategory`) added them as positive Eatery entries, so a `night_club`-primaried +member of the Eatery bucket is no longer residue by omission — it is now recognized as +correctly filed. The verdict (kept) is unchanged; the reason changed from "unmapped, benefit +of the doubt" to "positively belongs here." Conversely, `stadium`, `hardware_store`, +`shoe_store`, and `furniture_store` used to be unmapped-and-kept too; they are now positively +mapped to a category *other than* Eatery (Visit for `stadium`; Shopping for the other three — +`shoe_store` and `furniture_store` are two more instances of the `*_store` → Shopping +expansion that also added `hardware_store`), so a bucket member primaried with one of them is +newly REMOVABLE when this migration runs against `category=Eatery`. `movie_theater` follows +the identical rule (it maps to Visit) but, unlike the four types above, was never actually +found in the production Eatery bucket — it appears only as a synthetic fixture in +`TestRemoveMisclassifiedPlacesPrimaryTypeTruthTable`, included there to pin the rule generally +rather than to model an observed record. +`TestRemoveMisclassifiedPlacesPrimaryTypeTruthTable` (`test/redis_client_mocks/bucket_cleanup_test.go`) +pins all of these verdicts, including the newly-removable types. + +⚠️ Two different pairs of rules are in play here, and only one of them is now unified: + +- **Unified:** this migration's removal rule and the read filter the merchant endpoint applies + (`POI.ReclassifyForCategory`) both now key off the place's **primary** Google type + (`POI.PrimaryLocationType(place.Types)`) through the same `placeTypeToCategory` table via + `POI.GetPlaceCategory`. That unification landed in the place-text-search PR ("Expand + place-type reverse map and unify ReclassifyForCategory on it") and is intentional — one table + now decides "does this place belong in this category" for both the admin cleanup path and the + merchant-endpoint read path. (The two are not byte-for-byte identical in every case — the read + filter drops a place whose primary type is present but still unmapped, while this migration's + removal rule treats that same case as "not evidence of misclassification" and leaves it + in the bucket — but they agree on the case that matters for safety: a primary type that + positively resolves to a *different* category is excluded by both.) +- **Must stay apart:** this migration's removal rule and the **write** rule + (`SetPlacesAddGeoLocations`, which keys on the place's stamped `LocationType` — the type it was + *searched* under, not necessarily its true primary type) deliberately disagree, and must keep + disagreeing. A refactor that "unifies" the removal rule with the write rule reintroduces the + `fast_food_restaurant` incident: the write rule's whole job is to accept whatever type a search + was run under, and folding the primary-type check into it would let an unenforceable `?type=` + silently relabel results again. + +`TestRemoveMisclassifiedPlacesPrimaryTypeTruthTable` pins the dangerous direction. ## Expected scale @@ -56,12 +98,65 @@ mostly San Francisco / Tulsa / Boise hotels); the other 41 are pre-existing misclassifications the rule also catches (hotels stamped `restaurant`, supermarkets stamped `bakery`). -**Known residue:** 27 incident records are *not* removed because their primary type maps -to no category — `university` ×5, `airport` ×2, `real_estate_agency` ×2, `stadium`, -`night_club`, `hardware_store`, `doctor`, and 7 with no `types[]`. These stay in the +⚠️ The numbers above (145/0/12/4/3, 164 total) predate the place-type map expansion. The next +dry run must be diffed against *these* baseline numbers, and the counts are **expected to +change** — the removal rule now recognizes primary types (`stadium`, `hardware_store`, +`shoe_store`, `furniture_store`, and others) it used to treat as unmapped-and-kept, so the +Eatery candidate count in particular should go up by (at least) the 5 records itemized below. +A jump versus this baseline is not by itself a red flag; it is what widening the map is +supposed to do. What it does mean: **do not apply blind**. Spot-check each newly-appearing +candidate class (i.e. each distinct primary type showing up in `RemovedIDs` that wasn't there +before) against a few real records before running with `apply=true`, the same way +`123 fast_food_restaurant` vs `41 pre-existing` was broken out above. + +**Known residue:** complete production audit (2026-07-30) of all 27 records in `placeIDs:eatery` +whose primary type mapped to no category under the pre-expansion map: + +| Primary type | Count | Example | +| --- | ---: | --- | +| (no `types[]`) | 7 | Williams Co Inc, The Tulsa Theater, Oktoberfest Main Office, Sun Valley Music Festival | +| `university` | 5 | Ohlone College, The University of Tulsa, Boise State University, Concordia College | +| `shoe_store` | 2 | LOFT, JoS. A. Bank | +| `airport` | 2 | Boise Airport, Sun Valley Gun Club | +| `real_estate_agency` | 2 | Avalon Mountain View, Mission Peaks Apartments | +| `furniture_store` | 1 | Topnotch Fine Furnishings & Interior Design | +| `hardware_store` | 1 | The Home Depot | +| `finance` | 1 | The UPS Store | +| `doctor` | 1 | Ricardo Delgado, MD | +| `local_government_office` | 1 | Tulsa County Assessor | +| `stadium` | 1 | BOK Center | +| `night_club` | 1 | Cain's Ballroom | +| `general_contractor` | 1 | Pella Windows and Doors Showroom of Ketchum, ID | +| `veterinary_care` | 1 | Sun Valley Animal Center | + +(An earlier version of this table elided 7 of these records — `shoe_store` ×2, +`furniture_store`, `finance`, `local_government_office`, `general_contractor`, and +`veterinary_care` — which is why its stated "27" total didn't match its own itemization. The +table above is the complete audit.) + +With the expanded `placeTypeToCategory` map, this splits three ways: + +- **Removable (5):** `shoe_store` ×2 and `furniture_store` ×1 are now positively mapped to + Shopping, the same `*_store` → Shopping expansion that also added `hardware_store` ×1 + (a strict specialization of the already-mapped `store` type); `stadium` ×1 is now positively + mapped to Visit. These 4 primary types — 5 records, counting both `shoe_store` instances — + become removable candidates the next time this migration runs against `category=Eatery`. +- **Legitimized (1):** `night_club` ×1 (Cain's Ballroom) is now positively mapped to Eatery — + the same category its bucket already files it under — so it is no longer unresolved residue + at all, just a correctly-classified place. +- **Still residue (21):** `university` ×5, `airport` ×2, `real_estate_agency` ×2, `doctor` ×1, + `finance` ×1, `local_government_office` ×1, `general_contractor` ×1, `veterinary_care` ×1, + and the 7 records with no `types[]` at all remain unmapped and kept, for lack of any evidence + of misclassification — none of those types were added to the map. + +**27 → 5 removable + 1 legitimized + 21 residue.** The 21 still-residue records stay in the eatery buckets. The trip-planning path (`planner/solver.go`) does not reclassify, so they -remain reachable in generated plans. Broadening `GetPlaceCategory` to cover those types is -tracked as follow-up work. +remain reachable in generated plans. Broadening `GetPlaceCategory` to cover `university`, +`airport`, `real_estate_agency`, `doctor`, `finance`, `local_government_office`, +`general_contractor`, and `veterinary_care` is tracked as follow-up work. This migration has +not been re-run against production since the map expanded; the split above is derived from the +audited pre-expansion snapshot, not from a fresh dry run — re-confirm with one before relying on +it operationally. Separately, ~328 bucket members have no backing `place_details` record. The migration skips them. Their likely source has since been fixed: `removePlace` deleted the record but ZREMmed diff --git a/iowrappers/add_searched_place_test.go b/iowrappers/add_searched_place_test.go new file mode 100644 index 00000000..f5fbf8e8 --- /dev/null +++ b/iowrappers/add_searched_place_test.go @@ -0,0 +1,530 @@ +package iowrappers + +import ( + "context" + "errors" + "net/url" + "testing" + "time" + + "github.com/alicebob/miniredis/v2" + "github.com/weihesdlegend/Vacation-planner/POI" + "googlemaps.github.io/maps" +) + +// newAddSearchedPlaceFixture builds a PoiSearcher backed by its own miniredis instance, mirroring +// data_migrations_test.go's own-server-per-test harness. mapsClient is left nil: the unexported +// addSearchedPlaceToCache never touches it (that is the entire point of the placeDetailsEnricher +// seam), only the exported AddSearchedPlaceToCache does. +func newAddSearchedPlaceFixture(t *testing.T) (*PoiSearcher, context.Context) { + t.Helper() + redisMockSvr, err := miniredis.Run() + if err != nil { + t.Fatalf("miniredis.Run: %v", err) + } + t.Cleanup(redisMockSvr.Close) + + redisURL, _ := url.Parse("redis://" + redisMockSvr.Addr()) + redisClient := CreateRedisClient(redisURL) + if err := CreateLogger(); err != nil { + t.Fatalf("CreateLogger: %v", err) + } + + return &PoiSearcher{redisClient: redisClient}, context.Background() +} + +// stashCandidate writes a search candidate directly, standing in for what +// PoiSearcher.TextSearchPlaces would have stashed. +func stashCandidate(t *testing.T, s *PoiSearcher, ctx context.Context, place POI.Place) { + t.Helper() + if err := s.redisClient.SetPlaceSearchCandidate(ctx, place, PlaceSearchCandidateTTL); err != nil { + t.Fatalf("SetPlaceSearchCandidate: %v", err) + } +} + +func succeedingEnricher(details maps.PlaceDetailsResult) placeDetailsEnricher { + return func(ctx context.Context, placeID string) (maps.PlaceDetailsResult, error) { + return details, nil + } +} + +func failingEnricher(err error) placeDetailsEnricher { + return func(ctx context.Context, placeID string) (maps.PlaceDetailsResult, error) { + return maps.PlaceDetailsResult{}, err + } +} + +// assertNothingWrittenForPlace checks that neither a place_details record nor any category geo +// bucket membership exists for placeID — the guarantee the refusal path must uphold. +func assertNothingWrittenForPlace(t *testing.T, s *PoiSearcher, ctx context.Context, placeID string) { + t.Helper() + cached, err := s.redisClient.CachedPlaces(ctx, []string{placeID}) + if err != nil { + t.Fatalf("CachedPlaces: %v", err) + } + if len(cached) != 0 { + t.Errorf("place_details record exists for %s, want none written on refusal", placeID) + } + + for _, cat := range POI.AllPlaceCategories { + key := POI.EncodeNearbySearchRedisKey(cat) + score, err := s.redisClient.Get().ZScore(ctx, key, placeID).Result() + if err == nil { + t.Errorf("place %s found in bucket %s with score %v, want absent", placeID, key, score) + } + } +} + +// TestAddSearchedPlaceToCache_UnmappedPrimaryRefusesAndWritesNothing pins the refusal path: a +// candidate whose primary Google type has no POI.PlaceCategory mapping must be rejected with +// ErrUnsupportedPlaceType, and the refusal must be total — no place_details record, no bucket +// membership, before the enricher is even asked to run. +func TestAddSearchedPlaceToCache_UnmappedPrimaryRefusesAndWritesNothing(t *testing.T) { + s, ctx := newAddSearchedPlaceFixture(t) + + placeID := "unmapped-1" + candidate := POI.Place{ + ID: placeID, + Name: "Springfield Elementary", + Types: []string{"school", "point_of_interest", "establishment"}, // "school" is not in placeTypeToCategory + } + stashCandidate(t, s, ctx, candidate) + + enricherCalled := false + enrich := func(ctx context.Context, placeID string) (maps.PlaceDetailsResult, error) { + enricherCalled = true + return maps.PlaceDetailsResult{}, nil + } + + _, err := s.addSearchedPlaceToCache(ctx, placeID, enrich) + if err == nil { + t.Fatal("expected an error for an unmapped primary type, got nil") + } + if !errors.Is(err, ErrUnsupportedPlaceType) { + t.Errorf("error %v does not wrap ErrUnsupportedPlaceType", err) + } + if enricherCalled { + t.Error("enricher was called on the refusal path; it must short-circuit before any enrichment or write") + } + + assertNothingWrittenForPlace(t, s, ctx, placeID) +} + +// TestAddSearchedPlaceToCache_MappedPrimarySucceeds pins the happy path: a mapped primary type +// lands the place in its category's geo bucket and place_details record, tagged with the primary +// type. +func TestAddSearchedPlaceToCache_MappedPrimarySucceeds(t *testing.T) { + s, ctx := newAddSearchedPlaceFixture(t) + + placeID := "museum-1" + candidate := POI.Place{ + ID: placeID, + Name: "City History Museum", + LocationType: POI.LocationType("tourist_attraction"), // deliberately NOT the primary type + Types: []string{"museum", "point_of_interest", "establishment"}, + Location: POI.Location{Latitude: 37.4, Longitude: -122.1}, + } + stashCandidate(t, s, ctx, candidate) + + result, err := s.addSearchedPlaceToCache(ctx, placeID, failingEnricher(errors.New("no network in this test"))) + if err != nil { + t.Fatalf("addSearchedPlaceToCache: %v", err) + } + + if result.Category != POI.PlaceCategoryVisit { + t.Errorf("Category = %q, want %q", result.Category, POI.PlaceCategoryVisit) + } + if result.AlreadyCached { + t.Error("AlreadyCached = true on a first confirm, want false") + } + if result.Place.LocationType != POI.LocationType("museum") { + t.Errorf("LocationType = %q, want the primary type %q", result.Place.LocationType, "museum") + } + + key := POI.EncodeNearbySearchRedisKey(POI.PlaceCategoryVisit) + if _, err := s.redisClient.Get().ZScore(ctx, key, placeID).Result(); err != nil { + t.Errorf("expected %s to be a member of %s, ZScore error: %v", placeID, key, err) + } + + cached, err := s.redisClient.CachedPlaces(ctx, []string{placeID}) + if err != nil { + t.Fatalf("CachedPlaces: %v", err) + } + if _, ok := cached[placeID]; !ok { + t.Errorf("place_details record missing for %s after a successful confirm", placeID) + } +} + +// TestAddSearchedPlaceToCache_CrossCategoryLandsInWellnessNotEatery pins that a pharmacy-primary +// candidate is filed under placeIDs:wellness and never under placeIDs:eatery. +func TestAddSearchedPlaceToCache_CrossCategoryLandsInWellnessNotEatery(t *testing.T) { + s, ctx := newAddSearchedPlaceFixture(t) + + placeID := "pharmacy-1" + candidate := POI.Place{ + ID: placeID, + Name: "Corner Drugstore", + Types: []string{"pharmacy", "point_of_interest", "establishment"}, + Location: POI.Location{Latitude: 37.4, Longitude: -122.1}, + } + stashCandidate(t, s, ctx, candidate) + + result, err := s.addSearchedPlaceToCache(ctx, placeID, failingEnricher(errors.New("no network in this test"))) + if err != nil { + t.Fatalf("addSearchedPlaceToCache: %v", err) + } + if result.Category != POI.PlaceCategoryWellness { + t.Fatalf("Category = %q, want %q", result.Category, POI.PlaceCategoryWellness) + } + + wellnessKey := POI.EncodeNearbySearchRedisKey(POI.PlaceCategoryWellness) + if _, err := s.redisClient.Get().ZScore(ctx, wellnessKey, placeID).Result(); err != nil { + t.Errorf("expected %s in %s, ZScore error: %v", placeID, wellnessKey, err) + } + + eateryKey := POI.EncodeNearbySearchRedisKey(POI.PlaceCategoryEatery) + if _, err := s.redisClient.Get().ZScore(ctx, eateryKey, placeID).Result(); err == nil { + t.Errorf("place %s unexpectedly present in %s", placeID, eateryKey) + } +} + +// TestAddSearchedPlaceToCache_AlreadyCachedRealHoursPreserved pins restoreCachedDetails' role in +// this flow: confirming a place that is already cached with real opening hours must never +// overwrite those hours with the lean (default-hours) stashed candidate, even when the Details +// enrich call fails. +func TestAddSearchedPlaceToCache_AlreadyCachedRealHoursPreserved(t *testing.T) { + s, ctx := newAddSearchedPlaceFixture(t) + + placeID := "museum-hours-1" + existing := POI.Place{ + ID: placeID, + Name: "City History Museum", + LocationType: POI.LocationType("museum"), + Types: []string{"museum", "point_of_interest", "establishment"}, + Location: POI.Location{Latitude: 37.4, Longitude: -122.1}, + Hours: [7]string{ + "9:00 am – 5:00 pm", "9:00 am – 5:00 pm", "9:00 am – 5:00 pm", "9:00 am – 5:00 pm", + "9:00 am – 5:00 pm", "9:00 am – 5:00 pm", "Closed", + }, + } + if !existing.HasRealOpeningHours() { + t.Fatal("fixture setup bug: existing.HasRealOpeningHours() should be true") + } + s.redisClient.SetPlacesAddGeoLocations(ctx, []POI.Place{existing}) + + // The stashed candidate is lean: zero-value Hours, which is not "real" per HasRealOpeningHours + // (empty strings do not count). + candidate := POI.Place{ + ID: placeID, + Name: "City History Museum", + Types: []string{"museum", "point_of_interest", "establishment"}, + Location: POI.Location{Latitude: 37.4, Longitude: -122.1}, + } + stashCandidate(t, s, ctx, candidate) + + result, err := s.addSearchedPlaceToCache(ctx, placeID, failingEnricher(errors.New("enrich down"))) + if err != nil { + t.Fatalf("addSearchedPlaceToCache: %v", err) + } + if !result.AlreadyCached { + t.Error("AlreadyCached = false, want true (the place was cached before this confirm)") + } + if !result.Place.HasRealOpeningHours() { + t.Fatal("result.Place lost its real opening hours") + } + + cached, err := s.redisClient.CachedPlaces(ctx, []string{placeID}) + if err != nil { + t.Fatalf("CachedPlaces: %v", err) + } + stored, ok := cached[placeID] + if !ok { + t.Fatal("place_details record missing after confirm") + } + if !stored.HasRealOpeningHours() { + t.Error("stored record's real opening hours were clobbered by the lean confirm") + } + if stored.Hours != existing.Hours { + t.Errorf("stored hours = %+v, want unchanged %+v", stored.Hours, existing.Hours) + } +} + +// TestAddSearchedPlaceToCache_ConfirmTwiceIsIdempotent pins that confirming the same place twice +// leaves a single geo-bucket member and flips AlreadyCached from false to true. +func TestAddSearchedPlaceToCache_ConfirmTwiceIsIdempotent(t *testing.T) { + s, ctx := newAddSearchedPlaceFixture(t) + + placeID := "museum-twice-1" + candidate := POI.Place{ + ID: placeID, + Name: "City History Museum", + Types: []string{"museum", "point_of_interest", "establishment"}, + Location: POI.Location{Latitude: 37.4, Longitude: -122.1}, + } + stashCandidate(t, s, ctx, candidate) + + first, err := s.addSearchedPlaceToCache(ctx, placeID, failingEnricher(errors.New("down"))) + if err != nil { + t.Fatalf("first confirm: %v", err) + } + if first.AlreadyCached { + t.Error("first confirm: AlreadyCached = true, want false") + } + + // Re-stash: the candidate stash is what a second TextSearchPlaces call (or the same result + // still within TTL) would have left behind. + stashCandidate(t, s, ctx, candidate) + + second, err := s.addSearchedPlaceToCache(ctx, placeID, failingEnricher(errors.New("down"))) + if err != nil { + t.Fatalf("second confirm: %v", err) + } + if !second.AlreadyCached { + t.Error("second confirm: AlreadyCached = false, want true") + } + + key := POI.EncodeNearbySearchRedisKey(POI.PlaceCategoryVisit) + members, err := s.redisClient.Get().ZRange(ctx, key, 0, -1).Result() + if err != nil { + t.Fatalf("ZRange: %v", err) + } + count := 0 + for _, m := range members { + if m == placeID { + count++ + } + } + if count != 1 { + t.Errorf("place %s appears %d times in %s, want exactly 1", placeID, count, key) + } +} + +// TestAddSearchedPlaceToCache_MissingCandidateReturnsNotFound pins that confirming a place ID that +// was never searched (or whose stash entry expired) fails with ErrSearchCandidateNotFound. +func TestAddSearchedPlaceToCache_MissingCandidateReturnsNotFound(t *testing.T) { + s, ctx := newAddSearchedPlaceFixture(t) + + _, err := s.addSearchedPlaceToCache(ctx, "never-searched", failingEnricher(errors.New("should not be called"))) + if err == nil { + t.Fatal("expected an error for a missing candidate, got nil") + } + if !errors.Is(err, ErrSearchCandidateNotFound) { + t.Errorf("error %v does not wrap ErrSearchCandidateNotFound", err) + } +} + +// TestAddSearchedPlaceToCache_EnricherErrorStillCaches pins that a Details enrich failure is +// best-effort: the place is still confirmed into the cache using the lean stashed record. +func TestAddSearchedPlaceToCache_EnricherErrorStillCaches(t *testing.T) { + s, ctx := newAddSearchedPlaceFixture(t) + + placeID := "museum-enrich-fail-1" + candidate := POI.Place{ + ID: placeID, + Name: "City History Museum", + Types: []string{"museum", "point_of_interest", "establishment"}, + Location: POI.Location{Latitude: 37.4, Longitude: -122.1}, + } + stashCandidate(t, s, ctx, candidate) + + result, err := s.addSearchedPlaceToCache(ctx, placeID, failingEnricher(errors.New("Google is down"))) + if err != nil { + t.Fatalf("addSearchedPlaceToCache should succeed despite an enrich failure, got: %v", err) + } + if result.Place.ID != placeID { + t.Errorf("result.Place.ID = %q, want %q", result.Place.ID, placeID) + } + + cached, err := s.redisClient.CachedPlaces(ctx, []string{placeID}) + if err != nil { + t.Fatalf("CachedPlaces: %v", err) + } + if _, ok := cached[placeID]; !ok { + t.Error("place was not cached despite the enrich failure being best-effort") + } +} + +// TestAddSearchedPlaceToCache_EnricherSuccessFoldsDetails pins that a successful enrich call folds +// weekday hours, formatted address, URL and editorial summary onto the confirmed place. +func TestAddSearchedPlaceToCache_EnricherSuccessFoldsDetails(t *testing.T) { + s, ctx := newAddSearchedPlaceFixture(t) + + placeID := "museum-enrich-ok-1" + candidate := POI.Place{ + ID: placeID, + Name: "City History Museum", + Types: []string{"museum", "point_of_interest", "establishment"}, + Location: POI.Location{Latitude: 37.4, Longitude: -122.1}, + } + stashCandidate(t, s, ctx, candidate) + + weekdayText := []string{ + "Monday: 9:00 am – 5:00 pm", "Tuesday: 9:00 am – 5:00 pm", "Wednesday: 9:00 am – 5:00 pm", + "Thursday: 9:00 am – 5:00 pm", "Friday: 9:00 am – 5:00 pm", "Saturday: 10:00 am – 4:00 pm", + "Sunday: Closed", + } + details := maps.PlaceDetailsResult{ + OpeningHours: &maps.OpeningHours{WeekdayText: weekdayText}, + FormattedAddress: "1 Museum Way, Springfield", + AdrAddress: `1 Museum Way`, + URL: "https://maps.google.com/?cid=123", + EditorialSummary: &maps.PlaceEditorialSummary{Overview: "A fine local museum."}, + } + + result, err := s.addSearchedPlaceToCache(ctx, placeID, succeedingEnricher(details)) + if err != nil { + t.Fatalf("addSearchedPlaceToCache: %v", err) + } + if !result.Place.HasRealOpeningHours() { + t.Error("enriched place should have real opening hours") + } + if result.Place.FormattedAddress != details.FormattedAddress { + t.Errorf("FormattedAddress = %q, want %q", result.Place.FormattedAddress, details.FormattedAddress) + } + if result.Place.URL != details.URL { + t.Errorf("URL = %q, want %q", result.Place.URL, details.URL) + } + if result.Place.Summary != details.EditorialSummary.Overview { + t.Errorf("Summary = %q, want %q", result.Place.Summary, details.EditorialSummary.Overview) + } +} + +// TestAddSearchedPlaceToCache_AlreadyCachedPhotoPreserved pins the Finding-1 fix: restoreCachedDetails +// (iowrappers/nearby_search.go) restores URL/Summary/FormattedAddress/Address/Hours but NOT Photo, +// because it is shared with the nearby-search write path and this task must not touch it. Without a +// local gap-fill, a lean re-confirm of an already-cached place (e.g. a text search result that +// carried no photos, combined with a failed Details enrich) would silently overwrite a real +// Photo.Reference with the zero value while still reporting success. +func TestAddSearchedPlaceToCache_AlreadyCachedPhotoPreserved(t *testing.T) { + s, ctx := newAddSearchedPlaceFixture(t) + + placeID := "museum-photo-1" + existing := POI.Place{ + ID: placeID, + Name: "City History Museum", + LocationType: POI.LocationType("museum"), + Types: []string{"museum", "point_of_interest", "establishment"}, + Location: POI.Location{Latitude: 37.4, Longitude: -122.1}, + Photo: POI.PlacePhoto{Reference: "existing-photo-ref", Height: 400, Width: 600}, + } + s.redisClient.SetPlacesAddGeoLocations(ctx, []POI.Place{existing}) + + // The stashed candidate is lean: no photo, as a text-search result commonly has none. + candidate := POI.Place{ + ID: placeID, + Name: "City History Museum", + Types: []string{"museum", "point_of_interest", "establishment"}, + Location: POI.Location{Latitude: 37.4, Longitude: -122.1}, + } + stashCandidate(t, s, ctx, candidate) + + // Enrich fails too (Google down), so nothing supplies a fresh photo either — the only source + // of truth for a photo here is the previously-cached record. + result, err := s.addSearchedPlaceToCache(ctx, placeID, failingEnricher(errors.New("enrich down"))) + if err != nil { + t.Fatalf("addSearchedPlaceToCache: %v", err) + } + if result.Place.Photo != existing.Photo { + t.Errorf("result.Place.Photo = %+v, want the preserved %+v", result.Place.Photo, existing.Photo) + } + + cached, err := s.redisClient.CachedPlaces(ctx, []string{placeID}) + if err != nil { + t.Fatalf("CachedPlaces: %v", err) + } + stored, ok := cached[placeID] + if !ok { + t.Fatal("place_details record missing after confirm") + } + if stored.Photo != existing.Photo { + t.Errorf("stored.Photo = %+v, want the preserved %+v (a lean confirm silently clobbered it)", stored.Photo, existing.Photo) + } +} + +// TestAddSearchedPlaceToCache_EnricherPhotoFoldedWhenCandidateHasNone pins the second half of the +// Finding-1 fix: "photos" is already requested in config/config.yml's detailed_search_fields, so +// the confirm path already pays for it in the enrich call; a candidate with no photo of its own +// should pick up the Details photo rather than the call's cost being thrown away. +func TestAddSearchedPlaceToCache_EnricherPhotoFoldedWhenCandidateHasNone(t *testing.T) { + s, ctx := newAddSearchedPlaceFixture(t) + + placeID := "museum-photo-2" + candidate := POI.Place{ + ID: placeID, + Name: "City History Museum", + Types: []string{"museum", "point_of_interest", "establishment"}, + Location: POI.Location{Latitude: 37.4, Longitude: -122.1}, + } + stashCandidate(t, s, ctx, candidate) + + details := maps.PlaceDetailsResult{ + Photos: []maps.Photo{{PhotoReference: "fresh-photo-ref", Height: 300, Width: 500}}, + } + + result, err := s.addSearchedPlaceToCache(ctx, placeID, succeedingEnricher(details)) + if err != nil { + t.Fatalf("addSearchedPlaceToCache: %v", err) + } + want := POI.PlacePhoto{Reference: "fresh-photo-ref", Height: 300, Width: 500} + if result.Place.Photo != want { + t.Errorf("result.Place.Photo = %+v, want %+v (Details photo should be folded in when the candidate had none)", result.Place.Photo, want) + } +} + +// TestNewPlaceDetailsEnricher_BoundsContextWithDeadline pins the Finding-2 fix: the real Google +// Place Details call must be bounded by GoogleMapsSearchTimeout before the semaphore is acquired. +// The maps SDK's HTTP client has no timeout of its own and an inbound request context carries no +// deadline by default, so without this bound a single hung call would park a process-wide +// apiSemaphore slot indefinitely. Uses a stub search func rather than a real client or a hang +// harness — CreatePoiSearcher always builds a real maps.Client even with a fake key, so there is +// no way to observe this via the real enricher without either a live network call or an actual +// hang. +func TestNewPlaceDetailsEnricher_BoundsContextWithDeadline(t *testing.T) { + sem := make(chan struct{}, 1) + var sawDeadline bool + var sawWithin time.Duration + stub := func(ctx context.Context, placeID string, fields []string) (maps.PlaceDetailsResult, error) { + deadline, ok := ctx.Deadline() + sawDeadline = ok + if ok { + sawWithin = time.Until(deadline) + } + return maps.PlaceDetailsResult{}, nil + } + + enrich := newPlaceDetailsEnricher(sem, []string{"name"}, stub) + if _, err := enrich(context.Background(), "some-id"); err != nil { + t.Fatalf("enrich: %v", err) + } + + if !sawDeadline { + t.Fatal("search func's ctx had no deadline; a hung Google call could hold the semaphore slot forever") + } + if sawWithin <= 0 || sawWithin > GoogleMapsSearchTimeout { + t.Errorf("ctx deadline is %v from now, want in (0, %v]", sawWithin, GoogleMapsSearchTimeout) + } +} + +// TestNewPlaceDetailsEnricher_AcquiresAndReleasesSemaphore pins that the semaphore is held for the +// duration of the search call and released afterward, preserving the existing rate-limiting +// behavior across the refactor that extracted newPlaceDetailsEnricher for testability. +func TestNewPlaceDetailsEnricher_AcquiresAndReleasesSemaphore(t *testing.T) { + sem := make(chan struct{}, 1) + var sawSemaphoreHeld bool + stub := func(ctx context.Context, placeID string, fields []string) (maps.PlaceDetailsResult, error) { + sawSemaphoreHeld = len(sem) == 1 + return maps.PlaceDetailsResult{}, nil + } + + enrich := newPlaceDetailsEnricher(sem, nil, stub) + if _, err := enrich(context.Background(), "some-id"); err != nil { + t.Fatalf("enrich: %v", err) + } + + if !sawSemaphoreHeld { + t.Error("semaphore was not held while the search call was in flight") + } + if len(sem) != 0 { + t.Errorf("semaphore not released after enrich returned, len(sem) = %d, want 0", len(sem)) + } +} diff --git a/iowrappers/data_migrations.go b/iowrappers/data_migrations.go index 703b9074..0bbb98ad 100644 --- a/iowrappers/data_migrations.go +++ b/iowrappers/data_migrations.go @@ -374,24 +374,28 @@ func (s *PoiSearcher) UnionEateryPriceBucketsIntoCategoryBucket(ctx context.Cont // ignored the unenforceable type filter and returned prominence-ranked establishments, and // those were stamped with the queried type and written into placeIDs:eatery:level*. // -// The removal rule is the exact inverse of the WRITE rule, not of POI.ReclassifyForCategory. -// SetPlacesAddGeoLocations files a place under GetPlaceCategory(place.LocationType) and -// refuses to write anything whose type maps to no category, so this migration removes a -// member only when its primary type positively maps to a DIFFERENT category (lodging -> -// Lodging, supermarket -> Shopping). An UNMAPPED primary type is deliberately kept: types -// like "meal_delivery" and "night_club" are legal legacy types that Google routinely lists -// first for genuine eateries (a delivery-first restaurant, a bar that is also a club), as are -// records with no Types at all (cached before Types was captured). Those places carry a -// stamped LocationType the write path maps straight back to this category, so removing them -// would only delete rows the next cold search re-creates — while shrinking the trip-planning -// candidate pool for up to MinMapsResultRefreshDuration, because the trip-planning path -// (planner/solver.go -> matching.NearbySearchForCategory) reads these buckets with no -// reclassification at all. +// The removal rule is the exact inverse of the WRITE rule. SetPlacesAddGeoLocations files a +// place under GetPlaceCategory(place.LocationType) and refuses to write anything whose type +// maps to no category, so this migration removes a member only when its primary type +// positively maps to a DIFFERENT category (lodging -> Lodging, supermarket -> Shopping). An +// UNMAPPED primary type is deliberately kept: types like "university", "airport", and +// "real_estate_agency" are legal legacy types placeTypeToCategory (POI/categories.go) does not +// classify, as are records with no Types at all (cached before Types was captured). Those +// places carry a stamped LocationType the write path maps straight back to this category, so +// removing them would only delete rows the next cold search re-creates — while shrinking the +// trip-planning candidate pool for up to MinMapsResultRefreshDuration, because the +// trip-planning path (planner/solver.go -> matching.NearbySearchForCategory) reads these +// buckets with no reclassification at all. // -// Note this is a broader keep-set than POI.ReclassifyForCategory applies on the merchant -// endpoint: that function keeps a place only when its primary type is one of the category's -// five search types. Divergence is intended — one function decides what to show in a single -// response, this one decides what may exist in the shared cache. +// Since the place-text-search PR unified POI.ReclassifyForCategory (the merchant-endpoint read +// filter) onto the same placeTypeToCategory table via GetPlaceCategory, both this migration and +// that read filter now agree on the case that matters: a primary type that positively resolves +// to a different category is excluded by both. They are not identical in every case — the read +// filter also drops a place whose primary type is present but still unmapped, while this +// migration treats that same case as "not evidence of misclassification" and leaves it in the +// bucket (see docs/migrations/reclassify-buckets.md) — but that divergence is intentional: one +// function decides what to show in a single response, this one decides what may exist in the +// shared cache. // // dryRun reports what would be removed without deleting anything. Always dry-run first. func (r *RedisClient) RemoveMisclassifiedPlacesFromCategoryBuckets(ctx context.Context, cat POI.PlaceCategory, dryRun bool) (BucketCleanupReport, error) { @@ -437,9 +441,9 @@ func (r *RedisClient) RemoveMisclassifiedPlacesFromCategoryBuckets(ctx context.C place := places[i] primary := POI.PrimaryLocationType(place.Types) // Only remove members whose primary type positively belongs to a DIFFERENT - // category. An unmapped primary type (meal_delivery, night_club, or no Types - // at all) is not evidence of misclassification — the write path would - // legitimately place it here. + // category. An unmapped primary type (university, airport, + // real_estate_agency, or no Types at all) is not evidence of + // misclassification — the write path would legitimately place it here. if c, ok := POI.GetPlaceCategory(primary); !ok || c == cat { continue } diff --git a/iowrappers/redis_client.go b/iowrappers/redis_client.go index 285e021e..3cb74c30 100644 --- a/iowrappers/redis_client.go +++ b/iowrappers/redis_client.go @@ -139,6 +139,38 @@ func (r *RedisClient) setPlace(context context.Context, place POI.Place) error { return err } +// SetPlaceSearchCandidate stashes a text-search result under its place ID with a TTL, so a later +// confirm (PoiSearcher.AddSearchedPlaceToCache) can resolve it by ID alone without trusting any +// place data an HTTP caller might send back. Plain SET with expiration, mirroring setPlace's +// style but never overwriting the permanent place_details record it lives beside. +func (r *RedisClient) SetPlaceSearchCandidate(ctx context.Context, place POI.Place, ttl time.Duration) error { + json_, err := json.Marshal(place) + if err != nil { + return err + } + + _, err = r.client.Set(ctx, PlaceSearchCandidateRedisKeyPrefix+place.ID, json_, ttl).Result() + return err +} + +// PlaceSearchCandidate resolves a stashed text-search result by place ID. A miss (never stashed, +// or the TTL expired) wraps ErrSearchCandidateNotFound so callers can distinguish it from other +// Redis failures. +func (r *RedisClient) PlaceSearchCandidate(ctx context.Context, placeID string) (POI.Place, error) { + var place POI.Place + res, err := r.client.Get(ctx, PlaceSearchCandidateRedisKeyPrefix+placeID).Result() + if err != nil { + if errors.Is(err, redis.Nil) { + return place, fmt.Errorf("%w: place_id %q", ErrSearchCandidateNotFound, placeID) + } + return place, err + } + if err := json.Unmarshal([]byte(res), &place); err != nil { + return place, err + } + return place, nil +} + // GetMapsLastSearchTime reports when an external maps search last covered the location cell // containing (lat, lng) for this category and price level. The field is keyed on the cell // rather than the city because the geo buckets it guards are read from arbitrary coordinates — diff --git a/iowrappers/text_search.go b/iowrappers/text_search.go new file mode 100644 index 00000000..92310a66 --- /dev/null +++ b/iowrappers/text_search.go @@ -0,0 +1,347 @@ +package iowrappers + +// Free-text place search: a user types a query ("Joe's Pizza"), Google Text Search returns +// candidates, and each candidate is stashed server-side under its place ID so a subsequent +// confirm (AddSearchedPlaceToCache) can look it up by ID alone rather than trusting anything an +// HTTP caller sends back. The category a candidate would land in is always derived server-side +// from POI.GetPlaceCategory(POI.PrimaryLocationType(...)) — never accepted from a caller — and an +// unmapped primary type refuses the write entirely rather than guessing a bucket. + +import ( + "context" + "errors" + "fmt" + "time" + + "github.com/weihesdlegend/Vacation-planner/POI" + "googlemaps.github.io/maps" +) + +const ( + // PlaceSearchCandidateRedisKeyPrefix stores the full POI.Place a text search returned, keyed + // by place ID, so a later confirm can resolve it without a second Google call and without + // trusting any place data the confirm request itself might carry. + PlaceSearchCandidateRedisKeyPrefix = "place_search_candidate:place_ID:" + // PlaceSearchCandidateTTL bounds how long a search result stays confirmable. Long enough for + // a user to review results and pick one, short enough that stale candidates do not accumulate. + PlaceSearchCandidateTTL = 30 * time.Minute + // PlaceTextSearchMaxResults is one legacy Text Search page; the legacy API does not support + // paging through more without a page token round trip, which this feature does not need. + PlaceTextSearchMaxResults = 20 +) + +var ( + // ErrSearchCandidateNotFound means the place ID either was never searched or its stash entry + // expired (see PlaceSearchCandidateTTL). + ErrSearchCandidateNotFound = errors.New("place search candidate not found or expired") + // ErrUnsupportedPlaceType means the candidate's primary Google type does not map to any + // POI.PlaceCategory. The insert refuses entirely on this path: writing the place would force + // a guess at which geo bucket it belongs in, which is exactly how the fast_food_restaurant + // incident poisoned the eatery bucket with hotels (see POI/categories.go). + ErrUnsupportedPlaceType = errors.New("place type does not map to a supported category") +) + +// TextSearchRequest is the input to MapsClient.TextSearchPlaces / PoiSearcher.TextSearchPlaces. +type TextSearchRequest struct { + Query string + Location POI.Location + Radius uint + // Limit caps the number of results returned. <= 0 means PlaceTextSearchMaxResults. + Limit int +} + +// PlaceSearchCandidate is one text-search result, annotated with the category it would be filed +// under if confirmed. Category is "" and Insertable is false when the place's primary Google type +// is not one POI.GetPlaceCategory recognizes; such candidates are still returned and stashed so a +// confirm attempt gets an accurate 422 instead of a confusing 404 (see AddSearchedPlaceToCache). +type PlaceSearchCandidate struct { + Place POI.Place `json:"place"` + Category POI.PlaceCategory `json:"category"` + Insertable bool `json:"insertable"` +} + +// AddSearchedPlaceResult is the outcome of confirming a search candidate into the shared cache. +type AddSearchedPlaceResult struct { + Place POI.Place `json:"place"` + Category POI.PlaceCategory `json:"category"` + AlreadyCached bool `json:"alreadyCached"` +} + +// placeDetailsEnricher fetches Place Details for a place ID. It exists as an injectable seam so +// tests can confirm a candidate without a real Google call: CreatePoiSearcher always builds a +// real maps.Client, even when constructed with a fake API key, so there is no way to make a +// PoiSearcher's mapsClient a test double directly. +type placeDetailsEnricher func(ctx context.Context, placeID string) (maps.PlaceDetailsResult, error) + +// TextSearchPlaces issues a Google Places Text Search and parses the response into POI.Place +// values. The request deliberately sets nothing beyond Query, Location and Radius: no Type, no +// OpenNow, no MinPrice/MaxPrice/Language/Region. A Type filter is especially dangerous here — +// Google silently ignores an unenforceable type on the legacy API rather than erroring, which is +// how hotels once got written into the eatery cache (see POI/categories.go's GetPlaceCategory +// doc). Text search has no type to filter on in the first place; the category is decided entirely +// after the fact, server-side, from what Google actually returns. +func (c *MapsClient) TextSearchPlaces(ctx context.Context, req *TextSearchRequest) ([]POI.Place, error) { + ctx, cancel := context.WithTimeout(ctx, GoogleMapsSearchTimeout) + defer cancel() + + mapsReq := &maps.TextSearchRequest{ + Query: req.Query, + Location: &maps.LatLng{ + Lat: req.Location.Latitude, + Lng: req.Location.Longitude, + }, + Radius: req.Radius, + } + + // Acquire semaphore for API rate limiting, mirroring Geocode/ReverseGeocode above. + c.apiSemaphore <- struct{}{} + defer func() { <-c.apiSemaphore }() + + resp, err := c.client.TextSearch(ctx, mapsReq) + if err != nil { + return nil, err + } + + return parseTextSearchResponse(resp, req.Limit), nil +} + +// parseTextSearchResponse converts a Text Search response into POI.Place values. Pure and +// side-effect free so the parsing rules can be pinned by table tests without a Redis or Google +// dependency. +// +// Deliberately does NOT filter UserRatingsTotal == 0, unlike parsePlacesSearchResponse (nearby +// search). That filter drops exactly the new-and-obscure places this feature exists to let a user +// add by name. +func parseTextSearchResponse(resp maps.PlacesSearchResponse, limit int) []POI.Place { + effectiveLimit := PlaceTextSearchMaxResults + if limit > 0 && limit < PlaceTextSearchMaxResults { + effectiveLimit = limit + } + + seen := make(map[string]bool, len(resp.Results)) + places := make([]POI.Place, 0, len(resp.Results)) + for _, res := range resp.Results { + if len(places) >= effectiveLimit { + break + } + if res.PlaceID == "" { + continue + } + if res.Geometry.Location == (maps.LatLng{}) { + continue + } + if seen[res.PlaceID] { + continue + } + if res.BusinessStatus == string(POI.ClosedPermanently) { + continue + } + seen[res.PlaceID] = true + + businessStatus := res.BusinessStatus + if businessStatus == "" { + // A blank status becomes StatusNotAvailable via Place.SetStatus, and the read path + // (RedisClient.NearbySearch) filters to Operational-only, so a blank status would + // make the place permanently invisible. A place the user just free-text-searched is + // operational by observation, so treat a missing status as Operational rather than + // as "unknown, hide it". + Logger.Debugf("parseTextSearchResponse: place %s (%q) has no business_status from Google Text Search; treating as Operational", res.PlaceID, res.Name) + businessStatus = string(POI.Operational) + } + + locationType := POI.PrimaryLocationType(res.Types) + + var photo *maps.Photo + if len(res.Photos) > 0 { + photo = &res.Photos[0] + } + + // No weekday hours exist in a Text Search response (openingHours is nil here), so the + // resulting place always has CreatePlace's DefaultOpeningHours placeholder and + // HasRealOpeningHours() is false. That is intentional: it is why confirming a candidate + // buys a Place Details call (see AddSearchedPlaceToCache). + place := POI.CreatePlace(res.Name, "", res.FormattedAddress, businessStatus, locationType, nil, + res.PlaceID, res.PriceLevel, res.Rating, "", photo, res.UserRatingsTotal, + res.Geometry.Location.Lat, res.Geometry.Location.Lng, nil) + // Preserve Google's actual feature types so callers (and the confirm path) can classify + // the place by its primary function rather than by the free-text query that found it. + place.Types = res.Types + places = append(places, place) + } + return places +} + +// TextSearchPlaces runs a free-text Google search and stashes every result — insertable or not — +// as a confirmable candidate keyed by place ID. Non-insertable candidates are stashed too: doing +// so is cheap, and it lets a later confirm attempt on that ID report an accurate 422 (unsupported +// type) instead of a confusing 404 (candidate not found). +func (s *PoiSearcher) TextSearchPlaces(ctx context.Context, req *TextSearchRequest) ([]PlaceSearchCandidate, error) { + places, err := s.mapsClient.TextSearchPlaces(ctx, req) + if err != nil { + return nil, err + } + + candidates := make([]PlaceSearchCandidate, 0, len(places)) + for _, place := range places { + // The category is always derived server-side from the place's own Types, never trusted + // from place.LocationType (which parseTextSearchResponse also derives the same way, but + // this recomputation keeps the rule in one place regardless of how the place arrived). + primary := POI.PrimaryLocationType(place.Types) + cat, ok := POI.GetPlaceCategory(primary) + + candidates = append(candidates, PlaceSearchCandidate{ + Place: place, + Category: cat, + Insertable: ok, + }) + + if stashErr := s.redisClient.SetPlaceSearchCandidate(ctx, place, PlaceSearchCandidateTTL); stashErr != nil { + Logger.Errorf("TextSearchPlaces: failed to stash search candidate %s: %v", place.ID, stashErr) + } + } + + return candidates, nil +} + +// AddSearchedPlaceToCache confirms a previously text-searched candidate into the shared Redis geo +// cache. See addSearchedPlaceToCache for the step-by-step behavior; this exported form supplies +// the real Google Place Details enricher. +func (s *PoiSearcher) AddSearchedPlaceToCache(ctx context.Context, placeID string) (AddSearchedPlaceResult, error) { + enrich := newPlaceDetailsEnricher(s.mapsClient.apiSemaphore, s.mapsClient.DetailedSearchFields, s.mapsClient.PlaceDetailedSearch) + return s.addSearchedPlaceToCache(ctx, placeID, enrich) +} + +// placeDetailsSearchFunc matches MapsClient.PlaceDetailedSearch's signature. Factored out purely +// so newPlaceDetailsEnricher's context-bounding and semaphore-acquiring behavior can be pinned by +// a test with a stub in place of a real Google call — CreatePoiSearcher always builds a real +// maps.Client even with a fake key, so there is no way to exercise this seam with a real client +// without either a live call or an actual hang. +type placeDetailsSearchFunc func(ctx context.Context, placeID string, fields []string) (maps.PlaceDetailsResult, error) + +// newPlaceDetailsEnricher builds the placeDetailsEnricher used against a real Google client. It +// bounds every call to GoogleMapsSearchTimeout BEFORE acquiring the semaphore: the maps SDK's HTTP +// client has no timeout of its own, and an inbound request context (e.g. a gin request context) +// carries no deadline by default. Without this bound, a single hung Google call would park one of +// the process-wide apiSemaphore slots indefinitely, and a handful of those starves every other +// Google call — including nearby search — service-wide. Every other call site in this package +// bounds itself with GoogleMapsSearchTimeout the same way. +func newPlaceDetailsEnricher(sem chan struct{}, fields []string, search placeDetailsSearchFunc) placeDetailsEnricher { + return func(ctx context.Context, placeID string) (maps.PlaceDetailsResult, error) { + ctx, cancel := context.WithTimeout(ctx, GoogleMapsSearchTimeout) + defer cancel() + + sem <- struct{}{} + defer func() { <-sem }() + return search(ctx, placeID, fields) + } +} + +// addSearchedPlaceToCache does the real work, taking the Place Details lookup as a seam so tests +// can exercise it without a real Google client (see placeDetailsEnricher). +// +// Order of operations: +// 1. Resolve the stashed candidate by place ID; a miss wraps ErrSearchCandidateNotFound. +// 2. Derive the category from the candidate's own primary Google type. An unmapped type refuses +// the whole operation before anything is written — this is the guarantee later tests assert: +// no geo bucket member, no place_details record. +// 3. Read whether the place was already cached, BEFORE this confirm writes anything, so +// AlreadyCached reflects prior state. +// 4. Best-effort enrich via Place Details for hours/address/URL/summary; a failure here is +// logged and the lean (stash-only) record is used instead of failing the confirm. +// 5. Restore any Details-sourced fields (esp. real opening hours) a previously cached record had +// that this pass didn't obtain, so confirming an already-cached place can never regress it to +// placeholder data. +// 6. Tag the place with its true primary type and write it via the audited +// SetPlacesAddGeoLocations, which is what actually buckets it under placeIDs:. +// 7. SetPlacesAddGeoLocations only logs on failure, so read back via CachedPlaces and fail loudly +// if the record did not land — otherwise a Redis failure would look like a success. +func (s *PoiSearcher) addSearchedPlaceToCache(ctx context.Context, placeID string, enrich placeDetailsEnricher) (AddSearchedPlaceResult, error) { + candidate, err := s.redisClient.PlaceSearchCandidate(ctx, placeID) + if err != nil { + return AddSearchedPlaceResult{}, err + } + + primary := POI.PrimaryLocationType(candidate.Types) + cat, ok := POI.GetPlaceCategory(primary) + if !ok { + // NOTHING is written past this point: no CachedPlaces read result is used, no enrich + // call, no SetPlacesAddGeoLocations. The refusal is total. + return AddSearchedPlaceResult{}, fmt.Errorf("%w: %q", ErrUnsupportedPlaceType, primary) + } + + cached, cacheErr := s.redisClient.CachedPlaces(ctx, []string{placeID}) + if cacheErr != nil { + Logger.Debugf("addSearchedPlaceToCache: cached-place lookup failed for %s, proceeding as not-cached: %v", placeID, cacheErr) + } + alreadyCached := len(cached) > 0 + + place := candidate + details, enrichErr := enrich(ctx, placeID) + if enrichErr != nil { + Logger.Errorf("addSearchedPlaceToCache: Place Details enrich failed for %s, continuing with the lean stashed record: %v", placeID, enrichErr) + } else { + foldPlaceDetailsIntoPlace(&place, details) + } + + places := []POI.Place{place} + restoreCachedDetails(places, cached) + place = places[0] + + // restoreCachedDetails (iowrappers/nearby_search.go) restores URL/Summary/FormattedAddress/ + // Address/Hours but deliberately NOT Photo — that helper is shared with the nearby-search + // write path and is not being touched here. Do the same gap-fill locally: a text search + // result commonly omits photos, and without this a lean re-confirm of an already-cached place + // would silently overwrite a real Photo.Reference with the zero value while still reporting + // success. + if place.Photo == (POI.PlacePhoto{}) { + if stored, ok := cached[placeID]; ok { + place.Photo = stored.Photo + } + } + + // Re-tag with the true primary type (never trust candidate.LocationType, which may reflect + // whatever the original search happened to be querying for). + place.LocationType = primary + s.redisClient.SetPlacesAddGeoLocations(ctx, []POI.Place{place}) + + verify, verifyErr := s.redisClient.CachedPlaces(ctx, []string{placeID}) + if verifyErr != nil { + return AddSearchedPlaceResult{}, fmt.Errorf("verifying cache write for place %s: %w", placeID, verifyErr) + } + if _, ok := verify[placeID]; !ok { + return AddSearchedPlaceResult{}, fmt.Errorf("place %s was not persisted to the cache", placeID) + } + + return AddSearchedPlaceResult{Place: place, Category: cat, AlreadyCached: alreadyCached}, nil +} + +// foldPlaceDetailsIntoPlace folds Place Details fields onto an existing place, the same fields +// extensiveNearbySearch folds in for a nearby-search Details call. Details responses carry no +// geometry, so latitude/longitude are left untouched — they come from the stashed text-search +// candidate. +func foldPlaceDetailsIntoPlace(place *POI.Place, details maps.PlaceDetailsResult) { + if details.OpeningHours != nil && len(details.OpeningHours.WeekdayText) == 7 { + for weekday := POI.DateMonday; weekday <= POI.DateSunday; weekday++ { + place.SetHour(weekday, details.OpeningHours.WeekdayText[weekday]) + } + } + if details.FormattedAddress != "" { + place.FormattedAddress = details.FormattedAddress + } + if details.AdrAddress != "" { + place.SetAddress(details.AdrAddress) + } + if details.URL != "" { + place.URL = details.URL + } + if details.EditorialSummary != nil { + place.Summary = details.EditorialSummary.Overview + } + // "photos" is already requested in config/config.yml's detailed_search_fields, so this call + // is already paying for it; fold it in rather than discarding it. Only fills a gap — a photo + // the text search result itself carried (or a previously-cached one, restored afterward by + // addSearchedPlaceToCache) is never replaced by this. + if place.Photo == (POI.PlacePhoto{}) && len(details.Photos) > 0 { + place.SetPhoto(&details.Photos[0]) + } +} diff --git a/iowrappers/text_search_test.go b/iowrappers/text_search_test.go new file mode 100644 index 00000000..67c307eb --- /dev/null +++ b/iowrappers/text_search_test.go @@ -0,0 +1,351 @@ +package iowrappers + +import ( + "encoding/json" + "testing" + + "github.com/weihesdlegend/Vacation-planner/POI" + "googlemaps.github.io/maps" +) + +func mustLogger(t *testing.T) { + t.Helper() + if err := CreateLogger(); err != nil { + t.Fatalf("CreateLogger: %v", err) + } +} + +func TestParseTextSearchResponse_SkipsEmptyPlaceID(t *testing.T) { + mustLogger(t) + resp := maps.PlacesSearchResponse{ + Results: []maps.PlacesSearchResult{ + {PlaceID: "", Name: "No ID", Geometry: maps.AddressGeometry{Location: maps.LatLng{Lat: 1, Lng: 1}}}, + {PlaceID: "has-id", Name: "Has ID", Geometry: maps.AddressGeometry{Location: maps.LatLng{Lat: 1, Lng: 1}}}, + }, + } + + places := parseTextSearchResponse(resp, 0) + if len(places) != 1 { + t.Fatalf("got %d places, want 1", len(places)) + } + if places[0].ID != "has-id" { + t.Errorf("got place %q, want has-id", places[0].ID) + } +} + +func TestParseTextSearchResponse_SkipsZeroGeometry(t *testing.T) { + mustLogger(t) + resp := maps.PlacesSearchResponse{ + Results: []maps.PlacesSearchResult{ + {PlaceID: "zero-geo", Name: "Null Island", Geometry: maps.AddressGeometry{Location: maps.LatLng{Lat: 0, Lng: 0}}}, + {PlaceID: "real-geo", Name: "Real Place", Geometry: maps.AddressGeometry{Location: maps.LatLng{Lat: 12.5, Lng: -70.1}}}, + }, + } + + places := parseTextSearchResponse(resp, 0) + if len(places) != 1 { + t.Fatalf("got %d places, want 1", len(places)) + } + if places[0].ID != "real-geo" { + t.Errorf("got place %q, want real-geo", places[0].ID) + } +} + +func TestParseTextSearchResponse_DedupesByPlaceID(t *testing.T) { + mustLogger(t) + geo := maps.AddressGeometry{Location: maps.LatLng{Lat: 1, Lng: 1}} + resp := maps.PlacesSearchResponse{ + Results: []maps.PlacesSearchResult{ + {PlaceID: "dup-1", Name: "First", Geometry: geo}, + {PlaceID: "dup-1", Name: "Second (duplicate)", Geometry: geo}, + }, + } + + places := parseTextSearchResponse(resp, 0) + if len(places) != 1 { + t.Fatalf("got %d places, want 1 (dedupe by PlaceID failed)", len(places)) + } + if places[0].Name != "First" { + t.Errorf("got name %q, want First (first occurrence should win)", places[0].Name) + } +} + +// TestParseTextSearchResponse_KeepsZeroRatings pins the deliberate divergence from +// parsePlacesSearchResponse (nearby search), which drops UserRatingsTotal == 0. That filter would +// drop exactly the new/obscure places this feature exists to let a user add by name. +func TestParseTextSearchResponse_KeepsZeroRatings(t *testing.T) { + mustLogger(t) + resp := maps.PlacesSearchResponse{ + Results: []maps.PlacesSearchResult{ + { + PlaceID: "zero-ratings", + Name: "Brand New Place", + Geometry: maps.AddressGeometry{Location: maps.LatLng{Lat: 1, Lng: 1}}, + UserRatingsTotal: 0, + }, + }, + } + + places := parseTextSearchResponse(resp, 0) + if len(places) != 1 { + t.Fatalf("got %d places, want 1 (zero-rating place must be kept)", len(places)) + } +} + +// TestParseTextSearchResponse_BlankStatusBecomesOperational pins that an empty business_status +// (common for text search) is treated as Operational rather than as StatusNotAvailable, which +// would make the place invisible under RedisClient.NearbySearch's Operational-only filter. +func TestParseTextSearchResponse_BlankStatusBecomesOperational(t *testing.T) { + mustLogger(t) + resp := maps.PlacesSearchResponse{ + Results: []maps.PlacesSearchResult{ + { + PlaceID: "blank-status", + Name: "No Status Given", + Geometry: maps.AddressGeometry{Location: maps.LatLng{Lat: 1, Lng: 1}}, + BusinessStatus: "", + }, + }, + } + + places := parseTextSearchResponse(resp, 0) + if len(places) != 1 { + t.Fatalf("got %d places, want 1", len(places)) + } + if places[0].Status != POI.Operational { + t.Errorf("got status %q, want %q", places[0].Status, POI.Operational) + } +} + +func TestParseTextSearchResponse_SkipsClosedPermanently(t *testing.T) { + mustLogger(t) + resp := maps.PlacesSearchResponse{ + Results: []maps.PlacesSearchResult{ + { + PlaceID: "closed", + Name: "Shuttered", + Geometry: maps.AddressGeometry{Location: maps.LatLng{Lat: 1, Lng: 1}}, + BusinessStatus: "CLOSED_PERMANENTLY", + }, + { + PlaceID: "open", + Name: "Still Open", + Geometry: maps.AddressGeometry{Location: maps.LatLng{Lat: 1, Lng: 1}}, + BusinessStatus: "OPERATIONAL", + }, + }, + } + + places := parseTextSearchResponse(resp, 0) + if len(places) != 1 { + t.Fatalf("got %d places, want 1", len(places)) + } + if places[0].ID != "open" { + t.Errorf("got place %q, want open", places[0].ID) + } +} + +func TestParseTextSearchResponse_TruncatesToDefaultMax(t *testing.T) { + mustLogger(t) + results := make([]maps.PlacesSearchResult, 0, PlaceTextSearchMaxResults+5) + for i := 0; i < PlaceTextSearchMaxResults+5; i++ { + results = append(results, maps.PlacesSearchResult{ + PlaceID: placeIDFor(i), + Name: "Place", + Geometry: maps.AddressGeometry{Location: maps.LatLng{Lat: 1, Lng: 1}}, + }) + } + resp := maps.PlacesSearchResponse{Results: results} + + places := parseTextSearchResponse(resp, 0) // limit <= 0 -> PlaceTextSearchMaxResults + if len(places) != PlaceTextSearchMaxResults { + t.Fatalf("got %d places, want %d (limit<=0 must fall back to PlaceTextSearchMaxResults)", len(places), PlaceTextSearchMaxResults) + } +} + +func TestParseTextSearchResponse_TruncatesToRequestedLimit(t *testing.T) { + mustLogger(t) + results := make([]maps.PlacesSearchResult, 0, PlaceTextSearchMaxResults) + for i := 0; i < PlaceTextSearchMaxResults; i++ { + results = append(results, maps.PlacesSearchResult{ + PlaceID: placeIDFor(i), + Name: "Place", + Geometry: maps.AddressGeometry{Location: maps.LatLng{Lat: 1, Lng: 1}}, + }) + } + resp := maps.PlacesSearchResponse{Results: results} + + places := parseTextSearchResponse(resp, 5) + if len(places) != 5 { + t.Fatalf("got %d places, want 5", len(places)) + } +} + +func TestParseTextSearchResponse_LimitAboveMaxStillCapsAtMax(t *testing.T) { + mustLogger(t) + results := make([]maps.PlacesSearchResult, 0, PlaceTextSearchMaxResults+5) + for i := 0; i < PlaceTextSearchMaxResults+5; i++ { + results = append(results, maps.PlacesSearchResult{ + PlaceID: placeIDFor(i), + Name: "Place", + Geometry: maps.AddressGeometry{Location: maps.LatLng{Lat: 1, Lng: 1}}, + }) + } + resp := maps.PlacesSearchResponse{Results: results} + + places := parseTextSearchResponse(resp, PlaceTextSearchMaxResults+100) + if len(places) != PlaceTextSearchMaxResults { + t.Fatalf("got %d places, want %d (a limit above the max must still cap at the max)", len(places), PlaceTextSearchMaxResults) + } +} + +// TestParseTextSearchResponse_PreservesTypesAndSetsLocationTypeToPrimary pins that Types is kept +// verbatim and LocationType is set to the primary (first non-umbrella) type, not the raw first +// entry and not left blank. +func TestParseTextSearchResponse_PreservesTypesAndSetsLocationTypeToPrimary(t *testing.T) { + mustLogger(t) + types := []string{"point_of_interest", "museum", "establishment"} + resp := maps.PlacesSearchResponse{ + Results: []maps.PlacesSearchResult{ + { + PlaceID: "typed-place", + Name: "History Museum", + Geometry: maps.AddressGeometry{Location: maps.LatLng{Lat: 1, Lng: 1}}, + Types: types, + }, + }, + } + + places := parseTextSearchResponse(resp, 0) + if len(places) != 1 { + t.Fatalf("got %d places, want 1", len(places)) + } + got := places[0] + if len(got.Types) != len(types) { + t.Fatalf("Types not preserved: got %+v, want %+v", got.Types, types) + } + for i := range types { + if got.Types[i] != types[i] { + t.Fatalf("Types not preserved verbatim: got %+v, want %+v", got.Types, types) + } + } + if got.LocationType != POI.LocationTypeMuseum { + t.Errorf("LocationType = %q, want the primary type %q", got.LocationType, POI.LocationTypeMuseum) + } +} + +// TestParseTextSearchResponse_NoRealOpeningHours pins that a text-search-derived place never has +// real opening hours (no weekday data exists in the Text Search response), which is precisely why +// confirming a candidate buys a Place Details call in AddSearchedPlaceToCache. +func TestParseTextSearchResponse_NoRealOpeningHours(t *testing.T) { + mustLogger(t) + resp := maps.PlacesSearchResponse{ + Results: []maps.PlacesSearchResult{ + { + PlaceID: "no-hours", + Name: "Mystery Hours", + Geometry: maps.AddressGeometry{Location: maps.LatLng{Lat: 1, Lng: 1}}, + }, + }, + } + + places := parseTextSearchResponse(resp, 0) + if len(places) != 1 { + t.Fatalf("got %d places, want 1", len(places)) + } + if places[0].HasRealOpeningHours() { + t.Errorf("HasRealOpeningHours() = true, want false (text search carries no weekday hours)") + } +} + +func placeIDFor(i int) string { + return "place-" + string(rune('a'+i%26)) + string(rune('0'+i/26)) +} + +// TestPlaceSearchCandidate_MarshalsExpectedTopLevelKeys and +// TestAddSearchedPlaceResult_MarshalsExpectedTopLevelKeys pin the wire contract the external +// Convex client depends on: PlaceSearchCandidate/AddSearchedPlaceResult must serialize with +// exactly the documented camelCase top-level keys, and the nested Place object must keep +// POI.Place's own (capitalized, untagged) field names, since POI.Place deliberately carries no +// json tags of its own — tagging it would change every endpoint's wire shape at once, not just +// these two. Purely in-memory json.Marshal/Unmarshal: no HTTP, no network, no Redis. +func TestPlaceSearchCandidate_MarshalsExpectedTopLevelKeys(t *testing.T) { + candidate := PlaceSearchCandidate{ + Place: POI.Place{ID: "p1", Name: "History Museum", LocationType: POI.LocationTypeMuseum, Types: []string{"museum", "point_of_interest"}}, + Category: POI.PlaceCategoryVisit, + Insertable: true, + } + + data, err := json.Marshal(candidate) + if err != nil { + t.Fatalf("json.Marshal(PlaceSearchCandidate): %v", err) + } + + assertTopLevelKeys(t, data, []string{"place", "category", "insertable"}) + assertPlaceKeysPresent(t, data) +} + +func TestAddSearchedPlaceResult_MarshalsExpectedTopLevelKeys(t *testing.T) { + result := AddSearchedPlaceResult{ + Place: POI.Place{ID: "p1", Name: "History Museum", LocationType: POI.LocationTypeMuseum, Types: []string{"museum", "point_of_interest"}}, + Category: POI.PlaceCategoryVisit, + AlreadyCached: false, + } + + data, err := json.Marshal(result) + if err != nil { + t.Fatalf("json.Marshal(AddSearchedPlaceResult): %v", err) + } + + assertTopLevelKeys(t, data, []string{"place", "category", "alreadyCached"}) + assertPlaceKeysPresent(t, data) +} + +// assertTopLevelKeys asserts that data's top-level JSON object has exactly the given key set — +// not a subset, not a superset, so an accidental new/dropped field on either struct fails here. +func assertTopLevelKeys(t *testing.T, data []byte, want []string) { + t.Helper() + var raw map[string]json.RawMessage + if err := json.Unmarshal(data, &raw); err != nil { + t.Fatalf("json.Unmarshal top level: %v", err) + } + if len(raw) != len(want) { + t.Fatalf("got %d top-level keys %v, want %d %v", len(raw), keysOf(raw), len(want), want) + } + for _, k := range want { + if _, ok := raw[k]; !ok { + t.Errorf("missing top-level key %q, got keys %v", k, keysOf(raw)) + } + } +} + +// assertPlaceKeysPresent asserts the nested "place" object carries POI.Place's untagged +// (capitalized) field names, pinning that POI.Place itself stays free of json tags. +func assertPlaceKeysPresent(t *testing.T, data []byte) { + t.Helper() + var raw map[string]json.RawMessage + if err := json.Unmarshal(data, &raw); err != nil { + t.Fatalf("json.Unmarshal top level: %v", err) + } + placeRaw, ok := raw["place"] + if !ok { + t.Fatal(`missing "place" top-level key`) + } + var placeFields map[string]json.RawMessage + if err := json.Unmarshal(placeRaw, &placeFields); err != nil { + t.Fatalf("json.Unmarshal nested place object: %v", err) + } + for _, key := range []string{"ID", "Name", "LocationType", "Types"} { + if _, ok := placeFields[key]; !ok { + t.Errorf("place object missing capitalized key %q (external Convex client wire contract); got keys %v", key, keysOf(placeFields)) + } + } +} + +func keysOf(m map[string]json.RawMessage) []string { + keys := make([]string, 0, len(m)) + for k := range m { + keys = append(keys, k) + } + return keys +} diff --git a/planner/place_search_auth_test.go b/planner/place_search_auth_test.go new file mode 100644 index 00000000..4585e9be --- /dev/null +++ b/planner/place_search_auth_test.go @@ -0,0 +1,189 @@ +package planner + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "net/url" + "strings" + "testing" + + "github.com/gin-gonic/gin" + "github.com/weihesdlegend/Vacation-planner/POI" + "github.com/weihesdlegend/Vacation-planner/iowrappers" + "github.com/weihesdlegend/Vacation-planner/test/redis_client_mocks" +) + +// newPlaceSearchTestPlanner builds a MyPlanner wired to the shared mock Redis, with a real +// PoiSearcher pointed at the same backing miniredis instance (see +// TestReclassifyBucketsMigrationDryRunDefault for the established pattern). No real Google Maps +// calls are exercised by any test in this file: every case either fails auth/validation before the +// handler would call out, or exercises AddSearchedPlaceToCache purely against the stashed-candidate +// Redis path. +func newPlaceSearchTestPlanner(t *testing.T) *MyPlanner { + t.Helper() + redisURL, err := url.Parse("redis://" + redis_client_mocks.RedisMockSvr.Addr()) + if err != nil { + t.Fatalf("failed to parse mock redis URL: %v", err) + } + return &MyPlanner{ + RedisClient: redis_client_mocks.RedisClient, + Solver: Solver{Searcher: iowrappers.CreatePoiSearcher("test-maps-api-key", redisURL)}, + } +} + +func newPlaceSearchTestRouter(p *MyPlanner) *gin.Engine { + router := gin.New() + router.POST("/v1/place-search", p.searchPlacesByText) + router.POST("/v1/place-search/confirm", p.confirmSearchedPlace) + return router +} + +func doPlaceSearchRequest(router *gin.Engine, method, path, authorization, body string) (int, map[string]any) { + req := httptest.NewRequest(method, path, strings.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + if authorization != "" { + req.Header.Set("Authorization", authorization) + } + w := httptest.NewRecorder() + router.ServeHTTP(w, req) + + var respBody map[string]any + // Not every case (e.g. malformed-auth 401s) is guaranteed to emit JSON in every possible gin + // configuration, but every handler path in this file always calls ctx.JSON, so this should + // always succeed; ignore unmarshal errors here and let individual assertions on w.Code/body + // surface any real problem. + _ = json.Unmarshal(w.Body.Bytes(), &respBody) + return w.Code, respBody +} + +// TestPlaceSearchRoutesRequireAuth pins that both routes reject unauthenticated and +// garbage-token requests before doing any work (case 1 and case 2 from the task brief). +func TestPlaceSearchRoutesRequireAuth(t *testing.T) { + gin.SetMode(gin.TestMode) + p := newPlaceSearchTestPlanner(t) + router := newPlaceSearchTestRouter(p) + + routes := []struct { + name string + path string + body string + }{ + {name: "search", path: "/v1/place-search", body: `{"query":"konjoe"}`}, + {name: "confirm", path: "/v1/place-search/confirm", body: `{"placeId":"some-id"}`}, + } + + for _, rt := range routes { + t.Run(rt.name+"/no credentials", func(t *testing.T) { + if code, body := doPlaceSearchRequest(router, http.MethodPost, rt.path, "", rt.body); code != http.StatusUnauthorized { + t.Errorf("expected %d without credentials, got %d (%v)", http.StatusUnauthorized, code, body) + } + }) + t.Run(rt.name+"/garbage token", func(t *testing.T) { + if code, body := doPlaceSearchRequest(router, http.MethodPost, rt.path, "Bearer not-a-real-token", rt.body); code != http.StatusUnauthorized { + t.Errorf("expected %d with an invalid token, got %d (%v)", http.StatusUnauthorized, code, body) + } + }) + } +} + +// TestSearchPlacesByTextValidation exercises the /v1/place-search request validation, all with a +// valid regular-user PAT: binding failures (too-short query, missing query) and the zero-location +// rejection (case 3, 4, 5 from the task brief). The zero-location case doubles as proof the +// handler validates before ever reaching TextSearchPlaces / a Google call. +func TestSearchPlacesByTextValidation(t *testing.T) { + gin.SetMode(gin.TestMode) + p := newPlaceSearchTestPlanner(t) + router := newPlaceSearchTestRouter(p) + token := newRegularPAT(t, "place_search_regular", "place-search-regular-token") + auth := "Bearer " + token + + tests := []struct { + name string + body string + }{ + {name: "query too short", body: `{"query":"x"}`}, + {name: "missing query", body: `{"location":{"latitude":37.0,"longitude":-122.0}}`}, + {name: "zero location", body: `{"query":"konjoe","location":{"latitude":0,"longitude":0}}`}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + code, body := doPlaceSearchRequest(router, http.MethodPost, "/v1/place-search", auth, tt.body) + if code != http.StatusBadRequest { + t.Errorf("expected %d, got %d (%v)", http.StatusBadRequest, code, body) + } + if _, ok := body["error"]; !ok { + t.Errorf("expected an \"error\" field in response body, got %v", body) + } + }) + } +} + +// TestConfirmSearchedPlaceValidation exercises /v1/place-search/confirm: a candidate ID that was +// never stashed (case 6), a missing placeId (case 7), and the 422 unsupported-place-type refusal +// end to end (case 8). All three run past authentication with a valid regular-user PAT. +func TestConfirmSearchedPlaceValidation(t *testing.T) { + gin.SetMode(gin.TestMode) + p := newPlaceSearchTestPlanner(t) + router := newPlaceSearchTestRouter(p) + token := newRegularPAT(t, "place_search_confirm_regular", "place-search-confirm-regular-token") + auth := "Bearer " + token + + t.Run("candidate never stashed", func(t *testing.T) { + code, body := doPlaceSearchRequest(router, http.MethodPost, "/v1/place-search/confirm", auth, + `{"placeId":"nope-never-stashed"}`) + if code != http.StatusNotFound { + t.Fatalf("expected %d, got %d (%v)", http.StatusNotFound, code, body) + } + if body["code"] != "candidate_expired" { + t.Errorf(`expected code "candidate_expired", got %v (full body: %v)`, body["code"], body) + } + if _, ok := body["error"]; !ok { + t.Errorf("expected an \"error\" field in response body, got %v", body) + } + }) + + t.Run("missing placeId", func(t *testing.T) { + code, body := doPlaceSearchRequest(router, http.MethodPost, "/v1/place-search/confirm", auth, `{}`) + if code != http.StatusBadRequest { + t.Errorf("expected %d, got %d (%v)", http.StatusBadRequest, code, body) + } + }) + + t.Run("unsupported place type", func(t *testing.T) { + const placeID = "place-search-confirm-unsupported-type" + // "roofing_contractor" is a real Google Maps place type that maps to no + // POI.PlaceCategory (see POI/categories.go's placeTypeToCategory) and is not an + // umbrella type PrimaryLocationType would skip over, so it is a faithful stand-in + // for "the candidate's primary type is unmapped." + candidate := POI.Place{ + ID: placeID, + Name: "Roofing Co", + Types: []string{"roofing_contractor", "point_of_interest", "establishment"}, + Location: POI.Location{Latitude: 37.0, Longitude: -122.0}, + } + if err := redis_client_mocks.RedisClient.SetPlaceSearchCandidate(redis_client_mocks.RedisContext, candidate, iowrappers.PlaceSearchCandidateTTL); err != nil { + t.Fatalf("failed to stash test candidate: %v", err) + } + t.Cleanup(func() { + _ = redis_client_mocks.RedisClient.RemoveKeys(redis_client_mocks.RedisContext, + []string{iowrappers.PlaceSearchCandidateRedisKeyPrefix + placeID}) + }) + + code, body := doPlaceSearchRequest(router, http.MethodPost, "/v1/place-search/confirm", auth, + `{"placeId":"`+placeID+`"}`) + if code != http.StatusUnprocessableEntity { + t.Fatalf("expected %d, got %d (%v)", http.StatusUnprocessableEntity, code, body) + } + if body["code"] != "unsupported_place_type" { + t.Errorf(`expected code "unsupported_place_type", got %v (full body: %v)`, body["code"], body) + } + if body["placeType"] != "roofing_contractor" { + t.Errorf(`expected placeType "roofing_contractor", got %v (full body: %v)`, body["placeType"], body) + } + if _, ok := body["error"]; !ok { + t.Errorf("expected an \"error\" field in response body, got %v", body) + } + }) +} diff --git a/planner/planner.go b/planner/planner.go index 22a6e70a..c8d1818c 100644 --- a/planner/planner.go +++ b/planner/planner.go @@ -1480,6 +1480,128 @@ func (p *MyPlanner) getNearbyPlacesByCategory(ctx *gin.Context) { ctx.JSON(http.StatusOK, gin.H{"results": results}) } +type placeTextSearchRequest struct { + Query string `json:"query" binding:"required,min=2,max=120"` + Location POI.Location `json:"location"` + Radius uint `json:"radius"` + Limit int `json:"limit"` +} + +// searchPlacesByText runs a free-text Google Places search around a coordinate and returns every +// result as a confirmable candidate. Requires authentication (PAT Bearer header or JWT cookie): +// each request buys a billed Google Text Search call, so the endpoint must not be open. Location +// is required (same zero-location rejection getNearbyPlaces/getNearbyPlacesByCategory apply) +// because an unbiased text query like "konjoe" can resolve to the wrong continent without a +// coordinate to anchor it. +func (p *MyPlanner) searchPlacesByText(ctx *gin.Context) { + _, authenticationErr := p.UserAuthentication(ctx, user.LevelRegular) + if authenticationErr != nil { + ctx.JSON(http.StatusUnauthorized, gin.H{"error": authenticationErr.GetErrorMessage()}) + return + } + + req := &placeTextSearchRequest{} + if err := ctx.ShouldBindJSON(req); err != nil { + ctx.JSON(http.StatusBadRequest, gin.H{"error": err.Error()}) + return + } + if req.Location.Latitude == 0 && req.Location.Longitude == 0 { + ctx.JSON(http.StatusBadRequest, gin.H{"error": "location with latitude and longitude is required"}) + return + } + + radius := req.Radius + if radius == 0 || radius > iowrappers.MaxSearchRadius { + radius = iowrappers.MaxSearchRadius + } + limit := req.Limit + if limit <= 0 { + limit = 10 + } + if limit > iowrappers.PlaceTextSearchMaxResults { + limit = iowrappers.PlaceTextSearchMaxResults + } + + searchContext := context.WithValue(ctx.Request.Context(), iowrappers.ContextRequestIdKey, requestid.Get(ctx)) + + candidates, err := p.Solver.Searcher.TextSearchPlaces(searchContext, &iowrappers.TextSearchRequest{ + Query: req.Query, + Location: req.Location, + Radius: radius, + Limit: limit, + }) + if err != nil { + ctx.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()}) + return + } + + ctx.JSON(http.StatusOK, gin.H{"results": candidates}) +} + +type confirmSearchedPlaceRequest struct { + PlaceId string `json:"placeId" binding:"required"` +} + +// unsupportedPlaceTypeFromError extracts the quoted primary place type from an +// iowrappers.ErrUnsupportedPlaceType error, whose message has the form +// `place type does not map to a supported category: ""` (see +// iowrappers.addSearchedPlaceToCache: `fmt.Errorf("%w: %q", ErrUnsupportedPlaceType, primary)`). +// Falls back to the raw suffix if it is ever not a valid quoted Go string, so a formatting change +// upstream degrades to a slightly-off value here instead of an empty one. +func unsupportedPlaceTypeFromError(err error) string { + msg := err.Error() + idx := strings.LastIndex(msg, ": ") + if idx == -1 { + return "" + } + suffix := msg[idx+2:] + if unquoted, unquoteErr := strconv.Unquote(suffix); unquoteErr == nil { + return unquoted + } + return suffix +} + +// confirmSearchedPlace inserts a previously text-searched candidate (by place ID) into the shared +// place cache. Requires authentication (PAT Bearer header or JWT cookie), matching searchPlacesByText. +func (p *MyPlanner) confirmSearchedPlace(ctx *gin.Context) { + _, authenticationErr := p.UserAuthentication(ctx, user.LevelRegular) + if authenticationErr != nil { + ctx.JSON(http.StatusUnauthorized, gin.H{"error": authenticationErr.GetErrorMessage()}) + return + } + + req := &confirmSearchedPlaceRequest{} + if err := ctx.ShouldBindJSON(req); err != nil { + ctx.JSON(http.StatusBadRequest, gin.H{"error": err.Error()}) + return + } + + searchContext := context.WithValue(ctx.Request.Context(), iowrappers.ContextRequestIdKey, requestid.Get(ctx)) + + result, err := p.Solver.Searcher.AddSearchedPlaceToCache(searchContext, req.PlaceId) + if err != nil { + switch { + case errors.Is(err, iowrappers.ErrSearchCandidateNotFound): + ctx.JSON(http.StatusNotFound, gin.H{"error": err.Error(), "code": "candidate_expired"}) + case errors.Is(err, iowrappers.ErrUnsupportedPlaceType): + ctx.JSON(http.StatusUnprocessableEntity, gin.H{ + "error": err.Error(), + "code": "unsupported_place_type", + "placeType": unsupportedPlaceTypeFromError(err), + }) + default: + ctx.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()}) + } + return + } + + ctx.JSON(http.StatusOK, gin.H{ + "place": result.Place, + "category": result.Category, + "alreadyCached": result.AlreadyCached, + }) +} + func (p *MyPlanner) GetPlaceDetails(ctx *gin.Context) { id := ctx.Param("id") if id == "" { @@ -1729,6 +1851,8 @@ func (p *MyPlanner) SetupRouter(serverPort string) *http.Server { v1.POST("/nearby-cities", p.getNearbyCities) v1.POST("/nearby-places", p.getNearbyPlaces) v1.POST("/nearby-places-by-category", p.getNearbyPlacesByCategory) + v1.POST("/place-search", p.searchPlacesByText) + v1.POST("/place-search/confirm", p.confirmSearchedPlace) v1.POST("/optimal-plan", p.getOptimalPlan) v1.POST("/create-token", p.createNewPAT) v1.DELETE("/revoke-token", p.RevokePAT) diff --git a/test/place_category_test.go b/test/place_category_test.go index 5beb93f5..f765f6fc 100644 --- a/test/place_category_test.go +++ b/test/place_category_test.go @@ -4,11 +4,16 @@ import ( "testing" "github.com/weihesdlegend/Vacation-planner/POI" + "googlemaps.github.io/maps" ) // TestGetPlaceTypesByCategory pins the Google Maps place types each category expands to. func TestGetPlaceTypesByCategory(t *testing.T) { cases := map[POI.PlaceCategory][]POI.LocationType{ + POI.PlaceCategoryVisit: { + POI.LocationTypePark, POI.LocationTypeAmusementPark, + POI.LocationTypeGallery, POI.LocationTypeMuseum, + }, POI.PlaceCategoryEatery: { POI.LocationTypeCafe, POI.LocationTypeRestaurant, POI.LocationTypeBar, POI.LocationTypeBakery, POI.LocationTypeMealTakeaway, @@ -61,12 +66,26 @@ func TestPlaceCategoryRoundTrip(t *testing.T) { // A default-to-Eatery branch made TestPlaceCategoryRoundTrip un-failable, so two // Places-API-(New)-only types were added to GetPlaceTypes(Eatery) and hotels were // written into the eatery geo buckets. +// +// university/airport/real_estate_agency/doctor/casino/train_station/campground/ +// physiotherapist are legal legacy Places API types (maps.ParsePlaceType accepts all of +// them) that this service simply has no use for yet. They pin the refusal list the +// text-search insert endpoint's 422 path depends on: an unmapped-but-legal type must stay +// refused, not get swept in by a future overly-broad edit to placeTypeToCategory. func TestGetPlaceCategoryRejectsUnknownTypes(t *testing.T) { unknown := []POI.LocationType{ POI.LocationType("fast_food_restaurant"), POI.LocationType("food_court"), POI.LocationType("lodging_but_not_really"), POI.LocationType(""), + POI.LocationType("university"), + POI.LocationType("airport"), + POI.LocationType("real_estate_agency"), + POI.LocationType("doctor"), + POI.LocationType("casino"), + POI.LocationType("train_station"), + POI.LocationType("campground"), + POI.LocationType("physiotherapist"), } for _, placeType := range unknown { if got, ok := POI.GetPlaceCategory(placeType); ok { @@ -75,7 +94,8 @@ func TestGetPlaceCategoryRejectsUnknownTypes(t *testing.T) { } } -// TestGetPlaceCategoryKnownTypes pins that every mapped type still resolves. +// TestGetPlaceCategoryKnownTypes pins that every mapped type still resolves, including the +// 25 primary-type entries Task 1 added on top of the original 18 searched types. func TestGetPlaceCategoryKnownTypes(t *testing.T) { cases := map[POI.LocationType]POI.PlaceCategory{ POI.LocationTypeCafe: POI.PlaceCategoryEatery, @@ -88,6 +108,39 @@ func TestGetPlaceCategoryKnownTypes(t *testing.T) { POI.LocationTypeStore: POI.PlaceCategoryShopping, POI.LocationTypeLodging: POI.PlaceCategoryLodging, POI.LocationTypeGym: POI.PlaceCategoryWellness, + + // New entries: Eatery + POI.LocationTypeMealDelivery: POI.PlaceCategoryEatery, + POI.LocationTypeNightClub: POI.PlaceCategoryEatery, + + // New entries: Visit + POI.LocationTypeTouristAttraction: POI.PlaceCategoryVisit, + POI.LocationTypeZoo: POI.PlaceCategoryVisit, + POI.LocationTypeAquarium: POI.PlaceCategoryVisit, + POI.LocationTypeMovieTheater: POI.PlaceCategoryVisit, + POI.LocationTypeStadium: POI.PlaceCategoryVisit, + POI.LocationTypeBowlingAlley: POI.PlaceCategoryVisit, + + // New entries: Shopping + POI.LocationTypeGroceryOrSupermarket: POI.PlaceCategoryShopping, + POI.LocationTypeConvenienceStore: POI.PlaceCategoryShopping, + POI.LocationTypeHardwareStore: POI.PlaceCategoryShopping, + POI.LocationTypeHomeGoodsStore: POI.PlaceCategoryShopping, + POI.LocationTypeElectronicsStore: POI.PlaceCategoryShopping, + POI.LocationTypeFurnitureStore: POI.PlaceCategoryShopping, + POI.LocationTypeBookStore: POI.PlaceCategoryShopping, + POI.LocationTypeShoeStore: POI.PlaceCategoryShopping, + POI.LocationTypeJewelryStore: POI.PlaceCategoryShopping, + POI.LocationTypePetStore: POI.PlaceCategoryShopping, + POI.LocationTypeBicycleStore: POI.PlaceCategoryShopping, + POI.LocationTypeFlorist: POI.PlaceCategoryShopping, + POI.LocationTypeLiquorStore: POI.PlaceCategoryShopping, + POI.LocationTypeGasStation: POI.PlaceCategoryShopping, + + // New entries: Wellness + POI.LocationTypeDrugstore: POI.PlaceCategoryWellness, + POI.LocationTypeBeautySalon: POI.PlaceCategoryWellness, + POI.LocationTypeHairCare: POI.PlaceCategoryWellness, } for placeType, want := range cases { got, ok := POI.GetPlaceCategory(placeType) @@ -101,6 +154,46 @@ func TestGetPlaceCategoryKnownTypes(t *testing.T) { } } +// TestGetPlaceCategoryKeysAreGoogleTypes is the structural guard against another +// fast_food_restaurant-style typo entering placeTypeToCategory: every mapped key must be a +// legal legacy Places API type per the SDK's own ParsePlaceType, except the documented +// types[]-only allowlist. +func TestGetPlaceCategoryKeysAreGoogleTypes(t *testing.T) { + // grocery_or_supermarket appears in a place's Types[] but is not a legal ?type= search + // value (maps.ParsePlaceType rejects it) — see the constant's comment in POI/categories.go. + allowlist := map[POI.LocationType]bool{ + POI.LocationTypeGroceryOrSupermarket: true, + } + for _, lt := range POI.MappedLocationTypes() { + if allowlist[lt] { + continue + } + if _, err := maps.ParsePlaceType(string(lt)); err != nil { + t.Errorf("MappedLocationTypes() contains %q, which is not a legal Places API type: %v", lt, err) + } + } +} + +// TestGetPlaceTypesSubsetOfCategoryMap makes the superset relation explicit: every type a +// category actively searches for (GetPlaceTypes) must map back to that category via the +// broader placeTypeToCategory map (GetPlaceCategory). This strengthens TestPlaceCategoryRoundTrip +// by pinning the relationship the docstrings now describe — placeTypeToCategory is a superset +// of GetPlaceTypes' inverse, not an exact inverse. +func TestGetPlaceTypesSubsetOfCategoryMap(t *testing.T) { + for _, cat := range POI.AllPlaceCategories { + for _, placeType := range POI.GetPlaceTypes(cat) { + got, ok := POI.GetPlaceCategory(placeType) + if !ok { + t.Errorf("category %s: searched type %q has no entry in GetPlaceCategory", cat, placeType) + continue + } + if got != cat { + t.Errorf("category %s: searched type %q maps to %s via GetPlaceCategory", cat, placeType, got) + } + } + } +} + func TestParsePlaceCategory(t *testing.T) { valid := []string{"Visit", "Eatery", "Shopping", "Lodging", "Wellness"} for _, s := range valid { @@ -263,4 +356,87 @@ func TestReclassifyForCategory(t *testing.T) { t.Errorf("LocationType = %q, want restaurant (unchanged)", rp.LocationType) } }) + + t.Run("meal_delivery is kept in Eatery and re-tagged", func(t *testing.T) { + p := POI.Place{LocationType: POI.LocationTypeRestaurant, Types: []string{"meal_delivery", "restaurant", "food"}} + rp, keep := POI.ReclassifyForCategory(p, POI.PlaceCategoryEatery) + if !keep { + t.Fatal("expected a meal_delivery place to be kept in Eatery") + } + if rp.LocationType != POI.LocationType("meal_delivery") { + t.Errorf("LocationType = %q, want meal_delivery", rp.LocationType) + } + }) + + t.Run("night_club is kept in Eatery", func(t *testing.T) { + p := POI.Place{LocationType: POI.LocationTypeBar, Types: []string{"night_club", "bar", "point_of_interest"}} + if _, keep := POI.ReclassifyForCategory(p, POI.PlaceCategoryEatery); !keep { + t.Error("expected a night_club place to be kept in Eatery") + } + }) + + t.Run("convenience_store is kept in Shopping", func(t *testing.T) { + p := POI.Place{LocationType: POI.LocationTypeStore, Types: []string{"convenience_store", "store", "food"}} + if _, keep := POI.ReclassifyForCategory(p, POI.PlaceCategoryShopping); !keep { + t.Error("expected a convenience_store place to be kept in Shopping") + } + }) + + t.Run("grocery_or_supermarket is kept in Shopping", func(t *testing.T) { + p := POI.Place{LocationType: POI.LocationTypeSupermarket, Types: []string{"grocery_or_supermarket", "food", "store"}} + if _, keep := POI.ReclassifyForCategory(p, POI.PlaceCategoryShopping); !keep { + t.Error("expected a grocery_or_supermarket place to be kept in Shopping") + } + }) + + t.Run("drugstore is kept in Wellness", func(t *testing.T) { + p := POI.Place{LocationType: POI.LocationTypePharmacy, Types: []string{"drugstore", "point_of_interest"}} + if _, keep := POI.ReclassifyForCategory(p, POI.PlaceCategoryWellness); !keep { + t.Error("expected a drugstore place to be kept in Wellness") + } + }) + + t.Run("tourist_attraction is kept in Visit", func(t *testing.T) { + p := POI.Place{LocationType: POI.LocationTypePark, Types: []string{"tourist_attraction", "point_of_interest"}} + if _, keep := POI.ReclassifyForCategory(p, POI.PlaceCategoryVisit); !keep { + t.Error("expected a tourist_attraction place to be kept in Visit") + } + }) + + t.Run("university is dropped from Eatery", func(t *testing.T) { + p := POI.Place{LocationType: POI.LocationTypeRestaurant, Types: []string{"university", "point_of_interest", "establishment"}} + if _, keep := POI.ReclassifyForCategory(p, POI.PlaceCategoryEatery); keep { + t.Error("expected a university to be dropped from the Eatery category") + } + }) + + t.Run("movie_theater is dropped from Eatery and kept in Visit", func(t *testing.T) { + p := POI.Place{LocationType: POI.LocationTypeRestaurant, Types: []string{"movie_theater", "point_of_interest", "establishment"}} + if _, keep := POI.ReclassifyForCategory(p, POI.PlaceCategoryEatery); keep { + t.Error("expected a movie_theater to be dropped from the Eatery category") + } + rp, keep := POI.ReclassifyForCategory(p, POI.PlaceCategoryVisit) + if !keep { + t.Fatal("expected a movie_theater to be kept in Visit") + } + if rp.LocationType != POI.LocationType("movie_theater") { + t.Errorf("LocationType = %q, want movie_theater", rp.LocationType) + } + }) +} + +// TestReclassifyForCategoryKeepsAllFormerlySearchedTypes is the monotonicity guarantee the +// task brief requires: because placeTypeToCategory is a superset of GetPlaceTypes' inverse, +// widening it must never cause ReclassifyForCategory to drop a place that the OLD +// primary-in-GetPlaceTypes(cat) rule would have kept. For every category and every type it +// actively searches for, a place whose primary type is that search type must still be kept. +func TestReclassifyForCategoryKeepsAllFormerlySearchedTypes(t *testing.T) { + for _, cat := range POI.AllPlaceCategories { + for _, placeType := range POI.GetPlaceTypes(cat) { + p := POI.Place{LocationType: placeType, Types: []string{string(placeType)}} + if _, keep := POI.ReclassifyForCategory(p, cat); !keep { + t.Errorf("category %s: a place searched-and-primary-typed %q must be kept, was dropped", cat, placeType) + } + } + } } diff --git a/test/redis_client_mocks/bucket_cleanup_test.go b/test/redis_client_mocks/bucket_cleanup_test.go index bc522fda..8d91b99c 100644 --- a/test/redis_client_mocks/bucket_cleanup_test.go +++ b/test/redis_client_mocks/bucket_cleanup_test.go @@ -17,6 +17,7 @@ var bucketCleanupFixtureIDs = []string{ "hotel-1", "cafe-1", "hotel-2", "cafe-2", "legacy-1", "tt-lodging", "tt-supermarket", "tt-meal-delivery", "tt-night-club", "tt-no-types", "tt-cafe", "tt-orphan", + "tt-movie-theater", "tt-stadium", "tt-hardware-store", "tt-university", } // resetBucketCleanupFixtures gives each test in this file a clean slate for its own fixture @@ -115,11 +116,14 @@ func TestRemoveMisclassifiedPlacesKeepsUntypedRecords(t *testing.T) { // TestRemoveMisclassifiedPlacesPrimaryTypeTruthTable pins the exact rule the cleanup applies: // a bucket member is removed only when its PRIMARY Google type positively maps to a DIFFERENT -// category. An unmapped primary type is NOT evidence of misclassification — the fixed write -// path keys on the stamped LocationType, so it would legitimately file a delivery-first -// restaurant ("meal_delivery" first in Types) or a bar/club ("night_club" first) under Eatery. -// Purging those would make the migration delete rows the write path immediately re-creates, -// while shrinking the trip-planning candidate pool for up to MinMapsResultRefreshDuration. +// category. Two distinct reasons keep a member in place, and this table covers both: (1) the +// primary type positively maps to the SAME category — since Task 1 expanded placeTypeToCategory, +// this now includes a delivery-first restaurant ("meal_delivery" first in Types) and a bar/club +// ("night_club" first), both of which resolve straight to Eatery; (2) the primary type is still +// unmapped residue (e.g. "university"), which is NOT evidence of misclassification because the +// write path refuses to file an unmapped type anywhere in the first place. Purging either case +// would make the migration delete rows the write path would legitimately re-create, while +// shrinking the trip-planning candidate pool for up to MinMapsResultRefreshDuration. func TestRemoveMisclassifiedPlacesPrimaryTypeTruthTable(t *testing.T) { resetBucketCleanupFixtures(t) t.Cleanup(func() { resetBucketCleanupFixtures(t) }) @@ -151,15 +155,48 @@ func TestRemoveMisclassifiedPlacesPrimaryTypeTruthTable(t *testing.T) { id: "tt-meal-delivery", name: "Wok This Way Delivery", stamped: POI.LocationTypeRestaurant, types: []string{"meal_delivery", "restaurant", "food", "point_of_interest"}, - // GetPlaceCategory("meal_delivery") == ("", false): legal legacy type, unmapped - wantRemove: false, why: "primary type meal_delivery maps to no category", + // GetPlaceCategory("meal_delivery") == (Eatery, true) == cat: kept because it + // positively matches the bucket's category, not because it's unmapped (Task 1 + // added meal_delivery as an Eatery entry in placeTypeToCategory). + wantRemove: false, why: "primary type meal_delivery maps to Eatery, matches cat", }, { id: "tt-night-club", name: "The Basement", stamped: POI.LocationTypeBar, types: []string{"night_club", "bar", "point_of_interest", "establishment"}, - // GetPlaceCategory("night_club") == ("", false): legal legacy type, unmapped - wantRemove: false, why: "primary type night_club maps to no category", + // GetPlaceCategory("night_club") == (Eatery, true) == cat: kept because it + // positively matches the bucket's category, not because it's unmapped (Task 1 + // added night_club as an Eatery entry in placeTypeToCategory). + wantRemove: false, why: "primary type night_club maps to Eatery, matches cat", + }, + { + id: "tt-movie-theater", name: "Downtown Cineplex", + stamped: POI.LocationTypeRestaurant, + types: []string{"movie_theater", "point_of_interest", "establishment"}, + // GetPlaceCategory("movie_theater") == (Visit, true) != Eatery (Task 1 entry) + wantRemove: true, why: "primary type movie_theater maps to Visit", + }, + { + id: "tt-stadium", name: "Civic Arena", + stamped: POI.LocationTypeRestaurant, + types: []string{"stadium", "point_of_interest", "establishment"}, + // GetPlaceCategory("stadium") == (Visit, true) != Eatery (Task 1 entry) + wantRemove: true, why: "primary type stadium maps to Visit", + }, + { + id: "tt-hardware-store", name: "Ace Hardware", + stamped: POI.LocationTypeRestaurant, + types: []string{"hardware_store", "point_of_interest", "establishment"}, + // GetPlaceCategory("hardware_store") == (Shopping, true) != Eatery (Task 1 entry) + wantRemove: true, why: "primary type hardware_store maps to Shopping", + }, + { + id: "tt-university", name: "State University Dining Hall", + stamped: POI.LocationTypeRestaurant, + types: []string{"university", "point_of_interest", "establishment"}, + // GetPlaceCategory("university") == ("", false): legal legacy type, still + // unmapped after Task 1 — kept as unmapped residue, same as before. + wantRemove: false, why: "primary type university maps to no category (still unmapped residue)", }, { id: "tt-no-types", name: "Old Cached Diner", diff --git a/test/redis_client_mocks/place_search_candidate_test.go b/test/redis_client_mocks/place_search_candidate_test.go new file mode 100644 index 00000000..be863df7 --- /dev/null +++ b/test/redis_client_mocks/place_search_candidate_test.go @@ -0,0 +1,145 @@ +package redis_client_mocks + +import ( + "errors" + "testing" + "time" + + "github.com/weihesdlegend/Vacation-planner/POI" + "github.com/weihesdlegend/Vacation-planner/iowrappers" +) + +// placeSearchCandidateFixtureIDs lists every place ID this file writes, so cleanup can remove +// exactly these keys rather than touching the shared RedisClient/RedisMockSvr fixtures other test +// files in this package depend on (see bucket_cleanup_test.go's resetBucketCleanupFixtures for the +// established pattern). NEVER FlushAll here. +var placeSearchCandidateFixtureIDs = []string{ + "psc-round-trip-1", "psc-ttl-1", "psc-expire-1", "psc-collision-1", +} + +func cleanupPlaceSearchCandidateFixtures(t *testing.T) { + t.Helper() + keys := make([]string, 0, len(placeSearchCandidateFixtureIDs)*2) + for _, id := range placeSearchCandidateFixtureIDs { + keys = append(keys, + iowrappers.PlaceSearchCandidateRedisKeyPrefix+id, + iowrappers.PlaceDetailsRedisKeyPrefix+id, + ) + } + if err := RedisClient.RemoveKeys(RedisContext, keys); err != nil { + t.Fatalf("RemoveKeys: %v", err) + } +} + +func TestPlaceSearchCandidate_RoundTripPreservesTypes(t *testing.T) { + cleanupPlaceSearchCandidateFixtures(t) + t.Cleanup(func() { cleanupPlaceSearchCandidateFixtures(t) }) + + place := POI.Place{ + ID: "psc-round-trip-1", + Name: "Test Museum", + LocationType: POI.LocationTypeMuseum, + Types: []string{"museum", "point_of_interest", "establishment"}, + Location: POI.Location{Latitude: 1.23, Longitude: 4.56}, + } + + if err := RedisClient.SetPlaceSearchCandidate(RedisContext, place, iowrappers.PlaceSearchCandidateTTL); err != nil { + t.Fatalf("SetPlaceSearchCandidate: %v", err) + } + + got, err := RedisClient.PlaceSearchCandidate(RedisContext, place.ID) + if err != nil { + t.Fatalf("PlaceSearchCandidate: %v", err) + } + if got.ID != place.ID || got.Name != place.Name { + t.Errorf("round trip mismatch: got %+v, want ID/Name from %+v", got, place) + } + if len(got.Types) != len(place.Types) { + t.Fatalf("Types not preserved: got %+v, want %+v", got.Types, place.Types) + } + for i := range place.Types { + if got.Types[i] != place.Types[i] { + t.Errorf("Types[%d] = %q, want %q", i, got.Types[i], place.Types[i]) + } + } +} + +func TestPlaceSearchCandidate_TTLIsSet(t *testing.T) { + cleanupPlaceSearchCandidateFixtures(t) + t.Cleanup(func() { cleanupPlaceSearchCandidateFixtures(t) }) + + place := POI.Place{ID: "psc-ttl-1", Name: "TTL Test"} + if err := RedisClient.SetPlaceSearchCandidate(RedisContext, place, iowrappers.PlaceSearchCandidateTTL); err != nil { + t.Fatalf("SetPlaceSearchCandidate: %v", err) + } + + ttl := RedisClient.Get().TTL(RedisContext, iowrappers.PlaceSearchCandidateRedisKeyPrefix+place.ID).Val() + if ttl <= 0 || ttl > iowrappers.PlaceSearchCandidateTTL { + t.Errorf("TTL = %v, want in (0, %v]", ttl, iowrappers.PlaceSearchCandidateTTL) + } +} + +func TestPlaceSearchCandidate_ExpiresAfterTTL(t *testing.T) { + cleanupPlaceSearchCandidateFixtures(t) + t.Cleanup(func() { cleanupPlaceSearchCandidateFixtures(t) }) + + place := POI.Place{ID: "psc-expire-1", Name: "Expiring Candidate"} + if err := RedisClient.SetPlaceSearchCandidate(RedisContext, place, iowrappers.PlaceSearchCandidateTTL); err != nil { + t.Fatalf("SetPlaceSearchCandidate: %v", err) + } + + // Advance the mock server's virtual clock past the 30-minute TTL. This is scoped to keys this + // test owns; no other fixture in this package sets a TTL under 31 minutes (checked at the time + // this test was written), so this does not prematurely expire unrelated tests' data. + RedisMockSvr.FastForward(31 * time.Minute) + + _, err := RedisClient.PlaceSearchCandidate(RedisContext, place.ID) + if !errors.Is(err, iowrappers.ErrSearchCandidateNotFound) { + t.Errorf("PlaceSearchCandidate after expiry: got err %v, want it to wrap ErrSearchCandidateNotFound", err) + } +} + +func TestPlaceSearchCandidate_MissReturnsNotFoundSentinel(t *testing.T) { + _, err := RedisClient.PlaceSearchCandidate(RedisContext, "psc-never-searched-at-all") + if !errors.Is(err, iowrappers.ErrSearchCandidateNotFound) { + t.Errorf("got err %v, want it to wrap ErrSearchCandidateNotFound", err) + } +} + +// TestPlaceSearchCandidate_PrefixDoesNotCollideWithPlaceDetails pins that the candidate stash key +// space (place_search_candidate:place_ID:) is distinct from the permanent place record key +// space (place_details:place_ID:) for the same place ID: writing one must not be readable +// through the other. +func TestPlaceSearchCandidate_PrefixDoesNotCollideWithPlaceDetails(t *testing.T) { + cleanupPlaceSearchCandidateFixtures(t) + t.Cleanup(func() { cleanupPlaceSearchCandidateFixtures(t) }) + + id := "psc-collision-1" + candidate := POI.Place{ID: id, Name: "Collision Candidate"} + details := POI.Place{ID: id, Name: "Collision Details", LocationType: POI.LocationTypeMuseum} + + if err := RedisClient.SetPlaceSearchCandidate(RedisContext, candidate, iowrappers.PlaceSearchCandidateTTL); err != nil { + t.Fatalf("SetPlaceSearchCandidate: %v", err) + } + if err := RedisClient.SetPlace(RedisContext, details); err != nil { + t.Fatalf("SetPlace: %v", err) + } + + gotCandidate, err := RedisClient.PlaceSearchCandidate(RedisContext, id) + if err != nil { + t.Fatalf("PlaceSearchCandidate: %v", err) + } + if gotCandidate.Name != "Collision Candidate" { + t.Errorf("PlaceSearchCandidate returned %q, want %q (key prefix collision with place_details?)", + gotCandidate.Name, "Collision Candidate") + } + + gotDetails, err := RedisClient.CachedPlaces(RedisContext, []string{id}) + if err != nil { + t.Fatalf("CachedPlaces: %v", err) + } + if gotDetails[id].Name != "Collision Details" { + t.Errorf("CachedPlaces returned %q, want %q (key prefix collision with place_search_candidate?)", + gotDetails[id].Name, "Collision Details") + } +}