#3472 AssociationCollection#include? ignoring build associated objects - Ruby on Rails

Type	To find
responsible:me	tickets assigned to you
tagged:"@high"	tickets tagged @high
milestone:next	tickets in the upcoming milestone
state:invalid	tickets with the state invalid
created:"last week"	tickets created last week
sort:number, importance, updated	tickets sorted by #, importance or updated
Combine keywords for powerful searching.
Use advanced searching »

This project is archived and is in readonly mode.

#3472 ✓resolved

AssociationCollection#include? ignoring build associated objects

Reported by Dmitry Ratnikov | November 9th, 2009 @ 05:59 AM | in 2.3.10

Consider:

class Post < ActiveRecord::Base
  has_many :comments
end

class Comment < ActiveRecord::Base
  belongs_to :post
end

The following test fails:


def test_build
  post = Post.new
  comment = post.comments.build
  assert post.comments.include?(comment)
end

The logs show:


  Comment Load (0.2ms)   SELECT "comments".id FROM "comments" WHERE ("comments"."id" = NULL) AND ("comments".post_id = NULL) LIMIT 1

It seems that the collection ignores that it has been setup (loaded? #=> false) and reloads it from the database. Regarding that the database is a new record and hence definitely cannot be in the database, that is probably not expected behavior.

Any suggestions?

Comments and changes to this ticket

You flagged this item as spam.
Marcelo Giorgi November 23rd, 2009 @ 02:13 AM
- Tag set to “patch”
I've just create a patch for this.

The idea of the fix is to delegate the job to Array#include? for the collection present in memory even if the object is not stored in the database. Without the fix, Rails only looks in the Array collection if the association collection were loaded previously.

Please, let me know what do you think about the implementation.

BTW: I took your test to check the fix worked.
- associationcollection-include-working-properly-for-build-associated-objects.diff 1.9 KB
You flagged this item as spam.
Marcelo Giorgi November 23rd, 2009 @ 04:46 PM
Hey,

My mistake, I had a typo in the test. Now uploading the correct version.

Now you can tell me what you think about the PATCH.
- associationcollection-include-working-properly-for-build-associated-objects.diff 1.8 KB
You flagged this item as spam.
Diego Algorta November 23rd, 2009 @ 04:50 PM
+1
You flagged this item as spam.
rabble November 23rd, 2009 @ 04:57 PM
+1
You flagged this item as spam.
Dmitry Ratnikov November 23rd, 2009 @ 05:00 PM
- Tag cleared.
Looks good to me :)

I will apply that to 2.3 stable to check that it works.
I'll also check that it works if the has_many has finder_sql defined.
You flagged this item as spam.
Dmitry Ratnikov November 23rd, 2009 @ 05:03 PM
- Tag set to “patch”
You flagged this item as spam.
Dmitry Ratnikov November 23rd, 2009 @ 06:09 PM
- Tag changed from “patch” to “for, patch”
Patches passes tests for mysql and sqlite3 adapters for 2-3-stable branch.
You flagged this item as spam.
Marcelo Giorgi November 30th, 2009 @ 05:24 PM
- Tag changed from “for, patch” to “verified”
As there are three positive opinions about the patch, I changed it to verified.
You flagged this item as spam.
Marcelo Giorgi December 1st, 2009 @ 03:27 PM
- Tag changed from “verified” to “patch, verified”
Michael Koziarski December 18th, 2009 @ 09:05 AM
This patch doesn't apply cleanly right now, could you rebase it for me

However the change doesn't quite look right to me, shouldn't the check be whether the target is loaded or the owner is new, not the argument that's being checked against the collection?
You flagged this item as spam.
Marcelo Giorgi December 18th, 2009 @ 08:15 PM
Hi Michael,

IMHO: I think it makes sense to ask the argument in this case, because that's the only object that can tell if it is in memory or already stored, which is the key point to decide if traversing the collection makes sense...as we are looking for an in memory object or we would look into the database (which is being done in the last statement of the method).

Also, there are cases when @owner is loaded and still the test would fail. For example, without the patch, this code (in which @owner is NOT new_record?) fails:

p=Post.create(:title=>'title', :body=>'body') => # c=p.comments.build => # p.comments.include?(c) => false

About checking the @target to see if it is loaded, as you can see from the code @target is already been asked for loaded? condition, to determine whether or not to traverse the collection looking for the object being passed as parameter:

return @target.include?(record) if loaded?

And this code (the current code) doesn't handle the test that I added, in which neither the @owner nor the @target are being loaded...

If we don't ask the in_memory_object passed as parameter the only option left is to ALWAYS traverse the collection in memory just to check if there is an object that matches the one passed as parameter. Something like this:
```
  def include?(record)
    return false unless record.is_a?(@reflection.klass)
    load_target if @reflection.options[:finder_sql] && !loaded?
    return true if @target.include?(record)
    exists?(record)
  end
```
BUT there are some test that breaks (i.e. /activerecord/test/cases/associations/has_many_through_associations_test#test_replace_association) with this solution, because they assume that when the collection is 'loaded?', then association_collection#include? method will NOT execute a new query, it would just check in memory and return 'false' if not found (but not look into the database).

For this reason, I've attached the same patch ('rebased' with the latest code in 2-3-stable), PLEASE let me know what you think...
- associationcollection-include-working-properly-for-build-associated-objects.diff 1.9 KB
You flagged this item as spam.
Dmitry Ratnikov December 18th, 2009 @ 09:02 PM
To be honest, I was leaning towards something like to underline the differences between new_record and not:
```
def include?(record)
         return false unless record.is_a?(@reflection.klass)
         if record.new_record?
           @target.include?(record)
         else
           load_target if @reflection.options[:finder_sql] && !loaded?
           return @target.include?(record) if loaded?
           exists?(record)
         end
       end
```
The main reason is that if the record is a new_record? the DB specific code (load, exists?) really doesn't make much sense to me in the context of trying to see if the record is included in the collection.

Of course Marcello's patch accomplishes same with with a tad less repetition, hence why I +1 it.
You flagged this item as spam.
Marcelo Giorgi August 8th, 2010 @ 07:34 PM
Hey,

I've just made some adjustments according to the Dimity's suggestion, about making the case of record.new_record? more explicit.

Also, I've added more logic to make it work with HasManyThroughAssociation, which had also the same problem.

More tests were added, to check that it is working for HasManyThroughAssociation and HasAndBelongsToManyAssociation.

BTW: This patch has been made from 2-3-stable branch. But I'm also going to start a new ticket for master, as this issue still happens for Rails 3 version.

Please, let me know if it looks good or have any suggestion so I can improve it
- assoc-collection-includes-not-ignoring-object-added-by-build.diff 4.7 KB
You flagged this item as spam.
Marcelo Giorgi August 18th, 2010 @ 01:06 AM
- Assigned user set to “Santiago Pastorino”
Santiago Pastorino August 28th, 2010 @ 08:52 PM
- Importance changed from “” to “Low”
This issue is for 2.3 only? or 3.0 has the same problem?
You flagged this item as spam.
Marcelo Giorgi September 3rd, 2010 @ 11:35 PM
Good point, yes in fact Rails 3.0 has the same problem, and I pointed out in this ticket:
https://rails.lighthouseapp.com/projects/8994/tickets/5336-associat...

Btw: I've provided a similar patch and tests (which surprisingly still applies to master ;)) for this issue on Rails 3.0 on the other ticket.
You flagged this item as spam.
Marcelo Giorgi September 4th, 2010 @ 04:19 PM
Here I attach an update of the patch so that it applies cleanly (also a subtle change is introduced) on 2-3-stable.
- updated-assoc-collection-includes-not-ignoring-object-added-by-build.diff 4.7 KB
Santiago Pastorino September 28th, 2010 @ 07:20 PM
- State changed from “new” to “open”
- Milestone set to “2.3.10”
- Assigned user changed from “Santiago Pastorino” to “Aaron Patterson”
Repository September 30th, 2010 @ 06:29 PM
- State changed from “open” to “resolved”
(from [96c19ff7cc1521b9f10ffb1004601d70b7025cba]) AssociationCollection#include? working properly for objects added with build method [#3472 state:resolved] http://github.com/rails/rails/commit/96c19ff7cc1521b9f10ffb1004601d...
Repository September 30th, 2010 @ 08:14 PM
(from [2221b701b4eff33e282f283c759b70789b122df3]) AssociationCollection#include? working properly for objects added with build method [#3472 state:resolved] http://github.com/rails/rails/commit/2221b701b4eff33e282f283c759b70...
Repository September 30th, 2010 @ 08:14 PM
(from [ef6df93a8ddb675f1298973bb347825c554e95f9]) AssociationCollection#include? working properly for objects added with build method [#3472 state:resolved] http://github.com/rails/rails/commit/ef6df93a8ddb675f1298973bb34782...
Ryan Bigg October 9th, 2010 @ 10:10 PM
- Tag cleared.
Automatic cleanup of spam.
Ryan Bigg October 11th, 2010 @ 12:12 PM
Automatic cleanup of spam.
Ryan Bigg October 21st, 2010 @ 03:37 AM
Automatic cleanup of spam.
You flagged this item as spam.
bingbing March 26th, 2011 @ 01:23 AM
gucci watches
You flagged this item as spam.
klkk May 23rd, 2011 @ 02:53 AM
louisvuittonwarehouse.com
You flagged this item as spam.
klkk May 23rd, 2011 @ 02:55 AM
www.louisvuittonwarehouse.com