Add and update Javadoc on realtime classes

src/ext/java/org/opentripplanner/ext/siri/SiriAlertsUpdateHandler.java

@@ -35,20 +35,36 @@
 import uk.org.siri.siri20.WorkflowStatusEnumeration;
 
 /**
- * This updater applies the equivalent of GTFS Alerts, but from SIRI Situation Exchange feeds. NOTE
- * this cannot handle situations where there are multiple feeds with different IDs (for now it may
- * only work in single-feed regions).
+ * This updater applies the equivalent of GTFS Alerts, but from SIRI Situation Exchange (SX) feeds.
+ * As the incoming SIRI SX messages are mapped to internal TransitAlerts, their FeedScopedIds will
+ * be the single feed ID associated with this update handler, plus the situation number provided in
+ * the SIRI SX message.
+ * This class cannot handle situations where incoming messages are being applied to multiple static
+ * feeds with different IDs. For now it may only work in single-feed regions. A possible workaround
+ * is to assign the same feed ID to multiple static feeds where it is known that their entity IDs
+ * are all drawn from the same namespace (i.e. they are functionally fragments of the same feed).
+ * TODO RT_AB: Internal FeedScopedId creation strategy should probably be pluggable or configurable.
+ *   TG has indicated this is a necessary condition for moving this updater out of sandbox.
+ * TODO RT_AB: The name should be clarified, as there is no such thing as "SIRI Alerts", and it
+ *   is referencing the internal model concept of "Alerts" which are derived from GTFS terminology.
  */
 public class SiriAlertsUpdateHandler {
 
   private static final Logger LOG = LoggerFactory.getLogger(SiriAlertsUpdateHandler.class);
   private final String feedId;
   private final Set<TransitAlert> alerts = new HashSet<>();
   private final TransitAlertService transitAlertService;
-  /** How long before the posted start of an event it should be displayed to users */
   private final Duration earlyStart;
+
+  /**
+   * This takes the parts of the SIRI SX message saying which transit entities are affected and
+   * maps them to the corresponding OTP internal model entity or entities.
+   */
   private final AffectsMapper affectsMapper;
 
+  /**
+   * @param earlyStart display the alerts to users this long before their activePeriod begins
+   */
   public SiriAlertsUpdateHandler(
     String feedId,
     TransitModel transitModel,
@@ -90,7 +106,7 @@ public void update(ServiceDelivery delivery) {
           } else {
             TransitAlert alert = null;
             try {
-              alert = handleAlert(sxElement);
+              alert = mapSituationToAlert(sxElement);
               addedCounter++;
             } catch (Exception e) {
               LOG.info(
@@ -120,7 +136,12 @@ public void update(ServiceDelivery delivery) {
     }
   }
 
-  private TransitAlert handleAlert(PtSituationElement situation) {
+  /**
+   * Build an internal model Alert from an incoming SIRI situation exchange element.
+   * May return null if the header, description, and detail text are all empty or missing in the
+   * SIRI message. In all other cases it will return a valid TransitAlert instance.
+   */
+  private TransitAlert mapSituationToAlert(PtSituationElement situation) {
     TransitAlertBuilder alert = createAlertWithTexts(situation);
 
     if (
@@ -199,7 +220,10 @@ private long getEpochSecond(ZonedDateTime startTime) {
   }
 
   /*
-   * Creates alert from PtSituation with all textual content
+   * Creates a builder for an internal model TransitAlert. The builder is pre-filled with all
+   * textual content from the supplied SIRI PtSituation. The builder also has the feed scoped ID
+   * pre-set to the single feed ID associated with this update handler, plus the situation number
+   * provided in the SIRI PtSituation.
    */
   private TransitAlertBuilder createAlertWithTexts(PtSituationElement situation) {
     return TransitAlert

src/ext/java/org/opentripplanner/ext/siri/SiriTimetableSnapshotSource.java

@@ -133,10 +133,12 @@ public TimetableSnapshot getTimetableSnapshot() {
 
   /**
    * Method to apply a trip update list to the most recent version of the timetable snapshot.
+   * FIXME RT_AB: TripUpdate is the GTFS term, and these SIRI ETs are never converted into that
+   *              same internal model.
    *
    * @param fullDataset  true iff the list with updates represent all updates that are active right
    *                     now, i.e. all previous updates should be disregarded
-   * @param updates      SIRI VehicleMonitoringDeliveries that should be applied atomically
+   * @param updates      SIRI EstimatedTimetable deliveries that should be applied atomically.
    */
   public UpdateResult applyEstimatedTimetable(
     @Nullable SiriFuzzyTripMatcher fuzzyTripMatcher,
@@ -372,12 +374,9 @@ private Result<UpdateSuccess, UpdateError> addTripToGraphAndBuffer(TripUpdate tr
       trip,
       serviceDate
     );
-
-    // Add new trip times to the buffer and return success
+    // Add new trip times to buffer, making protective copies as needed. Bubble success/error up.
     var result = buffer.update(pattern, tripUpdate.tripTimes(), serviceDate);
-
     LOG.debug("Applied real-time data for trip {} on {}", trip, serviceDate);
-
     return result;
   }
 

src/ext/java/org/opentripplanner/ext/siri/SiriTripPatternCache.java

@@ -12,31 +12,63 @@
 import javax.annotation.Nonnull;
 import org.opentripplanner.transit.model.network.StopPattern;
 import org.opentripplanner.transit.model.network.TripPattern;
-import org.opentripplanner.transit.model.network.TripPatternBuilder;
 import org.opentripplanner.transit.model.site.RegularStop;
 import org.opentripplanner.transit.model.site.StopLocation;
 import org.opentripplanner.transit.model.timetable.Trip;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
 /**
- * A synchronized cache of trip patterns that are added to the graph due to GTFS-realtime messages.
+ * Threadsafe mechanism for tracking any TripPatterns added to the graph via SIRI realtime messages.
+ * This tracks only patterns added by realtime messages, not ones that already existed from the
+ * scheduled NeTEx. This is a "cache" in the sense that it will keep returning the same TripPattern
+ * when presented with the same StopPattern, so if realtime messages add many trips passing through
+ * the same sequence of stops, they will all end up on this same TripPattern.
+ * <p>
+ * Note that there are two versions of this class, this one for GTFS-RT and another for SIRI.
+ * See additional comments in the Javadoc of the GTFS-RT version of this class, whose name is
+ * simply TripPatternCache.
+ * TODO RT_AB: To the extent that double SIRI/GTFS implementations are kept, prefix all names
+ *             with GTFS or SIRI or NETEX rather than having no prefix on the GTFS versions.
+ *  TODO RT_TG: There is no clear strategy for what should be in the cache and the transit model and the flow
+ *             between them. The NeTEx and a GTFS version of this should be merged. Having NeTex and GTFS
+ *             specific indexes inside is ok. With the increased usage of DatedServiceJourneys, this should probably
+ *             be part of the main model - not a separate cashe. It is possible that this class works when it comes to
+ *             the thread-safety, but just by looking at a few lines of code I see problems - a strategy needs to be
+ *             analysed, designed and documented.
  */
 public class SiriTripPatternCache {
 
-  private static final Logger log = LoggerFactory.getLogger(SiriTripPatternCache.class);
+  private static final Logger LOG = LoggerFactory.getLogger(SiriTripPatternCache.class);
 
+  // TODO RT_AB: Improve documentation. This seems to be the primary collection of added
+  //   TripPatterns, with other collections serving as indexes. Similar to TripPatternCache.cache
+  //   in the GTFS version of this class, but with service date as part of the key.
   private final Map<StopPatternServiceDateKey, TripPattern> cache = new HashMap<>();
 
+  // TODO RT_AB: Improve documentation. This field appears to be an index that exists only in the
+  //   SIRI version of this class (i.e. this version and not the older TripPatternCache that
+  //   handles GTFS-RT). This index appears to be tailored for use by the Transmodel GraphQL APIs.
   private final ListMultimap<StopLocation, TripPattern> patternsForStop = Multimaps.synchronizedListMultimap(
     ArrayListMultimap.create()
   );
 
+  // TODO RT_AB: clarify name and add documentation to this field.
   private final Map<TripServiceDateKey, TripPattern> updatedTripPatternsForTripCache = new HashMap<>();
 
+  // TODO RT_AB: generalize this so we can generate IDs for SIRI or GTFS-RT sources.
   private final SiriTripPatternIdGenerator tripPatternIdGenerator;
+
   private final Function<Trip, TripPattern> getPatternForTrip;
 
+  /**
+   * TODO RT_AB: This class could potentially be reused for both SIRI and GTFS-RT, which may
+   *   involve injecting a different ID generator and pattern fetching method.
+   *
+   * @param getPatternForTrip SiriTripPatternCache needs only this one feature of TransitService, so we retain
+   *                          only this function reference to effectively narrow the interface. This should also facilitate
+   *                          testing.
+   */
   public SiriTripPatternCache(
     SiriTripPatternIdGenerator tripPatternIdGenerator,
     Function<Trip, TripPattern> getPatternForTrip
@@ -46,8 +78,13 @@ public SiriTripPatternCache(
   }
 
   /**
-   * Get cached trip pattern or create one if it doesn't exist yet. If a trip pattern is created,
-   * vertices and edges for this trip pattern are also created in the transitModel.
+   * Get cached trip pattern or create one if it doesn't exist yet.
+   *
+   * TODO RT_AB: Improve documentation and/or merge with GTFS version of this class.
+   *  This was clearly derived from a method from TripPatternCache. This is the only non-dead-code
+   *  public method on this class, and mirrors the only public method on the GTFS-RT version of
+   *  TripPatternCache. It also explains why this class is called a "cache". It allows reusing the
+   *  same TripPattern instance when many different trips are created or updated with the same pattern.
    *
    * @param stopPattern stop pattern to retrieve/create trip pattern
    * @param trip        Trip containing route of new trip pattern in case a new trip pattern will be
@@ -61,6 +98,9 @@ public synchronized TripPattern getOrCreateTripPattern(
   ) {
     TripPattern originalTripPattern = getPatternForTrip.apply(trip);
 
+    // TODO RT_AB: Verify implementation, which is different than the GTFS-RT version.
+    //   It can return a TripPattern from the scheduled data, but protective copies are handled in
+    //   TimetableSnapshot.update. Better document this aspect of the contract in this method's Javadoc.
     if (originalTripPattern.getStopPattern().equals(stopPattern)) {
       return originalTripPattern;
     }
@@ -72,56 +112,57 @@ public synchronized TripPattern getOrCreateTripPattern(
     // Create TripPattern if it doesn't exist yet
     if (tripPattern == null) {
       var id = tripPatternIdGenerator.generateUniqueTripPatternId(trip);
-      TripPatternBuilder tripPatternBuilder = TripPattern
-        .of(id)
-        .withRoute(trip.getRoute())
-        .withMode(trip.getMode())
-        .withNetexSubmode(trip.getNetexSubMode())
-        .withStopPattern(stopPattern);
-
-      // TODO - SIRI: Add pattern to transitModel index?
-
-      tripPatternBuilder.withCreatedByRealtimeUpdater(true);
-      tripPatternBuilder.withOriginalTripPattern(originalTripPattern);
-
-      tripPattern = tripPatternBuilder.build();
+      tripPattern =
+        TripPattern
+          .of(id)
+          .withRoute(trip.getRoute())
+          .withMode(trip.getMode())
+          .withNetexSubmode(trip.getNetexSubMode())
+          .withStopPattern(stopPattern)
+          .withCreatedByRealtimeUpdater(true)
+          .withOriginalTripPattern(originalTripPattern)
+          .build();
+      // TODO: Add pattern to transitModel index?
 
       // Add pattern to cache
       cache.put(key, tripPattern);
     }
 
-    /**
-     *
-     * When the StopPattern is first modified (e.g. change of platform), then updated (or vice versa), the stopPattern is altered, and
-     * the StopPattern-object for the different states will not be equal.
-     *
-     * This causes both tripPatterns to be added to all unchanged stops along the route, which again causes duplicate results
-     * in departureRow-searches (one departure for "updated", one for "modified").
-     *
-     * Full example:
-     *      Planned stops: Stop 1 - Platform 1, Stop 2 - Platform 1
-     *
-     *      StopPattern #rt1: "updated" stopPattern cached in 'patternsForStop':
-     *          - Stop 1, Platform 1
-     *          	- StopPattern #rt1
-     *          - Stop 2, Platform 1
-     *          	- StopPattern #rt1
-     *
-     *      "modified" stopPattern: Stop 1 - Platform 1, Stop 2 - Platform 2
-     *
-     *      StopPattern #rt2: "modified" stopPattern cached in 'patternsForStop' will then be:
-     *          - Stop 1, Platform 1
-     *          	- StopPattern #rt1, StopPattern #rt2
-     *          - Stop 2, Platform 1
-     *          	- StopPattern #rt1
-     *          - Stop 2, Platform 2
-     *          	- StopPattern #rt2
-     *
-     *
-     * Therefore, we must cleanup the duplicates by deleting the previously added (and thus outdated)
-     * tripPattern for all affected stops. In example above, "StopPattern #rt1" should be removed from all stops
-     *
-     */
+    /*
+     When the StopPattern is first modified (e.g. change of platform), then updated (or vice
+     versa), the stopPattern is altered, and the StopPattern-object for the different states will
+     not be equal.
+
+     This causes both tripPatterns to be added to all unchanged stops along the route, which again
+     causes duplicate results in departureRow-searches (one departure for "updated", one for
+     "modified").
+
+     Full example:
+          Planned stops: Stop 1 - Platform 1, Stop 2 - Platform 1
+
+          StopPattern #rt1: "updated" stopPattern cached in 'patternsForStop':
+              - Stop 1, Platform 1
+              	- StopPattern #rt1
+              - Stop 2, Platform 1
+              	- StopPattern #rt1
+
+          "modified" stopPattern: Stop 1 - Platform 1, Stop 2 - Platform 2
+
+          StopPattern #rt2: "modified" stopPattern cached in 'patternsForStop' will then be:
+              - Stop 1, Platform 1
+              	- StopPattern #rt1, StopPattern #rt2
+              - Stop 2, Platform 1
+              	- StopPattern #rt1
+              - Stop 2, Platform 2
+              	- StopPattern #rt2
+
+     Therefore, we must clean up the duplicates by deleting the previously added (and thus
+     outdated) tripPattern for all affected stops. In example above, "StopPattern #rt1" should be
+     removed from all stops.
+
+     TODO RT_AB: review why this particular case is handled in an ad-hoc manner. It seems like all
+       such indexes should be constantly rebuilt and versioned along with the TimetableSnapshot.
+    */
     TripServiceDateKey tripServiceDateKey = new TripServiceDateKey(trip, serviceDate);
     if (updatedTripPatternsForTripCache.containsKey(tripServiceDateKey)) {
       // Remove previously added TripPatterns for the trip currently being updated - if the stopPattern does not match
@@ -132,16 +173,14 @@ public synchronized TripPattern getOrCreateTripPattern(
         patternsForStop.values().removeAll(Arrays.asList(cachedTripPattern));
         int sizeAfter = patternsForStop.values().size();
 
-        log.debug(
+        LOG.debug(
           "Removed outdated TripPattern for {} stops in {} ms - tripId: {}",
           (sizeBefore - sizeAfter),
           (System.currentTimeMillis() - t1),
           trip.getId()
         );
-        /*
-                  TODO: Also remove previously updated - now outdated - TripPattern from cache ?
-                  cache.remove(new StopPatternServiceDateKey(cachedTripPattern.stopPattern, serviceDate));
-                */
+        // TODO: Also remove previously updated - now outdated - TripPattern from cache ?
+        // cache.remove(new StopPatternServiceDateKey(cachedTripPattern.stopPattern, serviceDate));
       }
     }
 
@@ -160,6 +199,7 @@ public synchronized TripPattern getOrCreateTripPattern(
 
   /**
    * Returns any new TripPatterns added by real time information for a given stop.
+   * TODO RT_AB: this appears to be currently unused. Perhaps remove it if the API has changed.
    *
    * @param stop the stop
    * @return list of TripPatterns created by real time sources for the stop.
@@ -169,6 +209,16 @@ public List<TripPattern> getAddedTripPatternsForStop(RegularStop stop) {
   }
 }
 
+// TODO RT_AB: move the below classes inside the above class as private static inner classes.
+//   Defining these additional classes in the same top-level class file is unconventional.
+
+/**
+ * Serves as the key for the collection of TripPatterns added by realtime messages.
+ * Must define hashcode and equals to confer semantic identity.
+ * TODO RT_AB: clarify why each date has a different TripPattern instead of a different Timetable.
+ *   It seems like there's a separate TripPattern instance for each StopPattern and service date,
+ *   rather a single TripPattern instance associated with a separate timetable for each date.
+ */
 class StopPatternServiceDateKey {
 
   StopPattern stopPattern;
@@ -194,6 +244,11 @@ public boolean equals(Object thatObject) {
   }
 }
 
+/**
+ * An alternative key for looking up realtime-added TripPatterns by trip and service date instead
+ * of stop pattern and service date. Must define hashcode and equals to confer semantic identity.
+ * TODO RT_AB: verify whether one map is considered the definitive collection and the other an index.
+ */
 class TripServiceDateKey {
 
   Trip trip;

src/ext/java/org/opentripplanner/ext/siri/SiriTripPatternIdGenerator.java

@@ -8,20 +8,20 @@
 import org.opentripplanner.transit.model.timetable.Trip;
 
 /**
- * This class generate a new id for new TripPatterns created real-time by the SIRI updaters. It is
- * important to creat only on instance of this class, and inject it where it is needed.
- * <p>
- * The id generation is thread-safe, even if that is probably not needed.
+ * This class generates new unique IDs for TripPatterns created in response to real-time updates
+ * from the SIRI updaters. In non-test usage it is important to create only one instance of this
+ * class, and inject that single instance wherever it is needed. However, this single-instance
+ * usage pattern is not enforced due to differing needs in tests.
+ * The ID generation is threadsafe, even if that is probably not needed.
  */
 class SiriTripPatternIdGenerator {
 
   private final AtomicInteger counter = new AtomicInteger(0);
 
   /**
-   * Generate unique trip pattern code for real-time added trip pattern. This function roughly
-   * follows the format of {@link GenerateTripPatternsOperation}.
-   * <p>
-   * The generator add a postfix 'RT' to indicate that this trip pattern is generated at REAL-TIME.
+   * Generate a unique ID for a trip pattern added in response to a realtime message. This function
+   * roughly follows the format of {@link GenerateTripPatternsOperation}. The generator suffixes the
+   * ID with 'RT' to indicate that this trip pattern is generated in response to a realtime message.
    */
   FeedScopedId generateUniqueTripPatternId(Trip trip) {
     Route route = trip.getRoute();

src/ext/java/org/opentripplanner/ext/siri/mapper/AffectsMapper.java

@@ -33,6 +33,10 @@
 
 /**
  * Maps a {@link AffectsScopeStructure} to a list of {@link EntitySelector}s
+ *
+ * Concretely: this takes the parts of the SIRI SX (Alerts) message describing which transit
+ * entities are concerned by the alert, and maps them to EntitySelectors, which can match multiple
+ * OTP internal model entities that should be associated with the message.
  */
 public class AffectsMapper {