#2262 Associations are picking the wrong class - Ruby on Rails

Type	To find
responsible:me	tickets assigned to you
tagged:"@high"	tickets tagged @high
milestone:next	tickets in the upcoming milestone
state:invalid	tickets with the state invalid
created:"last week"	tickets created last week
sort:number, importance, updated	tickets sorted by #, importance or updated
Combine keywords for powerful searching.
Use advanced searching »

This project is archived and is in readonly mode.

#2262 ✓invalid

Associations are picking the wrong class

Reported by José Valim | March 17th, 2009 @ 12:58 AM | in 2.x

UPDATE

After long discussion, this ticket is no longer valid. Some one from core just mark it as invalid please.

This is being discussed on the mailing list:

http://groups.google.com/group/r...

Problem


  class User < ActiveRecord::Base
  end

  class Admin::User < ActiveRecord::Base
  end

  class Account < ActiveRecord::Base
    belongs_to :user
  end

When I was calling Account.new.build_user, it was trying to create a new Admin::User and not an ::User, since the default class_name is just "User".

Where?

Rails 2.3.2

Why this is a problem?

Let's suppose I'm building an application (without using namespaced models at all) and then I install a plugin that has Plugin::User. My associations will start to pick Plugin::User, and this is really hard to track down.

Solution?

I propose a solution where all associations :class_name is, by default, namespaced. So:


  # have default class_name of ::User
  class Account < ActiveRecord::Base; belongs_to :user; end

  # have default class_name of ::Admin::User
  class Admin::Account < ActiveRecord::Base; belongs_to :user; end

Compatibility?

It will break apps that are relying in this wrong behavior. The good point is that the error will be raised when the application starts (can't find association error). The way it's now, takes quite sometime for you to discover that Account was trying to build a Admin::User instead of User.

namespaced_associations.diff 2.8 KB

Comments and changes to this ticket

You flagged this item as spam.
Hugo Barauna March 17th, 2009 @ 03:14 AM
+1
Andrew White March 17th, 2009 @ 03:10 PM
- Title changed from “ Associations are picking the wrong class ” to “Associations are picking the wrong class”
I can't replicate this in a new 2.3.2 application even if I create the Admin::User model in a plugin. I can nearly replicate the opposite with Admin::Account, but what happens is also depends on the store_full_sti_class setting along with the loaded states of the User and Admin::User models.

Historically STI models were stored with a demodulized class name in the type column and the compute_type method was used to restore the namespace when loading the constant. This had the side-effect of making namespaced associated models working as well. However now that the STI class is stored in full the type_name_with_module method just returns the default class name unmodified and reverts to the standard Rails constant loading mechanism.

So what happens here is that when store_full_sti_class is false the type_name_with_module method prefixes the name with Admin and Admin::User is found successfully. If store_full_sti_class is true and neither model is loaded then an Admin::User instance is returned. If the Admin::User model is loaded beforehand then either an exception is raised if the User model is not loaded or an instance of User is returned if it is loaded.

The patch does fix this specific problem but I think the real bug is actually in AS::Dependencies.load_missing_constant. Rather than raising the exception it should just return the already defined constant as the test name for it seems to indicate that it's there to prevent double loading of constants. Changing that only breaks one test and that's checking to see if the exception is raised.

Attached is a patch that makes this change.
- 0001-load_missing_constant_fix.diff 1.9 KB
José Valim March 17th, 2009 @ 03:57 PM
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.
Andrew White March 17th, 2009 @ 04:37 PM
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?
Andrew White March 17th, 2009 @ 04:53 PM
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?
José Valim March 17th, 2009 @ 04:54 PM
I don't have a clue, debuggers don't go inside class_eval. :) But I agree that it's totally strange.
José Valim March 17th, 2009 @ 05:23 PM
Nops.
```
User == Admin::User #=> false
```
Have you tried with Admin::User being a module?
Andrew White March 17th, 2009 @ 05:37 PM
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
José Valim March 17th, 2009 @ 05:42 PM
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.
José Valim March 17th, 2009 @ 06:00 PM
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?
Andrew White March 18th, 2009 @ 08:14 AM
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.
José Valim March 18th, 2009 @ 08:43 AM
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.
Andrew White March 18th, 2009 @ 09:47 AM
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.
Andrew White March 18th, 2009 @ 09:53 AM
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.
José Valim March 18th, 2009 @ 01:17 PM
I just updated the ticket. Someone from core might close this one soon.
Frederick Cheung May 8th, 2009 @ 12:11 AM
- State changed from “new” to “invalid”