Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

relation ships are now saved after the record itself #365

Merged
99 changes: 69 additions & 30 deletions addon/adapters/firebase.js
Original file line number Diff line number Diff line change
Expand Up @@ -418,42 +418,81 @@ export default DS.Adapter.extend(Waitable, {
});

return new Promise((resolve, reject) => {
var savedRelationships = Ember.A();


var relationshipsToSave = [];


// first we remove all relationships data from the serialized record, we backup the
// removed data so that we can save it at a later stage.
snapshot.record.eachRelationship((key, relationship) => {
var save;
if (relationship.kind === 'hasMany') {
if (serializedRecord[key]) {
save = this._saveHasManyRelationship(store, typeClass, relationship, serializedRecord[key], recordRef, recordCache);
savedRelationships.push(save);
// Remove the relationship from the serializedRecord because otherwise we would clobber the entire hasMany
delete serializedRecord[key];
}
} else {
if (this.isRelationshipEmbedded(store, typeClass.modelName, relationship) && serializedRecord[key]) {
save = this._saveEmbeddedBelongsToRecord(store, typeClass, relationship, serializedRecord[key], recordRef);
savedRelationships.push(save);
const data = serializedRecord[key];
const isEmbedded = this.isRelationshipEmbedded(store, typeClass.modelName, relationship);
const hasMany = relationship.kind === 'hasMany';
if ( hasMany || isEmbedded ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

remove the padding inside the (), to be consistent with the code style elsewhere

if (!Ember.isNone(data)) {
relationshipsToSave.push(
{
data:data,
relationship:relationship,
isEmbedded:isEmbedded,
hasMany:hasMany
}
);
}
delete serializedRecord[key];
}
}
});

var relationshipsPromise = Ember.RSVP.allSettled(savedRelationships);
);
var reportError = (errors) => {
var error = new Error(`Some errors were encountered while saving ${typeClass} ${snapshot.id}`);
error.errors = errors;
reject(error);
};
// now we update the record
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(
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

() => {
if (recordPromise.state === 'rejected') {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

// TODO: FIXME: Do we need to rollback ? I think we do.
reportError([recordPromise.reason]);
Copy link
Contributor

Choose a reason for hiding this comment

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

The error handling code can be simplified to:

this._updateRecord()
  .then(() => { /*... */ })
  .catch(reportError);

} else {
// and now we construct the list of promise to save relationships.
var savedRelationships = Ember.A();
relationshipsToSave.forEach(
Copy link
Contributor

Choose a reason for hiding this comment

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

this could be simplified with a map:

var savedRelationships = relationshipsToSave.map((rel) => {})

(relationshipToSave) => {
const data = relationshipToSave.data;
const relationship = relationshipToSave.relationship;
if (relationshipToSave.hasMany) {
savedRelationships.push(
this._saveHasManyRelationship(store, typeClass, relationship, data, recordRef, recordCache)
);
} else {
// embedded belongsTo, we need to fill in the informations.
if (relationshipToSave.isEmbedded) {
savedRelationships.push(
this._saveEmbeddedBelongsToRecord(store, typeClass, relationship, data, recordRef)
);
}
}
}
);
// and wait for them to be all settled so that we can see if we had an error,
// TODO: FIXME: Do we need to rollback ? I think we do.
var relationshipsPromise = Ember.RSVP.allSettled(savedRelationships);
relationshipsPromise.then(
() => {
var rejected = Ember.A(relationshipsPromise.value).filterBy('state', 'rejected');
if (rejected.length !== 0) {
reportError(Ember.A(rejected).mapBy('reason'));
} else {
resolve();
}
}
);
}
}
});
);
}, `DS: FirebaseAdapter#updateRecord ${typeClass} to ${recordRef.toString()}`);
},

Expand Down