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

Conversation

kamipo
Copy link
Member

@kamipo kamipo commented Apr 19, 2019

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.

all = [treasures(:diamond), treasures(:sapphire), cars(:honda), treasures(:sapphire)]
assert_equal all, PriceEstimate.all.map(&:estimate_of)

In Rails 6.0:

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:

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.

…ails 6.1

`where.not` with polymorphic association is partly fixed incidentally at
213796f (refer rails#33493, rails#26207, rails#17010, rails#16983, rails#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 rails#31209.
@kamipo
Copy link
Member Author

kamipo commented Apr 22, 2019

I'll going to merge the deprecation in 6.0 to will fix where.not in 6.1.
Let me know if anyone has any objection.

@kamipo kamipo merged commit 9d1f9b9 into rails:master Apr 23, 2019
@kamipo kamipo deleted the deprecate_where_not branch April 23, 2019 07:15
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

stejskalleos added a commit to stejskalleos/foreman_ansible that referenced this pull request Apr 22, 2022
Changes:
- loading of host extension helper
- Rails 6 'where.not' use NAND instead of NOR [0]

[0] rails/rails#36029
stejskalleos added a commit to stejskalleos/foreman that referenced this pull request Apr 27, 2022
Changes overview:
- Set default Rails version to 6.1
- Required dynfow >= 1.6.5
- Cleanup deprecation warnings in config/as_deprecation_whitelist.yaml
- ActiveSupport::ParameterFilter instead of ActionDispatch::Http::ParameterFilter
- model.errors.attribute_names instead of model.errors.keys
- Fixed issue with iterating over the model.errors
- use .add for adding error messages instead of <<
- 'where.not' use NAND instead of NOR [0]

[0] rails/rails#36029
adamruzicka pushed a commit to theforeman/foreman_ansible that referenced this pull request May 11, 2022
Changes:
- loading of host extension helper
- Rails 6 'where.not' use NAND instead of NOR [0]

[0] rails/rails#36029
adamruzicka pushed a commit to theforeman/foreman that referenced this pull request May 24, 2022
Changes overview:
- Set default Rails version to 6.1
- Required dynfow >= 1.6.5
- Cleanup deprecation warnings in config/as_deprecation_whitelist.yaml
- ActiveSupport::ParameterFilter instead of ActionDispatch::Http::ParameterFilter
- model.errors.attribute_names instead of model.errors.keys
- Fixed issue with iterating over the model.errors
- use .add for adding error messages instead of <<
- 'where.not' use NAND instead of NOR [0]

[0] rails/rails#36029
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The construct "where.not" doesn't respect boolean algebra
2 participants