Search route short and long names in routes-for-location query - #1289
Search route short and long names in routes-for-location query#1289ARCoder181105 wants to merge 6 commits into
Conversation
FTS5 tokenizes a term like "%" to nothing, which raises a syntax error at MATCH time instead of just matching zero rows. Filter such terms out before building the query so they collapse to a no-op search, matching how extractFTS5Terms already handles this in the stop search handler. Extracted the shared predicate into utils.ContainsLetterOrDigit so both call sites use one definition.
routes-for-location's query param only matched routes.short_name via exact equality, so a query like "Shasta" never matched a route whose name only appears in long_name (e.g. "Shasta View/Shasta College"). The spec requires searching a text index of both short and long names, considering at most maxCount+1 candidates by relevance before filtering to those with a stop in bounds. Reuse the existing routes_fts index (Manager.SearchRoutes) for the ranked candidate lookup, then filter GetActiveRoutesWithinBounds to just those candidate route IDs instead of an exact short_name match. An empty candidate set must short-circuit before reaching the bounds query, since an empty RouteIDs filter there means "no filter" and would otherwise return every in-bounds route regardless of query. Also drop GetRoutesForLocation's queryTime parameter, which the function never read; it's the same call site being touched by the signature change above.
Add handler tests for the new text-search path: matching a route by long name only, a prefix token, ANDed multi-word queries, a query with no matching route text, candidates truncated at maxCount+1, and a text match whose stops fall outside the search bounds (still excluded). Existing exact short-name, case-insensitive, and literal wildcard tests already covered the prior behaviour and needed no changes.
|
Warning Review limit reached
Next review available in: 38 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe location route search now uses text-indexed route candidates, filters candidates by geographic bounds, and supports dynamic route-ID SQL filtering. Shared term validation prevents punctuation-only FTS5 queries from causing errors. ChangesRoute location search
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@gtfsdb/stops_rtree.go`:
- Around line 168-179: Add Go doc comments in gtfsdb/stops_rtree.go for exported
GetActiveRoutesWithinBoundsParams and GetActiveRoutesWithinBounds, with each
comment beginning with its identifier. In internal/gtfs/gtfs_manager.go, update
the GetRoutesForLocation doc comment so it begins with GetRoutesForLocation.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c4bb3756-a436-4d17-9342-55887def5990
📒 Files selected for processing (8)
gtfsdb/stops_rtree.gointernal/gtfs/gtfs_manager.gointernal/gtfs/route_search.gointernal/gtfs/route_search_test.gointernal/restapi/routes_for_location_handler.gointernal/restapi/routes_for_location_handler_test.gointernal/restapi/search_stops_handler.gointernal/utils/string_utils.go
golint requires exported identifiers' doc comments to start with their own name; GetActiveRoutesWithinBoundsParams and GetActiveRoutesWithinBounds had none.
Ahmedhossamdev
left a comment
Ahmedhossamdev
left a comment
There was a problem hiding this comment.
Hey @ARCoder181105, Great work on this.
I verified the long-name search against the live Unitrans Java server and it checks out. The punctuation-only FTS5 fix and the ContainsLetterOrDigit extraction are clean. LGTM, one optional note on the dynamic SQL inline. Lint and all tests pass.
The IN clause is assembled with fmt.Sprintf, which reads as concatenated SQL. Record that only generated "?N" placeholders are formatted into the query text and route IDs travel as bind values.
|
2 participants
Summary
queryonly matchedroutes.short_nameexactly, so long-name-onlymatches (e.g. "Shasta View/Shasta College") never worked. Now searches
both short and long names via the existing
routes_ftsindex, rankedby relevance, then filtered to routes with a stop in bounds.
routes_fts/Manager.SearchRoutesfor the candidate lookup%) crashing FTS5 instead of matching nothingqueryTimeparam fromGetRoutesForLocationFixes #1228
Note for reviewer
Verified against
api.pugetsound.onebusaway.org: the real server doesexact-token matching (
query=Rap→ 0 results, needsRap*). This PRalways does prefix matching (
query=Romatches "Roosevelt" routes, no*needed) — kept intentionally, better for live search. Flagging, notfiling as a defect.
Test plan
go vet(both build tags),make test,go fmtcandidate cap, out-of-bounds text match
Summary by CodeRabbit
New Features
Bug Fixes
Tests