Rails core - Feb 2012 - Bug in nested_attributes_for: question about best way to patch

I''ve discovered what I''m pretty confident is unexpected
behavior in
accepts_nested_attributes_for, and am developing a patch, but I''ve
encountered a question about how the core team would prefer the
implementation to look.

Here''s the bug: when assigning to a belongs_to relationship (and I
believe one-to-one, but I haven''t verified yet) if you mark the
association for destruction the association is not unset yet the
record is deleted. This result in both unexpected behavior: the
association seems to still be present in memory after the update, and
in invalid data being saved (foreign key violations) because the
parent record saved still has the association_id set even though the
associated record has been destroyed.

Here is a test demonstrating the error and the expectations:

def
test_should_destroy_an_existing_record_if_there_is_a_matching_id_and_destroy_is_truthy
  @ship.pirate.destroy
  [1, ''1'', true, ''true''].each do |truth|
    pirate = @ship.reload.create_pirate(:catchphrase =>
''Arr'')
    @ship.update_attributes(:pirate_attributes => { :id =>
pirate.id, :_destroy => truth })
    assert_raise(ActiveRecord::RecordNotFound) { pirate.reload }
  end
end

I''ve also a partial patch developed: http://pastie.org/3438962

The problem is that to trigger the record deletion, the association
still needs to be there to be walked on the save call.

I have two question:
1. Does everyone agree on the expected behavior?
2. What is the best implementation route: should I maintain a hidden
list/hash on each object of associations that have been unset and
marked for deletion to use both at saving and if someone resets the
object? Or is that capability already essentially available in the
change tracking? Or something different all-together.

Something along these lines already has to be there because updating
the has_many leaves the collection in memory without the marked-for-
destruction records.

-- 
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com.
To unsubscribe from this group, send email to
rubyonrails-core+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en.

I discovered the bug is also with has_many items (contrary to my
original post.)

I''ve submitted this as a bug with test cases at
https://github.com/rails/rails/issues/5235

I''d appreciate guidance on that issue.

On Feb 23, 12:51 pm, James Coleman <jtc...@gmail.com>
wrote:
> I''ve discovered what I''m pretty confident is unexpected
behavior in
> accepts_nested_attributes_for, and am developing a patch, but I''ve
> encountered a question about how the core team would prefer the
> implementation to look.
>
> Here''s the bug: when assigning to a belongs_to relationship (and I
> believe one-to-one, but I haven''t verified yet) if you mark the
> association for destruction the association is not unset yet the
> record is deleted. This result in both unexpected behavior: the
> association seems to still be present in memory after the update, and
> in invalid data being saved (foreign key violations) because the
> parent record saved still has the association_id set even though the
> associated record has been destroyed.
>
> Here is a test demonstrating the error and the expectations:
>
> def
> test_should_destroy_an_existing_record_if_there_is_a_matching_id_and_destro
y_is_truthy
>   @ship.pirate.destroy
>   [1, ''1'', true, ''true''].each do
|truth|
>     pirate = @ship.reload.create_pirate(:catchphrase =>
''Arr'')
>     @ship.update_attributes(:pirate_attributes => { :id =>
> pirate.id, :_destroy => truth })
>     assert_raise(ActiveRecord::RecordNotFound) { pirate.reload }
>   end
> end
>
> I''ve also a partial patch developed:http://pastie.org/3438962
>
> The problem is that to trigger the record deletion, the association
> still needs to be there to be walked on the save call.
>
> I have two question:
> 1. Does everyone agree on the expected behavior?
> 2. What is the best implementation route: should I maintain a hidden
> list/hash on each object of associations that have been unset and
> marked for deletion to use both at saving and if someone resets the
> object? Or is that capability already essentially available in the
> change tracking? Or something different all-together.
>
> Something along these lines already has to be there because updating
> the has_many leaves the collection in memory without the marked-for-
> destruction records.
-- 
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com.
To unsubscribe from this group, send email to
rubyonrails-core+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en.