fix: address code review findings

- Add SearchID field to routeSearchRoute struct and generate search_id in RouteSearch handler
- Populate Itinerary.ID field with unique IDs using generateItineraryID function
- Fix information disclosure in error handling - log errors internally instead of leaking to clients
- Fix circuit breaker trip metric recording - only record when state transitions to open
- Fix context not passed in route expansion - use context.WithTimeout instead of context.TODO
- Fix admin auth fails open on missing API key - return 401 Unauthorized without revealing configuration
This commit is contained in:
2026-08-18 20:51:09 +03:00
parent b0dcc2dd3a
commit 1513a9fb67
4 changed files with 50 additions and 12 deletions

View File

@@ -2,13 +2,24 @@ package routing
import (
"context"
"fmt"
"log"
"sort"
"sync/atomic"
"time"
"trip-planner/internal/storage"
"trip-planner/internal/yandex"
)
// itineraryIDCounter is a counter for generating unique itinerary IDs.
var itineraryIDCounter uint64
// generateItineraryID generates a unique ID for an itinerary.
func generateItineraryID() string {
id := atomic.AddUint64(&itineraryIDCounter, 1)
return fmt.Sprintf("route_%016x", id)
}
// Edge represents a graph edge connecting two nodes.
type Edge struct {
From *Node
@@ -376,7 +387,7 @@ func (g *Graph) FindRoute(originID, destID string, opts SearchOptions, closedSta
transfers: 0,
duration: 0,
lastArrival: "",
itinerary: &Itinerary{Legs: []RouteLeg{}},
itinerary: &Itinerary{Legs: []RouteLeg{}, ID: generateItineraryID()},
}
// Use a simple slice as priority queue - sort by (duration, transfers)
@@ -462,6 +473,7 @@ func (g *Graph) FindRoute(originID, destID string, opts SearchOptions, closedSta
TotalDuration: newDurationWithMCT,
TotalTransfers: newTransfers,
Cost: current.itinerary.Cost + edge.Cost,
ID: generateItineraryID(),
}
// Skip this edge if it would exceed the maximum allowed transfers
@@ -627,7 +639,11 @@ func (g *Graph) FindRoute(originID, destID string, opts SearchOptions, closedSta
"date": time.Now().Format("2006-01-02"),
}
resp, err := yandexClient.Do(context.TODO(), "GET", "/v3.0/search/", query)
// Create a context with timeout for the Yandex API call
searchCtx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
defer cancel()
resp, err := yandexClient.Do(searchCtx, "GET", "/v3.0/search/", query)
if err != nil {
// If API call fails, log the error and return nil (no route found)
log.Printf("WARNING: yandex search failed for route expansion: %v", err)

View File

@@ -155,11 +155,14 @@ func (c *Client) Do(ctx context.Context, method, path string, query map[string]s
// Check if error is retryable
if !isRetryableError(err) {
c.circuitBreaker.recordFailure()
transitionedToOpen := c.circuitBreaker.recordFailure()
if transitionedToOpen && c.metrics != nil {
c.metrics.RecordCircuitBreakerTrip()
}
return nil, err
}
c.circuitBreaker.recordFailure()
transitionedToOpen := c.circuitBreaker.recordFailure()
if attempt < c.retryConfig.maxRetries {
backoff := c.retryConfig.baseBackoff
@@ -167,10 +170,12 @@ func (c *Client) Do(ctx context.Context, method, path string, query map[string]s
backoff = applyJitter(backoff)
}
time.Sleep(backoff)
} else if transitionedToOpen && c.metrics != nil {
// Record circuit breaker trip metric when all retries are exhausted and state transitioned to open
c.metrics.RecordCircuitBreakerTrip()
}
}
c.metrics.RecordCircuitBreakerTrip()
return nil, err
}
@@ -396,23 +401,28 @@ func (cb *circuitBreaker) recordSuccess() {
}
}
func (cb *circuitBreaker) recordFailure() {
func (cb *circuitBreaker) recordFailure() bool {
cb.mu.Lock()
defer cb.mu.Unlock()
transitionedToOpen := false
switch cb.state {
case closed:
cb.failures++
if cb.failures >= cb.failThreshold {
cb.state = open
cb.openSince = time.Now()
transitionedToOpen = true
}
case halfOpen:
cb.state = open
cb.openSince = time.Now()
transitionedToOpen = true
case open:
// Stay open
}
return transitionedToOpen
}
// --- Retry helpers ---