Skip to content
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

Create a blacklist to disallow mutator methods to be delegated to Array #13314

Merged
merged 1 commit into from Dec 17, 2013

Conversation

@laurocaetano
Copy link
Contributor

laurocaetano commented Dec 14, 2013

This change was necessary because the whitelist wouldn't work.
It would be painful for users trying to update their applications.

Related with: #12129

@jeremy
jeremy reviewed Dec 14, 2013
View changes
activerecord/lib/active_record/relation/delegation.rb Outdated
true
else
false
end

This comment has been minimized.

Copy link
@jeremy

jeremy Dec 14, 2013

Member
def array_delegable?(method)
  Array.method_defined?(method) && BLACKLISTED_ARRAY_METHODS.exclude?(method)
end

This comment has been minimized.

Copy link
@laurocaetano

laurocaetano Dec 14, 2013

Author Contributor

Nice catch 😄

@jeremy
jeremy reviewed Dec 14, 2013
View changes
activerecord/lib/active_record/relation/delegation.rb Outdated
:compact!, :flatten!, :reject!, :reverse!, :rotate!,
:shuffle!, :slice!, :sort!, :sort_by!, :delete_if,
:keep_if, :pop, :shift, :delete_at, :compact
] # :nodoc:

This comment has been minimized.

Copy link
@jeremy

jeremy Dec 14, 2013

Member

Consider making this a Set. Not sure how lookup times would compare, but at 15 elements it's probably quicker than scanning an Array.

@jeremy
jeremy reviewed Dec 14, 2013
View changes
activerecord/lib/active_record/relation/delegation.rb Outdated
@@ -45,7 +43,13 @@ def inherited(child_class)
:map, :none?, :one?, :partition, :reject, :reverse,
:sample, :second, :sort, :sort_by, :third,
:to_ary, :to_set, :to_xml, :to_yaml
]
] # :nodoc:

This comment has been minimized.

Copy link
@jeremy

jeremy Dec 14, 2013

Member

Can remove ARRAY_DELEGATES now.

This comment has been minimized.

Copy link
@rafaelfranca

rafaelfranca Dec 14, 2013

Member

Maybe we should keep this. It will speed up the lookup for these methods that will not need to pass for method_missing

This comment has been minimized.

Copy link
@jeremy

jeremy Dec 14, 2013

Member

If we keep it, we should generate it from Array + Enumerable instance methods - blacklist. Weird to have a hardcoded list.

This comment has been minimized.

Copy link
@rafaelfranca

rafaelfranca Dec 14, 2013

Member

I have to agree. Before @laurocaetano's path we have a small hardcoded list but I think it was of common methods between Array and the class. @laurocaetano lets revert to the original code. If any of these methods became hot spots in the future we can add to this list.

This comment has been minimized.

Copy link
@laurocaetano

laurocaetano Dec 14, 2013

Author Contributor

👍

@jeremy
jeremy reviewed Dec 14, 2013
View changes
activerecord/lib/active_record/relation/delegation.rb Outdated
:compact!, :flatten!, :reject!, :reverse!, :rotate!,
:shuffle!, :slice!, :sort!, :sort_by!, :delete_if,
:keep_if, :pop, :shift, :delete_at, :compact
] # :nodoc:

delegate(*ARRAY_DELEGATES, to: :to_a)

This comment has been minimized.

Copy link
@jeremy

jeremy Dec 14, 2013

Member

Ditto, can remove this now.

@jeremy
jeremy reviewed Dec 14, 2013
View changes
activerecord/lib/active_record/relation/delegation.rb Outdated
@@ -118,15 +122,28 @@ def relation_class_for(klass)
end

def respond_to?(method, include_private = false)
super || @klass.respond_to?(method, include_private) ||
super || array_delegable?(method) ||
super || @klass.respond_to?(method, include_private) ||

This comment has been minimized.

Copy link
@jeremy

jeremy Dec 14, 2013

Member

We have two super calls here now. Remove the second one?

@jeremy
jeremy reviewed Dec 14, 2013
View changes
activerecord/CHANGELOG.md Outdated

To use any other method, instead first call `#to_a` on the association.
To use those methods, instead first call `#to_a` on the association.

This comment has been minimized.

Copy link
@jeremy

jeremy Dec 14, 2013

Member

This changelog entry describes the change, but doesn't explain why it was made.

Why is there a blacklist? Why are this methods blacklisted? Why do we want to disallow certain methods?

The presence of the blacklist is not important for the changelog. The fact that Relation no longer delegates bang methods to to_a is important, because it resolves odd bugs and confusion in code that attempts to call these methods directly on a Relation.

This comment has been minimized.

Copy link
@rafaelfranca

rafaelfranca Dec 14, 2013

Member

To use those methods it is needed to call#to_aon the association.

@laurocaetano
laurocaetano reviewed Dec 14, 2013
View changes
activerecord/CHANGELOG.md Outdated

To use any other method, instead first call `#to_a` on the association.
To use those methods it is needed to call `#to_a` on the association.

This comment has been minimized.

Copy link
@laurocaetano

laurocaetano Dec 14, 2013

Author Contributor

👍

@jeremy
jeremy reviewed Dec 14, 2013
View changes
activerecord/CHANGELOG.md Outdated
@@ -1,15 +1,8 @@
* Create a whitelist of delegable methods to `Array`.
* `Relation` no longer has mutator methods like `#map!` and `#delete_if`. Convert
to an `Array` before using these methods.

This comment has been minimized.

Copy link
@jeremy

jeremy Dec 14, 2013

Member

by calling #to_a.

@jeremy
jeremy reviewed Dec 14, 2013
View changes
activerecord/CHANGELOG.md Outdated

To use any other method, instead first call `#to_a` on the association.
It intents to prevent odd bugs and confusion in code that call mutator
methods directely on the `Relation`.

This comment has been minimized.

Copy link
@jeremy

jeremy Dec 14, 2013

Member

intents -> intends, directely -> directly

@chancancode
chancancode reviewed Dec 14, 2013
View changes
activerecord/lib/active_record/relation/delegation.rb Outdated
:compact!, :flatten!, :reject!, :reverse!, :rotate!,
:shuffle!, :slice!, :sort!, :sort_by!, :delete_if,
:keep_if, :pop, :shift, :delete_at, :compact
].to_set # :nodoc:

This comment has been minimized.

Copy link
@chancancode

chancancode Dec 14, 2013

Member

Wouldn't this needs require "set"? Or is it implicitly required through one of the AS files you required here? (Either case, seems better to just explicitly require it here in case those changes, wdyt?)

This comment has been minimized.

Copy link
@jeremy

jeremy Dec 14, 2013

Member

Yes, needs the require 👍

This comment has been minimized.

Copy link
@laurocaetano

laurocaetano Dec 14, 2013

Author Contributor

Thanks @chancancode 👍

@chancancode
chancancode reviewed Dec 14, 2013
View changes
activerecord/CHANGELOG.md Outdated
@@ -1,15 +1,8 @@
* Create a whitelist of delegable methods to `Array`.
* `Relation` no longer has mutator methods like `#map!` and `#delete_if`. Convert
to an `Array` by calling `#to_a` before using these methods.

This comment has been minimized.

Copy link
@chancancode

chancancode Dec 14, 2013

Member

It might be nice to add a short example here, "instead of this... [code]... you would do this instead... [code]...)", because the average user is probably using Relation without even knowing it :P

Also, since this requires action, it might be worthy of a mention in the upgrade guide.

This comment has been minimized.

Copy link
@jeremy

jeremy Dec 14, 2013

Member

👍 on both counts.

This comment has been minimized.

Copy link
@chancancode

chancancode Dec 14, 2013

Member

By the way, sorry for brining this up now since I missed the original PR, but the original commit included a deprecation message for this change, which I find quite nice, because if I'm upgrading, I'd see the message before I get the error, which give me some hints about what I should do to make things work again.

Maybe it's not exactly a "deprecation warning" in the usual Rails sense, because this doesn't work at all anymore, but some sort of warning or error would be quite nice if I am currently using this today.

This comment has been minimized.

Copy link
@chancancode

chancancode Dec 14, 2013

Member

Actually, looking at the original PR #12129 vs #12590, the behaviour has shifted subtly.

The original PR simply warns about that this will not be supported in the future (array_delegable? still returns true if the method ends with a bang) but doesn't change the behaviour, which is how breaking changes like these are usually handled (deprecated then removed in next version).

In #12590, the behaviour changed to "this would no longer work without warning". Is this shift intentional? I mean if this is rarely used or will cause serious errors, than maybe this is justified, but otherwise, the original behaviour seems more inline with the usual policy. (It should still at least warn though, imo)

Sorry for bombarding you with questions :P

cc @rafaelfranca

This comment has been minimized.

Copy link
@rafaelfranca

rafaelfranca Dec 14, 2013

Member

The original intention was to deprecate these methods but after discussing with @jeremy we decided to remove support. If these methods are used right now it will cause unexpected behavior so they are not working at all. Deprecating these method will not help since they are still broken

This comment has been minimized.

Copy link
@laurocaetano

laurocaetano Dec 14, 2013

Author Contributor

Done the changes :)

This comment has been minimized.

Copy link
@chancancode

chancancode Dec 14, 2013

Member

👍 @rafaelfranca thanks for providing the context

@carlosantoniodasilva
carlosantoniodasilva reviewed Dec 16, 2013
View changes
activerecord/lib/active_record/relation/delegation.rb Outdated

delegate(*ARRAY_DELEGATES, to: :to_a)
delegate :to_xml, :to_yaml, :length, :collect, :map, :each, :all?, :include?, :to_ary, :to => :to_a

This comment has been minimized.

Copy link
@carlosantoniodasilva

carlosantoniodasilva Dec 16, 2013

Member

1.9 style hash.

def method_missing(method, *args, &block)
if @klass.respond_to?(method)
scoping { @klass.public_send(method, *args, &block) }
elsif array_delegable?(method)
to_a.public_send(method, *args, &block)

This comment has been minimized.

Copy link
@carlosantoniodasilva

carlosantoniodasilva Dec 16, 2013

Member

It might be good to keep the same call order when checking respond_to? and method_missing.

This comment has been minimized.

Copy link
@laurocaetano

laurocaetano Dec 16, 2013

Author Contributor

👍 done!

@jeremy
Copy link
Member

jeremy commented Dec 16, 2013

Please rebase ❤️

@laurocaetano
Copy link
Contributor Author

laurocaetano commented Dec 16, 2013

rebased 💚

…ray`.

This change was necessary because the whitelist wouldn't work.
It would be painful for users trying to update their applications.

This blacklist intent to prevent odd bugs and confusion in code that call mutator
methods directely on the `Relation`.
jeremy added a commit that referenced this pull request Dec 17, 2013
Create a blacklist to disallow mutator methods to be delegated to Array

Conflicts:
	guides/source/upgrading_ruby_on_rails.md
@jeremy jeremy merged commit d4ee09c into rails:master Dec 17, 2013
1 check passed
1 check passed
default The Travis CI build passed
Details
@laurocaetano
Copy link
Contributor Author

laurocaetano commented Dec 17, 2013

💚 ❤️ 💙 💛

oliverguenther added a commit to oliverguenther/openproject that referenced this pull request Aug 10, 2015
rails/rails#13314 removed mutator methods
(e.g., `Array#sort!`) from relations.

This commit enforces returned work packages to be ordered by ID instead.
oliverguenther added a commit to oliverguenther/openproject that referenced this pull request Aug 11, 2015
rails/rails#13314 removed mutator methods
(e.g., `Array#sort!`) from relations.

This commit enforces either an ordering on the query itself where
applicable, or uses a non-mutator sort.
myabc added a commit to myabc/openproject that referenced this pull request Aug 19, 2015
rails/rails#13314 removed mutator methods
(e.g., `Array#sort!`) from relations.

This commit enforces either an ordering on the query itself where
applicable, or uses a non-mutator sort.
myabc added a commit to eurucamp/eurucamp-activities that referenced this pull request Mar 11, 2017
rails/rails#13314 removed mutator methods
(e.g., `Array#sort!`) from relations.
myabc added a commit to eurucamp/eurucamp-activities that referenced this pull request Apr 10, 2018
rails/rails#13314 removed mutator methods
(e.g., `Array#sort!`) from relations.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.