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: upgrade knex from 2.4.0 to 2.4.2 #4309

Merged
merged 1 commit into from Jan 27, 2023
Merged

fix: upgrade knex from 2.4.0 to 2.4.2 #4309

merged 1 commit into from Jan 27, 2023

Conversation

owlas
Copy link
Collaborator

@owlas owlas commented Jan 27, 2023

Also fixes new warnings with double rollbacks in transaction blocks

2.4.0 contains a bug when inserting arrays: knex/knex#5365

Reviewing

The diff looks big but I've only made two changes:

  • upgrading to 2.4.2
  • Removing the try {} except {trx.rollback(e); throw e;} from a bunch of transacting blocks

Also fixes new warnings with double rollbacks in transaction blocks
@netlify
Copy link

netlify bot commented Jan 27, 2023

Deploy Preview for peaceful-bassi-cbf284 canceled.

Name Link
🔨 Latest commit a5b35d5
🔍 Latest deploy log https://app.netlify.com/sites/peaceful-bassi-cbf284/deploys/63d3ff600d79e600083c4b8d

@owlas owlas requested a deployment to fix/patch-knex - jaffle_db PR #4309 January 27, 2023 16:44 — with Render Abandoned
@owlas owlas temporarily deployed to fix/patch-knex - headless-browser PR #4309 January 27, 2023 16:44 — with Render Destroyed
@owlas owlas temporarily deployed to fix/patch-knex - lightdash PR #4309 January 27, 2023 16:46 — with Render Destroyed
@owlas owlas requested a review from a team January 27, 2023 16:49
Copy link
Collaborator

@rephus rephus left a comment

Choose a reason for hiding this comment

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

IF e2e pass, it should be safe

}),
);
} catch (e) {
trx.rollback(e);
Copy link
Collaborator

Choose a reason for hiding this comment

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

why don't we want those rollbacks ? are they applied automatically ?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, knex handles that for us. This was a bad practice when we started using knex.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's right! About half way down this page: https://knexjs.org/guide/transactions.html

And in dev when the error happened it was inside this transaction block. And the knex threw another error (hiding the real one) saying "you're trying to exit a transaction that has already been closed" (or something like that).

This fixed the dev experience. It doesn't seem to happen in production though, which is curious

@owlas owlas merged commit 4bb578c into main Jan 27, 2023
@owlas owlas deleted the fix/patch-knex branch January 27, 2023 17:47
github-actions bot pushed a commit that referenced this pull request Jan 27, 2023
## [0.395.1](0.395.0...0.395.1) (2023-01-27)

### Bug Fixes

* upgrade knex from 2.4.0 to 2.4.2 ([#4309](#4309)) ([4bb578c](4bb578c))
@github-actions
Copy link

🎉 This PR is included in version 0.395.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

None yet

3 participants