aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorRyuta Kamizono <kamipo@gmail.com>2019-04-09 23:25:30 +0900
committerRyuta Kamizono <kamipo@gmail.com>2019-04-19 13:49:31 +0900
commit12a9664ff60f0e2712fd1f79f8dbec06e2f004a2 (patch)
tree1a45630ea34f3c2fee7f1b3be1ffb9d894c9f442
parent16dae7684edc480ee3fe65dfff8e19989402c987 (diff)
downloadrails-12a9664ff60f0e2712fd1f79f8dbec06e2f004a2.tar.gz
rails-12a9664ff60f0e2712fd1f79f8dbec06e2f004a2.tar.bz2
rails-12a9664ff60f0e2712fd1f79f8dbec06e2f004a2.zip
Deprecate `where.not` working as NOR and will be changed to NAND in Rails 6.1
`where.not` with polymorphic association is partly fixed incidentally at 213796f (refer #33493, #26207, #17010, #16983, #14161), and I've added test case e9ba12f to avoid lose that fix accidentally in the future. In Rails 5.2, `where.not(polymorphic: object)` works as expected as NAND, but `where.not(polymorphic_type: object.class.polymorphic_name, polymorphic_id: object.id)` still unexpectedly works as NOR. To will make `where.not` working desiredly as NAND in Rails 6.1, this deprecates `where.not` working as NOR. If people want to continue NOR conditions, we'd encourage to them to `where.not` each conditions manually. ```ruby all = [treasures(:diamond), treasures(:sapphire), cars(:honda), treasures(:sapphire)] assert_equal all, PriceEstimate.all.map(&:estimate_of) ``` In Rails 6.0: ```ruby sapphire = treasures(:sapphire) nor = all.reject { |e| e.estimate_of_type == sapphire.class.polymorphic_name }.reject { |e| e.estimate_of_id == sapphire.id } assert_equal [cars(:honda)], nor without_sapphire = PriceEstimate.where.not( estimate_of_type: sapphire.class.polymorphic_name, estimate_of_id: sapphire.id ) assert_equal nor, without_sapphire.map(&:estimate_of) ``` In Rails 6.1: ```ruby sapphire = treasures(:sapphire) nand = all - [sapphire] assert_equal [treasures(:diamond), cars(:honda)], nand without_sapphire = PriceEstimate.where.not( estimate_of_type: sapphire.class.polymorphic_name, estimate_of_id: sapphire.id ) assert_equal nand, without_sapphire.map(&:estimate_of) ``` Resolves #31209.
-rw-r--r--activerecord/CHANGELOG.md41
-rw-r--r--activerecord/lib/active_record/relation/query_methods.rb21
-rw-r--r--activerecord/lib/active_record/relation/where_clause.rb14
-rw-r--r--activerecord/test/cases/relation/where_clause_test.rb2
-rw-r--r--activerecord/test/cases/relation/where_test.rb55
5 files changed, 118 insertions, 15 deletions
diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index 485547f036..adc7e754a4 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -1,3 +1,44 @@
+* Deprecate `where.not` working as NOR and will be changed to NAND in Rails 6.1.
+
+ ```ruby
+ all = [treasures(:diamond), treasures(:sapphire), cars(:honda), treasures(:sapphire)]
+ assert_equal all, PriceEstimate.all.map(&:estimate_of)
+ ```
+
+ In Rails 6.0:
+
+ ```ruby
+ sapphire = treasures(:sapphire)
+
+ nor = all.reject { |e|
+ e.estimate_of_type == sapphire.class.polymorphic_name
+ }.reject { |e|
+ e.estimate_of_id == sapphire.id
+ }
+ assert_equal [cars(:honda)], nor
+
+ without_sapphire = PriceEstimate.where.not(
+ estimate_of_type: sapphire.class.polymorphic_name, estimate_of_id: sapphire.id
+ )
+ assert_equal nor, without_sapphire.map(&:estimate_of)
+ ```
+
+ In Rails 6.1:
+
+ ```ruby
+ sapphire = treasures(:sapphire)
+
+ nand = all - [sapphire]
+ assert_equal [treasures(:diamond), cars(:honda)], nand
+
+ without_sapphire = PriceEstimate.where.not(
+ estimate_of_type: sapphire.class.polymorphic_name, estimate_of_id: sapphire.id
+ )
+ assert_equal nand, without_sapphire.map(&:estimate_of)
+ ```
+
+ *Ryuta Kamizono*
+
* Fix dirty tracking after rollback.
Fixes #15018, #30167, #33868.
diff --git a/activerecord/lib/active_record/relation/query_methods.rb b/activerecord/lib/active_record/relation/query_methods.rb
index 90b5e9a118..0a481dcbdc 100644
--- a/activerecord/lib/active_record/relation/query_methods.rb
+++ b/activerecord/lib/active_record/relation/query_methods.rb
@@ -41,18 +41,31 @@ module ActiveRecord
#
# User.where.not(name: %w(Ko1 Nobu))
# # SELECT * FROM users WHERE name NOT IN ('Ko1', 'Nobu')
- #
- # User.where.not(name: "Jon", role: "admin")
- # # SELECT * FROM users WHERE name != 'Jon' AND role != 'admin'
def not(opts, *rest)
opts = sanitize_forbidden_attributes(opts)
where_clause = @scope.send(:where_clause_factory).build(opts, rest)
@scope.references!(PredicateBuilder.references(opts)) if Hash === opts
- @scope.where_clause += where_clause.invert
+
+ if not_behaves_as_nor?(opts)
+ ActiveSupport::Deprecation.warn(<<~MSG.squish)
+ NOT conditions will no longer behave as NOR in Rails 6.1.
+ To continue using NOR conditions, NOT each conditions manually
+ (`#{ opts.keys.map { |key| ".where.not(#{key.inspect} => ...)" }.join }`).
+ MSG
+ @scope.where_clause += where_clause.invert(:nor)
+ else
+ @scope.where_clause += where_clause.invert
+ end
+
@scope
end
+
+ private
+ def not_behaves_as_nor?(opts)
+ opts.is_a?(Hash) && opts.size > 1
+ end
end
FROZEN_EMPTY_ARRAY = [].freeze
diff --git a/activerecord/lib/active_record/relation/where_clause.rb b/activerecord/lib/active_record/relation/where_clause.rb
index 47728aac30..b91b135867 100644
--- a/activerecord/lib/active_record/relation/where_clause.rb
+++ b/activerecord/lib/active_record/relation/where_clause.rb
@@ -70,7 +70,15 @@ module ActiveRecord
predicates == other.predicates
end
- def invert
+ def invert(as = :nand)
+ if predicates.size == 1
+ inverted_predicates = [ invert_predicate(predicates.first) ]
+ elsif as == :nor
+ inverted_predicates = predicates.map { |node| invert_predicate(node) }
+ else
+ inverted_predicates = [ Arel::Nodes::Not.new(ast) ]
+ end
+
WhereClause.new(inverted_predicates)
end
@@ -115,10 +123,6 @@ module ActiveRecord
node.respond_to?(:operator) && node.operator == :==
end
- def inverted_predicates
- predicates.map { |node| invert_predicate(node) }
- end
-
def invert_predicate(node)
case node
when NilClass
diff --git a/activerecord/test/cases/relation/where_clause_test.rb b/activerecord/test/cases/relation/where_clause_test.rb
index 0b06cec40b..b26a1a1d80 100644
--- a/activerecord/test/cases/relation/where_clause_test.rb
+++ b/activerecord/test/cases/relation/where_clause_test.rb
@@ -106,7 +106,7 @@ class ActiveRecord::Relation
Arel::Nodes::Not.new(random_object)
])
- assert_equal expected, original.invert
+ assert_equal expected, original.invert(:nor)
end
test "except removes binary predicates referencing a given column" do
diff --git a/activerecord/test/cases/relation/where_test.rb b/activerecord/test/cases/relation/where_test.rb
index b045184d7d..2ecccfe1f3 100644
--- a/activerecord/test/cases/relation/where_test.rb
+++ b/activerecord/test/cases/relation/where_test.rb
@@ -115,13 +115,58 @@ module ActiveRecord
assert_equal expected.to_sql, actual.to_sql
end
- def test_polymorphic_shallow_where_not
- treasure = treasures(:sapphire)
+ def test_where_not_polymorphic_association
+ sapphire = treasures(:sapphire)
- expected = [price_estimates(:diamond), price_estimates(:honda)]
- actual = PriceEstimate.where.not(estimate_of: treasure)
+ all = [treasures(:diamond), sapphire, cars(:honda), sapphire]
+ assert_equal all, PriceEstimate.all.sort_by(&:id).map(&:estimate_of)
- assert_equal expected.sort_by(&:id), actual.sort_by(&:id)
+ actual = PriceEstimate.where.not(estimate_of: sapphire)
+ only = PriceEstimate.where(estimate_of: sapphire)
+
+ expected = all - [sapphire]
+ assert_equal expected, actual.sort_by(&:id).map(&:estimate_of)
+ assert_equal all - expected, only.sort_by(&:id).map(&:estimate_of)
+ end
+
+ def test_where_not_polymorphic_id_and_type_as_nand
+ sapphire = treasures(:sapphire)
+
+ all = [treasures(:diamond), sapphire, cars(:honda), sapphire]
+ assert_equal all, PriceEstimate.all.sort_by(&:id).map(&:estimate_of)
+
+ actual = PriceEstimate.where.yield_self do |where_chain|
+ where_chain.stub(:not_behaves_as_nor?, false) do
+ where_chain.not(estimate_of_type: sapphire.class.polymorphic_name, estimate_of_id: sapphire.id)
+ end
+ end
+ only = PriceEstimate.where(estimate_of_type: sapphire.class.polymorphic_name, estimate_of_id: sapphire.id)
+
+ expected = all - [sapphire]
+ assert_equal expected, actual.sort_by(&:id).map(&:estimate_of)
+ assert_equal all - expected, only.sort_by(&:id).map(&:estimate_of)
+ end
+
+ def test_where_not_polymorphic_id_and_type_as_nor_is_deprecated
+ sapphire = treasures(:sapphire)
+
+ all = [treasures(:diamond), sapphire, cars(:honda), sapphire]
+ assert_equal all, PriceEstimate.all.sort_by(&:id).map(&:estimate_of)
+
+ message = <<~MSG.squish
+ NOT conditions will no longer behave as NOR in Rails 6.1.
+ To continue using NOR conditions, NOT each conditions manually
+ (`.where.not(:estimate_of_type => ...).where.not(:estimate_of_id => ...)`).
+ MSG
+ actual = assert_deprecated(message) do
+ PriceEstimate.where.not(estimate_of_type: sapphire.class.polymorphic_name, estimate_of_id: sapphire.id)
+ end
+ only = PriceEstimate.where(estimate_of_type: sapphire.class.polymorphic_name, estimate_of_id: sapphire.id)
+
+ expected = all - [sapphire]
+ # NOT (estimate_of_type = 'Treasure' OR estimate_of_id = sapphire.id) matches only `cars(:honda)` unfortunately.
+ assert_not_equal expected, actual.sort_by(&:id).map(&:estimate_of)
+ assert_equal all - expected, only.sort_by(&:id).map(&:estimate_of)
end
def test_polymorphic_nested_array_where