Add previousLegs into GTFS GraphQL API

[miklcct]

PR Instructions

Add previousLegs into GTFS GraphQL API in parallel of nextLegs. This already exists in Transmodel API.

Issue

Closes #6139

Unit tests

The fetcher code is not tested at all.

Documentation

Added to GraphQL schema

[habrahamsson-skanetrafiken]

This has some merge conflicts. Can you fix them?

[codecov]

Codecov Report

Attention: Patch coverage is 24.39024% with 31 lines in your changes missing coverage. Please review.

Project coverage is 69.71%. Comparing base (ee53e50) to head (d437d66).
Report is 171 commits behind head on dev-2.x.

Files with missing lines	Patch %	Lines
...ntripplanner/apis/gtfs/generated/GraphQLTypes.java	0.00%	31 Missing ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##             dev-2.x    #6142      +/-   ##
=============================================
- Coverage      69.91%   69.71%   -0.21%     
+ Complexity     17736    17685      -51     
=============================================
  Files           2006     2008       +2     
  Lines          75526    75806     +280     
  Branches        7730     7759      +29     
=============================================
+ Hits           52804    52847      +43     
- Misses         20036    20247     +211     
- Partials        2686     2712      +26     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[optionsome]

You could try adding GraphQL integration tests for this field in https://github.com/opentripplanner/OpenTripPlanner/blob/dev-2.x/application/src/test/resources/org/opentripplanner/apis/gtfs/queries/planConnection-extended.graphql . I'm not sure how difficult it is, you might need to edit https://github.com/opentripplanner/OpenTripPlanner/blob/dev-2.x/application/src/test/java/org/opentripplanner/apis/gtfs/GraphQLIntegrationTest.java as well.

[miklcct]

You could try adding GraphQL integration tests for this field in https://github.com/opentripplanner/OpenTripPlanner/blob/dev-2.x/application/src/test/resources/org/opentripplanner/apis/gtfs/queries/planConnection-extended.graphql . I'm not sure how difficult it is, you might need to edit https://github.com/opentripplanner/OpenTripPlanner/blob/dev-2.x/application/src/test/java/org/opentripplanner/apis/gtfs/GraphQLIntegrationTest.java as well.

The integration test uses a fake, hardcoded routing response from a stub routing service which has no relationship to the stub transit service, which makes me impossible to cross reference between these two in tests.

These fetchers are basically untestable in the current setup because it queries the transit service using something returned from the routing service.

[optionsome]

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.

[optionsome]

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.

[miklcct]

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.

[optionsome]

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.

[miklcct]

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.

[miklcct]

I'll need to discuss the introduction of an utility enum SearchDirection with FORWARD and BACKWARD values for the whole of OpenTripPlanner, as I need to merge departure / arrival handling for frequencies in #6211.

[leonardehrenfried]

Please replace the variable name.