Skip to content
This repository

finders and scopes on has_many association on new object incorrectly query db for null foreign keys #5215

Closed
nsolter opened this Issue · 15 comments

12 participants

nsolter Brian Delaney Christoph Olszowka Vipul A M Steve Klabnik Rafael Mendonça França Andrew White Jon Leighton Justin George David Whitby Carlos Antonio da Silva Aaron Patterson
nsolter

Rails version 3.2.1

Consider a new, unsaved (such that it has no id yet), instance of a model with a has_many association. Finders and scopes on the has_many association of this object behave incorrectly. They issue db queries with null for the foreign key. These queries return incorrect results if the foreign key is allowed to be null, because it will match all records with null values for that key.

For example, consider a School model:

class School < ActiveRecord::Base
  has_many :students
end

Creating a new, unsaved, instance of a school and accessing the students association correctly returns an empty array:

ruby-1.9.2-p290 :009 > s = School.new
 => #<School id: nil, name: nil, created_at: nil, updated_at: nil> 
ruby-1.9.2-p290 :010 > s.students
 => [] 

However, adding a dynamic finder on the association incorrectly matches student records with null school_id foreign keys:

ruby-1.9.2-p290 :011 > s.students.find_by_name("Aaron Aardvark")
  Student Load (0.4ms)  SELECT `students`.* FROM `students` WHERE `students`.`school_id` IS NULL AND `students`.`name` = 'Aaron Aardvark' LIMIT 1
 => #<Student id: 1, name: "Aaron Aardvark", school_id: nil, created_at: "2012-02-29 02:08:37", updated_at: "2012-02-29 02:08:37"> 

count, exists?, and other methods behave similarly incorrectly:

ruby-1.9.2-p290 :012 > s.students.count
   (0.3ms)  SELECT COUNT(*) FROM `students` WHERE `students`.`school_id` IS NULL
 => 2 
ruby-1.9.2-p290 :013 > s.students.exists?(Student.last)
  Student Load (0.3ms)  SELECT `students`.* FROM `students` ORDER BY `students`.`id` DESC LIMIT 1
  Student Exists (0.3ms)  SELECT 1 FROM `students` WHERE `students`.`school_id` IS NULL AND `students`.`id` = 2 LIMIT 1
 => true 

Scopes exhibit similarly bad behavior. Suppose Student has the following scope:

  scope :starts_with, lambda { |prefix| where(['LOWER(name) LIKE ?', "#{prefix}%"])}

Accessing that scope on the association on a new, unsaved, school object gives this:

ruby-1.9.2-p290 :014 > s.students.starts_with("a")
  Student Load (0.4ms)  SELECT `students`.* FROM `students` WHERE `students`.`school_id` IS NULL AND (LOWER(name) LIKE 'a%')
 => [#<Student id: 1, name: "Aaron Aardvark", school_id: nil, created_at: "2012-02-29 02:08:37", updated_at: "2012-02-29 02:08:37">] 

The correct behavior should probably be to return an empty array for the finders and scopes, 0 for the count, and false for exists?. However, throwing an exception would be fine with me as well.

nsolter

This issue is similar to, but broader in scope than, #1856 .

Brian Delaney

I think it's important to point out that, while in a lot of cases, this bug appears harmless and the results seem to be correct (because a lot of times related tables have non-nullable foreign keys), if it is correct it is just coincidental. The logic here is completely wrong, and it is using completely unrelated database records to try to fulfill the query.

Christoph Olszowka

Yes, #1856 mentions one of the possible things a developer can trip over with this issue. It also leads to very hard-to-diagnose bugs, so I think it really should be fixed.

Please also note the solution suggestions by @tenderlove in the aforementioned #1856 for the discussed #count example.

Vipul A M

Anyone working on this?

Steve Klabnik
Collaborator

Not that I'm aware of.

Rafael Mendonça França
Owner

The count issue was fixed with #6978 but we need to find a solution to the other cases.

Vipul A M vipulnsward referenced this issue from a commit
Commit has since been removed from the repository and is no longer available.
Vipul A M

@rafaelfranca gave a try at fixing exists? vipulnsward@c2824a0 can you have a look at it?

Andrew White
Owner

@vipulnsward I don't think that's going to be an acceptable fix - we're leaking association implementation details into the relation code. I did something similar (#5717) for fixing inverse associations when using a scope and @jonleighton wasn't keen - see his comments on this commit: 9f607fb

Jon Leighton
Owner

We should fix this by returning a NullRelation (Relation#none) when the owner of the association is new_record?

Justin George

I'm taking a whack at this - wouldn't returning a null relation fail when you do something like

p = Person.new
p.pets.build
p.save

Where we want to add the new instance to the load target? Maybe I'm not understanding the case very well - I'm assuming we should be returning a NullRelation when we're responding to the pets method above.

Vipul A M

@pixeltrix the only other way around I found was to define exists? at CollectionProxy https://gist.github.com/3928087 , but that too would be "leaking" details to the proxy.{even though it would just be a check}
@jonleighton shouldn't exists? return false. I tried fixing find() to return NullRelation which made sense. Can you suggest where the check should lie in the above case @pixeltrix mentioned?

Jon Leighton jonleighton closed this issue from a commit
Jon Leighton jonleighton Relations built off collection associations with an unsaved owner sho…
…uld be null relations

For example, the following should not run any query on the database:

Post.new.comments.where(body: 'omg').to_a # => []

Fixes #5215.
0130c17
Jon Leighton
Owner

Hi guys, I fixed this with the above commit. Would have given more pointers but I needed to dig into the code myself in order to work out what needed to be done.

Vipul A M vipulnsward referenced this issue from a commit
Commit has since been removed from the repository and is no longer available.
David Whitby

Which branch is this fix in? I tried both 3.2.12 and the 3-2-stable branch but both seem to have this issue again, although I could swear I was running on 3-2-stable a while back and the fix was in there - can someone point me in the right direction? I want to get the latest security fixes but also have this issue fixed. Thanks!

Carlos Antonio da Silva

This is in master only, which means it'll be only available when Rails 4 ships.

David Whitby
Christopher Chow Soliah referenced this issue from a commit
Commit has since been removed from the repository and is no longer available.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.