rails / rails

Ruby on Rails
https://rubyonrails.org
MIT License
55.63k stars 21.56k forks source link

Bug with loading top-level class if namespaced is defined. #25155

Closed madmax closed 8 years ago

madmax commented 8 years ago

Steps to reproduce

Create 2 files:

app/controllers/concerns/example.rb

module Concerns::Example; end

app/models/example.rb

class Example < ActiveRecord::Base; end

Next start console rails c [1] pry(main)> Example

Expected behavior

We should get Class defined in models/example.rb

Actual behavior

When cache_classes or eager_load = false we will get

[1] pry(main)> Example LoadError: Unable to autoload constant Example, expected app/controllers/concerns/example.rb to define it from rails-3d01d00e348f/activesupport/lib/active_support/dependencies.rb:512:in `load_missing_constant'

When cache_classes and eager_load = true we will get

[1] pry(main)> Example NameError: uninitialized constant Example

System configuration

Rails version: 5.0.0 (3d01d00e348f) 5-0-stable but tested also on cc7bd7cc394d0bbd9ead5763e077c8965c305755 (beta3)

Ruby version: ruby 2.3.0p0

fxn commented 8 years ago

This is not a bug.

The directories called concerns are directly put in autoload_paths (check the guide, on my phone, can't link easily).

This is an exception to the rule that says that subdirectories correspond to namespaces, unfortunately, but it is the way it is. Documented anyway.

madmax commented 8 years ago

@fxn I understand that concerns are in autoload_paths. By why

module Concerns::Example (Namespaced module)

Prevent loading class Example (TOP-LEVEL class)

This works correctly in rails 4.2

fxn commented 8 years ago

Still on my phone.

The conflicting file is called example.rb, therefore a candidate for autoloading the top-level constant Example. Rails leaves unespeficied what happens when there are more than one file with the same name mod autoload dirs. There shouldn't be.

For whatever reason, the one with the module is picked first, Rails detects it doesn't define what it should define, and that is an error condition.

madmax commented 8 years ago

@fxn Then filenames should be unique? Then why we have Namespaces? And what about cache_classes and eager_load = true. Why Example is then uninitialized constant?

I don't believe this is expected behavior. What about namespaced controllers? Like

admin/posts_controller.rb vs posts_controller.rb ?

This also should not work? Case is exactly the same.

Just remember this was working in rails 4.2 :)

fxn commented 8 years ago

Unique mod autoload dirs. That is, not the basename, but the path relative to the autoload dir.

In the situation that motivated the issue, example.rb is repeated because it is a filename that lives directly below two autoload dirs, namely app/controllers/concerns and app/models. That situation has undefined behaviour (the algorithm is deterministic, but Rails does not commit to what it does today). Which one is going to be loaded first? You don't know, so you can't rely on it. I know of no use case where you want to have conflicts so no need to say what to do in case of conflict, the message is to not have conflicts.

The previous example is fine, because relative to the autoload dir they are _postcontroller.rb and _admin/postcontroller.rb. Different.

The main point to be taken from this thread is that, regardless of what may seem, app/controllers/concerns is not introducing a Concerns namespace, concerns is special-cased (the name is hard-coded in the Rails code base) to be also a top-level autoload dir.

This is documented here: http://guides.rubyonrails.org/autoloading_and_reloading_constants.html#autoload-paths.

madmax commented 8 years ago

@fxn This was changed in rails 5 vs rails 4.2 because this quide that you are reffering is for 4.2?

In rails 4.2 I have % rails c Loading development environment (Rails 4.2.5)

[1] pry(main)> Example => Example [2] pry(main)> Concerns::Example => Concerns::Example

Why we then use namespace in Concerns directory? We should not just define module Example not Concerns::Example ? Whats the point of having namespace in Concerns?

fxn commented 8 years ago

No, it has not changed.

In general, our naming conventions say that a subdirectory corresponds to a module or class acting as namespace. Well, this convention was broken with the introduction of the concerns subdirectories, now we have an exception.

Your example works because for whatever reason in 4.2.5 app/models/example.rb is found first, so it loads OK. That is the undefined part of this, it worked by chance, but you cannot rely on it.

Why does Concern::Example work? Because of the unfortunate exception. Since app/controllers is in autoload paths, Rails is able to resolve Concerns::Example loading the relative filename concerns/example.rb.

Do you see the problem introduced with those concerns directories in autoload paths? The file app/controllers/concerns/example.rb maps to two possible constants instead of one. The intended one is Example, but accidentally it would also autoload Concerns::Example.

Bottom line: Do not use Concerns as a namespace, and keep your filenames mod autoload dirs unique.

madmax commented 8 years ago

@fxn thanks for clarification. Can you tell me why concerns directories are treated as root directories? Why not just load them like controllers namespaces? like admin/example_controller.rb ?

fxn commented 8 years ago

@madmax not really, we'd need to ask to the author of the feature. My guess, though, is that it was a directory structure that made sense, plus the fact that experience tells you generally want concerns in the top-level by default as anything else, plus possibly overlooking (my guess) this little departure.

madmax commented 8 years ago

@dhh and @jeremy can you confirm that this is not bug?

dhh commented 8 years ago

Yes, @fxn is right on all points. This is working as intended. Concerns don't have a namespace for the same reason that Controller isn't a namespace. It's a technical grouping that needn't leak into domain language for the modules themselves.

madmax commented 8 years ago

@dhh thanks.

We should then define in controllers/concerns/example.rb

module Example
  extend ActiveSupport::Concern
end

not

module Concerns::Example
  extend ActiveSupport::Concern
end

right?

dhh commented 8 years ago

Correct.

On Fri, May 27, 2016 at 12:49 PM, Grzegorz Derebecki < notifications@github.com> wrote:

@dhh https://github.com/dhh thanks.

We should then define

module Example extend ActiveSupport::Concern end

not

module Concerns::Example extend ActiveSupport::Concern end

right?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/rails/rails/issues/25155#issuecomment-222117216, or mute the thread https://github.com/notifications/unsubscribe/AAAKtWfar-NMw_FBEcZyeb22CfZwgI9qks5qFsw4gaJpZM4In9zD .

fxn commented 8 years ago

@madmax Let me add a detail just in case.

The examples above make me wonder if you think ActiveSupport::Concern and concerns are related (if not, please ignore this comment).

In reality, they are totally orthogonal features that have a coincidence in their name. Conventionally concerns are Ruby modules that play a certain role in your app design, and those modules can be just regular Ruby modules. Concerns become concerns because of their role, there is nothing technical marking them. In particular they are not required to extend ActiveSupport::Concern.

And as with any Ruby module, you may decide that ActiveSupport::Concern is useful for its implementation. But that is something per module, unrelated to whether it acts as a concern.

madmax commented 8 years ago

@fxn I understand, My question was just related to controllers/concerns, models/concerns directories Thanks all for clarification :)

kinnrot commented 7 years ago

Hi guys, what about regular module name (not named concern), is that the expected behavior also? see question

Cause I'm having similar issue now when trying to upgrade.