Skip to content

Instantly share code, notes, and snippets.

@claudiob
Last active August 29, 2015 14:15
Show Gist options
  • Save claudiob/34f610b240a3fec364ae to your computer and use it in GitHub Desktop.
Save claudiob/34f610b240a3fec364ae to your computer and use it in GitHub Desktop.
I need feedback 😀

This stems from the open PR #15719 on Collection Routing.

That PR is very big and adds many new features to Rails, so I'd like to follow a conservative approach and add one feature at the time, making sure it's fully tested and backwards-compatible.


The first feature is to change the match of a path like GET /posts/1,3,4:

  • currently, Rails matches to posts#show with the params set to id: "1,3,4"
  • the idea is instead to match to posts#index with the params set to ids: ["1", "3", "4"].

Feedback required: do you think this is a valuable feature to add to Rails, independently of the other PR?

To achieve this goal and be backwards-compatible, I propose we add a new :subset boolean option to the resources method.

If you don't specify the option, and simply write resources :posts in config/routes.rb, then the current routing behavior remains, that is:

$ rake routes
    Prefix Verb  URI Pattern               Controller#Action
    posts GET    /posts(.:format)          posts#index
          POST   /posts(.:format)          posts#create
 new_post GET    /posts/new(.:format)      posts#new
 ...

If instead you specify the option, and write resources :posts, subset: true, the routes change to:

$ rake routes
    Prefix Verb  URI Pattern               Controller#Action
    posts GET    /posts(/:ids)(.:format)   posts#index {:ids=>/.?,.?/}
          POST   /posts(.:format)          posts#create
 new_post GET    /posts/new(.:format)      posts#new
 ...

Feedback required: what do you think of this :subset parameter? What about its name?

step-1.diff is the code required for this change.


The next step is to clearly identify the Regex that would make the router switch between index and show. For instance:

  • /posts => index
  • /posts/1 => show (params: {id: "1"})
  • /posts/1,2 => index (params: {ids: ["1", "2"]})
  • /posts/, => ???
  • /posts/1, => ???
  • /posts/,2,, => ??

Feedback required: what do you think the regex should be?

In my opinion, it can simply be /.?,.?/, that is, if you have specified subset: true, then whenever you have a comma in your parameter, it means the parameter is an array of ids, and you want to hit the index action.


The last step is to tell the router then, in case of subset, we want the parameters to be returned as an Array, not as a String.

For instance, GET /posts/1,3,4 should match posts#index with parameters {"ids"=>["1", "3", "4"]}, and not {"ids"=>"1,3,4"}.

In other words, GET /posts/1,3,4 should be equivalent to GET /posts?ids[]=1&ids[]=3&ids[]=4.

To achieve this, the router must somehow know that:

  • 1,3,4 comes from a path that accepts subsets (and it's not simply an ID with commas in it)
  • the , (comma) character is used to separate items in the subset

Feedback required: what is the best way to achieve this?

I wrote some code in step-2.diff which achieves the desired result, but I'm not sure whether storing the Regex in the Journey::Router module and then accessing it from Routing::Mapper is the best way to do this

--- a/actionpack/lib/action_dispatch/routing/mapper.rb
+++ b/actionpack/lib/action_dispatch/routing/mapper.rb
@@ -1036,7 +1036,7 @@ module ActionDispatch
# CANONICAL_ACTIONS holds all actions that does not need a prefix or
# a path appended since they fit properly in their scope level.
VALID_ON_OPTIONS = [:new, :collection, :member]
- RESOURCE_OPTIONS = [:as, :controller, :path, :only, :except, :param, :concerns]
+ RESOURCE_OPTIONS = [:as, :controller, :path, :only, :except, :param, :concerns, :subset]
CANONICAL_ACTIONS = %w(index create new show update destroy)
class Resource #:nodoc:
@@ -1249,6 +1263,17 @@ module ActionDispatch
#
# The resource and all segments will now route to /postings instead of /posts
#
+ # [:subset]
+ # Changes the segment component of the +index+ action to also match
+ # requests that specify a subset of resources to retrieve.
+ #
+ # resources :posts, subset: true
+ #
+ # The above example will still match any GET request made to /posts to
+ # the +index+ action, and will additionally match requests like
+ # <tt>GET /posts/1,4,5</tt>, which will be routed to the +index+
+ # action with the param <tt>:ids</tt> set to <tt>["1", "4", "5"]</tt>.
+ #
# [:only]
# Only generate routes for the given actions.
#
@@ -1341,8 +1366,11 @@ module ActionDispatch
concerns(options[:concerns]) if options[:concerns]
+ collection(options.slice :subset) do
+ get :index if parent_resource.actions.include?(:index)
+ end
+
collection do
- get :index if parent_resource.actions.include?(:index)
post :create if parent_resource.actions.include?(:create)
end
@@ -1368,13 +1396,18 @@ module ActionDispatch
# with GET, and route to the search action of +PhotosController+. It will also
# create the <tt>search_photos_url</tt> and <tt>search_photos_path</tt>
# route helpers.
- def collection
+ #
+ # The method accepts an optional boolean parameter +subset+. When set to
+ # true, Rails will also recognize paths such as <tt>/photos/search/1,3</tt>
+ # with GET, and route to the search action of +PhotosController+ with
+ # the params set to <tt>{"ids"=>["1", "3"]}</tt>.
+ def collection(subset: false)
unless resource_scope?
raise ArgumentError, "can't use collection outside resource(s) scope"
end
with_scope_level(:collection) do
- scope(parent_resource.collection_scope) do
+ scope(parent_resource.collection_scope subset: subset) do
yield
end
end
--- a/actionpack/lib/action_dispatch/journey/router.rb
+++ b/actionpack/lib/action_dispatch/journey/router.rb
@@ -20,6 +20,11 @@ module ActionDispatch
# :nodoc:
VERSION = '2.0.0'
+ # The character that separates items in a subset
+ SUBSET_SEPARATOR = ",".freeze
+
+ SUBSET_REGEX = %r{.*?#{SUBSET_SEPARATOR}.*?}
+
attr_accessor :routes
def initialize(routes)
@@ -114,7 +119,13 @@ module ActionDispatch
match_data = r.path.match(req.path_info)
path_parameters = r.defaults.dup
match_data.names.zip(match_data.captures) { |name,val|
- path_parameters[name.to_sym] = Utils.unescape_uri(val) if val
+ if val
+ if r.requirements[name.to_sym] == SUBSET_REGEX
+ path_parameters[name.to_sym] = val.split(SUBSET_SEPARATOR).map{|v| Utils.unescape_uri(v)}
+ else
+ path_parameters[name.to_sym] = Utils.unescape_uri(val)
+ end
+ end
}
[match_data, path_parameters, r]
}
diff --git a/actionpack/lib/action_dispatch/routing/mapper.rb b/actionpack/lib/action_dispatch/routing/mapper.rb
index f2c9e7b..309ceef 100644
--- a/actionpack/lib/action_dispatch/routing/mapper.rb
+++ b/actionpack/lib/action_dispatch/routing/mapper.rb
@@ -1090,7 +1090,16 @@ module ActionDispatch
{ :controller => controller }
end
- alias :collection_scope :path
+ def collection_scope(subset: false)
+ if subset
+ {}.tap do |options|
+ options[:path] = "#{path}(/:#{subset_param})"
+ options[subset_param] = Journey::Router::SUBSET_REGEX
+ end
+ else
+ path
+ end
+ end
def member_scope
"#{path}/:#{param}"
@@ -1117,6 +1126,11 @@ module ActionDispatch
def shallow?
@shallow
end
+
+ private
+ def subset_param
+ param.to_s.pluralize.to_sym
+ end
end
class SingletonResource < Resource #:nodoc:
@ujjwalt
Copy link

ujjwalt commented Apr 7, 2015

@claudiob: Agreed. I have also been waiting for a long time for this. I developed this as part of GSoC where it was proposed by the core team itself so all this while I've been thinking that this is something we've always wanted. A closure on it would be good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment