emberjs/data — pull request #999 by heartsentwined (2013-05-26 04:01:03+00:00)
Title: embedded create: remove orphans

When one creates a record with (new) children, `commit()` it through the adapter, with the child being `{ embedded: 'always' }`, this will cause the store to not being able to identify the (existing) created children, and simply load the "new" ones in the payload from the `201 Created` response. The parent record will now have two sets of children: the originally created one with `id = null` and status still in `inFlight`, and the new one extracted from the server response, with `id` properly set to the one returned by the server. This happens recursively to grandchildren too.

# Example

```javascript
// this fragment may not work on its own,
// my actual use case involves transactions and a couple of post-create logic
// I just try to demonstrate the concept of a minimal case here

// App.Parent, App.Child, App.Grandchild are DS.Models
DS.RESTAdapter.map('App.Parent', { children: { embedded: 'always' } });
DS.RESTAdapter.map('App.Child', { grandchildren: { embedded: 'always' } });

App.Parent.find().get('length'); // => 0
App.Child.find().get('length'); // => 0
App.Grandchild.find().get('length'); // => 0

// create some records
parent = App.Parent.createRecord();
child = parent.children.createRecord();
grandchild = child.grandchildren.createRecord();

// commit it
// note that there is only a single `POST` request because we are using { embedded: 'always' }
store.commit()

// we got a 201 Created, payload something like:
// {
// "parent":
// {
// "id": "1",
// "children":
// [{
// "id": "1",
// "grandchildren":
// [{
// "id": "1"
// }]
// }]
// }
// }

child.get('id'); // => null (!)
grandchild.get('id'); // => null (!)

child.get('stateManager.currentPath'); // => 'created.inFlight' (!)
grandchild.get('stateManager.currentPath'); // => 'created.inFlight' (!)

App.Parent.find().get('length'); // => 1
App.Child.find().get('length'); // => 2 (!)
App.Grandchild.find().get('length'); // => 2 (!)

parent.get('children'); // => (both the `id = 1` and the `id = null` records) (!)
child.get('grandchildren'); // => (both the `id = 1` and the `id = null` records) (!)
```

# Fix

This happens because the adapter only has a reference to the parent record. When the `201 Created` successful response comes back, the adapter updates the referenced parent record, but then proceeds to *extract* children from the payload, not updating the existing children.

This PR fixes it, by **removing** the existing orphaned children, i.e. all the `id = null` / `inFlight` records identified above. The orphaned children will be transitioned to the `uncommitted` state without making any lifecycle callbacks. It then rollbacks the orphaned record. The idea is to avoid any callbacks altogether (because this is not a normal `deleteRecord()` call).

# Rationale / Discussion

The proper fix should have been updating the `id` of existing children, but at the current state of affairs, the adapter has no reliable way of knowing which correspond to which. Both the serialized request data and the response data only use arrays to store embedded children models. Unless we can assume / enforce that the order will be the same for both arrays, we cannot link existing children together. There is complicated discussion at json-api/json-api#7 on this, but there is no consensus yet.

There is also no way of guaranteeing that all the children originally created will exist at all, or that the data in child models are preserved intact. However, I take it that a `201 Created` successful response is indicator that everything is in order. First, if any validation errors happen on child models, then a `422 Unprocessable Entity` should have been returned in the first place. Second, if it is considered normal / successful (in app logic) to modify the child model before returning it (e.g. setting a default value to a field when none is provided), then this modified data is part of the expected behavior. Finally, even removing some children might still be considered normal / successful in some app use case. In sshort, trust the `201 Created`, and expect to use its payload to override the existing models.

In other matters, it is also mandatory for a (compliant) server to return a payload for a `201 Created` in response to a `POST /resources` creation action; a `204 No Content` should not be expected.

Why using `{ embedded: 'always' }` in the first place though? One word: **atomicity**. emberjs/data#724 introduces an alternative, but the multistage-commit has no way of guaranteeing atomicity, nor any way of rolling back changes if, let's say, the second-stage server request fails (and thus we should rollback the first-stage server request). The embedded style will allow the server to work on the full set of changes at once.