relation ships are now saved after the record itself #365

olivierchatry · 2016-02-10T09:48:21Z

solve #327, save record and then save relationships, which allows for rules to be applied ( generally rules are using record data, not relationship data).

…to be applied while creating records

googlebot · 2016-02-10T09:48:23Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please let us know the company's name.

olivierchatry · 2016-02-10T09:49:25Z

I signed it!

googlebot · 2016-02-10T09:54:59Z

CLAs look good, thanks!

not copy pasting code is always a good idea.

olivierchatry · 2016-02-11T05:01:51Z

Seems like the last commit did not trigger a rebuild :(, @tstirrat any chance you can help or should I just push a dummy commit ?

I also found why I needed to reload some records after saving it happens when you create a record with relationship belongsTo -> hasMany relationship, and save the belongsTo ( parent ) first. Ember data has some bad code that check if the relationship element is "new" and is so push it again in the "hasMany" relationship. I have filled a bug at ember data, but you can fix it by saving the child before the parent ( in some case it it is not possible ).

tstirrat · 2016-02-11T19:03:33Z

for reference: emberjs/data#4149

tstirrat · 2016-02-11T19:04:39Z

I tried to kick off the build again but it runs on the second to last commit, sorry

olivierchatry · 2016-02-11T19:04:57Z

Submitted à pullrequest to ember data for the fix as well as test for repro & proof of fix

olivierchatry · 2016-02-11T19:05:52Z

Then I'm going to submit a dummy

tstirrat · 2016-02-11T19:09:17Z

addon/adapters/firebase.js

-          reject(error);
-        } else {
-          resolve();
+      recordPromise.then(


you can combine this with the prior line this._updateRecord().then()

I dont think I can, since recordPromise is used later in the callback

On Thu, Feb 11, 2016 at 8:09 PM, Tim Stirrat [email protected]
wrote:

In addon/adapters/firebase.js
#365 (comment):

var recordPromise = this._updateRecord(recordRef, serializedRecord);

Ember.RSVP.hashSettled({relationships: relationshipsPromise, record: recordPromise}).then((promises) => {

var rejected = Ember.A(promises.relationships.value).filterBy('state', 'rejected');

if (promises.record.state === 'rejected') {

rejected.push(promises.record);

}

// Throw an error if any of the relationships failed to save

if (rejected.length !== 0) {

var error = new Error(`Some errors were encountered while saving ${typeClass} ${snapshot.id}`);

error.errors = Ember.A(rejected).mapBy('reason');

reject(error);

} else {

resolve();

recordPromise.then(

you can combine this with the prior line this._updateRecord().then()

—
Reply to this email directly or view it on GitHub
https://github.com/firebase/emberfire/pull/365/files#r52650315.

recordPromise.state should not be rejected after the promise resolved, it would be inside the rejected (.catch())

Also, we would not need to rollback inside the then block. The good thing about your new approach, due to promise chaining, is that when Firebase returns an error, we will not attempt to save the relationships.

…of foreach

olivierchatry · 2016-02-11T19:20:54Z

@tstirrat any more comment ?

tstirrat · 2016-02-11T19:20:57Z

addon/adapters/firebase.js

-          resolve();
+      recordPromise.then(
+        () => {
+          if (recordPromise.state === 'rejected') {


this check should not be here, it should be in the .catch() handler

Ok, but what about if the attributes are saving OK, but the relations are
failing ?

On Thu, Feb 11, 2016 at 8:20 PM, Tim Stirrat [email protected]
wrote:

In addon/adapters/firebase.js
#365 (comment):

Ember.RSVP.hashSettled({relationships: relationshipsPromise, record: recordPromise}).then((promises) => {

var rejected = Ember.A(promises.relationships.value).filterBy('state', 'rejected');

if (promises.record.state === 'rejected') {

rejected.push(promises.record);

}

// Throw an error if any of the relationships failed to save

if (rejected.length !== 0) {

var error = new Error(`Some errors were encountered while saving ${typeClass} ${snapshot.id}`);

error.errors = Ember.A(rejected).mapBy('reason');

reject(error);

} else {

resolve();

recordPromise.then(

() => {

if (recordPromise.state === 'rejected') {

this check should not be here, it should be in the .catch() handler

—
Reply to this email directly or view it on GitHub
https://github.com/firebase/emberfire/pull/365/files#r52651996.

this would probably be related to security rules. its a good question.

If we want to support this, we'd need a backup of previous attributes. This could be grabbed from ember-data.

Done, test are passing, but I did not try yet on my application ( waiting
for it to build ).

On Thu, Feb 11, 2016 at 8:28 PM, Tim Stirrat [email protected]
wrote:

In addon/adapters/firebase.js
#365 (comment):

Ember.RSVP.hashSettled({relationships: relationshipsPromise, record: recordPromise}).then((promises) => {

var rejected = Ember.A(promises.relationships.value).filterBy('state', 'rejected');

if (promises.record.state === 'rejected') {

rejected.push(promises.record);

}

// Throw an error if any of the relationships failed to save

if (rejected.length !== 0) {

var error = new Error(`Some errors were encountered while saving ${typeClass} ${snapshot.id}`);

error.errors = Ember.A(rejected).mapBy('reason');

reject(error);

} else {

resolve();

recordPromise.then(

() => {

if (recordPromise.state === 'rejected') {

this would probably be related to security rules. its a good question.

If we want to support this, we'd need a backup of previous attributes.
This could be grabbed from ember-data.

—
Reply to this email directly or view it on GitHub
https://github.com/firebase/emberfire/pull/365/files#r52653256.

tstirrat · 2016-02-11T19:35:10Z

Thanks for this PR, btw. I appreciate all the help I can get!

One final nitpick: arrow functions blocks should be on the same line:

method.then(() => {
  // ...
});

olivierchatry · 2016-02-11T19:37:18Z

no problem, I just dont like to wait for things to happen :). If my PR is validated for ember-data, that would be cool for emberfire as well ( and probably other adapter/serializer ).

tstirrat · 2016-02-11T19:43:29Z

addon/adapters/firebase.js

        }
-      });
+      ).then(
+        (results) => {


still a lambda here on new line

tstirrat · 2016-02-11T19:47:22Z

so. we could reduce the number of saves to exactly 2 by using a multi-path update for the relationships

writing something like this:

rootReference.child('posts').child(postId).update({
  // ignore unchanged comment links
  'comments/comment_5': null, // deleted
  'comments/comment_6': true, // added
  'comments/comment_7': true, // added
});

olivierchatry · 2016-02-11T19:50:04Z

yes I think it should be easily feasible, the code is pretty much already
there. that would also greatly reduce the risk of errors

On Thu, Feb 11, 2016 at 8:47 PM, Tim Stirrat [email protected]
wrote:

so. we could reduce the number of saves to exactly 2 by using a multi-path
update for the relationships

writing something like this:

rootReference.child('posts').child(postId).update({
// ignore unchanged comment links
'comments/comment_5': null, // deleted
'comments/comment_6': true, // added
'comments/comment_7': true, // added
});

—
Reply to this email directly or view it on GitHub
#365 (comment).

tstirrat · 2016-02-11T19:53:19Z

Actually, we could probably reduce it to 1, with an update like this:

rootReference.child('posts').update({
  // model attributes:
  'post_1/title': 'Title',
  'post_1/body': 'Body here',

  // relationships:
  // ignore unchanged comment links
  'post_1/comments/comment_5': null, // deleted
  'post_1/comments/comment_6': true, // added
  'post_1/comments/comment_7': true, // added
});

olivierchatry · 2016-02-11T19:54:41Z

yep that make sense ( trying to figure out if something is wrong with either my app or the change I commited )

tstirrat · 2016-02-11T19:55:17Z

if you find out its an edge case, or a slight bug.. please write a test that exposes it

olivierchatry · 2016-02-11T19:56:05Z

it was working before the change I have made ( catch, etc. ) so I'm
guessing something is not working as it should.

On Thu, Feb 11, 2016 at 8:55 PM, Tim Stirrat [email protected]
wrote:

if you find out its an edge case, or a slight bug.. please write a test
that exposes it

—
Reply to this email directly or view it on GitHub
#365 (comment).

tstirrat · 2016-02-11T20:04:30Z

The more I think about this, the more I wonder if we can take the other atomic save branch, take the serialized json hash, convert it to a multi-path update with only changed attributes.. and we solve both problems.

e.g.

if the serializer returns only changed attributes:

{
  title: 'New title',
  comments: {
    comment_1: null, // deleted
    comment_5: true, // added
  }
}

we can convert this to a multi-path update:

{
  'post_1/title': 'New title',
  'post_1/comment_1': null, // deleted
  'post_1/comment_5': true, // added
}

olivierchatry · 2016-02-11T20:05:35Z

Well that would definitly solve the problem :) can you try ?

On Thu, Feb 11, 2016 at 9:04 PM, Tim Stirrat [email protected]
wrote:

The more I think about this, the more I wonder if we can take the other
atomic save branch, take the serialized json hash, convert it to a
multi-path update with only changed attributes.. and we solve both problems.

e.g.

if the serializer returns only changed attributes:

{
title: 'New title',
comments: {
comment_1: null, // deleted
comment_5: true, // added
}
}

we can convert this to a multi-path update:

{
'post_1/title': 'New title',
'post_1/comment_1': null, // deleted
'post_1/comment_5': true, // added
}

—
Reply to this email directly or view it on GitHub
#365 (comment).

tstirrat · 2016-02-11T20:05:50Z

However this will cause problems with validation rules where say, "post must have a body".

So, an adapted version of this is that we could save all model attributes, and only the changed relationships..

olivierchatry · 2016-02-11T20:06:29Z

yes, attributes are fine to overrides normally

On Thu, Feb 11, 2016 at 9:05 PM, Tim Stirrat [email protected]
wrote:

However this will cause problems with validation rules where say, "post
must have a body".

So, an adapted version of this is that we could save all model attributes,
and only the changed relationships..

—
Reply to this email directly or view it on GitHub
#365 (comment).

tstirrat · 2016-02-11T20:10:57Z

I'll get better time to work on this over the weekend. Sadly wont get a chance today/tomorrow.

olivierchatry · 2016-02-11T20:11:40Z

I can try to do it then, as I think it would be a very cool way of doing :)

On Thu, Feb 11, 2016 at 9:11 PM, Tim Stirrat [email protected]
wrote:

I'll get better time to work on this over the weekend. Sadly wont get a
chance today/tomorrow.

—
Reply to this email directly or view it on GitHub
#365 (comment).

olivierchatry · 2016-02-11T21:54:05Z

Got a first version that works, but does not handle embedded record, going
to submit inside my fork, but your branch

On Thu, Feb 11, 2016 at 9:11 PM, Olivier Chatry [email protected]
wrote:

I can try to do it then, as I think it would be a very cool way of doing :)

On Thu, Feb 11, 2016 at 9:11 PM, Tim Stirrat [email protected]
wrote:

I'll get better time to work on this over the weekend. Sadly wont get a
chance today/tomorrow.

—
Reply to this email directly or view it on GitHub
#365 (comment).

olivierchatry · 2016-02-11T22:22:50Z

For some odd reason, I cannot update your branch to my fork without getting
conflict ( weird as I did not do anything on it ), I'm always 33 commit
behinds :(. Any chance you would know anything about that ?

On Thu, Feb 11, 2016 at 10:54 PM, Olivier Chatry [email protected]
wrote:

Got a first version that works, but does not handle embedded record, going
to submit inside my fork, but your branch

On Thu, Feb 11, 2016 at 9:11 PM, Olivier Chatry [email protected]
wrote:

I can try to do it then, as I think it would be a very cool way of doing
:)

On Thu, Feb 11, 2016 at 9:11 PM, Tim Stirrat [email protected]
wrote:

I'll get better time to work on this over the weekend. Sadly wont get a
chance today/tomorrow.

—
Reply to this email directly or view it on GitHub
#365 (comment).

tstirrat · 2016-02-11T22:24:51Z

ah, sorry. I rebased it, so your history and the remote one are out of sync.

delete the branch locally and then check it out again

git checkout master
git branch -D serialize-relationships-atomic
git checkout serialize-relationships-atomic

olivierchatry · 2016-02-12T08:33:07Z

Nop, not working, I always endup with conflict one way or the other. I will
solve the conflict by taking what is last on the branch I guess.

On Thu, Feb 11, 2016 at 11:24 PM, Tim Stirrat [email protected]
wrote:

ah, sorry. I rebased it, so your history and the remote one are out of
sync.

delete the branch locally and then check it out again

git checkout master
git branch -D serialize-relationships-atomic
git checkout serialize-relationships-atomic

—
Reply to this email directly or view it on GitHub
#365 (comment).

olivierchatry · 2016-02-12T17:05:44Z

I think, if you think this branch is OK ( it looks and works OK at least in my software ), we should merge it and release a version before the multipath update is working.

olivierchatry · 2016-02-14T10:59:27Z

squashed everything together.

tstirrat · 2016-02-16T22:38:43Z

addon/adapters/firebase.js

-          rejected.push(promises.record);
-        }
-        // Throw an error if any of the relationships failed to save
+      recordPromise.then(() => {


We don't need recordPromise separated into a named variable anymore. We can just use this._updateRecord().then() now.

Otherwise, we're good to go on this PR.

olivierchatry · 2016-02-17T08:08:10Z

done :) if you have time, take a look at #366 :)

relation ships are now saved after the record itself

relation ships are now saved after the record itself, allowing rules …

c65e425

…to be applied while creating records

fixes missing semicolon

36d4c4a

Olivier Chatry added 4 commits February 10, 2016 11:41

fixes typo while checking for embedded record

2c9e437

fix handling of belongsTo embedded / hasMany

c143565

removed debug log in test

fef30b7

a bit better error handling

bcfa16b

not copy pasting code is always a good idea.

tstirrat mentioned this pull request Feb 11, 2016

Embedded hasMany not saving #354

Open

tstirrat reviewed Feb 11, 2016
View reviewed changes

removed not very useful comment, code style consistency, map instead …

cfd00fa

…of foreach

tstirrat reviewed Feb 11, 2016
View reviewed changes

using catch to find error instead of record.state

7483f1a

code consistency, lambda on same line as caller

65858a3

tstirrat reviewed Feb 11, 2016
View reviewed changes

addon/adapters/firebase.js

}

});

).then(

(results) => {

Copy link

Contributor

tstirrat Feb 11, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still a lambda here on new line

more code consistency

aa8dc67

fixes FirebaseExtended#327, relationships are now saved last.

cc8f9e6

olivierchatry force-pushed the relationships-saved-last branch from aa8dc67 to cc8f9e6 Compare February 14, 2016 10:58

tstirrat reviewed Feb 16, 2016
View reviewed changes

olivierchatry added 2 commits February 17, 2016 09:02

merging squashing

3a98acc

more code convention fixes

4d448a6

tstirrat added a commit that referenced this pull request Feb 17, 2016

Merge pull request #365 from olivierchatry/relationships-saved-last

74cd3b3

relation ships are now saved after the record itself

tstirrat merged commit 74cd3b3 into FirebaseExtended:master Feb 17, 2016

relation ships are now saved after the record itself #365

relation ships are now saved after the record itself #365

Conversation

olivierchatry commented Feb 10, 2016

googlebot commented Feb 10, 2016

olivierchatry commented Feb 10, 2016

googlebot commented Feb 10, 2016

olivierchatry commented Feb 11, 2016

tstirrat commented Feb 11, 2016

tstirrat commented Feb 11, 2016

olivierchatry commented Feb 11, 2016

olivierchatry commented Feb 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olivierchatry commented Feb 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tstirrat commented Feb 11, 2016

olivierchatry commented Feb 11, 2016

Choose a reason for hiding this comment

tstirrat commented Feb 11, 2016

olivierchatry commented Feb 11, 2016

tstirrat commented Feb 11, 2016

olivierchatry commented Feb 11, 2016

tstirrat commented Feb 11, 2016

olivierchatry commented Feb 11, 2016

tstirrat commented Feb 11, 2016

olivierchatry commented Feb 11, 2016

tstirrat commented Feb 11, 2016

olivierchatry commented Feb 11, 2016

tstirrat commented Feb 11, 2016

olivierchatry commented Feb 11, 2016

olivierchatry commented Feb 11, 2016

olivierchatry commented Feb 11, 2016

tstirrat commented Feb 11, 2016

olivierchatry commented Feb 12, 2016

olivierchatry commented Feb 12, 2016

olivierchatry commented Feb 14, 2016

Choose a reason for hiding this comment

olivierchatry commented Feb 17, 2016