fix: resolve nova_apps in declaration order and let applications override the default error pages - #406
Open
Taure wants to merge 1 commit into
Open
fix: resolve nova_apps in declaration order and let applications override the default error pages#406Taure wants to merge 1 commit into
Taure wants to merge 1 commit into
Conversation
…e 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two defects found while boot-testing Nova end to end for #405. Both are on master today and neither depends on the routing rewrite, so they are split out here to be reviewed — and backported — on their own.
resolve_nova_apps/2returns applications in the wrong orderresolve_nova_apps/2(added in #380) callslists:reverse/1in its base clause, on an accumulator that the caller then keeps accumulating into. So the reversal happens once per recursion step rather than once at the end.The effect is not limited to nested applications, which is what I first assumed — it is wrong for any
nova_appslist of two or more entries:Routes are first-wins, so this silently inverted route precedence between sibling applications. Anyone relying on declaration order to decide which application owns an overlapping path has had the opposite of what they asked for.
It also resolves nested applications with the wrong key:
Apphere can be a{Name, Options}tuple — the formguides/multi-app.mddocuments for setting a prefix — andapplication:get_env/3misses on a tuple key and returns the default. A sub-application declared in tuple form therefore never has its ownnova_appsresolved at all.Rewritten with the seen-set kept separate from the output, so the reversal happens once, and with the application name extracted from either form before the environment lookup.
An application's own status-code routes are unreachable
start_cowboy/1compiled[nova, App | ExtraApps], so Nova's own routes went in first. Routes are first-wins, so Nova's default 404 and 500 always won and this in an application's router:compiled, registered, and then never matched. No error, no warning — the custom error page simply never appeared.
novais now compiled last, which makes its error pages the defaults they were always meant to be. An application without its own status routes is unaffected.The ordering decision is extracted into
compile_order/1so it can be tested without starting a listener.Tests
test/nova_sup_tests.erlcovers both. 8 of its 14 cases fail against the current implementation — flat ordering, nested ordering, cycle handling, deduplication, both tuple-form defects, and the compile order.rebar3 eunit372/372,xrefanddialyzerclean.Relationship to #405
#405 contains both fixes already, as part of its
nova_suprewrite. If this merges first, #405 needs agit merge origin/masterand its ownnova_supkept — it is a strict superset. Happy to land them in either order; this one is the smaller, safer half.