Added support for new Route Search API#149
Conversation
aaronbrethorst
left a comment
There was a problem hiding this comment.
I appreciate your help, but any solution that reverts the very intentional DB driver change I made two months ago isn't going to work, unfortunately. Bhup-GitHUB@fae9999
|
@Bhup-GitHUB The README on https://github.com/mattn/go-sqlite3 explains how to enable fts5. You just need to read it and apply it. |
Thanks for the guidance. I’ll keep the github.com/mattn/go-sqlite3 driver and enable FTS5 per the README by building/testing with -tags "sqlite_fts5" (with CGO enabled). I’ll also document the build flag so others can run the suite successfully. Does this approach align with what you want? |
|
Yes, that sounds greatSent from my iPhoneOn Dec 9, 2025, at 12:06 AM, Bhupesh Kumar ***@***.***> wrote:Bhup-GitHUB left a comment (OneBusAway/maglev#149)
@Bhup-GitHUB The README on https://github.com/mattn/go-sqlite3 explains how to enable fts5. You just need to read it and apply it.
Thanks for the guidance. I’ll keep the github.com/mattn/go-sqlite3 driver and enable FTS5 per the README by building/testing with -tags "sqlite_fts5" (with CGO enabled). I’ll also document the build flag so others can run the suite successfully. Does this approach align with what you want?
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were assigned.Message ID: ***@***.***>
|
This reverts commit 8110f8c.
|
hi @aaronbrethorst , I’ve made a few changes . Could you please take a look when you get a chance? If you notice any issues, let me know. thanks |
aaronbrethorst
left a comment
There was a problem hiding this comment.
please make sure the tests pass!
|
@aaronbrethorst done ! |
aaronbrethorst
left a comment
There was a problem hiding this comment.
Thanks for implementing the route search feature! The FTS5 approach is solid, and the overall structure follows the project patterns well. I have some feedback that needs to be addressed before we merge.
Critical
1. Empty query string will crash the database
When buildRouteSearchQuery returns an empty string (e.g., if input is only whitespace or special characters that get filtered out), it gets passed directly to SQLite FTS5. An empty MATCH "" clause causes a syntax error.
File: internal/gtfs/route_search.go:37-45
// Current code
query := buildRouteSearchQuery(input)
return manager.GtfsDB.Queries.SearchRoutesByFullText(ctx, gtfsdb.SearchRoutesByFullTextParams{
Query: query,
Limit: int64(limit),
})
// Fix: return early if query is empty
query := buildRouteSearchQuery(input)
if query == "" {
return []gtfsdb.Route{}, nil
}
return manager.GtfsDB.Queries.SearchRoutesByFullText(ctx, gtfsdb.SearchRoutesByFullTextParams{
Query: query,
Limit: int64(limit),
})2. Missing .Valid checks on nullable fields
The project guidelines in CLAUDE.md specify that nullable SQL fields should be checked before accessing. This is a pattern we follow throughout the codebase.
File: internal/restapi/route_search_handler.go:63-76
// Current code accesses .String directly
routeRow.ShortName.String,
routeRow.LongName.String,
routeRow.Desc.String,
// Preferred pattern per CLAUDE.md
shortName := ""
if routeRow.ShortName.Valid {
shortName = routeRow.ShortName.String
}You can use a helper or inline checks. The key thing is being explicit about nullable handling.
3. README says wrong SQLite driver
The README addition mentions github.com/mattn/go-sqlite3, but your PR description says you switched to modernc.org/sqlite. One of these is wrong — please verify which driver is actually being used and update the docs to match.
File: README.markdown:158
High Priority
4. Add error context for debugging
When database errors occur, it helps to know what query caused the failure. Wrapping the error with context makes production debugging much easier.
File: internal/gtfs/route_search.go:37-45
routes, err := manager.GtfsDB.Queries.SearchRoutesByFullText(ctx, gtfsdb.SearchRoutesByFullTextParams{
Query: query,
Limit: int64(limit),
})
if err != nil {
return nil, fmt.Errorf("route search failed for query %q: %w", query, err)
}
return routes, nil5. Redundant maxCount validation
The handler rejects maxCount > 100 with an error, but the manager also caps it at 100. Since the handler already validates, the manager's cap is unreachable code.
Pick one approach:
- Option A: Keep handler validation, remove the cap in the manager
- Option B: Remove handler validation, let manager silently cap (matches
stops-for-locationpattern)
Either is fine, but having both creates confusion about the intended behavior.
5a. Test coverage gaps
The existing tests cover the happy path well. These additional cases need coverage:
Empty results
func TestRouteSearchHandlerNoResults(t *testing.T) {
_, resp, model := serveAndRetrieveEndpoint(t,
"/api/where/search/route.json?key=TEST&input=zzzznonexistent99999")
assert.Equal(t, http.StatusOK, resp.StatusCode)
data := model.Data.(map[string]interface{})
list := data["list"].([]interface{})
assert.Empty(t, list)
}Whitespace-only input
func TestRouteSearchHandlerWhitespaceInput(t *testing.T) {
_, resp, _ := serveAndRetrieveEndpoint(t,
"/api/where/search/route.json?key=TEST&input=%20%20%20")
assert.Equal(t, http.StatusBadRequest, resp.StatusCode)
}maxCount boundaries
func TestRouteSearchHandlerMaxCountBoundaries(t *testing.T) {
// Exactly 100 should work
_, resp, _ := serveAndRetrieveEndpoint(t,
"/api/where/search/route.json?key=TEST&input=shasta&maxCount=100")
assert.Equal(t, http.StatusOK, resp.StatusCode)
// 101 should fail
_, resp, _ = serveAndRetrieveEndpoint(t,
"/api/where/search/route.json?key=TEST&input=shasta&maxCount=101")
assert.Equal(t, http.StatusBadRequest, resp.StatusCode)
}Lower Priority
6. Add comments explaining FTS5 setup
The FTS5 external content table pattern isn't obvious to developers unfamiliar with it. A few comments in the schema would help future maintainers:
File: gtfsdb/schema.sql
-- FTS5 external content table for full-text route search.
-- Data lives in 'routes' table; only the search index is stored here.
-- The triggers below keep the index synchronized with the content table.
CREATE VIRTUAL TABLE IF NOT EXISTS routes_fts USING fts5 (
...
);
-- Trigger naming: ai=After Insert, ad=After Delete, au=After Update
CREATE TRIGGER IF NOT EXISTS routes_fts_ai AFTER INSERT ON routes BEGIN
...
END;7. Add logging for search queries
Adding debug-level logging for search operations helps diagnose issues in production:
// In SearchRoutes, after building the query
manager.Logger.Debug("route search", "input", input, "query", query, "limit", limit)What's Good
- Clean separation between query building (
buildRouteSearchQuery) and execution (SearchRoutes) - Proper use of FTS5 external content tables with triggers — this is the right approach
- BM25 ranking gives users relevant results first
- Tests follow existing project patterns
- Handler validation catches common input errors
- Input sanitization via
ValidateAndSanitizeQueryhandles XSS concerns
Summary
| Priority | Issue | Location |
|---|---|---|
| Critical | Empty query crashes database | route_search.go:37-45 |
| Critical | Missing .Valid checks |
route_search_handler.go:63-76 |
| Critical | README has wrong driver name | README.markdown:158 |
| High | Add error context | route_search.go:37-45 |
| High | Redundant maxCount validation | Handler + Manager |
| High | Add test coverage | route_search_handler_test.go |
| Lower | Add FTS5 schema comments | schema.sql |
| Lower | Add search query logging | route_search.go |
Let me know if you have questions about any of this feedback.
|
Thanks for the detailed feedback really helpful @aaronbrethorst . I’m working through the issues now. Quick clarification on issue #5 (the redundant maxCount validation): I’m leaning toward A since it’s more explicit, but wanted to confirm that this aligns with how you envision the API behaving. Everything else is clear |
|
All feedback addressed. For issue #5, I kept handler validation and removed manager cap . Question : Should I update Makefile's |
aaronbrethorst
left a comment
There was a problem hiding this comment.
great work, Bhupesh. Thanks for seeing this through. I'll take care of the build flags you mentioned in a followup PR.
Summary #146
/api/where/search/route.jsonusing SQLite FTS5 for full-text route search, matching OneBusAway’s route search spec.routes_ftsvirtual table with triggers to keep FTS data in sync and a rebuild to cover existing data.SearchRoutesByFullTextquery and GTFS manager helper that builds safe prefix queries and caps result size.input, checksmaxCount, and returns list + references consistent with existing/whereendpoints.modernc.org/sqliteto ensure FTS5 is available in tests/builds without native SQLite deps.Implementation Notes
routes_ftswith insert/update/delete triggers; rebuild inserted to backfill existing routes.SearchRoutesByFullText; manager helper constructs FTS-safe"term"*AND-joined queries with limits.internal/restapi/routes.go; uses existing validation/response helpers to match schema of other list endpoints.github.com/mattn/go-sqlite3withmodernc.org/sqlitefor FTS5 availability across environments.Testing
go test ./...