From 0d603ad15f42d2b795cf12526ee1d97f7d667812 Mon Sep 17 00:00:00 2001 From: Vladimir Zagainov Date: Tue, 18 Aug 2026 14:25:34 +0300 Subject: [PATCH] fix: address code review findings --- cmd/api/handlers.go | 14 +++++++++++++- cmd/cron/station_status.go | 22 ++++++++++++++++------ internal/cache/preferences.go | 17 ++++++++--------- internal/cache/store.go | 5 +++++ internal/routing/graph.go | 14 ++++++++------ internal/routing/search_cache.go | 8 ++++++-- 6 files changed, 56 insertions(+), 24 deletions(-) diff --git a/cmd/api/handlers.go b/cmd/api/handlers.go index b75194f..cc90685 100644 --- a/cmd/api/handlers.go +++ b/cmd/api/handlers.go @@ -205,6 +205,18 @@ func RouteSearch(hc *HandlerContext, w http.ResponseWriter, r *http.Request) { RankingMode: rankingMode, } + // Determine if this is a far-term search (date is more than 7 days in the future) + if req.Date != "" { + requestDate, err := time.Parse("2006-01-02", req.Date) + if err == nil { + now := time.Now() + daysDiff := int(requestDate.Sub(now).Hours() / 24) + if daysDiff >= 7 { + opts.FarTerm = true + } + } + } + // Detect closed stations and get neighbors for fallback closedStationsMap := make(map[string]bool) neighborsMap := make(map[string][]storage.StationNeighbor) @@ -431,7 +443,7 @@ func StationStatus(hc *HandlerContext, w http.ResponseWriter, r *http.Request) { func adminAuth(hc *HandlerContext, w http.ResponseWriter, r *http.Request) bool { expectedAPIKey := os.Getenv("TRIP_PLANNER_ADMIN_API_KEY") if expectedAPIKey == "" { - http.Error(w, "unauthorized", http.StatusUnauthorized) + http.Error(w, "server configuration error: TRIP_PLANNER_ADMIN_API_KEY is not set", http.StatusInternalServerError) return false } providedAPIKey := r.Header.Get("X-Admin-Api-Key") diff --git a/cmd/cron/station_status.go b/cmd/cron/station_status.go index f64bbbb..f05ed94 100644 --- a/cmd/cron/station_status.go +++ b/cmd/cron/station_status.go @@ -131,12 +131,17 @@ func (sm *StationMonitor) updateStationStatus(ctx context.Context, tripCount int zeroSinceData, err := sm.Cache.Get(ctx, zeroSinceKey) var zeroSince time.Time if err == nil && zeroSinceData != nil { - var zeroSinceUnix int64 - _, parseErr := fmt.Sscanf(string(zeroSinceData), "%d", &zeroSinceUnix) - if parseErr == nil { - zeroSince = time.Unix(zeroSinceUnix, 0) - } else { + // Handle "0" marker for time.Time{} (no zero-since) + if string(zeroSinceData) == "0" { zeroSince = time.Time{} + } else { + var zeroSinceUnix int64 + _, parseErr := fmt.Sscanf(string(zeroSinceData), "%d", &zeroSinceUnix) + if parseErr == nil { + zeroSince = time.Unix(zeroSinceUnix, 0) + } else { + zeroSince = time.Time{} + } } } @@ -184,7 +189,12 @@ func (sm *StationMonitor) updateStationStatus(ctx context.Context, tripCount int } // Write updated zero-since timestamp to cache with 24h TTL - if err := sm.Cache.Set(ctx, zeroSinceKey, []byte(fmt.Sprintf("%d", zeroSince.Unix())), 24*time.Hour); err != nil { + // Use "0" marker for time.Time{} to indicate no zero-since + zeroSinceStr := "0" + if !zeroSince.IsZero() { + zeroSinceStr = fmt.Sprintf("%d", zeroSince.Unix()) + } + if err := sm.Cache.Set(ctx, zeroSinceKey, []byte(zeroSinceStr), 24*time.Hour); err != nil { return newStatus, fmt.Errorf("cache set zero since: %w", err) } diff --git a/internal/cache/preferences.go b/internal/cache/preferences.go index 95d8093..94e88d7 100644 --- a/internal/cache/preferences.go +++ b/internal/cache/preferences.go @@ -107,15 +107,6 @@ func (p *Preferences) AddSavedCity(ctx context.Context, userID, cityCode, cityNa // RemoveSavedCity removes a city from the user's saved cities. func (p *Preferences) RemoveSavedCity(ctx context.Context, userID, cityCode string) error { - key := &CacheKey{ - Kind: "prefs:saved_city:" + userID, - Code: userID, - From: "", - To: "", - Date: "", - Request: "", - } - // Load existing cities cities, err := p.GetSavedCities(ctx, userID) if err != nil { @@ -132,6 +123,10 @@ func (p *Preferences) RemoveSavedCity(ctx context.Context, userID, cityCode stri if len(result) == 0 { // If no cities left, delete the key + key := &CacheKey{ + Kind: "prefs:saved_city:" + userID, + Code: userID, + } return p.store.Delete(ctx, key) } @@ -141,6 +136,10 @@ func (p *Preferences) RemoveSavedCity(ctx context.Context, userID, cityCode stri return err } + key := &CacheKey{ + Kind: "prefs:saved_city:" + userID, + Code: userID, + } return p.store.Set(ctx, key, data, PreferenceTTL) } diff --git a/internal/cache/store.go b/internal/cache/store.go index 640701e..c473127 100644 --- a/internal/cache/store.go +++ b/internal/cache/store.go @@ -167,6 +167,11 @@ func GetSearchKey(from, to, date string) *CacheKey { return &CacheKey{Kind: "search", From: from, To: to, Date: date} } +// GetSearchKeyWithFarTerm returns the cache key for a search query with far-term flag. +func GetSearchKeyWithFarTerm(from, to, date, farTermFlag string) *CacheKey { + return &CacheKey{Kind: "search", From: from, To: to, Date: date, Request: farTermFlag} +} + // CacheAside represents the cache-aside pattern implementation. // It follows the pattern: Redis → miss → Postgres/API → write-back to Redis. type CacheAside struct { diff --git a/internal/routing/graph.go b/internal/routing/graph.go index 70be1c4..4ff5c17 100644 --- a/internal/routing/graph.go +++ b/internal/routing/graph.go @@ -2,6 +2,7 @@ package routing import ( "context" + "log" "sort" "time" "trip-planner/internal/storage" @@ -626,9 +627,10 @@ func (g *Graph) FindRoute(originID, destID string, opts SearchOptions, closedSta "date": time.Now().Format("2006-01-02"), } - resp, err := yandexClient.Do(context.Background(), "GET", "/v3.0/search/", query) + resp, err := yandexClient.Do(context.TODO(), "GET", "/v3.0/search/", query) if err != nil { - // If API call fails, return nil (no route found) + // If API call fails, log the error and return nil (no route found) + log.Printf("WARNING: yandex search failed for route expansion: %v", err) return nil } @@ -1106,9 +1108,9 @@ func (g *Graph) checkRouteForChanges(itinerary *Itinerary) bool { // rescheduleRoute performs a re-search for the route with updated graph data. // This is called when significant changes are detected in the route legs. -func (g *Graph) rescheduleRoute(originID, destID string, opts SearchOptions, closedStations map[string]bool, neighbors map[string][]storage.StationNeighbor) *Itinerary { +func (g *Graph) rescheduleRoute(originID, destID string, opts SearchOptions, closedStations map[string]bool, neighbors map[string][]storage.StationNeighbor, yclient ...*yandex.Client) *Itinerary { // Re-run the search with the same options to get an updated route - result := g.FindRoute(originID, destID, opts, closedStations, neighbors) + result := g.FindRoute(originID, destID, opts, closedStations, neighbors, yclient...) if result != nil { result.LastChecked = time.Now().Unix() result.NeedsReSearch = false @@ -1119,9 +1121,9 @@ func (g *Graph) rescheduleRoute(originID, destID string, opts SearchOptions, clo // CheckAndRescheduleRoute checks a route for changes and returns an updated route if needed. // This is the main entry point for flight change notification logic. -func (g *Graph) CheckAndRescheduleRoute(itinerary *Itinerary, originID, destID string, opts SearchOptions, closedStations map[string]bool, neighbors map[string][]storage.StationNeighbor) *Itinerary { +func (g *Graph) CheckAndRescheduleRoute(itinerary *Itinerary, originID, destID string, opts SearchOptions, closedStations map[string]bool, neighbors map[string][]storage.StationNeighbor, yclient ...*yandex.Client) *Itinerary { if g.checkRouteForChanges(itinerary) { - return g.rescheduleRoute(originID, destID, opts, closedStations, neighbors) + return g.rescheduleRoute(originID, destID, opts, closedStations, neighbors, yclient...) } return itinerary } diff --git a/internal/routing/search_cache.go b/internal/routing/search_cache.go index 495870e..af79cdd 100644 --- a/internal/routing/search_cache.go +++ b/internal/routing/search_cache.go @@ -29,8 +29,12 @@ func NewSearchCacheService(cacheStore cache.Cache, yclient *yandex.Client, m *me // SearchWithCache performs a route search with caching support. // It uses the cache-aside pattern: try cache first, then Yandex API, then write back to cache. func (s *SearchCacheService) SearchWithCache(ctx context.Context, from, to, date string, opts SearchOptions) (*yandex.Response, error) { - // Generate cache key - searchKey := cache.GetSearchKey(from, to, date) + // Generate cache key including far-term flag to distinguish near-term vs far-term searches + farTermFlag := "near" + if opts.FarTerm { + farTermFlag = "far" + } + searchKey := cache.GetSearchKeyWithFarTerm(from, to, date, farTermFlag) // Try to get from cache first fetchFunc := func() ([]byte, error) {