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

ActiveRecord's `.where.not()` improperly negates polymorphic associations #16983

Closed
pvande opened this issue Sep 19, 2014 · 9 comments
Closed

ActiveRecord's `.where.not()` improperly negates polymorphic associations #16983

pvande opened this issue Sep 19, 2014 · 9 comments

Comments

@pvande
Copy link

@pvande pvande commented Sep 19, 2014

Example: https://gist.github.com/pvande/90ee91d4f09d0a125da2

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.

@sgrif sgrif added the activerecord label Sep 20, 2014
@pvande
Copy link
Author

@pvande pvande commented Sep 21, 2014

Ignore 77e5a82; I'd accidentally left some debugging code in the tests.

@rails-bot
Copy link

@rails-bot rails-bot commented Mar 20, 2015

This issue has been automatically marked as stale because it has not been commented on for at least
three months.

The resources of the Rails team are limited, and so we are asking for your help.

If you can still reproduce this error on the 4-2-stable, 4-1-stable branches or on master,
please reply with all of the information you have about it in order to keep the issue open.

Thank you for all your contributions.

@rails-bot rails-bot added the stale label Mar 20, 2015
@pvande
Copy link
Author

@pvande pvande commented Mar 31, 2015

The attached Gist still fails under master, 4-2-stable, and 4-1-stable.

@xdmx
Copy link

@xdmx xdmx commented Jul 23, 2015

I confirm this under ruby 2.2.2 and rails 4.2.3

I have a Conversation with polymorphic associated records (User and Company):

Conversation.create(entity: User.first) # id 1
Conversation.create(entity: User.lastt) # id 2 
Conversation.create(entity: Company.first) # id 3

user = User.first
Conversation.where(entity: user)
# this works, returning the Conversation with id 1

Conversation.where.not(entity: user)
# this doesn't work. It returns only the conversation id 3, but not 2
# even though the entity/user is not the same

From the log I see that the query is ("conversations"."entity_type" != 'User') AND ("conversations"."entity_id" != 1) but it should be ("conversations"."entity_type" != 'User') OR ("conversations"."entity_id" != 1), this way it'd return the conversations 2 and 3

@tombroomfield
Copy link

@tombroomfield tombroomfield commented Sep 14, 2015

I can also confirm I am getting this on Rails 4.2.4 and Ruby 2.2.3.
Entity is polymorphic, which has many templates.

Template.where.not(entity: entity) => Template Load (0.6ms)  SELECT "templates".* FROM "templates" WHERE ("templates"."entity_type" != 'Country') AND ("templates"."entity_id" != 1)
@devilankur18
Copy link

@devilankur18 devilankur18 commented Feb 10, 2016

+1 rails 5.0

@geckofu
Copy link

@geckofu geckofu commented May 9, 2016

+1 to keep this issue active.

Should replace the generated SQL AND with OR

Template.where.not(entity: entity) => SELECT "templates".* FROM "templates" WHERE ("templates"."entity_type" != 'Country') AND ("templates"."entity_id" != 1)
@DonGiulio
Copy link

@DonGiulio DonGiulio commented Aug 24, 2016

check the analysis on #26269

the where.not query should have OR, not an AND.

@DonGiulio
Copy link

@DonGiulio DonGiulio commented Aug 24, 2016

workaround:


type = 'Hat'
id = 3
sql = <<-SQL
SELECT \"comments\".* FROM \"comments\" WHERE (\"comments\".\"commentable_type\" != '#{type}') OR (\"comments\".\"commentable_id\" != #{id})
SQL
ActiveRecord::Base.connection.execute(sql)
@kamipo kamipo closed this in e9ba12f Aug 17, 2017
kamipo added a commit to kamipo/rails that referenced this issue 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 issue 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 issue 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 issue 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 issue 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 pull requests

Successfully merging a pull request may close this issue.

10 participants