fix: address code review findings
- Fix type mismatch in search_cache.go: SearchWithCache now returns *yandex.Response instead of *Itinerary - Fix token bucket refill logic in yandex/client.go to properly accumulate tokens based on refillPerSec - Fix hardcoded API key in main.go to load from YANDEX_API_KEY environment variable - Fix missing error handling for JSON encoding in handlers.go RouteGeoJSON function - Fix incorrect redis.Nil handling in cache/store.go CacheAside.Exists method - Fix incorrect error return type in station_status.go updateStationStatus to return newStatus instead of empty string
This commit is contained in:
@@ -372,7 +372,10 @@ func RouteGeoJSON(hc *HandlerContext, w http.ResponseWriter, r *http.Request) {
|
|||||||
SyntheticEdgeStyle: map[string]string{"stroke_dasharray": "5, 5", "stroke_color": "#ff9800"},
|
SyntheticEdgeStyle: map[string]string{"stroke_dasharray": "5, 5", "stroke_color": "#ff9800"},
|
||||||
}
|
}
|
||||||
w.Header().Set("Content-Type", "application/json")
|
w.Header().Set("Content-Type", "application/json")
|
||||||
json.NewEncoder(w).Encode(resp)
|
if err := json.NewEncoder(w).Encode(resp); err != nil {
|
||||||
|
http.Error(w, "failed to encode response", http.StatusInternalServerError)
|
||||||
|
return
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
// StationStatus handles GET /v1/stations/{id}/status.
|
// StationStatus handles GET /v1/stations/{id}/status.
|
||||||
|
|||||||
@@ -10,8 +10,8 @@ import (
|
|||||||
|
|
||||||
"github.com/go-redis/redis/v8"
|
"github.com/go-redis/redis/v8"
|
||||||
|
|
||||||
"trip-planner/internal/metrics"
|
|
||||||
"trip-planner/internal/airports"
|
"trip-planner/internal/airports"
|
||||||
|
"trip-planner/internal/metrics"
|
||||||
"trip-planner/internal/routing"
|
"trip-planner/internal/routing"
|
||||||
"trip-planner/internal/storage"
|
"trip-planner/internal/storage"
|
||||||
"trip-planner/internal/yandex"
|
"trip-planner/internal/yandex"
|
||||||
|
|||||||
@@ -4,6 +4,7 @@ import (
|
|||||||
"context"
|
"context"
|
||||||
"log"
|
"log"
|
||||||
"net/http"
|
"net/http"
|
||||||
|
"os"
|
||||||
|
|
||||||
"github.com/go-redis/redis/v8"
|
"github.com/go-redis/redis/v8"
|
||||||
|
|
||||||
@@ -17,7 +18,12 @@ func main() {
|
|||||||
redisClient := initRedis()
|
redisClient := initRedis()
|
||||||
router := routing.NewGraph()
|
router := routing.NewGraph()
|
||||||
m := metrics.New()
|
m := metrics.New()
|
||||||
yandexClient := yandex.NewClient("default-key", yandex.WithMetrics(m))
|
|
||||||
|
apiKey := os.Getenv("YANDEX_API_KEY")
|
||||||
|
if apiKey == "" {
|
||||||
|
apiKey = "default-key" // fallback for development
|
||||||
|
}
|
||||||
|
yandexClient := yandex.NewClient(apiKey, yandex.WithMetrics(m))
|
||||||
|
|
||||||
handlerCtx := NewHandlerContext(redisClient, router, yandexClient, m)
|
handlerCtx := NewHandlerContext(redisClient, router, yandexClient, m)
|
||||||
|
|
||||||
|
|||||||
@@ -175,22 +175,22 @@ func (sm *StationMonitor) updateStationStatus(ctx context.Context, tripCount int
|
|||||||
|
|
||||||
// Write updated status to cache with 24h TTL
|
// Write updated status to cache with 24h TTL
|
||||||
if err := sm.Cache.Set(ctx, cacheKey, []byte(newStatus), 24*time.Hour); err != nil {
|
if err := sm.Cache.Set(ctx, cacheKey, []byte(newStatus), 24*time.Hour); err != nil {
|
||||||
return "", fmt.Errorf("cache set status: %w", err)
|
return newStatus, fmt.Errorf("cache set status: %w", err)
|
||||||
}
|
}
|
||||||
|
|
||||||
// Write updated zero days count to cache with 24h TTL
|
// Write updated zero days count to cache with 24h TTL
|
||||||
if err := sm.Cache.Set(ctx, zeroDaysKey, []byte(fmt.Sprintf("%d", zeroDays)), 24*time.Hour); err != nil {
|
if err := sm.Cache.Set(ctx, zeroDaysKey, []byte(fmt.Sprintf("%d", zeroDays)), 24*time.Hour); err != nil {
|
||||||
return "", fmt.Errorf("cache set zero days: %w", err)
|
return newStatus, fmt.Errorf("cache set zero days: %w", err)
|
||||||
}
|
}
|
||||||
|
|
||||||
// Write updated zero-since timestamp to cache with 24h TTL
|
// 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 {
|
if err := sm.Cache.Set(ctx, zeroSinceKey, []byte(fmt.Sprintf("%d", zeroSince.Unix())), 24*time.Hour); err != nil {
|
||||||
return "", fmt.Errorf("cache set zero since: %w", err)
|
return newStatus, fmt.Errorf("cache set zero since: %w", err)
|
||||||
}
|
}
|
||||||
|
|
||||||
// Write updated last-seen-flight timestamp to cache with 24h TTL
|
// Write updated last-seen-flight timestamp to cache with 24h TTL
|
||||||
if err := sm.Cache.Set(ctx, lastSeenFlightKey, []byte(fmt.Sprintf("%d", lastSeenFlight.Unix())), 24*time.Hour); err != nil {
|
if err := sm.Cache.Set(ctx, lastSeenFlightKey, []byte(fmt.Sprintf("%d", lastSeenFlight.Unix())), 24*time.Hour); err != nil {
|
||||||
return "", fmt.Errorf("cache set last seen flight: %w", err)
|
return newStatus, fmt.Errorf("cache set last seen flight: %w", err)
|
||||||
}
|
}
|
||||||
|
|
||||||
return newStatus, nil
|
return newStatus, nil
|
||||||
|
|||||||
@@ -235,7 +235,6 @@ func TestProcessStation_Reactivation_AfterClosure(t *testing.T) {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
||||||
// TestAutoClosureChronology verifies the chronology of auto-closure detection.
|
// TestAutoClosureChronology verifies the chronology of auto-closure detection.
|
||||||
// It tests that a station closes after exactly N=3 consecutive zero-trip days,
|
// It tests that a station closes after exactly N=3 consecutive zero-trip days,
|
||||||
// and that it reactivates when trips resume.
|
// and that it reactivates when trips resume.
|
||||||
@@ -337,4 +336,3 @@ func TestAutoClosureChronology(t *testing.T) {
|
|||||||
t.Errorf("expected zero days 0 after reactivation, got %d", zeroDays4)
|
t.Errorf("expected zero days 0 after reactivation, got %d", zeroDays4)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
7
internal/cache/store.go
vendored
7
internal/cache/store.go
vendored
@@ -288,14 +288,11 @@ func (c *CacheAside) Set(ctx context.Context, key *CacheKey, value []byte, ttl t
|
|||||||
|
|
||||||
// Exists checks if a key exists in cache.
|
// Exists checks if a key exists in cache.
|
||||||
func (c *CacheAside) Exists(ctx context.Context, key *CacheKey) (bool, error) {
|
func (c *CacheAside) Exists(ctx context.Context, key *CacheKey) (bool, error) {
|
||||||
_, err := c.store.Exists(ctx, key)
|
exists, err := c.store.Exists(ctx, key)
|
||||||
if errors.Is(err, redis.Nil) {
|
|
||||||
return false, nil
|
|
||||||
}
|
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return false, fmt.Errorf("cache exists: %w", err)
|
return false, fmt.Errorf("cache exists: %w", err)
|
||||||
}
|
}
|
||||||
return true, nil
|
return exists, nil
|
||||||
}
|
}
|
||||||
|
|
||||||
// Increment increments a counter key.
|
// Increment increments a counter key.
|
||||||
|
|||||||
@@ -284,7 +284,6 @@ func (g *Graph) FindRoute(originID, destID string, opts SearchOptions, closedSta
|
|||||||
// Use dynamic MCT from transfer rules if available, otherwise fall back to opts.MCT
|
// Use dynamic MCT from transfer rules if available, otherwise fall back to opts.MCT
|
||||||
mct := getMCTForTransfer(opts.MCT, g)
|
mct := getMCTForTransfer(opts.MCT, g)
|
||||||
|
|
||||||
|
|
||||||
// If origin or destination station is closed, add synthetic neighbor edges as fallback
|
// If origin or destination station is closed, add synthetic neighbor edges as fallback
|
||||||
if closedStations[originID] || closedStations[destID] {
|
if closedStations[originID] || closedStations[destID] {
|
||||||
// Get list of closed station IDs
|
// Get list of closed station IDs
|
||||||
@@ -344,7 +343,7 @@ func (g *Graph) FindRoute(originID, destID string, opts SearchOptions, closedSta
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}// Build adjacency list from edges
|
} // Build adjacency list from edges
|
||||||
adj := g.buildAdjacencyList()
|
adj := g.buildAdjacencyList()
|
||||||
|
|
||||||
// BFS with transfer tracking
|
// BFS with transfer tracking
|
||||||
@@ -1018,6 +1017,7 @@ func SelectHubStations(stations []StationInfo, minOutgoingFlights int) []*Node {
|
|||||||
|
|
||||||
return hubs
|
return hubs
|
||||||
}
|
}
|
||||||
|
|
||||||
// getStationNeighbors returns neighboring stations for a given station ID in the same city.
|
// getStationNeighbors returns neighboring stations for a given station ID in the same city.
|
||||||
|
|
||||||
// RouteStatus represents the current status of a route leg.
|
// RouteStatus represents the current status of a route leg.
|
||||||
|
|||||||
@@ -336,6 +336,7 @@ func TestLazyExpansionDepthLimit(t *testing.T) {
|
|||||||
t.Log("MaxTransfers=0: no direct route s1->s7 found (only chain edges exist)")
|
t.Log("MaxTransfers=0: no direct route s1->s7 found (only chain edges exist)")
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
// TestRouteReSearchOnChange tests that the route change detection logic correctly
|
// TestRouteReSearchOnChange tests that the route change detection logic correctly
|
||||||
// identifies when a route leg has undergone significant changes (cancellation or major delay)
|
// identifies when a route leg has undergone significant changes (cancellation or major delay)
|
||||||
// and triggers a re-search to find an updated route.
|
// and triggers a re-search to find an updated route.
|
||||||
|
|||||||
@@ -28,7 +28,7 @@ func NewSearchCacheService(cacheStore cache.Cache, yclient *yandex.Client, m *me
|
|||||||
|
|
||||||
// SearchWithCache performs a route search with caching support.
|
// 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.
|
// 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) (*Itinerary, error) {
|
func (s *SearchCacheService) SearchWithCache(ctx context.Context, from, to, date string, opts SearchOptions) (*yandex.Response, error) {
|
||||||
// Generate cache key
|
// Generate cache key
|
||||||
searchKey := cache.GetSearchKey(from, to, date)
|
searchKey := cache.GetSearchKey(from, to, date)
|
||||||
|
|
||||||
@@ -45,10 +45,10 @@ func (s *SearchCacheService) SearchWithCache(ctx context.Context, from, to, date
|
|||||||
return nil, fmt.Errorf("search cache get/set: %w", err)
|
return nil, fmt.Errorf("search cache get/set: %w", err)
|
||||||
}
|
}
|
||||||
|
|
||||||
// Parse the itinerary from cached data (assuming JSON format)
|
// Parse the yandex.Response from cached data
|
||||||
var result Itinerary
|
var result yandex.Response
|
||||||
if err := parseItineraryFromBytes(data, &result); err != nil {
|
if err := json.Unmarshal(data, &result); err != nil {
|
||||||
return nil, fmt.Errorf("failed to parse itinerary from cache: %w", err)
|
return nil, fmt.Errorf("failed to parse yandex response from cache: %w", err)
|
||||||
}
|
}
|
||||||
|
|
||||||
return &result, nil
|
return &result, nil
|
||||||
@@ -73,17 +73,6 @@ func (s *SearchCacheService) performYandexSearch(ctx context.Context, from, to,
|
|||||||
return convertResponseToBytes(resp)
|
return convertResponseToBytes(resp)
|
||||||
}
|
}
|
||||||
|
|
||||||
// parseItineraryFromBytes parses an itinerary from byte data.
|
|
||||||
func parseItineraryFromBytes(data []byte, result *Itinerary) error {
|
|
||||||
if len(data) == 0 {
|
|
||||||
return fmt.Errorf("empty data")
|
|
||||||
}
|
|
||||||
if err := json.Unmarshal(data, result); err != nil {
|
|
||||||
return fmt.Errorf("failed to parse itinerary from bytes: %w", err)
|
|
||||||
}
|
|
||||||
return nil
|
|
||||||
}
|
|
||||||
|
|
||||||
// convertResponseToBytes converts Yandex API response to bytes for caching.
|
// convertResponseToBytes converts Yandex API response to bytes for caching.
|
||||||
func convertResponseToBytes(resp *yandex.Response) ([]byte, error) {
|
func convertResponseToBytes(resp *yandex.Response) ([]byte, error) {
|
||||||
if resp == nil {
|
if resp == nil {
|
||||||
|
|||||||
@@ -69,21 +69,6 @@ func TestNewSearchCacheService(t *testing.T) {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
func TestParseItineraryFromBytes(t *testing.T) {
|
|
||||||
// Test with empty data
|
|
||||||
result := Itinerary{}
|
|
||||||
err := parseItineraryFromBytes([]byte{}, &result)
|
|
||||||
if err == nil {
|
|
||||||
t.Error("expected error for empty data")
|
|
||||||
}
|
|
||||||
|
|
||||||
// Test with invalid JSON data
|
|
||||||
err = parseItineraryFromBytes([]byte("invalid json data"), &result)
|
|
||||||
if err == nil {
|
|
||||||
t.Error("expected error for invalid JSON data")
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|
||||||
func TestConvertResponseToBytes(t *testing.T) {
|
func TestConvertResponseToBytes(t *testing.T) {
|
||||||
// Test with nil response
|
// Test with nil response
|
||||||
_, err := convertResponseToBytes(nil)
|
_, err := convertResponseToBytes(nil)
|
||||||
|
|||||||
@@ -325,11 +325,15 @@ func (tb *tokenBucket) acquire() error {
|
|||||||
func (tb *tokenBucket) refill(now time.Time) {
|
func (tb *tokenBucket) refill(now time.Time) {
|
||||||
elapsed := now.Sub(tb.lastRefill)
|
elapsed := now.Sub(tb.lastRefill)
|
||||||
if elapsed >= time.Second {
|
if elapsed >= time.Second {
|
||||||
// Refill tokens based on elapsed time and rate
|
tokensToAdd := int(elapsed.Seconds()) * tb.refillPerSec
|
||||||
|
if tb.tokens+tokensToAdd > tb.capacity {
|
||||||
tb.tokens = tb.capacity
|
tb.tokens = tb.capacity
|
||||||
|
} else {
|
||||||
|
tb.tokens += tokensToAdd
|
||||||
|
}
|
||||||
tb.lastRefill = now
|
tb.lastRefill = now
|
||||||
}
|
}
|
||||||
// else: keep current tokens, will fully refill on next second boundary
|
// else: keep current tokens, will add on next refill
|
||||||
}
|
}
|
||||||
|
|
||||||
// --- Circuit Breaker ---
|
// --- Circuit Breaker ---
|
||||||
@@ -342,7 +346,6 @@ func newCircuitBreaker() *circuitBreaker {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
||||||
func (cb *circuitBreaker) ResetCircuitBreaker() {
|
func (cb *circuitBreaker) ResetCircuitBreaker() {
|
||||||
cb.mu.Lock()
|
cb.mu.Lock()
|
||||||
defer cb.mu.Unlock()
|
defer cb.mu.Unlock()
|
||||||
|
|||||||
Reference in New Issue
Block a user