From 22ca710f20c3c656811df006cbf1f4dbc359f7a6 Mon Sep 17 00:00:00 2001 From: Ryuta Kamizono Date: Mon, 21 Nov 2016 05:01:41 +0900 Subject: [PATCH 1/2] Fix unscope with subquery Currently cannot unscope subquery properly. This commit fixes the issue. Fixes #26323. --- .../active_record/relation/where_clause.rb | 29 ++++++++++++------- activerecord/test/cases/relations_test.rb | 12 ++++++++ 2 files changed, 31 insertions(+), 10 deletions(-) diff --git a/activerecord/lib/active_record/relation/where_clause.rb b/activerecord/lib/active_record/relation/where_clause.rb index 402f8acfd1..e862a9048e 100644 --- a/activerecord/lib/active_record/relation/where_clause.rb +++ b/activerecord/lib/active_record/relation/where_clause.rb @@ -25,10 +25,7 @@ def merge(other) end def except(*columns) - WhereClause.new( - predicates_except(columns), - binds_except(columns), - ) + WhereClause.new(*except_predicates_and_binds(columns)) end def or(other) @@ -132,20 +129,32 @@ def invert_predicate(node) end end - def predicates_except(columns) - predicates.reject do |node| + def except_predicates_and_binds(columns) + except_binds = [] + binds_index = 0 + + predicates = self.predicates.reject do |node| case node when Arel::Nodes::Between, Arel::Nodes::In, Arel::Nodes::NotIn, Arel::Nodes::Equality, Arel::Nodes::NotEqual, Arel::Nodes::LessThan, Arel::Nodes::LessThanOrEqual, Arel::Nodes::GreaterThan, Arel::Nodes::GreaterThanOrEqual + binds_contains = node.grep(Arel::Nodes::BindParam).size subrelation = (node.left.kind_of?(Arel::Attributes::Attribute) ? node.left : node.right) columns.include?(subrelation.name.to_s) + end.tap do |except| + if except && binds_contains > 0 + (binds_index...(binds_index + binds_contains)).each do |i| + except_binds[i] = true + end + + binds_index += binds_contains + end end end - end - def binds_except(columns) - binds.reject do |attr| - columns.include?(attr.name) + binds = self.binds.reject.with_index do |_, i| + except_binds[i] end + + [predicates, binds] end def predicates_with_wrapped_sql_literals diff --git a/activerecord/test/cases/relations_test.rb b/activerecord/test/cases/relations_test.rb index 96833ad428..716ace9686 100644 --- a/activerecord/test/cases/relations_test.rb +++ b/activerecord/test/cases/relations_test.rb @@ -1962,6 +1962,18 @@ def test_presence assert !Post.all.respond_to?(:by_lifo) end + def test_unscope_with_subquery + p1 = Post.where(id: 1) + p2 = Post.where(id: 2) + + assert_not_equal p1, p2 + + comments = Comment.where(post: p1).unscope(where: :post_id).where(post: p2) + + assert_not_equal p1.first.comments, comments + assert_equal p2.first.comments, comments + end + def test_unscope_removes_binds left = Post.where(id: Arel::Nodes::BindParam.new) column = Post.columns_hash["id"] From bbf1f152644177774710c52a230813e7d06935df Mon Sep 17 00:00:00 2001 From: Ryuta Kamizono Date: Mon, 13 Feb 2017 12:44:37 +0900 Subject: [PATCH 2/2] Refactor `except_predicates_and_binds` to avoid `tap` --- .../active_record/relation/where_clause.rb | 27 ++++++++++--------- 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/activerecord/lib/active_record/relation/where_clause.rb b/activerecord/lib/active_record/relation/where_clause.rb index e862a9048e..e550fcf7ae 100644 --- a/activerecord/lib/active_record/relation/where_clause.rb +++ b/activerecord/lib/active_record/relation/where_clause.rb @@ -134,20 +134,23 @@ def except_predicates_and_binds(columns) binds_index = 0 predicates = self.predicates.reject do |node| - case node - when Arel::Nodes::Between, Arel::Nodes::In, Arel::Nodes::NotIn, Arel::Nodes::Equality, Arel::Nodes::NotEqual, Arel::Nodes::LessThan, Arel::Nodes::LessThanOrEqual, Arel::Nodes::GreaterThan, Arel::Nodes::GreaterThanOrEqual - binds_contains = node.grep(Arel::Nodes::BindParam).size - subrelation = (node.left.kind_of?(Arel::Attributes::Attribute) ? node.left : node.right) - columns.include?(subrelation.name.to_s) - end.tap do |except| - if except && binds_contains > 0 - (binds_index...(binds_index + binds_contains)).each do |i| - except_binds[i] = true - end - - binds_index += binds_contains + except = \ + case node + when Arel::Nodes::Between, Arel::Nodes::In, Arel::Nodes::NotIn, Arel::Nodes::Equality, Arel::Nodes::NotEqual, Arel::Nodes::LessThan, Arel::Nodes::LessThanOrEqual, Arel::Nodes::GreaterThan, Arel::Nodes::GreaterThanOrEqual + binds_contains = node.grep(Arel::Nodes::BindParam).size + subrelation = (node.left.kind_of?(Arel::Attributes::Attribute) ? node.left : node.right) + columns.include?(subrelation.name.to_s) end + + if except && binds_contains > 0 + (binds_index...(binds_index + binds_contains)).each do |i| + except_binds[i] = true + end + + binds_index += binds_contains end + + except end binds = self.binds.reject.with_index do |_, i|