Improve handling of realtime updated StopPatterns #6909

flaktack · 2025-09-26T20:27:04Z

Summary

Two somewhat related issues are improved relating to realtime stop pattern modifications:

a new StopPattern and TripPattern was created if any of headsign, stop id, pickup or alight type were set, even if the value remained the same.
A check is added that the values are actually modified.
old TripPatternForDate values could be used if a trip's stop pattern changed, which could lead to transit planning using an old/invalid version.
When applying updates RealTimeRaptorTransitDataUpdater removes TripPatternForDates without a corresponding Timetable.

Issue

Unit tests

New tests are added for the improvements.

Documentation

No changes.

This avoids planning trips with old trip patterns leading to invalid itineraries.

codecov · 2025-09-26T20:33:58Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.29%. Comparing base (6d407ec) to head (cfa755c).
⚠️ Report is 17 commits behind head on dev-2.x.

Additional details and impacted files

@@              Coverage Diff              @@
##             dev-2.x    #6909      +/-   ##
=============================================
+ Coverage      72.22%   72.29%   +0.07%     
- Complexity     20148    20198      +50     
=============================================
  Files           2186     2188       +2     
  Lines          81152    81203      +51     
  Branches        8139     8141       +2     
=============================================
+ Hits           58609    58703      +94     
+ Misses         19666    19636      -30     
+ Partials        2877     2864      -13

☔ 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.

flaktack · 2025-09-27T15:18:03Z

This seems to be related to #6197 and #6280

miklcct · 2025-09-29T16:04:58Z

This seems to be related to #6197 and #6280

I will check if it fixes #6197 and will report the finding afterwards.

leonardehrenfried · 2025-10-14T11:58:35Z

...c/test/java/org/opentripplanner/updater/trip/gtfs/moduletests/delay/AssignedStopIdsTest.java

+        .getTransitService()
+        .getRealtimeRaptorTransitData()
+        .getTripPatternsForRunningDate(SERVICE_DATE)
+        .stream()
+        .map(TripPatternForDate::getTripPattern)
+        .map(RoutingTripPattern::getPattern)
+        .map(AbstractTransitEntity::getId)
+        .map(Object::toString)
+        .toList()


This is repeated several times. Can you extract a reusable method, perhaps in RealtimeTestEnv?

It will probably heavily collide with #6899 but worth it nonetheless.

leonardehrenfried · 2025-10-14T11:59:43Z

...c/test/java/org/opentripplanner/updater/trip/gtfs/moduletests/delay/AssignedStopIdsTest.java

+/**
+ * Tests that stops can be SKIPPED for a trip which repeats times for consecutive stops.
+ *
+ * @link <a href="https://github.com/opentripplanner/OpenTripPlanner/issues/6848">issue</a>
+ */


Please update or remove.

leonardehrenfried · 2025-10-14T12:01:34Z

application/src/test/java/org/opentripplanner/updater/trip/gtfs/TripTimesUpdaterTest.java

+    GtfsRealtime.TripUpdate.Builder tripUpdateBuilder = GtfsRealtime.TripUpdate.newBuilder();
+    tripUpdateBuilder.setTrip(tripDescriptorBuilder);
+    tripUpdateBuilder.getTripPropertiesBuilder().setTripHeadsign("foo");
+    StopTimeUpdate.Builder stopTimeUpdateBuilder = tripUpdateBuilder.addStopTimeUpdateBuilder(0);
+    stopTimeUpdateBuilder.setStopSequence(1);
+    stopTimeUpdateBuilder.getArrivalBuilder().setDelay(0);
+    stopTimeUpdateBuilder.getDepartureBuilder().setDelay(0);
+    stopTimeUpdateBuilder.getStopTimePropertiesBuilder().setStopHeadsign("foo");
+    stopTimeUpdateBuilder.getStopTimePropertiesBuilder().setAssignedStopId("A");
+    stopTimeUpdateBuilder = tripUpdateBuilder.addStopTimeUpdateBuilder(1);
+    stopTimeUpdateBuilder.setStopSequence(2);
+    stopTimeUpdateBuilder
+      .getStopTimePropertiesBuilder()
+      .setPickupType(StopTimeUpdate.StopTimeProperties.DropOffPickupType.REGULAR);
+    stopTimeUpdateBuilder
+      .getStopTimePropertiesBuilder()
+      .setDropOffType(StopTimeUpdate.StopTimeProperties.DropOffPickupType.REGULAR);
+    stopTimeUpdateBuilder.setScheduleRelationship(StopTimeUpdate.ScheduleRelationship.SCHEDULED);
+    stopTimeUpdateBuilder = tripUpdateBuilder.addStopTimeUpdateBuilder(2);
+    stopTimeUpdateBuilder.setStopSequence(3);
+    stopTimeUpdateBuilder.getArrivalBuilder().setDelay(0);
+    stopTimeUpdateBuilder.getDepartureBuilder().setDelay(0);


I know that the other tests in this class do the same, but I find the repetition pretty grating.

Can you please make this a little less verbose?

leonardehrenfried · 2025-10-14T12:02:22Z

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

+    this.timetableRepository.setRaptorTransitData(
+        RaptorTransitDataMapper.map(new TestTransitTuningParameters(), timetableRepository)
+      );
+    this.timetableRepository.setRealtimeRaptorTransitData(
+        new RaptorTransitData(timetableRepository.getRaptorTransitData())
+      );
    this.snapshotManager = new TimetableSnapshotManager(
-      null,
+      new RealTimeRaptorTransitDataUpdater(timetableRepository),


I think it's great that we are adding test coverage to this critical, yet poorly-understood component.

leonardehrenfried · 2025-10-14T12:15:21Z

...lanner/routing/algorithm/raptoradapter/transit/mappers/RealTimeRaptorTransitDataUpdater.java

          } else {
-            LOG.warn("Could not fetch timetable for {}", pattern);
+            LOG.warn("Could not fetch timetable for {}, removing.", pattern);
+            patternsForDate.remove(tripPatternForDate);


This is the most important change in the PR. I absolutely support changing this class but I'm having a hard time understanding the consequences of this change: the code is very complex with several collections updating each other.

If you have a better understanding, do you have time to walk me through the code? Maybe @vpaturet is interested as well.

leonardehrenfried · 2025-10-16T10:28:52Z

application/src/test/java/org/opentripplanner/updater/trip/gtfs/TripTimesUpdaterTest.java

  public void tripNotFoundInPattern() {
    // non-existing trip
-    var tripDescriptorBuilder = tripDescriptorBuilder("b");
+    var nonExistingTripId = "b";


Wow, thanks for this. I didn't expect you to fix the whole file.

leonardehrenfried

I think this already in good shape. @vpaturet what do you think of a group review session about RealTimeRaptorTransitDataUpdater?

leonardehrenfried · 2025-10-16T10:39:43Z

...lanner/routing/algorithm/raptoradapter/transit/mappers/RealTimeRaptorTransitDataUpdater.java

    this.timetableRepository = timetableRepository;
  }

  public void update(


If you have the knowledge to do so, can you add some Javadoc to this method?

habrahamsson-skanetrafiken · 2025-10-16T13:58:41Z

Lets have a group session to look at this after @leonardehrenfried is back from vacation.

…-gtfs-rt-patterns

leonardehrenfried · 2025-11-05T10:22:36Z

...lanner/routing/algorithm/raptoradapter/transit/mappers/RealTimeRaptorTransitDataUpdater.java

    this.timetableRepository = timetableRepository;
  }

+  /// Updates the real-time [RaptorTransitData] to use the modified timetables.


Can you make this proper Javadoc, please?

Or is this now valid Markdown-ish Javadoc?

Can you use the correct Javadoc syntax here please?

We discussed this while you were gone. Apparently IntelliJ IDEA already supports this new markdown format and then no one was really against using it. However, we didn't discuss it too much and not sure what is the way forward with it.

No, it's fine. i was unaware that it will be officially supported. Intellij is the only place I would read Javadoc so I don't care if other tools don't.

Yes, it is Markdown-ish javadoc

leonardehrenfried · 2025-11-05T10:24:40Z

...c/test/java/org/opentripplanner/updater/trip/gtfs/moduletests/delay/AssignedStopIdsTest.java

+    var rt = GtfsRtTestHelper.of(env);
+    var tripUpdate1 = new TripUpdateBuilder(TRIP_1_ID, SERVICE_DATE, SCHEDULED, TIME_ZONE)


Suggested change

var rt = GtfsRtTestHelper.of(env);

var tripUpdate1 = new TripUpdateBuilder(TRIP_1_ID, SERVICE_DATE, SCHEDULED, TIME_ZONE)

var rt = GtfsRtTestHelper.of(env);

var tripUpdate1 = rt.tripUpdateScheduled(TRIP_1_ID)

Please also change the other places.

leonardehrenfried

There are a few smaller things that I would like to see changed, but we're close.

miklcct · 2025-11-12T17:02:07Z

I have just confirmed that this patch does NOT fix #6197 . The problem still exists using your branch.

…-gtfs-rt-patterns

leonardehrenfried

Please fix the Javadoc, otherwise I'm good to approve.

There is a remaining risk but it's managable, IMO.

flaktack added 2 commits September 26, 2025 22:01

Only create modified trip patterns if the data changes

aeaa862

Remove unused realtime trip patterns

0208cbf

This avoids planning trips with old trip patterns leading to invalid itineraries.

flaktack marked this pull request as ready for review October 14, 2025 07:41

flaktack requested a review from a team as a code owner October 14, 2025 07:41

optionsome requested review from leonardehrenfried, t2gran and vpaturet October 14, 2025 08:48

leonardehrenfried reviewed Oct 14, 2025

View reviewed changes

flaktack added 2 commits October 15, 2025 14:12

Reduce code duplication in AssignedStopIdsTest

a7ff17b

Use TripUpdateBuilder in TripTimesUpdaterTest

dcc9067

leonardehrenfried reviewed Oct 16, 2025

View reviewed changes

Merge remote-tracking branch 'upstream/dev-2.x' into feature/modified…

fa0f0b2

…-gtfs-rt-patterns

rejkmackan assigned t2gran and vpaturet Oct 30, 2025

optionsome assigned leonardehrenfried and unassigned t2gran Nov 4, 2025

optionsome requested a review from leonardehrenfried November 4, 2025 09:20

flaktack force-pushed the feature/modified-gtfs-rt-patterns branch from 8b54a10 to 5e81f97 Compare November 5, 2025 09:45

leonardehrenfried reviewed Nov 5, 2025

View reviewed changes

Add tests and documentation for RealTimeRaptorTransitDataUpdater

d7df30d

flaktack force-pushed the feature/modified-gtfs-rt-patterns branch from 5e81f97 to d7df30d Compare November 5, 2025 10:49

leonardehrenfried requested changes Nov 5, 2025

View reviewed changes

flaktack added 2 commits November 13, 2025 09:29

Merge remote-tracking branch 'upstream/dev-2.x' into feature/modified…

b022e57

…-gtfs-rt-patterns

AssignedStopIdsTest: use tripUpdateScheduled() for tests

cfa755c

leonardehrenfried requested changes Nov 14, 2025

View reviewed changes

leonardehrenfried approved these changes Nov 14, 2025

View reviewed changes

t2gran added this to the 2.9 (next release) milestone Nov 19, 2025

		var rt = GtfsRtTestHelper.of(env);
		var tripUpdate1 = new TripUpdateBuilder(TRIP_1_ID, SERVICE_DATE, SCHEDULED, TIME_ZONE)

Improve handling of realtime updated StopPatterns #6909

Are you sure you want to change the base?

Improve handling of realtime updated StopPatterns #6909

Conversation

flaktack commented Sep 26, 2025

Summary

Issue

Unit tests

Documentation

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

flaktack commented Sep 27, 2025

Uh oh!

miklcct commented Sep 29, 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!

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!

leonardehrenfried left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

habrahamsson-skanetrafiken commented Oct 16, 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!

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!

leonardehrenfried left a comment

Choose a reason for hiding this comment

Uh oh!

miklcct commented Nov 12, 2025

Uh oh!

leonardehrenfried left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

codecov bot commented Sep 26, 2025 •

edited

Loading