opentripplanner / OpenTripPlanner

An open source multi-modal trip planner
http://www.opentripplanner.org
Other
2.2k stars 1.03k forks source link

Plan query that follows the relay connection specification #4803

Closed optionsome closed 4 months ago

optionsome commented 1 year ago

Expected behavior

There should be a plan query that follows the relay connection specification.

Observed behavior

Unfortunately it's not possible to use the pagination functions of relay with the regular plan query in the GraphQL APIs.

Version of OTP used (exact commit hash or JAR name)

2.x

optionsome commented 1 year ago

I have created a draft schema for the legacy GraphQL API:

"""
    Plans an itinerary from point A to point B based on the given arguments. This query follows the
    relay connection specification https://relay.dev/graphql/connections.htm.
    """
    planConnection(
        """
        Date of departure or arrival in format YYYY-MM-DD. Default value: current date
        """
        date: String

        """
        Time of departure or arrival in format hh:mm:ss. Default value: current time
        """
        time: String

        """
        The geographical location where the itinerary begins.
        Use either this argument or `fromPlace`, but not both.
        """
        from: InputCoordinates

        """
        The geographical location where the itinerary ends.
        Use either this argument or `toPlace`, but not both.
        """
        to: InputCoordinates

        """
        The place where the itinerary begins in format `name::place`, where `place`
        is either a lat,lng pair (e.g. `Pasila::60.199041,24.932928`) or a stop id
        (e.g. `Pasila::HSL:1000202`).
        Use either this argument or `from`, but not both.
        """
        fromPlace: String

        """
        The place where the itinerary ends in format `name::place`, where `place` is
        either a lat,lng pair (e.g. `Pasila::60.199041,24.932928`) or a stop id
        (e.g. `Pasila::HSL:1000202`).
        Use either this argument or `to`, but not both.
        """
        toPlace: String

        """
        Whether the itinerary must be wheelchair accessible. Default value: false
        """
        wheelchair: Boolean

        """The maximum number of itineraries to return."""
        numItineraries: Int

        """
        The length of the search-window in seconds. This parameter is optional.

        The search-window is defined as the duration between the earliest-departure-time(EDT) and
        the latest-departure-time(LDT). OTP will search for all itineraries in this departure
        window. If `arriveBy=true` the `dateTime` parameter is the latest-arrival-time, so OTP
        will dynamically calculate the EDT. Using a short search-window is faster than using a
        longer one, but the search duration is not linear. Using a \"too\" short search-window will
        waste resources server side, while using a search-window that is too long will be slow.

        OTP will dynamically calculate a reasonable value for the search-window, if not provided.
        The calculation comes with a significant overhead (10-20% extra). Whether you should use the
        dynamic calculated value or pass in a value depends on your use-case. For a travel planner
        in a small geographical area, with a dense network of public transportation, a fixed value
        between 40 minutes and 2 hours makes sense. To find the appropriate search-window, adjust
        it so that the number of itineraries on average is around the wanted `numItineraries`. Make
        sure you set the `numItineraries` to a high number while testing. For a country wide area
        like Norway, using the dynamic search-window is the best.

        When paginating, the search-window is calculated using the `numItineraries` in the original
        search together with statistics from the search for the last page. This behaviour is
        configured server side, and can not be overridden from the client.

        The search-window used is returned to the response metadata as `searchWindowUsed` for
        debugging purposes.
        """
        searchWindow: Long,

        """
        Use the cursor to get the next or previous page of results.
        The next page is a set of itineraries departing after the last itinerary in this result and
        the previous page is a set of itineraries departing before the first itinerary.
        This is only usable when public transportation mode(s) are included in the query.
        """
        pageCursor: String

        """
        A multiplier for how bad biking is, compared to being in transit for equal
        lengths of time. Default value: 2.0
        """
        bikeReluctance: Float

        """
        A multiplier for how bad walking with a bike is, compared to being in transit for equal
        lengths of time. Default value: 5.0
        """
        bikeWalkingReluctance: Float

        """
        A multiplier for how bad driving is, compared to being in transit for equal
        lengths of time. Default value: 3.0
        """
        carReluctance: Float

        """
        A multiplier for how bad walking is, compared to being in transit for equal
        lengths of time. Empirically, values between 2 and 4 seem to correspond
        well to the concept of not wanting to walk too much without asking for
        totally ridiculous itineraries, but this observation should in no way be
        taken as scientific or definitive. Your mileage may vary. See
        https://github.com/opentripplanner/OpenTripPlanner/issues/4090 for impact on
        performance with high values. Default value: 2.0
        """
        walkReluctance: Float

        """
        How much worse is waiting for a transit vehicle than being on a transit
        vehicle, as a multiplier. The default value treats wait and on-vehicle time
        as the same. It may be tempting to set this higher than walkReluctance (as
        studies often find this kind of preferences among riders) but the planner
        will take this literally and walk down a transit line to avoid waiting at a
        stop. This used to be set less than 1 (0.95) which would make waiting
        offboard preferable to waiting onboard in an interlined trip. That is also
        undesirable. If we only tried the shortest possible transfer at each stop to
        neighboring stop patterns, this problem could disappear. Default value: 1.0.
        """
        waitReluctance: Float

        """
        Max walk speed along streets, in meters per second. Default value: 1.33
        """
        walkSpeed: Float

        """Max bike speed along streets, in meters per second. Default value: 5.0"""
        bikeSpeed: Float

        """Time to get on and off your own bike, in seconds. Default value: 0"""
        bikeSwitchTime: Int

        """
        Cost of getting on and off your own bike. Unit: seconds. Default value: 0
        """
        bikeSwitchCost: Int

        """
        Optimization type for bicycling legs, e.g. prefer flat terrain. Default value: `QUICK`
        """
        optimize: OptimizeType

        """
        Triangle optimization parameters for bicycling legs. Only effective when `optimize` is set to **TRIANGLE**.
        """
        triangle: InputTriangle

        """
        Whether the itinerary should depart at the specified time (false), or arrive
        to the destination at the specified time (true). Default value: false.
        """
        arriveBy: Boolean

        """
        List of routes and agencies which are given lower preference when planning the itinerary
        """
        unpreferred: InputUnpreferred

        """
        This prevents unnecessary transfers by adding a cost for boarding a vehicle. Unit: seconds. Default value: 600
        """
        walkBoardCost: Int

        """
        Separate cost for boarding a vehicle with a bicycle, which is more difficult
        than on foot. Unit: seconds. Default value: 600
        """
        bikeBoardCost: Int

        """
        List of routes, trips, agencies and stops which are not used in the itinerary
        """
        banned: InputBanned

        """
        An extra penalty added on transfers (i.e. all boardings except the first
        one). Not to be confused with bikeBoardCost and walkBoardCost, which are the
        cost of boarding a vehicle with and without a bicycle. The boardCosts are
        used to model the 'usual' perceived cost of using a transit vehicle, and the
        transferPenalty is used when a user requests even less transfers. In the
        latter case, we don't actually optimize for fewest transfers, as this can
        lead to absurd results. Consider a trip in New York from Grand Army Plaza
        (the one in Brooklyn) to Kalustyan's at noon. The true lowest transfers
        route is to wait until midnight, when the 4 train runs local the whole way.
        The actual fastest route is the 2/3 to the 4/5 at Nevins to the 6 at Union
        Square, which takes half an hour. Even someone optimizing for fewest
        transfers doesn't want to wait until midnight. Maybe they would be willing
        to walk to 7th Ave and take the Q to Union Square, then transfer to the 6.
        If this takes less than optimize_transfer_penalty seconds, then that's what
        we'll return. Default value: 0.
        """
        transferPenalty: Int

        """
        List of transportation modes that the user is willing to use. Default: `["WALK","TRANSIT"]`
        """
        transportModes: [TransportMode]

        """
        The weight multipliers for transit modes. WALK, BICYCLE, CAR, TRANSIT and LEG_SWITCH are not included.
        """
        modeWeight: InputModeWeight

        """
        Debug the itinerary-filter-chain. The filters will mark itineraries as deleted, but does NOT delete them when this is enabled.
        """
        debugItineraryFilter: Boolean

        """
        Whether arriving at the destination with a rented (station) bicycle is allowed without
        dropping it off. Default: false.
        """
        allowKeepingRentedBicycleAtDestination: Boolean

        """
        The cost of arriving at the destination with the rented bicycle, to discourage doing so.
        Default value: 0.
        """
        keepingRentedBicycleAtDestinationCost: Int

        """
        Invariant: `boardSlack + alightSlack <= transferSlack`. Default value: 0
        """
        boardSlack: Int

        """
        Invariant: `boardSlack + alightSlack <= transferSlack`. Default value: 0
        """
        alightSlack: Int

        """
        A global minimum transfer time (in seconds) that specifies the minimum
        amount of time that must pass between exiting one transit vehicle and
        boarding another. This time is in addition to time it might take to walk
        between transit stops. Default value: 0
        """
        minTransferTime: Int

        """
        When false, return itineraries using canceled trips. Default value: true.
        """
        omitCanceled: Boolean = true

        """
        When true, realtime updates are ignored during this search. Default value: false
        """
        ignoreRealtimeUpdates: Boolean

        """
        Two-letter language code (ISO 639-1) used for returned text.
        **Note:** only part of the data has translations available and names of
        stops and POIs are returned in their default language. Due to missing
        translations, it is sometimes possible that returned text uses a mixture of two languages.
        """
        locale: String

        """
        Which vehicle rental networks can be used. By default, all networks are allowed.
        """
        allowedVehicleRentalNetworks: [String]

        """
        Which vehicle rental networks cannot be used. By default, all networks are allowed.
        """
        bannedVehicleRentalNetworks: [String]

        """
        Factor for how much the walk safety is considered in routing. Value should be between 0 and 1.
        If the value is set to be 0, safety is ignored. Default is 1.0.
        """
        walkSafetyFactor: Float
        before: String
        after: String
        first: Int
        last: Int
    ): PlanConnection @async
}

type PlanConnection {
    """The time and date of travel. Format: Unix timestamp in milliseconds."""
    date: Long

    """The origin"""
    from: Place!

    """The destination"""
    to: Place!

    """A list of routing errors, and fields which caused them"""
    routingErrors: [RoutingError!]!

    """
    This is the `searchWindow` used by the raptor search. It is provided here for debugging
    purpousess.

    The unit is seconds.
    """
    searchWindowUsed: Long

    """Information about the timings for the plan generation"""
    debugOutput: debugOutput!

    edges: [PlanEdge]

    pageInfo: PageInfo!
}

type PlanEdge {
    node: Itinerary
    cursor: String!
}
optionsome commented 1 year ago

There is some issues with the implementation I still have to solve. The existing connection types have interfaces, for example LegacyGraphQLPlaceAtDistanceConnection that are not implemented anywhere. In this case, we would probably want to have some other variables on the connection type than the standard edges and pageInfo, and therefore an implementation for the interface is required.

leonardehrenfried commented 1 year ago

Is it only about adding edges and pageInfo? If so, couldn't it be made backwards-compatible rather than duplicating the largest resolver in the schema?

optionsome commented 1 year ago

We can discuss this in the dev meeting. I wasn't quite sure what was the optimal solution here. Having a lot of these "duplicate" cursor things in the schema can be confusing.

hannesj commented 1 year ago

@leonardehrenfried We need to change the names of the types and replace the cursor with the pagination arguments, so it is a bit complicated to do it in a backwards-compatible way.

leonardehrenfried commented 1 year ago

I'm more worried about different org using different resolvers and then splitting the GraphQL API into "sections that IBI" uses and "sections that HSL uses"...

leonardehrenfried commented 1 year ago

Lets discuss at the meeting but I'm in favour of a little breaking change if it means that we don't use different resolvers for different orgs.

hannesj commented 1 year ago

Why would we need a completely separate resolver?

leonardehrenfried commented 1 year ago

Ah, I thought that that is what @optionsome is proposing.

leonardehrenfried commented 1 year ago

Will the existing plan() query be deprecated?

t2gran commented 1 year ago

I do not have a strong opinion on this, but my advice would be to avoid living with two plan queries for long. Just having to maintain the two schemas (mostly the docs) is a maintenance burden. If you deprecate the old query then it become more eatable :smiley:

leonardehrenfried commented 1 year ago

I would be fine with it too, if we aggressively deprecate and eventually remove the old query.

optionsome commented 1 year ago

Yes that is ok for me as well.

optionsome commented 1 year ago

If I didn't understand it wrong, @hannesj suggested in the last developer meeting that I probably should keep mostly the arguments from the normal plan query in this new query. However, I think removing the deprecated/unimplemented arguments is fine as the resolver doesn't expect the parameters to be defined so we can still reuse it. It might cause some confusion for developers if the resolver tries to use the arguments but they are not in the schema, but I think that is not a major issue. What do you think? We can have another round of discussion today in the developer meeting.

leonardehrenfried commented 1 year ago

I think you should package up all (or at least some of) the parameters into an input object. That way reusing them for the via query and future iterations of plan() will have shared resources where you don't have to maintain two sets of documentation.

optionsome commented 1 year ago

Yes that would indeed be ideal but would that lessen the possibility to reuse the resolvers for the existing plan query unless I would also add the new input objects into the old plan query which would duplicate some of the arguments? I don't know if this refactoring should be done or in a future API version?

hannesj commented 1 year ago

Basically we have two choices now.

leonardehrenfried commented 1 year ago

I'm in favour of the second option. It may mean some pain now but it's also an opportunity to introduce some engineering and testing rigour into the API code.

optionsome commented 1 year ago

Ok, I can draft a suggestion that follows the second option. How locked are we on the idea of having separate queries for via point search btw? I have liked the OTP1 implementation as it's so simple to use in clients but having a separate query does provide some possibilities to do more complex stuff but it also puts the burden a bit on the clients.

optionsome commented 1 year ago

I wrote a quick draft for the planConnection arguments. If this looks even a bit correct, I can write a schema draft.

dateTimeOptions
  date: ISO8601DateScalar
  time: ISO8601TimeScalar
  arriveBy
locations
  origin
    @oneOf
    coordinate
    place
  destination
    @oneOf
    coordinate
    place
streetRoutingPreferences
  modePreferences
    directModes: StreetMode
    modes: StreetMode
  bikePreferences
    bikeReluctance
    bikeWalkingReluctance
    bikeSpeed
    bikeSwitchTime
    bikeSwitchCost
    optimize
      @oneOf
      type 
        QUICK | SAFE | FLAT | GREENWAYS
      triangle
        safetyFactor
        slopeFactor
        timeFactor
  carPreferences
    carReluctance
  walkPreferences
    walkSpeed
    walkReluctance
    walkSafetyFactor
  rentalPreferences
    allowKeepingRentedVehicleAtDestination
    keepingRentedVehicleAtDestinationCost
    allowedVehicleRentalNetworks
    bannedVehicleRentalNetworks
transitRoutingPreferences
  numItineraries
  searchWindow
  unpreferred
  banned
  boardPreferences
    waitReluctance
    boardSlack
    walkBoardCost
    bikeBoardCost
  alightPreferences
    alightSlack
  transferPreferences
    transferPenalty
    minTransferTime
  realtimePreferences
    omitCanceled
    ignoreRealtimeUpdates
  modePreferences
    modes: TransitMode
    reluctanceForMode: [{TransitMode!, Float!}] #TODO float is probably not correct here as we will support cost functions in the future
accessibility
  enabled: boolean
debugSettings
  debugItineraryFilter
locale
before
after
first
last

While I'm at it, this would be the perfect possibility to also create an ItineraryV2 type to return from the planConnection but getting the itinerary type right is a much more difficult task.... and there is a possibility of getting into a really deep rabbit hole as might as well rewrite stop/station types etc. What do you think?

leonardehrenfried commented 1 year ago

I think this looks great! I have a couple of smaller comments but I will make them on the schema itself when it's published as I can't make line comments here.

About ItineraryV2 I'm also not sure. I personally would try hard to extend the current itinerary and deprecate fields but that is just my personal style. I'm not good at grand visions.

optionsome commented 1 year ago

Should we have scooterPreferences in the schema or just document that the bike preferences are also used for scooters (at least I think they are, I'm not sure)?

optionsome commented 1 year ago

I had some second thoughts over the weekend about the "locations" wrapper over origin/destination. The name doesn't make sense if we re-implement the startTransitTripId from OTP1. Should the origin and destination just be on the top level?

leonardehrenfried commented 1 year ago

I think we can deal with startTransitTripId separately. Wouldn't it be another @oneOf of origin?

optionsome commented 1 year ago

Yes but if the origin is under locations, transit trip is not really a location.

leonardehrenfried commented 1 year ago

That's a good point.

hannesj commented 1 year ago
    """
    Date of departure or arrival in format YYYY-MM-DD. Default value: current date
    """
    date: String

    """
    Time of departure or arrival in format hh:mm:ss. Default value: current time
    """
    time: String

Is there a reason not specifying these in a single timestamp. ~Also they should have proper scalars.~ Also, what about timezone?

optionsome commented 1 year ago

I created a new draft version:

dateTimeOptions
  @oneOf #this can be removed when we support defining both
  departure
    dateTime: ISO8601DateTimeScalar
    maxBeforeOffset: ISO8601DurationScalar (can be negative) 
    maxAfterOffset: ISO8601DurationScalar (can be negative) 
  arrival
    dateTime: ISO8601DateTimeScalar
    maxBeforeOffset: ISO8601DurationScalar (can be negative) 
    maxAfterOffset: ISO8601DurationScalar (can be negative)
locations
  origin
    @oneOf
    coordinate
    place
  destination
    @oneOf
    coordinate
    place
preferences
  streetRouting
    modes
      direct: StreetMode
      access: StreetMode
      egress: StreetMode
      transfer: StreetMode
    bike
      reluctance
      bikeWalkingReluctance
      speed
      switchTime
      switchCost
      optimize
        @oneOf
        type 
          QUICK | SAFE | FLAT | GREENWAYS
        triangle
          safetyFactor
          slopeFactor
          timeFactor
    scooter
      reluctance
      scooterWalkingReluctance
      speed
      switchTime
      switchCost
      optimize
        @oneOf
        type 
          QUICK | SAFE | FLAT | GREENWAYS
        triangle
          safetyFactor
          slopeFactor
          timeFactor
    car
      reluctance
    walk
      speed
      reluctance
      safetyFactor
    rental
      allowKeepingVehicleAtDestination
      keepingVehicleAtDestinationCost
      allowedNetworks
      bannedNetworks
  transitRouting
    numItineraries #TODO should we have separate number of itineraries before/after defined times
    unpreferred
    banned
    board
      waitReluctance
      slack
      walkBoardCost
      bikeBoardCost
    alight
      slack
    transfer
      penalty
      minimumTime
    realtime
      omitCanceled
      ignoreRealtimeUpdates
    modes
      modes: TransitMode
      reluctance: [{TransitMode!, {@oneOf Float, cost function String (or maybe Scalar?)}}]
  accessibility
    enabled: boolean
debugSettings
  debugItineraryFilter
locale
before
after
first
last

Regarding to @hannesj comment, isn't it possible to define the timezone as part of the ISO8601 datetime (or time)?

The changes for this draft from the previous draft:

t2gran commented 1 year ago

Feel free to do as much of my suggestions as you like - I do not think we will be able to reuse much when we design the new "unified" API.

t2gran commented 1 year ago

This is related https://github.com/opentripplanner/OpenTripPlanner/issues/3217.

t2gran commented 1 year ago

Not sure if you want to change the returned itinerary, but removing access, egress and transfer legs with duration=0 creates problems, because these legs may have other information like cost, notices/alerts and so on. Instead we should always return these and hide them from the user in the client.

github-actions[bot] commented 1 year ago

This issue is stale because it has been open 90 days with no activity. Remove stale label or comment or this will be closed in 30 days

optionsome commented 1 year ago
  • @optionsome I suggest you keep the "latest" draft of the schema in the issue description. If someone comments you can just reply with a emoji, and update the schema. If you approve, we can also edit the schema and add comments inside, then if you approve - just delete the comment. Here is an issue I updated 32 times, if you would like to see how the history works. Feel free to change the default headers.

I think I will next open a pr where I will create the actual schema draft. I think it's easier to comment a pr and there are things there I can already start working on even if there are still some things to be decided

* What about: `preferences { street { bike { reluctance, speed, bikeWalking : WalkPreferences }, car {}, scooter{}, walk : WalkPreferences }, transit {}, accessibility : AccessibilityPreferences }`

I think reusing the WalkPreferences could work here but there are also pitfalls. I think currently we do not support setting different walk safetyFactor for bike walking, I don't know if we should. We might also add new parameters in the future that we don't/can't support separately for bike walking. If we do not support all the walk preferences for bike walking, then the user experience will be confusing as we will probably always use those unsupported ones from the walk preferences.

* `AccessibilityPreferences` - Even if accessibility is wheelchair-accessibility now, we should have a wrapper class here. It should be part of preferences, or we should treat it like we treat other modes - I am in favor of the later.

I'm not sure if I understand this. Could you elaborate these two approaches.

* Consider using ZonedDateTime for all instance of time - you may default to `now()`. Pasting "2023-02-15T12:00Z" is not so hard anyway.

Was there something in the schema draft currently which was not suitable for ZonedDateTime?

* We support 3 modes for time: TIMETABLE, DEPART_AFTER and ARRIVE_BY - I think the API will be easier to understand if you drop the old flags `timetable` and `arriveBy` and replaces it with `timeMode/searchMode` (name can be discucced).

In the current draft, the depart after/arrive by are defined under dateTimeOptions. I didn't think of the timetable mode when I made the draft and I have forgotten what the timetable mode even did. Was it documented somewhere?

* Can you include the Input types in the schema as well, then we avoid missunderstandings. Skip the comments for now, unless they are important for the design.

I will create a real draft schema this week so this will be covered.

t2gran commented 1 year ago

I think reusing the WalkPreferences could work here but there are also pitfalls. I think currently we do not support setting different walk safetyFactor for bike walking, I don't know if we should. We might also add new parameters in the future that we don't/can't support separately for bike walking. If we do not support all the walk preferences for bike walking, then the user experience will be confusing as we will probably always use those unsupported ones from the walk preferences.

I think you can use an interface for this - maybe that is better. I do chair your consern about the reuse.

optionsome commented 1 year ago

I think you can use an interface for this - maybe that is better. I do chair your consern about the reuse.

Unfortunately GraphQL doesn't allow interfaces for input types.

github-actions[bot] commented 1 year ago

This issue is stale because it has been open 90 days with no activity. Remove stale label or comment or this will be closed in 30 days

github-actions[bot] commented 10 months ago

This issue is stale because it has been open 90 days with no activity. Remove stale label or comment or this will be closed in 30 days

github-actions[bot] commented 7 months ago

This issue is stale because it has been open 90 days with no activity. Remove stale label or comment or this will be closed in 30 days