Merge pull request #5934 from entur/use_immutable_collections_in_time…

…table_snapshot

Enforce immutability of published timetable snapshot

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. <p>
    * 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. <p>
+   * least one update.<p>
    * 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<TripPattern, SortedSet<Timetable>> timetables = new HashMap();
+  private Map<TripPattern, SortedSet<Timetable>> 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<TripIdAndServiceDate, TripPattern> realtimeAddedTripPattern = new HashMap<>();
+  private Map<TripIdAndServiceDate, TripPattern> 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<TripPattern, SortedSet<Timetable>>) this.timetables.clone();
-    ret.realtimeAddedTripPattern =
-      (HashMap<TripIdAndServiceDate, TripPattern>) 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<TripPattern> getPatternsForStop(StopLocation stop) {
     return patternsForStop.get(stop);
   }
 
-  public void setPatternsForStop(SetMultimap<StopLocation, TripPattern> 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 ->

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<?, UpdateError> updateResolver(
     TimetableSnapshot resolver,
     TripPattern pattern,