Allow Mounted Engine url_helpers to use config.relative_url_root

[bensheldon]

Summary

Allows mounted engine URL helpers, in the form of MyEngine::Engine.routes.url_helpers.some_path to use config.relative_url_root.

The logic change is to only override config.relative_url_root with "" if there is a SCRIPT_NAME in the URL options. This override prevented RouteSet#find_relative_url_root from accessing config.relative_url_root when it is set.

I expect this to address some of the issues like #31476, #42243.

Other Information

The existing tests imply that the proper way to mount Rails with a relative url root is to customize config.ru with a map directive, which produces the SCRIPT_NAME which Rails handles implicitly and correctly for the main Application and Mounted Engines.

The config.relative_url_root seems to be intended only for generating paths outside of a request/controller/view (e.g. when using *.routes.url_helpers.some_path).

My understanding seems validated by this comment: #24396 (comment)

Once this PR is in a good place, I can turn my attention to documentation in #42248. I've spent a long time tracking this down 😄

[jonathanhefner]

Excellent investigating! 🕵️ Please bear with me, as I am digging into this code for the first time myself!

#21804 appears to be at the root of this issue, and also has lots of great background info.

I think the code from #21804 makes sense in its context, but without that context it feels a bit like "spooky action at a distance". I am particularly curious about the "just set prefix_options[:script_name] to an empty string" approach mentioned in #21804 (comment). The author says that approach caused tests to fail, but I am wondering which tests, and if they would have been fixed by adding your if options[:original_script_name]. Specifically:

diff --git a/actionpack/lib/action_dispatch/routing/mapper.rb b/actionpack/lib/action_dispatch/routing/mapper.rb
index dc775ce83e..5990ff60d3 100644
--- a/actionpack/lib/action_dispatch/routing/mapper.rb
+++ b/actionpack/lib/action_dispatch/routing/mapper.rb
@@ -652,7 +652,7 @@ def define_generate_prefix(app, name)

             script_namer = ->(options) do
               prefix_options = options.slice(*_route.segment_keys)
-              prefix_options[:relative_url_root] = "" if options[:original_script_name]
+              prefix_options[:script_name] = "" if options[:original_script_name]

               if options[:_recall]
                 prefix_options.reverse_merge!(options[:_recall].slice(*_route.segment_keys))

diff --git a/actionpack/lib/action_dispatch/routing/route_set.rb b/actionpack/lib/action_dispatch/routing/route_set.rb
index 31bb5affe6..6b36c41cca 100644
--- a/actionpack/lib/action_dispatch/routing/route_set.rb
+++ b/actionpack/lib/action_dispatch/routing/route_set.rb
@@ -779,18 +779,14 @@ def generate(route_name, options, recall = {}, method_name = nil)

       RESERVED_OPTIONS = [:host, :protocol, :port, :subdomain, :domain, :tld_length,
                           :trailing_slash, :anchor, :params, :only_path, :script_name,
-                          :original_script_name, :relative_url_root]
+                          :original_script_name]

       def optimize_routes_generation?
         default_url_options.empty?
       end

       def find_script_name(options)
-        options.delete(:script_name) || find_relative_url_root(options) || ""
-      end
-
-      def find_relative_url_root(options)
-        options.delete(:relative_url_root) || relative_url_root
+        options.delete(:script_name) || relative_url_root || ""
       end

       def path_for(options, route_name = nil, reserved = RESERVED_OPTIONS)

If that works, I feel like it might be a clearer solution.

[bensheldon]

@jonathanhefner I agree this is complicated 😄 I'll give your patch a try and let's see what CI tells us 👍🏻

[bensheldon]

@jonathanhefner I've made those changes and tests are passing ✅

Your change does change the route helper behavior:

# old behavior
Rails.application.routes.url_helpers.root_path(relative_url_root: "/foo") #=> "/foo/" 

# new behavior
Rails.application.routes.url_helpers.root_path(relative_url_root: "/foo") #=> "/?relative_url_root=foo"

...though I'm not sure if anyone is using that argument of the url helpers. I didn't find anything public with GitHub Code Search. 🙇‍♀️

[bensheldon]

Thinking about this more, I will add some additional test-cases.

Currently there is the expectation that the config.ru is using map which results in the current test-cases using SCRIPT_NAME in their requests e.g. whatever reverse-proxy they are using for routing is sending down the full path to the Ruby Server, which then maps it to Rails, rather than the reverse-proxy only sending down the partial path:

https://github.com/rails/rails/pull/45719/files#diff-b93579f6233ade0fc8e05d8b79cbe745b7546465310477554f4e268e994c5e08R1530

I don't think everyone configures their reverse-proxy and config.ru in that way. So I will add test-cases when SCRIPT_NAME is not populated and ensure that relative_url_root is still included in the URLs.

[bensheldon]

I changed the tests locally, to what I described and they didn't pass.

But I feel confident that the behavior in this PR is now consistent across the main Rails app and Engines.

So, @jonathanhefner, this is ready for review as-is.

[jonathanhefner]

I changed the tests locally, to what I described and they didn't pass.

Thank you for digging into it further! Do you mean those tests did not pass with main or with this branch (or both)?

Your change does change the route helper behavior ... though I'm not sure if anyone is using that argument of the url helpers

That behavior was added as a by-product of #21804. I don't believe it was intended to be part of the public API. (It's not documented, as far as I can see.)

Here is my thinking for setting prefix_options[:script_name] instead of prefix_options[:relative_url_root] (and some justification for checking if options[:original_script_name]):

• SCRIPT_NAME is the primary source for the root path.
• ENV["RAILS_RELATIVE_URL_ROOT"] / config.relative_url_root / ActionDispatch::Routing::RouteSet.relative_url_root is used as a fallback in contexts where SCRIPT_NAME is not defined.
• Via this code (and tested by these tests), generating an engine route from...
  • ...the engine sets options[:script_name] == env["SCRIPT_NAME"] and options[:original_script_name] == nil.
  • ...the application sets options[:original_script_name] == env["ORIGINAL_SCRIPT_NAME"] == env["SCRIPT_NAME"] and options[:script_name] == nil.
  • ...another engine sets options[:original_script_name] == env["ORIGINAL_SCRIPT_NAME"] and options[:script_name] == nil.
• If options[:original_script_name] is present, the root path is built from options[:original_script_name] + find_script_name
• The first thing find_script_name tries is options[:script_name].
• If options[:original_script_name] is present, then SCRIPT_NAME is defined. i.e. We are in a context where we normally would not fall back to relative_url_root.
• Thus, the most direct way to ensure that relative_url_root isn't used when it should not be used is to set options[:script_name] to a value that does not interfere with other use cases (i.e. "" if options[:original_script_name]).

If any of that sounds questionable, though, please question it! 😃 Otherwise, I agree this looks good, and will merge soon.

[bensheldon]

@jonathanhefner that seems like the right explanation to me ✅ I just squashed down to a single commit.

Do you mean those tests did not pass with main or with this branch (or both)?

They fail on main too. Both. And I think we should discuss it in another Issue because I really want this fix to go out because it makes Engines consistent with the rest of Rails behavior... and then, if we want (probably through the documentation) we can discuss if that global behavior should be different 😄

[jonathanhefner]

Thank you, @bensheldon! 🚀