Skip to content

Add import readMigrations function in the Database class #179

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion jest.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@ module.exports = {
testPathIgnorePatterns: [
'/build',
'/node_modules/',
'/vendor-typings'
'/vendor-typings',
'__tests__/data'
],
coverageThreshold: {
global: {
Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "sqlite",
"version": "5.1.1",
"version": "5.1.2",
"description": "SQLite client for Node.js applications with SQL-based migrations API written in Typescript",
"main": "build/index.js",
"types": "build/index.d.ts",
Expand Down
11 changes: 10 additions & 1 deletion src/Database.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import * as sqlite3 from 'sqlite3'
import { ISqlite, IMigrate } from './interfaces'

import { Statement } from './Statement'
import { migrate } from './utils/migrate'
import { migrate, readMigrations } from './utils/migrate'
import { toSqlParams } from './utils/strings'

import MigrationParams = IMigrate.MigrationParams
Expand Down Expand Up @@ -365,6 +365,15 @@ export class Database<
await migrate(this, config)
}

/**
* Performs a database migration read operation.
* @returns List of objects with name, id of migrations and SQL code for up and down migration
* You can run up and down with `db.exec(migrations[0].up)` or `db.exec(migrations[0].down)`. Or with other commands that can run SQL code.
*/
readMigrations (migrationPath?: string): Promise<IMigrate.MigrationData[]> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the intent to just read a list of migrations and then execute them?

It almost feels like this should belong in its own Migrations class and the constructor would accept a Database instance

then you can have a method for just reading the list of migrations / status

and then have another method(s) for executing up / down based on the list of migrations passed through

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I understand that it's better. But it'll take some time. So let's me finish my tasks and I plan to make this changes on the next week

Changes:

  • add Migration class
  • add run, show, reverse migration functions
  • add a commander to run these functions to look on the migrations
  • add config options where to look for Database instance or config options. With default behaviour. But I plan to add config as a parameter in the command line

WDYT?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The overall idea sounds good. I'd have to see the impl / interface to have additional thoughts

Thanks for doing this. We could also probably move the migration logic to the migration class too.

The Database instance can still keep the migrate method for backwards-compat but would call the new Migration class

Copy link
Author

Choose a reason for hiding this comment

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

Hi, I replied a little later but I have time to make changes)

return readMigrations(migrationPath)
}

/**
* The methods underneath requires creative work to implement. PRs / proposals accepted!
*/
Expand Down
45 changes: 45 additions & 0 deletions src/__tests__/data/migrations.data.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
export const initial001 = {
up: `CREATE TABLE Category (
id INTEGER PRIMARY KEY,
name TEXT NOT NULL
);

CREATE TABLE Post (
id INTEGER PRIMARY KEY,
categoryId INTEGER NOT NULL,
title TEXT NOT NULL,
isPublished NUMERIC NOT NULL DEFAULT 0,
CONSTRAINT Post_fk_categoryId FOREIGN KEY (categoryId)
REFERENCES Category (id) ON UPDATE CASCADE ON DELETE CASCADE,
CONSTRAINT Post_ck_isPublished CHECK (isPublished IN (0, 1))
);

CREATE INDEX Post_ix_categoryId ON Post (categoryId);

INSERT INTO Category (id, name) VALUES (1, 'Test');`,
down: `DROP INDEX Post_ix_categoryId;
DROP TABLE Post;
DROP TABLE Category;`
}

export const someFeature002 = {
up: `CREATE TABLE Test (
id INTEGER PRIMARY KEY,
name TEXT NOT NULL
);`,
down: `DROP TABLE Test;`
}

export const testCert003 = {
up: `CREATE TABLE whatever ( certificate TEXT );
INSERT INTO whatever ( certificate ) VALUES (
'-----BEGIN CERTIFICATE-----
some contents
-----END CERTIFICATE-----');`,
down: `DROP TABLE whatever;`
}

export const noDown004 = {
up: `CREATE TABLE IF NOT EXISTS downless ( value TEXT );
INSERT INTO downless ( value ) VALUES ('down migration is optional');`
}
42 changes: 42 additions & 0 deletions src/__tests__/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,13 @@
import { open } from '..'
import * as sqlite3 from 'sqlite3'

import {
initial001,
noDown004,
someFeature002,
testCert003
} from './data/migrations.data'

describe('index', () => {
// enable the sqlite cached database or not
const driver = [
Expand Down Expand Up @@ -40,6 +47,41 @@ describe('index', () => {
})
})

driver.forEach(c => {
it(`should return list of migrations with name, id, down and up variables, cached = ${
c.cached
}`, async () => {
// const expectedData =
const db = await open({
filename: ':memory:',
driver: c.driver
})

await db.migrate()

const migrations = await db.readMigrations()

expect(migrations).toEqual([
{ id: 1, name: 'initial', up: initial001.up, down: initial001.down },
{
id: 2,
name: 'some-feature',
up: someFeature002.up,
down: someFeature002.down
},
{
id: 3,
name: 'test-cert',
up: testCert003.up,
down: testCert003.down
},
{ id: 4, name: 'no-down', up: noDown004.up, down: '' }
])

await db.close()
})
})

/* disabled since sqlite3-offline-next hasn't been maintained in two years
and there are no recent builds compatible with newer versions of node
it('should allow an anonymous database', async () => {
Expand Down
5 changes: 4 additions & 1 deletion src/utils/migrate.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,11 @@ export async function readMigrations (migrationPath?: string) {
return reject(err)
}

const [up, down] = data.split(/^--\s+?down\b/im)
const [up, down] = data
.split(/^--\s+?down\b/im)
.map(value => value.replace(/^-{3,}$/gim, '').trim())

delete migration.filename
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why remove it?

Copy link
Author

Choose a reason for hiding this comment

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

For the types consistency.

Do you want to save all parameters from migration that may be add in the future?

So maybe we will make another type in Migration class for the readMigrations function and in the migrate function we will just require needed arguments but not the exact object as MigrationData.

What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

That sounds appropriate

const migrationData = migration as Partial<MigrationData>
migrationData.up = up.replace(/^-- .*?$/gm, '').trim() // Remove comments
migrationData.down = down ? down.trim() : '' // and trim whitespaces
Expand Down