From 31ca857c8559763ab737ebf640c05576bf6972f9 Mon Sep 17 00:00:00 2001 From: "David J. Hamilton" Date: Sun, 2 Oct 2016 17:40:49 -0700 Subject: [PATCH 1/5] First draft nested save rfc paired @igort --- text/0000-nested-save.md | 178 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 178 insertions(+) create mode 100644 text/0000-nested-save.md diff --git a/text/0000-nested-save.md b/text/0000-nested-save.md new file mode 100644 index 0000000000..7e9b5177c9 --- /dev/null +++ b/text/0000-nested-save.md @@ -0,0 +1,178 @@ +- Start Date: 2016-10-02 +- RFC PR: +- Ember Issue: + +# Summary + +Add support for saving a record and nested records within the same request. + +# Motivation + +Apps commonly want to save nested structures. This can happen, for instance, on +a page for creating an object in which child objects are created at the same +time. Usually the motivations are twofold: performance and transactional saves. + +Achieving this today has problems both in the create and update case. The +create case requires manually transitioning the child objects, or discarding +them, both of which are undesirable. + +The update case is achieved today using EmbeddedRecordsMixin, but the child +objects will not go through the normal save lifecycle, and so miss out on +correctly dirty tracking &c. + + +# Detailed design + +## Overview + +There are two main things the implementation needs to concern itself with. + + 1. Handling the save lifecycle for saved nested objects and + 2. Assigning ids to saved nested objects during create. + +The nested object lifecycle is managed by entangling the nested object with the +promise for saving the parent object. + +Assigning ids from the server is handled by having the store generate client ids +when the child objects are entangled with their parent object's save promise, +and then having those client ids added to the child object's payload within +`included`, during `normalizePayload`. + +In the event that the server echoes back the client ids, users will not need to +add anything to `normalizePayload`. To handle cases where the server does not +do this, `normalizePayload` will be passed a snapshot of the parent object, and +an array of snapshots of the saved nested objects, so that it can find their +client ids and add them to their respective structures within the `included` +portion of the payload. + +## Example Usage with API Server Echoing clientId + +Here is an example of nested saving new objects: + +```js +// models/order.js +DS.Model.extend({ + items: DS.hasMany('items'), +}); + +// routes/order.js +Ember.Route.extend({ + actions: { + saveOrder(order) { + let orderSavePromise = order.save(); + order.get('items').forEach(item => item.savedWith(orderSavePromise)) + } + } +}); +``` + +## Example Usage with API Server Not Echoing clientId + + +```js +// models/order.js +// same as previous example + +// routes/order.js +// same as previous example + +// serializers/order.js +JsonApiSerializer.extend({ + normalizeResponse(store, + primaryModelClass, + payload, + id, + requestType, + snapshot, + nestedSaves) { + + let jsonApiPayload = this.super(...arguments); + nestedSaves.forEach((childSnapshot) => { + let nestedSavePayload = + jsonApiPayload.included.find((childPayload) => { + /* + App specific logic for finding the included payload for this nested + save object. + */ + }); + + nestedSavePayload.clientId = childSnapshot.clientId; + }); + + return jsonApiPayload; + } +}); +``` + +## Matching From `nestedSaves` vs Matching From `snapshot` + +## Nesting Depth > 1 + +Nothing changes if saving a nested structure in which nesting occurs at depth +greater than 1 (eg if saving an object, its children, and grandchildren). It +will simply be the case that more objects need to have their `savedWith` methods +called, those same objects will be added to `nestedSaves`, and they will need to +be included in the `included` section of the JSON API response payload. + +Similarly, saving a nested structure in which some levels are skipped works. +For instance, saving a grandparent and grandchild, but not the intermediate +child, would result in the grandparent and grandchild being persisted and the +relationships set up but with an as-yet unpersisted child. Supporting this case +is not a goal of this rfc, and I am not aware of any sensible use case for it, +but noting that it is supported is included here for completeness. + +## Errors + +If the adapter save promise rejects, all saved objects (ie the primary saved +object and the nested saved objects) transition to an error state. The specific +error state depends on the specific type of error the promise rejected with, but +all saved objects transition to the same state. In particular, they will all +transition to an invalid state if the promise rejects with an `InvalidError` +even though the errors returned from `extractErrors` may not include errors for +each of the saved objects. + +For consistency, `snapshot` and `nestedSaves` will also be passed to +`extractErrors`. + +# How We Teach This + +What names and terminology work best for these concepts and why? How is this +idea best presented? As a continuation of existing Ember patterns, or as a +wholly new one? + +Would the acceptance of this proposal mean the Ember guides must be +re-organized or altered? Does it change how Ember is taught to new users +at any level? + +How should this feature be introduced and taught to existing Ember +users? + + +The additional functionality proposed in this rfc is quite limited in scope. +All that should be needed is a guide exploring how to support nested saves, and +the additional API added to any discussion of implementing a custom serializer. + +# Drawbacks + +One obvious drawback is that this rfc introduces a new API, `savedWith`, and +adds parameters to `normalizePayload` and `extractErrors`. Although this +doesn't add *much* to the API surface area, all such complexity comes at some +cost. + +A second potential objection may be that the rfc feels a bit scenario solving, +albeit for a very common scenario. + + +# Alternatives + +Other possibilities include: + + - Adding support for nested saving as part of a broader save coalescing API, + similar to existing find coalescing and + - Making the state transition API public. + +# Unresolved questions + +Are there issues with transitioning nested saved objects to an invalid state +even in cases where the nested object itself did not have errors? + From 93e225b526df05d62d949a64c92c2f92c98e2063 Mon Sep 17 00:00:00 2001 From: "David J. Hamilton" Date: Sun, 2 Oct 2016 20:20:30 -0700 Subject: [PATCH 2/5] wip --- text/0000-nested-save.md | 49 +++++++++++++++++++++++++--------------- 1 file changed, 31 insertions(+), 18 deletions(-) diff --git a/text/0000-nested-save.md b/text/0000-nested-save.md index 7e9b5177c9..f68dc7e4ed 100644 --- a/text/0000-nested-save.md +++ b/text/0000-nested-save.md @@ -20,7 +20,6 @@ The update case is achieved today using EmbeddedRecordsMixin, but the child objects will not go through the normal save lifecycle, and so miss out on correctly dirty tracking &c. - # Detailed design ## Overview @@ -45,7 +44,26 @@ an array of snapshots of the saved nested objects, so that it can find their client ids and add them to their respective structures within the `included` portion of the payload. -## Example Usage with API Server Echoing clientId +## APIs + +This RFC introduces the following changes: + +- Add the method `DS.Model#savedWith` +- Add the parameter `snapshot` to `DS.Serializer#normalizePaylaod` +- Add the property `savedWith` to `DS.Snapshot` + +`DS.Model#savedWith(promise)` is passed the `promise` for an ancestor object's +save. It indicates that this record is being saved within the same promise. +When invoked, it will transition the record to `inFlight`. + +If `promise` rejects, the record will transition to an error state, depending on +what type of error the promise rejects with. + +If `promise` fulfills and the + +## Examples + +### Example Usage with API Server Echoing clientId Here is an example of nested saving new objects: @@ -66,7 +84,7 @@ Ember.Route.extend({ }); ``` -## Example Usage with API Server Not Echoing clientId +### Example Usage with API Server Not Echoing clientId ```js @@ -83,11 +101,10 @@ JsonApiSerializer.extend({ payload, id, requestType, - snapshot, - nestedSaves) { + snapshot) { let jsonApiPayload = this.super(...arguments); - nestedSaves.forEach((childSnapshot) => { + snapshot.get('savedWith').forEach((childSnapshot) => { let nestedSavePayload = jsonApiPayload.included.find((childPayload) => { /* @@ -136,18 +153,6 @@ For consistency, `snapshot` and `nestedSaves` will also be passed to # How We Teach This -What names and terminology work best for these concepts and why? How is this -idea best presented? As a continuation of existing Ember patterns, or as a -wholly new one? - -Would the acceptance of this proposal mean the Ember guides must be -re-organized or altered? Does it change how Ember is taught to new users -at any level? - -How should this feature be introduced and taught to existing Ember -users? - - The additional functionality proposed in this rfc is quite limited in scope. All that should be needed is a guide exploring how to support nested saves, and the additional API added to any discussion of implementing a custom serializer. @@ -171,8 +176,16 @@ Other possibilities include: similar to existing find coalescing and - Making the state transition API public. +Related work: + +- [Discarding nested saved objects](https://github.com/emberjs/data/pull/4441) +- [ember-data-save-relationships](https://github.com/frank06/ember-data-save-relationships) + (handles only the case where the server echoes the client id) + # Unresolved questions Are there issues with transitioning nested saved objects to an invalid state even in cases where the nested object itself did not have errors? +TODO: Improve names. + From da47efe2829ac98f9f6987b8b1b0c999afa707d7 Mon Sep 17 00:00:00 2001 From: IgorT Date: Sun, 2 Oct 2016 21:59:22 -0700 Subject: [PATCH 3/5] igor changes --- text/0000-nested-save.md | 34 +++++++++++++++++++++++++++++++++- 1 file changed, 33 insertions(+), 1 deletion(-) diff --git a/text/0000-nested-save.md b/text/0000-nested-save.md index f68dc7e4ed..b6e6a13eb6 100644 --- a/text/0000-nested-save.md +++ b/text/0000-nested-save.md @@ -4,7 +4,21 @@ # Summary -Add support for saving a record and nested records within the same request. +Support accurate state tracking and record matching when +saving a record and nested records within the same request. + +This rfc proposes two main API changes: + + 1. Add a `record.savedWith` method + To be used like: + ```js + let promise = parentRecord.save(); + childRecord.savedWith(promise): + ``` + where the childRecord will now follow the state transitions of the parent record. + + 2. Allow the `store.push` method to receive an optional client id, which it will + use as a fallback to avoid creating duplicate records. # Motivation @@ -20,6 +34,19 @@ The update case is achieved today using EmbeddedRecordsMixin, but the child objects will not go through the normal save lifecycle, and so miss out on correctly dirty tracking &c. +There have been several issues and PRs filed to address this problem, +some are: +https://github.com/emberjs/data/pull/4441 +https://github.com/emberjs/data/issues/1829 + +There is also an Ember Data addon which attempts to solve this problem: +https://www.npmjs.com/package/ember-data-save-relationships + +In general, the existing addons and PRs tend to hack & scenario solve the +problem without presenting a cohesive story for managing nested saves. + +--Maybe add duplicate records as a reason to use clientIds-- + # Detailed design ## Overview @@ -29,9 +56,14 @@ There are two main things the implementation needs to concern itself with. 1. Handling the save lifecycle for saved nested objects and 2. Assigning ids to saved nested objects during create. +## 1. Handling the save lifecycle for saved nested objects + +Currently, evif your serializer saves multiple records The nested object lifecycle is managed by entangling the nested object with the promise for saving the parent object. + + Assigning ids from the server is handled by having the store generate client ids when the child objects are entangled with their parent object's save promise, and then having those client ids added to the child object's payload within From cd6f0f9f79e0f87b50d3cdb6295b7eb50674ed43 Mon Sep 17 00:00:00 2001 From: IgorT Date: Mon, 17 Oct 2016 19:53:56 +0300 Subject: [PATCH 4/5] fleshed out --- text/0000-nested-save.md | 129 ++++++++++++++++++++++++++++++++------- 1 file changed, 106 insertions(+), 23 deletions(-) diff --git a/text/0000-nested-save.md b/text/0000-nested-save.md index b6e6a13eb6..d375f45799 100644 --- a/text/0000-nested-save.md +++ b/text/0000-nested-save.md @@ -42,10 +42,27 @@ https://github.com/emberjs/data/issues/1829 There is also an Ember Data addon which attempts to solve this problem: https://www.npmjs.com/package/ember-data-save-relationships -In general, the existing addons and PRs tend to hack & scenario solve the -problem without presenting a cohesive story for managing nested saves. +-- In general, the existing addons and solutions have been great as workarounds +for people, but have not presented a cohesive and stable story for managing nested saves. +in Ember Data. ---Maybe add duplicate records as a reason to use clientIds-- +-- Another problem which has often been brought up in Ember Data is the race condition +when receiving out of band updates on record creation. + +When you save a newly created record, and if you receive an out of band web socket update, +before the save has returned, you will end up with two ember data records. +```js + let user1 = this.store.createRecord('user'); + user1.save(); + // out of band web socket update comes and tells you have a new user on the server + let user2 = store.push({ id:1, type: 'user'}); + // now save comes back and tells you the newly assigned id to user1 is '1' + // it turns out that the out of band update was for the newly created user, and now + // you have two user objects with same id +``` +clientId support offers a mitigation strategy for this problem, because, if the clientId is +reflected back in the web socket push, we can tell that the incoming record is the same +as the inflight one. # Detailed design @@ -55,44 +72,110 @@ There are two main things the implementation needs to concern itself with. 1. Handling the save lifecycle for saved nested objects and 2. Assigning ids to saved nested objects during create. + 3. Convenience methods for serializers ## 1. Handling the save lifecycle for saved nested objects -Currently, evif your serializer saves multiple records -The nested object lifecycle is managed by entangling the nested object with the +Currently, even if your serializer saves multiple records, it is very hard to/impossible +manage the lifecycle of the records being bulk saved. + +This RFC proposes an API for entangling nested objects with the promise for saving the parent object. +### APIs +- Add the method `DS.Model#savedWith` + +`DS.Model#savedWith(promise)` is passed the `promise` for an ancestor object's +save. It indicates that this record is being saved within the same promise. +When invoked, it will transition the record to `inFlight`. + +`savedWith` returns a promise for the child record. + +If `promise` rejects, the record will transition to an error state, depending on +what type of error the promise rejects with, and the childPromise will reject with +the error. + +If `promise` fulfills, the record will transition to the saved state, and the child promise +will fulfill with the child record. + +## 2. Assigning ids to saved nested objects during create. + +Currently, it is impossible to match incoming records which were newly saved, with +records that exists in Ember Data store. Consider the problem of saving multiple records +together in an embedded structure, for example saving an order with multiple items in +an online shopping portal. + +```js +order.save(); +// results in: +{ + type: 'order', + items: [{ type: 'item', name: 'laptop' }, + { type:'item', name: 'camera' }] +} +``` + +Currently it is impossible to easily match up returning items to existing records in the +ember data store. + +The canonical way of solving this problem is by using client ids, which the server reflects +back to the client. However, even if you do this, there are no easy ways of matching the records +up in ember data. + +We are proposing Ember Data add support for client ids, such that if the request returns back +client ids with records, the store will do the matching. +### APIs -Assigning ids from the server is handled by having the store generate client ids -when the child objects are entangled with their parent object's save promise, -and then having those client ids added to the child object's payload within -`included`, during `normalizePayload`. +- Add the getter `DS.Snapshot#clientId` +It is lazily generated when accessed and persisted locally. + +- If the record resource returned from store.push is not already in the identity map, +but has a clientId, the store will try to look up the record by the clientId before pushing +a new record to the identity map. + +For example, if you sent to the server + +```js +// results in: +{ + type: 'order', + items: [{ clientId: 1, type: 'item', name: 'laptop' }, + { clientId:2, type:'item', name: 'camera' }] +} +``` + +and the Serializer returns + +```js +{ + data: { + id: 1, + type: 'order', + }, + included: [ + { id:1, clientId: 1, type: 'item', name: 'laptop' } + { id:2, clientId:2, type:'item', name: 'camera' } + ] +} +``` + +The store would match up the records by the clientIds. + +## 3. Convenience methods for serializers In the event that the server echoes back the client ids, users will not need to -add anything to `normalizePayload`. To handle cases where the server does not +do anything. To handle cases where the server does not do this, `normalizePayload` will be passed a snapshot of the parent object, and an array of snapshots of the saved nested objects, so that it can find their client ids and add them to their respective structures within the `included` portion of the payload. -## APIs - -This RFC introduces the following changes: +### APIs -- Add the method `DS.Model#savedWith` - Add the parameter `snapshot` to `DS.Serializer#normalizePaylaod` - Add the property `savedWith` to `DS.Snapshot` -`DS.Model#savedWith(promise)` is passed the `promise` for an ancestor object's -save. It indicates that this record is being saved within the same promise. -When invoked, it will transition the record to `inFlight`. - -If `promise` rejects, the record will transition to an error state, depending on -what type of error the promise rejects with. - -If `promise` fulfills and the - ## Examples ### Example Usage with API Server Echoing clientId From 824c6a268bac4ecd9f8fd22db93cfff5478524d4 Mon Sep 17 00:00:00 2001 From: "David J. Hamilton" Date: Tue, 18 Oct 2016 07:12:55 -0700 Subject: [PATCH 5/5] Minor changes - small editing - expand "scenario solving" --- text/0000-nested-save.md | 85 ++++++++++++++++++++++------------------ 1 file changed, 47 insertions(+), 38 deletions(-) diff --git a/text/0000-nested-save.md b/text/0000-nested-save.md index d375f45799..819dcf039c 100644 --- a/text/0000-nested-save.md +++ b/text/0000-nested-save.md @@ -1,4 +1,4 @@ -- Start Date: 2016-10-02 +- Start Date: 2016-10-18 - RFC PR: - Ember Issue: @@ -15,9 +15,9 @@ This rfc proposes two main API changes: let promise = parentRecord.save(); childRecord.savedWith(promise): ``` - where the childRecord will now follow the state transitions of the parent record. + where `childRecord` will now follow the state transitions of the parent record. - 2. Allow the `store.push` method to receive an optional client id, which it will + 2. Allow `store.push` to receive an optional client id, which it will use as a fallback to avoid creating duplicate records. # Motivation @@ -30,27 +30,29 @@ Achieving this today has problems both in the create and update case. The create case requires manually transitioning the child objects, or discarding them, both of which are undesirable. -The update case is achieved today using EmbeddedRecordsMixin, but the child +The update case is achieved today using `EmbeddedRecordsMixin`, but the child objects will not go through the normal save lifecycle, and so miss out on correctly dirty tracking &c. There have been several issues and PRs filed to address this problem, -some are: -https://github.com/emberjs/data/pull/4441 -https://github.com/emberjs/data/issues/1829 +including but not limited to: + + - https://github.com/emberjs/data/pull/4441 + - https://github.com/emberjs/data/issues/1829 There is also an Ember Data addon which attempts to solve this problem: -https://www.npmjs.com/package/ember-data-save-relationships --- In general, the existing addons and solutions have been great as workarounds -for people, but have not presented a cohesive and stable story for managing nested saves. -in Ember Data. + - https://www.npmjs.com/package/ember-data-save-relationships + +In general, the existing addons and solutions have been great as workarounds, +but have not presented a cohesive and stable story for managing nested saves. --- Another problem which has often been brought up in Ember Data is the race condition +Another problem which has often been brought up is the race condition when receiving out of band updates on record creation. -When you save a newly created record, and if you receive an out of band web socket update, -before the save has returned, you will end up with two ember data records. +When you save a newly created record, and receive an out of band update before +the save has returned, eg via a websocket, you will end up with two ember data +records. ```js let user1 = this.store.createRecord('user'); user1.save(); @@ -60,15 +62,19 @@ before the save has returned, you will end up with two ember data records. // it turns out that the out of band update was for the newly created user, and now // you have two user objects with same id ``` -clientId support offers a mitigation strategy for this problem, because, if the clientId is -reflected back in the web socket push, we can tell that the incoming record is the same -as the inflight one. +The problem is that the store has no way of knowing that the payload pushed +during the out of band update is the newly created record, and so cannot assign +the payload to the correct `internalModel`. + +`clientId` support offers a mitigation strategy for this problem, because, if +the `clientId` is reflected back in the web socket push, we can tell that the +incoming record is the same as the inflight one. # Detailed design ## Overview -There are two main things the implementation needs to concern itself with. +There are three main things the implementation needs to concern itself with. 1. Handling the save lifecycle for saved nested objects and 2. Assigning ids to saved nested objects during create. @@ -76,8 +82,8 @@ There are two main things the implementation needs to concern itself with. ## 1. Handling the save lifecycle for saved nested objects -Currently, even if your serializer saves multiple records, it is very hard to/impossible -manage the lifecycle of the records being bulk saved. +Currently, even if your serializer saves multiple records, it is very +hard/impossible to manage the lifecycle of the records being bulk saved. This RFC proposes an API for entangling nested objects with the promise for saving the parent object. @@ -100,10 +106,10 @@ will fulfill with the child record. ## 2. Assigning ids to saved nested objects during create. -Currently, it is impossible to match incoming records which were newly saved, with -records that exists in Ember Data store. Consider the problem of saving multiple records -together in an embedded structure, for example saving an order with multiple items in -an online shopping portal. +Currently, it is impossible to match incoming records which were newly saved, +with records that exists in the Ember Data store. Consider the problem of saving +multiple records together in an embedded structure, for example saving an order +with multiple items in an online shopping portal. ```js order.save(); @@ -120,7 +126,7 @@ ember data store. The canonical way of solving this problem is by using client ids, which the server reflects back to the client. However, even if you do this, there are no easy ways of matching the records -up in ember data. +up within a serializer or other public API of Ember Data. We are proposing Ember Data add support for client ids, such that if the request returns back client ids with records, the store will do the matching. @@ -130,9 +136,9 @@ client ids with records, the store will do the matching. - Add the getter `DS.Snapshot#clientId` It is lazily generated when accessed and persisted locally. -- If the record resource returned from store.push is not already in the identity map, -but has a clientId, the store will try to look up the record by the clientId before pushing -a new record to the identity map. +- If the record resource returned from store.push is not already in the identity + map, but has a `clientId`, the store will try to look up the record by the + `clientId` before pushing a new record to the identity map. For example, if you sent to the server @@ -154,19 +160,19 @@ and the Serializer returns type: 'order', }, included: [ - { id:1, clientId: 1, type: 'item', name: 'laptop' } - { id:2, clientId:2, type:'item', name: 'camera' } + { id:1, clientId: 1, type: 'item', attributes: { name: 'laptop' } } + { id:2, clientId: 2, type: 'item', attributes: { name: 'camera' } } ] } ``` -The store would match up the records by the clientIds. +The store would match up the records by `clientId`. ## 3. Convenience methods for serializers In the event that the server echoes back the client ids, users will not need to do anything. To handle cases where the server does not -do this, `normalizePayload` will be passed a snapshot of the parent object, and +echo client ids, `normalizePayload` will be passed a snapshot of the parent object, and an array of snapshots of the saved nested objects, so that it can find their client ids and add them to their respective structures within the `included` portion of the payload. @@ -250,7 +256,7 @@ Similarly, saving a nested structure in which some levels are skipped works. For instance, saving a grandparent and grandchild, but not the intermediate child, would result in the grandparent and grandchild being persisted and the relationships set up but with an as-yet unpersisted child. Supporting this case -is not a goal of this rfc, and I am not aware of any sensible use case for it, +is not a goal of this rfc, and we are not aware of any sensible use case for it, but noting that it is supported is included here for completeness. ## Errors @@ -279,8 +285,13 @@ adds parameters to `normalizePayload` and `extractErrors`. Although this doesn't add *much* to the API surface area, all such complexity comes at some cost. -A second potential objection may be that the rfc feels a bit scenario solving, -albeit for a very common scenario. +A second potential objection may be that the rfc is scenario solving. This is +true, but + + 1. The scenario solved is very common, + 2. The changes required to support it are minimal, and + 3. Generalising the problem to something like arbitrary save coalescing makes + the problem considerably more difficult for questionable marginal gain. # Alternatives @@ -289,7 +300,7 @@ Other possibilities include: - Adding support for nested saving as part of a broader save coalescing API, similar to existing find coalescing and - - Making the state transition API public. + - Making the model state transition API public. Related work: @@ -302,5 +313,3 @@ Related work: Are there issues with transitioning nested saved objects to an invalid state even in cases where the nested object itself did not have errors? -TODO: Improve names. -