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

Already on GitHub? Sign in to your account

'Where' (.not) with polymorphic association in AR::Relation bug fix #14161

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants
Contributor

konukhov commented Feb 22, 2014

Hi!

While working on a Rails 4 app I ran into a nasty bug that builds wrong queries for polymorphic associations.

Here's an example:

class Like < ActiveRecord::Base
  belongs_to :likeable, polymorphic: true
end

In current versions of Rails it works this way:

Like.where.not(likeable: Post.first).to_sql 
=> "... where (likes.likeable_type != 'Post') and (likes.likeable_id != 1)"

This query finds all likes whose likeables are not a 'Post' and their ids are not 1. This behavior is wrong.

The idea is to use parentheses around both polymorphic conditions to treat them as a unified expression.

Here's how it works in this PR:

Like.where(likeable: Post.first).to_sql
=> "... where (likes.likeable_type = 'Post' and likes.likeable_id = 1)"

Like.where.not(likeable: Post.first).to_sql
=> "... where (not (likes.likeable_type = 'Post' and likes.likeable_id = 1))"
@konukhov konukhov Fixed a bug in ActiveRecord::Relation that builds a wrong query with …
…polymorphic associations.

Example.

class Like < ActiveRecord::Base
  belongs_to :likeable, polymorphic: true
end

> Like.where.not(likeable: Post.first).to_sql # => where likes.likeable_type != 'Post' and likes.likeable_id != 1

This query finds all likeables which are not a 'Post' and their id is not 1. This behavior is wrong.

The idea is to use parentheses around polymorphic conditions to treat them as a unified expression.

Here's how it should work after this commit:

> Like.where(likeable: Post.first).to_sql     # => where (likes.likeable_type = 'Post' and likes.likeable_id = 1)
> Like.where.not(likeable: Post.first).to_sql # => where (not (likes.likeable_type = 'Post' and likes.likeable_id = 1))
e6087d2
Contributor

konukhov commented Sep 29, 2014

@robin850

Hi Robin,

I saw you starred this pull. I'm just wondering if this bug is already fixed, this PR is not relevant or whatever. It has been created a long time ago, so let's close it if it's never gonna be merged, merge it if it's okay or discuss and fix it if this PR is not clear. Need feedback!

Is there an associated issue for this? We're also running into the issue and while it's relatively easy to work around it feels like a significant issue for new users to Rails running into incorrect behavior.

Contributor

konukhov commented Feb 12, 2015

@jredburn I hadn't found one when I looked through issues and PRs before submitting my workaround.

Yeah, I also think that's a significant issue because it's hard to notice that parentheses in the resulting query are wrong. I ended up just not using .not method and writing a plain SQL like .where("not (... and ...)").

@kamipo kamipo closed this in e9ba12f Aug 17, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment