Fix stop time fetching in added or replacement trips #6245

miklcct · 2024-11-08T15:52:29Z

Summary

This fixes problems when reading stop times for added or replacement trips.

The logic of fetching trip times is moved into TransitService. The list is optional because it can be empty if the trip doesn't run at all on a specified date (in contrast, an empty list would theoretically mean that the trip does run on the date, but it doesn't make any call for the whole journey).

Also, the behaviour of invalid date is changed to produce an error instead of returning null silently in the GTFS GraphQL API, since null is a valid return value meaning the trip doesn't run.

Issue

Fixes #6088
Fixes #6242

Unit tests

Unit tests are added into TransitService for the new methods, for both existing and added trips.

GraphQL Integration tests are added for one added trip and one replacement trip. This is the first time real-time updates can be integration tested.

Documentation

None needed

codecov · 2024-11-08T15:59:16Z

Codecov Report

Attention: Patch coverage is 63.38028% with 26 lines in your changes missing coverage. Please review.

Project coverage is 69.94%. Comparing base (cbdb640) to head (4e542a7).
Report is 68 commits behind head on dev-2.x.

Files with missing lines	Patch %	Lines
...java/org/opentripplanner/model/TripTimeOnDate.java	9.09%	3 Missing and 7 partials ⚠️
...entripplanner/apis/gtfs/datafetchers/TripImpl.java	87.09%	3 Missing and 1 partial ⚠️
...model/model/timetable/DatedServiceJourneyType.java	0.00%	4 Missing ⚠️
...transmodel/model/timetable/ServiceJourneyType.java	20.00%	4 Missing ⚠️
...planner/transit/service/DefaultTransitService.java	80.00%	3 Missing and 1 partial ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##             dev-2.x    #6245      +/-   ##
=============================================
+ Coverage      69.85%   69.94%   +0.09%     
- Complexity     17920    17948      +28     
=============================================
  Files           2035     2034       -1     
  Lines          76495    76494       -1     
  Branches        7824     7826       +2     
=============================================
+ Hits           53433    53503      +70     
+ Misses         20325    20241      -84     
- Partials        2737     2750      +13

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

It has nothing to do with routing and belongs to the wrong package

better to let error propagate

it can be empty if the trip doesn't run at all

application/src/main/java/org/opentripplanner/transit/service/TransitService.java

application/src/main/java/org/opentripplanner/transit/service/DefaultTransitService.java

application/src/test/java/org/opentripplanner/apis/gtfs/GraphQLIntegrationTest.java

application/src/main/java/org/opentripplanner/model/TripTimeOnDate.java

miklcct · 2024-11-23T22:58:22Z

I am going to wait for #5393 first.

# Conflicts: # application/src/test/java/org/opentripplanner/transit/service/DefaultTransitServiceTest.java

application/src/main/java/org/opentripplanner/model/TripTimeOnDate.java

# Conflicts: # application/src/main/java/org/opentripplanner/apis/gtfs/datafetchers/TripImpl.java # application/src/main/java/org/opentripplanner/apis/transmodel/model/timetable/ServiceJourneyType.java # application/src/main/java/org/opentripplanner/routing/TripTimeOnDateHelper.java # application/src/main/java/org/opentripplanner/transit/service/TransitService.java

application/src/main/java/org/opentripplanner/updater/trip/TimetableSnapshotSource.java

application/src/main/java/org/opentripplanner/transit/service/DefaultTransitService.java

application/src/test/java/org/opentripplanner/updater/trip/TripUpdateBuilder.java

leonardehrenfried · 2024-12-16T11:55:45Z

Please resolve the conflicts.

# Conflicts: # application/src/test/java/org/opentripplanner/apis/gtfs/GraphQLIntegrationTest.java # application/src/test/java/org/opentripplanner/transit/service/DefaultTransitServiceTest.java # application/src/test/resources/org/opentripplanner/apis/gtfs/expectations/patterns.json

application/src/test/java/org/opentripplanner/updater/trip/TripUpdateBuilder.java

vpaturet · 2025-01-17T13:04:07Z

application/src/main/java/org/opentripplanner/transit/service/DefaultTransitService.java

+    Timetable timetable = findTimetable(pattern, serviceDate);
+
+    // If realtime moved pattern back to original trip, fetch it instead
+    if (timetable.getTripIndex(trip.getId()) == -1) {


This check was a workaround/fallback for a bug that has been fixed recently when refactoring TimetableSnapshot. I would rather remove this. If the bug is reintroduced it will fail hard with an NPE in the same method and that will be easy to catch.
Non-blocking for this PR, I can do it in a follow-up PR.

vpaturet · 2025-01-17T13:31:45Z

application/src/main/java/org/opentripplanner/model/TripTimeOnDate.java

  public static List<TripTimeOnDate> fromTripTimes(
    Timetable table,
    Trip trip,
    LocalDate serviceDate,
    Instant midnight
  ) {
    TripTimes times = table.getTripTimes(trip);
+    if (times == null) {


Is there any code path that leads to this condition after your fix is applied?
(I do not see any unit test that touches this line)

vpaturet · 2025-01-17T13:31:46Z

application/src/main/java/org/opentripplanner/model/TripTimeOnDate.java

  public static List<TripTimeOnDate> fromTripTimes(Timetable table, Trip trip) {
    TripTimes times = table.getTripTimes(trip);
+    if (times == null) {


Is there any code path that leads to this condition after your fix is applied?
(I do not see any unit test that touches this line)

vpaturet · 2025-01-20T16:42:57Z

application/src/main/java/org/opentripplanner/transit/service/DefaultTransitService.java

@@ -96,6 +101,47 @@ public DefaultTransitService(
    this.timetableSnapshot = timetableSnapshotBuffer;
  }

+  @Override
+  public Optional<List<TripTimeOnDate>> getScheduledTripTimes(Trip trip) {
+    TripPattern tripPattern = findPattern(trip);


According to the javadoc, TransitService#findPattern(Trip) should not return null, even for an added trip:

Return the scheduled trip pattern for a given trip.
If the trip is an added trip (extra journey), return the initial trip pattern for this trip.

When an added trip is submitted to TimetableSnapshot, it should be registered in the index TimetableSnapshot.realTimeAddedPatternForTrip. DefaultTransitService#findPattern(Trip) queries this index to return the pattern of an added trip.

In the case of the SIRI updater, there is logic in AddedTripBuilder that ensures that the flag RealTimeTripUpdate.tripCreation is set, and this guarantees that the realTimeAddedPatternForTrip index is updated in TimetableSnapshot.
It seems this logic is missing in the case of the GTFS updater.

Since we aim at minimizing the differences between the SIRI updater and the GTFS updater, we should probably apply the logic present in the SIRI updater to the GTFS updater.

miklcct requested a review from a team as a code owner November 8, 2024 15:52

miklcct force-pushed the fix-departureArrivalStopTime-npe branch from 36ef65d to ed3bb9b Compare November 8, 2024 15:53

miklcct force-pushed the fix-departureArrivalStopTime-npe branch 3 times, most recently from 428a549 to df345c2 Compare November 8, 2024 16:08

miklcct added 4 commits November 8, 2024 16:11

add test for opentripplanner#6242

5fcdf4a

add Nullable annotation

782730d

refactor arrival and departure stop time

98c5d13

fix opentripplanner#6242

38c3a5a

miklcct force-pushed the fix-departureArrivalStopTime-npe branch from df345c2 to 38c3a5a Compare November 8, 2024 16:12

t2gran added this to the 2.7 (next release) milestone Nov 12, 2024

t2gran requested review from leonardehrenfried and optionsome November 12, 2024 10:26

miklcct mentioned this pull request Nov 14, 2024

Add query for cancelled trips to GTFS GraphQL API #5393

Merged

miklcct added 9 commits November 14, 2024 16:47

delete TripTimeOnDateHelper and move the logic to DefaultTransitService

8b4b0fe

It has nothing to do with routing and belongs to the wrong package

move scheduled trip time fetching logic to TransitService

816e9c0

use trip time logic in TransitService from GTFS GraphQL TripImpl

9df81a3

reformat code

8cf5e9b

do not silently return null in case of invalid date

d6da964

better to let error propagate

refactor date parsing

cd74120

reformat code

29db2ef

add test for new transit service methods

38dd2aa

use optional for trip time on dates

d3d283f

it can be empty if the trip doesn't run at all

miklcct force-pushed the fix-departureArrivalStopTime-npe branch from 39ca3c3 to d3d283f Compare November 14, 2024 18:32

Merge branch 'dev-2.x' into fix-departureArrivalStopTime-npe

a7dc78e

t2gran reviewed Nov 19, 2024

View reviewed changes

application/src/main/java/org/opentripplanner/transit/service/TransitService.java Show resolved Hide resolved

t2gran reviewed Nov 19, 2024

View reviewed changes

application/src/main/java/org/opentripplanner/transit/service/TransitService.java Show resolved Hide resolved

t2gran reviewed Nov 19, 2024

View reviewed changes

application/src/main/java/org/opentripplanner/transit/service/DefaultTransitService.java Show resolved Hide resolved

leonardehrenfried reviewed Nov 21, 2024

View reviewed changes

application/src/test/java/org/opentripplanner/apis/gtfs/GraphQLIntegrationTest.java Outdated Show resolved Hide resolved

leonardehrenfried reviewed Nov 21, 2024

View reviewed changes

application/src/main/java/org/opentripplanner/model/TripTimeOnDate.java Show resolved Hide resolved

miklcct added 5 commits November 26, 2024 16:19

Merge branch 'dev-2.x' into fix-departureArrivalStopTime-npe

5ab09f4

# Conflicts: # application/src/test/java/org/opentripplanner/transit/service/DefaultTransitServiceTest.java

update test for latest dev

7ad5e63

stub the real time information in GraphQL intergration test

bdc6c0a

Merge branch 'dev-2.x' into fix-departureArrivalStopTime-npe

f6c7cb7

format JSON files

4b50129

t2gran reviewed Nov 29, 2024

View reviewed changes

application/src/main/java/org/opentripplanner/model/TripTimeOnDate.java Show resolved Hide resolved

habrahamsson-skanetrafiken requested a review from leonardehrenfried December 10, 2024 10:19

leonardehrenfried reviewed Dec 16, 2024

View reviewed changes

application/src/main/java/org/opentripplanner/updater/trip/TimetableSnapshotSource.java Outdated Show resolved Hide resolved

leonardehrenfried reviewed Dec 16, 2024

View reviewed changes

application/src/main/java/org/opentripplanner/transit/service/DefaultTransitService.java Show resolved Hide resolved

leonardehrenfried reviewed Dec 16, 2024

View reviewed changes

application/src/test/java/org/opentripplanner/updater/trip/TripUpdateBuilder.java Outdated Show resolved Hide resolved

miklcct added 2 commits December 17, 2024 12:08

fix test conflict

f72b9a4

optionsome removed their request for review December 19, 2024 14:05

apply review feedback

236dc6e

leonardehrenfried reviewed Jan 2, 2025

View reviewed changes

application/src/test/java/org/opentripplanner/updater/trip/TripUpdateBuilder.java Outdated Show resolved Hide resolved

TripUpdateBuilder is no longer needed

4e542a7

habrahamsson-skanetrafiken requested a review from vpaturet January 7, 2025 09:45

leonardehrenfried added the Real-Time Update The issue/PR is related to RealTime updates label Jan 9, 2025

leonardehrenfried approved these changes Jan 9, 2025

View reviewed changes

Merge branch 'dev-2.x' into fix-departureArrivalStopTime-npe

7a85d1f

vpaturet requested changes Jan 20, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix stop time fetching in added or replacement trips #6245

Fix stop time fetching in added or replacement trips #6245

miklcct commented Nov 8, 2024 •

edited

Loading

codecov bot commented Nov 8, 2024 •

edited

Loading

miklcct commented Nov 23, 2024

leonardehrenfried commented Dec 16, 2024

vpaturet Jan 17, 2025

vpaturet Jan 17, 2025

vpaturet Jan 17, 2025

vpaturet Jan 20, 2025 •

edited

Loading

Fix stop time fetching in added or replacement trips #6245

Are you sure you want to change the base?

Fix stop time fetching in added or replacement trips #6245

Conversation

miklcct commented Nov 8, 2024 • edited Loading

Summary

Issue

Unit tests

Documentation

codecov bot commented Nov 8, 2024 • edited Loading

Codecov Report

miklcct commented Nov 23, 2024

leonardehrenfried commented Dec 16, 2024

vpaturet Jan 17, 2025

Choose a reason for hiding this comment

vpaturet Jan 17, 2025

Choose a reason for hiding this comment

vpaturet Jan 17, 2025

Choose a reason for hiding this comment

vpaturet Jan 20, 2025 • edited Loading

Choose a reason for hiding this comment

miklcct commented Nov 8, 2024 •

edited

Loading

codecov bot commented Nov 8, 2024 •

edited

Loading

vpaturet Jan 20, 2025 •

edited

Loading