Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Not compatible with ActiveRecord_Associations_CollectionProxy #26

Closed
ryanmccarthypdx opened this issue Jun 21, 2024 · 3 comments
Closed

Comments

@ryanmccarthypdx
Copy link
Contributor

ryanmccarthypdx commented Jun 21, 2024

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
Copy link
Contributor

jhollinger commented Jun 24, 2024

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
Copy link
Contributor Author

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: #28

jhollinger added a commit that referenced this issue Jun 25, 2024
…ection_proxy

[Issue #26] Make pre_render compatible with all children of ActiveRecord::Relation
@jhollinger
Copy link
Contributor

Fixed in 1.2.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants