Reject SIRI messages with invalid stops and missing times by vpaturet · Pull Request #6499 · opentripplanner/OpenTripPlanner

vpaturet · 2025-02-27T17:59:28Z

Summary

This PR applies strictly the validation rules specified in the Nordic SIRI profile.
In particular:

A SIRI-ET message must contain recorded or estimated times for all stops.
A SIRI-ET message cannot refer to an unknown stop.
A SIRI-ET message that updates an existing trip cannot replace a stop by another stop unless they belong to the same station.

This PR ensures that messages that fail these validation rules are rejected.
As a consequence, the time interpolation logic is not used anymore and can be removed .

The rules are relaxed for full trip cancellation: a message is accepted even if it contains incomplete times or invalid stops.

Issue

No

Unit tests

Updated unit tests.

Documentation

No

codecov · 2025-02-27T18:06:29Z

Codecov Report

Attention: Patch coverage is 81.39535% with 8 lines in your changes missing coverage. Please review.

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

Files with missing lines	Patch %	Lines
...planner/updater/trip/siri/ModifiedTripBuilder.java	79.48%	6 Missing and 2 partials ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##             dev-2.x    #6499      +/-   ##
=============================================
- Coverage      70.20%   70.18%   -0.02%     
+ Complexity     18313    18309       -4     
=============================================
  Files           2080     2082       +2     
  Lines          77182    77201      +19     
  Branches        7831     7832       +1     
=============================================
- Hits           54183    54182       -1     
- Misses         20230    20250      +20     
  Partials        2769     2769

☔ 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-02-28T09:37:25Z

+import org.opentripplanner.updater.trip.TripInput;
+import org.opentripplanner.updater.trip.siri.SiriEtBuilder;
+
+class IncompleteMessageTest implements RealtimeTestConstants {


Back when I started adding these module tests @t2gran asked me to put these into packages to group them by feature. If you like this pattern, can you put this one into the package rejection?

vpaturet · 2025-02-28T09:42:35Z

+        result.failureValue().errorType(),
+        invalidStopIndex
+      );
+      return result.toFailureResult();


create a new result with trip id and datasource

leonardehrenfried · 2025-03-04T09:54:39Z

  }

-  @Test
-  void testUpdateCascading() {


These tests just test something that we have now removed, right?

Yes, testUpdateCascading assumes that we accept a message with fewer calls than in the scheduled trip. We now refuse these messages except for full cancellation, and full cancellation is tested elsewhere.
testCreateStopPatternNoCalls and testCreateStopPatternSingleCall call ModifiedTripBuilder.createStopPattern with preconditions that are now impossible.

habrahamsson-skanetrafiken · 2025-03-04T11:13:59Z

        break;
      }
+      if (!matchFound) {
+        return Result.failure(new UpdateError(null, STOP_MISMATCH, i));


We will fail with this error if there is a missing call as well. I guess that's not a big probeblem but it seems like an inaccurate error code for that case.

I have added validation rules for too few and too many calls.

habrahamsson-skanetrafiken · 2025-03-04T11:38:10Z

      StopLocation stop = builder.stops.original(i);

+      boolean matchFound = false;
      for (CallWrapper call : calls) {


The logic in this method will still accept if there are too many calls in the ET update. Maybe fixing that is part of your other extra-call-PR?

I have added validation rules for too few and too many calls.

leonardehrenfried · 2025-03-04T11:51:12Z

    assertTrue(addedTrip.isFailure(), "Trip creation should fail");
    assertEquals(
-      UpdateError.UpdateErrorType.NO_VALID_STOPS,
+      UpdateError.UpdateErrorType.UNKNOWN_STOP,


There is a test for UNKNOWN_STOP here but perhaps you could add one for regular, non-extra-journey updates.

I have added unit tests for unknown stop and stop mismatch

# Conflicts: # application/src/test/java/org/opentripplanner/updater/trip/siri/ModifiedTripBuilderTest.java # application/src/test/java/org/opentripplanner/updater/trip/siri/moduletests/InterpolationTest.java

leonardehrenfried · 2025-03-05T13:46:18Z

+   * Full cancellation of a trip.
+   */
+  private Result<TripUpdate, UpdateError> cancelTrip(RealTimeTripTimes newTimes) {
+    LOG.debug("Trip is cancelled");


This is a pretty useless log message, isn't it? I think you should either add the trip id or remove it.

Pretty useless indeed. I've removed it.

habrahamsson-skanetrafiken · 2025-03-06T08:39:41Z

@@ -242,16 +253,22 @@ static StopPattern createStopPattern(
    for (int i = 0; i < numberOfStops; i++) {


Can you add a line of documentation in both createStopPattern() and applyUpdates() that the number of calls are guaranteed to be the same as the numberOfStops. I think that will make it easier to if we decide to refactor this code in the future.

vpaturet added the +Real-Time The issue/PR is related to RealTime updates label Feb 27, 2025

Reject SIRI messages with invalid stops and missing times

02cf113

vpaturet force-pushed the reject_siri_messages_with_invalid_stops branch from ab30c6a to 02cf113 Compare February 28, 2025 09:29

leonardehrenfried reviewed Feb 28, 2025

View reviewed changes

vpaturet commented Feb 28, 2025

View reviewed changes

vpaturet added 3 commits March 3, 2025 16:30

Move unit test

24adf92

Return detailed failure for invalid stop pattern

f7abc81

Accept full trip cancellation even if times are incomplete

cc97c9d

vpaturet marked this pull request as ready for review March 3, 2025 17:32

vpaturet requested a review from a team as a code owner March 3, 2025 17:32

vpaturet requested a review from habrahamsson-skanetrafiken March 3, 2025 17:34

leonardehrenfried reviewed Mar 4, 2025

View reviewed changes

habrahamsson-skanetrafiken requested changes Mar 4, 2025

View reviewed changes

leonardehrenfried reviewed Mar 4, 2025

View reviewed changes

vpaturet added 3 commits March 4, 2025 15:56

Merge branch 'dev-2.x' into reject_siri_messages_with_invalid_stops

c1a93f4

# Conflicts: # application/src/test/java/org/opentripplanner/updater/trip/siri/ModifiedTripBuilderTest.java # application/src/test/java/org/opentripplanner/updater/trip/siri/moduletests/InterpolationTest.java

Fix merge conflict

47847cf

Apply review suggestions

3a549a8

vpaturet requested review from habrahamsson-skanetrafiken and leonardehrenfried March 5, 2025 13:21

leonardehrenfried reviewed Mar 5, 2025

View reviewed changes

Remove log message

36ed324

vpaturet requested a review from leonardehrenfried March 5, 2025 14:02

leonardehrenfried previously approved these changes Mar 5, 2025

View reviewed changes

vpaturet added Entur Test This is currently being tested at Entur !Improvement A functional improvement or micro feature and removed Entur Test This is currently being tested at Entur labels Mar 5, 2025

habrahamsson-skanetrafiken requested changes Mar 6, 2025

View reviewed changes

Added documentation on precondition for updating times and pattern

20dbee1

vpaturet dismissed leonardehrenfried’s stale review via 20dbee1 March 6, 2025 12:37

vpaturet requested review from habrahamsson-skanetrafiken and leonardehrenfried March 6, 2025 12:38

leonardehrenfried approved these changes Mar 6, 2025

View reviewed changes

habrahamsson-skanetrafiken approved these changes Mar 6, 2025

View reviewed changes

vpaturet merged commit 6175642 into opentripplanner:dev-2.x Mar 6, 2025

t2gran pushed a commit that referenced this pull request Mar 6, 2025

Add changelog entry for #6499 [ci skip]

77f0326

vpaturet deleted the reject_siri_messages_with_invalid_stops branch March 7, 2025 09:29

t2gran added this to the 2.7 milestone Mar 12, 2025

		@@ -242,16 +253,22 @@ static StopPattern createStopPattern(
		for (int i = 0; i < numberOfStops; i++) {

Uh oh!

Conversation

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

Uh oh!

Summary

Issue

Unit tests

Documentation

Uh oh!

codecov Bot commented Feb 27, 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!

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!

Choose a reason for hiding this comment

Uh oh!

vpaturet Mar 5, 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

vpaturet commented Feb 27, 2025 •

edited

Loading

codecov Bot commented Feb 27, 2025 •

edited

Loading

vpaturet Mar 5, 2025 •

edited

Loading