Add migration lifecycle hooks #5541

timorthi · 2023-04-12T17:12:01Z

This is an attempt at addressing the use cases described in #2983 .

This adds the ability for the user to define a beforeAll, afterAll, beforeEach, afterEach hook during migrations.

timorthi · 2023-04-12T17:13:51Z

lib/migrations/migrate/Migrator.js

-    if (commitFn) {
-      commitFn();
-    }


I removed the explicit commit so that we can run the afterEach hook in the same transaction (line 527)

coveralls · 2023-04-12T17:17:26Z

coverage: 92.888% (+0.1%) from 92.787%
when pulling f7a3147 on leaselock:timorthi/issue2983-migration-lifecycle-hooks
into 4ca3dd5 on knex:master.

timorthi · 2023-04-17T15:59:32Z

@kibertoad gentle ping - would love to get your feedback on this 😄

rluvaton

Looking good! thanks for the great job!
could you please also update the types?

left few comments and I also think docs should be updated as well (Cc @kibertoad)

Also few questions:

If you have 2 migrations, and the second one failed, should you run the afterAll should you rollback the beforeAll?
In case transaction are disabled for migrations, should we allow hooks?

test/integration2/migrate/migration-integration.spec.js

…-migration-lifecycle-hooks

test/integration2/migrate/migration-lifecycle-integration.spec.js

timorthi · 2023-12-19T20:11:16Z

@rluvaton

If you have 2 migrations, and the second one failed, should you run the afterAll should you rollback the beforeAll?

In this scenario, I feel like it may be appropriate to abandon the afterAll and roll back the beforeAll.

In case transaction are disabled for migrations, should we allow hooks?

I can't personally think of a situation in which someone would want to run these hooks without also running their migrations in a transaction, though I wouldn't be surprised if that's a use case for someone out there. Maybe for the initial release, we can require transactions to run hooks, and see if anyone raises an issue with that.

rluvaton

Could you please add test when transaction are disabled for migration? And also update the types

test/integration2/migrate/migration-lifecycle-integration.spec.js

rluvaton · 2023-12-20T07:31:10Z

test/integration2/migrate/migration-lifecycle-integration.spec.js

+
+      // Force clean slate before each test
+      beforeEach(async () => {
+        rimraf.sync(path.join(__dirname, './migration'));


Not 100% sure - this setup block was copied from migration-integration.spec.js and I chose not to change anything.

Co-authored-by: Raz Luvaton <16746759+rluvaton@users.noreply.github.com>

….com:leaselock/knex into timorthi/issue2983-migration-lifecycle-hooks

timorthi · 2023-12-20T17:34:01Z

lib/migrations/migrate/Migrator.js

-              name
-            );
+            return await this.knex.transaction(async (trx) => {
+              await beforeEach(trx, [migration]);


The migration that's passed into the hook is wrapped in an array here to maintain consistent types between the each and all hooks (instead of passing in an array in all but a migration object in each)

timorthi · 2023-12-20T17:35:05Z

test/integration2/migrate/migration-lifecycle-integration.spec.js

+
+      // Force clean slate before each test
+      beforeEach(async () => {
+        rimraf.sync(path.join(__dirname, './migration'));


Not 100% sure - this setup block was copied from migration-integration.spec.js and I chose not to change anything.

timorthi · 2023-12-20T17:42:06Z

@rluvaton

Could you please add test when transaction are disabled for migration?

I took a closer look at the code and it appears that the user is allowed to customize whether each migration is run in a transaction or not:

knex/lib/migrations/migrate/Migrator.js

Lines 480 to 490 in f7a3147

    
           // If transaction config for a single migration is defined, use that. 
        
           // Otherwise, rely on the common config. This allows enabling/disabling 
        
           // transaction for a single migration at will, regardless of the common 
        
           // config. 
        
           _useTransaction(migrationContent, allTransactionsDisabled) { 
        
             const singleTransactionValue = get(migrationContent, 'config.transaction'); 
        
             return isBoolean(singleTransactionValue) 
        
               ? singleTransactionValue 
        
               : !allTransactionsDisabled; 
        
           }

Because of this, I think it may be simpler to just run the lifecycle hooks all the time, even if transactions are disabled. Otherwise, we will be dealing with a bunch of edge cases regarding when or when not to run lifecycle hooks. What do you think and would you still like me to test the non-transaction case if we choose to always run hooks?

rluvaton · 2023-12-20T20:23:29Z

Can you please update the types and fix the CI so we can merge this

timorthi · 2023-12-20T21:13:43Z

@rluvaton I've updated index.d.ts, did I miss any other type definitions?

For CI, it looks like this particular step (Legacy Types Linting) has been failing for the last 3 weeks, unrelated to this PR https://github.com/knex/knex/actions/workflows/linting-legacy.yml

rluvaton

Thank you for your contribution

@kibertoad LGTM

michaelwiles · 2023-12-29T10:26:05Z

Please can this be merged? I need this as well.

m33ch · 2024-05-31T13:59:33Z

Hello and thank you all for your contribution! I really needed this feature.

I noticed that the change has been merged into master but I see that a new version hasn't been pubilshed to npm.
Approximately, when is a new release on npm expected?

Thanks a lot!

timorthi added 2 commits April 12, 2023 09:30

feat: add migration lifecycle hooks

3f8e597

test: add tests for migration lifecycle hooks

2d53af4

timorthi commented Apr 12, 2023

View reviewed changes

test: fix test setup

ffcc089

timorthi added 2 commits April 18, 2023 10:19

fix: add missing await

c822fac

feat: add beforeAll/afterAll to up, down, rollback

999cef0

rluvaton reviewed Dec 2, 2023

View reviewed changes

timorthi added 6 commits December 19, 2023 10:33

Merge branch 'master' of github.com:knex/knex into timorthi/issue2983…

5dafbbe

…-migration-lifecycle-hooks

test: rename to count

9c53658

test: create separate file for lifecycle hooks tests

81d19c1

test: remove extraneous setup

0f45133

test: rename to count

d681c86

test: add more comprehensive tests

e2bc612

timorthi commented Dec 19, 2023

View reviewed changes

test/integration2/migrate/migration-lifecycle-integration.spec.js Outdated Show resolved Hide resolved

test: force clean slate before each test

25cd3bc

timorthi requested a review from rluvaton December 19, 2023 20:24

timorthi added 4 commits December 19, 2023 16:26

feat: short circuit if no migrations to run

5bb8d4e

refactor: remove unnecessary asyncs

6f975bd

refactor: remove unnecessary asyncs

772caa1

fix: run hooks and migrations only if there are pending migrations

c88ff9c

timorthi marked this pull request as ready for review December 20, 2023 00:52

rluvaton reviewed Dec 20, 2023

View reviewed changes

timorthi and others added 5 commits December 20, 2023 08:10

Update test/integration2/migrate/migration-lifecycle-integration.spec.js

6eaaf7f

Co-authored-by: Raz Luvaton <16746759+rluvaton@users.noreply.github.com>

test: always return error value

87016d4

test: simplify expression

cce4371

feat: always pass list of migrations in hooks

30b8711

feat: add types for lifecycle hooks

cc2680b

timorthi added 2 commits December 20, 2023 09:31

Merge branch 'timorthi/issue2983-migration-lifecycle-hooks' of github…

ce0ace0

….com:leaselock/knex into timorthi/issue2983-migration-lifecycle-hooks

refactor: use brackets to denote array

f7a3147

timorthi commented Dec 20, 2023

View reviewed changes

timorthi requested a review from rluvaton December 20, 2023 17:49

timorthi mentioned this pull request Dec 20, 2023

Migrations: Add lifecycle callbacks #2983

Open

rluvaton approved these changes Dec 21, 2023

View reviewed changes

kibertoad merged commit 3764ded into knex:master Jan 13, 2024
35 of 36 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add migration lifecycle hooks #5541

Add migration lifecycle hooks #5541

timorthi commented Apr 12, 2023 •

edited

Loading

timorthi Apr 12, 2023

coveralls commented Apr 12, 2023 •

edited

Loading

timorthi commented Apr 17, 2023

rluvaton left a comment

timorthi commented Dec 19, 2023

rluvaton left a comment •

edited

Loading

rluvaton Dec 20, 2023

timorthi Dec 20, 2023

timorthi Dec 20, 2023

timorthi Dec 20, 2023

timorthi commented Dec 20, 2023

rluvaton commented Dec 20, 2023 •

edited

Loading

timorthi commented Dec 20, 2023

rluvaton left a comment •

edited

Loading

michaelwiles commented Dec 29, 2023

m33ch commented May 31, 2024

Add migration lifecycle hooks #5541

Add migration lifecycle hooks #5541

Conversation

timorthi commented Apr 12, 2023 • edited Loading

timorthi Apr 12, 2023

Choose a reason for hiding this comment

coveralls commented Apr 12, 2023 • edited Loading

timorthi commented Apr 17, 2023

rluvaton left a comment

Choose a reason for hiding this comment

timorthi commented Dec 19, 2023

rluvaton left a comment • edited Loading

Choose a reason for hiding this comment

rluvaton Dec 20, 2023

Choose a reason for hiding this comment

timorthi Dec 20, 2023

Choose a reason for hiding this comment

timorthi Dec 20, 2023

Choose a reason for hiding this comment

timorthi Dec 20, 2023

Choose a reason for hiding this comment

timorthi commented Dec 20, 2023

rluvaton commented Dec 20, 2023 • edited Loading

timorthi commented Dec 20, 2023

rluvaton left a comment • edited Loading

Choose a reason for hiding this comment

michaelwiles commented Dec 29, 2023

m33ch commented May 31, 2024

timorthi commented Apr 12, 2023 •

edited

Loading

coveralls commented Apr 12, 2023 •

edited

Loading

rluvaton left a comment •

edited

Loading

rluvaton commented Dec 20, 2023 •

edited

Loading

rluvaton left a comment •

edited

Loading