Spot icon indicating copy to clipboard operation
Spot copied to clipboard

Nested save

Open pnomolos opened this issue 13 years ago • 14 comments

Did some work on the plane today. Probably needs some more cleaning up and more test cases, but wanted to get your thoughts on where this is going.

pnomolos avatar Dec 05 '12 02:12 pnomolos

Looking good so far. Really nice work here. Before I merge this, just one question: I see delete called in a few places. What circumstances would cause the deletion of entity records, and is that expected by the user?

vlucas avatar Dec 05 '12 16:12 vlucas

Also - it would be sweet if save could automatically figure this out and use the saveNested method internally so the API would be consistent for the end user. How much additional work would that be?

vlucas avatar Dec 05 '12 16:12 vlucas

Re: delete - if something fails to save then I roll everything back, including deleting the original object. It's a bit of a chicken-and-egg problem. Nested models aren't valid until they have a parent object ID and you can't get that until you actually persist the parent object to the store. I didn't really see a better way - I'm sure I could do some partial validation to make it happen not as much, but ultimately there will need to be cases where things are rolled back.

For this reason it's probably best if saveNested isn't automatically called from save due to how it behaves. I felt that it has a specific use case and should probably not be automatically called with respect to how it interacts.

pnomolos avatar Dec 05 '12 17:12 pnomolos

How about wrapping everything in a transaction? It's the perfect use-case for this. Examples: https://github.com/vlucas/Spot/blob/master/tests/Test/Transactions.php

vlucas avatar Dec 05 '12 17:12 vlucas

I thought about that - are they fully available across both adapters?

pnomolos avatar Dec 05 '12 17:12 pnomolos

No - MongoDB does not support transactions.

vlucas avatar Dec 05 '12 17:12 vlucas

I guess the delete is fine as a rolback as long as all the originally set data is set back on the object (if it was modified), so that it's state doesn't change on a failed transaction. On a successful transaction, the relations should be loaded on the entity so they will work as normal immediately after save.

vlucas avatar Dec 05 '12 17:12 vlucas

OK. At this point saveNested doesn't play with existing objects so I don't have the first concern to worry about (yet). I'm going to work on that, though. I guess this also invalidates the second point because the base object isn't returned, just the primary key.

pnomolos avatar Dec 05 '12 17:12 pnomolos

It may not be returned, but objects are passed by reference by default in PHP, so when you modify them it modifies the original object instance as well. All you should have to do is call $this->loadRelations($entity); for it to work. It does that in get, insert, etc. and should do it in this case, because the nested array data needs to be transformed into working relation proxies so the relations can be retrieved and worked with as expected (this will discard the nested arrays, but that is okay since they were all saved okay).

vlucas avatar Dec 05 '12 17:12 vlucas

This will enable you to do (and we should test for this):

$mapper->saveNested($post);
$this->assertEquals(2, $post->comments->count());

vlucas avatar Dec 05 '12 17:12 vlucas

Ah, I see what you're getting at. We can't do that - saveNested only accepts an array, not an Entity object. Passing in an Entity object wouldn't work because there's no way to assign values to the relationships, unless we do something like Rails/ActiveRecord does with the *_attributes proxies.

pnomolos avatar Dec 05 '12 19:12 pnomolos

I think passing in the Entity is the best way to do this. We don't have to pre-load all the relationship objects - all we're doing is restoring the normal way to retrieve relations through the Spot\Relation proxy object, and that can be done using the loadRelations method.

vlucas avatar Dec 05 '12 20:12 vlucas

No - MongoDB does not support transactions.

Neither does MyISAM if I'm not mistaken ?

On Wed, Dec 5, 2012 at 9:28 PM, Vance Lucas [email protected]:

I think passing in the Entity is the best way to do this. We don't have to pre-load all the relationship objects - all we're doing is restoring the normal way to retrieve relations through the Spot\Relation proxy object, and that can be done using the loadRelations method.

— Reply to this email directly or view it on GitHubhttps://github.com/vlucas/Spot/pull/25#issuecomment-11059007.

marvelade avatar Dec 07 '12 07:12 marvelade

No, MyISAM does not support transaction either, but Spot uses InnoDB be default for MySQL.

vlucas avatar Dec 08 '12 17:12 vlucas