sqlite: Properly handle app-level errors in transactions

src/storage/__tests__/sqlite-test.js

@@ -391,4 +391,68 @@ describe('our promisified sqlite', () => {
 
     expect(data).toEqual({ double: 8, square: 16 });
   });
+
+  describe('transactions failing in app code', () => {
+    // These tests correspond to "BROKEN" tests above for expo-sqlite
+    // itself, and check that we've successfully worked around those issues.
+
+    test('throws early', async () => {
+      // Do a bit of boring setup.
+      const db = new SQLDatabase(dbName);
+      await db.transaction(async tx => {
+        tx.executeSql('CREATE TABLE foo (x INT)');
+        tx.executeSql('INSERT INTO foo (x) VALUES (?)', [1]);
+      });
+
+      // Now attempt a transaction, and hit an exception in the transaction
+      // callback.
+      await expect(
+        db.transaction(async tx => {
+          tx.executeSql('INSERT INTO foo (x) VALUES (?)', [2]);
+          throw new Error('Fiddlesticks!');
+        }),
+      ).rejects.toThrowError();
+
+      // The transaction gets rolled back.  Its data wasn't committed:
+      const rows = await db.query<{ x: number }>('SELECT x FROM foo', []);
+      expect(rows).toEqual([{ x: 1 }]);
+
+      // … and we can move on to other transactions, and those go just fine:
+      await db.transaction(async tx => {
+        tx.executeSql('INSERT INTO foo (x) VALUES (?)', [3]);
+      });
+      const rows2 = await db.query<{ x: number }>('SELECT x FROM foo', []);
+      expect(rows2).toEqual([{ x: 1 }, { x: 3 }]);
+    });
+
+    test('throws later', async () => {
+      // Do a bit of boring setup.
+      const db = new SQLDatabase(dbName);
+      await db.transaction(async tx => {
+        tx.executeSql('CREATE TABLE foo (x INT)');
+        tx.executeSql('INSERT INTO foo (x) VALUES (?)', [1]);
+      });
+
+      // Now attempt a transaction, and hit an exception after awaiting some
+      // previous result.
+      await expect(
+        db.transaction(async tx => {
+          await tx.executeSql('INSERT INTO foo (x) VALUES (?)', [2]);
+          tx.executeSql('INSERT INTO foo (x) VALUES (?)', [3]);
+          throw new Error('Fiddlesticks!');
+        }),
+      ).rejects.toThrowError();
+
+      // The transaction gets rolled back.  Its data wasn't committed:
+      const rows = await db.query<{ x: number }>('SELECT x FROM foo', []);
+      expect(rows).toEqual([{ x: 1 }]);
+
+      // … and we can move on to other transactions, and those go just fine:
+      await db.transaction(async tx => {
+        tx.executeSql('INSERT INTO foo (x) VALUES (?)', [4]);
+      });
+      const rows2 = await db.query<{ x: number }>('SELECT x FROM foo', []);
+      expect(rows2).toEqual([{ x: 1 }, { x: 4 }]);
+    });
+  });
 });

src/storage/sqlite.js

@@ -16,9 +16,6 @@ type SQLArgument = number | string;
 
 /**
  * A Promise-based wrapper on expo-sqlite.
- *
- * Note the important limitations of the current promisification, described
- * on `transaction` and `readTransaction`.
  */
 export class SQLDatabase {
   db: WebSQLDatabase;
@@ -29,16 +26,6 @@ export class SQLDatabase {
 
   /**
    * Like the `transaction` method in expo-sqlite, but promisified.
-   *
-   * Note that the callback Promise is not treated in all the ways one would
-   * hope for from a Promise-based API:
-   *  * If the callback throws, the transaction gets committed (!) and the
-   *    Promise returned by `transaction` still resolves, unless one of the
-   *    SQL queries itself hits an error.
-   *  * If the callback returns a Promise that ultimately rejects, the same
-   *    thing applies, and the rejection goes unhandled.
-   *
-   * We'll fix those in the near future.
    */
   transaction(cb: SQLTransaction => void | Promise<void>): Promise<void> {
     return new Promise((resolve, reject) =>
@@ -52,15 +39,6 @@ export class SQLDatabase {
 
   /**
    * Like the `readTransaction` method in expo-sqlite, but promisified.
-   *
-   * Note that the callback Promise is not treated in all the ways one would
-   * hope for from a Promise-based API:
-   *  * If the callback throws, the Promise returned by `readTransaction`
-   *    still resolves, unless one of the SQL queries itself hits an error.
-   *  * If the callback returns a Promise that ultimately rejects, the same
-   *    thing applies, and the rejection goes unhandled.
-   *
-   * We'll fix those in the near future.
    */
   readTransaction(cb: SQLTransaction => void | Promise<void>): Promise<void> {
     return new Promise((resolve, reject) =>
@@ -97,18 +75,37 @@ export class SQLDatabase {
 // An absurd little workaround for expo-sqlite, or really
 // the @expo/websql library under it, being too eager to check a
 // transaction's queue and declare it complete.
-async function keepQueueLiveWhile(
-  tx: WebSQLTransaction,
-  f: () => void | Promise<void>,
-): Promise<void> {
+//
+// Also for it not handling errors in callbacks, so that an exception in a
+// transaction's application-level code causes it to commit (!) if the
+// transaction callback itself throws an exception, and to get the whole
+// database object stuck if a statement callback throws an exception.
+// Instead, on any such exception we cause the transaction to roll back.
+async function keepQueueLiveWhile(tx: WebSQLTransaction, f: () => void | Promise<void>) {
+  let error = false;
   let done = false;
-  const hold = () => tx.executeSql('SELECT 1', [], () => (done ? undefined : hold()));
+  const hold = () =>
+    tx.executeSql('SELECT 1', [], () =>
+      error ? tx.executeSql('SELECT error') : done ? undefined : hold(),
+    );
   hold();
   try {
     await f();
+  } catch (e) {
+    error = true;
   } finally {
     done = true;
   }
+
+  // A neat touch would be to pass through the actual error message.
+  // Sadly `tx.executeSql('SELECT ?, error', [String(error)])` doesn't
+  // produce any hint of the string; the resulting message is just
+  //   SQLITE_ERROR: no such column: error
+  // So, encode the error string as a SQL identifier?  (With a prefix
+  // to ensure it doesn't hit a builtin, or reserved word?)  Need to
+  // be darn sure the encoding is correct and doesn't cause injection.
+  //
+  // For now, we content ourselves with aborting the transaction at all.
 }
 
 class SQLTransactionImpl {