fix: address code review findings
This commit is contained in:
11
internal/cache/store.go
vendored
11
internal/cache/store.go
vendored
@@ -222,17 +222,16 @@ func (c *CacheAside) GetSearch(ctx context.Context, key *CacheKey, fetch func()
|
||||
|
||||
// Try cache first
|
||||
data, err := c.store.Get(ctx, key)
|
||||
if err == nil && data != nil {
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
if data != nil {
|
||||
c.metrics.RecordCacheHit("search") // cache hit
|
||||
return data, nil
|
||||
}
|
||||
|
||||
// Cache miss: fetch from backend
|
||||
if errors.Is(err, redis.Nil) {
|
||||
c.metrics.RecordCacheMiss("search") // record search cache miss
|
||||
} else if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
c.metrics.RecordCacheMiss("search") // record search cache miss
|
||||
|
||||
// Fetch from backend
|
||||
data, err = fetch()
|
||||
|
||||
@@ -418,7 +418,7 @@ func (g *Graph) FindRoute(originID, destID string, opts SearchOptions, closedSta
|
||||
newDurationWithMCT := newDuration + transferTime
|
||||
|
||||
// Check if we've visited this node with fewer transfers
|
||||
visKey := current.nodeID
|
||||
visKey := nextNode.ID
|
||||
if existingTransfers, ok := visited[visKey]; ok {
|
||||
if current.transfers+1 > existingTransfers {
|
||||
// Already visited this node with fewer transfers, skip
|
||||
@@ -880,7 +880,7 @@ type SearchResult struct {
|
||||
// route in all three metrics simultaneously. Routes are sorted according to the RankingMode
|
||||
// in SearchOptions: "fastest" (default, by duration), "fewest_transfers" (by transfers),
|
||||
// or "cheapest" (by cost).
|
||||
func (g *Graph) FindRoutesPareto(originID, destID string, opts SearchOptions) []*Itinerary {
|
||||
func (g *Graph) FindRoutesPareto(originID, destID string, opts SearchOptions, closedStations map[string]bool, neighbors map[string][]storage.StationNeighbor) []*Itinerary {
|
||||
// Run multiple searches with different strategies to find diverse routes
|
||||
var allItineraries []*Itinerary
|
||||
|
||||
@@ -889,7 +889,7 @@ func (g *Graph) FindRoutesPareto(originID, destID string, opts SearchOptions) []
|
||||
optsCopy := opts
|
||||
optsCopy.MaxTransfers = maxTransfers
|
||||
|
||||
result := g.FindRoute(originID, destID, optsCopy, nil, nil)
|
||||
result := g.FindRoute(originID, destID, optsCopy, closedStations, neighbors)
|
||||
if result != nil && result.TotalDuration > 0 {
|
||||
allItineraries = append(allItineraries, result)
|
||||
}
|
||||
|
||||
@@ -1,9 +1,11 @@
|
||||
package routing
|
||||
|
||||
import (
|
||||
"time"
|
||||
"fmt"
|
||||
"testing"
|
||||
"time"
|
||||
|
||||
"trip-planner/internal/storage"
|
||||
)
|
||||
|
||||
func TestFindRouteMaxTransfers(t *testing.T) {
|
||||
@@ -40,7 +42,9 @@ func TestFindRouteMaxTransfers(t *testing.T) {
|
||||
|
||||
// Test with MaxTransfers=0: should only find the direct route (0 transfers)
|
||||
opts0 := SearchOptions{MaxTransfers: 0}
|
||||
results0 := graph.FindRoutesPareto("s1", "s6", opts0)
|
||||
closedStations0 := make(map[string]bool)
|
||||
neighborsMap0 := make(map[string][]storage.StationNeighbor)
|
||||
results0 := graph.FindRoutesPareto("s1", "s6", opts0, closedStations0, neighborsMap0)
|
||||
t.Logf("MaxTransfers=0: found %d route(s)", len(results0))
|
||||
for _, r := range results0 {
|
||||
t.Logf(" Route: duration=%d, transfers=%d", r.TotalDuration, r.TotalTransfers)
|
||||
@@ -60,7 +64,9 @@ func TestFindRouteMaxTransfers(t *testing.T) {
|
||||
|
||||
// Test with MaxTransfers=1: should find direct route + 1-transfer route if any
|
||||
opts1 := SearchOptions{MaxTransfers: 1}
|
||||
results1 := graph.FindRoutesPareto("s1", "s6", opts1)
|
||||
closedStations1 := make(map[string]bool)
|
||||
neighborsMap1 := make(map[string][]storage.StationNeighbor)
|
||||
results1 := graph.FindRoutesPareto("s1", "s6", opts1, closedStations1, neighborsMap1)
|
||||
t.Logf("MaxTransfers=1: found %d route(s)", len(results1))
|
||||
for _, r := range results1 {
|
||||
t.Logf(" Route: duration=%d, transfers=%d", r.TotalDuration, r.TotalTransfers)
|
||||
@@ -74,7 +80,9 @@ func TestFindRouteMaxTransfers(t *testing.T) {
|
||||
|
||||
// Test with MaxTransfers=2: should find more routes
|
||||
opts2 := SearchOptions{MaxTransfers: 2}
|
||||
results2 := graph.FindRoutesPareto("s1", "s6", opts2)
|
||||
closedStations2 := make(map[string]bool)
|
||||
neighborsMap2 := make(map[string][]storage.StationNeighbor)
|
||||
results2 := graph.FindRoutesPareto("s1", "s6", opts2, closedStations2, neighborsMap2)
|
||||
t.Logf("MaxTransfers=2: found %d route(s)", len(results2))
|
||||
for _, r := range results2 {
|
||||
t.Logf(" Route: duration=%d, transfers=%d", r.TotalDuration, r.TotalTransfers)
|
||||
@@ -163,7 +171,9 @@ func TestParetoFrontGeneration(t *testing.T) {
|
||||
|
||||
t.Run("fastest mode (default) sorts by duration", func(t *testing.T) {
|
||||
opts := SearchOptions{MaxTransfers: 3}
|
||||
results := graph.FindRoutesPareto("s1", "s8", opts)
|
||||
closedStations := make(map[string]bool)
|
||||
neighborsMap := make(map[string][]storage.StationNeighbor)
|
||||
results := graph.FindRoutesPareto("s1", "s8", opts, closedStations, neighborsMap)
|
||||
|
||||
// Should find at least some Pareto-optimal routes
|
||||
if len(results) == 0 {
|
||||
@@ -199,7 +209,9 @@ func TestParetoFrontGeneration(t *testing.T) {
|
||||
|
||||
t.Run("fewest_transfers mode sorts by transfers first", func(t *testing.T) {
|
||||
opts := SearchOptions{MaxTransfers: 3, RankingMode: "fewest_transfers"}
|
||||
results := graph.FindRoutesPareto("s1", "s8", opts)
|
||||
closedStations := make(map[string]bool)
|
||||
neighborsMap := make(map[string][]storage.StationNeighbor)
|
||||
results := graph.FindRoutesPareto("s1", "s8", opts, closedStations, neighborsMap)
|
||||
|
||||
if len(results) == 0 {
|
||||
t.Fatal("expected at least one Pareto-optimal route with fewest_transfers mode")
|
||||
@@ -230,7 +242,9 @@ func TestParetoFrontGeneration(t *testing.T) {
|
||||
|
||||
t.Run("cheapest mode sorts by cost first", func(t *testing.T) {
|
||||
opts := SearchOptions{MaxTransfers: 3, RankingMode: "cheapest"}
|
||||
results := graph.FindRoutesPareto("s1", "s8", opts)
|
||||
closedStations := make(map[string]bool)
|
||||
neighborsMap := make(map[string][]storage.StationNeighbor)
|
||||
results := graph.FindRoutesPareto("s1", "s8", opts, closedStations, neighborsMap)
|
||||
|
||||
if len(results) == 0 {
|
||||
t.Fatal("expected at least one Pareto-optimal route with cheapest mode")
|
||||
|
||||
@@ -7,6 +7,7 @@ import (
|
||||
"math/rand"
|
||||
"net/http"
|
||||
"net/url"
|
||||
"strings"
|
||||
"sync"
|
||||
"time"
|
||||
|
||||
@@ -269,8 +270,17 @@ func isRetryableError(err error) bool {
|
||||
if err == nil {
|
||||
return false
|
||||
}
|
||||
// Network-level errors are retryable
|
||||
return true
|
||||
// Check for HTTP status codes that are retryable (5xx errors)
|
||||
apiErr, ok := err.(*APIError)
|
||||
if ok {
|
||||
return apiErr.Code >= 500 && apiErr.Code < 600
|
||||
}
|
||||
// Check for network errors
|
||||
errStr := err.Error()
|
||||
return strings.Contains(errStr, "timeout") ||
|
||||
strings.Contains(errStr, "connection refused") ||
|
||||
strings.Contains(errStr, "dial tcp") ||
|
||||
strings.Contains(errStr, "context deadline exceeded")
|
||||
}
|
||||
|
||||
// buildURL constructs a Yandex API URL with query parameters.
|
||||
|
||||
Reference in New Issue
Block a user