procore-oss / blueprinter-activerecord

MIT License
9 stars 3 forks source link

Not compatible with ActiveRecord_Associations_CollectionProxy #26

Closed ryanmccarthypdx closed 4 months ago

ryanmccarthypdx commented 4 months ago

Consider the following:

class Foo < ActiveRecord::Base
  belongs_to :bar
  belongs_to :baz

  def bar_dependent_method
    "I'm a little teapot #{bar.stature_and_girth}"
  end
end

class Bar < ActiveRecord::Base
  def stature_and_girth
    "short and stout"
  end
end

class Baz < ActiveRecord::Base
  has_many :foos
end

class FooBlueprint
  field: :bar_dependent_method, preload: :bar
end

bar = Bar.create()
baz = Baz.create()
Foo.create(bar: bar, baz: baz)
Foo.create(bar: bar, baz: baz)
Foo.create(bar: bar, baz: baz)

baz.foos.class
=> Foo::ActiveRecord_Associations_CollectionProxy
Foo.where(baz_id: baz.id).class
=> Foo::ActiveRecord_Relation
baz.foos.to_sql == Foo.where(baz_id: baz.id).to_sql
=> true

FooBlueprint.render(baz.foos)                  # this N+1s
FooBlueprint.render(Foo.where(baz_id: baz.id)) # this does not N+1

Basically, I want to change the check in the Preloader#pre_render from matching on class name to is_a?(ActiveRecord::Relation) to include all relevant child classes.

jhollinger commented 4 months ago

We recently undid that exact thing as it causes performance issues. This is because there is no way to ensure that pre_render is called only once. During rendering, every time Blueprinter hits an association, it makes a "render" call, which then calls pre_render. This causes things that had already been pre-loaded at a higher level to be preloaded again.

As things currently stand in Blueprinter's codebase, there's no way to distinguish between that original pre_render call and the nested ones.

ryanmccarthypdx commented 4 months ago

I think the issue is that we are using the CollectionProxy as a shorthand for 'nested relation, and therefore already loaded'. But there are more direct ways of getting that information.

See PR here: https://github.com/procore-oss/blueprinter-activerecord/pull/28

jhollinger commented 4 months ago

Fixed in 1.2.0.