How to handle two-way dependencies in modularized codebase using `belongs_to` / `has_many` and ActiveSupport hooks

System Design
61

Message originally sent by slack user U723PSM8PD2

Apologies, thank you for the correction to my terminology.

62

Message excluded from import.

63

Message excluded from import.

64

<@U71102MDOF5> I’ll try to take a look over the weekend and get you an answer.

65

Message excluded from import.

66

Thanks for the link to that talk, <@U71102MDOF5>. Very interesting.

In order to test that when a company is deleted, an employee is deleted, one would have to create a company and delete that company.
Yes. this being an integration test, the test needs to know both parties. Just like in Scott’s example in that talk the test needs to ask the http_client spy if it was posted or not, in this case you’d need to ask the Employee model if the companies’ employees got deleted or not. You’re testing the side-effects of doing something.

That there isn’t actually as much of a partition (or boundary) in place as was intended.
Well I guess the question is “what boundary is intended?” In my proposed code above, my intention is for the companies pack to have no knowledge about nor dependency to the employees pack. And I think that’s achieved. The fact that it supports a sort-of PubSub interface to notify others about some specific action doesn’t violate that, IMHO. And my other (very explicit) intention is to say the employees pack DOES have a dependency to the companies pack. And based on that, I’d expect to have to include integration tests on that same pack to test how specific actions on my dependencies affect my own objects.

But I would certainly love to be able to see what you’re trying to show me.

Just to put some perspective on my background: yes, I took the “rails pill” back in 2003/4 when it was released. I learnt Ruby since a bit before that, though, in my quest for a “modern Smalltalk”. I remember back in those days how I felt I was having to ignore a lot of my learnings from reading the “gang of four”'s Design Patterns book (among other things) and DBMS’s good practices like composite keys, and foreign keys and… in exchange for the conveniences Rails gave me. It hurt big time! But OMG how happy I was I wasn’t having to deal with Java/JBoss/Hibernate and what not anymore :sweat_smile:. So I’m actually very happy to learn about “better” (always subjective) ways to use Rails.

But regardless of how much I get to “unlearn” the rails way to adopt better OO patterns and what not… I know I can’t (and shouldn’t) rebuild a system running in production from scratch. So if I find a way to gradually make changes to our code to make it more maintainable, while still dedicating most of our small team’s time into fixing bugs and adding new features (thus, satisfying our stakeholders), then I’ll take that path even if it’s not the best code possible. It’s enough for it to be “better than before”.

Back to the company/employee code example we’ve been discussing… Here’s an alternative I think you guys could potentially like more:

# engines/companies/app/models/company.rb
class Company < ApplicationRecord
end

# engines/employees/app/models/employee.rb
class Employee < ApplicationRecord
  belongs_to :company
end

# engines/admin_or_whatever/lib/commands/destroy_company.rb
# This is in a pack that depends on both companies and employees.
class DestroyCompany
  def initialize(company)
    @company = company
  end

  def call
    ActiveRecord::Base.transaction do
      Employee.where(company_id: @company).destroy_all
      @company.destroy!
    end
  end
end

This alternative uses a specialized object to delete the company and the employees, and the AR classes are used only for interacting wit the DB. Is it better? Well, IMHO, it depends on what you’re looking for. This solution also opens the door for someone to delete a company using the Company class directly, without doing it through the DestroyCompany class. And in that situation I’d end up potentially having inconsistent data in the DB which, IMO, leads to the worst-cases of and hardest to find bugs. Personally, I would make sure the foreign keys are properly set in the DB and stuff like that to avoid data issues from having a chance to happen, but many people don’t do that either.

So as I said in one of my previous comments, this kind of thing can be made at many different levels of the abstraction stack. I think that, for most of the cases, I’d still go with the approach of using the before_destroy callback.

67

Message excluded from import.

68

Message excluded from import.

69

Fair enough.

70

Message excluded from import.

71

True

72

I also feel like we have taken the Employee/Company example too far. Those names don’t necessarily represent everything we’ve been discussing.

73

Message excluded from import.

74

Message excluded from import.

75

Message excluded from import.

76

Message originally sent by slack user U71N3KA8TMX

This is a bit of a thread necro, but I just wanted to say thanks to y’all in this thread for the discussion, I thought it was great and I have shared it with some colleagues who are working on cleaning up our Rails monolith :+1::skin-tone-2:

77

Message excluded from import.