diff --git a/gtfsdb/client.go b/gtfsdb/client.go index ff76fd41..70b8c054 100644 --- a/gtfsdb/client.go +++ b/gtfsdb/client.go @@ -43,6 +43,7 @@ func NewClient(config Config) (*Client, error) { // fallback, so a missing or stale index yields an empty combined ID rather than a // merely degraded one - this must succeed before the client is usable. if err := client.backfillStopAgencyIndex(context.Background()); err != nil { + _ = client.Close() return nil, fmt.Errorf("unable to backfill stop agency index: %w", err) } @@ -54,7 +55,7 @@ func NewClient(config Config) (*Client, error) { // the feed hash is unchanged, so such a database would otherwise keep stale data for as // long as its feed stays the same. // -// The legacy check and the indexed/served counts are read-only and run outside any +// The legacy check and the indexed/joinable checks are read-only and run outside any // transaction; the rebuild and the rebuild alone runs inside one, so a failure partway // through never leaves stop_agencies dropped or half-populated. func (c *Client) backfillStopAgencyIndex(ctx context.Context) error { @@ -64,16 +65,25 @@ func (c *Client) backfillStopAgencyIndex(ctx context.Context) error { } if !legacy { - var indexed, served int + var indexed, joinable int + // joinable checks stop times that resolve to a route, not merely that stop_times + // has rows: a stop_times row with a dangling trip_id or route_id would otherwise + // make BuildStopAgencies insert nothing every time, leaving indexed permanently 0 + // and triggering a full rebuild on every NewClient forever. err := c.DB.QueryRowContext(ctx, ` SELECT - (SELECT COUNT(*) FROM stop_agencies), - (SELECT EXISTS (SELECT 1 FROM stop_times)) - `).Scan(&indexed, &served) + (SELECT EXISTS (SELECT 1 FROM stop_agencies)), + (SELECT EXISTS ( + SELECT 1 + FROM stop_times + JOIN trips ON stop_times.trip_id = trips.id + JOIN routes ON trips.route_id = routes.id + )) + `).Scan(&indexed, &joinable) if err != nil { return fmt.Errorf("failed to check stop agency index: %w", err) } - if indexed > 0 || served == 0 { + if indexed > 0 || joinable == 0 { return nil } } diff --git a/gtfsdb/driver_cgo.go b/gtfsdb/driver_cgo.go index f0ce2eea..66c22f39 100644 --- a/gtfsdb/driver_cgo.go +++ b/gtfsdb/driver_cgo.go @@ -5,3 +5,8 @@ package gtfsdb import _ "github.com/mattn/go-sqlite3" // CGo-based SQLite driver const DriverName = "sqlite3" + +// DSN returns the connection string for path with foreign key enforcement enabled. +// foreign_keys is connection-scoped and not persistent, so setting it via PRAGMA on a +// single connection leaves the rest of the pool unenforced; it has to ride the DSN. +func DSN(path string) string { return path + "?_foreign_keys=on" } diff --git a/gtfsdb/driver_pure.go b/gtfsdb/driver_pure.go index 0da6bd98..33046854 100644 --- a/gtfsdb/driver_pure.go +++ b/gtfsdb/driver_pure.go @@ -5,3 +5,8 @@ package gtfsdb import _ "modernc.org/sqlite" // Pure Go SQLite driver const DriverName = "sqlite" + +// DSN returns the connection string for path with foreign key enforcement enabled. +// foreign_keys is connection-scoped and not persistent, so setting it via PRAGMA on a +// single connection leaves the rest of the pool unenforced; it has to ride the DSN. +func DSN(path string) string { return path + "?_pragma=foreign_keys(1)" } diff --git a/gtfsdb/foreign_keys_test.go b/gtfsdb/foreign_keys_test.go new file mode 100644 index 00000000..be82e0f8 --- /dev/null +++ b/gtfsdb/foreign_keys_test.go @@ -0,0 +1,46 @@ +package gtfsdb + +import ( + "context" + "database/sql" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "maglev.onebusaway.org/internal/appconf" +) + +// TestForeignKeysEnabledOnEveryPooledConnection guards against foreign_keys being set on +// only the connection that runs migrations. foreign_keys is connection-scoped and +// non-persistent, so it must ride the DSN to reach every connection in the pool; holding +// several connections open at once (rather than reusing one via sequential Conn calls) is +// what actually exercises that. +func TestForeignKeysEnabledOnEveryPooledConnection(t *testing.T) { + dbPath := filepath.Join(t.TempDir(), "foreign_keys.db") + client, err := NewClient(Config{DBPath: dbPath, Env: appconf.Development}) + require.NoError(t, err) + t.Cleanup(func() { _ = client.Close() }) + + ctx := context.Background() + const connCount = 5 + + conns := make([]*sql.Conn, connCount) + for i := range conns { + conn, err := client.DB.Conn(ctx) + require.NoError(t, err) + conns[i] = conn + } + t.Cleanup(func() { + for _, conn := range conns { + _ = conn.Close() + } + }) + + for i, conn := range conns { + var enabled int + err := conn.QueryRowContext(ctx, "PRAGMA foreign_keys").Scan(&enabled) + require.NoError(t, err) + assert.Equal(t, 1, enabled, "connection %d should have foreign key enforcement on", i) + } +} diff --git a/gtfsdb/fts_queries.go b/gtfsdb/fts_queries.go index 5c836196..a57dc8fe 100644 --- a/gtfsdb/fts_queries.go +++ b/gtfsdb/fts_queries.go @@ -4,7 +4,7 @@ package gtfsdb // sqlc cannot handle queries that use FTS5-specific syntax (MATCH operator, // bm25() function), so these are maintained manually instead of in query.sql. // -// IMPORTANT: If the 'routes', 'stops', 'routes_fts', or 'stops_fts' table schemas change, +// IMPORTANT: If the 'routes', 'stops', 'stop_agencies', 'routes_fts', or 'stops_fts' table schemas change, // the SQL and Go types in this file must be updated manually to match. // Running 'make models' will NOT update this file. @@ -80,6 +80,14 @@ func (q *Queries) SearchRoutesByFullText(ctx context.Context, arg SearchRoutesBy return items, nil } +// searchStopsByName orders results by the combined {agency_id}_{stop_id} ID callers see +// rather than by the raw stop ID. The sort key has to be built here, before LIMIT, or the +// wrong set of stops would be truncated away. +// +// stop_agencies holds one row per (stop, agency) pair, so a stop served by more than one +// agency is resolved with MIN() to the single agency whose prefix its combined ID carries - +// otherwise the join would fan out into one result row per agency. A stop no route serves +// has no row at all and therefore no combined ID; those sort last, by raw stop ID. const searchStopsByName = ` SELECT s.id, @@ -90,12 +98,17 @@ SELECT s.location_type, s.wheelchair_boarding, s.direction, - s.parent_station + s.parent_station, + ( + SELECT MIN(sa.agency_id) + FROM stop_agencies sa + WHERE sa.stop_id = s.id + ) AS agency_id FROM stops s JOIN stops_fts fts ON s.rowid = fts.rowid WHERE fts.stop_name MATCH ? -ORDER BY s.id +ORDER BY agency_id IS NULL, agency_id || '_' || s.id, s.id LIMIT ? ` @@ -114,6 +127,9 @@ type SearchStopsByNameRow struct { WheelchairBoarding sql.NullInt64 Direction sql.NullString ParentStation sql.NullString + // AgencyID is the lowest agency ID among the routes serving the stop, and is null + // when no route serves it. + AgencyID sql.NullString } func (q *Queries) SearchStopsByName(ctx context.Context, arg SearchStopsByNameParams) ([]SearchStopsByNameRow, error) { @@ -136,6 +152,7 @@ func (q *Queries) SearchStopsByName(ctx context.Context, arg SearchStopsByNamePa &i.WheelchairBoarding, &i.Direction, &i.ParentStation, + &i.AgencyID, ); err != nil { return nil, err } diff --git a/gtfsdb/fts_queries_test.go b/gtfsdb/fts_queries_test.go index 6e21acfc..04001583 100644 --- a/gtfsdb/fts_queries_test.go +++ b/gtfsdb/fts_queries_test.go @@ -254,7 +254,8 @@ func TestSearchStopsByName(t *testing.T) { require.NoError(t, err) require.Len(t, results, 3) - // Assert that the result set is ordered ascending by s.id (hub1 < s1 < s3). + // No route serves these stops, so none resolves an agency to build a combined ID + // from, and they fall through to the raw stop ID tiebreak (hub1 < s1 < s3). assert.Equal(t, "Main Street Hub", results[0].Name.String) assert.Equal(t, "Main Street Station", results[1].Name.String) assert.Equal(t, "Main Street Mall", results[2].Name.String) @@ -327,6 +328,92 @@ func TestSearchStopsByName(t *testing.T) { }) } +// stopServedByAgency creates a stop plus the route, trip and stop time that make the given +// agency serve it, so the search query has an agency to resolve for that stop. +func stopServedByAgency(t *testing.T, client *Client, agencyID, stopID, stopName string) { + t.Helper() + ctx := context.Background() + + _, err := client.Queries.CreateAgency(ctx, CreateAgencyParams{ + ID: agencyID, + Name: "Agency " + agencyID, + Url: "http://" + agencyID + ".example.test", + Timezone: "America/New_York", + }) + require.NoError(t, err) + + _, err = client.Queries.CreateStop(ctx, CreateStopParams{ + ID: stopID, Name: nulls.String(stopName), Lat: 40.0, Lon: -74.0, + }) + require.NoError(t, err) + + routeID := "route_" + stopID + _, err = client.Queries.CreateRoute(ctx, CreateRouteParams{ + ID: routeID, AgencyID: agencyID, Type: 3, + }) + require.NoError(t, err) + + tripID := "trip_" + stopID + _, err = client.Queries.CreateTrip(ctx, CreateTripParams{ + ID: tripID, RouteID: routeID, ServiceID: "service_1", + }) + require.NoError(t, err) + + _, err = client.Queries.CreateStopTime(ctx, CreateStopTimeParams{ + TripID: tripID, StopID: stopID, StopSequence: 1, ArrivalTime: 28800, DepartureTime: 28800, + }) + require.NoError(t, err) +} + +func TestSearchStopsByNameOrdersByCombinedID(t *testing.T) { + client := createFTSTestClient(t) + defer func() { _ = client.Close() }() + + ctx := context.Background() + + _, err := client.DB.ExecContext(ctx, ` + INSERT INTO calendar (id, monday, tuesday, wednesday, thursday, friday, saturday, sunday, start_date, end_date) + VALUES ('service_1', 1, 1, 1, 1, 1, 1, 1, '20240101', '20251231') + `) + require.NoError(t, err) + + // Raw stop ID order (aaa, zzz) is the reverse of combined ID order (111_zzz, 999_aaa). + stopServedByAgency(t, client, "999", "aaa", "Ordertest Alpha") + stopServedByAgency(t, client, "111", "zzz", "Ordertest Zulu") + + // No route serves this stop, so it has no combined ID to sort by. + _, err = client.Queries.CreateStop(ctx, CreateStopParams{ + ID: "bbb", Name: nulls.String("Ordertest Bravo"), Lat: 40.0, Lon: -74.0, + }) + require.NoError(t, err) + + require.NoError(t, buildStopAgencyIndex(ctx, client.Queries)) + + results, err := client.Queries.SearchStopsByName(ctx, SearchStopsByNameParams{ + SearchQuery: "Ordertest", + Limit: 10, + }) + require.NoError(t, err) + require.Len(t, results, 3) + + assert.Equal(t, []string{"zzz", "aaa", "bbb"}, []string{results[0].ID, results[1].ID, results[2].ID}, + "stops must be ordered by combined ID, with agency-less stops last") + + assert.Equal(t, "111", results[0].AgencyID.String) + assert.Equal(t, "999", results[1].AgencyID.String) + assert.False(t, results[2].AgencyID.Valid, "a stop no route serves resolves no agency") + + t.Run("limit keeps the lowest combined IDs", func(t *testing.T) { + capped, err := client.Queries.SearchStopsByName(ctx, SearchStopsByNameParams{ + SearchQuery: "Ordertest", + Limit: 1, + }) + require.NoError(t, err) + require.Len(t, capped, 1) + assert.Equal(t, "zzz", capped[0].ID) + }) +} + func TestSearchStopsByNameEmptyDB(t *testing.T) { client := createFTSTestClient(t) defer func() { _ = client.Close() }() diff --git a/gtfsdb/helpers.go b/gtfsdb/helpers.go index bb1a4378..efbd3dfd 100644 --- a/gtfsdb/helpers.go +++ b/gtfsdb/helpers.go @@ -137,7 +137,7 @@ func createDB(config Config) (*sql.DB, error) { return nil, fmt.Errorf("test database must use in-memory storage, got path: %s", config.DBPath) } - db, err := sql.Open(DriverName, config.DBPath) + db, err := sql.Open(DriverName, DSN(config.DBPath)) if err != nil { return nil, err } @@ -146,11 +146,13 @@ func createDB(config Config) (*sql.DB, error) { ctx := context.Background() err = configureSQLitePerformance(ctx, db) if err != nil { + _ = db.Close() return nil, fmt.Errorf("error configuring SQLite performance: %w", err) } err = performDatabaseMigration(ctx, db) if err != nil { + _ = db.Close() return nil, fmt.Errorf("error performing database migration: %w", err) } diff --git a/gtfsdb/helpers_test.go b/gtfsdb/helpers_test.go index b391eac9..5ba83c3b 100644 --- a/gtfsdb/helpers_test.go +++ b/gtfsdb/helpers_test.go @@ -197,6 +197,13 @@ func TestNewClient_RecordsQueryMetricsWhenOnlyMetricsEnabled(t *testing.T) { agency_id TEXT NOT NULL, PRIMARY KEY (stop_id, agency_id) ); + CREATE TABLE IF NOT EXISTS routes ( + id TEXT PRIMARY KEY + ); + CREATE TABLE IF NOT EXISTS trips ( + id TEXT PRIMARY KEY, + route_id TEXT NOT NULL + ); CREATE TABLE IF NOT EXISTS stop_times ( trip_id TEXT NOT NULL ); diff --git a/gtfsdb/schema.sql b/gtfsdb/schema.sql index 538997a7..5ed5d0d9 100644 --- a/gtfsdb/schema.sql +++ b/gtfsdb/schema.sql @@ -1,3 +1,7 @@ +-- foreign_keys is connection-scoped, so this PRAGMA only enforces on the one connection +-- that runs migrations. Pool-wide enforcement actually comes from the DSN built by +-- gtfsdb.DSN (see driver_cgo.go / driver_pure.go); this line is kept because it's free +-- and documents the intent at the schema level. PRAGMA foreign_keys = ON; -- migrate @@ -170,7 +174,8 @@ CREATE TABLE -- per query, which is too slow to do while serving a request. Rebuilt from scratch on every -- import. A caller wanting a single agency per stop takes MIN(agency_id) GROUP BY stop_id. -- IF NOT EXISTS cannot reshape a table created under an earlier, single-column-PK version --- of this schema; Client.rebuildLegacyStopAgenciesTable handles that case at startup. +-- of this schema; Client.hasLegacyStopAgenciesTable detects that case at startup and +-- Client.recreateStopAgenciesTable rebuilds it. CREATE TABLE IF NOT EXISTS stop_agencies ( stop_id TEXT NOT NULL, diff --git a/gtfsdb/stop_agency_test.go b/gtfsdb/stop_agency_test.go index 36a7e31f..6c2b73cf 100644 --- a/gtfsdb/stop_agency_test.go +++ b/gtfsdb/stop_agency_test.go @@ -1,7 +1,9 @@ package gtfsdb import ( + "bytes" "context" + "log/slog" "os" "path/filepath" "testing" @@ -270,3 +272,46 @@ func TestBuildStopAgencyIndex_RebuildsFromScratch(t *testing.T) { require.NoError(t, err) assert.Zero(t, stray, "rebuild should drop rows for stops no route serves") } + +func TestBackfillStopAgencyIndex_SettlesWhenNoStopTimeJoinsARoute(t *testing.T) { + client := newTestClientWithRABA(t) + ctx := context.Background() + + _, err := client.DB.ExecContext(ctx, "DELETE FROM stop_agencies") + require.NoError(t, err) + + // Replace every stop_times row with one whose trip_id resolves to nothing, so the + // index has stop_times but none of them join through to a route. FK checks are off + // just long enough to write the dangling reference. + _, err = client.DB.ExecContext(ctx, "PRAGMA foreign_keys = OFF") + require.NoError(t, err) + _, err = client.DB.ExecContext(ctx, "DELETE FROM stop_times") + require.NoError(t, err) + _, err = client.DB.ExecContext(ctx, ` + INSERT INTO stop_times (trip_id, arrival_time, departure_time, stop_id, stop_sequence) + SELECT 'no-such-trip', 0, 0, id, 0 FROM stops LIMIT 1 + `) + require.NoError(t, err) + _, err = client.DB.ExecContext(ctx, "PRAGMA foreign_keys = ON") + require.NoError(t, err) + + require.NoError(t, client.backfillStopAgencyIndex(ctx), + "a stop_times row that can't join to a route should settle, not error") + + var indexed int + require.NoError(t, client.DB.QueryRowContext(ctx, "SELECT COUNT(*) FROM stop_agencies").Scan(&indexed)) + assert.Zero(t, indexed, "nothing joins to a route, so the index should stay empty") + + // A second call must settle without rebuilding: capture the default logger, since + // backfillStopAgencyIndex logs "rebuilding stop agency index" only when it actually + // runs the rebuild transaction. Before the fix, indexed staying at 0 forever meant + // every call rebuilt regardless of whether anything was joinable. + previousLogger := slog.Default() + var logBuf bytes.Buffer + slog.SetDefault(slog.New(slog.NewTextHandler(&logBuf, nil))) + err = client.backfillStopAgencyIndex(ctx) + slog.SetDefault(previousLogger) + require.NoError(t, err) + assert.NotContains(t, logBuf.String(), "rebuilding stop agency index", + "a settled index with nothing joinable should not trigger another rebuild") +} diff --git a/internal/restapi/search_stops_handler.go b/internal/restapi/search_stops_handler.go index bf5b4496..2ec6ee7a 100644 --- a/internal/restapi/search_stops_handler.go +++ b/internal/restapi/search_stops_handler.go @@ -200,10 +200,9 @@ func (api *RestAPI) searchStopsHandler(w http.ResponseWriter, r *http.Request) { continue } - // GetRoutesForStops orders by (agency_id, route_id) as TEXT (lexicographic, not - // numeric), so the first route yields the lexicographically lowest agency ID - // serving this stop - a stable, if not numeric-minimal, choice for multi-agency stops. - agencyID, _, _ := utils.ExtractAgencyIDAndCodeID(routeIDs[0]) + // The search query resolves this from the precomputed index, so the stop's combined + // ID here matches the key the results were sorted by. + agencyID := nulls.StringOrEmpty(s.AgencyID) stopModels = append(stopModels, api.buildSearchStopModel(ctx, agencyID, stopFromSearchRow(s), routeIDs)) keptStopIDs = append(keptStopIDs, s.ID) diff --git a/internal/restapi/search_stops_handler_test.go b/internal/restapi/search_stops_handler_test.go index 00d2cad9..5fe58255 100644 --- a/internal/restapi/search_stops_handler_test.go +++ b/internal/restapi/search_stops_handler_test.go @@ -36,6 +36,22 @@ func registerFixtureCleanup(t *testing.T, db *sql.DB, statements ...string) { }) } +// rebuildStopAgencyIndex refreshes the precomputed stop-to-agency index that stop search +// orders by. Tests insert fixture stops straight into the database, bypassing the import +// that builds the index in production. +func rebuildStopAgencyIndex(t *testing.T, api *RestAPI) { + t.Helper() + ctx := context.Background() + require.NoError(t, api.GtfsManager.GtfsDB.Queries.ClearStopAgencies(ctx)) + require.NoError(t, api.GtfsManager.GtfsDB.Queries.BuildStopAgencies(ctx)) +} + +// clearIndexedStopAgencies removes the given stops from the index. Fixture cleanup has to +// run this before deleting the stops or agencies the index rows point at. +func clearIndexedStopAgencies(stopIDs string) string { + return `DELETE FROM stop_agencies WHERE stop_id IN (` + stopIDs + `)` +} + func TestSearchStopsHandlerRequiresValidApiKey(t *testing.T) { api := createTestApi(t) defer api.Shutdown() @@ -357,6 +373,81 @@ func TestSearchStopsHandlerLimitExceeded(t *testing.T) { assert.Len(t, stopsRespExceeded.Data.List, totalMatches-1) } +func TestSearchStopsHandlerOrdersByCombinedID(t *testing.T) { + api := createTestApi(t) + defer api.Shutdown() + + ctx := context.Background() + + registerFixtureCleanup(t, api.GtfsManager.GtfsDB.DB, + clearIndexedStopAgencies(`'aaa_stop', 'zzz_stop'`), + `DELETE FROM stop_times WHERE trip_id IN ('trip_ord_alpha', 'trip_ord_zulu')`, + `DELETE FROM trips WHERE id IN ('trip_ord_alpha', 'trip_ord_zulu')`, + `DELETE FROM routes WHERE id IN ('route_ord_alpha', 'route_ord_zulu')`, + `DELETE FROM agencies WHERE id IN ('111', '999')`, + `DELETE FROM stops WHERE id IN ('aaa_stop', 'zzz_stop')`, + ) + + _, err := api.GtfsManager.GtfsDB.DB.ExecContext(ctx, ` + INSERT INTO agencies (id, name, url, timezone) + VALUES ('111', 'Agency Low', 'http://low.example.test', 'America/Los_Angeles'), + ('999', 'Agency High', 'http://high.example.test', 'America/Los_Angeles') + `) + require.NoError(t, err) + + _, err = api.GtfsManager.GtfsDB.DB.ExecContext(ctx, ` + INSERT OR IGNORE INTO calendar (id, monday, tuesday, wednesday, thursday, friday, saturday, sunday, start_date, end_date) + VALUES ('service_1', 1, 1, 1, 1, 1, 1, 1, '20240101', '20251231') + `) + require.NoError(t, err) + + // Raw stop ID order (aaa_stop, zzz_stop) is the reverse of combined ID order + // (111_zzz_stop, 999_aaa_stop). + _, err = api.GtfsManager.GtfsDB.DB.ExecContext(ctx, ` + INSERT INTO stops (id, code, name, lat, lon, location_type, wheelchair_boarding) + VALUES ('aaa_stop', 'A1', 'Ordertest Alpha', 40.0, -120.0, 0, 1), + ('zzz_stop', 'Z1', 'Ordertest Zulu', 40.0, -120.0, 0, 1) + `) + require.NoError(t, err) + + _, err = api.GtfsManager.GtfsDB.DB.ExecContext(ctx, ` + INSERT INTO routes (id, agency_id, short_name, type) + VALUES ('route_ord_alpha', '999', 'RT-A', 3), ('route_ord_zulu', '111', 'RT-Z', 3) + `) + require.NoError(t, err) + + _, err = api.GtfsManager.GtfsDB.DB.ExecContext(ctx, ` + INSERT INTO trips (id, route_id, service_id) + VALUES ('trip_ord_alpha', 'route_ord_alpha', 'service_1'), + ('trip_ord_zulu', 'route_ord_zulu', 'service_1') + `) + require.NoError(t, err) + + _, err = api.GtfsManager.GtfsDB.DB.ExecContext(ctx, ` + INSERT INTO stop_times (trip_id, stop_id, stop_sequence, arrival_time, departure_time) + VALUES ('trip_ord_alpha', 'aaa_stop', 1, 28800, 28800), + ('trip_ord_zulu', 'zzz_stop', 1, 28800, 28800) + `) + require.NoError(t, err) + + rebuildStopAgencyIndex(t, api) + + resp, stopsResp := callAPIHandler[StopsResponse](t, api, searchStopsURL(url.Values{"input": {"Ordertest"}})) + require.Equal(t, http.StatusOK, resp.StatusCode) + require.Len(t, stopsResp.Data.List, 2) + + assert.Equal(t, []string{"111_zzz_stop", "999_aaa_stop"}, + []string{stopsResp.Data.List[0].ID, stopsResp.Data.List[1].ID}, + "results must be ordered by combined stop ID") + + // maxCount truncates on that order, so it decides which stop is returned at all. + respCapped, capped := callAPIHandler[StopsResponse](t, api, searchStopsURL(url.Values{"input": {"Ordertest"}, "maxCount": {"1"}})) + require.Equal(t, http.StatusOK, respCapped.StatusCode) + require.Len(t, capped.Data.List, 1) + assert.Equal(t, "111_zzz_stop", capped.Data.List[0].ID) + assert.True(t, capped.Data.LimitExceeded) +} + func TestSearchStopsHandlerParentStationReferences(t *testing.T) { api := createTestApi(t) defer api.Shutdown() @@ -364,6 +455,7 @@ func TestSearchStopsHandlerParentStationReferences(t *testing.T) { ctx := context.Background() registerFixtureCleanup(t, api.GtfsManager.GtfsDB.DB, + clearIndexedStopAgencies(`'child_stop_1', 'parent_stat_1', 'child_stop_2', 'parent_stat_2', 'child_stop_shared', 'child_stop_multi_agency'`), `DELETE FROM stop_times WHERE trip_id IN ('trip_parent_test', 'trip_parent_test_2', 'trip_parent_only', 'trip_multi_agency')`, `DELETE FROM trips WHERE id IN ('trip_parent_test', 'trip_parent_test_2', 'trip_parent_only', 'trip_multi_agency')`, `DELETE FROM routes WHERE id IN ('route_parent_test', 'route_parent_test_2', 'route_parent_only')`, @@ -500,6 +592,8 @@ func TestSearchStopsHandlerParentStationReferences(t *testing.T) { `) require.NoError(t, err) + rebuildStopAgencyIndex(t, api) + resp, stopsResp := callAPIHandler[StopsResponse](t, api, searchStopsURL(url.Values{"input": {"Child Stop"}})) assert.Equal(t, http.StatusOK, resp.StatusCode) assert.Equal(t, http.StatusOK, stopsResp.Code) @@ -507,8 +601,14 @@ func TestSearchStopsHandlerParentStationReferences(t *testing.T) { require.Len(t, stopsResp.Data.List, 4) stopsByName := make(map[string]models.Stop, len(stopsResp.Data.List)) + seenIDs := make(map[string]bool, len(stopsResp.Data.List)) for _, stop := range stopsResp.Data.List { stopsByName[stop.Name] = stop + // child_stop_multi_agency is served by both 888 and 999: stop_agencies holds one + // row per (stop, agency) pair, so joining it without collapsing to a single agency + // would return this stop twice. + assert.False(t, seenIDs[stop.ID], "stop %q appeared more than once in the result list", stop.ID) + seenIDs[stop.ID] = true } child1, ok := stopsByName["Child Stop One"] @@ -606,6 +706,7 @@ func TestSearchStopsHandlerRouteTypeExclusion(t *testing.T) { // The RABA agency and the service_1 calendar are shared records and are left in place. registerFixtureCleanup(t, db, + clearIndexedStopAgencies(`'zero_route_stop', 'school_bus_stop', 'valid_bus_stop', 'two_school_routes_stop', 'limit_ghost_1', 'limit_ghost_2', 'limit_valid_3'`), `DELETE FROM stop_times WHERE trip_id IN ('school_trip_1', 'school_trip_2', 'valid_trip_1')`, `DELETE FROM trips WHERE id IN ('school_trip_1', 'school_trip_2', 'valid_trip_1')`, `DELETE FROM routes WHERE id IN ('school_route_1', 'school_route_2', 'valid_route_1')`, @@ -646,6 +747,8 @@ func TestSearchStopsHandlerRouteTypeExclusion(t *testing.T) { `) require.NoError(t, err) + rebuildStopAgencyIndex(t, api) + // Test 0 routes exclusion resp, stopsResp := callAPIHandler[StopsResponse](t, api, searchStopsURL(url.Values{"input": {"Ghost"}})) assert.Equal(t, http.StatusOK, resp.StatusCode) @@ -674,8 +777,9 @@ func TestSearchStopsHandlerRouteTypeExclusion(t *testing.T) { resp, stopsResp = callAPIHandler[StopsResponse](t, api, searchStopsURL(url.Values{"input": {"Limit Test"}, "maxCount": {"2"}})) assert.Equal(t, http.StatusOK, resp.StatusCode) assert.True(t, stopsResp.Data.LimitExceeded, "Expected LimitExceeded to be true because FTS query matched 3 limits") - // SearchStopsByName orders by stop ID, so the 3 matches are fetched as limit_ghost_1, - // limit_ghost_2, limit_valid_3; truncating to maxCount=2 leaves the two zero-route - // ghosts, both of which the filter drops. - assert.Empty(t, stopsResp.Data.List, "Expected no items after filtering truncated results") + // The two zero-route ghosts resolve no agency, so they have no combined ID to sort by + // and fall after limit_valid_3. It takes the first of the two cap slots; the ghost + // filling the second is then dropped by the zero-route filter. + require.Len(t, stopsResp.Data.List, 1, "Expected only the routed stop to survive filtering") + assert.True(t, strings.HasSuffix(stopsResp.Data.List[0].ID, "limit_valid_3")) }