Rails Ruby on Rails → 2.x

Comments and changes to this ticket

• 

  Pratik October 15th, 2008 @ 04:22 PM

  • Assigned user set to “Michael Koziarski”
• 

  Andrew White October 21st, 2008 @ 07:43 AM

  Having tried to get this functionality added before (http://dev.rubyonrails.org/ticke..., I'm definitely +1 on this idea. There was a discussion on the core mailing list as well so it might be worth your while to see why people objected to the idea before.

• You flagged this item as spam.
  

  Tom Stuart October 21st, 2008 @ 09:32 AM

  Thanks for the heads-up, Andrew. For the record, DHH said:

  Rails already deals with resource routes that lead to nowhere. We give you a 406 Not Supported back. Which is a lot better than the 404 Not Found that you'd get if this patch went into effect.

  I'm -1 on this patch. (Thanks for taking the time and a shot at improving Rails, though. Please do keep that up!)

  He's right that Rails already handles routes that lead to no action at all, although I'm surprised to hear that a 406 is considered "a lot better" than a 404 in this situation, since 406 is for when "the request is only capable of generating response entities which have content characteristics not acceptable according to the accept headers sent in the request", which isn't the case here at all -- 403-405 are all more appropriate.

  HTTP semantics aside, the three pragmatic benefits I listed in the ticket (RAM consumption, granularity, documentation) still stand, and it would be appropriate for Rails to "get real" in this case. In particular, the amount of RAM used by all the unnecessary routes in a project I'm working on is really dealbreakingly prohibitive -- we've converted routes.rb to be written out "longhand" with named routes instead of map.resources, but that's much harder to maintain.

  I'd like this functionality to be reconsidered. I'll see if I can merge the tests from the patch on Trac and then ask the core list to review.

• You flagged this item as spam.
  

  Tom Stuart October 22nd, 2008 @ 08:46 AM

  Here's an updated patch, with tests adapted from those on http://dev.rubyonrails.org/ticke...>

  • allowed_actions.patch 25.1 KB
• 

  Michael Koziarski October 22nd, 2008 @ 11:17 AM

  This has missed the 2.2 boat sorry, we're due to cut a release any day now.

  However can you show the memory stats and methodology that you're referring to with the routing changes here, it definitely seems interesting.

  After we've branched for 2.2 I'd be happy to take something like this into edge, your tests are great :)

• 

  Pratik November 9th, 2008 @ 08:23 PM

  This looks relevant http://blog.hungrymachine.com/20...

• 

  Pratik November 9th, 2008 @ 08:23 PM

  And http://github.com/methodmissing/...

• You flagged this item as spam.
  

  Norbert Crombach November 10th, 2008 @ 02:02 PM

  I remember supporting Andrew's Trac ticket for this. The syntax in the first post looks a bit iffy but this is definitely a feature I want.

• You flagged this item as spam.
  

  Tom Stuart November 10th, 2008 @ 02:10 PM

  Any syntax suggestions? The difficulty is in allowing the sets of both unformatted and formatted routes to be customised, and in making it easy to only include or only exclude a couple of actions from each.

  The implementation assumes that you won't want a formatted route to an action that doesn't also have an unformatted route, which I think is sensible for almost all purposes. Don't know if there's a way to constrain the syntax to communicate that.

• You flagged this item as spam.
  

  Seth Thomas Rasmussen November 10th, 2008 @ 03:42 PM

  Might be nice to carry over the same terminology for ignoring/including actions from controller filters.

  map.resources :things, :only => [:new, :create]

  While this seems like a nice addition to routes, what do you guys think of having it be automatic based on action definitions? http://gist.github.com/23253 I'd prefer that if it were reasonable to achieve.

  Also, I'm curious if a simpler routing implementation might be helpful. I haven't digged into how it's done now so I don't know if that would work, but just wondering if things could be done with fewer special objects or just simpler objects somehow.

• You flagged this item as spam.
  

  Lourens Naudé November 10th, 2008 @ 06:36 PM

  Guys,

  Here's another shot at this :

  http://github.com/methodmissing/...

  Refactoring of my initial plugin @ http://github.com/methodmissing/... with better test coverage and tighter framework integration.

  Commits ( http://github.com/methodmissing/... ) >= 'Initialize routing after application classes loaded', 11/09/08.

  Notes :

  • Introduces AC::Base.formatted_routes = true | false

  • AC::Base.formatted_routes true by default for backwards compat

  • Supports a :format => true | false syntax for resource definition, with the sweet spot being AC::Base.formatted_routes turned off, and selectively enabled with the :format => true option

  • Adjusts the Rails init order to setup routing AFTER preloading application classes ( default for production )

  • This allows Routing to infer exactly which actions is defined, and only map routes && named routes for those.

  • config.cache_classes = true required to enable route pruning

  • There's off course also one more over the top optimization that can also yield smaller memory footprints ... the path && url named route variants, but that's pushing it too far.

  All existing and new tests passing.I'll take it for a spin through a large app later today.

  Also not clear on how to handle 404 and 406 responses, but it seems to be less important now that ActionController::RoutingError can be rescued by the app.

  Thoughts ?

  • Lourens

• You flagged this item as spam.
  

  Zack Chandler November 10th, 2008 @ 07:27 PM

  +1

  I had similar results (heavy route memory usage) from running bleak as per

  http://blog.hungrymachine.com/20...

  A way to cut out these formatted routes like:

  resources :users, :include_formatted_routes => false

  would be very nice :)

• You flagged this item as spam.
  

  Tom Stuart November 10th, 2008 @ 07:43 PM

  Hi Lourens,

  The plugin is good work, but not fine-grained enough for some common cases. In particular:

  1. The choice between "no formatted routes" and "all formatted routes" per resource doesn't support the common case of only wanting formatted routes for one or two actions; it seems likely that most blog implementations fall into this category (i.e. only formatted_posts_path is required).

  2. As per point 2 in the original ticket, deciding the set of available routes per-controller is too coarse-grained in the case where a single controller might be responsible for handling requests for multiple resources (e.g. in both nested and non-nested contexts) that naturally require different routes to be available (e.g. allow creation only when nested).

  In addition to those granularity requirements, the controller introspection just strikes me as being too much magic, particularly since it requires changing the load order. It's also important that routes don't get hooked up by mistake, which IMHO justifies the "belt and braces" approach of explicitly specifying the routed actions in routes.rb, even if that does make things less DRY.

  And I agree there'll never be any benefit in quibbling over _path/_url methods! :-)

  Cheers, -Tom

• You flagged this item as spam.
  

  Tom Stuart November 10th, 2008 @ 07:46 PM

  Seth: that's almost how it works already, it's just that you need separate :only/:except options for both unformatted and formatted routes, so at the moment you'd need to say :actions => { :only => [:new, :create] } to distinguish it from :formatted => { :only => [:new, :create] }.

• You flagged this item as spam.
  

  Lourens Naudé November 10th, 2008 @ 08:09 PM

  Tom,

  Great points.Thanks for the feedback.

  We have a x5 namespace project with a huge number of routes and my assumptions might have been biased towards not manually pruning and defining all of those, as well as the maintenance thereof.

• 

  Michael Koziarski November 10th, 2008 @ 08:10 PM

  @Tom: I agree on the granularity thing, it makes much more sense to allow some of the routes (posts) to be available formatted but others (users) not.

  Beyond this issue though, has any work been done to see where the memory usage is within those segments?

  Also, can you reveal a bit more information about the 100MB savings, what the routes looked like, how nested they were, etc etc.?

• 

  Michael Koziarski November 10th, 2008 @ 10:50 PM

  I definitely prefer the idea of :only and :except as we use those in other places. Using the definition of actions seems risky / tightly coupled. The lack of a method doesn't imply that there's no action with that name, as the presence of a template is sufficient.

  A simpler routing implementation would certainly help some of this, but it's a significant undertaking. If we can find enough motivated hackers perhaps we can get it done in 2.3. I think the first step is to draw slightly thicker borders between routes and the rest of rails, and allow people to experiment in plugins.

• You flagged this item as spam.
  

  aaronbatalion November 11th, 2008 @ 02:00 AM

  After writing the original "100M" post, I believe I have a better solution to essentially make ".:format" optional, creating 50% of the routes than before.

  See: http://blog.hungrymachine.com/20...

  gist: http://gist.github.com/23712

  This is not an original idea AFAIK. The Merb router has optional format's.

  That said, I think the prune solutions are also awesome, and could be a secondary pass. I would point out, that a prune solution needs to account for "def foo" and the existence of a "foo.html.erb" file, which isn't accounted for in FooController#actions.

• 

  Michael Koziarski November 11th, 2008 @ 08:12 AM

  I like the idea of making formats optional, perhaps we can investigate that for 2.3 instead of adding an option like this?

• You flagged this item as spam.
  

  Seth Thomas Rasmussen November 11th, 2008 @ 03:29 PM

  Good point about implied actions via the presence of templates being a problem with the idea of making things automatic, Michael.

  Tom, I see what you mean about the options interface now. I still don't like it, though. It got me to thinking..

  What is the real value of the formatted named routes? Can you not just pass a :format param to any named route and get all the same behavior? What about just ditching formatted named routes altogether..?

• You flagged this item as spam.
  

  Tom Stuart November 11th, 2008 @ 03:57 PM

  I agree that making :format optional is a great idea, not least because it removes the confusion around how to specify options to map.resources in this patch (i.e. you could say :only => [:new, :create]) as desired, without worrying about how to turn formatted routes on/off (there won't be any).

• You flagged this item as spam.
  

  Tom Stuart November 12th, 2008 @ 11:16 AM

  • Title changed from “Add :actions and :formatted options to map.resources” to “Add :only/:except options to map.resources”

  Attached is a cut-down version of the original patch which provides simple :only and :except options affecting both unformatted and formatted routes, rather than trying to handle the formatted routes separately. This would still be an improvement in terms of RAM and performance, the syntax is much easier, and it's worth treating the behaviour of map.resources as orthogonal to the overall issue of formatted routes.

  I've opened a separate ticket, #1359, to discuss dispensing with the formatted routes altogether and replacing them with an optional :format argument.

  • 0001-Add-only-except-options-to-map.resources.patch 20.8 KB
• 

  Repository November 12th, 2008 @ 12:03 PM

  • State changed from “new” to “committed”

  (from [44a3009ff068bf080de6764a8c884fbf0ceb920e]) Add :only/:except options to map.resources

  This allows people with huge numbers of resource routes to cut down on the memory consumption caused by the generated code.

  Signed-off-by: Michael Koziarski michael@koziarski.com [#1215 state:committed] http://github.com/rails/rails/co...

• You flagged this item as spam.
  

  aaronbatalion November 13th, 2008 @ 02:09 AM

  For those following this thread, I've added a patch for the optional :format token here: http://rails.lighthouseapp.com/p...

  Please take a look.

• You flagged this item as spam.
  

  Tom Stuart November 13th, 2008 @ 02:47 PM

  There's a small problem with this functionality, pointed out by Lourens: generating the index and show routes for a PostsController creates the posts and post named routes respectively, so that the posts_path and post_path helpers are made available to the application. The issue is that you might still need these helpers even if you don't want the index or show route: create also resides at posts_path, and update and destroy also reside at post_path.

  Attached is a simple additional patch to fix this up; the idea is to ask for the appropriate named route when creating every route, not just index/show/new/edit, but only create the named route if it doesn't already exist. (This prevents the named route being reconnected by later actions, so that e.g. the post named route ends up being associated with destroy.)

  The attempt to detect whether such a named route has already been defined is kind of clunky (@set.named_routes[route_name.to_sym].nil?) but it works fine. Extra tests included.

  • 0001-Fix-map.resources-to-always-generate-named-routes-if.patch 6 KB
• 

  Michael Koziarski November 13th, 2008 @ 04:50 PM

  Good point, applied and pushed.

• 

  Michael Koziarski November 13th, 2008 @ 04:56 PM

  @Lourens: You should take a look and get involved in aaronbatalion's patch at #1359

• You flagged this item as spam.
  

  Tom Stuart November 13th, 2008 @ 05:28 PM

  Before this ships, we should try to fix the unintuitive behaviour of inherited :only/:except as pointed out by Ryan Daigle on his blog. Currently the two options are inherited separately - when you supply one of them it doesn't knock out any inherited version of the other - but since :only takes priority you get unexpected behaviour when you inherit :only but supply :except (:except just gets ignored).

  It's probably be better to have no inheritance at all than to have it work like that. It shouldn't be hard to fix, though. Just not from this iPhone.

• 

  Michael Koziarski November 13th, 2008 @ 05:33 PM

  Sure, take a crack at it and let us know. perhaps raise an error if both are present?

• You flagged this item as spam.
  

  Lourens Naudé November 13th, 2008 @ 05:44 PM

  @Michael - 1359 updated

• You flagged this item as spam.
  

  Tom Stuart November 13th, 2008 @ 08:06 PM

  Okay, this patch should fix it. The new implementation uses :only and :except as two different ways to set the actual inherited option, :actions, which is kept private (much like :namespace). Since :only and :except aren't actually inherited any more, they can't interact in this weird way. Even more tests included.

  • 0001-Make-inheritance-of-map.resources-only-except-opti.patch 4.5 KB
• 

  Tarmo Tänav January 27th, 2009 @ 05:33 PM

  Is there an actual use case for inheriting :only/:except ?

  To me it seems to create more problems than it could ever solve. For example I can't limit the actions for a toplevel resource without setting explicit limits on all the child resources (trying to do the former is how I found out about the inheritance feature).

  There also seems to be a bug with normal named routes under a resource with limiting actions, the named routes all get ":actions => [...]" requirement implicitly added to them which basically breaks all of them unless they explicitly set ":actions => nil".

  
    map.resources :posts, :except => [ :destroy ] do |posts|
      posts.random_named_route '/random'
    end
  
    # rake routes
    # post_random_named_route  /posts/:post_id/random   {:actions=>[:index, :create, :new, :edit, :show, :update], :controller=>"something", :action=>"random"}
  

• You flagged this item as spam.
  

  Norbert Crombach January 27th, 2009 @ 09:59 PM

  I agree with Tarmo, inheriting doesn't really make sense.

• 

  Michael Koziarski January 29th, 2009 @ 04:26 AM

  Sounds reasonable guys. Open a new ticket with a patch and assign it straight to me.