Conditionals in Migrations

[nikeee]

I'm building an application that uses SQLite in tests + local development and Postgres in production.

As stated in a comment (https://github.com/koskimas/kysely/issues/301#issuecomment-1403115097), Kysely intentionally doesn't do magic. This means that we need to explicitly support specific dialects. To do that, there must be multiple mugrations, for example:

Migration for Postgres:

export function up(db: Kysely<any>) {
  await db.schema
    .createTable('person')
    .addColumn('id', 'serial', (col) => col.primaryKey())
}

Migration for SQLite:

export function up(db: Kysely<any>) {
  await db.schema
    .createTable('person')
    .addColumn('id', 'integer', (col) => col.autoIncrement().primaryKey())
}

To keep migrations more in sync, it would be nice to have them in the same file. It would suffice to know the dialect name (or the instance of the dialect) of the database instance:

export function up(db: Kysely<any>) {
  if (db.dialect === somePostgresDialect) {
    await db.schema
      .createTable('person')
      .addColumn('id', 'serial', (col) => col.primaryKey())
  } else {
    await db.schema
      .createTable('person')
      .addColumn('id', 'integer', (col) => col.autoIncrement().primaryKey())
  }
}

While this would basically solve the problem, it may be interesting to adopt the $if syntax for that:

export function up(db: Kysely<any>) {
  const isPostgres = db.dialect === somePostgresDialect;

  await db.schema
      .createTable('person')
      .$if(isPostgres,
        s => s.addColumn('id', 'serial', (col) => col.primaryKey())
      )
      .$if(!isPostgres,
        s => s.addColumn('id', 'integer', (col) => col.autoIncrement().primaryKey())
      )
      .execute();
}

[igalklebanov]

Hey 👋

I'm building an application that uses SQLite in tests + local development and Postgres in production.

I think it's pointless testing against a different engine & driver. It just doesn't provide the required level of confidence a proper integration test is there for.

I think developing against a different engine just adds complexity, and might force you to opt for sub-optimal solutions for the sake of supporting 2 engines.

Unnecessary complexity and maintance overhead.

To keep migrations more in sync, it would be nice to have them in the same file. It would suffice to know the dialect name (or the instance of the dialect) of the database instance:

export function up(db: Kysely<any>) {
  if (db.dialect === somePostgresDialect) {
    await db.schema
      .createTable('person')
      .addColumn('id', 'serial', (col) => col.primaryKey())
  } else {
    await db.schema
      .createTable('person')
      .addColumn('id', 'integer', (col) => col.autoIncrement().primaryKey())
  }
}

While this would basically solve the problem, it may be interesting to adopt the $if syntax for that:

export function up(db: Kysely<any>) {
  const isPostgres = db.dialect === somePostgresDialect;

  await db.schema
      .createTable('person')
      .$if(isPostgres,
        s => s.addColumn('id', 'serial', (col) => col.primaryKey())
      )
      .$if(!isPostgres,
        s => s.addColumn('id', 'integer', (col) => col.autoIncrement().primaryKey())
      )
      .execute();
}

Past instance creation, Kysely is dialect agnostic by design.

You could simply check for process.env.ENV, 'test'/'local' vs. 'prod'. Or am I missing something?

There's no need for .$if either. a. You always add an id column. b. In contrast to DML query builders, Kysely doesn't care what you do here.

export function up(db: Kysely<any>) {
  await db.schema
    .createTable('person')
    .addColumn(
      'id', 
      condition ? 'serial' : 'integer', 
      (col) => condition ? col.primaryKey() : col.autoIncrement().primaryKey()
    )
    .execute();
}

This is perfectly fine.

[koskimas]

I agree with @igalklebanov. $if should only be used when a normal if doesn't do the job i.e. with conditional selects. We shouldn't promote the method's usage in other contexts as it complicates the types unnecessarily when used.

I also agree that writing your code to work with multiple databases "just in case" is a bad practice and should be avoided when possible. Writing tests using a different DB is just asking for trouble.

[nikeee]

Thanks for your feedback!

This is perfectly fine.

For that case, it works. However, when using other features that require different syntax/concepts, that does not work.

You could simply check for process.env.ENV, 'test'/'local' vs. 'prod'. Or am I missing something?

Maybe I took the rule that says that migrations have to be "frozen in time and shouldn't depend on outside code" too seriously. The env vars (or their meanings) could change. However, you're probably right here.

I also agree that writing your code to work with multiple databases "just in case" is a bad practice and should be avoided when possible. Writing tests using a different DB is just asking for trouble.

I agree on that. But it's not "just in case". The initial intention to use SQLite in tests is that firing up 100 instances of postgres when running the tests in parallel is not a viable solution either. But I agree, the solution to this problem would be to use a single DB technology or a query builder that translates concepts across DBs.

[koskimas]

firing up 100 instances of postgres when running the tests in parallel is not a viable solution either.

You could create a separate database for each parallel tester. Postgres can easily handle that. Running tests with more parallelism than cores in the machine won't do you much good either. Unless you have 100 cores, using 100 parallel runners will just slow things down. Unless your tests have a lot of sleeping and waiting for stuff of course.

[nikeee]

Of course, that was kinda metaphorically. On a dev machine, this would be more like 24. But even then, the tests would most likely spend the most time in setting up or starting a database. In-memory SQLite is hard to beat in that regard. But that's unrelated to this issue.