mirror: replace transaction helper with builtin (#1771)
Summary: Version 5.0.0 of `better-sqlite3` redesigned the `Database.transaction` method to do exactly what we want, obviating the need for our custom helper function and tests. It’s actually more flexible: it allows the transaction function to take arguments, and allows nested transactions. Test Plan: All tests pass as written. If this patch is instead made by changing the implementation of `_inTransaction` to `return db.transaction(fn)()`, most tests still pass, excepting only some tests about the behavior of `_inTransaction` itself around transaction nesting and manual rollbacks; those behavior changes are acceptable, especially given that they’re not exercised in the code under test. wchargin-branch: mirror-builtin-transaction
This commit is contained in:
William Chargin
GitHub
parent
1f8925fe77
commit
830c83bedf
|
|
@ -211,7 +211,7 @@ export class Mirror {
|
|||
},
|
||||
});
|
||||
const db = this._db;
|
||||
_inTransaction(db, () => {
|
||||
db.transaction(() => {
|
||||
// We store the metadata in a singleton table `meta`, whose unique row
|
||||
// has primary key `0`. Only the first ever insert will succeed; we
|
||||
// are locked into the first config.
|
||||
|
|
@ -354,7 +354,7 @@ export class Mirror {
|
|||
for (const sql of tables) {
|
||||
db.prepare(sql).run();
|
||||
}
|
||||
});
|
||||
})();
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
@ -382,9 +382,9 @@ export class Mirror {
|
|||
+typename: null | Schema.Typename,
|
||||
+id: Schema.ObjectId,
|
||||
|}): void {
|
||||
_inTransaction(this._db, () => {
|
||||
this._db.transaction(() => {
|
||||
this._nontransactionallyRegisterObject(object);
|
||||
});
|
||||
})();
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
@ -583,7 +583,7 @@ export class Mirror {
|
|||
*/
|
||||
_findOutdated(since: Date): QueryPlan {
|
||||
const db = this._db;
|
||||
return _inTransaction(db, () => {
|
||||
return db.transaction(() => {
|
||||
const typenames: $PropertyType<QueryPlan, "typenames"> = db
|
||||
.prepare(
|
||||
dedent`\
|
||||
|
|
@ -642,7 +642,7 @@ export class Mirror {
|
|||
return result;
|
||||
});
|
||||
return {typenames, objects, connections};
|
||||
});
|
||||
})();
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
@ -826,9 +826,9 @@ export class Mirror {
|
|||
* See: `_queryFromPlan`.
|
||||
*/
|
||||
_updateData(updateId: UpdateId, queryResult: UpdateResult): void {
|
||||
_inTransaction(this._db, () => {
|
||||
this._db.transaction(() => {
|
||||
this._nontransactionallyUpdateData(updateId, queryResult);
|
||||
});
|
||||
})();
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
@ -978,11 +978,11 @@ export class Mirror {
|
|||
const result: UpdateResult = await postQuery({body, variables});
|
||||
this._logResponse(requestRowId, result, options.now());
|
||||
|
||||
_inTransaction(this._db, () => {
|
||||
this._db.transaction(() => {
|
||||
const updateId = this._createUpdate(options.now());
|
||||
this._logRequestUpdateId(requestRowId, updateId);
|
||||
this._nontransactionallyUpdateData(updateId, result);
|
||||
});
|
||||
})();
|
||||
return Promise.resolve(true);
|
||||
}
|
||||
|
||||
|
|
@ -1262,14 +1262,14 @@ export class Mirror {
|
|||
fieldname: Schema.Fieldname,
|
||||
queryResult: ConnectionFieldResult
|
||||
): void {
|
||||
_inTransaction(this._db, () => {
|
||||
this._db.transaction(() => {
|
||||
this._nontransactionallyUpdateConnection(
|
||||
updateId,
|
||||
objectId,
|
||||
fieldname,
|
||||
queryResult
|
||||
);
|
||||
});
|
||||
})();
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
@ -1451,9 +1451,9 @@ export class Mirror {
|
|||
* See: `_queryOwnData`.
|
||||
*/
|
||||
_updateOwnData(updateId: UpdateId, queryResult: OwnDataUpdateResult): void {
|
||||
_inTransaction(this._db, () => {
|
||||
this._db.transaction(() => {
|
||||
this._nontransactionallyUpdateOwnData(updateId, queryResult);
|
||||
});
|
||||
})();
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
@ -1737,9 +1737,9 @@ export class Mirror {
|
|||
* See: `_queryTypenames`.
|
||||
*/
|
||||
_updateTypenames(queryResult: TypenamesUpdateResult): void {
|
||||
_inTransaction(this._db, () => {
|
||||
this._db.transaction(() => {
|
||||
this._nontransactionallyUpdateTypenames(queryResult);
|
||||
});
|
||||
})();
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
@ -1842,7 +1842,7 @@ export class Mirror {
|
|||
*/
|
||||
extract(rootId: Schema.ObjectId): mixed {
|
||||
const db = this._db;
|
||||
return _inTransaction(db, () => {
|
||||
return db.transaction(() => {
|
||||
// Pre-compute transitive dependencies into a temporary table.
|
||||
db.prepare(
|
||||
dedent`\
|
||||
|
|
@ -2085,7 +2085,7 @@ export class Mirror {
|
|||
} finally {
|
||||
this._db.prepare("DROP TABLE temp.transitive_dependencies").run();
|
||||
}
|
||||
});
|
||||
})();
|
||||
}
|
||||
}
|
||||
|
||||
|
|
@ -2361,44 +2361,6 @@ export const _FIELD_PREFIXES = deepFreeze({
|
|||
NODE_CONNECTIONS: "node_",
|
||||
});
|
||||
|
||||
/**
|
||||
* Execute a function inside a database transaction.
|
||||
*
|
||||
* The database must not be in a transaction. A new transaction will be
|
||||
* entered, and then the callback will be invoked.
|
||||
*
|
||||
* If the callback completes normally, then its return value is passed
|
||||
* up to the caller, and the currently active transaction (if any) is
|
||||
* committed.
|
||||
*
|
||||
* If the callback throws an error, then the error is propagated to the
|
||||
* caller, and the currently active transaction (if any) is rolled back.
|
||||
*
|
||||
* Note that the callback may choose to commit or roll back the
|
||||
* transaction before returning or throwing an error. Conversely, note
|
||||
* that if the callback commits the transaction, and then begins a new
|
||||
* transaction but does not end it, then this function will commit the
|
||||
* new transaction if the callback returns (or roll it back if it
|
||||
* throws).
|
||||
*/
|
||||
export function _inTransaction<R>(db: Database, fn: () => R): R {
|
||||
if (db.inTransaction) {
|
||||
throw new Error("already in transaction");
|
||||
}
|
||||
try {
|
||||
db.prepare("BEGIN").run();
|
||||
const result = fn();
|
||||
if (db.inTransaction) {
|
||||
db.prepare("COMMIT").run();
|
||||
}
|
||||
return result;
|
||||
} finally {
|
||||
if (db.inTransaction) {
|
||||
db.prepare("ROLLBACK").run();
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Convert a prepared statement into a JS function that executes that
|
||||
* statement and asserts that it makes exactly one change to the
|
||||
|
|
|
|||
|
|
@ -10,7 +10,6 @@ import * as Queries from "./queries";
|
|||
import {
|
||||
_FIELD_PREFIXES,
|
||||
_buildSchemaInfo,
|
||||
_inTransaction,
|
||||
_makeSingleUpdateFunction,
|
||||
Mirror,
|
||||
type _OwnDataUpdateResult,
|
||||
|
|
@ -3987,118 +3986,6 @@ describe("graphql/mirror", () => {
|
|||
});
|
||||
});
|
||||
|
||||
describe("_inTransaction", () => {
|
||||
it("runs its callback inside a transaction", () => {
|
||||
// We use an on-disk database file here because we need to open
|
||||
// two connections.
|
||||
const filename = tmp.fileSync().name;
|
||||
const db0 = new Database(filename);
|
||||
const db1 = new Database(filename);
|
||||
db0.prepare("CREATE TABLE tab (col PRIMARY KEY)").run();
|
||||
|
||||
const countRows = (db) =>
|
||||
db.prepare("SELECT COUNT(1) AS n FROM tab").get().n;
|
||||
expect(countRows(db0)).toEqual(0);
|
||||
expect(countRows(db1)).toEqual(0);
|
||||
|
||||
let called = false;
|
||||
_inTransaction(db0, () => {
|
||||
called = true;
|
||||
db0.prepare("INSERT INTO tab (col) VALUES (1)").run();
|
||||
expect(countRows(db0)).toEqual(1);
|
||||
expect(countRows(db1)).toEqual(0);
|
||||
});
|
||||
expect(called).toBe(true);
|
||||
|
||||
expect(countRows(db0)).toEqual(1);
|
||||
expect(countRows(db1)).toEqual(1);
|
||||
});
|
||||
|
||||
it("passes up the return value", () => {
|
||||
const db = new Database(":memory:");
|
||||
db.prepare("CREATE TABLE tab (col PRIMARY KEY)").run();
|
||||
expect(
|
||||
_inTransaction(db, () => {
|
||||
db.prepare("INSERT INTO tab (col) VALUES (3)").run();
|
||||
db.prepare("INSERT INTO tab (col) VALUES (4)").run();
|
||||
return db.prepare("SELECT TOTAL(col) AS n FROM tab").get().n;
|
||||
})
|
||||
).toBe(7);
|
||||
});
|
||||
|
||||
it("rolls back and rethrows on SQL error", () => {
|
||||
// In practice, this is a special case of a JavaScript error, but
|
||||
// we test it explicitly in case it goes down a different codepath
|
||||
// internally.
|
||||
const db = new Database(":memory:");
|
||||
db.prepare("CREATE TABLE tab (col PRIMARY KEY)").run();
|
||||
|
||||
let threw = false;
|
||||
try {
|
||||
_inTransaction(db, () => {
|
||||
db.prepare("INSERT INTO tab (col) VALUES (1)").run();
|
||||
db.prepare("INSERT INTO tab (col) VALUES (1)").run(); // throws
|
||||
throw new Error("Should not get here.");
|
||||
});
|
||||
} catch (e) {
|
||||
threw = true;
|
||||
expect(e.name).toBe("SqliteError");
|
||||
expect(e.code).toBe("SQLITE_CONSTRAINT_PRIMARYKEY");
|
||||
}
|
||||
expect(threw).toBe(true);
|
||||
|
||||
expect(db.prepare("SELECT COUNT(1) AS n FROM tab").get()).toEqual({n: 0});
|
||||
});
|
||||
|
||||
it("rolls back and rethrows on JavaScript error", () => {
|
||||
const db = new Database(":memory:");
|
||||
db.prepare("CREATE TABLE tab (col PRIMARY KEY)").run();
|
||||
|
||||
expect(() => {
|
||||
_inTransaction(db, () => {
|
||||
db.prepare("INSERT INTO tab (col) VALUES (1)").run();
|
||||
throw new Error("and then something goes wrong");
|
||||
});
|
||||
}).toThrow("and then something goes wrong");
|
||||
|
||||
expect(db.prepare("SELECT COUNT(1) AS n FROM tab").get()).toEqual({n: 0});
|
||||
});
|
||||
|
||||
it("allows the callback to commit the transaction and throw", () => {
|
||||
const db = new Database(":memory:");
|
||||
db.prepare("CREATE TABLE tab (col)").run();
|
||||
expect(() =>
|
||||
_inTransaction(db, () => {
|
||||
db.prepare("INSERT INTO tab (col) VALUES (33)").run();
|
||||
db.prepare("COMMIT").run();
|
||||
throw new Error("and then something goes wrong");
|
||||
})
|
||||
).toThrow("and then something goes wrong");
|
||||
expect(db.prepare("SELECT TOTAL(col) AS n FROM tab").get().n).toBe(33);
|
||||
});
|
||||
|
||||
it("allows the callback to roll back the transaction and return", () => {
|
||||
const db = new Database(":memory:");
|
||||
db.prepare("CREATE TABLE tab (col)").run();
|
||||
expect(
|
||||
_inTransaction(db, () => {
|
||||
db.prepare("INSERT INTO tab (col) VALUES (33)").run();
|
||||
db.prepare("ROLLBACK").run();
|
||||
return "tada";
|
||||
})
|
||||
).toEqual("tada");
|
||||
expect(db.prepare("SELECT TOTAL(col) AS n FROM tab").get().n).toBe(0);
|
||||
});
|
||||
|
||||
it("throws if the database is already in a transaction", () => {
|
||||
const db = new Database(":memory:");
|
||||
db.prepare("BEGIN").run();
|
||||
expect(() => _inTransaction(db, () => {})).toThrow(
|
||||
"already in transaction"
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
describe("logging network requests", () => {
|
||||
describe("_logRequest", () => {
|
||||
it("saves query, query parameters, and request timestamp", async () => {
|
||||
|
|
|
|||
Loading…
Reference in New Issue