Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

AS::Dependencies.load_missing_constant should return the constant if it is already loaded #1422

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
5 participants
Contributor

spohlenz commented May 31, 2011

https://github.com/spohlenz/AS-autoload contains a demonstration of the problem that this commit fixes.

This issue was uncovered because of the way the inherited_resources gem autodetects constants based on the controller name, but I don't believe that inherited_resources is doing anything wrong here.

This commit also rids ActiveSupport of the confusing "X is not missing constant Y" error.

+1. Change looks good to me, tested on a fairly large application and it didn't cause any strife.

Owner

pixeltrix commented May 31, 2011

Owner

pixeltrix commented May 31, 2011

The reason it wasn't changed is that it would change the behaviour of constant lookup - Blog::Admin::Post would return Blog::Post which it doesn't without the presence of AS::Dependencies. Basically inherited_resources needs to rescue ArgumentError as well as NameError, but not NoMethodError which it will at the moment because NoMethodError is a subclass of NameError. Look at how AR::Base.compute_type for how to do it.

Owner

pixeltrix commented Jun 1, 2011

I've made a slight change to load_missing_constant - it now raises NameError instead of ArgumentError but inherited_resources will still need to be updated to search back up the ancestor chain, e.g. Blog::Post as well as Blog::Admin::Post.

@pixeltrix pixeltrix closed this Jun 1, 2011

Unfortunately a mere member of the public such as myself can no longer view https://rails.lighthouseapp.com/projects/8994/tickets/2283, despite having wasted hours of frustrating time with errors related to this issue in 2.3.x, which now seem to still be occurring in 3.0.7! With namespaced models all sorts of trouble occurs with autoloading/unloading/reloading in dev mode, forcing restarts of rails console or server sessions for each iteration. Might as well have to link and compile between changes!

So, please illuminate how to access the existing lighthouse tickets (interestingly #2283 was not imported to github--maybe all of the tickets should be imported even if they were supposedly closed?) and what the current solutions are to fix namespaced model loading/unloading/reloading in dev mode.

Owner

pixeltrix commented Jun 7, 2011

I'm using namespaced models and have no problems with loading/unloading/autoloading. Please give a specific example of where it's failing and I will look at it. The problem here was that AS::Dependencies was raising a misleading error - it should raise NameError instead of ArgumentError and that's what has been fixed.

One source of problems is when declaring a namespace in a single line like:

class Blog::Post < ActiveRecord::Base
end

this can cause problems because constant lookup behaves differently that when declaring it like this:

module Blog
  class Post < ActiveRecord::Base
  end
end

Always try and use the latter if possible and especially if you see errors like:

toplevel constant XX referenced by YY::XX

I had a look at whether this could be fixed however there was no way to intercept the constant lookup.

Thanks for the quick response. Yes indeed that is one problem of using the fully qualified class name rather than explicitly nesting within modules, although the enclosing module has always been previously declared before encountering the FQCN which then appear in top-level scope.

I can make these edits, but from my limited vantage point I cannot see why this should make any difference to any code that intercepts constant lookups. If ruby allows the syntax, Rails/AS should not interfere with it. Also I'm trying to remember why this exists at all--something to do with scoping of association declarations? This reeks of code to be deleted from the framework in favor of what ruby provides intrinsically or at least something with fewer side effects.

I'll make the changes to our code and see if I can give you a further concrete example. All I know is that installing the patches from lighthouse #2283 fixed the problem in 2.3.x so I was about to look and see if the same patches could be applied to 3.0.x. Importing that ticket and all related/referenced/referencing tickets would probably save future grief.

Actually what we're seeing a lot of is:
On the first request after starting the dev server, the page renders correctly. On subsequent requests we get e.g.:
Expected /Users/xxx/dev/xxx/app/models/xx/location.rb to define XX::Location

and in /Users/xxx/dev/xxx/app/models/xx/location.rb:

require 'xx/location/location'
require 'xx/location/geoip_geocoder'
  ... additional declarations...

and in /Users/xxx/dev/xxx/app/models/xx/location/location.rb:

require 'geokit'
require 'xx/legacy/location'

module XX
 class Location < Model
  include GeoKit::Mappable

  Mappable = GeoKit::Mappable
  LatLng = GeoKit::LatLng
 end
end

and in config/application.rb we have:
config.autoload_paths += Dir["#{config.root}/app/models/**/"]

So, it is complaining because of what exactly that ruby by itself would have no problem with?

Having then moved the declaration of XX:Location from app/models/xx/location/location.rb to app/models/xx/location.rb we're still then seeing:
Expected /Users/xxx/dev/xxx/app/models/xx/location/geoip_geocoder.rb to define GeoipGeocoder
where in app/models/xx/location/geoip_geocoder.rb we have:

require 'geokit/geocoders'
module XX
 class Location

  class GeoipGeocoder < GeoKit::Geocoders::Geocoder
      ...
  end
 end
end

ruby would not have a problem with this--why does Rails/AS create a problem? Hours wasted on this brah.

Also the full trace for that "Expected (file where foo is defined) to define foo" is:

activesupport (3.0.7) lib/active_support/dependencies.rb:492:in `load_missing_constant'
activesupport (3.0.7) lib/active_support/dependencies.rb:183:in `block in const_missing'
activesupport (3.0.7) lib/active_support/dependencies.rb:181:in `each'
activesupport (3.0.7) lib/active_support/dependencies.rb:181:in `const_missing'
Contributor

thedarkone commented Jun 7, 2011

Could you try replacing require calls with require_dependency? You can't use require for paths that are supposed to be autoloaded/reloaded by Rails in development mode.

Owner

pixeltrix commented Jun 7, 2011

You'll also want to lose the config.autoload_paths += Dir["#{config.root}/app/models/**/"] - that's probably the cause of the errors you're seeing as you've added every sub-directory of app/models to the load path.

If you want to use autoloading it's best to stick with a folder hierarchy that matches the constants. If you've got a bunch of code that's doesn't need reloading then stick it in #{config.root}/lib and require it explicitly - you can use autoload :Constant, 'path/to/file' to speed up boot times.

Owner

pixeltrix commented Jun 7, 2011

The call to require 'geokit/geocoders' doesn't need to be require_dependency if you're loading the code from the Geokit gem - you don't want that to be unloaded on every request.

@thedar changing the nested class require to require_dependency does make this example work--thanks! However, why couldn't we eliminate this explicit require altogether and have the nested classes autoload from app/models/** with that Rails magic?

@pixeltrix 1) the folder hierarchy does match the class/module nesting. That is unless Rails is doing something brilliant like converting the enclosing module named XX (caps) in folder xx (lowercase) to Xx (mixed case) instead of XX ?

  1. numerous blog posts (fwiw) advise adding all model subdirs to the autoload_paths in config/application.rb as we've done. Are you implying that all subdirs of app/models (or of app) are already autoloaded without explicitly adding to the path? If not, how would nested classes of models living in subdirs be autoloaded?

  2. replacing the require 'xx/location/geoip_geocoders' (i.e. the nested class require from the enclosing class) to autoload XX::Location::GeoipGeocoders does not work and yields a new error,

Routing Error You tried to call GeoipGeocoder#respond_to?, but GeoipGeocoder is not defined on /Users/xxx/dev/xxx/app/models/xx/location.rb:432:in<top (required)>'`

  1. there's no problem with a hard require for gems that are truly 'external' to the app. However, now with Bundler especially in dev mode with local source paths, it is common to want to reload the source for a gem which is being co-developed with the app. Thoughts on how to do this without cluttering the source? Would require_dependency work if the gem is then packaged as an external gem in production?
Contributor

thedarkone commented Jun 7, 2011

@tribalvibes that's what Rails does by default. I suggest you create a small empty app and try adding namespaced models I'm pretty sure it will work out of the box as long as you follow the Rails naming conventions.

  1. Rails is following its conventions here, fire up the console and try running few experiments like this 'Abc::Xxx'.underscore or 'abc/xxx'.camelize.

  2. Don't do that, it should work correctly out of the box.

  3. Module.autoload isn't going to work with Rails' constant reloading in dev. mode as far as I know.

  4. Reloading gems in dev. mode via Rails's ActiveSupport::Dependencies is technically possible, but you'd have to be pretty intimate with its internals and configuration. You might try reading its source code.

Owner

pixeltrix commented Jun 7, 2011

  1. Yes - require 'xx/location/geoip_geocoders' will expect to define Xx::Location::GeoipGeocoder:
>> "xx".classify
=> "Xx"
  1. If your hierarchy matches the class/module nesting then AS::Dependencies will walk the hierarchy so you're just adding more locations to search.

  2. autoload :Class, 'path/to/file' is for non-reloadable code that you want to defer loading until referenced - don't use it for model code.

  3. You can always use unloadable to mark a class/module as reloadable - but you wouldn't do that with a released gem. If the gem is an engine then use a relative path spec in your Gemfile and it will reload in development. require_dependency would work for the first file but any requires in the gem code would revert to non-reloadable

Anyway we're straying off the point - this is the Rails issue tracker not Rails-Talk. If there's a specific, reproducible example where you think it isn't reloading properly or not finding a constant properly then please open a new issue with ideally a failing test or at least a link to a Github repo that exhibits the problem.

Thanks, that's clarifying, except that #2 doesn't work. If I remove the require_dependency 'xx/location/geoip_geocoders' it does not find XX::Location::GeoipGeocoders although it is properly nested and located. It gives the error:

You tried to call GeoipGeocoder#geocode, but GeoipGeocoder is not defined on /Users/xxx/dev/xxx/app/models/xx/location/location.rb:267:ingeoip'`

where it is referenced. Does AS::Dependencies walk the folder tree only from the referencing __FILE__'s dir or does it start with app/models?

Unless it's the problem of #1, in which case how do I tell Rails that the enclosing top-level module is really named XX and not Xx ?

It may be off-topic, but hopefully definitive answers will help others hitting the same problems over and over again. Also could someone import lighthouse 2283 to github, and/or how could I gain read access to the old lighthouse tickets (other than goog cache?)

I will try to extract a gist that demonstrates the problem, but it is exactly the situation we are discussing--Rails is not autoloading the tertiary nested model classes.

Owner

pixeltrix commented Jun 7, 2011

Looking at XX::Location::GeoipGeocoders and the error message it seems you still don't have the files in the right place - it needs to be:

XX                          -> 'app/models/xx.rb'
XX::Location                -> 'app/models/xx/location.rb'
XX::Location::GeoipGeocoder -> 'app/models/xx/location/geoip_geocoder.rb'

@pixeltrix Thanks, but nope that's not the problem. It works regardless of whether XX:Location is defined in app/models/xx/location.rb or app/models/xx/location/location.rb as we would prefer, but only if require_dependency 'xx/location/geoip_geocoder' is present. Without the require_dependency it does not work even if Location is defined in app/models/xx/location.rb rather than xx/location/location.rb. Also note there is no app/models/xx.rb as the top-level module is merely an empty namespace.

I'll extract a bare example this eve.

@pixeltrix OK here ya go brah. I was able to reproduce in vitro.
https://github.com/tribalvibes/autoload_test

Minimal example that clearly demonstrates that the nested class is not being autoloaded out of the box. See the README.

Updated to 3.0.8, same behavior. Let me know if I should open a new issue for this. Also please answer regarding access to old lighthouse tickets, and also please import lighthouse 2283 to github.

@pixeltrix ah, sorry, my bad. That helped find the problem though--another component (mixin) was overriding const_missing. ugh.

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