diff --git a/.gitignore b/.gitignore index 989af4c0..bbabcf2a 100644 --- a/.gitignore +++ b/.gitignore @@ -5,3 +5,4 @@ bin/ .idea .DS_Store node_modules/ +Vacation-planner diff --git a/POI/categories.go b/POI/categories.go index ec42ed1c..e8c29f26 100644 --- a/POI/categories.go +++ b/POI/categories.go @@ -38,8 +38,6 @@ const ( LocationTypeBar = LocationType("bar") LocationTypeBakery = LocationType("bakery") LocationTypeMealTakeaway = LocationType("meal_takeaway") - LocationTypeFastFood = LocationType("fast_food_restaurant") - LocationTypeFoodCourt = LocationType("food_court") LocationTypeMuseum = LocationType("museum") LocationTypeGallery = LocationType("art_gallery") LocationTypeAmusementPark = LocationType("amusement_park") @@ -58,26 +56,33 @@ const ( LocationTypePharmacy = LocationType("pharmacy") ) -// GetPlaceCategory maps a Google Maps place type back to its category. It is the inverse of -// GetPlaceTypes and MUST stay consistent with it: the nearby-search cache writes each place -// under EncodeNearbySearchRedisKey(GetPlaceCategory(place.LocationType), ...), so a type that +// GetPlaceCategory maps a Google Maps place type back to its category, reporting whether +// the type is mapped at all. It is the inverse of GetPlaceTypes and MUST stay consistent +// with it: the nearby-search cache writes each place under +// EncodeNearbySearchRedisKey(GetPlaceCategory(place.LocationType), ...), so a type that // resolves to a different category than the one it was searched under would never cache-hit. -func GetPlaceCategory(placeType LocationType) (placeCategory PlaceCategory) { +// +// It deliberately has NO default category. An earlier version defaulted to Eatery, which +// silently absorbed place types the legacy Nearby Search does not understand — two +// Places-API-(New)-only types ("fast_food_restaurant", "food_court") were added to +// GetPlaceTypes(Eatery), Google ignored the unenforceable filter, and prominence-ranked +// hotels were written into the eatery geo buckets. Returning ok=false forces every caller +// to decide what an unmapped type means, and makes TestPlaceCategoryRoundTrip able to fail. +func GetPlaceCategory(placeType LocationType) (PlaceCategory, bool) { switch placeType { case LocationTypePark, LocationTypeAmusementPark, LocationTypeGallery, LocationTypeMuseum: - placeCategory = PlaceCategoryVisit + return PlaceCategoryVisit, true case LocationTypeCafe, LocationTypeRestaurant, LocationTypeBar, LocationTypeBakery, LocationTypeMealTakeaway: - placeCategory = PlaceCategoryEatery + return PlaceCategoryEatery, true case LocationTypeShoppingMall, LocationTypeDepartmentStore, LocationTypeSupermarket, LocationTypeClothingStore, LocationTypeStore: - placeCategory = PlaceCategoryShopping + return PlaceCategoryShopping, true case LocationTypeLodging: - placeCategory = PlaceCategoryLodging + return PlaceCategoryLodging, true case LocationTypeGym, LocationTypeSpa, LocationTypePharmacy: - placeCategory = PlaceCategoryWellness + return PlaceCategoryWellness, true default: - placeCategory = PlaceCategoryEatery + return PlaceCategory(""), false } - return } // GetPlaceTypes returns a set of types defined in Google Maps API given a location type @@ -88,7 +93,7 @@ func GetPlaceTypes(placeCat PlaceCategory) (placeTypes []LocationType) { []LocationType{LocationTypePark, LocationTypeAmusementPark, LocationTypeGallery, LocationTypeMuseum}...) case PlaceCategoryEatery: placeTypes = append(placeTypes, - []LocationType{LocationTypeCafe, LocationTypeRestaurant, LocationTypeBar, LocationTypeBakery, LocationTypeMealTakeaway, LocationTypeFastFood, LocationTypeFoodCourt}...) + []LocationType{LocationTypeCafe, LocationTypeRestaurant, LocationTypeBar, LocationTypeBakery, LocationTypeMealTakeaway}...) case PlaceCategoryShopping: placeTypes = append(placeTypes, []LocationType{LocationTypeShoppingMall, LocationTypeDepartmentStore, LocationTypeSupermarket, LocationTypeClothingStore, LocationTypeStore}...) diff --git a/docs/migrations/reclassify-buckets.md b/docs/migrations/reclassify-buckets.md new file mode 100644 index 00000000..90b2bb3b --- /dev/null +++ b/docs/migrations/reclassify-buckets.md @@ -0,0 +1,86 @@ +# Migration: purge misclassified places from category geo buckets + +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*`. + +`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. + +## Run the fixes first + +This must run **after** the write-path fixes in this PR are deployed. `8644199` is still +creating new bad records in production: every city/category/price-bucket whose +`MapsLastSearchTime` marker passes the 14-day `MinMapsResultRefreshDuration` does a fresh +cold search. Cleaning before the fix deploys just gets re-polluted. + +## The rule + +A member is removed only when its **primary** Google type maps to a *different* +category — the exact inverse of the write rule. An unmapped primary type is not +evidence of misclassification, because the write path would legitimately place it there. + +| Primary type | Resolves to | Action | +| --- | --- | --- | +| `lodging` | Lodging | removed | +| `supermarket`, `department_store` | Shopping | removed | +| `cafe`, `restaurant` | Eatery | kept | +| `meal_delivery`, `night_club` | unmapped | kept | +| no `types[]` at all | unmapped | kept | + +⚠️ This rule and the write rule (`SetPlacesAddGeoLocations`, which keys on the stamped +`LocationType`) deliberately disagree. A refactor that "unifies" them reintroduces the +incident. `TestRemoveMisclassifiedPlacesPrimaryTypeTruthTable` pins the dangerous direction. + +## Expected scale + +Measured against production on 2026-07-30 (24,203 bucket members scanned): + +| Category | Members | Candidates | +| --- | --- | --- | +| Eatery | 8,589 | 145 | +| Visit | 14,674 | 0 | +| Shopping | 498 | 12 | +| Wellness | 319 | 4 | +| Lodging | 123 | 3 | + +**164 total.** Of these, 123 are incident-attributable (stamped `fast_food_restaurant`, +mostly San Francisco / Tulsa / Boise hotels); the other 41 are pre-existing +misclassifications the rule also catches (hotels stamped `restaurant`, supermarkets +stamped `bakery`). + +**Known residue:** 27 incident records are *not* removed because their primary type maps +to no category — `university` ×5, `airport` ×2, `real_estate_agency` ×2, `stadium`, +`night_club`, `hardware_store`, `doctor`, and 7 with no `types[]`. These stay in the +eatery buckets. The trip-planning path (`planner/solver.go`) does not reclassify, so they +remain reachable in generated plans. Broadening `GetPlaceCategory` to cover those types is +tracked as follow-up work. + +Separately, ~328 bucket members have no backing `place_details` record. Pre-existing and +unrelated; the migration skips them. + +## Steps + +```bash +# 1. Dry run and READ THE OUTPUT before going further. +curl -s -H "Authorization: Bearer $ADMIN_JWT" \ + "https://best-vacation-planner.herokuapp.com/v1/migrate/reclassify-buckets?category=Eatery" | jq + +# 2. Apply. +curl -s -H "Authorization: Bearer $ADMIN_JWT" \ + "https://best-vacation-planner.herokuapp.com/v1/migrate/reclassify-buckets?category=Eatery&apply=true" | jq + +# 3. Re-run the dry run; misclassified should now be 0 (modulo the residue above). +``` + +Repeat per category as needed (`Shopping`, `Wellness`, `Lodging`). Reads are pipelined in +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 `::::`): + +```bash +redis-cli HDEL MapsLastSearchTime "united states:ca:los altos:eatery:0" +``` diff --git a/iowrappers/data_migrations.go b/iowrappers/data_migrations.go index 06664e1c..a984165e 100644 --- a/iowrappers/data_migrations.go +++ b/iowrappers/data_migrations.go @@ -2,6 +2,8 @@ package iowrappers import ( "context" + "encoding/json" + "errors" "fmt" "reflect" "strconv" @@ -9,6 +11,7 @@ import ( "sync" "github.com/bobg/go-generics/set" + "github.com/redis/go-redis/v9" "github.com/weihesdlegend/Vacation-planner/POI" ) @@ -189,6 +192,178 @@ func (s *PoiSearcher) AddUserRatingsTotal(context context.Context) error { return nil } +// bucketCleanupReadBatchSize is how many place records the bucket scan reads per pipeline +// round trip, matching the batch size SetPlacesAddGeoLocations uses on the write side. +const bucketCleanupReadBatchSize = 100 + +// BucketCleanupReport summarizes a RemoveMisclassifiedPlacesFromCategoryBuckets run. +// BucketSizes/TotalMembers are measured with ZCARD before the scan begins so a dry-run +// report states the scale of the job even for buckets an operator has never sized. +type BucketCleanupReport struct { + BucketSizes map[string]int64 `json:"bucket_sizes"` + TotalMembers int64 `json:"total_members"` + Scanned int `json:"scanned"` + Misclassified int `json:"misclassified"` + Removed int `json:"removed"` + RemovedIDs []string `json:"removed_ids"` +} + +// SetPlace stores a single place record. Exported wrapper over setPlace for migrations and tests. +func (r *RedisClient) SetPlace(ctx context.Context, place POI.Place) error { + return r.setPlace(ctx, place) +} + +// AddGeoLocation adds a place to a geo bucket under an explicit key. Exported for +// migrations and tests that need to write buckets the normal write path would reject. +func (r *RedisClient) AddGeoLocation(ctx context.Context, key string, place POI.Place) error { + loc := place.GetLocation() + _, err := r.client.GeoAdd(ctx, key, &redis.GeoLocation{ + Name: place.ID, + Latitude: loc.Latitude, + Longitude: loc.Longitude, + }).Result() + return err +} + +// 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 +// ignored the unenforceable type filter and returned prominence-ranked establishments, and +// those were stamped with the queried type and written into placeIDs:eatery:level*. +// +// The removal rule is the exact inverse of the WRITE rule, not of POI.ReclassifyForCategory. +// SetPlacesAddGeoLocations files a place under GetPlaceCategory(place.LocationType) and +// refuses to write anything whose type maps to no category, so this migration removes a +// member only when its primary type positively maps to a DIFFERENT category (lodging -> +// Lodging, supermarket -> Shopping). An UNMAPPED primary type is deliberately kept: types +// like "meal_delivery" and "night_club" are legal legacy types that Google routinely lists +// first for genuine eateries (a delivery-first restaurant, a bar that is also a club), as are +// records with no Types at all (cached before Types was captured). Those places carry a +// stamped LocationType the write path maps straight back to this category, so removing them +// would only delete rows the next cold search re-creates — while shrinking the trip-planning +// candidate pool for up to MinMapsResultRefreshDuration, because the trip-planning path +// (planner/solver.go -> matching.NearbySearchForCategory) reads these buckets with no +// reclassification at all. +// +// Note this is a broader keep-set than POI.ReclassifyForCategory applies on the merchant +// endpoint: that function keeps a place only when its primary type is one of the category's +// five search types. Divergence is intended — one function decides what to show in a single +// response, this one decides what may exist in the shared cache. +// +// dryRun reports what would be removed without deleting anything. Always dry-run first. +func (r *RedisClient) RemoveMisclassifiedPlacesFromCategoryBuckets(ctx context.Context, cat POI.PlaceCategory, dryRun bool) (BucketCleanupReport, error) { + 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)) + } + + // 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 + // to judge whether a dry run can complete inside their client/router timeout. + for _, key := range keys { + size, err := r.client.ZCard(ctx, key).Result() + if err != nil { + return report, fmt.Errorf("sizing geo bucket %s: %w", key, err) + } + report.BucketSizes[key] = size + report.TotalMembers += size + } + + for _, key := range keys { + members, err := r.client.ZRange(ctx, key, 0, -1).Result() + if err != nil { + return report, fmt.Errorf("reading geo bucket %s: %w", key, err) + } + // Read place records in pipelined batches rather than one GET per member: a serial + // N+1 over a real bucket cannot finish inside a 30s request timeout, which would + // make the mandatory dry-run review impossible and defeat the safety property. + for start := 0; start < len(members); start += bucketCleanupReadBatchSize { + batch := members[start:min(start+bucketCleanupReadBatchSize, len(members))] + places, found, err := r.getPlacesPipelined(ctx, batch) + if err != nil { + return report, fmt.Errorf("reading place records for %s: %w", key, err) + } + for i, placeID := range batch { + report.Scanned++ + if !found[i] { + // no place record backing this bucket member; leave it for RemovePlaces + Logger.Debugf("RemoveMisclassifiedPlacesFromCategoryBuckets: no record for %s in %s", placeID, key) + continue + } + place := places[i] + primary := POI.PrimaryLocationType(place.Types) + // Only remove members whose primary type positively belongs to a DIFFERENT + // category. An unmapped primary type (meal_delivery, night_club, or no Types + // at all) is not evidence of misclassification — the write path would + // legitimately place it here. + if c, ok := POI.GetPlaceCategory(primary); !ok || c == cat { + continue + } + report.Misclassified++ + report.RemovedIDs = append(report.RemovedIDs, placeID) + Logger.Infof("RemoveMisclassifiedPlacesFromCategoryBuckets: %s (%q, LocationType=%q, primary=%q, Types=%v) does not belong in %s", + placeID, place.Name, place.LocationType, primary, place.Types, key) + if dryRun { + continue + } + if _, err := r.client.ZRem(ctx, key, placeID).Result(); err != nil { + return report, fmt.Errorf("removing %s from %s: %w", placeID, key, err) + } + report.Removed++ + } + } + } + return report, nil +} + +// getPlacesPipelined fetches the place records for placeIDs in a single round trip. +// found[i] reports whether placeIDs[i] had a usable record: a bucket member with no (or an +// unparsable) backing record is not this migration's problem to fix, so it is reported as +// missing rather than failing the whole batch. A transport-level failure is returned as an +// error, because then nothing in the batch was actually read. +func (r *RedisClient) getPlacesPipelined(ctx context.Context, placeIDs []string) ([]POI.Place, []bool, error) { + cmds := make([]*redis.StringCmd, len(placeIDs)) + _, err := r.client.Pipelined(ctx, func(pipe redis.Pipeliner) error { + for i, placeID := range placeIDs { + cmds[i] = pipe.Get(ctx, PlaceDetailsRedisKeyPrefix+placeID) + } + return nil + }) + // Pipelined surfaces the first non-nil command error, and a missing key is reported as + // redis.Nil — an expected outcome here, not a failure. + if err != nil && !errors.Is(err, redis.Nil) { + return nil, nil, err + } + + places := make([]POI.Place, len(placeIDs)) + found := make([]bool, len(placeIDs)) + for i, cmd := range cmds { + res, cmdErr := cmd.Result() + if cmdErr != nil { + continue + } + if unmarshalErr := json.Unmarshal([]byte(res), &places[i]); unmarshalErr != nil { + Logger.Debugf("getPlacesPipelined: cannot parse record for %s: %v", placeIDs[i], unmarshalErr) + continue + } + found[i] = true + } + return places, found, nil +} + +// RemoveMisclassifiedPlacesFromCategoryBuckets forwards to the RedisClient method so the +// admin handler can call it through the concrete PoiSearcher (p.Solver.Searcher). +func (s *PoiSearcher) RemoveMisclassifiedPlacesFromCategoryBuckets(ctx context.Context, cat POI.PlaceCategory, dryRun bool) (BucketCleanupReport, error) { + return s.redisClient.RemoveMisclassifiedPlacesFromCategoryBuckets(ctx, cat, dryRun) +} + func (s *PoiSearcher) AddUrl(context context.Context) error { placeIdToDetailedSearchResults, err := s.addDataFieldsToPlaces(context, "url", BatchSize) if err != nil { diff --git a/iowrappers/nearby_search.go b/iowrappers/nearby_search.go index 8c7159ad..dd92c68c 100644 --- a/iowrappers/nearby_search.go +++ b/iowrappers/nearby_search.go @@ -72,8 +72,23 @@ func MatchesBrandName(placeName, keyword string) bool { return strings.Contains(POI.NormalizeBrandKey(placeName), normalizedKeyword) } -// CreateMapSearchRequest creates a NearbySearchRequest for maps NearbySearch, adjust key settings such as radius and price levels -func CreateMapSearchRequest(reqIn *PlaceSearchRequest, placeType POI.LocationType, token string) (reqOut maps.NearbySearchRequest) { +// CreateMapSearchRequest creates a NearbySearchRequest for maps NearbySearch, adjust key settings such as radius and price levels. +// It rejects any place type the legacy Places API does not define: POI.LocationType is cast +// straight to maps.PlaceType and forwarded as ?type=, and Google responds to an unknown value +// by IGNORING the filter and returning prominence-ranked establishments rather than erroring. +// Those results are then stamped with the queried type, so an unvalidated type silently +// poisons the cache. maps.ParsePlaceType is the SDK's own list of legal values. +func CreateMapSearchRequest(reqIn *PlaceSearchRequest, placeType POI.LocationType, token string) (maps.NearbySearchRequest, error) { + // LocationTypeAny is the keyword (brand) search case: the type is deliberately unset so + // Google matches the keyword across all place types. + if placeType != POI.LocationTypeAny { + if _, err := maps.ParsePlaceType(string(placeType)); err != nil { + return maps.NearbySearchRequest{}, fmt.Errorf( + "place type %q is not a legacy Places API type (Places API (New) types are not accepted by /maps/api/place/nearbysearch): %w", + placeType, err) + } + } + // Adjust radius, minPrice and maxPrice settings in search request var radius = reqIn.Radius var exactPriceLevel maps.PriceLevel @@ -96,7 +111,7 @@ func CreateMapSearchRequest(reqIn *PlaceSearchRequest, placeType POI.LocationTyp RankBy: maps.RankBy("prominence"), MinPrice: exactPriceLevel, MaxPrice: exactPriceLevel, - } + }, nil } func (c *MapsClient) GoogleMapsNearbySearchWrapper(ctx context.Context, mapsReq maps.NearbySearchRequest) (resp maps.PlacesSearchResponse, err error) { @@ -139,14 +154,20 @@ func (c *MapsClient) extensiveNearbySearch(ctx context.Context, maxRequestTimes var reqTimes uint = 0 // number of queries for each location type var totalPlaceCount uint = 0 // number of results so far, keep this number low - maxRetries := reqTimes * uint(len(placeTypes)) + // Bail out of the whole search once a single round sees every place type fail. + // mapsFailuresCount is reset at the top of each round below (it must NOT be hoisted + // out here), so reaching maxRetries means every place type failed in THAT round, not + // cumulatively across rounds. A per-round counter is required: one persistently-failing + // type must not be able to spend down a shared budget and cut off its healthy siblings' + // remaining rounds. This was previously computed as reqTimes * len(placeTypes) while + // reqTimes was still 0, making the cap 0 and the break below unreachable. + maxRetries := uint(len(placeTypes)) microAddrMap := make(map[string]string) // map place ID to its micro-address placeMap := make(map[string]bool) // remove duplication for place with same ID urlMap := make(map[string]string) // map place ID to url summaryMap := make(map[string]string) // map place ID to summary - var mapsFailuresCount uint = 0 detailsBudget := request.DetailsLimit // remaining Place Details calls across all pages; only enforced when DetailsLimit > 0 // One place type's Nearby Search HTTP result (Phase A). Holds no shared state, @@ -159,6 +180,10 @@ func (c *MapsClient) extensiveNearbySearch(ctx context.Context, maxRequestTimes outer: for totalPlaceCount < request.MinNumResults { reqTimes++ + // Reset every round: this must count failures within the CURRENT round only, so + // one place type failing round after round cannot exhaust the shared budget and + // cut off healthy sibling types (see maxRetries comment above). + var mapsFailuresCount uint = 0 // Phase A — fetch every eligible place type's Nearby Search CONCURRENTLY. // These are independent, slow HTTP calls and the dominant cold-cache cost; @@ -175,7 +200,11 @@ outer: wg.Add(1) go func(i int, placeType POI.LocationType, token string) { defer wg.Done() - searchReq := CreateMapSearchRequest(request, placeType, token) + searchReq, reqErr := CreateMapSearchRequest(request, placeType, token) + if reqErr != nil { + fetched[i].err = reqErr + return + } select { case c.apiSemaphore <- struct{}{}: defer func() { <-c.apiSemaphore }() @@ -200,7 +229,7 @@ outer: Logger.Error(fmt.Errorf("places nearby search with Maps failed for place type %s with error: %w", placeType, fetched[i].err)) mapsFailuresCount++ - if mapsFailuresCount == maxRetries { + if mapsFailuresCount >= maxRetries { break outer } // we should still retry for the next place type if the number of failures is below maxRetries diff --git a/iowrappers/nearby_search_validation_test.go b/iowrappers/nearby_search_validation_test.go new file mode 100644 index 00000000..29b46704 --- /dev/null +++ b/iowrappers/nearby_search_validation_test.go @@ -0,0 +1,80 @@ +package iowrappers + +import ( + "strings" + "testing" + + "github.com/weihesdlegend/Vacation-planner/POI" +) + +// TestCreateMapSearchRequestRejectsUnknownPlaceType guards the fast_food_restaurant +// incident at the request boundary. The SDK casts POI.LocationType straight to +// maps.PlaceType and forwards it as ?type=, so an unknown value silently disables +// the filter server-side instead of erroring. Validate before spending the call. +func TestCreateMapSearchRequestRejectsUnknownPlaceType(t *testing.T) { + req := &PlaceSearchRequest{ + Location: POI.Location{Latitude: 37.38006, Longitude: -122.11612}, + PlaceCat: POI.PlaceCategoryEatery, + Radius: 8000, + PriceLevel: POI.PriceLevelTwo, + } + for _, placeType := range []POI.LocationType{ + POI.LocationType("fast_food_restaurant"), + POI.LocationType("food_court"), + POI.LocationType("not_a_google_type"), + } { + if _, err := CreateMapSearchRequest(req, placeType, ""); err == nil { + t.Errorf("CreateMapSearchRequest(%q) returned nil error, want validation failure", placeType) + } else if !strings.Contains(err.Error(), string(placeType)) { + t.Errorf("CreateMapSearchRequest(%q) error %q should name the offending type", placeType, err) + } + } +} + +// TestCreateMapSearchRequestAcceptsKnownPlaceTypes pins that every type the +// categories actually search for still builds a request. +func TestCreateMapSearchRequestAcceptsKnownPlaceTypes(t *testing.T) { + req := &PlaceSearchRequest{ + Location: POI.Location{Latitude: 37.38006, Longitude: -122.11612}, + PlaceCat: POI.PlaceCategoryEatery, + Radius: 8000, + PriceLevel: POI.PriceLevelTwo, + } + categories := []POI.PlaceCategory{ + POI.PlaceCategoryVisit, POI.PlaceCategoryEatery, + POI.PlaceCategoryShopping, POI.PlaceCategoryLodging, POI.PlaceCategoryWellness, + } + for _, category := range categories { + for _, placeType := range POI.GetPlaceTypes(category) { + got, err := CreateMapSearchRequest(req, placeType, "") + if err != nil { + t.Errorf("CreateMapSearchRequest(%q) in category %q: unexpected error %v", placeType, category, err) + continue + } + if string(got.Type) != string(placeType) { + t.Errorf("CreateMapSearchRequest(%q) set Type=%q, want %q", placeType, got.Type, placeType) + } + } + } +} + +// TestCreateMapSearchRequestAcceptsAnyType pins that keyword (brand) searches, +// which intentionally leave the type unset, are not rejected. +func TestCreateMapSearchRequestAcceptsAnyType(t *testing.T) { + req := &PlaceSearchRequest{ + Location: POI.Location{Latitude: 37.38006, Longitude: -122.11612}, + PlaceCat: POI.PlaceCategoryEatery, + Radius: 8000, + Keyword: "Dunkin'", + } + got, err := CreateMapSearchRequest(req, POI.LocationTypeAny, "") + if err != nil { + t.Fatalf("CreateMapSearchRequest(LocationTypeAny) returned error %v, want nil", err) + } + if got.Type != "" { + t.Errorf("CreateMapSearchRequest(LocationTypeAny) set Type=%q, want empty", got.Type) + } + if got.Keyword != "Dunkin'" { + t.Errorf("CreateMapSearchRequest kept Keyword=%q, want %q", got.Keyword, "Dunkin'") + } +} diff --git a/iowrappers/place_distance_sort.go b/iowrappers/place_distance_sort.go new file mode 100644 index 00000000..c4f23cf7 --- /dev/null +++ b/iowrappers/place_distance_sort.go @@ -0,0 +1,36 @@ +package iowrappers + +import ( + "sort" + + "github.com/weihesdlegend/Vacation-planner/POI" + "github.com/weihesdlegend/Vacation-planner/utils" +) + +// SortPlacesByDistance orders places ascending by distance from (lat, lng). +// +// Callers that truncate a candidate list to a limit MUST sort first. Only the Redis +// cache path returns places in distance order; the fresh path appends one place type's +// results after another in Google prominence order, so an unsorted slice[:limit] drops +// the last place types wholesale and can rank a 3km result above a 250m one. +// +// The sort is stable so places at equal distance keep their existing relative order. +func SortPlacesByDistance(places []POI.Place, lat, lng float64) { + origin := []float64{lat, lng} + dist := make(map[int]float64, len(places)) + for i := range places { + loc := places[i].GetLocation() + dist[i] = utils.HaversineDist(origin, []float64{loc.Latitude, loc.Longitude}) + } + idx := make([]int, len(places)) + for i := range idx { + idx[i] = i + } + sort.SliceStable(idx, func(a, b int) bool { return dist[idx[a]] < dist[idx[b]] }) + + sorted := make([]POI.Place, len(places)) + for newPos, oldPos := range idx { + sorted[newPos] = places[oldPos] + } + copy(places, sorted) +} diff --git a/iowrappers/place_distance_sort_test.go b/iowrappers/place_distance_sort_test.go new file mode 100644 index 00000000..494a73ce --- /dev/null +++ b/iowrappers/place_distance_sort_test.go @@ -0,0 +1,54 @@ +package iowrappers + +import ( + "testing" + + "github.com/weihesdlegend/Vacation-planner/POI" +) + +func placeAt(id string, lat, lng float64) POI.Place { + var p POI.Place + p.SetID(id) + p.SetLocationCoordinates([2]float64{lat, lng}) + return p +} + +// TestSortPlacesByDistance pins that truncation keeps the NEAREST places. The fresh +// (Google) path returns results grouped by place type in prominence order, so slicing +// without sorting first drops whole place types and can rank a 3km result above a 250m one. +func TestSortPlacesByDistance(t *testing.T) { + // State Street Market, Los Altos + lat, lng := 37.38006, -122.11612 + + places := []POI.Place{ + placeAt("far-sunnyvale", 37.3688, -122.0363), // ~7km east + placeAt("mid-mountainview", 37.3861, -122.0839), // ~3km east + placeAt("near-state-st", 37.38025, -122.11655), // ~40m away + } + + SortPlacesByDistance(places, lat, lng) + + want := []string{"near-state-st", "mid-mountainview", "far-sunnyvale"} + for i, id := range want { + if places[i].GetID() != id { + t.Errorf("position %d = %q, want %q (full order: %v)", i, places[i].GetID(), id, placeIDs(places)) + } + } +} + +// TestSortPlacesByDistanceEmpty pins that the no-result case does not panic. +func TestSortPlacesByDistanceEmpty(t *testing.T) { + var places []POI.Place + SortPlacesByDistance(places, 37.38006, -122.11612) + if len(places) != 0 { + t.Errorf("got %d places, want 0", len(places)) + } +} + +func placeIDs(places []POI.Place) []string { + ids := make([]string, 0, len(places)) + for _, p := range places { + ids = append(ids, p.GetID()) + } + return ids +} diff --git a/iowrappers/redis_client.go b/iowrappers/redis_client.go index fa5cd9e9..4505e2e8 100644 --- a/iowrappers/redis_client.go +++ b/iowrappers/redis_client.go @@ -179,7 +179,13 @@ func (r *RedisClient) StorePlacesForLocation(context context.Context, geocodeInS latLng, _ := utils.ParseLocation(geocodeInString) lat, lng := latLng[0], latLng[1] for _, place := range places { - sortedSetKey := strings.Join([]string{geocodeInString, string(POI.GetPlaceCategory(place.LocationType))}, "_") + placeCategory, ok := POI.GetPlaceCategory(place.LocationType) + if !ok { + Logger.Errorf("StorePlacesForLocation: place %s has unmapped location type %q, skipping", + place.ID, place.LocationType) + continue + } + sortedSetKey := strings.Join([]string{geocodeInString, string(placeCategory)}, "_") dist := utils.HaversineDist([]float64{lat, lng}, []float64{place.GetLocation().Latitude, place.GetLocation().Longitude}) _, err := client.ZAdd(context, sortedSetKey, redis.Z{Score: dist, Member: place.ID}).Result() if err != nil { @@ -219,7 +225,15 @@ func (r *RedisClient) SetPlacesAddGeoLocations(c context.Context, places []POI.P defer wg.Done() _, err := r.Get().Pipelined(c, func(pipe redis.Pipeliner) error { for _, place := range placeBatch { - placeCategory := POI.GetPlaceCategory(place.LocationType) + placeCategory, ok := POI.GetPlaceCategory(place.LocationType) + if !ok { + // Refuse to guess a bucket. A place whose type maps to no category + // came from a search whose type filter Google did not enforce, so + // writing it would poison whichever bucket we picked. + Logger.Errorf("SetPlacesAddGeoLocations: place %s has unmapped location type %q, skipping geo bucket write", + place.ID, place.LocationType) + continue + } geoLocation := &redis.GeoLocation{ Name: place.ID, Latitude: place.GetLocation().Latitude, diff --git a/planner/planner.go b/planner/planner.go index 623e1b52..b94cfe8d 100644 --- a/planner/planner.go +++ b/planner/planner.go @@ -277,6 +277,32 @@ func (p *MyPlanner) removePlacesMigrationHandler(ctx *gin.Context) { } } +// reclassifyBucketsMigrationHandler removes places from a category's geo buckets whose +// primary Google type does not belong to that category. Dry-run unless ?apply=true. +// +// Usage: GET /v1/migrate/reclassify-buckets?category=Eatery +// +// GET /v1/migrate/reclassify-buckets?category=Eatery&apply=true +func (p *MyPlanner) reclassifyBucketsMigrationHandler(ctx *gin.Context) { + _, authenticationErr := p.UserAuthentication(ctx, user.LevelAdmin) + if authenticationErr != nil { + ctx.JSON(http.StatusUnauthorized, gin.H{"error": authenticationErr.Error()}) + return + } + category, ok := POI.ParsePlaceCategory(ctx.DefaultQuery("category", string(POI.PlaceCategoryEatery))) + if !ok { + ctx.JSON(http.StatusBadRequest, gin.H{"error": "unknown category"}) + return + } + dryRun := ctx.Query("apply") != "true" + report, err := p.Solver.Searcher.RemoveMisclassifiedPlacesFromCategoryBuckets(ctx.Request.Context(), category, 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, "category": category, "report": report}) +} + func (p *MyPlanner) placeStatsHandler(ctx *gin.Context) { var placeCount int var err error @@ -807,7 +833,14 @@ func (p *MyPlanner) getUserSavedPlanDetails(ctx *gin.Context) { } resp.LatLongs[i] = [2]float64{place.Location.Latitude, place.Location.Longitude} resp.ShownActive[i] = i == 0 - resp.PlaceCategories[i] = POI.GetPlaceCategory(place.LocationType) + // Saved plans can contain older cached records, including brand-search places + // written with an empty LocationType. Preserve the historical Eatery default for + // display only — the write path (redis_client.go) is where guessing is unsafe. + if placeCategory, ok := POI.GetPlaceCategory(place.LocationType); ok { + resp.PlaceCategories[i] = placeCategory + } else { + resp.PlaceCategories[i] = POI.PlaceCategoryEatery + } details, err := p.placeDetailsResp(ctx, place) if err != nil { @@ -1410,7 +1443,10 @@ func (p *MyPlanner) getNearbyPlacesByCategory(ctx *gin.Context) { places = reclassified // drop places explicitly marked closed on the requested day places = iowrappers.Filter(places, func(place POI.Place) bool { return !place.KnownClosedOnDay(day) }) - // Redis results are sorted by distance ascending; keep the nearest ones + // Only the Redis path returns places in distance order. The fresh path + // appends each place type's results in Google prominence order, so sort + // before truncating or the last place types get dropped wholesale. + iowrappers.SortPlacesByDistance(places, req.Location.Latitude, req.Location.Longitude) if len(places) > limit { places = places[:limit] } @@ -1682,6 +1718,7 @@ func (p *MyPlanner) SetupRouter(serverPort string) *http.Server { migrations.GET("/user-ratings-total", p.UserRatingsTotalMigrationHandler) migrations.GET("/url", p.UrlMigrationHandler) migrations.GET("/remove-places", p.removePlacesMigrationHandler) + migrations.GET("/reclassify-buckets", p.reclassifyBucketsMigrationHandler) } v1.GET("/blob_url", p.getBlobObjectURL) diff --git a/planner/reclassify_buckets_dry_run_test.go b/planner/reclassify_buckets_dry_run_test.go new file mode 100644 index 00000000..5da8b8d0 --- /dev/null +++ b/planner/reclassify_buckets_dry_run_test.go @@ -0,0 +1,155 @@ +package planner + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "net/url" + "testing" + "time" + + "github.com/gin-gonic/gin" + "github.com/weihesdlegend/Vacation-planner/iowrappers" + "github.com/weihesdlegend/Vacation-planner/test/redis_client_mocks" + "github.com/weihesdlegend/Vacation-planner/user" +) + +// The reclassify-buckets migration deletes members from the shared place cache that the +// trip-planning path reads, and a deleted bucket row is not re-created until the city's +// 14-day MapsLastSearchTime marker expires. `dryRun := ctx.Query("apply") != "true"` is +// therefore the last line of defence on the endpoint: every value other than the exact +// string "true" — including the truthy-looking "TRUE" and "1" — must keep the run read-only. +func TestReclassifyBucketsMigrationDryRunDefault(t *testing.T) { + gin.SetMode(gin.TestMode) + + // point a real PoiSearcher at the same mock Redis the shared RedisClient uses, so the + // handler exercises the whole path down to RemoveMisclassifiedPlacesFromCategoryBuckets + redisURL, err := url.Parse("redis://" + redis_client_mocks.RedisMockSvr.Addr()) + if err != nil { + t.Fatalf("failed to parse mock redis URL: %v", err) + } + p := &MyPlanner{ + RedisClient: redis_client_mocks.RedisClient, + Solver: Solver{Searcher: iowrappers.CreatePoiSearcher("test-maps-api-key", redisURL)}, + } + + router := gin.New() + router.GET("/v1/migrate/reclassify-buckets", p.reclassifyBucketsMigrationHandler) + + adminToken := newAdminPAT(t, "reclassify_buckets_admin", "reclassify-buckets-admin-token") + + get := func(t *testing.T, query string) (int, map[string]any) { + t.Helper() + req := httptest.NewRequest(http.MethodGet, "/v1/migrate/reclassify-buckets"+query, nil) + req.Header.Set("Authorization", "Bearer "+adminToken) + w := httptest.NewRecorder() + router.ServeHTTP(w, req) + + var body map[string]any + if err := json.Unmarshal(w.Body.Bytes(), &body); err != nil { + t.Fatalf("failed to parse response %q: %v", w.Body.String(), err) + } + return w.Code, body + } + + tests := []struct { + name string + query string + wantDryRun bool + }{ + {name: "apply absent", query: "?category=Eatery", wantDryRun: true}, + {name: "apply empty", query: "?category=Eatery&apply=", wantDryRun: true}, + {name: "apply uppercase TRUE", query: "?category=Eatery&apply=TRUE", wantDryRun: true}, + {name: "apply 1", query: "?category=Eatery&apply=1", wantDryRun: true}, + {name: "apply yes", query: "?category=Eatery&apply=yes", wantDryRun: true}, + {name: "no query at all", query: "", wantDryRun: true}, + // the one and only spelling that is allowed to delete + {name: "apply exactly true", query: "?category=Eatery&apply=true", wantDryRun: false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + code, body := get(t, tt.query) + if code != http.StatusOK { + t.Fatalf("status = %d, want %d (body: %+v)", code, http.StatusOK, body) + } + dryRun, ok := body["dry_run"].(bool) + if !ok { + t.Fatalf("response has no boolean dry_run field: %+v", body) + } + if dryRun != tt.wantDryRun { + t.Errorf("dry_run = %v for %q, want %v", dryRun, tt.query, tt.wantDryRun) + } + }) + } +} + +// TestReclassifyBucketsMigrationRequiresAdmin pins that the destructive endpoint is not +// reachable without admin credentials, so the dry-run default is not its only protection. +func TestReclassifyBucketsMigrationRequiresAdmin(t *testing.T) { + gin.SetMode(gin.TestMode) + + redisURL, err := url.Parse("redis://" + redis_client_mocks.RedisMockSvr.Addr()) + if err != nil { + t.Fatalf("failed to parse mock redis URL: %v", err) + } + p := &MyPlanner{ + RedisClient: redis_client_mocks.RedisClient, + Solver: Solver{Searcher: iowrappers.CreatePoiSearcher("test-maps-api-key", redisURL)}, + } + + router := gin.New() + router.GET("/v1/migrate/reclassify-buckets", p.reclassifyBucketsMigrationHandler) + + get := func(authorization string) int { + req := httptest.NewRequest(http.MethodGet, "/v1/migrate/reclassify-buckets?category=Eatery&apply=true", nil) + if authorization != "" { + req.Header.Set("Authorization", authorization) + } + w := httptest.NewRecorder() + router.ServeHTTP(w, req) + return w.Code + } + + t.Run("no credentials", func(t *testing.T) { + if code := get(""); code != http.StatusUnauthorized { + t.Errorf("status = %d without credentials, want %d", code, http.StatusUnauthorized) + } + }) + + t.Run("regular user PAT", func(t *testing.T) { + token := newRegularPAT(t, "reclassify_buckets_regular", "reclassify-buckets-regular-token") + if code := get("Bearer " + token); code != http.StatusUnauthorized { + t.Errorf("status = %d for a non-admin PAT, want %d", code, http.StatusUnauthorized) + } + }) +} + +func newAdminPAT(t *testing.T, username, rawToken string) string { + t.Helper() + return newPATForLevel(t, username, rawToken, user.LevelStringAdmin) +} + +func newRegularPAT(t *testing.T, username, rawToken string) string { + t.Helper() + return newPATForLevel(t, username, rawToken, user.LevelStringRegular) +} + +func newPATForLevel(t *testing.T, username, rawToken, level string) string { + t.Helper() + userView, err := redis_client_mocks.RedisClient.CreateUser( + redis_client_mocks.RedisContext, + user.View{Username: username, Email: username + "@example.com", Password: "pwd", UserLevel: level}, + false, + ) + if err != nil { + t.Fatalf("failed to create %s test user: %v", level, err) + } + pat, err := redis_client_mocks.RedisClient.NewPAT( + redis_client_mocks.RedisContext, username+"-pat", userView.ID, rawToken, time.Hour, + ) + if err != nil { + t.Fatalf("failed to create test PAT: %v", err) + } + return pat.TokenHash +} diff --git a/test/place_category_test.go b/test/place_category_test.go index 5bd42249..82c8b61b 100644 --- a/test/place_category_test.go +++ b/test/place_category_test.go @@ -12,7 +12,6 @@ func TestGetPlaceTypesByCategory(t *testing.T) { POI.PlaceCategoryEatery: { POI.LocationTypeCafe, POI.LocationTypeRestaurant, POI.LocationTypeBar, POI.LocationTypeBakery, POI.LocationTypeMealTakeaway, - POI.LocationTypeFastFood, POI.LocationTypeFoodCourt, }, POI.PlaceCategoryShopping: { POI.LocationTypeShoppingMall, POI.LocationTypeDepartmentStore, @@ -45,13 +44,63 @@ func TestPlaceCategoryRoundTrip(t *testing.T) { } for _, category := range categories { for _, placeType := range POI.GetPlaceTypes(category) { - if got := POI.GetPlaceCategory(placeType); got != category { + got, ok := POI.GetPlaceCategory(placeType) + if !ok { + t.Errorf("round-trip broken: GetPlaceCategory(%q) has no category, want %q", placeType, category) + continue + } + if got != category { t.Errorf("round-trip broken: GetPlaceCategory(%q) = %q, want %q", placeType, got, category) } } } } +// TestGetPlaceCategoryRejectsUnknownTypes pins the fix for the fast_food_restaurant +// incident: GetPlaceCategory must NOT silently absorb unmapped types into Eatery. +// A default-to-Eatery branch made TestPlaceCategoryRoundTrip un-failable, so two +// Places-API-(New)-only types were added to GetPlaceTypes(Eatery) and hotels were +// written into the eatery geo buckets. +func TestGetPlaceCategoryRejectsUnknownTypes(t *testing.T) { + unknown := []POI.LocationType{ + POI.LocationType("fast_food_restaurant"), + POI.LocationType("food_court"), + POI.LocationType("lodging_but_not_really"), + POI.LocationType(""), + } + for _, placeType := range unknown { + if got, ok := POI.GetPlaceCategory(placeType); ok { + t.Errorf("GetPlaceCategory(%q) = (%q, true), want ok=false", placeType, got) + } + } +} + +// TestGetPlaceCategoryKnownTypes pins that every mapped type still resolves. +func TestGetPlaceCategoryKnownTypes(t *testing.T) { + cases := map[POI.LocationType]POI.PlaceCategory{ + POI.LocationTypeCafe: POI.PlaceCategoryEatery, + POI.LocationTypeRestaurant: POI.PlaceCategoryEatery, + POI.LocationTypeBar: POI.PlaceCategoryEatery, + POI.LocationTypeBakery: POI.PlaceCategoryEatery, + POI.LocationTypeMealTakeaway: POI.PlaceCategoryEatery, + POI.LocationTypePark: POI.PlaceCategoryVisit, + POI.LocationTypeMuseum: POI.PlaceCategoryVisit, + POI.LocationTypeStore: POI.PlaceCategoryShopping, + POI.LocationTypeLodging: POI.PlaceCategoryLodging, + POI.LocationTypeGym: POI.PlaceCategoryWellness, + } + for placeType, want := range cases { + got, ok := POI.GetPlaceCategory(placeType) + if !ok { + t.Errorf("GetPlaceCategory(%q) returned ok=false, want %q", placeType, want) + continue + } + if got != want { + t.Errorf("GetPlaceCategory(%q) = %q, want %q", placeType, got, want) + } + } +} + func TestParsePlaceCategory(t *testing.T) { valid := []string{"Visit", "Eatery", "Shopping", "Lodging", "Wellness"} for _, s := range valid { diff --git a/test/redis_client_mocks/bucket_cleanup_test.go b/test/redis_client_mocks/bucket_cleanup_test.go new file mode 100644 index 00000000..d11ff5e0 --- /dev/null +++ b/test/redis_client_mocks/bucket_cleanup_test.go @@ -0,0 +1,333 @@ +package redis_client_mocks + +import ( + "testing" + + "github.com/alicebob/miniredis/v2" + "github.com/weihesdlegend/Vacation-planner/POI" + "github.com/weihesdlegend/Vacation-planner/iowrappers" +) + +// bucketCleanupFixtureIDs lists every place ID this test file writes into Redis. +// RedisClient/RedisMockSvr are process-wide fixtures shared with every other test in this +// package (some of which seed cities and places once via their own package init()), so +// resetBucketCleanupFixtures below clears only these specific keys between runs rather than +// flushing the whole mock server, which would also erase those unrelated fixtures. +var bucketCleanupFixtureIDs = []string{ + "hotel-1", "cafe-1", "hotel-2", "cafe-2", "legacy-1", + "tt-lodging", "tt-supermarket", "tt-meal-delivery", "tt-night-club", "tt-no-types", "tt-cafe", + "tt-orphan", +} + +// resetBucketCleanupFixtures gives each test in this file a clean slate for its own fixture +// IDs without disturbing state other test files depend on. +func resetBucketCleanupFixtures(t *testing.T) { + t.Helper() + detailKeys := make([]string, 0, len(bucketCleanupFixtureIDs)) + for _, id := range bucketCleanupFixtureIDs { + detailKeys = append(detailKeys, iowrappers.PlaceDetailsRedisKeyPrefix+id) + } + 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) + } + } + } +} + +// TestRemoveMisclassifiedPlacesDryRun pins that a dry run reports the hotels that the +// fast_food_restaurant incident wrote into placeIDs:eatery:level* without deleting them. +func TestRemoveMisclassifiedPlacesDryRun(t *testing.T) { + hotel := newPlaceWithTypes("hotel-1", "Residence Inn by Marriott Palo Alto", + POI.LocationType("fast_food_restaurant"), []string{"lodging", "point_of_interest", "establishment"}) + cafe := newPlaceWithTypes("cafe-1", "Peet's Coffee", + POI.LocationTypeCafe, []string{"cafe", "food", "point_of_interest", "establishment"}) + RedisClient.SetPlacesAddGeoLocations(RedisContext, []POI.Place{cafe}) + seedGeoBucket(t, POI.PlaceCategoryEatery, hotel) + + report, err := RedisClient.RemoveMisclassifiedPlacesFromCategoryBuckets(RedisContext, POI.PlaceCategoryEatery, true) + if err != nil { + t.Fatalf("RemoveMisclassifiedPlacesFromCategoryBuckets error: %v", err) + } + if report.Misclassified != 1 { + t.Errorf("Misclassified = %d, want 1 (report: %+v)", report.Misclassified, report) + } + if report.Removed != 0 { + t.Errorf("dry run Removed = %d, want 0", report.Removed) + } + if len(report.RemovedIDs) != 1 || report.RemovedIDs[0] != "hotel-1" { + t.Errorf("RemovedIDs = %v, want [hotel-1]", report.RemovedIDs) + } + // the hotel must still be present after a dry run + if got := countInEateryBuckets(t, "hotel-1"); got == 0 { + t.Error("dry run deleted hotel-1, want it retained") + } +} + +// TestRemoveMisclassifiedPlacesApply pins that a real run removes only the hotel. +func TestRemoveMisclassifiedPlacesApply(t *testing.T) { + resetBucketCleanupFixtures(t) + + hotel := newPlaceWithTypes("hotel-2", "The Westin Palo Alto", + POI.LocationType("fast_food_restaurant"), []string{"lodging", "point_of_interest", "establishment"}) + cafe := newPlaceWithTypes("cafe-2", "Red Rock Coffee", + POI.LocationTypeCafe, []string{"cafe", "food", "point_of_interest", "establishment"}) + RedisClient.SetPlacesAddGeoLocations(RedisContext, []POI.Place{cafe}) + seedGeoBucket(t, POI.PlaceCategoryEatery, hotel) + + report, err := RedisClient.RemoveMisclassifiedPlacesFromCategoryBuckets(RedisContext, POI.PlaceCategoryEatery, false) + if err != nil { + t.Fatalf("RemoveMisclassifiedPlacesFromCategoryBuckets error: %v", err) + } + if report.Removed != 1 { + t.Errorf("Removed = %d, want 1 (report: %+v)", report.Removed, report) + } + if got := countInEateryBuckets(t, "hotel-2"); got != 0 { + t.Errorf("hotel-2 still in %d eatery buckets, want 0", got) + } + if got := countInEateryBuckets(t, "cafe-2"); got == 0 { + t.Error("cafe-2 was removed, want it retained") + } +} + +// TestRemoveMisclassifiedPlacesKeepsUntypedRecords pins that older cached records with +// no Types list are left alone, matching ReclassifyForCategory's keep-on-unknown rule. +func TestRemoveMisclassifiedPlacesKeepsUntypedRecords(t *testing.T) { + resetBucketCleanupFixtures(t) + + legacy := newPlaceWithTypes("legacy-1", "Old Cached Diner", POI.LocationTypeRestaurant, nil) + RedisClient.SetPlacesAddGeoLocations(RedisContext, []POI.Place{legacy}) + + report, err := RedisClient.RemoveMisclassifiedPlacesFromCategoryBuckets(RedisContext, POI.PlaceCategoryEatery, false) + if err != nil { + t.Fatalf("RemoveMisclassifiedPlacesFromCategoryBuckets error: %v", err) + } + if report.Removed != 0 { + t.Errorf("Removed = %d, want 0 — records without Types must be kept", report.Removed) + } +} + +// TestRemoveMisclassifiedPlacesPrimaryTypeTruthTable pins the exact rule the cleanup applies: +// a bucket member is removed only when its PRIMARY Google type positively maps to a DIFFERENT +// category. An unmapped primary type is NOT evidence of misclassification — the fixed write +// path keys on the stamped LocationType, so it would legitimately file a delivery-first +// restaurant ("meal_delivery" first in Types) or a bar/club ("night_club" first) under Eatery. +// Purging those would make the migration delete rows the write path immediately re-creates, +// while shrinking the trip-planning candidate pool for up to MinMapsResultRefreshDuration. +func TestRemoveMisclassifiedPlacesPrimaryTypeTruthTable(t *testing.T) { + resetBucketCleanupFixtures(t) + t.Cleanup(func() { resetBucketCleanupFixtures(t) }) + + cases := []struct { + id string + name string + stamped POI.LocationType + types []string + wantRemove bool + why string + }{ + { + id: "tt-lodging", name: "Residence Inn by Marriott Palo Alto", + // the incident: searched as fast_food_restaurant, truthfully a hotel + stamped: POI.LocationType("fast_food_restaurant"), + types: []string{"lodging", "point_of_interest", "establishment"}, + // GetPlaceCategory("lodging") == (Lodging, true) != Eatery + wantRemove: true, why: "primary type lodging maps to Lodging", + }, + { + id: "tt-supermarket", name: "Whole Foods Market", + stamped: POI.LocationTypeRestaurant, + types: []string{"supermarket", "grocery_or_supermarket", "food", "store"}, + // GetPlaceCategory("supermarket") == (Shopping, true) != Eatery + wantRemove: true, why: "primary type supermarket maps to Shopping", + }, + { + id: "tt-meal-delivery", name: "Wok This Way Delivery", + stamped: POI.LocationTypeRestaurant, + types: []string{"meal_delivery", "restaurant", "food", "point_of_interest"}, + // GetPlaceCategory("meal_delivery") == ("", false): legal legacy type, unmapped + wantRemove: false, why: "primary type meal_delivery maps to no category", + }, + { + id: "tt-night-club", name: "The Basement", + stamped: POI.LocationTypeBar, + types: []string{"night_club", "bar", "point_of_interest", "establishment"}, + // GetPlaceCategory("night_club") == ("", false): legal legacy type, unmapped + wantRemove: false, why: "primary type night_club maps to no category", + }, + { + id: "tt-no-types", name: "Old Cached Diner", + stamped: POI.LocationTypeRestaurant, + types: nil, + // PrimaryLocationType(nil) == "" -> GetPlaceCategory("") == ("", false) + wantRemove: false, why: "no Types at all (record cached before Types was captured)", + }, + { + id: "tt-cafe", name: "Peet's Coffee", + stamped: POI.LocationTypeCafe, + types: []string{"cafe", "food", "point_of_interest", "establishment"}, + // GetPlaceCategory("cafe") == (Eatery, true) == the bucket's category + wantRemove: false, why: "primary type cafe maps to Eatery", + }, + } + + for _, tc := range cases { + seedGeoBucket(t, POI.PlaceCategoryEatery, newPlaceWithTypes(tc.id, tc.name, tc.stamped, tc.types)) + } + + report, err := RedisClient.RemoveMisclassifiedPlacesFromCategoryBuckets(RedisContext, POI.PlaceCategoryEatery, false) + if err != nil { + t.Fatalf("RemoveMisclassifiedPlacesFromCategoryBuckets error: %v", err) + } + + removed := make(map[string]bool, len(report.RemovedIDs)) + for _, id := range report.RemovedIDs { + removed[id] = true + } + + for _, tc := range cases { + t.Run(tc.id, func(t *testing.T) { + if removed[tc.id] != tc.wantRemove { + t.Errorf("reported removal of %s = %v, want %v (%s); report: %+v", + tc.id, removed[tc.id], tc.wantRemove, tc.why, report) + } + inBuckets := countInEateryBuckets(t, tc.id) + if tc.wantRemove && inBuckets != 0 { + t.Errorf("%s still in %d eatery buckets after apply, want 0 (%s)", tc.id, inBuckets, tc.why) + } + if !tc.wantRemove && inBuckets == 0 { + t.Errorf("%s was deleted from the eatery buckets, want it retained (%s)", tc.id, tc.why) + } + }) + } +} + +// TestRemoveMisclassifiedPlacesToleratesMissingRecords pins that a bucket member with no +// backing place_details record is counted, skipped, and left in place rather than failing the +// run or being deleted. Orphaned members are RemovePlaces' job, not this migration's. This +// guards the batched read path specifically: a missing key surfaces as redis.Nil inside the +// pipeline, which must not be mistaken for a transport failure and abort the whole scan. +func TestRemoveMisclassifiedPlacesToleratesMissingRecords(t *testing.T) { + resetBucketCleanupFixtures(t) + t.Cleanup(func() { resetBucketCleanupFixtures(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) + if err := RedisClient.AddGeoLocation(RedisContext, key, orphan); err != nil { + t.Fatalf("AddGeoLocation(%s): %v", key, err) + } + // a real misclassified place in the same batch, to prove the scan keeps going + seedGeoBucket(t, POI.PlaceCategoryEatery, newPlaceWithTypes("tt-lodging", "The Westin Palo Alto", + POI.LocationType("fast_food_restaurant"), []string{"lodging", "point_of_interest", "establishment"})) + + report, err := RedisClient.RemoveMisclassifiedPlacesFromCategoryBuckets(RedisContext, POI.PlaceCategoryEatery, false) + if err != nil { + t.Fatalf("a bucket member with no place record must not fail the run: %v", err) + } + if report.Scanned < 2 { + t.Errorf("Scanned = %d, want at least the 2 seeded members (report: %+v)", report.Scanned, report) + } + for _, id := range report.RemovedIDs { + if id == "tt-orphan" { + t.Errorf("tt-orphan was reported for removal; a member with no record must be skipped (report: %+v)", report) + } + } + if got := countInEateryBuckets(t, "tt-orphan"); got == 0 { + t.Error("tt-orphan was deleted; a member with no record must be left for RemovePlaces") + } + // the scan must not have stopped at the orphan + if got := countInEateryBuckets(t, "tt-lodging"); got != 0 { + t.Errorf("tt-lodging still in %d eatery buckets; the scan stopped at the orphaned member", got) + } +} + +// TestRemoveMisclassifiedPlacesReportsBucketSizes pins that the report states the scale of the +// scan up front. An operator has to review a dry-run report before applying, so the report has +// to say how many members exist even when the scan itself is what makes the run expensive. +func TestRemoveMisclassifiedPlacesReportsBucketSizes(t *testing.T) { + resetBucketCleanupFixtures(t) + t.Cleanup(func() { resetBucketCleanupFixtures(t) }) + + seedGeoBucket(t, POI.PlaceCategoryEatery, newPlaceWithTypes("tt-cafe", "Peet's Coffee", + POI.LocationTypeCafe, []string{"cafe", "food", "point_of_interest", "establishment"})) + + report, err := RedisClient.RemoveMisclassifiedPlacesFromCategoryBuckets(RedisContext, POI.PlaceCategoryEatery, true) + if err != nil { + 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) + } + } + if report.TotalMembers < 1 { + t.Errorf("TotalMembers = %d, want at least the 1 seeded place", report.TotalMembers) + } + // ZCARD is taken before the scan; nothing writes concurrently in this test, so every + // counted member must also have been visited. + if int(report.TotalMembers) != report.Scanned { + t.Errorf("TotalMembers = %d but Scanned = %d, want equal", report.TotalMembers, report.Scanned) + } +} + +func newPlaceWithTypes(id, name string, locationType POI.LocationType, types []string) POI.Place { + var p POI.Place + p.SetID(id) + p.SetName(name) + p.SetType(locationType) + p.SetStatus(string(POI.Operational)) + p.SetPriceLevel(POI.PriceLevelDefault) + p.SetUserRatingsTotal(100) + p.SetLocationCoordinates([2]float64{37.38006, -122.11612}) + p.Types = types + return p +} + +// seedGeoBucket writes a place record plus its eatery geo-bucket membership directly, +// bypassing SetPlacesAddGeoLocations, which after Task 1 refuses unmapped types. +func seedGeoBucket(t *testing.T, cat POI.PlaceCategory, place POI.Place) { + t.Helper() + if err := RedisClient.SetPlace(RedisContext, place); err != nil { + t.Fatalf("SetPlace(%s): %v", place.GetID(), err) + } + key := POI.EncodeNearbySearchRedisKey(cat, place.PriceLevel) + if err := RedisClient.AddGeoLocation(RedisContext, key, place); err != nil { + t.Fatalf("AddGeoLocation(%s): %v", key, err) + } +} + +func countInEateryBuckets(t *testing.T, placeID string) int { + t.Helper() + count := 0 + for _, lvl := range POI.AllPriceLevels { + key := POI.EncodeNearbySearchRedisKey(POI.PlaceCategoryEatery, lvl) + if RedisMockSvr.Exists(key) { + members, err := RedisMockSvr.ZMembers(key) + if err != nil { + continue + } + for _, m := range members { + if m == placeID { + count++ + } + } + } + } + return count +}