feat(sqlite): add preupdate hook #3625

aschey · 2024-12-04T04:44:41Z

Adds bindings for SQLite's preupdate hook.

This is exposed as a separate feature because the system SQLite version generally does not have this flag enabled, so using it with sqlite-unbundled may cause linker errors.

If we don't want to create a new feature flag in the main crate just for this, we could enable it by default with the sqlite (bundled) feature only. That would make the configuration a little simpler.

aschey · 2024-12-04T04:59:32Z

tests/sqlite/sqlite.rs

    });

    let _ = sqlx::query("INSERT INTO tweet ( id, text ) VALUES ( 3, 'Hello, World' )")
        .execute(&mut conn)
        .await?;
+    assert!(CALLED.load(Ordering::Relaxed));


Without these asserts, it's possible that these tests would still pass if the callback functions were never invoked. I went ahead and added these checks to the existing tests as well as the new tests that I created.

abonander · 2024-12-12T21:03:37Z

Cargo.toml

@@ -108,6 +108,7 @@ postgres = ["sqlx-postgres", "sqlx-macros?/postgres"]
 mysql = ["sqlx-mysql", "sqlx-macros?/mysql"]
 sqlite = ["_sqlite", "sqlx-sqlite/bundled", "sqlx-macros?/sqlite"]
 sqlite-unbundled = ["_sqlite", "sqlx-sqlite/unbundled", "sqlx-macros?/sqlite-unbundled"]
+sqlite-preupdate-hook = ["sqlx-sqlite/preupdate-hook"]


This should emit a compile error if neither sqlite or sqlite-unbundled is enabled or else it could cause weird errors if it's only enabled on its own.

Added this.

abonander · 2024-12-12T21:03:53Z

sqlx-sqlite/src/connection/mod.rs

@@ -544,3 +549,219 @@ impl Statements {
        self.temp = None;
    }
 }
+
+#[cfg(feature = "preupdate-hook")]
+mod preupdate_hook {


This module has enough going on that it should be its own file.

Done. I considered moving all the hooks into their own module since there are quite a few now, but that would be a larger change. I'm happy to do that if you'd like though.

abonander · 2024-12-13T00:36:05Z

sqlx-sqlite/src/connection/mod.rs

+    /// An accessor for the old values of the row being deleted/updated during the preupdate callback.
+    #[derive(Debug)]
+    pub struct PreupdateOldValueAccessor {
+        db: *mut sqlite3,


This needs a lifetime tying it to the duration of the callback or else it could lead to a use-after-free. The internal pointer makes this !Send/!Sync but it could still be stored in a thread-local.

PreupdateHookResult has a lifetime on it due to the string parameters so I believe this should be fine, especially now that everything is consolidated into that struct. I made a comment in there that these lifetimes need to be preserved for this reason.

abonander · 2024-12-13T00:39:25Z

sqlx-sqlite/src/connection/mod.rs

+        pub fn get_old_column_value(&self, i: i32) -> Result<SqliteValue, Error> {
+            let mut p_value: *mut sqlite3_value = ptr::null_mut();
+            unsafe {
+                let ret = sqlite3_preupdate_old(self.db, i, &mut p_value);


The sqlite3_value returned by this call and sqlite3_preupdate_new has weird semantics. It's a "protected" value so it's thread-safe, but at the same time the documentation also specifies:

The sqlite3_value that P points to will be destroyed when the preupdate callback returns.

We should handle this as a SqliteValueRef instead, using the same lifetime. The user can then use .to_owned() to get a fully independent value if they need it.

You'll need to add a new case here:

sqlx/sqlx-sqlite/src/value.rs

Line 19 in 1678b19

Value(&'r SqliteValue),

This should also check i against sqlite3_preupdate_count because the result is undefined if the index is out of bounds.

We should handle this as a SqliteValueRef instead, using the same lifetime. The user can then use .to_owned() to get a fully independent value if they need it.

SqliteValue::new calls sqlite3_value_dup which creates a copy of the sqlite3_value. I tested this out by storing the returned SqliteValue in a mutex and ensuring the returned value could still be decoded properly after the callback was completed.

I can add something to SqliteValueRef for this to avoid the call to sqlite3_value_dup, but I think that would require re-implementing a lot of the logic in SqliteValue or modifying it so that it can operate on both an owned or borrowed value. Let me know if I'm missing something.

This should also check i against sqlite3_preupdate_count because the result is undefined if the index is out of bounds.

I think the documentation might be a bit conservative here (or they don't want to make any guarantees) because it does check for these conditions and return an error, but I went ahead and added an explicit check here too.

abonander · 2024-12-13T02:23:14Z

sqlx-sqlite/src/connection/mod.rs

+    pub enum PreupdateCase {
+        /// Pre-update hook was triggered by an insert.
+        Insert(PreupdateNewValueAccessor),
+        /// Pre-update hook was triggered by a delete.
+        Delete(PreupdateOldValueAccessor),
+        /// Pre-update hook was triggered by an update.
+        Update {
+            old_value_accessor: PreupdateOldValueAccessor,
+            new_value_accessor: PreupdateNewValueAccessor,
+        },
+        /// This variant is not normally produced by SQLite. You may encounter it
+        /// if you're using a different version than what's supported by this library.
+        Unknown,
+    }


I'm dubious about the utility of this enum and separating PreupdateOldValueAccessor and PreupdateNewValueAccessor. If a user wants to handle updates generically, they'd have to match on this and then synthesize some sort of tuple of (Option<PreupdateOldValueAccessor>, Option<PreupdateNewValueAccessor>).

Instead, I'd just merge all this functionality into PreUpdateHookResult.

Note that it's also undefined behavior to use sqlite3_preupdate_old_value() when it's an INSERT operation or _new_value() when it's a DELETE operation, so that also needs to be checked.

abonander · 2024-12-13T02:24:58Z

sqlx-sqlite/src/connection/establish.rs

@@ -296,6 +296,8 @@ impl EstablishParams {
            log_settings: self.log_settings.clone(),
            progress_handler_callback: None,
            update_hook_callback: None,
+            #[cfg(feature = "preupdate-hook")]
+            preupdate_hook_callback: None,


I don't see this being set or referenced by anything. Did you mean to expose this on SqliteConnectOptions?

This was being used within the preupdate_hook module to avoid having to add more cfg checks in the main connection module and make additional fields pub(super), but that does make it a bit harder to find. I went ahead and moved that logic to be with the rest of the hooks.

aschey force-pushed the feat/preupdate_hook branch from d9eb625 to 5264ada Compare December 4, 2024 04:59

aschey commented Dec 4, 2024

View reviewed changes

aschey force-pushed the feat/preupdate_hook branch 3 times, most recently from 436694b to 03d9a3f Compare December 4, 2024 07:36

aschey marked this pull request as draft December 4, 2024 16:24

aschey force-pushed the feat/preupdate_hook branch from 03d9a3f to bcdb609 Compare December 5, 2024 02:01

feat: add preupdate hook

7f1fdd9

aschey force-pushed the feat/preupdate_hook branch from bcdb609 to 7f1fdd9 Compare December 5, 2024 02:09

aschey marked this pull request as ready for review December 5, 2024 02:24

abonander requested changes Dec 13, 2024

View reviewed changes

address some PR comments

cc725fa

aschey requested a review from abonander December 13, 2024 06:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(sqlite): add preupdate hook #3625

feat(sqlite): add preupdate hook #3625

aschey commented Dec 4, 2024 •

edited

Loading

aschey Dec 4, 2024

abonander Dec 12, 2024

aschey Dec 13, 2024

abonander Dec 12, 2024

aschey Dec 13, 2024 •

edited

Loading

abonander Dec 13, 2024 •

edited

Loading

aschey Dec 13, 2024

abonander Dec 13, 2024 •

edited

Loading

abonander Dec 13, 2024

aschey Dec 13, 2024

aschey Dec 13, 2024

abonander Dec 13, 2024

abonander Dec 13, 2024

aschey Dec 13, 2024

abonander Dec 13, 2024

aschey Dec 13, 2024

feat(sqlite): add preupdate hook #3625

Are you sure you want to change the base?

feat(sqlite): add preupdate hook #3625

Conversation

aschey commented Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aschey Dec 13, 2024 • edited Loading

Choose a reason for hiding this comment

abonander Dec 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abonander Dec 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aschey commented Dec 4, 2024 •

edited

Loading

aschey Dec 13, 2024 •

edited

Loading

abonander Dec 13, 2024 •

edited

Loading

abonander Dec 13, 2024 •

edited

Loading