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

Fixed transaction promise mutation issue #1991

Merged
merged 3 commits into from Mar 26, 2017

Conversation

@elhigu
Copy link
Member

@elhigu 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
Author Member

This is the actual fix

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

This comment has been minimized.

@elhigu

elhigu Mar 25, 2017
Author Member

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
Author Member

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

@elhigu
Copy link
Member Author

@elhigu elhigu commented Mar 25, 2017

@wubzz could you check this out please.

@wubzz
Copy link
Member

@wubzz 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
Copy link
Member Author

@elhigu elhigu commented Mar 25, 2017

Check src/dialects/maria/transaction.js

@wubzz
Copy link
Member

@wubzz 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
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
@kibertoad kibertoad deleted the fixed-transaction-promise-mutation-issue branch Dec 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants