Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reject SIRI messages with invalid stops and missing times #6499

Open
wants to merge 9 commits into
base: dev-2.x
Choose a base branch
from
Open

Reject SIRI messages with invalid stops and missing times #6499

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
02cf113
Reject SIRI messages with invalid stops and missing times
vpaturet Feb 27, 2025
24adf92
Move unit test
vpaturet Mar 3, 2025
f7abc81
Return detailed failure for invalid stop pattern
vpaturet Mar 3, 2025
cc97c9d
Accept full trip cancellation even if times are incomplete
vpaturet Mar 3, 2025
c1a93f4
Merge branch 'dev-2.x' into reject_siri_messages_with_invalid_stops
vpaturet Mar 4, 2025
47847cf
Fix merge conflict
vpaturet Mar 4, 2025
3a549a8
Apply review suggestions
vpaturet Mar 5, 2025
36ed324
Remove log message
vpaturet Mar 5, 2025
20dbee1
Added documentation on precondition for updating times and pattern
vpaturet Mar 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions application/src/main/java/org/opentripplanner/updater/spi/UpdateError.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,11 @@ public enum UpdateErrorType {
NO_TRIP_ID,
TOO_FEW_STOPS,
NO_VALID_STOPS,
// the stop cannot be found in the site repository
UNKNOWN_STOP,
// the stop exists in the site repository, but the planned stop cannot be replaced by this stop
// since they do not belong to the same station.
STOP_MISMATCH,
NO_SERVICE_ON_DATE,
INVALID_ARRIVAL_TIME,
INVALID_DEPARTURE_TIME,
Expand Down
4 changes: 2 additions & 2 deletions application/src/main/java/org/opentripplanner/updater/trip/siri/AddedTripBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@
import static org.opentripplanner.updater.alert.siri.mapping.SiriTransportModeMapper.mapTransitMainMode;
import static org.opentripplanner.updater.spi.UpdateError.UpdateErrorType.CANNOT_RESOLVE_AGENCY;
import static org.opentripplanner.updater.spi.UpdateError.UpdateErrorType.NO_START_DATE;
import static org.opentripplanner.updater.spi.UpdateError.UpdateErrorType.NO_VALID_STOPS;
import static org.opentripplanner.updater.spi.UpdateError.UpdateErrorType.TOO_FEW_STOPS;
import static org.opentripplanner.updater.spi.UpdateError.UpdateErrorType.UNKNOWN_STOP;

import java.time.LocalDate;
import java.time.ZoneId;
Expand Down Expand Up @@ -207,7 +207,7 @@ Result<TripUpdate, UpdateError> build() {

// Drop this update if the call refers to an unknown stop (not present in the site repository).
if (stopTime == null) {
return UpdateError.result(tripId, NO_VALID_STOPS, dataSource);
return UpdateError.result(tripId, UNKNOWN_STOP, dataSource);
}

aimedStopTimes.add(stopTime);
Expand Down
124 changes: 70 additions & 54 deletions application/src/main/java/org/opentripplanner/updater/trip/siri/ModifiedTripBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
package org.opentripplanner.updater.trip.siri;

import static java.lang.Boolean.TRUE;
import static org.opentripplanner.model.PickDrop.NONE;
import static org.opentripplanner.updater.spi.UpdateError.UpdateErrorType.STOP_MISMATCH;
import static org.opentripplanner.updater.spi.UpdateError.UpdateErrorType.TOO_FEW_STOPS;
import static org.opentripplanner.updater.spi.UpdateError.UpdateErrorType.UNKNOWN_STOP;

import java.time.LocalDate;
import java.time.ZoneId;
Expand Down Expand Up @@ -102,14 +103,36 @@ public ModifiedTripBuilder(
public Result<TripUpdate, UpdateError> build() {
RealTimeTripTimes newTimes = existingTripTimes.copyScheduledTimes();

var stopPattern = createStopPattern(pattern, calls, entityResolver);
if (cancellation) {
return cancelTrip(newTimes);
}

if (cancellation || stopPattern.isAllStopsNonRoutable()) {
LOG.debug("Trip is cancelled");
newTimes.cancelTrip();
return Result.success(
new TripUpdate(pattern.getStopPattern(), newTimes, serviceDate, dataSource)
if (calls.size() < 2) {
return UpdateError.result(existingTripTimes.getTrip().getId(), TOO_FEW_STOPS, dataSource);
}

var result = createStopPattern(pattern, calls, entityResolver);
if (result.isFailure()) {
int invalidStopIndex = result.failureValue().stopIndex();
LOG.info(
"Invalid SIRI-ET data for trip {} - {} at stop index {}",
existingTripTimes.getTrip().getId(),
result.failureValue().errorType(),
invalidStopIndex
);
return Result.failure(
new UpdateError(
existingTripTimes.getTrip().getId(),
result.failureValue().errorType(),
invalidStopIndex,
dataSource
)
);
}

StopPattern stopPattern = result.successValue();
if (stopPattern.isAllStopsNonRoutable()) {
return cancelTrip(newTimes);
}

applyUpdates(newTimes);
Expand Down Expand Up @@ -150,15 +173,24 @@ public Result<TripUpdate, UpdateError> build() {
return Result.success(new TripUpdate(stopPattern, newTimes, serviceDate, dataSource));
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty useless indeed. I've removed it.

newTimes.cancelTrip();
return Result.success(
new TripUpdate(pattern.getStopPattern(), newTimes, serviceDate, dataSource)
);
}

/**
* Applies real-time updates from the calls into newTimes.
*/
private void applyUpdates(RealTimeTripTimes newTimes) {
ZonedDateTime startOfService = ServiceDateUtils.asStartOfService(serviceDate, zoneId);
Set<CallWrapper> alreadyVisited = new HashSet<>();

int departureFromPreviousStop = 0;
int lastDepartureDelay = 0;
List<StopLocation> stopsInPattern = pattern.getStops();
for (int stopIndex = 0; stopIndex < stopsInPattern.size(); stopIndex++) {
StopLocation stopInPattern = stopsInPattern.get(stopIndex);
Expand All @@ -176,51 +208,26 @@ private void applyUpdates(RealTimeTripTimes newTimes) {
}
}

if (matchingCall != null) {
TimetableHelper.applyUpdates(
startOfService,
newTimes,
stopIndex,
stopIndex == (stopsInPattern.size() - 1),
predictionInaccurate,
matchingCall,
occupancy
if (matchingCall == null) {
throw new IllegalStateException(
"The stop at index %d on the trip %s cannot be matched with any call. This implies a bug.".formatted(
stopIndex,
newTimes.getTrip().getId()
)
);

alreadyVisited.add(matchingCall);

lastDepartureDelay = newTimes.getDepartureDelay(stopIndex);
} else {
// No update found in calls
if (pattern.isBoardAndAlightAt(stopIndex, NONE)) {
// When newTimes contains stops without pickup/dropoff - set both arrival/departure to previous stop's departure
// This necessary to accommodate the case when delay is reduced/eliminated between to stops with pickup/dropoff, and
// multiple non-pickup/dropoff stops are in between.
newTimes.updateArrivalTime(stopIndex, departureFromPreviousStop);
newTimes.updateDepartureTime(stopIndex, departureFromPreviousStop);

LOG.info(
"Siri non-pickup/dropoff stop time interpolation for tripId: {}",
newTimes.getTrip().getId()
);
} else {
int arrivalDelay = lastDepartureDelay;
int departureDelay = lastDepartureDelay;

if (lastDepartureDelay == 0) {
//No match has been found yet (i.e. still in RecordedCalls) - keep existing delays
arrivalDelay = existingTripTimes.getArrivalDelay(stopIndex);
departureDelay = existingTripTimes.getDepartureDelay(stopIndex);
}

newTimes.updateArrivalDelay(stopIndex, arrivalDelay);
newTimes.updateDepartureDelay(stopIndex, departureDelay);

LOG.info("Siri stop time interpolation for tripId: {}", newTimes.getTrip().getId());
}
}

departureFromPreviousStop = newTimes.getDepartureTime(stopIndex);
TimetableHelper.applyUpdates(
startOfService,
newTimes,
stopIndex,
stopIndex == (stopsInPattern.size() - 1),
predictionInaccurate,
matchingCall,
occupancy
);

alreadyVisited.add(matchingCall);
}
}

Expand All @@ -229,7 +236,7 @@ private void applyUpdates(RealTimeTripTimes newTimes) {
* replaced with stops belonging to the same Station/StopPlace. The PickDrop values are updated
* as well.
*/
static StopPattern createStopPattern(
static Result<StopPattern, UpdateError> createStopPattern(
TripPattern pattern,
List<CallWrapper> calls,
EntityResolver entityResolver
Expand All @@ -242,16 +249,22 @@ static StopPattern createStopPattern(
for (int i = 0; i < numberOfStops; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

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

boolean matchFound = false;
for (CallWrapper call : calls) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (alreadyVisited.contains(call)) {
continue;
}

//Current stop is being updated
var callStop = entityResolver.resolveQuay(call.getStopPointRef());
if (callStop == null) {
return Result.failure(new UpdateError(null, UNKNOWN_STOP, i));
}

if (!stop.equals(callStop) && !stop.isPartOfSameStationAs(callStop)) {
continue;
}
matchFound = true;

// Used in lambda
final int stopIndex = i;
Expand All @@ -268,10 +281,13 @@ static StopPattern createStopPattern(
alreadyVisited.add(call);
break;
}
if (!matchFound) {
return Result.failure(new UpdateError(null, STOP_MISMATCH, i));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}
var newStopPattern = builder.build();
return (pattern.isModified() && pattern.getStopPattern().equals(newStopPattern))
? pattern.getStopPattern()
: newStopPattern;
? Result.success(pattern.getStopPattern())
: Result.success(newStopPattern);
}
}
2 changes: 1 addition & 1 deletion application/src/test/java/org/opentripplanner/updater/trip/siri/AddedTripBuilderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -551,7 +551,7 @@ void testAddedTripFailOnUnknownStop() {

assertTrue(addedTrip.isFailure(), "Trip creation should fail");
assertEquals(
UpdateError.UpdateErrorType.NO_VALID_STOPS,
UpdateError.UpdateErrorType.UNKNOWN_STOP,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@vpaturet vpaturet Mar 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added unit tests for unknown stop and stop mismatch

addedTrip.failureValue().errorType(),
"Trip creation should fail with call referring to unknown stop"
);
Expand Down
Loading
Loading