remove check for uniq from method add_record_to_target_with_callbacks

This ticket is continuation of discussion from ticket #5053. Please read that ticket to get a background.

As part of fix for #5053 I created a personal branch where some discussion took place. Here I am going to copy paste some of the discussions that took place. It will not contain all the discussions, just some of them which seem relevant.

After the fix the method add_record_to_target_with_callbacks look like this

def add_record_to_target_with_callbacks(record)
   callback(:before_add, record)
   yield(record) if block_given?
   @target ||= [] unless loaded?
   index = @target.index(record)
   unless @reflection.options[:uniq] && index
     if index
       @target[index] = record
     else
      @target << record
     end
   end
   callback(:after_add, record)
   set_inverse_instance(record, @owner)
   record
 end

James Le Curiot:

I understand the nature of the fix and I suppose it doesn't really do any harm but it might seem a little confusing if you decide to enable the :uniq option and suddenly find that the updates don't take affect any more.

I mean if the record for a particular id was already saved and present in the collection. Thinking about it, you shouldn't even check the :uniq option at all here because what you are doing already forces it to be unique anyway - a record with a matching id will replace any existing entry instead of being appended. It should therefore just look like this.

      @target ||= [] unless loaded?
      if index = @target.index(record)
        @target[index] = record
      else
        @target << record
      end
      callback(:after_add, record)

I just ran the tests with this and they still passed. But I reserve the right to be confused. :)

Neeraj Singh:

I am with you on that one. We (myself and Subba) agreed that there is no need for uniq check. However our reasoning was different.

Although we are talking about a piece of code in association_collection, we are here because we came from nested_attributes call. Also note that we are here because association is not loaded and @target has elements. That is possible only through user.comments.create kind of call. When we do create we can't have two records with same id.

At this point of time I am leaning towards committing this ticket. We can refactor and will remove uniq check later if there is strong desire for it.

James Le Curiot:

You have to ask yourself, what is :uniq actually for? Given that primary keys have to be unique and the foreign key only signifies the presence of a record in the collection, not how times it is present, the records would normally be unique anyway. This would only change when using unusual joins that return rows more than once. But that only relates to fetching the collection. I don't think it makes sense to allow duplicates when adding to it, even when not using :uniq, because when the collection is reloaded, the duplicates simply disappear. Am I missing something here?

I think the check should be removed now, otherwise it won't be possible to replace the records when using :uniq. I can't think of any reason why this shouldn't be allowed, except that it will change the existing behaviour - but we're doing that already and this is for a new version of Rails. :)