diff --git a/src/ext/java/org/opentripplanner/ext/siri/SiriTimetableSnapshotSource.java b/src/ext/java/org/opentripplanner/ext/siri/SiriTimetableSnapshotSource.java index 99e9063418c..e0a3f6c715c 100644 --- a/src/ext/java/org/opentripplanner/ext/siri/SiriTimetableSnapshotSource.java +++ b/src/ext/java/org/opentripplanner/ext/siri/SiriTimetableSnapshotSource.java @@ -268,7 +268,7 @@ private Result handleModifiedTrip( // Also check whether trip id has been used for previously ADDED/MODIFIED trip message and // remove the previously created trip - removePreviousRealtimeUpdate(trip, serviceDate); + this.buffer.revertTripToScheduledTripPattern(trip.getId(), serviceDate); return updateResult; } @@ -295,7 +295,6 @@ private Result addTripToGraphAndBuffer(TripUpdate tr /** * Mark the scheduled trip in the buffer as deleted, given trip on service date * - * @param serviceDate service date * @return true if scheduled trip was marked as deleted */ private boolean markScheduledTripAsDeleted(Trip trip, final LocalDate serviceDate) { @@ -320,25 +319,4 @@ private boolean markScheduledTripAsDeleted(Trip trip, final LocalDate serviceDat return success; } - - /** - * Removes previous trip-update from buffer if there is an update with given trip on service date - * - * @param serviceDate service date - * @return true if a previously added trip was removed - */ - private boolean removePreviousRealtimeUpdate(final Trip trip, final LocalDate serviceDate) { - boolean success = false; - - final TripPattern pattern = buffer.getRealtimeAddedTripPattern(trip.getId(), serviceDate); - if (pattern != null) { - // Remove the previous real-time-added TripPattern from buffer. - // Only one version of the real-time-update should exist - buffer.removeLastAddedTripPattern(trip.getId(), serviceDate); - buffer.removeRealtimeUpdatedTripTimes(pattern, trip.getId(), serviceDate); - success = true; - } - - return success; - } } diff --git a/src/main/java/org/opentripplanner/model/TimetableSnapshot.java b/src/main/java/org/opentripplanner/model/TimetableSnapshot.java index e1dfd5681c0..933f89b75a5 100644 --- a/src/main/java/org/opentripplanner/model/TimetableSnapshot.java +++ b/src/main/java/org/opentripplanner/model/TimetableSnapshot.java @@ -141,44 +141,10 @@ public Timetable resolve(TripPattern pattern, LocalDate serviceDate) { return pattern.getScheduledTimetable(); } - public void removeRealtimeUpdatedTripTimes( - TripPattern tripPattern, - FeedScopedId tripId, - LocalDate serviceDate - ) { - SortedSet sortedTimetables = this.timetables.get(tripPattern); - if (sortedTimetables != null) { - TripTimes tripTimesToRemove = null; - for (Timetable timetable : sortedTimetables) { - if (timetable.isValidFor(serviceDate)) { - final TripTimes tripTimes = timetable.getTripTimes(tripId); - if (tripTimes == null) { - LOG.debug("No triptimes to remove for trip {}", tripId); - } else if (tripTimesToRemove != null) { - LOG.debug("Found two triptimes to remove for trip {}", tripId); - } else { - tripTimesToRemove = tripTimes; - } - } - } - - if (tripTimesToRemove != null) { - for (Timetable sortedTimetable : sortedTimetables) { - boolean isDirty = sortedTimetable.getTripTimes().remove(tripTimesToRemove); - if (isDirty) { - dirtyTimetables.add(sortedTimetable); - } - } - } - } - } - /** * Get the current trip pattern given a trip id and a service date, if it has been changed from * the scheduled pattern with an update, for which the stopPattern is different. * - * @param tripId trip id - * @param serviceDate service date * @return trip pattern created by the updater; null if trip is on the original trip pattern */ public TripPattern getRealtimeAddedTripPattern(FeedScopedId tripId, LocalDate serviceDate) { @@ -325,11 +291,55 @@ public void clear(String feedId) { } /** - * Removes the latest added trip pattern from the cache. This should be done when removing the - * trip times from the timetable the trip has been added to. + * If a previous realtime update has changed which trip pattern is associated with the given trip + * on the given service date, this method will dissociate the trip from that pattern and remove + * the trip's timetables from that pattern on that particular service date. + * + * For this service date, the trip will revert to its original trip pattern from the scheduled + * data, remaining on that pattern unless it's changed again by a future realtime update. + * + * @return true if the trip was found to be shifted to a different trip pattern by a realtime + * message and an attempt was made to re-associate it with its originally scheduled trip pattern. */ - public void removeLastAddedTripPattern(FeedScopedId feedScopedTripId, LocalDate serviceDate) { - realtimeAddedTripPattern.remove(new TripIdAndServiceDate(feedScopedTripId, serviceDate)); + public boolean revertTripToScheduledTripPattern(FeedScopedId tripId, LocalDate serviceDate) { + boolean success = false; + + final TripPattern pattern = getRealtimeAddedTripPattern(tripId, serviceDate); + if (pattern != null) { + // Dissociate the given trip from any realtime-added pattern. + // The trip will then fall back to its original scheduled pattern. + realtimeAddedTripPattern.remove(new TripIdAndServiceDate(tripId, serviceDate)); + // Remove times for the trip from any timetables + // under that now-obsolete realtime-added pattern. + SortedSet sortedTimetables = this.timetables.get(pattern); + if (sortedTimetables != null) { + TripTimes tripTimesToRemove = null; + for (Timetable timetable : sortedTimetables) { + if (timetable.isValidFor(serviceDate)) { + final TripTimes tripTimes = timetable.getTripTimes(tripId); + if (tripTimes == null) { + LOG.debug("No triptimes to remove for trip {}", tripId); + } else if (tripTimesToRemove != null) { + LOG.debug("Found two triptimes to remove for trip {}", tripId); + } else { + tripTimesToRemove = tripTimes; + } + } + } + + if (tripTimesToRemove != null) { + for (Timetable sortedTimetable : sortedTimetables) { + boolean isDirty = sortedTimetable.getTripTimes().remove(tripTimesToRemove); + if (isDirty) { + dirtyTimetables.add(sortedTimetable); + } + } + } + } + success = true; + } + + return success; } /** diff --git a/src/main/java/org/opentripplanner/updater/trip/TimetableSnapshotSource.java b/src/main/java/org/opentripplanner/updater/trip/TimetableSnapshotSource.java index a530f348a00..66ababa791d 100644 --- a/src/main/java/org/opentripplanner/updater/trip/TimetableSnapshotSource.java +++ b/src/main/java/org/opentripplanner/updater/trip/TimetableSnapshotSource.java @@ -190,15 +190,17 @@ public UpdateResult applyTripUpdates( // starts for example at 40:00, yesterday would probably be a better guess. serviceDate = localDateNow(); } - - uIndex += 1; - LOG.debug("trip update #{} ({} updates) :", uIndex, tripUpdate.getStopTimeUpdateCount()); - LOG.trace("{}", tripUpdate); - // Determine what kind of trip update this is final TripDescriptor.ScheduleRelationship tripScheduleRelationship = determineTripScheduleRelationship( tripDescriptor ); + if (!fullDataset) { + purgePatternModifications(tripScheduleRelationship, tripId, serviceDate); + } + + uIndex += 1; + LOG.debug("trip update #{} ({} updates) :", uIndex, tripUpdate.getStopTimeUpdateCount()); + LOG.trace("{}", tripUpdate); Result result; try { @@ -216,8 +218,18 @@ public UpdateResult applyTripUpdates( tripId, serviceDate ); - case CANCELED -> handleCanceledTrip(tripId, serviceDate, CancelationType.CANCEL); - case DELETED -> handleCanceledTrip(tripId, serviceDate, CancelationType.DELETE); + case CANCELED -> handleCanceledTrip( + tripId, + serviceDate, + CancelationType.CANCEL, + fullDataset + ); + case DELETED -> handleCanceledTrip( + tripId, + serviceDate, + CancelationType.DELETE, + fullDataset + ); case REPLACEMENT -> validateAndHandleModifiedTrip( tripUpdate, tripDescriptor, @@ -255,6 +267,51 @@ public UpdateResult applyTripUpdates( return updateResult; } + /** + * Remove previous realtime updates for this trip. This is necessary to avoid previous stop + * pattern modifications from persisting. If a trip was previously added with the + * ScheduleRelationship ADDED and is now cancelled or deleted, we still want to keep the realtime + * added trip pattern. + */ + private void purgePatternModifications( + TripDescriptor.ScheduleRelationship tripScheduleRelationship, + FeedScopedId tripId, + LocalDate serviceDate + ) { + final TripPattern pattern = buffer.getRealtimeAddedTripPattern(tripId, serviceDate); + if ( + !isPreviouslyAddedTrip(tripId, pattern, serviceDate) || + ( + tripScheduleRelationship != TripDescriptor.ScheduleRelationship.CANCELED && + tripScheduleRelationship != TripDescriptor.ScheduleRelationship.DELETED + ) + ) { + // Remove previous realtime updates for this trip. This is necessary to avoid previous + // stop pattern modifications from persisting. If a trip was previously added with the ScheduleRelationship + // ADDED and is now cancelled or deleted, we still want to keep the realtime added trip pattern. + this.buffer.revertTripToScheduledTripPattern(tripId, serviceDate); + } + } + + private boolean isPreviouslyAddedTrip( + FeedScopedId tripId, + TripPattern pattern, + LocalDate serviceDate + ) { + if (pattern == null) { + return false; + } + var timetable = buffer.resolve(pattern, serviceDate); + if (timetable == null) { + return false; + } + var tripTimes = timetable.getTripTimes(tripId); + if (tripTimes == null) { + return false; + } + return tripTimes.getRealTimeState() == RealTimeState.ADDED; + } + private static void logUpdateResult( String feedId, Map failuresByRelationship, @@ -327,11 +384,6 @@ private Result handleScheduledTrip( return UpdateError.result(tripId, NO_SERVICE_ON_DATE); } - // If this trip_id has been used for previously ADDED/MODIFIED trip message (e.g. when the - // sequence of stops has changed, and is now changing back to the originally scheduled one), - // mark that previously created trip as DELETED. - cancelPreviouslyAddedTrip(tripId, serviceDate, CancelationType.DELETE); - // Get new TripTimes based on scheduled timetable var result = pattern .getScheduledTimetable() @@ -578,10 +630,6 @@ private Result handleAddedTrip( "number of stop should match the number of stop time updates" ); - // Check whether trip id has been used for previously ADDED trip message and mark previously - // created trip as DELETED - cancelPreviouslyAddedTrip(tripId, serviceDate, CancelationType.DELETE); - Route route = getOrCreateRoute(tripDescriptor, tripId); // Create new Trip @@ -827,8 +875,6 @@ private Result addTripToGraphAndBuffer( /** * Cancel scheduled trip in buffer given trip id on service date * - * @param tripId trip id - * @param serviceDate service date * @return true if scheduled trip was cancelled */ private boolean cancelScheduledTrip( @@ -864,14 +910,11 @@ private boolean cancelScheduledTrip( /** * Cancel previously added trip from buffer if there is a previously added trip with given trip id - * (without agency id) on service date. This does not remove the modified/added trip from the - * buffer, it just marks it as canceled. This also does not remove the corresponding vertices and - * edges from the Graph. Any TripPattern that was created for the added/modified trip continues to - * exist, and will be reused if a similar added/modified trip message is received with the same - * route and stop sequence. + * on service date. This does not remove the added trip from the buffer, it just marks it as + * canceled or deleted. Any TripPattern that was created for the added trip continues to exist, + * and will be reused if a similar added trip message is received with the same route and stop + * sequence. * - * @param tripId trip id without agency id - * @param serviceDate service date * @return true if a previously added trip was cancelled */ private boolean cancelPreviouslyAddedTrip( @@ -879,10 +922,10 @@ private boolean cancelPreviouslyAddedTrip( final LocalDate serviceDate, CancelationType cancelationType ) { - boolean success = false; + boolean cancelledAddedTrip = false; final TripPattern pattern = buffer.getRealtimeAddedTripPattern(tripId, serviceDate); - if (pattern != null) { + if (isPreviouslyAddedTrip(tripId, pattern, serviceDate)) { // Cancel trip times for this trip in this pattern final Timetable timetable = buffer.resolve(pattern, serviceDate); final int tripIndex = timetable.getTripIndex(tripId); @@ -897,11 +940,10 @@ private boolean cancelPreviouslyAddedTrip( case DELETE -> newTripTimes.deleteTrip(); } buffer.update(pattern, newTripTimes, serviceDate); - success = true; + cancelledAddedTrip = true; } } - - return success; + return cancelledAddedTrip; } /** @@ -996,10 +1038,6 @@ private Result handleModifiedTrip( var tripId = trip.getId(); cancelScheduledTrip(tripId, serviceDate, CancelationType.DELETE); - // Check whether trip id has been used for previously ADDED/REPLACEMENT trip message and mark it - // as DELETED - cancelPreviouslyAddedTrip(tripId, serviceDate, CancelationType.DELETE); - // Add new trip return addTripToGraphAndBuffer( trip, @@ -1014,19 +1052,25 @@ private Result handleModifiedTrip( private Result handleCanceledTrip( FeedScopedId tripId, final LocalDate serviceDate, - CancelationType markAsDeleted + CancelationType cancelationType, + boolean fullDataset ) { - // Try to cancel scheduled trip - final boolean cancelScheduledSuccess = cancelScheduledTrip(tripId, serviceDate, markAsDeleted); + var canceledPreviouslyAddedTrip = fullDataset + ? false + : cancelPreviouslyAddedTrip(tripId, serviceDate, cancelationType); - // Try to cancel previously added trip - final boolean cancelPreviouslyAddedSuccess = cancelPreviouslyAddedTrip( + // if previously an added trip was removed, there can't be a scheduled trip to remove + if (canceledPreviouslyAddedTrip) { + return Result.success(UpdateSuccess.noWarnings()); + } + // Try to cancel scheduled trip + final boolean cancelScheduledSuccess = cancelScheduledTrip( tripId, serviceDate, - markAsDeleted + cancelationType ); - if (!cancelScheduledSuccess && !cancelPreviouslyAddedSuccess) { + if (!cancelScheduledSuccess) { debug(tripId, "No pattern found for tripId. Skipping cancellation."); return UpdateError.result(tripId, NO_TRIP_FOR_CANCELLATION_FOUND); } diff --git a/src/test/java/org/opentripplanner/updater/trip/RealtimeTestEnvironment.java b/src/test/java/org/opentripplanner/updater/trip/RealtimeTestEnvironment.java index 1b385df153b..52b0547e8c9 100644 --- a/src/test/java/org/opentripplanner/updater/trip/RealtimeTestEnvironment.java +++ b/src/test/java/org/opentripplanner/updater/trip/RealtimeTestEnvironment.java @@ -256,15 +256,22 @@ public UpdateResult applyEstimatedTimetable(List updates) { + public UpdateResult applyTripUpdate(GtfsRealtime.TripUpdate update, boolean differential) { + return applyTripUpdates(List.of(update), differential); + } + + public UpdateResult applyTripUpdates( + List updates, + boolean differential + ) { Objects.requireNonNull(gtfsSource, "Test environment is configured for SIRI only"); return gtfsSource.applyTripUpdates( null, BackwardsDelayPropagationType.REQUIRED_NO_DATA, - true, + !differential, updates, getFeedId() ); diff --git a/src/test/java/org/opentripplanner/updater/trip/TimetableSnapshotSourceTest.java b/src/test/java/org/opentripplanner/updater/trip/TimetableSnapshotSourceTest.java index 36ba293e508..96396610aa1 100644 --- a/src/test/java/org/opentripplanner/updater/trip/TimetableSnapshotSourceTest.java +++ b/src/test/java/org/opentripplanner/updater/trip/TimetableSnapshotSourceTest.java @@ -604,7 +604,7 @@ private TripPattern assertAddedTrip( TimetableSnapshotSource updater ) { var stopA = transitModel.getStopModel().getRegularStop(new FeedScopedId(feedId, "A")); - // Get trip pattern of last (most recently added) outgoing edge + // Get the trip pattern of the added trip which goes through stopA var snapshot = updater.getTimetableSnapshot(); var patternsAtA = snapshot.getPatternsForStop(stopA); diff --git a/src/test/java/org/opentripplanner/updater/trip/moduletests/cancellation/CancellationDeletionTest.java b/src/test/java/org/opentripplanner/updater/trip/moduletests/cancellation/CancellationDeletionTest.java index 8af13ac286f..f6b6b128330 100644 --- a/src/test/java/org/opentripplanner/updater/trip/moduletests/cancellation/CancellationDeletionTest.java +++ b/src/test/java/org/opentripplanner/updater/trip/moduletests/cancellation/CancellationDeletionTest.java @@ -1,6 +1,7 @@ package org.opentripplanner.updater.trip.moduletests.cancellation; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNotSame; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -58,4 +59,71 @@ public void cancelledTrip(ScheduleRelationship relationship, RealTimeState state assertEquals(state, tripTimes.getRealTimeState()); assertTrue(tripTimes.isCanceledOrDeleted()); } + + /** + * Test behavior of the realtime system in a case related to #5725 that is discussed at: + * https://github.com/opentripplanner/OpenTripPlanner/pull/5726#discussion_r1521653840 + * When a trip is added by a realtime message, in the realtime data indexes a corresponding + * trip pattern should be associated with the stops that trip visits. When a subsequent + * realtime message cancels or deletes that trip, the pattern should continue to be present in + * the realtime data indexes, and it should still contain the previously added trip, but that + * trip should be marked as having canceled or deleted status. At no point should the trip + * added by realtime data be present in the trip pattern for scheduled service. + */ + @ParameterizedTest + @MethodSource("cases") + public void cancelingAddedTrip(ScheduleRelationship relationship, RealTimeState state) { + var env = RealtimeTestEnvironment.gtfs(); + var addedTripId = "added-trip"; + // First add ADDED trip + var update = new TripUpdateBuilder( + addedTripId, + RealtimeTestEnvironment.SERVICE_DATE, + ScheduleRelationship.ADDED, + env.timeZone + ) + .addStopTime(env.stopA1.getId().getId(), 30) + .addStopTime(env.stopB1.getId().getId(), 40) + .addStopTime(env.stopC1.getId().getId(), 55) + .build(); + + var result = env.applyTripUpdate(update, true); + + assertEquals(1, result.successful()); + + // Cancel or delete the added trip + update = + new TripUpdateBuilder( + addedTripId, + RealtimeTestEnvironment.SERVICE_DATE, + relationship, + env.timeZone + ) + .build(); + result = env.applyTripUpdate(update, true); + + assertEquals(1, result.successful()); + + final TimetableSnapshot snapshot = env.getTimetableSnapshot(); + // Get the trip pattern of the added trip which goes through stopA + var patternsAtA = snapshot.getPatternsForStop(env.stopA1); + + assertNotNull(patternsAtA, "Added trip pattern should be found"); + var tripPattern = patternsAtA.stream().findFirst().get(); + + final Timetable forToday = snapshot.resolve(tripPattern, RealtimeTestEnvironment.SERVICE_DATE); + final Timetable schedule = snapshot.resolve(tripPattern, null); + + assertNotSame(forToday, schedule); + + final int forTodayAddedTripIndex = forToday.getTripIndex(addedTripId); + assertTrue( + forTodayAddedTripIndex > -1, + "Added trip should be found in time table for the service date" + ); + assertEquals(state, forToday.getTripTimes(forTodayAddedTripIndex).getRealTimeState()); + + final int scheduleTripIndex = schedule.getTripIndex(addedTripId); + assertEquals(-1, scheduleTripIndex, "Added trip should not be found in scheduled time table"); + } } diff --git a/src/test/java/org/opentripplanner/updater/trip/moduletests/delay/SkippedTest.java b/src/test/java/org/opentripplanner/updater/trip/moduletests/delay/SkippedTest.java index d97704549ee..7bee7c1b5d0 100644 --- a/src/test/java/org/opentripplanner/updater/trip/moduletests/delay/SkippedTest.java +++ b/src/test/java/org/opentripplanner/updater/trip/moduletests/delay/SkippedTest.java @@ -4,6 +4,7 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotSame; +import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertTrue; import org.junit.jupiter.api.Test; @@ -116,4 +117,108 @@ public void scheduledTripWithSkippedAndScheduled() { assertFalse(newTripTimes.isNoDataStop(2)); } } + + /** + * Test realtime system behavior under one very particular case from issue #5725. + * When applying differential realtime updates, an update may cancel some stops on a trip. A + * later update may then revert the trip back to its originally scheduled sequence of stops. + * When this happens, we expect the trip to be associated with a new trip pattern (where some + * stops have no pickup or dropoff) then dissociated from that new pattern and re-associated + * with its originally scheduled pattern. Any trip times that were created in timetables under + * the new stop-skipping trip pattern should also be removed. + */ + @Test + public void scheduledTripWithPreviouslySkipped() { + var env = RealtimeTestEnvironment.gtfs(); + var tripId = env.trip2.getId(); + + var tripUpdate = new TripUpdateBuilder( + tripId.getId(), + RealtimeTestEnvironment.SERVICE_DATE, + SCHEDULED, + env.timeZone + ) + .addDelayedStopTime(0, 0) + .addSkippedStop(1) + .addDelayedStopTime(2, 90) + .build(); + + var result = env.applyTripUpdate(tripUpdate, true); + + assertEquals(1, result.successful()); + + // Create update to the same trip but now the skipped stop is no longer skipped + var scheduledBuilder = new TripUpdateBuilder( + tripId.getId(), + RealtimeTestEnvironment.SERVICE_DATE, + SCHEDULED, + env.timeZone + ) + .addDelayedStopTime(0, 0) + .addDelayedStopTime(1, 50) + .addDelayedStopTime(2, 90); + + tripUpdate = scheduledBuilder.build(); + + // apply the update with the previously skipped stop now scheduled + result = env.applyTripUpdate(tripUpdate, true); + + assertEquals(1, result.successful()); + // Check that the there is no longer a realtime added trip pattern for the trip and that the + // stoptime updates have gone through + var snapshot = env.getTimetableSnapshot(); + + { + final TripPattern newTripPattern = snapshot.getRealtimeAddedTripPattern( + env.trip2.getId(), + RealtimeTestEnvironment.SERVICE_DATE + ); + assertNull(newTripPattern); + final Trip trip = env.transitModel.getTransitModelIndex().getTripForId().get(tripId); + + final TripPattern originalTripPattern = env.transitModel + .getTransitModelIndex() + .getPatternForTrip() + .get(trip); + final Timetable originalTimetableForToday = snapshot.resolve( + originalTripPattern, + RealtimeTestEnvironment.SERVICE_DATE + ); + + final Timetable originalTimetableScheduled = snapshot.resolve(originalTripPattern, null); + + assertNotSame(originalTimetableForToday, originalTimetableScheduled); + + final int originalTripIndexScheduled = originalTimetableScheduled.getTripIndex(tripId); + + assertTrue( + originalTripIndexScheduled > -1, + "Original trip should be found in scheduled time table" + ); + final TripTimes originalTripTimesScheduled = originalTimetableScheduled.getTripTimes( + originalTripIndexScheduled + ); + assertFalse( + originalTripTimesScheduled.isCanceledOrDeleted(), + "Original trip times should not be canceled in scheduled time table" + ); + assertEquals(RealTimeState.SCHEDULED, originalTripTimesScheduled.getRealTimeState()); + final int originalTripIndexForToday = originalTimetableForToday.getTripIndex(tripId); + + assertTrue( + originalTripIndexForToday > -1, + "Original trip should be found in time table for service date" + ); + final TripTimes originalTripTimesForToday = originalTimetableForToday.getTripTimes( + originalTripIndexForToday + ); + assertEquals(RealTimeState.UPDATED, originalTripTimesForToday.getRealTimeState()); + assertEquals(0, originalTripTimesForToday.getArrivalDelay(0)); + assertEquals(0, originalTripTimesForToday.getDepartureDelay(0)); + assertEquals(50, originalTripTimesForToday.getArrivalDelay(1)); + assertEquals(50, originalTripTimesForToday.getDepartureDelay(1)); + assertEquals(90, originalTripTimesForToday.getArrivalDelay(2)); + assertEquals(90, originalTripTimesForToday.getDepartureDelay(2)); + } + } }