search

rails / solid_queue

Database-backed Active Job backend
MIT License
1.95k stars 130 forks source link

Fix: Replace static table names in schema definition #354

Closed m2meier closed 1 month ago

m2meier commented 2 months ago

The schema definition in db/queue_schema.rb uses static plural table names. If rails runs with config.active_record.pluralize_table_names = false, this leads to an error as soon as solid_queue tries to access the DB.

The static strings for the table names in the schema definition are replaced by the table_name method of the corresponding model class. This ensures that the tables are named correctly in plural or singular respectively.

rosa commented 2 months ago

Huh, I didn't know about config.active_record.pluralize_table_names!

@m2meier, do you run into the same problems for Active Storage, Action Text, Action Mailbox...?

m2meier commented 2 months ago

@m2meier, do you run into the same problems for Active Storage, Action Text, Action Mailbox...?

Same problem for ActiveStorage. My pull request: https://github.com/rails/rails/pull/52970. On ActionText and ActionMailbox I'm currently investigating.

Update: ActionText and ActionMailbox have their table names fixed to plural, ignoring config.active_record.pluralize_table_names.

rosa commented 2 months ago

I wonder why this started failing after Rails 7.1, though 😕 Did Rails handle this automatically in the past? I see the pluralize_table_names config option has existed for way longer. Maybe this is a regression of some sort?

m2meier commented 1 month ago

I wonder why this started failing after Rails 7.1, though 😕 Did Rails handle this automatically in the past? I see the pluralize_table_names config option has existed for way longer. Maybe this is a regression of some sort?

The option already existed when I started with Rails (v5 or earlier). But before the introduction of ActiveStorage, there were no tables that Rails created itself. And only since v7.1 does pluralize_table_names lead to a problem with these tables.

rosa commented 1 month ago

And only since v7.1 does pluralize_table_names lead to a problem with these tables.

Right! So, this must be a regression of some sort. I think that's what should be addressed instead of working around it here or in the other core frameworks 🤔

m2meier commented 1 month ago

Right! So, this must be a regression of some sort. I think that's what should be addressed instead of working around it here or in the other core frameworks 🤔

IMHO that's not a regression. Rather, the core frameworks now correctly comply with the configuration for table names and therefore static strings in (generated) migrations must be replaced by the correct, “calculated” table names. But who am I to hold a strong opinion here. 😉 I will simply cancel my pull request. Never mind.