Add Migrations class #180

Roma-Kyrnis · 2024-01-21T17:01:48Z

For this, I needed to

bump the version of the prettier-standard
add optional property filename to IMigrate.MigrationData

Other changes happen because I am supposed to the prettier-standard version bump or because of pre-commit commands

…e comments from migrations

…s class

theogravity · 2024-01-22T02:26:06Z

src/Migrations.ts

+    )`)
+
+    // Get the list of already applied migrations
+    let dbMigrations = await this.db.all(


The typing might be better with this.db.all<MigrationData[]>

Rename the variable to appliedMigrations so its easier to tell what the variable is used for

theogravity · 2024-01-22T02:26:19Z

src/Migrations.ts

+
+    // Get the list of already applied migrations
+    let dbMigrations = await this.db.all(
+      `SELECT id, name, up, down FROM "${table}" ORDER BY id ASC`


probably add filename to the selection too

I've added a filename to CRuD methods. And I moved methods to functions. Maybe we can move them to another class, but I think we don't need this complexity now.

But it's good to move them to external functions, IMHO, because we can see all operations to the database in less than 50 lines. It gives us the opportunity to update every request to DB

theogravity · 2024-01-22T02:28:57Z

src/Migrations.ts

+   * @param migrations IMigrate.MigrationData
+   */
+  private async upNewMigrations (
+    dbMigrations: any[],


I would just change this to lastMigrationId: number

Thinking about it, we should apply the same method as we're doing with the down one, so let's break up this method into:

migrateToLatest

targetedUpMigration

migrateToLatest would call this under the hood for each item to migrate

Is it downMigrations function is executing for every new migration in the list? So it's not executing only one.

It's a good point to make up and down operations possible to execute only one or all of the migrations. Do you propose to make this change? Do we need to add it in this PR?

Okay, I've read the point below! I'll update both up and down

I don't think down should do all migrations, it's usually the last or a targeted migration to undo

We should have the following:

a method to migrate to latest (up)

a method to migrate up a targeted file

a method to undo the last migration (down)

a method to migrate down a targeted file

theogravity · 2024-01-22T02:31:33Z

src/Migrations.ts

+    dbMigrations: any[],
+    migrations: readonly MigrationData[]
+  ): Promise<void> {
+    const lastMigrationId = dbMigrations.length


Create a method on the class called

protected getLastMigrationId(dbMigrations: MigrationData[]) and move the logic to it

Okay. For now, I'll do it. Waiting for the above response)

I'll update private statement with protected because everyone who will extends his class can use our internal methods

theogravity · 2024-01-22T02:32:21Z

src/Migrations.ts

+            migration.id,
+            migration.name,
+            migration.up,
+            migration.down


Add migration.filename

theogravity · 2024-01-22T02:39:13Z

src/Migrations.ts

+    )
+  }
+
+  // TODO: propose to make method GetInstance of the Migrations class, which will be async and we will call


Let's save this TODO for another PR

theogravity · 2024-01-22T02:46:43Z

src/Migrations.ts

+   * @param migrations IMigrate.MigrationData
+   */
+  private async downMigrations (
+    dbMigrations: any[],


rename this to appliedMigrations: MigrationData[] so it's easier to tell what this is for

theogravity · 2024-01-22T02:57:13Z

src/Migrations.ts

+   * @param dbMigrations exists migrations in the `table`
+   * @param migrations IMigrate.MigrationData
+   */
+  private async downMigrations (


This method is a bit strange to me. It looks like it does two things:

undo last migration

undo migrations that don't match the migration files

which is really weird to me - what's the use-case for this? I can see things going wrong if someone used this.

I think you should do the following:

Remove this method

Create a method called undoLastMigration

Create a method called targetedUndoMigration (so you can undo a specific migration id / file)

undoLastMigration would call targetedUndoMigration under the hood

If you want to do your undo delta, then you can use targetedUndoMigration in your own script that uses this class

theogravity · 2024-01-22T03:01:01Z

Thanks for this, this is a really great start.

theogravity · 2024-01-22T03:02:04Z

You should be able to delete the migrate.ts file and in the Database class, you can do:

  async migrate (config?: MigrationParams) {
    const migration = new Migrations(...)
    // call migrate to latest
  }

Edit: Since we're doing a major version bump, we can introduce breaking changes.

Let's remove that method entirely, and we'll direct users to use the new class instead

theogravity · 2024-01-22T03:02:38Z

package.json

@@ -1,6 +1,6 @@
 {
  "name": "sqlite",
-  "version": "5.1.1",


Let's make it a major version bump to 6.0.0 since we're going to remove the old migrations.ts file and introduce your new class instead

Okay. Why have you decided to change your mind?)

we're removing the old migrate.ts file which someone may use directly and replacing it with this class

theogravity · 2024-01-22T03:14:02Z

src/Migrations.ts

+   * in the the MigrationData if `readMigrations` function executes or user can put filename
+   * for each migration in the config.migrations
+   */
+  async migrate (): Promise<void> {


let's rename this to initMigration

theogravity · 2024-01-22T03:14:20Z

src/Migrations.ts

+      `SELECT id, name, up, down FROM "${table}" ORDER BY id ASC`
+    )
+
+    await this.downMigrations(dbMigrations, migrations)


Remove these two calls so this fn does one specific thing - just inits the tables

theogravity · 2024-01-22T03:15:12Z

src/Migrations.ts

+    )`)
+
+    // Get the list of already applied migrations
+    let dbMigrations = await this.db.all(


move the logic to protected getAppliedMigrations

theogravity · 2024-01-22T03:22:20Z

Ok, I've made all the comments for the first round. Looking forward to the changes!

Roma-Kyrnis · 2024-01-24T12:43:13Z

src/Migrations.ts

+            migration.id
+          )
+          await this.db.run('COMMIT')
+          dbMigrations = dbMigrations.filter(x => x.id !== migration.id)


Why do we need this one?
As I understand it'll remove downed migrations and pass migrations that exists to the upMigrations function

But I tried to run this code and got:

Logs

npx ts-node-dev test.ts 14:36:46 ∞ [INFO] 14:36:55 ts-node-dev ver. 1.1.8 (using ts-node ver. 9.1.1, typescript ver. 4.6.3) [ { id: 1, name: 'initial', up: 'CREATE TABLE Category (\n' + ' id INTEGER PRIMARY KEY,\n' + ' name TEXT NOT NULL\n' + ');\n' + '\n' + 'CREATE TABLE Post (\n' + ' id INTEGER PRIMARY KEY,\n' + ' categoryId INTEGER NOT NULL,\n' + ' title TEXT NOT NULL,\n' + ' isPublished NUMERIC NOT NULL DEFAULT 0,\n' + ' CONSTRAINT Post_fk_categoryId FOREIGN KEY (categoryId)\n' + ' REFERENCES Category (id) ON UPDATE CASCADE ON DELETE CASCADE,\n' + ' CONSTRAINT Post_ck_isPublished CHECK (isPublished IN (0, 1))\n' + ');\n' + '\n' + 'CREATE INDEX Post_ix_categoryId ON Post (categoryId);\n' + '\n' + "INSERT INTO Category (id, name) VALUES (1, 'Test');", down: 'DROP INDEX Post_ix_categoryId;\nDROP TABLE Post;\nDROP TABLE Category;' }, { id: 2, name: 'some-feature', up: 'CREATE TABLE Test (\n' + ' id INTEGER PRIMARY KEY,\n' + ' name TEXT NOT NULL\n' + ');', down: 'DROP TABLE Test;' }, { id: 3, name: 'test-cert', up: 'CREATE TABLE whatever ( certificate TEXT );\n' + 'INSERT INTO whatever ( certificate ) VALUES (\n' + " '-----BEGIN CERTIFICATE-----\n" + 'some contents\n' + "-----END CERTIFICATE-----');", down: 'DROP TABLE whatever;' }, { id: 4, name: 'no-down', up: 'CREATE TABLE IF NOT EXISTS downless ( value TEXT );\n' + "INSERT INTO downless ( value ) VALUES ('down migration is optional');", down: '' } ] [ { id: 1, name: 'initial', up: 'CREATE TABLE Category (\n' + ' id INTEGER PRIMARY KEY,\n' + ' name TEXT NOT NULL\n' + ');\n' + '\n' + 'CREATE TABLE Post (\n' + ' id INTEGER PRIMARY KEY,\n' + ' categoryId INTEGER NOT NULL,\n' + ' title TEXT NOT NULL,\n' + ' isPublished NUMERIC NOT NULL DEFAULT 0,\n' + ' CONSTRAINT Post_fk_categoryId FOREIGN KEY (categoryId)\n' + ' REFERENCES Category (id) ON UPDATE CASCADE ON DELETE CASCADE,\n' + ' CONSTRAINT Post_ck_isPublished CHECK (isPublished IN (0, 1))\n' + ');\n' + '\n' + 'CREATE INDEX Post_ix_categoryId ON Post (categoryId);\n' + '\n' + "INSERT INTO Category (id, name) VALUES (1, 'Test');", down: 'DROP INDEX Post_ix_categoryId;\nDROP TABLE Post;\nDROP TABLE Category;' }, { id: 2, name: 'some-feature', up: 'CREATE TABLE Test (\n' + ' id INTEGER PRIMARY KEY,\n' + ' name TEXT NOT NULL\n' + ');', down: 'DROP TABLE Test;' }, { id: 3, name: 'test-cert', up: 'CREATE TABLE whatever ( certificate TEXT );\n' + 'INSERT INTO whatever ( certificate ) VALUES (\n' + " '-----BEGIN CERTIFICATE-----\n" + 'some contents\n' + "-----END CERTIFICATE-----');", down: 'DROP TABLE whatever;' } ] [ { id: 1, name: 'initial', up: 'CREATE TABLE Category (\n' + ' id INTEGER PRIMARY KEY,\n' + ' name TEXT NOT NULL\n' + ');\n' + '\n' + 'CREATE TABLE Post (\n' + ' id INTEGER PRIMARY KEY,\n' + ' categoryId INTEGER NOT NULL,\n' + ' title TEXT NOT NULL,\n' + ' isPublished NUMERIC NOT NULL DEFAULT 0,\n' + ' CONSTRAINT Post_fk_categoryId FOREIGN KEY (categoryId)\n' + ' REFERENCES Category (id) ON UPDATE CASCADE ON DELETE CASCADE,\n' + ' CONSTRAINT Post_ck_isPublished CHECK (isPublished IN (0, 1))\n' + ');\n' + '\n' + 'CREATE INDEX Post_ix_categoryId ON Post (categoryId);\n' + '\n' + "INSERT INTO Category (id, name) VALUES (1, 'Test');", down: 'DROP INDEX Post_ix_categoryId;\nDROP TABLE Post;\nDROP TABLE Category;' }, { id: 2, name: 'some-feature', up: 'CREATE TABLE Test (\n' + ' id INTEGER PRIMARY KEY,\n' + ' name TEXT NOT NULL\n' + ');', down: 'DROP TABLE Test;' }, { id: 3, name: 'test-cert', up: 'CREATE TABLE whatever ( certificate TEXT );\n' + 'INSERT INTO whatever ( certificate ) VALUES (\n' + " '-----BEGIN CERTIFICATE-----\n" + 'some contents\n' + "-----END CERTIFICATE-----');", down: 'DROP TABLE whatever;' }, { id: 4, name: 'no-down', up: 'CREATE TABLE IF NOT EXISTS downless ( value TEXT );\n' + "INSERT INTO downless ( value ) VALUES ('down migration is optional');", down: '' } ]

test.ts

import * as sqlite3 from 'sqlite3' import { Database } from "./src"; import { Migrations } from "./src/Migrations"; const boot = async () => { const db = new Database({ filename: '.db', driver: sqlite3.Database, }) await db.open() const instance = new Migrations(db, {force: true}) await instance.migrate(); } boot()

Logs are in three places:

before await this.downMigrations(appliedMigrations, migrations)

in downMigrations after appliedMigrations = appliedMigrations.filter(x => x.id !== migration.id)

in upNewMigrations on the first line of the function

tests may have to be altered / rewritten / added as the new methods are created

…s to appliedMigrations

Roma-Kyrnis · 2024-01-24T14:57:33Z

So If we are making breaking changes. I have an idea to discuss with you:
Our migrate - initMigration function will look like:

async migrate (): Promise<void> {
    const migrations = this.config.migrations ?? (await this.readMigrations())
    this.config.migrations = migrations

    // Create a database table for migrations meta data if it doesn't exist
    await this.createMigrationsTable()
  }

It's the first variant to have "stable" migrations variable in the config to not call await this.readMigrations() on every down or up
We can create static method to run const migrations = this.config.migrations ?? (await this.readMigrations()) asyncronously
Get const migrations = this.config.migrations ?? (await this.readMigrations()) migrations variable on every up or down operation
User can retrieve migrations using the readMigrations method and set migrations variable on class creation

theogravity · 2024-01-27T23:28:35Z

We shouldn't make this.config.migrations part of the Database constructor config

createMigrationsTable shouldn't be part of Database - it's specifically a migration method and should be in the migration class instead

if you need to do some kind of initialization, then add a protected async init() method to your migration class

If there are async calls that must be done when the migration class is created, then do the following:

Make the constructor of the migration class private
Create a static method on the migration class called static async initMigration(config) that does:
- Creates a new instance of the migration class, passing the config to it
- Then calls the async init() method to initialize
- Returns the new instance of the migration class

You can see the pattern here:

https://stackoverflow.com/questions/51134172/what-is-the-usage-of-private-and-protected-constructors-in-typescript

Roma-Kyrnis · 2024-01-28T14:04:23Z

Hi Theo, I'll go tomorrow to the base training. I'll not have a laptop with me for 1-2 months. I'll push in 1-2 hours last updates before I leave.

May I continue to develop Migrations later?

It'll be great if someone wants to bring his impact here! If not I'll finish development later

theogravity · 2024-01-28T18:47:11Z

Hi Theo, I'll go tomorrow to the base training. I'll not have a laptop with me for 1-2 months. I'll push in 1-2 hours last updates before I leave.

May I continue to develop Migrations later?

It'll be great if someone wants to bring his impact here! If not I'll finish development later

Take as much time as needed!

Roma-Kyrnis added 3 commits January 4, 2024 22:51

feat: add import readMigrations function in the Database class; remov…

b6294e9

…e comments from migrations

update prettier-standart for optional chains on object; add Migration…

8ae768b

…s class

remove changes from utils/migrate.ts

809598d

Roma-Kyrnis mentioned this pull request Jan 21, 2024

Add import readMigrations function in the Database class #179

Open

theogravity reviewed Jan 22, 2024

View reviewed changes

src/Migrations.ts Outdated

migration.id,

migration.name,

migration.up,

migration.down

Copy link

Collaborator

theogravity Jan 22, 2024

Choose a reason for hiding this comment

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

Add migration.filename

theogravity reviewed Jan 22, 2024

View reviewed changes

Roma-Kyrnis commented Jan 24, 2024

View reviewed changes

move database request functions to the methods; update types and name…

ec170b2

…s to appliedMigrations

Add Migrations class #180

Are you sure you want to change the base?

Add Migrations class #180

Conversation

Roma-Kyrnis commented Jan 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theogravity Jan 22, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theogravity Jan 22, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theogravity commented Jan 22, 2024

theogravity commented Jan 22, 2024 • edited

theogravity Jan 22, 2024 • edited

Choose a reason for hiding this comment

Roma-Kyrnis Jan 24, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theogravity commented Jan 22, 2024

Roma-Kyrnis Jan 24, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Roma-Kyrnis commented Jan 24, 2024

theogravity commented Jan 27, 2024 • edited

Roma-Kyrnis commented Jan 28, 2024

theogravity commented Jan 28, 2024

theogravity Jan 22, 2024 •

edited

theogravity Jan 22, 2024 •

edited

theogravity commented Jan 22, 2024 •

edited

theogravity Jan 22, 2024 •

edited

Roma-Kyrnis Jan 24, 2024 •

edited

Roma-Kyrnis Jan 24, 2024 •

edited

theogravity commented Jan 27, 2024 •

edited