Skip to content
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

Consider migrating sqlx-sqlite to use rusqlite internally #2656

Closed
nyurik opened this issue Jul 26, 2023 · 1 comment
Closed

Consider migrating sqlx-sqlite to use rusqlite internally #2656

nyurik opened this issue Jul 26, 2023 · 1 comment
Labels
enhancement New feature or request

Comments

@nyurik
Copy link
Contributor

nyurik commented Jul 26, 2023

Is your feature request related to a problem? Please describe.
rusqlite crate has been rapidly evolving, adding useful features such as create_scalar_function (regex example).

SQLx is an amazing library with focus on async cross-DB support with pooling and query validation, without the goal of matching entire SQLite API, so it is expected that SQLx will never cover all features of a specific DB.

Describe the solution you'd like
A way to use all/most of rusqlite functionality from the sqlx::SqliteConnection in a safe way.

Ideally sqlx::SqliteConnection should be a thin wrapper over rusqlite::Connection, plus re-export the entire rusqlite library to avoid dependency conflicts.

Describe alternatives you've considered
Using rusqlite side-by-side with SQLx and gluing them with unsafe code (this code is a rough draft, possibly incorrect)

async fn add_md5(&self, conn: &mut SqliteConnection) -> Result<String> {
    let mut handle = conn.lock_handle().await?;
    let conn2 =
        unsafe { rusqlite::Connection::from_handle(handle.as_raw_handle().as_mut()).unwrap() };
    conn2
        .create_scalar_function("md5", 1, SQLITE_UTF8 | SQLITE_DETERMINISTIC, calc_md5)
        .unwrap();

    // FIXME: query is used on a connection that is currently mutably borrowed by rusqlite
    query(r#"SELECT md5("abc")"#).fetch_one(conn).await

    // FIXME: possible memory leak: the function is attached on a Connection that doesn't own handle
}

fn calc_md5(ctx: &Context) -> rusqlite::Result<Option<Vec<u8>>> {
   ...
}
@nyurik nyurik added the enhancement New feature or request label Jul 26, 2023
@abonander
Copy link
Collaborator

abonander commented Aug 1, 2023

I think evolving separately from rusqlite is healthier for the ecosystem as a whole.

We're not beholden to their API design when choosing how to implement ours.

If they stop maintaining libsqlite3-sys it's not as big of a lift to migrate to another wrapper or even our own, since it's generated from sqlite3.h anyway.

Also, now that we're considering the libsqlite3-sys linkage to be semver-exempt we can upgrade it as needed to address CVEs for our users in backwards-compatible releases. We would only ever offer rusqlite compatibility under a similar exemption, which would limit its utility greatly.

I'd prefer just discussing the addition of specific features instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants