return stoptimesForDate from the schedule if it is called with no parameter on a day when the trip does not run #6480

tkalvas · 2025-02-21T13:15:06Z

Fixes Issue #6475

Technically the solution here is wrong, but longstanding UIs (like digitransit-ui) rely on getting the scheduled timetable when running stoptimesForDate without a parameter on a wrong day.

…is queried

codecov · 2025-02-21T13:21:06Z

Codecov Report

Attention: Patch coverage is 71.42857% with 4 lines in your changes missing coverage. Please review.

Project coverage is 70.18%. Comparing base (04fadbf) to head (fd6c9bb).
Report is 223 commits behind head on dev-2.x.

Files with missing lines	Patch %	Lines
...ipplanner/apis/gtfs/datafetchers/StoptimeImpl.java	33.33%	2 Missing ⚠️
...entripplanner/apis/gtfs/datafetchers/TripImpl.java	50.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##             dev-2.x    #6480      +/-   ##
=============================================
- Coverage      70.20%   70.18%   -0.03%     
- Complexity     18313    18317       +4     
=============================================
  Files           2080     2082       +2     
  Lines          77182    77220      +38     
  Branches        7831     7833       +2     
=============================================
+ Hits           54183    54193      +10     
- Misses         20230    20259      +29     
+ Partials        2769     2768       -1

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

🚀 New features to boost your workflow:

📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

…es even if the trip does not run today. Technically wrong, but UIs rely on this, and have for a long time

…explicit parameter of today

…the corresponding case with useScheduledWhenNonRunning

…-fix

optionsome · 2025-03-06T11:32:07Z

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

+  public Optional<List<TripTimeOnDate>> getTripTimeOnDates(
+    Trip trip,
+    LocalDate serviceDate,
+    boolean useScheduledWhenNonRunning


The name of this parameter is a bit confusing as my first intuition at least is that it would use scheduled times from the given date instead of trying to use real-time when trip is cancelled, or something like that. Maybe something like fallbackToNextScheduledDate instead.

I think we usually use the word "schedule" for the describing the "planned data". We had this discussion last year at some point but I can't quite remember what we decided. However, in this case I think "scheduled date" doesn't indicate as strongly that we would be using planned data so I think we could improve the name. Problem with fallbackToBaseTimetable is that it doesn't tell that the returned times could actually be from another service date.

fallbackToBaseTimetableOnRunningDate?

I know Joel will say that doesn't indicate which date, but the fact is that it does not matter which date it is, because it's not really any date. It's just the base case, unchanged by realtime data. The returned result will not include dates, if you try to fetch one it will be null.

I think we use "running date" sometimes to call the actual date that the trip is servicing on. So for example, if a trip lasts 3 days, it's running on 3+ running dates. So this could be slightly confusing as it could confuse people into thinking that the serviceDate parameter could be used as a running date if there is no service on the service date. Maybe fallbackToNextPlannedTimetable? I think the words we use for the planned data are either scheduled or planned.

fallbackToPlannedTimetableOnNonRunningDate ?

As said, I think we should avoid using the word running date here because it can mean slightly different thing. Also, I first thought you mean timetable on non running date, but I guess you mean you fall back to planned timetable when there is no service on the service date? I know that the "next" I used in my suggestion is not really that relevant, it could be any date that the trip is servicing on, but I wanted to somehow tell that the timetables don't necessarily come from the given service date.

Yes, we want to specify that "we fall back to the planned timetable when the service does not run on the given service date".

How about fallbackToPlannedTimetableOnNoServiceDate?

That's ok for me.

optionsome · 2025-03-06T11:37:25Z

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

+   * @return if the trip doesn't run on the date specified, return schedule if
+   * useScheduledWhenNonRunning is true, empty otherwise. Logically this is confusing,
+   * but existing UIs depend on this.


Suggested change

* @return if the trip doesn't run on the date specified, return schedule if

* useScheduledWhenNonRunning is true, empty otherwise. Logically this is confusing,

* but existing UIs depend on this.

* @return if the trip doesn't run on the specified service date, return trip times from

* the next scheduled service date if fallbackToNextScheduledDate is true, empty

* otherwise. Logically this is confusing, but existing API users depend on this.

I think the description above is not accurate as well. There is a possibility that the next scheduled service date has been modified by the real-time updates. For example, today is 2025-03-06 when the trip does not run, and the service on 2025-03-07 has been replaced. This method won't return the replaced service, but instead the original scheduled times without real-time updates.

Yep, I think we could add a sentence describing that real-time data is only used for the current date.

application/src/main/resources/org/opentripplanner/apis/gtfs/schema.graphqls

application/src/test/java/org/opentripplanner/transit/service/DefaultTransitServiceTest.java

…TransitService.java Co-authored-by: Joel Lappalainen <[email protected]>

…chema.graphqls Co-authored-by: Joel Lappalainen <[email protected]>

…DefaultTransitServiceTest.java Co-authored-by: Joel Lappalainen <[email protected]>

miklcct · 2025-03-06T14:11:11Z

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

+   * @return if the trip doesn't run on the date specified, return schedule if
+   * useScheduledWhenNonRunning is true, empty otherwise. Logically this is confusing,
+   * but existing UIs depend on this.


I think the description above is not accurate as well. There is a possibility that the next scheduled service date has been modified by the real-time updates. For example, today is 2025-03-06 when the trip does not run, and the service on 2025-03-07 has been replaced. This method won't return the replaced service, but instead the original scheduled times without real-time updates.

miklcct · 2025-03-06T14:23:19Z

application/src/test/java/org/opentripplanner/transit/service/DefaultTransitServiceTest.java

+          new TripTimeOnDate(REALTIME_TRIP_TIMES, 1, REAL_TIME_PATTERN, SERVICE_DATE, midnight)
+        )
+      ),
+      service.getTripTimeOnDates(TRIP, SERVICE_DATE, true)


I think you are testing for a wrong date. SERVICE_DATE is a date the service does run, and NO_SERVICE_DATE is a date the service does not run.

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

application/src/main/resources/org/opentripplanner/apis/gtfs/schema.graphqls

miklcct · 2025-03-07T11:33:33Z

My only concern left is the name of the boolean parameter. I have merged this PR into our test deployment which will be deployed over the weekend, and I will check if it works early next week (especially regarding to added trips).

…TransitService.java Co-authored-by: Michael Tsang <[email protected]>

…chema.graphqls Co-authored-by: Michael Tsang <[email protected]>

optionsome · 2025-03-07T16:13:16Z

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

+  public Optional<List<TripTimeOnDate>> getTripTimeOnDates(
+    Trip trip,
+    LocalDate serviceDate,
+    boolean fallbackToPlannedTimetableOnNonRunningDate


The latest suggestion for the name is fallbackToPlannedTimetableOnNoServiceDate.

optionsome · 2025-03-07T16:14:07Z

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

+  Optional<List<TripTimeOnDate>> getTripTimeOnDates(
+    Trip trip,
+    LocalDate serviceDate,
+    boolean fallbackToPlannedTimetableOnNonRunningDate


fallbackToPlannedTimetableOnNoServiceDate

optionsome · 2025-03-07T16:15:07Z

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

+  /**
+   * @return if the trip doesn't run on the specified service date, return scheduled trip times,
+   * unmodified by any realtime data, if
+   * fallbackToNextScheduledDate is true, empty otherwise. Logically this is confusing, but
+   * existing API users depend on this.
+   */


Suggested change

/**

* @return if the trip doesn't run on the specified service date, return scheduled trip times,

* unmodified by any realtime data, if

* fallbackToNextScheduledDate is true, empty otherwise. Logically this is confusing, but

* existing API users depend on this.

*/

/**

* @return if the trip doesn't run on the specified service date, return scheduled trip times,

* unmodified by any realtime data, if fallbackToPlannedTimetableOnNoServiceDate is true,

* empty otherwise. Logically this is confusing, but existing API users depend on this.

*/

application/src/test/java/org/opentripplanner/transit/service/DefaultTransitServiceTest.java

optionsome · 2025-03-10T10:36:58Z

@miklcct in #6245 you did some changes to trip's arrivalStoptime, departureStoptime, stoptimes and stoptimesForDate. Were there any functional changes to arrivalStoptime, departureStoptime or stoptimes? Also, was it required to change the behaviour of stoptimesForDate (when you provide the date as a parameter) to get some feature of yours to work or did you just want to fix the bug of it returning times for other days?

I have thought a bit more about how people might have used the stoptimesForDate even with the serviceDate parameter and unfortunately I think we might need to revert changes to it since I think some software might rely on the old "broken" logic. I think we should deprecate stoptimesForDate field on trip and replace it with one or two new fields that do this type of validation that you wanted to add and perhaps are a better fit for some use cases where people want to fetch real-time stop times for multiple days.

miklcct · 2025-03-10T11:08:57Z

@miklcct in #6245 you did some changes to trip's arrivalStoptime, departureStoptime, stoptimes and stoptimesForDate. Were there any functional changes to arrivalStoptime, departureStoptime or stoptimes? Also, was it required to change the behaviour of stoptimesForDate (when you provide the date as a parameter) to get some feature of yours to work or did you just want to fix the bug of it returning times for other days?

I have thought a bit more about how people might have used the stoptimesForDate even with the serviceDate parameter and unfortunately I think we might need to revert changes to it since I think some software might rely on the old "broken" logic. I think we should deprecate stoptimesForDate field on trip and replace it with one or two new fields that do this type of validation that you wanted to add and perhaps are a better fit for some use cases where people want to fetch real-time stop times for multiple days.

I am using stoptimesForDate field to fetch real-time stop times for yesterday, today and tomorrow as a workaround for the inability to query the times for the whole trip from a stoptimes, so I have to match the service dates and pick the correct one, and I plan to use this same field in another page where the user can query the timetable for the same trip on an arbitrary date as well.

For arrivalStopTime and departureStopTime, if the date is not specified, the scheduled time is used so there are no functional changes for these two fields, and stoptimes do not accept a date parameter so no functional change is done on that, and it always return the scheduled times.

In the long run I plan to deprecate the whole class of Stoptime and replace them with undated and dated classes.

Btw, what is the issue caused on Digitransit by my change? Can you please explain how Digitransit uses the information from stoptimesForDate without a parameter, on a date when the service does not run?

miklcct · 2025-03-10T11:24:47Z

Btw I think that there is also a possible bug in the digitransit-ui after I do a Github search on the use of stoptimesForDate without a date parameter that was the very reason I had to make a workaround to get the stop times in my journey plans.

This bug should happen if you get an itinerary which involves taking service across two service days (such as an overnight trip), or plan a future itinerary, in this case you will be fetching the stoptimes for today even though that the itinerary is for tomorrow, so at worst the complete wrong times will be shown and mislead the user, in case the trip today is heavily delayed (where the user will think that the trip tomorrow is delayed) or even replaced completely with a REPLACEMENT trip.

https://github.com/HSLdevcom/digitransit-ui/blob/917ccb4917a6c320f1aedcc920a45efd087f154d/app/component/itinerary/ItineraryDetails.js#L667

https://github.com/HSLdevcom/digitransit-ui/blob/917ccb4917a6c320f1aedcc920a45efd087f154d/app/component/itinerary/PlanConnection.js#L146

May I kindly suggest that everyone affected by this change check your intention of your GraphQL queries that you are showing the stop times of the correct date to the user, thanks, or if you ALWAYS intend to show the scheduled times (not even today's delays) if stoptimesForDate is called with no parameters.

optionsome · 2025-03-10T12:30:39Z

what is the issue caused on Digitransit by my change? Can you please explain how Digitransit uses the information from stoptimesForDate without a parameter, on a date when the service does not run?

The first problem we spotted was when you did a itinerary search for the future and opened some itinerary suggestion because it was using stoptimesForDate without serviceDate parameter.

May I kindly suggest that everyone affected by this change check your intention of your GraphQL queries that you are showing the stop times of the correct date to the user, thanks, or if you ALWAYS intend to show the scheduled times (not even today's delays) if stoptimesForDate is called with no parameters.

Yep, we plan to go through our usage of the stoptimesForDate field to see if it is used properly. However, the problem is that there hundreds of API users and it's difficult to have them all change their software to match the new implementation. Therefore, I think we will look into added new fields to the trip to replace the old stoptimes fields. Perhaps we can discuss this a bit tomorrow either in the normal dev meeting or in the design meeting?

miklcct · 2025-03-10T14:46:27Z

what is the issue caused on Digitransit by my change? Can you please explain how Digitransit uses the information from stoptimesForDate without a parameter, on a date when the service does not run?

The first problem we spotted was when you did a itinerary search for the future and opened some itinerary suggestion because it was using stoptimesForDate without serviceDate parameter.

In such case, it is highly likely that you are showing the wrong delays if the same trip on today is delayed. Try to plan a journey for the future on a route which has a severe delay today.

May I kindly suggest that everyone affected by this change check your intention of your GraphQL queries that you are showing the stop times of the correct date to the user, thanks, or if you ALWAYS intend to show the scheduled times (not even today's delays) if stoptimesForDate is called with no parameters.

Yep, we plan to go through our usage of the stoptimesForDate field to see if it is used properly. However, the problem is that there hundreds of API users and it's difficult to have them all change their software to match the new implementation. Therefore, I think we will look into added new fields to the trip to replace the old stoptimes fields. Perhaps we can discuss this a bit tomorrow either in the normal dev meeting or in the design meeting?

I can discuss this in a design meeting as I have intention to deprecate serviceDay field in Stoptime type, or even all date-dependent fields in Trip type in the future (and use TripOnServiceDate) for such purpose.

miklcct · 2025-03-13T14:52:22Z

After the developer meeting, we have discussed that we will revert to the old behavior (return the planned schedule regardless if the service actually run) since we are not supposed to introduce breaking changes into the API, and leave a documentation for this strange behavior and a workaround for the intended behavior using Trip.activeDates.

In the long term, we will deprecate this field, and all date-dependent fields in Trip, and move all such similar usages to TripOnServiceDate instead.

P.S. unfortunately the change has already been released in the 2.7 version into the wild, possibly we need to gather feedback from API users as well if they actually depend on the old behavior, or if this change has uncovered long-standing bugs when planning journeys tomorrow.

miklcct · 2025-03-13T17:16:13Z

Also, please don't remove my test. Change it to test the behavior that we have agreed and document it.

leonardehrenfried · 2025-03-20T05:53:20Z

I'm assigned the reviewer for this PR. Is there anything for me to do or will it be dropped?

miklcct · 2025-03-20T09:20:59Z

Let's discuss if we want to revert to the pre-2.7 non-intuitive behaviour, or if my change actually resulted in hard-to-find bugs discovered in the consumers where they should fix instead.

Unfortunately the damage has been done and people will rightly complain in either case.

tkalvas · 2025-03-20T13:19:41Z

Discarded, we will instead just revert the offending PR.

return stoptimesForDate even if the trip does not run on the date it …

d3b2721

…is queried

tkalvas requested a review from a team as a code owner February 21, 2025 13:15

tkalvas marked this pull request as draft February 21, 2025 13:15

tkalvas mentioned this pull request Feb 21, 2025

trip.stoptimesForDate in the GTFS GraphQL API doesn't work without serviceDate for trips that don't happen today #6475

Closed

optionsome added Regression This feature is no longer working. Digitransit Test Feature is under testing in Digitransit environment(s) labels Feb 27, 2025

make stoptimesForDate without parameters return the scheduled stoptim…

07478d6

…es even if the trip does not run today. Technically wrong, but UIs rely on this, and have for a long time

tkalvas marked this pull request as ready for review March 4, 2025 09:00

tkalvas marked this pull request as draft March 4, 2025 09:35

tkalvas added 2 commits March 4, 2025 12:38

handle separate behaviors on no parameter to stoptimesForDate and an …

900fde1

…explicit parameter of today

reinstate removed test getTripTimesOnNoServireDay and add a test for …

1bdabf6

…the corresponding case with useScheduledWhenNonRunning

tkalvas marked this pull request as ready for review March 4, 2025 10:45

tkalvas changed the title ~~return stoptimesForDate even if the trip does not run on the date it is queried~~ return stoptimesForDate from the schedule if it is called with no parameter on a day when the trip does not run Mar 4, 2025

tkalvas added 2 commits March 4, 2025 16:10

Merge remote-tracking branch 'upstream/dev-2.x' into issue-6475-quick…

f63ce10

…-fix

fix merge conflicts

1a1926d

optionsome self-requested a review March 6, 2025 10:52

describe funny stoptimesForDate behavior in graphql documentation

6a28d6a

optionsome requested changes Mar 6, 2025

View reviewed changes

tkalvas and others added 4 commits March 6, 2025 15:21

Update application/src/main/java/org/opentripplanner/transit/service/…

eba69af

…TransitService.java Co-authored-by: Joel Lappalainen <[email protected]>

Update application/src/main/resources/org/opentripplanner/apis/gtfs/s…

4365f47

…chema.graphqls Co-authored-by: Joel Lappalainen <[email protected]>

Update application/src/test/java/org/opentripplanner/transit/service/…

11a531a

…DefaultTransitServiceTest.java Co-authored-by: Joel Lappalainen <[email protected]>

rename useScheduledWhenNonRunning to fallbackToNextScheduledDate

362627a

tkalvas requested a review from optionsome March 6, 2025 13:26

optionsome requested a review from leonardehrenfried March 6, 2025 14:06

miklcct reviewed Mar 6, 2025

View reviewed changes

tkalvas added 2 commits March 7, 2025 11:53

enhance description of TransitService.getTripTimeOnDates, fix test

9cb7647

change fallback boolean parameter name

6087e25

tkalvas requested a review from miklcct March 7, 2025 09:55

change fallback boolean parameter name again

cd3b8d8

miklcct reviewed Mar 7, 2025

View reviewed changes

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

application/src/main/resources/org/opentripplanner/apis/gtfs/schema.graphqls Outdated Show resolved Hide resolved

tkalvas and others added 3 commits March 7, 2025 14:07

Update application/src/main/java/org/opentripplanner/transit/service/…

e63988a

…TransitService.java Co-authored-by: Michael Tsang <[email protected]>

Update application/src/main/resources/org/opentripplanner/apis/gtfs/s…

6cafd9e

…chema.graphqls Co-authored-by: Michael Tsang <[email protected]>

change fallback boolean parameter name again, again

26abec4

tkalvas requested a review from miklcct March 7, 2025 12:08

optionsome reviewed Mar 7, 2025

View reviewed changes

use naming fallbackToPlannedTimetableOnNoServiceDate

fd6c9bb

tkalvas requested a review from optionsome March 10, 2025 07:38

t2gran added this to the 2.8 (next release) milestone Mar 12, 2025

tkalvas closed this Mar 20, 2025

return stoptimesForDate from the schedule if it is called with no parameter on a day when the trip does not run #6480

return stoptimesForDate from the schedule if it is called with no parameter on a day when the trip does not run #6480

Uh oh!

Conversation

tkalvas commented Feb 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Feb 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miklcct Mar 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

miklcct commented Mar 7, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

optionsome commented Mar 10, 2025

Uh oh!

miklcct commented Mar 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miklcct commented Mar 10, 2025

Uh oh!

optionsome commented Mar 10, 2025

Uh oh!

miklcct commented Mar 10, 2025

Uh oh!

miklcct commented Mar 13, 2025

Uh oh!

miklcct commented Mar 13, 2025

Uh oh!

leonardehrenfried commented Mar 20, 2025

Uh oh!

miklcct commented Mar 20, 2025

Uh oh!

tkalvas commented Mar 20, 2025

Uh oh!

Uh oh!

tkalvas commented Feb 21, 2025 •

edited

Loading

codecov bot commented Feb 21, 2025 •

edited

Loading

miklcct Mar 7, 2025 •

edited

Loading

miklcct commented Mar 10, 2025 •

edited

Loading