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

feat(transactions): Add afterCommit hooks for transactions #9287

Merged

Conversation

@dylanlingelbach
Copy link
Contributor

@dylanlingelbach dylanlingelbach commented Apr 10, 2018

Pull Request check-list

Please make sure to review and check all of these items:

  • Does npm run test or npm run test-DIALECT pass with this change (including linting)?
  • Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?
  • Have you added new tests to prevent regressions?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
  • Did you follow the commit message conventions explained in CONTRIBUTING.md?

Description of change

Add afterCommit hooks for transactions that allows deferring work to
after a transaction has been committed, regardless of if you have
access to the promise chain that created the transaction

Closes: #2805

@dylanlingelbach
Copy link
Contributor Author

@dylanlingelbach dylanlingelbach commented Apr 10, 2018

@codecov
Copy link

@codecov codecov bot commented Apr 10, 2018

Codecov Report

Merging #9287 into master will increase coverage by <.01%.
The diff coverage is 100%.

Copy link
Contributor

@sushantdhiman sushantdhiman left a comment

Some changes, otherwise looks like a good addition

@@ -22,6 +22,7 @@ class Transaction {
constructor(sequelize, options) {
this.sequelize = sequelize;
this.savepoints = [];
this.afterCommitHooks = [];

This comment has been minimized.

@sushantdhiman

sushantdhiman Apr 14, 2018
Contributor

_afterCommitHooks

});
}).tap(
() => this.sequelize.Promise.each(
this.afterCommitHooks,

This comment has been minimized.

@sushantdhiman

sushantdhiman Apr 14, 2018
Contributor

Use const Promise = require('./promise');

* @name afterCommit
* @memberOf Sequelize.Transaction
*/
afterCommit(fn) {

This comment has been minimized.

@sushantdhiman

sushantdhiman Apr 14, 2018
Contributor

assert for fn being a function + test

@@ -79,6 +79,32 @@ if (current.dialect.supports.transactions) {
});
});

it('supports running hooks when a transaction is commited', function() {
const self = this;

This comment has been minimized.

@sushantdhiman

sushantdhiman Apr 14, 2018
Contributor

No self = this

it('supports running hooks when a transaction is commited', function() {
const self = this;
const hook = sinon.spy();
let t;

This comment has been minimized.

@sushantdhiman

sushantdhiman Apr 14, 2018
Contributor

let transaction;

const self = this;
const hook = sinon.spy();
let t;
return expect(this.sequelize.transaction(transaction => {

This comment has been minimized.

@sushantdhiman

sushantdhiman Apr 14, 2018
Contributor

Call this t

return expect(this.sequelize.transaction(transaction => {
t = transaction;
transaction.afterCommit(hook);
return self.sequelize.query('SELECT 1+1', {transaction, raw: true});

This comment has been minimized.

@sushantdhiman

sushantdhiman Apr 14, 2018
Contributor

No need to pass { raw: true }, its not required. Instead pass type: QueryType.SELECT

@dylanlingelbach
Copy link
Contributor Author

@dylanlingelbach dylanlingelbach commented Apr 16, 2018

@sushantdhiman thanks for the feedback! Just updated - let me know if you see anything else

@dylanlingelbach dylanlingelbach force-pushed the dylanlingelbach:feat/transaction-commit-hooks branch from 7b43f41 to e6bdc82 Apr 17, 2018
@dylanlingelbach
Copy link
Contributor Author

@dylanlingelbach dylanlingelbach commented Apr 17, 2018

Actually just updated now - my push failed yesterday

@sushantdhiman
Copy link
Contributor

@sushantdhiman sushantdhiman commented Apr 17, 2018

Linting errors

/home/travis/build/sequelize/sequelize/lib/transaction.js
  200:23  error  Strings must use singlequote  quotes
/home/travis/build/sequelize/sequelize/test/integration/transaction.test.js
  274:39  error  Strings must use singlequote  quotes
  295:39  error  Strings must use singlequote  quotes
  316:39  error  Strings must use singlequote  quotes
✖ 4 problems (4 errors, 0 warnings)
  4 errors, 0 warnings potentially fixable with the `--fix` option.
@dylanlingelbach dylanlingelbach force-pushed the dylanlingelbach:feat/transaction-commit-hooks branch from e6bdc82 to 8f6aa84 Apr 17, 2018
Copy link
Contributor

@sushantdhiman sushantdhiman left a comment

LGTM, just remove self = this approach from extra tests you have added

@@ -189,6 +215,107 @@ if (current.dialect.supports.transactions) {
).to.be.rejectedWith('Transaction cannot be committed because it has been finished with state: commit');
});

it('should run hooks if a non-auto callback transaction is committed', function() {
const self = this;

This comment has been minimized.

@sushantdhiman

sushantdhiman Apr 18, 2018
Contributor

This test is still using self, next one too

@sushantdhiman sushantdhiman requested a review from sequelize/orm Apr 18, 2018
Copy link
Member

@eseliger eseliger left a comment

other than the self in the tests, as mentioned by @sushantdhiman , looks good

Copy link
Contributor

@mickhansen mickhansen left a comment

Please add documentation covering the fact that you cannot modify the return value of a transaction inside afterCommit (most other hooks allow modification)

@dylanlingelbach dylanlingelbach force-pushed the dylanlingelbach:feat/transaction-commit-hooks branch from 8f6aa84 to 7de7551 Apr 18, 2018
@dylanlingelbach
Copy link
Contributor Author

@dylanlingelbach dylanlingelbach commented Apr 18, 2018

@sushantdhiman fixed, sorry for missing that again

@mickhansen added

@eseliger
Copy link
Member

@eseliger eseliger commented Apr 18, 2018

There's still lint issues😕

Add `afterCommit` hooks for transactions that allows deferring work to
after a transaction has been committed, regardless of if you have
access to the promise chain that created the transaction

Fixes: #2805
@dylanlingelbach dylanlingelbach force-pushed the dylanlingelbach:feat/transaction-commit-hooks branch from 7de7551 to ca3025a Apr 18, 2018
@dylanlingelbach
Copy link
Contributor Author

@dylanlingelbach dylanlingelbach commented Apr 18, 2018

@eseliger 🤦‍♂️ whoops. Fixed now

@eseliger eseliger requested a review from mickhansen Apr 18, 2018
@Arwid
Copy link

@Arwid Arwid commented Apr 26, 2018

+1

@sushantdhiman sushantdhiman merged commit f92a7e7 into sequelize:master Apr 29, 2018
4 checks passed
4 checks passed
codecov/patch 100% of diff hit (target 95.94%)
Details
codecov/project 95.94% (+<.01%) compared to e6de171
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Strate
Copy link

@Strate Strate commented Aug 20, 2018

I think there is a bug in this feature: if transaction is a child (transaction.parent is set), then afterCommit hooks should not be invoked right after this transaction committed, because parent transaction is not committed yet, and is not guaranteed to be committed at all.

The easiest way to fix this is change the way how hooks are added:

afterCommit(fn) {
  if (this.parent) {
    this.parent.afterCommit(fn)
  } else {
    this._afterCommitHooks.push(fn);
  }
}
@jedwards1211
Copy link
Contributor

@jedwards1211 jedwards1211 commented Sep 5, 2018

could you guys port this back to v4? Pretty please? I am basically going to have to monkeypatch support for it into my codebase...

@esalter
Copy link

@esalter esalter commented Dec 12, 2018

Seconded for asking for a backport to v4, if possible. It would be very much appreciated!

@jedwards1211
Copy link
Contributor

@jedwards1211 jedwards1211 commented Dec 12, 2018

@esalter I just made a PR to backport, hopefully they will merge and release it!

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

Successfully merging this pull request may close these issues.

8 participants