From 82757665e94d542c4cb3095f22258631073d2489 Mon Sep 17 00:00:00 2001 From: Vladimir Zagainov Date: Mon, 17 Aug 2026 20:34:17 +0300 Subject: [PATCH] fix: address code review findings --- internal/cache/store.go | 7 ++----- internal/routing/graph.go | 2 +- internal/routing/graph_test.go | 3 --- internal/routing/search_cache.go | 13 +++++++++---- internal/routing/search_cache_test.go | 8 ++++---- internal/yandex/client_test.go | 28 ++++++++++++--------------- 6 files changed, 28 insertions(+), 33 deletions(-) diff --git a/internal/cache/store.go b/internal/cache/store.go index 1b5eda7..b7c2dad 100644 --- a/internal/cache/store.go +++ b/internal/cache/store.go @@ -70,14 +70,11 @@ func (r *redisClient) Set(ctx context.Context, key *CacheKey, value []byte, ttl // Exists checks if a key exists in cache. func (r *redisClient) Exists(ctx context.Context, key *CacheKey) (bool, error) { - _, err := r.client.Exists(ctx, keyString(key)).Result() - if errors.Is(err, redis.Nil) { - return false, nil - } + count, err := r.client.Exists(ctx, keyString(key)).Result() if err != nil { return false, fmt.Errorf("cache exists: %w", err) } - return true, nil + return count > 0, nil } // Delete removes a key from cache. diff --git a/internal/routing/graph.go b/internal/routing/graph.go index 730929b..5a8eb2f 100644 --- a/internal/routing/graph.go +++ b/internal/routing/graph.go @@ -1068,7 +1068,7 @@ func (g *Graph) checkRouteForChanges(itinerary *Itinerary) bool { break } // Check for significant delay (more than 2x normal duration) - if edge.Duration > leg.Cost*2 && leg.Cost > 0 { // simplified delay check + if edge.Duration > leg.Duration*2 && leg.Duration > 0 { if !needsReSearch || itinerary.ReSearchReason == string(reasonNone) { needsReSearch = true itinerary.NeedsReSearch = true diff --git a/internal/routing/graph_test.go b/internal/routing/graph_test.go index c557a24..bd778e6 100644 --- a/internal/routing/graph_test.go +++ b/internal/routing/graph_test.go @@ -325,9 +325,6 @@ func TestLazyExpansionDepthLimit(t *testing.T) { // TestRouteReSearchOnChange tests that the route change detection logic correctly // identifies when a route leg has undergone significant changes (cancellation or major delay) // and triggers a re-search to find an updated route. -// TestRouteReSearchOnChange tests that the route change detection logic correctly -// identifies when a route leg has undergone significant changes (cancellation or major delay) -// and triggers a re-search to find an updated route. func TestRouteReSearchOnChange(t *testing.T) { graph := NewGraph() diff --git a/internal/routing/search_cache.go b/internal/routing/search_cache.go index f645051..72da1ad 100644 --- a/internal/routing/search_cache.go +++ b/internal/routing/search_cache.go @@ -2,6 +2,7 @@ package routing import ( "context" + "encoding/json" "fmt" "trip-planner/internal/cache" @@ -74,19 +75,23 @@ func (s *SearchCacheService) performYandexSearch(ctx context.Context, from, to, // parseItineraryFromBytes parses an itinerary from byte data. func parseItineraryFromBytes(data []byte, result *Itinerary) error { - // This is a placeholder - in real implementation, this would parse JSON - // into the Itinerary struct 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. func convertResponseToBytes(resp *yandex.Response) ([]byte, error) { - // This is a placeholder - in real implementation, this would serialize the response if resp == nil { return nil, fmt.Errorf("nil response") } - return []byte(`{"search":{"from":"%s","to":"%s"},"segments":[]}`), nil + data, err := json.Marshal(resp) + if err != nil { + return nil, fmt.Errorf("failed to marshal response: %w", err) + } + return data, nil } diff --git a/internal/routing/search_cache_test.go b/internal/routing/search_cache_test.go index ebf4ec5..879c53e 100644 --- a/internal/routing/search_cache_test.go +++ b/internal/routing/search_cache_test.go @@ -77,10 +77,10 @@ func TestParseItineraryFromBytes(t *testing.T) { t.Error("expected error for empty data") } - // Test with non-empty data (placeholder implementation returns nil error) - err = parseItineraryFromBytes([]byte("test"), &result) - if err != nil { - t.Errorf("expected no error for non-empty data, got %v", err) + // Test with invalid JSON data + err = parseItineraryFromBytes([]byte("invalid json data"), &result) + if err == nil { + t.Error("expected error for invalid JSON data") } } diff --git a/internal/yandex/client_test.go b/internal/yandex/client_test.go index ec92bf9..b07cce7 100644 --- a/internal/yandex/client_test.go +++ b/internal/yandex/client_test.go @@ -85,13 +85,18 @@ func TestCircuitBreakerOpenAfterFailures(t *testing.T) { t.Errorf("expected state open, got %v", cb.state) } - // Wait for timeout - time.Sleep(31 * time.Second) + // Test state transition to half-open by manually setting state and time + cb.state = open + cb.openSince = time.Now().Add(-31 * time.Second) - // Should transition to half-open/open after timeout - allow() should return true + // Should transition to half-open after timeout - allow() should return true if !cb.allow() { t.Error("expected allow() to return true after timeout") } + + if cb.state != halfOpen { + t.Errorf("expected state halfOpen after timeout, got %v", cb.state) + } } func TestCircuitBreakerRecordSuccess(t *testing.T) { @@ -199,19 +204,10 @@ func TestRetryExhaustion(t *testing.T) { jitter: false, } - // Simulate consecutive failures - var lastErr error - for attempt := 0; attempt <= cfg.maxRetries; attempt++ { - // Simulate a non-retryable error that gets recorded as failure - // In real code, isRetryableError would return false - lastErr = fmt.Errorf("attempt %d failed", attempt) - _ = lastErr // track last error - } - - // After maxRetries+1 attempts (0-indexed: 0 to maxRetries), we've done 3 attempts - // with 2 retries (attempts 0->1, 1->2), the 3rd attempt (index 2) is the last - if cfg.maxRetries+1 < 3 { - t.Error("expected at least 3 attempts with maxRetries=2") + // Verify maxRetries=2 means 3 total attempts (0, 1, 2) + totalAttempts := cfg.maxRetries + 1 + if totalAttempts != 3 { + t.Errorf("expected 3 total attempts with maxRetries=2, got %d", totalAttempts) } }