From 5e5d942e2003c0d83a9f9bac8fee5a3d483cda42 Mon Sep 17 00:00:00 2001 From: tim-eternos Date: Thu, 30 Jul 2026 14:46:01 -0700 Subject: [PATCH 1/3] fix: stop re-searching Google Maps on every nearby-search request MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Production logged "the number of results from redis is 0" on nearly every request, each followed by a full Google Maps fan-out. Two defects compounded. The newer categories were never wired like Visit. EncodeNearbySearchRedisKey appended a price segment for Eatery only, so Shopping/Lodging/Wellness got one price-agnostic bucket — but Get/SetMapsLastSearchTime special-cased only Visit when dropping that segment, leaving those three with a price-scoped marker over a price-agnostic bucket. Worse, the marker was keyed on country:admin1:city while the bucket it guards is a geo index read from the caller's exact coordinates. Trip planning searches from a city centroid so the two roughly agreed; the merchant endpoint, the only caller of the new categories, searches from the user's precise location. A suburban user read "Shopping in Los Angeles is fresh" against a 498-member bucket with nothing inside 16km. And a zero-result read could never be suppressed: the gate required len(savedPlaces) > 0, so the marker was consulted only alongside a non-empty result. Every such request fell through to Google, re-stamped the marker, and repeated on the next request — forever rather than once. Fixes, in the order they matter: - One price-agnostic bucket per category, mirroring placeIDs:visit. The eatery price split fragmented the index for nothing: Google omits price_level for most places (so they collapsed into level0) and only accepts a price filter at level >= 3, so searches for levels 0-2 issued identical requests yet each read back a fifth of the data. Callers already filter on price after the read. - The marker is scoped to the external SEARCH, keyed on an ~8km location cell matching ColdStartSearchRadius: :, plus :pricey for eatery levels 3-4, the only levels where Google applies a real price filter. Levels 0-2 share one field, removing two of every three eatery fan-outs. Visit needing no price segment now falls out of the rule instead of being an exception the newer categories were never added to. - A fresh marker is honoured at zero results, with a 24h window instead of 14 days so a newly built-out area is retried within a day. The marker stamp moves after a successful search, or a failed one would silence retries for that day. - Place Details, the dominant cost of a cold search at one call per place, is no longer re-bought for places already stored. The placesToUpdate hook was stubbed with a "placeholder" comment; it is now populated from a pipelined cache lookup, filtered before the DetailsLimit cap so the budget goes to places we lack, and gated on LastUpdatedAt so records cannot freeze. restoreCachedDetails fills gaps only, since the write path is a blind upsert and a skipped place would otherwise overwrite the record it was skipped for. - Read amplification: one pipelined fetch instead of a GET per member, a GEORADIUS COUNT bound, and no more req.Radius write-back. The union/merge-sort path and AllPriceLevels are deleted along with the price split. - removePlace ZREMmed "placeIDs:eatery:"+priceLevel — missing the "level" prefix writes used — and never touched the three newer buckets, so it deleted records and orphaned their geo members. It now iterates POI.AllPlaceCategories through the shared encoder. GetPlaceCountByCategory had the same bug, which is why /stats/places always reported zero eateries. GET /v1/migrate/union-eatery-buckets merges the legacy keys, dry-run by default. AGGREGATE MIN is mandatory: a GEO member's score IS its geohash, and the SUM that redis.ZStore defaults to would relocate any place present in two buckets. Run it BEFORE deploying — it is additive and invisible to the running code, whereas deploying first points every eatery read at a key that does not exist yet. Verified against local dev data: 111 members merged, 0 of 111 coordinates drifted, reads returning 49 places at every price level where they previously returned 0. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01WHNC45T5vRTKuEmifJANCG --- POI/categories.go | 89 +++++++-- POI/places.go | 29 ++- docs/migrations/collapse-eatery-buckets.md | 86 ++++++++ docs/migrations/reclassify-buckets.md | 33 ++- iowrappers/data_migrations.go | 163 +++++++++++++-- iowrappers/data_migrations_test.go | 51 +++++ iowrappers/eatery_bucket_union_test.go | 171 ++++++++++++++++ iowrappers/maps_client.go | 33 +++ iowrappers/nearby_search.go | 73 +++++-- iowrappers/nearby_search_keys_test.go | 71 +++---- iowrappers/nearby_search_test.go | 189 +++++++++++++++++- iowrappers/poi_searcher.go | 96 +++++++-- iowrappers/poi_searcher_cache_test.go | 109 ++++++++++ iowrappers/redis_client.go | 173 ++++++++-------- iowrappers/redis_data_inspections.go | 12 +- planner/planner.go | 27 ++- test/place_category_test.go | 76 ++++++- .../brand_nearby_search_test.go | 12 +- .../redis_client_mocks/bucket_cleanup_test.go | 43 ++-- .../maps_last_search_time_test.go | 79 +++++++- test/redis_client_mocks/nearby_search_test.go | 55 +++-- 21 files changed, 1401 insertions(+), 269 deletions(-) create mode 100644 docs/migrations/collapse-eatery-buckets.md create mode 100644 iowrappers/eatery_bucket_union_test.go create mode 100644 iowrappers/poi_searcher_cache_test.go diff --git a/POI/categories.go b/POI/categories.go index e8c29f260..be8364ea7 100644 --- a/POI/categories.go +++ b/POI/categories.go @@ -2,6 +2,7 @@ package POI import ( "fmt" + "math" "strings" ) @@ -107,16 +108,29 @@ func GetPlaceTypes(placeCat PlaceCategory) (placeTypes []LocationType) { return } +// AllPlaceCategories enumerates every place category. It is the single source of truth for +// "what categories exist": ParsePlaceCategory validates against it, and callers that must touch +// every category's geo bucket (e.g. deleting a place that may be filed under several) iterate +// it rather than hardcoding a subset — which is how Shopping, Lodging, and Wellness came to be +// missed by cleanup paths written before they existed. +var AllPlaceCategories = []PlaceCategory{ + PlaceCategoryVisit, + PlaceCategoryEatery, + PlaceCategoryShopping, + PlaceCategoryLodging, + PlaceCategoryWellness, +} + // ParsePlaceCategory converts a category string (e.g. from an API request) into a known // PlaceCategory, reporting whether it matched. Matching is exact against the canonical // category names ("Eatery", "Shopping", "Lodging", "Wellness", "Visit"). func ParsePlaceCategory(s string) (PlaceCategory, bool) { - switch PlaceCategory(s) { - case PlaceCategoryVisit, PlaceCategoryEatery, PlaceCategoryShopping, PlaceCategoryLodging, PlaceCategoryWellness: - return PlaceCategory(s), true - default: - return PlaceCategory(""), false + for _, cat := range AllPlaceCategories { + if PlaceCategory(s) == cat { + return cat, true + } } + return PlaceCategory(""), false } // umbrellaLocationTypes are Google's generic feature types that describe almost @@ -175,15 +189,64 @@ func PriceyEatery(placeCategory PlaceCategory, priceLevel PriceLevel) bool { return (placeCategory == PlaceCategoryEatery) && (priceLevel >= PriceLevelThree) } -// EncodeNearbySearchRedisKey generates a Redis Key for Redis nearby search with place category and price info -// The key includes the price level info for eatery and no price info for visit -func EncodeNearbySearchRedisKey(placeCategory PlaceCategory, level PriceLevel) string { - keys := []string{"placeIDs", strings.ToLower(string(placeCategory))} - // add price levels for eatery category - if placeCategory == PlaceCategoryEatery { - keys = append(keys, fmt.Sprintf("level%d", level)) +// EncodeNearbySearchRedisKey generates the Redis geo-index key for a category's nearby search. +// +// One bucket per category, with no price segment. Eateries used to be split into +// placeIDs:eatery:level0..4 keyed on each place's own price level, which fragmented the index +// for no benefit: Google omits price_level for most places (so they collapsed into level0) and +// only accepts a price filter at level >= 3, so searches for levels 0-2 were identical yet each +// read back a fifth of the data. Callers that care about price already filter after the read +// (matching.filterPlacesOnPriceLevel). Redis GEO is a sorted set scored by 52-bit geohash and +// GEORADIUS probes 9 geohash cells at O(log N + M), so one bucket holds millions of members +// without degrading — which is how placeIDs:visit has always worked. +func EncodeNearbySearchRedisKey(placeCategory PlaceCategory) string { + return strings.Join([]string{"placeIDs", strings.ToLower(string(placeCategory))}, ":") +} + +// searchCellDegrees sizes the freshness grid to iowrappers.ColdStartSearchRadius (~8 km), the +// area one cold external search actually populates. A fixed-degree grid narrows in meters as +// latitude rises, which only shrinks cells — erring toward an extra cold search, never toward +// claiming coverage we do not have. +const searchCellDegrees = 0.072 + +// EncodeSearchCell quantizes coordinates to the freshness grid. This is a cache key, not a +// spatial index: it is never range-queried, so it needs no neighbor probing or Z-order +// ordering. The only property that matters is that a cell is no larger than the area a cold +// search populates. +func EncodeSearchCell(lat, lng float64) string { + return fmt.Sprintf("%d_%d", + int(math.Floor(lat/searchCellDegrees)), + int(math.Floor(lng/searchCellDegrees))) +} + +// EncodeLastSearchTimeField identifies the external search variant that last covered a cell: +// +// : the unfiltered search — every category, and eatery levels 0-2 +// :eatery:pricey the price-filtered 4x-radius search, N in {3,4} +// +// Note this is scoped to the SEARCH, not to the bucket. Levels 0-2 share one field because +// Google is issued an identical unfiltered request for all three, so two of every three +// fan-outs were redundant. Levels 3-4 keep their own field because PriceyEatery makes Google +// apply a real price filter at four times the radius: a fresh generic marker must not suppress +// that search, or expensive places beyond the generic search's reach are never fetched. +// +// It is keyed on a location cell rather than country/admin1/city because the buckets it guards +// are geo indexes read from arbitrary coordinates. A city name has no extent, so it cannot +// answer "did we populate the area this query covers?" — a request 20 km from a city centroid +// would read a marker claiming freshness over ground no search had reached. +func EncodeLastSearchTimeField(placeCategory PlaceCategory, level PriceLevel, lat, lng float64) string { + segments := []string{EncodeSearchCell(lat, lng), strings.ToLower(string(placeCategory))} + if PriceyEatery(placeCategory, level) { + segments = append(segments, fmt.Sprintf("pricey%d", level)) } - return strings.Join(keys, ":") + return strings.Join(segments, ":") +} + +// EncodeBrandLastSearchTimeField is the brand-search equivalent of EncodeLastSearchTimeField. +// Brand buckets are geo indexes read from precise coordinates too, so they need the same cell +// scoping. +func EncodeBrandLastSearchTimeField(keyword string, lat, lng float64) string { + return strings.Join([]string{EncodeSearchCell(lat, lng), "brand", NormalizeBrandKey(keyword)}, ":") } // NormalizeBrandKey converts a brand keyword into a stable slug used in Redis keys and diff --git a/POI/places.go b/POI/places.go index 960351126..b1000c959 100644 --- a/POI/places.go +++ b/POI/places.go @@ -144,11 +144,12 @@ const ( PriceLevelDefault = 2 ) -// AllPriceLevels enumerates the distinct price buckets an eatery can be filed -// under. Eateries are partitioned by price on write (EncodeNearbySearchRedisKey -// appends the level only for the Eatery category), so a merchant/category eatery -// search — which has no price preference — must union across all of these or it -// only sees one price tier. Other categories are single-bucket. +// AllPriceLevels enumerates every price level a place can carry. +// +// Eateries used to be partitioned across one geo bucket per level +// (placeIDs:eatery:level0..4); they no longer are, so this is not a list of buckets. Its +// remaining use is the migration that unions those retired keys into placeIDs:eatery, which +// needs to enumerate them. See EncodeNearbySearchRedisKey for why the split was collapsed. var AllPriceLevels = []PriceLevel{ PriceLevelZero, PriceLevelOne, PriceLevelTwo, PriceLevelThree, PriceLevelFour, } @@ -165,10 +166,26 @@ func (place *Place) GetStatus() BusinessStatus { return place.Status } +// DefaultOpeningHours is the placeholder CreatePlace writes for any weekday the source data left +// blank. Because it is always filled in, a stored place's Hours are never empty and their +// emptiness cannot be used to detect missing data — use HasRealOpeningHours instead. +const DefaultOpeningHours = "8:30 am – 9:30 pm" + func (place *Place) GetHour(day Weekday) string { return place.Hours[day] } +// HasRealOpeningHours reports whether any weekday carries hours that came from source data +// rather than the DefaultOpeningHours placeholder. +func (place *Place) HasRealOpeningHours() bool { + for day := DateMonday; day <= DateSunday; day++ { + if hour := place.GetHour(day); hour != "" && hour != DefaultOpeningHours { + return true + } + } + return false +} + // KnownClosedOnDay reports whether the place's cached hours explicitly mark it closed on // the given weekday, e.g. "Sunday: Closed". Places with unknown or default hours return // false — absence of data is not treated as closed. @@ -365,7 +382,7 @@ func CreatePlace(name, addr, formattedAddr, businessStatus string, locationType // set default for weekday = DateMonday; weekday <= DateSunday; weekday++ { if place.GetHour(weekday) == "" { - place.SetHour(weekday, "8:30 am – 9:30 pm") + place.SetHour(weekday, DefaultOpeningHours) } } diff --git a/docs/migrations/collapse-eatery-buckets.md b/docs/migrations/collapse-eatery-buckets.md new file mode 100644 index 000000000..069d86ab9 --- /dev/null +++ b/docs/migrations/collapse-eatery-buckets.md @@ -0,0 +1,86 @@ +# Migration: collapse the eatery price buckets + +One-time merge of `placeIDs:eatery:level0..4` into a single `placeIDs:eatery` geo index, so every +category has exactly one bucket the way `placeIDs:visit` always has. + +`GET /v1/migrate/union-eatery-buckets` — admin only, **dry-run unless `apply=true`**. + +## Run this BEFORE deploying + +The union is purely additive and invisible to the running code: it creates a key nothing reads +yet. Deploying first would instead point every eatery read at a key that does not exist and +trigger a global cold-search burst. + +``` +1. dry run → 2. apply → 3. deploy → 4. verify → 5. delete the level* keys +``` + +## Why the split is going away + +Eateries were filed under `placeIDs:eatery:level` keyed on each place's own price level, while +reads asked for the level the *caller* wanted. Three things made that lossy: + +- Google omits `price_level` for most places, so `res.PriceLevel == 0` and they nearly all landed + in `level0`. +- Google only accepts a price filter at level ≥ 3, so searches for levels 0/1/2 issued + **identical** requests — three cold fan-outs per area instead of one — then scattered the + results across five buckets and each read back its own fifth. +- Price selection was already happening downstream anyway, in + `matching.filterPlacesOnPriceLevel` (`planner/solver.go`). + +Redis GEO is a sorted set scored by 52-bit geohash and `GEORADIUS` probes 9 geohash cells at +O(log N + M), so one bucket holds millions of members without degrading. The split bought nothing +and cost 5× fragmentation. + +## `AGGREGATE MIN` is mandatory + +A GEO member's score **is** its 52-bit geohash. `redis.ZStore.Aggregate` defaults to `SUM`, which +would add the scores of any place present in two source buckets and silently relocate it — in +practice into the ocean. The migration passes `MIN`, which keeps a real geohash; a place's +coordinates are identical across buckets, so which one survives does not matter. + +`TestUnionEateryPriceBucketsPreservesCoordinates` pins this by seeding one place into two buckets +and asserting a 100 m `GEORADIUS` around its true coordinates still finds it. + +The target key is included in the union sources, so the migration is re-runnable and cannot drop +members that already-deployed code has written to the collapsed key. + +## Steps + +```bash +BASE=https://best-vacation-planner.herokuapp.com + +# 1. Dry run. Reports each source bucket's size, their total, and the exact resulting +# member count (computed by reading members — a dry run writes nothing). +curl -s -H "Authorization: Bearer $ADMIN_JWT" \ + "$BASE/v1/migrate/union-eatery-buckets" | jq + +# 2. Apply. expected_after and target_after must match. +curl -s -H "Authorization: Bearer $ADMIN_JWT" \ + "$BASE/v1/migrate/union-eatery-buckets?apply=true" | jq + +# 3. Deploy. + +# 4. Verify: a non-zero eatery count, which /stats/places could never report before +# (it built "placeIDs:eatery", a key nothing wrote). +curl -s -H "Authorization: Bearer $ADMIN_JWT" "$BASE/stats/places" | jq + +# Spot-check that scores survived, against the place's own stored coordinates: +redis-cli GEOPOS placeIDs:eatery "" +redis-cli GET "place_details:place_ID:" | jq '.Location' + +# 5. Only once the deploy is healthy: +redis-cli DEL placeIDs:eatery:level0 placeIDs:eatery:level1 placeIDs:eatery:level2 \ + placeIDs:eatery:level3 placeIDs:eatery:level4 +``` + +`expected_after` is the deduped union size, so it will be **lower** than `source_total` whenever a +place appears in more than one price bucket. That is the intended outcome, not data loss. + +## Marker fields are also changing + +The same change re-keys `MapsLastSearchTime` from `:::…` to +`:` (see [reclassify-buckets.md](reclassify-buckets.md#steps) for the format). +Old fields are simply never read again. Expect one cold search per occupied cell per category +after deploy; the geo buckets are untouched, so reads return the full member set immediately and +only the markers re-establish. `redis-cli DEL MapsLastSearchTime` afterwards is optional cleanup. diff --git a/docs/migrations/reclassify-buckets.md b/docs/migrations/reclassify-buckets.md index 90b2bb3bf..c22e91486 100644 --- a/docs/migrations/reclassify-buckets.md +++ b/docs/migrations/reclassify-buckets.md @@ -3,7 +3,13 @@ One-time cleanup for the `fast_food_restaurant` incident (#446). The legacy Nearby Search ignores an unknown `?type=` instead of erroring, so searches for two Places-API-(New)-only types returned prominence-ranked establishments that were then -stamped with the queried type — writing hotels into `placeIDs:eatery:level*`. +stamped with the queried type — writing hotels into the eatery geo bucket. + +> **Note:** this document predates the bucket collapse. Eateries were split across +> `placeIDs:eatery:level0..4` when the incident happened; they are now a single +> `placeIDs:eatery` bucket. See [collapse-eatery-buckets.md](collapse-eatery-buckets.md). +> The migration itself is unaffected — it resolves keys through +> `POI.EncodeNearbySearchRedisKey` — but the member counts below were measured pre-collapse. `GET /v1/migrate/reclassify-buckets?category=Eatery` — admin only, **dry-run unless `apply=true`**. Any other value of `apply` (absent, empty, `TRUE`, `1`) is a dry run. @@ -57,8 +63,12 @@ eatery buckets. The trip-planning path (`planner/solver.go`) does not reclassify remain reachable in generated plans. Broadening `GetPlaceCategory` to cover those types is tracked as follow-up work. -Separately, ~328 bucket members have no backing `place_details` record. Pre-existing and -unrelated; the migration skips them. +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 +`placeIDs:eatery:` — missing the `level` prefix the write path used — and never +touched the Shopping/Lodging/Wellness buckets at all, so it orphaned every member it meant to +remove. `RemovePlaces` now clears every category bucket through the shared encoder, so no new +orphans accumulate; the existing ones still need one `GET /v1/migrate/remove-places` pass. ## Steps @@ -78,9 +88,20 @@ Repeat per category as needed (`Shopping`, `Wellness`, `Lodging`). Reads are pip batches of 100 and the report carries `bucket_sizes`/`total_members`; the full Eatery scan completed in ~1.3s against production, well inside Heroku's 30s H12. -To force a cold search when spot-checking a city afterwards, drop its marker field -(format `::::`): +To force a cold search when spot-checking an area afterwards, drop its marker field. The format +is now `:`, where `` is the search coordinates quantized by +`POI.EncodeSearchCell` — `floor(lat/0.072)_floor(lng/0.072)`. Eatery price levels 3 and 4 add a +`:pricey` segment; nothing else carries a price segment. ```bash -redis-cli HDEL MapsLastSearchTime "united states:ca:los altos:eatery:0" +# Los Altos (37.3852, -122.1141) -> floor(37.3852/0.072)=519, floor(-122.1141/0.072)=-1697 +redis-cli HDEL MapsLastSearchTime "519_-1697:eatery" + +# or find the field for an area you have a request log line for — the debug log emits "cell" +redis-cli HKEYS MapsLastSearchTime | grep ':eatery' ``` + +⚠️ Marker fields written before the cell change used +`::::` and are no longer read by anything. +`redis-cli DEL MapsLastSearchTime` is safe cleanup once the new code is deployed — it only +forces one cold search per occupied cell per category, and the geo buckets are untouched. diff --git a/iowrappers/data_migrations.go b/iowrappers/data_migrations.go index a984165e7..703b90741 100644 --- a/iowrappers/data_migrations.go +++ b/iowrappers/data_migrations.go @@ -6,9 +6,9 @@ import ( "errors" "fmt" "reflect" - "strconv" "strings" "sync" + "time" "github.com/bobg/go-generics/set" "github.com/redis/go-redis/v9" @@ -77,13 +77,49 @@ func (r *RedisClient) removePlace(context context.Context, placeRedisKey string, Logger.Debugf("removing place %+v from Redis", place) *count++ - // remove keys from all categorized sorted lists in case a place belongs to multiple categories - _, _ = r.client.ZRem(context, "placeIDs:visit", placeID).Result() - _, _ = r.client.ZRem(context, "placeIDs:eatery:"+strconv.Itoa(int(place.PriceLevel)), placeID).Result() + // Remove the member from every category bucket, since a place can be filed under more than + // one. This used to hardcode "placeIDs:visit" plus "placeIDs:eatery:"+priceLevel — which + // produced "placeIDs:eatery:2" while the write path used "placeIDs:eatery:level2", and never + // touched the Shopping/Lodging/Wellness buckets at all. The result was a deleted + // place_details record with its geo members left behind: orphans that count toward the + // MinNumResults radius gate and then resolve to nothing on read. Going through the shared + // encoder is what stops the two sides drifting again. + for _, cat := range POI.AllPlaceCategories { + if _, err := r.client.ZRem(context, POI.EncodeNearbySearchRedisKey(cat), placeID).Result(); err != nil { + return fmt.Errorf("removing place %s from bucket %s: %w", placeID, POI.EncodeNearbySearchRedisKey(cat), err) + } + } return r.RemoveKeys(context, []string{placeRedisKey}) } +// detailsSourcedFields are the stored-record fields that only a Place Details call can supply. +// +// URL is the whole list on purpose. Opening hours look like the obvious signal but cannot be +// used: POI.CreatePlace backfills every missing weekday with a default string +// ("8:30 am – 9:30 pm"), so hours are never empty on a stored record and the check would always +// pass. FormattedAddress is also unusable because the Nearby Search response carries one of its +// own. URL has exactly one source — urlMap, populated only from a Details result — so a +// non-empty URL is proof a Details call has landed on this record. +var detailsSourcedFields = []PlaceDetailsFields{PlaceDetailsFieldURL} + +// placeDetailsAreCurrent reports whether a stored record can stand in for a Place Details call, +// so the call can be skipped. Requires both that a Details call has populated the record and +// that it is recent: skipping on mere existence would freeze a record permanently, since the +// external-search refresh is the only thing that ever rewrites it. +func placeDetailsAreCurrent(place POI.Place, now time.Time) bool { + if !isPlaceDetailsValid(place, detailsSourcedFields) { + return false + } + lastUpdated, err := time.Parse(time.RFC3339, place.LastUpdatedAt) + if err != nil { + // Records written before LastUpdatedAt was populated, or with an unparsable value, + // cannot be aged — refresh them rather than trusting them forever. + return false + } + return now.Sub(lastUpdated) <= PlaceDetailsRefreshDuration +} + func isPlaceDetailsValid(place POI.Place, nonEmptyFields []PlaceDetailsFields) bool { for _, field := range nonEmptyFields { switch field { @@ -225,6 +261,113 @@ func (r *RedisClient) AddGeoLocation(ctx context.Context, key string, place POI. return err } +// EateryBucketUnionReport describes a run of UnionEateryPriceBucketsIntoCategoryBucket. +// ExpectedAfter is computed by reading members rather than by writing, so a dry run states the +// exact resulting size without touching anything. +type EateryBucketUnionReport struct { + SourceKeys []string `json:"source_keys"` + SourceSizes map[string]int64 `json:"source_sizes"` + SourceTotal int64 `json:"source_total"` + TargetKey string `json:"target_key"` + TargetBefore int64 `json:"target_before"` + ExpectedAfter int64 `json:"expected_after"` + TargetAfter int64 `json:"target_after"` +} + +// UnionEateryPriceBucketsIntoCategoryBucket merges the legacy per-price eatery geo indexes +// (placeIDs:eatery:level0..4) into the single placeIDs:eatery bucket that +// POI.EncodeNearbySearchRedisKey now names. +// +// Run this BEFORE deploying the code that reads the collapsed key. It is purely additive and +// invisible to the running code, whereas deploying first would point every eatery read at a key +// that does not exist yet and trigger a global cold-search burst. +// +// The legacy key format is spelled out literally here on purpose: the encoder no longer emits +// it, and a migration is the one place a retired format belongs. +// +// AGGREGATE MIN is mandatory. redis.ZStore.Aggregate defaults to SUM, and a GEO member's score +// IS its 52-bit geohash — summing the scores of a place that appears in two source buckets +// would silently relocate it, in our case to somewhere in the ocean. MIN keeps a real geohash, +// and since a place's coordinates are identical across buckets, which one survives is +// immaterial. +// +// The target is included in the union sources so the migration is re-runnable and cannot drop +// members that new code has already written to the collapsed key. +func (r *RedisClient) UnionEateryPriceBucketsIntoCategoryBucket(ctx context.Context, dryRun bool) (EateryBucketUnionReport, error) { + target := POI.EncodeNearbySearchRedisKey(POI.PlaceCategoryEatery) + sources := make([]string, 0, len(POI.AllPriceLevels)) + for _, level := range POI.AllPriceLevels { + sources = append(sources, fmt.Sprintf("%s:level%d", target, level)) + } + + report := EateryBucketUnionReport{ + SourceKeys: sources, + SourceSizes: make(map[string]int64, len(sources)), + TargetKey: target, + } + + distinct := set.Of[string]{} + for _, key := range sources { + size, err := r.client.ZCard(ctx, key).Result() + if err != nil { + return report, fmt.Errorf("sizing legacy bucket %s: %w", key, err) + } + report.SourceSizes[key] = size + report.SourceTotal += size + + members, err := r.client.ZRange(ctx, key, 0, -1).Result() + if err != nil { + return report, fmt.Errorf("reading legacy bucket %s: %w", key, err) + } + distinct.Add(members...) + } + + before, err := r.client.ZCard(ctx, target).Result() + if err != nil { + return report, fmt.Errorf("sizing target bucket %s: %w", target, err) + } + report.TargetBefore = before + + targetMembers, err := r.client.ZRange(ctx, target, 0, -1).Result() + if err != nil { + return report, fmt.Errorf("reading target bucket %s: %w", target, err) + } + distinct.Add(targetMembers...) + report.ExpectedAfter = int64(distinct.Len()) + + if dryRun { + report.TargetAfter = before + return report, nil + } + + unionKeys := make([]string, 0, len(sources)+1) + unionKeys = append(unionKeys, sources...) + unionKeys = append(unionKeys, target) + if _, err := r.client.ZUnionStore(ctx, target, &redis.ZStore{ + Keys: unionKeys, + Aggregate: "MIN", + }).Result(); err != nil { + return report, fmt.Errorf("unioning legacy eatery buckets into %s: %w", target, err) + } + + after, err := r.client.ZCard(ctx, target).Result() + if err != nil { + return report, fmt.Errorf("re-sizing target bucket %s: %w", target, err) + } + report.TargetAfter = after + if after != report.ExpectedAfter { + Logger.Errorf("UnionEateryPriceBucketsIntoCategoryBucket: %s has %d members, expected %d", + target, after, report.ExpectedAfter) + } + return report, nil +} + +// UnionEateryPriceBucketsIntoCategoryBucket forwards to the RedisClient method so the admin +// handler can call it through the concrete PoiSearcher. +func (s *PoiSearcher) UnionEateryPriceBucketsIntoCategoryBucket(ctx context.Context, dryRun bool) (EateryBucketUnionReport, error) { + return s.redisClient.UnionEateryPriceBucketsIntoCategoryBucket(ctx, dryRun) +} + // RemoveMisclassifiedPlacesFromCategoryBuckets removes places from cat's geo buckets whose // PRIMARY Google type does not belong to cat. It repairs the fast_food_restaurant incident: // two Places-API-(New)-only types were searched against the legacy Nearby Search, which @@ -254,15 +397,9 @@ func (r *RedisClient) AddGeoLocation(ctx context.Context, key string, place POI. func (r *RedisClient) RemoveMisclassifiedPlacesFromCategoryBuckets(ctx context.Context, cat POI.PlaceCategory, dryRun bool) (BucketCleanupReport, error) { report := BucketCleanupReport{RemovedIDs: make([]string, 0), BucketSizes: make(map[string]int64)} - levels := []POI.PriceLevel{POI.PriceLevelDefault} - if cat == POI.PlaceCategoryEatery { - levels = POI.AllPriceLevels - } - - keys := make([]string, 0, len(levels)) - for _, level := range levels { - keys = append(keys, POI.EncodeNearbySearchRedisKey(cat, level)) - } + // One bucket per category since the eatery price split was collapsed; the loops below still + // take a slice so the shape survives if a category is ever partitioned again. + keys := []string{POI.EncodeNearbySearchRedisKey(cat)} // Size the job before doing it. The scan cost is linear in bucket membership and the // handler runs under the caller's request context, so an operator needs the member count diff --git a/iowrappers/data_migrations_test.go b/iowrappers/data_migrations_test.go index 7f2d6428f..5a3a1b3fd 100644 --- a/iowrappers/data_migrations_test.go +++ b/iowrappers/data_migrations_test.go @@ -119,3 +119,54 @@ func TestRemovePlaces(t *testing.T) { return } } + +// TestRemovePlacesClearsEveryCategoryBucket covers the orphan bug: removePlace used to ZREM a +// hardcoded "placeIDs:visit" plus "placeIDs:eatery:"+priceLevel — which produced +// "placeIDs:eatery:2" while writes used "placeIDs:eatery:level2" — and never touched the +// Shopping, Lodging, or Wellness buckets. The record was deleted while its geo members stayed +// behind, leaving members that count toward the radius gate and then resolve to nothing. +func TestRemovePlacesClearsEveryCategoryBucket(t *testing.T) { + RedisMockSvr, _ := miniredis.Run() + defer RedisMockSvr.Close() + + redisURL, _ := url.Parse("redis://" + RedisMockSvr.Addr()) + redisClient := CreateRedisClient(redisURL) + ctx := context.Background() + _ = CreateLogger() + + // One place filed under every category's bucket, as a place matching several searches would + // be. It has no URL, so the URL requirement below marks it for removal. + place := POI.Place{ + ID: "multi-bucket-1", + Name: "Everything Emporium", + LocationType: POI.LocationTypeStore, + Location: POI.Location{Latitude: 12.5635, Longitude: 14.7834}, + Photo: POI.PlacePhoto{Reference: "photo-ref"}, + } + if err := redisClient.SetPlace(ctx, place); err != nil { + t.Fatalf("SetPlace: %v", err) + } + for _, cat := range POI.AllPlaceCategories { + key := POI.EncodeNearbySearchRedisKey(cat) + if err := redisClient.AddGeoLocation(ctx, key, place); err != nil { + t.Fatalf("AddGeoLocation(%s): %v", key, err) + } + } + + if err := redisClient.RemovePlaces(ctx, []PlaceDetailsFields{PlaceDetailsFieldURL}); err != nil { + t.Fatalf("RemovePlaces: %v", err) + } + + for _, cat := range POI.AllPlaceCategories { + key := POI.EncodeNearbySearchRedisKey(cat) + members, err := redisClient.Get().ZRange(ctx, key, 0, -1).Result() + if err != nil { + t.Fatalf("ZRange(%s): %v", key, err) + } + for _, member := range members { + if member == place.ID { + t.Errorf("%s left behind in bucket %s as an orphan", place.ID, key) + } + } + } +} diff --git a/iowrappers/eatery_bucket_union_test.go b/iowrappers/eatery_bucket_union_test.go new file mode 100644 index 000000000..7f94c4d68 --- /dev/null +++ b/iowrappers/eatery_bucket_union_test.go @@ -0,0 +1,171 @@ +package iowrappers + +import ( + "context" + "fmt" + "net/url" + "testing" + + "github.com/alicebob/miniredis/v2" + "github.com/redis/go-redis/v9" + "github.com/weihesdlegend/Vacation-planner/POI" +) + +func unionTestClient(t *testing.T) (*RedisClient, 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()) + _ = CreateLogger() + return CreateRedisClient(redisURL), context.Background() +} + +func legacyEateryKey(level POI.PriceLevel) string { + return fmt.Sprintf("%s:level%d", POI.EncodeNearbySearchRedisKey(POI.PlaceCategoryEatery), level) +} + +func TestUnionEateryPriceBuckets(t *testing.T) { + r, ctx := unionTestClient(t) + + // one place per legacy price bucket, at distinct coordinates + seeded := map[POI.PriceLevel]POI.Place{ + POI.PriceLevelZero: placeAt("lvl0", 37.7749, -122.4194), + POI.PriceLevelOne: placeAt("lvl1", 37.7750, -122.4195), + POI.PriceLevelTwo: placeAt("lvl2", 37.7751, -122.4196), + POI.PriceLevelThree: placeAt("lvl3", 37.7752, -122.4197), + POI.PriceLevelFour: placeAt("lvl4", 37.7753, -122.4198), + } + for level, place := range seeded { + if err := r.AddGeoLocation(ctx, legacyEateryKey(level), place); err != nil { + t.Fatalf("AddGeoLocation(%s): %v", legacyEateryKey(level), err) + } + } + + t.Run("a dry run reports the outcome without writing", func(t *testing.T) { + report, err := r.UnionEateryPriceBucketsIntoCategoryBucket(ctx, true) + if err != nil { + t.Fatalf("dry run: %v", err) + } + if report.SourceTotal != 5 { + t.Errorf("SourceTotal = %d, want 5", report.SourceTotal) + } + if report.ExpectedAfter != 5 { + t.Errorf("ExpectedAfter = %d, want 5", report.ExpectedAfter) + } + if report.TargetAfter != 0 { + t.Errorf("a dry run wrote to the target: TargetAfter = %d, want 0", report.TargetAfter) + } + count, err := r.Get().ZCard(ctx, report.TargetKey).Result() + if err != nil { + t.Fatalf("ZCard: %v", err) + } + if count != 0 { + t.Errorf("target has %d members after a dry run, want 0", count) + } + }) + + t.Run("apply merges every legacy bucket", func(t *testing.T) { + report, err := r.UnionEateryPriceBucketsIntoCategoryBucket(ctx, false) + if err != nil { + t.Fatalf("apply: %v", err) + } + if report.TargetAfter != 5 { + t.Errorf("TargetAfter = %d, want 5 (report: %+v)", report.TargetAfter, report) + } + + members, err := r.Get().ZRange(ctx, report.TargetKey, 0, -1).Result() + if err != nil { + t.Fatalf("ZRange: %v", err) + } + got := make(map[string]bool, len(members)) + for _, m := range members { + got[m] = true + } + for _, place := range seeded { + if !got[place.ID] { + t.Errorf("%s missing from the merged bucket", place.ID) + } + } + }) +} + +// TestUnionEateryPriceBucketsPreservesCoordinates is the AGGREGATE MIN guard. A GEO member's +// score IS its 52-bit geohash, so the SUM that redis.ZStore.Aggregate defaults to would add the +// scores of a place present in two source buckets and relocate it — in practice to the middle of +// the ocean. The place below is seeded into two buckets specifically to exercise that path. +func TestUnionEateryPriceBucketsPreservesCoordinates(t *testing.T) { + r, ctx := unionTestClient(t) + + const lat, lng = 37.7749, -122.4194 + duplicated := placeAt("in-two-buckets", lat, lng) + for _, level := range []POI.PriceLevel{POI.PriceLevelZero, POI.PriceLevelTwo} { + if err := r.AddGeoLocation(ctx, legacyEateryKey(level), duplicated); err != nil { + t.Fatalf("AddGeoLocation: %v", err) + } + } + + report, err := r.UnionEateryPriceBucketsIntoCategoryBucket(ctx, false) + if err != nil { + t.Fatalf("apply: %v", err) + } + if report.TargetAfter != 1 { + t.Fatalf("TargetAfter = %d, want 1 (the duplicate must merge to one member)", report.TargetAfter) + } + + // A tight radius around the true coordinates finds the member only if its score survived as a + // real geohash. Under SUM the score would be roughly doubled and this would return nothing. + found, err := r.Get().GeoRadius(ctx, report.TargetKey, lng, lat, &redis.GeoRadiusQuery{ + Radius: 100, + Unit: "m", + Sort: "ASC", + }).Result() + if err != nil { + t.Fatalf("GeoRadius: %v", err) + } + if len(found) != 1 || found[0].Name != duplicated.ID { + t.Errorf("a 100 m search around the true coordinates returned %+v; the geohash score did not survive the union", found) + } +} + +// TestUnionEateryPriceBucketsIsRerunnable pins that the target is included in the union sources, +// so members written by already-deployed code are not dropped and the migration can be repeated. +func TestUnionEateryPriceBucketsIsRerunnable(t *testing.T) { + r, ctx := unionTestClient(t) + target := POI.EncodeNearbySearchRedisKey(POI.PlaceCategoryEatery) + + legacy := placeAt("legacy", 37.7749, -122.4194) + if err := r.AddGeoLocation(ctx, legacyEateryKey(POI.PriceLevelOne), legacy); err != nil { + t.Fatalf("AddGeoLocation: %v", err) + } + // a place the new write path already put in the collapsed bucket + fresh := placeAt("already-collapsed", 37.7760, -122.4200) + if err := r.AddGeoLocation(ctx, target, fresh); err != nil { + t.Fatalf("AddGeoLocation: %v", err) + } + + for run := 1; run <= 2; run++ { + report, err := r.UnionEateryPriceBucketsIntoCategoryBucket(ctx, false) + if err != nil { + t.Fatalf("run %d: %v", run, err) + } + if report.TargetAfter != 2 { + t.Errorf("run %d: TargetAfter = %d, want 2 (report: %+v)", run, report.TargetAfter, report) + } + } + + members, err := r.Get().ZRange(ctx, target, 0, -1).Result() + if err != nil { + t.Fatalf("ZRange: %v", err) + } + got := make(map[string]bool, len(members)) + for _, m := range members { + got[m] = true + } + if !got[legacy.ID] || !got[fresh.ID] { + t.Errorf("members = %v, want both %s and %s", members, legacy.ID, fresh.ID) + } +} diff --git a/iowrappers/maps_client.go b/iowrappers/maps_client.go index 25ca3af55..1fb6d174f 100644 --- a/iowrappers/maps_client.go +++ b/iowrappers/maps_client.go @@ -22,11 +22,18 @@ type SearchClient interface { NearbySearch(context.Context, *PlaceSearchRequest) ([]POI.Place, error) // search nearby places in a category around a central location } +// CachedPlaceLookup resolves already-stored place records by ID, returning only the IDs it +// found. It is injected as a function rather than a RedisClient reference so MapsClient keeps +// no dependency on the cache and stays constructible without one; a nil lookup means every +// candidate gets a Place Details call, which is the behaviour before caching was consulted. +type CachedPlaceLookup func(ctx context.Context, placeIDs []string) (map[string]POI.Place, error) + type MapsClient struct { client *maps.Client apiKey string DetailedSearchFields []string apiSemaphore chan struct{} + cachedPlaces CachedPlaceLookup } func (c *MapsClient) SetDetailedSearchFields(fields []string) { @@ -35,6 +42,32 @@ func (c *MapsClient) SetDetailedSearchFields(fields []string) { strings.Join(c.DetailedSearchFields, ", ")) } +// SetCachedPlaceLookup wires in the cache so a nearby search can skip buying Place Details for +// places it already has. Place Details is the dominant cost of a cold search — one call per +// place — so on an area we have already covered this takes that spend to near zero. +func (c *MapsClient) SetCachedPlaceLookup(lookup CachedPlaceLookup) { + c.cachedPlaces = lookup +} + +// lookupCachedPlaces resolves the stored records for a Nearby Search response in one round +// trip. A lookup failure is not fatal: it only means details we may already have get bought +// again, so the search continues with an empty result. +func (c *MapsClient) lookupCachedPlaces(ctx context.Context, results []maps.PlacesSearchResult) map[string]POI.Place { + if c.cachedPlaces == nil || len(results) == 0 { + return nil + } + placeIDs := make([]string, 0, len(results)) + for _, res := range results { + placeIDs = append(placeIDs, res.PlaceID) + } + cached, err := c.cachedPlaces(ctx, placeIDs) + if err != nil { + Logger.Debugf("cached place lookup failed, falling back to a full Place Details pass: %v", err) + return nil + } + return cached +} + // CreateMapsClient is a factory method for MapsClient func CreateMapsClient(apiKey string) *MapsClient { mapsClient, err := maps.NewClient(maps.WithAPIKey(apiKey)) diff --git a/iowrappers/nearby_search.go b/iowrappers/nearby_search.go index dd92c68cc..9f04632a0 100644 --- a/iowrappers/nearby_search.go +++ b/iowrappers/nearby_search.go @@ -53,13 +53,6 @@ type PlaceSearchRequest struct { // DetailsLimit caps how many places get the expensive Place Details API call, chosen // by proximity to the request location. Zero means no cap (previous behavior). DetailsLimit int - - // AllPriceLevels, for an Eatery search with no price preference (merchant / - // category endpoint), unions every price bucket on read. Eateries are - // partitioned by price in the geo index, so without this a category read only - // returns the single PriceLevel bucket named by the request. No effect on - // non-Eatery categories (they are not price-partitioned) or keyword searches. - AllPriceLevels bool } // MatchesBrandName reports whether a place name matches a brand keyword after normalization, @@ -239,19 +232,25 @@ outer: processingStartTime := time.Now() searchResp := fetched[i].resp + // What we already hold for these places, in one round trip. Place Details is the + // dominant cost of a cold search (one call per place), so this is what keeps a + // re-search over already-covered ground from re-buying all of it. + cached := c.lookupCachedPlaces(ctx, searchResp.Results) + // places for Google Maps place details search (https://developers.google.com/maps/documentation/places/web-service/details) // the original purpose of doing a details search is getting opening hours info // later on we added more fields of interest as specified in the config/config.yaml file - placeIdMap := selectPlacesForDetails(request, &searchResp, &detailsBudget) + placeIdMap := selectPlacesForDetails(request, &searchResp, &detailsBudget, cached, processingStartTime) - // placeholder for filtering places that do no need updates placesToUpdate := set.Of[string]{} for _, placeId := range placeIdMap { placesToUpdate.Add(placeId) } searchDuration := c.searchPlaceDetails(ctx, placeIdMap, processingStartTime, &searchResp, summaryMap, microAddrMap, urlMap, placesToUpdate) - *places = append(*places, parsePlacesSearchResponse(searchResp, placeType, microAddrMap, placeMap, urlMap, summaryMap)...) + parsed := parsePlacesSearchResponse(searchResp, placeType, microAddrMap, placeMap, urlMap, summaryMap) + restoreCachedDetails(parsed, cached) + *places = append(*places, parsed...) totalPlaceCount += uint(len(searchResp.Results)) placeCountPerPlaceType[placeType] += len(searchResp.Results) nextPageTokenMap[placeType] = searchResp.NextPageToken @@ -281,9 +280,13 @@ outer: // selectPlacesForDetails picks the search results worth a Place Details API call: places // missing opening hours, excluding results a strict brand-name match would later drop -// (details on those are wasted spend), and — when the request sets DetailsLimit — capped -// to the remaining budget by proximity to the request location. -func selectPlacesForDetails(request *PlaceSearchRequest, searchResp *maps.PlacesSearchResponse, detailsBudget *int) map[int]string { +// (details on those are wasted spend), excluding places whose stored record already carries +// current Details data, and — when the request sets DetailsLimit — capped to the remaining +// budget by proximity to the request location. +// +// The cache check runs BEFORE the budget cap, not after, so a limited budget is spent entirely +// on places we do not already have instead of being consumed by ones we do. +func selectPlacesForDetails(request *PlaceSearchRequest, searchResp *maps.PlacesSearchResponse, detailsBudget *int, cached map[string]POI.Place, now time.Time) map[int]string { type candidate struct { idx int dist float64 @@ -296,6 +299,9 @@ func selectPlacesForDetails(request *PlaceSearchRequest, searchResp *maps.Places if request.Keyword != "" && request.StrictNameMatch && !MatchesBrandName(res.Name, request.Keyword) { continue } + if place, ok := cached[res.PlaceID]; ok && placeDetailsAreCurrent(place, now) { + continue + } dist := utils.HaversineDist( []float64{request.Location.Latitude, request.Location.Longitude}, []float64{res.Geometry.Location.Lat, res.Geometry.Location.Lng}) @@ -317,6 +323,47 @@ func selectPlacesForDetails(request *PlaceSearchRequest, searchResp *maps.Places return placeIdMap } +// restoreCachedDetails puts back the Details-sourced fields of places we already had stored, +// for any place that did not come away from this search with fresh ones. +// +// Required because the write path is a blind upsert: SetPlacesAddGeoLocations unconditionally +// Sets every place handed to it. Without this, a place whose Details call was skipped would be +// rebuilt from the bare Nearby result — CreatePlace backfilling default opening hours +// ("8:30 am – 9:30 pm") and leaving URL empty — and then overwrite the very record that caused +// it to be skipped, so the optimisation would quietly destroy the data it was exploiting. +// +// Restoring is per field and only ever fills a gap, never overwrites something this search +// obtained. That matters most for hours: a place whose hours arrived with the Nearby response is +// skipped for Details and so has no URL, and a blanket copy would replace those real hours with +// the stored record's DefaultOpeningHours placeholder. Hours cannot be tested for emptiness +// because CreatePlace always fills them, hence HasRealOpeningHours. +func restoreCachedDetails(places []POI.Place, cached map[string]POI.Place) { + if len(cached) == 0 { + return + } + for i := range places { + stored, ok := cached[places[i].ID] + if !ok { + continue + } + if places[i].URL == "" { + places[i].URL = stored.URL + } + if places[i].Summary == "" { + places[i].Summary = stored.Summary + } + if places[i].FormattedAddress == "" { + places[i].FormattedAddress = stored.FormattedAddress + } + if places[i].Address == (POI.Address{}) { + places[i].Address = stored.Address + } + if !places[i].HasRealOpeningHours() && stored.HasRealOpeningHours() { + places[i].Hours = stored.Hours + } + } +} + func (c *MapsClient) searchPlaceDetails( ctx context.Context, placeIdMap map[int]string, diff --git a/iowrappers/nearby_search_keys_test.go b/iowrappers/nearby_search_keys_test.go index 2dfc307f1..651da9e4f 100644 --- a/iowrappers/nearby_search_keys_test.go +++ b/iowrappers/nearby_search_keys_test.go @@ -6,60 +6,43 @@ import ( "github.com/weihesdlegend/Vacation-planner/POI" ) -func TestNearbySearchRedisKeys(t *testing.T) { - t.Run("eatery with AllPriceLevels unions every price bucket", func(t *testing.T) { - got := nearbySearchRedisKeys(&PlaceSearchRequest{ - PlaceCat: POI.PlaceCategoryEatery, - AllPriceLevels: true, - }) - want := []string{ - "placeIDs:eatery:level0", - "placeIDs:eatery:level1", - "placeIDs:eatery:level2", - "placeIDs:eatery:level3", - "placeIDs:eatery:level4", - } - if len(got) != len(want) { - t.Fatalf("got %d keys %v, want %d %v", len(got), got, len(want), want) +func TestNearbySearchRedisKey(t *testing.T) { + t.Run("every category reads one price-agnostic bucket", func(t *testing.T) { + want := map[POI.PlaceCategory]string{ + POI.PlaceCategoryVisit: "placeIDs:visit", + POI.PlaceCategoryEatery: "placeIDs:eatery", + POI.PlaceCategoryShopping: "placeIDs:shopping", + POI.PlaceCategoryLodging: "placeIDs:lodging", + POI.PlaceCategoryWellness: "placeIDs:wellness", } - for i := range want { - if got[i] != want[i] { - t.Errorf("key[%d] = %q, want %q", i, got[i], want[i]) + for cat, wantKey := range want { + if got := nearbySearchRedisKey(&PlaceSearchRequest{PlaceCat: cat}); got != wantKey { + t.Errorf("category %s: got %q, want %q", cat, got, wantKey) } } }) - t.Run("eatery without AllPriceLevels reads a single bucket", func(t *testing.T) { - got := nearbySearchRedisKeys(&PlaceSearchRequest{ - PlaceCat: POI.PlaceCategoryEatery, - PriceLevel: POI.PriceLevelTwo, - }) - if len(got) != 1 || got[0] != "placeIDs:eatery:level2" { - t.Errorf("got %v, want [placeIDs:eatery:level2]", got) - } - }) - - t.Run("non-eatery categories are single-bucket even with AllPriceLevels", func(t *testing.T) { - for _, cat := range []POI.PlaceCategory{ - POI.PlaceCategoryShopping, POI.PlaceCategoryLodging, POI.PlaceCategoryWellness, - } { - got := nearbySearchRedisKeys(&PlaceSearchRequest{PlaceCat: cat, AllPriceLevels: true}) - want := POI.EncodeNearbySearchRedisKey(cat, POI.PriceLevelZero) - if len(got) != 1 || got[0] != want { - t.Errorf("category %s: got %v, want [%s]", cat, got, want) + // The regression guard for the original defect: the eatery bucket key must not vary with + // price level, or a search at one level cannot see places stored at another. + t.Run("the bucket key is identical across every price level", func(t *testing.T) { + for _, cat := range POI.AllPlaceCategories { + want := nearbySearchRedisKey(&PlaceSearchRequest{PlaceCat: cat, PriceLevel: POI.PriceLevelZero}) + for _, level := range POI.AllPriceLevels { + got := nearbySearchRedisKey(&PlaceSearchRequest{PlaceCat: cat, PriceLevel: level}) + if got != want { + t.Errorf("category %s at price level %d: got %q, want %q", cat, level, got, want) + } } } }) - t.Run("keyword search uses the brand bucket, ignoring AllPriceLevels", func(t *testing.T) { - got := nearbySearchRedisKeys(&PlaceSearchRequest{ - Keyword: "Dunkin'", - PlaceCat: POI.PlaceCategoryEatery, - AllPriceLevels: true, + t.Run("keyword search uses the brand bucket", func(t *testing.T) { + got := nearbySearchRedisKey(&PlaceSearchRequest{ + Keyword: "Dunkin'", + PlaceCat: POI.PlaceCategoryEatery, }) - want := POI.EncodeBrandNearbySearchRedisKey("Dunkin'") - if len(got) != 1 || got[0] != want { - t.Errorf("got %v, want [%s]", got, want) + if want := POI.EncodeBrandNearbySearchRedisKey("Dunkin'"); got != want { + t.Errorf("got %q, want %q", got, want) } }) } diff --git a/iowrappers/nearby_search_test.go b/iowrappers/nearby_search_test.go index 2b76f2dca..e04f09a82 100644 --- a/iowrappers/nearby_search_test.go +++ b/iowrappers/nearby_search_test.go @@ -2,14 +2,14 @@ package iowrappers import ( "testing" + "time" "github.com/weihesdlegend/Vacation-planner/POI" "googlemaps.github.io/maps" ) -func TestSelectPlacesForDetails(t *testing.T) { - requestLocation := POI.Location{Latitude: 40.7484, Longitude: -73.9857} - resp := &maps.PlacesSearchResponse{ +func brandSearchResponse() *maps.PlacesSearchResponse { + return &maps.PlacesSearchResponse{ Results: []maps.PlacesSearchResult{ { // 0: has hours already, never needs details Name: "Dunkin'", @@ -34,6 +34,22 @@ func TestSelectPlacesForDetails(t *testing.T) { }, }, } +} + +// storedPlace builds a cached record. A non-empty URL is what marks a record as having been +// populated by a Place Details call — see detailsSourcedFields. +func storedPlace(id string, lastUpdated time.Time) POI.Place { + var p POI.Place + p.SetID(id) + p.SetURL("https://maps.google.com/?cid=" + id) + p.SetLastUpdatedAt(lastUpdated) + return p +} + +func TestSelectPlacesForDetails(t *testing.T) { + requestLocation := POI.Location{Latitude: 40.7484, Longitude: -73.9857} + resp := brandSearchResponse() + now := time.Now() request := &PlaceSearchRequest{ Keyword: "Dunkin'", @@ -43,7 +59,7 @@ func TestSelectPlacesForDetails(t *testing.T) { } budget := request.DetailsLimit - placeIdMap := selectPlacesForDetails(request, resp, &budget) + placeIdMap := selectPlacesForDetails(request, resp, &budget, nil, now) if len(placeIdMap) != 1 { t.Fatalf("expect 1 place selected for details, got %d: %v", len(placeIdMap), placeIdMap) @@ -56,7 +72,7 @@ func TestSelectPlacesForDetails(t *testing.T) { } // budget exhausted: subsequent pages select nothing - nextPage := selectPlacesForDetails(request, resp, &budget) + nextPage := selectPlacesForDetails(request, resp, &budget, nil, now) if len(nextPage) != 0 { t.Errorf("expect no selections once the budget is exhausted, got %v", nextPage) } @@ -65,8 +81,169 @@ func TestSelectPlacesForDetails(t *testing.T) { // (possibly sparse) result index uncapped := &PlaceSearchRequest{Keyword: "Dunkin'", StrictNameMatch: true, Location: requestLocation} unlimited := 0 - all := selectPlacesForDetails(uncapped, resp, &unlimited) + all := selectPlacesForDetails(uncapped, resp, &unlimited, nil, now) if len(all) != 2 || all[2] != "far" || all[3] != "near" { t.Errorf("expect indices 2 and 3 selected without a cap, got %v", all) } } + +// TestSelectPlacesForDetailsSkipsCachedPlaces covers the Place Details saving: Details is the +// dominant cost of a cold search, one call per place, and re-searching ground we already cover +// used to re-buy all of it. +func TestSelectPlacesForDetailsSkipsCachedPlaces(t *testing.T) { + requestLocation := POI.Location{Latitude: 40.7484, Longitude: -73.9857} + now := time.Now() + uncapped := func() (*PlaceSearchRequest, int) { + return &PlaceSearchRequest{Keyword: "Dunkin'", StrictNameMatch: true, Location: requestLocation}, 0 + } + + t.Run("nothing is selected when every candidate is already stored and current", func(t *testing.T) { + request, budget := uncapped() + cached := map[string]POI.Place{ + "far": storedPlace("far", now.Add(-time.Hour)), + "near": storedPlace("near", now.Add(-time.Hour)), + } + got := selectPlacesForDetails(request, brandSearchResponse(), &budget, cached, now) + if len(got) != 0 { + t.Errorf("expect no Place Details calls when everything is cached, got %v", got) + } + }) + + t.Run("a stored record with no Details data is still selected", func(t *testing.T) { + request, budget := uncapped() + var thin POI.Place + thin.SetID("near") + thin.SetLastUpdatedAt(now) + cached := map[string]POI.Place{"near": thin} + got := selectPlacesForDetails(request, brandSearchResponse(), &budget, cached, now) + if got[3] != "near" { + t.Errorf("expect a record with no URL to still need details, got %v", got) + } + }) + + // Without this, skipping on mere existence would freeze a record permanently: the external + // search refresh is the only thing that ever rewrites it. + t.Run("a stale stored record is refreshed", func(t *testing.T) { + request, budget := uncapped() + cached := map[string]POI.Place{ + "near": storedPlace("near", now.Add(-PlaceDetailsRefreshDuration-time.Hour)), + } + got := selectPlacesForDetails(request, brandSearchResponse(), &budget, cached, now) + if got[3] != "near" { + t.Errorf("expect a stale record to be refreshed, got %v", got) + } + }) + + t.Run("a record with an unparsable timestamp is refreshed", func(t *testing.T) { + request, budget := uncapped() + stored := storedPlace("near", now) + stored.LastUpdatedAt = "not-a-timestamp" + cached := map[string]POI.Place{"near": stored} + got := selectPlacesForDetails(request, brandSearchResponse(), &budget, cached, now) + if got[3] != "near" { + t.Errorf("expect a record that cannot be aged to be refreshed, got %v", got) + } + }) + + // The cache filter must run BEFORE the budget cap. "near" is the closest candidate, so under + // the reverse ordering it would win the single budgeted slot and "far" — the one place we do + // not have — would be dropped. + t.Run("a limited budget is spent on places we do not have", func(t *testing.T) { + request := &PlaceSearchRequest{ + Keyword: "Dunkin'", + StrictNameMatch: true, + Location: requestLocation, + DetailsLimit: 1, + } + budget := request.DetailsLimit + cached := map[string]POI.Place{"near": storedPlace("near", now.Add(-time.Hour))} + got := selectPlacesForDetails(request, brandSearchResponse(), &budget, cached, now) + if len(got) != 1 || got[2] != "far" { + t.Errorf("expect the budget spent on the uncached place 'far', got %v", got) + } + }) +} + +// TestRestoreCachedDetails pins the half of the optimisation that protects the data: the write +// path is a blind upsert, so a place whose Details call was skipped must not be written back +// stripped of the fields it was skipped because of. +func TestRestoreCachedDetails(t *testing.T) { + stored := storedPlace("near", time.Now()) + stored.Summary = "A donut shop." + stored.FormattedAddress = "1 Main St, New York, NY 10001, USA" + stored.Hours = [7]string{"Monday: 6AM-9PM", "", "", "", "", "", ""} + + t.Run("a place rebuilt without details recovers its stored fields", func(t *testing.T) { + // what parsePlacesSearchResponse produces for a skipped place: default hours, no URL + rebuilt := POI.Place{ID: "near", Hours: [7]string{"8:30 am – 9:30 pm"}} + places := []POI.Place{rebuilt} + + restoreCachedDetails(places, map[string]POI.Place{"near": stored}) + + if places[0].URL != stored.URL { + t.Errorf("URL = %q, want %q", places[0].URL, stored.URL) + } + if places[0].Summary != stored.Summary { + t.Errorf("Summary = %q, want %q", places[0].Summary, stored.Summary) + } + if places[0].FormattedAddress != stored.FormattedAddress { + t.Errorf("FormattedAddress = %q, want %q", places[0].FormattedAddress, stored.FormattedAddress) + } + if places[0].Hours != stored.Hours { + t.Errorf("Hours = %v, want %v — the default hours would have overwritten real ones", places[0].Hours, stored.Hours) + } + }) + + t.Run("a freshly detailed place keeps its new data", func(t *testing.T) { + fresh := POI.Place{ + ID: "near", + URL: "https://maps.google.com/?cid=fresh", + Summary: "Newly fetched.", + FormattedAddress: "2 Second St", + Hours: [7]string{"Monday: 5AM-10PM"}, + } + places := []POI.Place{fresh} + + restoreCachedDetails(places, map[string]POI.Place{"near": stored}) + + if places[0].URL != fresh.URL || places[0].Summary != fresh.Summary || places[0].Hours != fresh.Hours { + t.Errorf("a place with fresh details was overwritten from cache: %+v", places[0]) + } + }) + + t.Run("a place we have never stored is left alone", func(t *testing.T) { + places := []POI.Place{{ID: "unknown"}} + restoreCachedDetails(places, map[string]POI.Place{"near": stored}) + if places[0].URL != "" { + t.Errorf("URL = %q, want empty", places[0].URL) + } + }) + + // A place whose hours arrived with the Nearby response is skipped for Details and so has no + // URL. Restoring must not mistake that for "no data" and copy the stored record's placeholder + // hours over the real ones. + t.Run("real hours from the nearby response survive a stored placeholder", func(t *testing.T) { + var placeholderStored POI.Place + placeholderStored.SetID("near") + placeholderStored.SetURL("https://maps.google.com/?cid=near") + for day := POI.DateMonday; day <= POI.DateSunday; day++ { + placeholderStored.SetHour(day, POI.DefaultOpeningHours) + } + + fromNearby := POI.Place{ID: "near"} + for day := POI.DateMonday; day <= POI.DateSunday; day++ { + fromNearby.SetHour(day, "Monday: 7AM-11PM") + } + places := []POI.Place{fromNearby} + + restoreCachedDetails(places, map[string]POI.Place{"near": placeholderStored}) + + if places[0].Hours != fromNearby.Hours { + t.Errorf("Hours = %v, want %v — placeholder hours overwrote real ones", places[0].Hours, fromNearby.Hours) + } + // the URL is still a genuine gap and should be filled + if places[0].URL != placeholderStored.URL { + t.Errorf("URL = %q, want %q", places[0].URL, placeholderStored.URL) + } + }) +} diff --git a/iowrappers/poi_searcher.go b/iowrappers/poi_searcher.go index d17d58343..f07c53651 100644 --- a/iowrappers/poi_searcher.go +++ b/iowrappers/poi_searcher.go @@ -2,6 +2,7 @@ package iowrappers import ( "context" + "fmt" "net/url" "strings" "time" @@ -21,9 +22,18 @@ const ( MaxSearchRadius = 16000 // 10 miles, upper bound for radius requested by callers ColdStartSearchRadius = 8000 // 5 miles, radius used for external maps searches that populate the cache MinMapsResultRefreshDuration = time.Hour * 24 * 14 // 14 days - GoogleSearchHomePageURL = "https://www.google.com/" - ContextRequestIdKey = ContextKey("request_id") - ContextRequestUserId = ContextKey("user_id") + // MinEmptyResultRefreshDuration is the refresh window for a search that came back with + // nothing. Shorter than MinMapsResultRefreshDuration so an area that has just been built + // out is retried within a day, rather than being frozen for the full two weeks. + MinEmptyResultRefreshDuration = time.Hour * 24 // 1 day + // PlaceDetailsRefreshDuration is how long a stored place's Details-sourced fields (opening + // hours, formatted address, URL, editorial summary) are trusted before a cold search buys + // them again. Business status arrives with every Nearby Search, so closures are still caught + // by the Operational filter regardless of this window. + PlaceDetailsRefreshDuration = time.Hour * 24 * 90 // 90 days + GoogleSearchHomePageURL = "https://www.google.com/" + ContextRequestIdKey = ContextKey("request_id") + ContextRequestUserId = ContextKey("user_id") ) type PoiSearcher struct { @@ -61,6 +71,10 @@ func CreatePoiSearcher(mapsApiKey string, redisUrl *url.URL) *PoiSearcher { mapsClient: CreateMapsClient(mapsApiKey), redisClient: CreateRedisClient(redisUrl), } + // Let external searches consult the cache before buying Place Details for a place we already + // have. Wired here rather than in CreateMapsClient so the maps client keeps no dependency on + // Redis. + poiSearcher.mapsClient.SetCachedPlaceLookup(poiSearcher.redisClient.CachedPlaces) return &poiSearcher } @@ -139,11 +153,36 @@ func (s *PoiSearcher) ReverseGeocode(ctx context.Context, lat, lng float64) (*Ge return s.mapsClient.ReverseGeocode(ctx, lat, lng) } +// canServeFromCache decides whether cached places can satisfy a request without an external +// search. cachedCount is how many places the geo read returned, readErr its failure if any, +// markerMiss whether this cell has no freshness marker, and markerAge how long ago the marker +// says an external search last covered the cell. +// +// A fresh marker is honoured even at cachedCount == 0: it records that we already asked Google +// about this cell, including when the honest answer was "nothing here". The previous version +// additionally required cachedCount > 0, which left the marker unable to suppress anything — a +// bucket that read back zero fell through to Google, re-stamped the marker, and did the same +// again on the very next request, forever rather than once. That is what made sparse categories +// re-search on every single request. +// +// An empty result gets a shorter window than a populated one so an area that has just been built +// out is retried within a day instead of being frozen for the full refresh period. +func canServeFromCache(cachedCount int, readErr, markerMiss error, markerAge time.Duration) bool { + if readErr != nil || markerMiss != nil { + return false + } + refreshWindow := MinMapsResultRefreshDuration + if cachedCount == 0 { + refreshWindow = MinEmptyResultRefreshDuration + } + return markerAge <= refreshWindow +} + func (s *PoiSearcher) NearbySearch(context context.Context, request *PlaceSearchRequest) ([]POI.Place, error) { if err := s.processLocation(context, request); err != nil { return nil, err } - location := request.Location + lat, lng := request.Location.Latitude, request.Location.Longitude var savedPlaces, places []POI.Place var placesErr error @@ -152,24 +191,38 @@ func (s *PoiSearcher) NearbySearch(context context.Context, request *PlaceSearch Logger.Error(placesErr) } - Logger.Debugf("(PoiSearcher)NearbySearch: [request_id: %s] the number of results from redis is %d", context.Value(ContextRequestIdKey), len(savedPlaces)) - - // update last search time for the city + // when an external search last covered this location cell var lastSearchTime time.Time var lastSearchTimeMiss error if request.Keyword != "" { - lastSearchTime, lastSearchTimeMiss = s.redisClient.GetBrandMapsLastSearchTime(context, location, request.Keyword) + lastSearchTime, lastSearchTimeMiss = s.redisClient.GetBrandMapsLastSearchTime(context, lat, lng, request.Keyword) } else { - lastSearchTime, lastSearchTimeMiss = s.redisClient.GetMapsLastSearchTime(context, location, request.PlaceCat, request.PriceLevel) + lastSearchTime, lastSearchTimeMiss = s.redisClient.GetMapsLastSearchTime(context, lat, lng, request.PlaceCat, request.PriceLevel) } currentTime := time.Now() - isSavedPlacesFresh := func() bool { - return currentTime.Sub(lastSearchTime) <= MinMapsResultRefreshDuration && lastSearchTimeMiss == nil - } - // use place data from the database if it is fresh and at least one saved place satisfies the request - if isSavedPlacesFresh() && placesErr == nil && len(savedPlaces) > 0 { + markerAge := currentTime.Sub(lastSearchTime) + isFresh := canServeFromCache(len(savedPlaces), placesErr, lastSearchTimeMiss, markerAge) + + // Log everything needed to tell an empty bucket from a stale marker from a wrong key. The + // count alone cannot distinguish them. + Logger.Debugw("(PoiSearcher)NearbySearch: redis lookup", + "request_id", context.Value(ContextRequestIdKey), + "places_from_redis", len(savedPlaces), + "bucket_key", nearbySearchRedisKey(request), + "category", request.PlaceCat, + "price_level", request.PriceLevel, + "keyword", request.Keyword, + "search_center", fmt.Sprintf("%.4f,%.4f", lat, lng), + "cell", POI.EncodeSearchCell(lat, lng), + "radius", request.Radius, + "marker_age", markerAge, + "marker_missing", lastSearchTimeMiss != nil, + "fresh", isFresh, + ) + + if isFresh { Logger.Infof("(PoiSearcher)NearbySearch: [request_id: %s] Using Redis to fulfill request for location %+v with category %s, keyword %q and price level %d", context.Value(ContextRequestIdKey), request.Location, @@ -180,18 +233,21 @@ func (s *PoiSearcher) NearbySearch(context context.Context, request *PlaceSearch return places, nil } - if request.Keyword != "" { - utils.LogErrorWithLevel(s.redisClient.SetBrandMapsLastSearchTime(context, location, request.Keyword, currentTime.Format(time.RFC3339)), utils.LogError) - } else { - utils.LogErrorWithLevel(s.redisClient.SetMapsLastSearchTime(context, location, request.PlaceCat, request.PriceLevel, currentTime.Format(time.RFC3339)), utils.LogError) - } - // initiate a new external search newPlaces, searchErr := s.searchPlacesWithMaps(context, request) if searchErr != nil { return nil, searchErr } + // Stamp the marker only after a search that actually succeeded. Stamping before the call + // was harmless while empty results were ignored, but now that a fresh marker suppresses the + // call, a failed search would silence retries for a full day. + if request.Keyword != "" { + utils.LogErrorWithLevel(s.redisClient.SetBrandMapsLastSearchTime(context, lat, lng, request.Keyword, currentTime.Format(time.RFC3339)), utils.LogError) + } else { + utils.LogErrorWithLevel(s.redisClient.SetMapsLastSearchTime(context, lat, lng, request.PlaceCat, request.PriceLevel, currentTime.Format(time.RFC3339)), utils.LogError) + } + if request.Keyword != "" && request.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 diff --git a/iowrappers/poi_searcher_cache_test.go b/iowrappers/poi_searcher_cache_test.go new file mode 100644 index 000000000..d6fd9b754 --- /dev/null +++ b/iowrappers/poi_searcher_cache_test.go @@ -0,0 +1,109 @@ +package iowrappers + +import ( + "errors" + "testing" + "time" +) + +// TestCanServeFromCache covers the defect behind the repeated Google fan-outs: a cached result of +// zero could never be suppressed by the freshness marker, so every request for a sparse +// (category, area) re-ran the full external search indefinitely. +func TestCanServeFromCache(t *testing.T) { + readFailure := errors.New("redis read failed") + markerAbsent := errors.New("redis: nil") + + cases := []struct { + name string + cachedCount int + readErr error + markerMiss error + markerAge time.Duration + want bool + }{ + { + name: "empty result with a recent marker is served from cache", + cachedCount: 0, + markerAge: time.Hour, + want: true, + }, + { + name: "empty result past the empty-result window triggers a search", + cachedCount: 0, + markerAge: MinEmptyResultRefreshDuration + time.Hour, + want: false, + }, + { + name: "empty result exactly at the window boundary is still served", + cachedCount: 0, + markerAge: MinEmptyResultRefreshDuration, + want: true, + }, + { + // An empty result must NOT get the full populated window, or a genuinely new area + // stays empty for two weeks. + name: "empty result inside the populated window but past the empty one searches", + cachedCount: 0, + markerAge: MinMapsResultRefreshDuration - time.Hour, + want: false, + }, + { + name: "populated result within the refresh window is served from cache", + cachedCount: 12, + markerAge: MinMapsResultRefreshDuration - time.Hour, + want: true, + }, + { + name: "populated result past the refresh window triggers a search", + cachedCount: 12, + markerAge: MinMapsResultRefreshDuration + time.Hour, + want: false, + }, + { + name: "a missing marker always triggers a search", + cachedCount: 12, + markerMiss: markerAbsent, + markerAge: time.Hour, + want: false, + }, + { + name: "a failed read always triggers a search", + cachedCount: 12, + readErr: readFailure, + markerAge: time.Hour, + want: false, + }, + { + // Both signals bad: still one search, never a served-from-cache result. + name: "a failed read with a missing marker triggers a search", + cachedCount: 0, + readErr: readFailure, + markerMiss: markerAbsent, + markerAge: time.Hour, + want: false, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := canServeFromCache(tc.cachedCount, tc.readErr, tc.markerMiss, tc.markerAge) + if got != tc.want { + t.Errorf("canServeFromCache(%d, %v, %v, %v) = %v, want %v", + tc.cachedCount, tc.readErr, tc.markerMiss, tc.markerAge, got, tc.want) + } + }) + } +} + +// TestEmptyRefreshWindowIsShorter guards the relationship the two constants must keep. If the +// empty window ever reached the populated one, an area that returned nothing would be frozen for +// the full period; if it were zero, empty results would fan out on every request again. +func TestEmptyRefreshWindowIsShorter(t *testing.T) { + if MinEmptyResultRefreshDuration <= 0 { + t.Errorf("MinEmptyResultRefreshDuration = %v, must be positive or empty results re-search every request", MinEmptyResultRefreshDuration) + } + if MinEmptyResultRefreshDuration >= MinMapsResultRefreshDuration { + t.Errorf("MinEmptyResultRefreshDuration (%v) must be shorter than MinMapsResultRefreshDuration (%v)", + MinEmptyResultRefreshDuration, MinMapsResultRefreshDuration) + } +} diff --git a/iowrappers/redis_client.go b/iowrappers/redis_client.go index 4505e2e82..285e021e1 100644 --- a/iowrappers/redis_client.go +++ b/iowrappers/redis_client.go @@ -9,7 +9,6 @@ import ( "errors" "fmt" "net/url" - "sort" "strconv" "strings" "sync" @@ -140,12 +139,14 @@ func (r *RedisClient) setPlace(context context.Context, place POI.Place) error { return err } -func (r *RedisClient) GetMapsLastSearchTime(context context.Context, location POI.Location, category POI.PlaceCategory, priceLevel POI.PriceLevel) (lastSearchTime time.Time, err error) { - redisField := strings.ToLower(strings.Join([]string{location.Country, location.AdminAreaLevelOne, location.City, string(category), strconv.Itoa(int(priceLevel))}, ":")) - // for places in Visit category Google Maps do not provide pricing info, this is subject to change in the future - if category == POI.PlaceCategoryVisit { - redisField = strings.ToLower(strings.Join([]string{location.Country, location.AdminAreaLevelOne, location.City, string(category)}, ":")) - } +// 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 — +// see POI.EncodeLastSearchTimeField. There is no longer a per-category exception: the price +// segment is derived from PriceyEatery, so Visit needing none is a consequence of the rule +// rather than a special case that the newer categories were never added to. +func (r *RedisClient) GetMapsLastSearchTime(context context.Context, lat, lng float64, category POI.PlaceCategory, priceLevel POI.PriceLevel) (lastSearchTime time.Time, err error) { + redisField := POI.EncodeLastSearchTimeField(category, priceLevel, lat, lng) lst, cacheErr := r.client.HGet(context, MapsLastSearchTimeRedisKey, redisField).Result() if cacheErr != nil { err = cacheErr @@ -160,12 +161,8 @@ func (r *RedisClient) GetMapsLastSearchTime(context context.Context, location PO return } -func (r *RedisClient) SetMapsLastSearchTime(context context.Context, location POI.Location, category POI.PlaceCategory, priceLevel POI.PriceLevel, requestTime string) (err error) { - redisField := strings.ToLower(strings.Join([]string{location.Country, location.AdminAreaLevelOne, location.City, string(category), strconv.Itoa(int(priceLevel))}, ":")) - // for places in Visit category Google Maps do not provide pricing info, this is subject to change in the future - if category == POI.PlaceCategoryVisit { - redisField = strings.ToLower(strings.Join([]string{location.Country, location.AdminAreaLevelOne, location.City, string(category)}, ":")) - } +func (r *RedisClient) SetMapsLastSearchTime(context context.Context, lat, lng float64, category POI.PlaceCategory, priceLevel POI.PriceLevel, requestTime string) (err error) { + redisField := POI.EncodeLastSearchTimeField(category, priceLevel, lat, lng) _, err = r.client.HSet(context, MapsLastSearchTimeRedisKey, redisField, requestTime).Result() return } @@ -240,7 +237,7 @@ func (r *RedisClient) SetPlacesAddGeoLocations(c context.Context, places []POI.P Longitude: place.GetLocation().Longitude, } - redisKey := POI.EncodeNearbySearchRedisKey(placeCategory, place.PriceLevel) + redisKey := POI.EncodeNearbySearchRedisKey(placeCategory) pipe.GeoAdd(c, redisKey, geoLocation) json_, err := json.Marshal(place) @@ -292,13 +289,10 @@ func (r *RedisClient) SetPlacesAddGeoLocationsForBrand(c context.Context, keywor } } -func brandLastSearchTimeRedisField(location POI.Location, keyword string) string { - return strings.ToLower(strings.Join([]string{location.Country, location.AdminAreaLevelOne, location.City, "brand", POI.NormalizeBrandKey(keyword)}, ":")) -} - -// GetBrandMapsLastSearchTime returns the last time an external maps search ran for a brand keyword in a city -func (r *RedisClient) GetBrandMapsLastSearchTime(context context.Context, location POI.Location, keyword string) (lastSearchTime time.Time, err error) { - lst, cacheErr := r.client.HGet(context, MapsLastSearchTimeRedisKey, brandLastSearchTimeRedisField(location, keyword)).Result() +// GetBrandMapsLastSearchTime returns the last time an external maps search ran for a brand +// keyword in the location cell containing (lat, lng) +func (r *RedisClient) GetBrandMapsLastSearchTime(context context.Context, lat, lng float64, keyword string) (lastSearchTime time.Time, err error) { + lst, cacheErr := r.client.HGet(context, MapsLastSearchTimeRedisKey, POI.EncodeBrandLastSearchTimeField(keyword, lat, lng)).Result() if cacheErr != nil { err = cacheErr return @@ -312,9 +306,10 @@ func (r *RedisClient) GetBrandMapsLastSearchTime(context context.Context, locati return } -// SetBrandMapsLastSearchTime records the last time an external maps search ran for a brand keyword in a city -func (r *RedisClient) SetBrandMapsLastSearchTime(context context.Context, location POI.Location, keyword string, requestTime string) (err error) { - _, err = r.client.HSet(context, MapsLastSearchTimeRedisKey, brandLastSearchTimeRedisField(location, keyword), requestTime).Result() +// SetBrandMapsLastSearchTime records the last time an external maps search ran for a brand +// keyword in the location cell containing (lat, lng) +func (r *RedisClient) SetBrandMapsLastSearchTime(context context.Context, lat, lng float64, keyword string, requestTime string) (err error) { + _, err = r.client.HSet(context, MapsLastSearchTimeRedisKey, POI.EncodeBrandLastSearchTimeField(keyword, lat, lng), requestTime).Result() return } @@ -447,6 +442,26 @@ func (r *RedisClient) NearbyCities(ctx context.Context, lat, lng, radius float64 return nearbyCities, nil } +// CachedPlaces resolves stored place records for the given IDs in one round trip, omitting IDs +// with no usable record. Used by the maps client to avoid re-buying Place Details for places we +// already have; see MapsClient.SetCachedPlaceLookup. +func (r *RedisClient) CachedPlaces(ctx context.Context, placeIDs []string) (map[string]POI.Place, error) { + if len(placeIDs) == 0 { + return nil, nil + } + fetched, found, err := r.getPlacesPipelined(ctx, placeIDs) + if err != nil { + return nil, err + } + cached := make(map[string]POI.Place, len(placeIDs)) + for i, ok := range found { + if ok { + cached[placeIDs[i]] = fetched[i] + } + } + return cached, nil +} + // obtain place info from Redis based with key place_details:place_ID:placeID func (r *RedisClient) getPlace(context context.Context, placeId string) (place POI.Place, err error) { res, err := r.client.Get(context, PlaceDetailsRedisKeyPrefix+placeId).Result() @@ -458,27 +473,25 @@ func (r *RedisClient) getPlace(context context.Context, placeId string) (place P return } -// nearbySearchRedisKeys returns the geo-index keys a nearby search reads. Usually -// one key, but an Eatery search with AllPriceLevels set unions every price bucket -// (placeIDs:eatery:level0..4). Eateries are partitioned by price on write, so a -// category/merchant search that has no price preference must read them all or it -// only sees the tier named by req.PriceLevel (e.g. only price-unknown eateries). -func nearbySearchRedisKeys(req *PlaceSearchRequest) []string { +// nearbySearchRedisKey returns the single geo-index key a nearby search reads: the brand bucket +// for a keyword search, otherwise the category's one bucket. Eateries were previously split +// across placeIDs:eatery:level0..4, which forced callers with no price preference to union five +// keys; see POI.EncodeNearbySearchRedisKey for why that split is gone. +func nearbySearchRedisKey(req *PlaceSearchRequest) string { if req.Keyword != "" { - return []string{POI.EncodeBrandNearbySearchRedisKey(req.Keyword)} + return POI.EncodeBrandNearbySearchRedisKey(req.Keyword) } - if req.AllPriceLevels && req.PlaceCat == POI.PlaceCategoryEatery { - keys := make([]string, 0, len(POI.AllPriceLevels)) - for _, lvl := range POI.AllPriceLevels { - keys = append(keys, POI.EncodeNearbySearchRedisKey(req.PlaceCat, lvl)) - } - return keys - } - return []string{POI.EncodeNearbySearchRedisKey(req.PlaceCat, req.PriceLevel)} + return POI.EncodeNearbySearchRedisKey(req.PlaceCat) } +// geoCandidateMultiplier sizes the GEORADIUS COUNT above MinNumResults. Headroom is needed +// because a single bucket now holds every price level and callers filter on price after the +// read (matching.filterPlacesOnPriceLevel), so the nearest MinNumResults members are not +// necessarily MinNumResults usable results. +const geoCandidateMultiplier = 5 + func (r *RedisClient) NearbySearch(ctx context.Context, req *PlaceSearchRequest) ([]POI.Place, error) { - redisKeys := nearbySearchRedisKeys(req) + redisKey := nearbySearchRedisKey(req) requestLat, requestLng := req.Location.Latitude, req.Location.Longitude searchRadius := req.Radius @@ -486,49 +499,22 @@ func (r *RedisClient) NearbySearch(ctx context.Context, req *PlaceSearchRequest) searchRadius = MaxSearchRadius } - // The multi-key union path (WithDist merge-sort) is used ONLY when reading more - // than one bucket — i.e. an AllPriceLevels eatery category search. The common - // single-key path (plan generation, non-eatery categories, brand searches) is - // left exactly as it was: one GeoRadius call, its native ASC order, no re-sort. - singleKey := len(redisKeys) == 1 - var cachedQualifiedPlaces []redis.GeoLocation for searchRadius <= MaxSearchRadius { Logger.Debugf("[request_id: %s] Redis geo radius is using search radius of %d meters", ctx.Value(ContextRequestIdKey), searchRadius) - geoQuery := &redis.GeoRadiusQuery{ - Radius: float64(searchRadius), - Unit: "m", - Sort: "ASC", // sort ascending - WithDist: !singleKey, // only needed to merge-sort across multiple buckets - } - - if singleKey { - var err error - if cachedQualifiedPlaces, err = r.client.GeoRadius(ctx, redisKeys[0], requestLng, requestLat, geoQuery).Result(); err != nil { - return nil, err - } - } else { - // Union across buckets by member, keeping the nearest sighting of a place - // that appears in more than one, then merge-sort by distance. - merged := make(map[string]redis.GeoLocation) - for _, key := range redisKeys { - locs, err := r.client.GeoRadius(ctx, key, requestLng, requestLat, geoQuery).Result() - if err != nil { - return nil, err - } - for _, loc := range locs { - if existing, seen := merged[loc.Name]; !seen || loc.Dist < existing.Dist { - merged[loc.Name] = loc - } - } - } - cachedQualifiedPlaces = make([]redis.GeoLocation, 0, len(merged)) - for _, loc := range merged { - cachedQualifiedPlaces = append(cachedQualifiedPlaces, loc) - } - sort.SliceStable(cachedQualifiedPlaces, func(i, j int) bool { - return cachedQualifiedPlaces[i].Dist < cachedQualifiedPlaces[j].Dist - }) + var err error + cachedQualifiedPlaces, err = r.client.GeoRadius(ctx, redisKey, requestLng, requestLat, &redis.GeoRadiusQuery{ + Radius: float64(searchRadius), + Unit: "m", + Sort: "ASC", // sort ascending + // Bound the candidate set. Without this a wide radius in a dense area returns + // every member in range and we fetch a record for each, only for the caller to + // truncate to a handful. go-redis omits COUNT entirely when this is 0, so a + // caller that leaves MinNumResults unset is unbounded exactly as before. + Count: int(req.MinNumResults) * geoCandidateMultiplier, + }).Result() + if err != nil { + return nil, err } if len(cachedQualifiedPlaces) >= int(req.MinNumResults) { @@ -537,15 +523,32 @@ func (r *RedisClient) NearbySearch(ctx context.Context, req *PlaceSearchRequest) searchRadius *= 2 } - req.Radius = searchRadius + placeIDs := make([]string, len(cachedQualifiedPlaces)) + for i, placeInfo := range cachedQualifiedPlaces { + placeIDs[i] = placeInfo.Name + } - places := make([]POI.Place, 0) - for _, placeInfo := range cachedQualifiedPlaces { - if place, err := r.getPlace(ctx, placeInfo.Name); err == nil { - places = append(places, place) + // One pipelined round trip rather than a GET per member: a wide radius in a dense area + // returns hundreds of members, and serial round trips there dwarf the geo lookup itself. + fetched, found, err := r.getPlacesPipelined(ctx, placeIDs) + if err != nil { + return nil, err + } + places := make([]POI.Place, 0, len(fetched)) + for i, ok := range found { + if ok { + places = append(places, fetched[i]) } } + // A bucket member with no backing place_details record is an orphan. It still counts toward + // the MinNumResults radius gate above but resolves to nothing here, so a bucket full of + // orphans is indistinguishable from an empty one unless the gap is logged. + if orphans := len(placeIDs) - len(places); orphans > 0 { + Logger.Debugf("(RedisClient)NearbySearch: [request_id: %s] key %s had %d geo members with no place_details record", + ctx.Value(ContextRequestIdKey), redisKey, orphans) + } + if req.BusinessStatus == POI.Operational { totalPlacesCount := len(places) places = Filter(places, func(place POI.Place) bool { return place.Status == POI.Operational }) diff --git a/iowrappers/redis_data_inspections.go b/iowrappers/redis_data_inspections.go index 55a6fd318..402b94355 100644 --- a/iowrappers/redis_data_inspections.go +++ b/iowrappers/redis_data_inspections.go @@ -2,14 +2,12 @@ package iowrappers import ( "context" - "strings" "github.com/weihesdlegend/Vacation-planner/POI" ) const ( PlaceDetailsKeyPrefix = "place_details" - PlaceIDsKeyPrefix = "placeIDs" ) func (r *RedisClient) GetPlaceCountInRedis(context context.Context) (placeKeys []string, count int, err error) { @@ -41,10 +39,10 @@ func (r *RedisClient) GetCities(context context.Context) (map[string]string, err return geocodes, nil } +// GetPlaceCountByCategory returns how many places sit in a category's geo bucket. It goes +// through POI.EncodeNearbySearchRedisKey rather than assembling the key locally: the hand-rolled +// version produced "placeIDs:eatery" while the write path used "placeIDs:eatery:level", so +// this reported zero eateries no matter how many were stored. func (r *RedisClient) GetPlaceCountByCategory(context context.Context, category POI.PlaceCategory) (int64, error) { - redisKey := strings.Join([]string{PlaceIDsKeyPrefix, strings.ToLower(string(category))}, ":") - var count int64 - var err error - count, err = r.client.ZCard(context, redisKey).Result() - return count, err + return r.client.ZCard(context, POI.EncodeNearbySearchRedisKey(category)).Result() } diff --git a/planner/planner.go b/planner/planner.go index b94cfe8dc..22a6e70a5 100644 --- a/planner/planner.go +++ b/planner/planner.go @@ -303,6 +303,29 @@ func (p *MyPlanner) reclassifyBucketsMigrationHandler(ctx *gin.Context) { ctx.JSON(http.StatusOK, gin.H{"dry_run": dryRun, "category": category, "report": report}) } +// unionEateryBucketsMigrationHandler merges the legacy placeIDs:eatery:level0..4 geo indexes +// into the single placeIDs:eatery bucket. Run this BEFORE deploying the code that reads the +// collapsed key — it is additive and invisible to the running code, whereas deploying first +// points every eatery read at a key that does not exist yet. +// +// Usage: GET /v1/migrate/union-eatery-buckets +// +// GET /v1/migrate/union-eatery-buckets?apply=true +func (p *MyPlanner) unionEateryBucketsMigrationHandler(ctx *gin.Context) { + _, authenticationErr := p.UserAuthentication(ctx, user.LevelAdmin) + if authenticationErr != nil { + ctx.JSON(http.StatusUnauthorized, gin.H{"error": authenticationErr.Error()}) + return + } + dryRun := ctx.Query("apply") != "true" + report, err := p.Solver.Searcher.UnionEateryPriceBucketsIntoCategoryBucket(ctx.Request.Context(), dryRun) + if err != nil { + ctx.JSON(http.StatusInternalServerError, gin.H{"error": err.Error(), "partial_report": report}) + return + } + ctx.JSON(http.StatusOK, gin.H{"dry_run": dryRun, "report": report}) +} + func (p *MyPlanner) placeStatsHandler(ctx *gin.Context) { var placeCount int var err error @@ -1419,9 +1442,6 @@ func (p *MyPlanner) getNearbyPlacesByCategory(ctx *gin.Context) { // pool (limit) is widened for dedup — details cost stays ~today's. DetailsLimit: min(limit, 20), BusinessStatus: POI.Operational, - // Merchant search has no price preference: union all eatery price - // buckets so the food category isn't limited to one price tier. - AllPriceLevels: true, } result := nearbyPlacesByCategoryResult{Category: string(placeCat), Places: []POI.Place{}} places, searchErr := p.Solver.Searcher.NearbySearch(searchContext, searchReq) @@ -1719,6 +1739,7 @@ func (p *MyPlanner) SetupRouter(serverPort string) *http.Server { migrations.GET("/url", p.UrlMigrationHandler) migrations.GET("/remove-places", p.removePlacesMigrationHandler) migrations.GET("/reclassify-buckets", p.reclassifyBucketsMigrationHandler) + migrations.GET("/union-eatery-buckets", p.unionEateryBucketsMigrationHandler) } v1.GET("/blob_url", p.getBlobObjectURL) diff --git a/test/place_category_test.go b/test/place_category_test.go index 82c8b61bf..5beb93f5a 100644 --- a/test/place_category_test.go +++ b/test/place_category_test.go @@ -118,13 +118,9 @@ func TestParsePlaceCategory(t *testing.T) { // TestEncodeNearbySearchRedisKeyDistinct guards against two categories sharing a Redis geo // bucket, which would cross-contaminate their cached results. func TestEncodeNearbySearchRedisKeyDistinct(t *testing.T) { - categories := []POI.PlaceCategory{ - POI.PlaceCategoryVisit, POI.PlaceCategoryEatery, - POI.PlaceCategoryShopping, POI.PlaceCategoryLodging, POI.PlaceCategoryWellness, - } seen := make(map[string]POI.PlaceCategory) - for _, category := range categories { - key := POI.EncodeNearbySearchRedisKey(category, POI.PriceLevelDefault) + for _, category := range POI.AllPlaceCategories { + key := POI.EncodeNearbySearchRedisKey(category) if other, dup := seen[key]; dup { t.Errorf("categories %s and %s share Redis key %q", other, category, key) } @@ -132,6 +128,74 @@ func TestEncodeNearbySearchRedisKeyDistinct(t *testing.T) { } } +// TestEncodeLastSearchTimeFieldMatchesSearchVariant pins the marker's scoping rule. The original +// defect was a marker scoped differently from what it guarded: only Visit was special-cased to +// drop the price segment, so Shopping/Lodging/Wellness carried a price-scoped marker over a +// price-agnostic bucket. The rule now is that the field identifies the external SEARCH variant. +func TestEncodeLastSearchTimeFieldMatchesSearchVariant(t *testing.T) { + const lat, lng = 37.38, -122.11 + + t.Run("non-eatery categories never carry a price segment", func(t *testing.T) { + for _, cat := range POI.AllPlaceCategories { + if cat == POI.PlaceCategoryEatery { + continue + } + want := POI.EncodeLastSearchTimeField(cat, POI.PriceLevelZero, lat, lng) + for _, level := range POI.AllPriceLevels { + got := POI.EncodeLastSearchTimeField(cat, level, lat, lng) + if got != want { + t.Errorf("category %s at price level %d: got %q, want %q", cat, level, got, want) + } + } + } + }) + + // Levels 0-2 produce an identical, unfiltered Google request, so they must share one marker + // or two of every three fan-outs are redundant. + t.Run("eatery levels 0-2 share one field", func(t *testing.T) { + want := POI.EncodeLastSearchTimeField(POI.PlaceCategoryEatery, POI.PriceLevelZero, lat, lng) + for _, level := range []POI.PriceLevel{POI.PriceLevelZero, POI.PriceLevelOne, POI.PriceLevelTwo} { + if got := POI.EncodeLastSearchTimeField(POI.PlaceCategoryEatery, level, lat, lng); got != want { + t.Errorf("eatery level %d: got %q, want %q", level, got, want) + } + } + }) + + // PriceyEatery makes Google apply a real price filter at four times the radius, so a fresh + // generic marker must not suppress it. + t.Run("eatery levels 3 and 4 each get their own field", func(t *testing.T) { + generic := POI.EncodeLastSearchTimeField(POI.PlaceCategoryEatery, POI.PriceLevelZero, lat, lng) + three := POI.EncodeLastSearchTimeField(POI.PlaceCategoryEatery, POI.PriceLevelThree, lat, lng) + four := POI.EncodeLastSearchTimeField(POI.PlaceCategoryEatery, POI.PriceLevelFour, lat, lng) + for _, pair := range [][2]string{{generic, three}, {generic, four}, {three, four}} { + if pair[0] == pair[1] { + t.Errorf("fields must differ, both are %q", pair[0]) + } + } + }) +} + +func TestEncodeSearchCell(t *testing.T) { + const lat, lng = 37.38, -122.11 + base := POI.EncodeSearchCell(lat, lng) + + // ~1 km north: well inside a cell sized to the ~8 km cold-search radius, so a second request + // nearby must reuse the first one's freshness rather than re-searching. + if near := POI.EncodeSearchCell(lat+0.009, lng); near != base { + t.Errorf("a point ~1 km away landed in cell %q, want %q", near, base) + } + + // ~22 km north: beyond anything the first search populated, so it must be its own cell. This + // is the case a city-scoped marker got wrong — claiming coverage over ground no search reached. + if far := POI.EncodeSearchCell(lat+0.2, lng); far == base { + t.Errorf("a point ~22 km away shares cell %q", far) + } + + if crossed := POI.EncodeSearchCell(-lat, -lng); crossed == base { + t.Errorf("the opposite hemisphere shares cell %q", crossed) + } +} + func TestPrimaryLocationType(t *testing.T) { cases := []struct { name string diff --git a/test/redis_client_mocks/brand_nearby_search_test.go b/test/redis_client_mocks/brand_nearby_search_test.go index 3ea5d8f55..d6b7cc6f5 100644 --- a/test/redis_client_mocks/brand_nearby_search_test.go +++ b/test/redis_client_mocks/brand_nearby_search_test.go @@ -60,20 +60,26 @@ func TestBrandNearbySearch_shouldOnlyReturnPlacesFromBrandBucket(t *testing.T) { } func TestBrandMapsLastSearchTime_roundTrip(t *testing.T) { - location := POI.Location{City: "New York", AdminAreaLevelOne: "NY", Country: "USA"} + const lat, lng = 40.7128, -74.0060 // New York currentTime := time.Now() - if err := RedisClient.SetBrandMapsLastSearchTime(RedisContext, location, "Dunkin'", currentTime.Format(time.RFC3339)); err != nil { + if err := RedisClient.SetBrandMapsLastSearchTime(RedisContext, lat, lng, "Dunkin'", currentTime.Format(time.RFC3339)); err != nil { t.Fatal(err) } - gotLastSearchTime, err := RedisClient.GetBrandMapsLastSearchTime(RedisContext, location, "Dunkin'") + gotLastSearchTime, err := RedisClient.GetBrandMapsLastSearchTime(RedisContext, lat, lng, "Dunkin'") if err != nil { t.Fatal(err) } if gotLastSearchTime.Format(time.RFC3339) != currentTime.Format(time.RFC3339) { t.Errorf("expect last search time %v, got %v", currentTime, gotLastSearchTime) } + + // Brand buckets are geo indexes read from precise coordinates too, so their marker is + // cell-scoped for the same reason the category markers are. + if _, err := RedisClient.GetBrandMapsLastSearchTime(RedisContext, lat+0.2, lng, "Dunkin'"); err == nil { + t.Error("expected a brand marker miss ~22 km away, got a hit") + } } func TestMatchesBrandName(t *testing.T) { diff --git a/test/redis_client_mocks/bucket_cleanup_test.go b/test/redis_client_mocks/bucket_cleanup_test.go index d11ff5e09..bc522fda8 100644 --- a/test/redis_client_mocks/bucket_cleanup_test.go +++ b/test/redis_client_mocks/bucket_cleanup_test.go @@ -30,15 +30,13 @@ func resetBucketCleanupFixtures(t *testing.T) { if err := RedisClient.RemoveKeys(RedisContext, detailKeys); err != nil { t.Fatalf("RemoveKeys: %v", err) } - for _, lvl := range POI.AllPriceLevels { - key := POI.EncodeNearbySearchRedisKey(POI.PlaceCategoryEatery, lvl) - if !RedisMockSvr.Exists(key) { - continue - } - for _, id := range bucketCleanupFixtureIDs { - if _, err := RedisMockSvr.ZRem(key, id); err != nil && err != miniredis.ErrKeyNotFound { - t.Fatalf("ZRem(%s, %s): %v", key, id, err) - } + key := POI.EncodeNearbySearchRedisKey(POI.PlaceCategoryEatery) + if !RedisMockSvr.Exists(key) { + return + } + for _, id := range bucketCleanupFixtureIDs { + if _, err := RedisMockSvr.ZRem(key, id); err != nil && err != miniredis.ErrKeyNotFound { + t.Fatalf("ZRem(%s, %s): %v", key, id, err) } } } @@ -221,7 +219,7 @@ func TestRemoveMisclassifiedPlacesToleratesMissingRecords(t *testing.T) { // a geo-bucket member whose place record is deliberately never written orphan := newPlaceWithTypes("tt-orphan", "Vanished Diner", POI.LocationTypeRestaurant, nil) - key := POI.EncodeNearbySearchRedisKey(POI.PlaceCategoryEatery, orphan.PriceLevel) + key := POI.EncodeNearbySearchRedisKey(POI.PlaceCategoryEatery) if err := RedisClient.AddGeoLocation(RedisContext, key, orphan); err != nil { t.Fatalf("AddGeoLocation(%s): %v", key, err) } @@ -265,16 +263,13 @@ func TestRemoveMisclassifiedPlacesReportsBucketSizes(t *testing.T) { t.Fatalf("RemoveMisclassifiedPlacesFromCategoryBuckets error: %v", err) } - // one entry per eatery price bucket, whether or not the key exists yet - if len(report.BucketSizes) != len(POI.AllPriceLevels) { - t.Errorf("BucketSizes has %d entries, want %d: %+v", - len(report.BucketSizes), len(POI.AllPriceLevels), report.BucketSizes) - } - for _, lvl := range POI.AllPriceLevels { - key := POI.EncodeNearbySearchRedisKey(POI.PlaceCategoryEatery, lvl) - if _, ok := report.BucketSizes[key]; !ok { - t.Errorf("BucketSizes missing key %s: %+v", key, report.BucketSizes) - } + // one entry for the category's single bucket, whether or not the key exists yet + if len(report.BucketSizes) != 1 { + t.Errorf("BucketSizes has %d entries, want 1: %+v", len(report.BucketSizes), report.BucketSizes) + } + key := POI.EncodeNearbySearchRedisKey(POI.PlaceCategoryEatery) + if _, ok := report.BucketSizes[key]; !ok { + t.Errorf("BucketSizes missing key %s: %+v", key, report.BucketSizes) } if report.TotalMembers < 1 { t.Errorf("TotalMembers = %d, want at least the 1 seeded place", report.TotalMembers) @@ -306,7 +301,7 @@ func seedGeoBucket(t *testing.T, cat POI.PlaceCategory, place POI.Place) { if err := RedisClient.SetPlace(RedisContext, place); err != nil { t.Fatalf("SetPlace(%s): %v", place.GetID(), err) } - key := POI.EncodeNearbySearchRedisKey(cat, place.PriceLevel) + key := POI.EncodeNearbySearchRedisKey(cat) if err := RedisClient.AddGeoLocation(RedisContext, key, place); err != nil { t.Fatalf("AddGeoLocation(%s): %v", key, err) } @@ -315,12 +310,12 @@ func seedGeoBucket(t *testing.T, cat POI.PlaceCategory, place POI.Place) { func countInEateryBuckets(t *testing.T, placeID string) int { t.Helper() count := 0 - for _, lvl := range POI.AllPriceLevels { - key := POI.EncodeNearbySearchRedisKey(POI.PlaceCategoryEatery, lvl) + { + key := POI.EncodeNearbySearchRedisKey(POI.PlaceCategoryEatery) if RedisMockSvr.Exists(key) { members, err := RedisMockSvr.ZMembers(key) if err != nil { - continue + return count } for _, m := range members { if m == placeID { diff --git a/test/redis_client_mocks/maps_last_search_time_test.go b/test/redis_client_mocks/maps_last_search_time_test.go index 83844d723..a4385bc25 100644 --- a/test/redis_client_mocks/maps_last_search_time_test.go +++ b/test/redis_client_mocks/maps_last_search_time_test.go @@ -13,7 +13,8 @@ func TestRedisClient_GetMapsLastSearchTime(t *testing.T) { currentTime := time.Now() type args struct { context context.Context - location POI.Location + lat float64 + lng float64 category POI.PlaceCategory priceLevel POI.PriceLevel timeToSave time.Time @@ -28,7 +29,8 @@ func TestRedisClient_GetMapsLastSearchTime(t *testing.T) { name: "Redis client should retrieve Maps last search time", args: args{ context: context.Background(), - location: POI.Location{City: "San Francisco", AdminAreaLevelOne: "CA", Country: "USA"}, + lat: 37.7749, + lng: -122.4194, category: POI.PlaceCategoryEatery, priceLevel: POI.PriceLevelFour, timeToSave: currentTime, @@ -40,18 +42,83 @@ func TestRedisClient_GetMapsLastSearchTime(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { r := RedisClient - err := r.SetMapsLastSearchTime(tt.args.context, tt.args.location, tt.args.category, tt.args.priceLevel, tt.args.timeToSave.Format(time.RFC3339)) + err := r.SetMapsLastSearchTime(tt.args.context, tt.args.lat, tt.args.lng, tt.args.category, tt.args.priceLevel, tt.args.timeToSave.Format(time.RFC3339)) if err != nil { t.Fatal(err) } - gotLastSearchTime, err := r.GetMapsLastSearchTime(tt.args.context, tt.args.location, tt.args.category, tt.args.priceLevel) + gotLastSearchTime, err := r.GetMapsLastSearchTime(tt.args.context, tt.args.lat, tt.args.lng, tt.args.category, tt.args.priceLevel) if !tt.wantErr && err != nil { - t.Errorf("GetMapsLastSearchTime(%v, %v, %v, %v) encountered error: %v", tt.args.context, tt.args.location, tt.args.category, tt.args.priceLevel, err) + t.Errorf("GetMapsLastSearchTime(%v, %v, %v, %v, %v) encountered error: %v", tt.args.context, tt.args.lat, tt.args.lng, tt.args.category, tt.args.priceLevel, err) return } - assert.Equalf(t, tt.wantLastSearchTime.Format(time.RFC3339), gotLastSearchTime.Format(time.RFC3339), "GetMapsLastSearchTime(%v, %v, %v, %v)", tt.args.context, tt.args.location, tt.args.category, tt.args.priceLevel) + assert.Equalf(t, tt.wantLastSearchTime.Format(time.RFC3339), gotLastSearchTime.Format(time.RFC3339), "GetMapsLastSearchTime(%v, %v, %v, %v, %v)", tt.args.context, tt.args.lat, tt.args.lng, tt.args.category, tt.args.priceLevel) }) } } + +// TestMapsLastSearchTimeIsCellScoped covers the defect that made the cache miss on nearly every +// merchant request: the marker used to be keyed on country/admin1/city while the geo bucket it +// guards is read from the caller's exact coordinates. A user far from the city centroid would +// read a marker claiming freshness over ground no search had populated. +func TestMapsLastSearchTimeIsCellScoped(t *testing.T) { + ctx := context.Background() + searchTime := time.Now().Format(time.RFC3339) + + // Deliberately mid-cell, near Los Angeles. A point picked at random has a real chance of + // sitting against a cell edge, where a neighbour 1 km away is legitimately a different cell — + // the boundary duplication the grid accepts in exchange for being a fixed, stateless key. The + // precondition below keeps that property from being mistaken for a bug in the marker. + const lat, lng = 34.092, -118.26 + const nearLat = lat + 0.009 // ~1 km north + const farLat = lat + 0.2 // ~22 km north + + if POI.EncodeSearchCell(lat, lng) != POI.EncodeSearchCell(nearLat, lng) { + t.Fatalf("fixture is not mid-cell: %s vs %s", POI.EncodeSearchCell(lat, lng), POI.EncodeSearchCell(nearLat, lng)) + } + + if err := RedisClient.SetMapsLastSearchTime(ctx, lat, lng, POI.PlaceCategoryShopping, POI.PriceLevelZero, searchTime); err != nil { + t.Fatal(err) + } + + t.Run("a nearby request reuses the marker", func(t *testing.T) { + if _, err := RedisClient.GetMapsLastSearchTime(ctx, nearLat, lng, POI.PlaceCategoryShopping, POI.PriceLevelZero); err != nil { + t.Errorf("expected a nearby request to hit the marker, got %v", err) + } + }) + + t.Run("a request ~22 km away misses", func(t *testing.T) { + if _, err := RedisClient.GetMapsLastSearchTime(ctx, farLat, lng, POI.PlaceCategoryShopping, POI.PriceLevelZero); err == nil { + t.Error("expected a miss far outside the populated area, got a hit") + } + }) + + // The specific asymmetry that was broken: Shopping/Lodging/Wellness buckets are not + // price-partitioned, so their marker must not vary with price level either. + t.Run("price level does not affect non-eatery markers", func(t *testing.T) { + for _, level := range POI.AllPriceLevels { + if _, err := RedisClient.GetMapsLastSearchTime(ctx, lat, lng, POI.PlaceCategoryShopping, level); err != nil { + t.Errorf("price level %d missed a marker written at level 0: %v", level, err) + } + } + }) + + // Eatery levels 3-4 trigger a genuinely different Google request, so they must NOT be + // satisfied by the generic marker. + t.Run("pricey eatery searches keep their own marker", func(t *testing.T) { + if err := RedisClient.SetMapsLastSearchTime(ctx, lat, lng, POI.PlaceCategoryEatery, POI.PriceLevelZero, searchTime); err != nil { + t.Fatal(err) + } + for _, level := range []POI.PriceLevel{POI.PriceLevelThree, POI.PriceLevelFour} { + if _, err := RedisClient.GetMapsLastSearchTime(ctx, lat, lng, POI.PlaceCategoryEatery, level); err == nil { + t.Errorf("price level %d was served by the generic eatery marker", level) + } + } + for _, level := range []POI.PriceLevel{POI.PriceLevelOne, POI.PriceLevelTwo} { + if _, err := RedisClient.GetMapsLastSearchTime(ctx, lat, lng, POI.PlaceCategoryEatery, level); err != nil { + t.Errorf("price level %d should share the generic eatery marker: %v", level, err) + } + } + }) +} diff --git a/test/redis_client_mocks/nearby_search_test.go b/test/redis_client_mocks/nearby_search_test.go index b2808002c..59785ce6a 100644 --- a/test/redis_client_mocks/nearby_search_test.go +++ b/test/redis_client_mocks/nearby_search_test.go @@ -98,24 +98,51 @@ func TestGetPlaces_shouldExcludePlacesOutsideOfSearchRadius(t *testing.T) { } } -func TestGetPlaces_resultShouldBeEmptyAfterPriceMatch(t *testing.T) { - // expect result should be empty, because mock data has no PriceLevelTwo places. - placeSearchRequest := iowrappers.PlaceSearchRequest{ - Location: POI.Location{Longitude: -74.0060, Latitude: 40.7128}, - PlaceCat: POI.PlaceCategoryEatery, - Radius: uint(5000), - PriceLevel: POI.PriceLevelTwo, +// TestGetPlaces_readsEveryPriceLevel pins that the geo read is price-agnostic. +// +// This replaces a test that asserted a PriceLevelTwo eatery search returned nothing because no +// fixture place carries price level 2. That only held while eateries were split across +// placeIDs:eatery:level0..4, and the split was the defect: Google omits price_level for most +// places (so they all landed in level0) and only accepts a price filter at level >= 3, so +// searches for levels 0-2 issued identical requests yet each read back a fifth of the data. +// +// Price selection is the caller's job — matching.MatcherForPriceRange applies +// filterPlacesOnPriceLevel to these results (planner/solver.go). The cache's contract is +// "everything of this category near here", nothing narrower. +func TestGetPlaces_readsEveryPriceLevel(t *testing.T) { + requestAt := func(level POI.PriceLevel) []POI.Place { + t.Helper() + req := iowrappers.PlaceSearchRequest{ + Location: POI.Location{Longitude: -74.0060, Latitude: 40.7128}, + PlaceCat: POI.PlaceCategoryEatery, + Radius: uint(5000), + PriceLevel: level, + } + got, err := RedisClient.NearbySearch(RedisContext, &req) + if err != nil { + t.Fatalf("RedisClient.NearbySearch at price level %d: %v", level, err) + } + return got } - cachedEateryPlaces, err := RedisClient.NearbySearch(RedisContext, &placeSearchRequest) - if err != nil { - t.Error(err) - return + // Keens Steakhouse (price level 4) is the one eatery inside the radius. + atLevelFour := requestAt(POI.PriceLevelFour) + if len(atLevelFour) != 1 || atLevelFour[0].ID != places[2].ID { + t.Fatalf("expected only %s in radius, got %+v", places[2].Name, atLevelFour) } - if len(cachedEateryPlaces) != 0 { - t.Errorf("Expect to have 0 place, but got %d instead", len(cachedEateryPlaces)) - return + // A level-2 search must see it too: no fixture place has price level 2, yet the read is not + // scoped by price. + for _, level := range POI.AllPriceLevels { + got := requestAt(level) + if len(got) != len(atLevelFour) { + t.Errorf("price level %d returned %d places, want %d — the read must not be price-scoped", + level, len(got), len(atLevelFour)) + continue + } + if got[0].ID != atLevelFour[0].ID { + t.Errorf("price level %d returned place %s, want %s", level, got[0].ID, atLevelFour[0].ID) + } } } From 21c9c3d2a40c23e0241c50db06700791d4bedc38 Mon Sep 17 00:00:00 2001 From: tim-eternos Date: Thu, 30 Jul 2026 15:10:40 -0700 Subject: [PATCH 2/3] docs: correct the eatery union runbook's deploy ordering MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The runbook said to run the migration before deploying, but the migration endpoint is part of the code being deployed — it does not exist until the deploy that introduces it, so it cannot be the pre-deploy step. Production returns the 404 page for /v1/migrate/union-eatery-buckets today, which is how this surfaced. The pre-deploy step is a plain redis-cli ZUNIONSTORE; no application code is involved. The endpoint's value is dry-run reporting and post-deploy verification. Steps now spell out the six-key form (the target is included so the command is idempotent), the coordinate spot-check, and the rollback. Also corrects an overstatement: the marker re-key forces one cold search per occupied cell per category regardless of migration order. Running the union first protects the existing eatery members from being unreadable in the interim, which is a smaller and more accurate claim. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01WHNC45T5vRTKuEmifJANCG --- docs/migrations/collapse-eatery-buckets.md | 80 +++++++++++++++------- 1 file changed, 57 insertions(+), 23 deletions(-) diff --git a/docs/migrations/collapse-eatery-buckets.md b/docs/migrations/collapse-eatery-buckets.md index 069d86ab9..2c7103487 100644 --- a/docs/migrations/collapse-eatery-buckets.md +++ b/docs/migrations/collapse-eatery-buckets.md @@ -3,18 +3,36 @@ One-time merge of `placeIDs:eatery:level0..4` into a single `placeIDs:eatery` geo index, so every category has exactly one bucket the way `placeIDs:visit` always has. -`GET /v1/migrate/union-eatery-buckets` — admin only, **dry-run unless `apply=true`**. +Two ways to run it: -## Run this BEFORE deploying +- **`redis-cli ZUNIONSTORE`** — the pre-deploy path. One command, no application code involved. +- **`GET /v1/migrate/union-eatery-buckets`** — admin only, dry-run unless `apply=true`. Reports + sizes and predicts the result, and is re-runnable. Only available *after* this change ships. -The union is purely additive and invisible to the running code: it creates a key nothing reads -yet. Deploying first would instead point every eatery read at a key that does not exist and -trigger a global cold-search burst. +## Ordering + +The union must land before the collapsed-key read goes live, or eatery reads hit a key that does +not exist yet and every request returns empty until organic traffic refills it. + +But the HTTP endpoint **is part of the code being deployed**, so it cannot be the pre-deploy step +— it does not exist until the deploy that introduces it. Use `redis-cli` for that step: ``` -1. dry run → 2. apply → 3. deploy → 4. verify → 5. delete the level* keys +1. redis-cli union → 2. deploy → 3. verify (endpoint dry run) → 4. delete the level* keys ``` +The union is purely additive and invisible to the running code — it creates a key nothing reads +yet — so step 1 is safe to run at any point beforehand. + +If you deploy first by mistake it is recoverable, not fatal: the new write path populates +`placeIDs:eatery` organically and the union later merges the legacy members in (the target is +included in the union sources, so nothing is lost). The cost is degraded eatery results until +each area gets re-searched, plus the Google spend for those searches. + +Note what the ordering does **not** buy you: the marker re-key (see below) forces one cold search +per occupied cell per category regardless of migration order. What running the union first +protects is the ~8.5k existing eatery members staying readable in the meantime. + ## Why the split is going away Eateries were filed under `placeIDs:eatery:level` keyed on each place's own price level, while @@ -49,31 +67,47 @@ members that already-deployed code has written to the collapsed key. ```bash BASE=https://best-vacation-planner.herokuapp.com +# Heroku: eval $(heroku config:get REDIS_URL -a ) or use `heroku redis:cli -a ` +R="redis-cli -u $REDIS_URL" -# 1. Dry run. Reports each source bucket's size, their total, and the exact resulting -# member count (computed by reading members — a dry run writes nothing). -curl -s -H "Authorization: Bearer $ADMIN_JWT" \ - "$BASE/v1/migrate/union-eatery-buckets" | jq +# --- 1. Union, before deploying. ------------------------------------------------- +# Record the starting sizes so step 3 has something to check against. +for L in 0 1 2 3 4; do echo -n "level$L "; $R ZCARD "placeIDs:eatery:level$L"; done +$R ZCARD placeIDs:eatery # expected 0 on a first run -# 2. Apply. expected_after and target_after must match. -curl -s -H "Authorization: Bearer $ADMIN_JWT" \ - "$BASE/v1/migrate/union-eatery-buckets?apply=true" | jq +# Note the SIX keys: the target is included so the command is idempotent and cannot +# drop members already written to the collapsed key. AGGREGATE MIN is mandatory. +$R ZUNIONSTORE placeIDs:eatery 6 \ + placeIDs:eatery:level0 placeIDs:eatery:level1 placeIDs:eatery:level2 \ + placeIDs:eatery:level3 placeIDs:eatery:level4 placeIDs:eatery AGGREGATE MIN -# 3. Deploy. +$R ZCARD placeIDs:eatery # <= the sum above; lower means a place was in two buckets -# 4. Verify: a non-zero eatery count, which /stats/places could never report before -# (it built "placeIDs:eatery", a key nothing wrote). -curl -s -H "Authorization: Bearer $ADMIN_JWT" "$BASE/stats/places" | jq +# Spot-check that a geohash score survived, against that place's own record. +# These two must agree to within a few metres. +ID=$($R ZRANGE placeIDs:eatery 0 0 | head -1) +$R GEOPOS placeIDs:eatery "$ID" +$R GET "place_details:place_ID:$ID" | jq '.Location' + +# --- 2. Deploy. ----------------------------------------------------------------- -# Spot-check that scores survived, against the place's own stored coordinates: -redis-cli GEOPOS placeIDs:eatery "" -redis-cli GET "place_details:place_ID:" | jq '.Location' +# --- 3. Verify. ----------------------------------------------------------------- +# The endpoint exists now. A dry run re-reports the sizes and predicts the same count, +# which confirms the deployed code resolves the key you just populated. +curl -s -H "Authorization: Bearer $ADMIN_JWT" \ + "$BASE/v1/migrate/union-eatery-buckets" | jq -# 5. Only once the deploy is healthy: -redis-cli DEL placeIDs:eatery:level0 placeIDs:eatery:level1 placeIDs:eatery:level2 \ - placeIDs:eatery:level3 placeIDs:eatery:level4 +# A non-zero eatery count — which /stats/places could never report before, because it +# built "placeIDs:eatery", a key nothing wrote. +curl -s -H "Authorization: Bearer $ADMIN_JWT" "$BASE/stats/places" | jq + +# --- 4. Only once the deploy is healthy. ---------------------------------------- +$R DEL placeIDs:eatery:level0 placeIDs:eatery:level1 placeIDs:eatery:level2 \ + placeIDs:eatery:level3 placeIDs:eatery:level4 ``` +To roll back before step 4, `DEL placeIDs:eatery` — the legacy keys are untouched until then. + `expected_after` is the deduped union size, so it will be **lower** than `source_total` whenever a place appears in more than one price bucket. That is the intended outcome, not data loss. From e7e70ee9468f847e1a3dec6e1cd38f6531d5f907 Mon Sep 17 00:00:00 2001 From: tim-eternos Date: Thu, 30 Jul 2026 15:23:26 -0700 Subject: [PATCH 3/3] docs: record what the production eatery union actually found MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit placeIDs:eatery already existed in production with 7,113 members — a pre-2023 artefact from before 20c1eb7 introduced the price split, when eateries were written to that un-suffixed name. Nothing has read it since, so this change making it the live read key would have silently revived three-year-old data. 4,634 of those members exist in no level* bucket, and a 300-sample found 286 of 287 parsed records carry no Types field at all, so neither ReclassifyForCategory nor the purge migration can audit them — both keep no-Types records by design. Spot checks included a horse racing track filed as an eatery. The pre-deploy union therefore takes five source keys and overwrites the target, yielding exactly the 8,500 members already served. The six-key form is for post-deploy re-runs only, where an overwrite would drop members the new write path has written. Since five-key overwrites, the runbook now takes a COPY backup first. Also documents the 34 members found in two price buckets: AGGREGATE MIN picks one of two real positions, which can differ from the place_details coordinates by tens of metres. Verified 33 exact and one 82 m difference already present in the sources — pre-existing inconsistency, not migration damage. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01WHNC45T5vRTKuEmifJANCG --- docs/migrations/collapse-eatery-buckets.md | 48 +++++++++++++++++++++- 1 file changed, 47 insertions(+), 1 deletion(-) diff --git a/docs/migrations/collapse-eatery-buckets.md b/docs/migrations/collapse-eatery-buckets.md index 2c7103487..3ccfda7ca 100644 --- a/docs/migrations/collapse-eatery-buckets.md +++ b/docs/migrations/collapse-eatery-buckets.md @@ -106,7 +106,53 @@ $R DEL placeIDs:eatery:level0 placeIDs:eatery:level1 placeIDs:eatery:level2 \ placeIDs:eatery:level3 placeIDs:eatery:level4 ``` -To roll back before step 4, `DEL placeIDs:eatery` — the legacy keys are untouched until then. +To roll back before step 4, restore from the backup taken in step 1 — the `level*` keys are +untouched until then, so `ZUNIONSTORE` can also simply be re-run. + +## Five source keys, not six — and why the target needs a backup + +`placeIDs:eatery` **already existed in production** when this ran, holding 7,113 members. It is a +pre-2023 artefact: the price split was introduced in `20c1eb7` (2023-02-20), and before that +eateries were written to exactly this un-suffixed name. Nothing has read it since, so it sat as +dead data for roughly three years — until this change made that name the live read key again. + +4,634 of those members existed in no `level*` bucket. Sampling 300 of them: + +| | | +| --- | --- | +| no `place_details` record (orphans) | 13 (4.3%) | +| records with **no `Types` field at all** | 286 of 287 | +| primary type detectably non-Eatery | 1 (a hotel) | + +The missing `Types` is what settles it: those records predate the field, and both +`POI.ReclassifyForCategory` and the purge migration deliberately *keep* no-Types records, so +neither can audit them. Spot checks turned up a horse racing track filed as an eatery. Merging +~4,600 unauditable three-year-old records into the live eatery bucket — right after an incident +about misclassified places in eatery buckets — invites a repeat. + +So the pre-deploy union uses **five** source keys and lets `ZUNIONSTORE` overwrite the target, +producing exactly the 8,500 members production already serves. No coverage regression, because +those 4,634 were not being served. Their `place_details` records are untouched, so reviving them +later stays possible as a deliberate decision. + +Because that overwrites the target, **back it up first**: + +```bash +$R COPY placeIDs:eatery placeIDs:eatery:pre-collapse-backup # preserves scores exactly +``` + +Use the **six**-key form (target included) only for re-runs *after* the deploy, where the new +write path is filling the target and an overwrite would drop members it has already written. + +## Expect a small number of members in two price buckets + +34 members were in more than one `level*` bucket — the same place written under different price +levels as Google's answer changed over time, which is the fragmentation this change removes. For +those, `AGGREGATE MIN` picks one of the two real positions, which may differ from the coordinates +in the `place_details` record by tens of metres. That is pre-existing inconsistency surfacing, not +migration damage: verified 33 exact matches and one 82 m difference that was present in the source +buckets beforehand. A `SUM` corruption looks nothing like this — it roughly doubles the score and +throws the place thousands of kilometres. `expected_after` is the deduped union size, so it will be **lower** than `source_total` whenever a place appears in more than one price bucket. That is the intended outcome, not data loss.