Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mounted engine route fix #21804

Merged
merged 1 commit into from Oct 7, 2015
Merged

Conversation

@merhard
Copy link
Contributor

merhard commented Sep 28, 2015

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

This comment has been minimized.

Copy link

rails-bot commented Sep 28, 2015

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

This comment has been minimized.

Copy link
Contributor Author

merhard commented Sep 28, 2015

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

/cc @arthurnn

@pixeltrix

This comment has been minimized.

Copy link
Member

pixeltrix commented Sep 29, 2015

@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

This comment has been minimized.

Copy link
Contributor Author

merhard commented Sep 29, 2015

@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

This comment has been minimized.

Copy link

eltiare commented Oct 3, 2015

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

@pixeltrix

This comment has been minimized.

Copy link
Member

pixeltrix commented Oct 5, 2015

@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

This comment has been minimized.

Copy link
Contributor Author

merhard commented Oct 5, 2015

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

@pixeltrix

This comment has been minimized.

Copy link
Member

pixeltrix commented Oct 6, 2015

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

@sgrif

This comment has been minimized.

Copy link
Contributor

sgrif commented Oct 6, 2015

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

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 `ActionDispatch::Routing::RouteSet#relative_url_root`.
2. A prefix (the engine's mounted path).
3. The path of the named route inside the engine.

Since commit 44ff031, this has been broken. Step 2 has been changed to:

2. A prefix (the value of `ActionDispatch::Routing::RouteSet#relative_url_root` + the engine's mounted path).

The value of `ActionDispatch::Routing::RouteSet#relative_url_root` is taken into account in step 1 of the route generation and should be ignored when generating the mounted engine's prefix in step 2.

This commit fixes the regression by having `ActionDispatch::Routing::RouteSet#url_for` check `options[:relative_url_root]` before falling back to `ActionDispatch::Routing::RouteSet#relative_url_root`. The prefix generating code then sets `options[:relative_url_root]` to an empty string. This empty string is used instead of `ActionDispatch::Routing::RouteSet#relative_url_root` and avoids the duplicate `relative_url_root` value in the final result.

This resolves #20920 and resolves #21459
@merhard merhard force-pushed the merhard:mounted_engine_route_fix branch from 45cc179 to bcfbd8b Oct 7, 2015
@merhard

This comment has been minimized.

Copy link
Contributor Author

merhard commented Oct 7, 2015

@pixeltrix CHANGELOG entry added. Commits squashed and rebased.

pixeltrix added a commit that referenced this pull request Oct 7, 2015
Mounted engine route fix
@pixeltrix pixeltrix merged commit fef1064 into rails:master Oct 7, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@pixeltrix

This comment has been minimized.

Copy link
Member

pixeltrix commented Oct 7, 2015

@merhard thanks very much for your patience and perseverance 👍

@merhard

This comment has been minimized.

Copy link
Contributor Author

merhard commented Oct 7, 2015

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

@merhard merhard deleted the merhard:mounted_engine_route_fix branch Oct 7, 2015
@merhard

This comment has been minimized.

Copy link
Contributor Author

merhard commented Oct 7, 2015

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

@merhard merhard restored the merhard:mounted_engine_route_fix branch Oct 7, 2015
@pixeltrix

This comment has been minimized.

Copy link
Member

pixeltrix commented Oct 8, 2015

@merhard backported in 37864d7 - thanks again.

@merhard merhard deleted the merhard:mounted_engine_route_fix branch Oct 8, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.