Fix: Deterministic API response order in trips-for-route

[sreenivasivbieb]

Summary

This PR resolves #198 by improving the reliability and cleanliness of the tripsForRouteHandler.

• Sort block IDs before iteration to avoid non-deterministic map ordering in Go maps
• Remove unnecessary intermediate variables (nanosSinceMidnight, todayMidnight)
• Reuse serviceDayMidnight for service date calculation

Changes

• Added sort import
• Extract block IDs to slice, sort with sort.Strings(), then iterate deterministically
• Removed duplicate time calculations

Impact

• API responses are now deterministic (same order every time)
• Snapshot/diff-based tests will be reliable
• Cleaner code with fewer intermediate variables

Closes #198

[CLAassistant]

All committers have signed the CLA.

[Copilot]

Pull request overview

This PR fixes non-deterministic API response ordering in the trips-for-route endpoint by sorting block IDs before iteration. It also refactors time calculation code by removing duplicate intermediate variables.

Changes:

• Added sort package import for deterministic block ID ordering
• Implemented sorted iteration over block IDs using sort.Strings() to ensure consistent API response order
• Consolidated time calculation variables by removing nanosSinceMidnight and todayMidnight, reusing serviceDayMidnight instead

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[sreenivasivbieb]

@aaronbrethorst can you pls review my pr

[aaronbrethorst]

Hey there! Good catch on the non-deterministic map iteration issue - this is exactly the kind of subtle bug that causes flaky tests and confuses API consumers.

Overall Assessment

The core fix is correct, but the PR needs formatting fixes before it can be merged.

Critical Issues (1 found)

1. Formatting violations - must run go fmt

File: internal/restapi/trips_for_route_handler.go

The code has multiple formatting issues that go fmt will fix:

Import ordering (lines 3-14):

// Current - imports not properly grouped:
"net/http"
"time"
"sort"              // stdlib imports should be grouped together

// After go fmt:
"net/http"
"sort"
"time"

Missing spaces (lines 130-135):

// Current:
deterministicBlockIDs:=make([]string,0,len(allLinkedBlocks))
for blockID := range  allLinkedBlocks{
    deterministicBlockIDs=append( deterministicBlockIDs,blockID)
}
sort.Strings(deterministicBlockIDs)

// After go fmt:
deterministicBlockIDs := make([]string, 0, len(allLinkedBlocks))
for blockID := range allLinkedBlocks {
    deterministicBlockIDs = append(deterministicBlockIDs, blockID)
}
sort.Strings(deterministicBlockIDs)

Fix: Run go fmt ./...

Suggestions (Non-blocking)

1. Simplify comments

The new comments are a bit redundant:

// Extract block IDs from map and sort them to enable deterministic iteration.  // Good - keep this one
deterministicBlockIDs := make([]string, 0, len(allLinkedBlocks))
...
// sort blockIDs to have deterministic order     // Redundant - sort.Strings() is self-explanatory
sort.Strings(deterministicBlockIDs)
// iterating doesnt change the order of response now  // Redundant + typo ("doesnt")

Consider keeping just the first comment.

2. Remove refactoring history comment

Line 60: // Removed unnecessary intermediate variables (todayMidnight and nanosSinceMidnight).

Comments should describe what the code does, not what the PR changed. This will be confusing to future readers.

3. PR description accuracy

The description claims to "remove unnecessary intermediate variables," but the net change is +6 lines. The todayMidnight removal is a genuine simplification, but the PR primarily adds sorting logic rather than removing code.

What's Good

• Core fix is correct: Sorting block IDs before iteration ensures deterministic API responses
• Tests pass: make test succeeds
• Lint passes: make lint reports 0 issues (after formatting is fixed)
• Genuine duplicate removal: Reusing serviceDayMidnight instead of creating duplicate todayMidnight is a good simplification

Summary

Please run go fmt ./... and commit the formatting fixes. The rest are suggestions you can take or leave.

[sreenivasivbieb]

@aaronbrethorst can you recheck for the requested changes

[aaronbrethorst]

Hey Sreenivas, you've made solid progress on the deterministic ordering fix. The formatting issues from the first review are resolved, the redundant comments are cleaned up, and the core sorting logic is correct. Before we can merge this, I need you to make a couple more changes:

Important

1. Additional non-deterministic map iterations

The block ID sorting fixes the primary source of non-determinism (the order of activeTrips which determines the response list order), but there are other map iterations that produce non-deterministic orderings:

• Line 184: for id := range tripIDsSet - builds tripIDs slice in random order
• Line 204: for id := range routeIDsSet - builds routeIDs slice in random order

These feed into batch DB queries (GetTripsByIDs, GetRoutesByIDs) where input order may not matter, but the results could be returned in input-dependent order depending on the query. More importantly, these maps are used to build lookup tables, so the ordering impact is indirect.

However, the references section has more visible non-determinism:

• Line 347: for id := range presentTrips - builds tripsRefList in random order
• Line 375: for id := range presentRoutes - builds routes in random order

These directly affect the references section of the API response. If the goal of issue #198 is fully deterministic responses, these should be sorted too. If you'd prefer to scope this PR to just the primary result list, that's fine - but please add a comment noting these remaining sources of non-determinism, or open a follow-up issue.

2. Potential timezone difference in ServiceDate

The original code used two different midnight values:

// Line 60 (still present): uses currentTime.Location()
serviceDayMidnight := time.Date(..., currentTime.Location())

// Line 223 (removed): used currentLocation (agency timezone)
todayMidnight := time.Date(..., currentLocation)

When no time query parameter is provided (the common case), these are identical because ParseTimeParameter sets currentTime to time.Now().In(currentLocation). But when a YYYY-MM-DD date string is provided, time.Parse returns a UTC time, making serviceDayMidnight midnight UTC while the old todayMidnight was midnight in the agency's timezone.

For ServiceDate, the agency timezone midnight is semantically more correct. Two options:

Option A: Keep using serviceDayMidnight but fix its timezone:

serviceDayMidnight := time.Date(currentTime.Year(), currentTime.Month(), currentTime.Day(), 0, 0, 0, 0, currentLocation)

Option B: Note this is a pre-existing bug (the nanosSinceMidnight calculation on main already uses currentTime.Location()) and file a separate issue to fix the timezone handling holistically.

Either approach is fine - just want to make sure this is a conscious decision.

Fit and Finish

1. Commit hygiene

The branch has 6 commits including "requested_changes are done can you merge this pr" and multiple "Update file" commits. Please squash these into a single clean commit before merge. Something like:

fix: ensure deterministic API response order in trips-for-route

Sort block IDs before iteration to avoid non-deterministic map ordering.
Remove duplicate todayMidnight variable, reuse serviceDayMidnight.

Fixes #198

If you're not familiar with interactive rebasing to squash commits, here's a guide that may help: https://www.brethorsting.com/blog/2026/01/git-rebase-for-the-terrified/

Thanks again, and I look forward to merging this change.

[sreenivasivbieb]

@aaronbrethorst as you requested i made changes accordingly can you review and merge the pr to main

[aaronbrethorst]

please make sure the linter and tests all pass.