Associations: #decorate queries DB every time

[Alexander-Senko]

ActiveRecord::Associations::CollectionProxy#decorate ignores target (be it already loaded or not) and loads the association again every time it's called.

That's why unsaved records get missing from the decorated collection.

Testing

• it 'returns a decorated collection'

  1. Ensure a record and some records associated with it are persisted.
  2. Get the decorated association: record.associated_records.decorate.
  3. Ensure that it matches record.associated_records.

• it 'uses cached records'

  1. Ensure a record and some records associated with it are persisted.
  2. Get the association: record.associated_records.
  3. Get the decorated association: record.associated_records.decorate.
  4. Ensure the association was loaded only once.

• it 'caches records'

  1. Ensure a record and some records associated with it are persisted.
  2. Get the decorated association: record.associated_records.decorate (twice).
  3. Get the association: record.associated_records.
  4. Ensure the association was loaded only once.

• it 'respects unsaved records'

  1. Ensure a record and some records associated with it are persisted.
  2. build new associated records.
  3. Get the decorated association: record.associated_records.decorate.
  4. Ensure that it matches both record.associated_records and the new ones.

To-Dos

• tests

References

• Decorating associations created with FactoryGirl.build_stubbed fails with 1.2 #518
• Relation#decorate() doesn't cache objects returned by joins, ExampleDraper.decorate_collection does #538
• Work in progress fix for #538 #588
• Draper forces DB queries on decorated model's associations #646
• .decorate on an ActiveRecord::Associations::CollectionProxy ignores current value #706
• model.assocations.decorate loads the association even if it is preloaded (AR) #827

[Alexander-Senko]

I'm sorry, but the test environment looks like a mess for me 😕 I spent some time figuring out how it's supposed to test Active Record integration — and found nothing looking like a valid example. Was it ever tested? 🤔

So, I've tested it manually. Could we pull this in without a test coverage for now?

[y-yagi]

I'm sorry, but the test environment looks like a mess for me 😕 I spent some time figuring out how it's supposed to test Active Record integration — and found nothing looking like a valid example. Was it ever tested? 🤔

I'm sorry I've misunderstood something, but I think we can add these kinds of specs under the spec/dummy you already did in #929. At least, the following spec works as expected.

diff --git a/spec/dummy/app/decorators/comment_decorator.rb b/spec/dummy/app/decorators/comment_decorator.rb
new file mode 100644
index 0000000..8257739
--- /dev/null
+++ b/spec/dummy/app/decorators/comment_decorator.rb
@@ -0,0 +1,2 @@
+class CommentDecorator < Draper::Decorator
+end
\ No newline at end of file
diff --git a/spec/dummy/app/models/comment.rb b/spec/dummy/app/models/comment.rb
new file mode 100644
index 0000000..8b86c56
--- /dev/null
+++ b/spec/dummy/app/models/comment.rb
@@ -0,0 +1,3 @@
+class Comment < ApplicationRecord
+  belongs_to :post
+end
diff --git a/spec/dummy/app/models/post.rb b/spec/dummy/app/models/post.rb
index 6352fdb..d143128 100644
--- a/spec/dummy/app/models/post.rb
+++ b/spec/dummy/app/models/post.rb
@@ -4,4 +4,6 @@ class Post < ApplicationRecord
   # attr_accessible :title, :body

   broadcasts if defined? Turbo::Broadcastable
+
+  has_many :comments
 end
diff --git a/spec/dummy/db/schema.rb b/spec/dummy/db/schema.rb
index 9aebf2c..8213494 100644
--- a/spec/dummy/db/schema.rb
+++ b/spec/dummy/db/schema.rb
@@ -18,4 +18,9 @@ ActiveRecord::Schema.define(version: 20121019115657) do
     t.datetime "updated_at", null: false
   end

+  create_table "comments", force: true do |t|
+    t.integer "post_id", null: false
+    t.datetime "created_at", null: false
+    t.datetime "updated_at", null: false
+  end
 end
diff --git a/spec/dummy/spec/models/post_spec.rb b/spec/dummy/spec/models/post_spec.rb
index 8bc05a0..c8dc10e 100644
--- a/spec/dummy/spec/models/post_spec.rb
+++ b/spec/dummy/spec/models/post_spec.rb
@@ -18,4 +18,13 @@ RSpec.describe Post do
       }
     end
   end if defined? Turbo::Broadcastable
+
+  describe 'association' do
+    it 'respects unsaved records' do
+      post = Post.create!
+      Comment.create!(post: post)
+      post.comments.build
+      expect(post.comments.decorate.count).to eq 2
+    end
+  end
 end

[y-yagi]

LGTM!