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*
This commit is contained in:
parent
9ed66648b5
commit
7e8e91c439
@ -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.
|
||||
|
Loading…
Reference in New Issue
Block a user