Merge pull request #24 from procore-oss/fix-recursive-blueprint

For recursive or cyclic blueprints, halt after 10 levels by default

CHANGELOG.md

@@ -1,4 +1,12 @@
+### 1.2.0 (2024-06-26)
+
+- [BUGFIX] Fixes an issue where an association wouldn't be preloaded if it used a dynamic blueprint.
+- [BUGFIX] Fixes an infinite loop when [a Blueprint has an association to itself](https://github.com/procore-oss/blueprinter-activerecord/issues/13).
+- Added the `max_recursion` option to customize the new default behavior for recursive/cyclic blueprints.
+- Make `pre_render` compatible with all children of ActiveRecord::Relation ([#28](https://github.com/procore-oss/blueprinter-activerecord/pull/28)).
+
 ### 1.1.0 (2024-06-10)
+
 - [FEATURE] Ability to annotate a field or association for extra preloads (e.g. `field :category_name, preload: :category`)
 
 ### 1.0.2 (2024-05-21)

README.md

@@ -102,6 +102,28 @@ class WidgetBlueprint < Blueprinter::Base
 end
 ```
 
+## Recursive Blueprints
+
+Sometimes a model, and its blueprint, will have recursive associations. Think of a nested Category model:
+
+```ruby
+class Category < ApplicationRecord
+  belongs_to :parent, class_name: "Category", optional: true
+  has_many :children, foreign_key: :parent_id, class_name: "Category", inverse_of: :parent
+end
+
+class CategoryBlueprint < Blueprinter::Base
+  field :name
+  association :children, blueprint: CategoryBlueprint
+end
+```
+
+For these kinds of recursive blueprints, the extension will preload up to 10 levels deep by default. If this isn't enough, you can increase it:
+
+```ruby
+association :children, blueprint: CategoryBlueprint, max_recursion: 20
+```
+
 ## Notes on use
 
 ### Pass the *query* to render, not query *results*

lib/blueprinter-activerecord/added_preloads_logger.rb

@@ -42,7 +42,7 @@ def initialize(&log_proc)
     def pre_render(object, blueprint, view, options)
       if object.is_a?(ActiveRecord::Relation) && object.before_preload_blueprint
         from_code = object.before_preload_blueprint
-        from_blueprint = Preloader.preloads(blueprint, view, object.model)
+        from_blueprint = Preloader.preloads(blueprint, view, model: object.model)
         info = PreloadInfo.new(object, from_code, from_blueprint, caller)
         @log_proc&.call(info)
       end

lib/blueprinter-activerecord/missing_preloads_logger.rb

@@ -39,7 +39,7 @@ def initialize(&log_proc)
     def pre_render(object, blueprint, view, options)
       if object.is_a?(ActiveRecord::Relation) && !object.before_preload_blueprint
         from_code = extract_preloads object
-        from_blueprint = Preloader.preloads(blueprint, view, object.model)
+        from_blueprint = Preloader.preloads(blueprint, view, model: object.model)
         info = PreloadInfo.new(object, from_code, from_blueprint, caller)
         @log_proc&.call(info)
       end

lib/blueprinter-activerecord/preloader.rb

@@ -4,6 +4,7 @@ module BlueprinterActiveRecord
   # A Blueprinter extension to automatically preload a Blueprint view's ActiveRecord associations during render
   class Preloader < Blueprinter::Extension
     include Helpers
+    DEFAULT_MAX_RECURSION = 10
 
     attr_reader :use, :auto, :auto_proc
 
@@ -40,7 +41,7 @@ def pre_render(object, blueprint, view, options)
       if object.is_a?(ActiveRecord::Relation) && !object.loaded?
         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)
+          blueprint_preloads = self.class.preloads(blueprint, view, model: object.model)
           loader = object.preload_blueprint_method || use
           object.public_send(loader, blueprint_preloads)
         else
@@ -62,22 +63,21 @@ def pre_render(object, blueprint, view, options)
     #
     # Example:
     #
-    #   preloads = BlueprinterActiveRecord::Preloader.preloads(WidgetBlueprint, :extended, Widget)
+    #   preloads = BlueprinterActiveRecord::Preloader.preloads(WidgetBlueprint, :extended, model: Widget)
     #   q = Widget.where(...).order(...).preload(preloads)
     #
     # @param blueprint [Class] The Blueprint class
     # @param view_name [Symbol] Name of the view in blueprint
-    # @param model [Class] The ActiveRecord model class that blueprint represents
+    # @param model [Class|:polymorphic] The ActiveRecord model class that blueprint represents
+    # @param cycles [Hash<String, Integer>] (internal) Preloading will halt if recursion/cycles gets too high
     # @return [Hash] A Hash containing preload/eager_load/etc info for ActiveRecord
     #
-    def self.preloads(blueprint, view_name, model=nil)
+    def self.preloads(blueprint, view_name, model:, cycles: {})
       view = blueprint.reflections.fetch(view_name)
       preload_vals = view.associations.each_with_object({}) { |(_name, assoc), acc|
         # look for a matching association on the model
-        ref = model ? model.reflections[assoc.name.to_s] : nil
-        if (ref || model.nil?) && !assoc.blueprint.is_a?(Proc)
-          ref_model = ref && !(ref.belongs_to? && ref.polymorphic?) ? ref.klass : nil
-          acc[assoc.name] = preloads(assoc.blueprint, assoc.view, ref_model)
+        if (preload = association_preloads(assoc, model, cycles))
+          acc[assoc.name] = preload
         end
 
         # look for a :preload option on the association
@@ -93,5 +93,38 @@ def self.preloads(blueprint, view_name, model=nil)
         end
       }
     end
+
+    def self.association_preloads(assoc, model, cycles)
+      max_cycles = assoc.options.fetch(:max_recursion, DEFAULT_MAX_RECURSION)
+      if model == :polymorphic
+        if assoc.blueprint.is_a? Proc
+          {}
+        else
+          cycles, count = count_cycles(assoc.blueprint, assoc.view, cycles)
+          count < max_cycles ? preloads(assoc.blueprint, assoc.view, model: model, cycles: cycles) : {}
+        end
+      elsif (ref = model.reflections[assoc.name.to_s])
+        if assoc.blueprint.is_a? Proc
+          {}
+        elsif ref.belongs_to? && ref.polymorphic?
+          cycles, count = count_cycles(assoc.blueprint, assoc.view, cycles)
+          count < max_cycles ? preloads(assoc.blueprint, assoc.view, model: :polymorphic, cycles: cycles) : {}
+        else
+          cycles, count = count_cycles(assoc.blueprint, assoc.view, cycles)
+          count < max_cycles ? preloads(assoc.blueprint, assoc.view, model: ref.klass, cycles: cycles) : {}
+        end
+      end
+    end
+
+    def self.count_cycles(blueprint, view, cycles)
+      id = "#{blueprint.name || blueprint.inspect}/#{view}"
+      cycles = cycles.dup
+      if cycles[id].nil?
+        cycles[id] = 0
+      else
+        cycles[id] += 1
+      end
+      return cycles, cycles[id]
+    end
   end
 end

lib/blueprinter-activerecord/query_methods.rb

@@ -43,7 +43,7 @@ def preload_blueprint!(blueprint = nil, view = :default, use: :preload)
 
       if blueprint and view
         # preload right now
-        preloads = Preloader.preloads(blueprint, view, model)
+        preloads = Preloader.preloads(blueprint, view, model: model)
         public_send(use, preloads)
       else
         # preload during render

lib/blueprinter-activerecord/version.rb

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

lib/tasks/blueprinter_activerecord.rake

@@ -14,7 +14,7 @@ namespace :blueprinter do
 
       model = args[:model].constantize
       blueprint = args[:blueprint].constantize
-      preloads = BlueprinterActiveRecord::Preloader.preloads(blueprint, args[:view].to_sym, model)
+      preloads = BlueprinterActiveRecord::Preloader.preloads(blueprint, args[:view].to_sym, model: model)
       puts pretty preloads
     end
   end

test/active_record_test.rb

@@ -62,4 +62,12 @@ def test_a_more_complicated_duplication_case
     assert_match(/FROM "widgets"/, lines[1])
     assert_nil lines[2]
   end
+
+  def test_invalid_associations_throw
+    assert_raises { Widget.preload(foo).to_a }
+  end
+
+  def test_invalid_associations_under_polymorphic_works
+    Widget.preload(battery1: {foo: :bar}).to_a
+  end
 end

test/added_preloads_logger_test.rb

@@ -16,8 +16,8 @@ def test_adds_missing_preloads
 
     assert_equal({
       :category=>{},
-      :battery1=>{:fake_assoc=>{}, :refurb_plan=>{}},
-      :battery2=>{:fake_assoc=>{}, :refurb_plan=>{}},
+      :battery1=>{:fake_assoc=>{}, :fake_assoc2=>{}, :refurb_plan=>{}},
+      :battery2=>{:fake_assoc=>{}, :fake_assoc2=>{}, :refurb_plan=>{}},
       :project=>{:customer=>{}},
     }, BlueprinterActiveRecord::Helpers.extract_preloads(q3))
 
@@ -27,14 +27,16 @@ def test_adds_missing_preloads
     assert_equal [
       "battery1",
       "battery1 > fake_assoc",
+      "battery1 > fake_assoc2",
       "battery1 > refurb_plan",
       "battery2",
       "battery2 > fake_assoc",
+      "battery2 > fake_assoc2",
       "battery2 > refurb_plan",
       "project",
       "project > customer",
     ], @info.found.map { |f| f.join " > " }
-    assert_equal 89, @info.percent_found
+    assert_equal 91, @info.percent_found
   end
 
   def test_finds_visible_blueprints

test/missing_preloads_logger_test.rb

@@ -18,14 +18,16 @@ def test_finds_missing_preloads_without_preloader_ext
     assert_equal [
       "battery1",
       "battery1 > fake_assoc",
+      "battery1 > fake_assoc2",
       "battery1 > refurb_plan",
       "battery2",
       "battery2 > fake_assoc",
+      "battery2 > fake_assoc2",
       "battery2 > refurb_plan",
       "project",
       "project > customer",
     ], @info.found.map { |f| f.join " > " }
-    assert_equal 89, @info.percent_found
+    assert_equal 91, @info.percent_found
   end
 
   def test_finds_visible_blueprints
@@ -50,14 +52,16 @@ def test_finds_missing_preloads_with_dynamic_preloader_ext
     assert_equal [
       "battery1",
       "battery1 > fake_assoc",
+      "battery1 > fake_assoc2",
       "battery1 > refurb_plan",
       "battery2",
       "battery2 > fake_assoc",
+      "battery2 > fake_assoc2",
       "battery2 > refurb_plan",
       "project",
       "project > customer",
     ], @info.found.map { |f| f.join " > " }
-    assert_equal 89, @info.percent_found
+    assert_equal 91, @info.percent_found
   end
 
   def test_ignores_queries_from_preloader_ext

test/nested_render_test.rb

@@ -24,7 +24,7 @@ def setup
     end
     @queries = []
     @sub = ActiveSupport::Notifications.subscribe 'sql.active_record' do |_name, _started, _finished, _uid, data|
-      @queries << data.fetch(:sql)
+      @queries << [data.fetch(:sql), data.fetch(:type_casted_binds)]
     end
     @test_customer = customer2
   end
@@ -44,15 +44,15 @@ def test_queries_with_auto
       'SELECT "projects".* FROM "projects"',
       'SELECT "customers".* FROM "customers" WHERE "customers"."id" IN (?, ?)',
       'SELECT "widgets".* FROM "widgets" WHERE "widgets"."project_id" IN (?, ?, ?)',
-    ], @queries
+    ], @queries.map(&:first)
   end
 
   def test_queries_for_collection_proxies
     ProjectBlueprint.render(@test_customer.projects, view: :extended_plus_with_widgets)
     assert_equal [
       'SELECT "projects".* FROM "projects" WHERE "projects"."customer_id" = ?',
       'SELECT "widgets".* FROM "widgets" WHERE "widgets"."project_id" IN (?, ?)'
-    ], @queries
+    ], @queries.map(&:first)
   end
 
   def test_queries_with_auto_and_nested_render_and_manual_preloads
@@ -74,6 +74,38 @@ def test_queries_with_auto_and_nested_render_and_manual_preloads
       '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
+    ], @queries.map(&:first)
+  end
+
+  def test_preload_with_recursive_association_default_max
+    cat = Category.create!(name: "A")
+
+    cat2 = Category.create!(name: "B", parent_id: cat.id)
+    cat3 = Category.create!(name: "B", parent_id: cat.id)
+
+    cat4 = Category.create!(name: "C", parent_id: cat2.id)
+    cat5 = Category.create!(name: "C", parent_id: cat2.id)
+    cat6 = Category.create!(name: "C", parent_id: cat3.id)
+    cat7 = Category.create!(name: "C", parent_id: cat3.id)
+
+    cat8 = Category.create!(name: "D", parent_id: cat4.id)
+    cat9 = Category.create!(name: "D", parent_id: cat4.id)
+    cat10 = Category.create!(name: "D", parent_id: cat5.id)
+    cat11 = Category.create!(name: "D", parent_id: cat5.id)
+    cat12 = Category.create!(name: "D", parent_id: cat6.id)
+    cat13 = Category.create!(name: "D", parent_id: cat6.id)
+    cat14 = Category.create!(name: "D", parent_id: cat7.id)
+    cat15 = Category.create!(name: "D", parent_id: cat7.id)
+    @queries.clear
+
+    CategoryBlueprint.render(cat, view: :nested)
+    assert_equal [
+      %Q|SELECT "categories".* FROM "categories" WHERE "categories"."parent_id" = #{cat.id}|,
+      %Q|SELECT "categories".* FROM "categories" WHERE "categories"."parent_id" IN (#{cat2.id}, #{cat3.id})|,
+      %Q|SELECT "categories".* FROM "categories" WHERE "categories"."parent_id" IN (#{cat4.id}, #{cat5.id}, #{cat6.id}, #{cat7.id})|,
+      %Q|SELECT "categories".* FROM "categories" WHERE "categories"."parent_id" IN (#{cat8.id}, #{cat9.id}, #{cat10.id}, #{cat11.id}, #{cat12.id}, #{cat13.id}, #{cat14.id}, #{cat15.id})|,
+    ], @queries.map { |(sql, binds)|
+      binds.reduce(sql) { |acc, bind| acc.sub("?", bind.to_s) }
+    }
   end
 end

test/preloader_extension_test.rb

@@ -105,7 +105,7 @@ def test_blueprinter_preload_now
       preload_blueprint(WidgetBlueprint, :extended).
       strict_loading
 
-    assert_equal [{:battery1=>{:fake_assoc=>{}, :refurb_plan=>{}}, :battery2=>{:fake_assoc=>{}, :refurb_plan=>{}}, :category=>{}, :project=>{:customer=>{}}}], q.values[:preload]
+    assert_equal [{:battery1=>{:fake_assoc=>{}, :fake_assoc2=>{}, :refurb_plan=>{}}, :battery2=>{:fake_assoc=>{}, :fake_assoc2=>{}, :refurb_plan=>{}}, :category=>{}, :project=>{:customer=>{}}}], q.values[:preload]
   end
 
   def test_blueprinter_preload_now_with_existing_preloads
@@ -129,7 +129,7 @@ def test_auto_preload
 
     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]
+    assert_equal [{:battery1=>{:fake_assoc=>{}, :fake_assoc2=>{}, :refurb_plan=>{}}, :battery2=>{:fake_assoc=>{}, :fake_assoc2=>{}, :refurb_plan=>{}}, :category=>{}, :project=>{:customer=>{}}}], q.values[:preload]
   end
 
   def test_auto_preload_with_existing_preloads
@@ -153,7 +153,7 @@ def test_auto_preload_with_association_relation
 
     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]
+    assert_equal [{:battery1=>{:fake_assoc=>{}, :fake_assoc2=>{}, :refurb_plan=>{}}, :battery2=>{:fake_assoc=>{}, :fake_assoc2=>{}, :refurb_plan=>{}}, :category=>{}, :project=>{:customer=>{}}}], q.values[:preload]
   end
 
   def test_auto_preload_with_block_true
@@ -166,7 +166,7 @@ def test_auto_preload_with_block_true
 
     refute_nil ext.auto_proc
     assert_equal :preload, ext.use
-    assert_equal [{:battery1=>{:fake_assoc=>{}, :refurb_plan=>{}}, :battery2=>{:fake_assoc=>{}, :refurb_plan=>{}}, :category=>{}, :project=>{:customer=>{}}}], q.values[:preload]
+    assert_equal [{:battery1=>{:fake_assoc=>{}, :fake_assoc2=>{}, :refurb_plan=>{}}, :battery2=>{:fake_assoc=>{}, :fake_assoc2=>{}, :refurb_plan=>{}}, :category=>{}, :project=>{:customer=>{}}}], q.values[:preload]
   end
 
   def test_auto_preload_with_block_false
@@ -192,6 +192,6 @@ def test_auto_includes
 
     assert ext.auto
     assert_equal :includes, ext.use
-    assert_equal [{:battery1=>{:fake_assoc=>{}, :refurb_plan=>{}}, :battery2=>{:fake_assoc=>{}, :refurb_plan=>{}}, :category=>{}, :project=>{:customer=>{}}}], q.values[:includes]
+    assert_equal [{:battery1=>{:fake_assoc=>{}, :fake_assoc2=>{}, :refurb_plan=>{}}, :battery2=>{:fake_assoc=>{}, :fake_assoc2=>{}, :refurb_plan=>{}}, :category=>{}, :project=>{:customer=>{}}}], q.values[:includes]
   end
 end