From 55000295f07e8ff67c28e162de583a9220394985 Mon Sep 17 00:00:00 2001 From: Thomas BESSOU Date: Thu, 25 Jul 2024 04:23:20 +0200 Subject: [PATCH] Make Instrumentation downcasting work correctly by avoiding additional dyn dispatch level we actually returned &mut Option> as &mut dyn Instrumentation, so downcasting would have to be done in these two steps, which is counter-intuitive and doesn't seem ideal inside diesel itself either. --- diesel/src/connection/instrumentation.rs | 85 +++++++++++++++++++++++- diesel/src/mysql/connection/mod.rs | 19 +++--- diesel/src/pg/connection/cursor.rs | 1 - diesel/src/pg/connection/mod.rs | 49 ++++++-------- diesel/src/sqlite/connection/mod.rs | 16 ++--- 5 files changed, 120 insertions(+), 50 deletions(-) diff --git a/diesel/src/connection/instrumentation.rs b/diesel/src/connection/instrumentation.rs index baca07dec282..dc15dfe04755 100644 --- a/diesel/src/connection/instrumentation.rs +++ b/diesel/src/connection/instrumentation.rs @@ -1,7 +1,7 @@ use downcast_rs::Downcast; use std::fmt::{Debug, Display}; use std::num::NonZeroU32; -use std::ops::DerefMut; +use std::ops::{Deref, DerefMut}; static GLOBAL_INSTRUMENTATION: std::sync::RwLock Option>> = std::sync::RwLock::new(|| None); @@ -316,3 +316,86 @@ where } } } + +#[diesel_derives::__diesel_public_if( + feature = "i-implement-a-third-party-backend-and-opt-into-breaking-changes" +)] +/// An optional dyn instrumentation. +/// For ease of use, this type implements [`Deref`] and [`DerefMut`] to `&dyn Instrumentation`, +/// falling back to a no-op implementation if no instrumentation is set. +pub(crate) struct DynInstrumentation { + /// zst + no_instrumentation: NoInstrumentation, + inner: Option>, +} +impl Deref for DynInstrumentation { + type Target = dyn Instrumentation; + + fn deref(&self) -> &Self::Target { + self.inner.as_deref().unwrap_or(&self.no_instrumentation) + } +} +impl DerefMut for DynInstrumentation { + fn deref_mut(&mut self) -> &mut Self::Target { + self.inner + .as_deref_mut() + .unwrap_or(&mut self.no_instrumentation) + } +} +impl DynInstrumentation { + #[diesel_derives::__diesel_public_if( + feature = "i-implement-a-third-party-backend-and-opt-into-breaking-changes" + )] + pub(crate) fn default_instrumentation() -> Self { + Self { + inner: get_default_instrumentation(), + no_instrumentation: NoInstrumentation, + } + } + + #[diesel_derives::__diesel_public_if( + feature = "i-implement-a-third-party-backend-and-opt-into-breaking-changes" + )] + pub(crate) fn none() -> Self { + Self { + inner: None, + no_instrumentation: NoInstrumentation, + } + } + + #[diesel_derives::__diesel_public_if( + feature = "i-implement-a-third-party-backend-and-opt-into-breaking-changes" + )] + pub(crate) fn on_connection_event(&mut self, event: InstrumentationEvent<'_>) { + if let Some(inner) = self.inner.as_deref_mut() { + inner.on_connection_event(event) + } + } +} +impl From for DynInstrumentation { + fn from(instrumentation: I) -> Self { + Self { + inner: Some(unpack_instrumentation(Box::new(instrumentation))), + no_instrumentation: NoInstrumentation, + } + } +} + +struct NoInstrumentation; +impl Instrumentation for NoInstrumentation { + fn on_connection_event(&mut self, _: InstrumentationEvent<'_>) {} +} + +/// Unwrap unnecessary boxing levels +fn unpack_instrumentation( + mut instrumentation: Box, +) -> Box { + loop { + match instrumentation.downcast::>() { + Ok(extra_boxed_instrumentation) => instrumentation = *extra_boxed_instrumentation, + Err(not_extra_boxed_instrumentation) => { + break not_extra_boxed_instrumentation; + } + } + } +} diff --git a/diesel/src/mysql/connection/mod.rs b/diesel/src/mysql/connection/mod.rs index 992d92c4f915..76f3773dd25a 100644 --- a/diesel/src/mysql/connection/mod.rs +++ b/diesel/src/mysql/connection/mod.rs @@ -8,8 +8,7 @@ use self::stmt::iterator::StatementIterator; use self::stmt::Statement; use self::url::ConnectionOptions; use super::backend::Mysql; -use crate::connection::instrumentation::DebugQuery; -use crate::connection::instrumentation::StrQueryHelper; +use crate::connection::instrumentation::{DebugQuery, DynInstrumentation, StrQueryHelper}; use crate::connection::statement_cache::{MaybeCached, StatementCache}; use crate::connection::*; use crate::expression::QueryMetadata; @@ -111,7 +110,7 @@ pub struct MysqlConnection { raw_connection: RawConnection, transaction_state: AnsiTransactionManager, statement_cache: StatementCache, - instrumentation: Option>, + instrumentation: DynInstrumentation, } // mysql connection can be shared between threads according to libmysqlclients documentation @@ -156,7 +155,7 @@ impl Connection for MysqlConnection { /// * `ssl_mode` expects a value defined for MySQL client command option `--ssl-mode` /// See fn establish(database_url: &str) -> ConnectionResult { - let mut instrumentation = crate::connection::instrumentation::get_default_instrumentation(); + let mut instrumentation = DynInstrumentation::default_instrumentation(); instrumentation.on_connection_event(InstrumentationEvent::StartEstablishConnection { url: database_url, }); @@ -181,7 +180,7 @@ impl Connection for MysqlConnection { &source, &mut self.statement_cache, &mut self.raw_connection, - &mut self.instrumentation, + &mut *self.instrumentation, ) .and_then(|stmt| { // we have not called result yet, so calling `execute` is @@ -200,11 +199,11 @@ impl Connection for MysqlConnection { } fn instrumentation(&mut self) -> &mut dyn Instrumentation { - &mut self.instrumentation + &mut *self.instrumentation } fn set_instrumentation(&mut self, instrumentation: impl Instrumentation) { - self.instrumentation = Some(Box::new(instrumentation)); + self.instrumentation = instrumentation.into(); } } @@ -212,7 +211,7 @@ impl Connection for MysqlConnection { fn update_transaction_manager_status( query_result: QueryResult, transaction_manager: &mut AnsiTransactionManager, - instrumentation: &mut Option>, + instrumentation: &mut DynInstrumentation, query: &dyn DebugQuery, ) -> QueryResult { if let Err(Error::DatabaseError(DatabaseErrorKind::SerializationFailure, _)) = query_result { @@ -244,7 +243,7 @@ impl LoadConnection for MysqlConnection { &source, &mut self.statement_cache, &mut self.raw_connection, - &mut self.instrumentation, + &mut *self.instrumentation, ) .and_then(|stmt| { let mut metadata = Vec::new(); @@ -329,7 +328,7 @@ impl MysqlConnection { raw_connection, transaction_state: AnsiTransactionManager::default(), statement_cache: StatementCache::new(), - instrumentation: None, + instrumentation: DynInstrumentation::none(), }; conn.set_config_options() .map_err(CouldntSetupConfiguration)?; diff --git a/diesel/src/pg/connection/cursor.rs b/diesel/src/pg/connection/cursor.rs index 2a72138fd294..f763a3c5c8ae 100644 --- a/diesel/src/pg/connection/cursor.rs +++ b/diesel/src/pg/connection/cursor.rs @@ -1,7 +1,6 @@ use super::raw::RawConnection; use super::result::PgResult; use super::row::PgRow; -use crate::connection::Instrumentation; use crate::pg::Pg; use crate::query_builder::QueryFragment; use std::rc::Rc; diff --git a/diesel/src/pg/connection/mod.rs b/diesel/src/pg/connection/mod.rs index d1dc4eb17609..8b0e1925c071 100644 --- a/diesel/src/pg/connection/mod.rs +++ b/diesel/src/pg/connection/mod.rs @@ -5,15 +5,14 @@ mod result; mod row; mod stmt; -use self::copy::CopyFromSink; -use self::copy::CopyToBuffer; +use self::copy::{CopyFromSink, CopyToBuffer}; use self::cursor::*; use self::private::ConnectionAndTransactionManager; use self::raw::{PgTransactionStatus, RawConnection}; use self::stmt::Statement; -use crate::connection::instrumentation::DebugQuery; -use crate::connection::instrumentation::Instrumentation; -use crate::connection::instrumentation::StrQueryHelper; +use crate::connection::instrumentation::{ + DebugQuery, DynInstrumentation, Instrumentation, StrQueryHelper, +}; use crate::connection::statement_cache::{MaybeCached, StatementCache}; use crate::connection::*; use crate::expression::QueryMetadata; @@ -29,9 +28,7 @@ use std::ffi::CString; use std::fmt::Debug; use std::os::raw as libc; -use super::query_builder::copy::CopyFromExpression; -use super::query_builder::copy::CopyTarget; -use super::query_builder::copy::CopyToCommand; +use super::query_builder::copy::{CopyFromExpression, CopyTarget, CopyToCommand}; pub(super) use self::result::PgResult; @@ -178,7 +175,7 @@ impl Connection for PgConnection { type TransactionManager = AnsiTransactionManager; fn establish(database_url: &str) -> ConnectionResult { - let mut instrumentation = crate::connection::instrumentation::get_default_instrumentation(); + let mut instrumentation = DynInstrumentation::default_instrumentation(); instrumentation.on_connection_event(InstrumentationEvent::StartEstablishConnection { url: database_url, }); @@ -187,7 +184,7 @@ impl Connection for PgConnection { connection_and_transaction_manager: ConnectionAndTransactionManager { raw_connection: raw_conn, transaction_state: AnsiTransactionManager::default(), - instrumentation: None, + instrumentation: DynInstrumentation::none(), }, statement_cache: StatementCache::new(), metadata_cache: PgMetadataCache::new(), @@ -233,11 +230,11 @@ impl Connection for PgConnection { } fn instrumentation(&mut self) -> &mut dyn Instrumentation { - &mut self.connection_and_transaction_manager.instrumentation + &mut *self.connection_and_transaction_manager.instrumentation } fn set_instrumentation(&mut self, instrumentation: impl Instrumentation) { - self.connection_and_transaction_manager.instrumentation = Some(Box::new(instrumentation)); + self.connection_and_transaction_manager.instrumentation = instrumentation.into(); } } @@ -517,7 +514,7 @@ impl PgConnection { }; Statement::prepare(conn, sql, query_name.as_deref(), &metadata) }, - &mut self.connection_and_transaction_manager.instrumentation, + &mut *self.connection_and_transaction_manager.instrumentation, ); if !execute_returning_count { if let Err(ref e) = query { @@ -557,7 +554,7 @@ mod private { pub struct ConnectionAndTransactionManager { pub(super) raw_connection: RawConnection, pub(super) transaction_state: AnsiTransactionManager, - pub(super) instrumentation: Option>, + pub(super) instrumentation: DynInstrumentation, } pub trait PgLoadingMode { @@ -800,8 +797,7 @@ mod tests { #[test] fn transaction_manager_returns_an_error_when_attempting_to_commit_outside_of_a_transaction() { - use crate::connection::AnsiTransactionManager; - use crate::connection::TransactionManager; + use crate::connection::{AnsiTransactionManager, TransactionManager}; use crate::result::Error; use crate::PgConnection; @@ -818,8 +814,7 @@ mod tests { #[test] fn transaction_manager_returns_an_error_when_attempting_to_rollback_outside_of_a_transaction() { - use crate::connection::AnsiTransactionManager; - use crate::connection::TransactionManager; + use crate::connection::{AnsiTransactionManager, TransactionManager}; use crate::result::Error; use crate::PgConnection; @@ -838,8 +833,7 @@ mod tests { fn postgres_transaction_is_rolled_back_upon_syntax_error() { use std::num::NonZeroU32; - use crate::connection::AnsiTransactionManager; - use crate::connection::TransactionManager; + use crate::connection::{AnsiTransactionManager, TransactionManager}; use crate::pg::connection::raw::PgTransactionStatus; use crate::*; let conn = &mut crate::test_helpers::pg_connection_no_transaction(); @@ -883,8 +877,7 @@ mod tests { fn nested_postgres_transaction_is_rolled_back_upon_syntax_error() { use std::num::NonZeroU32; - use crate::connection::AnsiTransactionManager; - use crate::connection::TransactionManager; + use crate::connection::{AnsiTransactionManager, TransactionManager}; use crate::pg::connection::raw::PgTransactionStatus; use crate::*; let conn = &mut crate::test_helpers::pg_connection_no_transaction(); @@ -1165,8 +1158,7 @@ mod tests { #[test] fn postgres_transaction_is_rolled_back_upon_deferred_constraint_failure() { - use crate::connection::AnsiTransactionManager; - use crate::connection::TransactionManager; + use crate::connection::{AnsiTransactionManager, TransactionManager}; use crate::pg::connection::raw::PgTransactionStatus; use crate::result::Error; use crate::*; @@ -1215,8 +1207,7 @@ mod tests { #[test] fn postgres_transaction_is_rolled_back_upon_deferred_trigger_failure() { - use crate::connection::AnsiTransactionManager; - use crate::connection::TransactionManager; + use crate::connection::{AnsiTransactionManager, TransactionManager}; use crate::pg::connection::raw::PgTransactionStatus; use crate::result::Error; use crate::*; @@ -1291,8 +1282,7 @@ mod tests { #[test] fn nested_postgres_transaction_is_rolled_back_upon_deferred_trigger_failure() { - use crate::connection::AnsiTransactionManager; - use crate::connection::TransactionManager; + use crate::connection::{AnsiTransactionManager, TransactionManager}; use crate::pg::connection::raw::PgTransactionStatus; use crate::result::Error; use crate::*; @@ -1374,8 +1364,7 @@ mod tests { #[test] fn nested_postgres_transaction_is_rolled_back_upon_deferred_constraint_failure() { - use crate::connection::AnsiTransactionManager; - use crate::connection::TransactionManager; + use crate::connection::{AnsiTransactionManager, TransactionManager}; use crate::pg::connection::raw::PgTransactionStatus; use crate::result::Error; use crate::*; diff --git a/diesel/src/sqlite/connection/mod.rs b/diesel/src/sqlite/connection/mod.rs index fa77c0571a81..deee02775795 100644 --- a/diesel/src/sqlite/connection/mod.rs +++ b/diesel/src/sqlite/connection/mod.rs @@ -21,7 +21,7 @@ use self::raw::RawConnection; use self::statement_iterator::*; use self::stmt::{Statement, StatementUse}; use super::SqliteAggregateFunction; -use crate::connection::instrumentation::StrQueryHelper; +use crate::connection::instrumentation::{DynInstrumentation, StrQueryHelper}; use crate::connection::statement_cache::StatementCache; use crate::connection::*; use crate::deserialize::{FromSqlRow, StaticallySizedRow}; @@ -127,7 +127,7 @@ pub struct SqliteConnection { // 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>, + instrumentation: DynInstrumentation, } // This relies on the invariant that RawConnection or Statement are never @@ -165,7 +165,7 @@ impl Connection for SqliteConnection { /// If the database does not exist, this method will try to /// create a new database and then establish a connection to it. fn establish(database_url: &str) -> ConnectionResult { - let mut instrumentation = crate::connection::instrumentation::get_default_instrumentation(); + let mut instrumentation = DynInstrumentation::default_instrumentation(); instrumentation.on_connection_event(InstrumentationEvent::StartEstablishConnection { url: database_url, }); @@ -198,11 +198,11 @@ impl Connection for SqliteConnection { } fn instrumentation(&mut self) -> &mut dyn Instrumentation { - &mut self.instrumentation + &mut *self.instrumentation } fn set_instrumentation(&mut self, instrumentation: impl Instrumentation) { - self.instrumentation = Some(Box::new(instrumentation)); + self.instrumentation = instrumentation.into(); } } @@ -352,7 +352,7 @@ impl SqliteConnection { &Sqlite, &[], |sql, is_cached| Statement::prepare(raw_connection, sql, is_cached), - &mut self.instrumentation, + &mut *self.instrumentation, ) { Ok(statement) => statement, Err(e) => { @@ -366,7 +366,7 @@ impl SqliteConnection { } }; - StatementUse::bind(statement, source, &mut self.instrumentation) + StatementUse::bind(statement, source, &mut *self.instrumentation) } #[doc(hidden)] @@ -540,7 +540,7 @@ impl SqliteConnection { raw_connection, transaction_state: AnsiTransactionManager::default(), metadata_lookup: (), - instrumentation: None, + instrumentation: DynInstrumentation::none(), }; conn.register_diesel_sql_functions() .map_err(CouldntSetupConfiguration)?;