Skip to content

Commit ff93aa0

Browse files
iamjpottsiamjpotts
authored
refactor(sqlite): do not borrow bound values, delete lifetime on SqliteArguments (#3957)
* bug(sqlite): query macro argument lifetime use inconsistent with other db platforms Signed-off-by: Joshua Potts <[email protected]> * refactor(sqlite): Improve support for references as query macro bind arguments by removing lifetime parameter from SqliteArguments Signed-off-by: Joshua Potts <[email protected]> Signed-off-by: Joshua Potts <[email protected]> * refactor(sqlite): Introduce SqliteArgumentsBuffer type Signed-off-by: Joshua Potts <[email protected]> * refactor(sqlite): Improve cloning of SqliteArgumentValue, SqliteArguments, and SqliteArgumentsBuffer Signed-off-by: Joshua Potts <[email protected]> * refactor(any): Simplify AnyArguments::convert_to to convert_into Signed-off-by: Joshua Potts <[email protected]> --------- Signed-off-by: Joshua Potts <[email protected]>
1 parent e77f32e commit ff93aa0

File tree

22 files changed

+220
-274
lines changed

22 files changed

+220
-274
lines changed

‎sqlx-core/src/any/arguments.rs

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ impl Default for AnyArguments<'_> {
4242

4343
impl<'q> AnyArguments<'q> {
4444
#[doc(hidden)]
45-
pub fn convert_to<'a, A: Arguments<'a>>(&'a self) -> Result<A, BoxDynError>
45+
pub fn convert_into<'a, A: Arguments<'a>>(self) -> Result<A, BoxDynError>
4646
where
4747
'q: 'a,
4848
Option<i32>: Type<A::Database> + Encode<'a, A::Database>,
@@ -60,12 +60,12 @@ impl<'q> AnyArguments<'q> {
6060
i64: Type<A::Database> + Encode<'a, A::Database>,
6161
f32: Type<A::Database> + Encode<'a, A::Database>,
6262
f64: Type<A::Database> + Encode<'a, A::Database>,
63-
&'a str: Type<A::Database> + Encode<'a, A::Database>,
64-
&'a [u8]: Type<A::Database> + Encode<'a, A::Database>,
63+
String: Type<A::Database> + Encode<'a, A::Database>,
64+
Vec<u8>: Type<A::Database> + Encode<'a, A::Database>,
6565
{
6666
let mut out = A::default();
6767

68-
for arg in &self.values.0 {
68+
for arg in self.values.0 {
6969
match arg {
7070
AnyValueKind::Null(AnyTypeInfoKind::Null) => out.add(Option::<i32>::None),
7171
AnyValueKind::Null(AnyTypeInfoKind::Bool) => out.add(Option::<bool>::None),
@@ -82,8 +82,8 @@ impl<'q> AnyArguments<'q> {
8282
AnyValueKind::BigInt(i) => out.add(i),
8383
AnyValueKind::Real(r) => out.add(r),
8484
AnyValueKind::Double(d) => out.add(d),
85-
AnyValueKind::Text(t) => out.add(&**t),
86-
AnyValueKind::Blob(b) => out.add(&**b),
85+
AnyValueKind::Text(t) => out.add(String::from(t)),
86+
AnyValueKind::Blob(b) => out.add(Vec::from(b)),
8787
}?
8888
}
8989
Ok(out)

‎sqlx-mysql/src/any.rs

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -84,7 +84,7 @@ impl AnyConnectionBackend for MySqlConnection {
8484
arguments: Option<AnyArguments<'q>>,
8585
) -> BoxStream<'q, sqlx_core::Result<Either<AnyQueryResult, AnyRow>>> {
8686
let persistent = persistent && arguments.is_some();
87-
let arguments = match arguments.as_ref().map(AnyArguments::convert_to).transpose() {
87+
let arguments = match arguments.map(AnyArguments::convert_into).transpose() {
8888
Ok(arguments) => arguments,
8989
Err(error) => {
9090
return stream::once(future::ready(Err(sqlx_core::Error::Encode(error)))).boxed()
@@ -111,8 +111,7 @@ impl AnyConnectionBackend for MySqlConnection {
111111
) -> BoxFuture<'q, sqlx_core::Result<Option<AnyRow>>> {
112112
let persistent = persistent && arguments.is_some();
113113
let arguments = arguments
114-
.as_ref()
115-
.map(AnyArguments::convert_to)
114+
.map(AnyArguments::convert_into)
116115
.transpose()
117116
.map_err(sqlx_core::Error::Encode);
118117

‎sqlx-postgres/src/any.rs

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -86,7 +86,7 @@ impl AnyConnectionBackend for PgConnection {
8686
arguments: Option<AnyArguments<'q>>,
8787
) -> BoxStream<'q, sqlx_core::Result<Either<AnyQueryResult, AnyRow>>> {
8888
let persistent = persistent && arguments.is_some();
89-
let arguments = match arguments.as_ref().map(AnyArguments::convert_to).transpose() {
89+
let arguments = match arguments.map(AnyArguments::convert_into).transpose() {
9090
Ok(arguments) => arguments,
9191
Err(error) => {
9292
return stream::once(future::ready(Err(sqlx_core::Error::Encode(error)))).boxed()
@@ -113,8 +113,7 @@ impl AnyConnectionBackend for PgConnection {
113113
) -> BoxFuture<'q, sqlx_core::Result<Option<AnyRow>>> {
114114
let persistent = persistent && arguments.is_some();
115115
let arguments = arguments
116-
.as_ref()
117-
.map(AnyArguments::convert_to)
116+
.map(AnyArguments::convert_into)
118117
.transpose()
119118
.map_err(sqlx_core::Error::Encode);
120119

‎sqlx-sqlite/src/any.rs

Lines changed: 24 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -12,13 +12,15 @@ use sqlx_core::any::{
1212
};
1313
use sqlx_core::sql_str::SqlStr;
1414

15+
use crate::arguments::SqliteArgumentsBuffer;
1516
use crate::type_info::DataType;
1617
use sqlx_core::connection::{ConnectOptions, Connection};
1718
use sqlx_core::database::Database;
1819
use sqlx_core::describe::Describe;
1920
use sqlx_core::executor::Executor;
2021
use sqlx_core::transaction::TransactionManager;
2122
use std::pin::pin;
23+
use std::sync::Arc;
2224

2325
sqlx_core::declare_driver_with_optional_migrate!(DRIVER = Sqlite);
2426

@@ -203,27 +205,29 @@ impl<'a> TryFrom<&'a AnyConnectOptions> for SqliteConnectOptions {
203205
}
204206
}
205207

206-
/// Instead of `AnyArguments::convert_into()`, we can do a direct mapping and preserve the lifetime.
207-
fn map_arguments(args: AnyArguments<'_>) -> SqliteArguments<'_> {
208+
// Infallible alternative to AnyArguments::convert_into()
209+
fn map_arguments(args: AnyArguments<'_>) -> SqliteArguments {
210+
let values = args
211+
.values
212+
.0
213+
.into_iter()
214+
.map(|val| match val {
215+
AnyValueKind::Null(_) => SqliteArgumentValue::Null,
216+
AnyValueKind::Bool(b) => SqliteArgumentValue::Int(b as i32),
217+
AnyValueKind::SmallInt(i) => SqliteArgumentValue::Int(i as i32),
218+
AnyValueKind::Integer(i) => SqliteArgumentValue::Int(i),
219+
AnyValueKind::BigInt(i) => SqliteArgumentValue::Int64(i),
220+
AnyValueKind::Real(r) => SqliteArgumentValue::Double(r as f64),
221+
AnyValueKind::Double(d) => SqliteArgumentValue::Double(d),
222+
AnyValueKind::Text(t) => SqliteArgumentValue::Text(Arc::new(t.to_string())),
223+
AnyValueKind::Blob(b) => SqliteArgumentValue::Blob(Arc::new(b.to_vec())),
224+
// AnyValueKind is `#[non_exhaustive]` but we should have covered everything
225+
_ => unreachable!("BUG: missing mapping for {val:?}"),
226+
})
227+
.collect();
228+
208229
SqliteArguments {
209-
values: args
210-
.values
211-
.0
212-
.into_iter()
213-
.map(|val| match val {
214-
AnyValueKind::Null(_) => SqliteArgumentValue::Null,
215-
AnyValueKind::Bool(b) => SqliteArgumentValue::Int(b as i32),
216-
AnyValueKind::SmallInt(i) => SqliteArgumentValue::Int(i as i32),
217-
AnyValueKind::Integer(i) => SqliteArgumentValue::Int(i),
218-
AnyValueKind::BigInt(i) => SqliteArgumentValue::Int64(i),
219-
AnyValueKind::Real(r) => SqliteArgumentValue::Double(r as f64),
220-
AnyValueKind::Double(d) => SqliteArgumentValue::Double(d),
221-
AnyValueKind::Text(t) => SqliteArgumentValue::Text(t),
222-
AnyValueKind::Blob(b) => SqliteArgumentValue::Blob(b),
223-
// AnyValueKind is `#[non_exhaustive]` but we should have covered everything
224-
_ => unreachable!("BUG: missing mapping for {val:?}"),
225-
})
226-
.collect(),
230+
values: SqliteArgumentsBuffer::new(values),
227231
}
228232
}
229233

‎sqlx-sqlite/src/arguments.rs

Lines changed: 30 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -4,62 +4,56 @@ use crate::statement::StatementHandle;
44
use crate::Sqlite;
55
use atoi::atoi;
66
use libsqlite3_sys::SQLITE_OK;
7-
use std::borrow::Cow;
7+
use std::sync::Arc;
88

99
pub(crate) use sqlx_core::arguments::*;
1010
use sqlx_core::error::BoxDynError;
1111

1212
#[derive(Debug, Clone)]
13-
pub enum SqliteArgumentValue<'q> {
13+
pub enum SqliteArgumentValue {
1414
Null,
15-
Text(Cow<'q, str>),
16-
Blob(Cow<'q, [u8]>),
15+
Text(Arc<String>),
16+
TextSlice(Arc<str>),
17+
Blob(Arc<Vec<u8>>),
1718
Double(f64),
1819
Int(i32),
1920
Int64(i64),
2021
}
2122

2223
#[derive(Default, Debug, Clone)]
23-
pub struct SqliteArguments<'q> {
24-
pub(crate) values: Vec<SqliteArgumentValue<'q>>,
24+
pub struct SqliteArguments {
25+
pub(crate) values: SqliteArgumentsBuffer,
2526
}
2627

27-
impl<'q> SqliteArguments<'q> {
28+
#[derive(Default, Debug, Clone)]
29+
pub struct SqliteArgumentsBuffer(Vec<SqliteArgumentValue>);
30+
31+
impl<'q> SqliteArguments {
2832
pub(crate) fn add<T>(&mut self, value: T) -> Result<(), BoxDynError>
2933
where
3034
T: Encode<'q, Sqlite>,
3135
{
32-
let value_length_before_encoding = self.values.len();
36+
let value_length_before_encoding = self.values.0.len();
3337

3438
match value.encode(&mut self.values) {
35-
Ok(IsNull::Yes) => self.values.push(SqliteArgumentValue::Null),
39+
Ok(IsNull::Yes) => self.values.0.push(SqliteArgumentValue::Null),
3640
Ok(IsNull::No) => {}
3741
Err(error) => {
3842
// reset the value buffer to its previous value if encoding failed so we don't leave a half-encoded value behind
39-
self.values.truncate(value_length_before_encoding);
43+
self.values.0.truncate(value_length_before_encoding);
4044
return Err(error);
4145
}
4246
};
4347

4448
Ok(())
4549
}
46-
47-
pub(crate) fn into_static(self) -> SqliteArguments<'static> {
48-
SqliteArguments {
49-
values: self
50-
.values
51-
.into_iter()
52-
.map(SqliteArgumentValue::into_static)
53-
.collect(),
54-
}
55-
}
5650
}
5751

58-
impl<'q> Arguments<'q> for SqliteArguments<'q> {
52+
impl<'q> Arguments<'q> for SqliteArguments {
5953
type Database = Sqlite;
6054

6155
fn reserve(&mut self, len: usize, _size_hint: usize) {
62-
self.values.reserve(len);
56+
self.values.0.reserve(len);
6357
}
6458

6559
fn add<T>(&mut self, value: T) -> Result<(), BoxDynError>
@@ -70,11 +64,11 @@ impl<'q> Arguments<'q> for SqliteArguments<'q> {
7064
}
7165

7266
fn len(&self) -> usize {
73-
self.values.len()
67+
self.values.0.len()
7468
}
7569
}
7670

77-
impl SqliteArguments<'_> {
71+
impl SqliteArguments {
7872
pub(super) fn bind(&self, handle: &mut StatementHandle, offset: usize) -> Result<usize, Error> {
7973
let mut arg_i = offset;
8074
// for handle in &statement.handles {
@@ -103,7 +97,7 @@ impl SqliteArguments<'_> {
10397
arg_i
10498
};
10599

106-
if n > self.values.len() {
100+
if n > self.values.0.len() {
107101
// SQLite treats unbound variables as NULL
108102
// we reproduce this here
109103
// If you are reading this and think this should be an error, open an issue and we can
@@ -113,32 +107,31 @@ impl SqliteArguments<'_> {
113107
break;
114108
}
115109

116-
self.values[n - 1].bind(handle, param_i)?;
110+
self.values.0[n - 1].bind(handle, param_i)?;
117111
}
118112

119113
Ok(arg_i - offset)
120114
}
121115
}
122116

123-
impl SqliteArgumentValue<'_> {
124-
fn into_static(self) -> SqliteArgumentValue<'static> {
125-
use SqliteArgumentValue::*;
117+
impl SqliteArgumentsBuffer {
118+
#[allow(dead_code)] // clippy incorrectly reports this as unused
119+
pub(crate) fn new(values: Vec<SqliteArgumentValue>) -> SqliteArgumentsBuffer {
120+
Self(values)
121+
}
126122

127-
match self {
128-
Null => Null,
129-
Text(text) => Text(text.into_owned().into()),
130-
Blob(blob) => Blob(blob.into_owned().into()),
131-
Int(v) => Int(v),
132-
Int64(v) => Int64(v),
133-
Double(v) => Double(v),
134-
}
123+
pub(crate) fn push(&mut self, value: SqliteArgumentValue) {
124+
self.0.push(value);
135125
}
126+
}
136127

128+
impl SqliteArgumentValue {
137129
fn bind(&self, handle: &mut StatementHandle, i: usize) -> Result<(), Error> {
138130
use SqliteArgumentValue::*;
139131

140132
let status = match self {
141133
Text(v) => handle.bind_text(i, v),
134+
TextSlice(v) => handle.bind_text(i, v),
142135
Blob(v) => handle.bind_blob(i, v),
143136
Int(v) => handle.bind_int(i, *v),
144137
Int64(v) => handle.bind_int64(i, *v),

‎sqlx-sqlite/src/connection/execute.rs

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ pub struct ExecuteIter<'a> {
1010
handle: &'a mut ConnectionHandle,
1111
statement: &'a mut VirtualStatement,
1212
logger: QueryLogger,
13-
args: Option<SqliteArguments<'a>>,
13+
args: Option<SqliteArguments>,
1414

1515
/// since a `VirtualStatement` can encompass multiple actual statements,
1616
/// this keeps track of the number of arguments so far
@@ -19,12 +19,12 @@ pub struct ExecuteIter<'a> {
1919
goto_next: bool,
2020
}
2121

22-
pub(crate) fn iter<'a>(
23-
conn: &'a mut ConnectionState,
22+
pub(crate) fn iter(
23+
conn: &mut ConnectionState,
2424
query: impl SqlSafeStr,
25-
args: Option<SqliteArguments<'a>>,
25+
args: Option<SqliteArguments>,
2626
persistent: bool,
27-
) -> Result<ExecuteIter<'a>, Error> {
27+
) -> Result<ExecuteIter<'_>, Error> {
2828
let query = query.into_sql_str();
2929
// fetch the cached statement or allocate a new one
3030
let statement = conn.statements.get(query.as_str(), persistent)?;
@@ -43,7 +43,7 @@ pub(crate) fn iter<'a>(
4343

4444
fn bind(
4545
statement: &mut StatementHandle,
46-
arguments: &Option<SqliteArguments<'_>>,
46+
arguments: &Option<SqliteArguments>,
4747
offset: usize,
4848
) -> Result<usize, Error> {
4949
let mut n = 0;
@@ -56,7 +56,7 @@ fn bind(
5656
}
5757

5858
impl ExecuteIter<'_> {
59-
pub fn finish(&mut self) -> Result<(), Error> {
59+
pub fn finish(self) -> Result<(), Error> {
6060
for res in self {
6161
let _ = res?;
6262
}

‎sqlx-sqlite/src/connection/worker.rs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,7 @@ enum Command {
6363
},
6464
Execute {
6565
query: SqlStr,
66-
arguments: Option<SqliteArguments<'static>>,
66+
arguments: Option<SqliteArguments>,
6767
persistent: bool,
6868
tx: flume::Sender<Result<Either<SqliteQueryResult, SqliteRow>, Error>>,
6969
limit: Option<usize>,
@@ -360,7 +360,7 @@ impl ConnectionWorker {
360360
pub(crate) async fn execute(
361361
&mut self,
362362
query: SqlStr,
363-
args: Option<SqliteArguments<'_>>,
363+
args: Option<SqliteArguments>,
364364
chan_size: usize,
365365
persistent: bool,
366366
limit: Option<usize>,
@@ -371,7 +371,7 @@ impl ConnectionWorker {
371371
.send_async((
372372
Command::Execute {
373373
query,
374-
arguments: args.map(SqliteArguments::into_static),
374+
arguments: args,
375375
persistent,
376376
tx,
377377
limit,

‎sqlx-sqlite/src/database.rs

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,9 @@
11
pub(crate) use sqlx_core::database::{Database, HasStatementCache};
22

3+
use crate::arguments::SqliteArgumentsBuffer;
34
use crate::{
4-
SqliteArgumentValue, SqliteArguments, SqliteColumn, SqliteConnection, SqliteQueryResult,
5-
SqliteRow, SqliteStatement, SqliteTransactionManager, SqliteTypeInfo, SqliteValue,
6-
SqliteValueRef,
5+
SqliteArguments, SqliteColumn, SqliteConnection, SqliteQueryResult, SqliteRow, SqliteStatement,
6+
SqliteTransactionManager, SqliteTypeInfo, SqliteValue, SqliteValueRef,
77
};
88

99
/// Sqlite database driver.
@@ -26,8 +26,8 @@ impl Database for Sqlite {
2626
type Value = SqliteValue;
2727
type ValueRef<'r> = SqliteValueRef<'r>;
2828

29-
type Arguments<'q> = SqliteArguments<'q>;
30-
type ArgumentBuffer<'q> = Vec<SqliteArgumentValue<'q>>;
29+
type Arguments<'q> = SqliteArguments;
30+
type ArgumentBuffer<'q> = SqliteArgumentsBuffer;
3131

3232
type Statement = SqliteStatement;
3333

‎sqlx-sqlite/src/lib.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,7 @@ extern crate sqlx_core;
7474

7575
use std::sync::atomic::AtomicBool;
7676

77-
pub use arguments::{SqliteArgumentValue, SqliteArguments};
77+
pub use arguments::{SqliteArgumentValue, SqliteArguments, SqliteArgumentsBuffer};
7878
pub use column::SqliteColumn;
7979
#[cfg(feature = "deserialize")]
8080
#[cfg_attr(docsrs, doc(cfg(feature = "deserialize")))]
@@ -147,7 +147,7 @@ impl<'c, T: Executor<'c, Database = Sqlite>> SqliteExecutor<'c> for T {}
147147
pub type SqliteTransaction<'c> = sqlx_core::transaction::Transaction<'c, Sqlite>;
148148

149149
// NOTE: required due to the lack of lazy normalization
150-
impl_into_arguments_for_arguments!(SqliteArguments<'q>);
150+
impl_into_arguments_for_arguments!(SqliteArguments);
151151
impl_column_index_for_row!(SqliteRow);
152152
impl_column_index_for_statement!(SqliteStatement);
153153
impl_acquire!(Sqlite, SqliteConnection);

0 commit comments

Comments
 (0)