procore-oss / blueprinter-activerecord

MIT License
9 stars 3 forks source link

Infinite loop when association targets the same model #13

Closed jamesst20 closed 4 months ago

jamesst20 commented 6 months ago

Hi,

I have this model

module MyModule
  class Field < ApplicationRecord
    belongs_to :parent, class_name: "MyModule::Field", optional: true

    has_many :children, foreign_key: :parent_id, class_name: "MyModule::Field", dependent: :destroy, inverse_of: :parent
  end
end

and it seems to be causing an infinite loop:

web    | blueprinter-activerecord (1.0.1) lib/blueprinter-activerecord/preloader.rb:71:in `preloads'
web    | blueprinter-activerecord (1.0.1) lib/blueprinter-activerecord/preloader.rb:75:in `block in preloads'
web    | blueprinter-activerecord (1.0.1) lib/blueprinter-activerecord/preloader.rb:71:in `each'
web    | blueprinter-activerecord (1.0.1) lib/blueprinter-activerecord/preloader.rb:71:in `each_with_object'
web    | blueprinter-activerecord (1.0.1) lib/blueprinter-activerecord/preloader.rb:71:in `preloads'
web    | blueprinter-activerecord (1.0.1) lib/blueprinter-activerecord/preloader.rb:75:in `block in preloads'
web    | blueprinter-activerecord (1.0.1) lib/blueprinter-activerecord/preloader.rb:71:in `each'
web    | blueprinter-activerecord (1.0.1) lib/blueprinter-activerecord/preloader.rb:71:in `each_with_object'
web    | blueprinter-activerecord (1.0.1) lib/blueprinter-activerecord/preloader.rb:71:in `preloads'
web    | blueprinter-activerecord (1.0.1) lib/blueprinter-activerecord/preloader.rb:75:in `block in preloads'
web    | blueprinter-activerecord (1.0.1) lib/blueprinter-activerecord/preloader.rb:71:in `each'
web    | blueprinter-activerecord (1.0.1) lib/blueprinter-activerecord/preloader.rb:71:in `each_with_object'
web    | blueprinter-activerecord (1.0.1) lib/blueprinter-activerecord/preloader.rb:71:in `preloads'
...

is there a workaround? This happens with both use: :proload and use: :includes

njbbaer commented 6 months ago

@jamesst20 Can you include your blueprint?

james-em commented 6 months ago

@njbbaer

Sorry for late reply

module MyModule
  class FieldBlueprint < ApplicationBlueprint
    identifier :id

    association :children, blueprint: MyModule::FieldBlueprint
  end
end

This is happening because the relationships are self referencing itself

MyModule::Field#parent -> MyModule::Field
MyModule::Field#children -> MyModule::Field[]
github-actions[bot] commented 5 months ago

This issue is stale because it has been open for 30 days with no activity and will be closed in 14 days unless you add a comment.

jamesst20 commented 5 months ago

Just adding a comment so it doesn't get closed

jhollinger commented 4 months ago

This is an interesting case. I'm working on a PR that halts the recursion after one level, resulting in a preload of {children: {}}. It fixes the immediate problem, but of course it wouldn't handle cases where things are nested N levels deep. Not sure there's a way to. Thoughts?

jamesst20 commented 4 months ago

This is an interesting case. I'm working on a PR that halts the recursion after one level, resulting in a preload of {children: {}}. It fixes the immediate problem, but of course it wouldn't handle cases where things are nested N levels deep. Not sure there's a way to. Thoughts?

I'm happy to see some activities finally ! :) This is still bothering me as of today

Indeed fixing to one level would fix the issue, but wouldn't preload everything.

Fun fact, I wrote a gem recently and faced similar challenges. I wasn't doing it for preloading but for accepting nested attributes at infinite n deep level through circular references.

I wrote an interface that could infer automatically what models are associated to other models so I can easily parse received HTTP parameters,

https://github.com/jamesst20/pretty_api/blob/master/lib/pretty_api/active_record/associations.rb
https://github.com/jamesst20/pretty_api/blob/master/spec/pretty_api/active_record/associations_spec.rb
PrettyApi::ActiveRecord::Associations.nested_attributes_tree User
# Model: https://github.com/jamesst20/pretty_api/blob/master/spec/app/models/user.rb
# User associations
{
  CompanyCar => {
    organization: { allow_destroy: true, model: Organization, type: :belongs_to }
  },
  Organization => {
    company_car: { allow_destroy: true, model: CompanyCar, type: :has_one },
    services: { allow_destroy: true, model: Service, type: :has_many }
  },
  Phone => {},
  Service => {
    phones: { allow_destroy: true, model: Phone, type: :has_many }
  },
  User => {
    # You can see here it self reference
    children: { allow_destroy: true, model: User, type: :has_many },
    organizations: { allow_destroy: true, model: Organization, type: :has_many }
  }
}

Not sure if this can be of any help. It would require a little tweaking because I extract my associations with model_class.nested_attributes_options.

In the case of Blueprinter, I guess the hard part is building the final association tree (or plan it seems to be called) whereas in my gem the received parameters kind of act as a plan and I just need to know how to traverse it.

Maybe if it ain't harmful to build too many levels, we could have some way to configure a maximum? It seems to be recursively parsed so we could build an hash like so:

counts = { my_model.associations: 1, my_model.other_associations: 3}

we increment the count everytime we traverse a model association and then stop the recursion when we reach our maximum

jamesst20 commented 4 months ago

@jhollinger

I would like to add that it's also worth preloading self-referencing blueprint siblings aswell.

class User
  belongs_to :parent, optional: true

  has_many :cars
  has_many children, foreign_key: :parent_id, class_name: "User"
end

class Car
  belongs_to :user
end

This should be preloaded like so too:

{
  cars: :cars,
  children: {
    cars: :cars,
    children: {
      cars: :cars,
      children: {
        cars: :cars,
        children: :children
      }
    }
  }
}

Edit: This gem apparently is able to take care of that because as soon as I install the gem without any extra steps everything n deep is properly eager loaded: https://github.com/salsify/goldiloader. This one works too: https://github.com/DmitryTsepelev/ar_lazy_preload

Before Capture d’écran, le 2024-06-12 à 15 03 58 After Capture d’écran, le 2024-06-12 à 15 02 27

jhollinger commented 4 months ago

Yes, preloading siblings should work as-is.

Based on the query logs above, those other gems must be cleverly "reacting" to the query results in real-time. This gem works very differently by essentially converting your Blueprint tree into a giant nested Hash and feeding it to preload. Although we did have a bug (fixed in 1.0.2) that kind of behaved like those other gems. There's potentially a chance we could leverage that behavior here in a good way, but I'll have to think on it...

On the other hand, your counter idea would be pretty simple to implement with something like:

association :children, blueprint: self, max_recursion: 5
jhollinger commented 4 months ago

Fixed in 1.2.0.