Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecate `where.not` working as NOR and will be changed to NAND in Rails 6.1 #36029

Merged
merged 1 commit into from Apr 23, 2019
Merged
Changes from all commits
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -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.
@@ -41,18 +41,31 @@ def initialize(scope)
#
# 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

This comment has been minimized.

Copy link
@rafaelfranca

rafaelfranca Apr 23, 2019

Member

This message is confusing. How about?

To continue using NOR conditions, chain each `NOT` condition manually.

We probably should add a new method that do the NAND too. Maybe invert as suggested by @matthewd?

This comment has been minimized.

Copy link
@kamipo

kamipo Apr 23, 2019

Author Member

I'm not sure that people want to hurry to migrate to new method, since we have left this issue alone for 5 years (oldest one #14161 I can find).

This issue has already mitigated by 213796f (e9ba12f), and will eventually be fixed even if new method isn't introduced.

(`#{ 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
@@ -70,7 +70,15 @@ def ==(other)
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 @@ def equality_node?(node)
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
@@ -106,7 +106,7 @@ class WhereClauseTest < ActiveRecord::TestCase
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
@@ -115,13 +115,58 @@ def test_polymorphic_shallow_where
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
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.