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

Fixed transaction promise mutation issue #1991

Merged
merged 3 commits into from Mar 26, 2017

Conversation

Projects
None yet
2 participants
@elhigu
Collaborator

elhigu commented Mar 25, 2017

Fixes #1052 and fixed + disabled test case of #1970.

promiseInterface.forEach(function(method) {
Transaction.prototype[method] = function() {
return (this._promise = this._promise[method].apply(this._promise, arguments))
return this._promise[method].apply(this._promise, arguments)

This comment has been minimized.

@elhigu

elhigu Mar 25, 2017

Collaborator

This is the actual fix

})
.then(expectQueryEventToHaveBeenTriggered)
.catch(expectQueryEventToHaveBeenTriggered);
return knex.transaction(function(trx) {

This comment has been minimized.

@elhigu

elhigu Mar 25, 2017

Collaborator

changed done() callback async test to just return promise

expect(error.message).to.equal('Transaction rejected with non-error: undefined');
});
it.skip('Rollback without an error should not reject with undefined #1966', function() {
return knex.transaction(function(tr) {

This comment has been minimized.

@elhigu

elhigu Mar 25, 2017

Collaborator

Added missing return + disabled the test because the implemented feature is broken

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Mar 25, 2017

@wubzz could you check this out please.

@wubzz

This comment has been minimized.

Collaborator

wubzz commented Mar 25, 2017

@elhigu I don't understand what you mean by mysql and maria overriding transaction.query(), there are no dialect specific extensions of the Transaction class as far as I know?

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Mar 25, 2017

Check src/dialects/maria/transaction.js

@wubzz

This comment has been minimized.

Collaborator

wubzz commented Mar 25, 2017

Ah I see it now. The copy-pasta for a single extra error handler hurts my eyes. I was under the impression transaction was unified across all dialets. Will look into this..

@elhigu elhigu merged commit 9682446 into master Mar 26, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment