Associations are picking the wrong class

+1

Andrew, I understand your problem but you're solving the other way: why Admin::Account is loading "Admin::User" instead of "User".

My problem is Account loading "Admin::User" instead of "User". I've just tried your patch here and it hasn't fixed it.

Who is returning the wrong class is class_eval. The backtrace would be: reflections call compute_type with "User", compute_type calls type_name_with_module with "User" and returns "User", and then class_eval is called and picks up "Admin::User" instead of "User".

I think this problem is strictly related with namespaces because both "Admin::User" and "User" are loaded before the error occurs (so can I discard this an AS::Dependencies bug?).

"I can't replicate this in a new 2.3.2 application even if I create the Admin::User model in a plugin."

I don't think this is an easy bug to reproduce, as you said depends who is loaded and who is not, but it's an easy one to understand why. If we say to ruby that I want ::User, it will load the right one.

However, your discussion just pointed me that we could solve the problem making type_name_with_module always returns "::User" or "::Admin::User".

But it doesn't change the fact that associations should default to a namespaced class, and thus, I think the patch I propose is still needed.

José, I just don't why it should be getting Admin::User instead of User as the constant lookup should Account::User and then Object::User - why would it go looking in the Admin namespace?

There is one way I can reproduce it and that's by defining the User constant within Account, e.g:

class Account < ActiveRecord::Base
  User = Admin::User
  belongs_to :user
end

Is it possible that something similar is happening in your code?

I don't have a clue, debuggers don't go inside class_eval. :) But I agree that it's totally strange.

Nops.

User == Admin::User #=> false

Have you tried with Admin::User being a module?

I just cannot reproduce - I've tried Admin being both a AR model and just a module. I've tried both methods in a plugin and in the main application. I've also tried Admin::User as a module but that didn't make any difference either.

What happens if you tweak the class_eval to be parent.class_eval in compute_type? Also what's the value of store_full_sti_class

I'm afraid this can't be easily reproduced. The plugin I'm using that showed me this conflict is Typus (http://github.com/fesplugas/typu...>

In this case, Typus::User is a module and it's included in ActiveRecord::Base:

  module Typus
    module User
    end
  end

  ActiveRecord::Base.send :include, Typus::User

However I've cut down their code in order to have just the lines above, and the problem couldn't be reproduced.

Anyway, with this bug or not, association are still wrong. It should have a default class name of ::Admin::User if I'm declaring it inside ::Admin::Account, and default class name of ::User if I'm declaring it on ::Account.

store_full_sti_class is true. Calling parent.class_eval in compute_type works, but in my case the parent is Object.

However, I cannot supply a patch with parent.class_eval because this would make namespaces break, since we would be calling Admin.class_eval(Admin::User).

On the other hand, a possible solution could be calling Object.class_eval(User), which is the same as class_eval(::User). This is likely to don't break anything because type_name_with_module is already responsible to return a namespaced class.

So I propose: Object.class_eval in compute_type AND make associations namespaced by default, but without '::' at the beginning, which looks strange.

Andrew, your patch is still needed?

José, I've tracked down your problem with Typus. Typus includes Typus::User in AR::Base so all AR models have a constant User, e.g: Account::User and even User::User is defined. This is what compute_type is finding.

Your patch works around the problem by starting the search one level up, avoiding the match with Account::User. Unfortunately your patch would break one of my apps where I have the following:

class DeliveryMethod < ActiveRecord::Base
  class PriceBand < ActiveRecord::Base; end

  has_many :price_bands
end

If you modify your patch so that the search starts at the same level then you'll still get the same problem with Account::User matching. The solution lies with Typus having a less common name for its User module - should be a simple fix.

As for my patch - I think it probably does still need to be applied but I'll open a new ticket.

Andrew, I see you case.

However, I still think that a simple patch should be applied.

At least one that makes Admin::Account when belongs_to User have a default class name of Admin::User (without the '::'). This wouldn't break your application, it wouldn't fix my problem, but I think it's needed.

José, that's essentially what happens at the moment - the string "User" is evaluated within the Admin class scope so that if Admin::User is defined it will find it first before looking elsewhere.

Sorry, slightly wrong on the last comment. What actually happens is the following lookup:

1. Admin::Account::User
2. Admin::User
3. User

What causes this to fail at the moment is the fact that load_missing_constant is called after not finding Admin::Account::User and the exception is raised if the Admin::User has been previously loaded. This is what my patch fixes.

I just updated the ticket. Someone from core might close this one soon.