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.

Sign up for GitHub

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

Jump to bottom

Railtie.respond_to_missing? creates an instance unexpectedly #44761

Closed
eregon opened this issue Mar 24, 2022 · 4 comments · Fixed by GulajavaMinistudio/rails#279
Closed

Railtie.respond_to_missing? creates an instance unexpectedly #44761

eregon opened this issue Mar 24, 2022 · 4 comments · Fixed by GulajavaMinistudio/rails#279
Labels
railties

Comments

@eregon
Copy link
Contributor

eregon commented Mar 24, 2022

Steps to reproduce

The code is

rails/railties/lib/rails/railtie.rb

Lines 216 to 218 in ad25dfa

def respond_to_missing?(name, _)
instance.respond_to?(name) || super
end

This can lead to very confusing bugs, because it's not safe to e.g. check respond_to?(:ruby2_keywords, true) on Railtie or any of the subclasses.

So this line in Rails 6.1 actually creates an instance of Railtie, that's very likely unexpected:

rails/railties/lib/rails/railtie.rb

Line 212 in d38ff07

ruby2_keywords(:method_missing) if respond_to?(:ruby2_keywords, true)

https://github.com/rails/rails/blob/6-1-stable/railties/lib/rails/railtie.rb

An example bug caused by this is RSpec for instance can't use respond_to?(:ruby2_keywords, true) when mock'ing ::Rails.application (and so defining methods on the singleton class of the ::Rails.application).
(details in rspec/rspec-mocks#1385 (comment))

In short, this basically breaks respond_to? (or unintentionally creates instances of them) on Railtie and all its subclasses (on the class objects themselves), and so makes it difficult to check if e.g. some method defined is available on the class.

I think ideally Rails should never define respond_to_missing? on modules/classes objects themselves, it should only do so for instances of classes.

A workaround for the specific case of checking if ruby2_keywords is available is to use Module.private_method_defined?(:ruby2_keywords) instead.
@eregon eregon mentioned this issue Mar 24, 2022
Merged
@rafaelfranca
Copy link
Member

I'm not sure there is any other way to do what we want. Railties is using the singleton pattern. There is only one instance and people should not know it exists. If we don't implement respond_to_missing? or respond_to? for that matter they will not be able to know the methods existent in the singleton exists or not. What is the problem of creating a new instance, just because of the mocks?

@rafaelfranca
Copy link
Member

I actually not sure what that Rspec issue is about. Creating an instance of the Railtie only fail if you try to create an instance of an abstract raitie and the Rails application is not an abstract railtie.

class Foo < Rails::Application
end
Foo.respond_to?(:something)
=> false

So if the person is mocking the Rails.application which is not an abstract railtie they should not see any exception, or any non-abstract railtie. Now if they are mocking Rails::Railtie or Rails::Engine directly, I'm not sure there is anything we can do.

@rafaelfranca
Copy link
Member

I end up figuring out what was happening. Fixed.

@GulajavaMinistudio GulajavaMinistudio mentioned this issue Mar 27, 2022
Merged
@eregon
Copy link
Contributor Author

eregon commented Mar 28, 2022 •
edited

Thank you for the fix! That should help avoiding the confusing case.

I think ideally the singleton pattern would be explicit (using SomeClass.instance.foo), that way SomeClass wouldn't mix the behavior of both being a class and an instance. That's likely not easily fixable for Rails if SomeRailtie.method_of_the_instance is used and so would be a compatibility issue.

FWIW, another idea is SomeRailtie could be the instance and the class could be a separate constant (e.g. SomeRailtieClass).

rafaelfranca added a commit that referenced this issue Mar 28, 2022
@rafaelfranca
Make sure respond_to? and method_missing works in abstract Railties
3f79e98 
Those methods were raising an confusing exception when trying to call
`Rails::Railtie.respond_to?(:something)`. Now they just work.

Fixes #44761
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
railties
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Create a new pull request by comparing changes rails/rails
3 participants
@rafaelfranca @eregon @gmcgibbon