diff --git a/rust/src/storage/db/mod.rs b/rust/src/storage/db/mod.rs index 78fd0b4efb..ce159a6429 100644 --- a/rust/src/storage/db/mod.rs +++ b/rust/src/storage/db/mod.rs @@ -142,6 +142,11 @@ impl DatabasePoolExt for T {} /// Based on the ergonomics of [`tokio_postgres::Transaction`] #[async_trait::async_trait] pub trait Transaction: Send { + /// Run a database query, returning a list of resulting rows. + /// + /// We expect the `sql` query should use `?` placeholders for the `args`. Downstream + /// implementations should string-replace `?` as necessary. + // // `async` as this is representing a round-trip between the app and database async fn query(&mut self, sql: &str, args: &[&str]) -> Result, anyhow::Error>; } diff --git a/rust/src/storage/db/python_db_pool.rs b/rust/src/storage/db/python_db_pool.rs index 6d164ae779..12a879a007 100644 --- a/rust/src/storage/db/python_db_pool.rs +++ b/rust/src/storage/db/python_db_pool.rs @@ -116,11 +116,12 @@ impl DatabasePool for PythonDatabasePoolWrapper { let txn_py = args.get_item(0)?; let mut txn = txn_py.extract::()?; - // Since we expect people to only call `.await` on [`Transaction`] - // related methods (mentioned in the [`Transaction`] docstring) AND - // because there is no async work to suspend on in the Python - // [`Transaction`] synchronously, we can get away with polling once - // as it should immediately resolve to [`Poll::Ready`]. Getting + // Since we expect people to only call `.await` on + // [`Transaction`] related methods (mentioned in the + // [`Transaction`] docstring) AND because there is no actual + // async work to suspend on in the Python [`Transaction`] + // (resolves synchonously), we can get away with polling once as + // it should immediately resolve to [`Poll::Ready`]. Getting // [`Poll::Pending`] would be considered a programming error. // // Alternatively, we could just use `futures::executor::block_on` @@ -183,10 +184,9 @@ impl DatabasePool for PythonDatabasePoolWrapper { /// Poll a future exactly once. /// -/// Returns [`Poll::Ready`] if the future resolves immediately, or -/// [`Poll::Pending`] if it would need to suspend. We use this where a future is -/// expected to never genuinely suspend (e.g. Synapse's synchronous DB query -/// path) and want to enforce that, rather than driving it to completion with a +/// Returns [`Poll::Ready`] if the future resolves immediately, or [`Poll::Pending`] if +/// it would need to suspend. We use this where a future is expected to never genuinely +/// suspend and want to enforce/check that, rather than driving it to completion with a /// blocking executor like `futures::executor::block_on`. fn poll_once(future: F) -> Poll { let mut future = pin!(future); @@ -208,6 +208,7 @@ fn anyhow_to_pyerr(err: &anyhow::Error) -> PyErr { PyRuntimeError::new_err(format!("{err:#}")) } +/// Given a Python `LoggingTransaction`, figures out the database engine that backs it fn detect_engine(txn_py: &Bound<'_, PyAny>) -> PyResult { let name = txn_py .getattr("database_engine") @@ -279,10 +280,10 @@ impl Transaction for LoggingTransactionWrapper { async fn query(&mut self, sql: &str, args: &[&str]) -> Result, anyhow::Error> { Python::attach(|py| -> PyResult> { // Convert the Rust `&[&str]` of SQL parameters into a Python sequence - // so it can be passed through to the Python-side `execute`. Note that - // `LoggingTransaction.execute` converts `?` placeholders into the - // appropriate param style for the underlying engine, so we pass - // `?`-style SQL. + // so it can be passed through to the Python-side `execute`. + // + // We don't need to do anything to the SQL as the `?`-style arg placeholders + // already align with what `LoggingTransaction` expects. let args = PyList::new(py, args)?; self.execute(py, sql, args.as_any())?;