Browse files

Fixed STI classes not defining an attribute method if there is a

conflicting private method defined on its ancestors.

The problem is that `method_defined_within?(name, klass, superklass)`
only works correclty when `klass` and `superklass` are both `Class`es.

If both `klass` and `superklass` are both `Class`es, they share the
same inheritance chain, so if a method is defined on `klass` but not
`superklass`, this method must be introduced at some point between
`klass` and `superklass`.

This does not work when `superklass` is a `Module`. A `Module`'s
inheritance chain contains just itself. So if a method is defined on
`klass` but not on `superklass`, the method could still be defined
somewhere upstream, e.g. in `Object`.

This fix works by avoiding calling `method_defined_within?` with a
module while still fufilling the requirement (checking that the
method is defined withing `superclass` but not is not a generated
attribute method).

4d8ee28 is likely an attempted partial fix for this problem. This
unrolls that fix and properly check the `superclass` as intended.

Fixes #11569.

Conflicts:
	activerecord/CHANGELOG.md
  • Loading branch information...
1 parent 42a1c30 commit 7b98746bf0428fe85e34c8a7b2548d4017a27490 @chancancode chancancode committed Feb 13, 2014
View
7 activerecord/CHANGELOG.md
@@ -1,3 +1,10 @@
+* Fixed STI classes not defining an attribute method if there is a
+ conflicting private method defined on its ancestors.
+
+ Fixes #11569.
+
+ *Godfrey Chan*
+
* Default scopes are no longer overriden by chained conditions.
Before this change when you defined a `default_scope` in a model
View
5 activerecord/lib/active_record/attribute_methods.rb
@@ -105,8 +105,9 @@ def instance_method_already_implemented?(method_name)
super
else
# If B < A and A defines its own attribute method, then we don't want to overwrite that.
- defined = method_defined_within?(method_name, superclass, superclass.generated_attribute_methods)
- defined && !ActiveRecord::Base.method_defined?(method_name) || super
+ defined = method_defined_within?(method_name, superclass) &&
+ superclass.instance_method(method_name).owner != superclass.generated_attribute_methods
+ defined || super
end
end
View
30 activerecord/test/cases/attribute_methods_test.rb
@@ -728,19 +728,27 @@ def test_bulk_update_raise_unknown_attribute_errro
assert "unknown attribute: hello", error.message
end
- def test_read_attribute_overwrites_private_method_not_considered_implemented
- # simulate a model with a db column that shares its name an inherited
- # private method (e.g. Object#system)
- #
- Object.class_eval do
- private
- def title; "private!"; end
+ def test_global_methods_are_overwritten
+ klass = Class.new(ActiveRecord::Base) do
+ self.table_name = 'computers'
+ end
+
+ assert !klass.instance_method_already_implemented?(:system)
+ computer = klass.new
+ assert_nil computer.system
+ end
+
+ def test_global_methods_are_overwritte_when_subclassing
+ klass = Class.new(ActiveRecord::Base) { self.abstract_class = true }
+
+ subklass = Class.new(klass) do
+ self.table_name = 'computers'
end
- assert !@target.instance_method_already_implemented?(:title)
- topic = @target.new
- assert_nil topic.title
- Object.send(:undef_method, :title) # remove test method from object
+ assert !klass.instance_method_already_implemented?(:system)
+ assert !subklass.instance_method_already_implemented?(:system)
+ computer = subklass.new
+ assert_nil computer.system
end
def test_instance_method_should_be_defined_on_the_base_class
View
1 activerecord/test/fixtures/computers.yml
@@ -1,4 +1,5 @@
workstation:
id: 1
+ system: 'Linux'
developer: 1
extendedWarranty: 1
View
1 activerecord/test/schema/schema.rb
@@ -198,6 +198,7 @@ def create_table(*args, &block)
end
create_table :computers, force: true do |t|
+ t.string :system
t.integer :developer, null: false
t.integer :extendedWarranty, null: false
end

0 comments on commit 7b98746

Please sign in to comment.