diff --git a/src/main/java/org/opentripplanner/model/TimetableSnapshot.java b/src/main/java/org/opentripplanner/model/TimetableSnapshot.java index 5c018412572..2499264d8c8 100644 --- a/src/main/java/org/opentripplanner/model/TimetableSnapshot.java +++ b/src/main/java/org/opentripplanner/model/TimetableSnapshot.java @@ -1,6 +1,7 @@ package org.opentripplanner.model; import com.google.common.collect.HashMultimap; +import com.google.common.collect.ImmutableSetMultimap; import com.google.common.collect.SetMultimap; import java.time.LocalDate; import java.util.Collection; @@ -9,6 +10,7 @@ import java.util.HashMap; import java.util.HashSet; import java.util.Iterator; +import java.util.Map; import java.util.Map.Entry; import java.util.Objects; import java.util.Set; @@ -80,8 +82,7 @@ public class TimetableSnapshot { * include ones from the scheduled GTFS, as well as ones added by realtime messages and * tracked by the TripPatternCache.

* Note that the keys do not include all scheduled TripPatterns, only those for which we have at - * least one update. The type of the field is specifically HashMap (rather than the more general - * Map interface) because we need to efficiently clone it.

+ * least one update.

* The members of the SortedSet (the Timetable for a particular day) are treated as copy-on-write * when we're updating them. If an update will modify the timetable for a particular day, that * timetable is replicated before any modifications are applied to avoid affecting any previous @@ -91,16 +92,15 @@ public class TimetableSnapshot { * The compound key approach better reflects the fact that there should be only one Timetable per * TripPattern and date. */ - private HashMap> timetables = new HashMap(); + private Map> timetables = new HashMap<>(); /** * For cases where the trip pattern (sequence of stops visited) has been changed by a realtime * update, a Map associating the updated trip pattern with a compound key of the feed-scoped - * trip ID and the service date. The type of this field is HashMap rather than the more general - * Map interface because we need to efficiently clone it whenever we start building up a new - * snapshot. TODO RT_AB: clarify if this is an index or the original source of truth. + * trip ID and the service date. + * TODO RT_AB: clarify if this is an index or the original source of truth. */ - private HashMap realtimeAddedTripPattern = new HashMap<>(); + private Map realtimeAddedTripPattern = new HashMap<>(); /** * This is an index of TripPatterns, not the primary collection. It tracks which TripPatterns @@ -256,9 +256,8 @@ public TimetableSnapshot commit(TransitLayerUpdater transitLayerUpdater, boolean if (!force && !this.isDirty()) { return null; } - ret.timetables = (HashMap>) this.timetables.clone(); - ret.realtimeAddedTripPattern = - (HashMap) this.realtimeAddedTripPattern.clone(); + ret.timetables = Map.copyOf(timetables); + ret.realtimeAddedTripPattern = Map.copyOf(realtimeAddedTripPattern); if (transitLayerUpdater != null) { transitLayerUpdater.update(dirtyTimetables, timetables); @@ -267,7 +266,7 @@ public TimetableSnapshot commit(TransitLayerUpdater transitLayerUpdater, boolean this.dirtyTimetables.clear(); this.dirty = false; - ret.setPatternsForStop(HashMultimap.create(this.patternsForStop)); + ret.patternsForStop = ImmutableSetMultimap.copyOf(patternsForStop); ret.readOnly = true; // mark the snapshot as henceforth immutable return ret; @@ -304,6 +303,10 @@ public void clear(String feedId) { * message and an attempt was made to re-associate it with its originally scheduled trip pattern. */ public boolean revertTripToScheduledTripPattern(FeedScopedId tripId, LocalDate serviceDate) { + if (readOnly) { + throw new ConcurrentModificationException("This TimetableSnapshot is read-only."); + } + boolean success = false; final TripPattern pattern = getRealtimeAddedTripPattern(tripId, serviceDate); @@ -407,10 +410,6 @@ public Collection getPatternsForStop(StopLocation stop) { return patternsForStop.get(stop); } - public void setPatternsForStop(SetMultimap patternsForStop) { - this.patternsForStop = patternsForStop; - } - /** * Does this snapshot contain any realtime data or is it completely empty? */ @@ -424,7 +423,7 @@ public boolean isEmpty() { * @param feedId feed id to clear out * @return true if the timetable changed as a result of the call */ - protected boolean clearTimetable(String feedId) { + private boolean clearTimetable(String feedId) { return timetables.keySet().removeIf(tripPattern -> feedId.equals(tripPattern.getFeedId())); } @@ -434,7 +433,7 @@ protected boolean clearTimetable(String feedId) { * @param feedId feed id to clear out * @return true if the realtimeAddedTripPattern changed as a result of the call */ - protected boolean clearRealtimeAddedTripPattern(String feedId) { + private boolean clearRealtimeAddedTripPattern(String feedId) { return realtimeAddedTripPattern .keySet() .removeIf(realtimeAddedTripPattern -> diff --git a/src/test/java/org/opentripplanner/model/TimetableSnapshotTest.java b/src/test/java/org/opentripplanner/model/TimetableSnapshotTest.java index 26d93f6a848..89c30da79e4 100644 --- a/src/test/java/org/opentripplanner/model/TimetableSnapshotTest.java +++ b/src/test/java/org/opentripplanner/model/TimetableSnapshotTest.java @@ -5,6 +5,7 @@ import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNotSame; import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import com.google.transit.realtime.GtfsRealtime.TripDescriptor; @@ -261,6 +262,52 @@ public void testPurge() { assertFalse(resolver.isDirty()); } + @Test + void testCannotUpdateReadOnlyTimetableSnapshot() { + TimetableSnapshot committedSnapshot = createCommittedSnapshot(); + LocalDate today = LocalDate.now(timeZone); + TripPattern pattern = patternIndex.get(new FeedScopedId(feedId, "1.1")); + assertThrows( + ConcurrentModificationException.class, + () -> committedSnapshot.update(pattern, null, today) + ); + } + + @Test + void testCannotCommitReadOnlyTimetableSnapshot() { + TimetableSnapshot committedSnapshot = createCommittedSnapshot(); + assertThrows(ConcurrentModificationException.class, () -> committedSnapshot.commit(null, true)); + } + + @Test + void testCannotClearReadOnlyTimetableSnapshot() { + TimetableSnapshot committedSnapshot = createCommittedSnapshot(); + assertThrows(ConcurrentModificationException.class, () -> committedSnapshot.clear(null)); + } + + @Test + void testCannotPurgeReadOnlyTimetableSnapshot() { + TimetableSnapshot committedSnapshot = createCommittedSnapshot(); + assertThrows( + ConcurrentModificationException.class, + () -> committedSnapshot.purgeExpiredData(null) + ); + } + + @Test + void testCannotRevertReadOnlyTimetableSnapshot() { + TimetableSnapshot committedSnapshot = createCommittedSnapshot(); + assertThrows( + ConcurrentModificationException.class, + () -> committedSnapshot.revertTripToScheduledTripPattern(null, null) + ); + } + + private static TimetableSnapshot createCommittedSnapshot() { + TimetableSnapshot timetableSnapshot = new TimetableSnapshot(); + return timetableSnapshot.commit(null, true); + } + private Result updateResolver( TimetableSnapshot resolver, TripPattern pattern,