From 9b6d167430bcf1587ff236548c9c6c0c29bc2666 Mon Sep 17 00:00:00 2001 From: tim-eternos Date: Thu, 30 Jul 2026 16:59:39 -0700 Subject: [PATCH 1/8] Expand place-type reverse map and unify ReclassifyForCategory on it GetPlaceCategory was a switch covering only the 18 types the categories actively search for. Expand it into a package-level placeTypeToCategory map with 25 more entries so Google primary types (tourist_attraction, grocery_or_supermarket, drugstore, movie_theater, etc.) classify correctly, while keeping the no-default-category invariant intact (GetPlaceCategory("") still returns ("", false)) and leaving GetPlaceTypes - the searched subset - untouched. Unify ReclassifyForCategory onto the same map: it now keeps a place when GetPlaceCategory(primary) == cat instead of scanning GetPlaceTypes(cat), giving one classification rule shared by the nearby-search write path, the bucket-cleanup migration, and the merchant-endpoint read filter. Because the map is a strict superset of the old searched-types union, this is provably monotonic: it can only keep more places than before, never fewer (pinned by TestReclassifyForCategoryKeepsAllFormerlySearchedTypes). Extend test/place_category_test.go with the 25 new known-type cases, 8 new unmapped-type refusals used by the future text-search 422 path, a structural guard that every mapped key is a real Places API type (TestGetPlaceCategoryKeysAreGoogleTypes), and the searched-subset round-trip test. Update test/redis_client_mocks/bucket_cleanup_test.go: meal_delivery/night_club rows now keep their verdict for the correct reason (positively mapped to Eatery, not unmapped), plus four new truth-table rows covering types that only just became mapped. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01DjdGnZM6cUBeg6jWwoQfU3 --- POI/categories.go | 206 ++++++++++++++---- test/place_category_test.go | 174 ++++++++++++++- .../redis_client_mocks/bucket_cleanup_test.go | 55 ++++- 3 files changed, 383 insertions(+), 52 deletions(-) diff --git a/POI/categories.go b/POI/categories.go index be8364ea..ca638299 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,33 @@ 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. +// - primary type is unmapped, or there is no Types → keep unchanged (older cached records, +// or a legal-but-uninteresting type), so coverage never regresses on data written before +// Types was captured. +// +// 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/test/place_category_test.go b/test/place_category_test.go index 5beb93f5..b8979277 100644 --- a/test/place_category_test.go +++ b/test/place_category_test.go @@ -4,6 +4,7 @@ import ( "testing" "github.com/weihesdlegend/Vacation-planner/POI" + "googlemaps.github.io/maps" ) // TestGetPlaceTypesByCategory pins the Google Maps place types each category expands to. @@ -61,12 +62,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 +90,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 +104,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 +150,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 +352,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", From 2a2ecdb063d0ade74d6fdba6cb7b566c4272f90a Mon Sep 17 00:00:00 2001 From: tim-eternos Date: Thu, 30 Jul 2026 17:16:42 -0700 Subject: [PATCH 2/8] Add text search client, candidate stash, and confirm-insert flow Builds the iowrappers-layer machinery for free-text place search: Google Text Search -> parsed POI.Place candidates -> stashed server-side by place ID with a 30-minute TTL -> user-confirmed insert into the shared Redis geo cache. No HTTP wiring; that is a later task against PoiSearcher.TextSearchPlaces and PoiSearcher.AddSearchedPlaceToCache. - MapsClient.TextSearchPlaces sets only Query/Location/Radius on the Google request (no Type, no OpenNow) since an unenforceable Type filter is what previously let hotels get cached as eateries. - parseTextSearchResponse (pure) skips empty PlaceID, {0,0} geometry, CLOSED_PERMANENTLY, and dedupes by PlaceID, but deliberately keeps zero-rating results (unlike parsePlacesSearchResponse) since those are exactly the new/obscure places this feature exists for. A blank business status is treated as Operational rather than invisible. - PoiSearcher.AddSearchedPlaceToCache refuses entirely (no write at all) when the candidate's primary Google type has no POI.PlaceCategory mapping, and otherwise best-effort enriches via Place Details, restores any previously-cached real opening hours, writes through SetPlacesAddGeoLocations, and reads back via CachedPlaces to fail loudly on a silent Redis write failure. - RedisClient gains SetPlaceSearchCandidate/PlaceSearchCandidate stash methods beside setPlace/getPlace, under their own key prefix. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01DjdGnZM6cUBeg6jWwoQfU3 --- iowrappers/add_searched_place_test.go | 389 ++++++++++++++++++ iowrappers/redis_client.go | 32 ++ iowrappers/text_search.go | 307 ++++++++++++++ iowrappers/text_search_test.go | 262 ++++++++++++ .../place_search_candidate_test.go | 145 +++++++ 5 files changed, 1135 insertions(+) create mode 100644 iowrappers/add_searched_place_test.go create mode 100644 iowrappers/text_search.go create mode 100644 iowrappers/text_search_test.go create mode 100644 test/redis_client_mocks/place_search_candidate_test.go diff --git a/iowrappers/add_searched_place_test.go b/iowrappers/add_searched_place_test.go new file mode 100644 index 00000000..a73f945d --- /dev/null +++ b/iowrappers/add_searched_place_test.go @@ -0,0 +1,389 @@ +package iowrappers + +import ( + "context" + "errors" + "net/url" + "testing" + + "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) + } +} 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..faef3f06 --- /dev/null +++ b/iowrappers/text_search.go @@ -0,0 +1,307 @@ +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 + Category POI.PlaceCategory + AlreadyCached bool +} + +// 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 := func(ctx context.Context, placeID string) (maps.PlaceDetailsResult, error) { + s.mapsClient.apiSemaphore <- struct{}{} + defer func() { <-s.mapsClient.apiSemaphore }() + return s.mapsClient.PlaceDetailedSearch(ctx, placeID, s.mapsClient.DetailedSearchFields) + } + return s.addSearchedPlaceToCache(ctx, placeID, enrich) +} + +// 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] + + // 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 + } +} diff --git a/iowrappers/text_search_test.go b/iowrappers/text_search_test.go new file mode 100644 index 00000000..33c909dc --- /dev/null +++ b/iowrappers/text_search_test.go @@ -0,0 +1,262 @@ +package iowrappers + +import ( + "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)) +} 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") + } +} From a23d808cb8344cd775e6e7bb3a539f910b14092c Mon Sep 17 00:00:00 2001 From: tim-eternos Date: Thu, 30 Jul 2026 17:31:30 -0700 Subject: [PATCH 3/8] Fix Task-2 review findings: photo clobber and unbounded Details call Two Important findings from Task 2 review, both local to text_search.go: 1. A lean re-confirm of an already-cached place could silently overwrite a real Photo.Reference with the zero value: restoreCachedDetails (shared with the nearby-search write path, left untouched) restores URL/Summary/ FormattedAddress/Address/Hours but not Photo. Gap-fill the photo locally from the cached record after restoreCachedDetails runs. Also fold a Details-sourced photo into the place when the candidate had none - "photos" is already a requested detailed_search_fields entry, so the confirm path was paying for it and throwing it away. 2. The Place Details enrich closure passed the caller's raw context through to the Google call. The maps SDK's HTTP client has no timeout of its own and an inbound request context carries no deadline by default, so a hung call could park one of the 5 shared apiSemaphore slots indefinitely, starving every other Google call process-wide. Extracted the closure into newPlaceDetailsEnricher (bounded by GoogleMapsSearchTimeout before acquiring the semaphore) so the fix is independently testable with a stub search function instead of a hang harness or a real Google client. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01DjdGnZM6cUBeg6jWwoQfU3 --- iowrappers/add_searched_place_test.go | 141 ++++++++++++++++++++++++++ iowrappers/text_search.go | 50 ++++++++- 2 files changed, 186 insertions(+), 5 deletions(-) diff --git a/iowrappers/add_searched_place_test.go b/iowrappers/add_searched_place_test.go index a73f945d..f5fbf8e8 100644 --- a/iowrappers/add_searched_place_test.go +++ b/iowrappers/add_searched_place_test.go @@ -5,6 +5,7 @@ import ( "errors" "net/url" "testing" + "time" "github.com/alicebob/miniredis/v2" "github.com/weihesdlegend/Vacation-planner/POI" @@ -387,3 +388,143 @@ func TestAddSearchedPlaceToCache_EnricherSuccessFoldsDetails(t *testing.T) { 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/text_search.go b/iowrappers/text_search.go index faef3f06..93b850fc 100644 --- a/iowrappers/text_search.go +++ b/iowrappers/text_search.go @@ -207,14 +207,35 @@ func (s *PoiSearcher) TextSearchPlaces(ctx context.Context, req *TextSearchReque // 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 := func(ctx context.Context, placeID string) (maps.PlaceDetailsResult, error) { - s.mapsClient.apiSemaphore <- struct{}{} - defer func() { <-s.mapsClient.apiSemaphore }() - return s.mapsClient.PlaceDetailedSearch(ctx, placeID, s.mapsClient.DetailedSearchFields) - } + 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). // @@ -266,6 +287,18 @@ func (s *PoiSearcher) addSearchedPlaceToCache(ctx context.Context, placeID strin 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 @@ -304,4 +337,11 @@ func foldPlaceDetailsIntoPlace(place *POI.Place, details maps.PlaceDetailsResult 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]) + } } From 7777028cb44f6f881077a56c4351534f030013ca Mon Sep 17 00:00:00 2001 From: tim-eternos Date: Thu, 30 Jul 2026 17:42:32 -0700 Subject: [PATCH 4/8] Add HTTP handlers and routes for place text-search Wires Task 2's iowrappers text-search machinery to two authenticated endpoints: POST /v1/place-search (free-text Google search, returns the {"results": [...]} envelope an external Convex client expects) and POST /v1/place-search/confirm (inserts a candidate into the shared cache, returns a single {"place","category","alreadyCached"} object). Both handlers mirror getNearbyPlaces/getNearbyPlacesByCategory's style exactly: auth first, ShouldBindJSON -> 400, the same zero-location rejection (proven by test to short-circuit before any Google call), and the same searchContext construction. confirmSearchedPlace maps the two iowrappers sentinel errors to 404 candidate_expired and 422 unsupported_place_type (extracting the quoted primary type via strconv.Unquote, the exact inverse of the %q the sentinel was built with). Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01DjdGnZM6cUBeg6jWwoQfU3 --- planner/place_search_auth_test.go | 189 ++++++++++++++++++++++++++++++ planner/planner.go | 123 +++++++++++++++++++ 2 files changed, 312 insertions(+) create mode 100644 planner/place_search_auth_test.go 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..50fb5f2c 100644 --- a/planner/planner.go +++ b/planner/planner.go @@ -1480,6 +1480,127 @@ 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 (not optional, unlike nearby search's default) 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 > 20 { + limit = 20 + } + + 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 +1850,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) From d6b71da41678b78a19ee47c7a9539a7bf947a614 Mon Sep 17 00:00:00 2001 From: tim-eternos Date: Thu, 30 Jul 2026 17:56:04 -0700 Subject: [PATCH 5/8] Document place-search endpoints and fix stale reclassify docs Add README coverage for the new POST /v1/place-search and /v1/place-search/confirm endpoints (request/response shapes, error codes, auth, and the server-side-category safety design). Update docs/migrations/reclassify-buckets.md to reflect that meal_delivery and night_club now positively map to Eatery instead of falling out as unmapped residue, that movie_theater/stadium/hardware_store are newly removable when found in the wrong bucket, and that the cleanup rule is now unified with the read filter (ReclassifyForCategory) on the same primary-type map while still deliberately diverging from the write rule. Fix the RemoveMisclassifiedPlacesFromCategoryBuckets docstring's stale meal_delivery/night_club "unmapped" examples (comment-only, no logic change). Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01DjdGnZM6cUBeg6jWwoQfU3 --- README.md | 102 ++++++++++++++++++++++++++ docs/migrations/reclassify-buckets.md | 74 ++++++++++++++++--- iowrappers/data_migrations.go | 44 ++++++----- 3 files changed, 190 insertions(+), 30 deletions(-) diff --git a/README.md b/README.md index 50b500c2..6bd5959b 100644 --- a/README.md +++ b/README.md @@ -20,6 +20,108 @@ 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 — unlike nearby search, 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. `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, and summary 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. + +### 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..38a8bbfd 100644 --- a/docs/migrations/reclassify-buckets.md +++ b/docs/migrations/reclassify-buckets.md @@ -32,12 +32,47 @@ 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 | +| `movie_theater`, `stadium` (found in the Eatery bucket) | Visit | removed | +| `hardware_store` (found in the Eatery bucket) | Shopping | removed | +| `university`, `airport`, `real_estate_agency`, `doctor` | 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, `movie_theater`, `stadium`, and +`hardware_store` used to be unmapped-and-kept too; they are now positively mapped to a +category *other than* Eatery, so a bucket member primaried with one of them is newly +REMOVABLE when this migration runs against `category=Eatery`. +`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 +91,31 @@ 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 -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. +⚠️ 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 (`movie_theater`, `stadium`, +`hardware_store`, and others) it used to treat as unmapped-and-kept, so the Eatery candidate +count in particular should go up. 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:** as of the numbers above, 27 incident records were *not* removed because +their primary type mapped to no category — `university` ×5, `airport` ×2, +`real_estate_agency` ×2, `stadium`, `night_club`, `hardware_store`, `doctor`, and 7 with no +`types[]`. With the expanded `placeTypeToCategory` map, three of those records change status: +`stadium` (→ Visit) and `hardware_store` (→ Shopping) are now positively mapped to a +*different* category, so they become removable candidates instead of residue; `night_club` +(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. Expected residue after this PR: **~24** (27 − 3), still `university` ×5, `airport` ×2, +`real_estate_agency` ×2, `doctor`, and 7 with no `types[]`, unchanged because none of those +types were added to the map. These 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. As always, +treat ~24 as an estimate to confirm with the next dry run, not a number to assert without +running it — this migration has not been re-run against production since the map expanded. 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/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 } From a27236596beebf258795c5c9fc3d3f00514e4f26 Mon Sep 17 00:00:00 2001 From: tim-eternos Date: Thu, 30 Jul 2026 18:10:18 -0700 Subject: [PATCH 6/8] Fix review findings: residue arithmetic, false comparison, docstring MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the unsubstantiated movie_theater residue claim and the "~24" estimate in reclassify-buckets.md with the controller's authoritative production audit: all 27 records in placeIDs:eatery by primary type, correctly splitting into 5 removable (shoe_store x2, furniture_store, hardware_store, stadium), 1 legitimized (night_club), and 21 still unmapped residue. movie_theater is now labeled as a rule/test-fixture example only, never an observed production record. Note that the original itemization's 27-vs-20 mismatch was from eliding 7 records, not an error in the total. Fix the false "unlike nearby search" location-requirement comparison in README.md and the matching planner.go comment: both getNearbyPlaces/getNearbyPlacesByCategory already enforce the same zero-location rejection, so there is no such asymmetry. Split the ReclassifyForCategory docstring's inaccurate merged bullet in POI/categories.go into two correct ones: no Types keeps a place unchanged, but a primary type that is present-and-unmapped drops it (same as the pre-unification behavior) — comment-only, no logic change. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01DjdGnZM6cUBeg6jWwoQfU3 --- POI/categories.go | 12 +-- README.md | 11 +-- docs/migrations/reclassify-buckets.md | 101 ++++++++++++++++++-------- planner/planner.go | 7 +- 4 files changed, 88 insertions(+), 43 deletions(-) diff --git a/POI/categories.go b/POI/categories.go index ca638299..b119c23d 100644 --- a/POI/categories.go +++ b/POI/categories.go @@ -282,13 +282,15 @@ func PrimaryLocationType(types []string) LocationType { // (RemoveMisclassifiedPlacesFromCategoryBuckets) key on — so there is one rule everywhere for // "does this place belong in this category": // -// - primary type maps to cat → 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 maps to a DIFFERENT category → 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). -// - primary type is unmapped, or there is no Types → keep unchanged (older cached records, -// or a legal-but-uninteresting type), 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) diff --git a/README.md b/README.md index 6bd5959b..c2580121 100644 --- a/README.md +++ b/README.md @@ -46,10 +46,10 @@ Request: ``` `query` is required (2-120 characters). `location` (`latitude`/`longitude`) is required, with -no default — unlike nearby search, 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. `limit` defaults to 10 -and is capped at 20. +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. `limit` defaults to 10 and is capped at 20. Response (`200`, fields elided for brevity): @@ -84,7 +84,8 @@ caller can see it), but with `category: ""` and `insertable: false`. 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, and summary before writing. +URL, summary, and (as a gap-fill only, never overwriting an existing photo) photo before +writing. Request: diff --git a/docs/migrations/reclassify-buckets.md b/docs/migrations/reclassify-buckets.md index 38a8bbfd..1757a254 100644 --- a/docs/migrations/reclassify-buckets.md +++ b/docs/migrations/reclassify-buckets.md @@ -33,9 +33,10 @@ evidence of misclassification, because the write path would legitimately place i | `supermarket`, `department_store` | Shopping | removed | | `cafe`, `restaurant` | Eatery | kept | | `meal_delivery`, `night_club` | Eatery (positively mapped) | kept | -| `movie_theater`, `stadium` (found in the Eatery bucket) | Visit | removed | -| `hardware_store` (found in the Eatery bucket) | Shopping | removed | -| `university`, `airport`, `real_estate_agency`, `doctor` | unmapped | 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 | `meal_delivery` and `night_club` used to fall in the "unmapped, kept" row above: the map had @@ -44,10 +45,16 @@ lack of any evidence of misclassification. The place-type map expansion (`POI/ca `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, `movie_theater`, `stadium`, and -`hardware_store` used to be unmapped-and-kept too; they are now positively mapped to a -category *other than* Eatery, so a bucket member primaried with one of them is newly -REMOVABLE when this migration runs against `category=Eatery`. +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. @@ -93,29 +100,63 @@ stamped `bakery`). ⚠️ 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 (`movie_theater`, `stadium`, -`hardware_store`, and others) it used to treat as unmapped-and-kept, so the Eatery candidate -count in particular should go up. 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:** as of the numbers above, 27 incident records were *not* removed because -their primary type mapped to no category — `university` ×5, `airport` ×2, -`real_estate_agency` ×2, `stadium`, `night_club`, `hardware_store`, `doctor`, and 7 with no -`types[]`. With the expanded `placeTypeToCategory` map, three of those records change status: -`stadium` (→ Visit) and `hardware_store` (→ Shopping) are now positively mapped to a -*different* category, so they become removable candidates instead of residue; `night_club` -(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. Expected residue after this PR: **~24** (27 − 3), still `university` ×5, `airport` ×2, -`real_estate_agency` ×2, `doctor`, and 7 with no `types[]`, unchanged because none of those -types were added to the map. These 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. As always, -treat ~24 as an estimate to confirm with the next dry run, not a number to assert without -running it — this migration has not been re-run against production since the map expanded. +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 records — 5 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 `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/planner/planner.go b/planner/planner.go index 50fb5f2c..7c583b1c 100644 --- a/planner/planner.go +++ b/planner/planner.go @@ -1489,9 +1489,10 @@ type placeTextSearchRequest struct { // 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 (not optional, unlike nearby search's default) because an unbiased text query like -// "konjoe" can resolve to the wrong continent without a coordinate to anchor it. +// 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 { From 3c35c962c2a6b64c3c09a7683521d4388af72b13 Mon Sep 17 00:00:00 2001 From: tim-eternos Date: Thu, 30 Jul 2026 19:57:40 -0700 Subject: [PATCH 7/8] Pin GetPlaceTypes(Visit) in TestGetPlaceTypesByCategory The cases map covered Eatery/Shopping/Lodging/Wellness but omitted Visit, the one category where GetPlaceTypes had grown most and where silently widening the searched-types list would go unguarded. Add the Visit row (Park/AmusementPark/Gallery/Museum) so the suite fails if a fifth type is ever added to GetPlaceTypes(Visit). Verified RED: temporarily added LocationTypeTouristAttraction to GetPlaceTypes(Visit) and reran the test, which failed with "got 5 place types ... want 4" before the mutation was reverted. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01DjdGnZM6cUBeg6jWwoQfU3 --- test/place_category_test.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/test/place_category_test.go b/test/place_category_test.go index b8979277..f765f6fc 100644 --- a/test/place_category_test.go +++ b/test/place_category_test.go @@ -10,6 +10,10 @@ import ( // 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, From 2a61462cb8b6c718ae57366baf2077c554c07805 Mon Sep 17 00:00:00 2001 From: tim-eternos Date: Thu, 30 Jul 2026 19:57:52 -0700 Subject: [PATCH 8/8] Fix remaining review findings: docs, wire-contract tags, and a magic number MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - README: document cold-cell visibility for the confirm endpoint — cache membership is immediate (verify via the confirm response or ZSCORE placeIDs: ), but appearance in /v1/nearby-places-by-category needs a warm cell or a second read after a cold search replaces (not unions) the cached bucket. Never manually stamp MapsLastSearchTime to force it. Docs only, no code. - README: fix radius wording from "when zero or larger" (reads as "always") to "when zero or larger than that maximum". - iowrappers/text_search.go: add json tags to AddSearchedPlaceResult (place/category/alreadyCached) to match its sibling PlaceSearchCandidate; POI.Place itself stays untagged so this doesn't change every endpoint's wire shape at once. Added a hermetic marshal test pinning the exact top-level key sets and the nested place object's capitalized field names the Convex client depends on. - planner/planner.go: use iowrappers.PlaceTextSearchMaxResults instead of a literal 20 for the search handler's limit cap so the two constants can't drift; the default-10 literal is unchanged. - docs/migrations/reclassify-buckets.md: clarify "These 4 records — 5 counting both shoe_store instances" to "These 4 primary types — 5 records, counting both shoe_store instances" since the count is over primary types, not records. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01DjdGnZM6cUBeg6jWwoQfU3 --- README.md | 21 ++++++- docs/migrations/reclassify-buckets.md | 4 +- iowrappers/text_search.go | 6 +- iowrappers/text_search_test.go | 89 +++++++++++++++++++++++++++ planner/planner.go | 4 +- 5 files changed, 116 insertions(+), 8 deletions(-) diff --git a/README.md b/README.md index c2580121..3efd2f5f 100644 --- a/README.md +++ b/README.md @@ -49,7 +49,7 @@ Request: 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. `limit` defaults to 10 and is capped at 20. +~10 miles) when zero or larger than that maximum. `limit` defaults to 10 and is capped at 20. Response (`200`, fields elided for brevity): @@ -110,6 +110,25 @@ Error responses: * `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 diff --git a/docs/migrations/reclassify-buckets.md b/docs/migrations/reclassify-buckets.md index 1757a254..f64154b5 100644 --- a/docs/migrations/reclassify-buckets.md +++ b/docs/migrations/reclassify-buckets.md @@ -139,8 +139,8 @@ 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 records — 5 counting both `shoe_store` instances — become removable - candidates the next time this migration runs against `category=Eatery`. + 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. diff --git a/iowrappers/text_search.go b/iowrappers/text_search.go index 93b850fc..92310a66 100644 --- a/iowrappers/text_search.go +++ b/iowrappers/text_search.go @@ -62,9 +62,9 @@ type PlaceSearchCandidate struct { // AddSearchedPlaceResult is the outcome of confirming a search candidate into the shared cache. type AddSearchedPlaceResult struct { - Place POI.Place - Category POI.PlaceCategory - AlreadyCached bool + 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 diff --git a/iowrappers/text_search_test.go b/iowrappers/text_search_test.go index 33c909dc..67c307eb 100644 --- a/iowrappers/text_search_test.go +++ b/iowrappers/text_search_test.go @@ -1,6 +1,7 @@ package iowrappers import ( + "encoding/json" "testing" "github.com/weihesdlegend/Vacation-planner/POI" @@ -260,3 +261,91 @@ func TestParseTextSearchResponse_NoRealOpeningHours(t *testing.T) { 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/planner.go b/planner/planner.go index 7c583b1c..c8d1818c 100644 --- a/planner/planner.go +++ b/planner/planner.go @@ -1518,8 +1518,8 @@ func (p *MyPlanner) searchPlacesByText(ctx *gin.Context) { if limit <= 0 { limit = 10 } - if limit > 20 { - limit = 20 + if limit > iowrappers.PlaceTextSearchMaxResults { + limit = iowrappers.PlaceTextSearchMaxResults } searchContext := context.WithValue(ctx.Request.Context(), iowrappers.ContextRequestIdKey, requestid.Get(ctx))