Extract MFA for only, only_matching

[ryansch]

This PR adds an MFA tuple option to both only and only_matching.

I'm using this in a phx project like so:

defmodule PagesWeb.StaticDigest do
  @moduledoc """
  This module contains utilities for working with static assets
  that have been digested (`mix phx.digest`).
  """

  @doc """
  Generate 'only rules' for digested static assets based on existing
  rules from the user.
  """
  def generate_only_rules(endpoint, only)
      when is_list(only) do
    latest = endpoint.config(:cache_static_manifest_latest, %{})

    digest_only =
      only
      |> Stream.filter(fn x -> Map.has_key?(latest, x) end)
      |> Enum.map(fn x -> Map.get(latest, x) end)

    only ++ digest_only
  end
end

# In the Endpoint
  plug Plug.Static,
    at: "/",
    from: :pages_web,
    gzip: false,
    only:
      {PagesWeb.StaticDigest, :generate_only_rules,
       [
         __MODULE__,
         ~w(assets css fonts images js sprites favicon.ico favicon.svg site.webmanifest browserconfig.xml robots.txt)
       ]}

This has the effect of allowing the digested versions of all of the allowed paths without needing to add a rule like only_matching: ~w(site).

[josevalim]

Hi @ryansch, thanks for the PR!

We typically want to avoid MFA tuples whenever possible because you can just as easily configure plugs at runtime. In your paticular case, only/only_matching can be customized by doing this:

@static Plug.Static.init(all_options_but_only)

def my_static(conn, _) do
  if my_only?(conn), do: Plug.Static.call(conn, @static), else: conn
end

[ryansch]

@josevalim I was able to come up with the following code based on your comment.

  @static Plug.Static.init(
    at: "/",
    from: :pages_web,
    gzip: false,
    only: ~w(assets css fonts images js sprites favicon.ico favicon.svg site.webmanifest browserconfig.xml robots.txt)
  )
  def static(conn, _) do
    {only, only_matching} = @static.only_rules

    only =
      PagesWeb.StaticDigest.generate_only_rules(
        __MODULE__,
        only
    )

    opts =
      @static
      |> Map.put(:only_rules, {only, only_matching})

    Plug.Static.call(conn, opts)
  end
  plug :static

Am I going about this the right way? I don't know until runtime what the asset digests will be from mix phx.digest. I don't like how much this code needs to know about the internal structure of the map that Plug.Static passes itself from init to call.

[josevalim]

I don't think you need to do that at all. What only does is something like this:

%{path_info: [path | _]} = conn
if path in only or :binary.match(path, only_matching) do
  ...

And you can directly reproduce this logic without building your own only/only_matching,.

[ryansch]

@josevalim While your solution does work, a more complete solution seems to require borrowing code from Plug.Static.

I started with a small function based plug like you suggested. I then realized that if I want this to be reusable across projects, I needed to handle the combination of only with at. I also looked at Plug.Static's implementation of allowed? and noticed that it's being careful to pin the match at the beginning of the string.

defmodule PagesWeb.Plugs.Static do
  @behaviour Plug
  @allowed_methods ~w(GET HEAD)

  alias Plug.Conn

  @impl Plug
  def init(opts) do
    plug_static_opts =
      opts
      |> Keyword.delete(:only)
      |> Keyword.delete(:only_matching)

    %{
      at: opts |> Keyword.fetch!(:at) |> Plug.Router.Utils.split(),
      endpoint: Keyword.fetch!(opts, :endpoint),
      only: Keyword.get(opts, :only, []),
      only_matching: Keyword.get(opts, :only_matching, []),
      plug_static: Plug.Static.init(plug_static_opts)
    }
  end

  @impl Plug
  def call(
        conn = %Conn{method: meth},
        %{at: at, endpoint: endpoint, only: only, only_matching: only_matching, plug_static: plug_static} = _opts
      )
      when meth in @allowed_methods do
    only =
      PagesWeb.StaticDigest.generate_only_rules(
        endpoint,
        only
      )

    segments = subset(at, conn.path_info)

    if allowed?({only, only_matching}, segments) do
      Plug.Static.call(conn, plug_static)
    else
      conn
    end
  end

  defp allowed?(_only_rules, []), do: false
  defp allowed?({[], []}, _list), do: true

  defp allowed?({full, prefix}, [h | _]) do
    h in full or (prefix != [] and match?({0, _}, :binary.match(h, prefix)))
  end

  defp subset([h | expected], [h | actual]), do: subset(expected, actual)
  defp subset([], actual), do: actual
  defp subset(_, _), do: []
end

Plug.Static is already doing a good job of handing the logic around the only and only_matching rules. I was hoping to not reimplement what it's already doing.