Fix ODBC prepare metadata caching

Author: lovasoa

sqlx-core/src/odbc/connection/mod.rs

@@ -13,7 +13,11 @@ use crate::odbc::{OdbcStatement, OdbcStatementMetadata};
 use futures_core::future::BoxFuture;
 use futures_util::future;
 use odbc_api::ConnectionTransitions;
-use odbc_api::{handles::StatementConnection, Prepared, ResultSetMetadata, SharedConnection};
+use odbc_api::Error as OdbcApiError;
+use odbc_api::{
+    handles::{slice_to_cow_utf8, StatementConnection},
+    Prepared, ResultSetMetadata, SharedConnection,
+};
 use odbc_bridge::{establish_connection, execute_sql};
 use std::borrow::Cow;
 use std::sync::{Arc, Mutex};
@@ -23,20 +27,28 @@ mod executor;
 type PreparedStatement = Prepared<StatementConnection<SharedConnection<'static>>>;
 type SharedPreparedStatement = Arc<Mutex<PreparedStatement>>;
 
+struct CollectedColumns {
+    columns: Vec<OdbcColumn>,
+    deferred: bool,
+}
+
 fn collect_columns(
     prepared: &mut PreparedStatement,
     parameter_count: usize,
     allow_deferred_result_columns: bool,
-) -> Result<Vec<OdbcColumn>, Error> {
+) -> Result<CollectedColumns, Error> {
     let count = match prepared.num_result_cols() {
         Ok(count) => count,
-        Err(error) if allow_deferred_result_columns && parameter_count > 0 => {
-            // Some ODBC drivers only expose result columns for parameterized
-            // statements after values are bound and the statement is executed.
-            // Row execution still gets authoritative metadata from the cursor;
-            // describe() uses the strict path and will surface this error.
-            log::debug!("ODBC prepare did not expose result columns: {error}");
-            return Ok(Vec::new());
+        Err(error)
+            if allow_deferred_result_columns
+                && parameter_count > 0
+                && is_unbound_parameter_metadata_error(&error) =>
+        {
+            log::debug!("ODBC prepare deferred result columns until execution: {error}");
+            return Ok(CollectedColumns {
+                columns: Vec::new(),
+                deferred: true,
+            });
         }
         Err(error) => return Err(error.into()),
     };
@@ -45,19 +57,38 @@ fn collect_columns(
     for i in 1..=count {
         columns.push(describe_column(prepared, i as u16)?);
     }
-    Ok(columns)
+    Ok(CollectedColumns {
+        columns,
+        deferred: false,
+    })
 }
 
 fn collect_statement_metadata(
     prepared: &mut PreparedStatement,
     allow_deferred_result_columns: bool,
-) -> Result<OdbcStatementMetadata, Error> {
+) -> Result<(OdbcStatementMetadata, bool), Error> {
     let parameters = usize::from(prepared.num_params()?);
+    let collected = collect_columns(prepared, parameters, allow_deferred_result_columns)?;
+    let metadata_complete =
+        !collected.deferred && !(parameters > 0 && collected.columns.is_empty());
+
+    Ok((
+        OdbcStatementMetadata {
+            columns: collected.columns,
+            parameters,
+        },
+        metadata_complete,
+    ))
+}
 
-    Ok(OdbcStatementMetadata {
-        columns: collect_columns(prepared, parameters, allow_deferred_result_columns)?,
-        parameters,
-    })
+fn is_unbound_parameter_metadata_error(error: &OdbcApiError) -> bool {
+    match error {
+        OdbcApiError::Diagnostics { record, .. } if record.state.as_str() == "01000" => {
+            let message = slice_to_cow_utf8(&record.message).to_ascii_lowercase();
+            message.contains("parameter") && message.contains("bound")
+        }
+        _ => false,
+    }
 }
 
 pub(super) fn describe_column<S>(stmt: &mut S, index: u16) -> Result<OdbcColumn, Error>
@@ -241,6 +272,7 @@ impl OdbcConnection {
         store_to_cache: bool,
         allow_deferred_result_columns: bool,
     ) -> Result<OdbcStatement<'a>, Error> {
+        let sql_owned = sql.to_string();
         let cached = self
             .stmt_cache
             .get_mut(sql)
@@ -252,6 +284,7 @@ impl OdbcConnection {
                     Error::Protocol("ODBC prepare: failed to lock prepared statement".into())
                 })?;
                 collect_statement_metadata(&mut prepared, allow_deferred_result_columns)
+                    .map(|(metadata, _)| metadata)
             })
             .await?;
 
@@ -262,17 +295,16 @@ impl OdbcConnection {
         }
 
         let conn = Arc::clone(&self.conn);
-        let sql_owned = sql.to_string();
         let sql_clone = sql_owned.clone();
-        let (prepared, metadata) = spawn_blocking(move || {
+        let (prepared, metadata, metadata_complete) = spawn_blocking(move || {
             let mut prepared = conn.into_prepared(&sql_clone)?;
             let metadata =
                 collect_statement_metadata(&mut prepared, allow_deferred_result_columns)?;
-            Ok::<_, Error>((prepared, metadata))
+            Ok::<_, Error>((prepared, metadata.0, metadata.1))
         })
         .await?;
 
-        if store_to_cache && self.stmt_cache.is_enabled() {
+        if store_to_cache && metadata_complete && self.stmt_cache.is_enabled() {
             self.stmt_cache
                 .insert(&sql_owned, Arc::new(Mutex::new(prepared)));
         }

tests/odbc/odbc.rs

@@ -333,6 +333,77 @@ async fn describe_does_not_succeed_with_missing_columns() -> anyhow::Result<()>
     Ok(())
 }
 
+const PARAMETERIZED_SELECT_WITH_COLUMN: &str = "SELECT ? AS value";
+const MISSING_TABLE_READ: &str = "SELECT contents from sqlx_missing_fs WHERE path = ?";
+const MISSING_TABLE_EXISTS: &str = "SELECT 1 from sqlx_missing_fs WHERE path = ?";
+const MISSING_TABLE_MODIFIED: &str =
+    "SELECT 1 from sqlx_missing_fs WHERE last_modified >= ? AND path = ?";
+
+#[tokio::test]
+async fn prepare_missing_table_does_not_return_empty_metadata() -> anyhow::Result<()> {
+    let mut conn = new::<Odbc>().await?;
+
+    for sql in [
+        MISSING_TABLE_READ,
+        MISSING_TABLE_EXISTS,
+        MISSING_TABLE_MODIFIED,
+    ] {
+        if let Ok(statement) = conn.prepare(sql).await {
+            assert!(
+                !statement.columns().is_empty(),
+                "ODBC prepare must not turn a metadata error into zero columns for {sql}"
+            );
+        }
+    }
+
+    Ok(())
+}
+
+#[tokio::test]
+async fn empty_metadata_prepare_is_not_cached() -> anyhow::Result<()> {
+    let mut conn = new::<Odbc>().await?;
+
+    conn.clear_cached_statements().await?;
+    assert_eq!(conn.cached_statements_size(), 0);
+
+    match conn.prepare(PARAMETERIZED_SELECT_WITH_COLUMN).await {
+        Ok(statement) if statement.columns().is_empty() => assert_eq!(
+            conn.cached_statements_size(),
+            0,
+            "ODBC prepare must not cache statements whose result metadata is deferred"
+        ),
+        Ok(_) => {}
+        Err(_) => assert_eq!(
+            conn.cached_statements_size(),
+            0,
+            "ODBC prepare must not cache a statement after result-column metadata failed"
+        ),
+    }
+
+    Ok(())
+}
+
+#[tokio::test]
+async fn failed_metadata_prepare_does_not_poison_execute() -> anyhow::Result<()> {
+    let mut conn = new::<Odbc>().await?;
+
+    let _ = conn.prepare(MISSING_TABLE_READ).await;
+
+    let error = sqlx_oldapi::query::<Odbc>(MISSING_TABLE_READ)
+        .bind("index.sql")
+        .fetch_optional(&mut conn)
+        .await
+        .expect_err("querying a missing table should fail");
+    let message = format!("{error:#}");
+
+    assert!(
+        message.contains("sqlx_missing_fs"),
+        "failed ODBC prepare metadata poisoned a later execute instead of returning a normal missing-table error: {message}"
+    );
+
+    Ok(())
+}
+
 #[tokio::test]
 async fn it_can_bind_many_params_dynamically() -> anyhow::Result<()> {
     let mut conn = new::<Odbc>().await?;