Ruby on Rails | Screencasts | Download | Documentation | Weblog | Community | Source

Ticket #4221 (closed defect: fixed)

Opened 2 years ago

Last modified 2 years ago

[PATCH] Nested model classes fail with nested associations in certain cases

Reported by: justin@aspect.net Assigned to: ulysses
Priority: high Milestone: 1.1
Component: ActiveSupport Version: 1.1.0 RC1
Severity: normal Keywords: needs_review
Cc:

Description

If you are using a nested model and that model is using an association in that same namespace, AND you are in development mode wherein Dependencies.mechanism is set to :load for automatic reloading, const_missing will fail on the second load of an action calling that association. This is on edge rails.

This is easily illustrated by using acts_as_tree. Calling Resources::Category#children will raise this exception on the second load of an action or view making that call.

The basic flow goes like this:

In the first request, Resources::Category is called and const_missing loads the class properly with require_dependency. Resources (the module) is then called and const_missing defines a new module (line 115 in dependencies.rb) named Resources. Since Resources::Category was called first in this initial load, the problem is not apparent (and thus is also fine in production mode since Dependencies.mechanism is set to :require).

Then, in refreshing the action, the load order changes slightly (not sure why). Resources::Category is called and const_missing loads the class through require_dependency, then Resources (the module again) is called and const_missing again instantiates Resources as a new Module again (overridding what was previously loaded). Then Resources::Category is called again, but since const_missing redefined the Resources module, the previously loaded Resources::Category is no longer available and the NameError on line 106 is raised.

Coincidentally, Ruby does warn that the class is being redefined:

"./script/../config/../vendor/rails/activesupport/lib/active_support/dependencies.rb:115: warning: already initialized constant Resources"

Now, I seem to have solved the problem by preventing the module from being redefined if the class_id is defined (see attached patch).

Additionally, here is the debug output from Dependencies#const_missing that I used to trace the load order for the first and second requests, respectively:

Class ID: Resources, File Name: resources, File Path: resources
Class ID: CategoriesController, File Name: categories_controller, File Path: resources/categories_controller
Class ID: Category, File Name: category, File Path: resources/category
Class ID: Resources, File Name: resources, File Path: resources/resources
Class ID: Resources, File Name: resources, File Path: resources
Class ID: Resource, File Name: resource, File Path: resources/resource
Class ID: CategoriesController, File Name: categories_controller, File Path: resources/categories_controller
Class ID: Category, File Name: category, File Path: resources/category
Class ID: Resources, File Name: resources, File Path: resources/resources
Class ID: Resources, File Name: resources, File Path: resources
Class ID: Category, File Name: category, File Path: resources/category
Class ID: Category, File Name: category, File Path: resources/category

I tried for about 2 hours to write a functional test that could replicate these conditions, but did not succeed. Instead, I did write an app that can replicate the issue (also attached). Hopefully that is enough for someone more knowledgeable than I to write a proper test for this.

Attachments

patch (0.8 kB) - added by justin@aspect.net on 03/14/06 00:34:47.

Change History

03/14/06 00:34:47 changed by justin@aspect.net

  • attachment patch added.

03/14/06 00:38:19 changed by justin@aspect.net

I wasn't able to attach the test project (too large), but it is located at http://www.aspect.net/NestedClassLoadingIssueTest.zip. Just unzip, migrate, and fire up webrick.

03/18/06 21:21:37 changed by david

  • owner changed from David to nseckar@gmail.com.

03/19/06 18:45:39 changed by rick

  • status changed from new to closed.
  • resolution set to invalid.

That patch didn't do anything for me. Moduled models are pretty much broken in rails, and there's not a whole lot of momentum to get them working.

The first issue is the load path. Rails adds subdirectories under app/models to the load path. So if you have:

app/models/foo.rb
app/models/blah/foo.rb

Foo will load blah/foo.rb first, failing if it creates Blah::Foo and not Foo. This can easily be worked around by adding the app/models to the top of the load path in the rails initializer:

config.load_path.shift("#{RAILS_ROOT}/app/models")

Now, let's pretend this all worked. Blah::Foo still uses the table name foos. It would require you to manually set the table name of all the moduled models then.

Suggestion: set up your models like this:

app/models/blah/blah_foo.rb
app/models/foo.rb

BlahFoo uses blah_foos table, and Foo uses foos. If someone enables this, you can happily add the :: in between your model names and go on your merry way.

03/22/06 23:50:24 changed by anonymous

See ticket #4155

03/27/06 05:03:29 changed by ulysses

  • status changed from closed to reopened.
  • resolution deleted.

Reopening as this issue is indicative of deeper trouble than just model wackiness.

03/27/06 05:12:43 changed by anonymous

  • keywords set to needs_review.
  • priority changed from normal to high.
  • version set to 1.1.0 RC1.
  • milestone set to 1.1.

Ticket #4155 (marked fd) refers to this so I'm marking this as needs_review.

03/27/06 05:13:57 changed by ulysses

  • status changed from reopened to closed.
  • resolution set to fixed.

(In [4060]) Dependencies cleanup. Fixes #4221.

07/24/06 03:10:41 changed by anonymous

boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke boke