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

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

Open
mfkrause opened this issue May 27, 2024 · 11 comments
Labels
bug Something isn't working driver/expo-sqlite drizzle/kit priority Will be worked on next

Comments

@mfkrause
Copy link

mfkrause commented May 27, 2024

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:

session.run(sql.raw(stmt));

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

  • iOS 17
  • Expo SDK 51.0.8
  • expo-sqlite 14.0.3
@mfkrause mfkrause added the bug Something isn't working label May 27, 2024
@mfkrause
Copy link
Author

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

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.

  • Expo SDK 51.0.8
  • expo-sqlite 14.0.3
  • drizzle-orm 0.30.10

@edwinwong90
Copy link

edwinwong90 commented Jul 11, 2024

I have the same experience too. Adding .notNull() will crash the app when is not the first time migration. I found out that the generated migration sql file it has mentioned

/*
 SQLite does not support "Set not null to column" out of the box, we do not generate automatic migration for that, so it has to be done manually
 Please refer to: https://www.techonthenet.com/sqlite/tables/alter_table.php
                  https://www.sqlite.org/lang_altertable.html
                  https://stackoverflow.com/questions/2083543/modify-a-columns-type-in-sqlite3

 Due to that we don't generate migration automatically and it has to be done manually
*/

In order to alter the column with .notNull(), you have to re-create the table. Here is the example script

PRAGMA foreign_keys=off;

BEGIN TRANSACTION;

# rename table to backup
ALTER TABLE table1 RENAME TO _table1_old;

# create a new table
CREATE TABLE table1 (
( column1 datatype [ NULL | NOT NULL ],
  column2 datatype [ NULL | NOT NULL ],
  ...
);

# migrate the data from backup table to new table
INSERT INTO table1 (column1, column2, ... column_n)
  SELECT column1, column2, ... column_n
  FROM _table1_old;

COMMIT;

PRAGMA foreign_keys=on;
"react-native": "0.74.3",
"expo": "~51.0.18",
"expo-sqlite": "~14.0.4",
"drizzle-orm": "^0.31.4",
"drizzle-kit": "^0.22.8",

The app will longer crash in my case. Hope this help someone!

@devYonz
Copy link

devYonz commented Aug 23, 2024

I believe I had the same issue and thank you all for saving my behind. This issue is reproducible on Android & expo-sql with latest versions

package.json -- original failure, also reproduced on new version

    "drizzle-orm": "^0.32.2",
    "drizzle-kit": "^0.22.7",
    "expo-sqlite": "^14.0.4",

package.json -- reproduced after bumping versions, drizzle-kit up & drizzle-kit migrate.
FYI pnpx drizzle-kit up fails but npx works

    "drizzle-orm": "^0.33.0",
    "expo-sqlite": "^14.0.4",
    "drizzle-kit": "^0.24.1",

🚩 Some where down the line it started crashing again. This was after it briefly started working when I removed the multi-line comments. I think bumping versions to reproduce created more issues.. I have now resorted to nuking the drizzle/ folder and re-created migrations. This needed a further step in the app to allow users to remove the app.db file because in Android the file still stays around even in a new install.

Multi-line comment was generated because - can't generate foreign key & can't add primary column

You're trying to add PRIMARY KEY(cluster_id,email_id) to 'emails_to_clusters' table
SQLite does not support adding primary key to an already created table
You can do it in 3 steps with drizzle orm:
 - create new mirror table with needed pk, rename current table to old_table, generate SQL
 - migrate old data from one table to another
 - delete old_table in schema, generate sql
...
SQLite does not support "Creating foreign key on existing column" out of the box, 
....

Ignore the below, just adding some specific tags to help people find this issue

sqlite3_clear_bindings
base.apk!libexpo-sqlite.so
Failed to deliver inset control state change to w=Window{99e8545 u0 ai.vella/ai.vella.MainActivity EXITING} (Ask Gemini)
                                                                                                    android.os.DeadObjectException
Fatal signal 11 (SIGSEGV), code 1 (SEGV_MAPERR), fault addr 0x0 in tid 19855 (mqt_js), pid 19388 (****)
Package [***] reported as REPLACED, but missing application info. Assuming REMOVED.

@joaobibiano
Copy link

regenerating the last migration and removing the comments solved the issue

@mphill
Copy link

mphill commented Sep 8, 2024

I think the issue is with the Expo SQLite library, but it can be solved from the Drizzle side.

The app crashes if you attempt to run a SQL statement that is just a comment.

The migration code splits the SQL files based on this string: --> statement-breakpoint

image

Because --> statement-breakpoint encloses the comment, Drizzle is attempting to execute this comment which is why the app crashes on start.

Stripping the comments from the file before running the SQL fixes the problem.

      const result = query
       // strip comments 
      .replace(/(\/\*[^*]*\*\/)|(\/\/[^*]*)|(--[^.].*)/gm, '')
      .split("--> statement-breakpoint").map((it) => {
        return it;
      })
      .filter((it) => it.trim().length > 0);
 
      if(result.length === 0) {
        continue;
      }

This will make the migrations run correctly now.

There might be other ways to solve this:

  • It's also possible to remove the extra --> statement-breakpoint after the comment, which may fix the issue.
  • Or maybe using -- instead of /* */ for these comments.

If you manually edit the migration files, completely remove any build folders (iOS/Android) locally to ensure your build gets fresh migrations.

@mirague
Copy link

mirague commented Sep 15, 2024

I'm also seeing RN crash when running useMigrations() with Expo SQLite.

Even when there's an empty migration or one without comments. I realise the __drizzle_migrations gets rows created, but both the id and the hash is empty, which surely must be problematic:
CleanShot 2024-09-15 at 17 37 23@2x

The logs state its explicitly passed an empty string for the hash:

Query: INSERT INTO "__drizzle_migrations" ("hash", "created_at") VALUES(?, ?) -- params: ["", 1726414504752]

No useful crash logs, but happy to dig into this with some instructions.

"expo-drizzle-studio-plugin": "^0.0.2",
"expo-sqlite": "~14.0.6",
"drizzle-kit": "^0.24.2",
"drizzle-orm": "^0.33.0",

@mphill
Copy link

mphill commented Sep 15, 2024

Can you link to your migrations so I can see them?

The way it looks like drizzle handles the migrations is to only look at the timestamp. It might be ok.

I opened a PR to fix the migration table primary key FYI.

@mirague
Copy link

mirague commented Sep 15, 2024

Can you link to your migrations so I can see them?

The way it looks like drizzle handles the migrations is to only look at the timestamp. It might be ok.

I opened a PR to fix the migration table primary key FYI.

@mphill Yeah I also just realised that SERIAL isn't a valid SQLite type and integer should be used for autoincrementing primary keys.

Here's the migrations and journal files:
https://gist.github.com/mirague/fad1248d7460c3e48a9f7dee34567e67#file-migrations-js

@mphill
Copy link

mphill commented Sep 15, 2024

@mirague

#2970 should fix the migration tables using serial

Also, see this:
#2435

STRICT mode would have caught this and many other bugs in the SQLite dialect. Please upvote this.

Your migrations file looks good to me. If you are using the Dev Expo client, make sure to remove the iOS or Android folder and try again with a fresh install.

@mirague
Copy link

mirague commented Sep 15, 2024

I can confirm migrations work after patching drizzle-orm/sqlite-core/dialect.js to include the changes from that PR:

diff --git a/sqlite-core/dialect.js b/sqlite-core/dialect.js
index f249890cb4f00498cec0ab394221a1dc0e781304..f05cb57a847b7c02ea317d5fe9ba4faa5dee61f9 100644
--- a/sqlite-core/dialect.js
+++ b/sqlite-core/dialect.js
@@ -531,9 +531,9 @@ class SQLiteSyncDialect extends SQLiteDialect {
     const migrationsTable = config === void 0 ? "__drizzle_migrations" : typeof config === "string" ? "__drizzle_migrations" : config.migrationsTable ?? "__drizzle_migrations";
     const migrationTableCreate = sql`
 			CREATE TABLE IF NOT EXISTS ${sql.identifier(migrationsTable)} (
-				id SERIAL PRIMARY KEY,
-				hash text NOT NULL,
-				created_at numeric
+        id INTEGER PRIMARY KEY AUTOINCREMENT NOT NULL,
+        hash TEXT NOT NULL,
+        created_at INTEGER
 			)
 		`;
     session.run(migrationTableCreate);
@@ -566,9 +566,9 @@ class SQLiteAsyncDialect extends SQLiteDialect {
     const migrationsTable = config === void 0 ? "__drizzle_migrations" : typeof config === "string" ? "__drizzle_migrations" : config.migrationsTable ?? "__drizzle_migrations";
     const migrationTableCreate = sql`
 			CREATE TABLE IF NOT EXISTS ${sql.identifier(migrationsTable)} (
-				id SERIAL PRIMARY KEY,
-				hash text NOT NULL,
-				created_at numeric
+        id INTEGER PRIMARY KEY AUTOINCREMENT NOT NULL,
+        hash TEXT NOT NULL,
+        created_at INTEGER
 			)
 		`;
     await session.run(migrationTableCreate);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working driver/expo-sqlite drizzle/kit priority Will be worked on next
Projects
None yet
Development

No branches or pull requests

8 participants