Skip to content

Add an implementation for fetching alternative legs - #4071

Merged
hannesj merged 6 commits into
opentripplanner:dev-2.xfrom
entur:otp2_alternative_legs
Apr 8, 2022
Merged

Add an implementation for fetching alternative legs#4071
hannesj merged 6 commits into
opentripplanner:dev-2.xfrom
entur:otp2_alternative_legs

Conversation

@hannesj

@hannesj hannesj commented Apr 5, 2022

Copy link
Copy Markdown
Contributor

Summary

This PR adds an implementation for re-fetching legs, and fetching "alternative legs", i.e. trips which use the same origin/destination station and depart prior/after the current leg.

Unit tests

Module test added for the new functions.

Code style

Documentation

Added documentation to the GraphQL schema.

@hannesj hannesj added !New Feature A functional feature targeting the end user. Entur On Entur Roadmap Entur Test This is currently being tested at Entur labels Apr 5, 2022
@hannesj hannesj added this to the 2.2 milestone Apr 5, 2022
@hannesj
hannesj requested a review from a team as a code owner April 5, 2022 17:31

static void writeScheduledTransitLeg(LegReference ref, ObjectOutputStream out) throws IOException {
if (ref instanceof ScheduledTransitLegReference s) {
out.writeUTF(s.tripId().toString());

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I couldn't immediately understand from the Java documentation how this works. How are the various parts separated from each other? Does ObjectOutputStream do it automatically?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I find the LegReference concept very interesting since some of my clients would like to have a feature to completely restore an itinerary including realtime updates.

The use case is the following: you buy a ticket for a particular itinerary which is saved on your phone. On the day of the travel you open the app and want to see the exact itinerary that you booked but also want to see the realtime updates for it.

Why am I talking about this? I think for this feature we want to future-proof those references a tiny bit. What to you think about including a version number in the encoded form?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, ObjectOutputStream is responsible for writing the values to the buffer, the various parts are not separated from each other, but instead it is the responsibility of the implementer to write and read them in the same order.

This feature can support that functionality, at the moment for only scheduled transit leg, but is is possible to create references to other types of legs as well. When we have an implementation for all types, it would be possible to concatenate the legs in order to create a reference to a complete itinerary. Already now, you could store references to all transit legs, and use them to fetch the status of each trip.

The LegReferenceType already has a version in the type name. That way we can have an individual version number for each type of leg.

tripPattern,
fromStopPositionInPattern,
toStopPositionInPattern,
GregorianCalendar.from(serviceDate.toZonedDateTime(timeZone, boardingTime)),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Totally out of scope but want to get your feedback: what do you think about replacing the Calendar type with something from java.time?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. It needs to be done for all Leg types

Comment thread src/main/java/org/opentripplanner/routing/stoptimes/AlternativeLegs.java Outdated

@leonardehrenfried leonardehrenfried left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I find this a very interesting feature and am happy to approve it.

If you don't want to give any guarantees about the future-proofing of the serialised form that is fine. I can add when it becomes an actual requirement, which at the moment it is not.

@hannesj
hannesj force-pushed the otp2_alternative_legs branch from 0f8658a to 769c934 Compare April 8, 2022 07:58
optionsome
optionsome previously approved these changes Apr 8, 2022
* Enum for different types of LegReferences
*/
enum LegReferenceType {
SCHEDULED_TRANSIT_LEG_V1(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

V1 = version 1? How will the versioning work here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We can have different types of references, and they can have different versions. Eg. we could add FREQUENCY_TRANSIT_LEG_V1 and FLEX_TRANSIT_LEG_V1 now, and if we at a later stage find out we need to adjust the serialization of the flex leg, we can add a FLEX_TRANSIT_LEG_V2, with updated serialization with the missing information.

boolean searchBackward
) {
TripPattern pattern = patternWithBoardAlightPositions.first;
int boardingPosition = patternWithBoardAlightPositions.second.first;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I know this might change in the future but this .second.first is a bit ambiguous

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I added custom record types for these.

@hannesj
hannesj dismissed stale reviews from optionsome and leonardehrenfried via 1e503ba April 8, 2022 10:59
@hannesj
hannesj merged commit d337863 into opentripplanner:dev-2.x Apr 8, 2022
t2gran pushed a commit that referenced this pull request Apr 8, 2022
@hannesj
hannesj deleted the otp2_alternative_legs branch April 8, 2022 12:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Entur Test This is currently being tested at Entur Entur On Entur Roadmap !New Feature A functional feature targeting the end user.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants