Add query for cancelled trips to GTFS GraphQL API

[vesameskanen]

Summary

This PR adds a query, which returns information about cancelled trips, into Gtfs graphQL API. The query supports paging and filtering by feed id. It returns a list of (trip, date) pairs.

Unit tests

None. The new code has been tested manually using graphiQL endpoint. Changing trip selector in DefaultTransitService class temporarily to return UPDATED (not CANCELED) trips helps to collect test trips with any realtime modified data.

Documentation

Docs in the graphQL schema

[codecov]

Codecov Report

Attention: Patch coverage is 70.54264% with 76 lines in your changes missing coverage. Please review.

Project coverage is 69.81%. Comparing base (e919778) to head (5217ce5).

Files with missing lines	Patch %	Lines
...ntripplanner/apis/gtfs/generated/GraphQLTypes.java	0.00%	19 Missing ⚠️
...planner/transit/service/DefaultTransitService.java	31.81%	12 Missing and 3 partials ⚠️
...ipplanner/apis/gtfs/datafetchers/StopCallImpl.java	66.66%	4 Missing and 6 partials ⚠️
.../apis/gtfs/datafetchers/TripOnServiceDateImpl.java	82.97%	4 Missing and 4 partials ⚠️
...ntripplanner/apis/gtfs/mapping/PickDropMapper.java	28.57%	4 Missing and 1 partial ⚠️
...pentripplanner/apis/gtfs/datafetchers/LegImpl.java	20.00%	4 Missing ⚠️
...g/opentripplanner/ext/flex/FlexibleTransitLeg.java	0.00%	2 Missing ⚠️
...va/org/opentripplanner/apis/gtfs/GraphQLUtils.java	33.33%	1 Missing and 1 partial ⚠️
...fs/datafetchers/CallScheduledTimeTypeResolver.java	66.66%	1 Missing and 1 partial ⚠️
...tfs/datafetchers/CallStopLocationTypeResolver.java	66.66%	1 Missing and 1 partial ⚠️
... and 4 more

Additional details and impacted files

@@              Coverage Diff              @@
##             dev-2.x    #5393      +/-   ##
=============================================
+ Coverage      69.78%   69.81%   +0.02%     
- Complexity     17781    17856      +75     
=============================================
  Files           2017     2030      +13     
  Lines          76040    76234     +194     
  Branches        7781     7799      +18     
=============================================
+ Hits           53067    53222     +155     
- Misses         20269    20288      +19     
- Partials        2704     2724      +20     

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

[vesameskanen]

My current understanding of what should be done:

• Change the serviceDate of DatedTrip record to the new date+time scalar Joel is working on (i.e return a single datetime scalar instead of separate date and departure time )
• Add Trip fields which are represented in bad way which we wish to deprecate to DatedTrip
• Add fields which we expect to depend on the resolved realtime context to DatedTrip (headsign? realtime status?)

While doing this, we should not add features which are not needed yet, or unnecessarily duplicated fields of the Trip. In my opinion, duplicating a field from the Trip object to DatedTrip level indicates that the values presumably differ.

[leonardehrenfried]

Yes, I would keep the implementation of DatedTrip to an absolute minimum and only include what you need right now.

@optionsome however thinks that you should include stop times. I can live without them and can add them when I need it.

[leonardehrenfried]

@vesameskanen can you be the second reviewer?

[optionsome]

This is ready for review again. I didn't end up major refactorings to the schema. If @t2gran really wants to use one TripOnServiceDate type for all trips, I can do it, but I would prefer to model trips that have some flex properties with a separate type.

[optionsome]

I've updated this based on yesterday's discussion. I ended up using a union for the stop call because the structure of the call would be completely different if there is a scheduled time window. The time window should be modified separately from the estimates vs how it makes sense to group schedules and estimates together when a time window is not in use.

[leonardehrenfried]

This code is repeated several times.

[leonardehrenfried]

Very nice.

[leonardehrenfried]

👍

[leonardehrenfried]

I've taken the liberty to add a module test for cancelling a trip: leonardehrenfried@0423999

Can you check it out and if you like it, apply it to this branch.

[leonardehrenfried]

I'm fine with the schema now but I made a few suggestion about the implementation.

However, I find our way of getting actual times from a TripOnServiceDate viat the Timetable quite strange. This is of course not your fault and I hope this will be tackled in the upcoming refactorings.

[t2gran]

There has been a long discussion about the API schema above. I just want to summarise my input for future reference. I quickly put together an "API domain model" for this. The diagram below is my suggestion to the design, it follows the guideline in this PR.

In the future I want us to draw diagrams like the on below - they are much easier to read and discuss. If we draw at a API level or OTP domain level probably does not matter - but try to focus on the entities, value-objects and the relationships. When we agree on the "language", then it become a technical task to map it to a GraphQL schema or Java model.

Diagram

1. Stuff in red exist in OTP, but should not be included in the API now - only when it is requested.
2. TripOnDate - Service is not part of the name, because there is no ambiguity - having TripOnRunningDate does not make any sence.
3. There is no support for flex in the first version, but letting stop be of type StopLocation and scheduledTime optional allow us to reuse this if we want. The is a big IF, but the point is to be able to - not forced. There are open questions:
   1. Can the scheduled stop be e.g. a GroupStop and the estimated/actual stop be a RegularStop - I think this make perfect sence - but the business rules is not known for RT on FLEX.
   2. Flex estinated/actual times should not be a window in my view. If a plan for the flex trip exist the route should be planned and there should be estimates for when to arrive where. There might be more stop-locations than in the scheduled plan - but again this must be analysed. My guess is that we will not be able to guess the final solution here today, so it is pointless to put to much of it in the design.
4. scheduledTime should be optimal, ADDED Trips does not have a schedule - we face it today, but I am not sure if this is ok. Also, for Flex the scheduled time might be a time-instant or a time-window.
5. I would like to find a better name for ArrivalDepartureTime, but has not done it. I like to find good names for value-objects types - these are the easiest types to reuse and they do not need to change. If a change is need, the entity using it can be changed instead. Value-objects are also easy to use for the client, reusing logic to handle it.
6. Departure and arrival-times are not required. This is also a simplification we have done in the internal model witch has leaked out. I am not worried here, and it might be just much easier for the client if we say they are required. If needed we can add information about the origin [OTP generated/feed] of these fields instead.
7. I have focused on the "domain" not the "graph QL" schema - I have not included all fields, and some names are probably wrong.
8. Note! there is no interface for the ArrivalDepartureTime, ArrivalDepartureTimeWindow and EstimatedArrivalDepartureTimeArrivalDepartureEstimatedTime - these types does not have anything in common. You could argue that there should be a union for the scheduled time/window - I would suggest it should be CallScheduledTime - it belong to Call and should not be reused in other places, so call need to be part of the name. I suggest using the owner type as a prefix.
9. Using union for call->stop is also ok for example CallStopLocation.

[leonardehrenfried]

@optionsome You have conflicts again.

[optionsome]

2. `TripOnDate` - `Service` is not part of the name, because there is no ambiguity - having `TripOnRunningDate` does not make any sence.

I think people can confuse the concept of a normal date vs service date.

3. There is no support for flex in the first version, but letting stop be of type StopLocation and scheduledTime optional allow us to reuse this if we want. The is a big IF, but the point is to be able to - not forced. There are open questions:
   
   1. Can the scheduled stop be e.g. a GroupStop and the estimated/actual stop be a RegularStop - I think this make perfect sence - but the business rules is not known for RT on FLEX.
   2. Flex estinated/actual times should not be a window in my view. If a plan for the flex trip exist the route should be planned and there should be estimates for when to arrive where. There might be more stop-locations than in the scheduled plan - but again this must be analysed. My guess is that we will not be able to guess the final solution here today, so it is pointless to put to much of it in the design.

The thing is, if we don't make the TripOnDate a union in the schema, "There might be more stop-locations than in the scheduled plan" issue (or similar issues) might be difficult to solve. Making the calls a union/interface could lessen these types of issues but I don't know if even that is a solution for the aforementioned issue.

5. I would like to find a better name for `ArrivalDepartureTime`, but has not done it. I like to find good names for value-objects types - these are the easiest types to reuse and they do not need to change. If a change is need, the entity using it can be changed instead. Value-objects are also easy to use for the client, reusing logic to handle it.

I think we decided to call it CallTime.

6. Departure and arrival-times are not required. This is also a simplification we have done in the internal model witch has leaked out. I am not worried here, and it might be just much easier for the client if we say they are required. If needed we can add information about the origin [OTP generated/feed] of these fields instead.

I didn't make them required because I'm not sure if we always will have arrival time for the first stop or departure time for the last stop in the future.

7. I have focused on the "domain" not the "graph QL" schema - I have not included all fields, and some names are probably wrong.

In general, I think designing the domain is a good idea, however I think designing the schema is even more important since changing that is much more difficult.

8. Note! there is no interface for the `ArrivalDepartureTime`,  `ArrivalDepartureTimeWindow` and `EstimatedArrivalDepartureTime` - these types does not have anything in common. You could argue that there should be a union for the scheduled time/window - I would suggest it should be `ScheduledCallTime` - it belong to `Call` and should not be reused in other places, so call need to be part of the name.

I wasn't quite sure what is the best model for this. Should the scheduled/estimates be grouped together inside departure or arrival fields or should there be schedules and estimates which have departure and arrival fields. For the non-flex trips, grouping the estimates and schedules together feels more natural. However, with flex time windows it's not possible because there is no scheduled arrival or departure. That was the reason why I ended up using a union for the Calls so these could be modeled in different fashion.

[optionsome]

We did some schema design with @t2gran, I'll update this pr to match the design later:

{
  calls {
    stopLocation = (Stop | StopArea | GroupOfStops) : CallStopLocation Union
    scheduled {
      plannedStatus = SKIPPED/SCHEDULED
      time { (TimeWindow | ArrivalDepartureTime) : CallScheduledTime
        timeWindow {
          start OffsetDateTime
          end OffsetDateTime
        }
        or
        {
          arrival OffsetDateTime
          departure OffsetDateTime
        }
      }
    }
    realTime CallRealTime  {
      cancelled = SKIPPED/SCHEDULED
      arrival EstimatedTime {
        time
        delay
        uncertainty percentage
        recorded RECORDED/ESTIMATE/UNKNOWN
      }
      departure {
        time
        delay
        isActualTime = false
      }
    }
  }
}

[optionsome]

I've implemented new version of schema now. I'm not quite sure where some of the model classes needed by the API should be. They are now in the GTFS API package and elsewhere which is probably not ideal at least.

[optionsome]

The schema design we did with @t2gran includes some things that I have not implemented in the scope of this pr as they are not needed for this.