diff --git a/POI/categories.go b/POI/categories.go index e8c29f26..be8364ea 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 96035112..b1000c95 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 00000000..3ccfda7c --- /dev/null +++ b/docs/migrations/collapse-eatery-buckets.md @@ -0,0 +1,166 @@ +# 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. + +Two ways to run it: + +- **`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. + +## 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. 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 +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 +# Heroku: eval $(heroku config:get REDIS_URL -a ) or use `heroku redis:cli -a ` +R="redis-cli -u $REDIS_URL" + +# --- 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 + +# 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 + +$R ZCARD placeIDs:eatery # <= the sum above; lower means a place was in two buckets + +# 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. ----------------------------------------------------------------- + +# --- 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 + +# 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, 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. + +## 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 90b2bb3b..c22e9148 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 a984165e..703b9074 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 7f2d6428..5a3a1b3f 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 00000000..7f94c4d6 --- /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 25ca3af5..1fb6d174 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 dd92c68c..9f04632a 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 2dfc307f..651da9e4 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 2b76f2dc..e04f09a8 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 d17d5834..f07c5365 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 00000000..d6fd9b75 --- /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 4505e2e8..285e021e 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 55a6fd31..402b9435 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 b94cfe8d..22a6e70a 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 82c8b61b..5beb93f5 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 3ea5d8f5..d6b7cc6f 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 d11ff5e0..bc522fda 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 83844d72..a4385bc2 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 b2808002..59785ce6 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) + } } }