Add Route/Trip/TripOnServiceDate replacement to query apis #7126
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## dev-2.x #7126 +/- ##
=============================================
+ Coverage 72.05% 72.11% +0.06%
- Complexity 20699 20865 +166
=============================================
Files 2242 2272 +30
Lines 83853 84506 +653
Branches 8366 8426 +60
=============================================
+ Hits 60418 60943 +525
- Misses 20499 20581 +82
- Partials 2936 2982 +46 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
tkalvas
added
New GraphQL API
optionsome
left a comment
optionsome
left a comment
There was a problem hiding this comment.
You could try adding/updating some integration test for the GTFS API.
| type Replacement { | ||
| isReplacement: Boolean! | ||
| replacementFor: [TripOnServiceDate!] | ||
| } |
| id: ID! | ||
| """ | ||
| Is this a replacement route. | ||
| Only true for GTFS-sourced data if set by the extended GTFS type, because GTFS does not implement |
There was a problem hiding this comment.
| Only true for GTFS-sourced data if set by the extended GTFS type, because GTFS does not implement | |
| Only true for GTFS-sourced data if set by the extended GTFS route type, because GTFS does not implement |
| id: ID! | ||
| """ | ||
| Is this a replacement trip. | ||
| Only true for GTFS-sourced data if set by the extended GTFS type, because GTFS does not implement |
There was a problem hiding this comment.
| Only true for GTFS-sourced data if set by the extended GTFS type, because GTFS does not implement | |
| Only true for GTFS-sourced data if set by the extended GTFS route type, because GTFS does not implement |
| "Information related to trip's scheduled arrival to the final stop location. Can contain real-time information." | ||
| end: StopCall! | ||
| "Replaced by these trips." | ||
| replacedBy: [TripOnServiceDate!]! |
There was a problem hiding this comment.
I'm not sure if this replacedBy should be here or in the Replacement type. Also, a bit inconsistent that this is marked as non-null while the replacementFor is not. I guess in both cases we should return an empty list and explain that the empty list doesn't necessarily mean that there is no replacement happening.
There was a problem hiding this comment.
I'm opposed to folding null and [] together when they so obviously mean different things. Based on where our information here is coming from, we might not know what the replacedBy should be (signified by null here), or we do know what it should be, but that can be nothing (signified by [] here) or a non-empty collection (signified by a non-empty list).
But it might be the case that replacementFor is already implemented incorrectly, in that it will be [] even when it should be null. This kind of thing is purposeful degradation of quality we could have.
| "Is a replacement trip." | ||
| replacement: Replacement! |
There was a problem hiding this comment.
The docs here make it sound like this is a boolean field, when the type can potentially contain more information than just that.
| Map<Trip, TripPattern> realTimeAddedPatternForTrip, | ||
| Multimap<Route, TripPattern> realTimeAddedPatternsForRoute, | ||
| Map<FeedScopedId, TripOnServiceDate> realTimeAddedTripOnServiceDateById, | ||
| Map<FeedScopedId, List<TripOnServiceDate>> realTimeAddedReplacedByTripOnServiceDateById, |
There was a problem hiding this comment.
Should we use a multimap here? I wonder what is the cost of having this separate index here vs going through all the realtime added TripOnServiceDate (my intuition is that this separate collection probably makes sense).
| import org.opentripplanner.transit.model.timetable.Trip; | ||
| import org.opentripplanner.transit.model.timetable.TripOnServiceDate; | ||
|
|
||
| public class ReplacementHelper { |
| } | ||
|
|
||
| public boolean isReplacementRoute(Route route) { | ||
| if (route.getGtfsType() != null && route.getGtfsType() == 714) { |
There was a problem hiding this comment.
Create some constant for 714 in this file. Also while we are at it, we could probably check for 110 as well since it's also some replacement submode.
|
|
||
| private final Map<FeedScopedId, TripPattern> tripPatternForId = new HashMap<>(); | ||
| private final Map<FeedScopedId, TripOnServiceDate> tripOnServiceDates = new HashMap<>(); | ||
| private final Map<FeedScopedId, List<TripOnServiceDate>> replacedByTripOnServiceDates = |
|
|
||
| var addedTripOnServiceDate2 = createReplacementTrip(ADDED_TRIP_ID, ADDED_TRIP_2_ID, "2"); | ||
| assertNotNull(addedTripOnServiceDate2); | ||
| assertEquals(1, addedTripOnServiceDate2.getReplacementFor().size()); |
There was a problem hiding this comment.
You can use truth.assertThat for checking collection sizes, although it is a not a requirement.
3 participants
Summary
Adds two-directional replacement relation for TripOnServiceDates, and simpler queries for Routes and Trips to find
if there are replacements. Works for both GTFS and NeTEx sources, which have very different capabilities in this
respect.
Issue
Discussion and plan in #6817
Unit tests
Yes.
Testing revealed that when we create a TripOnServiceDate replacing a Trip on some ServiceDate, we do not currently generate a synthetic TripOnServiceDate from that Trip, that the created TOSD could include it in its replacementFor list. For cancellations we create synthetic TOSDs, should we create them for replacements too?
Documentation
Todo.
Changelog
Yes.
Bumping the serialization version id
No.