embedded create: remove orphans #999

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
5 participants
@heartsentwined
Contributor

heartsentwined commented May 26, 2013

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

// 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 short, 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.

@heartsentwined heartsentwined referenced this pull request in heartsentwined/ember-auth-rails-demo Jun 11, 2013

Closed

The app is not remembering me #3

@ghost ghost assigned wycats Aug 13, 2013

@wycats

This comment has been minimized.

Show comment Hide comment
@wycats

wycats Sep 5, 2013

Owner

We don't have embedded at the moment, but this discussion is sufficiently well described that I'm leaving this open for when we do transactions.

Owner

wycats commented Sep 5, 2013

We don't have embedded at the moment, but this discussion is sufficiently well described that I'm leaving this open for when we do transactions.

@fivetanley

This comment has been minimized.

Show comment Hide comment
@fivetanley

fivetanley Aug 13, 2014

Member

We have support for embedded stuff now via EmbeddedRecordsMixin. Closing this due to staleness, but feel free to reopen.

Member

fivetanley commented Aug 13, 2014

We have support for embedded stuff now via EmbeddedRecordsMixin. Closing this due to staleness, but feel free to reopen.

@fivetanley fivetanley closed this Aug 13, 2014

@Binarytales

This comment has been minimized.

Show comment Hide comment
@Binarytales

Binarytales Jan 15, 2015

@fivetanley I am still seeing the behaviour described by @heartsentwined in Ember Data 1.0.0-beta.14.1.

Is there something in EmbeddedRecordsMixin that resolves this? If so it might be helpful to provide a code example so others that find this discussion, as I did, can resolve the problem.

If not, is this expected behaviour or something that is still not yet resolved in EmbeddedRecordsMixin?

@fivetanley I am still seeing the behaviour described by @heartsentwined in Ember Data 1.0.0-beta.14.1.

Is there something in EmbeddedRecordsMixin that resolves this? If so it might be helpful to provide a code example so others that find this discussion, as I did, can resolve the problem.

If not, is this expected behaviour or something that is still not yet resolved in EmbeddedRecordsMixin?

@stevievines

This comment has been minimized.

Show comment Hide comment
@stevievines

stevievines Mar 16, 2015

@Binarytales I'm experiencing the same issue using Ember Data 1.0.0-beta.15, and I'm using the EmbeddedRecordsMixin. Were you able to find a working solution for this issue?

@Binarytales I'm experiencing the same issue using Ember Data 1.0.0-beta.15, and I'm using the EmbeddedRecordsMixin. Were you able to find a working solution for this issue?

@Binarytales

This comment has been minimized.

Show comment Hide comment
@Binarytales

Binarytales Mar 16, 2015

@stevievines I'm currently working around this using a couple of methods. Firstly, in the main situation I was hitting this problem I was able to create "shell" records on the server side so Ember already had reference-able IDs and was limited to just updating and saving existing records. Secondly, where I absolutely needed to create child/nested records on the client side I am issuing a .reload() on the parent object.

@stevievines I'm currently working around this using a couple of methods. Firstly, in the main situation I was hitting this problem I was able to create "shell" records on the server side so Ember already had reference-able IDs and was limited to just updating and saving existing records. Secondly, where I absolutely needed to create child/nested records on the client side I am issuing a .reload() on the parent object.

@stevievines

This comment has been minimized.

Show comment Hide comment
@stevievines

stevievines Mar 18, 2015

@Binarytales thanks for the response. I had to go a slightly different route. I'll leave it here for posterity, but it's definitely hacky.

In my specific situation, we have a list (basically an email list) model, that has many rules. The main complexity is that the rule has a polymorphic relationship to a parent object (it can belong to a list, or to a couple of other objects), and there are different rule types with different fields for each type. I've included a screenshot for reference. I didn't design the rule model, and it's possible that it's too complex...

image

When creating/editing a list, a user can add a rule to the list, which immediately creates a shell record on the ember side that they can then edit. We just pass the rules as nested attributes to our rails backend.

I had to just delete the null rules out of the store in the onSucess action for the list.save.then(onSuccess, onFail).

I ended up doing this in the onSuccess for my list.save()

 var rules = list.get('rules');
 var nullRules = rules.filterBy('id', null);
 nullRules.forEach(function(rule) {
   rule.destroyRecord();
 });

Hopefully this helps someone else! And if anyone has suggestions for how to make it better, or why I definitely should avoid this pattern, please feel free to share. I am very new to ember and loving learning it!

@Binarytales thanks for the response. I had to go a slightly different route. I'll leave it here for posterity, but it's definitely hacky.

In my specific situation, we have a list (basically an email list) model, that has many rules. The main complexity is that the rule has a polymorphic relationship to a parent object (it can belong to a list, or to a couple of other objects), and there are different rule types with different fields for each type. I've included a screenshot for reference. I didn't design the rule model, and it's possible that it's too complex...

image

When creating/editing a list, a user can add a rule to the list, which immediately creates a shell record on the ember side that they can then edit. We just pass the rules as nested attributes to our rails backend.

I had to just delete the null rules out of the store in the onSucess action for the list.save.then(onSuccess, onFail).

I ended up doing this in the onSuccess for my list.save()

 var rules = list.get('rules');
 var nullRules = rules.filterBy('id', null);
 nullRules.forEach(function(rule) {
   rule.destroyRecord();
 });

Hopefully this helps someone else! And if anyone has suggestions for how to make it better, or why I definitely should avoid this pattern, please feel free to share. I am very new to ember and loving learning it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment