Skip to content
This repository

Identity Map caching bug #423

Merged
merged 3 commits into from almost 3 years ago

3 participants

Richard Millan José Valim Emilio Tagua
Richard Millan

Currently Identity Map has a small bug when using STI. It is currently calculating the caching key by using the symbolized_base_class. To accomplish that, I had to add symbolized_sti_name to ActiveRecord::Base since I didn't find anything else that could be used. Let me know if this is a problem and if there is a method that can be used instead.

Initial discussion with @miloops
richardiux@564922b

Richard Millan 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.
564922b
Emilio Tagua

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.

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)

Richard Millan

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

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,  ... >

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?

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

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.

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

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

added some commits May 05, 2011
José Valim
Owner

@miloops, +1 for merging these commits?

Emilio Tagua
miloops commented May 06, 2011

Yes, they look good, although i don't have time now to run tests. If you can do that then I think is ok to merge.

José Valim
Owner

Is symbolized base class still used somewhere else?

José Valim josevalim merged commit b8f08c4 into from May 06, 2011
José Valim josevalim closed this May 06, 2011
José Valim josevalim referenced this pull request from a commit May 06, 2011
José Valim Revert the merge because tests did not pass.
Revert "Merge pull request #423 from richardiux/master"

This reverts commit b8f08c4, reversing
changes made to fd9df1b.
886818d
José Valim
Owner

I have applied but had to revert those changes because tests did not pass. It seems Github do not allow me to reopen the issue, @richardiux could please open a new issue and point to this one?

Richard Millan

@josevalim It idn't allow me to send a pull request since it has already been applied to master. I ran the tests with rake test_mysql under the activerecord directory and had no errors. What issues did you have?

José Valim
Owner

I had issues with "rake test_sqlite3". I think that to bring your commits again, you will have to rebase your branch against rails master, revert my revert (lolgit) and apply the fix.

Richard Millan

Tested with rake test_sqlite3 and rake test_mysql and I didn't get any errors on my machine. Checked out a new version of rails, and reverted the revert, and I still didn't get any errors. Let's try again, maybe there was other commit that caused trouble?

Do you know what is the git command to rebase from my clone on github? Otherwise I'll just re-clone it.

José Valim
Owner

So here is what I just did:

1) I have forked Rails again, created a new branch and typed:

git checkout -b imfix
git revert 886818d

2) Then, I went to activerecord, under REE and did:

rake test_sqlite3

3) I got the following failures:

1) Error:
test_find_by_sess_id_compat(ActiveRecord::SessionStore::SessionTest):
ArgumentError: interning empty string
/Users/jose/Work/github/rails/activerecord/lib/active_record/base.rb:834:in to_sym'
/Users/jose/Work/github/rails/activerecord/lib/active_record/base.rb:834:in
symbolized_sti_name'
/Users/jose/Work/github/rails/activerecord/lib/active_record/identity_map.rb:71:in remove'
/Users/jose/Work/github/rails/activerecord/lib/active_record/attribute_methods/dirty.rb:38:in
save!'
/Users/jose/Work/github/rails/activerecord/lib/active_record/transactions.rb:246:in save!'
/Users/jose/Work/github/rails/activerecord/lib/active_record/transactions.rb:295:in
with_transaction_returning_status'
/Users/jose/Work/github/rails/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb:191:in transaction'
/Users/jose/Work/github/rails/activerecord/lib/active_record/transactions.rb:208:in
transaction'
/Users/jose/Work/github/rails/activerecord/lib/active_record/transactions.rb:293:in with_transaction_returning_status'
/Users/jose/Work/github/rails/activerecord/lib/active_record/transactions.rb:246:in
save!'
./test/cases/session_store/session_test.rb:43:in test_find_by_sess_id_compat'
/Users/jose/Work/github/rails/activesupport/lib/active_support/testing/setup_and_teardown.rb:67:in
send'
/Users/jose/Work/github/rails/activesupport/lib/active_support/testing/setup_and_teardown.rb:67:in run'
/Users/jose/Work/github/rails/activesupport/lib/active_support/callbacks.rb:408:in
_run_setup_callbacks'
/Users/jose/Work/github/rails/activesupport/lib/active_support/callbacks.rb:81:in send'
/Users/jose/Work/github/rails/activesupport/lib/active_support/callbacks.rb:81:in
run_callbacks'
/Users/jose/Work/github/rails/activesupport/lib/active_support/testing/setup_and_teardown.rb:65:in `run'

2) Error:
test_read_attributes_after_type_cast_on_datetime(AttributeMethodsTest):
ArgumentError: interning empty string
/Users/jose/Work/github/rails/activerecord/lib/active_record/base.rb:834:in to_sym'
/Users/jose/Work/github/rails/activerecord/lib/active_record/base.rb:834:in
symbolized_sti_name'
/Users/jose/Work/github/rails/activerecord/lib/active_record/identity_map.rb:71:in remove'
/Users/jose/Work/github/rails/activerecord/lib/active_record/transactions.rb:254:in
rollback_active_record_state!'
/Users/jose/Work/github/rails/activerecord/lib/active_record/transactions.rb:240:in save'
./test/cases/attribute_methods_test.rb:162:in
test_read_attributes_after_type_cast_on_datetime'
./test/cases/attribute_methods_test.rb:667:in in_time_zone'
./test/cases/attribute_methods_test.rb:151:in
test_read_attributes_after_type_cast_on_datetime'
/Users/jose/Work/github/rails/activesupport/lib/active_support/testing/setup_and_teardown.rb:67:in __send__'
/Users/jose/Work/github/rails/activesupport/lib/active_support/testing/setup_and_teardown.rb:67:in
run'
/Users/jose/Work/github/rails/activesupport/lib/active_support/callbacks.rb:408:in _run_setup_callbacks'
/Users/jose/Work/github/rails/activesupport/lib/active_support/callbacks.rb:81:in
send'
/Users/jose/Work/github/rails/activesupport/lib/active_support/callbacks.rb:81:in run_callbacks'
/Users/jose/Work/github/rails/activesupport/lib/active_support/testing/setup_and_teardown.rb:65:in
run'

3) Error:
test_time_attributes_changes_with_time_zone(DirtyTest):
ArgumentError: interning empty string
/Users/jose/Work/github/rails/activerecord/lib/active_record/base.rb:834:in to_sym'
/Users/jose/Work/github/rails/activerecord/lib/active_record/base.rb:834:in
symbolized_sti_name'
/Users/jose/Work/github/rails/activerecord/lib/active_record/identity_map.rb:71:in remove'
/Users/jose/Work/github/rails/activerecord/lib/active_record/attribute_methods/dirty.rb:38:in
save!'
/Users/jose/Work/github/rails/activerecord/lib/active_record/transactions.rb:246:in save!'
/Users/jose/Work/github/rails/activerecord/lib/active_record/transactions.rb:295:in
with_transaction_returning_status'
/Users/jose/Work/github/rails/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb:191:in transaction'
/Users/jose/Work/github/rails/activerecord/lib/active_record/transactions.rb:208:in
transaction'
/Users/jose/Work/github/rails/activerecord/lib/active_record/transactions.rb:293:in with_transaction_returning_status'
/Users/jose/Work/github/rails/activerecord/lib/active_record/transactions.rb:246:in
save!'
./test/cases/dirty_test.rb:69:in test_time_attributes_changes_with_time_zone'
./test/cases/dirty_test.rb:522:in
in_time_zone'
./test/cases/dirty_test.rb:58:in test_time_attributes_changes_with_time_zone'
/Users/jose/Work/github/rails/activesupport/lib/active_support/testing/setup_and_teardown.rb:67:in
send'
/Users/jose/Work/github/rails/activesupport/lib/active_support/testing/setup_and_teardown.rb:67:in run'
/Users/jose/Work/github/rails/activesupport/lib/active_support/callbacks.rb:408:in
_run_setup_callbacks'
/Users/jose/Work/github/rails/activesupport/lib/active_support/callbacks.rb:81:in send'
/Users/jose/Work/github/rails/activesupport/lib/active_support/callbacks.rb:81:in
run_callbacks'
/Users/jose/Work/github/rails/activesupport/lib/active_support/testing/setup_and_teardown.rb:65:in `run'

4) Error:
test_time_attributes_changes_without_time_zone(DirtyTest):
ArgumentError: interning empty string
/Users/jose/Work/github/rails/activerecord/lib/active_record/base.rb:834:in to_sym'
/Users/jose/Work/github/rails/activerecord/lib/active_record/base.rb:834:in
symbolized_sti_name'
/Users/jose/Work/github/rails/activerecord/lib/active_record/identity_map.rb:71:in remove'
/Users/jose/Work/github/rails/activerecord/lib/active_record/attribute_methods/dirty.rb:38:in
save!'
/Users/jose/Work/github/rails/activerecord/lib/active_record/transactions.rb:246:in save!'
/Users/jose/Work/github/rails/activerecord/lib/active_record/transactions.rb:295:in
with_transaction_returning_status'
/Users/jose/Work/github/rails/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb:191:in transaction'
/Users/jose/Work/github/rails/activerecord/lib/active_record/transactions.rb:208:in
transaction'
/Users/jose/Work/github/rails/activerecord/lib/active_record/transactions.rb:293:in with_transaction_returning_status'
/Users/jose/Work/github/rails/activerecord/lib/active_record/transactions.rb:246:in
save!'
./test/cases/dirty_test.rb:125:in test_time_attributes_changes_without_time_zone'
/Users/jose/Work/github/rails/activesupport/lib/active_support/testing/setup_and_teardown.rb:67:in
send'
/Users/jose/Work/github/rails/activesupport/lib/active_support/testing/setup_and_teardown.rb:67:in run'
/Users/jose/Work/github/rails/activesupport/lib/active_support/callbacks.rb:408:in
_run_setup_callbacks'
/Users/jose/Work/github/rails/activesupport/lib/active_support/callbacks.rb:81:in send'
/Users/jose/Work/github/rails/activesupport/lib/active_support/callbacks.rb:81:in
run_callbacks'
/Users/jose/Work/github/rails/activesupport/lib/active_support/testing/setup_and_teardown.rb:65:in `run'

5) Error:
test_time_attributes_changes_without_time_zone_by_skip(DirtyTest):
ArgumentError: interning empty string
/Users/jose/Work/github/rails/activerecord/lib/active_record/base.rb:834:in to_sym'
/Users/jose/Work/github/rails/activerecord/lib/active_record/base.rb:834:in
symbolized_sti_name'
/Users/jose/Work/github/rails/activerecord/lib/active_record/identity_map.rb:71:in remove'
/Users/jose/Work/github/rails/activerecord/lib/active_record/attribute_methods/dirty.rb:38:in
save!'
/Users/jose/Work/github/rails/activerecord/lib/active_record/transactions.rb:246:in save!'
/Users/jose/Work/github/rails/activerecord/lib/active_record/transactions.rb:295:in
with_transaction_returning_status'
/Users/jose/Work/github/rails/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb:191:in transaction'
/Users/jose/Work/github/rails/activerecord/lib/active_record/transactions.rb:208:in
transaction'
/Users/jose/Work/github/rails/activerecord/lib/active_record/transactions.rb:293:in with_transaction_returning_status'
/Users/jose/Work/github/rails/activerecord/lib/active_record/transactions.rb:246:in
save!'
./test/cases/dirty_test.rb:96:in test_time_attributes_changes_without_time_zone_by_skip'
./test/cases/dirty_test.rb:522:in
in_time_zone'
./test/cases/dirty_test.rb:83:in test_time_attributes_changes_without_time_zone_by_skip'
/Users/jose/Work/github/rails/activesupport/lib/active_support/testing/setup_and_teardown.rb:67:in
send'
/Users/jose/Work/github/rails/activesupport/lib/active_support/testing/setup_and_teardown.rb:67:in run'
/Users/jose/Work/github/rails/activesupport/lib/active_support/callbacks.rb:408:in
_run_setup_callbacks'
/Users/jose/Work/github/rails/activesupport/lib/active_support/callbacks.rb:81:in send'
/Users/jose/Work/github/rails/activesupport/lib/active_support/callbacks.rb:81:in
run_callbacks'
/Users/jose/Work/github/rails/activesupport/lib/active_support/testing/setup_and_teardown.rb:65:in `run'

6) Error:
test_observing_subclasses(LifecycleTest):
ArgumentError: interning empty string
/Users/jose/Work/github/rails/activerecord/lib/active_record/base.rb:834:in to_sym'
/Users/jose/Work/github/rails/activerecord/lib/active_record/base.rb:834:in
symbolized_sti_name'
/Users/jose/Work/github/rails/activerecord/lib/active_record/identity_map.rb:52:in get'
/Users/jose/Work/github/rails/activerecord/lib/active_record/relation/finder_methods.rb:319:in
find_one'
/Users/jose/Work/github/rails/activerecord/lib/active_record/relation/finder_methods.rb:304:in find_with_ids'
/Users/jose/Work/github/rails/activerecord/lib/active_record/relation/finder_methods.rb:107:in
find'
/Users/jose/Work/github/rails/activerecord/lib/active_record/base.rb:440:in __send__'
/Users/jose/Work/github/rails/activerecord/lib/active_record/base.rb:440:in
find'
./test/cases/lifecycle_test.rb:173:in test_observing_subclasses'
/Users/jose/Work/github/rails/activesupport/lib/active_support/testing/setup_and_teardown.rb:67:in
send'
/Users/jose/Work/github/rails/activesupport/lib/active_support/testing/setup_and_teardown.rb:67:in run'
/Users/jose/Work/github/rails/activesupport/lib/active_support/callbacks.rb:408:in
_run_setup_callbacks'
/Users/jose/Work/github/rails/activesupport/lib/active_support/callbacks.rb:81:in send'
/Users/jose/Work/github/rails/activesupport/lib/active_support/callbacks.rb:81:in
run_callbacks'
/Users/jose/Work/github/rails/activesupport/lib/active_support/testing/setup_and_teardown.rb:65:in `run'

2937 tests, 8925 assertions, 0 failures, 6 errors

You are probably finding it hard to rebase because you applied your commits to your branch master. Generally, it is easier if you work on branches and always leave the branch master for sync.

José Valim
Owner

Actually, looking at the errors, the fix is actually trivial. The issue is because Class.new.name returns an empty string on 1.8.7 but nil on 1.9.2. I am pushing a fix and a revert of the revert. :)

José Valim
Owner

Generally speaking, you should have two remotes in your checkout: origin (which is your fork) and rails (which you should add manually). Usually you don't commit to your master branch and leave it just for sync. So when you want to sync your fork with Rails master, you do:

# Get changes from Rails
git pull --rebase rails master

# Push changes to your fork
git push origin master

# Then you can also go to your branches and rebase
git checkout my_feature
git rebase master

Is this what you have asked?

José Valim
Owner

Here is the fix:

ba23bf4

Now everything pass. :)

Richard Millan

Great! I did the same thing on my machine. Took me a while to install REE, I was just testing with 1.9.2. Now I know better :)
Thanks for all your help in getting the fix in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Showing 3 unique commits by 1 author.

May 05, 2011
Richard Millan 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.
564922b
Richard Millan Test: identity on inherited classes should behave the same when turne…
…d on or off
d53b406
May 06, 2011
Richard Millan Adding base method symbolized_sti_name to activerecord base to be use…
…d on identity map. Identity map now considers the inheritance when creating the caching keys
fc2823a
This page is out of date. Refresh to see the latest.
4  activerecord/lib/active_record/base.rb
@@ -830,6 +830,10 @@ def symbolized_base_class
830 830
         @symbolized_base_class ||= base_class.to_s.to_sym
831 831
       end
832 832
 
  833
+      def symbolized_sti_name
  834
+        @symbolized_sti_name ||= sti_name ? sti_name.to_sym : symbolized_base_class
  835
+      end
  836
+
833 837
       # Returns the base AR subclass that this class descends from. If A
834 838
       # extends AR::Base, A.base_class will return A. If B descends from A
835 839
       # through some arbitrarily deep hierarchy, B.base_class will return A.
10  activerecord/lib/active_record/identity_map.rb
@@ -49,7 +49,7 @@ def without
49 49
       end
50 50
 
51 51
       def get(klass, primary_key)
52  
-        record = repository[klass.symbolized_base_class][primary_key]
  52
+        record = repository[klass.symbolized_sti_name][primary_key]
53 53
 
54 54
         if record.is_a?(klass)
55 55
           ActiveSupport::Notifications.instrument("identity.active_record",
@@ -64,15 +64,15 @@ def get(klass, primary_key)
64 64
       end
65 65
 
66 66
       def add(record)
67  
-        repository[record.class.symbolized_base_class][record.id] = record
  67
+        repository[record.class.symbolized_sti_name][record.id] = record
68 68
       end
69 69
 
70 70
       def remove(record)
71  
-        repository[record.class.symbolized_base_class].delete(record.id)
  71
+        repository[record.class.symbolized_sti_name].delete(record.id)
72 72
       end
73 73
 
74  
-      def remove_by_id(symbolized_base_class, id)
75  
-        repository[symbolized_base_class].delete(id)
  74
+      def remove_by_id(symbolized_sti_name, id)
  75
+        repository[symbolized_sti_name].delete(id)
76 76
       end
77 77
 
78 78
       def clear
35  activerecord/test/cases/identity_map_test.rb
@@ -129,6 +129,41 @@ def test_creation
129 129
   end
130 130
 
131 131
   ##############################################################################
  132
+  # Tests checking if IM is functioning properly on classes with multiple      #
  133
+  # types of inheritance                                                       #
  134
+  ##############################################################################
  135
+
  136
+  def test_inherited_without_type_attribute_without_identity_map
  137
+    ActiveRecord::IdentityMap.without do
  138
+      p1 = DestructivePirate.create!(:catchphrase => "I'm not a regular Pirate")
  139
+      p2 = Pirate.find(p1.id)
  140
+      assert_not_same(p1, p2)
  141
+    end
  142
+  end
  143
+  
  144
+  def test_inherited_with_type_attribute_without_identity_map
  145
+    ActiveRecord::IdentityMap.without do
  146
+      c = comments(:sub_special_comment)
  147
+      c1 = SubSpecialComment.find(c.id)
  148
+      c2 = Comment.find(c.id)
  149
+      assert_same(c1.class, c2.class)
  150
+    end
  151
+  end
  152
+
  153
+  def test_inherited_without_type_attribute
  154
+    p1 = DestructivePirate.create!(:catchphrase => "I'm not a regular Pirate")
  155
+    p2 = Pirate.find(p1.id)
  156
+    assert_not_same(p1, p2)
  157
+  end
  158
+
  159
+  def test_inherited_with_type_attribute
  160
+    c = comments(:sub_special_comment)
  161
+    c1 = SubSpecialComment.find(c.id)
  162
+    c2 = Comment.find(c.id)
  163
+    assert_same(c1, c2)
  164
+  end
  165
+
  166
+  ##############################################################################
132 167
   # Tests checking dirty attribute behaviour with IM                           #
133 168
   ##############################################################################
134 169
 
Commit_comment_tip

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.