Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Identity Map caching bug #423

Merged
merged 3 commits into from

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

richardiux added some commits
José Valim
Owner

@miloops, +1 for merging these commits?

Emilio Tagua

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
José Valim josevalim referenced this pull request from a commit
José Valim josevalim 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
Commits on May 5, 2011
  1. Richard Millan

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

    richardiux authored
    …els that use the same database table shouldn't be retrieved as the same object when there is not a type attribute.
  2. Richard Millan
Commits on May 6, 2011
  1. Richard Millan

    Adding base method symbolized_sti_name to activerecord base to be use…

    richardiux authored
    …d on identity map. Identity map now considers the inheritance when creating the caching keys
This page is out of date. Refresh to see the latest.
4 activerecord/lib/active_record/base.rb
View
@@ -830,6 +830,10 @@ def symbolized_base_class
@symbolized_base_class ||= base_class.to_s.to_sym
end
+ def symbolized_sti_name
+ @symbolized_sti_name ||= sti_name ? sti_name.to_sym : symbolized_base_class
+ end
+
# Returns the base AR subclass that this class descends from. If A
# extends AR::Base, A.base_class will return A. If B descends from A
# through some arbitrarily deep hierarchy, B.base_class will return A.
10 activerecord/lib/active_record/identity_map.rb
View
@@ -49,7 +49,7 @@ def without
end
def get(klass, primary_key)
- record = repository[klass.symbolized_base_class][primary_key]
+ record = repository[klass.symbolized_sti_name][primary_key]
if record.is_a?(klass)
ActiveSupport::Notifications.instrument("identity.active_record",
@@ -64,15 +64,15 @@ def get(klass, primary_key)
end
def add(record)
- repository[record.class.symbolized_base_class][record.id] = record
+ repository[record.class.symbolized_sti_name][record.id] = record
end
def remove(record)
- repository[record.class.symbolized_base_class].delete(record.id)
+ repository[record.class.symbolized_sti_name].delete(record.id)
end
- def remove_by_id(symbolized_base_class, id)
- repository[symbolized_base_class].delete(id)
+ def remove_by_id(symbolized_sti_name, id)
+ repository[symbolized_sti_name].delete(id)
end
def clear
35 activerecord/test/cases/identity_map_test.rb
View
@@ -129,6 +129,41 @@ def test_creation
end
##############################################################################
+ # Tests checking if IM is functioning properly on classes with multiple #
+ # types of inheritance #
+ ##############################################################################
+
+ def test_inherited_without_type_attribute_without_identity_map
+ ActiveRecord::IdentityMap.without do
+ p1 = DestructivePirate.create!(:catchphrase => "I'm not a regular Pirate")
+ p2 = Pirate.find(p1.id)
+ assert_not_same(p1, p2)
+ end
+ end
+
+ def test_inherited_with_type_attribute_without_identity_map
+ ActiveRecord::IdentityMap.without do
+ c = comments(:sub_special_comment)
+ c1 = SubSpecialComment.find(c.id)
+ c2 = Comment.find(c.id)
+ assert_same(c1.class, c2.class)
+ end
+ end
+
+ def test_inherited_without_type_attribute
+ p1 = DestructivePirate.create!(:catchphrase => "I'm not a regular Pirate")
+ p2 = Pirate.find(p1.id)
+ assert_not_same(p1, p2)
+ end
+
+ def test_inherited_with_type_attribute
+ c = comments(:sub_special_comment)
+ c1 = SubSpecialComment.find(c.id)
+ c2 = Comment.find(c.id)
+ assert_same(c1, c2)
+ end
+
+ ##############################################################################
# Tests checking dirty attribute behaviour with IM #
##############################################################################
Something went wrong with that request. Please try again.