diff --git a/src/storage/__tests__/sqlite-test.js b/src/storage/__tests__/sqlite-test.js index 5a944b22403..076d006e4ea 100644 --- a/src/storage/__tests__/sqlite-test.js +++ b/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 }]); + }); + }); }); diff --git a/src/storage/sqlite.js b/src/storage/sqlite.js index 7588c87537b..f070479492d 100644 --- a/src/storage/sqlite.js +++ b/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): Promise { 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): Promise { 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, -): Promise { +// +// 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) { + 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 {