Identity Map caching bug #423

Merged
merged 3 commits into from May 6, 2011

3 participants

@richardiux

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

@richardiux 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
@miloops

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)

@richardiux

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 May 5, 2011
@richardiux richardiux Test: identity on inherited classes should behave the same when turne…
…d on or off
d53b406
@richardiux richardiux 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
@josevalim
Ruby on Rails member

@miloops, +1 for merging these commits?

@miloops

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.

@josevalim
Ruby on Rails member

Is symbolized base class still used somewhere else?

@josevalim josevalim merged commit b8f08c4 into rails:master May 6, 2011
@josevalim josevalim added a commit that referenced this pull request May 6, 2011
@josevalim 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
@josevalim
Ruby on Rails member

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?

@richardiux

@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?

@josevalim
Ruby on Rails member

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.

@richardiux

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.

@josevalim
Ruby on Rails member

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.

@josevalim
Ruby on Rails member

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. :)

@josevalim
Ruby on Rails member

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?

@josevalim
Ruby on Rails member

Here is the fix:

ba23bf4

Now everything pass. :)

@richardiux

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