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

Libsql client: execute batch #607

Merged
merged 5 commits into from
Nov 13, 2023

Conversation

Horusiath
Copy link
Contributor

@Horusiath Horusiath commented Nov 12, 2023

Add support for executing batch statements (in rusqlite fashion) to libsql client: including Cloudflare compilation target.

There was a small mismatch in an API:

  • rusqlite expects execute_batch(sql: &str): all statements are glued together into a single string.
  • Hrana protocol expects execute batch to give an array of individual statements to execute.

This means that we need to at least parse requests on the client side (which has its upsides and downsides), so I pulled sqlite3-parser out of the replication module and make it a dependency on hrana feature as well.

/cc @psarna @LucioFranco

@Horusiath Horusiath marked this pull request as ready for review November 13, 2023 02:11
@psarna
Copy link
Collaborator

psarna commented Nov 13, 2023

@Horusiath I think everyone agrees that rusqlite's batch API that accepts a single string is not the best match for us, so perhaps we should only add it as a compatibility layer? By that I mean:

  1. Have a proper function that accepts a slice/array/whatever of statements (related: libsql crate: allow passing multiple statements with bound parameters to execute_batch #562)
  2. Based on that function, we also add one that accepts a &str of glued statements, and does all the unfortunate reparsing.

Then we can get our users used to the "proper" API, and gradually deprecate the &str one, which doesn't accept bound parameters either, so it's a security issue as well, begs for SQL injection.

@psarna
Copy link
Collaborator

psarna commented Nov 13, 2023

we already have raw_batch from the libsql-client-rs days, so I think it's only a matter of exposing another function that accepts a slice of statements and uses raw_batch underneath?

Copy link
Contributor

@LucioFranco LucioFranco left a comment

Choose a reason for hiding this comment

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

LGTM! Really nice work!

@@ -1,3 +1,5 @@
#![allow(dead_code)]
Copy link
Contributor

Choose a reason for hiding this comment

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

When does this warning trigger? We could use cfg_attr to only enable this in certain feature flag situations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's mostly happening since I moved the parser.rs as an entire file, however in context of hrana we're only using a small subset of its functions.

pub(crate) ResultRows,
pub(crate) usize,
);
pub(crate) struct RemoteRows(pub(crate) ResultRows, pub(crate) usize);
Copy link
Contributor

Choose a reason for hiding this comment

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

is this rustfmt making this change? Seems odd since I should be running it on every save.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apparently so. I have fmt running on every file save.

@LucioFranco
Copy link
Contributor

@psarna I think you're right, we should instead accept a IntoStatements trait that we can impl for &str and IntoIterator<impl Into<Statement>>. That said, this is fine for now, I want to ship this instead of waiting for that change.

@LucioFranco
Copy link
Contributor

and we already have a separate issue for it #562, lets do this in a follow up when we have time.

@LucioFranco LucioFranco added this pull request to the merge queue Nov 13, 2023
Merged via the queue into tursodatabase:main with commit a30080f Nov 13, 2023
6 checks passed
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.

3 participants