diff --git a/config/config.yml b/config/config.yml index 812dbbb5d..7ccc53385 100644 --- a/config/config.yml +++ b/config/config.yml @@ -1,12 +1,14 @@ server: google_maps: + # Every field is billed per Details call, tiered by field group — see + # TestDetailedSearchFieldsMask for why name and user_ratings_total are + # deliberately absent (nothing reads them from a Details response; both + # arrive free with every Nearby/Text Search result). detailed_search_fields: - - name - opening_hours - formatted_address - adr_address - url - - user_ratings_total - editorial_summary - photos plan_solver: diff --git a/config_yaml_test.go b/config_yaml_test.go new file mode 100644 index 000000000..4440c22a5 --- /dev/null +++ b/config_yaml_test.go @@ -0,0 +1,46 @@ +package main + +import ( + "os" + "testing" + + "gopkg.in/yaml.v3" +) + +// Pins the Place Details field mask in config/config.yml. Every field here is billed per Details +// call, tiered by field group (Basic / Contact / Atmosphere on the legacy API), so an unused +// field is pure spend: +// - name and user_ratings_total must stay OUT: neither is read from a Details response on any +// path (both already arrive free with every Nearby/Text Search result), and +// user_ratings_total alone pulls the call into the Atmosphere tier. The AddUserRatingsTotal +// admin migration passes its own single-field list and is unaffected. +// - the remaining fields are load-bearing: opening_hours (open-now filtering), +// formatted_address/adr_address (display + address parsing), url (also the Details-freshness +// signal — see iowrappers/data_migrations.go detailsSourcedFields), editorial_summary (trip +// planner), photos (place photos + confirm gap-fill). +func TestDetailedSearchFieldsMask(t *testing.T) { + raw, err := os.ReadFile("config/config.yml") + if err != nil { + t.Fatalf("reading config/config.yml: %v", err) + } + var configs Configurations + if err := yaml.Unmarshal(raw, &configs); err != nil { + t.Fatalf("unmarshal config.yml: %v", err) + } + + fields := make(map[string]bool) + for _, f := range configs.Server.GoogleMaps.DetailedSearchFields { + fields[f] = true + } + + for _, banned := range []string{"name", "user_ratings_total"} { + if fields[banned] { + t.Errorf("detailed_search_fields contains %q, which no Details consumer reads — it only adds billing tier", banned) + } + } + for _, required := range []string{"opening_hours", "formatted_address", "adr_address", "url", "editorial_summary", "photos"} { + if !fields[required] { + t.Errorf("detailed_search_fields is missing load-bearing field %q", required) + } + } +} diff --git a/iowrappers/add_searched_place_currency_test.go b/iowrappers/add_searched_place_currency_test.go new file mode 100644 index 000000000..0c65991cb --- /dev/null +++ b/iowrappers/add_searched_place_currency_test.go @@ -0,0 +1,110 @@ +package iowrappers + +import ( + "context" + "errors" + "testing" + "time" + + "github.com/weihesdlegend/Vacation-planner/POI" + "googlemaps.github.io/maps" +) + +// These tests pin when a confirm buys a Place Details call. A Details call is the single most +// expensive Google request the service makes (max field tier), and a re-confirm of a place whose +// stored record already carries current Details-sourced fields holds all the data the confirm +// needs — the same placeDetailsAreCurrent rule the nearby-search path already trusts. + +func countingEnricher(calls *int) placeDetailsEnricher { + return func(ctx context.Context, placeID string) (maps.PlaceDetailsResult, error) { + *calls++ + return maps.PlaceDetailsResult{}, errors.New("no network in this test") + } +} + +func TestConfirmSkipsDetailsWhenCachedRecordIsCurrent(t *testing.T) { + s, ctx := newAddSearchedPlaceFixture(t) + + placeID := "museum-current" + stored := POI.Place{ + ID: placeID, + Name: "City History Museum", + LocationType: POI.LocationType("museum"), + Location: POI.Location{Latitude: 37.4, Longitude: -122.1}, + URL: "https://maps.google.com/?cid=42", // proof a Details call landed (detailsSourcedFields) + LastUpdatedAt: time.Now().Format(time.RFC3339), + } + s.redisClient.SetPlacesAddGeoLocations(ctx, []POI.Place{stored}) + + stashCandidate(t, s, ctx, POI.Place{ + ID: placeID, + Name: "City History Museum", + Types: []string{"museum", "point_of_interest", "establishment"}, + Location: POI.Location{Latitude: 37.4, Longitude: -122.1}, + }) + + calls := 0 + result, err := s.addSearchedPlaceToCache(ctx, placeID, countingEnricher(&calls)) + if err != nil { + t.Fatalf("addSearchedPlaceToCache: %v", err) + } + if calls != 0 { + t.Errorf("enricher called %d times for a current cached record, want 0 — this is a billed Place Details call", calls) + } + if !result.AlreadyCached { + t.Error("AlreadyCached = false, want true") + } + if result.Place.URL != stored.URL { + t.Errorf("Place.URL = %q, want the cached record's %q restored", result.Place.URL, stored.URL) + } +} + +func TestConfirmBuysDetailsWhenCachedRecordIsStale(t *testing.T) { + s, ctx := newAddSearchedPlaceFixture(t) + + placeID := "museum-stale" + stale := time.Now().Add(-(PlaceDetailsRefreshDuration + 24*time.Hour)) + s.redisClient.SetPlacesAddGeoLocations(ctx, []POI.Place{{ + ID: placeID, + Name: "Old Museum", + LocationType: POI.LocationType("museum"), + Location: POI.Location{Latitude: 37.4, Longitude: -122.1}, + URL: "https://maps.google.com/?cid=43", + LastUpdatedAt: stale.Format(time.RFC3339), + }}) + + stashCandidate(t, s, ctx, POI.Place{ + ID: placeID, + Name: "Old Museum", + Types: []string{"museum", "point_of_interest", "establishment"}, + Location: POI.Location{Latitude: 37.4, Longitude: -122.1}, + }) + + calls := 0 + if _, err := s.addSearchedPlaceToCache(ctx, placeID, countingEnricher(&calls)); err != nil { + t.Fatalf("addSearchedPlaceToCache: %v", err) + } + if calls != 1 { + t.Errorf("enricher called %d times for a stale cached record, want 1", calls) + } +} + +func TestConfirmBuysDetailsWhenNotCached(t *testing.T) { + s, ctx := newAddSearchedPlaceFixture(t) + + placeID := "museum-uncached" + stashCandidate(t, s, ctx, POI.Place{ + ID: placeID, + Name: "Brand New Museum", + Types: []string{"museum", "point_of_interest", "establishment"}, + Location: POI.Location{Latitude: 37.4, Longitude: -122.1}, + }) + + calls := 0 + if _, err := s.addSearchedPlaceToCache(ctx, placeID, countingEnricher(&calls)); err != nil { + t.Fatalf("addSearchedPlaceToCache: %v", err) + } + if calls != 1 { + t.Errorf("enricher called %d times for an uncached place, want 1", calls) + } +} diff --git a/iowrappers/closure_persistence_test.go b/iowrappers/closure_persistence_test.go new file mode 100644 index 000000000..3a4875c35 --- /dev/null +++ b/iowrappers/closure_persistence_test.go @@ -0,0 +1,119 @@ +package iowrappers + +import ( + "testing" + + "github.com/weihesdlegend/Vacation-planner/POI" +) + +// Pins that a cold search PERSISTS permanently-closed places while still excluding them from the +// response. The previous behavior filtered non-Operational results BEFORE the cache write, which +// discarded the closure signal entirely: the stale record kept Status OPERATIONAL forever and the +// place kept being served from cache. Persisting the closure lets the read-side Operational +// filters (RedisClient.NearbySearch) retire the place everywhere, at zero extra API cost. +// +// Filtering is the DEFAULT: now that closed places genuinely live in the cache, a caller that +// forgets to ask for filtering must never receive them — planners consume these results and a +// zero-value request has to be safe. IncludeClosedPlaces is the explicit opt-in for the rare +// caller that wants everything. +func TestColdSearchPersistsClosuresButExcludesThemFromResponse(t *testing.T) { + s, ctx := newAddSearchedPlaceFixture(t) + + open := POI.Place{ + ID: "open-1", + Name: "Open Diner", + LocationType: POI.LocationType("restaurant"), + Status: POI.Operational, + Location: POI.Location{Latitude: 37.4, Longitude: -122.1}, + } + closed := POI.Place{ + ID: "closed-1", + Name: "Shuttered Grill", + LocationType: POI.LocationType("restaurant"), + Status: POI.ClosedPermanently, + Location: POI.Location{Latitude: 37.401, Longitude: -122.101}, + } + // Deliberately a zero-value request apart from the search identity: the default must filter. + req := &PlaceSearchRequest{ + PlaceCat: POI.PlaceCategoryEatery, + Location: POI.Location{Latitude: 37.4, Longitude: -122.1}, + } + + got := s.persistAndFilterSearchResults(ctx, req, []POI.Place{open, closed}) + + if len(got) != 1 || got[0].ID != "open-1" { + t.Fatalf("response = %+v, want only open-1 (closed places excluded by default)", got) + } + + cached, err := s.redisClient.CachedPlaces(ctx, []string{"open-1", "closed-1"}) + if err != nil { + t.Fatalf("CachedPlaces: %v", err) + } + if _, ok := cached["open-1"]; !ok { + t.Error("open-1 was not persisted") + } + stored, ok := cached["closed-1"] + if !ok { + t.Fatal("closed-1 was not persisted — the closure signal was discarded, the pre-write-filter bug") + } + if stored.Status != POI.ClosedPermanently { + t.Errorf("closed-1 Status = %q, want %q recorded", stored.Status, POI.ClosedPermanently) + } + + // The read path must retire it BY DEFAULT: a zero-value cache read excludes the closed place. + readReq := &PlaceSearchRequest{ + PlaceCat: POI.PlaceCategoryEatery, + Location: POI.Location{Latitude: 37.4, Longitude: -122.1}, + Radius: 1000, + MinNumResults: 1, + } + fromCache, err := s.redisClient.NearbySearch(ctx, readReq) + if err != nil { + t.Fatalf("RedisClient.NearbySearch: %v", err) + } + for _, p := range fromCache { + if p.ID == "closed-1" { + t.Error("closed-1 served from a default cache read — planners would receive a closed place") + } + } +} + +func TestIncludeClosedPlacesOptsIntoUnfilteredResults(t *testing.T) { + s, ctx := newAddSearchedPlaceFixture(t) + + places := []POI.Place{ + {ID: "a", Name: "A", LocationType: POI.LocationType("restaurant"), Status: POI.Operational, Location: POI.Location{Latitude: 37.4, Longitude: -122.1}}, + {ID: "b", Name: "B", LocationType: POI.LocationType("restaurant"), Status: POI.ClosedPermanently, Location: POI.Location{Latitude: 37.401, Longitude: -122.101}}, + } + req := &PlaceSearchRequest{ + PlaceCat: POI.PlaceCategoryEatery, + Location: POI.Location{Latitude: 37.4, Longitude: -122.1}, + IncludeClosedPlaces: true, + } + + got := s.persistAndFilterSearchResults(ctx, req, places) + if len(got) != 2 { + t.Fatalf("response has %d places, want 2 — IncludeClosedPlaces opted into everything", len(got)) + } + + readReq := &PlaceSearchRequest{ + PlaceCat: POI.PlaceCategoryEatery, + Location: POI.Location{Latitude: 37.4, Longitude: -122.1}, + Radius: 1000, + MinNumResults: 1, + IncludeClosedPlaces: true, + } + fromCache, err := s.redisClient.NearbySearch(ctx, readReq) + if err != nil { + t.Fatalf("RedisClient.NearbySearch: %v", err) + } + foundClosed := false + for _, p := range fromCache { + if p.ID == "b" { + foundClosed = true + } + } + if !foundClosed { + t.Error("IncludeClosedPlaces read did not return the closed place") + } +} diff --git a/iowrappers/data_migrations_test.go b/iowrappers/data_migrations_test.go index 5a3a1b3fd..c51a8270a 100644 --- a/iowrappers/data_migrations_test.go +++ b/iowrappers/data_migrations_test.go @@ -72,7 +72,8 @@ func TestRemovePlaces(t *testing.T) { var err error redisClient.SetPlacesAddGeoLocations(ctx, []POI.Place{placeA, placeB, placeC}) places, _ = redisClient.NearbySearch(ctx, &PlaceSearchRequest{ - PlaceCat: POI.PlaceCategoryVisit, + PlaceCat: POI.PlaceCategoryVisit, + IncludeClosedPlaces: true, // migration test reads raw bucket contents; fixtures carry no Status Location: POI.Location{ Latitude: 12.5636, Longitude: 14.7813, @@ -92,7 +93,8 @@ func TestRemovePlaces(t *testing.T) { } places, _ = redisClient.NearbySearch(ctx, &PlaceSearchRequest{ - PlaceCat: POI.PlaceCategoryVisit, + PlaceCat: POI.PlaceCategoryVisit, + IncludeClosedPlaces: true, // migration test reads raw bucket contents; fixtures carry no Status Location: POI.Location{ Latitude: 12.5636, Longitude: 14.7813, @@ -106,6 +108,7 @@ func TestRemovePlaces(t *testing.T) { } places, _ = redisClient.NearbySearch(ctx, &PlaceSearchRequest{PlaceCat: POI.PlaceCategoryEatery, + IncludeClosedPlaces: true, // migration test reads raw bucket contents; fixtures carry no Status Location: POI.Location{ Latitude: 12.5636, Longitude: 14.7813, diff --git a/iowrappers/nearby_search.go b/iowrappers/nearby_search.go index 9f04632a0..8c6194468 100644 --- a/iowrappers/nearby_search.go +++ b/iowrappers/nearby_search.go @@ -35,7 +35,13 @@ type PlaceSearchRequest struct { // suppose a location has more places established over time, this field would help trigger new searches to get those new establishments. MinNumResults uint - BusinessStatus POI.BusinessStatus + // IncludeClosedPlaces opts a caller INTO receiving non-Operational places. The zero value + // filters them from both cache reads and cold-search responses: closed places are persisted + // in the cache (their closure is the signal that retires them), so a request that forgot to + // ask for filtering must never serve one — planners consume these results directly. This + // replaces the old BusinessStatus field, whose zero value was the UNFILTERED behavior and + // therefore one forgotten assignment away from leaking closures. + IncludeClosedPlaces bool // true if using precise geolocation instead of using a grander administrative area UsePreciseLocation bool diff --git a/iowrappers/photos_client.go b/iowrappers/photos_client.go index 6ee61af10..c2709a678 100644 --- a/iowrappers/photos_client.go +++ b/iowrappers/photos_client.go @@ -139,7 +139,9 @@ func (c *MapsPhotoClient) GetPhotoURL(ctx context.Context, photoRef string, plac // Acquire semaphore for API rate limiting c.mapsClient.apiSemaphore <- struct{}{} - r, err = c.mapsClient.PlaceDetailedSearch(ctx, placeId, c.mapsClient.DetailedSearchFields) + // Only the photo reference is needed here — the full DetailedSearchFields mask + // would bill the Contact/Atmosphere field tiers for data this path throws away. + r, err = c.mapsClient.PlaceDetailedSearch(ctx, placeId, []string{"photos"}) <-c.mapsClient.apiSemaphore // Release semaphore if err != nil { return "", err diff --git a/iowrappers/poi_searcher.go b/iowrappers/poi_searcher.go index f07c53651..a67e2c020 100644 --- a/iowrappers/poi_searcher.go +++ b/iowrappers/poi_searcher.go @@ -148,9 +148,20 @@ func (s *PoiSearcher) Geocode(context context.Context, query *GeocodeQuery) (lat return } +// ReverseGeocode resolves city-level info for a coordinate, consulting the per-cell Redis cache +// before Google. This runs on EVERY nearby scan (processLocation), and on a warm place cache it +// was the only Google call left — caching it takes a warm scan's Google spend to zero. func (s *PoiSearcher) ReverseGeocode(ctx context.Context, lat, lng float64) (*GeocodeQuery, error) { Logger.Debugf("PoiSearcher ->ReverseGeocode: decoding latitude %.2f, longitude %.2f", lat, lng) - return s.mapsClient.ReverseGeocode(ctx, lat, lng) + if cached, err := s.redisClient.ReverseGeocode(ctx, lat, lng); err == nil { + return cached, nil + } + query, err := s.mapsClient.ReverseGeocode(ctx, lat, lng) + if err != nil { + return nil, err + } + s.redisClient.SetReverseGeocode(ctx, lat, lng, *query) + return query, nil } // canServeFromCache decides whether cached places can satisfy a request without an external @@ -248,27 +259,50 @@ func (s *PoiSearcher) NearbySearch(context context.Context, request *PlaceSearch utils.LogErrorWithLevel(s.redisClient.SetMapsLastSearchTime(context, lat, lng, request.PlaceCat, request.PriceLevel, currentTime.Format(time.RFC3339)), utils.LogError) } - if request.Keyword != "" && request.StrictNameMatch { + places = append(places, s.persistAndFilterSearchResults(context, request, newPlaces)...) + + return places, nil +} + +// persistAndFilterSearchResults writes a cold search's results to the cache and returns the slice +// the caller may serve. The write deliberately includes non-Operational places: a permanent +// closure reported by Google is a signal we must PERSIST (so the read-side Operational filters +// retire the stale record everywhere), not discard — the previous pre-write filter meant a closed +// place kept its OPERATIONAL record and cache membership forever. The response filter then keeps +// closures out of what the caller sees, same as before. +func (s *PoiSearcher) persistAndFilterSearchResults(ctx context.Context, req *PlaceSearchRequest, newPlaces []POI.Place) []POI.Place { + if req.Keyword != "" && req.StrictNameMatch { // drop keyword-search results that are merely related to the brand (Google Maps // matches keywords against reviews and other content, not just names) before they // reach the brand-scoped cache - newPlaces = Filter(newPlaces, func(place POI.Place) bool { return MatchesBrandName(place.Name, request.Keyword) }) + newPlaces = Filter(newPlaces, func(place POI.Place) bool { return MatchesBrandName(place.Name, req.Keyword) }) } // safeguard on accessing elements in a nil slice if len(newPlaces) > 0 { - // update Redis with all the new places obtained - if request.Keyword != "" { - s.redisClient.SetPlacesAddGeoLocationsForBrand(context, request.Keyword, newPlaces) + // update Redis with all the new places obtained, closures included + if req.Keyword != "" { + s.redisClient.SetPlacesAddGeoLocationsForBrand(ctx, req.Keyword, newPlaces) } else { - s.UpdateRedis(context, newPlaces) + s.UpdateRedis(ctx, newPlaces) } + } - // include places from cache in the result - places = append(places, newPlaces...) + if !req.IncludeClosedPlaces { + totalPlacesCount := len(newPlaces) + newPlaces = Filter(newPlaces, func(place POI.Place) bool { return place.Status == POI.Operational }) + Logger.Debugf("%d places out of %d left after business status filtering", len(newPlaces), totalPlacesCount) } - return places, nil + if uint(len(newPlaces)) < req.MinNumResults { + Logger.Debugf("Found %d POI results for place type %s, less than requested number of %d", + len(newPlaces), req.PlaceCat, req.MinNumResults) + } + if len(newPlaces) == 0 { + Logger.Debugf("No qualified POI result found in the given location %v, radius %d, and place type: %s. The location may be invalid", + req.Location, req.Radius, req.PlaceCat) + } + return newPlaces } // processLocation performs reverse geocoding for precise location to find city-level information and performs geocoding to find precise latitude and longitude values @@ -281,7 +315,7 @@ func (s *PoiSearcher) processLocation(ctx context.Context, req *PlaceSearchReque } if req.UsePreciseLocation { Logger.Debugf("->NearbySearch: using precise location") - geoQuery, err := s.GetMapsClient().ReverseGeocode(ctx, req.Location.Latitude, req.Location.Longitude) + geoQuery, err := s.ReverseGeocode(ctx, req.Location.Latitude, req.Location.Longitude) if err != nil { return err } @@ -319,20 +353,8 @@ func (s *PoiSearcher) searchPlacesWithMaps(ctx context.Context, req *PlaceSearch return nil, err } - if req.BusinessStatus == POI.Operational { - totalPlacesCount := len(places) - places = Filter(places, func(place POI.Place) bool { return place.Status == POI.Operational }) - Logger.Debugf("%d places out of %d left after business status filtering", len(places), totalPlacesCount) - } - - if uint(len(places)) < req.MinNumResults { - Logger.Debugf("Found %d POI results for place type %s, less than requested number of %d", - len(places), req.PlaceCat, req.MinNumResults) - } - if len(places) == 0 { - Logger.Debugf("No qualified POI result found in the given location %v, radius %d, and place type: %s. The location may be invalid", - req.Location, req.Radius, req.PlaceCat) - } + // No status filtering here: closures must reach the cache write so they are persisted — + // persistAndFilterSearchResults owns both the write and the response-side filter. return places, nil } diff --git a/iowrappers/redis_client.go b/iowrappers/redis_client.go index 3cb74c305..124f7b834 100644 --- a/iowrappers/redis_client.go +++ b/iowrappers/redis_client.go @@ -581,7 +581,7 @@ func (r *RedisClient) NearbySearch(ctx context.Context, req *PlaceSearchRequest) ctx.Value(ContextRequestIdKey), redisKey, orphans) } - if req.BusinessStatus == POI.Operational { + if !req.IncludeClosedPlaces { totalPlacesCount := len(places) places = Filter(places, func(place POI.Place) bool { return place.Status == POI.Operational }) Logger.Debugf("(RedisClient)NearbySearch -> %d places out of %d left after business status filtering", len(places), totalPlacesCount) @@ -673,8 +673,41 @@ func (r *RedisClient) Geocode(context context.Context, query *GeocodeQuery) (lat return } -func (r *RedisClient) ReverseGeocode(context.Context, float64, float64) (*GeocodeQuery, error) { - return nil, errors.New("->ReverseGeocode: not implemented for the RedisClient") +// ReverseGeocodeExpiration bounds how long a cached reverse-geocode result is served. City-level +// info is effectively static, but Google's Geocoding terms only allow temporary caching — 30 days +// keeps the cache self-cleaning and policy-friendly. +const ReverseGeocodeExpiration = 30 * 24 * time.Hour + +// reverseGeocodeRedisKey keys reverse-geocode results by the same ~8 km search cell the +// MapsLastSearchTime markers use: the result only carries country/admin1/locality, which is +// stable at that granularity. +func reverseGeocodeRedisKey(lat, lng float64) string { + return "geocode:reverse:" + POI.EncodeSearchCell(lat, lng) +} + +func (r *RedisClient) ReverseGeocode(ctx context.Context, lat, lng float64) (*GeocodeQuery, error) { + res, err := r.client.Get(ctx, reverseGeocodeRedisKey(lat, lng)).Result() + if err != nil { + return nil, err + } + var query GeocodeQuery + if err = json.Unmarshal([]byte(res), &query); err != nil { + return nil, err + } + return &query, nil +} + +// SetReverseGeocode caches a reverse-geocode result for the cell containing (lat, lng). +// Best-effort like SetGeocode: a write failure only means the next request buys the geocode again. +func (r *RedisClient) SetReverseGeocode(ctx context.Context, lat, lng float64, query GeocodeQuery) { + payload, err := json.Marshal(query) + if err != nil { + utils.LogErrorWithLevel(err, utils.LogError) + return + } + utils.LogErrorWithLevel( + r.client.Set(ctx, reverseGeocodeRedisKey(lat, lng), payload, ReverseGeocodeExpiration).Err(), + utils.LogError) } func (r *RedisClient) SetGeocode(context context.Context, query GeocodeQuery, lat float64, lng float64, originalQuery GeocodeQuery) { diff --git a/iowrappers/reverse_geocode_cache_test.go b/iowrappers/reverse_geocode_cache_test.go new file mode 100644 index 000000000..64f4560b2 --- /dev/null +++ b/iowrappers/reverse_geocode_cache_test.go @@ -0,0 +1,106 @@ +package iowrappers + +import ( + "context" + "net/url" + "testing" + "time" + + "github.com/alicebob/miniredis/v2" + "github.com/weihesdlegend/Vacation-planner/POI" +) + +// newReverseGeocodeFixture builds a RedisClient backed by its own miniredis instance, mirroring +// add_searched_place_test.go's own-server-per-test harness. +func newReverseGeocodeFixture(t *testing.T) (*RedisClient, *miniredis.Miniredis, context.Context) { + t.Helper() + svr, err := miniredis.Run() + if err != nil { + t.Fatalf("miniredis.Run: %v", err) + } + t.Cleanup(svr.Close) + + redisURL, _ := url.Parse("redis://" + svr.Addr()) + if err := CreateLogger(); err != nil { + t.Fatalf("CreateLogger: %v", err) + } + return CreateRedisClient(redisURL), svr, context.Background() +} + +func TestReverseGeocodeCacheRoundTrip(t *testing.T) { + r, _, ctx := newReverseGeocodeFixture(t) + want := GeocodeQuery{City: "Mountain View", AdminAreaLevelOne: "CA", Country: "US"} + + r.SetReverseGeocode(ctx, 37.4001, -122.0801, want) + + // Nearby coordinates land in the same ~8 km search cell and must hit. + got, err := r.ReverseGeocode(ctx, 37.4002, -122.0802) + if err != nil { + t.Fatalf("ReverseGeocode after Set: %v", err) + } + if *got != want { + t.Fatalf("ReverseGeocode = %+v, want %+v", *got, want) + } +} + +func TestReverseGeocodeCacheMiss(t *testing.T) { + r, _, ctx := newReverseGeocodeFixture(t) + + if _, err := r.ReverseGeocode(ctx, 37.4, -122.08); err == nil { + t.Fatal("ReverseGeocode on an empty cache must return an error") + } + + // A hit in one cell must not leak into another cell. + r.SetReverseGeocode(ctx, 37.4, -122.08, GeocodeQuery{City: "Mountain View", Country: "US"}) + if _, err := r.ReverseGeocode(ctx, 40.7, -74.0); err == nil { + t.Fatal("ReverseGeocode for a different cell must miss") + } +} + +func TestReverseGeocodeCacheExpires(t *testing.T) { + r, svr, ctx := newReverseGeocodeFixture(t) + r.SetReverseGeocode(ctx, 37.4, -122.08, GeocodeQuery{City: "Mountain View", Country: "US"}) + + svr.FastForward(ReverseGeocodeExpiration + time.Hour) + + if _, err := r.ReverseGeocode(ctx, 37.4, -122.08); err == nil { + t.Fatal("ReverseGeocode must miss after the cache entry expires") + } +} + +// A cached cell must satisfy PoiSearcher.ReverseGeocode without any Google call: the fixture's +// mapsClient is nil, so reaching for Google would panic — returning the cached value is the proof +// the cache is consulted first. This is the call every warm nearby scan makes. +func TestPoiSearcherReverseGeocodeServesFromCache(t *testing.T) { + r, _, ctx := newReverseGeocodeFixture(t) + s := &PoiSearcher{redisClient: r} + want := GeocodeQuery{City: "Los Altos", AdminAreaLevelOne: "CA", Country: "US"} + r.SetReverseGeocode(ctx, 37.379, -122.117, want) + + got, err := s.ReverseGeocode(ctx, 37.379, -122.117) + if err != nil { + t.Fatalf("ReverseGeocode: %v", err) + } + if *got != want { + t.Fatalf("ReverseGeocode = %+v, want %+v", *got, want) + } +} + +// processLocation's precise-location branch is the nearby-scan entry point for the reverse +// geocode — it must go through the searcher's cached path, not straight to the maps client. +func TestProcessLocationPreciseUsesCachedReverseGeocode(t *testing.T) { + r, _, ctx := newReverseGeocodeFixture(t) + s := &PoiSearcher{redisClient: r} + r.SetReverseGeocode(ctx, 37.379, -122.117, GeocodeQuery{City: "Los Altos", AdminAreaLevelOne: "CA", Country: "US"}) + + req := &PlaceSearchRequest{ + Location: POI.Location{Latitude: 37.379, Longitude: -122.117}, + UsePreciseLocation: true, + } + if err := s.processLocation(ctx, req); err != nil { + t.Fatalf("processLocation: %v", err) + } + if req.Location.City != "Los Altos" || req.Location.Country != "US" { + t.Fatalf("processLocation resolved %+v, want cached Los Altos/US", req.Location) + } +} diff --git a/iowrappers/text_search.go b/iowrappers/text_search.go index 92310a66a..66f733522 100644 --- a/iowrappers/text_search.go +++ b/iowrappers/text_search.go @@ -246,8 +246,10 @@ func newPlaceDetailsEnricher(sem chan struct{}, fields []string, search placeDet // 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. +// 4. Best-effort enrich via Place Details for hours/address/URL/summary — SKIPPED entirely when +// the already-cached record's details are current (placeDetailsAreCurrent), since step 5 +// restores those fields anyway and the call is the most expensive Google request we make. A +// failure 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. @@ -276,11 +278,19 @@ func (s *PoiSearcher) addSearchedPlaceToCache(ctx context.Context, placeID strin 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) + // Skip the (max-tier) Place Details call when the stored record already carries current + // Details-sourced fields — the same placeDetailsAreCurrent rule the nearby-search path + // trusts. restoreCachedDetails below copies those fields onto the confirmed place, so the + // skip loses nothing; re-confirming a place we already hold must not re-buy its details. + if stored, ok := cached[placeID]; ok && placeDetailsAreCurrent(stored, time.Now()) { + Logger.Debugf("addSearchedPlaceToCache: skipping Place Details for %s — cached record is current", placeID) } else { - foldPlaceDetailsIntoPlace(&place, details) + 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} diff --git a/matching/matcher.go b/matching/matcher.go index 61bc75212..2623a05e6 100644 --- a/matching/matcher.go +++ b/matching/matcher.go @@ -79,7 +79,6 @@ func NearbySearchForCategory(ctx context.Context, searcher iowrappers.SearchClie Location: req.Location, Radius: req.Radius, MinNumResults: MinResultsForTimePeriodMatching, - BusinessStatus: POI.Operational, UsePreciseLocation: req.UsePreciseLocation, PriceLevel: req.PriceLevel, } diff --git a/planner/planner.go b/planner/planner.go index c8d1818c5..3f24ff6f7 100644 --- a/planner/planner.go +++ b/planner/planner.go @@ -232,7 +232,7 @@ func (p *MyPlanner) Destroy() { func (p *MyPlanner) reverseGeocodingHandler(ctx *gin.Context) { latitude, _ := strconv.ParseFloat(ctx.Query("lat"), 64) longitude, _ := strconv.ParseFloat(ctx.Query("lng"), 64) - result, err := p.Solver.Searcher.GetMapsClient().ReverseGeocode(ctx, latitude, longitude) + result, err := p.Solver.Searcher.ReverseGeocode(ctx, latitude, longitude) if err != nil { log.Error(err) ctx.JSON(http.StatusInternalServerError, err.Error()) @@ -1312,7 +1312,6 @@ func (p *MyPlanner) getNearbyPlaces(ctx *gin.Context) { Radius: radius, MinNumResults: uint(limit), DetailsLimit: limit, - BusinessStatus: POI.Operational, } result := nearbyPlacesBrandResult{Brand: keyword, Places: []POI.Place{}} places, searchErr := p.Solver.Searcher.NearbySearch(searchContext, searchReq) @@ -1441,7 +1440,6 @@ func (p *MyPlanner) getNearbyPlacesByCategory(ctx *gin.Context) { // Bound the expensive Place Details calls even when the candidate // pool (limit) is widened for dedup — details cost stays ~today's. DetailsLimit: min(limit, 20), - BusinessStatus: POI.Operational, } result := nearbyPlacesByCategoryResult{Category: string(placeCat), Places: []POI.Place{}} places, searchErr := p.Solver.Searcher.NearbySearch(searchContext, searchReq) diff --git a/test/redis_client_mocks/nearby_search_test.go b/test/redis_client_mocks/nearby_search_test.go index 59785ce6a..24f0f217b 100644 --- a/test/redis_client_mocks/nearby_search_test.go +++ b/test/redis_client_mocks/nearby_search_test.go @@ -152,7 +152,6 @@ func TestGetPlaces_shouldExcludePlacesNotOperational(t *testing.T) { Location: POI.Location{Longitude: -74.0060, Latitude: 40.7128}, PlaceCat: POI.PlaceCategoryVisit, Radius: uint(20000), - BusinessStatus: POI.Operational, } cachedVisitPlaces, _ := RedisClient.NearbySearch(RedisContext, &placeSearchRequest)