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.

Sign up for GitHub

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
merged 13 commits into from
Sep 11, 2024
Merged

Update indexes in timetable snapshot #6007

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
5b71ff1
Update indexes in timetable snapshot
vpaturet Aug 9, 2024
db7ccb4
Apply review suggestions
vpaturet Aug 15, 2024
c1c809c
Rename boolean fields in record
vpaturet Aug 15, 2024
0caf435
Apply review suggestion
vpaturet Aug 16, 2024
1a5c1d5
Fix TransitService.getAllXXX methods
vpaturet Aug 16, 2024
e176b0b
Fix indexing of pattern by stop
vpaturet Aug 20, 2024
49f29b4
Update GTFS unit tests
vpaturet Aug 20, 2024
9baf38b
Ensure null-safe lookup on immutable map
vpaturet Aug 20, 2024
f34b32d
Use CollectionsView in DefaultTransitService
vpaturet Aug 21, 2024
c0c88ff
Remove redundant null-check
vpaturet Aug 27, 2024
dae1122
Apply review suggestions
vpaturet Sep 5, 2024
f579cbe
Fix merge conflict
vpaturet Sep 10, 2024
c21826b
Merge branch 'dev-2.x' into update_indexes_in_timetable_snapshot
vpaturet Sep 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 19 additions & 32 deletions src/ext-test/java/org/opentripplanner/ext/siri/AddedTripBuilderTest.java
View file
Open in desktop
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
75 changes: 63 additions & 12 deletions src/ext-test/java/org/opentripplanner/ext/siri/SiriTimetableSnapshotSourceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,19 @@
package org.opentripplanner.ext.siri;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.opentripplanner.updater.spi.UpdateResultAssertions.assertFailure;
import static org.opentripplanner.updater.trip.RealtimeTestEnvironment.SERVICE_DATE;

import java.util.List;
import org.junit.jupiter.api.Disabled;
import org.junit.jupiter.api.Test;
import org.opentripplanner.transit.model._data.TransitModelForTest;
import org.opentripplanner.transit.model.framework.FeedScopedId;
import org.opentripplanner.transit.model.network.Route;
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.service.TransitService;
import org.opentripplanner.updater.spi.UpdateError;
import org.opentripplanner.updater.trip.RealtimeTestEnvironment;
import uk.org.siri.siri20.EstimatedTimetableDeliveryStructure;
Expand All @@ -31,9 +38,51 @@ void testCancelTrip() {
}

@Test
void testAddJourney() {
void testAddJourneyWithExistingRoute() {
var env = RealtimeTestEnvironment.siri();
var updates = createValidAddedJourney(env);

Route route = env.getTransitService().getRouteForId(env.route1Id);
int numPatternForRoute = env.getTransitService().getPatternsForRoute(route).size();

String newJourneyId = "newJourney";
var updates = createValidAddedJourney(env).buildEstimatedTimetableDeliveries();

var result = env.applyEstimatedTimetable(updates);

assertEquals(1, result.successful());
assertEquals("ADDED | C1 [R] 0:02 0:02 | D1 0:04 0:04", env.getRealtimeTimetable(newJourneyId));
assertEquals(
"SCHEDULED | C1 0:01 0:01 | D1 0:03 0:03",
env.getScheduledTimetable(newJourneyId)
);
FeedScopedId tripId = TransitModelForTest.id(newJourneyId);
TransitService transitService = env.getTransitService();
Trip trip = transitService.getTripForId(tripId);
assertNotNull(trip);
assertNotNull(transitService.getPatternForTrip(trip));
assertNotNull(transitService.getTripOnServiceDateById(tripId));
assertNotNull(
transitService.getTripOnServiceDateForTripAndDay(
new TripIdAndServiceDate(tripId, SERVICE_DATE)
)
);
assertEquals(
numPatternForRoute + 1,
transitService.getPatternsForRoute(route).size(),
"The added trip should use a new pattern for this route"
);
}

@Test
void testAddJourneyWithNewRoute() {
var env = RealtimeTestEnvironment.siri();

String newRouteRef = "new route ref";
var updates = createValidAddedJourney(env)
.withLineRef(newRouteRef)
.buildEstimatedTimetableDeliveries();

int numRoutes = env.getTransitService().getAllRoutes().size();
var result = env.applyEstimatedTimetable(updates);

assertEquals(1, result.successful());
Expand All @@ -42,12 +91,18 @@ void testAddJourney() {
"SCHEDULED | C1 0:01 0:01 | D1 0:03 0:03",
env.getScheduledTimetable("newJourney")
);
TransitService transitService = env.getTransitService();
assertEquals(numRoutes + 1, transitService.getAllRoutes().size());
FeedScopedId newRouteId = TransitModelForTest.id(newRouteRef);
Route newRoute = transitService.getRouteForId(newRouteId);
assertNotNull(newRoute);
assertEquals(1, transitService.getPatternsForRoute(newRoute).size());
}

@Test
void testAddJourneyMultipleTimes() {
var env = RealtimeTestEnvironment.siri();
var updates = createValidAddedJourney(env);
var updates = createValidAddedJourney(env).buildEstimatedTimetableDeliveries();

int numTrips = env.getTransitService().getAllTrips().size();
var result1 = env.applyEstimatedTimetable(updates);
Expand Down Expand Up @@ -165,9 +220,7 @@ void testUpdateJourneyWithFramedVehicleJourneyRef() {

var updates = updatedJourneyBuilder(env)
.withFramedVehicleJourneyRef(builder ->
builder
.withServiceDate(RealtimeTestEnvironment.SERVICE_DATE)
.withVehicleJourneyRef(env.trip1.getId().getId())
builder.withServiceDate(SERVICE_DATE).withVehicleJourneyRef(env.trip1.getId().getId())
)
.buildEstimatedTimetableDeliveries();
var result = env.applyEstimatedTimetable(updates);
Expand Down Expand Up @@ -421,17 +474,15 @@ void testExtraUnknownStop() {
assertFailure(UpdateError.UpdateErrorType.INVALID_STOP_SEQUENCE, result);
}

private static List<EstimatedTimetableDeliveryStructure> createValidAddedJourney(
RealtimeTestEnvironment env
) {
private static SiriEtBuilder createValidAddedJourney(RealtimeTestEnvironment env) {
return new SiriEtBuilder(env.getDateTimeHelper())
.withEstimatedVehicleJourneyCode("newJourney")
.withIsExtraJourney(true)
.withOperatorRef(env.operator1Id.getId())
.withLineRef(env.route1Id.getId())
.withRecordedCalls(builder -> builder.call(env.stopC1).departAimedActual("00:01", "00:02"))
.withEstimatedCalls(builder -> builder.call(env.stopD1).arriveAimedExpected("00:03", "00:04"))
.buildEstimatedTimetableDeliveries();
.withEstimatedCalls(builder -> builder.call(env.stopD1).arriveAimedExpected("00:03", "00:04")
);
}

private static SiriEtBuilder updatedJourneyBuilder(RealtimeTestEnvironment env) {
Expand Down
24 changes: 11 additions & 13 deletions src/ext/java/org/opentripplanner/ext/siri/AddedTripBuilder.java
View file
Open in desktop
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
28 changes: 20 additions & 8 deletions src/ext/java/org/opentripplanner/ext/siri/SiriTimetableSnapshotSource.java
View file
Open in desktop
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 @@ -298,14 +299,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
Copy link
Contributor

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 @@ -330,7 +342,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
Loading
Loading