feat: set up GraphQL API #18
Conversation
boringcactus
requested review from
a team and
KaylaBrady
and removed request for
a team
| @@ -0,0 +1,216 @@ | |||
| # This file contains the configuration for Credo and you are probably reading | |||
envrc.example
Outdated
| @@ -0,0 +1,2 @@ | |||
| export API_KEY= | |||
| get_json("/stops/", params) | ||
| end | ||
|
|
||
| def by_gtfs_id(gtfs_id, params \\ [], opts \\ []) do |
There was a problem hiding this comment.
thought (non-blocking): I realize this is pulled from dotcom. Outside the scope of this PR, but I think it will be worth discussing how much to copy as-is vs tweak as we go. For example, my instinct is to suggest naming this by_id since I think gtfs is implied, but it all depends on how closely we want to align with the existing dotcom interface.
| @@ -0,0 +1,452 @@ | |||
| defmodule Stops.Api do | |||
There was a problem hiding this comment.
question: Should this be MBTAV3API.Stops.Api? Which maybe introduces a separate question about whether there is a different naming pattern that would be descriptive here rather than Stops.Api, but that one isn't blocking 😃
There was a problem hiding this comment.
In a vacuum, yes, but the module names from dotcom were kept intact in #13, so I figured it'd be better to keep that pattern. If we want to rename all those modules to be more idiomatic and less sprawling, we should probably do that in a separate PR. (Cleaning up the already-imported dotcom code in a separate PR may be a good idea anyway.)
There was a problem hiding this comment.
Personally I wouldn't bother since I'd assume they are just going to be renamed back once moved into the library.
There was a problem hiding this comment.
My main preference here is for consistency - I got a bit tripped up initially between the role of MBTAV3API.Stops vs Stops.Api, and I think prefix consistency would help with that.
My assumption is that we would want a prefix even if we move it into a separate library entirely, similar to how the ecto library prefixes all modules with Ecto, but open to other thoughts on that!
There was a problem hiding this comment.
In dotcom, as far as I can tell, the main reason every resource type has its own root module is that every resource also has its own application in the dotcom umbrella. A standalone mbta_v3_api client library would not do that, and so it would be extraordinarily wacky for us to just spew forth a dozen top-level namespaces and just hope there are no collisions with other modules from other libraries or the consuming application.
There was a problem hiding this comment.
Yep, that's true, that would be the client interface you'd want. I'm sold.
| @@ -44,9 +43,8 @@ defmodule Stops.Stop do | |||
| accessibility: [String.t()], | |||
| address: String.t() | nil, | |||
| municipality: String.t() | nil, | |||
| parking_lots: [Stop.ParkingLot.t()], | |||
| # TODO: Restore fare_facilities once we've copied in Stops.Api | |||
| Routes.Repo.by_stop_with_route_pattern(stop_id) | ||
| |> Enum.map(fn {route, route_patterns} -> Map.put(route, :route_patterns, route_patterns) end)} |
There was a problem hiding this comment.
question: Having the existing by_stop_with_route_patterns seems to make this work nicely! Do you have a sense of what it would take to add another relationship to the route, like stops? Or if they wanted routes but didn't care about route patterns?
There was a problem hiding this comment.
It looks to me like it would be fairly straightforward but not particularly elegant to add more relationships, since the included data is parsed independently and there's nothing tying the JSON:API type to the RoutePattern.new/1 that parses the route patterns. If I were designing the API client from scratch with an eye towards supporting use cases that aren't specifically anticipated, I might add a bit of judicious complexity around JSON:API relationships to allow things like include: [:route_patterns] to be passed to the repo, but if we're committed to lifting from dotcom without architectural overhauls, this is probably good enough.
Fetching route patterns is no more work, especially if the cache is full, so it doesn't cause a problem to fetch them anyway. It looks like Absinthe has a mechanism by which we could check which related objects the request actually includes and only fetch related objects conditionally, but that's not straightforward to express in the current architecture of the API client.
There was a problem hiding this comment.
👍 Totally agree this is good for now! Without having designs to inform our data needs, I agree with lifting from dotcom to start without architectural overhauls.
If down the line we find the constraints of the API client starts to result in complex workarounds, then I think we'd want to revisit how we can relax some of the constraints imposed by the API Client Library. But that is a future consideration 😃
| field(:longitude, non_null(:float)) | ||
|
|
||
| field :routes, list_of(:route) do | ||
| resolve(&Resolvers.Route.by_stop/3) |
There was a problem hiding this comment.
question: Overall, I'm wondering whether we'd need to make separate calls to the V3 API for every relationship that is requested. Can we take advantage of the V3 API's support for specifying the included relationships?
There was a problem hiding this comment.
The dotcom V3 API client code does this within a resource (e.g. for route patterns within a route) but not across resource boundaries (e.g. routes associated with a stop), because that matches how dotcom handles API response caching. If we want to use include in more ways than the dotcom V3 API allows, we need to mess around with the architecture of the API client.
There was a problem hiding this comment.
More to the point, however, /stops/{id} does not take ?include=routes; the only way to get routes for a stop is /routes?filter[stop]={id}. Apparently, it's possible to /routes?include=stop&filter[stop]={id} to get the routes and the stop info at the same time, which appears to have been added specifically to support this use case (since you can't /routes?include=stop if you don't also &filter[stop]={id} and you can't /routes/{id}?include=stop under any circumstances), although that means if we run into a similar but distinct use case we are likely on our own. I've asked in Slack why this is the case, but I do not expect to see a change in the API anytime soon as a consequence.
A client that encapsulated the maximal conceivable complexity could translate get(Stop, "place-boyls", include: []) to /stops/place-boyls and get(Stop, "place-boyls", include: [:routes]) to /routes?include=stop&filter[stop]=place-boyls, but that would very quickly descend into a maintenance nightmare.
There was a problem hiding this comment.
Interesting, thanks for looking into the API specifics & posting that question!
| defmodule Stops.Api do | ||
| @moduledoc """ | ||
| Wrapper around the remote stop information service. | ||
| """ |
There was a problem hiding this comment.
question: Are there any tests to copy over for this file?
There was a problem hiding this comment.
However, the dotcom tests frequently hit the actual API, so we'd need to either set up an API key for our CI workflow or risk intermittent failures due to rate limiting (with the likelihood increasing as our test suite grows).
There was a problem hiding this comment.
Good point. Definitely feels out of scope of this PR to change that - I know the dotcom team wants to move towards mocking more data in tests, tbd on how we can collaborate on that.
| @@ -31,6 +31,16 @@ defmodule MobileAppBackendWeb.Router do | |||
| get("/route/by-stop/:stop_id", RouteController, :by_stop) | |||
| end | |||
|
|
|||
| scope "/graphql" do | |||
There was a problem hiding this comment.
suggestion: Can you add tests for the new graphql pieces? It would be helpful to get a sense of what writing tests for it would look like.
There was a problem hiding this comment.
Looks good to me! One question about the new GraphQL test which I think would be nice to see, but not-blocking since this PR is for an investigation.
Thanks for the entertaining the discussion about the V3 API Client despite being beyond the scope of this PR!
| } | ||
| """ | ||
|
|
||
| test "query: stop", %{conn: conn} do |
There was a problem hiding this comment.
question (non-blocking): Can the data be mocked for this test?
There was a problem hiding this comment.
It can, but there's a lot of data in the real response, and I'm not sure which parts would and would not be reasonable to elide from the real response.
There was a problem hiding this comment.
It looks like a lot of parts can be elided without breaking the test, though.
Asana task: Determine how back-end service will provide non-real-time data to the client › Set up GraphQL
As it stands, this will have the GraphQL interactive API explorer at
/graphql/graphiql(it's technically using the GraphQL Playground rather than GraphiQL per se, because the Playground looks nicer and has support for subscriptions), with no access control. This is not necessarily a problem, though, because the API itself at/graphqlis also public with no access control.