Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .envrc.template
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,11 @@ export API_V3_URL="https://api-v3.mbta.com"

export SECRET_KEY_BASE="local_secret_key_base_at_least_64_bytes_________________________________"

export SCREENS_API_CLIENT_KEY="local_screens_api_client_key"

## Postgres configuration: username and password for local server
# export DATABASE_USER=
# export DATABASE_PASSWORD=

## Feature flag. Setting to true uses the Postgres DB for screen configurations
export CONFIG_MIGRATION="false"
2 changes: 2 additions & 0 deletions config/runtime.exs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
# remember to add this file to your .gitignore.
import Config

config :screens, :config_migration, System.get_env("CONFIG_MIGRATION", "false") == "true"

config :screens, Screens.Repo,
username: System.fetch_env!("DATABASE_USER"),
password: System.get_env("DATABASE_PASSWORD"),
Expand Down
2 changes: 2 additions & 0 deletions lib/screens/config/screen_config.ex
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ defmodule Screens.Config.ScreenConfig do

import Ecto.Changeset

@derive {Jason.Encoder, except: [:__meta__]}

@type t() :: %__MODULE__{
id: String.t(),
config: Config.t()
Expand Down
130 changes: 126 additions & 4 deletions lib/screens/screen_configs.ex
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,17 @@ defmodule Screens.ScreenConfigs do
"""

import Ecto.Query
import Screens.Inject

alias Screens.Config.Fetch, as: ConfigFetch
alias Screens.Config.ScreenConfig
alias Screens.Repo

@config_fetcher injected(Screens.Config.Fetch)

@spec import_from_file() :: {:ok, %{upserted: integer(), deleted: integer()}} | {:error, any()}
def import_from_file do
# This should be a part of post_config_migration_cleanup
with {:ok, config, _version} <- ConfigFetch.fetch_config(),
with {:ok, config, _version} <- @config_fetcher.fetch_config(),
config = Jason.decode!(config),
screens when is_map(screens) <- Map.get(config, "screens", %{}) do
screen_ids = Map.keys(screens)
Expand All @@ -28,8 +30,41 @@ defmodule Screens.ScreenConfigs do
end
end

def list do
Repo.all(ScreenConfig)
Comment thread
robbie-sundstrom marked this conversation as resolved.
@spec list_all() :: String.t() | :error
def list_all do
# Returns all Configs as a JSON to be used by Screens Admin
Comment thread
robbie-sundstrom marked this conversation as resolved.
Outdated
if config_migration_enabled?() do
screens =
ScreenConfig
|> Repo.all()
|> Map.new(fn %ScreenConfig{id: id, config: config} -> {id, config} end)

Jason.encode!(%{screens: screens})
else
with {:ok, config, _version} <- @config_fetcher.fetch_config() do
config
end
end
end

@spec list_screen_configs() :: [ScreenConfig.t()]
def list_screen_configs do
# Returns all configs as a list of ScreenConfig structs to be used by Screens Admin
# The API controller handles the JSON encoding and formatting for the response.
# As part of post_config_migration_cleanup, this and above function can be cleaned up
Comment thread
robbie-sundstrom marked this conversation as resolved.
Outdated
if config_migration_enabled?() do
Repo.all(ScreenConfig)
else
with {:ok, config_json, _version} <- @config_fetcher.fetch_config(),
{:ok, decoded_config} <- Jason.decode(config_json),
screens when is_map(screens) <- Map.get(decoded_config, "screens", %{}) do
Enum.map(screens, fn {id, config} ->
%ScreenConfig{id: id, config: config}
end)
else
_ -> []
end
end
end

@doc """
Expand All @@ -46,4 +81,91 @@ defmodule Screens.ScreenConfigs do
conflict_target: :id
)
end

@spec upsert_list([%{:id => String.t(), :config => map()}]) :: :ok | {:error, any()}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we able to update the any() in the tuple for errors a more concise type? It looks like this could be replaced with Ecto.Changeset.t(), but (assuming that's correct) I would think we'd want to transform this into a string, different kind of error struct, etc. to decouple what's returned from this function from the API.

That said, it looks like we assume this will succeed here. Is there any logging we should do in the event of a Postgres failure that we might not get from a stacktrace? I'm not familiar with the types of errors that Ecto will return, apologies if that's vague!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment inspired me to do some big improvements in this file. I added a commit_error type that correspond to the different type of errors in a more human readable form. For the legacy responses, I didn't iron out the final responses as well since I was focused on getting this up in a workable form.

{:upsert_failed, String.t()} and {:delete_failed, String.t()} do process the Ecto.Changeset.t() in a clean way to return useful info to consumers. I'm going to add tests and clean up the legacy erroring a little, but I like this general structure much more.

defp upsert_list(updates) do
Enum.reduce_while(updates, :ok, fn update, _acc ->
case upsert_screen_config(update) do
{:ok, _} -> {:cont, :ok}
{:error, reason} -> {:halt, {:error, reason}}
end
end)
end

@doc """

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For my own understanding, we have a mix of @doc comments and blocks of comments (e.g.). How do we determine what we want to doc with @doc vs not?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do need to be consistent with this 😅 I kind of like the @doc comments because of how they integrate with tools like ElixirLS in VSCode. But I do generally think the @doc comments are moreso for API consumers, which is why we generally don't have them throughout our codebase. I'll move towards consistency here, which I'd like to be @doc comments for this new file at least

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Elixir convention as far as I've seen is @doc for public functions and # blocks for private functions, since private functions are treated as "hidden" anyway and there's no way to access docs attached to them beyond looking at the code. Technically the main purpose of @doc is in generating ExDocs which are made available on Hex for published libraries; in an application codebase, it's mostly useful for editor LS integration. I usually include them if there's something important for callers to know that isn't covered by the function name, argument names, and typespec, which is all also readily accessible via LSP. (This is not just "for API consumers" in the sense of "consumers of some API external to the whole application", but if you take an expansive view of an "API" as "the set of public functions exported by a module", then that is accurate!)

Updates and deletes multiple screen configs.
Accepts a list of maps with :id and :config keys for updates, and a list of screen IDs for deletions.
"""
@spec commit_updates([%{:id => String.t(), :config => map()}], [String.t()]) ::
:ok | {:error, any()}
def commit_updates(updates, deletes \\ []) do

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not entirely sure we should combine updates + deletes in a single API call. Maybe I'm being too much of a purist/this reminds me a little bit of SOAP calls though. Is there an advantage of combining these other than one less round trip from the API to the server?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just recreating existing behavior, which calls a single API point for both updates and deletes at the same time. I agree in principle though, and I could update this so that the frontend make two separate API calls

if config_migration_enabled?() do
update_to_postgres(updates, deletes)
else
# This branch will be removed as part of post_config_migration_cleanup.
# When the feature flag is disabled, continue to update the JSON config.
update_to_legacy_json(updates, deletes)
end
end

@spec update_to_postgres([%{:id => String.t(), :config => map()}], [String.t()]) ::
:ok | {:error, any()}
defp update_to_postgres(updates, deletes) do
with :ok <- upsert_list(updates) do
perform_deletes(deletes)
end
end

@spec perform_deletes([String.t()]) :: :ok | {:error, any()}
defp perform_deletes(deletes) do
Enum.reduce_while(deletes, :ok, fn id, _acc ->
Repo.delete_all(from s in ScreenConfig, where: s.id == ^id)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this fn ever return {:error, any()}? I deleted {:error, any()} from the return type, and mix dialyzer passed. Maybe something to do with the reduce_while and delete_all I'm not seeing?

This may be tied to the first question, but for my own understanding, why do we need to use reduce_while here? Wouldn't Enum.each/2 suffice here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Enum.each did originally suffice here, since we're not short circuiting early like in update_to_postgres in the event that we do encounter an error with deletion. I'm making some changes to error handling based on your other comments to add more detailed error codes and details, so I have moved this back to a reduce_while

{:cont, :ok}
end)
end

defp update_to_legacy_json(updates, deletes) do
# This will be a part of post_config_migration_cleanup
# Merges updates into the existing config and removes deleted screens, then writes back to the legacy source.
# We need to fetch the existing config before writing updates to prevent overwriting any existing configs.
with {:ok, config_json, _version} <- @config_fetcher.fetch_config(),
{:ok, decoded_config} <- Jason.decode(config_json),
screens when is_map(screens) <- Map.get(decoded_config, "screens", %{}) do
updated_screens =
Enum.reduce(updates, screens, fn update, acc ->
id = extract_id(update)
config = extract_config(update)

existing_config = Map.get(acc, id, %{})
merged_config = Map.merge(existing_config, config)
Comment thread
robbie-sundstrom marked this conversation as resolved.
Outdated
Map.put(acc, id, merged_config)
end)

final_screens = Map.drop(updated_screens, deletes)
updated_config = Map.put(decoded_config, "screens", final_screens)

case Jason.encode(updated_config) do
{:ok, encoded_config} -> @config_fetcher.put_config(encoded_config)
{:error, reason} -> {:error, reason}
end
else
_ -> :error
end
end

@spec config_migration_enabled?() :: boolean()
def config_migration_enabled? do
# This will be a part of post_config_migration_cleanup
Application.get_env(:screens, :config_migration, false)
end

# This will be a part of post_config_migration_cleanup
defp extract_id(%{"id" => id}), do: id
defp extract_id(%{id: id}), do: id
defp extract_id(_), do: nil

# This will be a part of post_config_migration_cleanup
defp extract_config(%{"config" => config}), do: config
defp extract_config(%{config: config}), do: config
defp extract_config(_), do: %{}
end
38 changes: 38 additions & 0 deletions lib/screens_web/controllers/screen_configs_api_controller.ex
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
defmodule ScreensWeb.ScreenConfigsApiController do
use ScreensWeb, :controller

alias Screens.ScreenConfigs

def index(conn, _params) do
screen_configs =
ScreenConfigs.list_screen_configs()
|> Enum.map(fn screen_config ->

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this Enum.map call? Does the ScreenConfig list returned by list_screen_configs() have everything we need here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It actually has more info than is needed, since it includes the timestamp fields (created_at, updated_at). Since you pointed this out, I do think we could filter these out at the data access layer though

%{
id: screen_config.id,
config: screen_config.config
}
end)

json(conn, %{screen_configs: screen_configs})
end

def update(conn, %{"screen_configs" => screen_configs} = params) when is_list(screen_configs) do
deleted_screen_ids = Map.get(params, "deleted_screen_ids", [])

case ScreenConfigs.commit_updates(screen_configs, deleted_screen_ids) do
:ok ->
json(conn, %{success: true})

{:error, reason} ->
conn
|> put_status(500)
|> json(%{success: false, error: "Failed to update screen configs: #{inspect(reason)}"})
end
end

def update(conn, _params) do
conn
|> put_status(400)
|> json(%{success: false, error: "screen_configs parameter is required"})
end
end
46 changes: 46 additions & 0 deletions lib/screens_web/plug/ensure_bearer_token.ex
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
defmodule ScreensWeb.Plug.EnsureBearerToken do
@moduledoc """
Ensures requests include a valid bearer token in the Authorization header.

The expected token is loaded from an environment variable.
"""

import Plug.Conn

def init(opts) do
Keyword.fetch!(opts, :env_var)
end

def call(conn, env_var) do
configured_token = System.get_env(env_var)
bearer_token = bearer_token_from_header(conn)

if valid_token?(configured_token, bearer_token) do
conn
else
unauthorized(conn)
end
end

defp bearer_token_from_header(conn) do
case get_req_header(conn, "authorization") do
["Bearer " <> token] -> token
_ -> nil
end
end

defp valid_token?(configured_token, bearer_token)
when is_binary(configured_token) and is_binary(bearer_token) do
byte_size(configured_token) == byte_size(bearer_token) and
Plug.Crypto.secure_compare(configured_token, bearer_token)
end

defp valid_token?(_configured_token, _bearer_token), do: false

defp unauthorized(conn) do
conn
|> put_resp_content_type("application/json")
|> send_resp(401, "{\"error\":\"unauthorized\"}")
|> halt()
end
end
12 changes: 12 additions & 0 deletions lib/screens_web/router.ex
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,10 @@ defmodule ScreensWeb.Router do
plug(ScreensWeb.Plug.EnsureScreensGroup)
end

pipeline :screens_api_client_auth do
plug(ScreensWeb.Plug.EnsureBearerToken, env_var: "SCREENS_API_CLIENT_KEY")
end

scope "/", ScreensWeb do
get "/_health", HealthController, :index
end
Expand Down Expand Up @@ -65,6 +69,7 @@ defmodule ScreensWeb.Router do
pipe_through [:redirect_prod_http, :api, :auth, :ensure_auth, :ensure_screens_group]

get "/", AdminApiController, :index
post "/screen_configs", AdminApiController, :update_screen_configs
post "/screens/validate", AdminApiController, :validate
post "/screens/validate/:id", AdminApiController, :validate
post "/screens/confirm", AdminApiController, :confirm
Expand Down Expand Up @@ -132,4 +137,11 @@ defmodule ScreensWeb.Router do

get "/screens_by_alert", ScreensByAlertController, :index
end

scope "/api", ScreensWeb do

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking nit - From the perspective of someone who is trying to hit these endpoints for the first time, they'd probably have to start digging through the code to figure out why they'd getting 401's back. It might be worth putting in the README that you need to set an authorization header in requests to these endpoints

pipe_through [:redirect_prod_http, :api, :screens_api_client_auth]

get "/screen_configs", ScreenConfigsApiController, :index
post "/screen_configs", ScreenConfigsApiController, :update
end
end
Loading