Skip to content

Commit

Permalink
Review fixes
Browse files Browse the repository at this point in the history
  • Loading branch information
@hannesj
hannesj committed Jan 30, 2023
1 parent a712376 commit ea3a29b
Show file tree
Hide file tree
Showing 4 changed files with 96 additions and 21 deletions.
30 changes: 28 additions & 2 deletions src/main/java/org/opentripplanner/model/TripTimeOnDate.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,17 @@
import org.opentripplanner.transit.model.timetable.StopTimeKey;
import org.opentripplanner.transit.model.timetable.Trip;
import org.opentripplanner.transit.model.timetable.TripTimes;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

/**
* Represents a Trip at a specific stop index and on a specific service day. This is a read-only
* data transfer object used to pass information from the OTP internal model to the APIs.
*/
public class TripTimeOnDate {

private static final Logger LOG = LoggerFactory.getLogger(TripTimeOnDate.class);

public static final int UNDEFINED = -1;

private final TripTimes tripTimes;
Expand Down Expand Up @@ -213,13 +217,35 @@ public List<String> getHeadsignVias() {
}

public PickDrop getPickupType() {
return tripTimes.isCanceledOrDeleted() || tripTimes.isCancelledStop(stopIndex)
if (tripTimes.isDeleted()) {
LOG.warn(
"Returning pickup type for a deleted trip {} on pattern {} on date {}. This indicates a bug.",
tripTimes.getTrip().getId(),
tripPattern.getId(),
serviceDate
);

return tripPattern.getBoardType(stopIndex);
}

return tripTimes.isCanceled() || tripTimes.isCancelledStop(stopIndex)
? PickDrop.CANCELLED
: tripPattern.getBoardType(stopIndex);
}

public PickDrop getDropoffType() {
return tripTimes.isCanceledOrDeleted() || tripTimes.isCancelledStop(stopIndex)
if (tripTimes.isDeleted()) {
LOG.warn(
"Returning dropoff type for a deleted trip {} on pattern {} on date {}. This indicates a bug.",
tripTimes.getTrip().getId(),
tripPattern.getId(),
serviceDate
);

return tripPattern.getAlightType(stopIndex);
}

return tripTimes.isCanceled() || tripTimes.isCancelledStop(stopIndex)
? PickDrop.CANCELLED
: tripPattern.getAlightType(stopIndex);
}
Expand Down
3 changes: 2 additions & 1 deletion src/main/java/org/opentripplanner/transit/model/timetable/RealTimeState.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,8 @@ public enum RealTimeState {
MODIFIED,

/**
* The trip has been canceled, and should not be visible to the end user
* The trip should not be visible to the end user. Either it has been set as deleted in the
* real-time feed, or it has been replaced by another trip on another pattern.
*/
DELETED,
}
39 changes: 21 additions & 18 deletions src/main/java/org/opentripplanner/updater/trip/TimetableSnapshotSource.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -291,8 +291,8 @@ public UpdateResult applyTripUpdates(
tripId,
serviceDate
);
case CANCELED -> handleCanceledTrip(tripId, serviceDate, false);
case DELETED -> handleCanceledTrip(tripId, serviceDate, true);
case CANCELED -> handleCanceledTrip(tripId, serviceDate, CancelationType.CANCEL);
case DELETED -> handleCanceledTrip(tripId, serviceDate, CancelationType.DELETE);
case REPLACEMENT -> validateAndHandleModifiedTrip(
tripUpdate,
tripDescriptor,
Expand Down Expand Up @@ -433,7 +433,7 @@ private Result<UpdateSuccess, UpdateError> handleScheduledTrip(
// If this trip_id has been used for previously ADDED/MODIFIED trip message (e.g. when the
// sequence of stops has changed, and is now changing back to the originally scheduled one),
// mark that previously created trip as DELETED.
cancelPreviouslyAddedTrip(tripId, serviceDate, true);
cancelPreviouslyAddedTrip(tripId, serviceDate, CancelationType.DELETE);

// Get new TripTimes based on scheduled timetable
var result = pattern
Expand Down Expand Up @@ -470,7 +470,7 @@ private Result<UpdateSuccess, UpdateError> handleScheduledTrip(
pattern
);

cancelScheduledTrip(tripId, serviceDate, true);
cancelScheduledTrip(tripId, serviceDate, CancelationType.DELETE);
return buffer.update(newPattern, updatedTripTimes, serviceDate);
} else {
// Set the updated trip times in the buffer
Expand Down Expand Up @@ -679,7 +679,7 @@ private Result<UpdateSuccess, UpdateError> handleAddedTrip(

// Check whether trip id has been used for previously ADDED trip message and mark previously
// created trip as DELETED
cancelPreviouslyAddedTrip(tripId, serviceDate, true);
cancelPreviouslyAddedTrip(tripId, serviceDate, CancelationType.DELETE);

Route route = getOrCreateRoute(tripDescriptor, tripId);

Expand Down Expand Up @@ -930,7 +930,7 @@ private Result<UpdateSuccess, UpdateError> addTripToGraphAndBuffer(
private boolean cancelScheduledTrip(
final FeedScopedId tripId,
final LocalDate serviceDate,
boolean markAsDeleted
CancelationType cancelationType
) {
boolean success = false;

Expand All @@ -944,10 +944,9 @@ private boolean cancelScheduledTrip(
debug(tripId, "Could not cancel scheduled trip because it's not in the timetable");
} else {
final TripTimes newTripTimes = new TripTimes(timetable.getTripTimes(tripIndex));
if (markAsDeleted) {
newTripTimes.deleteTrip();
} else {
newTripTimes.cancelTrip();
switch (cancelationType) {
case CANCEL -> newTripTimes.cancelTrip();
case DELETE -> newTripTimes.deleteTrip();
}
buffer.update(pattern, newTripTimes, serviceDate);
success = true;
Expand All @@ -972,7 +971,7 @@ private boolean cancelScheduledTrip(
private boolean cancelPreviouslyAddedTrip(
final FeedScopedId tripId,
final LocalDate serviceDate,
boolean markAsDeleted
CancelationType cancelationType
) {
boolean success = false;

Expand All @@ -985,10 +984,9 @@ private boolean cancelPreviouslyAddedTrip(
debug(tripId, "Could not cancel previously added trip on {}", serviceDate);
} else {
final TripTimes newTripTimes = new TripTimes(timetable.getTripTimes(tripIndex));
if (markAsDeleted) {
newTripTimes.deleteTrip();
} else {
newTripTimes.cancelTrip();
switch (cancelationType) {
case CANCEL -> newTripTimes.cancelTrip();
case DELETE -> newTripTimes.deleteTrip();
}
buffer.update(pattern, newTripTimes, serviceDate);
success = true;
Expand Down Expand Up @@ -1088,11 +1086,11 @@ private Result<UpdateSuccess, UpdateError> handleModifiedTrip(

// Mark scheduled trip as DELETED
var tripId = trip.getId();
cancelScheduledTrip(tripId, serviceDate, true);
cancelScheduledTrip(tripId, serviceDate, CancelationType.DELETE);

// Check whether trip id has been used for previously ADDED/REPLACEMENT trip message and mark it
// as DELETED
cancelPreviouslyAddedTrip(tripId, serviceDate, true);
cancelPreviouslyAddedTrip(tripId, serviceDate, CancelationType.DELETE);

// Add new trip
return addTripToGraphAndBuffer(
Expand All @@ -1108,7 +1106,7 @@ private Result<UpdateSuccess, UpdateError> handleModifiedTrip(
private Result<UpdateSuccess, UpdateError> handleCanceledTrip(
FeedScopedId tripId,
final LocalDate serviceDate,
boolean markAsDeleted
CancelationType markAsDeleted
) {
// Try to cancel scheduled trip
final boolean cancelScheduledSuccess = cancelScheduledTrip(tripId, serviceDate, markAsDeleted);
Expand Down Expand Up @@ -1163,4 +1161,9 @@ private static void debug(String feedId, String tripId, String message, Object..
String m = "[feedId: %s, tripId: %s] %s".formatted(feedId, tripId, message);
LOG.debug(m, params);
}

private enum CancelationType {
CANCEL,
DELETE,
}
}
45 changes: 45 additions & 0 deletions src/test/java/org/opentripplanner/updater/trip/TimetableSnapshotSourceTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,51 @@ public void testHandleCanceledTrip() throws InvalidProtocolBufferException {
assertEquals(RealTimeState.CANCELED, tripTimes.getRealTimeState());
}

@Test
public void testHandleDeletedTrip() throws InvalidProtocolBufferException {
final FeedScopedId tripId = new FeedScopedId(feedId, "1.1");
final FeedScopedId tripId2 = new FeedScopedId(feedId, "1.2");
final Trip trip = transitModel.getTransitModelIndex().getTripForId().get(tripId);
final TripPattern pattern = transitModel.getTransitModelIndex().getPatternForTrip().get(trip);
final int tripIndex = pattern.getScheduledTimetable().getTripIndex(tripId);
final int tripIndex2 = pattern.getScheduledTimetable().getTripIndex(tripId2);

var updater = new TimetableSnapshotSource(
TimetableSnapshotSourceParameters.DEFAULT,
transitModel
);

final TripDescriptor.Builder tripDescriptorBuilder = TripDescriptor.newBuilder();

tripDescriptorBuilder.setTripId("1.1");
tripDescriptorBuilder.setScheduleRelationship(ScheduleRelationship.DELETED);

final TripUpdate.Builder tripUpdateBuilder = TripUpdate.newBuilder();

tripUpdateBuilder.setTrip(tripDescriptorBuilder);

var deletion = tripUpdateBuilder.build().toByteArray();

updater.applyTripUpdates(
TRIP_MATCHER_NOOP,
REQUIRED_NO_DATA,
fullDataset,
List.of(TripUpdate.parseFrom(deletion)),
feedId
);

final TimetableSnapshot snapshot = updater.getTimetableSnapshot();
final Timetable forToday = snapshot.resolve(pattern, serviceDate);
final Timetable schedule = snapshot.resolve(pattern, null);
assertNotSame(forToday, schedule);
assertNotSame(forToday.getTripTimes(tripIndex), schedule.getTripTimes(tripIndex));
assertSame(forToday.getTripTimes(tripIndex2), schedule.getTripTimes(tripIndex2));

final TripTimes tripTimes = forToday.getTripTimes(tripIndex);

assertEquals(RealTimeState.DELETED, tripTimes.getRealTimeState());
}

/**
* This test just asserts that invalid trip ids don't throw an exception and are ignored instead
*/
Expand Down

0 comments on commit ea3a29b

Please sign in to comment.