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.20%. Comparing base (899aebd) to head (dcc9067).
⚠️ Report is 274 commits behind head on dev-2.x.

Additional details and impacted files

@@              Coverage Diff              @@
##             dev-2.x    #6909      +/-   ##
=============================================
+ Coverage      72.09%   72.20%   +0.10%     
- Complexity     19663    19803     +140     
=============================================
  Files           2123     2151      +28     
  Lines          79432    79972     +540     
  Branches        8050     8058       +8     
=============================================
+ Hits           57270    57744     +474     
- Misses         19321    19392      +71     
+ Partials        2841     2836       -5

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

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

Improve handling of realtime updated StopPatterns #6909

Are you sure you want to change the base?

Improve handling of realtime updated StopPatterns #6909

Uh oh!

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!

leonardehrenfried Oct 14, 2025

Choose a reason for hiding this comment

Uh oh!

leonardehrenfried Oct 14, 2025

Choose a reason for hiding this comment

Uh oh!

leonardehrenfried Oct 14, 2025

Choose a reason for hiding this comment

Uh oh!

leonardehrenfried Oct 14, 2025

Choose a reason for hiding this comment

Uh oh!

leonardehrenfried Oct 14, 2025

Choose a reason for hiding this comment

Uh oh!

leonardehrenfried Oct 16, 2025

Choose a reason for hiding this comment

Uh oh!

leonardehrenfried left a comment

Choose a reason for hiding this comment

Uh oh!

leonardehrenfried Oct 16, 2025

Choose a reason for hiding this comment

Uh oh!

habrahamsson-skanetrafiken commented Oct 16, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Sep 26, 2025 •

edited

Loading