fix: pass CLS transaction to model hooks #13927

heroic · 2022-01-09T11:45:18Z

Pull Request Checklist

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

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

Description Of Change

Fixes: #12973

Todos

Figure out how and where to write a test case to prevent future regression.

ephys · 2022-01-09T11:56:48Z

Thank you for the PR :)
Could you add a test in (i think) test/integration/cls.test.js that checks the transaction is properly passed where it wasn't before?

heroic · 2022-01-09T16:21:29Z

Hi @ephys I tried! Couldn't figure out how to make it work though. I did test the change manually in our app though. Happy to give it a shot if you can guide.

ephys · 2022-01-11T18:34:08Z

I'll try to take a stab at it next time I can

ephys · 2022-01-12T07:47:50Z

~~I'm looking into this more and it's weird that your fix is necessary.~~

Model#save calls either QueryInterface#insert or QueryInterface#update depending on Model#isNewRecord.
Both of these methods then call Sequelize#query, which retrieves the transaction from Sequelize._cls.

Edit: nevermind it's not about query it's about the hook. 🤦‍♀️
It's early.

ephys · 2022-01-12T09:40:48Z

I've added tests for this in branch feature/ephys/cls-in-hooks

Here they are: https://github.com/sequelize/sequelize/blob/feature/ephys/cls-in-hooks/test/integration/cls.test.js#L143
And the commit: 6294414

Feel free to copy them in your PR / to cherry pick the commit

It tests every Model hooks:

test/integration/cls.test.js

heroic · 2022-01-13T16:03:56Z

@ephys Still fails. Do you know why?

ephys · 2022-01-13T16:11:52Z

Well passing the transaction to hooks has been implemented in the #save method but the other model methods aren't implementing it

heroic · 2022-01-17T09:58:37Z

Well passing the transaction to hooks has been implemented in the #save method but the other model methods aren't implementing it

So we'll need to pass the transaction in other functions too. Got it. Will update

WikiRik · 2022-01-31T12:43:50Z

Hi @heroic, did you get a chance to look at this again? Or is there something we can do to help?

heroic · 2022-01-31T16:07:17Z

@WikiRik a colleague of mine is looking to get this closed! We should have this working soon

AmirMelio · 2022-05-29T12:39:41Z

@WikiRik a colleague of mine is looking to get this closed! We should have this working soon

Hi @heroic , any update?

joker00777 · 2022-05-30T20:51:24Z

Hi @AmirMelio I will be continuing it from here.

test/integration/cls.test.js

joker00777

Test file typo

test/integration/cls.test.js

joker00777

any idea why these are failing?

lib/model.js

test/integration/cls.test.js

joker00777

Sorry for the late response : (

test/integration/cls.test.js

WikiRik · 2022-06-13T13:30:21Z

GitHub has issues with the caching today, I'll rerun the checks after they have solved that incident.

src/model-internals.ts

joker00777 · 2022-06-15T12:30:25Z

@ephys any updates?

joker00777 · 2022-06-21T04:44:43Z

@ephys waiting for your feedback : )

joker00777 · 2022-06-23T18:19:40Z

@ephys can we merge it now : ) ?

ephys

There are a few unresolved comments from the previous review :)

test/integration/cls.test.js

joker00777 · 2022-07-07T06:36:11Z

Hey @ephys can you take a look now?

ephys · 2022-07-08T13:49:01Z

This is great :) There are two remaining comments and then this is ready to merge:

Could you enable the option to allow maintainers to add commits on your branch once you are ready? We use it to update the branch before merging.

heroic · 2022-07-08T16:40:57Z

Hi @ephys, since the fork is owned by an org, there isn't an option to enable Allow edits by maintainers. We can add the maintainers write access on the repo by adding them individually. Would that work?

ephys · 2022-07-09T13:12:02Z

I think we can try coordinating. If you can click the "update branch" button then @ me and/or @WikiRik, I'll merge this immediately before another PR

joker00777 · 2022-07-09T19:36:47Z

@ephys @WikiRik update branch Done : )

WikiRik · 2022-07-09T19:51:29Z

@joker00777 we accidentally merged something else just now, could you update the branch once again?

ephys · 2022-07-09T19:56:06Z

Sorry 😅

joker00777 · 2022-07-09T20:26:31Z

haha done @WikiRik @ephys ; )

WikiRik · 2022-07-09T20:26:55Z

Thanks! After the tests pass, I'll merge this

joker00777 · 2022-07-09T20:27:23Z

Thank You : )

WikiRik · 2022-07-09T20:51:56Z

There we go! Thanks again for working on this @joker00777 @heroic

github-actions · 2022-08-07T20:15:17Z

🎉 This PR is included in version 7.0.0-alpha.15 🎉

The release is available on:

Your semantic-release bot 📦🚀

ephys added the type: bug label Jan 9, 2022

ephys self-assigned this Jan 11, 2022

ephys reviewed Jan 12, 2022

View reviewed changes

test/integration/cls.test.js Outdated Show resolved Hide resolved

ephys changed the title ~~fix: sequelize should take transaction from CLS everywhere~~ fix: pass CLS transaction to model hooks Jan 26, 2022

joker00777 reviewed Jun 2, 2022

View reviewed changes

test/integration/cls.test.js Outdated Show resolved Hide resolved

joker00777 reviewed Jun 2, 2022

View reviewed changes

joker00777 force-pushed the main branch 2 times, most recently from 7758520 to e1d4dc2 Compare June 2, 2022 05:03

joker00777 reviewed Jun 2, 2022

View reviewed changes

test/integration/cls.test.js Outdated Show resolved Hide resolved

joker00777 reviewed Jun 2, 2022

View reviewed changes

ephys requested changes Jun 2, 2022

View reviewed changes

lib/model.js Outdated Show resolved Hide resolved

test/integration/cls.test.js Outdated Show resolved Hide resolved

test/integration/cls.test.js Outdated Show resolved Hide resolved

joker00777 reviewed Jun 13, 2022

View reviewed changes

test/integration/cls.test.js Outdated Show resolved Hide resolved

test/integration/cls.test.js Outdated Show resolved Hide resolved

ephys requested changes Jun 13, 2022

View reviewed changes

src/model-internals.ts Outdated Show resolved Hide resolved

joker00777 force-pushed the main branch from 4faeaca to 3ec5ad0 Compare June 13, 2022 18:57

ephys self-requested a review June 21, 2022 05:31

joker00777 force-pushed the main branch from 67f8553 to 3bb86b0 Compare June 21, 2022 15:56

WikiRik requested a review from ephys June 24, 2022 10:49

ephys requested changes Jul 3, 2022

View reviewed changes

test/integration/cls.test.js Outdated Show resolved Hide resolved

joker00777 force-pushed the main branch 4 times, most recently from e8d2346 to aeecb25 Compare July 6, 2022 14:28

ephys self-requested a review July 8, 2022 13:41

fix: sequelize should take transaction from CLS everywhere

da1d44b

joker00777 force-pushed the main branch from aeecb25 to da1d44b Compare July 8, 2022 16:19

ephys approved these changes Jul 9, 2022

View reviewed changes

Merge branch 'main' into main

f276c69

Merge branch 'main' into main

c9b63b3

WikiRik merged commit be98856 into sequelize:main Jul 9, 2022

github-actions bot added the released on @v7 label Aug 7, 2022

joker00777 mentioned this pull request Mar 22, 2023

fix: cls transaction hook #15818

Merged

8 tasks

chriswheeldon-peakon mentioned this pull request Jun 22, 2023

Models loaded with includes retain a reference to the transaction #16161

Open

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: pass CLS transaction to model hooks #13927

fix: pass CLS transaction to model hooks #13927

heroic commented Jan 9, 2022

ephys commented Jan 9, 2022

heroic commented Jan 9, 2022

ephys commented Jan 11, 2022

ephys commented Jan 12, 2022 •

edited

ephys commented Jan 12, 2022 •

edited

heroic commented Jan 13, 2022

ephys commented Jan 13, 2022

heroic commented Jan 17, 2022

WikiRik commented Jan 31, 2022

heroic commented Jan 31, 2022

AmirMelio commented May 29, 2022

joker00777 commented May 30, 2022 •

edited

joker00777 left a comment

joker00777 left a comment

joker00777 left a comment

WikiRik commented Jun 13, 2022

joker00777 commented Jun 15, 2022

joker00777 commented Jun 21, 2022

joker00777 commented Jun 23, 2022

ephys left a comment

joker00777 commented Jul 7, 2022

ephys commented Jul 8, 2022

heroic commented Jul 8, 2022

ephys commented Jul 9, 2022 •

edited

joker00777 commented Jul 9, 2022

WikiRik commented Jul 9, 2022

ephys commented Jul 9, 2022 •

edited

joker00777 commented Jul 9, 2022

WikiRik commented Jul 9, 2022

joker00777 commented Jul 9, 2022

WikiRik commented Jul 9, 2022

github-actions bot commented Aug 7, 2022

fix: pass CLS transaction to model hooks #13927

fix: pass CLS transaction to model hooks #13927

Conversation

heroic commented Jan 9, 2022

Pull Request Checklist

Description Of Change

Todos

ephys commented Jan 9, 2022

heroic commented Jan 9, 2022

ephys commented Jan 11, 2022

ephys commented Jan 12, 2022 • edited

ephys commented Jan 12, 2022 • edited

heroic commented Jan 13, 2022

ephys commented Jan 13, 2022

heroic commented Jan 17, 2022

WikiRik commented Jan 31, 2022

heroic commented Jan 31, 2022

AmirMelio commented May 29, 2022

joker00777 commented May 30, 2022 • edited

joker00777 left a comment

Choose a reason for hiding this comment

joker00777 left a comment

Choose a reason for hiding this comment

joker00777 left a comment

Choose a reason for hiding this comment

WikiRik commented Jun 13, 2022

joker00777 commented Jun 15, 2022

joker00777 commented Jun 21, 2022

joker00777 commented Jun 23, 2022

ephys left a comment

Choose a reason for hiding this comment

joker00777 commented Jul 7, 2022

ephys commented Jul 8, 2022

heroic commented Jul 8, 2022

ephys commented Jul 9, 2022 • edited

joker00777 commented Jul 9, 2022

WikiRik commented Jul 9, 2022

ephys commented Jul 9, 2022 • edited

joker00777 commented Jul 9, 2022

WikiRik commented Jul 9, 2022

joker00777 commented Jul 9, 2022

WikiRik commented Jul 9, 2022

github-actions bot commented Aug 7, 2022

ephys commented Jan 12, 2022 •

edited

ephys commented Jan 12, 2022 •

edited

joker00777 commented May 30, 2022 •

edited

ephys commented Jul 9, 2022 •

edited

ephys commented Jul 9, 2022 •

edited