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

Revert "Merge pull request #48527 from ghiculescu/active-record-enum-id" #48534

Merged
merged 1 commit into from
Jun 20, 2023

Conversation

skipkayhil
Copy link
Member

Motivation / Background

Railties tests have been failing since this change. The issue is that calling primary_key as the model is loaded requires either a connection to the database or a populated schema cache. This becomes an issue when an app loads models that do not have underlying tables, as shown in the failing Railties tests.

When eager loading an app using rails/all, ActionMailbox::InboundEmail will be loaded whether or not rails g action_mailbox:install has been run. This means the primary_key for InboundEmail will not be in the schema cache and a database connection will be required to boot the app.

Detail

This reverts commit 8b36095, reversing changes made to e05245d.

Additional information

Example failures:

Ref #48524
Ref #48527

cc @ghiculescu @guilleiguaran

…enum-id"

This reverts commit 8b36095, reversing
changes made to e05245d.

Railties tests have been failing since this change. The issue is that
calling `primary_key` as the model is loaded requires either a
connection to the database or a populated schema cache. This becomes an
issue when an app loads models that do not have underlying tables, as
shown in the failing Railties tests.

When eager loading an app using `rails/all`,
`ActionMailbox::InboundEmail` will be loaded whether or not `rails g
action_mailbox:install` has been run. This means the `primary_key` for
`InboundEmail` will not be in the schema cache and a database connection
will be required to boot the app.
@rafaelfranca
Copy link
Member

Why is ActionMailbox::InboundEmail being loaded at boot?

@skipkayhil
Copy link
Member Author

skipkayhil commented Jun 20, 2023

Why is ActionMailbox::InboundEmail being loaded at boot?

All models in engines using app/ are eager loaded at boot along with the app's models because they share the app's loader.

(kind of related to the conversation we were having about Engines potentially needing their own loader so we can remove the dummy apps... although I think even if engines had their own loader that still wouldn't separate whether the migration for those models has been run in an app)

@rafaelfranca
Copy link
Member

Ah ok. I thought this was special for that model, but it is really all models that use enum but it happens we could replicate with our own model.

@guilleiguaran guilleiguaran merged commit 6499ccb into rails:main Jun 20, 2023
8 of 9 checks passed
@ghiculescu
Copy link
Member

Ahhh sorry I saw those tests but it didn't connect that they were related to this. Let me give this another try.

@ghiculescu
Copy link
Member

The original version of my PR checked for method_name.to_sym == :id. That didn't cause these issues, but it's not as comprehensive a fix. Given the root cause of the issue was a call to #id, I think it's probably okay to go with that...

@ghiculescu
Copy link
Member

#48536

@skipkayhil skipkayhil deleted the hm-revert-48527 branch June 21, 2023 00:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants