This project is archived and is in readonly mode.
remove check for uniq from method add_record_to_target_with_callbacks
Reported by Neeraj Singh | July 21st, 2010 @ 02:44 PM | in 3.x
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. :)
Comments and changes to this ticket
-
Neeraj Singh July 22nd, 2010 @ 08:43 PM
- Assigned user changed from Neeraj Singh to José Valim
-
José Valim July 26th, 2010 @ 04:18 PM
According to James thoughts, setting :uniq => true will remove the possibility to update records. Should we have a test then, asserting that records can be updated even with :uniq => true (which I consider to be the purpose of this patch)?
-
Neeraj Singh July 26th, 2010 @ 04:35 PM
This would only change when using unusual joins that return rows more than once.
If we need to support that case then all the duplicate records @target should be updated and not just the first one.
I agree with James when he says
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.
As far as testing is concerned, I can force a failure case by artificially populating @target with two identical records but that is just a force scenario.
James do you have any idea on how to further strengthen existing tests to take care of this case.
-
Repository July 26th, 2010 @ 04:50 PM
- State changed from open to resolved
(from [a44652baed1d26a4f63380c406e05f7a2ddd4a12]) No need to check for :uniq
[#5170 state:resolved]
Signed-off-by: José Valim jose.valim@gmail.com
http://github.com/rails/rails/commit/a44652baed1d26a4f63380c406e05f...
Create your profile
Help contribute to this project by taking a few moments to create your personal profile. Create your profile »
<h2 style="font-size: 14px">Tickets have moved to Github</h2>
The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>