From 5cbed2bc606750c71ee231d2b816471fe241459e Mon Sep 17 00:00:00 2001 From: thedarkone Date: Wed, 11 Sep 2013 14:18:31 +0200 Subject: Relation#merge should not lose readonly(false) flag. The original code ignores the `false` value because `false.blank? # => true`. --- activerecord/CHANGELOG.md | 4 ++++ activerecord/lib/active_record/relation/merger.rb | 6 +++++- activerecord/test/cases/relation_test.rb | 8 ++++++++ 3 files changed, 17 insertions(+), 1 deletion(-) (limited to 'activerecord') diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 9041f9d33b..d2f2c745a0 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,7 @@ +* Fix `AR::Relation#merge` sometimes failing to preserve `readonly(false)` flag. + + *thedarkone* + * Re-use `order` argument pre-processing for `reorder`. *Paul Nikitochkin* diff --git a/activerecord/lib/active_record/relation/merger.rb b/activerecord/lib/active_record/relation/merger.rb index 530c47d0d0..c05632e688 100644 --- a/activerecord/lib/active_record/relation/merger.rb +++ b/activerecord/lib/active_record/relation/merger.rb @@ -58,7 +58,11 @@ module ActiveRecord def merge normal_values.each do |name| value = values[name] - relation.send("#{name}!", *value) unless value.blank? + # The unless clause is here mostly for performance reasons (since the `send` call might be moderately + # expensive), most of the time the value is going to be `nil` or `.blank?`, the only catch is that + # `false.blank?` returns `true`, so there needs to be an extra check so that explicit `false` values + # don't fall through the cracks. + relation.send("#{name}!", *value) unless value.nil? || (value.blank? && false != value) end merge_multi_values diff --git a/activerecord/test/cases/relation_test.rb b/activerecord/test/cases/relation_test.rb index a327b0d3e5..e77de19ac3 100644 --- a/activerecord/test/cases/relation_test.rb +++ b/activerecord/test/cases/relation_test.rb @@ -193,6 +193,14 @@ module ActiveRecord assert_equal ['foo = bar'], relation.where_values end + def test_merging_readonly_false + relation = Relation.new FakeKlass, :b + readonly_false_relation = relation.readonly(false) + # test merging in both directions + assert_equal false, relation.merge(readonly_false_relation).readonly_value + assert_equal false, readonly_false_relation.merge(relation).readonly_value + end + def test_relation_merging_with_merged_joins_as_symbols special_comments_with_ratings = SpecialComment.joins(:ratings) posts_with_special_comments_with_ratings = Post.group("posts.id").joins(:special_comments).merge(special_comments_with_ratings) -- cgit v1.2.3