Merge pull request #5525 from entur/otp2_cleanup_trip_times_part_B

Cleanup trip times - part B

src/ext-test/java/org/opentripplanner/ext/realtimeresolver/RealtimeResolverTest.java

@@ -137,7 +137,7 @@ private static TripPattern delay(TripPattern pattern1, int seconds) {
   }
 
   private static TripTimes delay(TripTimes tt, int seconds) {
-    var delayed = tt.copyOfScheduledTimes();
+    var delayed = tt.copyScheduledTimes();
     IntStream
       .range(0, delayed.getNumStops())
       .forEach(i -> {

src/ext-test/java/org/opentripplanner/ext/siri/TimetableHelperTest.java

@@ -19,8 +19,8 @@
 import org.opentripplanner.transit.model.site.RegularStop;
 import org.opentripplanner.transit.model.site.Station;
 import org.opentripplanner.transit.model.timetable.OccupancyStatus;
+import org.opentripplanner.transit.model.timetable.RealTimeTripTimes;
 import org.opentripplanner.transit.model.timetable.Trip;
-import org.opentripplanner.transit.model.timetable.TripTimes;
 import org.opentripplanner.transit.model.timetable.TripTimesFactory;
 import org.opentripplanner.transit.service.StopModel;
 import uk.org.siri.siri20.OccupancyEnumeration;
@@ -46,7 +46,7 @@ public class TimetableHelperTest {
     ZoneIds.CET
   );
 
-  private TripTimes tripTimes;
+  private RealTimeTripTimes tripTimes;
 
   @BeforeEach
   public void setUp() {

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

@@ -18,6 +18,7 @@
 import org.opentripplanner.framework.time.ServiceDateUtils;
 import org.opentripplanner.model.StopTime;
 import org.opentripplanner.transit.model.basic.TransitMode;
+import org.opentripplanner.transit.model.framework.DataValidationException;
 import org.opentripplanner.transit.model.framework.FeedScopedId;
 import org.opentripplanner.transit.model.framework.Result;
 import org.opentripplanner.transit.model.network.Route;
@@ -27,11 +28,11 @@
 import org.opentripplanner.transit.model.organization.Operator;
 import org.opentripplanner.transit.model.site.RegularStop;
 import org.opentripplanner.transit.model.timetable.RealTimeState;
+import org.opentripplanner.transit.model.timetable.RealTimeTripTimes;
 import org.opentripplanner.transit.model.timetable.Trip;
-import org.opentripplanner.transit.model.timetable.TripTimes;
 import org.opentripplanner.transit.model.timetable.TripTimesFactory;
 import org.opentripplanner.transit.service.TransitModel;
-import org.opentripplanner.updater.spi.TripTimesValidationMapper;
+import org.opentripplanner.updater.spi.DataValidationExceptionMapper;
 import org.opentripplanner.updater.spi.UpdateError;
 import org.rutebanken.netex.model.BusSubmodeEnumeration;
 import org.rutebanken.netex.model.RailSubmodeEnumeration;
@@ -202,14 +203,14 @@ Result<TripUpdate, UpdateError> build() {
       .withStopPattern(stopPattern)
       .build();
 
-    TripTimes tripTimes = TripTimesFactory.tripTimes(
+    RealTimeTripTimes tripTimes = TripTimesFactory.tripTimes(
       trip,
       aimedStopTimes,
       transitModel.getDeduplicator()
     );
     tripTimes.setServiceCode(transitModel.getServiceCodes().get(trip.getServiceId()));
     pattern.add(tripTimes);
-    TripTimes updatedTripTimes = tripTimes.copyOfScheduledTimes();
+    RealTimeTripTimes updatedTripTimes = tripTimes.copyScheduledTimes();
 
     // Loop through calls again and apply updates
     for (int stopSequence = 0; stopSequence < calls.size(); stopSequence++) {
@@ -231,9 +232,10 @@ Result<TripUpdate, UpdateError> build() {
     }
 
     /* Validate */
-    var validityResult = updatedTripTimes.validateNonIncreasingTimes();
-    if (validityResult.isPresent()) {
-      return TripTimesValidationMapper.toResult(tripId, validityResult.get());
+    try {
+      updatedTripTimes.validateNonIncreasingTimes();
+    } catch (DataValidationException e) {
+      return DataValidationExceptionMapper.toResult(e);
     }
 
     // Adding trip to index necessary to include values in graphql-queries

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

@@ -12,15 +12,16 @@
 import java.util.Set;
 import org.opentripplanner.ext.siri.mapper.PickDropMapper;
 import org.opentripplanner.framework.time.ServiceDateUtils;
-import org.opentripplanner.transit.model.framework.FeedScopedId;
+import org.opentripplanner.transit.model.framework.DataValidationException;
 import org.opentripplanner.transit.model.framework.Result;
 import org.opentripplanner.transit.model.network.StopPattern;
 import org.opentripplanner.transit.model.network.TripPattern;
 import org.opentripplanner.transit.model.site.RegularStop;
 import org.opentripplanner.transit.model.site.StopLocation;
 import org.opentripplanner.transit.model.timetable.RealTimeState;
+import org.opentripplanner.transit.model.timetable.RealTimeTripTimes;
 import org.opentripplanner.transit.model.timetable.TripTimes;
-import org.opentripplanner.updater.spi.TripTimesValidationMapper;
+import org.opentripplanner.updater.spi.DataValidationExceptionMapper;
 import org.opentripplanner.updater.spi.UpdateError;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -33,7 +34,8 @@
  */
 public class ModifiedTripBuilder {
 
-  private static final Logger LOG = LoggerFactory.getLogger(TimetableHelper.class);
+  private static final Logger LOG = LoggerFactory.getLogger(ModifiedTripBuilder.class);
+
   private final TripTimes existingTripTimes;
   private final TripPattern pattern;
   private final LocalDate serviceDate;
@@ -94,7 +96,7 @@ public ModifiedTripBuilder(
    * in form the SIRI-ET update.
    */
   public Result<TripUpdate, UpdateError> build() {
-    TripTimes newTimes = existingTripTimes.copyOfScheduledTimes();
+    RealTimeTripTimes newTimes = existingTripTimes.copyScheduledTimes();
 
     StopPattern stopPattern = createStopPattern(pattern, calls, entityResolver);
 
@@ -114,24 +116,24 @@ public Result<TripUpdate, UpdateError> build() {
       newTimes.setRealTimeState(RealTimeState.MODIFIED);
     }
 
-    var error = newTimes.validateNonIncreasingTimes();
-    final FeedScopedId id = newTimes.getTrip().getId();
-    if (error.isPresent()) {
-      var updateError = error.get();
+    // TODO - Handle DataValidationException at the outemost level(pr trip)
+    try {
+      newTimes.validateNonIncreasingTimes();
+    } catch (DataValidationException e) {
       LOG.info(
-        "Invalid SIRI-ET data for trip {} - TripTimes are non-increasing after applying SIRI delay propagation at stop index {}",
-        id,
-        updateError.stopIndex()
+        "Invalid SIRI-ET data for trip {} - TripTimes failed to validate after applying SIRI delay propagation. {}",
+        newTimes.getTrip().getId(),
+        e.getMessage()
       );
-      return TripTimesValidationMapper.toResult(id, updateError);
+      return DataValidationExceptionMapper.toResult(e);
     }
 
     int numStopsInUpdate = newTimes.getNumStops();
     int numStopsInPattern = pattern.numberOfStops();
     if (numStopsInUpdate != numStopsInPattern) {
       LOG.info(
         "Invalid SIRI-ET data for trip {} - Inconsistent number of updated stops ({}) and stops in pattern ({})",
-        id,
+        newTimes.getTrip().getId(),
         numStopsInUpdate,
         numStopsInPattern
       );
@@ -145,7 +147,7 @@ public Result<TripUpdate, UpdateError> build() {
   /**
    * Applies real-time updates from the calls into newTimes.
    */
-  private void applyUpdates(TripTimes newTimes) {
+  private void applyUpdates(RealTimeTripTimes newTimes) {
     ZonedDateTime startOfService = ServiceDateUtils.asStartOfService(serviceDate, zoneId);
     Set<CallWrapper> alreadyVisited = new HashSet<>();
 

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

@@ -18,14 +18,17 @@
 import org.opentripplanner.model.TimetableSnapshot;
 import org.opentripplanner.model.TimetableSnapshotProvider;
 import org.opentripplanner.routing.algorithm.raptoradapter.transit.mappers.TransitLayerUpdater;
+import org.opentripplanner.transit.model.framework.DataValidationException;
 import org.opentripplanner.transit.model.framework.Result;
 import org.opentripplanner.transit.model.network.TripPattern;
+import org.opentripplanner.transit.model.timetable.RealTimeTripTimes;
 import org.opentripplanner.transit.model.timetable.Trip;
 import org.opentripplanner.transit.model.timetable.TripTimes;
 import org.opentripplanner.transit.service.DefaultTransitService;
 import org.opentripplanner.transit.service.TransitModel;
 import org.opentripplanner.transit.service.TransitService;
 import org.opentripplanner.updater.TimetableSnapshotSourceParameters;
+import org.opentripplanner.updater.spi.DataValidationExceptionMapper;
 import org.opentripplanner.updater.spi.UpdateError;
 import org.opentripplanner.updater.spi.UpdateResult;
 import org.opentripplanner.updater.spi.UpdateSuccess;
@@ -215,6 +218,8 @@ private Result<UpdateSuccess, UpdateError> apply(
 
       /* commit */
       return addTripToGraphAndBuffer(result.successValue());
+    } catch (DataValidationException e) {
+      return DataValidationExceptionMapper.toResult(e);
     } catch (Exception e) {
       LOG.warn(
         "{} EstimatedJourney {} failed.",
@@ -395,7 +400,7 @@ private boolean markScheduledTripAsDeleted(Trip trip, final LocalDate serviceDat
       if (tripTimes == null) {
         LOG.warn("Could not mark scheduled trip as deleted {}", trip.getId());
       } else {
-        final TripTimes newTripTimes = tripTimes.copyOfScheduledTimes();
+        final RealTimeTripTimes newTripTimes = tripTimes.copyScheduledTimes();
         newTripTimes.deleteTrip();
         buffer.update(pattern, newTripTimes, serviceDate);
         success = true;

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

@@ -7,7 +7,7 @@
 import org.opentripplanner.ext.siri.mapper.OccupancyMapper;
 import org.opentripplanner.framework.i18n.NonLocalizedString;
 import org.opentripplanner.framework.time.ServiceDateUtils;
-import org.opentripplanner.transit.model.timetable.TripTimes;
+import org.opentripplanner.transit.model.timetable.RealTimeTripTimes;
 import uk.org.siri.siri20.NaturalLanguageStringStructure;
 import uk.org.siri.siri20.OccupancyEnumeration;
 
@@ -52,7 +52,7 @@ private static int handleMissingRealtime(int... times) {
 
   public static void applyUpdates(
     ZonedDateTime departureDate,
-    TripTimes tripTimes,
+    RealTimeTripTimes tripTimes,
     int index,
     boolean isLastStop,
     boolean isJourneyPredictionInaccurate,

src/main/java/org/opentripplanner/api/mapping/TripPlanMapper.java

@@ -21,7 +21,7 @@ public ApiTripPlan mapTripPlan(TripPlan domain) {
     }
     ApiTripPlan api = new ApiTripPlan();
     api.date = Date.from(domain.date);
-    // The origin/destination do not have arrival/depature times; Hence {@code null} is used.
+    // The origin/destination do not have arrival/departure times; Hence {@code null} is used.
     api.from = placeMapper.mapPlace(domain.from, null, null, null, null);
     api.to = placeMapper.mapPlace(domain.to, null, null, null, null);
     api.itineraries = itineraryMapper.mapItineraries(domain.itineraries);

src/main/java/org/opentripplanner/framework/error/OtpError.java

@@ -0,0 +1,37 @@
+package org.opentripplanner.framework.error;
+
+import java.util.Locale;
+
+/**
+ * A generic representation of an error. The error should have a code used to group the same
+ * type of errors together. To avoid filling up memory with error strings during graph build
+ * we store errors in memory "decomposed". The {@link #messageTemplate()} and
+ * {@link #messageArguments()} is used to construct the message. Use the {@link Locale#ROOT}
+ * when constructing the message - we only support english with SI formatting.
+ */
+public interface OtpError {
+  /**
+   * An error code used to identify the error type. This is NOT an enum, but feel free
+   * to use an inum in the implementation, then use the enum NAME as the code or
+   * enum TYPE:NAME. Then name should be unique within OTP.
+   */
+  String errorCode();
+
+  /**
+   * The string template used to create a human-readable error message. Use the
+   * {@link String#format(Locale, String, Object...)} format.
+   */
+  String messageTemplate();
+
+  /**
+   * The arguments to inject into the message.
+   */
+  Object[] messageArguments();
+
+  /**
+   * Construct a message.
+   */
+  default String message() {
+    return String.format(Locale.ROOT, messageTemplate(), messageArguments());
+  }
+}

src/main/java/org/opentripplanner/graph_builder/issue/api/DataImportIssueStore.java

@@ -1,6 +1,7 @@
 package org.opentripplanner.graph_builder.issue.api;
 
 import java.util.List;
+import org.opentripplanner.framework.error.OtpError;
 
 /**
  * This service is used to store issued during data import. When the import is complete
@@ -19,6 +20,9 @@ public interface DataImportIssueStore {
   /** Add an issue to the issue report. */
   void add(DataImportIssue issue);
 
+  /** Add an issue to the issue report. */
+  void add(OtpError issue);
+
   /** Add an issue to the issue report without the need of creating an issue class. */
   void add(String type, String message);
 

src/main/java/org/opentripplanner/graph_builder/issue/api/NoopDataImportIssueStore.java

@@ -1,6 +1,7 @@
 package org.opentripplanner.graph_builder.issue.api;
 
 import java.util.List;
+import org.opentripplanner.framework.error.OtpError;
 
 /**
  * A no-op implementation of the issue store, convenient for unit testing. No issues are
@@ -11,6 +12,9 @@ class NoopDataImportIssueStore implements DataImportIssueStore {
   @Override
   public void add(DataImportIssue issue) {}
 
+  @Override
+  public void add(OtpError issue) {}
+
   @Override
   public void add(String type, String message) {}
 

src/main/java/org/opentripplanner/graph_builder/issue/service/DefaultDataImportIssueStore.java

@@ -3,6 +3,7 @@
 import jakarta.inject.Singleton;
 import java.util.ArrayList;
 import java.util.List;
+import org.opentripplanner.framework.error.OtpError;
 import org.opentripplanner.graph_builder.issue.api.DataImportIssue;
 import org.opentripplanner.graph_builder.issue.api.DataImportIssueStore;
 import org.opentripplanner.graph_builder.issue.api.Issue;
@@ -30,6 +31,11 @@ public void add(DataImportIssue issue) {
     }
   }
 
+  @Override
+  public void add(OtpError issue) {
+    add(issue.errorCode(), issue.messageTemplate(), issue.messageArguments());
+  }
+
   @Override
   public void add(String type, String message) {
     add(Issue.issue(type, message));

src/main/java/org/opentripplanner/gtfs/GenerateTripPatternsOperation.java

@@ -9,13 +9,15 @@
 import java.util.Map;
 import java.util.Set;
 import org.opentripplanner.ext.flex.trip.FlexTrip;
+import org.opentripplanner.framework.logging.ProgressTracker;
 import org.opentripplanner.graph_builder.issue.api.DataImportIssueStore;
 import org.opentripplanner.graph_builder.issues.TripDegenerate;
 import org.opentripplanner.graph_builder.issues.TripUndefinedService;
 import org.opentripplanner.graph_builder.module.geometry.GeometryProcessor;
 import org.opentripplanner.model.Frequency;
 import org.opentripplanner.model.StopTime;
 import org.opentripplanner.model.impl.OtpTransitServiceBuilder;
+import org.opentripplanner.transit.model.framework.DataValidationException;
 import org.opentripplanner.transit.model.framework.Deduplicator;
 import org.opentripplanner.transit.model.framework.FeedScopedId;
 import org.opentripplanner.transit.model.network.Route;
@@ -47,7 +49,6 @@ public class GenerateTripPatternsOperation {
   private final Multimap<StopPattern, TripPattern> tripPatterns;
   private final ListMultimap<Trip, Frequency> frequenciesForTrip = ArrayListMultimap.create();
 
-  private int tripCount = 0;
   private int freqCount = 0;
   private int scheduledCount = 0;
 
@@ -70,17 +71,21 @@ public void run() {
     collectFrequencyByTrip();
 
     final Collection<Trip> trips = transitDaoBuilder.getTripsById().values();
-    final int tripsSize = trips.size();
+    var progressLogger = ProgressTracker.track("build trip patterns", 50_000, trips.size());
+    LOG.info(progressLogger.startMessage());
 
     /* Loop over all trips, handling each one as a frequency-based or scheduled trip. */
     for (Trip trip : trips) {
-      if (++tripCount % 100000 == 0) {
-        LOG.debug("build trip patterns {}/{}", tripCount, tripsSize);
+      try {
+        buildTripPatternForTrip(trip);
+        //noinspection Convert2MethodRef
+        progressLogger.step(m -> LOG.info(m));
+      } catch (DataValidationException e) {
+        issueStore.add(e.error());
       }
-
-      buildTripPatternForTrip(trip);
     }
 
+    LOG.info(progressLogger.completeMessage());
     LOG.info(
       "Added {} frequency-based and {} single-trip timetable entries.",
       freqCount,