Permalink
Browse files

Testing identity map on inherited active record classes. Distinct mod…

…els that use the same database table shouldn't be retrieved as the same object when there is not a type attribute.
  • Loading branch information...
richardiux committed May 5, 2011
1 parent 5e1960e commit 564922b32ceec259c442e965ac8a61ea5545bd48
Showing with 17 additions and 0 deletions.
  1. +17 −0 activerecord/test/cases/identity_map_test.rb
@@ -128,6 +128,23 @@ def test_creation
assert_same(t1, t2)
end
+ ##############################################################################
+ # Tests checking if IM is functioning properly on classes with multiple #
+ # types of inheritance #
+ ##############################################################################
+
+ def test_inherited_without_type_attribute
+ p1 = DestructivePirate.create!(:catchphrase => "I'm not a regular Pirate")
+ p2 = Pirate.find(p1.id)

This comment has been minimized.

Show comment
Hide comment
@miloops

miloops May 5, 2011

Not really, the Pirate.find(p1.id) should be the same than p1. When finding over a STI class the loaded objects are instantiated depending on it's class. So Pirate.find p1.id should be a DestructivePirate.

@miloops

miloops May 5, 2011

Not really, the Pirate.find(p1.id) should be the same than p1. When finding over a STI class the loaded objects are instantiated depending on it's class. So Pirate.find p1.id should be a DestructivePirate.

This comment has been minimized.

Show comment
Hide comment
@miloops

miloops May 5, 2011

Try this with or without IM:

p1 = DestructivePirate.create!(:catchphrase => "I'm not a regular Pirate")
p2 = Pirate.find(p1.id)
assert_equal(p1, p2)

@miloops

miloops May 5, 2011

Try this with or without IM:

p1 = DestructivePirate.create!(:catchphrase => "I'm not a regular Pirate")
p2 = Pirate.find(p1.id)
assert_equal(p1, p2)

+ assert_not_same(p1, p2)
+ end
+
+ def test_inherited_with_type_attribute
+ c1 = comments(:sub_special_comment)
+ c2 = Comment.find(c1.id)
+ assert_same(c1, c2)
+ end
+
##############################################################################
# Tests checking dirty attribute behaviour with IM #
##############################################################################

7 comments on commit 564922b

@richardiux

This comment has been minimized.

Show comment
Hide comment
@richardiux

richardiux May 5, 2011

Owner

Currently both tests are failing, and I'm getting the expected behavior when I turn off IM.

My current test case:
I currently use the same database table to represent warehouse and store, but that does not mean that I always want to return a Store object. If I did, then I would have to add a type attribute to my database, and then Rails would make sure it returned the correct model.

If I do this:
Warehouse.all
all the returned objects will be of type warehouse

but if first I do this:
Store.find(1)
Warehouse.all
then all the returned objects will be of type warehouse, except the one with id=1

Owner

richardiux replied May 5, 2011

Currently both tests are failing, and I'm getting the expected behavior when I turn off IM.

My current test case:
I currently use the same database table to represent warehouse and store, but that does not mean that I always want to return a Store object. If I did, then I would have to add a type attribute to my database, and then Rails would make sure it returned the correct model.

If I do this:
Warehouse.all
all the returned objects will be of type warehouse

but if first I do this:
Store.find(1)
Warehouse.all
then all the returned objects will be of type warehouse, except the one with id=1

@miloops

This comment has been minimized.

Show comment
Hide comment
@miloops

miloops May 5, 2011

This is on a Rails 3.0.7 app:

ruby-1.9.2-p180 :001 > CouponPayment.first
<CouponPayment id: 26, currency_id: 1, registration_id: 58, ... >

ruby-1.9.2-p180 :002 > Payment.first
<CouponPayment id: 26, currency_id: 1, registration_id: 58, ... >

ruby-1.9.2-p180 :003 > CouponPayment.find 26
<CouponPayment id: 26, currency_id: 1, registration_id: 58,  ...>

ruby-1.9.2-p180 :004 > Payment.find 26
<CouponPayment id: 26, currency_id: 1, registration_id: 58, ... >

ruby-1.9.2-p180 :005 > Payment.all[0]
<CouponPayment id: 26, currency_id: 1, registration_id: 58,  ... >

This is on a Rails 3.0.7 app:

ruby-1.9.2-p180 :001 > CouponPayment.first
<CouponPayment id: 26, currency_id: 1, registration_id: 58, ... >

ruby-1.9.2-p180 :002 > Payment.first
<CouponPayment id: 26, currency_id: 1, registration_id: 58, ... >

ruby-1.9.2-p180 :003 > CouponPayment.find 26
<CouponPayment id: 26, currency_id: 1, registration_id: 58,  ...>

ruby-1.9.2-p180 :004 > Payment.find 26
<CouponPayment id: 26, currency_id: 1, registration_id: 58, ... >

ruby-1.9.2-p180 :005 > Payment.all[0]
<CouponPayment id: 26, currency_id: 1, registration_id: 58,  ... >
@richardiux

This comment has been minimized.

Show comment
Hide comment
@richardiux

richardiux May 5, 2011

Owner

I get the same result as you, but I don't think it should return payment object unless you explicitly have a type attribute in the model. It should behave exactly the same as when I turn off IM. In my previous example I show what I'm getting, not what I'm expecting. Makes sense?

Owner

richardiux replied May 5, 2011

I get the same result as you, but I don't think it should return payment object unless you explicitly have a type attribute in the model. It should behave exactly the same as when I turn off IM. In my previous example I show what I'm getting, not what I'm expecting. Makes sense?

@miloops

This comment has been minimized.

Show comment
Hide comment
@miloops

miloops May 5, 2011

Yes, it does.

You might want to take a look at:

https://github.com/rails/rails/blob/master/activerecord/lib/active_record/base.rb#L829

ActiveRecord::Base#symbolized_base_class is the one used to store the IM per class in IdentityMap:

https://github.com/rails/rails/blob/master/activerecord/lib/active_record/identity_map.rb

Yes, it does.

You might want to take a look at:

https://github.com/rails/rails/blob/master/activerecord/lib/active_record/base.rb#L829

ActiveRecord::Base#symbolized_base_class is the one used to store the IM per class in IdentityMap:

https://github.com/rails/rails/blob/master/activerecord/lib/active_record/identity_map.rb

@richardiux

This comment has been minimized.

Show comment
Hide comment
@richardiux

richardiux May 5, 2011

Owner

Thanks, I saw the implementation. Should I try to fix it? If we are on the same page on the expected result, then I'll give it a try.

Owner

richardiux replied May 5, 2011

Thanks, I saw the implementation. Should I try to fix it? If we are on the same page on the expected result, then I'll give it a try.

@miloops

This comment has been minimized.

Show comment
Hide comment
@miloops

miloops May 5, 2011

There should be no regression or different behavior when using IM or not, that's for sure. Let me know if you need anything. :)

There should be no regression or different behavior when using IM or not, that's for sure. Let me know if you need anything. :)

@richardiux

This comment has been minimized.

Show comment
Hide comment
@richardiux

richardiux May 5, 2011

Owner

Awesome! Thanks for your help today :)
I'll ping you when I figure something out, or if I get stuck.

Owner

richardiux replied May 5, 2011

Awesome! Thanks for your help today :)
I'll ping you when I figure something out, or if I get stuck.

Please sign in to comment.