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

autoload_paths not considering namespace? #8726

Closed
rodrigues opened this issue Jan 3, 2013 · 15 comments
Closed

autoload_paths not considering namespace? #8726

rodrigues opened this issue Jan 3, 2013 · 15 comments

Comments

@rodrigues
Copy link

In app/services, I have some classes, as Notification::Finder and Notification::Builder.

They are placed as app/services/notification/builder.rb and app/services/notification/finder.rb.

There is also the Notification class as a model, at app/models/notification.rb

The autoload_path is configurated as in config.autoload_paths += %W(#{config.root}/app/services)

When I try to load Finder, it works:

Loading development environment (Rails 3.2.9)
[1] pry(main)> Notification::Finder
=> Notification::Finder

But when I try the Builder, I get a problem with the rails autoloading:

Loading development environment (Rails 3.2.9)
[1] pry(main)> Notification::Builder
=> ActiveRecord::Associations::Builder

It just ignores the namespace I’ve used when the constant name (Builder) has already been defined by other namespace, and gets the ActiveRecord::Associations::Builder instead.

Is this the expected behavior, or a rails bug?

Going more detailed, the const_missing method at activesupport/dependencies.rb receives a const_name ‘Builder’, and nesting.inspect => ‘nil’.

Curious that when I use constantize, it resolves as expected:

Loading development environment (Rails 3.2.9)
[1] pry(main)> 'Notification::Builder'.constantize
=> Notification::Builder

I've created a project just with these classes for testing: https://github.com/rodrigues/autoload_zomg

The question was also opened at SO: http://stackoverflow.com/questions/14143318/autoload-paths-not-aware-of-namespace

@robin850
Copy link
Member

robin850 commented Jan 3, 2013

I think it's a conflict because builder it's in some way a reserved word. Rails uses it in the background. Can a team member or a good rails developer confirm this? (Or confirm that I'm saying bullshit)

@jiripospisil
Copy link
Contributor

Note that the behavior is different on the current master: https://gist.github.com/4445870

@senny
Copy link
Member

senny commented Jan 3, 2013

This problem exists because you are using an ActiveRecord model as a namespace. I created a gist with some experimentation until I saw the root cause.

ActiveRecord models include the ActiveRecord::Associations module. Since you can get to a constant when including a module the Builder constant defined within Associations is now also reachable through the AR model. You will get this behavior with every class defined in the modules, which are included into an AR model:

1.9.3-p194 :010 > Post.ancestors
=> [Post(id: integer, title: string, published_at: datetime, created_at: datetime, updated_at: datetime), Post::GeneratedFeatureMethods, #<Module:0x007fec74dc33a0>, ActiveRecord::Base, ActiveRecord::Core, ActiveRecord::Store, ActiveRecord::Serialization, ActiveModel::Serializers::Xml, ActiveModel::Serializers::JSON, ActiveModel::Serialization, ActiveRecord::Reflection, ActiveRecord::Transactions, ActiveRecord::Aggregations, ActiveRecord::NestedAttributes, ActiveRecord::AutosaveAssociation, ActiveModel::SecurePassword, ActiveRecord::Associations, ActiveRecord::Timestamp, ActiveModel::Validations::Callbacks, ActiveRecord::Callbacks, ActiveRecord::AttributeMethods::Serialization, ActiveRecord::AttributeMethods::Dirty, ActiveModel::Dirty, ActiveRecord::AttributeMethods::TimeZoneConversion, ActiveRecord::AttributeMethods::PrimaryKey, ActiveRecord::AttributeMethods::Query, ActiveRecord::AttributeMethods::BeforeTypeCast, ActiveRecord::AttributeMethods::Write, ActiveRecord::AttributeMethods::Read, ActiveRecord::AttributeMethods, ActiveModel::AttributeMethods, ActiveRecord::Locking::Pessimistic, ActiveRecord::Locking::Optimistic, ActiveRecord::CounterCache, ActiveRecord::Validations, ActiveModel::Validations::HelperMethods, ActiveSupport::Callbacks, ActiveModel::Validations, ActiveRecord::Integration, ActiveModel::Conversion, ActiveRecord::AttributeAssignment, ActiveModel::ForbiddenAttributesProtection, ActiveModel::DeprecatedMassAssignmentSecurity, ActiveRecord::Sanitization, ActiveRecord::Scoping::Named, ActiveRecord::Scoping::Default, ActiveRecord::Scoping, ActiveRecord::Inheritance, ActiveRecord::ModelSchema, ActiveRecord::ReadonlyAttributes, ActiveRecord::Persistence, Object, PP::ObjectMixin, ActiveSupport::Dependencies::Loadable, V8::Conversion::Object, JSON::Ext::Generator::GeneratorMethods::Object, Kernel, BasicObject] 

A possible solution is to use a module as a namespace. For example module Notifications. I guess the conclusion from this ticket is not to use AR models as namespaces 😄

@senny
Copy link
Member

senny commented Jan 3, 2013

This gist illustrates the situation https://gist.github.com/4446037

@rafaelfranca
Copy link
Member

cc @fxn

@rodrigues
Copy link
Author

Yeah @senny, I've realized that was the problem :)

I submitted this issue because I don't know if this is the intended behavior for this situation.

There are a lot of ancestors in this and other cases, and very often they have names that can be meaningful in the app context, and couldn't be used unless you require the classes before, as in require 'notification/builder'.

If there is a way to get the "right" Builder without being too expensive for the autoload, I think it would be the best behavior.

Let's see what you guys think ;)

@senny
Copy link
Member

senny commented Jan 3, 2013

Some of our applications grew pretty big and we used a lot of namespaces. As stated before, I would not suggest to use an AR model as a namespace though. I'd create a module Notifiactions for example and move everything in there. Most of the time we also move the "root" model into that namespace. (eg. Notifications::Notifcation). This loading behavior is not the only drawback when using AR models as namespaces. You also need to specify the superclass of Notification everywhere, which forces you to load ActiveRecord.

I don't see this as a bug but let's see what @fxn thinks.

@fxn
Copy link
Member

fxn commented Jan 14, 2013

Yep, that is just plain Ruby. Even if you use a qualified constant name, a constant path, the ancestors of the parent class/module are scanned looking for the constant. Since Ruby finds the constant Active Support is not even called.

In general it is safer to use bare modules as namespaces, because you inherit less things. But your example can still work if you want by loading notifications/builder with require_dependency. If the file is interpreted Ruby will find the constant you expect.

Note that there is no need to put app/services in autoload_paths, any directory below app is added automatically.

@fxn fxn closed this as completed Jan 14, 2013
@divoxx
Copy link
Contributor

divoxx commented May 31, 2013

I'd like to reopen this because it's not a Ruby thing. It is a Rails autoload problem. In Ruby, a qualified constant name will always return that constant and not return a different one.

module Bar
  module Baz
  end
end

class Foo
  include Bar::Baz
end

class Foo
  class Baz
  end
end

Foo::Baz # => Foo::Baz (it will never return Bar::Baz)

Or if you actually have clashing classes from within the same namespace, ruby will let you know.

module Bar
end

class Foo
  include Bar
end

class Foo
  class Bar # TypeError: wrong argument type Class (expected Module)
  end
end

I'm unfamiliar with the internals of autoloading/reloading so I can't tell what and how it does it but its very clear it's not a ruby issue nor expected.

We've been bitten by this as well:

[1] pry(main)> Foo::Read
=> ActiveRecord::AttributeMethods::Read

@fxn
Copy link
Member

fxn commented May 31, 2013

The problem of this issue is the following: let's suppose you have loaded in memory this:

module M
  X = 1
end

class C
  include M
end

class User < C
  p X
end

If you have a file called x.rb in autoload_paths with a top-level X defined, that code won't trigger autoloading of that file because Ruby resolves the X constant in one of the ancestors of User. Rails autoloading is only triggered if the constant is unknown, if the constant is resolved by Ruby AS autoloading does not get a chance to run.

@divoxx
Copy link
Contributor

divoxx commented May 31, 2013

Ok, I see what you mean. Adjusting my own example to actually reproduce the issue:

module Bar
  module Baz
  end
end

class Foo
  include Bar
end

Foo::Baz # => Bar::Baz

I guess its just another inherent issue of using inheritance/modules. Can this be documented somewhere though? It's time consuming and confusing to deal with problems like these, would be nice to educate people about this.

@fxn
Copy link
Member

fxn commented May 31, 2013

Exactly, that's the point.

Don't know if there's anything to document here. On one hand a Ruby programmer should know how constants work, and in the case of Active Record you have

ActiveRecord::Base.constants.sort
 => [:ACTIONS, :ATTRIBUTE_TYPES_CACHED_BY_DEFAULT, :AbsenceValidator, :AcceptanceValidator,
:AggregateReflection, :AliasTracker, :AssociatedValidator, :Association, :AssociationBuilderExtension, 
:AssociationReflection, :AssociationScope, :Attribute, :BeforeTypeCast, :Behavior, :BelongsToAssociation, 
:BelongsToPolymorphicAssociation, :Builder, :CALLBACKS, :CALLBACK_FILTER_TYPES, 
:CALL_COMPILABLE_REGEXP, :Callback, :CallbackChain, :Callbacks, :ClassMethods, :Clusivity, 
:CollectionAssociation, :CollectionProxy, :ConfirmationValidator, :Default, :Dirty, :ExclusionValidator, :FormatValidator, 
:HasAndBelongsToManyAssociation, :HasManyAssociation, :HasManyThroughAssociation, :HasOneAssociation, 
:HasOneThroughAssociation, :HelperMethods, :InclusionValidator, :IndifferentCoder, :InstanceMethodsOnActivation, 
:JoinDependency, :JoinHelper, :LengthValidator, :MacroReflection, :MultiparameterAttribute, 
:NAME_COMPILABLE_REGEXP, :Named, :NumericalityValidator, :OrmAdapter, :Preloader, :PresenceValidator, 
:PrimaryKey, :Query, :Read, :ScopeRegistry, :Serialization, :Serializer, :SingularAssociation, :ThroughAssociation, 
:ThroughReflection, :TimeZoneConversion, :TooManyRecords, :TransactionError, :Type, :UNASSIGNABLE_KEYS, 
:UniquenessValidator, :WithValidator, :Write]

So you know that Read is going to be resolved there. Well maybe you find by accident as you did, but you eventually do.

It could be argued, though, that precisely because of this the constants you inject in the ancestor chain belong in some sense to the public interface no matter whether their modules implement something internal. Meaning, a new constant may break existing applications.

@divoxx
Copy link
Contributor

divoxx commented Jun 1, 2013

Right, but what I'm saying is just to add a notice somewhere in the docs so people can prevent those kind of problems and explaining the issue.

If you don't know Rails' internals, it's hard to see what is going on, specially because you don't really know everything that is being included in your models through inheritance, unless you go and check. Anyway, just a thought to help other people to prevent from this kind of surprises.

@joaohornburg
Copy link

👍 this bug has bitten me more than once

@fxn
Copy link
Member

fxn commented Aug 3, 2013

@joaohornburg s/bug/gotcha/ it is not really a bug.

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

No branches or pull requests

8 participants