BatchInsert support for sync wrapped sqlite connection

[bwqr]

Hi there,

I have a project that uses diesel_async with its postgres feature. In order to port the project to WebAssembly and run inside the browser, I was trying to use sqlite feature of diesel_async. However, I came across a lack of implementation of diesel_async's ExecuteDsl trait for sqlite backend for InsertStatement and BatchInsert structs, which is actually implemented for diesel's ExecuteDsl.

You can check this example to understand difference easily

/*
dependencies
diesel = { version = "2", features = ["sqlite"] }
diesel-async = { version = "0.5", features = ["sqlite"] }
*/

use diesel::prelude::*;
use diesel_async::sync_connection_wrapper::SyncConnectionWrapper;

diesel::table! {
    apps (id) {
        id -> Int4,
        #[max_length = 8]
        name -> Varchar,
    }
}

fn execute<T: diesel::Connection>(
    _: impl diesel::query_dsl::methods::ExecuteDsl<T>,
) {
}

async fn async_execute<T: diesel_async::AsyncConnection>(
    _: impl diesel_async::methods::ExecuteDsl<T>,
) {
}

fn main() {
    let query = diesel::insert_into(apps::table)
        .values(vec![(apps::name.eq("App 1")), (apps::name.eq("App 2"))]);

    execute::<SqliteConnection>(query); // Compiles without error
    async_execute::<SyncConnectionWrapper<SqliteConnection>>(query); // Does not compile
}

I think sync version compiles thanks to implementation located at diesel/src/query_builder/insert_statement/insert_with_default_for_sqlite.rs:215 which looks like:

impl<V, T, QId, C, Op, O, const STATIC_QUERY_ID: bool> ExecuteDsl<C, Sqlite>
    for InsertStatement<T, BatchInsert<Vec<ValuesClause<V, T>>, T, QId, STATIC_QUERY_ID>, Op>
    ...

I tried to move insert_with_default_for_sqlite.rs into diesel_async and test whether it works, however, it causes conflicting implementation for.

I would like to add this missing implementation (if it is acceptable by diesel_async). But I first would like to get your opinions about this problem and how it should be solved.

Thanks.

[weiznich]

Given that the relevant ExecuteDsl trait comes from diesel-async I would assume that this impl can be just written there. It likely requires exposing some of the helper traits used to restrict the impl on diesel itself (behind the unstable feature flag)

[bwqr]

About restructuring the traits, I think this requires much more effort and also might be hard to do without causing breaking changes.

Exposing sync connection to a given function sounds really good, though this means we need different treatment for AsyncPgConnection and sync wrapped connections. But users can create their own traits to unify this difference.
I had implemented this idea yesterday by creating a helper trait to unify both AsyncPgConnection and sync wrapper in order to at least keep the application code generic over the AsyncConnection and be able to swap connection at compile time.

However, there is one thing that keeps me from using sync wrapper and it is its tokio::spawn_blocking usage which is unfortunately not implemented for wasm targets. For this reason, I had developed my own async sqlite connection which just directly calls the sync connection's methods.
If it is okay for you, I would like to open a PR to add async fn with_sync(...) method and also put tokio::spawn_blocking usage behind a cfg attribute. With cfg attribute, we may directly call the sync connection's method and not use the tokio at all (for wasm targest).

[bwqr]

For the record, this commit shows how I implemented the helper trait.

[weiznich]

If it is okay for you, I would like to open a PR to add async fn with_sync(...)

This would be welcome

also put tokio::spawn_blocking usage behind a cfg attribute. With cfg attribute, we may directly call the sync connection's method and not use the tokio at all (for wasm targest).

I rather would prefer a different approach here. Instead of using cfg flags (which are hard to test) I would rather introduce an abstraction that allows to replace the exact used spawn_blocking method. We already have something similar for the AsyncConnectionWrapper here:

diesel_async/src/async_connection_wrapper.rs

Line 21 in c6f9dc8

pub trait BlockOn {

Basically we would introduce a similar trait for SpawnBlocking + a default implementation that uses tokio::spawn_blocking. This should allow you (and others) to just put in a different type implementing that trait to configure how spawn_blocking should be handled in your application. You then can easily use a different async runtime or just present that it's not blocking or whatever other solution seems reasonable for your use-case.

[bwqr]

Thanks for the reference. I will look into that.

[bwqr]

I opened the PR. Surprisingly, SyncConnectionWrapper was already supporting running a closure with its inner connection (with spawn_blocking function 😃). I changed a few bits to also support other runtimes with a method similar to BlockOn as you pointed out.