Pattern for Breaking Circular Dependencies in Rails Models - Feedback and Suggestions Welcome!

This message was imported from the Ruby/Rails Modularity Slack server. Find more info in the import thread.

I’ve just published a gist with the simplest example I could imagine to showcase a pattern I’ve been using quite successfully to break the has_manybelongs_to circular dependencies for Rails models living in separate packs. You can check it out here: https://gist.github.com/oboxodo/7952fd5e83b7ce77e397b5f1c4ba543a

I’ve mentioned this pattern here before (moths ago) and got some valuable feedback. Some didn’t like it because I’m still leveraging ActiveRecord’s callbacks, and I can understand that POV too. But this pattern is still valid for many cases so I wanted to share in a more permanent place just for reference.

I’m considering the possibility of implementing something like a better_belongs_to (need a better name) wrapper for belongs_to which could potentially be a macro to avoid having to add the initializer stuff. But this is just fine for now.

Open for feedback. Be nice :sweat_smile:.

Message originally sent by slack user U712YWCKK8T

Instead of Post.before_destroy, does calling Post.has_many within config.after_initialize work?

Honestly, I think I tried that back when I was investigating and I have the impression it didn’t work. Sorry I’m not sure.

That being said, I wouldn’t do that. The main reason being that doing so would still allow code like @post.comments.some_method when you’re trying to decouple Post from Comment. That’s why I think the change from @post.comments to Comment.for_post(@post) is the real goal here. The Post.before_destroy hook does help maintaining the very useful (IMO) dependent option functionality from has_many, while still keeping the code decoupled.

Basically, I want any existing code using @post.comments to break and force me to refactor.

Message originally sent by slack user U70TIGAX94P

The reasoning seems solid, it’s definitely an interesting pattern.

My main concern is about discoverability and cohesion. It’d be nice to be able to define this in, or close to, the comment model. Business logic in initializers is icky.

Generally I like the pattern of injecting dependencies through initializers though.

Message originally sent by slack user U70TIGAX94P

Note though that the way you’ve set it up will load Post and all constants referenced during its initialization when your initializer is executed.

If you want application startup to be fast in autoloaded environments, you probably want to avoid that.

You can use ActiveSupport’s lazy load hooks to set it up so that your dependency is injected only after Post is loaded.

Thanks for the feedback, <@U70TIGAX94P>! Very valuable. I’ll take a look at those lazy load hooks later. Looks interesting. For now, I’m more interested in setting up and documenting patterns for the rest of the team to mimic so we can distribute the load of the modularization effort a bit more :sweat_smile:.

Regarding:

It’d be nice to be able to define this in, or close to, the comment model.
Yes. I totally agree. As I mentioned in my initial comment, I’m hoping to dedicate some time to try and implement some sort of better_belongs_to of sorts which would accept an option like dependent, but for the counterpart. Something along the lines of:

class Comment
  belongs_to :post, target_dependent: :restrict_with_exception
end

That should be enough for me to be able to automatically do the same that I’m currently doing from the initializer. Probably even creating the scope. What do you think?

I’m in search for a better name for the target_dependent option. Some other ideas: on_target_destroy, on_destroy, destroy_reaction.

We’ve tried a very similar pattern

Rails.configuration.after_initialize do
  X.before_destroy ModularUnit::Hooks::YDestroy
end
module ModularUnit
  module Hooks
    class YDestroy
      def self.before_destroy(x)
        # destroy Y that is related to x
      end
    end
  end
end

Discoverability isn’t great, but I like that it’s not just tucked away in the initializers, both for visibility and that’s it’s easier to test.

That said, we’ve typically viewed this as a step towards modularity because it’s not as loose a coupling as we are striving for.

Interesting!

<@U70TIGAX94P> I was looking at the lazy load hook stuff you pointed me to. Do you know if there’s a way to hook to the loading/reloading of a specific model without having to add a call to run_load_hooks to each class? I’d like to be able to listen to a hook reloading any specific class but without the hassle of having to trigger that from each class :sweat_smile:

Message originally sent by slack user U70TIGAX94P

There are some predefined hooks IIRC but I think all of those are at framework level, not for individual classes.

IMO the fact you have to do it explicitly is kind of nice because it gives you an explicit extension point. It tells the reader that this class is intended to be extended on load

Message originally sent by slack user U70TIGAX94P

I‘ve just implemented something similar for extending GraphQL objects and wrapped it in a shallow library so that these statements are more obviously extension points

yeah, I thought of that as a good thing… but honestly… it’ll end up all over the place :grimacing:

Message originally sent by slack user U70TIGAX94P

What does that tell you about your dependency graph? :sweat_smile:

it tells me that we’re just starting but moving forward! haha

it’s not final state.

I need “gradual” steps.

one thing we’re noticing is that we used config.after_initialize and for some stuff we need to change it to [config.to](http://config.to)_prepare so it works better on dev envs. But whatever code we add to a to_prepare block needs to be idempotent and I don’t think adding before_destroy callbacks is idempotent. We might end up adding repeated callbacks each tiem we reload the app on dev envs.