Skip to content

Improve handling of realtime updated StopPatterns #6909

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

Open
wants to merge 5 commits into
base: dev-2.x
Choose a base branch
from
Open

Improve handling of realtime updated StopPatterns #6909

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
aeaa862
Only create modified trip patterns if the data changes
flaktack Sep 24, 2025
0208cbf
Remove unused realtime trip patterns
flaktack Sep 24, 2025
a7ff17b
Reduce code duplication in AssignedStopIdsTest
flaktack Sep 24, 2025
dcc9067
Use TripUpdateBuilder in TripTimesUpdaterTest
flaktack Oct 15, 2025
fa0f0b2
Merge remote-tracking branch 'upstream/dev-2.x' into feature/modified…
flaktack Oct 30, 2025
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
3 changes: 2 additions & 1 deletion ...ner/routing/algorithm/raptoradapter/transit/mappers/RealTimeRaptorTransitDataUpdater.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,8 @@ public void update(
patternsForDate.remove(tripPatternForDate);
}
} else {
LOG.warn("Could not fetch timetable for {}", pattern);
LOG.warn("Could not fetch timetable for {}, removing.", pattern);
patternsForDate.remove(tripPatternForDate);
Copy link
Member

@leonardehrenfried leonardehrenfried Oct 14, 2025

Choose a reason for hiding this comment

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

This is the most important change in the PR. I absolutely support changing this class but I'm having a hard time understanding the consequences of this change: the code is very complex with several collections updating each other.

If you have a better understanding, do you have time to walk me through the code? Maybe @vpaturet is interested as well.

}
}
}
Expand Down
25 changes: 21 additions & 4 deletions application/src/main/java/org/opentripplanner/updater/trip/gtfs/TripTimesUpdater.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import java.util.Iterator;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Optional;
import java.util.OptionalInt;
import javax.annotation.Nullable;
Expand Down Expand Up @@ -132,10 +133,26 @@ public static Result<TripTimesPatch, UpdateError> createUpdatedTripTimesFromGtfs
}

if (match) {
update.stopHeadsign().ifPresent(x -> builder.withStopHeadsign(index, x));
update.pickup().ifPresent(x -> updatedPickups.put(index, x));
update.dropoff().ifPresent(x -> updatedDropoffs.put(index, x));
update.assignedStopId().ifPresent(x -> replacedStopIndices.put(index, x));
var scheduledStopId = timetable.getPattern().getStop(i).getId().getId();
var scheduledStopHeadsign = tripTimes.getHeadsign(i);
var scheduledPickup = timetable.getPattern().getBoardType(i);
var scheduledDropoff = timetable.getPattern().getAlightType(i);
update
.stopHeadsign()
.filter(x -> !Objects.equals(x, scheduledStopHeadsign))
.ifPresent(x -> builder.withStopHeadsign(index, x));
update
.pickup()
.filter(x -> x != scheduledPickup)
.ifPresent(x -> updatedPickups.put(index, x));
update
.dropoff()
.filter(x -> x != scheduledDropoff)
.ifPresent(x -> updatedDropoffs.put(index, x));
update
.assignedStopId()
.filter(x -> !Objects.equals(x, scheduledStopId))
.ifPresent(x -> replacedStopIndices.put(index, x));

var scheduleRelationship = update.scheduleRelationship();
// Handle each schedule relationship case
Expand Down
48 changes: 47 additions & 1 deletion application/src/test/java/org/opentripplanner/updater/trip/RealtimeTestEnvironment.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,17 +5,24 @@
import static org.opentripplanner.updater.trip.UpdateIncrementality.FULL_DATASET;

import com.google.transit.realtime.GtfsRealtime;
import java.time.Duration;
import java.time.LocalDate;
import java.time.ZoneId;
import java.util.List;
import java.util.Objects;
import org.opentripplanner.DateTimeHelper;
import org.opentripplanner.model.TimetableSnapshot;
import org.opentripplanner.routing.algorithm.raptoradapter.transit.RaptorTransitData;
import org.opentripplanner.routing.algorithm.raptoradapter.transit.TransitTuningParameters;
import org.opentripplanner.routing.algorithm.raptoradapter.transit.mappers.RaptorTransitDataMapper;
import org.opentripplanner.routing.algorithm.raptoradapter.transit.mappers.RealTimeRaptorTransitDataUpdater;
import org.opentripplanner.routing.api.request.RouteRequest;
import org.opentripplanner.routing.graph.Graph;
import org.opentripplanner.transit.model._data.TimetableRepositoryForTest;
import org.opentripplanner.transit.model.framework.FeedScopedId;
import org.opentripplanner.transit.model.network.TripPattern;
import org.opentripplanner.transit.model.site.RegularStop;
import org.opentripplanner.transit.model.site.StopTransferPriority;
import org.opentripplanner.transit.model.timetable.TripTimes;
import org.opentripplanner.transit.model.timetable.TripTimesStringBuilder;
import org.opentripplanner.transit.service.DefaultTransitService;
Expand Down Expand Up @@ -55,8 +62,14 @@ public static RealtimeTestEnvironmentBuilder of() {
this.timetableRepository = timetableRepository;

this.timetableRepository.index();
this.timetableRepository.setRaptorTransitData(
RaptorTransitDataMapper.map(new TestTransitTuningParameters(), timetableRepository)
);
this.timetableRepository.setRealtimeRaptorTransitData(
new RaptorTransitData(timetableRepository.getRaptorTransitData())
);
this.snapshotManager = new TimetableSnapshotManager(
null,
new RealTimeRaptorTransitDataUpdater(timetableRepository),
Copy link
Member

@leonardehrenfried leonardehrenfried Oct 14, 2025

Choose a reason for hiding this comment

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

I think it's great that we are adding test coverage to this critical, yet poorly-understood component.

TimetableSnapshotParameters.PUBLISH_IMMEDIATELY,
() -> defaultServiceDate
);
Expand Down Expand Up @@ -212,4 +225,37 @@ private UpdateResult applyEstimatedTimetable(
private void commitTimetableSnapshot() {
snapshotManager.purgeAndCommit();
}

private static class TestTransitTuningParameters implements TransitTuningParameters {

@Override
public boolean enableStopTransferPriority() {
return false;
}

@Override
public Integer stopBoardAlightDuringTransferCost(StopTransferPriority key) {
return 0;
}

@Override
public int transferCacheMaxSize() {
return 0;
}

@Override
public Duration maxSearchWindow() {
return null;
}

@Override
public List<Duration> pagingSearchWindowAdjustments() {
return List.of();
}

@Override
public List<RouteRequest> transferCacheRequests() {
return List.of();
}
}
}
59 changes: 56 additions & 3 deletions application/src/test/java/org/opentripplanner/updater/trip/TripUpdateBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,22 @@ public TripUpdateBuilder(
);
}

public TripUpdateBuilder addStopTime(int stopSequence, String time) {
return addStopTime(
null,
time,
stopSequence,
NO_DELAY,
NO_DELAY,
DEFAULT_SCHEDULE_RELATIONSHIP,
null,
null,
null,
null,
null
);
}

public TripUpdateBuilder addStopTime(String stopId, String time) {
return addStopTime(
stopId,
Expand All @@ -67,6 +83,7 @@ public TripUpdateBuilder addStopTime(String stopId, String time) {
null,
null,
null,
null,
null
);
}
Expand All @@ -82,6 +99,7 @@ public TripUpdateBuilder addStopTime(String stopId, String time, String headsign
null,
null,
headsign,
null,
null
);
}
Expand All @@ -97,6 +115,7 @@ public TripUpdateBuilder addStopTimeWithDelay(String stopId, String time, int de
null,
null,
null,
null,
null
);
}
Expand All @@ -116,7 +135,8 @@ public TripUpdateBuilder addStopTimeWithScheduled(
null,
null,
null,
scheduledTime
scheduledTime,
null
);
}

Expand All @@ -131,6 +151,7 @@ public TripUpdateBuilder addStopTime(String stopId, String time, DropOffPickupTy
pickDrop,
null,
null,
null,
null
);
}
Expand All @@ -150,6 +171,7 @@ public TripUpdateBuilder addStopTime(
null,
pickDrop,
null,
null,
null
);
}
Expand All @@ -165,6 +187,7 @@ public TripUpdateBuilder addDelayedStopTime(int stopSequence, int delay) {
null,
null,
null,
null,
null
);
}
Expand All @@ -184,6 +207,7 @@ public TripUpdateBuilder addDelayedStopTime(
null,
null,
null,
null,
null
);
}
Expand All @@ -202,6 +226,7 @@ public TripUpdateBuilder addNoDataStop(int stopSequence) {
null,
null,
null,
null,
null
);
}
Expand All @@ -220,6 +245,7 @@ public TripUpdateBuilder addSkippedStop(int stopSequence) {
null,
null,
null,
null,
null
);
}
Expand All @@ -235,6 +261,7 @@ public TripUpdateBuilder addSkippedStop(String stopId, String time) {
null,
null,
null,
null,
null
);
}
Expand All @@ -250,10 +277,31 @@ public TripUpdateBuilder addSkippedStop(String stopId, String time, DropOffPicku
pickDrop,
null,
null,
null,
null
);
}

public TripUpdateBuilder addAssignedStopTime(
int stopSequence,
String time,
String assignedStopId
) {
return addStopTime(
null,
time,
stopSequence,
NO_DELAY,
NO_DELAY,
StopTimeUpdate.ScheduleRelationship.SCHEDULED,
null,
null,
null,
null,
assignedStopId
);
}

/**
* As opposed to the other convenience methods, this one takes a raw {@link StopTimeUpdate} and
* adds it to the trip. This is useful if you want to test invalid ones.
Expand All @@ -273,7 +321,8 @@ private TripUpdateBuilder addStopTime(
@Nullable DropOffPickupType pickDrop,
@Nullable StopTimeUpdate.StopTimeProperties.DropOffPickupType gtfsPickDrop,
@Nullable String headsign,
@Nullable String scheduledTime
@Nullable String scheduledTime,
@Nullable String assignedStopId
) {
final StopTimeUpdate.Builder stopTimeUpdateBuilder =
tripUpdateBuilder.addStopTimeUpdateBuilder();
Expand All @@ -287,7 +336,7 @@ private TripUpdateBuilder addStopTime(
stopTimeUpdateBuilder.setStopSequence(stopSequence);
}

if (pickDrop != null || gtfsPickDrop != null || headsign != null) {
if (pickDrop != null || gtfsPickDrop != null || headsign != null || assignedStopId != null) {
var stopTimePropsBuilder = stopTimeUpdateBuilder.getStopTimePropertiesBuilder();

if (headsign != null) {
Expand All @@ -305,6 +354,10 @@ private TripUpdateBuilder addStopTime(
var ext = b.build();
stopTimePropsBuilder.setExtension(MfdzRealtimeExtensions.stopTimeProperties, ext);
}

if (assignedStopId != null) {
stopTimePropsBuilder.setAssignedStopId(assignedStopId);
}
}

final GtfsRealtime.TripUpdate.StopTimeEvent.Builder arrivalBuilder =
Expand Down
61 changes: 61 additions & 0 deletions application/src/test/java/org/opentripplanner/updater/trip/gtfs/TripTimesUpdaterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -456,6 +456,67 @@ public void testUpdateWithNoData() {
});
}

@Test
public void testUpdateWithUnchangedTripAndStopProperties() {
var tripDescriptorBuilder = tripDescriptorBuilder(TRIP_ID);

GtfsRealtime.TripUpdate.Builder tripUpdateBuilder = GtfsRealtime.TripUpdate.newBuilder();
tripUpdateBuilder.setTrip(tripDescriptorBuilder);
tripUpdateBuilder.getTripPropertiesBuilder().setTripHeadsign("foo");
StopTimeUpdate.Builder stopTimeUpdateBuilder = tripUpdateBuilder.addStopTimeUpdateBuilder(0);
stopTimeUpdateBuilder.setStopSequence(1);
stopTimeUpdateBuilder.getArrivalBuilder().setDelay(0);
stopTimeUpdateBuilder.getDepartureBuilder().setDelay(0);
stopTimeUpdateBuilder.getStopTimePropertiesBuilder().setStopHeadsign("foo");
stopTimeUpdateBuilder.getStopTimePropertiesBuilder().setAssignedStopId("A");
stopTimeUpdateBuilder = tripUpdateBuilder.addStopTimeUpdateBuilder(1);
stopTimeUpdateBuilder.setStopSequence(2);
stopTimeUpdateBuilder
.getStopTimePropertiesBuilder()
.setPickupType(StopTimeUpdate.StopTimeProperties.DropOffPickupType.REGULAR);
stopTimeUpdateBuilder
.getStopTimePropertiesBuilder()
.setDropOffType(StopTimeUpdate.StopTimeProperties.DropOffPickupType.REGULAR);
stopTimeUpdateBuilder.setScheduleRelationship(StopTimeUpdate.ScheduleRelationship.SCHEDULED);
stopTimeUpdateBuilder = tripUpdateBuilder.addStopTimeUpdateBuilder(2);
stopTimeUpdateBuilder.setStopSequence(3);
stopTimeUpdateBuilder.getArrivalBuilder().setDelay(0);
stopTimeUpdateBuilder.getDepartureBuilder().setDelay(0);
Copy link
Member

@leonardehrenfried leonardehrenfried Oct 14, 2025

Choose a reason for hiding this comment

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

I know that the other tests in this class do the same, but I find the repetition pretty grating.

Can you please make this a little less verbose?


GtfsRealtime.TripUpdate tripUpdate = tripUpdateBuilder.build();
var result = TripTimesUpdater.createUpdatedTripTimesFromGtfsRt(
timetable,
new TripUpdate(tripUpdate),
TIME_ZONE,
SERVICE_DATE,
ForwardsDelayPropagationType.DEFAULT,
BackwardsDelayPropagationType.REQUIRED_NO_DATA
);

assertTrue(result.isSuccess());

result.ifSuccess(p -> {
assertTrue(p.updatedDropoff().isEmpty(), "dropoffs are not modified");
assertTrue(p.updatedPickup().isEmpty(), "pickups are not modified");
assertTrue(p.replacedStopIndices().isEmpty(), "stop indices are not modified");
assertEquals(
"foo",
p.tripTimes().getHeadsign(0).toString(),
"headsigns [1] are not modified"
);
assertEquals(
"foo",
p.tripTimes().getHeadsign(1).toString(),
"headsigns [2] are not modified"
);
assertEquals(
"foo",
p.tripTimes().getHeadsign(2).toString(),
"headsigns [3] are not modified"
);
});
}

@Test
public void testUpdateWithTripAndStopProperties() {
var tripDescriptorBuilder = tripDescriptorBuilder(TRIP_ID);
Expand Down
Loading
Loading