Rails 4 - Polymorphic belongs_to with touch #10197

Closed
madmax opened this Issue Apr 12, 2013 · 38 comments
@madmax

When we define polymorphic association with touch: true rails will raise exception at startup

class Post
  belongs_to :resource, :polymorphic => true, :counter_cache => true, touch: true
end

/Users/madmax/.rbenv/versions/2.0.0-p0/lib/ruby/gems/2 .0.0/bundler/gems/rails-797fcdf738a2/activerecord/lib/active_record/inheritance.rb:125:in compute_type': uninitialized constant Post::Resource (NameError)
from /Users/madmax/.rbenv/versions/2.0.0-p0/lib/ruby/gems/2.0.0/bundler/gems/rails-797fcdf738a2/activerecord/lib/active_record/reflection.rb:173:in
klass'
from /Users/madmax/.rbenv/versions/2.0.0-p0/lib/ruby/gems/2.0.0/bundler/gems/rails-797fcdf738a2/activerecord/lib/active_record/associations/builder/belongs_to.rb:73:in `add_touch_callbacks'

The problem exists in add_touch_callbacks. It want use Resource as class (reflection)

It might be useful to take look at 034f254, and dce398d

@senny
Ruby on Rails member

Can you describe the situation with an executable test-case in a Gist? This Gist is a good foundation: https://gist.github.com/neerajdotname/5187092

@agis-

@senny It's easy to reproduce, just $ rails g scaffold Post resource:references && rails g scaffold Resource && rake db:migrate and then put the following code that @madmax gave into the Post model. Then navigate to /posts.

@senny
Ruby on Rails member

@Agis- It seems like simple steps but it sums up. Having an executable Gist for Active Record bug reports helps us spending less time on setup and more time on debugging and fixing bugs. The section on Active Record bug reports in our contribution guidelines references these templates.

@rubys

I can't reproduce using either master or v4.0.0.beta1 using the provided instructions:

http://intertwingly.net/tmp/issue_10197.html

@chadmoone

I can replicate this in master (4.0.0.beta1 seems to be fine), but the instructions need to be tweaked slightly:

$ rails g scaffold Post resource:references{polymorphic} && rails g scaffold Thing && rake db:migrate

and set up app/models/post.rb as such:

class Post < ActiveRecord::Base
  belongs_to :resource, :polymorphic => true, :counter_cache => true, touch: true
end

Then, just try to use the Post class (console, server, etc):

> Post.all
#=> NameError: uninitialized constant Post::Resource
    from /Users/chadmoone/.rvm/gems/ruby-2.0.0-p0/bundler/gems/rails-a489bfe48bc0/activerecord/lib/active_record/inheritance.rb:125:in `compute_type'
    from /Users/chadmoone/.rvm/gems/ruby-2.0.0-p0/bundler/gems/rails-a489bfe48bc0/activerecord/lib/active_record/reflection.rb:178:in `klass'
    from /Users/chadmoone/.rvm/gems/ruby-2.0.0-p0/bundler/gems/rails-a489bfe48bc0/activerecord/lib/active_record/associations/builder/belongs_to.rb:73:in `add_touch_callbacks'
    from /Users/chadmoone/.rvm/gems/ruby-2.0.0-p0/bundler/gems/rails-a489bfe48bc0/activerecord/lib/active_record/associations/builder/belongs_to.rb:18:in `build'
    from /Users/chadmoone/.rvm/gems/ruby-2.0.0-p0/bundler/gems/rails-a489bfe48bc0/activerecord/lib/active_record/associations/builder/association.rb:12:in `build'
    from /Users/chadmoone/.rvm/gems/ruby-2.0.0-p0/bundler/gems/rails-a489bfe48bc0/activerecord/lib/active_record/associations.rb:1401:in `belongs_to'
    from /Users/chadmoone/development/poly-touch/app/models/post.rb:2:in `<class:Post>'
    from /Users/chadmoone/development/poly-touch/app/models/post.rb:1:in `<top (required)>'
    from /Users/chadmoone/.rvm/gems/ruby-2.0.0-p0/bundler/gems/rails-a489bfe48bc0/activesupport/lib/active_support/dependencies.rb:423:in `load'
    from /Users/chadmoone/.rvm/gems/ruby-2.0.0-p0/bundler/gems/rails-a489bfe48bc0/activesupport/lib/active_support/dependencies.rb:423:in `block in load_file'
    from /Users/chadmoone/.rvm/gems/ruby-2.0.0-p0/bundler/gems/rails-a489bfe48bc0/activesupport/lib/active_support/dependencies.rb:615:in `new_constants_in'
    from /Users/chadmoone/.rvm/gems/ruby-2.0.0-p0/bundler/gems/rails-a489bfe48bc0/activesupport/lib/active_support/dependencies.rb:422:in `load_file'
    from /Users/chadmoone/.rvm/gems/ruby-2.0.0-p0/bundler/gems/rails-a489bfe48bc0/activesupport/lib/active_support/dependencies.rb:323:in `require_or_load'
    from /Users/chadmoone/.rvm/gems/ruby-2.0.0-p0/bundler/gems/rails-a489bfe48bc0/activesupport/lib/active_support/dependencies.rb:462:in `load_missing_constant'
    from /Users/chadmoone/.rvm/gems/ruby-2.0.0-p0/bundler/gems/rails-a489bfe48bc0/activesupport/lib/active_support/dependencies.rb:183:in `const_missing'
    from (irb):1
    from /Users/chadmoone/.rvm/gems/ruby-2.0.0-p0/bundler/gems/rails-a489bfe48bc0/railties/lib/rails/commands/console.rb:90:in `start'
    from /Users/chadmoone/.rvm/gems/ruby-2.0.0-p0/bundler/gems/rails-a489bfe48bc0/railties/lib/rails/commands/console.rb:9:in `start'
    from /Users/chadmoone/.rvm/gems/ruby-2.0.0-p0/bundler/gems/rails-a489bfe48bc0/railties/lib/rails/commands.rb:66:in `<top (required)>'
    from bin/rails:4:in `require'

I can try to get a test set up demonstrating this, but the above should show it for now. It does seem to stem from the commits that @madmax mentions above, which are post-beta1.

@rubys

I can reproduce with those instructions.

@chadmoone

Ok, a gist was actually pretty easy: https://gist.github.com/chadmoone/5423727

@rubys

Neeraj Singh and Carlos Antonio narrowed this down to 8fccbc1#L1R53

@madmax

@chadmoone Thanks for gist I didn't have time to prepare it yet :)

@adamgamble

I'm the one that wrote the code to fix a bug in the touch code. I'll try to look at this tonight and see if I can come up with a fix. Sorry about that.

@carlosantoniodasilva
Ruby on Rails member

That'd be great, thanks @adamgamble .

@dhh
Ruby on Rails member

@adamgamble, had a chance to make any progress on this?

@adamgamble

No i'm sorry I had prior engagements that took longer than I expected. I'll try my best tonight.

@dhh
Ruby on Rails member

Thanks. This is the last known blocker for the release candidate.

@adamgamble

ok i'll keep you updated.

@adamgamble adamgamble added a commit to adamgamble/rails that referenced this issue Apr 23, 2013
@adamgamble adamgamble added test cases for #10197 3308110
@adamgamble

I think i'm close, but my tests aren't passing and i'm not sure why. Any ideas? Also I'd love to hear refactor ideas because it gets a little hairy with all the interpolation.

adamgamble@2ca84b5

@neerajdotname
Ruby on Rails member

@adamgamble the issue is that when the code accesses reflection.klass then the polymorphic value is being treated as the real class which is not true.

I do not see an easy way out here.

To get the release out I would say drop the support for touching the old record. That can be fixed in a point release or in 4.1.

@carlosantoniodasilva
Ruby on Rails member

You should be able to "print out" in the eval'ed code reflection.class_name instead, and that should end up in the code itself referencing the class (the method body) rather than the class being evaluated at eval'ing time (method definition). Can you please try that?

@adamgamble
@adamgamble

@neerajdotname Thats why I was looking for the polymorphic => true and setting reflection_klass to either #{name}_type or what it was in the previous record. Definitely adding support for polymorphic relationships complicates things a lot.

Everything works except for polymorphic relationships that fit the original test case (where the relationship is swapped to a new instance).

In other words, I fixed the crash @madmax reported, but there is still a bug when you change the relationship to a new instance, the previous instance doesn't get touched.

@carlosantoniodasilva
Ruby on Rails member

Not actually, I was talking about printing the output of it, which would be the class name. So you'd end up with this:

reflection_class = #{reflection.class_name}
#
reflection_class = Foo

Which, if I got it right, is basically the same as interpolating reflection.class, printing the class name in the method body, but reflection.class tries to bring up to life the class itself.

@adamgamble

but the problem is for polymorphic relationships you don't know the class name until the call back actually runs since its stored in the DB. So it has to be figured out when the callback actually runs. reflection.klass was causing the initial crash because it was trying to find a class named Parent (in the case of the gist). I hope i'm not misunderstanding, if I am I apologize.

@carlosantoniodasilva
Ruby on Rails member

I am ok with that and we do need a proper fix, but force loading a class when not necessary is also a problem :)

@adamgamble

@carlosantoniodasilva I finally got what you were saying, I was loading the class then it was being coerced to a string, then eval'd as a class. Sorry i'm dense :)

@dhh
Ruby on Rails member

Are we close to a resolution? Otherwise I'm going to yank the feature and we can work on it for a later release.

@adamgamble

I probably wont have time to work on it tonight. So I guess we'll have to yank it for the next release :(

@dhh
Ruby on Rails member

I'll yank for now but maybe there'll be other things holding up the release. If that's the case, we can still make it in. Otherwise we'll do it for the next one. Thanks for your work on this!

@dhh dhh added a commit that referenced this issue Apr 23, 2013
@dhh dhh Revert "`belongs_to :touch` behavior now touches old association when…
… transitioning to new association" until a proper fix is found for #10197
7389df1
@adamgamble

@dhh np. I'll try my best to get the fix done as soon as possible. Hopefully i'll have time to work on it soon.

@chadmoone

Even though we're waiting to include the change, we may want to add in a test case based on the gist above to check for this issue in the future, since the ActiveRecord tests actually passed with these changes in. I'm happy to turn the gist into a test and submit a pull request if it will help.

@pixeltrix pixeltrix added a commit that referenced this issue Apr 24, 2013
@pixeltrix pixeltrix Revert "Revert "`belongs_to :touch` behavior now touches old associat…
…ion when transitioning to new association" until a proper fix is found for #10197"

This reverts commit 7389df1.

Conflicts:
	activerecord/test/cases/timestamp_test.rb
1a30cfe
@pixeltrix pixeltrix added a commit that referenced this issue Apr 24, 2013
@adamgamble adamgamble added test cases for #10197 697e346
@pixeltrix pixeltrix added a commit that closed this issue Apr 24, 2013
@pixeltrix pixeltrix Lookup the class at runtime, not when the association is built
Trying to lookup the parent class when the association is being built
runs the risk of generating uninitialized constant errors because
classes haven't been fully defined yet. To work around this we look up
the class at runtime through the `association` method.

Fixes #10197.
2a89941
@pixeltrix pixeltrix closed this in 2a89941 Apr 24, 2013
@pixeltrix
Ruby on Rails member

@adamgamble it's no wonder you were having problems getting your test to pass - if you look at 2d211c4 the primary key was being changed to :name and not being reset. This meant the query for old_record failed to return a result because it was looking for … WHERE "toys"."name" = ?, meaning the old record never got its timestamp updated.

@neerajdotname
Ruby on Rails member

@pixeltrix nice fix.

@adamgamble

@pixeltrix I noticed the sql was funky but I couldn't figure out why. I also noticed that when I ran that test case by itself it passed. That should have been a huge red flag but I was rushing :) I really appreciate your help.

@adamgamble

So I bet my code actually worked, though yours is much nicer, my tests were just polluted :(

@pixeltrix
Ruby on Rails member

I also noticed that when I ran that test case by itself it passed. That should have been a huge red flag

@adamgamble yes, as soon as you get something like that it's almost always some state that a previous test hasn't reset properly. Once I'd spotted in the log that the query was looking for a column called name it was just a matter of searching the tests for somewhere that set the primary key to that value.

@eljojo

Hi, I am getting problems with this change:
I have two models, User and Order:

class Order < ActiveRecord::Base
  belongs_to :user, touch: true
end
class User < ActiveRecord::Base
  has_many :orders,   inverse_of: :user, dependent: :destroy
end

If I run Order.last.touch from the rails console, I get the following:

>> Order.last.touch
  Order Load (4.2ms)  SELECT "orders".* FROM "orders" ORDER BY orders.last_event_at ASC LIMIT 1
  SQL (1.6ms)  UPDATE "orders" SET "updated_at" = '2013-05-25 12:45:34.891496' WHERE "orders"."id" = 267
  User Load (0.6ms)  SELECT "users".* FROM "users" WHERE "users"."id" = 2 LIMIT 1
  SQL (2.3ms)  UPDATE "users" SET "updated_at" = '2013-05-25 12:45:35.127072' WHERE "users"."id" = 2
  User Load (1.2ms)  SELECT "users".* FROM "users" WHERE "users"."id" = $1 ORDER BY "users"."id" ASC LIMIT 1  [["id", 2]]
  SQL (0.6ms)  UPDATE "users" SET "updated_at" = '2013-05-25 12:45:35.140432' WHERE "users"."id" = 2

I was able to trace back the second touch, which is unnecessary, to this line which is related to this Issue.

Is anyone experiencing something similar?

@pixeltrix
Ruby on Rails member

@eljojo I can see how that's happening - the attribute_was returns the id even it hasn't changed so old_record and record will both be found causing the double touch. Seems like we need attribute_changed? added to the conditional.

@eljojo

@pixeltrix I guess that would do the trick, but, is it an expected behaviour for attribute_was to return the value even if it hasn't changed?

@pftg

Just found this issue. Added PR to fix extra queries.

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