From e76ef6d919d4e19f9364b3c46cdf5129f4c8472f Mon Sep 17 00:00:00 2001 From: Leandro Pereira Date: Wed, 11 Dec 2024 15:04:44 -0500 Subject: [PATCH] fix: do not crash boot when routes are missing (#701) --- lib/beacon/router.ex | 39 ++++++------------ .../web/controllers/check_controller.ex | 16 ++++++++ test/beacon/router_test.exs | 31 +++++++------- test/support/routers.ex | 41 ++++++++++++++----- 4 files changed, 75 insertions(+), 52 deletions(-) create mode 100644 lib/beacon/web/controllers/check_controller.ex diff --git a/lib/beacon/router.ex b/lib/beacon/router.ex index d5d1cd44..debb1224 100644 --- a/lib/beacon/router.ex +++ b/lib/beacon/router.ex @@ -145,6 +145,9 @@ defmodule Beacon.Router do get "/__beacon_assets__/css-:md5", Beacon.Web.AssetsController, :css, assigns: %{site: opts[:site]} get "/__beacon_assets__/js-:md5", Beacon.Web.AssetsController, :js, assigns: %{site: opts[:site]} + # simulate a beacon page inside site prefix so we can check this site is reachable?/2 + get "/__beacon_check__", Beacon.Web.CheckController, :check, metadata: %{site: opts[:site]} + live "/*path", Beacon.Web.PageLive, :path end end @@ -250,27 +253,13 @@ defmodule Beacon.Router do @doc false # Tells if a `beacon_site` is reachable in the current environment. # - # Supposed the following router: - # - # scope "/", MyAppWeb, host: ["beacon-site-a.fly.dev"] do - # pipe_through [:browser] - # beacon_site "/", site: :site_a - # end - # - # scope "/", MyAppWeb, host: ["beacon-site-b.fly.dev"] do - # pipe_through [:browser] - # beacon_site "/", site: :site_b - # end - # - # On a node deployed to beacon-site-a.fly.dev, the second `beacon_site` - # will never match, so starting `:site_b` is a waste of resources - # and a common cause of problems on BeaconLiveAdmin since we can't resolve URLs properly. - # - # Similarly, if a `get "/"` is added _before_ either `beacon_site` that `get` would always - # match and invalidate the `beacon_site` mount. + # It's considered reachable if a dynamic page can be served on the site prefix. def reachable?(%Beacon.Config{} = config, opts \\ []) do %{site: site, endpoint: endpoint, router: router} = config - function_exported?(router, :__beacon_scoped_prefix_for_site__, 1) && reachable?(site, endpoint, router, opts) + reachable?(site, endpoint, router, opts) + rescue + # missing router or missing beacon macros in the router + _ -> false end defp reachable?(site, endpoint, router, opts) do @@ -281,14 +270,10 @@ defmodule Beacon.Router do router.__beacon_scoped_prefix_for_site__(site) end) - case Phoenix.Router.route_info(router, "GET", prefix, host) do - # bypass and allow booting beacon sites even though there's a route conflict - # but only for root paths, for example: - # live / - # beacon_site / - # that's because even though they share the same prefix, - # beacon can still serve pages at /:page - %{route: "/"} -> + path = Beacon.Router.sanitize_path(prefix <> "/__beacon_check__") + + case Phoenix.Router.route_info(router, "GET", path, host) do + %{site: ^site, plug: Beacon.Web.CheckController} -> true %{phoenix_live_view: {Beacon.Web.PageLive, _, _, %{extra: %{session: %{"beacon_site" => ^site}}}}} -> diff --git a/lib/beacon/web/controllers/check_controller.ex b/lib/beacon/web/controllers/check_controller.ex new file mode 100644 index 00000000..9a469052 --- /dev/null +++ b/lib/beacon/web/controllers/check_controller.ex @@ -0,0 +1,16 @@ +defmodule Beacon.Web.CheckController do + @moduledoc false + # TODO: replace CheckController with a plug in https://github.com/BeaconCMS/beacon/pull/694 + + import Plug.Conn + + def init(conn), do: conn + + def call(conn, _) do + conn + |> put_resp_header("content-type", "text/plain") + |> put_resp_header("cache-control", "public, max-age=31536000, immutable") + |> send_resp(200, "") + |> halt() + end +end diff --git a/test/beacon/router_test.exs b/test/beacon/router_test.exs index 25833863..8e5c2538 100644 --- a/test/beacon/router_test.exs +++ b/test/beacon/router_test.exs @@ -63,32 +63,35 @@ defmodule Beacon.RouterTest do end describe "reachable?" do - setup do - site = :host_test - config = Beacon.Config.fetch!(site) - [config: config] + defp config(site, opts \\ []) do + Map.merge( + Beacon.Config.fetch!(site), + Enum.into(opts, %{router: Beacon.BeaconTest.ReachTestRouter}) + ) end - test "match existing host", %{config: config} do - valid_host = "host.com" - assert Router.reachable?(config, host: valid_host) + test "match existing host" do + config = config(:host_test) + assert Router.reachable?(config, host: "host.com", prefix: "/host_test") end - test "existing nested conflicting route", %{config: config} do - valid_host = "host.com" - refute Router.reachable?(config, host: valid_host, prefix: "/nested/some_page") + test "existing nested conflicting route" do + config = config(:not_booted) + refute Router.reachable?(config, host: nil, prefix: "/conflict") end - test "with no specific host", %{config: config} do + test "root path with no host" do + config = config(:my_site) assert Router.reachable?(config, host: nil) end - test "do not match any existing host/path", %{config: config} do - refute Router.reachable?(config, host: nil, prefix: "/nested/invalid") + test "not reachable when does not match any existing host/path" do + config = config(:my_site) + refute Router.reachable?(config, host: nil, prefix: "/other") end test "router without beacon routes" do - config = Beacon.Config.fetch!(:no_routes) + config = config(:my_site, router: Beacon.BeaconTest.NoRoutesRouter) refute Router.reachable?(config) end end diff --git a/test/support/routers.ex b/test/support/routers.ex index e4c97810..a23befc3 100644 --- a/test/support/routers.ex +++ b/test/support/routers.ex @@ -2,7 +2,7 @@ defmodule Beacon.BeaconTest.NoRoutesRouter do use Beacon.BeaconTest.Web, :router end -defmodule Beacon.BeaconTest.Router do +defmodule Beacon.BeaconTest.ReachTestRouter do use Beacon.BeaconTest.Web, :router use Beacon.Router @@ -14,24 +14,43 @@ defmodule Beacon.BeaconTest.Router do plug :put_secure_browser_headers end - scope "/nested" do + scope path: "/host_test", host: "host.com" do pipe_through :browser - beacon_site "/site", site: :booted - beacon_site "/media", site: :s3_site + beacon_site "/", site: :host_test end - scope "/" do + scope path: "/conflict" do pipe_through :browser + live "/:page", Beacon.BeaconTest.LiveViews.FooBarLive + beacon_site "/", site: :not_booted + end - live_session :default do - live "/", Beacon.BeaconTest.LiveViews.FooBarLive - live "/nested/:page", Beacon.BeaconTest.LiveViews.FooBarLive - end + scope path: "/my_site" do + pipe_through :browser + beacon_site "/", site: :my_site end - scope path: "/", host: "host.com" do + scope path: "/other" do pipe_through :browser - beacon_site "/", site: :host_test + end +end + +defmodule Beacon.BeaconTest.Router do + use Beacon.BeaconTest.Web, :router + use Beacon.Router + + pipeline :browser do + plug :accepts, ["html"] + plug :fetch_session + plug :fetch_live_flash + plug :protect_from_forgery + plug :put_secure_browser_headers + end + + scope "/nested" do + pipe_through :browser + beacon_site "/site", site: :booted + beacon_site "/media", site: :s3_site end # `alias` is not really used but is present here to verify that `beacon_site` has no conflicts with custom aliases