Skip to content
This repository

DS.RESTAdapter: Robust Support for Parent->Child Hierarchies #724

Closed
wants to merge 1 commit into from
Gordon L. Hempton
Collaborator

We had the need in our app to save transactions in parent->child order to ensure foreign keys are present. This PR uses nested promises to resolve dependencies.

Bradley Priest
Collaborator

I'm just upgrading from version4 to version11 (yes I know it's a huge jump), does this mean the functionality to save a new object and have it's children save once the parent is returned has not been reimplemented yet?
Is there anything I can help with here?

Gordon L. Hempton
Collaborator

@bradleypriest afaik it has not. I could definitely use some help with the tests.

thingista

Gordon,

This is amazing stuff. Thank you so much for this.
I am trying to put together a simple Rails+Ember example app showing how to use this Relational adapter (hopefully it will help others coming from the same background as me).

Just one thing though: I am struggling with "delete" on a child. When {embedded : 'always'} is set, if you do:

comment.deleteRecord(); // Reusing your test example, with Post and Comments

the next time you commit the store, it only PUTs the whole post. The content of the PUT is correct (the deleted comment has been removed from the post) but the server will not now about the fact the comment was deleted.

Am I using this in a wrong way?

Thanks a lot
PJ

thingista

Gordon,

Testing further, it seems there is a bug when you add grand-children. I think it is here:

function createNestedPromise(node) {
  var promise = node.promise(store, adapter);
  if(node.children.length > 0) { // Culprit here: promise might be nil if !adapter.shouldSave(node.record), i.e. (I think?) if an ancestor of node.record has been marked for saving. It might happen if you have more than two "layers" of associations
    promise = promise.pipe(function() {
      var childPromises = Ember.A(node.children.map(createNestedPromise)).compact();
      return jQuery.when.apply(jQuery, childPromises);
    });
  }
  return promise;
}

Replacing by

if((node.children.length > 0) && (promise))

seems to work (sorry I did not manage to run your test suite, only my own ones).

PJ

Gordon L. Hempton
Collaborator

@thingista deleting an embedded child needs to be handled by your backend implementation. We inspect the params that are sent to the server and delete children that aren't present. We made a generic rails dsl that allows us to do this easily that we might open source eventually.

Gordon L. Hempton
Collaborator

@thingista also, if you could produce a test case for the bug you are hitting that would be a big help!

thingista

@ghempton I sent you a pull request with a test. I hope it is fine.

Thanks
PJ

Gordon L. Hempton
Collaborator

@thingista thanks! Merged it in and added a slightly different fix from the one your provided.

Bradley Priest
Collaborator

I'm running into an issue where an object that belongs to multiple hasMany associations where one parent is new and another is existing where the child is being set as both a rootNode and a child node and is thus getting sent to the server twice (the first time without the parent_id). I'll try and put together a failing test soon

Gordon L. Hempton
Collaborator

@bradleypriest ah I could see that happening. Shouldn't be too hard to fix once there is a test.

Bradley Priest
Collaborator

@ghempton I can't seem to get it to fail in QUnit just yet, I have found what I think is another bug though.
ghempton#2

Gordon L. Hempton
Collaborator

@bradleypriest Thanks for the test. Merged it in and pulled some logic in from #440 to fix the problem. :beer:

boy-jer

@ghempton, thamks for working on this. Do you mind giving a short clue on how you inspect the params that are sent to the server and delete children that aren't present.

I am thinking of something like this in the rails controller, let's say the child is :book

#extract the child params (books) sent by emberjs from the parent params

   parent_params.extract!(:books) 

#store the extracted params in a variable

  book_params =  book_params[:books]

#iterate through the books_params variable and delete the child :id's not included in it

  books_params.each do |pn| 
      Book.destroy unless pn.include?(:id)
 end

Do you think there is a better way. thanks.

Gordon L. Hempton
Collaborator

@boy-jer check out this gem we released which does exactly what you suggested: https://github.com/GroupTalent/embedded_associations

boy-jer

@ghempton many thanks for releasing that gem. It will be helpful in my work.

tobiaswerner

Aloha everybody, please see following setup.

App.Parent = DS.Model.extend({
oneChild: DS.belongsTo("App.OneChild"),
anotherChild: DS.belongsTo("App.AnotherChild")
});
App.OneChild = DS.Model.extend({
name: DS.attr("string")
});
App.AnotherChild = DS.Model.extend({
name: DS.attr("string")
});

Then I create the record in the routes model hook.

App.ParentNewRoute = Ember.Route.extend({
model: function() {
return App.Parent.createRecord({
    oneChild: App.OneChild.createRecord({}),
    anotherChild: App.AnotherChild.createRecord({})
  });
}
});

oneChild and anotherChild would be saved correctly via two POST to the server. But after that two POST to /parents would be made. Am I doing something wrong in setting the relationship the correct way or is this just a little bug? Unfortunately I didn't get my head wrapped around this so I'm not able to provide a pull request...
Thanks in advance for all advices...

Gordon L. Hempton
Collaborator

@tobiaswerner that looks like a bug. Could you do a favor and create a test case for this in hierarchies.js? This shouldn't be too hard to fix.

tobiaswerner

@ghempton Will try tomorrow. I'm really new to all this stuff ;)

Gordon L. Hempton
Collaborator

@tobiaswerner actually on second thought, I think there might need to be an inverse relationship on the child. Can you try adding additional DS.belongsTo's to the children?

tobiaswerner

@ghempton Thanks for the support but this was sadly not the solution...
Now I got following error after the second POST of Parent:
Error: Attempted to handle event didCommit on App.Parent:ember382:null while in state rootState.loaded.saved. Called with undefined

Bouke Haarsma
Bouke commented March 08, 2013

@ghempton your PR is against an old master, any chance you get it rebased on the updated master? I tried, but it seems that createRecord does not return with a promise anymore, which fails when trying to pipe on that (undefined) promise.
Edit: this was due to a custom adapter, which didn't return the jQuery promises. However, now I'm also running into the same issue as @tobiaswerner. This only happens when I have two or more children (of different types).

Uncaught Error: Attempted to handle event `didCommit` on <App.Account:ember371:1> while in state rootState.loaded.saved. Called with undefined ember-data.js:3499
DS.StateManager.Ember.StateManager.extend.unhandledEvent ember-data.js:3499
superWrapper ember-1.0.0-rc.1.js:884
sendRecursively ember-1.0.0-rc.1.js:26024
sendRecursively ember-1.0.0-rc.1.js:26032
sendRecursively ember-1.0.0-rc.1.js:26032
sendRecursively ember-1.0.0-rc.1.js:26032
sendEvent ember-1.0.0-rc.1.js:26041
sendRecursively ember-1.0.0-rc.1.js:26034
sendRecursively ember-1.0.0-rc.1.js:26032
sendRecursively ember-1.0.0-rc.1.js:26032
sendRecursively ember-1.0.0-rc.1.js:26032
sendEvent ember-1.0.0-rc.1.js:26041
Ember.StateManager.Ember.State.extend.send ember-1.0.0-rc.1.js:26564
DS.Model.Ember.Object.extend.send ember-data.js:3594
DS.Model.Ember.Object.extend.adapterDidCommit ember-data.js:3668
DS.Store.Ember.Object.extend.didSaveRecord ember-data.js:1840
DS.Adapter.Ember.Object.extend.didSaveRecord ember-data.js:6773
DS.Adapter.Ember.Object.extend.didUpdateRecord ember-data.js:6806
(anonymous function) adapter.js:44
(anonymous function) ember-1.0.0-rc.1.js:4064
Ember.handleErrors ember-1.0.0-rc.1.js:401
invoke ember-1.0.0-rc.1.js:4062
tryable ember-1.0.0-rc.1.js:4252
Ember.tryFinally ember-1.0.0-rc.1.js:1039
Ember.run ember-1.0.0-rc.1.js:4256
ajax.success adapter.js:43
l jquery.min.js:2
c.fireWith jquery.min.js:2
T jquery.min.js:2
r jquery.min.js:2
thingista thingista referenced this pull request in getoutreach/data March 22, 2013
Open

Failing many-to-many test #1

thingista

@Bouke @tobiaswerner Just a quick note: I was running into similar issues as you guys and spent hours in the ember-data code trying to figure out what was wrong (sometimes these error messages are a bit cryptic). It turned out there was a binding somewhere in my code that was triggering during the "save" process, and altering objects that were being saved. Once I removed this binding everything was fine.

Gordon L. Hempton
Collaborator

@igorT @tomdale I would like to put more work into this PR if you think it is a candidate to be merged. Thoughts?

Tom Dale
Owner

This is definitely an issue that we need to resolve ASAP. If it's possible, I'd like to solve the issue with less code than this spike requires—but we definitely need to solve it.

Gordon L. Hempton
Collaborator

I'm not sure what a more concise implementation would look like. Could definitely be trimmed down if we removed bulk support, but that works so why? I think @stefanpenner's work on rsvp will create some niceties here.

Stefan Penner
Owner

@tomdale agreed we want this soon. I am planning another push on the promise stuff this weekend, to continue where I left off wednesday evening. Once this work is in place @ghempton I are going to push for this asap.

packages/ember-data/tests/integration/hierarchies_test.js
((153 lines not shown))
  153
+  return result;
  154
+}
  155
+
  156
+asyncTest("creating parent->child hierarchy", function () {
  157
+  var post = store.createRecord(Post, {title: 'Who needs ACID??'});
  158
+  var comment = get(post, 'comments').createRecord({body: 'not me'});
  159
+
  160
+  ajaxResults = {
  161
+    'POST:/comments': function() { return dataForCreate(comment); },
  162
+    'POST:/posts': function() { return dataForCreate(post); }
  163
+  };
  164
+
  165
+  store.commit();
  166
+  equal(get(post, 'comments.firstObject'), comment, "post's comments should include comment");
  167
+
  168
+  waitForPromises(function() {
1
Stefan Penner Owner
stefanpenner added a note May 01, 2013

rather then these "waitForPromises" should we be using the thenable returned by store.commit ?

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

@ghempton do you have some sample code, where you createRecord() on a simple parent-hasMany-children (and child-belongsTo-parent) relationship? I tried this PR, it does send requests in the expected order (children after parent), but when I examine the data being sent, the parent_id field (in all children) in the post /children request is still 0. The POST /parent is returning a parent with the correct ID, though, just that this "new" ID is not updated into the child request.

heartsentwined

@ghempton never mind, I didn't realize I was on the wrong branch all along. Today I checked out relational-adapter, and it's working as expected. Just ignore my post above.

Ray Tiley

I would really love to try this out, but I can't get this PR to apply to master. Does anyone have a current build with this applied?

Thanks!

Ray Tiley

@ghempton I played around with this yesterday. It looks really promising. I did run into one issue. I'm trying to add a test that recreates it but I thought I would see if you had any initial ideas. My use case is like this:

App.Playlist = DS.Model.extend({
  clips: DS.hasMany('App.Clip'),
});

App.Clip = DS.Model.extend({
  playlist: DS.belongsTo('App.Playlist'),
  media: DS.belongsTo('App.Media'),
});

App.Media = DS.Model.extend({
});

Its essentially a one direction hasMany with a joining relationship. If I create a new Playlist with associated clips and media and call commit() on the store the Clip model only has one of its ids set (the playlist_id). If I pull the Playlist out into its own transaction and then commit that and then commit the defaultTransaction everything seems to work ok.

I'll try to reproduce this in a test over the weekend.

Thanks for the work!

Stefan Penner
Owner

@raytiley a failing test case, and a fix (if possible) would be great. I hope to get this PR in as time permits.

Ray Tiley

@stefanpenner @ghempton I just opened a pull request against ghemptons repo.

ghempton#4

The test fails by timing out, and I haven't yet figured out why. I figured I would throw it up just to see if someone else's eyes catch the problem quicker than my own. I'll keep investigating as best I can, but this is pretty deep for me, and I'm still getting used to the code.

Thanks

Ray Tiley

@stefanpenner @ghempton So I found the issue and just pushed a fix that doesn't really fix the underlying problem. I'll try to explain it here.

Lets say you have nodes A B and C. A and B are both parents to C. So in order to persist C properly you need IDs from both A and B.

The code in this pull request currently finds all the parent nodes and creates promise chains for them. In our example you would get two promise chains A -> C and B -> C. So in this case C gets persisted twice.

The code I just pushed ensures that no node ever gets more than one promise. So you'll likely get promise chains that look like A -> C and B. The problem here is that C isn't dependent on B anymore. Its a race condition that most times will end up in a failing ajax call.

What we really want is a promise chain that looks like (A & B) -> C.

Still trying to figure out the best way to do this. Hopefully someone smarter than me comes along :)

heartsentwined heartsentwined referenced this pull request in heartsentwined/ember-auth-rails-demo June 11, 2013
Closed

The app is not remembering me #3

pivotal-medici

Any idea when this would be ready to merge? This is a critical feature for most apps and this branch is now 6 weeks out of date.

-- AK/AB

scalability

ping. Ember-data really needs better parent-child relationship support. Where we at?

Gordon L. Hempton
Collaborator

I'm working on something big that will solve this and more... will post more info soon

Alex Kwiatkowski
rupurt commented June 18, 2013

Awesome. Looking forward to trying it out on our @pivotal-medici project. Do you have a list of todo's before it can be merged?

Shane Ardell

@ghempton Definitely in need of this feature, looking forward to using it. Any updates?

Gordon L. Hempton
Collaborator

I am tempted to close this PR. I have moved off of Ember Data and am now using Epf (which has the functionality of this PR and more). That is unless someone like @stefanpenner wants to take it over?

Michael Baudino

Is Ember-data going to be abandonned ? If not I guess we should leave this open, it's still a feature request, isn't it ?

Rob Monie

I just had a bit of a go at fixing this and would appreciate some feedback on my approach from anyone familiar with the issue. This solves the problem in my app for a single use case. I haven't tested it beyond that yet which is why i'm posting the solution as a gist. If anyone thinks that my approach is valid, i'll continue with this to support bulk commits and properly unit test it before creating a pull request.

The approach is based on observing the ids of records that have 'belongsTo' relationships that they depend on. Any records that depend on other records that don't have ids yet assigned apply an observer and defer their saving until the other record ids are resolved. It's quite likely i've missed various edge cases in this so i'd welcome any scenarios that people think this might not work under. If my approach is way off track, i'd prefer to find out earlier if possible.

https://gist.github.com/robmonie/6052753

EDIT: I've submitted a pull request with a test around some of the work done so far here - #1092

Yehuda Katz
Owner
wycats commented July 24, 2013

I am in favor of this PR. If somebody wants to bring it up to date, I'll happily pull it in.

No, Ember Data is not abandoned.

Trevor tjohn referenced this pull request from a commit April 19, 2013
Paul Chavard use a transaction to save records abb3bc2
Alex Kwiatkowski
rupurt commented July 25, 2013

@tjohn and I have successfully brought this up to master. We've push it to our branch.

We have removed the run loop around save as indicated in this comment thread. What should we do with that?

Do you want us to submit a separate PR for this? It appears @ghempton has abandoned the patch because EPF meets his needs

Yehuda Katz
Owner
wycats commented July 25, 2013

Can you submit an up-to-date PR? Should I close this PR?

Peter Wagenet
Owner

Superseded by #1095.

Peter Wagenet wagenet closed this August 10, 2013
Diego Muñoz Escalante escalant3 referenced this pull request in escalant3/ember-data-tastypie-adapter February 19, 2014
Closed

Add support for saving graphs of objects #10

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

Showing 1 unique commit by 1 author.

May 02, 2013
Gordon L. Hempton DS.RESTAdapter support for parent-child hierarchies 45a9079
Something went wrong with that request. Please try again.