Skip to content

Commit

Permalink
refactor: Use DataValidationException to signal TripTime errors.
Browse files Browse the repository at this point in the history
  • Loading branch information
t2gran committed Oct 16, 2023
1 parent 3be3d5e commit e2259be
Show file tree
Hide file tree
Showing 23 changed files with 320 additions and 162 deletions.
10 changes: 6 additions & 4 deletions src/ext/java/org/opentripplanner/ext/siri/AddedTripBuilder.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -31,7 +32,7 @@
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;
Expand Down Expand Up @@ -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
Expand Down
22 changes: 11 additions & 11 deletions src/ext/java/org/opentripplanner/ext/siri/ModifiedTripBuilder.java
Original file line number Diff line number Diff line change
Expand Up @@ -12,15 +12,15 @@
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.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;
Expand Down Expand Up @@ -114,24 +114,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
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
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.Trip;
Expand All @@ -26,6 +27,7 @@
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;
Expand Down Expand Up @@ -219,6 +221,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.",
Expand Down
37 changes: 37 additions & 0 deletions src/main/java/org/opentripplanner/framework/error/OtpError.java
Original file line number Diff line number Diff line change
@@ -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());
}
}
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -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);

Expand Down
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -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) {}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;

Expand All @@ -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,
Expand Down
15 changes: 6 additions & 9 deletions src/main/java/org/opentripplanner/model/Timetable.java
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import javax.annotation.Nonnull;
import javax.annotation.Nullable;
import org.opentripplanner.framework.time.ServiceDateUtils;
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.TripPattern;
Expand All @@ -33,7 +34,7 @@
import org.opentripplanner.transit.model.timetable.Trip;
import org.opentripplanner.transit.model.timetable.TripTimes;
import org.opentripplanner.updater.GtfsRealtimeMapper;
import org.opentripplanner.updater.spi.TripTimesValidationMapper;
import org.opentripplanner.updater.spi.DataValidationExceptionMapper;
import org.opentripplanner.updater.spi.UpdateError;
import org.opentripplanner.updater.trip.BackwardsDelayPropagationType;
import org.slf4j.Logger;
Expand Down Expand Up @@ -344,14 +345,10 @@ public Result<TripTimesPatch, UpdateError> createUpdatedTripTimes(
}
}

var error = newTimes.validateNonIncreasingTimes();
if (error.isPresent()) {
LOG.debug(
"TripTimes are non-increasing after applying GTFS-RT delay propagation to trip {} after stop index {}.",
tripId,
error.get().stopIndex()
);
return TripTimesValidationMapper.toResult(newTimes.getTrip().getId(), error.get());
try {
newTimes.validateNonIncreasingTimes();
} catch (DataValidationException e) {
return DataValidationExceptionMapper.toResult(e);
}

if (tripUpdate.hasVehicle()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
import org.opentripplanner.netex.mapping.support.FeedScopedIdFactory;
import org.opentripplanner.transit.model.basic.SubMode;
import org.opentripplanner.transit.model.basic.TransitMode;
import org.opentripplanner.transit.model.framework.DataValidationException;
import org.opentripplanner.transit.model.framework.Deduplicator;
import org.opentripplanner.transit.model.framework.EntityById;
import org.opentripplanner.transit.model.framework.FeedScopedId;
Expand Down Expand Up @@ -366,12 +367,16 @@ private void createTripTimes(List<Trip> trips, TripPattern tripPattern) {
trip.getId()
);
} else {
TripTimes tripTimes = TripTimesFactory.tripTimes(
trip,
result.tripStopTimes.get(trip),
deduplicator
);
tripPattern.add(tripTimes);
try {
TripTimes tripTimes = TripTimesFactory.tripTimes(
trip,
result.tripStopTimes.get(trip),
deduplicator
);
tripPattern.add(tripTimes);
} catch (DataValidationException e) {
issueStore.add(e.error());
}
}
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
package org.opentripplanner.transit.model.framework;

import org.opentripplanner.framework.error.OtpError;

/**
* This class is used to throw a data validation exception. It holds an error witch can be
* inserted into build issue store, into the updater logs or returned to the APIs. The framework
* to catch and handle this is NOT IN PLACE, see
* <a href="https://github.com/opentripplanner/OpenTripPlanner/issues/5070">Error code design #5070</a>.
* <p>
* MORE WORK ON THIS IS NEEDED!
*/
public class DataValidationException extends RuntimeException {

private final OtpError error;

public DataValidationException(OtpError error) {
super();
this.error = error;
}

public OtpError error() {
return error;
}

@Override
public String getMessage() {
return error.message();
}

@Override
public String toString() {
return getMessage();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,22 @@
* A type for containing either a success or a failure type as the result of a computation.
* <p>
* It's very similar to the Either or Validation type found in functional programming languages.
*
* @deprecated This not possible to use inside a constructor - can only return one thing. Witch,
* then makes encapsulation harder and leaves the door open to forget. Also, having to create
* error codes, mapping and handling code for hundreds of different possible validation error
* types make changes harder, and cleanup impossible. This is a nice way to design APIs, but
* faced with hundreds or even thousands of different validation error types and the same
* amount of code branches this breaks.
* <p>
* I will use the {@link DataValidationException} for now, but we need to make an error
* handling strategy witch take all use-cases and goals into account, also pragmatic goals
* like maintainability. The {@link DataValidationException} is not a solution, but it
* at least it allows me to omit returning all possible error on every method ...
* <p>
* See https://github.com/opentripplanner/OpenTripPlanner/issues/5070
*/
@Deprecated
public abstract sealed class Result<T, E> {

private Result() {}
Expand Down
Loading

0 comments on commit e2259be

Please sign in to comment.