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

Add Migrations class #180

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Roma-Kyrnis
Copy link

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

)`)

// Get the list of already applied migrations
let dbMigrations = await this.db.all(
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

Great!


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

Choose a reason for hiding this comment

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

probably add filename to the selection too

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

sure

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

Choose a reason for hiding this comment

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

I would just change this to lastMigrationId: number

Copy link
Collaborator

@theogravity theogravity Jan 22, 2024

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

Create a method on the class called

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

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

migration.id,
migration.name,
migration.up,
migration.down
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add migration.filename

)
}

// TODO: propose to make method GetInstance of the Migrations class, which will be async and we will call
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's save this TODO for another PR

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

Choose a reason for hiding this comment

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

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

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

@theogravity theogravity Jan 22, 2024

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

Sure!

@theogravity
Copy link
Collaborator

Thanks for this, this is a really great start.

@theogravity
Copy link
Collaborator

theogravity commented Jan 22, 2024

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

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

@theogravity theogravity Jan 22, 2024

Choose a reason for hiding this comment

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

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

Copy link
Author

@Roma-Kyrnis Roma-Kyrnis Jan 24, 2024

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

let's rename this to initMigration

`SELECT id, name, up, down FROM "${table}" ORDER BY id ASC`
)

await this.downMigrations(dbMigrations, migrations)
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

)`)

// Get the list of already applied migrations
let dbMigrations = await this.db.all(
Copy link
Collaborator

Choose a reason for hiding this comment

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

move the logic to protected getAppliedMigrations

@theogravity
Copy link
Collaborator

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

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

@Roma-Kyrnis Roma-Kyrnis Jan 24, 2024

Choose a reason for hiding this comment

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

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()

Copy link
Author

Choose a reason for hiding this comment

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

Logs are in three places:

  1. before await this.downMigrations(appliedMigrations, migrations)
  2. in downMigrations after appliedMigrations = appliedMigrations.filter(x => x.id !== migration.id)
  3. in upNewMigrations on the first line of the function

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

@Roma-Kyrnis
Copy link
Author

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()
  }
  1. It's the first variant to have "stable" migrations variable in the config to not call await this.readMigrations() on every down or up
  2. We can create static method to run const migrations = this.config.migrations ?? (await this.readMigrations()) asyncronously
  3. Get const migrations = this.config.migrations ?? (await this.readMigrations()) migrations variable on every up or down operation
  4. User can retrieve migrations using the readMigrations method and set migrations variable on class creation

@theogravity
Copy link
Collaborator

theogravity commented Jan 27, 2024

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
Copy link
Author

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
Copy link
Collaborator

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!

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

Successfully merging this pull request may close these issues.

2 participants