Repository navigation
honor sub-app routing settings inheritance - #7561
UlisesGascon wants to merge 1 commit into
Conversation
yxshee
left a comment
There was a problem hiding this comment.
I ran the branch (9e4f86a, tests only, so against current lib): 8 of the 14 fail, which matches the description. Both settings fail the same way: "registered before mount", "reported setting and router in agreement", "parent guard on a sub-path" and "conflicting parents". "Registered after mount", "explicit sub-app setting" and "guard on the full mount path" already pass, since the router is only created on the first route and by then app.get() resolves through the parent.
The guard case is the one I'd lead with when discussing this. On master app.all('/blog/uSer', deny) plus a case-sensitive parent still lets GET /blog/user reach the sub-app's /uSer route (200), because that sub-app's router was frozen case-insensitive.
A few cases the suite doesn't pin down yet, all checked on master:
- The parent changes a setting after the sub-app's router exists.
app.use('/b', b); b.get('/uSer', ...); app.enable('case sensitive routing')givesGET /b/user→ 200, whileb.enabled('case sensitive routing')reportstrue. It's the same mismatch as "registered before mount", but triggered from the parent's side. Should the router follow later parent changes too, or should the setting be fixed at mount? - Nested sub-apps. A grandchild with routes added after both mounts already inherits (
/b/c/user→ 404 with a case-sensitive top app). A test for the "before mount" version at two levels would keep that working when the fix lands. express.Router()mounted on a case-sensitive app stays case-insensitive (/r/user→ 200), since routers take their owncaseSensitiveoption. That's expected, but if the docs change in expressjs.com#2550 talks about guards, it may be worth saying it only covers sub-apps.- The conflicting-parents test expects
loose.use('/blog', blog)to throw. Today the second mount works, and the sub-app keeps the first parent's settings (GET /b/user/through the loose parent → 404 under strict). Throwing makes sense for the security case, but it would also break apps that mount one sub-app under two parents and never route anything that depends on the setting. Throwing only when the effective settings actually differ (which the regexp suggests) seems like the right line.
This PR is not solving the problem (for now) just documenting the breaking change we want to make (see: expressjs/expressjs.com#2550) in order to extend the inherit behavior.
We need to discuss first what changes are needed and where to put them (express vs. router), etc...
Originally this was planned for 5.x: #2564