-
Notifications
You must be signed in to change notification settings - Fork 21
feat(ApiWeb): add /transfers endpoint
#1074
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.
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
base: master
Are you sure you want to change the base?
Changes from all commits
6a5be6b
2ad01de
54d6024
393b1ef
a9dc8a1
8267d69
d3c129b
4816ce3
faa74f9
482708e
3b92cce
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,182 @@ | ||
| defmodule ApiWeb.TransferController do | ||
| @moduledoc """ | ||
| Controller for Transfers. Filterable by: | ||
|
|
||
| * from_trip (multiple) | ||
| * type (multiple) | ||
| """ | ||
| use ApiWeb.Web, :api_controller | ||
|
|
||
| @filters ~w(from_trip type)s | ||
| @pagination_opts ~w(offset limit)a | ||
|
|
||
| def state_module, do: State.Transfer | ||
|
|
||
| swagger_path :index do | ||
| get(path(__MODULE__, :index)) | ||
|
|
||
| description(""" | ||
| **NOTE:** `filter[type]` or `filter[from_trip]` **MUST** be present for any transfers to be returned. | ||
|
|
||
| List of transfers. Transfer specifies additional rules and overrides for a transfer between trips, routes, and/or stops. | ||
|
|
||
| ## Transfers of a certain type | ||
|
|
||
| `/transfers?filter[type]=TYPE` | ||
|
|
||
| ## Transfers from a certain trip | ||
|
|
||
| `/transfers?filter[from_trip]=TRIP_ID` | ||
| """) | ||
|
|
||
| common_index_parameters(__MODULE__, :transfer) | ||
|
|
||
| parameter( | ||
| "filter[from_trip]", | ||
| :query, | ||
| :string, | ||
| "Filter by trip ID. Multiple trips #{comma_separated_list()}." | ||
| ) | ||
|
|
||
| parameter( | ||
| "filter[type]", | ||
| :query, | ||
| :string, | ||
| "Filter by transfer type. Multiple types #{comma_separated_list()}." | ||
| ) | ||
|
|
||
| consumes("application/vnd.api+json") | ||
| produces("application/vnd.api+json") | ||
| response(200, "OK", Schema.ref(:Transfer)) | ||
| response(400, "Bad Request", Schema.ref(:BadRequest)) | ||
| response(403, "Forbidden", Schema.ref(:Forbidden)) | ||
| response(429, "Too Many Requests", Schema.ref(:TooManyRequests)) | ||
| end | ||
|
|
||
| def index_data(conn, params) do | ||
| case Params.filter_params(params, @filters, conn) do | ||
| {:ok, filters} when map_size(filters) > 0 -> | ||
| filters | ||
| |> format_filters() | ||
| |> State.Transfer.filter_by() | ||
| |> State.all(Params.filter_opts(params, @pagination_opts, conn)) | ||
|
|
||
| {:error, _, _} = error -> | ||
| error | ||
|
|
||
| _ -> | ||
| {:error, :filter_required} | ||
| end | ||
| end | ||
|
|
||
| defp format_filters(filters) do | ||
| filters | ||
| |> Enum.flat_map(&do_format_filter/1) | ||
| |> Enum.into(%{}) | ||
| end | ||
|
|
||
| defp do_format_filter({"from_trip", trip_string}) do | ||
| case Params.split_on_comma(trip_string) do | ||
| [] -> | ||
| [] | ||
|
|
||
| trip_ids -> | ||
| %{from_trip_ids: trip_ids} | ||
| end | ||
| end | ||
|
|
||
| defp do_format_filter({"type", type_string}) do | ||
| case Params.split_on_comma(type_string) do | ||
| [] -> | ||
| [] | ||
|
|
||
| types -> | ||
| %{types: types} | ||
| end | ||
| end | ||
|
|
||
| defp do_format_filter(_), do: [] | ||
|
|
||
| # No show action here | ||
| def show_data(_conn, _params), do: [] | ||
|
|
||
| def swagger_definitions do | ||
| import PhoenixSwagger.JsonApi, except: [page: 1] | ||
|
|
||
| %{ | ||
| TransferResource: | ||
| resource do | ||
| description("Transfer specifies additional rules and overrides for a transfer.") | ||
|
|
||
| attributes do | ||
| min_transfer_time( | ||
| :integer, | ||
| "Sum of `min_walk_time` and `suggested_buffer_time`, in seconds.", | ||
| "x-nullable": true, | ||
| example: 9 | ||
| ) | ||
|
|
||
| min_walk_time( | ||
| :integer, | ||
| "Experimental. Minimum time required to travel by foot from `from_stop_id` to `to_stop_id`, in seconds.", | ||
| "x-nullable": true, | ||
| example: 4 | ||
| ) | ||
|
|
||
| min_wheelchair_time( | ||
| :integer, | ||
| "Experimental. Minimum time required to travel by wheelchair `from_stop_id` to `to_stop_id`, in seconds. If the transfer is not wheelchair accessible, this field will be blank.", | ||
| "x-nullable": true, | ||
| example: 7 | ||
| ) | ||
|
|
||
| suggested_buffer_time( | ||
| :integer, | ||
| "Experimental. Recommended buffer time to allow to make a successful transfer between two services, in seconds. This is also partly based on the significance of missing the transfer (due to service frequency).", | ||
| "x-nullable": true, | ||
| example: 5 | ||
| ) | ||
|
thecristen marked this conversation as resolved.
|
||
|
|
||
| transfer_type( | ||
| :integer, | ||
| """ | ||
| Indicates the type of connection for the specified (`from_stop_id`, `to_stop_id`) pair. | ||
|
|
||
| | Value | Description | | ||
| |-------|-------------| | ||
| | `0` | Recommended transfer point between route | | ||
| | `1` | Timed transfer point between two routes. The departing vehicle is expected to wait for the arriving one and leave sufficient time for a rider to transfer between routes. | | ||
| | `2` | Transfer requires a minimum amount of time between arrival and departure to ensure a connection. The time required to transfer is specified by `min_transfer_time`. | | ||
| | `3` | Transfers are not possible between routes at the location. | | ||
| | `4` | Passengers can transfer from one trip to another by staying onboard the same vehicle (an "in-seat transfer"). | | ||
| | `5` | In-seat transfers are not allowed between sequential trips. The passenger must alight from the vehicle and re-board. | | ||
| """, | ||
| enum: Enum.to_list(0..5), | ||
| example: 1 | ||
| ) | ||
|
|
||
| wheelchair_transfer( | ||
| :integer, | ||
| """ | ||
| Experimental. Identifies whether a transfer is accessible to customers using a wheelchair. | ||
|
|
||
| | Value | Description | | ||
| |-------|-------------| | ||
| | `0` | No accessibility information for the transfer | | ||
| | `1` | Transfer is wheelchair accessible | | ||
| | `2` | Not accessible to persons in wheelchairs | | ||
| """, | ||
| enum: Enum.to_list(0..2), | ||
| example: 0 | ||
| ) | ||
| end | ||
|
|
||
| relationship(:from_stop) | ||
| relationship(:to_stop) | ||
| relationship(:from_trip) | ||
| relationship(:to_trip) | ||
|
Comment on lines
+176
to
+177
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you for adding transfers to the API π question(non-blocking): With this PR establishing the relationship from transfer => trips, do you have a sense of if it would be possible to later add a relationship from trip => transfer? Definitely out of scope for this PR, but I wonder if it is feasible to support a query like
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I admittedly don't know how implementing includes works, but I think with the rest of these pieces in place, it'd be a relatively light lift!
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I ended up figuring out how to implement it in feat(TripView): include transfers
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Amazing!!
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually the above was just a parsing issue on Dotcom's end. It's on dev-green if you'd like to try it! e.g. https://api-dev-green.mbtace.com/trips/76334596?include=transfers
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Renamed this include to |
||
| end, | ||
| Transfer: page(:TransferResource) | ||
| } | ||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,54 @@ | ||
| defmodule ApiWeb.TransferView do | ||
| use ApiWeb.Web, :api_view | ||
|
|
||
| attributes([ | ||
| :min_transfer_time, | ||
| :min_walk_time, | ||
| :min_wheelchair_time, | ||
| :suggested_buffer_time, | ||
| :transfer_type, | ||
| :wheelchair_transfer | ||
| ]) | ||
|
|
||
| has_one( | ||
| :from_stop, | ||
| type: :stop, | ||
| serializer: ApiWeb.StopView, | ||
| field: :from_stop_id | ||
| ) | ||
|
|
||
| has_one( | ||
| :to_stop, | ||
| type: :stop, | ||
| serializer: ApiWeb.StopView, | ||
| field: :to_stop_id | ||
| ) | ||
|
|
||
| has_one( | ||
| :from_trip, | ||
| type: :trip, | ||
| serializer: ApiWeb.TripView, | ||
| field: :from_trip_id | ||
| ) | ||
|
|
||
| has_one( | ||
| :to_trip, | ||
| type: :trip, | ||
| serializer: ApiWeb.TripView, | ||
| field: :to_trip_id | ||
| ) | ||
|
|
||
| def id( | ||
| %{ | ||
| from_trip_id: from_trip_id, | ||
| from_stop_id: from_stop_id, | ||
| to_trip_id: to_trip_id, | ||
| to_stop_id: to_stop_id | ||
| }, | ||
| _conn | ||
| ) do | ||
| from_trip = from_trip_id || "" | ||
| to_trip = to_trip_id || "" | ||
| "transfer-#{from_trip}-#{from_stop_id}-#{to_trip}-#{to_stop_id}" | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,6 +9,7 @@ defmodule ApiWeb.TripView do | |
| ServiceView, | ||
| ShapeView, | ||
| StopView, | ||
| TransferView, | ||
| VehicleView | ||
| } | ||
|
|
||
|
|
@@ -20,6 +21,7 @@ defmodule ApiWeb.TripView do | |
| Service, | ||
| Shape, | ||
| Stop, | ||
| Transfer, | ||
| Vehicle | ||
| } | ||
|
|
||
|
|
@@ -87,6 +89,12 @@ defmodule ApiWeb.TripView do | |
| identifiers: :always | ||
| ) | ||
|
|
||
| has_many( | ||
| :from_trip_transfers, | ||
| type: :transfer, | ||
| serializer: TransferView | ||
| ) | ||
|
|
||
| has_many( | ||
| :occupancies, | ||
| type: :occupancy, | ||
|
|
@@ -105,6 +113,13 @@ defmodule ApiWeb.TripView do | |
| optional_relationship("route_pattern", route_pattern_id, &RoutePattern.by_id/1, conn) | ||
| end | ||
|
|
||
| def from_trip_transfers(%{id: trip_id}, _conn) do | ||
| case Transfer.by_from_trip_id(trip_id) do | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. question: so to confirm, including transfers for a given trip will give you transfers from that trip, and not to it? If so I think I would argue for calling the
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated in feedback: rename include to from_trip_transfers, excellent tip |
||
| [] -> nil | ||
| transfers -> transfers | ||
| end | ||
| end | ||
|
|
||
| def vehicle(%{id: trip_id}, _conn) do | ||
| case Vehicle.by_trip_id(trip_id) do | ||
| [] -> nil | ||
|
|
||

There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue: so I worry a bit that by allowing only a filter by type, and allowing the type filter to take a comma-separated list of types, it's still trivially easy to write a query that would return all of the transfers. I'm not sure what to do about it, though, because we've presumably got to allow some filter more general than trip IDs for your use cases on dotcom.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could deploy to a dev environment and see what response times are actually like when filtering by multiple types to get a sense of how much of an issue it actually is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's send this to a dev environment! Maybe you can pick which environment/when? Don't want to interfere with anything else that might be going on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like
dev-blueauto-deploys the latestmasterevery night, so you probably wantdev-greenif you don't want to have to redeploy every day. It doesn't look like anyone else is usingdev-greenright now.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re: the volume concern, it looks like our current GTFS (with the just-recently-released Fall rating included) is 13930 entries, so that's the kind of scale we're talking about.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Put it on dev-green! Here's some observations:
As a reference point, even these available hefty queries still are much quicker:
So yeah, I guess it is too much! Your point about it being trivially easy to query for all transfers anyway... haha, oops!
Some ways forward I see
/transfersmore performant / limit the query more somehow/transfersdirectly, but make it available as an include.https://api-dev-green.mbtace.com/trips/76334596?include=transfers (to be imminently renamed to
include=from_trip_transfers) works, and seems like it could meet Dotcom's and Mobile App's needs. Curious what we think!There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the app side, I agree that the include approach should meet our needs!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I support the idea of making it an include-able resource without a top-level endpoint if that works for both teams! Alternatively we could keep the endpoint but only with the
from_trip_idfilter and nottype.