Merge pull request #16 from procore-oss/fix-nested-renders-with-auto

Don't auto-preload AGAIN during nested renders

.github/workflows/test.yaml

@@ -11,7 +11,7 @@ jobs:
     strategy:
       matrix:
         os: [ubuntu-latest]
-        ruby: ['3.0', '3.1', '3.2', '3.3']
+        ruby: ["3.0", "3.1", "3.2", "3.3"]
     runs-on: ${{ matrix.os }}
     steps:
       - uses: actions/checkout@v4
@@ -21,6 +21,8 @@ jobs:
           ruby-version: ${{ matrix.ruby }}
           bundler-cache: true
       - name: Installing dependencies
-        run: bundle check --path=vendor/bundle || bundle install --path=vendor/bundle
+        run: |
+          bundle check --path=vendor/bundle || bundle install --path=vendor/bundle
+          bundle exec appraisal install --path=vendor/bundle
       - name: Run tests
-        run: bundle exec rake test
+        run: bundle exec appraisal rake test

CHANGELOG.md

@@ -1,3 +1,7 @@
+### 1.0.2 (2024-05-21)
+
+- [BUGFIX] Fixes a potentially significant performance issue with `auto`. See https://github.com/procore-oss/blueprinter-activerecord/pull/16.
+
 ### 1.0.1 (2024-02-09)
 
 - Fix gem summary

lib/blueprinter-activerecord/preloader.rb

@@ -28,16 +28,17 @@ def initialize(auto: false, use: :preload, &auto_proc)
 
     #
     # Implements the "pre_render" Blueprinter Extension to preload associations from a view.
-    # If auto is true, all ActiveRecord::Relation objects will be preloaded. If auto is false,
-    # only queries that have called `.preload_blueprint` will be preloaded.
+    # If auto is true, all ActiveRecord::Relation and ActiveRecord::AssociationRelation objects
+    # will be preloaded. If auto is false, only queries that have called `.preload_blueprint`
+    # will be preloaded.
     #
     # NOTE: If auto is on, *don't* be concerned that you'll end up with duplicate preloads. Even if
     # the query ends up with overlapping members in 'preload' and 'includes', ActiveRecord
     # intelligently handles them. There are several unit tests which confirm this behavior.
     #
     def pre_render(object, blueprint, view, options)
-      case object
-      when ActiveRecord::Relation
+      case object.class.name
+      when "ActiveRecord::Relation", "ActiveRecord::AssociationRelation"
         if object.preload_blueprint_method || auto || auto_proc&.call(object, blueprint, view, options) == true
           object.before_preload_blueprint = extract_preloads object
           blueprint_preloads = self.class.preloads(blueprint, view, object.model)

lib/blueprinter-activerecord/version.rb

@@ -1,5 +1,5 @@
 # frozen_string_literal: true
 
 module BlueprinterActiveRecord
-  VERSION = "1.0.1"
+  VERSION = "1.0.2"
 end

test/nested_render_test.rb

@@ -0,0 +1,68 @@
+# frozen_string_literal: true
+
+require 'test_helper'
+
+class NestedRenderTest < Minitest::Test
+  def setup
+    DatabaseCleaner.start
+    customer1 = Customer.create!(name: "ACME")
+    customer2 = Customer.create!(name: "FOO")
+    project1 = Project.create!(customer_id: customer1.id, name: "Project A")
+    project2 = Project.create!(customer_id: customer2.id, name: "Project B")
+    category1 = Category.create!(name: "Foo")
+    category2 = Category.create!(name: "Bar")
+    ref_plan = RefurbPlan.create!(name: "Plan A")
+    battery1 = LiIonBattery.create!(num_ions: 100, num_other: 100, refurb_plan_id: ref_plan.id)
+    battery2 = LeadAcidBattery.create!(num_lead: 100, num_acid: 100)
+    Widget.create!(customer_id: customer1.id, project_id: project1.id, category_id: category1.id, name: "Widget A", battery1: battery1, battery2: battery2)
+    Widget.create!(customer_id: customer1.id, project_id: project1.id, category_id: category2.id, name: "Widget B", battery1: battery1)
+    Widget.create!(customer_id: customer2.id, project_id: project2.id, category_id: category1.id, name: "Widget C", battery1: battery1)
+    Blueprinter.configure do |config|
+      config.extensions << BlueprinterActiveRecord::Preloader.new(auto: true)
+    end
+    @queries = []
+    @sub = ActiveSupport::Notifications.subscribe 'sql.active_record' do |_name, _started, _finished, _uid, data|
+      @queries << data.fetch(:sql)
+    end
+  end
+
+  def teardown
+    DatabaseCleaner.clean
+    Blueprinter.configure do |config|
+      config.extensions = []
+    end
+    ActiveSupport::Notifications.unsubscribe @sub
+    @queries.clear
+  end
+
+  def test_queries_with_auto
+    ProjectBlueprint.render(Project.all.strict_loading, view: :extended_plus_with_widgets)
+    assert_equal [
+      'SELECT "projects".* FROM "projects"',
+      'SELECT "customers".* FROM "customers" WHERE "customers"."id" IN (?, ?)',
+      'SELECT "widgets".* FROM "widgets" WHERE "widgets"."project_id" IN (?, ?)',
+    ], @queries
+  end
+
+  def test_queries_with_auto_and_nested_render_and_manual_preloads
+    widget_blueprint = Class.new(Blueprinter::Base) do
+      association :category, blueprint: CategoryBlueprint
+    end
+
+    project_blueprint = Class.new(Blueprinter::Base) do
+      association :customer, blueprint: CustomerBlueprint
+      field :widgets do |project, options|
+        widget_blueprint.render_as_hash(project.widgets)
+      end
+    end
+
+    q = Project.all.preload(widgets: :category).strict_loading
+    project_blueprint.render(q)
+    assert_equal [
+      'SELECT "projects".* FROM "projects"',
+      'SELECT "widgets".* FROM "widgets" WHERE "widgets"."project_id" IN (?, ?)',
+      'SELECT "categories".* FROM "categories" WHERE "categories"."id" IN (?, ?)',
+      'SELECT "customers".* FROM "customers" WHERE "customers"."id" IN (?, ?)',
+    ], @queries
+  end
+end

test/preloader_extension_test.rb

@@ -5,15 +5,15 @@
 class PreloaderExtensionTest < Minitest::Test
   def setup
     DatabaseCleaner.start
-    customer = Customer.create!(name: "ACME")
-    project = Project.create!(customer_id: customer.id, name: "Project A")
+    @customer = Customer.create!(name: "ACME")
+    project = Project.create!(customer_id: @customer.id, name: "Project A")
     category = Category.create!(name: "Foo")
     ref_plan = RefurbPlan.create!(name: "Plan A")
     battery1 = LiIonBattery.create!(num_ions: 100, num_other: 100, refurb_plan_id: ref_plan.id)
     battery2 = LeadAcidBattery.create!(num_lead: 100, num_acid: 100)
-    Widget.create!(customer_id: customer.id, project_id: project.id, category_id: category.id, name: "Widget A", battery1: battery1, battery2: battery2)
-    Widget.create!(customer_id: customer.id, project_id: project.id, category_id: category.id, name: "Widget B", battery1: battery1)
-    Widget.create!(customer_id: customer.id, project_id: project.id, category_id: category.id, name: "Widget C", battery1: battery1)
+    Widget.create!(customer_id: @customer.id, project_id: project.id, category_id: category.id, name: "Widget A", battery1: battery1, battery2: battery2)
+    Widget.create!(customer_id: @customer.id, project_id: project.id, category_id: category.id, name: "Widget B", battery1: battery1)
+    Widget.create!(customer_id: @customer.id, project_id: project.id, category_id: category.id, name: "Widget C", battery1: battery1)
     Blueprinter.configure do |config|
       config.extensions << BlueprinterActiveRecord::Preloader.new
     end
@@ -108,6 +108,17 @@ def test_blueprinter_preload_now
     assert_equal [{:battery1=>{:fake_assoc=>{}, :refurb_plan=>{}}, :battery2=>{:fake_assoc=>{}, :refurb_plan=>{}}, :category=>{}, :project=>{:customer=>{}}}], q.values[:preload]
   end
 
+  def test_blueprinter_preload_now_with_existing_preloads
+    q = Widget.
+      where("widgets.name <> ?", "Widget C").
+      order(:name).
+      preload({battery1: :refurb_plan}).
+      preload_blueprint(WidgetBlueprint, :no_power).
+      strict_loading
+
+    assert_equal({:battery1=>{:refurb_plan=>{}}, :category=>{}, :project=>{:customer=>{}}}, BlueprinterActiveRecord::Helpers.merge_values(q.values[:preload]))
+  end
+
   def test_auto_preload
     ext = BlueprinterActiveRecord::Preloader.new(auto: true)
     q = Widget.
@@ -121,6 +132,30 @@ def test_auto_preload
     assert_equal [{:battery1=>{:fake_assoc=>{}, :refurb_plan=>{}}, :battery2=>{:fake_assoc=>{}, :refurb_plan=>{}}, :category=>{}, :project=>{:customer=>{}}}], q.values[:preload]
   end
 
+  def test_auto_preload_with_existing_preloads
+    ext = BlueprinterActiveRecord::Preloader.new(auto: true)
+    q = Widget.
+      where("name <> ?", "Widget C").
+      order(:name).
+      preload({battery1: :refurb_plan}).
+      strict_loading
+    q = ext.pre_render(q, WidgetBlueprint, :no_power, {})
+
+    assert ext.auto
+    assert_equal :preload, ext.use
+    assert_equal({:battery1=>{:refurb_plan=>{}}, :category=>{}, :project=>{:customer=>{}}}, BlueprinterActiveRecord::Helpers.merge_values(q.values[:preload]))
+  end
+
+  def test_auto_preload_with_association_relation
+    ext = BlueprinterActiveRecord::Preloader.new(auto: true)
+    q = @customer.widgets.order(:name).strict_loading
+    q = ext.pre_render(q, WidgetBlueprint, :extended, {})
+
+    assert ext.auto
+    assert_equal :preload, ext.use
+    assert_equal [{:battery1=>{:fake_assoc=>{}, :refurb_plan=>{}}, :battery2=>{:fake_assoc=>{}, :refurb_plan=>{}}, :category=>{}, :project=>{:customer=>{}}}], q.values[:preload]
+  end
+
   def test_auto_preload_with_block_true
     ext = BlueprinterActiveRecord::Preloader.new { |object| true }
     q = Widget.