Skip to content

Conversation

ujjwalt
Copy link
Contributor

@ujjwalt ujjwalt commented Jun 14, 2014

As part of Google Summer of Code 2013, I worked on implementing a collection routing api for Rails under the mentorship of @pixeltrix.
Majority of the work is over with enough stability to be considered for merging. I would like to open up the discussion on the api. The documentation still remains but this is on purpose considering the community might vote for changes.

Features

Generate collection routes by including a collection option.

resources :posts, collection: true
# rake routes
#     Prefix Verb   URI Pattern                Controller#Action
#      posts GET    /posts(/:ids)(.:format)    posts#index {:ids=>/(?:[^\.\/\?]|\.\.)+/, :collection=>true}
#            POST   /posts(.:format)           posts#create {:collection=>true}
#            PUT    /posts/:ids(.:format)      posts#replace {:ids=>/(?:[^\.\/\?]|\.\.)+/, :collection=>true}
#            PATCH  /posts/:ids(.:format)      posts#update_many {:ids=>/(?:[^\.\/\?]|\.\.)+/, :collection=>true}
#            DELETE /posts/:ids(.:format)      posts#destroy_many {:ids=>/(?:[^\.\/\?]|\.\.)+/, :collection=>true}
# edit_posts GET    /posts/:ids/edit(.:format) posts#edit_many {:ids=>/(?:[^\.\/\?]|\.\.)+/, :collection=>true}
#   new_post GET    /posts/new(.:format)       posts#new
#  edit_post GET    /posts/:id/edit(.:format)  posts#edit
#       post GET    /posts/:id(.:format)       posts#show
#            PATCH  /posts/:id(.:format)       posts#update
#            PUT    /posts/:id(.:format)       posts#update
#            DELETE /posts/:id(.:format)       posts#destroy

Pass ranges as parameters and filter them using the Strong Parameters API

Ranges can now be passed as url parameters. Following are some examples :-

http://example.com/posts/1..10,17,45...89 => 1 to 10, 17 and 45 to 88
http://example.com/posts/1..10,7 => 1 to 10
http://example.com/posts/1,5,9 => 1, 5 and 9
filtered_params = params.permit ids: 1...768
filtered_params = params.permit ids: 69..420

Generate collection routing enabled controllers using scaffolds

rails generate scaffold Posts --collection

class PostsController < ApplicationController
  before_action :set_post, only: [:show, :edit, :update, :destroy]

  # GET /posts/1..10,17
  def index
    @posts = post_ids ? Post.find(post_ids) : Post.all
  end

  # Post ids
  def post_ids
    @posts_ids ||= params.permit(ids: 1..2**32-1)
  end

  # GET /posts/1
  def show
  end

  # GET /posts/new
  def new
    @post = Post.new
  end

  # GET /posts/1/edit
  def edit
  end

  # POST /posts
  def create
    posts_params ? create_many : create_one
  end

  def create_many
    @posts = Post.create(posts_params)
    redirect_to posts_url(@posts), notice: 'Posts were successfully updated.'
  end

  def create_one
    @post = Post.create(post_params)
    redirect_to post_url(@post), notice: 'Post was successfully updated.'
  end

  # PATCH/PUT /posts/1
  def update
    if @post.update(post_params)
      redirect_to @post, notice: 'Post was successfully updated.'
    else
      render action: 'edit'
    end
  end

  # DELETE /posts/1
  def destroy
    @post.destroy
    redirect_to posts_url, notice: 'Post was successfully destroyed.'
  end

  # Collection routes
  # PATCH /posts/1..10
  def update_many
    Post.find(post_ids).each do |post|
      post.update(params[:posts][:"#{post.id}"])
    end
    redirect_to @post, notice: 'Posts were successfully updated.'
  end

  # PUT /posts/1..10
  def replace
    Post.destroy_all
    create
  end

  # DELETE /users/1..10
  def destroy_many
    @posts = Post.destroy(posts_ids)
  end

  private
    # Use callbacks to share common setup or constraints between actions.
    def set_post
      @post = Post.find(params[:id])
    end

    # Only allow a trusted parameter "white list" through.
    def post_params
      params[:post]
    end

    # Only allow a trusted collection parameter "white list" through.
    def posts_params
      params[:posts]
    end
end

Scaffold generators of dependencies like JBuilder need to be updated to incorporate collection routing.
Future work will include reducing replace and update_many to single line atomic database operations using ActiveRecord.

This is more of a test to understand that wether I'm on the right track!
:replace_all is now :replace and other *_all methods are now *_many
Renamed the :bulk option to :collection
Merged put, patch and delete into a single if
Created a small utility method collection_routing to check if the Resource is a collection too
Added a flag to Resource to mark collection_routing.
Now collection routing path generations matches the regular path generation flow.
If a range is detected then illegal values are removed from the final array of ranges and integers.
Forgot to track it previously.
The API makes sure that the parameter values lie within the given given range.
If you provide range while permitting a parameter then all comma separated values passed as the parameter should lie within that range.
Added some tests for strong parameters. Yet to be tested.
…ith appropriate messages.

Had wrongly used assert instead of assert_equal in 75ae0e
Need to think more about the need for :show_many and what url helpers will it generate since it's just a special case of :index.
Checks wether routing to :index happens without :ids parameter.
@dhh
Copy link
Member

dhh commented Apr 19, 2015

I agree that we don't have a solid convention here that'll just work for most people most of the time. I've done collection routes myself quite a few times and never found that magical line that leads to extraction. 👍 on closing. Thanks for all the work exploring this.

On Apr 7, 2015, at 16:55, Claudio B. notifications@github.com wrote:

Hello again! Let me recap the status of this PR.

Rails does not natively support collection routing
Four years, @dhh came up with a gist suggesting how methods like update_many and destroy_many could be added to Rails to benefit the community
Evolving from that initial idea, a Google Summer of Code 2013 project was run by @ujjwalt (mentored by @pixeltrix) which resulted in an original PR
A year ago, the PR was submitted to rails/rails and it became the PR you are reading now
Why did an idea that is simple to explain (provide methods that affect a collection or RESTful resources) took a long time to implement? Because of its subtleties.

Things that were not evident in the original idea turned into big questions along the way. The following ones are extracted from the discussion notes of this same PR:

Future work will include reducing replace and update_many to single line atomic database operations using ActiveRecord.
We thought about the choice of using "replace" vs "replace_many"
PUT on collection [...] might be rarely used but it's there in case you need to.
any specific integration with ActionView [...] indeed that's the next step. But we didn't want to jump ahead and implement soemthing that is dependent on a functionality that's not yet integrated.
we also need to update rails/jbuilder#157 to cater to building collection responses.
It is clear that is PR is onto something, but it looks like it is too big to be accepted.

Are we able to merge just a small part of this PR into rails/rails? For instance: what if we started by just changing Rails to route a request like GET /posts/1,3,4 to posts#index (with a collection of ids), rather than to posts#show?

That's what I suggested in this gist, and even such a small change raised more questions (see the discussion below the gist). As I wrote there:

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.

to which @ujjwalt replied:

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.

So what should the next step be?

I asked several members of the Rails core team what they think about it, and it became more evident that trying to merge this PR into Rails 5 is beyond the current expectations. There are gems out there that let people deal with collection routing, and the changes suggested by the current PR might take on too many edge-cases to be beneficial.

I think @ujjwalt and @pixeltrix did a great job in trying to assess the scope of Collection Routing in Rails. My personal suggestion is that we close this PR. Being on GitHub, we will always be able to look back and eventually implement portions of it, but I don't think there is real value in leaving it open now.

@dhh @pixeltrix I leave this to you! Thanks


Reply to this email directly or view it on GitHub.

@rafaelfranca
Copy link
Member

Thank you so much for the work on this.

@dhh
Copy link
Member

dhh commented Apr 24, 2015

Actually, I just had to do yet another set of bulk controllers in the past week, and I'm thinking we can get a lot of bang for just a few bucks by focusing on just the routing part, and just focus on the comma-separated list. Here's a proposal that also unifies the naming around "bulk":

resources :posts, bulk: true

class PostsController
  # GET /posts/1,4,5
  def show_bulk
  end

  # GET /posts/1,4,5/edit
  def edit_bulk
  end

  # PATCH/PUT /posts/1,4,5
  def update_bulk
  end

  # DELETE /posts/1,4,5
  def destroy_bulk
  end
end

@ujjwalt
Copy link
Contributor Author

ujjwalt commented Apr 24, 2015

@dhh: I can make those changes and push them in a couple of days. What say?
On Fri, 24 Apr 2015 at 6:44 pm, David Heinemeier Hansson <
notifications@github.com> wrote:

Actually, I just had to do yet another set of bulk controllers in the past
week, and I'm thinking we can get a lot of bang for just a few bucks by
focusing on just the routing part, and just focus on the comma-separated
list. Here's a proposal that also unifies the naming around "bulk":

resources :posts, bulk: true
class PostsController

GET /posts/1,4,5

def show_bulk
end

GET /posts/1,4,5/edit

def edit_bulk
end

PATCH/PUT /posts/1,4,5

def update_bulk
end

DELETE /posts/1,4,5

def destroy_bulk
endend


Reply to this email directly or view it on GitHub
#15719 (comment).

@dhh
Copy link
Member

dhh commented Apr 24, 2015

I'd be happy to review that, Ujjwal. I think part of the problem here was
trying to solve too wide a birth of use cases up front. Let's nip away at
this in bite sizes.

On Fri, Apr 24, 2015 at 3:16 PM, Ujjwal Thaakar notifications@github.com
wrote:

@dhh: I can make those changes and push them in a couple of days. What say?
On Fri, 24 Apr 2015 at 6:44 pm, David Heinemeier Hansson <
notifications@github.com> wrote:

Actually, I just had to do yet another set of bulk controllers in the
past
week, and I'm thinking we can get a lot of bang for just a few bucks by
focusing on just the routing part, and just focus on the comma-separated
list. Here's a proposal that also unifies the naming around "bulk":

resources :posts, bulk: true
class PostsController

GET /posts/1,4,5

def show_bulk
end

GET /posts/1,4,5/edit

def edit_bulk
end

PATCH/PUT /posts/1,4,5

def update_bulk
end

DELETE /posts/1,4,5

def destroy_bulk
endend


Reply to this email directly or view it on GitHub
#15719 (comment).


Reply to this email directly or view it on GitHub
#15719 (comment).

@morgoth
Copy link
Member

morgoth commented Apr 24, 2015

@dhh How would you handle creating many records? Would it be POST /posts/create_many or maybe POST /posts/bulk?

@dhh
Copy link
Member

dhh commented Apr 24, 2015

I was just thinking about that one too. I do think it makes sense to add
it. I like POST /posts/bulk => create_bulk.

On Fri, Apr 24, 2015 at 3:36 PM, Wojciech Wnętrzak <notifications@github.com

wrote:

@dhh https://github.com/dhh How would you handle creating many records?
Would it be POST /posts/create_many or maybe POST /posts/bulk?


Reply to this email directly or view it on GitHub
#15719 (comment).

@dhh
Copy link
Member

dhh commented Apr 24, 2015

In fact, in my own use, I've actually also needed GET /posts/new_bulk =>
new_bulk.

On Fri, Apr 24, 2015 at 3:37 PM, David Heinemeier Hannson <
david@loudthinking.com> wrote:

I was just thinking about that one too. I do think it makes sense to add
it. I like POST /posts/bulk => create_bulk.

On Fri, Apr 24, 2015 at 3:36 PM, Wojciech Wnętrzak <
notifications@github.com> wrote:

@dhh https://github.com/dhh How would you handle creating many
records? Would it be POST /posts/create_many or maybe POST /posts/bulk?


Reply to this email directly or view it on GitHub
#15719 (comment).

@ujjwalt
Copy link
Contributor Author

ujjwalt commented Apr 24, 2015

@dhh I'd like to understand your use case especially why you needed a bulk route for GET

@dhh
Copy link
Member

dhh commented Apr 24, 2015

You mean new_bulk? It's the difference between a place where you create 1 thing at the time vs stuff in bulk. Think 1 todo item vs 10 todo items at the time.

@ujjwalt
Copy link
Contributor Author

ujjwalt commented Apr 24, 2015

No I meant why can't you use the same route for both like I have
implemented here?
On Fri, 24 Apr 2015 at 10:40 pm, David Heinemeier Hansson <
notifications@github.com> wrote:

You mean new_bulk? It's the difference between a place where you create 1
thing at the time vs stuff in bulk. Think 1 todo item vs 10 todo items at
the time.


Reply to this email directly or view it on GitHub
#15719 (comment).

@dhh
Copy link
Member

dhh commented Apr 24, 2015

Because you want to use different setup and different templates depending on whether it's bulk or not. Just like all the others.

@ujjwalt
Copy link
Contributor Author

ujjwalt commented Apr 24, 2015

But we direct them to different actions they'll always tender into
different templates. Only the route remains the same with different regular
expressions for ID
On Fri, 24 Apr 2015 at 10:58 pm, David Heinemeier Hansson <
notifications@github.com> wrote:

Because you want to use different setup and different templates depending
on whether it's bulk or not. Just like all the others.


Reply to this email directly or view it on GitHub
#15719 (comment).

@dhh
Copy link
Member

dhh commented Apr 24, 2015

I don't understand what you mean? I'm saying that /posts/new_bulk would be
a new route.

On Fri, Apr 24, 2015 at 7:48 PM, Ujjwal Thaakar notifications@github.com
wrote:

But we direct them to different actions they'll always tender into
different templates. Only the route remains the same with different regular
expressions for ID
On Fri, 24 Apr 2015 at 10:58 pm, David Heinemeier Hansson <
notifications@github.com> wrote:

Because you want to use different setup and different templates depending
on whether it's bulk or not. Just like all the others.


Reply to this email directly or view it on GitHub
#15719 (comment).


Reply to this email directly or view it on GitHub
#15719 (comment).

@ujjwalt
Copy link
Contributor Author

ujjwalt commented Apr 24, 2015

Oh I misunderstood. My bad.
On Fri, 24 Apr 2015 at 11:17 pm, Ujjwal Thaakar ujjwalthaakar@gmail.com
wrote:

But we direct them to different actions they'll always tender into
different templates. Only the route remains the same with different regular
expressions for ID
On Fri, 24 Apr 2015 at 10:58 pm, David Heinemeier Hansson <
notifications@github.com> wrote:

Because you want to use different setup and different templates depending
on whether it's bulk or not. Just like all the others.


Reply to this email directly or view it on GitHub
#15719 (comment).

@derekprior
Copy link
Contributor

Because you want to use different setup and different templates depending on whether it's bulk or not. Just like all the others.

Then what is it that is the same that makes it better to keep in one controller rather than a controller dedicated to a collection resource? This seems likely to head to a lot of divergent code in a controller:

  • finders or before filters for single resource actions, finders or before filters for bulk actions.
  • Strong parameters for singular resource actions, strong parameters for bulk resource actions.
  • Authorization for a single resource actions, authorization for bulk resource actions.

Additionally, the idea of a show_bulk action seems pretty smelly. We're showing an index. We have an action for that.

@dhh
Copy link
Member

dhh commented Apr 24, 2015

Derek, this is a spectrum. I most often have controllers that have 1 or 2
bulk actions, but not all of them. And in that case, it's a better fit to
keep them with the existing controller. It's similar to the sense that the
single collection deals with both member actions (show/edit/update/destroy)
and collection actions (index/new/create). This is a new bulk state
in-between those two.

You can definitely split out a bulk controller as well too, if you like. I
did that on my current project, but since it had just two actions, it felt
more natural alongside everything else.

Also, show_many is not the same as index in my use cases. Showing a couple
of things has been quite different from showing all the things in my use.

On Fri, Apr 24, 2015 at 7:56 PM, Derek Prior notifications@github.com
wrote:

Because you want to use different setup and different templates depending
on whether it's bulk or not. Just like all the others.

Then what is it that is the same that makes it better to keep in one
controller rather than a controller dedicated to a collection resource?
This seems likely to head to a lot of divergent code in a controller:

  • finders or before filters for single resource actions, finders or
    before filters for bulk actions.
  • Strong parameters for singular resource actions, strong parameters
    for bulk resource actions.
  • Authorization for a single resource actions, authorization for bulk
    resource actions.

Additionally, the idea of a show_bulk action seems pretty smelly. We're
showing an index. We have an action for that.


Reply to this email directly or view it on GitHub
#15719 (comment).

@ujjwalt
Copy link
Contributor Author

ujjwalt commented Apr 25, 2015

@rafaelfranca Please reopen the PR. I'll implement a lean solution and take this one feature at a time with full consensus before me merge.

@ujjwalt
Copy link
Contributor Author

ujjwalt commented Apr 25, 2015

@dhh: I think collection is a better term than bulk. Personally I prefer new_collection_path than new_bulk_path. What is your opinion on this?

Regarding your last point. Don't you think show_many is a subset of index and we could handle it as we have done in this PR i.e. we use an optional ids parameter on the index path that could selectively show items in it's presence and all the items in it's absence.

@dhh
Copy link
Member

dhh commented Apr 25, 2015

I don't like collection because we already use that term to refer "all the things", as in index/new/create and with routing via collection do/end. Bulk is a new term we can define to mean "some of the things".

I don't like reusing index either. The whole point of this feature is to split one from some from all and give each their own actions.

On Apr 25, 2015, at 20:37, Ujjwal Thaakar notifications@github.com wrote:

@dhh: I think collection is a better term than bulk. Personally I prefer new_collection_path than new_bulk_path. What is your opinion on this?

Regarding your last point. Don't you think show_many is a subset of index and we could handle it as we have done in this PR i.e. we use an optional ids parameter on the index path that could selectively show items in it's presence and all the items in it's absence.


Reply to this email directly or view it on GitHub.

@matthewd
Copy link
Member

@ujjwalt I think we'd be better off with a new PR, leaving this one to be referred back to.

@dhh Thoughts on bulk_update over update_bulk?

@dhh
Copy link
Member

dhh commented Apr 25, 2015

I prefer update_bulk such that it can be listed under update and follow a
nice table-of-contents flow.

On Sat, Apr 25, 2015 at 9:47 PM, Matthew Draper notifications@github.com
wrote:

@ujjwalt https://github.com/ujjwalt I think we'd be better off with a
new PR, leaving this one to be referred back to.

@dhh https://github.com/dhh Thoughts on bulk_update over update_bulk?


Reply to this email directly or view it on GitHub
#15719 (comment).

@ujjwalt
Copy link
Contributor Author

ujjwalt commented Apr 29, 2015

@matthewd Hmmm, ok let me see if I can quickly build something and open a new PR. But don't you think we should continue with this PR only since it has all the discussion. Or do you feel it's already too big?

@ujjwalt
Copy link
Contributor Author

ujjwalt commented Jun 6, 2015

@dhh Do we want to route both PUT and PATCH to update_bulk. This was one of the points originally where we wanted two separate actions. PUT completes replaces the collection while PATCH only modifies certain resources.

@dhh
Copy link
Member

dhh commented Jun 7, 2015

I'd route both to the same like we do for regular update for now to stay
consistent.

On Saturday, June 6, 2015, Ujjwal Thaakar notifications@github.com wrote:

@dhh https://github.com/dhh Do we want to route both PUT and PATCH to
update_bulk. This was one of the points originally where we wanted two
separate actions. PUT completes replaces the collection while PATCH only
modifies certain resources.


Reply to this email directly or view it on GitHub
#15719 (comment).

@rafaelfranca rafaelfranca modified the milestones: 5.0.0 [temp], 5.0.0 Dec 30, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.