Skip to content

Add updateHook, commitHook, and rollbackHook#1337

Open
spinda wants to merge 1 commit intoWiseLibs:masterfrom
spinda:hooks
Open

Add updateHook, commitHook, and rollbackHook#1337
spinda wants to merge 1 commit intoWiseLibs:masterfrom
spinda:hooks

Conversation

@spinda
Copy link
Copy Markdown

@spinda spinda commented Mar 4, 2025

These new Database methods expose the functionality of sqlite3_update_hook, sqlite3_commit_hook, and
sqlite3_rollback_hook, respectively.

These new `Database` methods expose the functionality of
`sqlite3_update_hook`, `sqlite3_commit_hook`, and
`sqlite3_rollback_hook`, respectively.
@spinda spinda requested a review from JoshuaWise as a code owner March 4, 2025 00:19
@spinda
Copy link
Copy Markdown
Author

spinda commented Mar 4, 2025

@spinda
Copy link
Copy Markdown
Author

spinda commented Mar 4, 2025

I'm happy to add documentation for these functions to api.md if this feature addition is deemed acceptable to merge.

Copy link
Copy Markdown
Member

@JoshuaWise JoshuaWise left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job on this! I normally don't accept PRs for large features, but the code here is written very carefully. I think I can merge it if these comments are addressed, and if docs are added.

Comment thread src/objects/database.lzz
UpdateHook * self = static_cast<UpdateHook *>(data);

v8::Isolate * isolate = self->isolate;
v8::Isolate::Scope isolateScope(isolate);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using v8::Isolate::Scope is new to this library, and I'm not familiar with it. What does it do? And do you think it should be used in other similar situations (such as user-defined SQL functions, or the verbose hook)?

Comment thread src/objects/database.lzz
db(db),
fn(isolate, fn) {}

static void invoke(void * data, int op, char const * dbName, char const * tableName, sqlite3_int64 rowid) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since these hooks don't work with WITHOUT ROWID tables or with the truncate optimization, I think that should be called out in the docs.

Comment thread src/objects/database.lzz
v8::Local<v8::Value> opArg = self->addon->cs.Op(isolate, op);
v8::Local<v8::Value> dbNameArg = v8::String::NewFromUtf8(isolate, dbName, v8::NewStringType::kNormal).ToLocalChecked();
v8::Local<v8::Value> tableNameArg = v8::String::NewFromUtf8(isolate, tableName, v8::NewStringType::kNormal).ToLocalChecked();
v8::Local<v8::Value> rowidArg = v8::BigInt::New(isolate, rowid);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be more appropriate to use v8::Number if safeIntegers is not enabled, as that would be consistent with the rest of the library's behavior. I think it should accept a safeIntegers option similarly to how db.function() works.

Comment thread test/38.database.hooks.js
'use strict';
const Database = require('../.');

describe('Database hooks', function () {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to see an additional test: attempting to use the database within a hook should fail, since the db is busy

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants