bullet-train-co / bullet_train-core

The Open Source Ruby on Rails SaaS Framework
42 stars 46 forks source link

`has_many` association gets scaffolded improperly with `join-model` scaffolder #658

Open gazayas opened 1 year ago

gazayas commented 1 year ago
bin/super-scaffold crud News::Article Team title:text_field
bin/super-scaffold crud Client Team title:text_field
bin/super-scaffold join-model Clients::NewsArticle news_article_id{class_name=News::Article} client_id{class_name=Client}
bin/super-scaffold crud-field News::Article client_ids:super_select{class_name=Clients::NewsArticle}

This scaffolds the following to client.rb.

has_many :news_articles, class_name: "Clients::NewsArticle", dependent: :destroy
has_many :news_articles, through: :news_articles

It should be this.

has_many :clients_news_articles, class_name: "Clients::NewsArticle", dependent: :destroy
has_many :news_articles, through: :clients_news_articles

We can add News::Article records properly, but trying to access Clients::NewsArticle via the association yields a stack level too deep error.

> rails c
Loading development environment (Rails 7.0.8)
irb(main):001> Client.first.news_articles
  Client Load (0.5ms)  SELECT "clients".* FROM "clients" ORDER BY "clients"."id" ASC LIMIT $1  [["LIMIT", 1]]
/home/gazayas/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/activerecord-7.0.8/lib/active_record/reflection.rb:838:in `through_reflection': stack level too deep (SystemStackError)
gazayas commented 1 year ago

This one might be a bit tough to solve.

https://github.com/bullet-train-co/bullet_train-core/blob/cdccbdd270c62592458447e9ee0af5550fdb781c/bullet_train-super_scaffolding/lib/scaffolding/transformer.rb#L581-L582

I can fix the problem specifically for the model set up in this issue by changing this line to the following (by adding scaffolding_).

has_many_line = ["has_many :scaffolding_completely_concrete_tangible_things"]

However, this breaks our super scaffolding system test.

It looks like this is occurring due to a namespace overlap issue. For example, we have this in the Super Scaffolding system test setup

https://github.com/bullet-train-co/bullet_train/blob/0d69fc08d46e99b31876c8d8e9e5de3bb39f33fc/test/bin/setup-super-scaffolding-system-test#L20-L21

The has_many association gets scaffolded in the following way:

# ✅ This doesn't give us the extra namespace.
"has_many :completely_concrete_tangible_things"
has_many :deliverables

# 🚫 This gives us the whole namespace when we only need `deliverables`
"has_many :scaffolding_completely_concrete_tangible_things"
has_many :projects_deliverables

So I think we'll have to do something similar to what's here:

https://github.com/bullet-train-co/bullet_train-core/blob/cdccbdd270c62592458447e9ee0af5550fdb781c/bullet_train-super_scaffolding/lib/bullet_train/super_scaffolding/scaffolders/crud_scaffolder.rb#L45-L57

gazayas commented 1 year ago

Now that I've sifted through this a bit, I'm not sure this is a bug. The way we are scaffolding the has_many lines is technically correct. Here's another look.

# Because we're already in the Client namespace `client.rb`,
# " we pop off "clients_" part at the beginning.
has_many :news_articles, class_name: "Clients::NewsArticle", dependent: :destroy

# Also, has_many :news_articles is correct because the model
# we're referring to is News::Article.
# through: :news_articles is also correct because again,
# we're popping off "clients_" since we're already in the Client namespace.
has_many :news_articles, through: :news_articles

So, by naming the join model differently, I was able to make everything work properly.

bin/super-scaffold crud News::Article Team title:text_field
bin/super-scaffold crud Client Team title:text_field
bin/super-scaffold join-model Clients::TestArticle news_article_id{class_name=News::Article} client_id{class_name=Client}
bin/super-scaffold crud-field News::Article client_ids:super_select{class_name=Clients::TestArticle}

Because of this name overlapping, should we write something in the documentation to avoid this kind of model setup?

gazayas commented 1 year ago

Part of me thinks it's best to simply raise an error telling the developers to reconsider the name of the model they're trying to scaffold if we get something like this:

has_many :model_name, :model_name

I'm not sure if it's safe to assume that this is only a namespace-specific issue, so I think for now that's the best route we can take.