Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Allow parent and child records to be saved in the same commit #440

Closed
wants to merge 2 commits into from
@pivotal-medici

Currently (as of 4c47535) you can't create a new parent and child record in the same commit (see #437).

We're hoping to use this pull request as the start of a conversation about how best to fix this issue. We've added a failing integration test for the interaction between the store and REST adapter.

The current error is:

<DS.StateManager> could not respond to event didChangeData in state rootState.loaded.created.inFlight

This happens because the store, upon receiving a call to didSaveRecord, assumes the parent is in the loaded.saved state. It's not, however, because it still has a dirty factor for its has-many relationship.

@elliterate

In DS.RESTAdapter, we've implemented a delayed create and update for children of new parents.

In addition to integration tests, we've also added unit tests for DS.RESTAdapter#createRecord and DS.RESTAdapter#updateRecord that demonstrate just how the adapter is expected to behave.

To get around the "could not respond to event didChangeData" error, we cloned the existing didChangeData event onto the inFlight state. It didn't seem like that would be a problem, as that event wasn't triggering a state change—just materializing the new data.

@pivotal-medici

We removed the sinon.js depedency.

@pivotal-medici

We found and fixed another issue preventing some parent-child pairs from being saved in the same commit. If the response from the server includes updated data that needs to be sideloaded into one of the other in-flight objects, the state manager for that object will complain that it can't trigger 'loadedData' while in flight. See our second commit for a more thorough example of the problem.

@pivotal-medici

We added an adapter hook to determine whether dirty records should be preserved on materialization.

We also rebased against current master.

@tomdale
Owner

@wycats and I are in the middle of a huge refactor of the REST adapter to support embedded relationships. We will get this in as soon as that work is finished. Thank you for your patience!

@go1dfish

Any updates on this now that the embedded relationships refactor is merged?

@go1dfish go1dfish referenced this pull request in escalant3/ember-data-tastypie-adapter
Closed

Add support for saving graphs of objects #10

@renajohn

I'm really interested in this feature. Any update?

@frankmayer

Me too :)

@ghempton
Collaborator

This is a cool feature, however these semantics are a little too specific for a generic rest adapter. Maybe create a new DS.RelationalAdapter?

@thingista

I understand this issue might be a bit specific to the server-side technology, but I was just wondering if you guys ever managed to find a solution for this (even custom)? Even with the new {embedded: 'always'} setting on the router, it still does not work as far as I can tell and Ember keeps firing multiple POSTs.

@xamenrax

Interested in feature :(

@thingista

@xamenrax You might want to take a look at the Relational Adapter that @ghempton put together: #724

Very useful. It helped me a lot.

@pivotal-medici

This has been rebased against master. We're successfully using it in our application, but @tomdale has said it'll need work before it makes it into master. Namely, the mechanism for preserving dirty children needs to be abstracted into the adapter and/or serializer.

@ghempton
Collaborator

@pivotal-medici this is very similar to #724. We should find a way to combine our efforts.

Alex Kwiatko... and others added some commits
Alex Kwiatkowski & Ian Lesperance Delay saving children until parents are ready
Relational databases store the foreign key on the
child. If the parent of a child is new, that
record must be created first before the child can
be saved, as the child needs to know the parent's
primary key.

The DS.RESTAdapter now observes new parents of
dependent children, delaying their committal until
the parent has been created.
e5f89de
Ian Lesperance & Robert Gallagher Add hook for preserving dirty children on load
This is specifically needed for the REST adapter to ensure that dirty
children are preserved when new parent data comes back from the server.
Because the children are responsible for saving the relationship, the
parent record on the server won't yet know about the children. If the
children are dirty, it means they will be persisted to the server on
their transaction's next commit, at which point the server will then
know about them.
d579d14
@tomdale
Owner

What's the status of this? Wasn't this fixed?

@bradleypriest
Collaborator

Nope, this is still an issue in my application. There are two implementations, this one and #724, and hasMany relationships don't really work without using one of them.

@wamrewam

Just meant to add: this (we use #724 but it is the same idea) has proven extremely useful to us too. It would definitely be fantastic to have it merged. And it has been pretty stable as well.

@stefanpenner

closing in favour of: #724

@vineet-sinha vineet-sinha referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
@vineet-sinha vineet-sinha referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 25, 2013
  1. @elliterate

    Delay saving children until parents are ready

    Alex Kwiatkowski & Ian Lesperance authored elliterate committed
    Relational databases store the foreign key on the
    child. If the parent of a child is new, that
    record must be created first before the child can
    be saved, as the child needs to know the parent's
    primary key.
    
    The DS.RESTAdapter now observes new parents of
    dependent children, delaying their committal until
    the parent has been created.
  2. @elliterate

    Add hook for preserving dirty children on load

    Ian Lesperance & Robert Gallagher authored elliterate committed
    This is specifically needed for the REST adapter to ensure that dirty
    children are preserved when new parent data comes back from the server.
    Because the children are responsible for saving the relationship, the
    parent record on the server won't yet know about the children. If the
    children are dirty, it means they will be persisted to the server on
    their transaction's next commit, at which point the server will then
    know about them.
This page is out of date. Refresh to see the latest.
View
102 packages/ember-data/lib/adapters/rest_adapter.js
@@ -77,23 +77,29 @@ DS.RESTAdapter = DS.Adapter.extend({
},
createRecord: function(store, type, record) {
- var root = this.rootForType(type);
-
- var data = {};
- data[root] = this.serialize(record, { includeId: true });
+ waitForParents(record, function() {
+ var root = this.rootForType(type);
+
+ var data = {};
+ data[root] = this.serialize(record, { includeId: true });
+
+ this.ajax(this.buildURL(root), "POST", {
+ data: data,
+ context: this,
+ success: function(json) {
+ Ember.run(this, function(){
+ this.didCreateRecord(store, type, record, json);
+ });
+ },
+ error: function(xhr) {
+ this.didError(store, type, record, xhr);
+ }
+ });
+ }, this);
+ },
- this.ajax(this.buildURL(root), "POST", {
- data: data,
- context: this,
- success: function(json) {
- Ember.run(this, function(){
- this.didCreateRecord(store, type, record, json);
- });
- },
- error: function(xhr) {
- this.didError(store, type, record, xhr);
- }
- });
+ shouldPreserveDirtyRecords: function(relationship) {
+ return relationship.kind === 'hasMany';
},
dirtyRecordsForRecordChange: function(dirtySet, record) {
@@ -153,24 +159,26 @@ DS.RESTAdapter = DS.Adapter.extend({
},
updateRecord: function(store, type, record) {
- var id = get(record, 'id');
- var root = this.rootForType(type);
-
- var data = {};
- data[root] = this.serialize(record);
-
- this.ajax(this.buildURL(root, id), "PUT", {
- data: data,
- context: this,
- success: function(json) {
- Ember.run(this, function(){
- this.didSaveRecord(store, type, record, json);
- });
- },
- error: function(xhr) {
- this.didError(store, type, record, xhr);
- }
- });
+ waitForParents(record, function() {
+ var id = get(record, 'id');
+ var root = this.rootForType(type);
+
+ var data = {};
+ data[root] = this.serialize(record);
+
+ this.ajax(this.buildURL(root, id), "PUT", {
+ data: data,
+ context: this,
+ success: function(json) {
+ Ember.run(this, function(){
+ this.didSaveRecord(store, type, record, json);
+ });
+ },
+ error: function(xhr) {
+ this.didError(store, type, record, xhr);
+ }
+ });
+ }, this);
},
updateRecords: function(store, type, records) {
@@ -366,3 +374,29 @@ DS.RESTAdapter = DS.Adapter.extend({
}
});
+function waitForParents(record, callback, context) {
+ var observers = new Ember.Set();
+
+ record.eachRelationship(function(name, meta) {
+ var relationship = record.cacheFor(name);
+
+ if (meta.kind === 'belongsTo' && relationship && get(relationship, 'isNew')) {
+ var observer = function() {
+ relationship.removeObserver('id', context, observer);
+ observers.remove(name);
+ finish();
+ };
+
+ relationship.addObserver('id', context, observer);
+ observers.add(name);
+ }
+ });
+
+ finish();
+
+ function finish() {
+ if (observers.length === 0) {
+ callback.call(context);
+ }
+ }
+}
View
13 packages/ember-data/lib/system/adapter.js
@@ -462,6 +462,19 @@ DS.Adapter = Ember.Object.extend(DS._Mappable, {
/**
@private
+ Determines whether dirty records for a particular association
+ should be preserved in the face of re-materialization.
+
+ @param {Object} association the association meta information
+ @return {Boolean}
+ */
+ shouldPreserveDirtyRecords: function(association) {
+ return false;
+ },
+
+ /**
+ @private
+
This method recursively climbs the superclass hierarchy and
registers any class-registered transforms on the adapter's
serializer.
View
19 packages/ember-data/lib/system/model/model.js
@@ -185,12 +185,15 @@ DS.Model = Ember.Object.extend(Ember.Evented, LoadPromise, {
}, 'data'),
hasManyDidChange: function(key) {
- var cachedValue = this.cacheFor(key);
+ var cachedValue = this.cacheFor(key),
+ store = get(this, 'store'),
+ adapter = store.adapterForType(this.constructor);
if (cachedValue) {
- var type = get(this.constructor, 'relationshipsByName').get(key).type;
- var store = get(this, 'store');
- var ids = this._data.hasMany[key] || [];
+ var relationship = get(this.constructor, 'relationshipsByName').get(key),
+ type = relationship.type,
+ ids = this._data.hasMany[key] || [],
+ dirtyReferences;
var references = map(ids, function(id) {
// if it was already a reference, return the reference
@@ -198,6 +201,14 @@ DS.Model = Ember.Object.extend(Ember.Evented, LoadPromise, {
return store.referenceForId(type, id);
});
+ if (adapter.shouldPreserveDirtyRecords(relationship)) {
+ dirtyReferences = get(cachedValue, 'content').map(function(reference) {
+ var record = store.findByClientId(reference.type, reference.clientId);
+ return get(record, 'isDirty') ? reference : null;
+ });
+ references = references.concat(Ember.A(dirtyReferences).compact());
+ }
+
set(cachedValue, 'content', Ember.A(references));
}
},
View
177 packages/ember-data/tests/integration/relationships/rest_adapter_test.js
@@ -0,0 +1,177 @@
+var get = Ember.get, set = Ember.set;
+
+var Person, Comment, store, requests;
+
+Person = DS.Model.extend();
+Comment = DS.Model.extend();
+
+Person.reopen({
+ name: DS.attr('string'),
+ comments: DS.hasMany(Comment)
+});
+Person.toString = function() { return "Person"; };
+
+Comment.reopen({
+ body: DS.attr('string'),
+ person: DS.belongsTo(Person)
+});
+Comment.toString = function() { return "Comment"; };
+
+module('Relationships with the REST adapter', {
+ setup: function() {
+ var Adapter, adapter;
+
+ requests = [];
+
+ Adapter = DS.RESTAdapter.extend();
+ Adapter.configure('plurals', {
+ person: 'people'
+ });
+
+ adapter = Adapter.create({
+ ajax: function(url, method, options) {
+ var success = options.success,
+ error = options.error;
+
+ options.url = url;
+ options.method = method;
+
+ if (success) {
+ options.success = function() {
+ success.apply(options.context, arguments);
+ };
+ }
+
+ if (error) {
+ options.error = function() {
+ error.apply(options.context, arguments);
+ };
+ }
+
+ requests.push(options);
+ }
+ });
+
+ store = DS.Store.create({
+ isDefaultStore: true,
+ adapter: adapter
+ });
+ }
+});
+
+function expectState(record, state, value) {
+ if (value === undefined) { value = true; }
+
+ var flag = "is" + state.charAt(0).toUpperCase() + state.substr(1);
+ equal(get(record, flag), value, "the " + record.constructor + " is " + (value === false ? "not " : "") + state);
+}
+
+function expectStates(records, state, value) {
+ records.forEach(function(record) {
+ expectState(record, state, value);
+ });
+}
+
+test("creating a parent and child in the same commit", function() {
+ var person, comment;
+
+ comment = store.createRecord(Comment);
+
+ person = store.createRecord(Person, { name: "John Doe" });
+ person.get('comments').pushObject(comment);
+
+ store.commit();
+
+ expectStates([person, comment], 'saving', true);
+
+ equal(requests.length, 1, "Only one request is attempted");
+ equal(requests[0].url, "/people", "The person is created first");
+
+ requests[0].success({
+ person: { id: 1, name: "John Doe", comments: [] },
+ comments: []
+ });
+
+ stop();
+ setTimeout(function() {
+ start();
+
+ expectState(person, 'saving', false);
+ expectState(comment, 'saving', true);
+ expectStates([person, comment], 'error', false);
+
+ deepEqual(person.get('comments').toArray(), [ comment ], "The person has the comment");
+ equal(comment.get('person'), person, "The comment belongs to the person");
+
+ equal(requests.length, 2, "A second request is attempted");
+ equal(requests[1].url, "/comments", "The comment is created second");
+ equal(requests[1].data.comment.person_id, 1, "The submitted comment attributes include the person_id");
+
+ requests[1].success({
+ comment: { id: 2, person_id: 1 }
+ });
+ });
+
+ stop();
+ setTimeout(function() {
+ start();
+
+ expectStates([person, comment], 'saving', false);
+ expectStates([person, comment], 'error', false);
+
+ deepEqual(person.get('comments').toArray(), [ comment ], "The person has the comment");
+ equal(comment.get('person'), person, "The comment belongs to the person");
+ });
+});
+
+test("creating a parent and updating a child in the same commit", function() {
+ var person, comment;
+
+ store.load(Comment, { id: 2 });
+ comment = store.find(Comment, 2);
+ comment.set('body', 'Lorem ipsum dolor sit amet.');
+
+ person = store.createRecord(Person, { name: "John Doe" });
+ person.get('comments').pushObject(comment);
+
+ store.commit();
+
+ expectStates([person, comment], 'saving', true);
+
+ equal(requests.length, 1, "Only one request is attempted");
+ equal(requests[0].url, "/people", "The person is created first");
+
+ requests[0].success({
+ person: { id: 1, name: "John Doe", comments: [] },
+ comments: []
+ });
+
+ stop();
+ setTimeout(function() {
+ start();
+
+ expectState(person, 'saving', false);
+ expectState(comment, 'saving', true);
+ expectStates([person, comment], 'error', false);
+
+ deepEqual(person.get('comments').toArray(), [ comment ], "The person has the comment");
+ equal(comment.get('person'), person, "The comment belongs to the person");
+
+ equal(requests.length, 2, "A second request is attempted");
+ equal(requests[1].url, "/comments/2", "The comment is updated second");
+ equal(requests[1].data.comment.person_id, 1, "The submitted comment attributes include the person_id");
+
+ requests[1].success();
+ });
+
+ stop();
+ setTimeout(function() {
+ start();
+
+ expectStates([person, comment], 'saving', false);
+ expectStates([person, comment], 'error', false);
+
+ deepEqual(person.get('comments').toArray(), [ comment ], "The person has the comment");
+ equal(comment.get('person'), person, "The comment belongs to the person");
+ });
+});
View
98 packages/ember-data/tests/unit/rest_adapter_test.js
@@ -1030,3 +1030,101 @@ test("updating a record with a 500 error marks the record as error", function()
expectState('error');
});
+
+test("delaying create when the parent is new", function() {
+ group = store.createRecord(Group, { name: "Engineers" });
+
+ person = store.createRecord(Person, { name: "John" });
+ person.set('group', group);
+
+ ok(!group.hasObserverFor('id'), "The group's ID is not yet being observed");
+
+ adapter.createRecord(store, Person, person);
+
+ ok(!ajaxHash, "The AJAX call has not yet been made");
+ ok(group.hasObserverFor('id'), "The group's ID is being observed");
+
+ var observer = group.observersForKey('id')[0],
+ target = observer[0],
+ callback = observer[1];
+
+ callback.call(target);
+
+ ok(ajaxHash, "The AJAX call has been made");
+ ok(!group.hasObserverFor('id'), "The group's ID is no longer being observed");
+});
+
+test("immediately creating when the parent is already loaded", function() {
+ store.load(Group, { id: 1, name: "Engineers" });
+ group = store.find(Group, 1);
+
+ person = store.createRecord(Person, { name: "John" });
+ person.set('group', group);
+
+ ok(!group.hasObserverFor('id'), "The group's ID is not being observed");
+
+ adapter.createRecord(store, Person, person);
+
+ ok(ajaxHash, "The AJAX call has been made");
+ ok(!group.hasObserverFor('id'), "The group's ID is not being observed");
+});
+
+test("immediately creating when there is no parent", function() {
+ person = store.createRecord(Person, { name: "John" });
+
+ adapter.createRecord(store, Person, person);
+
+ ok(ajaxHash, "The AJAX call has been made");
+});
+
+test("delaying update when the parent is new", function() {
+ group = store.createRecord(Group, { name: "Engineers" });
+
+ store.load(Person, { id: 1, name: "John" });
+ person = store.find(Person, 1);
+ person.set('name', 'Jonathan');
+ person.set('group', group);
+
+ ok(!group.hasObserverFor('id'), "The group's ID is not yet being observed");
+
+ adapter.updateRecord(store, Person, person);
+
+ ok(!ajaxHash, "The AJAX call has not yet been made");
+ ok(group.hasObserverFor('id'), "The group's ID is being observed");
+
+ var observer = group.observersForKey('id')[0],
+ target = observer[0],
+ callback = observer[1];
+
+ callback.call(target);
+
+ ok(ajaxHash, "The AJAX call has been made");
+ ok(!group.hasObserverFor('id'), "The group's ID is no longer being observed");
+});
+
+test("immediately updating when the parent is already loaded", function() {
+ store.load(Group, { id: 1, name: "Engineers" });
+ group = store.find(Group, 1);
+
+ store.load(Person, { id: 1, name: "John" });
+ person = store.find(Person, 1);
+ person.set('name', 'Jonathan');
+ person.set('group', group);
+
+ ok(!group.hasObserverFor('id'), "The group's ID is not being observed");
+
+ adapter.updateRecord(store, Person, person);
+
+ ok(ajaxHash, "The AJAX call has been made");
+ ok(!group.hasObserverFor('id'), "The group's ID is not being observed");
+});
+
+test("immediately updating when there is no parent", function() {
+ store.load(Person, { id: 1, name: "John" });
+ person = store.find(Person, 1);
+ person.set('name', 'Jonathan');
+
+ adapter.updateRecord(store, Person, person);
+
+ ok(ajaxHash, "The AJAX call has been made");
+});
Something went wrong with that request. Please try again.