Implement SIRI extra calls #6493

vpaturet · 2025-02-26T12:35:10Z

Summary

This PR adds support for the SIRI feature "extra call".
An extra-call consists in adding unplanned stops to a scheduled trip.
It differs from the 2 other currently supported types of update:

"trip update" which is a limited modification of a scheduled trip (it only allows changes in the passing times, cancelation of a stop or replacement of a stop by another stop that belongs to the same station)
"replacement departure": creation of a completely new trip, not present in scheduled data.

Validation rules:

the update is expected to have the same number of non-extra calls as the number of stops in the original scheduled trip
the update cannot change the stop sequence on calls that are not labeled as "extra-call".
the update cannot refer to unknown stops.

As of today Entur does not receive any SIRI feed that contains extra calls, so this feature may be revisited later on when tested against real data.

There is a limited effort in this PR to de-duplicate logic shared between AddedTripBuilder and ExtraCallBuilder, the idea being that the code should be refactored anyway into a more generic model.

Implementation note: The two pre-existing methods on CallWrapper:

Boolean isCancellation();
Boolean isPredictionInaccurate()

return a wrapper object instead of a primitive boolean. This makes boolean comparison error-prone. This should be refactored in a follow-up PR using the same model as CallWrapper.isExtraCall() .

Issue

Closes #6188

Unit tests

Added unit tests

Documentation

No

codecov · 2025-02-26T12:47:12Z

Codecov Report

Attention: Patch coverage is 78.16092% with 38 lines in your changes missing coverage. Please review.

Project coverage is 70.98%. Comparing base (a50b595) to head (bb6cb63).
Report is 1 commits behind head on dev-2.x.

Files with missing lines	Patch %	Lines
...dater/trip/siri/SiriRealTimeTripUpdateAdapter.java	66.17%	17 Missing and 6 partials ⚠️
...lanner/updater/trip/siri/ExtraCallTripBuilder.java	83.58%	6 Missing and 5 partials ⚠️
...tripplanner/updater/trip/siri/StopTimesMapper.java	88.23%	2 Missing and 2 partials ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             dev-2.x    #6493    +/-   ##
===========================================
  Coverage      70.98%   70.98%            
- Complexity     18272    18294    +22     
===========================================
  Files           2000     2002     +2     
  Lines          75871    76004   +133     
  Branches        7785     7811    +26     
===========================================
+ Hits           53856    53953    +97     
- Misses         19268    19293    +25     
- Partials        2747     2758    +11

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

leonardehrenfried · 2025-04-01T10:46:46Z

...ation/src/main/java/org/opentripplanner/updater/trip/siri/SiriRealTimeTripUpdateAdapter.java

+      ((vehicleJourney.getRecordedCalls() != null &&
+          vehicleJourney
+            .getRecordedCalls()
+            .getRecordedCalls()
+            .stream()
+            .anyMatch(recordedCall -> Boolean.TRUE.equals(recordedCall.isExtraCall()))) ||
+        (vehicleJourney.getEstimatedCalls() != null &&
+          vehicleJourney
+            .getEstimatedCalls()
+            .getEstimatedCalls()
+            .stream()
+            .anyMatch(estimatedCall -> Boolean.TRUE.equals(estimatedCall.isExtraCall()))))
+    ) {


Probaby beyond the scope of the PR but what do you think of extracting these very wordy calls to vehicleJourney into a re-usable wrapper type?

There are a few other pieces of code that could be moved into it.

We already create a list of callwrappers on line 130 in this file so you could pass that into this method to make the check for extra calls cleaner.

leonardehrenfried · 2025-04-01T10:57:43Z

application/src/main/java/org/opentripplanner/updater/trip/siri/StopTimesMapper.java

+    var aimedArrivalTime = call.getAimedArrivalTime() != null
+      ? call.getAimedArrivalTime()
+      : call.getAimedDepartureTime();


Shouldn't there be a method in CallWrapper for this?

I see you only moved this piece of code. But it is a bit strange:

According to the spec the aimedArrivalTime should never be null except for possibly at the first stop. But a bit lower down on line 67 we ignore this value if isFirstStop is true. So in practice we will never use the departure time value except for if the message is violating the spec.

I think we should change this code to strictly reject messages that don't have an aimedArrivalTime (unless they are the first call). But maybe you don't want to do that change of behaviour in this PR?

leonardehrenfried · 2025-04-01T11:00:33Z

application/src/main/java/org/opentripplanner/updater/trip/siri/StopTimesMapper.java

+  }
+
+  //TODO move to helper
+  private static String getFirstNameFromList(List<NaturalLanguageStringStructure> names) {


Can you remove or resolve the TODO?

leonardehrenfried · 2025-04-01T11:45:33Z

...ation/src/main/java/org/opentripplanner/updater/trip/siri/SiriRealTimeTripUpdateAdapter.java

+  /**
+   * Types of SIRI update messages.
+   */
+  private enum SiriUpdateType {


What do you think of Henrik's idea of using an algebraic data type?

sealed interface SiriUpdate record TripUpdate( VehicleJourney vehicleJourney ... ) implements SiriUpdate record ReplacementDeparture implementes SiriUpdate

leonardehrenfried

Genereally speaking this works however, I wonder if we can move the ugly check methods into a wrapper type. This doesn't need to happen in this PR.

habrahamsson-skanetrafiken · 2025-04-09T14:40:50Z

application/src/main/java/org/opentripplanner/updater/trip/siri/ExtraCallTripBuilder.java

+    EstimatedVehicleJourney estimatedVehicleJourney,
+    TransitEditorService transitService,
+    EntityResolver entityResolver,
+    Function<Trip, FeedScopedId> getTripPatternId,


How about calling this tripPatternIdGenerator or generateTripPatternId instead? Otherwise it looks like we are getting an existing id.

habrahamsson-skanetrafiken · 2025-04-09T14:51:30Z

...ation/src/main/java/org/opentripplanner/updater/trip/siri/SiriRealTimeTripUpdateAdapter.java

+      ((vehicleJourney.getRecordedCalls() != null &&
+          vehicleJourney
+            .getRecordedCalls()
+            .getRecordedCalls()
+            .stream()
+            .anyMatch(recordedCall -> Boolean.TRUE.equals(recordedCall.isExtraCall()))) ||
+        (vehicleJourney.getEstimatedCalls() != null &&
+          vehicleJourney
+            .getEstimatedCalls()
+            .getEstimatedCalls()
+            .stream()
+            .anyMatch(estimatedCall -> Boolean.TRUE.equals(estimatedCall.isExtraCall()))))
+    ) {


We already create a list of callwrappers on line 130 in this file so you could pass that into this method to make the check for extra calls cleaner.

habrahamsson-skanetrafiken · 2025-04-09T15:06:28Z

application/src/main/java/org/opentripplanner/updater/trip/siri/StopTimesMapper.java

+  /**
+   * Map the call to the aimed StopTime or return null if the stop cannot be found in the site repository.
+   */
+  StopTime createAimedStopTime(


This should be @Nullable

habrahamsson-skanetrafiken · 2025-04-09T15:15:29Z

application/src/main/java/org/opentripplanner/updater/trip/siri/StopTimesMapper.java

+    var aimedArrivalTime = call.getAimedArrivalTime() != null
+      ? call.getAimedArrivalTime()
+      : call.getAimedDepartureTime();


I see you only moved this piece of code. But it is a bit strange:

According to the spec the aimedArrivalTime should never be null except for possibly at the first stop. But a bit lower down on line 67 we ignore this value if isFirstStop is true. So in practice we will never use the departure time value except for if the message is violating the spec.

I think we should change this code to strictly reject messages that don't have an aimedArrivalTime (unless they are the first call). But maybe you don't want to do that change of behaviour in this PR?

vpaturet force-pushed the implement_extra_calls branch from 868510b to fd300a7 Compare February 26, 2025 12:41

vpaturet force-pushed the implement_extra_calls branch from fd300a7 to 1177d2c Compare February 26, 2025 15:02

vpaturet force-pushed the implement_extra_calls branch from 1177d2c to 9c5928e Compare March 7, 2025 10:28

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

vpaturet force-pushed the implement_extra_calls branch 3 times, most recently from 10ed14c to 70c89ea Compare March 24, 2025 13:25

vpaturet added 3 commits March 31, 2025 12:19

Implement SIRI extra calls

f1999cb

Clarify method name

f54594a

Align error code for unknown stop

70b3593

vpaturet force-pushed the implement_extra_calls branch from 70c89ea to 70b3593 Compare March 31, 2025 10:20

Add validation rules

c32be07

vpaturet force-pushed the implement_extra_calls branch from 2817d38 to c32be07 Compare March 31, 2025 12:55

vpaturet marked this pull request as ready for review March 31, 2025 13:50

vpaturet requested a review from a team as a code owner March 31, 2025 13:50

vpaturet requested a review from habrahamsson-skanetrafiken March 31, 2025 13:50

vpaturet added the Entur Test This is currently being tested at Entur label Mar 31, 2025

optionsome requested a review from leonardehrenfried April 1, 2025 08:28

leonardehrenfried reviewed Apr 1, 2025

View reviewed changes

vpaturet removed the Entur Test This is currently being tested at Entur label Apr 2, 2025

Merge branch 'dev-2.x' into implement_extra_calls

b3906b6

vpaturet added the Entur Test This is currently being tested at Entur label Apr 8, 2025

Fix merge conflicts

bb6cb63

habrahamsson-skanetrafiken requested changes Apr 9, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement SIRI extra calls #6493

Implement SIRI extra calls #6493

vpaturet commented Feb 26, 2025 •

edited

Loading

codecov bot commented Feb 26, 2025 •

edited

Loading

leonardehrenfried Apr 1, 2025

leonardehrenfried Apr 1, 2025 •

edited

Loading

habrahamsson-skanetrafiken Apr 9, 2025

leonardehrenfried Apr 1, 2025

habrahamsson-skanetrafiken Apr 9, 2025

leonardehrenfried Apr 1, 2025

leonardehrenfried Apr 1, 2025

leonardehrenfried left a comment •

edited

Loading

habrahamsson-skanetrafiken Apr 9, 2025

habrahamsson-skanetrafiken Apr 9, 2025

habrahamsson-skanetrafiken Apr 9, 2025

habrahamsson-skanetrafiken Apr 9, 2025

Implement SIRI extra calls #6493

Are you sure you want to change the base?

Implement SIRI extra calls #6493

Conversation

vpaturet commented Feb 26, 2025 • edited Loading

Summary

Issue

Unit tests

Documentation

codecov bot commented Feb 26, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

leonardehrenfried Apr 1, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leonardehrenfried left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vpaturet commented Feb 26, 2025 •

edited

Loading

codecov bot commented Feb 26, 2025 •

edited

Loading

leonardehrenfried Apr 1, 2025 •

edited

Loading

leonardehrenfried left a comment •

edited

Loading