Relation#decorate() doesn't cache objects returned by joins, ExampleDraper.decorate_collection does

krainboltgreene commented 11 years ago

Latest stable Rails, latest stable draper

rails new draper_issue --skip-bundle -G
cd draper_issue
echo "gem 'draper'" >> Gemfile
bundle install
rails generate resource Account
rails generate resource Article account:belongs_to
rails generate resource Comment article:belongs_to loved:boolean:index
rake db:create db:migrate
echo "class Account < ActiveRecord::Base\n  has_many :articles\nend" > app/models/account.rb
echo "class Article < ActiveRecord::Base\n  belongs_to :account\n  has_many :comments\n  attr_accessor :loved\nend" > app/models/article.rb
echo "class Comment < ActiveRecord::Base\n  belongs_to :article\n  attr_accessible :loved\nend" > app/models/comment.rb
echo "class AccountDecorator < Draper::Decorator\n  delegate_all\n  decorates_association :articles\nend" > app/decorators/account_decorator.rb
echo "class ArticleDecorator < Draper::Decorator\n  delegate_all\n  decorates_association :comments\nend" > app/decorators/article_decorator.rb
echo "class CommentDecorator < Draper::Decorator\n  delegate_all\nend" > app/decorators/comment_decorator.rb
rails console

Which makes these:

class Account < ActiveRecord::Base
  has_many :articles
end

class Article < ActiveRecord::Base
  belongs :account
  has_many :comments

  attr_accessor :loved
end

class Comment < ActiveRecord::Base
  belongs_to :article

  attr_accessible :loved
end

class AccountDecorator < Draper::Decorator
  delegate_all
  decorates_association :articles

end

class ArticleDecorator < Draper::Decorator
  delegate_all
  decorates_association :comments

end

class CommentDecorator < Draper::Decorator
  delegate_all

end

Now we seed the database:

account = Account.create
article = account.articles.create
article.comments.create
article.comments.create loved: true
article.comments.create

article = account.articles.create
article.comments.create loved: true
article.comments.create

account = Account.create
article = account.articles.create
article.comments.create
article.comments.create loved: true
article.comments.create

article = account.articles.create
article.comments.create loved: true
article.comments.create

Now we show our problem:

# Clear previous bindings
reload!

# Get all accounts, join the article and comments
accounts = Account.joins(articles: :comments).uniq

# Store the loved comment in the article @loved accessor
accounts.each do |account|
  account.articles.each do |article|
    article.loved = article.comments.select(&:loved).first
  end
end

# This should be 4, and is 4
accounts.map(&:articles).flatten.select(&:loved).size

# This should be 4, but is 0
accounts.decorate.map(&:articles).flatten.select(&:loved).size

# This should be 4, and is 4
AccountDecorator.decorate_collection(accounts).map(&:articles).flatten.select(&:loved).size

steveklabnik commented 11 years ago

Whoah.

haines commented 11 years ago

Thanks for the detailed report! This is a consequence of the implementation of Relation#decorate as a class method on the model, which essentially does

# Account.decorate
AccountDecorator.decorate_collection(self.scoped)

The self.scoped is the key here, as it forces a database reload. That means that accounts and accounts.decorate end up with different model objects, and the latter does not see the modified articles.

It's a bit of a gotcha that has come up a couple of times recently. I wonder if there is a better way we can implement this that doesn't involve database reloads (it would avoid the need for the messy #530) - perhaps by directly including a module into Relation rather than defining a class method on the models.

tovodeverett commented 11 years ago

We'll still need the class method on the models, but perhaps there can be a second implementation of decorate on ActiveRecord::Relation that avoid the whole current_scope delegation stuff. One solution that relies on ActiveRecord and its internals (and is thus a bad long-term solution, but makes for an easy monkey patch) makes use of current_scope.

See https://github.com/drapergem/draper/issues/412#issuecomment-13308189 for the suggested workaround (and also an interesting discussion on how ActiveRecord::Relation delegates method calls back to the model), and see https://github.com/drapergem/draper/issues/484#issuecomment-14346295 for @haines's perfectly justifiable reasons not to use it. Note that I never came up with a strong enough argument to overcome the objections, which is why I didn't respond in the latter thread. Direct reimplementation on ActiveRecord::Relation is probably the cleanest.

brocktimus commented 10 years ago

I ran into this issue and did a bit of head scratching. Some testing using ::loaded? in combination with ActiveRecord::Relation? I don't know if this could be used for some conditional behaviour.

Using the above models as an example:

article = Article.first
article.comments.loaded? => false
article.comments.to_a
article.comments.loaded? => true

article = Article.includes(:comments).first
article.comments.loaded? => true

Thus, this shouldn't hit the DB. Tested and confirmed that is the case.

article = Article.includes(:comments).first
article.comments.loaded? => true
decorated_comments = CommentDecorator.decorate_collection(article.comments)

More thinking out loud, I think AR4 could make this easier. Then the above implementation (in @haines) could be changed to:

AccountDecorator.decorate_collection(self.all)
AccountDecorator.decorate_collection(self.to_a)

The goal being to call a method that doesn't force the reload as ::scoped does. Although ::to_a doesn't exist on a model in AR3 and I don't know if the behaviour of ::all is the same either. It could potentially be conditional behaviour based on the major version of AR?

Few potential avenues, what do you all think? Possibly a combination of a couple of them. I might try and prepare a PR for this. Problem is I don't know how to write a test for "the database isn't being hit"...

/cc @haines @tovodeverett

brocktimus commented 10 years ago

Urgh. I did more testing and draper is already doing what I thought might fix the issue. My explanation might be a bit off and a bit fuzzy, just jotting down and trying to explain observations.

From what I can tell the issue comes down to where AR sends method calls. When you call #comments you're actually calling methods against the CollectionProxy first (ActiveRecord::Associations::CollectionProxy < Relation). If more methods are chained onto the end, you seem to just be chaining methods onto the bare Relation.

So we have #decorate on the AR::Base model, but in that scope we no longer have access to the association cache. It's only available from within the CollectionProxy or the original AR::Base model (Article) and not the related one (Comment).

It might be possible if #decorate is also added to CollectionProxy instead. I'm going to test with a monkey patch and post in a Gist if I have success.

Also figured out it could be tested by comparing #object_id of related objects. But I'll look at that again later.

brocktimus commented 10 years ago

Gist isn't necessary, my idea worked. This monkey patch did what I wanted.

class ActiveRecord::Associations::CollectionProxy
  def decorate(options = {})
    @association.klass.decorator_class.decorate_collection(@association.target, options.reverse_merge(with: nil))
  end
end

Now to look at:

Writing tests
Figuring out an elegant way of integrating it in

Sorry for the lots of updates, kept having ideas and coming back to the problem :stuck_out_tongue:.

tovodeverett commented 10 years ago

I could be wrong, but it's possible that https://github.com/drapergem/draper/issues/412#issuecomment-13308189 might help illuminate the rabbit hole you are tumbling down! It was definitely a Red Pill exploration for me!

brocktimus commented 10 years ago

Yeah its getting that way. Mine wouldn't solve your problem however, this explicitly deals with the association rather than scopes in general. I'll comment Re: #412 in that thread. I ran into this problem because I was doing Model.includes(foos: [:bars]) and then realised ALL the queries were running as if there wasn't any includes.