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

Add previousLegs into GTFS GraphQL API #6142

Merged
merged 12 commits into from
Nov 14, 2024
Merged

Add previousLegs into GTFS GraphQL API #6142

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
124f589
add previousLegs into GTFS GraphQL API
miklcct Oct 10, 2024
364a085
remove duplicate imports from generated files
miklcct Oct 11, 2024
1f0cceb
Merge remote-tracking branch 'upstream/dev-2.x' into graphql-previous…
miklcct Oct 15, 2024
ccd001d
Merge branch 'dev-2.x' into graphql-previousLegs
miklcct Oct 22, 2024
c715329
use an enum instead of boolean to switch next / previous alternative …
miklcct Oct 29, 2024
22c1f73
rename nextOrPreviousLegs to alternativeLegs
miklcct Oct 29, 2024
30c787b
rename SearchMode to SearchDirection
miklcct Oct 29, 2024
c780d5b
Merge branch 'dev-2.x' into graphql-previousLegs
miklcct Nov 5, 2024
0482fb7
move SearchDirection to a separate class and rename to SearchTime
miklcct Nov 5, 2024
e129934
move SearchDirection to a separate class and rename to SearchTime
miklcct Nov 5, 2024
64d3978
rename the enum again
miklcct Nov 11, 2024
d437d66
rename parameters
miklcct Nov 13, 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
11 changes: 10 additions & 1 deletion application/src/main/java/org/opentripplanner/apis/gtfs/datafetchers/LegImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,8 +275,17 @@ private Leg getSource(DataFetchingEnvironment environment) {
return environment.getSource();
}

@Override
public DataFetcher<Iterable<Leg>> previousLegs() {
return nextOrPreviousLegs(true);
}

@Override
public DataFetcher<Iterable<Leg>> nextLegs() {
return nextOrPreviousLegs(false);
}

private DataFetcher<Iterable<Leg>> nextOrPreviousLegs(boolean includeDepartBefore) {
Copy link
Member

Choose a reason for hiding this comment

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

This includeDepartBefore is a slightly difficult to understand boolean name (I know the name is copied over from the getAlternativeLegs method) but I think you could give it some more understandable name like previousLegsOnly. In general, we try to avoid boolean method variables that change the behavior. Instead, we prefer to split the methods into two different methods where the name reflects what it does. However, in this case you are just repeating the design decision made in the getAlternativeLegs method so it might be ok. We can discuss this in a developer meeting.

Copy link
Member

Choose a reason for hiding this comment

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

We discussed this in the developer meeting and came to the conclusion that you should split this private method and also getAlternativeLegs into two methods. One for previous legs and one for next legs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The previousLegs and nextLegs are doing the same thing, with the only difference in direction. I don't want to duplicate codes so I will switch to an enum instead.

Copy link
Member

Choose a reason for hiding this comment

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

Enum isn't really any better than boolean. You can put some of the code that computes the other arguments into methods that can be used by both the nextLegs and previousLegs methods so you can avoid some duplication that way.

Copy link
Contributor Author

@miklcct miklcct Oct 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think I can do anything better than converting the boolean to an enum, as the result will be much more complicated.

The single flag is used for multiple purposes:

  • to sort the departures within a pattern
  • to filter the results to be before / after the requested time
  • to sort the leg results after combining them from different patterns

If I don't use such a flag, I will need two more parameters for generateLegs, one to sort the results, one to compare the stop times in the priority queue. I will also need another parameter for getAlternativeLegs to sort the legs across different trip patterns as well, so it will be 3 different interrelated new arguments passed down the call stack instead of a single flag, if these arguments become out of sync strange behavior will result.

nextLegs and previousLegs are basically the same thing. They are not different behavior. The same argument can be said of journey planning by departure time or arrival time which should be the same logic, yet I am seeing bugs which happens on arrival only but not departure (#6102).

Similarly, there are other flags in getAlternativeLegs like exactOriginStop, exactDestinationStop, filter which can't be split into different methods as well.

return environment -> {
if (environment.getSource() instanceof ScheduledTransitLeg originalLeg) {
var args = new GraphQLTypes.GraphQLLegNextLegsArgs(environment.getArguments());
Expand Down Expand Up @@ -311,7 +320,7 @@ public DataFetcher<Iterable<Leg>> nextLegs() {
environment.getSource(),
numberOfLegs,
environment.<GraphQLRequestContext>getContext().transitService(),
false,
includeDepartBefore,
AlternativeLegsFilter.NO_FILTER,
limitToExactOriginStop,
limitToExactDestinationStop
Expand Down
2 changes: 2 additions & 0 deletions application/src/main/java/org/opentripplanner/apis/gtfs/generated/GraphQLDataFetchers.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -503,6 +503,8 @@ public interface GraphQLLeg {

public DataFetcher<String> pickupType();

public DataFetcher<Iterable<Leg>> previousLegs();

public DataFetcher<Boolean> realTime();

public DataFetcher<GraphQLRealtimeState> realtimeState();
Expand Down
63 changes: 63 additions & 0 deletions application/src/main/java/org/opentripplanner/apis/gtfs/generated/GraphQLTypes.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1268,6 +1268,69 @@ public void setGraphQLOriginModesWithParentStation(
}
}

public static class GraphQLLegPreviousLegsArgs {

private List<GraphQLTransitMode> destinationModesWithParentStation;
private Integer numberOfLegs;
private List<GraphQLTransitMode> originModesWithParentStation;

public GraphQLLegPreviousLegsArgs(Map<String, Object> args) {
if (args != null) {
if (args.get("destinationModesWithParentStation") != null) {
this.destinationModesWithParentStation =
((List<Object>) args.get("destinationModesWithParentStation")).stream()
.map(item ->
item instanceof GraphQLTransitMode
? item
: GraphQLTransitMode.valueOf((String) item)
)
.map(GraphQLTransitMode.class::cast)
.collect(Collectors.toList());
}
this.numberOfLegs = (Integer) args.get("numberOfLegs");
if (args.get("originModesWithParentStation") != null) {
this.originModesWithParentStation =
((List<Object>) args.get("originModesWithParentStation")).stream()
.map(item ->
item instanceof GraphQLTransitMode
? item
: GraphQLTransitMode.valueOf((String) item)
)
.map(GraphQLTransitMode.class::cast)
.collect(Collectors.toList());
}
}
}

public List<GraphQLTransitMode> getGraphQLDestinationModesWithParentStation() {
return this.destinationModesWithParentStation;
}

public Integer getGraphQLNumberOfLegs() {
return this.numberOfLegs;
}

public List<GraphQLTransitMode> getGraphQLOriginModesWithParentStation() {
return this.originModesWithParentStation;
}

public void setGraphQLDestinationModesWithParentStation(
List<GraphQLTransitMode> destinationModesWithParentStation
) {
this.destinationModesWithParentStation = destinationModesWithParentStation;
}

public void setGraphQLNumberOfLegs(Integer numberOfLegs) {
this.numberOfLegs = numberOfLegs;
}

public void setGraphQLOriginModesWithParentStation(
List<GraphQLTransitMode> originModesWithParentStation
) {
this.originModesWithParentStation = originModesWithParentStation;
}
}

public static class GraphQLLocalDateRangeInput {

private java.time.LocalDate end;
Expand Down
18 changes: 18 additions & 0 deletions application/src/main/resources/org/opentripplanner/apis/gtfs/schema.graphqls
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -722,6 +722,24 @@ type Leg {
pickupBookingInfo: BookingInfo
"This is used to indicate if boarding this leg is possible only with special arrangements."
pickupType: PickupDropoffType
"Previous legs with same origin and destination stops or stations"
previousLegs(
"""
Transportation modes for which all stops in the parent station are used as possible destination stops
for the previous legs. For modes not listed, only the exact destination stop of the leg is considered.
"""
destinationModesWithParentStation: [TransitMode!],
"""
The number of alternative legs searched. If fewer than the requested number are found,
then only the found legs are returned.
"""
numberOfLegs: Int!,
"""
Transportation modes for which all stops in the parent station are used as possible origin stops
for the previous legs. For modes not listed, only the exact origin stop of the leg is considered.
"""
originModesWithParentStation: [TransitMode!]
): [Leg!]
"Whether there is real-time data about this Leg"
realTime: Boolean
"State of real-time data"
Expand Down
Loading