From 723933f83690e7065b3fd6c40a34d31fe6b7aed1 Mon Sep 17 00:00:00 2001 From: Mohamed Belaouad Date: Wed, 3 Apr 2024 12:36:45 +0200 Subject: [PATCH 1/3] Add MoveableBindCollector and CollectedQuery --- diesel/src/connection/mod.rs | 10 +++ diesel/src/query_builder/ast_pass.rs | 31 +++++++- diesel/src/query_builder/bind_collector.rs | 37 +++++++++ diesel/src/query_builder/collected_query.rs | 51 +++++++++++++ diesel/src/query_builder/mod.rs | 5 +- .../src/sqlite/connection/bind_collector.rs | 75 ++++++++++++++++++- diesel/src/sqlite/connection/mod.rs | 11 ++- 7 files changed, 216 insertions(+), 4 deletions(-) create mode 100644 diesel/src/query_builder/collected_query.rs diff --git a/diesel/src/connection/mod.rs b/diesel/src/connection/mod.rs index 2ad7df2ba1b8..4101ecb331b0 100644 --- a/diesel/src/connection/mod.rs +++ b/diesel/src/connection/mod.rs @@ -14,6 +14,7 @@ use crate::backend::Backend; use crate::expression::QueryMetadata; use crate::query_builder::{Query, QueryFragment, QueryId}; use crate::result::*; +use crate::sql_types::TypeMetadata; use std::fmt::Debug; #[doc(inline)] @@ -444,6 +445,15 @@ pub trait LoadConnection: Connection { Self::Backend: QueryMetadata; } +/// Describes a connection with an underlying [`crate::sql_types::TypeMetadata::MetadataLookup`] +#[diesel_derives::__diesel_public_if( + feature = "i-implement-a-third-party-backend-and-opt-into-breaking-changes" +)] +pub trait WithMetadataLookup: Connection { + /// Retrieves the underlying metadata lookup + fn metadata_lookup(&mut self) -> &mut ::MetadataLookup; +} + /// A variant of the [`Connection`](trait.Connection.html) trait that is /// usable with dynamic dispatch /// diff --git a/diesel/src/query_builder/ast_pass.rs b/diesel/src/query_builder/ast_pass.rs index f1bf8c2a1d45..90ce1544d7c9 100644 --- a/diesel/src/query_builder/ast_pass.rs +++ b/diesel/src/query_builder/ast_pass.rs @@ -1,7 +1,7 @@ use std::fmt; use crate::backend::Backend; -use crate::query_builder::{BindCollector, QueryBuilder}; +use crate::query_builder::{BindCollector, MoveableBindCollector, QueryBuilder}; use crate::result::QueryResult; use crate::serialize::ToSql; use crate::sql_types::HasSqlType; @@ -252,6 +252,35 @@ where Ok(()) } + /// Push bind collector values from its data onto the query + /// + /// This method works with [MoveableBindCollector] data [MoveableBindCollector::BindData] + /// and is used with already collected query meaning its SQL is already built and its + /// bind data already collected. + #[diesel_derives::__diesel_public_if( + feature = "i-implement-a-third-party-backend-and-opt-into-breaking-changes" + )] + pub(crate) fn push_bind_collector_data( + &mut self, + bind_collector_data: &MD, + ) -> QueryResult<()> + where + DB: Backend, + for<'bc> DB::BindCollector<'bc>: MoveableBindCollector, + { + match self.internals { + AstPassInternals::CollectBinds { + ref mut collector, + metadata_lookup: _, + } => collector.append_bind_data(bind_collector_data), + AstPassInternals::DebugBinds(ref mut f) => { + f.push(Box::new("Opaque bind collector data")) + } + _ => {} + } + Ok(()) + } + /// Get information about the backend that will consume this query #[cfg_attr( not(feature = "i-implement-a-third-party-backend-and-opt-into-breaking-changes"), diff --git a/diesel/src/query_builder/bind_collector.rs b/diesel/src/query_builder/bind_collector.rs index 7db821a75a98..a11cc568c496 100644 --- a/diesel/src/query_builder/bind_collector.rs +++ b/diesel/src/query_builder/bind_collector.rs @@ -46,6 +46,23 @@ pub trait BindCollector<'a, DB: TypeMetadata>: Sized { } } +/// A movable version of the bind collector which allows it to be extracted, moved and refilled. +/// +/// This is mostly useful in async context where bind data needs to be moved across threads. +#[diesel_derives::__diesel_public_if( + feature = "i-implement-a-third-party-backend-and-opt-into-breaking-changes" +)] +pub trait MoveableBindCollector { + /// The movable bind data of this bind collector + type BindData: Send + 'static; + + /// Builds a movable version of the bind collector + fn moveable(&self) -> Self::BindData; + + /// Refill the bind collector with its bind data + fn append_bind_data(&mut self, from: &Self::BindData); +} + #[derive(Debug)] /// A bind collector used by backends which transmit bind parameters as an /// opaque blob of bytes. @@ -125,6 +142,26 @@ where } } +impl MoveableBindCollector for RawBytesBindCollector +where + for<'a> DB: Backend = Self> + TypeMetadata + 'static, + ::TypeMetadata: Clone + Send, +{ + type BindData = Self; + + fn moveable(&self) -> Self::BindData { + RawBytesBindCollector { + binds: self.binds.clone(), + metadata: self.metadata.clone(), + } + } + + fn append_bind_data(&mut self, from: &Self::BindData) { + self.binds.extend(from.binds.iter().cloned()); + self.metadata.extend(from.metadata.clone()); + } +} + // This is private for now as we may want to add `Into` impls for the wrapper type // later on mod private { diff --git a/diesel/src/query_builder/collected_query.rs b/diesel/src/query_builder/collected_query.rs new file mode 100644 index 000000000000..1ffd74c1e458 --- /dev/null +++ b/diesel/src/query_builder/collected_query.rs @@ -0,0 +1,51 @@ +use super::{AstPass, MoveableBindCollector, Query, QueryFragment, QueryId}; +use crate::backend::{Backend, DieselReserveSpecialization}; +use crate::result::QueryResult; +use crate::sql_types::Untyped; + +#[derive(Debug)] +#[must_use = "Queries are only executed when calling `load`, `get_result` or similar."] +/// A SQL query variant with already collected bind data which can be moved +#[diesel_derives::__diesel_public_if( + feature = "i-implement-a-third-party-backend-and-opt-into-breaking-changes" +)] +pub struct CollectedQuery { + sql: String, + safe_to_cache_prepared: bool, + bind_data: T, +} + +impl CollectedQuery { + /// Builds a [CollectedQuery] with movable bind data + pub fn new(sql: String, safe_to_cache_prepared: bool, bind_data: T) -> Self { + Self { + sql, + safe_to_cache_prepared, + bind_data, + } + } +} + +impl QueryFragment for CollectedQuery +where + DB: Backend + DieselReserveSpecialization, + for<'a> ::BindCollector<'a>: MoveableBindCollector, +{ + fn walk_ast<'b>(&'b self, mut pass: AstPass<'_, 'b, DB>) -> QueryResult<()> { + if !self.safe_to_cache_prepared { + pass.unsafe_to_cache_prepared(); + } + pass.push_sql(&self.sql); + pass.push_bind_collector_data::(&self.bind_data) + } +} + +impl QueryId for CollectedQuery { + type QueryId = (); + + const HAS_STATIC_QUERY_ID: bool = false; +} + +impl Query for CollectedQuery { + type SqlType = Untyped; +} diff --git a/diesel/src/query_builder/mod.rs b/diesel/src/query_builder/mod.rs index 6ce2750cbbf1..ca6fe87c0d85 100644 --- a/diesel/src/query_builder/mod.rs +++ b/diesel/src/query_builder/mod.rs @@ -11,6 +11,7 @@ mod clause_macro; pub(crate) mod ast_pass; pub mod bind_collector; +mod collected_query; pub(crate) mod combination_clause; mod debug_query; mod delete_statement; @@ -37,7 +38,9 @@ pub(crate) mod where_clause; #[doc(inline)] pub use self::ast_pass::AstPass; #[doc(inline)] -pub use self::bind_collector::BindCollector; +pub use self::bind_collector::{BindCollector, MoveableBindCollector}; +#[doc(inline)] +pub use self::collected_query::CollectedQuery; #[doc(inline)] pub use self::debug_query::DebugQuery; #[doc(inline)] diff --git a/diesel/src/sqlite/connection/bind_collector.rs b/diesel/src/sqlite/connection/bind_collector.rs index e276c1b5b0c1..df4306632755 100644 --- a/diesel/src/sqlite/connection/bind_collector.rs +++ b/diesel/src/sqlite/connection/bind_collector.rs @@ -1,4 +1,4 @@ -use crate::query_builder::BindCollector; +use crate::query_builder::{BindCollector, MoveableBindCollector}; use crate::serialize::{IsNull, Output}; use crate::sql_types::HasSqlType; use crate::sqlite::{Sqlite, SqliteType}; @@ -200,3 +200,76 @@ impl<'a> BindCollector<'a, Sqlite> for SqliteBindCollector<'a> { Ok(()) } } + +#[derive(Debug)] +enum OwnedSqliteBindValue { + String(Box), + Binary(Box<[u8]>), + I32(i32), + I64(i64), + F64(f64), + Null, +} + +impl<'a> std::convert::From<&InternalSqliteBindValue<'a>> for OwnedSqliteBindValue { + fn from(value: &InternalSqliteBindValue<'a>) -> Self { + match value { + InternalSqliteBindValue::String(s) => Self::String(s.clone()), + InternalSqliteBindValue::BorrowedString(s) => { + Self::String(String::from(*s).into_boxed_str()) + } + InternalSqliteBindValue::Binary(b) => Self::Binary(b.clone()), + InternalSqliteBindValue::BorrowedBinary(s) => { + Self::Binary(Vec::from(*s).into_boxed_slice()) + } + InternalSqliteBindValue::I32(val) => Self::I32(*val), + InternalSqliteBindValue::I64(val) => Self::I64(*val), + InternalSqliteBindValue::F64(val) => Self::F64(*val), + InternalSqliteBindValue::Null => Self::Null, + } + } +} + +impl<'a> std::convert::From<&OwnedSqliteBindValue> for InternalSqliteBindValue<'a> { + fn from(value: &OwnedSqliteBindValue) -> Self { + match value { + OwnedSqliteBindValue::String(s) => Self::String(s.clone()), + OwnedSqliteBindValue::Binary(b) => Self::Binary(b.clone()), + OwnedSqliteBindValue::I32(val) => Self::I32(*val), + OwnedSqliteBindValue::I64(val) => Self::I64(*val), + OwnedSqliteBindValue::F64(val) => Self::F64(*val), + OwnedSqliteBindValue::Null => Self::Null, + } + } +} + +#[derive(Debug)] +/// Sqlite bind collector data that is movable across threads +pub struct SqliteBindCollectorData { + binds: Vec<(OwnedSqliteBindValue, SqliteType)>, +} + +impl MoveableBindCollector for SqliteBindCollector<'_> { + type BindData = SqliteBindCollectorData; + + fn moveable(&self) -> Self::BindData { + let mut binds = Vec::with_capacity(self.binds.len()); + for b in self + .binds + .iter() + .map(|(bind, tpe)| (OwnedSqliteBindValue::from(bind), *tpe)) + { + binds.push(b); + } + SqliteBindCollectorData { binds } + } + + fn append_bind_data(&mut self, from: &Self::BindData) { + self.binds.reserve_exact(from.binds.len()); + self.binds.extend( + from.binds + .iter() + .map(|(bind, tpe)| (InternalSqliteBindValue::from(bind), *tpe)), + ); + } +} diff --git a/diesel/src/sqlite/connection/mod.rs b/diesel/src/sqlite/connection/mod.rs index db1e7d294195..dab81bc92fbf 100644 --- a/diesel/src/sqlite/connection/mod.rs +++ b/diesel/src/sqlite/connection/mod.rs @@ -28,7 +28,7 @@ use crate::expression::QueryMetadata; use crate::query_builder::*; use crate::result::*; use crate::serialize::ToSql; -use crate::sql_types::HasSqlType; +use crate::sql_types::{HasSqlType, TypeMetadata}; use crate::sqlite::Sqlite; /// Connections for the SQLite backend. Unlike other backends, SQLite supported @@ -220,6 +220,15 @@ impl LoadConnection for SqliteConnection { } } +static mut SQLITE_METADATA_LOOKUP: () = (); + +impl WithMetadataLookup for SqliteConnection { + fn metadata_lookup(&mut self) -> &mut ::MetadataLookup { + // safe since it's a unit type + unsafe { &mut SQLITE_METADATA_LOOKUP } + } +} + #[cfg(feature = "r2d2")] impl crate::r2d2::R2D2Connection for crate::sqlite::SqliteConnection { fn ping(&mut self) -> QueryResult<()> { From 0b3699d7ab2ef3bff3831d6a9ea5657cab8df10b Mon Sep 17 00:00:00 2001 From: Mohamed Belaouad Date: Wed, 3 Apr 2024 12:40:53 +0200 Subject: [PATCH 2/3] Add owned version of Sqlite row --- diesel/src/row.rs | 12 +++ diesel/src/sqlite/connection/mod.rs | 1 + diesel/src/sqlite/connection/owned_row.rs | 90 ++++++++++++++++++++ diesel/src/sqlite/connection/row.rs | 42 ++++++++- diesel/src/sqlite/connection/sqlite_value.rs | 30 ++++++- 5 files changed, 172 insertions(+), 3 deletions(-) create mode 100644 diesel/src/sqlite/connection/owned_row.rs diff --git a/diesel/src/row.rs b/diesel/src/row.rs index a6bbcc3d31bb..b084e0852dbf 100644 --- a/diesel/src/row.rs +++ b/diesel/src/row.rs @@ -148,6 +148,18 @@ where } } +/// A row that can be turned into an owned version +#[diesel_derives::__diesel_public_if( + feature = "i-implement-a-third-party-backend-and-opt-into-breaking-changes" +)] +pub trait IntoOwnedRow<'a, DB: Backend>: Row<'a, DB> { + /// The owned version of the row + type OwnedRow: Row<'a, DB> + Send + 'static; + + /// Turn the row into its owned version + fn into_owned(self) -> Self::OwnedRow; +} + // These traits are not part of the public API // because: // * we want to control who can implement `Row` (for `RowSealed`) diff --git a/diesel/src/sqlite/connection/mod.rs b/diesel/src/sqlite/connection/mod.rs index dab81bc92fbf..38fd7e6ff6a1 100644 --- a/diesel/src/sqlite/connection/mod.rs +++ b/diesel/src/sqlite/connection/mod.rs @@ -2,6 +2,7 @@ extern crate libsqlite3_sys as ffi; mod bind_collector; mod functions; +mod owned_row; mod raw; mod row; mod serialized_database; diff --git a/diesel/src/sqlite/connection/owned_row.rs b/diesel/src/sqlite/connection/owned_row.rs new file mode 100644 index 000000000000..700c8b341d71 --- /dev/null +++ b/diesel/src/sqlite/connection/owned_row.rs @@ -0,0 +1,90 @@ +use super::sqlite_value::{OwnedSqliteValue, SqliteValue}; +use crate::backend::Backend; +use crate::row::{Field, PartialRow, Row, RowIndex, RowSealed}; +use crate::sqlite::Sqlite; + +#[derive(Debug)] +pub struct OwnedSqliteRow { + pub(super) values: Vec>, + column_names: Box<[Option]>, +} + +impl OwnedSqliteRow { + pub(super) fn new( + values: Vec>, + column_names: Box<[Option]>, + ) -> Self { + OwnedSqliteRow { + values, + column_names, + } + } +} + +impl RowSealed for OwnedSqliteRow {} + +impl<'a> Row<'a, Sqlite> for OwnedSqliteRow { + type Field<'field> = OwnedSqliteField<'field> where 'a: 'field, Self: 'field; + type InnerPartialRow = Self; + + fn field_count(&self) -> usize { + self.values.len() + } + + fn get<'field, I>(&'field self, idx: I) -> Option> + where + 'a: 'field, + Self: RowIndex, + { + let idx = self.idx(idx)?; + Some(OwnedSqliteField { + row: self, + col_idx: i32::try_from(idx).ok()?, + }) + } + + fn partial_row(&self, range: std::ops::Range) -> PartialRow<'_, Self::InnerPartialRow> { + PartialRow::new(self, range) + } +} + +impl RowIndex for OwnedSqliteRow { + fn idx(&self, idx: usize) -> Option { + if idx < self.field_count() { + Some(idx) + } else { + None + } + } +} + +impl<'idx> RowIndex<&'idx str> for OwnedSqliteRow { + fn idx(&self, field_name: &'idx str) -> Option { + self.column_names + .iter() + .position(|n| n.as_ref().map(|s| s as &str) == Some(field_name)) + } +} + +#[allow(missing_debug_implementations)] +pub struct OwnedSqliteField<'row> { + pub(super) row: &'row OwnedSqliteRow, + pub(super) col_idx: i32, +} + +impl<'row> Field<'row, Sqlite> for OwnedSqliteField<'row> { + fn field_name(&self) -> Option<&str> { + self.row + .column_names + .get(self.col_idx as usize) + .and_then(|o| o.as_ref().map(|s| s.as_ref())) + } + + fn is_null(&self) -> bool { + self.value().is_none() + } + + fn value(&self) -> Option<::RawValue<'row>> { + SqliteValue::from_owned_row(self.row, self.col_idx) + } +} diff --git a/diesel/src/sqlite/connection/row.rs b/diesel/src/sqlite/connection/row.rs index 585e0d69096c..6fe8262b431d 100644 --- a/diesel/src/sqlite/connection/row.rs +++ b/diesel/src/sqlite/connection/row.rs @@ -1,10 +1,11 @@ use std::cell::{Ref, RefCell}; use std::rc::Rc; +use super::owned_row::OwnedSqliteRow; use super::sqlite_value::{OwnedSqliteValue, SqliteValue}; use super::stmt::StatementUse; use crate::backend::Backend; -use crate::row::{Field, PartialRow, Row, RowIndex, RowSealed}; +use crate::row::{Field, IntoOwnedRow, PartialRow, Row, RowIndex, RowSealed}; use crate::sqlite::Sqlite; #[allow(missing_debug_implementations)] @@ -21,6 +22,14 @@ pub(super) enum PrivateSqliteRow<'stmt, 'query> { }, } +impl<'stmt> IntoOwnedRow<'stmt, Sqlite> for SqliteRow<'stmt, '_> { + type OwnedRow = OwnedSqliteRow; + + fn into_owned(self) -> Self::OwnedRow { + self.inner.borrow().moveable() + } +} + impl<'stmt, 'query> PrivateSqliteRow<'stmt, 'query> { pub(super) fn duplicate( &mut self, @@ -58,6 +67,37 @@ impl<'stmt, 'query> PrivateSqliteRow<'stmt, 'query> { }, } } + + pub(super) fn moveable(&self) -> OwnedSqliteRow { + match self { + PrivateSqliteRow::Direct(stmt) => { + let column_names: Box<[Option]> = (0..stmt.column_count()) + .map(|idx| stmt.field_name(idx).map(|s| s.to_owned())) + .collect::>() + .into_boxed_slice(); + OwnedSqliteRow::new( + (0..stmt.column_count()) + .map(|idx| stmt.copy_value(idx)) + .collect(), + column_names, + ) + } + PrivateSqliteRow::Duplicated { + values, + column_names, + } => OwnedSqliteRow::new( + values + .iter() + .map(|v| v.as_ref().map(|v| v.duplicate())) + .collect(), + (*column_names) + .iter() + .map(|s| s.to_owned()) + .collect::>() + .into_boxed_slice(), + ), + } + } } impl<'stmt, 'query> RowSealed for SqliteRow<'stmt, 'query> {} diff --git a/diesel/src/sqlite/connection/sqlite_value.rs b/diesel/src/sqlite/connection/sqlite_value.rs index d8e4d5bdfe77..7210d104bb4c 100644 --- a/diesel/src/sqlite/connection/sqlite_value.rs +++ b/diesel/src/sqlite/connection/sqlite_value.rs @@ -7,6 +7,7 @@ use std::{slice, str}; use crate::sqlite::SqliteType; +use super::owned_row::OwnedSqliteRow; use super::row::PrivateSqliteRow; /// Raw sqlite value as received from the database @@ -18,7 +19,7 @@ pub struct SqliteValue<'row, 'stmt, 'query> { // This field exists to ensure that nobody // can modify the underlying row while we are // holding a reference to some row value here - _row: Ref<'row, PrivateSqliteRow<'stmt, 'query>>, + _row: Option>>, // we extract the raw value pointer as part of the constructor // to safe the match statements for each method // According to benchmarks this leads to a ~20-30% speedup @@ -29,6 +30,7 @@ pub struct SqliteValue<'row, 'stmt, 'query> { value: NonNull, } +#[derive(Debug)] #[repr(transparent)] pub(super) struct OwnedSqliteValue { pub(super) value: NonNull, @@ -40,6 +42,10 @@ impl Drop for OwnedSqliteValue { } } +// Unsafe Send impl safe since sqlite3_value is built with sqlite3_value_dup +// see https://www.sqlite.org/c3ref/value.html +unsafe impl Send for OwnedSqliteValue {} + impl<'row, 'stmt, 'query> SqliteValue<'row, 'stmt, 'query> { pub(super) fn new( row: Ref<'row, PrivateSqliteRow<'stmt, 'query>>, @@ -52,7 +58,27 @@ impl<'row, 'stmt, 'query> SqliteValue<'row, 'stmt, 'query> { } }; - let ret = Self { _row: row, value }; + let ret = Self { + _row: Some(row), + value, + }; + if ret.value_type().is_none() { + None + } else { + Some(ret) + } + } + + pub(super) fn from_owned_row( + row: &'row OwnedSqliteRow, + col_idx: i32, + ) -> Option> { + let value = row + .values + .get(col_idx as usize) + .and_then(|v| v.as_ref())? + .value; + let ret = Self { _row: None, value }; if ret.value_type().is_none() { None } else { From e2c2c115874790d43baaa702c3132d20f2db2910 Mon Sep 17 00:00:00 2001 From: Mohamed Belaouad Date: Tue, 9 Apr 2024 09:31:50 +0200 Subject: [PATCH 3/3] Avoid clippy warning on reference to static mut It will be a hard error in 2024 edition so implement it "properly" warning: creating a mutable reference to mutable static is discouraged --> diesel/src/sqlite/connection/mod.rs:229:18 | 229 | unsafe { &mut SQLITE_METADATA_LOOKUP } | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ mutable reference to mutable static | = note: for more information, see issue #114447 = note: this will be a hard error in the 2024 edition = note: this mutable reference has lifetime `'static`, but if the static gets accessed (read or written) by any other means, or any other reference is created, then any further use of this mutable reference is Undefined Behavior = note: `#[warn(static_mut_refs)]` on by default --- diesel/src/sqlite/connection/mod.rs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/diesel/src/sqlite/connection/mod.rs b/diesel/src/sqlite/connection/mod.rs index 38fd7e6ff6a1..fa77c0571a81 100644 --- a/diesel/src/sqlite/connection/mod.rs +++ b/diesel/src/sqlite/connection/mod.rs @@ -124,6 +124,9 @@ pub struct SqliteConnection { statement_cache: StatementCache, raw_connection: RawConnection, transaction_state: AnsiTransactionManager, + // this exists for the sole purpose of implementing `WithMetadataLookup` trait + // and avoiding static mut which will be deprecated in 2024 edition + metadata_lookup: (), instrumentation: Option>, } @@ -221,12 +224,9 @@ impl LoadConnection for SqliteConnection { } } -static mut SQLITE_METADATA_LOOKUP: () = (); - impl WithMetadataLookup for SqliteConnection { fn metadata_lookup(&mut self) -> &mut ::MetadataLookup { - // safe since it's a unit type - unsafe { &mut SQLITE_METADATA_LOOKUP } + &mut self.metadata_lookup } } @@ -539,6 +539,7 @@ impl SqliteConnection { statement_cache: StatementCache::new(), raw_connection, transaction_state: AnsiTransactionManager::default(), + metadata_lookup: (), instrumentation: None, }; conn.register_diesel_sql_functions()