Save documents

[yamila-fraiman]

Is it possible to support saving existing documents. For example

db.User.findById('5997a409df4cc84e8ca3a8de').exec((err, user) => {
    lodash.assign(user, req.body);
    let transaction = Fawn.Task();
    transaction.save(user);
    transaction.update(....)
    transaction.run({useMongoose: true})
});

Actually if I try this code, I receive duplicated id because creates a new collection with same id

[e-oj]

The operation you described is an update so you should be using the update function

db.User.findById('5997a409df4cc84e8ca3a8de').exec((err, user) => {
    lodash.assign(user, req.body);
    let transaction = Fawn.Task();
    
    // update existing user
    transaction.update({_id: '5997a409df4cc84e8ca3a8de'}, user);
    transaction.update(....)
    transaction.run({useMongoose: true})
});

[yamila-fraiman]

Thank you for your answer. I know that it's an update but updates don't call pre save and if I use pre update I don't have access to the object so I'd like this feature

[e-oj]

I see your point. I'll tag this as an enhancement for the next release. Thanks for the suggestion.

[yamila-fraiman]

I think theres is a simple solution:

In goose.js inside function performSave, where you set doc._id, replace line by:

  if (doc._id) {
    doc.isNew = false;
  } else {
    doc._id = utils.generateId();
  }

It won't allow creating new documents by calling task.save(new Collection(data)). In order to solve that, maybe you can save a var isNew in the step which value is extracted from the collection parameter (step.options.isNew = collection.isNew;).

@e-oj do you think you can implement this feature?

[e-oj]

That's actually the plan but I'm implementing it as an extension of the update function to avoid confusion on how to rollback a save

[e-oj]

You'll be able to do it this way

db.User.findById('5997a409df4cc84e8ca3a8de').exec((err, user) => {
    let _user = lodash.assign({}, user, req.body);
    let transaction = Fawn.Task();
    
    // update existing user
    transaction.update(user, _user);
        .options({replace: true})
        .update(....)
        .run({useMongoose: true})
});

Will this work for you? @yamifr07

[yamila-fraiman]

No, because I use pre save. And I think it's a good idea to support saving existing documents. such as when I call collection.save in mongoose. It's not working with fawn because you serialize the collection and then create it without asking if is a new collection. So I think you need to save 'isNew' value before serializing and then set it to the Collection when restoring

[e-oj]

I know what you mean. this will be the underlying implementation for {replace: true}

if(step.options.replace){
  var doc = new Collection(data);
  doc.isNew = false;
  resultPromise = doc.save();
}
else{
  resultPromise = Collection.update(condition, data, step.options).exec();
}

return resultPromise.then(function (result) {
  results.push(result);

  return updateState(task, step.index, DONE, results);
});

[e-oj]

Is that different from what you want? @yamifr07

[yamila-fraiman]

mmm I think it's not a good idea to mix methods. I think that task.update should call update and task.save should call save. Otherwise users can have bugs without knowing.

[e-oj]

I don't think it's mixing methods because it's still an update; it's just an update implemented with a save function. There should be no confusion too, the replace option will be well documented. Also, It's for rollback convenience. Saves are rolled back by deleting; in this case though, what you're actually doing is an update. You don't want the whole updated document deleted during a rollback.

[yamila-fraiman]

Yes but update call update hooks and save call save hooks. And mongoose allows save and update to update elements. Why you don't want to use same methods as mongoose?

[e-oj]

I get your point. For convenience, and to reduce the learning curve, the functions in Fawn were modelled similarly to mongoose. However, they are two libraries with different goals. If a desired feature in fawn can be implemented in a way that deviates from the way mongoose is designed but makes development and usage of the library more intuitive, I have to go with that. As I said previously, saves are rolled back by deleting; in this case though, what you're actually doing is an update. You don't want the whole updated document deleted during a rollback. Check out the rollback code for saves

function rollbackSave(db, save, task) {
  var collection = db.collection(save.name);
  var _id = save.dataStore[0]._id;

  return collection.deleteOne({_id: _id}).then(function(){
    return updateState(task, save.index, ROLLED);
  });
}

and for updates

function rollbackRemoveOrUpdate(db, step, task) {
  var collection = db.collection(step.name);
  var chain = Promise.resolve();

  step.dataStore.forEach(function(data) {
    chain = chain.then(function() {
      var condition = {_id: data._id};

      return collection.findOne(condition).then(function (doc) {
        if (doc && step.type === UPDATE) {
          return collection.updateOne(condition, data);
        }
        else if (!doc && step.type === REMOVE) {
          return collection.insertOne(data);
        }

        return Promise.resolve();
      });
    });
  });

  return chain.then(function(){
    return updateState(task, step.index, ROLLED);
  });
}

Using {replace: true} (or a more descriptive option name) will require no changes to the code for rolling back both saves and updates. It also produces a more intuitive api for those kinds of updates IMHO.

[e-oj]

Also, Modifying the save function will require options because this

if (doc._id) {
  doc.isNew = false;
} else {
  doc._id = utils.generateId();
}

is sketchy for new documents with the _id field already set. So instead of adding options to save and rewriting that rollback function, I can extend the update options and not touch the rollback functions for the same effect

[yamila-fraiman]

Ok I understand. I think you should implement rollback and options changes to allow collection.save also if it takes more time. I can help you. Because I prefer more work than bad work.

[chikeud]

@yamifr07 This really isn't just about development time. It also includes our goal of maintaining a descriptive API. We prefer an option like {viaSave:true} on the update function to something like {isNew: false} on the save option because at the end of the day it is an update and it is more intuitively implemented through the update function. It is purely coincidental that this approach also requires less work. That said, I hope you have no further problems with this particular issue. Thanks for your input.

[e-oj]

I agree with @chikeud. @yamifr07 this will be added in the next release but PR's are always welcome. If you would like to work on this feel free to do so and open a PR.

[yamila-fraiman]

So you will never support saving existing documents as mongoose does. Ok :(

[e-oj]

It will be supported through a "viaSave" option on updates

[yamila-fraiman]

Thank you. let me know when it will be available.

[e-oj]

It will be done by Tuesday but I have some free time this weekend so it could be sooner

[e-oj]

viaSave is now available as an option on updates (options docs). If it doesn't work for you, feel free to reopen this issue. Thanks for the suggestion 👍 👍

[yamila-fraiman]

Thank you. I'll try it. Does the parameter option affect to all updates? Could I set it just for one update of the task?

[e-oj]

Options are set for a single update.

var task = Fawn.Task();

task.update(...) // update 1
  .options(...) // options for update 1
  .update(...) // update 2
  .options(...) // options for update 2
  .run();