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

Polymorphic `where`s should be treated as a unit. #17010

Closed
wants to merge 1 commit into from
Closed

Polymorphic `where`s should be treated as a unit. #17010

wants to merge 1 commit into from

Conversation

@pvande
Copy link

@pvande pvande commented Sep 22, 2014

ActiveRecord's default behavior around .where.not(hash) is to ensure that each key is appropriately filtered out from the query, with the effect being that the query is populated with (key1 != value1) AND (key2 != value2). In general, this feels like a fair implementation, despite not being a direct negation of .where(hash).

However, ActiveRecord also supports filtering on a polymorphic association. This works as expected, internally being expanded to a condition that verifies both the relation_id and the relation_type. Negating one of these conditions, however, excludes rows that share a relation_type or a relation_id.

Fixes #16983.

ActiveRecord's default behavior around `.where.not(hash)` is to ensure
that each key is appropriately filtered out from the query, with the
effect being that the query is populated with `(key1 != value1) AND
(key2 != value2)`. In general, this feels like a fair implementation,
despite not being a direct negation of `.where(hash)`.

However, ActiveRecord also supports filtering on a polymorphic
association. This works as expected, internally being expanded to a
condition that verifies both the relation_id and the relation_type.
Negating one of these conditions, however, excludes rows that share a
relation_type or a relation_id.

Fixes #16983.
@pvande
Copy link
Author

@pvande pvande commented Sep 23, 2014

The build error was a single job timing out for seemingly unrelated reasons.

@matthewd
Copy link
Member

@matthewd matthewd commented Sep 29, 2014

xref #14161

@pywebdesign
Copy link

@pywebdesign pywebdesign commented Jan 16, 2015

+1

@MisinformedDNA
Copy link

@MisinformedDNA MisinformedDNA commented Apr 13, 2015

There's a series of issues leading back to this one. But in order for any of them to close, this once needs to be merged. Any reason this hasn't been merged yet?

@acutemedical
Copy link

@acutemedical acutemedical commented Apr 14, 2015

@MisinformedDNA Looks like the CI build failed with merge conflicts. Someone from core will need to sort those out before it can pass.

@kamipo kamipo closed this in e9ba12f Aug 17, 2017
kamipo added a commit to kamipo/rails that referenced this pull request Apr 19, 2019
…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 accidentaly 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: treasure.class.polymorphic_name, estimate_of_id: treasure.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: treasure.class.polymorphic_name, estimate_of_id: treasure.id)
assert_equal nand, without_sapphire.map(&:estimate_of)
```

Resolves rails#31209.
kamipo added a commit to kamipo/rails that referenced this pull request Apr 19, 2019
…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: treasure.class.polymorphic_name, estimate_of_id: treasure.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: treasure.class.polymorphic_name, estimate_of_id: treasure.id)
assert_equal nand, without_sapphire.map(&:estimate_of)
```

Resolves rails#31209.
kamipo added a commit to kamipo/rails that referenced this pull request Apr 19, 2019
…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: treasure.class.polymorphic_name, estimate_of_id: treasure.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: treasure.class.polymorphic_name, estimate_of_id: treasure.id)
assert_equal nand, without_sapphire.map(&:estimate_of)
```

Resolves rails#31209.
kamipo added a commit to kamipo/rails that referenced this pull request Apr 19, 2019
…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: treasure.class.polymorphic_name, estimate_of_id: treasure.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: treasure.class.polymorphic_name, estimate_of_id: treasure.id
)
assert_equal nand, without_sapphire.map(&:estimate_of)
```

Resolves rails#31209.
kamipo added a commit to kamipo/rails that referenced this pull request Apr 19, 2019
…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

7 participants