Mounted engine route fix

[merhard]

This pull request aims to fix a regression in master that has been present in Rails since v4.2.3

When generating the url for a mounted engine through its proxy, the path should be the sum of three parts:

1. Any SCRIPT_NAME request header or the value of relative_url_root.
2. A prefix (the engine's mounted path).
3. The path of the named route inside the engine.

Since 44ff031, this has been broken. Instead, the path is the sum of four parts:

1. Any SCRIPT_NAME request header or the value of relative_url_root.
2. The value of relative_url_root.
3. A prefix (the engine's mounted path).
4. The path of the named route inside the engine.

This commit fixes the regression by using the value of default_url_options[:script_name] or an empty string as a benign truthy value for options[:script_name] when the prefix is being generated, avoiding the duplicate relative_url_root value in the final result.

This addresses issues #20920 and #21459.

[rails-bot]

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @sgrif (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[merhard]

Opened this pull request per @rafaelfranca suggestion here: #21743

/cc @arthurnn

[pixeltrix]

@merhard I'm slightly confused - why are we pulling :script_name out of default_url_options? The :script_name should be coming from the request's SCRIPT_NAME CGI variable - not from default url options specified outside of a request context. Won't this essentially mean that prefix_options[:script_name] is always set to an empty string and thereby nullify the effect of 44ff031 because options[:script_name] will always be set to something?

If 44ff031 is wrong then we need to fix it but this seems to just obfuscate matters.

[merhard]

@pixeltrix Good question. The SCRIPT_NAME CGI variable is accounted for at a different place in the code. At this point, Rails is just trying to find the path where the engine is mounted and should be ignoring SCRIPT_NAME and relative_url_root. The bug is that relative_url_root is not being ignored when generating the engine prefix.

As you can see from my first commit, I originally just set prefix_options[:script_name] to an empty string. As you can see from my second commit, this broke a couple tests. Checking default_url_options first fixed the broken tests.

I have an alternative solution to fix this bug at #20958. Maybe that solution is better?

Let me try to explain the flow of the code when Rails generates the value for mounted engine named routes:

module Foo
  class Engine < Rails::Engine
    isolate_namespace Foo

    routes.draw do
      get 'bar', to: 'bar#show'
    end
  end
end

module ExampleApp
  class Application < Rails::Application
    routes.draw do
      mount Foo::Engine, at: '/foo'
      root 'welcome#index'
    end

    config.relative_url_root = '/relative_url_root'
  end
end

class WelcomeController < ActionController::Base
  def index
    render text: foo.bar_path
  end
end

If you were to navigate to the root path of the ExampleApp, the response body should be "/relative_url_root/foo/bar".

If you were to navigate to the root path of the ExampleApp with a request header "SCRIPT_NAME" => "/script_name", the response body should be "/script_name/foo/bar" because SCRIPT_NAME should supersede relative_url_root.

In master (and Rails >= v4.2.3), the response body will be "/script_name/relative_url_root/foo/bar". This is the bug that 44ff031 introduced and this pull request is attempting to fix. The path should have SCRIPT_NAME or relative_url_root, not both. In Rails < v4.2.3, the response body would be "/script_name/foo/bar". This is a breaking change in behavior from v4.2.2 to v4.2.3

Now let's walk through what Rails does when resolving foo.bar_path with a request header "SCRIPT_NAME" => "/script_name":

1. The message bar_path is sent to the object foo, an ActionDispatch::Routing::RoutesProxy. This boils down to #url_for here.
   • options is a hash that is prepared in part by this code. Because we are in a route proxy, this method will set the request header "SCRIPT_NAME" => "/script_name" to options[:original_script_name] and leave options[:script_name] unset. Commit 5b3bb61 explains why this is done.
   • route_name is "bar".
2. Line 686 sets original_script_name to "/script_name"
3. Line 687 calls #find_script_name here. This code was extended into the engine routes when Foo::Engine was mounted in the application routes. Since options[:script_name] was left unset in step 1, the else branch is run.
   • name is the name of the engine, in this case "foo".
   • prefix_options will only be the route segment keys. Both prefix_options[:script_name] and prefix_options[:original_script_name] will be left unset.
   • The message foo_path is sent to the application's route set. This boils down to #url_for here with options equal to prefix_options and route_name equal to "foo".
4. Line 687 calls #find_script_name here. Since options.delete(:script_name) returns nil, this method will return the value of relative_url_root, "/relative_url_root". This #url_for returns "/relative_url_root/foo" as the result of the method call in step 3, setting script_name equal to "/relative_url_root/foo".
5. original_script_namefrom step 2 and script_name from step 3-4 are joined, resulting in script_name being set to "/script_name/relative_url_root/foo".
6. The #url_for call from step 1 returns "/script_name/relative_url_root/foo/bar" as the value of foo.bar_path.

Step 4 is where the bug resides. This step should return "/foo", ignoring relative_url_root because that value should already be accounted for in step 2.

[eltiare]

To make the case for this pull request clear: #21459

[pixeltrix]

@eltiare there's no doubt that it needs fixing - just want to make sure that it's the right fix. Often with these routing changes in patch releases we can end up with cascading regressions from one release to another as we try to patch edge cases.

[merhard]

@pixeltrix I pushed a new commit to this pull request. Do you think the end result is more intention revealing?

[pixeltrix]

@merhard I think that's much better - can you clean up the commit history and add a CHANGELOG entry, thanks

[sgrif]

r? @pixeltrix You seem to be on top of this one

[merhard]

@pixeltrix CHANGELOG entry added. Commits squashed and rebased.

[pixeltrix]

@merhard thanks very much for your patience and perseverance 👍

[merhard]

@pixeltrix You're welcome. This was my first Rails commit. Thank you for the feedback and your help getting this merged. 👍

[merhard]

@pixeltrix Any plans to backport this to 4-2-stable? The pull request that introduced the regression (#18775) was backported at 0703453

[pixeltrix]

@merhard backported in 37864d7 - thanks again.