From 8b407530ea38c5386d0abe72aaabf51a37a5ac8f Mon Sep 17 00:00:00 2001 From: Daniel Widgren Date: Mon, 10 Aug 2026 12:46:02 +0200 Subject: [PATCH] fix(sup): resolve nova_apps in order and let applications override the error pages Two defects found while boot-testing Nova end to end. Both are on master and neither depends on the routing rewrite, so they are split out to be reviewed and backported on their own. resolve_nova_apps/2 (#380) called lists:reverse/1 in its base clause on an accumulator the caller then kept accumulating into. The result is that the resolved order is scrambled for any nova_apps list of two or more entries, nested or not: [a, b] came back as [b, a] and [a, b, c] as [c, a, b]. Since routes are first-wins, that silently inverted route precedence between sibling applications. It also looked up nested applications using the whole {App, Options} tuple as the application name, so a sub-application declared in tuple form - the form the multi-app guide documents for setting a prefix - never had its own nova_apps resolved at all. Rewritten with the seen-set kept separate from the output, so the reversal happens once at the end, and with the application name taken out of either form before the environment lookup. Separately, nova was compiled before the bootstrap application. Because routes are first-wins, Nova's own 404 and 500 always won, so a {404, fun my_controller:not_found/1, #{}} entry in an application's router was silently ignored - the entry compiled, registered, and then never matched. nova is now compiled last, which makes its error pages the defaults they were meant to be. The ordering decision is extracted into compile_order/1 so it can be tested without starting a listener. nova_sup_tests covers both, and 8 of its 14 cases fail against the current implementation. --- src/nova_sup.erl | 52 +++++++++---- test/nova_sup_tests.erl | 157 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 196 insertions(+), 13 deletions(-) create mode 100644 test/nova_sup_tests.erl diff --git a/src/nova_sup.erl b/src/nova_sup.erl index 09731ef..e4bbd30 100644 --- a/src/nova_sup.erl +++ b/src/nova_sup.erl @@ -21,6 +21,8 @@ -define(NOVA_STD_PORT, 8080). -define(NOVA_STD_SSL_PORT, 8443). +-type nova_app() :: atom() | {atom(), map()}. + %%%=================================================================== %%% API functions @@ -149,8 +151,7 @@ start_cowboy(Configuration) -> ?LOG_ERROR(#{msg => <<"You need to define bootstrap_application option in configuration">>}), throw({error, no_nova_app_defined}); App -> - ExtraApps = application:get_env(App, nova_apps, []), - nova_router:compile(resolve_nova_apps([nova, App | ExtraApps], [])) + nova_router:compile(compile_order(App)) end, CowboyOptions2 = @@ -224,20 +225,45 @@ get_version(Application) -> not_found end. +%% @doc The order applications are compiled in. +%% +%% nova is compiled last so that its own 404 and 500 routes act as defaults. +%% Routes are first-wins, so compiling nova first made an application's own +%% status-code routes unreachable. +-spec compile_order(App :: atom()) -> [nova_app()]. +compile_order(App) -> + ExtraApps = application:get_env(App, nova_apps, []), + resolve_nova_apps([App | ExtraApps] ++ [nova]). + %% @doc Recursively resolve nested nova_apps. %% Each nova_app can declare its own nova_apps dependencies. %% Dependencies are resolved depth-first so child app routes -%% are registered before the parent. --spec resolve_nova_apps([atom()], [atom()]) -> [atom()]. -resolve_nova_apps([], Acc) -> - lists:reverse(Acc); -resolve_nova_apps([App | Rest], Acc) -> - case lists:member(App, Acc) of +%% are registered before the parent. An application already resolved is +%% skipped, so a cycle terminates. +-spec resolve_nova_apps([nova_app()]) -> [nova_app()]. +resolve_nova_apps(Apps) -> + {Resolved, _Seen} = resolve_nova_apps(Apps, [], []), + Resolved. + +-spec resolve_nova_apps([nova_app()], [nova_app()], [atom()]) -> {[nova_app()], [atom()]}. +resolve_nova_apps([], Acc, Seen) -> + {lists:reverse(Acc), Seen}; +resolve_nova_apps([App | Rest], Acc, Seen) -> + Name = nova_app_name(App), + case lists:member(Name, Seen) of true -> - %% Already resolved — skip to prevent cycles - resolve_nova_apps(Rest, Acc); + resolve_nova_apps(Rest, Acc, Seen); false -> - Nested = application:get_env(App, nova_apps, []), - Acc1 = resolve_nova_apps(Nested, [App | Acc]), - resolve_nova_apps(Rest, Acc1) + Nested = application:get_env(Name, nova_apps, []), + {NestedApps, Seen0} = resolve_nova_apps(Nested, [], [Name | Seen]), + resolve_nova_apps(Rest, [App | lists:reverse(NestedApps)] ++ Acc, Seen0) end. + +%% A nova_app is either the application name or {Name, Options}. +-spec nova_app_name(nova_app()) -> atom(). +nova_app_name({App, _Options}) -> App; +nova_app_name(App) -> App. + +-ifdef(TEST). +-compile(export_all). +-endif. diff --git a/test/nova_sup_tests.erl b/test/nova_sup_tests.erl new file mode 100644 index 0000000..d112998 --- /dev/null +++ b/test/nova_sup_tests.erl @@ -0,0 +1,157 @@ +-module(nova_sup_tests). +-include_lib("eunit/include/eunit.hrl"). +-include("../include/nova_router.hrl"). + +%%==================================================================== +%% resolve_nova_apps/1 +%%==================================================================== + +%% Regression: the base clause reversed an accumulator the caller then kept +%% accumulating into, so even a flat list came back out of order - [a, b, c] +%% resolved to [c, a, b]. Any nova_apps list of two or more was affected, not +%% only nested ones. +resolve_flat_list_test() -> + ?assertEqual([a, b, c], nova_sup:resolve_nova_apps([a, b, c])). + +resolve_pair_test() -> + ?assertEqual([a, b], nova_sup:resolve_nova_apps([a, b])). + +resolve_empty_test() -> + ?assertEqual([], nova_sup:resolve_nova_apps([])). + +resolve_nested_keeps_order_test() -> + with_env([{parent, nova_apps, [child]}], fun() -> + ?assertEqual([child, parent, nova], nova_sup:resolve_nova_apps([parent, nova])) + end). + +resolve_nested_depth_first_test() -> + with_env([{parent, nova_apps, [child]}, + {child, nova_apps, [grandchild]}], fun() -> + ?assertEqual([grandchild, child, parent], nova_sup:resolve_nova_apps([parent])) + end). + +resolve_several_parents_test() -> + with_env([{parent_a, nova_apps, [child_a]}, + {parent_b, nova_apps, [child_b]}], fun() -> + ?assertEqual([child_a, parent_a, child_b, parent_b, nova], + nova_sup:resolve_nova_apps([parent_a, parent_b, nova])) + end). + +resolve_deduplicates_test() -> + with_env([{parent_a, nova_apps, [shared]}, + {parent_b, nova_apps, [shared]}], fun() -> + ?assertEqual([shared, parent_a, parent_b], + nova_sup:resolve_nova_apps([parent_a, parent_b])) + end). + +resolve_terminates_on_a_cycle_test() -> + with_env([{a, nova_apps, [b]}, + {b, nova_apps, [a]}], fun() -> + ?assertEqual([b, a], nova_sup:resolve_nova_apps([a])) + end). + +%% Regression: a sub-application given as {Name, Options} was looked up with +%% the whole tuple as the application name, so its own nova_apps were never +%% resolved. +resolve_nested_under_tuple_form_test() -> + with_env([{parent, nova_apps, [child]}], fun() -> + ?assertEqual([child, {parent, #{prefix => "/p"}}], + nova_sup:resolve_nova_apps([{parent, #{prefix => "/p"}}])) + end). + +resolve_tuple_form_is_deduplicated_by_name_test() -> + ?assertEqual([{parent, #{}}], + nova_sup:resolve_nova_apps([{parent, #{}}, parent])). + +%%==================================================================== +%% compile_order/1 +%%==================================================================== + +%% Regression: nova was compiled first, and because routes are first-wins its +%% default 404 always won, so an application's own status-code route was +%% silently ignored. +nova_is_compiled_last_test() -> + with_env([{my_app, nova_apps, []}], fun() -> + ?assertEqual([my_app, nova], nova_sup:compile_order(my_app)) + end). + +nova_is_compiled_last_with_nested_apps_test() -> + with_env([{my_app, nova_apps, [child]}], fun() -> + ?assertEqual([child, my_app, nova], nova_sup:compile_order(my_app)) + end). + +%%==================================================================== +%% Status-code route precedence +%% +%% compile_order/1 only matters because of how the routing table resolves a +%% duplicate, so pin that too: whoever is compiled first keeps the route. +%%==================================================================== + +application_status_route_beats_nova_default_test_() -> + {setup, fun setup_compile/0, fun cleanup_compile/1, fun() -> + nova_router:compile([status_app, nova]), + {ok, _Bindings, Value} = nova_router:lookup_url('_', 404, '_'), + ?assertEqual(status_app, Value#nova_handler_value.app) + end}. + +nova_default_status_route_is_used_when_the_application_has_none_test_() -> + {setup, fun setup_compile/0, fun cleanup_compile/1, fun() -> + nova_router:compile([plain_app, nova]), + {ok, _Bindings, Value} = nova_router:lookup_url('_', 404, '_'), + ?assertEqual(nova, Value#nova_handler_value.app) + end}. + +%% The order the old code used, kept as an explicit statement of what the +%% regression looked like from the outside. +nova_first_hides_the_applications_status_route_test_() -> + {setup, fun setup_compile/0, fun cleanup_compile/1, fun() -> + nova_router:compile([nova, status_app]), + {ok, _Bindings, Value} = nova_router:lookup_url('_', 404, '_'), + ?assertEqual(nova, Value#nova_handler_value.app) + end}. + +setup_compile() -> + Prev = nova_test_helper:setup_nova_env(), + application:set_env(nova, dispatch_backend, persistent_term), + persistent_term:put(nova_dispatch, routing_tree:new(#{use_strict => false, convert_to_binary => true})), + persistent_term:put(nova_apps, []), + persistent_term:put(nova_plugins, []), + + meck:new(status_app_router, [non_strict]), + meck:expect(status_app_router, routes, + fun(_Env) -> + [#{routes => [{404, fun(_Req) -> {status, 404} end, #{}}]}] + end), + + meck:new(plain_app_router, [non_strict]), + meck:expect(plain_app_router, routes, + fun(_Env) -> + [#{routes => [{"/plain", fun(_Req) -> {status, 200} end, #{methods => [get]}}]}] + end), + Prev. + +cleanup_compile(Prev) -> + meck:unload(status_app_router), + meck:unload(plain_app_router), + persistent_term:erase(nova_dispatch), + persistent_term:erase(nova_apps), + persistent_term:erase(nova_plugins), + nova_test_helper:cleanup_nova_env(Prev). + +%%==================================================================== +%% Helpers +%%==================================================================== + +%% Set application environment keys for the duration of Fun, restoring +%% whatever was there before. +with_env(Vars, Fun) -> + Saved = [{App, Key, application:get_env(App, Key)} || {App, Key, _Value} <- Vars], + [application:set_env(App, Key, Value) || {App, Key, Value} <- Vars], + try + Fun() + after + [restore(App, Key, Previous) || {App, Key, Previous} <- Saved] + end. + +restore(App, Key, undefined) -> application:unset_env(App, Key); +restore(App, Key, {ok, Value}) -> application:set_env(App, Key, Value).