Permalink
Browse files

`scope` now raises on "dangerous" name conflicts

Similar to dangerous attribute methods, a scope name conflict is
dangerous if it conflicts with an existing class method defined within
`ActiveRecord::Base` but not its ancestors.

See also #13389.

*Godfrey Chan*, *Philippe Creux*
  • Loading branch information...
1 parent 9ed6664 commit 7e8e91c439c1a877f867cd7ba634f7297ccef04b @chancancode chancancode committed Jan 25, 2014
@@ -1,3 +1,13 @@
+* `scope` now raises on "dangerous" name conflicts
+
+ Similar to dangerous attribute methods, a scope name conflict is
+ dangerous if it conflicts with an existing class method defined within
+ `ActiveRecord::Base` but not its ancestors.
+
+ See also #13389.
+
+ *Godfrey Chan*, *Philippe Creux*
+
* Correctly send an user provided statement to a `lock!()` call.
person.lock! 'FOR SHARE NOWAIT'
@@ -110,16 +110,34 @@ def instance_method_already_implemented?(method_name)
end
end
- # A method name is 'dangerous' if it is already defined by Active Record, but
+ # A method name is 'dangerous' if it is already (re)defined by Active Record, but
# not by any ancestors. (So 'puts' is not dangerous but 'save' is.)
def dangerous_attribute_method?(name) # :nodoc:
method_defined_within?(name, Base)
end
- def method_defined_within?(name, klass, sup = klass.superclass) # :nodoc:
+ def method_defined_within?(name, klass, superklass = klass.superclass) # :nodoc:
if klass.method_defined?(name) || klass.private_method_defined?(name)
- if sup.method_defined?(name) || sup.private_method_defined?(name)
- klass.instance_method(name).owner != sup.instance_method(name).owner
+ if superklass.method_defined?(name) || superklass.private_method_defined?(name)
+ klass.instance_method(name).owner != superklass.instance_method(name).owner
+ else
+ true
+ end
+ else
+ false
+ end
+ end
+
+ # A class method is 'dangerous' if it is already (re)defined by Active Record, but
+ # not by any ancestors. (So 'puts' is not dangerous but 'new' is.)
+ def dangerous_class_method?(method_name)
+ class_method_defined_within?(method_name, Base)
+ end
+
+ def class_method_defined_within?(name, klass, superklass = klass.superclass) # :nodoc
+ if klass.respond_to?(name, true)
+ if superklass.respond_to?(name, true)
+ klass.method(name).owner != superklass.method(name).owner
else
true
end
@@ -139,6 +139,12 @@ def scope_attributes? # :nodoc:
# Article.published.featured.latest_article
# Article.featured.titles
def scope(name, body, &block)
+ if dangerous_class_method?(name)
+ raise ArgumentError, "You tried to define a scope named \"#{name}\" " \
+ "on the model \"#{self.name}\", but Active Record already defined " \
+ "a class method with the same name."
+ end
+
extension = Module.new(&block) if block
singleton_class.send(:define_method, name) do |*args|
@@ -266,6 +266,63 @@ def test_should_build_on_top_of_chained_scopes
assert_equal 'lifo', topic.author_name
end
+ def test_reserved_scope_names
+ klass = Class.new(ActiveRecord::Base) do
+ self.table_name = "topics"
+
+ scope :approved, -> { where(approved: true) }
+
+ class << self
+ public
+ def pub; end
+
+ private
+ def pri; end
+
+ protected
+ def pro; end
+ end
+ end
+
+ subklass = Class.new(klass)
+
+ conflicts = [
+ :create, # public class method on AR::Base
+ :relation, # private class method on AR::Base
+ :new, # redefined class method on AR::Base
+ :all, # a default scope
+ ]
+
+ non_conflicts = [
+ :find_by_title, # dynamic finder method
+ :approved, # existing scope
+ :pub, # existing public class method
+ :pri, # existing private class method
+ :pro, # existing protected class method
+ :open, # a ::Kernel method
+ ]
+
+ conflicts.each do |name|
+ assert_raises(ArgumentError, "scope `#{name}` should not be allowed") do
+ klass.class_eval { scope name, ->{ where(approved: true) } }
+ end
+
+ assert_raises(ArgumentError, "scope `#{name}` should not be allowed") do
+ subklass.class_eval { scope name, ->{ where(approved: true) } }
+ end
+ end
+
+ non_conflicts.each do |name|
+ assert_nothing_raised do
+ klass.class_eval { scope name, ->{ where(approved: true) } }
+ end
+
+ assert_nothing_raised do
+ subklass.class_eval { scope name, ->{ where(approved: true) } }
+ end
+ end
+ end
+
# Method delegation for scope names which look like /\A[a-zA-Z_]\w*[!?]?\z/
# has been done by evaluating a string with a plain def statement. For scope
# names which contain spaces this approach doesn't work.

4 comments on commit 7e8e91c

@phallstrom
Contributor

Might be worth noting that dangerous methods also include any that other libraries inject into AR as well. In my case, the ransack gem aliases 'search' to one of it's own methods which caused my 'search' scope to fail.

@chancancode
Member

That's an interesting scenario =/ However, I think the same logic would apply regardless of the source. For instance, if ransack calls that #search method internally, it'd stop working correctly if you have a "search" scope on the same class.

@phallstrom
Contributor

@chancancode Absolutely it would. From looking at Ransack's source though I think they do it mostly as a convenience to their users since they only do it if it's not already defined.

https://github.com/activerecord-hackery/ransack/blob/master/lib/ransack/adapters/active_record/base.rb#L7

I don't think Rails needs to change anything other than perhaps mention "hey if it's breaking and you don't see it in AR, check your gems"

@chancancode
Member

@phallstrom makes sense, if you have suggestions for the wordings of the message, feel free to suggest it here, or better yet open a PR 😄

Please sign in to comment.