-
Couldn't load subscription status.
- Fork 350
feat(Migrator): allow passing transactions to Migrator.
#1480
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
base: next
Are you sure you want to change the base?
feat(Migrator): allow passing transactions to Migrator.
#1480
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
commit: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey 👋
Thanks! 🙏
Left some comments.
This is not a fix. This is a whole use case that wasn't considered.
Migrator.
…tions` Previously, initializing Migrator with a `Transaction` instance (`trx`) and `disableTransactions: true` would lead to an error because Migrator would attempt `trx.connection()`. This change ensures that if `db` is already a `Transaction` and `disableTransactions` is true, the Migrator uses the provided `trx` directly, enabling transactional dry-runs.
7283087 to
84fcd27
Compare
Co-authored-by: Igal Klebanov <igalklebanov@gmail.com>
Co-authored-by: Igal Klebanov <igalklebanov@gmail.com>
Co-authored-by: Igal Klebanov <igalklebanov@gmail.com>
Co-authored-by: Igal Klebanov <igalklebanov@gmail.com>
Co-authored-by: Igal Klebanov <igalklebanov@gmail.com>
Co-authored-by: Igal Klebanov <igalklebanov@gmail.com>
|
Any updates on this? |
Hey team!
This PR addresses an issue where the
Migratorwould unexpectedly fail if initialized with a KyselyTransactioninstance (trx) whiledisableTransactionswas set totrue. The Migrator was attempting to calltrx.connection(), which isn't a supported operation on aTransactionobject.The Problem:
When
disableTransactionsis true, theMigratorfalls back to usingthis.#props.db.connection().execute(...). Ifthis.#props.dbis aTransactioninstance, this leads to:Error: calling the connection method for a Transaction is not supportedThis prevented a useful pattern: performing a "transactional dry-run" of migrations, where migrations are executed within an outer transaction that is subsequently rolled back.
The Solution:
This change modifies the
Migratorto check if the provideddbinstanceisTransaction. If it is, anddisableTransactionsis alsotrue, theMigratorwill now directly use the providedTransactioninstance (this.#props.db) to run the migration logic (run(this.#props.db)), bypassing the problematic.connection()call.Use Case Example (Transactional Migration Validation):
This fix makes it much easier to validate migrations without permanently altering the database, which is super handy for pre-push hooks or CI checks:
Testing:
I've added a test case to cover this specific scenario, ensuring the
Migratorbehaves as expected when used with aTransactionanddisableTransactions: true.