From b29e868cc335e2a696c36b6d7ccdf199ed8abf70 Mon Sep 17 00:00:00 2001 From: Arturo Pie Date: Sun, 25 Mar 2012 23:30:34 -0400 Subject: Adds a test that breaks IM when using #select --- activerecord/test/cases/identity_map_test.rb | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) (limited to 'activerecord') diff --git a/activerecord/test/cases/identity_map_test.rb b/activerecord/test/cases/identity_map_test.rb index 24132c7617..2a490126e2 100644 --- a/activerecord/test/cases/identity_map_test.rb +++ b/activerecord/test/cases/identity_map_test.rb @@ -404,18 +404,12 @@ class IdentityMapTest < ActiveRecord::TestCase assert comment.save end - def test_find_using_select_and_identity_map - author_id, author = Author.select('id').order(:id).first, Author.order(:id).first - - assert_equal author_id, author - assert_same author_id, author - assert_not_nil author.name - - post, post_id = Post.order(:id).first, Post.select('id').order(:id).first - - assert_equal post_id, post - assert_same post_id, post - assert_not_nil post.title + def test_do_not_add_to_identity_map_if_record_do_not_contain_all_columns + post = Post.select(:id).first + comment = post.comments[0] + assert_nothing_raised do + assert_not_nil comment.post.title + end end # Currently AR is not allowing changing primary key (see Persistence#update) -- cgit v1.2.3 From 2c8f84d3a6b199b04394ed1a5bba3bc0d8f1e811 Mon Sep 17 00:00:00 2001 From: Arturo Pie Date: Sun, 25 Mar 2012 23:38:10 -0400 Subject: Do not add record to identity map if the record doesn't have values for all the columns, so we don't get 'MissingAttributeError' later when trying to access other fields of the same record. --- activerecord/lib/active_record/identity_map.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/identity_map.rb b/activerecord/lib/active_record/identity_map.rb index 680d9ffea0..7ff755110f 100644 --- a/activerecord/lib/active_record/identity_map.rb +++ b/activerecord/lib/active_record/identity_map.rb @@ -90,7 +90,7 @@ module ActiveRecord end def add(record) - repository[record.class.symbolized_sti_name][record.id] = record + repository[record.class.symbolized_sti_name][record.id] = record if record.class.column_names - record.attribute_names == [] end def remove(record) -- cgit v1.2.3 From 714a2c810deb9d219442f7c981e61eb6d5c45d33 Mon Sep 17 00:00:00 2001 From: Arturo Pie Date: Mon, 26 Mar 2012 22:45:48 -0400 Subject: refactor the checking of the attributes of the record in IdentityMap#add, so it's more readable --- activerecord/lib/active_record/identity_map.rb | 8 +++++++- activerecord/test/cases/identity_map_test.rb | 9 +++++---- 2 files changed, 12 insertions(+), 5 deletions(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/identity_map.rb b/activerecord/lib/active_record/identity_map.rb index 7ff755110f..f3891b406e 100644 --- a/activerecord/lib/active_record/identity_map.rb +++ b/activerecord/lib/active_record/identity_map.rb @@ -90,7 +90,7 @@ module ActiveRecord end def add(record) - repository[record.class.symbolized_sti_name][record.id] = record if record.class.column_names - record.attribute_names == [] + repository[record.class.symbolized_sti_name][record.id] = record if contain_all_columns(record) end def remove(record) @@ -104,6 +104,12 @@ module ActiveRecord def clear repository.clear end + + private + + def contain_all_columns(record) + (record.class.column_names - record.attribute_names) == [] + end end # Reinitialize an Identity Map model object from +coder+. diff --git a/activerecord/test/cases/identity_map_test.rb b/activerecord/test/cases/identity_map_test.rb index 2a490126e2..63d0bcf1fc 100644 --- a/activerecord/test/cases/identity_map_test.rb +++ b/activerecord/test/cases/identity_map_test.rb @@ -404,11 +404,12 @@ class IdentityMapTest < ActiveRecord::TestCase assert comment.save end - def test_do_not_add_to_identity_map_if_record_do_not_contain_all_columns - post = Post.select(:id).first - comment = post.comments[0] + def test_do_not_add_to_repository_if_record_does_not_contain_all_columns + author = Author.select(:id).first + post = author.posts.first + assert_nothing_raised do - assert_not_nil comment.post.title + assert_not_nil post.author.name end end -- cgit v1.2.3 From 6896cd451545679a6413939fc4eae68f3cc3ba8b Mon Sep 17 00:00:00 2001 From: Arturo Pie Date: Mon, 26 Mar 2012 22:51:53 -0400 Subject: refactor instantiate method in base, so we remove nesting if's which make the code harder to read. Minor changes to contain_all_columns in IdentityMap. Conflicts: activerecord/lib/active_record/base.rb --- activerecord/lib/active_record/identity_map.rb | 6 +++--- activerecord/lib/active_record/inheritance.rb | 25 ++++++++++++++++--------- 2 files changed, 19 insertions(+), 12 deletions(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/identity_map.rb b/activerecord/lib/active_record/identity_map.rb index f3891b406e..b1da547142 100644 --- a/activerecord/lib/active_record/identity_map.rb +++ b/activerecord/lib/active_record/identity_map.rb @@ -90,7 +90,7 @@ module ActiveRecord end def add(record) - repository[record.class.symbolized_sti_name][record.id] = record if contain_all_columns(record) + repository[record.class.symbolized_sti_name][record.id] = record if contain_all_columns?(record) end def remove(record) @@ -107,8 +107,8 @@ module ActiveRecord private - def contain_all_columns(record) - (record.class.column_names - record.attribute_names) == [] + def contain_all_columns?(record) + (record.class.column_names - record.attribute_names).empty? end end diff --git a/activerecord/lib/active_record/inheritance.rb b/activerecord/lib/active_record/inheritance.rb index de9461982a..3e12a67c2a 100644 --- a/activerecord/lib/active_record/inheritance.rb +++ b/activerecord/lib/active_record/inheritance.rb @@ -63,15 +63,7 @@ module ActiveRecord record_id = sti_class.primary_key && record[sti_class.primary_key] if ActiveRecord::IdentityMap.enabled? && record_id - if (column = sti_class.columns_hash[sti_class.primary_key]) && column.number? - record_id = record_id.to_i - end - if instance = IdentityMap.get(sti_class, record_id) - instance.reinit_with('attributes' => record) - else - instance = sti_class.allocate.init_with('attributes' => record) - IdentityMap.add(instance) - end + instance = use_identity_map(sti_class, record_id, record) else instance = sti_class.allocate.init_with('attributes' => record) end @@ -122,6 +114,21 @@ module ActiveRecord private + def use_identity_map(sti_class, record_id, record) + if (column = sti_class.columns_hash[sti_class.primary_key]) && column.number? + record_id = record_id.to_i + end + + if instance = IdentityMap.get(sti_class, record_id) + instance.reinit_with('attributes' => record) + else + instance = sti_class.allocate.init_with('attributes' => record) + IdentityMap.add(instance) + end + + instance + end + def find_sti_class(type_name) if type_name.blank? || !columns_hash.include?(inheritance_column) self -- cgit v1.2.3