From 425ba83c28214ca97c5d3600c16ad7a796cd33e6 Mon Sep 17 00:00:00 2001 From: Maxime Lapointe Date: Thu, 13 Jul 2017 11:08:35 -0400 Subject: Bugfix: unscope(where: [columns]) would not remove the correct binds sometimes Post.where(id: 1).or(Post.where(id: 2)).where(foo: 3).unscope(where: :foo).where_clause.binds.map(&:value) Would return [2, 3] instead of the expected [1,2] --- activerecord/CHANGELOG.md | 15 +++++++++++++++ .../lib/active_record/relation/where_clause.rb | 3 ++- activerecord/test/cases/relation/where_clause_test.rb | 18 +++++++++++++++++- 3 files changed, 34 insertions(+), 2 deletions(-) diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 5ffbed28f6..bb617624c0 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,18 @@ +* Fix `unscoped(where: [columns])` removing the wrong bind values + + When the `where` is called on a relation after a `or`, unscoping the column of that later `where` removed + bind values used by the `or` instead. (possibly other cases too) + + ``` + Post.where(id: 1).or(Post.where(id: 2)).where(foo: 3).unscope(where: :foo).to_sql + # Currently: + # SELECT "posts".* FROM "posts" WHERE ("posts"."id" = 2 OR "posts"."id" = 3) + # With fix: + # SELECT "posts".* FROM "posts" WHERE ("posts"."id" = 1 OR "posts"."id" = 2) + ``` + + *Maxime Handfield Lapointe* + * Change sqlite3 boolean serialization to use 1 and 0 SQLite natively recognizes 1 and 0 as true and false, but does not natively diff --git a/activerecord/lib/active_record/relation/where_clause.rb b/activerecord/lib/active_record/relation/where_clause.rb index 119910ee79..7542ea4d8c 100644 --- a/activerecord/lib/active_record/relation/where_clause.rb +++ b/activerecord/lib/active_record/relation/where_clause.rb @@ -136,10 +136,11 @@ module ActiveRecord binds_index = 0 predicates = self.predicates.reject do |node| + binds_contains = node.grep(Arel::Nodes::BindParam).size if node.is_a?(Arel::Nodes::Node) + 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 diff --git a/activerecord/test/cases/relation/where_clause_test.rb b/activerecord/test/cases/relation/where_clause_test.rb index f8eb0dee91..57d7b315b0 100644 --- a/activerecord/test/cases/relation/where_clause_test.rb +++ b/activerecord/test/cases/relation/where_clause_test.rb @@ -97,7 +97,7 @@ class ActiveRecord::Relation assert_equal expected, original.invert end - test "accept removes binary predicates referencing a given column" do + test "except removes binary predicates referencing a given column" do where_clause = WhereClause.new([ table["id"].in([1, 2, 3]), table["name"].eq(bind_param), @@ -111,6 +111,22 @@ class ActiveRecord::Relation assert_equal expected, where_clause.except("id", "name") end + test "except jumps over unhandled binds (like with OR) correctly" do + wcs = (0..9).map do |i| + WhereClause.new([table["id#{i}"].eq(bind_param)], [bind_attribute("id#{i}", i)]) + end + + wc = wcs[0] + wcs[1] + wcs[2].or(wcs[3]) + wcs[4] + wcs[5] + wcs[6].or(wcs[7]) + wcs[8] + wcs[9] + + expected = wcs[0] + wcs[2].or(wcs[3]) + wcs[5] + wcs[6].or(wcs[7]) + wcs[9] + actual = wc.except("id1", "id2", "id4", "id7", "id8") + + # Easier to read than the inspect of where_clause + assert_equal expected.ast.to_sql, actual.ast.to_sql + assert_equal expected.binds.map(&:value), actual.binds.map(&:value) + assert_equal expected, actual + end + test "ast groups its predicates with AND" do predicates = [ table["id"].in([1, 2, 3]), -- cgit v1.2.3