AssociationCollection#include? ignoring build associated objects

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.

+1

+1

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?

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...

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.

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

Good point, yes in fact Rails 3.0 has the same problem, and I pointed out in this ticket:
#5336 AssociationCollection#include? ignoring build associated objects (not working...

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.

Here I attach an update of the patch so that it applies cleanly (also a subtle change is introduced) on 2-3-stable.

(from [2221b701b4eff33e282f283c759b70789b122df3]) AssociationCollection#include? working properly for objects added with build method [#3472 AssociationCollection#include? ignoring build associated objects state:resolved] http://github.com/rails/rails/commit/2221b701b4eff33e282f283c759b70...

(from [ef6df93a8ddb675f1298973bb347825c554e95f9]) AssociationCollection#include? working properly for objects added with build method [#3472 AssociationCollection#include? ignoring build associated objects state:resolved] http://github.com/rails/rails/commit/ef6df93a8ddb675f1298973bb34782...

Automatic cleanup of spam.

Automatic cleanup of spam.

gucci watches