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

Unable to perform migrations for sqlite. Foreign key constraint error #2800

Closed
michaelgambold opened this issue Feb 20, 2022 · 9 comments
Closed

Comments

@michaelgambold
Copy link

Describe the bug
Cannot apply migrations to sqlite database

Stack trace

MigrationError: Migration Migration20220220065100 (up) failed: Original error: DROP TABLE "door"; - SQLITE_CONSTRAINT_FOREIGNKEY: FOREIGN KEY constraint failed
    at /Users/michael/Source/.../node_modules/umzug/src/umzug.ts:267:12
    at async Umzug.runCommand (/Users/michael/Source/.../node_modules/umzug/src/umzug.ts:216:11)
    at async Migrator.runInTransaction (/Users/michael/Source/.../node_modules/@mikro-orm/migrations/Migrator.js:308:21)
    at async SqliteConnection.transactional (/Users/michael/Source/.../node_modules/@mikro-orm/knex/AbstractSqlConnection.js:36:25)
    at async Function.handleUpDownCommand (/Users/michael/Source/.../node_modules/@mikro-orm/cli/commands/MigrationCommandFactory.js:118:9)
    at async Function.handleMigrationCommand (/Users/michael/Source/.../node_modules/@mikro-orm/cli/commands/MigrationCommandFactory.js:103:17) {
  jse_shortmsg: 'Migration Migration20220220065100 (up) failed',
  jse_cause: VError: Original error: DROP TABLE "door"; - SQLITE_CONSTRAINT_FOREIGNKEY: FOREIGN KEY constraint failed
      at Function.wrap (/Users/michael/Source/.../node_modules/umzug/src/umzug.ts:37:11)
      at new MigrationError (/Users/michael/Source/.../node_modules/umzug/src/umzug.ts:54:24)
      at /Users/michael/Source/.../node_modules/umzug/src/umzug.ts:267:12
      at async Umzug.runCommand (/Users/michael/Source/.../node_modules/umzug/src/umzug.ts:216:11)
      at async Migrator.runInTransaction (/Users/michael/Source/.../node_modules/@mikro-orm/migrations/Migrator.js:308:21)
      at async SqliteConnection.transactional (/Users/michael/Source/.../node_modules/@mikro-orm/knex/AbstractSqlConnection.js:36:25)
      at async Function.handleUpDownCommand (/Users/michael/Source/.../node_modules/@mikro-orm/cli/commands/MigrationCommandFactory.js:118:9)
      at async Function.handleMigrationCommand (/Users/michael/Source/.../node_modules/@mikro-orm/cli/commands/MigrationCommandFactory.js:103:17) {
    jse_shortmsg: 'Original error',
    jse_cause: DriverException: DROP TABLE "door"; - SQLITE_CONSTRAINT_FOREIGNKEY: FOREIGN KEY constraint failed
        at SqliteExceptionConverter.convertException (/Users/michael/Source/.../node_modules/@mikro-orm/core/platforms/ExceptionConverter.js:8:16)
        at SqliteExceptionConverter.convertException (/Users/michael/Source/.../node_modules/@mikro-orm/sqlite/SqliteExceptionConverter.js:46:22)
        at SqliteDriver.convertException (/Users/michael/Source/.../node_modules/@mikro-orm/core/drivers/DatabaseDriver.js:180:54)
        at /Users/michael/Source/.../node_modules/@mikro-orm/core/drivers/DatabaseDriver.js:184:24
        at async Function.runSerial (/Users/michael/Source/.../node_modules/@mikro-orm/core/utils/Utils.js:535:22)
        at async connection.transactional.ctx (/Users/michael/Source/.../node_modules/@mikro-orm/migrations/MigrationRunner.js:23:17)
        at async SqliteConnection.transactional (/Users/michael/Source/.../node_modules/@mikro-orm/knex/AbstractSqlConnection.js:36:25)
        at async MigrationRunner.run (/Users/michael/Source/.../node_modules/@mikro-orm/migrations/MigrationRunner.js:20:13)
        at async createMigrationHandler (/Users/michael/Source/.../node_modules/@mikro-orm/migrations/Migrator.js:198:13)
        at async /Users/michael/Source/.../node_modules/umzug/src/umzug.ts:265:6
    
    previous Error: DROP TABLE "door"; - SQLITE_CONSTRAINT_FOREIGNKEY: FOREIGN KEY constraint failed {
      errno: 787,
      code: 'SQLITE_CONSTRAINT_FOREIGNKEY'
    },
    jse_info: {},
    cause: [Function: ve_cause]
  },
  jse_info: {
    direction: 'up',
    name: 'Migration20220220065100',
    path: '/Users/michael/Source/.../src/migrations/Migration20220220065100.ts',
    context: {}
  },
  cause: [Function: ve_cause]
}

To Reproduce
Steps to reproduce the behavior:

  1. Create a schema that has two tables with a one -> many relationship
  2. Seed data to both tables
  3. Add a single property to the "many" table. I.e. the one with the foreign key to the "one" table
  4. Create a new migration
  5. Try and "up" the migration

Expected behavior
Migration should succeed, data should still be in table and new table should be set.

Additional context
This is the migration that is generated

import { Migration } from '@mikro-orm/migrations';

export class Migration20220220065100 extends Migration {

  async up(): Promise<void> {
    this.addSql('PRAGMA foreign_keys = OFF;');
    this.addSql('CREATE TABLE `_knex_temp_alter206` (`id` integer NOT NULL, `label` text NOT NULL, `is_enabled` integer NOT NULL DEFAULT true, `state` text NOT NULL, PRIMARY KEY (`id`));');
    this.addSql('INSERT INTO "_knex_temp_alter206" SELECT * FROM "door";;');
    this.addSql('DROP TABLE "door";');
    this.addSql('ALTER TABLE "_knex_temp_alter206" RENAME TO "door";');
    this.addSql('PRAGMA foreign_keys = ON;');

    this.addSql('alter table `sequence` add column `another_property` text null;');
  }

}

I exported the sql and ran it directly on the sqlite database and it worked successfully. It's almost like it's not setting the ignore foreign key or something??

What's also a little strange is I'm not sure why the migration which only adds a column to sequence table (the many) drops and recreates the door table (the one)?

I've played around with the dropTables, disableForeignKeys, transactional, and allOrNothing to see if it makes a difference and it doesn't. I have also tried running the "up" via cli and in code and the result is the same.

Versions

Dependency Version
node 16.14.0
typescript 4.3.5
mikro-orm 5.0.1
your-driver sqlite
@B4nan
Copy link
Member

B4nan commented Feb 20, 2022

I need better reproduction for this, what you described is already in tests. Please provide at least the entity definition, and be sure to upgrade to latest before reporting things.

What's also a little strange is I'm not sure why the migration which only adds a column to sequence table (the many) drops and recreates the door table (the one)?

That's mostly knex doing this magic, unfortunately schema alters in sqlite are quite problematic due to lots of limitations with FKs, so using this temp table is usually the only way to do more complex alters.

@michaelgambold
Copy link
Author

I did some more digging and found the answer and thought I'd document it for all that follow. Sorry for the wall of text lol.

Turns out it's a little of not understanding the migrations fully, the migrations getting it what I would call a bad state and not reading the documentation fully.

When I created the first migration I used the npx mikro-orm migration:create --initial command as per the documentation. What is interesting here is that there is no snapshot file created.

I then proceeded to put data into the database and use it as normal with no issues.

Time came to add another column to the data and I did this and then ran the npx mikro-orm migration:create command to create the migration script to add the extra column. This is where things get interesting (and frustrating lol). What happened is that with the default settings a snapshot file is created at the point of the second migration (not the first). So the second migration seems not to know about the first and looks like it tries then to re-create the whole schema as it doesn't know what has been applied (hence the drop table).

At this point it makes sense that it's failing as the "many" table won't let the "one" table be dropped because of the foreign key relationship. This is why I say it allows you to get into a bad state as you will never be able to get any successful migrations as far as I understand what's going on.

For a sanity check I removed all the migrations dropped the DB to start the migrations from scratch. I ran npx mikro-orm migration:create (without the --initial) for my first migration and it created the snapshot right away with the full schema. I then added the extra column and performed the second migration (I've included it below). As you can see it only includes the extra column as expected (the other drop table was not included).

import { Migration } from '@mikro-orm/migrations';

export class Migration20220220100614 extends Migration {

  async up(): Promise<void> {
    this.addSql('alter table `sequence` add column `another_property` text null;');
  }

}

As for my not reading the docs perfectly I overlooked how important the snapshot was for the migrations as the --initial didn't create one. It's worth re-iterating that one the docs say

Creating new migration will automatically save the target schema snapshot into migrations folder

However I've verified this is not the case for --initial. Not having it allowed the migrations to get into a state that would never succeed.

image

Anyway thanks for getting back to me so quick and I have to say although the documentation for using mikro-orm with nestjs (both frameworks docs for integrating are equally missing the finer details), mikro-orm has been pretty good to use and will keep it in mind for future projects.

I won't close this as I think it's important that someone reads it first to understand how I got it into this state but feel free to close it off as I've fixed the issue now. :)

@B4nan
Copy link
Member

B4nan commented Feb 20, 2022

The initial flag is optional, you need it only for a specific use case where you already have your schema as well as your entities. Like the docs says:

If we want to start using migrations, and we already have the schema generated, we can do so by creating so called initial migration:

It is indeed a bug that it won't generate the snapshot. But this should still not really happen, I would still like to see your entity definition as there should be no alter queries even without the snapshot (I say alter but for sqlite it ends up with temp tables and recreating).

@michaelgambold
Copy link
Author

Re-reading that section I feel like it could be better written to make that the --initial is optional and that if you have generated a schema (via schema:create) do it this way otherwise do it this other way. In fact that section does not indicate it is optional.

I've included my entity definitions below. The second migration is just uncommenting the property anotherProperty from the Sequence entity

--- DOOR (THE ONE RELATION)

import {
  Collection,
  Entity,
  OneToMany,
  PrimaryKey,
  Property,
  QueryOrder,
} from '@mikro-orm/core';
import { Sequence } from './Sequence.entity';

@Entity()
export class Door {
  @PrimaryKey({ autoincrement: false })
  id: number;

  @Property()
  label: string;

  @Property({ default: true })
  isEnabled: boolean;

  @Property()
  state: 'open' | 'closed';

  @OneToMany(() => Sequence, (sequence) => sequence.door, {
    orderBy: { index: QueryOrder.ASC },
  })
  sequences = new Collection<Sequence>(this);
}


--- SEQUENCE (THE MANY RELATION)

import { Entity, ManyToOne, PrimaryKey, Property } from '@mikro-orm/core';
import { Door } from './Door.entity';

@Entity()
export class Sequence {
  @PrimaryKey()
  id!: number;

  @Property()
  index: number;

  @Property()
  action: 'on' | 'off' | 'low' | 'high';

  // @Property()
  // anotherProperty?: string;

  @Property()
  target:
    | 'relay1'
    | 'relay2'
    | 'relay3'
    | 'digitalOutput1'
    | 'digitalOutput2'
    | 'digitalOutput3';

  @Property()
  duration: number;

  @ManyToOne()
  door!: Door;
}

@B4nan
Copy link
Member

B4nan commented Feb 20, 2022

Well, I didn't really like the idea of even having this initial thing. Migrations do schema diffing, if there is no schema, it will create it. So for the regular flow, there is no need for it, just create the migration when you want to sync your entities with (empty or not) schema.

Are you using ts-morph? If not, your entity definition is missing some things (or maybe just one thing - @ManyToOne(() => Door).

@michaelgambold
Copy link
Author

Yes I'm using ts-morph (my current config is below).

I'm using nestjs so have been following the docs from here (apart from the normal mikro-orm docs). They all don't have the whole picture and have had to stitch/reverse engineer them to get it all linked up and working.

image

@B4nan
Copy link
Member

B4nan commented Feb 20, 2022

They all don't have the whole picture and have had to stitch/reverse engineer them to get it all linked up and working.

I am happy to accept PRs or specific suggestions on how to improve it, but don't expect to find every information in a recipe about nest integration, as that should really be just about the integration and not about ORM features.

Btw why do you disable the context middleware? If its because of ALS, that is enabled by default in v5, so no need to separate context handling.

Note that the version on nest docs is a bit outdated, will PR that once we have v5 of the nest package out, as there will be more changes in the docs. It is basically a copy of what we have in the recipe in our docs (which will be always more up to date).

@michaelgambold
Copy link
Author

I agree that not all the information should be there. However both nest and orm did not support typescript Config files out of the box. I had to carefully merge my code into the sample to find things like a index.js file, adding main to package .json and such as I would only get an error about not having the .js Config.

No idea on the context. I just copied and modified the Config from working sample project. I’ll give it a go without that

https://github.com/mikro-orm/nestjs-realworld-example-app/blob/master/src/mikro-orm.config.ts.example

@B4nan
Copy link
Member

B4nan commented Feb 20, 2022

Btw the problem is with @PrimaryKey({ autoincrement: false }), the schema reflection fails to mark it as non-autoincrement (as we consider all numeric PKs as autoincrement by default), and because of this it fires the alter query for door table too.

Unfortunately I am not sure if we can infer this from information schema, maybe by parsing the table definition swich quite sucks as it can be fragile, but what else can we do :]

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

No branches or pull requests

2 participants