Callback evaluated with wrong arguments when passing Collection2 parameter

[nscarcella]

Hello,

I'm trying to write transactional versions of the form types on the autoform package, so they'll use transactions on the inserts and updates.

To do so, I need to pass the validationContext collection2 argument (set to the form id) to the insert operation. which I try to do this way:

myCollection.insert(doc, {tx: true, instant: true, validationContext: "subject-insert", callback: (error, res) => ...)

My problem is I don't think this argument is properly being delivered because, when I add it, my callback function is always evaluated with error and res set to null even if the insert failed.

I went as deep in the code as I could follow and it seems to go fine up until this point in Transact.prototype._doInsert:

  if (this._Collection2Support(collection, opt)) {
    // My call gets here. It seems like the right place to be, but causes the callback to eval with the wrong arguments
  }
  else {
    // If I don't add the validationContext argument my call gets here and the callback works properly.
  }
}

Also, if I passed validationContext, calls to collection.simpleSchema().namedContext('subject-insert').invalidKeys() and collection.simpleSchema().namedContext().invalidKeys() booth return an empty array, even when the insert failed.

Is this a bug or am I doing something wrong?

[nscarcella]

Looking a bit deeper into Transact.prototype._doInsert, it seems like the user callback should be called inside the local insert callback; otherwise the insert may not have time to update the variables. So perhaps this:

Transact.prototype._doInsert = function (collection, newDoc, opt, callback) {
  // The following is a very sketchy attempt to support collection2 options
  // Requires aldeed:collection2 to be before babrahams:transactions in .packages
  // which we do through a weak dependency on aldeed:collection2
  if (this._Collection2Support(collection, opt)) {
    // This is a brutal workaround to allow collection2 options to do their work
    var newId = null;
    var error = null;
    collection.insert(newDoc, opt, function (err, newId) { 
      if (!err) {
        newId = newId;
      }
      else {
        error = err;    
      }
    });
    if (_.isFunction(callback)) { // Let the app handle the error via its own callback
      callback(error, newId);
    }
    if (newId) {
      return newId;
    }
    else {
      throw new Meteor.Error('Insert failed: ' + (error && error.message || 'reason unknown.'),(error && error.reason || ''));  
    }
  }
  else {
    return collection.insert(newDoc,callback);
  }
}

Should be:

Transact.prototype._doInsert = function (collection, newDoc, opt, callback) {
  // The following is a very sketchy attempt to support collection2 options
  // Requires aldeed:collection2 to be before babrahams:transactions in .packages
  // which we do through a weak dependency on aldeed:collection2
  if (this._Collection2Support(collection, opt)) {
    // This is a brutal workaround to allow collection2 options to do their work
    var newId = null;
    var error = null;
    var returnedId = collection.insert(newDoc, opt, function (err, newId) {
      if (!err) {
        newId = newId;
      }
      else {
        error = err;    
      }
      if (_.isFunction(callback)) { // Let the app handle the error via its own callback
        callback(error, newId);
      }
    });
    if (returnedId) {
      return returnedId;
    }
    else {
      throw new Meteor.Error('Insert failed: reason unknown.', '');  
    }
  }
  else {
    return collection.insert(newDoc, callback);
  }
}

This change seems to make this scenario work (though the error raised in case there is no returned id would be always unknown, it probably already is...). I could create a PR if you want me to, @JackAdams.

Also I noticed the other methods (like _doUpdate) are quite different and haven't got time to check them yet, so I'm not sure if something similar may be happening, but it may be worth to check.

[JackAdams]

Yes, I'm happy to accept a PR. I'll run it against the (fairly limited) test suite.

By the way, your change makes complete sense -- I don't know what I was thinking when I wrote that bit of code. :-p

Thanks for bringing this up.

[nscarcella]

Awesome, thanks for merging this so quickly!

When can I expect a version jump to see the changes reflected?

[JackAdams]

I'll release 0.7.8 on atmosphere in 1-1.5 hours. Just addressing another issue in the same release and it's non-trivial. Need to write an automated test and have it pass that, so doing that now.