Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: Replace static table names in schema definition #354

Closed
wants to merge 3 commits into from

Conversation

m2meier
Copy link

@m2meier m2meier commented Sep 18, 2024

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.

@m2meier m2meier changed the title Replace static table names in schema definition Fix: Replace static table names in schema definition Sep 19, 2024
@rosa
Copy link
Member

rosa commented Sep 22, 2024

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
Copy link
Author

m2meier commented Sep 22, 2024

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

Same problem for ActiveStorage. My pull request: rails/rails#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
Copy link
Member

rosa commented Sep 22, 2024

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
Copy link
Author

m2meier commented Oct 3, 2024

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
Copy link
Member

rosa commented Oct 3, 2024

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
Copy link
Author

m2meier commented Oct 3, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants