Fixed STI classes not defining an attribute method if there is a conflicting private method defined on its ancestors. (Fixes #11569) #14052

Merged
merged 1 commit into from Feb 25, 2014

Conversation

Projects
None yet
5 participants
Owner

chancancode commented Feb 13, 2014

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

If both klass and superklass are both Classes, 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.

See #11569 (comment) for the full explanation.

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.

@thedarkone thedarkone and 1 other commented on an outdated diff Feb 14, 2014

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
+ method_defined_within?(method_name, superclass) &&
+ superclass.instance_method(method_name).owner != superclass.generated_attribute_methods ||
@thedarkone

thedarkone Feb 14, 2014

Contributor

I know that the original code also has no parentheses, but would you mind adding them? I always get very confused about precedence rules whenever && and || intertwined :).

@chancancode

chancancode Feb 14, 2014

Owner

Sounds good!

Owner

chancancode commented Feb 14, 2014

@rafaelfranca can you check this also?

When we talked about this last time, we said it should be okay to overwrite methods defined on Kernel, but it's not actually working because of this bug.

Owner

rafaelfranca commented Feb 14, 2014

Seems fine

@jonleighton jonleighton and 1 other commented on an outdated diff Feb 18, 2014

activerecord/test/cases/attribute_methods_test.rb
- Object.send(:undef_method, :title) # remove test method from object
+ def test_read_attribute_overwrites_private_method_not_considered_implemented_when_subclassing
+ # 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
+ end
+ superklass = Class.new(ActiveRecord::Base) { self.abstract_class = true }
+ subklass = Class.new(superklass) { self.table_name = 'topics' }
+ topic = subklass.new
+ assert !subklass.instance_method_already_implemented?(:title)
+ assert_nil topic.title
+ ensure
+ Object.send(:remove_method, :title) # remove test method from object
@jonleighton

jonleighton Feb 18, 2014

Member

Is there a way to write this test without having to modify a global constant (namely Object)?

@chancancode

chancancode Feb 19, 2014

Owner

Heh, this one is tricky :) I added this based on the existing test case (the one above).

I'm not sure if this can be avoided, because the bug in question can only be reproduced if there is a conflicting method that exists somewhere in the inheritance chain above ActiveRecord::Base. So that would pretty much have to be on Object.

@chancancode

chancancode Feb 19, 2014

Owner

I guess one alternative is to fake a model and give it an attribute that conflicts with an existing global method (e.g. system), would that be better?

@jonleighton

jonleighton Feb 23, 2014

Member

I think that would be better. Maybe it doesn't matter all that much, but I've seen several situations in rails tests where a test has modified global scope and it's come back to bite us later on, so my default reaction is to want to avoid it as these things can be hard to track down.

Owner

chancancode commented Feb 23, 2014

@jonleighton I updated the tests to use the system method instead, also renamed them to make the intent more explicit.

Member

jonleighton commented Feb 23, 2014

I'm happy to merge this if the tests pass, but it needs a rebase. Thanks.

@chancancode chancancode 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.
4155431
Owner

chancancode commented Feb 23, 2014

Had to restart the build a few times to get pass some random hiccups, but it finally passed :)

I rebased against master, I can take care of back porting the fix once you are happy with the result.

jonleighton merged commit 4155431 into rails:master Feb 25, 2014

1 check passed

default The Travis CI build passed
Details
Member

jonleighton commented Feb 25, 2014

Thanks. Annoyingly it was stale even right after you had rebased, so I've just manually merged.

Owner

chancancode commented Feb 25, 2014

F'ing changelogs 😢 thanks for reviewing this, I'll backport this to 4-1-stable and 4-0-stable later

Owner

senny commented on 4155431 Feb 26, 2014

❤️

@arthurnn arthurnn added a commit to arthurnn/rails that referenced this pull request Mar 10, 2014

@arthurnn arthurnn Fixes STI when 2+ levels deep.
PR #14052 Added a regression where it was only looking for methods in one
level up, So when the method was defined in a 2+ levels up the
inheritance chain, the method was not found as defined.
0187f6e

@arthurnn arthurnn added a commit to arthurnn/rails that referenced this pull request Mar 10, 2014

@arthurnn arthurnn Fixes STI when 2+ levels deep.
PR #14052 Added a regression where it was only looking for methods in one
level up, So when the method was defined in a 2+ levels up the
inheritance chain, the method was not found as defined.
e5f15a8

@arthurnn arthurnn added a commit to arthurnn/rails that referenced this pull request Mar 10, 2014

@arthurnn arthurnn Fixes STI when 2+ levels deep.
PR #14052 Added a regression where it was only looking for methods in one
level up, So when the method was defined in a 2+ levels up the
inheritance chain, the method was not found as defined.

Conflicts:
	activerecord/test/cases/attribute_methods_test.rb
57d02ad

@arthurnn arthurnn added a commit that referenced this pull request Mar 11, 2014

@arthurnn @chancancode arthurnn + chancancode Fixes STI when 2+ levels deep.
PR #14052 Added a regression where it was only looking for methods in one
level up, So when the method was defined in a 2+ levels up the
inheritance chain, the method was not found as defined.

Conflicts:
	activerecord/test/cases/attribute_methods_test.rb
f48f51d

@arthurnn arthurnn added a commit that referenced this pull request Mar 11, 2014

@arthurnn @chancancode arthurnn + chancancode Fixes STI when 2+ levels deep.
PR #14052 Added a regression where it was only looking for methods in one
level up, So when the method was defined in a 2+ levels up the
inheritance chain, the method was not found as defined.
ed7bffc

@arthurnn arthurnn added a commit that referenced this pull request Mar 11, 2014

@arthurnn @rafaelfranca arthurnn + rafaelfranca Fixes STI when 2+ levels deep.
PR #14052 Added a regression where it was only looking for methods in one
level up, So when the method was defined in a 2+ levels up the
inheritance chain, the method was not found as defined.
0aecb71
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment