Duplicated positions when creating parent and children from scratch in 0.1.5

[jmbejar]

I have a problem after upgrade to 0.1.5.

class TodoList < ActiveRecord::Base
   has_many :todo_items, :order => "position"
end

class TodoItem < ActiveRecord::Base
   belongs_to :todo_list
   acts_as_list :scope => :todo_list
end

list = TodoList.new
3.times { list.todo_items.build }
list.save

Creating the parent and the children from scratch generate odd values for positions (sometimes the same repeated value). It used to work fine with 0.1.4 (I'm using rails 3.0.10)

As far I checked it was broken after move the move_to_bottom callback to a before_validation callback, it worked with 0.1.4 because move_to_bottom was a before_create callback.

[coreycerovsek]

I think you mean "add_to_list_bottom" where you wrote "move_to_bottom".

[jmbejar]

You're right.. it's add_to_list_bottom

[coreycerovsek]

The existence of the validation callback on the child class causes validation on the parent when it is saved, which in turn triggers validation of the children at that time. (See ActiveRecord::AutosaveAssociation::ClassMethods#add_autosave_association_callbacks.) But the position computation queries the database and at that point, neither the parent or children are persisted, so things go awry!

Looking at this, doesn't it seem like there's a mistake in ActiveRecord::AutosaveAssociation::ClassMethods#validate_collection_association...? Seems to me that ought to read "records.all?" instead of "records.each". I don't know where one reports these things... that just caught my eye.

[swanandp]

It actually works quite fine for me, but I am using Rails 3.2.2.

Edit: Confirmed the issue. It exists. See the log output below.

[swanandp]

With acts_as_list-0.1.5

Loading development environment (Rails 3.0.10)
1.9.3-p125 :001 > 
1.9.3-p125 :002 >   list = TodoList.new
 => #<TodoList id: nil, name: nil, created_at: nil, updated_at: nil> 
1.9.3-p125 :003 > 3.times { list.todo_items.build }
 => 3 
1.9.3-p125 :004 > list.save
 => true 
1.9.3-p125 :005 > list.todo_items
 => [#<TodoItem id: 1, name: nil, position: 1, todo_list_id: 1, created_at: "2012-03-29 06:41:34", updated_at: "2012-03-29 06:41:34">, #<TodoItem id: 2, name: nil, position: 1, todo_list_id: 1, created_at: "2012-03-29 06:41:34", updated_at: "2012-03-29 06:41:34">, #<TodoItem id: 3, name: nil, position: 1, todo_list_id: 1, created_at: "2012-03-29 06:41:34", updated_at: "2012-03-29 06:41:34">] 
1.9.3-p125 :006 > list.todo_items.map(&:position)
 => [1, 1, 1] 
1.9.3-p125 :007 > list.todo_items.reload
 => [#<TodoItem id: 3, name: nil, position: 1, todo_list_id: 1, created_at: "2012-03-29 06:41:34", updated_at: "2012-03-29 06:41:34">, #<TodoItem id: 2, name: nil, position: 2, todo_list_id: 1, created_at: "2012-03-29 06:41:34", updated_at: "2012-03-29 06:41:34">, #<TodoItem id: 1, name: nil, position: 3, todo_list_id: 1, created_at: "2012-03-29 06:41:34", updated_at: "2012-03-29 06:41:34">] 
1.9.3-p125 :008 > list.todo_items.map(&:position)
 => [1, 2, 3] 
1.9.3-p125 :009 > 

With acts_as_list-0.1.4:

1.9.3-p125 :008 > list = TodoList.new
 => #<TodoList id: nil, name: nil, created_at: nil, updated_at: nil> 
1.9.3-p125 :009 > 3.times { list.todo_items.build }
 => 3 
1.9.3-p125 :010 > list.save
 => true 
1.9.3-p125 :011 > list.todo_items.map(&:position)
 => [1, 2, 3] 
1.9.3-p125 :012 > list.todo_items.reload.map(&:position)
 => [1, 2, 3] 
1.9.3-p125 :013 > 

[coreycerovsek]

Actually I think the problem is not that the parent association is nil, as I first thought, it's rather that the computation of position breaks because it only considers things already persisted in the database. Because in this scenario the parent hasn't been saved yet either, that computation is picking up children in the scope with parent nil.

I don't think the before_validation callback in commit e26d03a is a good idea. First of all, it doesn't seem right for validation to have side effects. If you want to set the position before validation (that commit seems to have served purely to make validation of presence of position in a test sort of work) you could try a callback on after_initialize. The problem there is that the existing approach for computing a position only accounts for already-persisted records, and unless you require the use of IdentityMap, you can't easily coordinate the positioning of not-yet-persisted children attached to different instances of the same parent.

Personally I'd probably back off the effort to enforce presence of position. Setting the position before creation as in the previous version lets you use the database to sort out synchronization... on which note, now that I'm looking, I see that add_to_list_bottom is not concurrency-safe. Shouldn't one wrap add_to_list_bottom in a transaction, in the manner of move_to_bottom? I'm still worried about this posting about concurrency issues. Actually, that in turn suggests that one should really use an around_create callback, because if the transaction ends before the record is persisted, another process could try to grab the same position number.

[swanandp]

@coreycerovsek I tend to agree with you. On pure design terms, validations should not matter for acts_as_list - but its a commonly occurring practical situation, so we should try to resolve it.

[coreycerovsek]

I suspect it's really hard to provide a good position number until the record is actually persisted. If validation is important, maybe you could let validation succeed when you know you're going to compute a good position number automatically. Of course, ideally, validation would check not only presence but uniqueness! And, unless there's a transaction around the whole thing, you could lose position uniqueness between validation and the create or update...

[swanandp]

Yeah, its a valid concern. Let me do a little more brainstorming around the
idea. In the meantime, if you have any, do send a pull request.

On Thu, Mar 29, 2012 at 4:34 PM, Corey Cerovsek <
reply@reply.github.com

wrote:

I suspect it's really hard to provide a good position number until the
record is actually persisted. If validation is important, maybe you could
let validation succeed when you know you're going to compute a good
position number automatically. Of course, ideally, validation would check
not only presence but uniqueness! And, unless there's a transaction around
the whole thing, you could lose position uniqueness between validation and
the create or update...

---

Reply to this email directly or view it on GitHub:
#31 (comment)

[swanandp]

The more pressing concern, which I ignored before, is that validation has side effects. This is a complete no no.

[coreycerovsek]

Uh oh... the more I think about it, the more I'm worried about concurrency. For the methods like move_to_bottom, is atomicity of the transaction enough to prevent other processes from messing around in the same scope? Seems to me like some database locking would be needed for processes not to clobber each other when their transactions commit, something like selecting the relevant scope for update. And on the topic of validation, all the methods like increment_position that call update_attribute, they're going to skip validation.

[digitalhobbit]

@swanandp Are you still considering a fix for this issue? Your comment on #30 (comment) seems to suggest that you are. :)

Do you already know whether you're going to revert 59af94b (which changes from before_create to before_validate), or are you considering a different approach?

[swanandp]

@digitalhobbit I tried a different approach initially, but did not really come to a conclusion. So the result is, I'll revert the callback and then think of approaching the validation issue from a different angle. You can expect the changes and push to Rubygems in the next couple of days.

[digitalhobbit]

@swanandp That's great, thanks a lot for the fix!

[jmbejar]

Awesome, thanks guys!