Skip to content
Browse files

Fix mounting engines inside a resources block

When a route is mounted inside a resources block, it's automatically
prefixed, so a following code:

    resources :users do
      mount Blog::Engine => '/blog'
    end

will generate a user_blog path helper.

In order to access engine helpers, we also use "mounted_helpers", a list
of helpers associated with each mounted engine, so a path to blog's post
can be generated using user_blog.post_path(user, post).

The problem I'm fixing here is that mount used a raw :as option, without
taking nestings into account. As a result, blog was added to a route set
as a `user_blog`, but helper was generated for just `blog`.

This commit applies the proper logic for defining a helper for a mounted
engine nested in resources or resource block.

(closes #8533)
  • Loading branch information...
1 parent b5c5121 commit e6c602da9046a653747ce99c9cab7f08f572fa40 @drogus drogus committed Oct 30, 2013
View
4 actionpack/CHANGELOG.md
@@ -1,3 +1,7 @@
+* Fix generating a path for engine inside a resources block (#8533)
+
+ *Piotr Sarnacki*
+
* Add Mime::Type.register "text/vcard", :vcf to the default list of mime types
*DHH*
View
3 actionpack/lib/action_dispatch/routing/mapper.rb
@@ -502,11 +502,12 @@ def mount(app, options = nil)
raise "A rack application must be specified" unless path
options[:as] ||= app_name(app)
+ target_as = name_for_action(options[:as], path)
options[:via] ||= :all
match(path, options.merge(:to => app, :anchor => false, :format => false))
- define_generate_prefix(app, options[:as])
+ define_generate_prefix(app, target_as)
self
end
View
13 actionpack/test/dispatch/mount_test.rb
@@ -5,7 +5,7 @@ class TestRoutingMount < ActionDispatch::IntegrationTest
class FakeEngine
def self.routes
- Object.new
+ @routes ||= ActionDispatch::Routing::RouteSet.new
@jeremy
Ruby on Rails member
jeremy added a note Jul 16, 2014

FakeEngine was added to regression-test 28cf772. It needs to respond to #routes, but not behave like a RouteSet.

By changing this, 1ae9f05 was able to regress 28cf772 without failing tests.

@jeremy
Ruby on Rails member
jeremy added a note Jul 16, 2014
@tenderlove
Ruby on Rails member
tenderlove added a note Jul 16, 2014

Why would the app respond to routes, but not be a route set object? I'm curious about the actual use case. Supporting this case leaves our code cluttered with double respond_to? calls.

If this is actually a case we need to support, then we should come up with a clear definition of a "routed app".

@drogus
Ruby on Rails member
drogus added a note Jul 16, 2014

I totally missed that, sorry. I will work on a fix.

@drogus
Ruby on Rails member
drogus added a note Jul 16, 2014

I just saw @tenderlove's comment, that's a good question. I think it might break if an app is an object responding to routes, but not particularly using rails routing system, but I'm not sure if this is something we want to support.

/cc @josevalim

@drogus
Ruby on Rails member
drogus added a note Jul 16, 2014

@tenderlove define_generate_prefix is called in mount, so it seems it can be any rack application mounted in routes. So this might be a custom class with its' own routes method, but not using rails RouteSet

@jeremy
Ruby on Rails member
jeremy added a note Jul 16, 2014

Yep @drogus. Use case is mounting Rack apps like Resque::Server which respond to #routes but don't return a RouteSet: 1ae9f05#commitcomment-7035725

@drogus
Ruby on Rails member
drogus added a note Jul 16, 2014

drogus/rails@5851a88 should fix it, what do you think?

@tenderlove
Ruby on Rails member
tenderlove added a note Jul 16, 2014

Should be fixed in 4a7b959

@drogus
Ruby on Rails member
drogus added a note Jul 17, 2014

This change alters behaviour:

  • it will break mounting rack apps without giving :as option
  • it will break rack apps which are not a Railtie, but still use RouteSet

While the latter could be probably ignored, I'm not sure why return unless app.is_a?(Class) && app < Rails::Railtie is better than return unless app.respond_to?(:routes) && app.routes.respond_to?(:define_mounted_helper).

@tenderlove
Ruby on Rails member
tenderlove added a note Jul 17, 2014

@drogus can you write a test to demonstrate how it breaks mounting rack apps without the :as option? It seems like we have a test case to cover that.

While the latter could be probably ignored,

I think we can safely ignore it. RouteSet is internal to Rails. Not only is the class ":nodoc:"'d, it's also ridiculously hard to use.

I'm not sure why return unless app.is_a?(Class) && app < Rails::Railtie is better than return unless app.respond_to?(:routes) && app.routes.respond_to?(:define_mounted_helper).

It means we can generalize what it means to be an application with named routes. This lets us eliminate redundant tests. See 9b15828

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
end
def self.call(env)
@@ -27,12 +27,23 @@ def self.call(env)
scope "/its_a" do
mount SprocketsApp, :at => "/sprocket"
end
+
+ resources :users do
+ mount FakeEngine, :at => "/fakeengine", :as => :fake_mounted_at_resource
+ end
end
def app
Router
end
+ def test_app_name_is_properly_generated_when_engine_is_mounted_in_resources
+ assert Router.mounted_helpers.method_defined?(:user_fake_mounted_at_resource),
+ "A mounted helper should be defined with a parent's prefix"
+ assert Router.named_routes.routes[:user_fake_mounted_at_resource],
+ "A named route should be defined with a parent's prefix"
+ end
+
def test_trailing_slash_is_not_removed_from_path_info
get "/sprockets/omg/"
assert_equal "/sprockets -- /omg/", response.body

0 comments on commit e6c602d

Please sign in to comment.
Something went wrong with that request. Please try again.