feat(cli): migration:rollup command #3633

mareksuscak · 2022-10-22T12:20:36Z

As we iterate on database schemas in feature branches, the number of database migrations tends to grows fairly quickly. With an ever-increasing number of database migrations, it takes increasingly more time to run these migrations in the CI/CD environment and locally too. This PR introduces a convenient way to roll up (or combine) a list of migrations into a single migration. This is somewhat inspired by a similar Doctrine command. This command could eventually be used to roll up DB migrations in non-local environments too.

TODO:

add documentation once the API is locked (pending review)

mareksuscak · 2022-10-22T12:28:58Z

packages/cli/src/commands/MigrationCommandFactory.ts

+    // for snapshots, we always want to use the path based on `emit` option, regardless of whether we run in ts-node context
+    /* istanbul ignore next */
+    const snapshotDir = options.emit === 'ts' && options.pathTs ? options.pathTs : options.path!;
+    const absoluteSnapshotPath = Utils.absolutePath(snapshotDir, orm.config.get('baseDir'));
+    const dbName = basename(orm.config.get('dbName'));
+    const snapshotName = options.snapshotName ?? `.snapshot-${dbName}`;
+    const snapshotPath = Utils.normalizePath(absoluteSnapshotPath, `${snapshotName}.json`);


My original intent was to use migrator.snapshotPath. However it seems to be a private property right now and is not a part of the IMigrator interface. Some refactoring may be necessary.

Alternatively, is there a way to bypass having to delete the snapshot in order to roll up multiple migrations that are reflected in the snapshot?

mareksuscak · 2022-10-22T12:29:35Z

packages/cli/src/commands/MigrationCommandFactory.ts

+    // Create the rollup
+    await this.handleCreateCommand(migrator, args, orm.config);
+
+    // Execute the rollup
+    await this.handleUpDownCommand(args, migrator, 'up');


I'm questioning if it's a good idea to use the other command handlers directly or if I should try to implement all logic directly at a lower level.

In addition, I'm questioning whether or not the rollup should be automatically executed. In order to stay consistent with the create command, it most likely should not.

mareksuscak · 2022-10-22T12:56:35Z

tests/features/cli/RollupCommand.test.ts

+    await expect(cmd.handler({} as any)).resolves.toBeUndefined();
+    expect(down.mock.calls.length).toBe(1);
+    expect(down.mock.calls[0][0]).toEqual({});
+    expect(closeSpy).toBeCalledTimes(1);
+    await expect(cmd.handler({ to: 'a' } as any)).resolves.toBeUndefined();
+    expect(down.mock.calls.length).toBe(2);
+    expect(down.mock.calls[1][0]).toEqual({ to: 'a' });
+    expect(closeSpy).toBeCalledTimes(2);


This test needs more work, it's mostly just a stub for now. How do you mock filesystem commands (e.g. remove and pathExists)?

codecov-commenter · 2022-10-22T20:57:11Z

Codecov Report

Base: 99.89% // Head: 99.88% // Decreases project coverage by -0.00% ⚠️

Coverage data is based on head (c5b5e00) compared to base (925c798).
Patch coverage: 95.23% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3633      +/-   ##
==========================================
- Coverage   99.89%   99.88%   -0.01%     
==========================================
  Files         211      211              
  Lines       13192    13213      +21     
  Branches     3059     3063       +4     
==========================================
+ Hits        13178    13198      +20     
- Misses         14       15       +1

Impacted Files	Coverage Δ
...ckages/cli/src/commands/MigrationCommandFactory.ts	`99.19% <95.23%> (-0.81%)`	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

mareksuscak · 2022-10-23T23:43:08Z

packages/cli/src/commands/MigrationCommandFactory.ts

+    }
+
+    // Delete migrations
+    // TODO(marek)


Since we need to delete migration files, it may make sense to not run the down command above but rather re-implement it so we get access to the list of migrations here.

PodaruDragos · 2023-02-03T18:10:54Z

This is actually a very nice feature to have. "collapsing" 10 migrations into 1 is something really useful down the line.
Is there no interest in this being considered / finished ? @mareksuscak @B4nan

B4nan · 2023-02-03T18:22:02Z

The idea is good, I am not entirely sure about the implementation. I don't like that it first migrates all the way down and up again, it could mess up with the data easily, e.g. every migration that created a column will first drop it and recreate again - but without data. This could even potentically fail if its non null without a default, as that works only on tables without any rows.

Why don't we just merge the contents of the migrations, instead of messing up with the database?

mareksuscak · 2023-02-03T19:52:34Z

I don't like that it first migrates all the way down and up again, it could mess up with the data easily, e.g. every migration that created a column will first drop it and recreate it again - but without data.

Totally. This was the only way I was able to get the rollup to work without writing a lot of custom logic. Also, you technically need to roll back to the first migration included in the rollup so you can do the diff and generate the rolled-up migration that contains all changes. Obviously, this wouldn't work if you wanted to run it in a non-local environment. This particular implementation is only good for rolling up migrations in feature branches as part of the final cleanup.

I was inspired by a similar feature in Doctrine (PHP world):
https://localheinz.com/articles/2020/06/10/rolling-up-database-migrations-with-doctrine/

I'd be happy to revisit this but was holding to get some feedback from the community and I also outlined some questions as I wasn't sure what's the best way to do certain things (SDK-wise)

B4nan · 2023-02-03T19:59:23Z

Also, you technically need to roll back

But why? You have the migrations, they contain the queries, why recompute everything instead of just concatenating?

mareksuscak · 2023-02-03T20:13:47Z

But why? You have the migrations, they contain the queries, why recompute everything instead of just concatenating?

You're not wrong. But consider you adding a new table, then adding a few more columns in the following migrations. If we just combine all migrations, there will be extraneous statements whereas it could be optimized to become a single statement. I know it's an optimization but there are other cases where you're experimenting with different states and so some migrations become unnecessary - which again - you could edit manually after the rollup. I'm impartial here and would be happy to explore the other approach which I did consider but again - it requires writing more custom logic - the solution contained in this PR only relies on existing commands.

B4nan · 2023-02-03T20:36:29Z

I'd go with concatenation, as it will be safer (no chance of data loss) as well as faster (no queries involved), and you don't need to touch the snapshot file or the schema state at all. I don't mind supporting the down/up way too, but rather as opt-in than default, could be a --recreate flag/option.

Btw don't be afraid of doing changes around the codebase, this does not have to be just a self-contained CLI command - if you need to modify the snapshot, you can add some methods to the migrator, we can mark them as internal via jsdoc if they need to be public.

Maybe better target the v6 branch nowadays, I would be up to merging something not fully complete in there too and polish it a bit myself before the final release (which will take a few more months for sure).

mareksuscak · 2023-02-17T10:26:47Z

@B4nan thank you for your feedback, Martin. I'll take a stab at this. It seems that I should be able to rely on the methods available in the Migration class that store all SQL statements in a string array and makes them available through getQueries so here's what I'm thinking in order of execution:

Determine the list of migrations to be concatenated
Dynamically import them and instantiate
Run up method and extract queries using getQueries - aggregate queries across all migrations in a new array
Run reset and then down method and extract queries using getQueries - aggregate queries across all migrations in a new array
Create a new migration file with all up and down queries concatenated
Delete the old migration files

How does that sound?

B4nan · 2023-02-17T22:20:50Z

Migrations can contain code that directly works with the database (e.g. this.execute() calls), so I would rather go with direct code manipulation instead. It might be a bit tricky to do right, though. But we need to preserve those calls.

mareksuscak · 2023-02-19T11:35:45Z

That's actually a great callout. I was trying to avoid having to use a JS/TS parser but I'm now not sure it's avoidable. I'd rather not rely on regex matching. If not, I'll have to use one of the available parsers (see the list below) in order to extract up and down functions' body. This will introduce a new dependency. Can you think of a different way to do this?

feat(cli): migration:rollup command

5401aed

mareksuscak commented Oct 22, 2022

View reviewed changes

mareksuscak added 2 commits October 22, 2022 14:44

fix(cli): rollup command argument passing

5b691b2

fix(cli): tests

c5b5e00

mareksuscak commented Oct 22, 2022

View reviewed changes

mareksuscak commented Oct 23, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cli): migration:rollup command #3633

feat(cli): migration:rollup command #3633

mareksuscak commented Oct 22, 2022 •

edited

mareksuscak Oct 22, 2022

mareksuscak Oct 22, 2022

mareksuscak Oct 22, 2022

mareksuscak Oct 22, 2022

mareksuscak Oct 22, 2022 •

edited

codecov-commenter commented Oct 22, 2022

mareksuscak Oct 23, 2022

PodaruDragos commented Feb 3, 2023

B4nan commented Feb 3, 2023 •

edited

mareksuscak commented Feb 3, 2023

B4nan commented Feb 3, 2023

mareksuscak commented Feb 3, 2023

B4nan commented Feb 3, 2023

mareksuscak commented Feb 17, 2023

B4nan commented Feb 17, 2023

mareksuscak commented Feb 19, 2023 •

edited

feat(cli): migration:rollup command #3633

Are you sure you want to change the base?

feat(cli): migration:rollup command #3633

Conversation

mareksuscak commented Oct 22, 2022 • edited

mareksuscak Oct 22, 2022

Choose a reason for hiding this comment

mareksuscak Oct 22, 2022

Choose a reason for hiding this comment

mareksuscak Oct 22, 2022

Choose a reason for hiding this comment

mareksuscak Oct 22, 2022

Choose a reason for hiding this comment

mareksuscak Oct 22, 2022 • edited

Choose a reason for hiding this comment

codecov-commenter commented Oct 22, 2022

Codecov Report

mareksuscak Oct 23, 2022

Choose a reason for hiding this comment

PodaruDragos commented Feb 3, 2023

B4nan commented Feb 3, 2023 • edited

mareksuscak commented Feb 3, 2023

B4nan commented Feb 3, 2023

mareksuscak commented Feb 3, 2023

B4nan commented Feb 3, 2023

mareksuscak commented Feb 17, 2023

B4nan commented Feb 17, 2023

mareksuscak commented Feb 19, 2023 • edited

mareksuscak commented Oct 22, 2022 •

edited

mareksuscak Oct 22, 2022 •

edited

B4nan commented Feb 3, 2023 •

edited

mareksuscak commented Feb 19, 2023 •

edited