Fix/n plus one routes for stops#522
Merged
aaronbrethorst merged 2 commits intoOneBusAway:mainfrom Mar 1, 2026
Merged
Conversation
aaronbrethorst
approved these changes
Mar 1, 2026
Member
aaronbrethorst
left a comment
There was a problem hiding this comment.
Aditya, this is a really clean fix. You've not only eliminated the N+1 query problem in buildStopList, but you've also fixed a subtle pre-existing bug in the process — the old processRouteIds was storing combined IDs (e.g. "40_100479") in presentRoutes, but collectAgenciesAndRoutes passes those to GetRoutesByIDs, which queries WHERE routes.id IN (?) against raw IDs (e.g. "100479"). The combined IDs would never match, so route references were silently empty. Your use of utils.ExtractCodeID to store the raw ID fixes that. Nicely done.
Critical Issues (0 found)
None.
Important Issues (0 found)
None.
Strengths
- Eliminates up to 100 per-stop queries with a single batch
GetRouteIDsForStopscall - Fixes the pre-existing bug where
presentRoutesheld combined IDs that couldn't matchGetRoutesByIDs's raw ID lookup - The checked type assertion (
r.RouteID.(string)with, ok) is safer than the old unchecked panic-on-failure version - Good test that validates end-to-end data integrity — stops reference routes that actually appear in
references.routes - Clear comment at
trips_for_location_handler.go:522-525explaining the raw vs combined ID distinction
Checklist
- Tests pass
- Lint clean
- N+1 eliminated
- Route references bug fixed
- New test validates the fix
nice work, merging.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Every call to
trips-for-locationtriggered N+1 database queries insidebuildStopList— oneGetRouteIDsForStopquery per stop in the bounding box.With the stop limit capped at 100, this resulted in up to 101 queries just to build the stop list for a single API request.
Root Cause
buildStopListperformed a per-stop database query instead of batching route lookups.Fix
Replace the per-stop query loop with a single batch query:
Then:
stopID → []combinedRouteIDmap in memoryutils.ExtractCodeIDpresentRoutesThis eliminates N+1 queries and restores correct route references.
Changes
trips_for_location_handler.gobuildStopListwith batch implementation; removed deadprocessRouteIdstrips_for_location_handler_test.goTestTripsForLocationHandler_ReferencesContainStopsAndRoutesTesting
Ran all required checks:
make fmtmake lintmake testCloses
Fixes #304
Supersedes #305