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

fix(hook): maintain CLS context in after hooks #9473

Merged
merged 8 commits into from May 25, 2018

Conversation

msuret
Copy link

@msuret msuret commented May 23, 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

Encapsulate promises returned by retry-as-promised with Promise.resolve to maintain CLS context in afterXXX hooks.

fix #9472
fix #9071

encapsulate promises returned by `retry-as-promised` with `Promise.resolve` to maintain CLS context

fix sequelize#9472
@codecov
Copy link

codecov bot commented May 23, 2018

Codecov Report

Merging #9473 into master will not change coverage.
The diff coverage is 100%.

@sushantdhiman
Copy link
Contributor

Hmm so you are saying this can also fix #9071, Can you add a test for this with other cls tests?

@msuret
Copy link
Author

msuret commented May 23, 2018

@sushantdhiman Ok, I'll give it a try

Maxime Suret added 3 commits May 24, 2018 14:09
…ly patched

Promises returned by sequelize.query must be correctly patched to maintain CLS context
@msuret
Copy link
Author

msuret commented May 24, 2018

@sushantdhiman I added a simple test case which fails in current master and passes with my PR.
I believe this also fixes #9213 and #9383.

@@ -164,5 +164,17 @@ if (current.dialect.supports.transactions) {
it('CLS namespace is stored in Sequelize._cls', function() {
expect(Sequelize._cls).to.equal(this.ns);
});

it('promises returned by sequelize.query are correctly patched', function() {
const self = this;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No self = this

@@ -164,5 +164,17 @@ if (current.dialect.supports.transactions) {
it('CLS namespace is stored in Sequelize._cls', function() {
expect(Sequelize._cls).to.equal(this.ns);
});

it('promises returned by sequelize.query are correctly patched', function() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs a real world test, create row under transaction, try to access that row in nested context assuming cls context is passed

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or may be take inspiration from #9383 (2nd test looks neat)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually it's already almost exactly the 2nd test from #9383.
I just removed the first two .then(), which are useless because the first sequelize.query() already returns a non-patched promise.

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

Successfully merging this pull request may close these issues.

Hook queries are not run inside surrounding transaction retry-as-promised Not Patched With CLS
2 participants