Associations: #decorate queries DB every time

`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.

Resolves drapergem#646,
         drapergem#706,
         drapergem#827.

draper.gemspec

@@ -26,6 +26,7 @@ Gem::Specification.new do |s|
   s.add_development_dependency 'ammeter'
   s.add_development_dependency 'rake'
   s.add_development_dependency 'rspec-rails'
+  s.add_development_dependency 'rspec-activerecord-expectations', '~> 1.0'
   s.add_development_dependency 'minitest-rails'
   s.add_development_dependency 'capybara'
   s.add_development_dependency 'active_model_serializers', '>= 0.10'

lib/draper.rb

@@ -51,12 +51,6 @@ def self.setup_action_mailer(base)
     end
   end
 
-  def self.setup_orm(base)
-    base.class_eval do
-      include Draper::Decoratable
-    end
-  end
-
   class UninferrableDecoratorError < NameError
     def initialize(klass)
       super("Could not infer a decorator for #{klass}.")

lib/draper/decoratable.rb

@@ -12,6 +12,8 @@ module Decoratable
     extend ActiveSupport::Concern
     include Draper::Decoratable::Equality
 
+    autoload :CollectionProxy, 'draper/decoratable/collection_proxy'
+
     included do
       prepend Draper::Compatibility::Broadcastable if defined? Turbo::Broadcastable
     end

lib/draper/decoratable/collection_proxy.rb

@@ -0,0 +1,15 @@
+module Draper
+  module Decoratable
+    module CollectionProxy
+      # Decorates a collection of objects. Used at the end of a scope chain.
+      #
+      # @example
+      #   company.products.popular.decorate
+      # @param [Hash] options
+      #   see {Decorator.decorate_collection}.
+      def decorate(options = {})
+        decorator_class.decorate_collection(load_target, options.reverse_merge(with: nil))
+      end
+    end
+  end
+end

lib/draper/railtie.rb

@@ -33,10 +33,14 @@ class Railtie < Rails::Railtie
     end
 
     initializer 'draper.setup_orm' do
-      [:active_record, :mongoid].each do |orm|
-        ActiveSupport.on_load orm do
-          Draper.setup_orm self
-        end
+      ActiveSupport.on_load :active_record do
+        include Draper::Decoratable
+
+        ActiveRecord::Associations::CollectionProxy.include Draper::Decoratable::CollectionProxy
+      end
+
+      ActiveSupport.on_load :mongoid do
+        include Draper::Decoratable
       end
     end
 

spec/dummy/app/decorators/comment_decorator.rb

@@ -0,0 +1,13 @@
+class CommentDecorator < Draper::Decorator
+  delegate_all
+
+  # Define presentation-specific methods here. Helpers are accessed through
+  # `helpers` (aka `h`). You can override attributes, for example:
+  #
+  #   def created_at
+  #     helpers.content_tag :span, class: 'time' do
+  #       object.created_at.strftime("%a %m/%d/%y")
+  #     end
+  #   end
+
+end

spec/dummy/app/models/comment.rb

@@ -0,0 +1,3 @@
+class Comment < ApplicationRecord
+  belongs_to :post
+end

spec/dummy/app/models/post.rb

@@ -3,5 +3,7 @@
 class Post < ApplicationRecord
   # attr_accessible :title, :body
 
+  has_many :comments
+
   broadcasts if defined? Turbo::Broadcastable
 end

spec/dummy/db/migrate/20240907041839_create_comments.rb

@@ -0,0 +1,9 @@
+class CreateComments < ActiveRecord::Migration[6.1]
+  def change
+    create_table :comments do |t|
+      t.references :post, foreign_key: true
+
+      t.timestamps
+    end
+  end
+end

spec/dummy/db/schema.rb

@@ -1,21 +1,28 @@
-# encoding: UTF-8
 # This file is auto-generated from the current state of the database. Instead
 # of editing this file, please use the migrations feature of Active Record to
 # incrementally modify your database, and then regenerate this schema definition.
 #
-# Note that this schema.rb definition is the authoritative source for your
-# database schema. If you need to create the application database on another
-# system, you should be using db:schema:load, not running all the migrations
-# from scratch. The latter is a flawed and unsustainable approach (the more migrations
-# you'll amass, the slower it'll run and the greater likelihood for issues).
+# This file is the source Rails uses to define your schema when running `bin/rails
+# db:schema:load`. When creating a new database, `bin/rails db:schema:load` tends to
+# be faster and is potentially less error prone than running all of your
+# migrations from scratch. Old migrations may fail to apply correctly if those
+# migrations use external dependencies or application code.
 #
-# It's strongly recommended to check this file into your version control system.
+# It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20121019115657) do
+ActiveRecord::Schema.define(version: 2024_09_07_041839) do
 
-  create_table "posts", force: true do |t|
+  create_table "comments", force: :cascade do |t|
+    t.integer "post_id"
+    t.datetime "created_at", precision: 6, null: false
+    t.datetime "updated_at", precision: 6, null: false
+    t.index ["post_id"], name: "index_comments_on_post_id"
+  end
+
+  create_table "posts", force: :cascade do |t|
     t.datetime "created_at", null: false
     t.datetime "updated_at", null: false
   end
 
+  add_foreign_key "comments", "posts"
 end

spec/dummy/spec/models/post_spec.rb

@@ -2,6 +2,8 @@
 require 'shared_examples/decoratable'
 
 RSpec.describe Post do
+  let(:record) { described_class.create! }
+
   it_behaves_like 'a decoratable model'
 
   it { should be_a ApplicationRecord }
@@ -18,4 +20,45 @@
       }
     end
   end if defined? Turbo::Broadcastable
+
+  describe 'associations' do
+    context 'when decorated' do
+      subject { associated.decorate }
+
+      let(:associated) { record.comments }
+      let(:persisted)  { associated.create! [{}] * rand(0..2) }
+      let(:unsaved)    { associated.build   [{}] * rand(1..2) }
+
+      before { persisted } # should exist
+
+      it 'returns a decorated collection' do
+        is_expected.to match_array persisted
+        is_expected.to be_all &:decorated?
+      end
+
+      it 'uses cached records' do
+        expect(associated).not_to be_loaded
+
+        associated.load
+
+        expect { subject.to_a }.to execute.exactly(0).queries
+      end
+
+      it 'caches records' do
+        expect(associated).not_to be_loaded
+
+        associated.decorate
+
+        expect { subject.to_a; associated.load }.to execute.exactly(0).queries
+      end
+
+      context 'with unsaved records' do
+        before { unsaved } # should exist
+
+        it 'respects unsaved records' do
+          is_expected.to match_array persisted + unsaved
+        end
+      end
+    end
+  end
 end

spec/dummy/spec/spec_helper.rb

@@ -1,8 +1,11 @@
 ENV['RAILS_ENV'] ||= 'test'
 require File.expand_path('../../config/environment', __FILE__)
 require 'rspec/rails'
+require 'rspec/activerecord/expectations'
 
 RSpec.configure do |config|
   config.expect_with(:rspec) {|c| c.syntax = :expect}
   config.order = :random
+
+  include RSpec::ActiveRecord::Expectations
 end