Skip to content

Add updateHook, commitHook, and rollbackHook #1337

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

spinda
Copy link

@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
Author

spinda commented Mar 4, 2025

@spinda
Copy link
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
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.

UpdateHook * self = static_cast<UpdateHook *>(data);

v8::Isolate * isolate = self->isolate;
v8::Isolate::Scope isolateScope(isolate);
Copy link
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)?

db(db),
fn(isolate, fn) {}

static void invoke(void * data, int op, char const * dbName, char const * tableName, sqlite3_int64 rowid) {
Copy link
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.

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
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.

'use strict';
const Database = require('../.');

describe('Database hooks', function () {
Copy link
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