Add up/down hooks to insert custom Rust code

[matze]

Draft PR to make discussion/review a bit easier.

[cljoly]

Draft PR to make discussion/review a bit easier.

Good idea.

For the failing job because of the minimum rust version, we can consider incrementing it, because I can’t (and don’t) make much of a formal rust version promise due to rusqlite not making any either.

[cljoly]

From #27

Thinking a bit about it. Since we already have a Transaction object wouldn't it be nicer to pass that instead? It derefs to Connection when needed and you can use the savepoint API directly.

I think the problem is that someone could then change the semantics of the transaction in which we run the current migration (set_drop_behavior), which would render the migrations unsound.

And you can create savepoints from a the connection struct anyway, right?

Regarding performance: I think indeed this would indeed be premature optimization. A check for None/Some is extremely cheap because of the niche optimization, I/O is orders of magnitude slower, migrations rarely run. So, I don't think feature gating is necessary here.

Yep, but it does not harm to measure :) But again, it's not blocking and we can even release a beta version to test the API without the benchmarks.

[cljoly]

I took a quick look at the code, thanks for the documentation on the new methods!

[matze]

And you can create savepoints from a the connection struct anyway, right?

Yes, of course.

My main problem was that one cannot get a &mut Transaction hence it derefs only to &Connection unless I miss something here.

[cljoly]

Hum, ok, I'll take a deeper look this weekend.

[cljoly]

I have not forgotten, I’m still looking into this.

[cljoly]

Currently the user has to pass the hook in a Box::new(|tx| { closure_content }). By relaxing the constraints on the type a little bit and using static dispatch by default, the user can just pass the closure, i.e. |tx: &Transaction| { closure_content }.

To get to this point, I’ve made these changes locally (you can pipe the patch into git apply if you want):

diff --git a/src/lib.rs b/src/lib.rs
index 08f4e47..a8a28c4 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -105,14 +105,14 @@
 };
 
 /// Helper trait to make hook functions clonable.
-pub trait FnHook: Fn(&Transaction) + Send + Sync {
+pub trait FnHook: Fn(&Transaction) + Sync {
     /// Clone self.
     fn clone_box(&self) -> Box<dyn FnHook>;
 }
 
 impl<T> FnHook for T
 where
-    T: 'static + Fn(&Transaction) -> () + Clone + Send + Sync,
+    T: 'static + Clone + Sync + Fn(&Transaction),
 {
     fn clone_box(&self) -> Box<dyn FnHook> {
         Box::new(self.clone())
@@ -194,18 +194,19 @@ pub const fn up(sql: &'u str) -> Self {
     ///
     /// ```
     /// use rusqlite_migration::{M, Migrations};
+    /// use rusqlite::Transaction;
     ///
     /// let migrations = Migrations::new(vec![
     ///     M::up_with_hook(
     ///         "CREATE TABLE novels (text TEXT);",
-    ///         Box::new(move |tx| {
+    ///         move |tx: &Transaction| {
     ///             tx.execute("INSERT INTO novels (text) VALUES (?1)", ("Lorem Ipsum …",))
     ///                 .unwrap();
-    ///         }),
+    ///         },
     ///     ),
     ///     M::up_with_hook(
     ///         "ALTER TABLE novels ADD compressed TEXT;",
-    ///         Box::new(|tx| {
+    ///         |tx: &Transaction| {
     ///             let mut stmt = tx.prepare("SELECT rowid, text FROM novels").unwrap();
     ///             let rows = stmt
     ///                 .query_map([], |row| {
@@ -225,13 +226,13 @@ pub const fn up(sql: &'u str) -> Self {
     ///                 )
     ///                 .unwrap();
     ///             }
-    ///         }),
+    ///         },
     ///     ),
     /// ]);
     /// ```
-    pub fn up_with_hook(sql: &'u str, hook: Box<dyn FnHook>) -> Self {
+    pub fn up_with_hook(sql: &'u str, hook: impl FnHook + 'static) -> Self {
         let mut m = Self::up(sql);
-        m.up_hook = Some(hook);
+        m.up_hook = Some(hook.clone_box());
         m
     }
 
@@ -256,9 +257,9 @@ pub const fn down(mut self, sql: &'u str) -> Self {
     /// Define a down-migration running additional Rust code. This SQL statement should exactly
     /// reverse the changes performed in [`Self::up()`]. `hook` will run before the SQL statement
     /// is executed.
-    pub fn down_with_hook(mut self, sql: &'u str, hook: Box<dyn FnHook>) -> Self {
+    pub fn down_with_hook(mut self, sql: &'u str, hook: impl FnHook + 'static) -> Self {
         self.down = Some(sql);
-        self.down_hook = Some(hook);
+        self.down_hook = Some(hook.clone_box());
         self
     }
 
diff --git a/src/tests.rs b/src/tests.rs
index 96df52d..d809b57 100644
--- a/src/tests.rs
+++ b/src/tests.rs
@@ -206,14 +206,14 @@ fn hook_test() {
     let migrations = Migrations::new(vec![
         M::up_with_hook(
             "CREATE TABLE novels (text TEXT);",
-            Box::new(move |tx| {
+            move |tx: &Transaction| {
                 tx.execute("INSERT INTO novels (text) VALUES (?1)", (&cloned,))
                     .unwrap();
-            }),
+            },
         ),
         M::up_with_hook(
             "ALTER TABLE novels ADD compressed TEXT;",
-            Box::new(|tx| {
+            |tx: &Transaction| {
                 let mut stmt = tx.prepare("SELECT rowid, text FROM novels").unwrap();
                 let rows = stmt
                     .query_map([], |row| {
@@ -232,7 +232,7 @@ fn hook_test() {
                     )
                     .unwrap();
                 }
-            }),
+            },
         ),
     ]);
 

[cljoly]

I think it looks good overall, please see my comments for the requested changes. Please note, nit: comments are up to you.

[cljoly]

I’m done commenting for now, please ping me when you want me to take another look.

[matze]

I integrated all your suggestions/changes (very nice by the way to not have to box the hook). Please let me know if the error handling is okay and I should continue with that for down migrations. I will then follow up with improved tests etc.

[cljoly]

Looking…

[cljoly]

Please let me know if the error handling is okay and I should continue with that for down migrations. I will then follow up with improved tests etc.

I have suggested something for the error handling, please let me know what you think. Otherwise, the PR seems to be on a good path.

[cljoly]

Also I have checked the new required minimum version, it’s 1.61.0 (due to const & traits). You can get the corresponding CI check to pass (I think) by applying this:

diff --git a/Cargo.toml b/Cargo.toml
index d75a55f..b6be071 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -9,7 +9,7 @@ keywords = ["rusqlite", "sqlite", "user_version", "database", "migration"]
 categories = ["database"]
 readme = "README.md"
 homepage = "https://cj.rs/rusqlite_migration"
-rust-version = "1.56"
+rust-version = "1.61.0"
 
 # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
 

1.61.0 is quite old already (May 2022), so that’s fine

[matze]

So, of course I also had to export the HookError …

Anyway, things work nicely with the only exception that once_cell::Lazy does not work because the hooks are not Send but I can live with that. That usecase has been the origin of the issue but turns out I could really use that in our closed source corporate software as well.

[cljoly]

Thanks for sharing the example on Wastebin :)

I’ve taken a quick look, things look quite good.

once_cell::Lazy does not work because the hooks are not Send but I can live with that

Hum, if we lost (I may misunderstand here) the Send trait where we previously implement it, that would be a breaking change of the interface. But we can find a solution to impl Send again or figure something out, not necessarily in this PR (i.e. we could merge and iron that out later.)

[cljoly]

I’ll hopefully take a deeper look this weekend. Thanks for the additional work on this!

[matze]

Hum, if we lost (I may misunderstand here) the Send trait where we previously implement it, that would be a breaking change of the interface.

I should've looked closer, it was one of your early suggestions that removed the Send. Bringing it back makes it work also with lazy_static :-)

[cljoly]

I should've looked closer, it was one of your early suggestions that removed the Send

I recall now, I’m sorry I made this suggestion.

[cljoly]

Besides the two small documentation things I’ve noticed, looks all good to me.

Were there any more changes you wanted to make? If not, we can probably merge after addressing (can be declining if you think they are not valid) my last comments.

[cljoly]

Also I ran (the very imperfect) benchmarks I wrote a while ago, there is no detectable performance difference, which is great. Or at least it gives some confidence that the change won’t introduce a significant performance regression. Congrats!

[matze]

I changed your comment suggestions, made perfect sense.

Were there any more changes you wanted to make?

No, that's it from my side :-) Also good that the change has no noticeable performance impact.

[cljoly]

Awesome, thanks for all your work on this PR!

[matze]

Thank you for the thorough review and helpful suggestions!

[matze]

Just a general question and no pressure: when do you expect to release a new version? I can of course refer to the Git repo but that feels kind of icky.

[cljoly]

I can release an alpha version this week/weekend, with no compatibility guarantee so that we can change the API if it turns out to be unergonomic in some cases. You will then be able to refer to it with a version number, which I agree is more readable in the manifest of applications.

From the cargo book:

Cargo allows "newer" pre-releases to be used automatically. For example, if 1.0.0-beta is published, then a requirement foo = "1.0.0-alpha" will allow updating to the beta version.

That is unfortunate, I will try to avoid breaking compatibility between a beta and an alpha if that would result in a silent update. I’m not sure how I will do it, maybe by also changing the patch version, say 0.2.0-alpha and 0.2.1-alpha? That feels incompatible with the spirit of semver, but 🤷🏼 .

[cljoly]

As for a stable version with full API compatibility guarantees, I’m not sure, ideally we would first get some feedback on this new API through an alpha release.

[matze]

No rush, I just pin to the Git version at the moment :-)

[cljoly]

I’ve done the release, took longer than I expected: https://github.com/cljoly/rusqlite_migration/releases/tag/v1.1.0-alpha.1

[cljoly]

Commented on the wrong PR, sorry.