Bug when use #insert_at on an invalid ActiveRecord object

[mergulhao]

Hi,

I found something that seems to be a bug.

If I have an invalid ActiveRecord object (record.valid? => false) and calls record.insert_at(new_position), it updates all other records using #shuffle_positions_on_intermediate_items. But to save the current record it uses #set_list_position method that calls #save!on the record. Causing the save to abort since the record is invalid and leaving the database corrupted with a dup position.

To me seems #set_list_position should not call #save!. It should just save the record ignoring if it's invalid as already happens on #shuffle_positions_on_intermediate_items that uses #update_all method that ignore ActiveRecord validation.

The new implementation should be something like:

def set_list_position(new_position)
  self.update_column(position_column, new_position)
end

I can PR with a fix if you think is useful.

[knowuh]

I agree with this; reordering a list doesn't seem to be a good time to be validating the associated model. +1

[swanandp]

update_column skips callbacks, I think the right way would be save(validate: false)

[swanandp]

Fixed with 7390b48

[swanandp]

@mergulhao @knowuh FYI

[knowuh]

@swanandp awesome! thanks!

[mergulhao]

@swanandp I appreciate that you fixed it. I just don't think that the right way is to use the save method... I understood your point that update_column skips callbacks. But if you're going to use a method that do not skips callbacks you should also change the method #shuffle_positions_on_intermediate_items to not use #update_all, since it skips callbacks.

I think that the "desirable" is that to run or not callbacks should be consistent. The way it's implemented when we run #insert_at_position(position), it's going to run callbacks to a part of the records and not run it to one record. This is not consistent.

What do you think?

Reference:
http://api.rubyonrails.org/classes/ActiveRecord/Relation.html#method-i-update_all