Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update indexes in timetable snapshot #6007

Merged
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@
import org.opentripplanner.transit.model.site.RegularStop;
import org.opentripplanner.transit.model.timetable.RealTimeState;
import org.opentripplanner.transit.model.timetable.Trip;
import org.opentripplanner.transit.model.timetable.TripIdAndServiceDate;
import org.opentripplanner.transit.model.timetable.TripOnServiceDate;
import org.opentripplanner.transit.service.DefaultTransitService;
import org.opentripplanner.transit.service.StopModel;
import org.opentripplanner.transit.service.TransitEditorService;
Expand Down Expand Up @@ -135,15 +135,16 @@ void testAddedTrip() {

assertTrue(addedTrip.isSuccess(), "Trip creation should succeed");

TripUpdate tripUpdate = addedTrip.successValue();
// Assert trip
Trip trip = addedTrip.successValue().tripTimes().getTrip();
Trip trip = tripUpdate.tripTimes().getTrip();
assertEquals(TRIP_ID, trip.getId(), "Trip should be mapped");
assertEquals(OPERATOR, trip.getOperator(), "operator should be mapped");
assertEquals(TRANSIT_MODE, trip.getMode(), "transitMode should be mapped");
assertEquals(SubMode.of(SUB_MODE), trip.getNetexSubMode(), "submode should be mapped");
assertNotNull(trip.getHeadsign(), "Headsign should be mapped");
assertEquals(HEADSIGN, trip.getHeadsign().toString(), "Headsign should be mapped");
assertEquals(SERVICE_DATE, addedTrip.successValue().serviceDate());
assertEquals(SERVICE_DATE, tripUpdate.serviceDate());

// Assert route
Route route = trip.getRoute();
Expand All @@ -154,17 +155,11 @@ void testAddedTrip() {
assertEquals(SubMode.of(SUB_MODE), route.getNetexSubmode(), "submode should be mapped");
assertNotEquals(REPLACED_ROUTE, route, "Should not re-use replaced route");

assertEquals(
route,
transitService.getRouteForId(TransitModelForTest.id(LINE_REF)),
"Route should be added to transit index"
);
assertEquals(
trip,
transitService.getTripForId(TRIP_ID),
"Route should be added to transit index"
);
var pattern = transitService.getPatternForTrip(trip);
assertTrue(tripUpdate.routeCreation(), "The route is marked as created by real time updater");

assertTrue(tripUpdate.tripCreation(), "The trip is marked as created by real time updater");

TripPattern pattern = tripUpdate.addedTripPattern();
assertNotNull(pattern);
assertEquals(route, pattern.getRoute());
assertTrue(
Expand All @@ -173,19 +168,11 @@ void testAddedTrip() {
.contains(TRANSIT_MODEL.getServiceCodes().get(trip.getServiceId())),
"serviceId should be running on service date"
);
assertNotNull(
transitService.getTripOnServiceDateById(TRIP_ID),
"TripOnServiceDate should be added to transit index by id"
);
assertNotNull(
transitService.getTripOnServiceDateForTripAndDay(
new TripIdAndServiceDate(TRIP_ID, SERVICE_DATE)
),
"TripOnServiceDate should be added to transit index for trip and day"
);
TripOnServiceDate tripOnServiceDate = tripUpdate.addedTripOnServiceDate();
assertNotNull(tripOnServiceDate, "The TripUpdate should contain a new TripOnServiceDate");

// Assert stop pattern
var stopPattern = addedTrip.successValue().stopPattern();
var stopPattern = tripUpdate.stopPattern();
assertEquals(stopPattern, pattern.getStopPattern());
assertEquals(3, stopPattern.getSize());
assertEquals(STOP_A, stopPattern.getStop(0));
Expand Down Expand Up @@ -215,7 +202,7 @@ void testAddedTrip() {
assertEquals(0, scheduledTimes.getArrivalDelay(2));

// Assert updated trip times
var times = addedTrip.successValue().tripTimes();
var times = tripUpdate.tripTimes();
assertEquals(trip, times.getTrip());
assertEquals(RealTimeState.ADDED, times.getRealTimeState());
assertFalse(times.isScheduled());
Expand Down Expand Up @@ -259,6 +246,8 @@ void testAddedTripOnAddedRoute() {
.build();

assertTrue(firstAddedTrip.isSuccess(), "Trip creation should succeed");
assertTrue(firstAddedTrip.successValue().routeCreation());

var firstTrip = firstAddedTrip.successValue().tripTimes().getTrip();

var tripId2 = TransitModelForTest.id("TRIP_ID_2");
Expand Down Expand Up @@ -291,12 +280,6 @@ void testAddedTripOnAddedRoute() {
assertEquals(tripId2, secondTrip.getId(), "Trip should be mapped");
assertNotEquals(firstTrip, secondTrip);

// Assert route
Route route = secondTrip.getRoute();
assertSame(firstTrip.getRoute(), route, "route be reused from the first trip");

assertEquals(2, transitService.getPatternsForRoute(route).size());

// Assert trip times
var times = secondAddedTrip.successValue().tripTimes();
assertEquals(secondTrip, times.getTrip());
Expand Down Expand Up @@ -338,6 +321,9 @@ void testAddedTripOnExistingRoute() {
Trip trip = addedTrip.successValue().tripTimes().getTrip();
assertEquals(TRIP_ID, trip.getId(), "Trip should be mapped");
assertSame(REPLACED_ROUTE, trip.getRoute());

// Assert route
assertFalse(addedTrip.successValue().routeCreation(), "The existing route should be reused");
}

@Test
Expand Down Expand Up @@ -370,6 +356,7 @@ void testAddedTripWithoutReplacedRoute() {
assertEquals(TRIP_ID, trip.getId(), "Trip should be mapped");

// Assert route
assertTrue(addedTrip.successValue().routeCreation(), "A new route should be created");
Route route = trip.getRoute();
assertEquals(LINE_REF, route.getId().getId(), "route should be mapped");
assertEquals(AGENCY, route.getAgency(), "Agency should be taken from replaced route");
Expand Down
24 changes: 11 additions & 13 deletions src/ext/java/org/opentripplanner/ext/siri/AddedTripBuilder.java
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@
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.TripIdAndServiceDate;
import org.opentripplanner.transit.model.timetable.TripOnServiceDate;
import org.opentripplanner.transit.model.timetable.TripTimesFactory;
import org.opentripplanner.transit.service.TransitEditorService;
Expand Down Expand Up @@ -173,15 +172,16 @@ Result<TripUpdate, UpdateError> build() {
return UpdateError.result(tripId, NO_START_DATE);
}

boolean isAddedRoute = false;
Route route = entityResolver.resolveRoute(lineRef);
if (route == null) {
Agency agency = resolveAgency();
if (agency == null) {
return UpdateError.result(tripId, CANNOT_RESOLVE_AGENCY);
}
route = createRoute(agency);
isAddedRoute = true;
LOG.info("Adding route {} to transitModel.", route);
transitService.addRoutes(route);
}

Trip trip = createTrip(route, calServiceId);
Expand Down Expand Up @@ -265,18 +265,16 @@ Result<TripUpdate, UpdateError> build() {
.withReplacementFor(replacedTrips)
.build();

// Adding trip to index necessary to include values in graphql-queries
// TODO - SIRI: should more data be added to index?
transitService.addTripForId(tripId, trip);
transitService.addPatternForTrip(trip, pattern);
transitService.addPatternsForRoute(route, pattern);
transitService.addTripOnServiceDateById(tripOnServiceDate.getId(), tripOnServiceDate);
transitService.addTripOnServiceDateForTripAndDay(
new TripIdAndServiceDate(tripId, serviceDate),
tripOnServiceDate
return Result.success(
new TripUpdate(
stopPattern,
updatedTripTimes,
serviceDate,
tripOnServiceDate,
pattern,
isAddedRoute
)
);

return Result.success(new TripUpdate(stopPattern, updatedTripTimes, serviceDate));
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
import java.util.ArrayList;
import java.util.List;
import javax.annotation.Nullable;
import org.opentripplanner.model.RealTimeTripUpdate;
import org.opentripplanner.model.Timetable;
import org.opentripplanner.model.TimetableSnapshot;
import org.opentripplanner.model.TimetableSnapshotProvider;
Expand Down Expand Up @@ -292,14 +293,25 @@ private Result<UpdateSuccess, UpdateError> addTripToGraphAndBuffer(TripUpdate tr
Trip trip = tripUpdate.tripTimes().getTrip();
LocalDate serviceDate = tripUpdate.serviceDate();

// Get cached trip pattern or create one if it doesn't exist yet
final TripPattern pattern = tripPatternCache.getOrCreateTripPattern(
tripUpdate.stopPattern(),
trip,
serviceDate
);
final TripPattern pattern;
if (tripUpdate.tripPatternCreation()) {
pattern = tripUpdate.addedTripPattern();
} else {
// Get cached trip pattern or create one if it doesn't exist yet
pattern =
tripPatternCache.getOrCreateTripPattern(tripUpdate.stopPattern(), trip, serviceDate);
Comment on lines +307 to +308

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's pretty difficult to follow the logic with the different trip patterns being created/fetched depending on if it's an added trip or modified trip. Also this tripPatternCache seems to have a very similar responsibility to the indexes in the snapshot.

But it looks like you are keeping the behaviour the same as previously. So I'm ok with this and we can do any further improvements in a future PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, this is quite convoluted and should be addressed in a follow-up PR.

}

// Add new trip times to buffer, making protective copies as needed. Bubble success/error up.
var result = snapshotManager.updateBuffer(pattern, tripUpdate.tripTimes(), serviceDate);
RealTimeTripUpdate realTimeTripUpdate = new RealTimeTripUpdate(
pattern,
tripUpdate.tripTimes(),
serviceDate,
tripUpdate.addedTripOnServiceDate(),
tripUpdate.tripCreation(),
tripUpdate.routeCreation()
);
var result = snapshotManager.updateBuffer(realTimeTripUpdate);
LOG.debug("Applied real-time data for trip {} on {}", trip, serviceDate);
return result;
}
Expand All @@ -324,7 +336,7 @@ private boolean markScheduledTripAsDeleted(Trip trip, final LocalDate serviceDat
} else {
final RealTimeTripTimes newTripTimes = tripTimes.copyScheduledTimes();
newTripTimes.deleteTrip();
snapshotManager.updateBuffer(pattern, newTripTimes, serviceDate);
snapshotManager.updateBuffer(new RealTimeTripUpdate(pattern, newTripTimes, serviceDate));
success = true;
}
}
Expand Down
57 changes: 52 additions & 5 deletions src/ext/java/org/opentripplanner/ext/siri/TripUpdate.java
Original file line number Diff line number Diff line change
@@ -1,12 +1,59 @@
package org.opentripplanner.ext.siri;

import java.time.LocalDate;
import javax.annotation.Nonnull;
import java.util.Objects;
import javax.annotation.Nullable;
import org.opentripplanner.transit.model.network.StopPattern;
import org.opentripplanner.transit.model.network.TripPattern;
import org.opentripplanner.transit.model.timetable.RealTimeTripTimes;
import org.opentripplanner.transit.model.timetable.TripOnServiceDate;
import org.opentripplanner.transit.model.timetable.TripTimes;

/**
* Represents the SIRI real-time update of a single trip.
* @param stopPattern the stop pattern to which belongs the updated trip.
* @param tripTimes the new trip times for the updated trip.
* @param serviceDate the service date for which this update applies (updates are valid only for one service date)
* @param addedTripOnServiceDate optionally if this trip update adds a new trip, the TripOnServiceDate corresponding to this new trip.
* @param addedTripPattern optionally if this trip update adds a new trip pattern , the new trip pattern to this new trip.
* @param routeCreation true if an added trip cannot be registered under an existing route and a new route must be created.
*/
record TripUpdate(
@Nonnull StopPattern stopPattern,
@Nonnull TripTimes tripTimes,
@Nonnull LocalDate serviceDate
) {}
StopPattern stopPattern,
TripTimes tripTimes,
LocalDate serviceDate,
@Nullable TripOnServiceDate addedTripOnServiceDate,
@Nullable TripPattern addedTripPattern,
boolean routeCreation
) {
public TripUpdate {
Objects.requireNonNull(stopPattern);
Objects.requireNonNull(tripTimes);
Objects.requireNonNull(serviceDate);
}

/**
* Create a trip update for an existing trip.
*/
public TripUpdate(
StopPattern stopPattern,
RealTimeTripTimes updatedTripTimes,
LocalDate serviceDate
) {
this(stopPattern, updatedTripTimes, serviceDate, null, null, false);
}

/**
* Return true if this trip update creates a new trip pattern.
*/
public boolean tripPatternCreation() {
return addedTripPattern != null;
}

/**
* Return true if this trip update creates a new trip.
*/
public boolean tripCreation() {
return addedTripOnServiceDate != null;
}
}
43 changes: 43 additions & 0 deletions src/main/java/org/opentripplanner/model/RealTimeTripUpdate.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
package org.opentripplanner.model;

import java.time.LocalDate;
import java.util.Objects;
import javax.annotation.Nullable;
import org.opentripplanner.transit.model.network.TripPattern;
import org.opentripplanner.transit.model.timetable.TripOnServiceDate;
import org.opentripplanner.transit.model.timetable.TripTimes;

/**
* Represents the real-time update of a single trip.
* @param pattern the pattern to which belongs the updated trip. This can be a new pattern created in real-time.
* @param updatedTripTimes the new trip times for the updated trip.
* @param serviceDate the service date for which this update applies (updates are valid only for one service date)
* @param addedTripOnServiceDate optionally if this trip update adds a new trip, the TripOnServiceDate corresponding to this new trip.
* @param tripCreation true if this update creates a new trip, not present in scheduled data.
* @param routeCreation true if an added trip cannot be registered under an existing route and a new route must be created.
*/
public record RealTimeTripUpdate(
TripPattern pattern,
TripTimes updatedTripTimes,
LocalDate serviceDate,
@Nullable TripOnServiceDate addedTripOnServiceDate,
boolean tripCreation,
boolean routeCreation
) {
public RealTimeTripUpdate {
Objects.requireNonNull(pattern);
Objects.requireNonNull(updatedTripTimes);
Objects.requireNonNull(serviceDate);
}

/**
* Create a real-time update for an existing trip.
*/
public RealTimeTripUpdate(
TripPattern pattern,
TripTimes updatedTripTimes,
LocalDate serviceDate
) {
this(pattern, updatedTripTimes, serviceDate, null, false, false);
}
}
Loading
Loading