From e432a386df32bc599b43ac27f77f3d807df4a01e Mon Sep 17 00:00:00 2001 From: Joel Lappalainen Date: Mon, 4 Mar 2024 22:58:42 +0200 Subject: [PATCH 1/7] Fix issue with realtime added patterns persisting with DIFFERENTIAL updates --- .../ext/siri/SiriTimetableSnapshotSource.java | 23 +---- .../model/TimetableSnapshot.java | 94 +++++++++++-------- .../updater/trip/TimetableSnapshotSource.java | 54 ++++++----- 3 files changed, 88 insertions(+), 83 deletions(-) diff --git a/src/ext/java/org/opentripplanner/ext/siri/SiriTimetableSnapshotSource.java b/src/ext/java/org/opentripplanner/ext/siri/SiriTimetableSnapshotSource.java index 345f8deba20..87f100464a5 100644 --- a/src/ext/java/org/opentripplanner/ext/siri/SiriTimetableSnapshotSource.java +++ b/src/ext/java/org/opentripplanner/ext/siri/SiriTimetableSnapshotSource.java @@ -354,7 +354,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.removePreviousRealtimeUpdate(trip.getId(), serviceDate); return updateResult; } @@ -410,27 +410,6 @@ 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; - } - private boolean purgeExpiredData() { final LocalDate today = LocalDate.now(transitModel.getTimeZone()); final LocalDate previously = today.minusDays(2); // Just to be safe... diff --git a/src/main/java/org/opentripplanner/model/TimetableSnapshot.java b/src/main/java/org/opentripplanner/model/TimetableSnapshot.java index 066a27ba15a..937c3e13ffd 100644 --- a/src/main/java/org/opentripplanner/model/TimetableSnapshot.java +++ b/src/main/java/org/opentripplanner/model/TimetableSnapshot.java @@ -20,7 +20,6 @@ import org.opentripplanner.transit.model.network.TripPattern; import org.opentripplanner.transit.model.site.StopLocation; import org.opentripplanner.transit.model.timetable.TripIdAndServiceDate; -import org.opentripplanner.transit.model.timetable.TripOnServiceDate; import org.opentripplanner.transit.model.timetable.TripTimes; import org.opentripplanner.updater.spi.UpdateError; import org.opentripplanner.updater.spi.UpdateSuccess; @@ -111,38 +110,6 @@ 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. @@ -295,11 +262,24 @@ 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. + * 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 */ - public void removeLastAddedTripPattern(FeedScopedId feedScopedTripId, LocalDate serviceDate) { - realtimeAddedTripPattern.remove(new TripIdAndServiceDate(feedScopedTripId, serviceDate)); + public boolean removePreviousRealtimeUpdate(FeedScopedId tripId, LocalDate serviceDate) { + boolean success = false; + + final TripPattern pattern = getRealtimeAddedTripPattern(tripId, serviceDate); + if (pattern != null) { + // Remove the previous real-time-added TripPattern from buffer. + // Only one version of the real-time-update should exist + removeLastAddedTripPattern(tripId, serviceDate); + removeRealtimeUpdatedTripTimes(pattern, tripId, serviceDate); + success = true; + } + + return success; } /** @@ -391,6 +371,46 @@ protected boolean clearRealtimeAddedTripPattern(String feedId) { ); } + private 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); + } + } + } + } + } + + /** + * 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. + */ + private void removeLastAddedTripPattern(FeedScopedId feedScopedTripId, LocalDate serviceDate) { + realtimeAddedTripPattern.remove(new TripIdAndServiceDate(feedScopedTripId, serviceDate)); + } + /** * Add the patterns to the stop index, only if they come from a modified pattern */ diff --git a/src/main/java/org/opentripplanner/updater/trip/TimetableSnapshotSource.java b/src/main/java/org/opentripplanner/updater/trip/TimetableSnapshotSource.java index 19db2c7e309..1c2013f789d 100644 --- a/src/main/java/org/opentripplanner/updater/trip/TimetableSnapshotSource.java +++ b/src/main/java/org/opentripplanner/updater/trip/TimetableSnapshotSource.java @@ -272,6 +272,17 @@ public UpdateResult applyTripUpdates( serviceDate = localDateNow.get(); } + // Check whether trip id has been used for previously ADDED trip message and mark previously + // created trip as DELETED + var canceledPreviouslyAddedTrip = cancelPreviouslyAddedTrip( + tripId, + serviceDate, + CancelationType.DELETE + ); + // Remove previous realtime updates for this trip. This is necessary to avoid previous + // stop pattern modifications from persisting + this.buffer.removePreviousRealtimeUpdate(tripId, serviceDate); + uIndex += 1; LOG.debug("trip update #{} ({} updates) :", uIndex, tripUpdate.getStopTimeUpdateCount()); LOG.trace("{}", tripUpdate); @@ -297,8 +308,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, + canceledPreviouslyAddedTrip + ); + case DELETED -> handleCanceledTrip( + tripId, + serviceDate, + CancelationType.DELETE, + canceledPreviouslyAddedTrip + ); case REPLACEMENT -> validateAndHandleModifiedTrip( tripUpdate, tripDescriptor, @@ -435,11 +456,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() @@ -687,10 +703,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 @@ -1105,10 +1117,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, @@ -1123,19 +1131,17 @@ private Result handleModifiedTrip( private Result handleCanceledTrip( FeedScopedId tripId, final LocalDate serviceDate, - CancelationType markAsDeleted + CancelationType markAsDeleted, + boolean canceledPreviouslyAddedTrip ) { + // if previously a 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); - // Try to cancel previously added trip - final boolean cancelPreviouslyAddedSuccess = cancelPreviouslyAddedTrip( - tripId, - serviceDate, - markAsDeleted - ); - - if (!cancelScheduledSuccess && !cancelPreviouslyAddedSuccess) { + if (!cancelScheduledSuccess) { debug(tripId, "No pattern found for tripId. Skipping cancellation."); return UpdateError.result(tripId, NO_TRIP_FOR_CANCELLATION_FOUND); } From d3f467d1ab3567aac9c609918e70cfc88fbf1643 Mon Sep 17 00:00:00 2001 From: Joel Lappalainen Date: Mon, 11 Mar 2024 14:31:28 +0200 Subject: [PATCH 2/7] Only run removal of old leftovers if DIFFERENTIAL update --- .../updater/trip/TimetableSnapshotSource.java | 21 +++++++++---------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/src/main/java/org/opentripplanner/updater/trip/TimetableSnapshotSource.java b/src/main/java/org/opentripplanner/updater/trip/TimetableSnapshotSource.java index 1c2013f789d..eefa704b43d 100644 --- a/src/main/java/org/opentripplanner/updater/trip/TimetableSnapshotSource.java +++ b/src/main/java/org/opentripplanner/updater/trip/TimetableSnapshotSource.java @@ -271,17 +271,16 @@ public UpdateResult applyTripUpdates( // starts for example at 40:00, yesterday would probably be a better guess. serviceDate = localDateNow.get(); } - - // Check whether trip id has been used for previously ADDED trip message and mark previously - // created trip as DELETED - var canceledPreviouslyAddedTrip = cancelPreviouslyAddedTrip( - tripId, - serviceDate, - CancelationType.DELETE - ); - // Remove previous realtime updates for this trip. This is necessary to avoid previous - // stop pattern modifications from persisting - this.buffer.removePreviousRealtimeUpdate(tripId, serviceDate); + var canceledPreviouslyAddedTrip = false; + if (!fullDataset) { + // Check whether trip id has been used for previously ADDED trip message and mark previously + // created trip as DELETED + canceledPreviouslyAddedTrip = + cancelPreviouslyAddedTrip(tripId, serviceDate, CancelationType.DELETE); + // Remove previous realtime updates for this trip. This is necessary to avoid previous + // stop pattern modifications from persisting + this.buffer.removePreviousRealtimeUpdate(tripId, serviceDate); + } uIndex += 1; LOG.debug("trip update #{} ({} updates) :", uIndex, tripUpdate.getStopTimeUpdateCount()); From 9630a4ad83f1aa068f91b28ab18eda75b161fd04 Mon Sep 17 00:00:00 2001 From: Joel Lappalainen Date: Tue, 12 Mar 2024 17:20:05 +0200 Subject: [PATCH 3/7] Set added trip as canceled again if canceled update --- .../updater/trip/TimetableSnapshotSource.java | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/src/main/java/org/opentripplanner/updater/trip/TimetableSnapshotSource.java b/src/main/java/org/opentripplanner/updater/trip/TimetableSnapshotSource.java index eefa704b43d..63d8cf92283 100644 --- a/src/main/java/org/opentripplanner/updater/trip/TimetableSnapshotSource.java +++ b/src/main/java/org/opentripplanner/updater/trip/TimetableSnapshotSource.java @@ -271,12 +271,20 @@ public UpdateResult applyTripUpdates( // starts for example at 40:00, yesterday would probably be a better guess. serviceDate = localDateNow.get(); } + // Determine what kind of trip update this is + final TripDescriptor.ScheduleRelationship tripScheduleRelationship = determineTripScheduleRelationship( + tripDescriptor + ); var canceledPreviouslyAddedTrip = false; if (!fullDataset) { // Check whether trip id has been used for previously ADDED trip message and mark previously - // created trip as DELETED + // created trip as DELETED unless schedule relationship is CANCELED, then as CANCEL + var cancelationType = tripScheduleRelationship == + TripDescriptor.ScheduleRelationship.CANCELED + ? CancelationType.CANCEL + : CancelationType.DELETE; canceledPreviouslyAddedTrip = - cancelPreviouslyAddedTrip(tripId, serviceDate, CancelationType.DELETE); + cancelPreviouslyAddedTrip(tripId, serviceDate, cancelationType); // Remove previous realtime updates for this trip. This is necessary to avoid previous // stop pattern modifications from persisting this.buffer.removePreviousRealtimeUpdate(tripId, serviceDate); @@ -286,11 +294,6 @@ public UpdateResult applyTripUpdates( 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 - ); - Result result; try { result = From 0177fb4f074bd116543ecb641453d87a1d4395ef Mon Sep 17 00:00:00 2001 From: Joel Lappalainen Date: Tue, 12 Mar 2024 18:03:12 +0200 Subject: [PATCH 4/7] Add test for removing skipped update --- .../updater/trip/TimetableSnapshotSource.java | 36 +++--- .../trip/TimetableSnapshotSourceTest.java | 111 ++++++++++++++++++ 2 files changed, 131 insertions(+), 16 deletions(-) diff --git a/src/main/java/org/opentripplanner/updater/trip/TimetableSnapshotSource.java b/src/main/java/org/opentripplanner/updater/trip/TimetableSnapshotSource.java index 63d8cf92283..21fcbfd2d6e 100644 --- a/src/main/java/org/opentripplanner/updater/trip/TimetableSnapshotSource.java +++ b/src/main/java/org/opentripplanner/updater/trip/TimetableSnapshotSource.java @@ -370,6 +370,26 @@ public UpdateResult applyTripUpdates( return updateResult; } + /** + * This shouldn't be used outside of this class for other purposes than testing where the forced + * snapshot commit can guarantee consistent behaviour. + */ + TimetableSnapshot getTimetableSnapshot(final boolean force) { + final long now = System.currentTimeMillis(); + if (force || now - lastSnapshotTime > maxSnapshotFrequency.toMillis()) { + if (force || buffer.isDirty()) { + LOG.debug("Committing {}", buffer); + snapshot = buffer.commit(transitLayerUpdater, force); + } else { + LOG.debug("Buffer was unchanged, keeping old snapshot."); + } + lastSnapshotTime = System.currentTimeMillis(); + } else { + LOG.debug("Snapshot frequency exceeded. Reusing snapshot {}", snapshot); + } + return snapshot; + } + private static void logUpdateResult( String feedId, Map failuresByRelationship, @@ -390,22 +410,6 @@ private static void logUpdateResult( }); } - private TimetableSnapshot getTimetableSnapshot(final boolean force) { - final long now = System.currentTimeMillis(); - if (force || now - lastSnapshotTime > maxSnapshotFrequency.toMillis()) { - if (force || buffer.isDirty()) { - LOG.debug("Committing {}", buffer); - snapshot = buffer.commit(transitLayerUpdater, force); - } else { - LOG.debug("Buffer was unchanged, keeping old snapshot."); - } - lastSnapshotTime = System.currentTimeMillis(); - } else { - LOG.debug("Snapshot frequency exceeded. Reusing snapshot {}", snapshot); - } - return snapshot; - } - /** * Determine how the trip update should be handled. * diff --git a/src/test/java/org/opentripplanner/updater/trip/TimetableSnapshotSourceTest.java b/src/test/java/org/opentripplanner/updater/trip/TimetableSnapshotSourceTest.java index d25de6ff021..10952d0bde9 100644 --- a/src/test/java/org/opentripplanner/updater/trip/TimetableSnapshotSourceTest.java +++ b/src/test/java/org/opentripplanner/updater/trip/TimetableSnapshotSourceTest.java @@ -849,6 +849,117 @@ public void scheduledTripWithSkippedAndScheduled() { assertFalse(newTripTimes.isNoDataStop(2)); } } + + @Test + public void scheduledTripWithPreviouslySkipped() { + // Create update with a skipped stop at first + String scheduledTripId = "1.1"; + + var skippedBuilder = new TripUpdateBuilder( + scheduledTripId, + SERVICE_DATE, + SCHEDULED, + transitModel.getTimeZone() + ) + .addDelayedStopTime(1, 0) + .addSkippedStop(2) + .addDelayedStopTime(3, 90); + + var tripUpdate = skippedBuilder.build(); + + var updater = defaultUpdater(); + + // apply the update with a skipped stop + updater.applyTripUpdates( + TRIP_MATCHER_NOOP, + REQUIRED_NO_DATA, + false, + List.of(tripUpdate), + feedId + ); + + // Force a snapshot commit. This is done to mimic normal behaviour where a new update arrives + // after the original snapshot has been committed + updater.getTimetableSnapshot(true); + + // Create update to the same trip but now the skipped stop is no longer skipped + var scheduledBuilder = new TripUpdateBuilder( + scheduledTripId, + SERVICE_DATE, + SCHEDULED, + transitModel.getTimeZone() + ) + .addDelayedStopTime(1, 0) + .addDelayedStopTime(2, 60, 80) + .addDelayedStopTime(3, 90, 90); + + tripUpdate = scheduledBuilder.build(); + + // apply the update with the previously skipped stop now scheduled + updater.applyTripUpdates( + TRIP_MATCHER_NOOP, + REQUIRED_NO_DATA, + false, + List.of(tripUpdate), + feedId + ); + + // 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 = updater.getTimetableSnapshot(true); + { + final TripPattern newTripPattern = snapshot.getRealtimeAddedTripPattern( + new FeedScopedId(feedId, scheduledTripId), + SERVICE_DATE + ); + assertNull(newTripPattern); + + final FeedScopedId tripId = new FeedScopedId(feedId, scheduledTripId); + final Trip trip = transitModel.getTransitModelIndex().getTripForId().get(tripId); + final TripPattern originalTripPattern = transitModel + .getTransitModelIndex() + .getPatternForTrip() + .get(trip); + + final Timetable originalTimetableForToday = snapshot.resolve( + originalTripPattern, + 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(60, originalTripTimesForToday.getArrivalDelay(1)); + assertEquals(80, originalTripTimesForToday.getDepartureDelay(1)); + assertEquals(90, originalTripTimesForToday.getArrivalDelay(2)); + assertEquals(90, originalTripTimesForToday.getDepartureDelay(2)); + } + } } @Nested From f518363685913c97d65dba4272cd60e9b90447c6 Mon Sep 17 00:00:00 2001 From: Joel Lappalainen Date: Thu, 14 Mar 2024 11:48:37 +0200 Subject: [PATCH 5/7] Add tests for canceling/deleting added trips --- .../trip/TimetableSnapshotSourceTest.java | 173 +++++++++++++++++- 1 file changed, 166 insertions(+), 7 deletions(-) diff --git a/src/test/java/org/opentripplanner/updater/trip/TimetableSnapshotSourceTest.java b/src/test/java/org/opentripplanner/updater/trip/TimetableSnapshotSourceTest.java index 10952d0bde9..4936b522d87 100644 --- a/src/test/java/org/opentripplanner/updater/trip/TimetableSnapshotSourceTest.java +++ b/src/test/java/org/opentripplanner/updater/trip/TimetableSnapshotSourceTest.java @@ -992,17 +992,18 @@ public void addedTrip() { ); // THEN - assertAddedTrip(SERVICE_DATE, this.addedTripId, updater); + assertAddedTrip(SERVICE_DATE, this.addedTripId, updater, false); } private TripPattern assertAddedTrip( LocalDate serviceDate, String tripId, - TimetableSnapshotSource updater + TimetableSnapshotSource updater, + boolean forceSnapshotCommit ) { var stopA = transitModel.getStopModel().getRegularStop(new FeedScopedId(feedId, "A")); // Get trip pattern of last (most recently added) outgoing edge - var snapshot = updater.getTimetableSnapshot(); + var snapshot = updater.getTimetableSnapshot(forceSnapshotCommit); var patternsAtA = snapshot.getPatternsForStop(stopA); assertNotNull(patternsAtA, "Added trip pattern should be found"); @@ -1064,7 +1065,7 @@ public void addedTripWithNewRoute() { assertTrue(result.warnings().isEmpty()); - var pattern = assertAddedTrip(SERVICE_DATE, addedTripId, updater); + var pattern = assertAddedTrip(SERVICE_DATE, addedTripId, updater, false); var route = pattern.getRoute(); assertEquals(TripUpdateBuilder.ROUTE_URL, route.getUrl()); @@ -1117,7 +1118,7 @@ public void addedWithUnknownStop() { assertEquals(List.of(WarningType.UNKNOWN_STOPS_REMOVED_FROM_ADDED_TRIP), result.warnings()); - var pattern = assertAddedTrip(SERVICE_DATE, addedTripId, updater); + var pattern = assertAddedTrip(SERVICE_DATE, addedTripId, updater, false); assertEquals(2, pattern.getStops().size()); } @@ -1152,7 +1153,7 @@ public void repeatedlyAddedTripWithNewRoute() { List.of(tripUpdate), feedId ); - var pattern = assertAddedTrip(SERVICE_DATE, addedTripId, updater); + var pattern = assertAddedTrip(SERVICE_DATE, addedTripId, updater, false); var firstRoute = pattern.getRoute(); // apply the update a second time to check that no new route instance is created but the old one is reused @@ -1163,7 +1164,7 @@ public void repeatedlyAddedTripWithNewRoute() { List.of(tripUpdate), feedId ); - var secondPattern = assertAddedTrip(SERVICE_DATE, addedTripId, updater); + var secondPattern = assertAddedTrip(SERVICE_DATE, addedTripId, updater, false); var secondRoute = secondPattern.getRoute(); // THEN @@ -1171,6 +1172,164 @@ public void repeatedlyAddedTripWithNewRoute() { assertSame(firstRoute, secondRoute); assertNotNull(transitModel.getTransitModelIndex().getRouteForId(firstRoute.getId())); } + + @Test + public void cancelingAddedTrip() { + // TODO we might want to change the behaviour so that only the trip without pattern is + // persisted if the added trip is cancelled + var builder = new TripUpdateBuilder( + addedTripId, + SERVICE_DATE, + ADDED, + transitModel.getTimeZone() + ); + + builder.addStopTime("A", 30).addStopTime("C", 40).addStopTime("E", 55); + + var tripUpdate = builder.build(); + + var updater = defaultUpdater(); + + // WHEN + updater.applyTripUpdates( + TRIP_MATCHER_NOOP, + REQUIRED_NO_DATA, + fullDataset, + List.of(tripUpdate), + feedId + ); + + // THEN + assertAddedTrip(SERVICE_DATE, this.addedTripId, updater, true); + + builder = new TripUpdateBuilder( + addedTripId, + SERVICE_DATE, + ADDED, + transitModel.getTimeZone() + ); + + var tripDescriptorBuilder = TripDescriptor.newBuilder(); + tripDescriptorBuilder.setTripId(addedTripId); + tripDescriptorBuilder.setScheduleRelationship(ScheduleRelationship.CANCELED); + + tripDescriptorBuilder.setStartDate(ServiceDateUtils.asCompactString(SERVICE_DATE)); + tripUpdate = TripUpdate.newBuilder().setTrip(tripDescriptorBuilder).build(); + + // WHEN + updater.applyTripUpdates( + TRIP_MATCHER_NOOP, + REQUIRED_NO_DATA, + fullDataset, + List.of(tripUpdate), + feedId + ); + + // THEN + // Get trip pattern of last (most recently added) outgoing edge + var snapshot = updater.getTimetableSnapshot(true); + var stopA = transitModel.getStopModel().getRegularStop(new FeedScopedId(feedId, "A")); + var patternsAtA = snapshot.getPatternsForStop(stopA); + + assertNotNull(patternsAtA, "Added trip pattern should be found"); + var tripPattern = patternsAtA.stream().findFirst().get(); + + final Timetable forToday = snapshot.resolve(tripPattern, SERVICE_DATE); + final Timetable schedule = snapshot.resolve(tripPattern, null); + + assertNotSame(forToday, schedule); + + final int forTodayAddedTripIndex = forToday.getTripIndex(addedTripId); + assertTrue( + forTodayAddedTripIndex > -1, + "Added trip should not be found in time table for service date" + ); + assertEquals( + RealTimeState.CANCELED, + forToday.getTripTimes(forTodayAddedTripIndex).getRealTimeState() + ); + + final int scheduleTripIndex = schedule.getTripIndex(addedTripId); + assertEquals(-1, scheduleTripIndex, "Added trip should not be found in scheduled time table"); + } + + @Test + public void deletingAddedTrip() { + var builder = new TripUpdateBuilder( + addedTripId, + SERVICE_DATE, + ADDED, + transitModel.getTimeZone() + ); + + builder.addStopTime("A", 30).addStopTime("C", 40).addStopTime("E", 55); + + var tripUpdate = builder.build(); + + var updater = defaultUpdater(); + + // WHEN + updater.applyTripUpdates( + TRIP_MATCHER_NOOP, + REQUIRED_NO_DATA, + fullDataset, + List.of(tripUpdate), + feedId + ); + + // THEN + assertAddedTrip(SERVICE_DATE, this.addedTripId, updater, true); + + builder = new TripUpdateBuilder( + addedTripId, + SERVICE_DATE, + ADDED, + transitModel.getTimeZone() + ); + + var tripDescriptorBuilder = TripDescriptor.newBuilder(); + tripDescriptorBuilder.setTripId(addedTripId); + tripDescriptorBuilder.setScheduleRelationship(ScheduleRelationship.DELETED); + + tripDescriptorBuilder.setStartDate(ServiceDateUtils.asCompactString(SERVICE_DATE)); + tripUpdate = TripUpdate.newBuilder().setTrip(tripDescriptorBuilder).build(); + + // WHEN + updater.applyTripUpdates( + TRIP_MATCHER_NOOP, + REQUIRED_NO_DATA, + fullDataset, + List.of(tripUpdate), + feedId + ); + + // THEN + // Get trip pattern of last (most recently added) outgoing edge + var snapshot = updater.getTimetableSnapshot(true); + var stopA = transitModel.getStopModel().getRegularStop(new FeedScopedId(feedId, "A")); + var patternsAtA = snapshot.getPatternsForStop(stopA); + + assertNotNull(patternsAtA, "Added trip pattern should be found"); + var tripPattern = patternsAtA.stream().findFirst().get(); + + final Timetable forToday = snapshot.resolve(tripPattern, SERVICE_DATE); + final Timetable schedule = snapshot.resolve(tripPattern, null); + + assertNotSame(forToday, schedule); + + final int forTodayAddedTripIndex = forToday.getTripIndex(addedTripId); + assertTrue( + forTodayAddedTripIndex > -1, + "Added trip should not be found in time table for service date" + ); + assertEquals( + RealTimeState.DELETED, + forToday.getTripTimes(forTodayAddedTripIndex).getRealTimeState() + ); + + final int scheduleTripIndex = schedule.getTripIndex(addedTripId); + assertEquals(-1, scheduleTripIndex, "Added trip should not be found in scheduled time table"); + } } @Nonnull From dfe08afe9b1ad51e0f238b5e73caedea1360f1e5 Mon Sep 17 00:00:00 2001 From: Joel Lappalainen Date: Thu, 14 Mar 2024 11:51:46 +0200 Subject: [PATCH 6/7] Fix formatting --- .../updater/trip/TimetableSnapshotSourceTest.java | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-) diff --git a/src/test/java/org/opentripplanner/updater/trip/TimetableSnapshotSourceTest.java b/src/test/java/org/opentripplanner/updater/trip/TimetableSnapshotSourceTest.java index 4936b522d87..bf6759e8969 100644 --- a/src/test/java/org/opentripplanner/updater/trip/TimetableSnapshotSourceTest.java +++ b/src/test/java/org/opentripplanner/updater/trip/TimetableSnapshotSourceTest.java @@ -1202,12 +1202,7 @@ public void cancelingAddedTrip() { // THEN assertAddedTrip(SERVICE_DATE, this.addedTripId, updater, true); - builder = new TripUpdateBuilder( - addedTripId, - SERVICE_DATE, - ADDED, - transitModel.getTimeZone() - ); + builder = new TripUpdateBuilder(addedTripId, SERVICE_DATE, ADDED, transitModel.getTimeZone()); var tripDescriptorBuilder = TripDescriptor.newBuilder(); tripDescriptorBuilder.setTripId(addedTripId); @@ -1280,12 +1275,7 @@ public void deletingAddedTrip() { // THEN assertAddedTrip(SERVICE_DATE, this.addedTripId, updater, true); - builder = new TripUpdateBuilder( - addedTripId, - SERVICE_DATE, - ADDED, - transitModel.getTimeZone() - ); + builder = new TripUpdateBuilder(addedTripId, SERVICE_DATE, ADDED, transitModel.getTimeZone()); var tripDescriptorBuilder = TripDescriptor.newBuilder(); tripDescriptorBuilder.setTripId(addedTripId); From 7cdbe3d35978ea05b545e031c86642f8854ed247 Mon Sep 17 00:00:00 2001 From: Joel Lappalainen Date: Thu, 14 Mar 2024 14:02:07 +0200 Subject: [PATCH 7/7] Use parametrized test --- .../trip/TimetableSnapshotSourceTest.java | 96 ++++--------------- 1 file changed, 17 insertions(+), 79 deletions(-) diff --git a/src/test/java/org/opentripplanner/updater/trip/TimetableSnapshotSourceTest.java b/src/test/java/org/opentripplanner/updater/trip/TimetableSnapshotSourceTest.java index bf6759e8969..ec46cd5b93b 100644 --- a/src/test/java/org/opentripplanner/updater/trip/TimetableSnapshotSourceTest.java +++ b/src/test/java/org/opentripplanner/updater/trip/TimetableSnapshotSourceTest.java @@ -1,6 +1,8 @@ package org.opentripplanner.updater.trip; import static com.google.transit.realtime.GtfsRealtime.TripDescriptor.ScheduleRelationship.ADDED; +import static com.google.transit.realtime.GtfsRealtime.TripDescriptor.ScheduleRelationship.CANCELED; +import static com.google.transit.realtime.GtfsRealtime.TripDescriptor.ScheduleRelationship.DELETED; import static com.google.transit.realtime.GtfsRealtime.TripDescriptor.ScheduleRelationship.SCHEDULED; import static com.google.transit.realtime.GtfsRealtime.TripUpdate.StopTimeUpdate.ScheduleRelationship.SKIPPED; import static org.junit.jupiter.api.Assertions.assertEquals; @@ -33,6 +35,7 @@ import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; import org.opentripplanner.ConstantsForTests; import org.opentripplanner.TestOtpModel; import org.opentripplanner.framework.i18n.NonLocalizedString; @@ -1173,83 +1176,21 @@ public void repeatedlyAddedTripWithNewRoute() { assertNotNull(transitModel.getTransitModelIndex().getRouteForId(firstRoute.getId())); } - @Test - public void cancelingAddedTrip() { - // TODO we might want to change the behaviour so that only the trip without pattern is - // persisted if the added trip is cancelled - var builder = new TripUpdateBuilder( - addedTripId, - SERVICE_DATE, - ADDED, - transitModel.getTimeZone() - ); - - builder.addStopTime("A", 30).addStopTime("C", 40).addStopTime("E", 55); - - var tripUpdate = builder.build(); - - var updater = defaultUpdater(); - - // WHEN - updater.applyTripUpdates( - TRIP_MATCHER_NOOP, - REQUIRED_NO_DATA, - fullDataset, - List.of(tripUpdate), - feedId - ); - - // THEN - assertAddedTrip(SERVICE_DATE, this.addedTripId, updater, true); - - builder = new TripUpdateBuilder(addedTripId, SERVICE_DATE, ADDED, transitModel.getTimeZone()); - - var tripDescriptorBuilder = TripDescriptor.newBuilder(); - tripDescriptorBuilder.setTripId(addedTripId); - tripDescriptorBuilder.setScheduleRelationship(ScheduleRelationship.CANCELED); - - tripDescriptorBuilder.setStartDate(ServiceDateUtils.asCompactString(SERVICE_DATE)); - tripUpdate = TripUpdate.newBuilder().setTrip(tripDescriptorBuilder).build(); - - // WHEN - updater.applyTripUpdates( - TRIP_MATCHER_NOOP, - REQUIRED_NO_DATA, - fullDataset, - List.of(tripUpdate), - feedId - ); - - // THEN - // Get trip pattern of last (most recently added) outgoing edge - var snapshot = updater.getTimetableSnapshot(true); - var stopA = transitModel.getStopModel().getRegularStop(new FeedScopedId(feedId, "A")); - var patternsAtA = snapshot.getPatternsForStop(stopA); - - assertNotNull(patternsAtA, "Added trip pattern should be found"); - var tripPattern = patternsAtA.stream().findFirst().get(); - - final Timetable forToday = snapshot.resolve(tripPattern, SERVICE_DATE); - final Timetable schedule = snapshot.resolve(tripPattern, null); - - assertNotSame(forToday, schedule); - - final int forTodayAddedTripIndex = forToday.getTripIndex(addedTripId); - assertTrue( - forTodayAddedTripIndex > -1, - "Added trip should not be found in time table for service date" - ); - assertEquals( - RealTimeState.CANCELED, - forToday.getTripTimes(forTodayAddedTripIndex).getRealTimeState() + static List addedRemovalTestCase() { + return List.of( + // TODO we might want to change the behaviour so that only the trip without pattern is + // persisted if the added trip is cancelled + Arguments.of(CANCELED, RealTimeState.CANCELED), + Arguments.of(DELETED, RealTimeState.DELETED) ); - - final int scheduleTripIndex = schedule.getTripIndex(addedTripId); - assertEquals(-1, scheduleTripIndex, "Added trip should not be found in scheduled time table"); } - @Test - public void deletingAddedTrip() { + @ParameterizedTest + @MethodSource("addedRemovalTestCase") + public void cancelingAddedTrip( + ScheduleRelationship scheduleRelationship, + RealTimeState expectedState + ) { var builder = new TripUpdateBuilder( addedTripId, SERVICE_DATE, @@ -1279,7 +1220,7 @@ public void deletingAddedTrip() { var tripDescriptorBuilder = TripDescriptor.newBuilder(); tripDescriptorBuilder.setTripId(addedTripId); - tripDescriptorBuilder.setScheduleRelationship(ScheduleRelationship.DELETED); + tripDescriptorBuilder.setScheduleRelationship(scheduleRelationship); tripDescriptorBuilder.setStartDate(ServiceDateUtils.asCompactString(SERVICE_DATE)); tripUpdate = TripUpdate.newBuilder().setTrip(tripDescriptorBuilder).build(); @@ -1312,10 +1253,7 @@ public void deletingAddedTrip() { forTodayAddedTripIndex > -1, "Added trip should not be found in time table for service date" ); - assertEquals( - RealTimeState.DELETED, - forToday.getTripTimes(forTodayAddedTripIndex).getRealTimeState() - ); + assertEquals(expectedState, forToday.getTripTimes(forTodayAddedTripIndex).getRealTimeState()); final int scheduleTripIndex = schedule.getTripIndex(addedTripId); assertEquals(-1, scheduleTripIndex, "Added trip should not be found in scheduled time table");