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

Sequelize transaction is a promise; causes issues with promise libraries #1510

Closed
jarruda opened this issue Mar 15, 2014 · 7 comments
Closed

Comments

@jarruda
Copy link

jarruda commented Mar 15, 2014

The only way to create a sequelize transaction is by using callbacks, contrary to the pattern used elsewhere in the library. If one wants to start a transaction using the promise pattern, they must use a deferral.

Using the Q library:

  function startTransaction() {
    var deferral = Q.defer();
    sequelize.transaction(onTxStart);
    return deferral.promise;

    function onTxStart(err, tx) {
      if (err) {
        deferral.reject(err);
        return;
      }
      deferral.resolve(tx);
    }
  }

A conforming promise library accepts other promises to resolve them. Unbeknownst to me, the returned transaction is an unfulfilled promise. This isn't documented to my knowledge, nor is it obvious to me why this is the case, since both #commit and #rollback return promises.

Resolving a deferred promise with another promise just chains the two, and so it is impossible to return a transaction object using any conforming promise library because the promise is unfulfilled, and does not become so since it won't be passed down the chain.

@mickhansen mickhansen added the Bug label Mar 15, 2014
@jarruda
Copy link
Author

jarruda commented Mar 15, 2014

I would like to add to this issue, in the event others come across it, that it can be worked around with the Q library by utilizing the (undocumented) defer#fulfill method. This method accepts the object passed to it as a promise fulfillment and does not check if the object is a promise-like object.

@mohlman3
Copy link
Contributor

+1

Just ran into this myself. There is a workaround similar to @jarruda's comment above if using the Bluebird library, but it makes for messy and inconsistent code if you are using promises in a more standard way throughout your project.

@mohlman3
Copy link
Contributor

After taking a closer look at this, I was able to determine a better workaround if you are using the Bluebird library. Just use the Bluebird promisify function to make the Sequelize transaction function return a Promise, as follows:

var sequelize = new Sequelize(...);
sequelize.transactionPromise = Promise.promisify(sequelize.transaction, sequelize);
sequelize.transactionPromise().then(function (t) {
    // do stuff
});

I'm still supportive of including this functionality in the library by default, but at least this work around provides the same functionality as if it were included.

@mickhansen
Copy link
Contributor

Yeah that's not a great consistency pattern.
However we're probably planning to make transactions a little bit more magical by using something like CLS (https://github.com/othiym23/node-continuation-local-storage)

@mickhansen mickhansen added this to the 2.0.0 stable/non-dev milestone Apr 27, 2014
@mickhansen
Copy link
Contributor

Having looked at the code again it appears sequelize.transaction returns a promise aswell. Closing issue, re-open if you are still having issues.

@jarruda
Copy link
Author

jarruda commented May 5, 2014

@mickhansen perhaps this issue should reflect adding that fact to the docs?

It still makes passing around the transaction troublesome, as the object-as-promise pattern is generally frowned upon, but you mentioned 2.0 will sidestep the issue.

@mickhansen
Copy link
Contributor

@jarruda the object as a promise is just bad design, i'll fix that when i refactor transactions to use pools.
It's not specifically fixed by 2.0 since the transaction codebase is still the same - But it's definitely something that will be changed before a 2.0 (non-dev) release

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

No branches or pull requests

3 participants