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 loading all Tasks with namespace collision #939

Merged
merged 1 commit into from
Dec 20, 2023

Conversation

skipkayhil
Copy link
Contributor

After upgrading from Rails 7 to 7.1, we started seeing an error when trying to load the Maintenance Task UI:

undefined method `instance_of?` for #<BasicObject:... >

We ended up identifying the BasicObject as PRIMARY_KEY_NOT_SET, which was added to Active Record in 7.1. The reason for attempting to load this constant was that we have a model in our app named Maintenance which inherits from ActiveRecord::Base, and so all of the constants below ActiveRecord::Base get loaded by load_constants. (This clearly isn't a common case, nor something I would recommend other applications do).

To fix this, we can use the Rails application's loader to load all of the application's tasks instead of traversing the constant tree. This approach has multiple benefits:

  • it will only load constants managed by the application's loader (which is the intention anyway)
  • load_constants will be faster in production because the loader will recognize that the namespace has already been loaded and so will not have to traverse the tree

Since #eager_load_namespace was introduced in Zeitwerk 2.6.2 but the minimum required by Rails is 2.6, the higher minimum version was added to the gemspec as well.

After upgrading from Rails 7 to 7.1, we started seeing an error when
trying to load the Maintenance Task UI:

```
undefined method `instance_of?` for #<BasicObject:... >
```

We ended up identifying the BasicObject as PRIMARY_KEY_NOT_SET, which
was added to Active Record in 7.1. The reason for attempting to load
this constant was that we have a model in our app named Maintenance
which inherits from ActiveRecord::Base, and so all of the constants
below ActiveRecord::Base get loaded by load_constants. (This clearly
isn't a common case, nor something I would recommend other applications
do).

To fix this, we can use the Rails application's loader to load all of
the application's tasks instead of traversing the constant tree. This
approach has multiple benefits:
- it will only load constants managed by the application's loader (which
  is the intention anyway)
- load_constants will be faster in production because the loader will
  recognize that the namespace has already been loaded and so will not
  have to traverse the tree

Since `#eager_load_namespace` was introduced in Zeitwerk 2.6.2 but the
minimum required by Rails is 2.6, the higher minimum version was added
to the gemspec as well.
Copy link
Member

@etiennebarrie etiennebarrie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome thanks!

@etiennebarrie etiennebarrie merged commit dfcb137 into Shopify:main Dec 20, 2023
17 checks passed
@skipkayhil skipkayhil deleted the hm-fix-task-loading branch December 20, 2023 19:14
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