Permalink
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.
  • Loading branch information...
chancancode committed Feb 13, 2014
1 parent 96759cf commit 41554319f8454f667b5e315339d3e286dbf6e1a4
@@ -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*
+
* Deprecate half-baked support for PostgreSQL range values with excluding beginnings.
We currently map PostgreSQL ranges to Ruby ranges. This conversion is not fully
possible because the Ruby range does not support excluded beginnings.
@@ -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
@@ -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
@@ -1,4 +1,5 @@
workstation:
id: 1
+ system: 'Linux'
developer: 1
extendedWarranty: 1
@@ -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

1 comment on commit 4155431

@senny

This comment has been minimized.

Show comment Hide comment
@senny

senny Feb 26, 2014

Member

❤️

Member

senny commented on 4155431 Feb 26, 2014

❤️

Please sign in to comment.