I need feedback 😀

README.md

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

step-1.diff

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

step-2.diff

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

💛

@claudiob: these changes are already implemented. For the first point we have the collection option on the resources DSL. Collection mode only gets enabled if collection option is true. Secondly we have implemented the regex that Andrew came up with. I'm no expert in regexs so I can't really claim that it's the most efficient one. And yes the parsed ids are returned as an array of integers. So all in all both these suggestions are already part of the PR.

Also my next PR that is coming out soon rewrites the DSL mapper using resource route objects. Routes would be implemented as objects so that each route knows about its type when parsing the options.

The original PR by @ujjwalt is very big, and in order to get accepted we might want to split it into smaller places that can be discussed and accepted on their own.

The code above is just a minimal part of @ujjwalt's PR, but has already raised concerns about its value: should it really be part of Rails? Should we use gems like this instead?

The difference in opinions is probably the reason why the original PR has been open for 9 months, and my desire is now to move forward: whether by accepting it, or coming to the conclusion that Rails is better off rejecting it.

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