Skip to content

[WIP] Initial proposal for the preupdate hook.#897

Merged
gwenn merged 8 commits intorusqlite:masterfrom
MidasLamb:preupdate_hook
Mar 31, 2024
Merged

[WIP] Initial proposal for the preupdate hook.#897
gwenn merged 8 commits intorusqlite:masterfrom
MidasLamb:preupdate_hook

Conversation

@MidasLamb
Copy link
Contributor

I created an initial draft for the preupdate hook that seems to work ok after preliminary testing.

Some questions arose and I'm looking for some input/feedback:

  1. Does it needs it own feature flag instead of being bundled with all the other hooks (it needs a special feature flag on the libsqlite3-sys crate).
    • If it's behind a separate one, I'll probably have to extract some of the hooks code to be included if one or the other is active.
  2. How close do we want to map the original API:
    • According to SQLite3 docs, the 6th and 7th parameter of the original API are dependent (and can be undefined) on the type of operation. I think it would be more idiomatic Rust if this was exposed through an enum instead of closely mapping the API.
  3. During the preupdate_hook, you have access to some special functions (see docs again), but it depends on which type of operation is done.
    • Here I was thinking to use an enum for which the variants expose the appropriate functions, to prevent UB from occurring and provide a safe interface.

If anyone can help me and give some feedback on what the best way forward would be, that would be much appreciated.

Created an initial proposal/direction for the preupdate hook. It is
heavily based on the update hook, with only minor modifications to that
code. This means that it entirely translate the original C API.
@MidasLamb MidasLamb changed the title Added an initial proposal for the preupdate hook. [WIP] Initial proposal for the preupdate hook. Feb 2, 2021
@codecov
Copy link

codecov bot commented Feb 2, 2021

Codecov Report

Merging #897 (560ae50) into master (ddf69f7) will decrease coverage by 0.89%.
The diff coverage is 67.11%.

❗ Current head 560ae50 differs from pull request most recent head b51b541. Consider uploading reports for the commit b51b541 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #897      +/-   ##
==========================================
- Coverage   77.72%   76.83%   -0.90%     
==========================================
  Files          48       48              
  Lines        5621     5689      +68     
==========================================
+ Hits         4369     4371       +2     
- Misses       1252     1318      +66     
Impacted Files Coverage Δ
src/lib.rs 90.05% <ø> (ø)
src/types/value_ref.rs 70.42% <ø> (ø)
src/hooks.rs 66.81% <66.81%> (-27.57%) ⬇️
src/inner_connection.rs 67.30% <100.00%> (+0.42%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ddf69f7...b51b541. Read the comment docs.

@gwenn
Copy link
Collaborator

gwenn commented Feb 3, 2021

May I suggest that hooks is kept untouched and a new feature pre_update_hook = ["libsqlite3-sys/pre_update_hook", "hooks"] is used instead (libsqlite3-sys/pre_update_hook should be used to activate the SQLITE_ENABLE_PREUPDATE_HOOK compile-time option).

@MidasLamb
Copy link
Contributor Author

@gwenn , of course you can suggest that, that is also the first point I touch in my initial message. However the "problem" with that is that it might be confusing that hooks does not encompass all hooks. Of course changing this (i.e. rename hooks to something more specific so it doesn't encompass the preupdate hook) would be a breaking change.

This is my first time writing something that has an interface with C-code so I'm being very cautious and trying to make as little change as possible at a time.

@gwenn
Copy link
Collaborator

gwenn commented Feb 4, 2021

However the "problem" with that is that it might be confusing that hooks does not encompass all hooks.

By default, SQLite does not encompass all hooks so it seems "normal" that rusqlite does the same, no ?

go-sqlite3
tclsqlite

@MidasLamb
Copy link
Contributor Author

By default, SQLite does not encompass all hooks so it seems "normal" that rusqlite does the same, no ?

The difference (unless I'm mistaken about SQLite itself), is that SQLite will have some hooks by default and you can add a flag for the preupdate_hook. But in rusqlite you have to specify that you want hooks (by using the hooks feature), but then there are hooks that are not in there (i.e. the preupdate hook).

Just to clarify, I am completely on board with having a separate flag for the preupdate hook since it requires a special compilation of SQLite, but I'm concerned that it might not be clear if or how these different features interact.

Moved hooks and preupdate_hook into their own modules inside hooks.rs
Also created an initial way to access the functions that are available
during the callback.
}

#[cfg(feature = "hooks")]
mod datachanged_and_friends {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just a preliminary name, I couldn't come up with a succint naming for all the hooks, excluding the preupdate_hook.

@aschey aschey mentioned this pull request Mar 30, 2024
@gwenn gwenn merged commit 45e9db3 into rusqlite:master Mar 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants