Skip to content

Commit

Permalink
refactor: remove unused routes-at-stop agent
Browse files Browse the repository at this point in the history
d52ead8, which introduced more comprehensive V3 API request caching,
removed the only instance where `put` was ever called on this cache, so
it was effectively unused.

After removing a few layers of indirection:

* `Stop.create_station_with_routes_map` is renamed and relocated to
  `Route.serving_stop`. Unclear why the previous name suggested it would
  "create a map".

* The similar-in-nature `Route.fetch_routes_by_stop` is renamed to
  `Route.serving_stop_with_active` so these names are aligned.

* Logging is added to the retry logic of `serving_stop` so we can tell
  whether it's still being used. This seems like a workaround for a
  possible bug in the V3 API which may have since been fixed, and if it
  hasn't been, we'll know and can report it.

* `serving_stop_with_active` is fixed to conform to its typespec (and
  the spec of `LocationContext`) by returning a map containing _only_
  `route_id` and `active?` fields, rather than a map version of the
  entire `Route` struct with the `id` field renamed. It appears nothing
  was accessing the extra fields.
  • Loading branch information
digitalcora committed Jul 31, 2024
1 parent fb9dddc commit 297c302
Show file tree
Hide file tree
Showing 10 changed files with 114 additions and 200 deletions.
1 change: 0 additions & 1 deletion lib/screens/application.ex
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ defmodule Screens.Application do
:hackney_pool.child_spec(:ex_aws_pool, []),
:hackney_pool.child_spec(:blue_bikes_pool, []),
:hackney_pool.child_spec(:api_v3_pool, max_connections: 100),
{Screens.Stops.StationsWithRoutesAgent, %{}},
# Turning this off because it's not in use, and the process is failing
# {Screens.BlueBikes.State, name: Screens.BlueBikes.State},
# Task supervisor for ScreensByAlert async updates
Expand Down
54 changes: 39 additions & 15 deletions lib/screens/routes/route.ex
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
defmodule Screens.Routes.Route do
@moduledoc false

require Logger

alias Screens.Routes.Parser
alias Screens.RouteType
alias Screens.Stops.Stop
alias Screens.V3Api
Expand Down Expand Up @@ -39,7 +42,7 @@ defmodule Screens.Routes.Route do
@spec by_id(id()) :: {:ok, t()} | :error
def by_id(route_id) do
case V3Api.get_json("routes/" <> route_id) do
{:ok, %{"data" => data}} -> {:ok, Screens.Routes.Parser.parse_route(data)}
{:ok, %{"data" => data}} -> {:ok, Parser.parse_route(data)}
_ -> :error
end
end
Expand All @@ -53,41 +56,62 @@ defmodule Screens.Routes.Route do
|> Enum.into(%{})

case get_json_fn.("routes/", params) do
{:ok, %{"data" => data}} -> {:ok, Enum.map(data, &Screens.Routes.Parser.parse_route/1)}
{:ok, %{"data" => data}} -> {:ok, Enum.map(data, &Parser.parse_route/1)}
_ -> :error
end
end

@doc "Fetches routes that serve the given stop."
@spec serving_stop(Stop.id()) :: {:ok, t()} | :error
def serving_stop(
stop_id,
get_json_fn \\ &V3Api.get_json/2,
attempts_left \\ 3
)

def serving_stop(_stop_id, _get_json_fn, 0), do: :error

def serving_stop(
stop_id,
get_json_fn,
attempts_left
) do
case get_json_fn.("routes", %{"filter[stop]" => stop_id}) do
{:ok, %{"data" => []}, _} ->
Logger.warning("Route.serving_stop empty_retry attempts_left=#{attempts_left - 1}")
serving_stop(stop_id, get_json_fn, attempts_left - 1)

{:ok, %{"data" => data}} ->
{:ok, Enum.map(data, fn route -> Parser.parse_route(route) end)}

_ ->
:error
end
end

@doc """
Fetches routes that serve the given stop. `now` is used to determine whether
each route is actively running on the current day.
Similar to `serving_stop` but also determines whether each route has any scheduled service at
the given stop on the current day. Only route IDs and the `active?` flag are returned.
"""
@spec fetch_routes_by_stop(String.t()) ::
@spec serving_stop_with_active(Stop.id()) ::
{:ok, list(%{route_id: id(), active?: boolean()})} | :error
def fetch_routes_by_stop(
def serving_stop_with_active(
stop_id,
now \\ DateTime.utc_now(),
type_filters \\ [],
get_json_fn \\ &V3Api.get_json/2,
fetch_routes_fn \\ &fetch_routes/3
) do
Screens.Telemetry.span(
~w[screens routes route fetch_routes_by_stop]a,
~w[screens routes route serving_stop_with_active]a,
%{stop_id: stop_id, type_filters: type_filters},
fn ->
with {:ok, routes} <- fetch_routes_fn.(stop_id, get_json_fn, type_filters),
{:ok, active_route_ids} <- fetch_active_route_ids(stop_id, now, get_json_fn) do
active_set = MapSet.new(active_route_ids)

routes_at_stop =
Enum.map(
routes,
&(&1
|> Map.from_struct()
|> Map.put(:active?, MapSet.member?(active_set, &1.id))
|> Map.put(:route_id, &1.id)
|> Map.delete(:id))
)
Enum.map(routes, &%{route_id: &1.id, active?: MapSet.member?(active_set, &1.id)})

{:ok, routes_at_stop}
else
Expand Down
16 changes: 0 additions & 16 deletions lib/screens/stops/stations_with_routes_agent.ex

This file was deleted.

74 changes: 3 additions & 71 deletions lib/screens/stops/stop.ex
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,9 @@ defmodule Screens.Stops.Stop do

alias Screens.LocationContext
alias Screens.RoutePatterns.RoutePattern
alias Screens.{Routes, Stops}
alias Screens.Routes.Route
alias Screens.RouteType
alias Screens.Stops.StationsWithRoutesAgent
alias Screens.Stops
alias Screens.Util
alias Screens.V3Api
alias ScreensConfig.V2.{BusEink, BusShelter, Dup, GlEink, PreFare, Triptych}
Expand Down Expand Up @@ -271,74 +270,6 @@ defmodule Screens.Stops.Stop do
end
end

def fetch_routes_serving_stop(
station_id,
get_json_fn \\ &V3Api.get_json/2,
attempts_left \\ 3
)

def fetch_routes_serving_stop(_station_id, _get_json_fn, 0), do: :bad_response

def fetch_routes_serving_stop(
station_id,
get_json_fn,
attempts_left
) do
case get_json_fn.("routes", %{"filter[stop]" => station_id}) do
{:ok, %{"data" => []}, _} ->
fetch_routes_serving_stop(station_id, get_json_fn, attempts_left - 1)

{:ok, %{"data" => data}} ->
{:ok, Enum.map(data, fn route -> Routes.Parser.parse_route(route) end)}

_ ->
:error
end
end

# Returns a list of Route structs that serve the provided ID
@spec create_station_with_routes_map(String.t()) :: list(Routes.Route.t())
def create_station_with_routes_map(station_id) do
case StationsWithRoutesAgent.get(station_id) do
nil ->
get_routes_serving_stop(station_id)

routes ->
get_routes_serving_stop(station_id, routes)
end
end

defp get_routes_serving_stop(station_id, default_routes \\ []) do
case fetch_routes_serving_stop(station_id) do
{:ok, new_routes} ->
new_routes

:bad_response ->
Logger.error(
"[create_station_with_routes_map no routes] Received an empty list from API: stop_id=#{station_id}"
)

default_routes

:error ->
Logger.error(
"[create_station_with_routes_map fetch error] Received an error from API: stop_id=#{station_id}"
)

default_routes
end
end

def get_routes_serving_stop_ids(stop_ids) do
stop_ids
|> Enum.flat_map(fn stop_id ->
stop_id
|> create_station_with_routes_map()
|> Enum.map(& &1.id)
end)
|> Enum.uniq()
end

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
Expand Down Expand Up @@ -479,7 +410,8 @@ defmodule Screens.Stops.Stop do
%{app: app, stop_id: stop_id},
fn ->
with alert_route_types <- get_route_type_filter(app, stop_id),
{:ok, routes_at_stop} <- Route.fetch_routes_by_stop(stop_id, now, alert_route_types),
{:ok, routes_at_stop} <-
Route.serving_stop_with_active(stop_id, now, alert_route_types),
{:ok, tagged_stop_sequences} <-
fetch_tagged_stop_sequences_by_app(app, stop_id, routes_at_stop) do
stop_name = fetch_stop_name(stop_id)
Expand Down
18 changes: 9 additions & 9 deletions lib/screens/v2/candidate_generator/dup/departures.ex
Original file line number Diff line number Diff line change
Expand Up @@ -28,15 +28,15 @@ defmodule Screens.V2.CandidateGenerator.Dup.Departures do
fetch_departures_fn \\ &Departure.fetch/2,
fetch_alerts_fn \\ &Alert.fetch_or_empty_list/1,
fetch_schedules_fn \\ &Screens.Schedules.Schedule.fetch/2,
create_station_with_routes_map_fn \\ &Screens.Stops.Stop.create_station_with_routes_map/1,
fetch_routes_serving_stop_fn \\ &Screens.Routes.Route.serving_stop/1,
fetch_vehicles_fn \\ &Screens.Vehicles.Vehicle.by_route_and_direction/2
) do
primary_departures_instances =
primary_sections
|> get_sections_data(
fetch_departures_fn,
fetch_alerts_fn,
create_station_with_routes_map_fn,
fetch_routes_serving_stop_fn,
now
)
|> sections_data_to_departure_instances(
Expand Down Expand Up @@ -64,7 +64,7 @@ defmodule Screens.V2.CandidateGenerator.Dup.Departures do
|> get_sections_data(
fetch_departures_fn,
fetch_alerts_fn,
create_station_with_routes_map_fn,
fetch_routes_serving_stop_fn,
now
)
|> sections_data_to_departure_instances(
Expand Down Expand Up @@ -208,7 +208,7 @@ defmodule Screens.V2.CandidateGenerator.Dup.Departures do
sections,
fetch_departures_fn,
fetch_alerts_fn,
create_station_with_routes_map_fn,
fetch_routes_serving_stop_fn,
now
) do
Screens.Telemetry.span(
Expand All @@ -222,7 +222,7 @@ defmodule Screens.V2.CandidateGenerator.Dup.Departures do
&1,
fetch_departures_fn,
fetch_alerts_fn,
create_station_with_routes_map_fn,
fetch_routes_serving_stop_fn,
now,
ctx
),
Expand All @@ -249,15 +249,15 @@ defmodule Screens.V2.CandidateGenerator.Dup.Departures do
section,
fetch_departures_fn,
fetch_alerts_fn,
create_station_with_routes_map_fn,
fetch_routes_serving_stop_fn,
now,
ctx
) do
Screens.Telemetry.span(
[:screens, :v2, :candidate_generator, :dup, :departures, :get_section_data],
ctx,
fn ->
routes = get_routes_serving_section(params, create_station_with_routes_map_fn)
routes = get_routes_serving_section(params, fetch_routes_serving_stop_fn)
# DUP sections will always show no more than one mode.
# For subway, each route will have its own section.
# If the stop is served by two different subway/light rail routes, route_ids must be populated for each section
Expand Down Expand Up @@ -634,11 +634,11 @@ defmodule Screens.V2.CandidateGenerator.Dup.Departures do

defp get_routes_serving_section(
%{route_ids: route_ids, stop_ids: stop_ids},
create_station_with_routes_map_fn
fetch_routes_serving_stop_fn
) do
routes =
stop_ids
|> Enum.flat_map(&create_station_with_routes_map_fn.(&1))
|> Enum.flat_map(&fetch_routes_serving_stop_fn.(&1))
|> Enum.uniq()

if route_ids == [] do
Expand Down
10 changes: 5 additions & 5 deletions lib/screens/v2/candidate_generator/pre_fare.ex
Original file line number Diff line number Diff line change
Expand Up @@ -93,10 +93,10 @@ defmodule Screens.V2.CandidateGenerator.PreFare do
def audio_only_instances(
widgets,
config,
fetch_routes_by_stop_fn \\ &Route.fetch_routes_by_stop/1
routes_serving_stop_fn \\ &Route.serving_stop/1
) do
[
fn -> content_summary_instances(widgets, config, fetch_routes_by_stop_fn) end,
fn -> content_summary_instances(widgets, config, routes_serving_stop_fn) end,
fn -> alerts_intro_instances(widgets, config) end,
fn -> alerts_outro_instances(widgets, config) end
]
Expand Down Expand Up @@ -133,14 +133,14 @@ defmodule Screens.V2.CandidateGenerator.PreFare do
[%ShuttleBusInfoWidget{screen: config, now: now}]
end

defp content_summary_instances(widgets, config, fetch_routes_by_stop_fn) do
defp content_summary_instances(widgets, config, routes_serving_stop_fn) do
config.app_params.content_summary.parent_station_id
|> fetch_routes_by_stop_fn.()
|> routes_serving_stop_fn.()
|> case do
{:ok, routes_at_station} ->
subway_lines_at_station =
routes_at_station
|> Enum.map(& &1.route_id)
|> Enum.map(& &1.id)
|> Enum.map(fn
"Red" -> :red
"Orange" -> :orange
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ defmodule Screens.V2.CandidateGenerator.Widgets.ElevatorClosures do
@moduledoc false

alias Screens.Alerts.Alert
alias Screens.Routes.Route
alias Screens.Stops.Stop
alias Screens.V2.WidgetInstance.ElevatorStatus, as: ElevatorStatusWidget
alias ScreensConfig.Screen
Expand Down Expand Up @@ -51,7 +52,7 @@ defmodule Screens.V2.CandidateGenerator.Widgets.ElevatorClosures do
|> MapSet.new()
|> MapSet.put(home_parent_station_id)
|> Enum.map(fn station_id ->
{station_id, station_id |> Stop.create_station_with_routes_map() |> routes_to_icons()}
{station_id, station_id |> Route.serving_stop() |> routes_to_icons()}
end)
|> Enum.into(%{})
end
Expand Down
Loading

0 comments on commit 297c302

Please sign in to comment.