Comments on #9575 requested please.

rubyonrails-core
1

I've just created a new ticket with a patch that adds some extra searching to compute_type in AR. It starts with the existing behaviour and then walks up the model namespace. It also tries a name prefixed with current name so that it can catch models prefix with their parent's name. e.g. This model:

   class Shop::Product < ActiveRecord::Base      has_many :items    end

   class Shop::ProductItem < ActiveRecord::Base    end

would give the follow candidate names:

   ::Shop::Item    ::Item    ::Shop::Product::Item    ::Shop::ProductItem    ::Shop::Item    ::ShopItem

It passes all the existing tests but one of the changes it makes is to prefix the existing names that are checked with '::' to limit the scope of the check, so is there any chance that any of you out there with complicated models can check that it doesn't break your app? Also AFAICS the association reflection caches this result so performance impact should negligible - is this assumption correct?

Any other comments would be appreciated too.

Andrew White

2

I'm not really sure why you want it to find ProductItem in addition to Item? Couldnt you just use has_many :product_items?

3

You can but you end up with accessing stuff like @products.product_items which is a bit redundant. If you name the class Product::Item then referring to the nested class from somewhere else requires the full name and the reciprocal relationship requires a leading '::' e.g:

   class Product < ActiveRecord::Base      has_many :items    end

   class Product::Item < ActiveRecord::Base      belongs_to :product, :class_name => '::Product'    end

   class Basket < ActiveRecord::Base      has_many :items    end

   class Basket::Item < ActiveRecord::Base      belongs_to :basket      belongs_to :product_item, :class_name => '::Product::Item'    end

I've no doubt that my patch isn't perfect but I don't think the existing compute_type is adequate either.

Andrew

4

You can but you end up with accessing stuff like @products.product_items which is a bit redundant. If you name the class Product::Item then referring to the nested class from somewhere else requires the full name and the reciprocal relationship requires a leading '::' e.g:

If there are issues with using the association macros between classes in modules, I think we could look into fixing them. But I definitely don't think that the item -> ProductItem case is justified.

If you could write up some test cases which currently fail, then we can figure out where to go from here.

5

I went a bit further a came up with a set of tests that show all the issues with nested models that I can find. I've attached them to the same ticket.

Andrew White