New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ActiveRecord::QueryMethods#select! may be misleading #14752

Closed
eric-smartlove opened this Issue Apr 14, 2014 · 8 comments

Comments

Projects
None yet
6 participants
@eric-smartlove

eric-smartlove commented Apr 14, 2014

When we call select! on a object which is a ActiveRecord::Relation instance, while we thought is was an Array instance, nothing raises but the given block is ignored.

For instance, if User is an ActiveRecord model, the given block in the code below is ignored:

users = User.all
# Block is ignored
users.select!{|u| false}

I think it's misleading, especially since :

  • #all used to return an Array and it now returns an ActiveRecord::Relation
  • #select doesn't have this problem, so it promotes the idea that you don't have to care when you use select. However you do have to care when you use select! which contradict Principle of Least Surprise.

I think ActiveRecord::Relation#select! should raise ArgumentError when a block is given so, at least, developer knows he has to care.

Note: I don't make a pull request, because I'm still a Rails newbie, but I can make one, if my proposition is supported.

@estsauver

This comment has been minimized.

Show comment
Hide comment
@estsauver

estsauver Apr 15, 2014

Contributor

Eric, looks reasonable to me, I added a pull request with the desired change + adding support for the select! behavior.

Contributor

estsauver commented Apr 15, 2014

Eric, looks reasonable to me, I added a pull request with the desired change + adding support for the select! behavior.

@Intrepidd

This comment has been minimized.

Show comment
Hide comment
@Intrepidd

Intrepidd Apr 15, 2014

Contributor

@estsauver I think your pull request does not reflect the change proposed by @eric-smartlove

Indeed, he proposed to raise an ArgumentError when a block is given, what you do is typically .to_a.select! { ... }

Contributor

Intrepidd commented Apr 15, 2014

@estsauver I think your pull request does not reflect the change proposed by @eric-smartlove

Indeed, he proposed to raise an ArgumentError when a block is given, what you do is typically .to_a.select! { ... }

@Intrepidd

This comment has been minimized.

Show comment
Hide comment
@Intrepidd

Intrepidd Apr 15, 2014

Contributor

Bonus : Why I don't think delegating select! to array is a good idea (#14757) :

users = User.all
users.select! { |u| u.admin? }
render :json => {:users => users}

This will render all users.

Contributor

Intrepidd commented Apr 15, 2014

Bonus : Why I don't think delegating select! to array is a good idea (#14757) :

users = User.all
users.select! { |u| u.admin? }
render :json => {:users => users}

This will render all users.

@estsauver

This comment has been minimized.

Show comment
Hide comment
@estsauver

estsauver Apr 15, 2014

Contributor

@Intrepidd totally reasonable, I didn't think about that case.

I think it's reasonable to just raise an argument error. I'll update PR.

Contributor

estsauver commented Apr 15, 2014

@Intrepidd totally reasonable, I didn't think about that case.

I think it's reasonable to just raise an argument error. I'll update PR.

@laurocaetano

This comment has been minimized.

Show comment
Hide comment
@laurocaetano

laurocaetano Apr 15, 2014

Contributor

The select! method is private API. https://github.com/rails/rails/blob/4-0-stable/activerecord/lib/active_record/relation/query_methods.rb#L223-L226 and c5bdf6c have more information about it.

I think it does make sense to change this behavior, but not sure if raising ArgumentError would be the right thing to do, maybe raising NoMethodError when select! is called, like d4ee09c did for the other mutator methods on relation.

Also, I would change it only on master and maybe 4-1-stable. For 4-0-stable the solution could be #14757.

cc/ @jeremy @rafaelfranca

Contributor

laurocaetano commented Apr 15, 2014

The select! method is private API. https://github.com/rails/rails/blob/4-0-stable/activerecord/lib/active_record/relation/query_methods.rb#L223-L226 and c5bdf6c have more information about it.

I think it does make sense to change this behavior, but not sure if raising ArgumentError would be the right thing to do, maybe raising NoMethodError when select! is called, like d4ee09c did for the other mutator methods on relation.

Also, I would change it only on master and maybe 4-1-stable. For 4-0-stable the solution could be #14757.

cc/ @jeremy @rafaelfranca

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Apr 15, 2014

Member

@laurocaetano select! in a relation is used in a lot of place inside Rails we can't raise NoMethodError. I believe the only way to fix it is raising when block is provided.

Member

rafaelfranca commented Apr 15, 2014

@laurocaetano select! in a relation is used in a lot of place inside Rails we can't raise NoMethodError. I believe the only way to fix it is raising when block is provided.

@rafaelfranca rafaelfranca modified the milestones: 4.0.6, 4.0.5 Apr 15, 2014

@laurocaetano

This comment has been minimized.

Show comment
Hide comment
@laurocaetano

laurocaetano Apr 15, 2014

Contributor

@rafaelfranca I searched where it is being used and I found just two places:

Here and here.

I'm ok with the proposed fix in #14757, but I think we should be consistent with the other mutator methods called on Relation (at least on Rails 4.1). What do you think?

Contributor

laurocaetano commented Apr 15, 2014

@rafaelfranca I searched where it is being used and I found just two places:

Here and here.

I'm ok with the proposed fix in #14757, but I think we should be consistent with the other mutator methods called on Relation (at least on Rails 4.1). What do you think?

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Apr 15, 2014

Member

We are being consistent. select! is a relation method and we can't raise NoMethodError when people call it in their application because we call it in Rails. But we can raise NoMethodError if select! is called with block

Member

rafaelfranca commented Apr 15, 2014

We are being consistent. select! is a relation method and we can't raise NoMethodError when people call it in their application because we call it in Rails. But we can raise NoMethodError if select! is called with block

estsauver added a commit to estsauver/rails that referenced this issue Apr 21, 2014

select! renamed to avoid name collision Array#select!
Fixes rails#14752

Select mimics the block interface of arrays, but does not mock the
block interface for select!. This change moves the api to be a
private method, _select!.

rafaelfranca added a commit that referenced this issue Apr 22, 2014

Merge pull request #14757 from estsauver/14752
Fix behavior of select! to be consistent with select #14752

rafaelfranca added a commit that referenced this issue Apr 22, 2014

Merge pull request #14757 from estsauver/14752
Fix behavior of select! to be consistent with select #14752

rafaelfranca added a commit that referenced this issue Apr 23, 2014

Merge pull request #14757 from estsauver/14752
Fix behavior of select! to be consistent with select #14752

Conflicts:
	activerecord/CHANGELOG.md
	activerecord/lib/active_record/relation/delegation.rb
	activerecord/lib/active_record/relation/query_methods.rb
	activerecord/test/cases/relation/mutation_test.rb

@rafaelfranca rafaelfranca modified the milestones: 4.0.6, 4.0.7 May 21, 2014

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