This project is archived and is in readonly mode.

#1215 ✓committed
Tom Stuart

Add :only/:except options to map.resources

Reported by Tom Stuart | October 15th, 2008 @ 10:18 AM | in 2.x

Attached is a patch to allow explicit specification of which actions should be routed by map.resources. For example:

map.resources :posts, :actions => [:index, :show], :formatted => :index do |post|
  post.resources :comments, :actions => { :except => [:update, :destroy] }, :formatted => :none
end

This is probably controversial since it's "not DRY", but it's useful for a few reasons:

  1. In large Rails applications, the routes can be the number one consumer of RAM; map.resources can be very wasteful in CMS-style situations (e.g. a blog) where public-facing resources are mostly read-only. Better control of map.resources provides a way of cleaning up unused routes without having to fall back to vanilla named routes in routes.rb.

  2. Per-controller is too coarse-grained a level to control availability of actions when each controller may be exposed as multiple resources, e.g. nested and non-nested. For example, one may wish to allow #show on both nested and non-nested versions of a resource, but only allow #create on the nested collection. Providing this control at the routing level obviates the need for the controller to check the context (with before_filters etc): if an action is routable, it is permitted.

  3. There's a documentation benefit in being able to see explicit routing information in routes.rb and rake routes, rather than having to visually inspect each controller and its views directory to determine whether an action is hooked up. Of course there's a maintenance overhead here in keeping the routes file in sync with the controller functionality, but that's documentation for you.

The patch is not ready to go in yet. Advice please:

  1. NO TESTS YET. I don't know enough about testing routes to know how best to test for the absence of routes, particularly in the presence of the default :controller/:action/:id route that gets installed by RouteSet#reload. Need suggestions.

  2. Naming and syntax of options. I'm not thrilled by the option names (:actions, :formatted) or the syntax for their values (:actions => :all, :actions => :none, :actions => :index, :actions => [:index, :show], :actions => { :only => :index }, :actions => { :except => :destroy }) but I can't think of anything better.

  3. Is the fundamental idea acceptable?

Cheers, -Tom

Comments and changes to this ticket

  • Pratik

    Pratik October 15th, 2008 @ 04:22 PM

    • Assigned user set to “Michael Koziarski”
  • Andrew White

    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.

  • Tom Stuart

    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.

  • Tom Stuart

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

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

  • Michael Koziarski

    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
  • Pratik
  • Norbert Crombach

    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.

  • Tom Stuart

    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.

  • Seth Thomas Rasmussen

    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.

  • Lourens Naudé

    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
  • Zack Chandler

    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 :)

  • Tom Stuart

    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

  • Tom Stuart

    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] }.

  • Lourens Naudé

    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

    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

    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.

  • aaronbatalion

    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

    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?

  • Seth Thomas Rasmussen

    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..?

  • Tom Stuart

    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).

  • Tom Stuart

    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.

  • Repository

    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...

  • aaronbatalion

    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.

  • Tom Stuart

    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.

  • Michael Koziarski

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

    Good point, applied and pushed.

  • Michael Koziarski

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

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

  • Tom Stuart

    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

    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?

  • Lourens Naudé
  • Tom Stuart

    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.

  • Tarmo Tänav

    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"}
    
  • Norbert Crombach

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

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

  • Michael Koziarski

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

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

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

<h2 style="font-size: 14px">Tickets have moved to Github</h2>

The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>

Referenced by

Pages