Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[4.2.0-rc1] reset_column_information definitely drop attribute methods #17900

Closed
byroot opened this issue Dec 3, 2014 · 9 comments
Closed

[4.2.0-rc1] reset_column_information definitely drop attribute methods #17900

byroot opened this issue Dec 3, 2014 · 9 comments
Assignees
@sgrif
Labels
activerecord regression
Milestone
4.2.0

Comments

@byroot
Copy link
Member

byroot commented Dec 3, 2014

Sample app to reproduce: https://github.com/byroot/reset_column_information-bug

The test case:

  test "reset_column_informatin do not break attribute methods" do
    foo = Foo.create!
    refute foo.id_changed?
    Foo.reset_column_information
    refute foo.id_changed?
  end

Under 4.2:

FooTest#test_reset_column_informatin_do_not_break_attribute_methods:
NoMethodError: undefined method `id_changed?' for #<Foo:0x007fd3fbb2aee0>
    test/models/foo_test.rb:8:in `block in <class:FooTest>'

Works fine under 4.1. I'll try to figure out where it come from.

cc @rafaelfranca @pixeltrix
@byroot byroot mentioned this issue Dec 3, 2014
Merged
2 tasks
@senny
Copy link
Member

senny commented Dec 4, 2014

/cc @sgrif

@senny senny added this to the 4.2.0 milestone Dec 4, 2014
@sgrif
Copy link
Contributor

sgrif commented Dec 4, 2014

I'll look into it

@sgrif sgrif self-assigned this Dec 4, 2014
@sgrif
Copy link
Contributor

sgrif commented Dec 4, 2014

This was introduced by 84cf156. I think there's a better solution that doesn't involve method missing, however.

@sgrif
Copy link
Contributor

sgrif commented Dec 4, 2014

I'm also unsure if this is a case that we really need to worry about? reset_column_information is not a method that should be called under normal circumstances.

@byroot
Copy link
Member Author

byroot commented Dec 4, 2014

It is after each migrations. So if you have a migration that used attr_changed? or something it will break.

@byroot
Copy link
Member Author

byroot commented Dec 4, 2014

Also I have that test in my gem because we had the issue in production.

@byroot
Copy link
Member Author

byroot commented Dec 4, 2014

I found a very simple fix that do not involve method missing. PR on his way.

byroot added a commit to byroot/rails that referenced this issue Dec 4, 2014
@byroot
Redefine attribute methods after the column information have been reset
3bf42c6 
Fixes rails#17900
@byroot byroot mentioned this issue Dec 4, 2014
Closed
sgrif added a commit that referenced this issue Dec 4, 2014
@sgrif
Correctly handle multiple attribute method prefix/suffixes which match
20f60f2 
Active Record defines `attribute_method_suffix :?`. That suffix will
match any predicate method when the lookup occurs in Active Model. This
will make it incorrectly decide that `id_changed?` should not exist,
because it attempts to determine if the attribute `id_changed` is
present, rather than `id` with the `_changed?` suffix. Instead, we will
look for any correct match.

Fixes #17900
@sgrif
Copy link
Contributor

sgrif commented Dec 4, 2014

Fixed in d1f003e and backported in 20f60f2

@sgrif sgrif closed this as completed Dec 4, 2014
@byroot
Copy link
Member Author

byroot commented Dec 4, 2014

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@sgrif sgrif

Labels
activerecord regression
Projects
None yet
Milestone
4.2.0
Development

Successfully merging a pull request may close this issue.

Redefine attribute methods after the column information have been reset byroot/rails
3 participants
@senny @byroot @sgrif