This project is archived and is in readonly mode.

#2283 ✓ resolved
Andrew White

Unnecessary exception raised in AS::Dependencies.load_missing_constant

Reported by Andrew White | March 18th, 2009 @ 10:23 AM | in 3.0.2

When load_missing_constant is called with an already defined constant an exception is raised along the lines of "ArgumentError: Admin is not missing constant User!". This seems unnecessarily harsh - why not just return the existing constant? This would have the side effect of fixing a problem with namespaced models in ActiveRecord, e.g:


module Admin
  class User < ActiveRecord::Base
    has_one :account
  end

  class Account < ActiveRecord::Base
    belongs_to :user
  end
end

If both the Admin::User and Admin::Account models have previously been loaded then the compute_type method looks for Admin::User::Account or Admin::Account::User first and doesn't find them and then load_missing_constant gets called. This looks in the parent module and finds them already defined and then the exception gets raised.

The attached patch fixes this problem and modifies the test for double loading. All tests pass with this change on my computer.

Comments and changes to this ticket

  • ekolve

    ekolve April 16th, 2009 @ 07:33 PM

    We are experiencing the same issue from within migrations:

    @@ class FirstMigration < ActiveRecord::Migration

    class Book < ActiveRecord::Base

    belongs_to :author
    
    

    end

    class Author < ActiveRecord::Base end

    def self.up

    Book.create!(:name => 'blah')
    Book.first.author # blows up here with 'FirstMigration' is not missing constant Author!
    
    

    end

    def self.down end end

    @@

  • jmoses

    jmoses April 24th, 2009 @ 02:01 PM

    This is awfully helpful, since this has been driving us nuts.

    In addition, here's the above real patch turned into a monkey patch (via an initializer), for anybody that stumbles onto this issue and doesn't want to (or can't) patch the version of Rails their using.

  • jmoses

    jmoses April 24th, 2009 @ 02:03 PM

    • Tag changed from active_support, constant, dependencies, loading to active_support, constant, dependencies, loading, patched
  • Suraj N. Kurapati

    Suraj N. Kurapati June 5th, 2009 @ 10:10 PM

    Thanks for the monkeypatch & workaround! :)

    I wasted two days trying to debug this error because my Rails plugin absolutely must have namespaced models (so that my plugin's models do not interfere with the the Rails app that is using my plugin).

    I hope this bug is fixed in Rails mainline soon.

  • Pratik

    Pratik July 30th, 2009 @ 05:09 PM

    • Assigned user set to “Jeremy Kemper”
  • Andrew White
  • Jeremy Kemper

    Jeremy Kemper August 5th, 2009 @ 06:53 PM

    • State changed from “new” to “open”

    This is a symptom. Changing load_missing_constant to find sibling constants doesn't match Ruby behavior.

    AR::Base#compute_type should handle this case. It looks for a sibling constant first and falls back to a normal constant lookup. Why is it failing here?

  • Andrew White

    Andrew White August 5th, 2009 @ 09:36 PM

    The problem is that compute_type is looking for a nested constant first (type_name_with_module) and then falling back to normal constant lookup. So in the example above the has_one :account call tries Admin::User::Account and when that fails it then tries Account which uses the normal constant lookup. The Account constant doesn't exist within User so the const_missing defined in AS::Dependencies::ClassConstMissing is called. Since we're still inside a nested constant load_missing_constant is called. This fails and then the parent constant is sent const_missing. In the example above this is the Admin module and the const_missing in ModuleConstMissing calls load_missing_constant again. However if the constant is previously been loaded the exception removed in the patch is triggered.

    Removing the exception seemed the cleanest way of fixing it rather than trying to refactor the const_missing behaviour.

  • Rhett Sutphin

    Rhett Sutphin August 31st, 2009 @ 11:23 PM

    FWIW, I have run into a similar problem. I traced it through AR far enough to determine that commenting out the line

    ActiveRecord::Base.store_full_sti_class = true

    in new_rails_defaults.rb would work around it. This is clearly not a fix, but perhaps it's a clue. Since I'm not using STI with namespaced classes, I haven't looked into it further.

  • Ryan Kinderman

    Ryan Kinderman October 1st, 2009 @ 02:40 AM

    As Rhett suggested, this seems to be more of an issue with the store_full_sti_class option, specifically in how it's used in ActiveRecord::Base#type_name_with_module:

    def type_name_with_module(type_name)
      if store_full_sti_class
        type_name
      else      
        (/^::/ =~ type_name) ? type_name : "#{parent.name}::#{type_name}"
      end
    end
    

    Returning the type_name when store_full_sti_class is true seems to have no useful effect, and always prepending the parent module name when the type name is not fully-qualified seems to fix this issue. I've attached a patch w/the updated method and a a test to demonstrate.

  • Andrew White

    Andrew White October 2nd, 2009 @ 06:56 AM

    • Assigned user changed from “Jeremy Kemper” to “Carl Lerche”

    No, it's not related to STI as it happens even when ActiveRecord isn't involved, e.g:

    require 'rubygems'
    require 'active_support'
    
    module Admin
      class Account
        def self.user_class
          User
        end
      end
    
      class User
      end
    end
    

    If you do Admin::Account.user_class you get Admin::User returned, however if you do Admin::Account.class_eval("User") you will get 'Admin is not missing constant User!'. Without ActiveSupport the class_eval would still generate an error but it's the logical 'uninitialized constant Admin::Account::User'. Since Rails is overriding const_missing to autoload missing constants the fact that the constant is already loaded shouldn't be an error in my view since the bare User constant is found when used directly within the Admin::Account class.

  • Ryan Kinderman

    Ryan Kinderman October 13th, 2009 @ 03:34 AM

    I don't think we want to make this fix in the load_missing_constant method. I presume that the reason that it's raising the error is that it's not expecting the constant to be defined already, since we've just called a method that's designed to load missing constants.

    I believe that the correct place to make the fix for this issue is in the AS::Dependencies override of the Class#const_missing method, since this is the specific place that we're seeing the issue occur.

    I've attached a patch that fixes the issue that Andrew mentioned above in the Class#const_missing method.

  • Ryan Kinderman

    Ryan Kinderman October 13th, 2009 @ 03:48 AM

    Okay, so to summarize:

    I think that there are a few issues that are all manifesting themselves here:

    • The AR::Base.store_full_sti_class option seems to be used incorrectly in type_name_with_module. This patch I've submitted fixes that particular issue.

    • AS::Dependencies has a bug in how it currently resolves a constant that is evaluated from within a class when the constant is defined in a parent module of the evaluating class. This issue is manifesting itself most often when using namespaced AR models that have defined associations to other AR models defined in the same namespace, since this situation will cause the failing behavior to be invoked. However, since the AS::Dependencies logic affects everything, the problem could occur in any Ruby code that requires AS::Dependencies. This patch fixes that issue.

    • AS::Dependencies behavior itself overwrites the Ruby constant resolution logic to add file autoloading functionality. However, it implements the constant resolution logic in a way that is inconsistent with normal Ruby constant resolution logic, causing an ArgumentError to be raised about "constant not undefined" when a constant is already defined that it thinks it needs to try and load from the filesystem. Normal Ruby behavior in some cases is to raise a NameError instead, if the constant is being evaluated from within a class whose parent module defines the constant that is being evaluated.

    As I see it, there are two ways to deal with this. The first way is to keep things pretty close to the way they are now with the dependency loading logic, and make some minor changes to resolve the issues identified in this ticket; that might be a good short-term solution. The second way is to move some behavior around and refactor some things so that AR::Base is truly independent of AS::Dependencies for computing types from strings, and all that AS::Dependencies does is autoload files and attempt to resolve constants from them.

    Going with the second option would mean that AR::Base may need to do the module parent chain traversal as part of its compute_type logic. This makes sense, since the parent chain traversal logic is at least partly inconsistent with the default Ruby behavior for constant resolution, and so probably should be scoped to the needs of AR::Base when resolving constants from things like association reflections.

    Thoughts?

  • Andrew White

    Andrew White October 15th, 2009 @ 01:58 PM

    I'm happy with Ryan's patch if that'd be preferred. I'd thought of doing it that way but I think I was trying to be as unobtrusive as possible at the time due to the impending release of 2.3. Wouldn't it need to take account of the changes to const_defined? in Ruby 1.9 though?

  • Ryan Kinderman

    Ryan Kinderman October 15th, 2009 @ 03:52 PM

    You're probably right, Andrew. Good catch. I hadn't tested it with 1.9. I can do that sometime this week or next, unless you want to handle it. Let me know, and I won't.

  • Andrew White

    Andrew White November 3rd, 2009 @ 05:19 PM

    I'm quite prepared to write the patch, though we could probably do with some direction from a core team member at this point. I noticed a commit in a experimental branch which implies that AS::Dependencies is being looked at for Rails 3.0. If this is the case do core want a patch for 2.3-stable which fixes it in ClassConstMissing and another patch with just a failing test case for edge. Should I raise the matter on the rails-core mailing list?

  • Rhett Sutphin

    Rhett Sutphin January 4th, 2010 @ 11:15 PM

    I've attached a revised version of Ryan's 10/1 patch. The old one no longer would apply to master. I tested it (i.e., verified that the test suite passes) on MRI 1.8.7-p174 and MRI 1.9.1-p376.

    I also verified that the other patch (Ryan's from 10/13) works on 1.9.1-p376.

    I'd like to see that at least the first of these (the STI one) get applied to master before 3.0 comes out -- this is a longstanding irritation with namespaced models that can be fixed with a small code change. (I can see where the second one might require more consideration.) What else can I do to help?

  • Björn Grossmann

    Björn Grossmann February 5th, 2010 @ 11:52 AM

    thanks for working on this, guys, I can confirm having the same problem on Rails 2.3.4 using both STI and namespaced controllers. Do you know if there will be another release of Rails 2.3.x now that Rails 3 beta is out?

  • Andrew White

    Andrew White February 15th, 2010 @ 12:21 AM

    • Tag changed from active_support, constant, dependencies, loading, patched to active_support, constant, dependencies, loading, rails3

    Just a heads up to say that this is still a problem after wycat's recent changes to AS::Dependencies. All it needs to fix it is a check for whether the constant is already defined before calling load_missing_constant.

    I'm quite willing to write the patch if it'll get applied. I'd also throw in one for 2.3-stable as well.

  • Ryan Kinderman

    Ryan Kinderman February 15th, 2010 @ 12:30 AM

    I'm also still watching this. Any input from people on the core team who are familiar with the direction that this stuff is going, so we can come to a lasting solution?

  • Ryan Kinderman

    Ryan Kinderman February 15th, 2010 @ 12:48 AM

    I've posted to the rails-core list about this issue. Maybe we'll get someone's attention there.

    http://groups.google.com/group/rubyonrails-core/t/534dda4205adb692

  • Andrew White

    Andrew White February 16th, 2010 @ 01:53 PM

    • Tag changed from active_support, constant, dependencies, loading, rails3 to active_support, constant, dependencies, loading, patch, rails3

    Following discussion on the core mailing list I'm attaching a patch that appears to fix the problem within compute_type. The patch passes all AR tests and I added a failing test first that now passes.

  • Andrew White

    Andrew White March 15th, 2010 @ 10:18 AM

    Updated patch for HEAD with whitespace changes removed and updated offsets, no other code changes.

  • Jan De Poorter

    Jan De Poorter March 15th, 2010 @ 10:28 AM

    +1, applies & tests pass, and fixes the problem I had with this

  • Phillip Koebbe

    Phillip Koebbe March 25th, 2010 @ 10:27 PM

    I can't say that I'm following all of this very clearly, but I wanted to add something to the discussion just in case it might be useful in some way. I read through the thread on the rails-core list, but didn't see anything relating to this mentioned. I'm probably just going to expose my ignorance here, but oh well.

    I am getting the same error (x is not missing constant y) in 2.3.5, but I am not namespacing my models at all. I am, however, namespacing my controllers.

    class Web::BaseController < ApplicationController
    end
    
    class Web::SessionsController < Web::BaseController
      def create
        user_id = User.authenticate(params[:email], params[:password])
      end
    end
    
    class User < ActiveRecord::Base
    end
    

    I get the error in Web::SessionsController#create, but only in development. In testing, either through RSpec or Cucumber+Culerity, I didn't see it. So I compared environment files for test and development and noticed config.cache_classes was false in development but true in test. When I changed it to true in development, I didn't get the error.

    There may be more to it than that as I was also creating a very simple project that mimicked my structure but was unable to reproduce the problem. And I'm doing the same thing in another project without any problems. So this bit about cache_classes might not help in any way.

  • Andrew White

    Andrew White March 26th, 2010 @ 01:56 PM

    Phillip, what was the error message? If it was 'Object is not missing constant User' then it's still related to namespaces and models. If it's something like 'Web is not missing constant BaseController then it's something I've not come across before even though I do something similar with admin controllers.

  • Phillip Koebbe

    Phillip Koebbe March 26th, 2010 @ 02:06 PM

    Andrew,

    It was 'Web is not missing constant User!'. I assumed it was related, but didn't know if the additional information of cache_classes might be helpful.

    Oh, and I completely forgot to mention that I applied the changes in the patch, but they didn't help. [Why would they when they were on ActiveRecord? heh heh]

    Something I just thought of that might be contributing to my particular case. I'm namespacing my controllers in an effort to make role restrictions very clean, so I have the base namespace of Web which is for users not logged in, Admin for administrative functionality, and User for users who are logged in. It looks like

    class Web::User::BaseController < Web::BaseController
        before_filter :require_logged_in
        
        private
        
        def require_logged_in
            redirect_to(web_login_path) and return false unless logged_in?
            return true
        end
    end
    

    And that might be causing some confusion with the User model. If so, I'm confused why it would work in testing with cache_classes == true and not in development with cache_classes == false.

  • Andrew White

    Andrew White March 26th, 2010 @ 02:11 PM

    Yep, the Web::User::BaseController will be the cause of the problem. The difference due to cache_classes is probably down to a different load order. A fix in your case is to use ::User rather than User or change the Web::User namespace to something else.

  • Phillip Koebbe

    Phillip Koebbe March 26th, 2010 @ 03:22 PM

    Thanks, Andrew. Sorry to have added noise to the conversation.

  • Yehuda Katz (wycats)

    Yehuda Katz (wycats) March 27th, 2010 @ 03:13 AM

    • State changed from “open” to “verified”
    • Assigned user changed from “Carl Lerche” to “Yehuda Katz (wycats)”
    • Milestone cleared.
  • Andrew White

    Andrew White March 31st, 2010 @ 03:06 AM

    Updated patch with spelling fixed - somehow I managed to spell candidates wrong consistently.

  • Andrew White
  • Andrew White

    Andrew White April 4th, 2010 @ 07:58 AM

    Updated patch for master with tests for has_many with dependencies from ticket #2627.

  • Andrew White

    Andrew White April 4th, 2010 @ 08:01 AM

    Updated patch for 2-3-stable with tests for has_many with dependencies from ticket #2627. This needs the backport of lazy evaluation of has_many ..., :dependent => :___ patch before the tests will pass.

  • Andrew White

    Andrew White April 12th, 2010 @ 01:52 PM

    Rebased patch for latest HEAD.

    Is there anything that's holding this patch up? Anything I can do to help?

  • Yehuda Katz (wycats)

    Yehuda Katz (wycats) April 13th, 2010 @ 05:31 AM

    • State changed from “verified” to “resolved”
  • Jeremy Kemper

    Jeremy Kemper October 15th, 2010 @ 11:01 PM

    • Milestone set to 3.0.2
    • Importance changed from “” to “High”

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

Tickets have moved to Github

The new ticket tracker is available at https://github.com/rails/rails/issues

Shared Ticket Bins

Referenced by

Pages