drizzle-team / drizzle-orm

Headless TypeScript ORM with a head. Runs on Node, Bun and Deno. Lives on the Edge and yes, it's a JavaScript ORM too 😅
https://orm.drizzle.team
Apache License 2.0
21.52k stars 487 forks source link

[BUG]: Drizzle ORM with Expo SQLite driver crashes app when more than one migration is present #2384

Open mfkrause opened 1 month ago

mfkrause commented 1 month ago

What version of drizzle-orm are you using?

0.30.10

What version of drizzle-kit are you using?

0.21.4

Describe the Bug

When installing Drizzle into a blank Expo SDK 51 project as per the documentation, creating a database schema, the initial migration using drizzle-kit and the provided useMigrations() hook provided by Drizzle, everything works as expected. The migration is being applied and the database file created.

If you now generate another migration (it can even be an empty migration file generated through drizzle-kit generate --custom, or a migration file generated through actual schema changes) and re-open the app, the app immediately crashes. No error logs are printed.

The Xcode console also does not show any relevant log output.

The culprit seems to be somewhere in this function (which is performing the actual migration). Commenting it out leads to the crash not occurring and the __drizzle_migrations table being populated accordingly: https://github.com/drizzle-team/drizzle-orm/blob/a78eefe08e127922565486143e0150a718b27e8a/drizzle-orm/src/sqlite-core/dialect.ts#L758

Interestingly, when replacing lines 755-766 (the for loop) in the above file with the following code snippet, which ignores all the migration files other than the first one and instead performs some generic SQL query, the crash does not occur (but obviously the migrations aren't applied, this is simply for diagnostic purposes):

for (const index in migrations) {
  const migration = migrations[index];
  if (!lastDbMigration || Number(lastDbMigration[2]) < migration.folderMillis) {
    for (const stmt of migration.sql) {
      if (index == 0) session.run(sql.raw(stmt));
      else {
        session.run(sql.raw('SELECT 1'));
        console.log("Skipping migration: ", stmt)
      }
    }
    session.run(
      sql`INSERT INTO ${sql.identifier(migrationsTable)} ("hash", "created_at") VALUES(${migration.hash}, ${migration.folderMillis})`
    );
  }
}

I've logged the contents of stmt and that looks fine. Again, this also happens with a completely empty migration file, so the SQL query itself shouldn't be the problem.

Expected behavior

All migrations should be applied accordingly.

Environment & setup

mfkrause commented 1 month ago

Okay, it seems like the SQL file was in fact the problem.

Specifically, it seems like if there are any multi-line comments in the SQL file, the migration process will crash (and somehow not be catched by the try-catch block and therefore the ROLLBACK instruction). If I filter out the multi-line comment stmt blocks using the \n*\/\* RegEx, it works.

The fact that an empty migration file crashed as well for me seems to have been a cache issue.

I don't know if this is intended behavior, but if it is, it's nowhere documented.

jean-webdev commented 1 month ago

I may have experienced the same problem just now. I made some changes to the schema for the app I'm developing and ran the migration command. When I tried to start the app again, Expo Go entirely crashed and returned to the Expo dashboard. Tried a few things, clearing cache, etc. no luck in either Android or iOS simulators.

Fortunately I found your issue when searching for possible causes; sure enough the new migration SQL file contained a multiline comment regarding a .notNull() I was trying to remove from the schema. I took it out and now Expo successfully starts the app so I can continue working (Thanks to you 🫡 I don't know how long it would have taken me to troubleshoot).

I've really gotten to like Drizzle and would be happy to hear if this bug could be prevented so that other devs won't run into the same problem.