Resource#update_or_mark_as_destroyable should not save

[nathanl]

I'd like to propose a change to DataMapper::NestedAttributes::Resource#update_or_mark_as_destroyable. That method ends by saving the association object. I think that's incorrect and the line should be deleted. Here's why.

If I call @object.attributes = some_hash, I'm explicitly saying "update the attributes but don't save yet." If I wanted to update and save at the same time, I'd call @object.update(some_hash). This line breaks that paradigm: if the hash contains attributes for associations, this method automatically saves them.

This is broken because:

• I no longer have the option of using attributes= without saving the associations; update and attributes= do the same thing as far as nested resources are concerned
• Saving here is unnecessary anyway. If we don't save, calling save on the parent object will save these associations anyway, per the DataMapper documentation:

It is important to note that #save will save the complete loaded
object graph when called. This means that calling #save on a
resource that has relationships of any kind (established via
belongs_to or has) will also save those related resources, if
they are loaded at the time #save is being called. Related
resources are loaded if they've been accessed either for read or
for write purposes, prior to #save being called. contains
attributes for nested resorces

• Finally, saving here makes it impossible to use DataMapper's global Model.raise_on_save_failure setting, because as soon as one of these associations fails to save, the process of building the others is aborted.

For example, in a Rails controller, I want to be able to do:

@foo.attributes = params[:foo_attributes]
begin
 @foo.save
 redirect_to someplace
rescue
 render :new, alert: t('flash.save_failed')

This lets me rescue if any association failed to save, without having to manually check them all. However, if attributes= tries to save each associated object before I call @foo.save, the first invalid one short-circuts the process and the other objects never get built, which means they can't be displayed with errors when we re-render the form.

For all these reasons, I think this line should simply be deleted.

If this change is made, @object.attributes = attribute_hash will set up associated objects but not save them, and @object.update(attribute_hash) will both set them up and save them. In other words, the associations will behave just like the object itself.

Thoughts?

[snusnu]

I'm all for this change, but giving it a quick try made a lot of the specs fail. I remember always thinking that it's wrong to save at this time, iirc it was something tricky with testing under different circumstances (with or without dm-validations or dm-constraints present).

I've setup dm-accepts_nested_attributes on travis and I will accept a patch that introduces the behavioral change while still keeping our (huge) test matrix green

Sorry for taking so long to respond!

[EDIT] travis is only setup for the release-1.2 branch currently. I'm planning to release a gem from that branch soon.