From 1e87dca0af7b3088e22edb37867cec76dccb30f9 Mon Sep 17 00:00:00 2001 From: Cora Grant Date: Fri, 8 Nov 2024 14:56:25 -0500 Subject: [PATCH] refactor: fetch parents/children with stops Some stop functions were already using this relationship by effectively hand-rolling the relevant bits of the parsing. We can simplify by fully parsing the parent/child relationships in the main stop `Parser`, and having the special-purpose `Stop` functions delegate to a main `fetch` function as we do with other resources. Since we may be "starting from" either a parent or a child, we need a way to encode the complete data without recursing infinitely through this bidirectional link. For this purpose we introduce `:unloaded` as a special value indicating a relationship was not populated, similar to `Ecto.Association.NotLoaded`. This allows ending the recursion once we know we have all the data. It also allows the relationships to not be fetched in the first place, for functions that don't need this data or in contexts where stops are being parsed as a relationship on another resource. --- lib/screens/predictions/parser.ex | 2 +- lib/screens/route_patterns/parser.ex | 2 +- .../route_patterns/route_direction_stops.ex | 2 +- lib/screens/schedules/parser.ex | 2 +- lib/screens/stops/parser.ex | 69 ++++++-- lib/screens/stops/stop.ex | 165 +++++++++--------- .../route_patterns/route_pattern_test.exs | 30 +++- test/screens/stops/stop_test.exs | 117 +++++++++---- 8 files changed, 254 insertions(+), 135 deletions(-) diff --git a/lib/screens/predictions/parser.ex b/lib/screens/predictions/parser.ex index 90b1d5bc5..7d1000228 100644 --- a/lib/screens/predictions/parser.ex +++ b/lib/screens/predictions/parser.ex @@ -31,7 +31,7 @@ defmodule Screens.Predictions.Parser do included ) do trip = included |> Map.fetch!({trip_id, "trip"}) |> Trips.Parser.parse_trip(included) - stop = included |> Map.fetch!({stop_id, "stop"}) |> Stops.Parser.parse_stop() + stop = included |> Map.fetch!({stop_id, "stop"}) |> Stops.Parser.parse_stop(included) route = included |> Map.fetch!({route_id, "route"}) |> Routes.Parser.parse_route(included) vehicle = diff --git a/lib/screens/route_patterns/parser.ex b/lib/screens/route_patterns/parser.ex index c0edc7ab7..2b537658a 100644 --- a/lib/screens/route_patterns/parser.ex +++ b/lib/screens/route_patterns/parser.ex @@ -35,7 +35,7 @@ defmodule Screens.RoutePatterns.Parser do stops = Enum.map(stop_references, fn %{"id" => stop_id} -> - included |> Map.fetch!({stop_id, "stop"}) |> Stops.Parser.parse_stop() + included |> Map.fetch!({stop_id, "stop"}) |> Stops.Parser.parse_stop(included) end) %RoutePattern{ diff --git a/lib/screens/route_patterns/route_direction_stops.ex b/lib/screens/route_patterns/route_direction_stops.ex index 0e90a42ad..768088424 100644 --- a/lib/screens/route_patterns/route_direction_stops.ex +++ b/lib/screens/route_patterns/route_direction_stops.ex @@ -22,7 +22,7 @@ defmodule Screens.RoutePatterns.RouteDirectionStops do end defp parse_included(%{"type" => "stop"} = item) do - Screens.Stops.Parser.parse_stop(item) + Screens.Stops.Parser.parse_stop(item, %{}) end defp parse_included(%{ diff --git a/lib/screens/schedules/parser.ex b/lib/screens/schedules/parser.ex index bb1498bca..9e5715f9d 100644 --- a/lib/screens/schedules/parser.ex +++ b/lib/screens/schedules/parser.ex @@ -31,7 +31,7 @@ defmodule Screens.Schedules.Parser do included ) do trip = included |> Map.fetch!({trip_id, "trip"}) |> Trips.Parser.parse_trip(included) - stop = included |> Map.fetch!({stop_id, "stop"}) |> Stops.Parser.parse_stop() + stop = included |> Map.fetch!({stop_id, "stop"}) |> Stops.Parser.parse_stop(included) route = included |> Map.fetch!({route_id, "route"}) |> Routes.Parser.parse_route(included) %Schedule{ diff --git a/lib/screens/stops/parser.ex b/lib/screens/stops/parser.ex index b0e193b9f..0e2d7d1fa 100644 --- a/lib/screens/stops/parser.ex +++ b/lib/screens/stops/parser.ex @@ -1,21 +1,70 @@ defmodule Screens.Stops.Parser do @moduledoc false - def parse_stop(%{ - "id" => id, - "attributes" => %{ - "name" => name, - "location_type" => location_type, - "platform_code" => platform_code, - "platform_name" => platform_name - } - }) do + alias Screens.RouteType + + def parse(%{"data" => data} = response) do + included = + response + |> Map.get("included", []) + |> Map.new(fn %{"id" => id, "type" => type} = resource -> {{id, type}, resource} end) + + Enum.map(data, &parse_stop(&1, included)) + end + + def parse_stop( + %{ + "id" => id, + "attributes" => %{ + "name" => name, + "location_type" => location_type, + "platform_code" => platform_code, + "platform_name" => platform_name, + "vehicle_type" => vehicle_type + }, + "relationships" => relationships + }, + included, + load_parent_station? \\ true + ) do + parent_station = + case get_in(relationships, ~w[parent_station data id]) do + nil -> + nil + + id -> + data = Map.get(included, {id, "stop"}) + + if is_nil(data) or not load_parent_station?, + # Only non-parent-stations can have a parent station. + do: if(location_type != 1, do: :unloaded, else: nil), + else: parse_stop(data, included) + end + + child_stops = + case get_in(relationships, ~w[child_stops data]) do + nil -> + # Only parent stations can have child stops. + if location_type == 1, do: :unloaded, else: [] + + stop_references -> + Enum.map(stop_references, fn %{"id" => id} -> + # Always leave the `parent_station` of stops in `child_stops` unloaded, else parsing + # would recurse infinitely. This covers the complete "stop family" regardless of where + # we start. ("parent -> children" or "child -> parent -> all children") + included |> Map.fetch!({id, "stop"}) |> parse_stop(included, false) + end) + end + %Screens.Stops.Stop{ id: id, name: name, location_type: location_type, + parent_station: parent_station, + child_stops: child_stops, platform_code: platform_code, - platform_name: platform_name + platform_name: platform_name, + vehicle_type: if(is_nil(vehicle_type), do: nil, else: RouteType.from_id(vehicle_type)) } end end diff --git a/lib/screens/stops/stop.ex b/lib/screens/stops/stop.ex index 38a97a79e..fac1a0078 100644 --- a/lib/screens/stops/stop.ex +++ b/lib/screens/stops/stop.ex @@ -3,109 +3,118 @@ defmodule Screens.Stops.Stop do require Logger - alias Screens.Stops + alias Screens.RouteType + alias Screens.Stops.Parser alias Screens.V3Api - defstruct ~w[id name location_type platform_code platform_name]a + defstruct ~w[ + id + name + location_type + parent_station + child_stops + platform_code + platform_name + vehicle_type + ]a @type id :: String.t() + @type location_type :: 0 | 1 | 2 | 3 @type t :: %__MODULE__{ id: id, name: String.t(), - location_type: 0 | 1 | 2 | 3, + location_type: location_type(), + parent_station: t() | nil | :unloaded, + child_stops: [t()] | :unloaded, platform_code: String.t() | nil, - platform_name: String.t() | nil + platform_name: String.t() | nil, + vehicle_type: RouteType.t() | nil } - def fetch_parent_station_name_map(get_json_fn \\ &V3Api.get_json/2) do - case get_json_fn.("stops", %{ - "filter[location_type]" => 1 - }) do - {:ok, %{"data" => data}} -> - parsed = - data - |> Enum.map(fn %{"id" => id, "attributes" => %{"name" => name}} -> {id, name} end) - |> Enum.into(%{}) + @type params :: %{ + optional(:ids) => [id()], + optional(:location_types) => [location_type()], + optional(:route_types) => [RouteType.t()] + } + + @spec fetch(params()) :: {:ok, [t()]} | :error + @spec fetch(params(), boolean()) :: {:ok, [t()]} | :error + def fetch(params, include_related? \\ false, get_json_fn \\ &V3Api.get_json/2) do + encoded_params = + params + |> Enum.flat_map(&encode_param/1) + |> Map.new() + |> then(fn params -> + if include_related? do + Map.put(params, "include", Enum.join(~w[child_stops parent_station.child_stops], ",")) + else + params + end + end) + + case get_json_fn.("stops", encoded_params) do + {:ok, response} -> {:ok, Parser.parse(response)} + _ -> :error + end + end + + defp encode_param({:ids, ids}), do: [{"filter[id]", Enum.join(ids, ",")}] + defp encode_param({:location_types, lts}), do: [{"filter[location_type]", Enum.join(lts, ",")}] + defp encode_param({:route_types, rts}), do: [{"filter[route_type]", Enum.join(rts, ",")}] + + @doc """ + Returns a list of child stops for each given stop ID (in the same order). For stop IDs that are + already child stops, the list contains only the stop itself. For stop IDs that do not exist, the + list is empty. + """ + @callback fetch_child_stops([id()]) :: {:ok, [[t()]]} | :error + def fetch_child_stops(stop_ids) do + case fetch(%{ids: stop_ids}, true) do + {:ok, stops} -> + stops_by_id = Map.new(stops, fn %__MODULE__{id: id} = stop -> {id, stop} end) + + child_stops = + stop_ids + |> Enum.map(&stops_by_id[&1]) + |> Enum.map(fn + nil -> [] + %__MODULE__{location_type: 0} = stop -> [stop] + %__MODULE__{child_stops: stops} when is_list(stops) -> stops + end) - {:ok, parsed} + {:ok, child_stops} - _ -> + :error -> :error end end + @spec fetch_parent_station_name_map() :: {:ok, %{id() => String.t()}} | :error + def fetch_parent_station_name_map do + case fetch(%{location_types: [1]}) do + {:ok, stops} -> {:ok, Map.new(stops, fn %__MODULE__{id: id, name: name} -> {id, name} end)} + _ -> :error + end + end + @callback fetch_stop_name(id()) :: String.t() | nil def fetch_stop_name(stop_id) do Screens.Telemetry.span(~w[screens stops stop fetch_stop_name]a, %{stop_id: stop_id}, fn -> - case Screens.V3Api.get_json("stops", %{"filter[id]" => stop_id}) do - {:ok, %{"data" => [stop_data]}} -> - %{"attributes" => %{"name" => stop_name}} = stop_data - stop_name - - _ -> - nil + case fetch(%{ids: [stop_id]}) do + {:ok, [%__MODULE__{name: name}]} -> name + _ -> nil end end) end + @spec fetch_subway_platforms_for_stop(id()) :: [t()] def fetch_subway_platforms_for_stop(stop_id) do - case Screens.V3Api.get_json("stops/" <> stop_id, %{"include" => "child_stops"}) do - {:ok, %{"included" => child_stop_data}} -> - child_stop_data - |> Enum.filter(fn %{ - "attributes" => %{ - "location_type" => location_type, - "vehicle_type" => vehicle_type - } - } -> - location_type == 0 and vehicle_type in [0, 1] - end) - |> Enum.map(&Stops.Parser.parse_stop/1) - end - end + {:ok, [%__MODULE__{child_stops: child_stops}]} = fetch(%{ids: [stop_id]}, true) - @doc """ - Returns a list of child stops for each given stop ID (in the same order). For stop IDs that are - already child stops, the list contains only the stop itself. For stop IDs that do not exist, the - list is empty. - """ - @callback fetch_child_stops([id()]) :: {:ok, [[t()]]} | {:error, term()} - def fetch_child_stops(stop_ids, get_json_fn \\ &Screens.V3Api.get_json/2) do - case get_json_fn.("stops", %{ - "filter[id]" => Enum.join(stop_ids, ","), - "include" => "child_stops" - }) do - {:ok, %{"data" => data} = response} -> - child_stops = - response - |> Map.get("included", []) - |> Enum.map(&Stops.Parser.parse_stop/1) - |> Map.new(&{&1.id, &1}) - - stops_with_children = - data - |> Enum.map(fn %{"relationships" => %{"child_stops" => %{"data" => children}}} = stop -> - { - Stops.Parser.parse_stop(stop), - children - |> Enum.map(fn %{"id" => id} -> Map.fetch!(child_stops, id) end) - |> Enum.filter(&(&1.location_type == 0)) - } - end) - |> Map.new(&{elem(&1, 0).id, &1}) - - {:ok, - Enum.map(stop_ids, fn stop_id -> - case stops_with_children[stop_id] do - nil -> [] - {stop, []} -> [stop] - {_stop, children} -> children - end - end)} - - error -> - {:error, error} - end + Enum.filter(child_stops, fn + %__MODULE__{location_type: 0, vehicle_type: vt} when vt in ~w[light_rail subway]a -> true + _ -> false + end) end end diff --git a/test/screens/route_patterns/route_pattern_test.exs b/test/screens/route_patterns/route_pattern_test.exs index 9ddeadb8e..fcd061916 100644 --- a/test/screens/route_patterns/route_pattern_test.exs +++ b/test/screens/route_patterns/route_pattern_test.exs @@ -99,8 +99,10 @@ defmodule Screens.RoutePatterns.RoutePatternTest do "name" => "Wonderland", "location_type" => 0, "platform_code" => "1", - "platform_name" => "Bowdoin" - } + "platform_name" => "Bowdoin", + "vehicle_type" => 1 + }, + "relationships" => %{} }, %{ "id" => "70051", @@ -109,8 +111,10 @@ defmodule Screens.RoutePatterns.RoutePatternTest do "name" => "Orient Heights", "location_type" => 0, "platform_code" => nil, - "platform_name" => "Bowdoin" - } + "platform_name" => "Bowdoin", + "vehicle_type" => 1 + }, + "relationships" => %{} }, %{ "id" => "70838", @@ -119,8 +123,10 @@ defmodule Screens.RoutePatterns.RoutePatternTest do "name" => "Bowdoin", "location_type" => 0, "platform_code" => nil, - "platform_name" => "Exit Only" - } + "platform_name" => "Exit Only", + "vehicle_type" => 1 + }, + "relationships" => %{} } ] } @@ -148,7 +154,9 @@ defmodule Screens.RoutePatterns.RoutePatternTest do name: "Bowdoin", location_type: 0, platform_code: nil, - platform_name: "Exit Only" + platform_name: "Exit Only", + vehicle_type: :subway, + child_stops: [] } orient_heights = %Stop{ @@ -156,7 +164,9 @@ defmodule Screens.RoutePatterns.RoutePatternTest do name: "Orient Heights", location_type: 0, platform_code: nil, - platform_name: "Bowdoin" + platform_name: "Bowdoin", + vehicle_type: :subway, + child_stops: [] } wonderland = %Stop{ @@ -164,7 +174,9 @@ defmodule Screens.RoutePatterns.RoutePatternTest do name: "Wonderland", location_type: 0, platform_code: "1", - platform_name: "Bowdoin" + platform_name: "Bowdoin", + vehicle_type: :subway, + child_stops: [] } expected = [ diff --git a/test/screens/stops/stop_test.exs b/test/screens/stops/stop_test.exs index 367bcdec3..1de415737 100644 --- a/test/screens/stops/stop_test.exs +++ b/test/screens/stops/stop_test.exs @@ -3,53 +3,102 @@ defmodule Screens.Stops.StopTest do alias Screens.Stops.Stop - describe "fetch_child_stops/2" do - test "fetches the child stops of the provided stop IDs" do - stop_attributes = %{ - "name" => "test", - "location_type" => 0, - "platform_code" => "", - "platform_name" => "" - } + describe "fetch/2" do + defp stop_data(id, attributes, parent_station_ref \\ nil, child_stop_refs \\ nil) do + data = + %{ + "id" => id, + "type" => "stop", + "attributes" => + Map.merge( + %{ + "name" => "Test Stop", + "location_type" => 0, + "platform_code" => "", + "platform_name" => "", + "vehicle_type" => 3 + }, + attributes + ), + "relationships" => %{ + "parent_station" => %{"data" => parent_station_ref} + } + } + + if is_nil(child_stop_refs) do + data + else + put_in(data, ~w[relationships child_stops], %{"data" => child_stop_refs}) + end + end + + defp stop_ref(id), do: %{"id" => id, "type" => "stop"} + + test "fetches and parses stops and their parent/child relationships" do + stop_p2 = stop_data("p2", %{"location_type" => 1}, nil, [stop_ref("c3")]) + stop_c3 = stop_data("c3", %{}, stop_ref("p2")) get_json_fn = - fn "stops", %{"filter[id]" => "sX,s1,p1,p2", "include" => "child_stops"} -> + fn "stops", + %{ + "filter[id]" => "s1,p1,p2,c3", + "include" => "child_stops,parent_station.child_stops" + } -> { :ok, %{ "data" => [ - # suppose sX doesn't exist - %{ - "id" => "s1", - "attributes" => stop_attributes, - "relationships" => %{"child_stops" => %{"data" => []}} - }, - %{ - "id" => "p1", - "attributes" => Map.put(stop_attributes, "location_type", 1), - "relationships" => %{ - "child_stops" => %{"data" => [%{"id" => "c1"}, %{"id" => "c2"}]} - } - }, - %{ - "id" => "p2", - "attributes" => Map.put(stop_attributes, "location_type", 1), - "relationships" => %{ - "child_stops" => %{"data" => [%{"id" => "c3"}]} - } - } + stop_data("s1", %{}), + stop_data("p1", %{"location_type" => 1}, nil, [stop_ref("c1"), stop_ref("c2")]), + stop_p2, + stop_c3 ], "included" => [ - %{"id" => "c1", "attributes" => stop_attributes}, - %{"id" => "c2", "attributes" => stop_attributes}, - %{"id" => "c3", "attributes" => stop_attributes} + stop_data("c1", %{}, stop_ref("p1")), + stop_data("c2", %{}, stop_ref("p1")), + stop_c3, + stop_p2 ] } } end - assert {:ok, [[], [%Stop{id: "s1"}], [%Stop{id: "c1"}, %Stop{id: "c2"}], [%Stop{id: "c3"}]]} = - Stop.fetch_child_stops(~w[sX s1 p1 p2], get_json_fn) + assert { + :ok, + [ + %Stop{ + id: "s1", + location_type: 0, + parent_station: nil, + child_stops: [], + vehicle_type: :bus + }, + %Stop{ + id: "p1", + location_type: 1, + parent_station: nil, + child_stops: [ + %Stop{id: "c1", parent_station: :unloaded}, + %Stop{id: "c2", parent_station: :unloaded} + ] + }, + %Stop{ + id: "p2", + location_type: 1, + parent_station: nil, + child_stops: [%Stop{id: "c3", parent_station: :unloaded}] + }, + %Stop{ + id: "c3", + parent_station: %Stop{ + id: "p2", + location_type: 1, + parent_station: nil, + child_stops: [%Stop{id: "c3", parent_station: :unloaded}] + } + } + ] + } = Stop.fetch(%{ids: ~w[s1 p1 p2 c3]}, true, get_json_fn) end end end