fix: address code review findings
This commit is contained in:
@@ -4,6 +4,7 @@ import (
|
|||||||
"encoding/json"
|
"encoding/json"
|
||||||
"fmt"
|
"fmt"
|
||||||
"net/http"
|
"net/http"
|
||||||
|
"os"
|
||||||
"strings"
|
"strings"
|
||||||
"time"
|
"time"
|
||||||
|
|
||||||
@@ -109,11 +110,11 @@ func CityStations(hc *HandlerContext, w http.ResponseWriter, r *http.Request) {
|
|||||||
// Check if any main station is closed by looking for stations without real edges.
|
// Check if any main station is closed by looking for stations without real edges.
|
||||||
// If a station is closed, include neighboring stations as fallback options.
|
// If a station is closed, include neighboring stations as fallback options.
|
||||||
var closedStationIndices []int
|
var closedStationIndices []int
|
||||||
for i := range stations {
|
for i, station := range stations {
|
||||||
// For demo: if the graph has real edges, station is not closed
|
// Check if this specific station has real edges
|
||||||
hasRealEdges := false
|
hasRealEdges := false
|
||||||
if len(hc.Router.Edges()) > 0 {
|
for _, edge := range hc.Router.Edges() {
|
||||||
for _, edge := range hc.Router.Edges() {
|
if edge.From.ID == station[0] || edge.To.ID == station[0] {
|
||||||
if edge.Kind == routing.EdgeKindReal {
|
if edge.Kind == routing.EdgeKindReal {
|
||||||
hasRealEdges = true
|
hasRealEdges = true
|
||||||
break
|
break
|
||||||
@@ -139,18 +140,16 @@ func CityStations(hc *HandlerContext, w http.ResponseWriter, r *http.Request) {
|
|||||||
neighborTable.Add("2", "s8700100", "Leningradsky Alternative", "manual")
|
neighborTable.Add("2", "s8700100", "Leningradsky Alternative", "manual")
|
||||||
}
|
}
|
||||||
|
|
||||||
// Get non-excluded neighbors for closed stations
|
// Get non-excluded neighbors for the city
|
||||||
for range closedStationIndices {
|
cityNeighbors := neighborTable.GetNonExcluded(cityID)
|
||||||
cityNeighbors := neighborTable.GetNonExcluded(cityID)
|
for _, n := range cityNeighbors {
|
||||||
for _, n := range cityNeighbors {
|
neighbors = append(neighbors, CityNeighborResponse{
|
||||||
neighbors = append(neighbors, CityNeighborResponse{
|
StationID: n.StationID,
|
||||||
StationID: n.StationID,
|
Name: n.Name,
|
||||||
Name: n.Name,
|
CityCode: n.CityCode,
|
||||||
CityCode: n.CityCode,
|
Source: n.Source,
|
||||||
Source: n.Source,
|
IsExcluded: n.IsExcluded,
|
||||||
IsExcluded: n.IsExcluded,
|
})
|
||||||
})
|
|
||||||
}
|
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -404,14 +403,17 @@ func StationStatus(hc *HandlerContext, w http.ResponseWriter, r *http.Request) {
|
|||||||
// adminAuth checks authentication for admin endpoints.
|
// adminAuth checks authentication for admin endpoints.
|
||||||
// Returns true if the request is authenticated, false otherwise.
|
// Returns true if the request is authenticated, false otherwise.
|
||||||
func adminAuth(hc *HandlerContext, w http.ResponseWriter, r *http.Request) bool {
|
func adminAuth(hc *HandlerContext, w http.ResponseWriter, r *http.Request) bool {
|
||||||
// Check for admin API key in header
|
// Check for admin API key in header, fallback to default if not set
|
||||||
expectedAPIKey := "trip-planner-admin-key"
|
expectedAPIKey := os.Getenv("TRIP_PLANNER_ADMIN_API_KEY")
|
||||||
providedAPIKey := r.Header.Get("X-Admin-Api-Key")
|
if expectedAPIKey == "" {
|
||||||
if providedAPIKey != expectedAPIKey {
|
expectedAPIKey = "trip-planner-admin-key"
|
||||||
http.Error(w, "unauthorized: admin API key required", http.StatusUnauthorized)
|
}
|
||||||
return false
|
providedAPIKey := r.Header.Get("X-Admin-Api-Key")
|
||||||
}
|
if providedAPIKey != expectedAPIKey {
|
||||||
return true
|
http.Error(w, "unauthorized: admin API key required", http.StatusUnauthorized)
|
||||||
|
return false
|
||||||
|
}
|
||||||
|
return true
|
||||||
}
|
}
|
||||||
|
|
||||||
// AdminStationStatus handles POST /internal/admin/stations/{id}/status.
|
// AdminStationStatus handles POST /internal/admin/stations/{id}/status.
|
||||||
|
|||||||
@@ -19,9 +19,8 @@ import (
|
|||||||
|
|
||||||
func flushRedisForTest(t *testing.T, client *redis.Client) {
|
func flushRedisForTest(t *testing.T, client *redis.Client) {
|
||||||
// Clear preference-related keys from Redis to ensure test isolation
|
// Clear preference-related keys from Redis to ensure test isolation
|
||||||
// The keyString sanitization prepends "unknown:" and replaces ":" with "_colon_"
|
// Actual keys look like: "prefs:saved_city:testuser1" and "prefs:search_history:testuser1"
|
||||||
// Actual keys look like: "unknown:prefs_colon_saved_city_colon_testuser1"
|
keys, err := client.Keys(context.Background(), "prefs:saved_city:*").Result()
|
||||||
keys, err := client.Keys(context.Background(), "unknown:prefs*").Result()
|
|
||||||
if err != nil {
|
if err != nil {
|
||||||
t.Logf("warning: could not flush preference keys: %v", err)
|
t.Logf("warning: could not flush preference keys: %v", err)
|
||||||
return
|
return
|
||||||
@@ -32,7 +31,7 @@ func flushRedisForTest(t *testing.T, client *redis.Client) {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
// Also delete search_history keys
|
// Also delete search_history keys
|
||||||
keys2, err := client.Keys(context.Background(), "unknown:search_history*").Result()
|
keys2, err := client.Keys(context.Background(), "prefs:search_history:*").Result()
|
||||||
if err != nil {
|
if err != nil {
|
||||||
t.Logf("warning: could not flush search history keys: %v", err)
|
t.Logf("warning: could not flush search history keys: %v", err)
|
||||||
return
|
return
|
||||||
@@ -765,7 +764,7 @@ func TestUserPreferences(t *testing.T) {
|
|||||||
flushRedisForTest(t, h.Redis)
|
flushRedisForTest(t, h.Redis)
|
||||||
|
|
||||||
// Add a saved city
|
// Add a saved city
|
||||||
addReq := httptest.NewRequest("POST", "/v1/preferences/saved-cities?user_id=testuser2", strings.NewReader(`{"city_code":"c1","name":"Moscow"}`,))
|
addReq := httptest.NewRequest("POST", "/v1/preferences/saved-cities?user_id=testuser2", strings.NewReader(`{"city_code":"c1","name":"Moscow"}`))
|
||||||
addReq.Header.Set("Content-Type", "application/json")
|
addReq.Header.Set("Content-Type", "application/json")
|
||||||
addRR := httptest.NewRecorder()
|
addRR := httptest.NewRecorder()
|
||||||
AddSavedCity(h, addRR, addReq)
|
AddSavedCity(h, addRR, addReq)
|
||||||
|
|||||||
@@ -76,7 +76,7 @@ func checkStationSchedule(ctx context.Context, yc *yandex.Client, stationID stri
|
|||||||
// The Yandex Do method handles the API request with rate limiting,
|
// The Yandex Do method handles the API request with rate limiting,
|
||||||
// circuit breaking, and retry. It returns a Response with the
|
// circuit breaking, and retry. It returns a Response with the
|
||||||
// schedule data including interval segments.
|
// schedule data including interval segments.
|
||||||
resp, err := yc.Do(ctx, "schedule", "/station/"+stationID, map[string]string{
|
resp, err := yc.Do(ctx, "GET", "/station/"+stationID, map[string]string{
|
||||||
"date": time.Now().Format("2006-01-02"),
|
"date": time.Now().Format("2006-01-02"),
|
||||||
})
|
})
|
||||||
if err != nil {
|
if err != nil {
|
||||||
@@ -131,8 +131,11 @@ func (sm *StationMonitor) updateStationStatus(ctx context.Context, tripCount int
|
|||||||
zeroSinceData, err := sm.Cache.Get(ctx, zeroSinceKey)
|
zeroSinceData, err := sm.Cache.Get(ctx, zeroSinceKey)
|
||||||
var zeroSince time.Time
|
var zeroSince time.Time
|
||||||
if err == nil && zeroSinceData != nil {
|
if err == nil && zeroSinceData != nil {
|
||||||
_, err := fmt.Sscanf(string(zeroSinceData), "%d", (&zeroSince).Unix())
|
var zeroSinceUnix int64
|
||||||
if err != nil {
|
_, err := fmt.Sscanf(string(zeroSinceData), "%d", &zeroSinceUnix)
|
||||||
|
if err == nil {
|
||||||
|
zeroSince = time.Unix(zeroSinceUnix, 0)
|
||||||
|
} else {
|
||||||
zeroSince = time.Time{}
|
zeroSince = time.Time{}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@@ -141,8 +144,11 @@ func (sm *StationMonitor) updateStationStatus(ctx context.Context, tripCount int
|
|||||||
lastSeenFlightData, err := sm.Cache.Get(ctx, lastSeenFlightKey)
|
lastSeenFlightData, err := sm.Cache.Get(ctx, lastSeenFlightKey)
|
||||||
var lastSeenFlight time.Time
|
var lastSeenFlight time.Time
|
||||||
if err == nil && lastSeenFlightData != nil {
|
if err == nil && lastSeenFlightData != nil {
|
||||||
_, err := fmt.Sscanf(string(lastSeenFlightData), "%d", (&lastSeenFlight).Unix())
|
var lastSeenFlightUnix int64
|
||||||
if err != nil {
|
_, err := fmt.Sscanf(string(lastSeenFlightData), "%d", &lastSeenFlightUnix)
|
||||||
|
if err == nil {
|
||||||
|
lastSeenFlight = time.Unix(lastSeenFlightUnix, 0)
|
||||||
|
} else {
|
||||||
lastSeenFlight = time.Time{}
|
lastSeenFlight = time.Time{}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
12
internal/cache/store.go
vendored
12
internal/cache/store.go
vendored
@@ -106,12 +106,16 @@ func sanitizeKeyComponent(s string) string {
|
|||||||
}
|
}
|
||||||
|
|
||||||
func keyString(k *CacheKey) string {
|
func keyString(k *CacheKey) string {
|
||||||
switch k.Kind {
|
switch {
|
||||||
case "city":
|
case strings.HasPrefix(k.Kind, "prefs:saved_city:"):
|
||||||
|
return fmt.Sprintf("prefs:saved_city:%s", sanitizeKeyComponent(k.Code))
|
||||||
|
case strings.HasPrefix(k.Kind, "prefs:search_history:"):
|
||||||
|
return fmt.Sprintf("prefs:search_history:%s", sanitizeKeyComponent(k.Code))
|
||||||
|
case k.Kind == "city":
|
||||||
return fmt.Sprintf("cities:%s", sanitizeKeyComponent(k.Code))
|
return fmt.Sprintf("cities:%s", sanitizeKeyComponent(k.Code))
|
||||||
case "station":
|
case k.Kind == "station":
|
||||||
return fmt.Sprintf("stations:%s", sanitizeKeyComponent(k.Code))
|
return fmt.Sprintf("stations:%s", sanitizeKeyComponent(k.Code))
|
||||||
case "search":
|
case k.Kind == "search":
|
||||||
return fmt.Sprintf("search:%s:%s:%s",
|
return fmt.Sprintf("search:%s:%s:%s",
|
||||||
sanitizeKeyComponent(k.From),
|
sanitizeKeyComponent(k.From),
|
||||||
sanitizeKeyComponent(k.To),
|
sanitizeKeyComponent(k.To),
|
||||||
|
|||||||
Reference in New Issue
Block a user