From 636c01ded7d4cdd0f7b71d8a1c002b5a4cfed033 Mon Sep 17 00:00:00 2001 From: KaylaBrady <31781298+KaylaBrady@users.noreply.github.com> Date: Fri, 1 Nov 2024 12:23:33 -0400 Subject: [PATCH] perf(ScheduleController): Fetch schedules by individual stop for more cache hits As implemented, if any request for a stop fails, the entire request fails. This is to preserve the existing all-or-nothing behavior for fetching schedules for all stops together. If we want to have partial failure states in the future, we'll want to adopt a frontend changes to represent the partial failure state. --- .../controllers/schedule_controller.ex | 63 ++++++-- .../controllers/schedule_controller_test.exs | 148 ++++++++++++++++++ 2 files changed, 200 insertions(+), 11 deletions(-) diff --git a/lib/mobile_app_backend_web/controllers/schedule_controller.ex b/lib/mobile_app_backend_web/controllers/schedule_controller.ex index 5132c59..a68eb1a 100644 --- a/lib/mobile_app_backend_web/controllers/schedule_controller.ex +++ b/lib/mobile_app_backend_web/controllers/schedule_controller.ex @@ -1,17 +1,31 @@ defmodule MobileAppBackendWeb.ScheduleController do use MobileAppBackendWeb, :controller + require Logger alias MBTAV3API.JsonApi alias MBTAV3API.Repository - def schedules(conn, %{"stop_ids" => stop_ids, "date_time" => date_time}) do - if stop_ids == "" do + def schedules(conn, %{"stop_ids" => stop_ids_concat, "date_time" => date_time}) do + if stop_ids_concat == "" do json(conn, %{schedules: [], trips: %{}}) else - {:ok, data} = - get_filter(stop_ids, date_time) - |> fetch_schedules() + stop_ids = String.split(stop_ids_concat, ",") - json(conn, data) + service_date = parse_service_date(date_time) + + data = + stop_ids + |> Enum.map(&get_filter(&1, service_date)) + |> fetch_schedules_parallel() + + case data do + :error -> + conn + |> put_status(:internal_server_error) + |> json(%{error: "fetch_failed"}) + + data -> + json(conn, data) + end end end @@ -39,11 +53,38 @@ defmodule MobileAppBackendWeb.ScheduleController do json(conn, response) end - @spec get_filter(String.t(), String.t()) :: [JsonApi.Params.filter_param()] - defp get_filter(stop_ids, date_time) do - date_time = Util.parse_datetime!(date_time) - service_date = Util.datetime_to_gtfs(date_time) - [stop: stop_ids, date: service_date] + @spec parse_service_date(String.t()) :: Date.t() + defp parse_service_date(date_string) do + date_string + |> Util.parse_datetime!() + |> Util.datetime_to_gtfs() + end + + @spec get_filter(String.t(), Date.t()) :: [JsonApi.Params.filter_param()] + defp get_filter(stop_id, service_date) do + [stop: stop_id, date: service_date] + end + + @spec fetch_schedules_parallel([[JsonApi.Params.filter_param()]]) :: + %{schedules: [MBTAV3API.Schedule.t()], trips: JsonApi.Object.trip_map()} | :error + defp fetch_schedules_parallel(filters) do + filters + |> Task.async_stream(fn filter_params -> + {filter_params, fetch_schedules(filter_params)} + end) + |> Enum.reduce_while(%{schedules: [], trips: %{}}, fn result, acc -> + case result do + {:ok, {_params, {:ok, %{schedules: schedules, trips: trips}}}} -> + {:cont, %{schedules: acc.schedules ++ schedules, trips: Map.merge(acc.trips, trips)}} + + {_result_type, {params, api_response}} -> + Logger.warning( + "#{__MODULE__} skipped returning schedules due to error. params=#{inspect(params)} response=#{inspect(api_response)}" + ) + + {:halt, :error} + end + end) end @spec fetch_schedules([JsonApi.Params.filter_param()]) :: diff --git a/test/mobile_app_backend_web/controllers/schedule_controller_test.exs b/test/mobile_app_backend_web/controllers/schedule_controller_test.exs index 31ba194..7ee09ef 100644 --- a/test/mobile_app_backend_web/controllers/schedule_controller_test.exs +++ b/test/mobile_app_backend_web/controllers/schedule_controller_test.exs @@ -1,6 +1,7 @@ defmodule MobileAppBackendWeb.ScheduleControllerTest do use MobileAppBackendWeb.ConnCase use HttpStub.Case + import ExUnit.CaptureLog import Mox import MobileAppBackend.Factory import Test.Support.Helpers @@ -93,6 +94,153 @@ defmodule MobileAppBackendWeb.ScheduleControllerTest do } = json_response(conn, 200) end + test "returns schedules for multiple stops", %{conn: conn} do + s1 = + %MBTAV3API.Schedule{ + id: "schedule-60565179-70159-90", + arrival_time: ~B[2024-03-13 01:07:00], + departure_time: ~B[2024-03-13 01:07:00], + drop_off_type: :regular, + pick_up_type: :regular, + stop_sequence: 90, + route_id: "Green-B", + stop_id: "70159", + trip_id: "60565179" + } + + s2 = %MBTAV3API.Schedule{ + id: "schedule-60565145-70158-590", + arrival_time: "2024-03-13T01:15:00-04:00", + departure_time: "2024-03-13T01:15:00-04:00", + drop_off_type: :regular, + pick_up_type: :regular, + stop_sequence: 590, + route_id: "Green-C", + stop_id: "70158", + trip_id: "60565145" + } + + t1 = build(:trip, id: s1.trip_id) + t2 = build(:trip, id: s2.trip_id) + + RepositoryMock + |> expect(:schedules, fn [ + filter: [ + stop: "place-boyls", + date: ~D[2024-03-12] + ], + include: :trip, + sort: {:departure_time, :asc} + ], + _opts -> + ok_response([s1], [t1]) + end) + |> expect(:schedules, fn [ + filter: [ + stop: "place-pktrm", + date: ~D[2024-03-12] + ], + include: :trip, + sort: {:departure_time, :asc} + ], + _opts -> + ok_response([s2], [t2]) + end) + + conn = + get(conn, "/api/schedules", %{ + stop_ids: "place-boyls,place-pktrm", + date_time: "2024-03-13T01:06:30-04:00" + }) + + assert %{ + "schedules" => [ + %{ + "arrival_time" => "2024-03-13T01:07:00-04:00", + "departure_time" => "2024-03-13T01:07:00-04:00", + "drop_off_type" => "regular", + "id" => "schedule-60565179-70159-90", + "pick_up_type" => "regular", + "route_id" => "Green-B", + "stop_id" => "70159", + "stop_sequence" => 90, + "trip_id" => "60565179" + }, + %{ + "arrival_time" => "2024-03-13T01:15:00-04:00", + "departure_time" => "2024-03-13T01:15:00-04:00", + "drop_off_type" => "regular", + "id" => "schedule-60565145-70158-590", + "pick_up_type" => "regular", + "route_id" => "Green-C", + "stop_id" => "70158", + "stop_sequence" => 590, + "trip_id" => "60565145" + } + ], + "trips" => %{ + "60565145" => %{}, + "60565179" => %{} + } + } = json_response(conn, 200) + end + + @tag :capture_log + test "when any stop fetch errors, then returns error", %{conn: conn} do + s1 = + %MBTAV3API.Schedule{ + id: "schedule-60565179-70159-90", + arrival_time: ~B[2024-03-13 01:07:00], + departure_time: ~B[2024-03-13 01:07:00], + drop_off_type: :regular, + pick_up_type: :regular, + stop_sequence: 90, + route_id: "Green-B", + stop_id: "70159", + trip_id: "60565179" + } + + t1 = build(:trip, id: s1.trip_id) + + RepositoryMock + |> expect(:schedules, fn params, _opts -> + assert [ + filter: [ + stop: "place-boyls", + date: ~D[2024-03-12] + ], + include: :trip, + sort: {:departure_time, :asc} + ] = params + + ok_response([s1], [t1]) + end) + |> expect(:schedules, fn params, _opts -> + assert [ + filter: [ + stop: "place-pktrm", + date: ~D[2024-03-12] + ], + include: :trip, + sort: {:departure_time, :asc} + ] = params + + {:error, :some_error_message} + end) + + {conn, log} = + with_log([level: :warning], fn -> + get(conn, "/api/schedules", %{ + stop_ids: "place-boyls,place-pktrm", + date_time: "2024-03-13T01:06:30-04:00" + }) + end) + + assert %{"error" => "fetch_failed"} = json_response(conn, 500) + + assert log =~ "skipped returning schedules due to error" + end + test "gracefully handles empty stops", %{conn: conn} do conn = get(conn, "/api/schedules", %{stop_ids: "", date_time: "2024-10-28T15:29:06-04:00"}) assert json_response(conn, 200) == %{"schedules" => [], "trips" => %{}}