From d476553d1cdeee0805585c2d4e2e6ee6be841288 Mon Sep 17 00:00:00 2001 From: Ryuta Kamizono Date: Sun, 16 Jul 2017 21:31:08 +0900 Subject: Don't cache `scope_for_create` I investigated where `scope_for_create` is reused in tests with the following code: ```diff --- a/activerecord/lib/active_record/relation.rb +++ b/activerecord/lib/active_record/relation.rb @@ -590,6 +590,10 @@ def where_values_hash(relation_table_name = table_name) end def scope_for_create + if defined?(@scope_for_create) && @scope_for_create + puts caller + puts "defined" + end @scope_for_create ||= where_values_hash.merge!(create_with_value.stringify_keys) end ``` It was hit only `test_scope_for_create_is_cached`. This means that `scope_for_create` will not be reused in normal use cases. So we can remove caching `scope_for_create` to respect changing `where_clause` and `create_with_value`. --- activerecord/lib/active_record/associations/association.rb | 2 +- .../lib/active_record/associations/singular_association.rb | 2 +- activerecord/lib/active_record/relation.rb | 4 ++-- activerecord/test/cases/relation_test.rb | 12 ++---------- 4 files changed, 6 insertions(+), 14 deletions(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/associations/association.rb b/activerecord/lib/active_record/associations/association.rb index 6acf831759..c166cfd68f 100644 --- a/activerecord/lib/active_record/associations/association.rb +++ b/activerecord/lib/active_record/associations/association.rb @@ -171,7 +171,7 @@ module ActiveRecord skip_assign = [reflection.foreign_key, reflection.type].compact assigned_keys = record.changed_attribute_names_to_save assigned_keys += except_from_scope_attributes.keys.map(&:to_s) - attributes = scope_for_create.except(*(assigned_keys - skip_assign)) + attributes = scope_for_create.except!(*(assigned_keys - skip_assign)) record.send(:_assign_attributes, attributes) if attributes.any? set_inverse_instance(record) end diff --git a/activerecord/lib/active_record/associations/singular_association.rb b/activerecord/lib/active_record/associations/singular_association.rb index 4896afcca7..66993b9bf7 100644 --- a/activerecord/lib/active_record/associations/singular_association.rb +++ b/activerecord/lib/active_record/associations/singular_association.rb @@ -31,7 +31,7 @@ module ActiveRecord private def scope_for_create - super.except(klass.primary_key) + super.except!(klass.primary_key) end def find_target diff --git a/activerecord/lib/active_record/relation.rb b/activerecord/lib/active_record/relation.rb index 23cb1baaed..c5d98e970a 100644 --- a/activerecord/lib/active_record/relation.rb +++ b/activerecord/lib/active_record/relation.rb @@ -556,7 +556,7 @@ module ActiveRecord end def reset - @to_sql = @scope_for_create = @arel = @loaded = @should_eager_load = nil + @to_sql = @arel = @loaded = @should_eager_load = nil @records = [].freeze @offsets = {} self @@ -590,7 +590,7 @@ module ActiveRecord end def scope_for_create - @scope_for_create ||= where_values_hash.merge!(create_with_value.stringify_keys) + where_values_hash.merge!(create_with_value.stringify_keys) end # Returns true if relation needs eager loading. diff --git a/activerecord/test/cases/relation_test.rb b/activerecord/test/cases/relation_test.rb index d2859cbafd..82b071fb98 100644 --- a/activerecord/test/cases/relation_test.rb +++ b/activerecord/test/cases/relation_test.rb @@ -88,22 +88,14 @@ module ActiveRecord end def test_create_with_value_with_wheres - relation = Relation.new(Post, Post.arel_table, Post.predicate_builder) - relation.where!(id: 10) - relation.create_with_value = { hello: "world" } - assert_equal({ "hello" => "world", "id" => 10 }, relation.scope_for_create) - end - - # FIXME: is this really wanted or expected behavior? - def test_scope_for_create_is_cached relation = Relation.new(Post, Post.arel_table, Post.predicate_builder) assert_equal({}, relation.scope_for_create) relation.where!(id: 10) - assert_equal({}, relation.scope_for_create) + assert_equal({ "id" => 10 }, relation.scope_for_create) relation.create_with_value = { hello: "world" } - assert_equal({}, relation.scope_for_create) + assert_equal({ "hello" => "world", "id" => 10 }, relation.scope_for_create) end def test_bad_constants_raise_errors -- cgit v1.2.3