From 35c6b57476165f16ff39a28c615f8e549aaf5ff8 Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Fri, 13 Feb 2026 13:38:58 +0100 Subject: [PATCH 1/5] wip --- src/lib.rs | 4 ---- src/odbc/api/sqlconnect.rs | 2 +- src/odbc/api/sqldriverconnect.rs | 8 +++++--- src/odbc/api/sqlgetfunctions.rs | 1 + src/odbc/api/sqlgetinfo.rs | 2 +- src/odbc/def/c_data_type.rs | 5 +++-- src/odbc/implementation/alloc_handles.rs | 5 ++++- tests/basic_odbc_connection_test.rs | 4 +++- 8 files changed, 18 insertions(+), 13 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 918953d..d6971c6 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -10,25 +10,21 @@ mod odbc; // Windows: Use the built-in Windows ODBC library #[cfg_attr(windows, link(name = "odbc32"))] - // Unix + Dynamic + unixODBC (default case for Linux) #[cfg_attr( all(not(windows), not(feature = "static"), not(feature = "iodbc")), link(name = "odbcinst") )] - // Unix + Static + unixODBC (for self-contained binaries) #[cfg_attr( all(not(windows), feature = "static", not(feature = "iodbc")), link(name = "odbcinst", kind = "static") )] - // Unix + Dynamic + iODBC (common on macOS) #[cfg_attr( all(not(windows), not(feature = "static"), feature = "iodbc"), link(name = "iodbcinst") )] - // Unix + Static + iODBC (self-contained binaries with iODBC) #[cfg_attr( all(not(windows), feature = "static", feature = "iodbc"), diff --git a/src/odbc/api/sqlconnect.rs b/src/odbc/api/sqlconnect.rs index e6f6108..a0ccea9 100644 --- a/src/odbc/api/sqlconnect.rs +++ b/src/odbc/api/sqlconnect.rs @@ -1,5 +1,5 @@ //! -//! https://learn.microsoft.com/en-us/sql/odbc/reference/syntax/sqlconnect-function?view=sql-server-ver16 +//! //! //! ```c //! SQLRETURN SQLConnect( diff --git a/src/odbc/api/sqldriverconnect.rs b/src/odbc/api/sqldriverconnect.rs index 90591bd..252f448 100644 --- a/src/odbc/api/sqldriverconnect.rs +++ b/src/odbc/api/sqldriverconnect.rs @@ -1,5 +1,5 @@ //! -//! https://learn.microsoft.com/en-us/sql/odbc/reference/syntax/sqldriverconnect-function +//! //! //! ```c //! SQLRETURN SQLDriverConnect( @@ -18,7 +18,7 @@ use crate::odbc::implementation::connect::impl_connect; use crate::odbc::utils::{get_from_wrapper, maybe_utf16_to_string}; use odbc_sys::{HandleType, SmallInt, SqlReturn, USmallInt, WChar}; use std::ffi::c_void; -use tracing::{debug, error, info}; +use tracing::{error, info}; /// SQLDriverConnect establishes connections to a driver and a data source using a connection string /// @@ -64,7 +64,8 @@ pub extern "C" fn SQLDriverConnectW( } }; - debug!("Connection string: {}", connection_string); + println!("Connection string: {}", connection_string); + //println!("Connection string parts: {:#?}", connection_string.split(';').collect::>()); // TODO: Parse connection string properly (DSN=..., Database=..., etc.) // For now, just extract database path from a simple connection string @@ -81,6 +82,7 @@ pub extern "C" fn SQLDriverConnectW( // No database specified - this is an error None }; + println!("DEBUG: database_path result: {:?}", database_path); match database_path { Some(db_path) => { diff --git a/src/odbc/api/sqlgetfunctions.rs b/src/odbc/api/sqlgetfunctions.rs index 729cb11..c2b8ff3 100644 --- a/src/odbc/api/sqlgetfunctions.rs +++ b/src/odbc/api/sqlgetfunctions.rs @@ -1,3 +1,4 @@ +#![allow(dead_code)] use odbc_sys::{SqlReturn, USmallInt}; use std::ffi::c_void; use std::slice; diff --git a/src/odbc/api/sqlgetinfo.rs b/src/odbc/api/sqlgetinfo.rs index 63a90b0..d4acb49 100644 --- a/src/odbc/api/sqlgetinfo.rs +++ b/src/odbc/api/sqlgetinfo.rs @@ -9,7 +9,7 @@ const STRING_LENGTH_FOR_UINTEGER: i16 = std::mem::size_of::() as i16; /// Returns general information about the driver and data source associated with a connection /// -/// https://learn.microsoft.com/en-us/sql/odbc/reference/syntax/sqlgetinfo-function +/// /// /// # Returns /// `SUCCESS`, `SUCCESS_WITH_INFO`, `ERROR`, or `INVALID_HANDLE` diff --git a/src/odbc/def/c_data_type.rs b/src/odbc/def/c_data_type.rs index 6fe1b2e..0ac754a 100644 --- a/src/odbc/def/c_data_type.rs +++ b/src/odbc/def/c_data_type.rs @@ -1,3 +1,4 @@ +#![allow(dead_code)] /// Extended C Types range 4000 and above. Range of -100 thru 200 is reserved by Driver Manager. /// `SQL_C_TYPES_EXTENDED`. pub const C_TYPES_EXTENDED: i16 = 0x04000; @@ -82,5 +83,5 @@ pub enum CDataType { #[cfg(windows)] pub use CDataType::ULong as UBigInt; -#[cfg(not(windows))] -pub use CDataType::ULong as Bookmark; +//#[cfg(not(windows))] +//pub use CDataType::ULong as Bookmark; diff --git a/src/odbc/implementation/alloc_handles.rs b/src/odbc/implementation/alloc_handles.rs index 5d850d7..0120532 100644 --- a/src/odbc/implementation/alloc_handles.rs +++ b/src/odbc/implementation/alloc_handles.rs @@ -1,3 +1,4 @@ +#![allow(dead_code)] use odbc_sys::AttrOdbcVersion; use rusqlite::{Connection, Row, Rows, Statement}; @@ -38,7 +39,9 @@ pub(crate) fn impl_allocate_dbc_handle(_env_handle: &mut EnvironmentHandle) -> C } } -pub(crate) fn allocate_stmt_handle(connection_handle: &mut ConnectionHandle) -> StatementHandle<'_> { +pub(crate) fn allocate_stmt_handle( + connection_handle: &mut ConnectionHandle, +) -> StatementHandle<'_> { let connection_ref = connection_handle.sqlite_connection.as_ref().unwrap(); StatementHandle { sqlite_connection: connection_ref, diff --git a/tests/basic_odbc_connection_test.rs b/tests/basic_odbc_connection_test.rs index 33a500e..2da577b 100644 --- a/tests/basic_odbc_connection_test.rs +++ b/tests/basic_odbc_connection_test.rs @@ -1,5 +1,6 @@ +#![allow(dead_code)] use odbc_api::buffers::TextRowSet; -use odbc_api::{ConnectionOptions, Cursor, Environment, ResultSetMetadata}; +use odbc_api::{ConnectionOptions, Cursor, Environment}; use std::process::Command; /// Basic ODBC connection test using odbc-api crate @@ -8,6 +9,7 @@ use std::process::Command; /// to our driver through the standard ODBC client stack. const CONNECTION_STRING: &str = "DSN=test_connection"; +//const CONNECTION_STRING: &str = "Driver=/home/andrew/gitrepos/odbc-rs-sqlite/target/debug/libodbc_driver_rs.so;Database=/home/andrew/gitrepos/odbc-rs-sqlite/test_odbc.sqlite"; /// Set up test environment by building driver and configuring ODBC fn setup_test_environment() -> std::result::Result<(), Box> { From 12c4d3ab4d6739bbd95aeb558d7b6c401156c10e Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Fri, 13 Feb 2026 14:24:40 +0100 Subject: [PATCH 2/5] Fix clippy warnings and add DSN resolution to SQLDriverConnectW - Fix all 16 clippy warnings: empty line after attr, let-and-return, needless borrows, uninlined format args, clone-on-copy, module inception, enum variant names, too-many-arguments - Add DSN resolution to SQLDriverConnectW so connection strings with DSN= (but no Database=) resolve the database path from odbc.ini - Extract impl_connect_to_database for direct database path connections, avoiding double DSN lookup when called from SQLDriverConnectW - Untrack .claude/settings.local.json (already in .gitignore) Co-Authored-By: Claude Opus 4.6 --- .claude/settings.local.json | 9 ------ src/lib.rs | 1 - src/odbc/api/sqlallochandle.rs | 6 ++-- src/odbc/api/sqlcolattribute.rs | 4 +-- src/odbc/api/sqldriverconnect.rs | 41 ++++++++++++++++++++++++---- src/odbc/api/sqlspecialcolumns.rs | 2 +- src/odbc/api/sqlstatistics.rs | 2 +- src/odbc/implementation.rs | 1 + src/odbc/implementation/connect.rs | 13 ++++++++- src/odbc/implementation/env_attrs.rs | 2 +- src/odbc/implementation/getdata.rs | 4 +-- src/odbc/utils.rs | 5 ++-- 12 files changed, 60 insertions(+), 30 deletions(-) delete mode 100644 .claude/settings.local.json diff --git a/.claude/settings.local.json b/.claude/settings.local.json deleted file mode 100644 index c36ca14..0000000 --- a/.claude/settings.local.json +++ /dev/null @@ -1,9 +0,0 @@ -{ - "permissions": { - "allow": [ - "Bash(cargo test:*)", - "Bash(./scripts/setup-test-db.sh:*)" - ], - "deny": [] - } -} \ No newline at end of file diff --git a/src/lib.rs b/src/lib.rs index d6971c6..0c0d785 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -30,7 +30,6 @@ mod odbc; all(not(windows), feature = "static", feature = "iodbc"), link(name = "iodbcinst", kind = "static") )] - // This function is here because it's needed for ODBC configuration file parsing // but doesn't exist in the standard odbc-sys crate. // It was added here directly temporarily, eventually this might move to a separate file or library. diff --git a/src/odbc/api/sqlallochandle.rs b/src/odbc/api/sqlallochandle.rs index a1534a7..66b96c7 100644 --- a/src/odbc/api/sqlallochandle.rs +++ b/src/odbc/api/sqlallochandle.rs @@ -42,7 +42,7 @@ pub extern "C" fn SQLAllocHandle( // From the spec: // When allocating a handle other than an environment handle, if SQLAllocHandle returns SQL_ERROR, it sets OutputHandlePtr to SQL_NULL_HDBC, SQL_NULL_HSTMT, or SQL_NULL_HDESC, depending on the value of HandleType, unless the output argument is a null pointer. // The application can then obtain additional information from the diagnostic data structure associated with the handle in the InputHandle argument. - let result = match handle_type { + match handle_type { HandleType::Env => { // Spec: If HandleType is SQL_HANDLE_ENV, this is SQL_NULL_HANDLE. if !input_handle.is_null() { @@ -106,9 +106,7 @@ pub extern "C" fn SQLAllocHandle( } HandleType::Desc => SqlReturn::SUCCESS, HandleType::DbcInfoToken => SqlReturn::SUCCESS, - }; - - result + } } #[cfg(test)] diff --git a/src/odbc/api/sqlcolattribute.rs b/src/odbc/api/sqlcolattribute.rs index 43c14dd..cd332fd 100644 --- a/src/odbc/api/sqlcolattribute.rs +++ b/src/odbc/api/sqlcolattribute.rs @@ -103,7 +103,7 @@ pub extern "C" fn SQLColAttributeW( // Return column name match stmt.column_name(col_index) { Ok(name) => return_string_attribute( - &name, + name, character_attribute_ptr, buffer_length, string_length_ptr, @@ -172,7 +172,7 @@ pub extern "C" fn SQLColAttributeW( // Return column label (same as name for SQLite) match stmt.column_name(col_index) { Ok(name) => return_string_attribute( - &name, + name, character_attribute_ptr, buffer_length, string_length_ptr, diff --git a/src/odbc/api/sqldriverconnect.rs b/src/odbc/api/sqldriverconnect.rs index 252f448..276ee1a 100644 --- a/src/odbc/api/sqldriverconnect.rs +++ b/src/odbc/api/sqldriverconnect.rs @@ -14,8 +14,8 @@ //! ``` use crate::odbc::implementation::alloc_handles::ConnectionHandle; -use crate::odbc::implementation::connect::impl_connect; -use crate::odbc::utils::{get_from_wrapper, maybe_utf16_to_string}; +use crate::odbc::implementation::connect::impl_connect_to_database; +use crate::odbc::utils::{get_from_wrapper, get_private_profile_string, maybe_utf16_to_string}; use odbc_sys::{HandleType, SmallInt, SqlReturn, USmallInt, WChar}; use std::ffi::c_void; use tracing::{error, info}; @@ -64,7 +64,7 @@ pub extern "C" fn SQLDriverConnectW( } }; - println!("Connection string: {}", connection_string); + println!("Connection string: {connection_string}"); //println!("Connection string parts: {:#?}", connection_string.split(';').collect::>()); // TODO: Parse connection string properly (DSN=..., Database=..., etc.) @@ -78,18 +78,47 @@ pub extern "C" fn SQLDriverConnectW( .find(|part| part.starts_with("Database=")) .and_then(|part| part.strip_prefix("Database=")) .map(|path| path.trim().to_string()) + } else if connection_string.contains("DSN=") { + // Resolve Database from DSN configuration + let dsn = connection_string + .split(';') + .find(|part| part.starts_with("DSN=")) + .and_then(|part| part.strip_prefix("DSN=")) + .map(|s| s.trim().to_string()); + + if let Some(dsn_name) = dsn { + match get_private_profile_string(&dsn_name, "Database", "odbc.ini", 1024) { + Ok(Some(db)) => { + info!("Resolved Database={} from DSN={}", db, dsn_name); + Some(db) + } + Ok(None) => { + error!( + "DSN '{}' found but no Database setting configured", + dsn_name + ); + None + } + Err(e) => { + error!("Failed to look up DSN '{}': {}", dsn_name, e); + None + } + } + } else { + None + } } else { // No database specified - this is an error None }; - println!("DEBUG: database_path result: {:?}", database_path); + println!("DEBUG: database_path result: {database_path:?}"); match database_path { Some(db_path) => { info!("Connecting to database: {}", db_path); - // Use existing connection logic - impl_connect(connection_handle, db_path, None, None); + // Open the database directly — DSN resolution already happened above + impl_connect_to_database(connection_handle, db_path); // TODO: Copy connection string to output buffer if provided if !out_connection_string.is_null() && buffer_length > 0 { diff --git a/src/odbc/api/sqlspecialcolumns.rs b/src/odbc/api/sqlspecialcolumns.rs index 41a2225..bec3ca4 100644 --- a/src/odbc/api/sqlspecialcolumns.rs +++ b/src/odbc/api/sqlspecialcolumns.rs @@ -2,7 +2,7 @@ use odbc_sys::SqlReturn; use std::ffi::c_void; use tracing::info; -#[allow(non_snake_case)] +#[allow(non_snake_case, clippy::too_many_arguments)] #[unsafe(no_mangle)] pub fn SQLSpecialColumnsW( _statement_handle: *mut c_void, diff --git a/src/odbc/api/sqlstatistics.rs b/src/odbc/api/sqlstatistics.rs index 9ca9d61..4f9c249 100644 --- a/src/odbc/api/sqlstatistics.rs +++ b/src/odbc/api/sqlstatistics.rs @@ -2,7 +2,7 @@ use odbc_sys::SqlReturn; use std::ffi::c_void; use tracing::info; -#[allow(non_snake_case)] +#[allow(non_snake_case, clippy::too_many_arguments)] #[unsafe(no_mangle)] pub fn SQLStatisticsW( _statement_handle: *mut c_void, diff --git a/src/odbc/implementation.rs b/src/odbc/implementation.rs index 97bdad9..c9d14b1 100644 --- a/src/odbc/implementation.rs +++ b/src/odbc/implementation.rs @@ -2,4 +2,5 @@ pub(crate) mod alloc_handles; pub(crate) mod connect; pub(crate) mod env_attrs; pub(crate) mod getdata; +#[allow(clippy::module_inception)] pub(crate) mod implementation; diff --git a/src/odbc/implementation/connect.rs b/src/odbc/implementation/connect.rs index 2d8f6d7..594ee30 100644 --- a/src/odbc/implementation/connect.rs +++ b/src/odbc/implementation/connect.rs @@ -3,6 +3,8 @@ use crate::odbc::utils::get_private_profile_string; use rusqlite::{Connection, OpenFlags}; use tracing::{error, info}; +/// Connect via DSN name — looks up Database from ODBC configuration. +/// Used by SQLConnectW. pub(crate) fn impl_connect( connection_handle: &mut ConnectionHandle, server_name: String, @@ -21,8 +23,17 @@ pub(crate) fn impl_connect( } }; info!("Opening [{}] for DSN [{}]", database, server_name); + impl_connect_to_database(connection_handle, database); +} + +/// Connect directly to a database file path. +/// Used by SQLDriverConnectW after resolving the database path. +pub(crate) fn impl_connect_to_database( + connection_handle: &mut ConnectionHandle, + database_path: String, +) { let conn = match Connection::open_with_flags( - database, + &database_path, OpenFlags::SQLITE_OPEN_READ_WRITE | OpenFlags::SQLITE_OPEN_URI | OpenFlags::SQLITE_OPEN_NO_MUTEX, diff --git a/src/odbc/implementation/env_attrs.rs b/src/odbc/implementation/env_attrs.rs index 21336f7..52d34fc 100644 --- a/src/odbc/implementation/env_attrs.rs +++ b/src/odbc/implementation/env_attrs.rs @@ -7,5 +7,5 @@ pub(crate) fn set_odbc_version(env: &mut EnvironmentHandle, odbc_version: AttrOd } pub(crate) fn get_odbc_version(env: &EnvironmentHandle) -> AttrOdbcVersion { - env.odbc_version.clone() + env.odbc_version } diff --git a/src/odbc/implementation/getdata.rs b/src/odbc/implementation/getdata.rs index 520bfc8..26f8dce 100644 --- a/src/odbc/implementation/getdata.rs +++ b/src/odbc/implementation/getdata.rs @@ -45,7 +45,7 @@ pub(crate) fn impl_getdata( ValueRef::Blob(b) => { // Convert blob to hex string representation b.iter() - .map(|byte| format!("{:02x}", byte)) + .map(|byte| format!("{byte:02x}")) .collect::() } } @@ -101,7 +101,7 @@ pub(crate) fn impl_getdata( }, ValueRef::Blob(b) => b .iter() - .map(|byte| format!("{:02x}", byte)) + .map(|byte| format!("{byte:02x}")) .collect::(), } } diff --git a/src/odbc/utils.rs b/src/odbc/utils.rs index 8653131..101b39b 100644 --- a/src/odbc/utils.rs +++ b/src/odbc/utils.rs @@ -14,6 +14,7 @@ const DESC_TAG: i32 = 108498290; const DBC_INFO_TOKEN_TAG: i32 = 983280932; #[derive(Debug, Snafu)] +#[allow(clippy::enum_variant_names)] pub enum Error { #[snafu(display("SQLGetPrivateProfileStringW returned a negative value: {ret}"))] UnknownError { ret: i32 }, @@ -61,13 +62,13 @@ pub fn get_from_wrapper<'a, T>( ensure!( !wrapper_pointer.is_null(), NullPointerSnafu { - handle_type: handle_type.clone() + handle_type: *handle_type } ); let in_wrapper: &HandleWrapper = unsafe { &*(wrapper_pointer as *const HandleWrapper) }; - let tag = tag_for_handle(&handle_type); + let tag = tag_for_handle(handle_type); ensure!( in_wrapper.tag == tag, InvalidTagSnafu { From 0b0fef757e8f0b44524a2506fa95e5417dd05f27 Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Fri, 13 Feb 2026 14:27:14 +0100 Subject: [PATCH 3/5] Add ODBC trace file to .gitignore Co-Authored-By: Claude Opus 4.6 --- .gitignore | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitignore b/.gitignore index 5a8f89a..7dde91a 100644 --- a/.gitignore +++ b/.gitignore @@ -3,3 +3,4 @@ test_odbc.sqlite lars_notes.md .claude/ +ODBC From c72d60518a3c044efea1634503d03642ac26e63a Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Fri, 13 Feb 2026 14:58:36 +0100 Subject: [PATCH 4/5] Remove unwraps from FFI code and implement SQLDescribeColW - Replace 3 .unwrap() calls in FFI functions with proper error handling that returns SqlReturn::ERROR instead of panicking the host application - Change allocate_stmt_handle to return Option so callers handle missing connections gracefully - Implement SQLDescribeColW with column name (UTF-16), type (SQL_VARCHAR), size (255), decimal digits (0), and nullability (SQL_NULLABLE) - Enable `wide` feature on odbc-api dev-dependency for proper Unicode ODBC client testing - Add test_describe_columns integration test Co-Authored-By: Claude Opus 4.6 --- Cargo.toml | 2 +- plan.md | 12 +- src/odbc/api/sqlallochandle.rs | 9 +- src/odbc/api/sqldescribecol.rs | 142 ++++++++++++++++++++--- src/odbc/api/sqltables.rs | 24 +++- src/odbc/implementation/alloc_handles.rs | 8 +- tests/basic_odbc_connection_test.rs | 50 +++++++- 7 files changed, 217 insertions(+), 30 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index aab9374..9f9b049 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -48,4 +48,4 @@ tracing = "0.1" tracing-subscriber = { version = "0.3", features = ["env-filter", "fmt"] } [dev-dependencies] -odbc-api = "14.3.0" +odbc-api = { version = "14.3.0", features = ["wide"] } diff --git a/plan.md b/plan.md index 2130960..914aa57 100644 --- a/plan.md +++ b/plan.md @@ -105,9 +105,10 @@ - [ ] **Fix StatementHandle lifetime issues** (`src/odbc/implementation/alloc_handles.rs:24-29`) - Replace raw references with proper ownership patterns - Ensure statements don't outlive connections -- [ ] **Remove all `.unwrap()` from FFI functions** - - Replace with proper error return codes - - Add error logging without panicking +- [DONE - 2026-02-13] **Remove all `.unwrap()` from FFI functions** + - SOLUTION: Replaced 3 `.unwrap()` calls in FFI code with proper error handling + - FILES: Modified src/odbc/api/sqltables.rs (prepare/query unwraps → match with SqlReturn::ERROR), src/odbc/implementation/alloc_handles.rs (allocate_stmt_handle returns Option), src/odbc/api/sqlallochandle.rs (handles None from allocate_stmt_handle) + - OUTCOME: No more panics possible from FFI code; all errors return SqlReturn::ERROR with logging ### **Phase 2: Robustness & Completeness** *Goal: Production-ready error handling and core functionality* @@ -127,6 +128,11 @@ - Implement proper handle deallocation based on handle type - Free both `HandleWrapper` and inner objects - Add comprehensive tests +- [DONE - 2026-02-13] **SQLDescribeColW implementation** + - SOLUTION: Full implementation returning column name (UTF-16), data type (SQL_VARCHAR), column size (255), decimal digits (0), nullable (SQL_NULLABLE) + - FILES: Rewrote src/odbc/api/sqldescribecol.rs, added test_describe_columns integration test + - ALSO: Enabled `wide` feature on odbc-api dev-dependency for proper Unicode ODBC client testing, discovered `cargo build` is needed before integration tests (cdylib loaded at runtime by DM) + - KEY INSIGHT: ODBC spec for SQLDescribeColW uses character counts (not byte counts) for buffer_length and name_length_ptr - [ ] **SQLDriverConnect implementation** - Parse connection strings properly - Support standard SQLite connection parameters diff --git a/src/odbc/api/sqlallochandle.rs b/src/odbc/api/sqlallochandle.rs index 66b96c7..0db5123 100644 --- a/src/odbc/api/sqlallochandle.rs +++ b/src/odbc/api/sqlallochandle.rs @@ -97,7 +97,14 @@ pub extern "C" fn SQLAllocHandle( } }; - let handle = allocate_stmt_handle(connection_handle); + let handle = match allocate_stmt_handle(connection_handle) { + Some(handle) => handle, + None => { + error!("Cannot allocate statement handle: no active connection"); + unsafe { *output_handle = std::ptr::null_mut() } + return SqlReturn::ERROR; + } + }; wrap_and_set(handle_type, handle, output_handle); info!("Successfully allocated a Stmt handle"); diff --git a/src/odbc/api/sqldescribecol.rs b/src/odbc/api/sqldescribecol.rs index 935f178..a478637 100644 --- a/src/odbc/api/sqldescribecol.rs +++ b/src/odbc/api/sqldescribecol.rs @@ -1,20 +1,134 @@ -use odbc_sys::SqlReturn; +use crate::odbc::implementation::alloc_handles::StatementHandle; +use crate::odbc::utils::get_from_wrapper; +use odbc_sys::{HandleType, SqlReturn}; use std::ffi::c_void; -use tracing::info; +use std::ptr; +use tracing::{debug, error, info}; +/// SQLDescribeColW returns the result descriptor for one column in the result set. +/// +/// This function provides column metadata including name, data type, size, +/// decimal digits, and nullability. It must be called after a statement has +/// been prepared (via SQLPrepareW or SQLExecDirectW). #[allow(non_snake_case)] #[unsafe(no_mangle)] -pub extern "C" fn SQLDescribeCol( - _statement_handle: *mut c_void, - _column_number: u16, - _column_name: *mut u16, - _buffer_length: i16, - _name_length_ptr: *mut i16, - _data_type_ptr: *mut i16, - _column_size_ptr: *mut usize, - _decimal_digits_ptr: *mut i16, - _nullable_ptr: *mut i16, +pub extern "C" fn SQLDescribeColW( + statement_handle: *mut c_void, + column_number: u16, + column_name: *mut u16, + buffer_length: i16, + name_length_ptr: *mut i16, + data_type_ptr: *mut i16, + column_size_ptr: *mut usize, + decimal_digits_ptr: *mut i16, + nullable_ptr: *mut i16, ) -> SqlReturn { - info!("SQLDescribeCol"); - SqlReturn::SUCCESS + info!( + "column_number={}, buffer_length={}", + column_number, buffer_length + ); + + // Get the statement handle + let statement_handle: &mut StatementHandle = + match get_from_wrapper(&HandleType::Stmt, statement_handle) { + Ok(handle) => handle, + Err(err) => { + error!("Failed to get statement handle: {}", err); + return SqlReturn::INVALID_HANDLE; + } + }; + + // Check if we have a prepared statement + let stmt = match &statement_handle.statement { + Some(stmt) => stmt, + None => { + error!("No prepared statement found"); + return SqlReturn::ERROR; + } + }; + + // Validate column number (1-indexed in ODBC) + if column_number == 0 || column_number as usize > stmt.column_count() { + error!( + "Invalid column number {}. Valid range: 1-{}", + column_number, + stmt.column_count() + ); + return SqlReturn::ERROR; + } + + // Convert to 0-indexed for SQLite + let col_index = (column_number - 1) as usize; + + // Write column name as UTF-16 + let mut result = SqlReturn::SUCCESS; + match stmt.column_name(col_index) { + Ok(name) => { + debug!("Column {} name: '{}'", column_number, name); + + let utf16_value: Vec = name.encode_utf16().collect(); + let utf16_len = utf16_value.len() as i16; + + // Set the actual name length in characters (excluding null terminator) + if !name_length_ptr.is_null() { + unsafe { + *name_length_ptr = utf16_len; + } + } + + // Copy the name string if buffer is provided + // buffer_length is in characters (u16 elements) per ODBC spec + if !column_name.is_null() && buffer_length > 0 { + let max_chars = buffer_length as usize; + let copy_len = std::cmp::min(utf16_value.len(), max_chars.saturating_sub(1)); + + unsafe { + ptr::copy_nonoverlapping(utf16_value.as_ptr(), column_name, copy_len); + // Null-terminate + *column_name.add(copy_len) = 0; + } + + if copy_len < utf16_value.len() { + result = SqlReturn::SUCCESS_WITH_INFO; + } + } + } + Err(err) => { + error!("Could not get column name for index {}: {}", col_index, err); + return SqlReturn::ERROR; + } + } + + // Set data type - SQLite is dynamically typed, report VARCHAR for all columns + if !data_type_ptr.is_null() { + unsafe { + *data_type_ptr = 12; // SQL_VARCHAR + } + debug!("Returning type: SQL_VARCHAR"); + } + + // Set column size - default VARCHAR length + if !column_size_ptr.is_null() { + unsafe { + *column_size_ptr = 255; + } + debug!("Returning column size: 255"); + } + + // Set decimal digits - 0 for VARCHAR + if !decimal_digits_ptr.is_null() { + unsafe { + *decimal_digits_ptr = 0; + } + } + + // Set nullable - SQLite columns are generally nullable + if !nullable_ptr.is_null() { + unsafe { + *nullable_ptr = 1; // SQL_NULLABLE + } + debug!("Returning nullable: SQL_NULLABLE"); + } + + result } diff --git a/src/odbc/api/sqltables.rs b/src/odbc/api/sqltables.rs index 0e4f850..0fff081 100644 --- a/src/odbc/api/sqltables.rs +++ b/src/odbc/api/sqltables.rs @@ -31,18 +31,30 @@ pub extern "C" fn SQLTablesW( } }; - // Assuming `statement_handle` is mutable and has fields for storing `stmt` and `rows` - let stmt = statement_handle + // Prepare the query to list tables + let stmt = match statement_handle .sqlite_connection .prepare("SELECT name FROM sqlite_master WHERE type='table'") - .unwrap(); + { + Ok(stmt) => stmt, + Err(err) => { + error!("Failed to prepare table listing query: {}", err); + return SqlReturn::ERROR; + } + }; statement_handle.statement = Some(stmt); if let Some(ref mut stmt) = statement_handle.statement { - statement_handle.rows = Some(stmt.query([]).unwrap()); + match stmt.query([]) { + Ok(rows) => { + statement_handle.rows = Some(rows); + } + Err(err) => { + error!("Failed to execute table listing query: {}", err); + return SqlReturn::ERROR; + } + } } - //let catalog_name = maybe_utf16_to_string(catalog_name, catalog_name_length).unwrap_or("DEFAULT".to_string()); - SqlReturn::SUCCESS } diff --git a/src/odbc/implementation/alloc_handles.rs b/src/odbc/implementation/alloc_handles.rs index 0120532..7df3809 100644 --- a/src/odbc/implementation/alloc_handles.rs +++ b/src/odbc/implementation/alloc_handles.rs @@ -41,12 +41,12 @@ pub(crate) fn impl_allocate_dbc_handle(_env_handle: &mut EnvironmentHandle) -> C pub(crate) fn allocate_stmt_handle( connection_handle: &mut ConnectionHandle, -) -> StatementHandle<'_> { - let connection_ref = connection_handle.sqlite_connection.as_ref().unwrap(); - StatementHandle { +) -> Option> { + let connection_ref = connection_handle.sqlite_connection.as_ref()?; + Some(StatementHandle { sqlite_connection: connection_ref, statement: None, rows: None, row: None, - } + }) } diff --git a/tests/basic_odbc_connection_test.rs b/tests/basic_odbc_connection_test.rs index 2da577b..cc3451d 100644 --- a/tests/basic_odbc_connection_test.rs +++ b/tests/basic_odbc_connection_test.rs @@ -1,6 +1,10 @@ #![allow(dead_code)] use odbc_api::buffers::TextRowSet; -use odbc_api::{ConnectionOptions, Cursor, Environment}; +use odbc_api::{ + ColumnDescription, ConnectionOptions, Cursor, DataType, Environment, Nullability, + ResultSetMetadata, +}; +use std::num::NonZeroUsize; use std::process::Command; /// Basic ODBC connection test using odbc-api crate @@ -136,6 +140,50 @@ fn test_multiple_connections() { println!("✅ Second connection closed successfully"); } +#[test] +fn test_describe_columns() { + let env = Environment::new().expect("Failed to create ODBC environment"); + + let connection = env + .connect("test_connection", "", "", ConnectionOptions::default()) + .expect("Failed to connect to database"); + + // Execute a query so we have a result set with known columns + let mut cursor = connection + .execute( + "SELECT name FROM sqlite_master WHERE type='table'", + (), + None, + ) + .expect("Failed to execute query") + .expect("Expected a cursor for SELECT statement"); + + // Describe column 1 (the "name" column) + let mut col_desc = ColumnDescription::default(); + cursor + .describe_col(1, &mut col_desc) + .expect("Failed to describe column 1"); + + let col_name = col_desc + .name_to_string() + .expect("Failed to decode column name"); + println!( + "Column 1 name: '{}', data_type: {:?}, nullable: {:?}", + col_name, col_desc.data_type, col_desc.nullability + ); + + assert_eq!(col_name, "name"); + assert_eq!( + col_desc.data_type, + DataType::Varchar { + length: NonZeroUsize::new(255) + } + ); + assert_eq!(col_desc.nullability, Nullability::Nullable); + + println!("describe_columns test passed"); +} + #[test] fn test_invalid_connection_string() { setup_test_environment().expect("Test environment setup failed"); From 7aa9aebd5dfca1acfcd6bb2f252bff2a45bda19f Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Fri, 13 Feb 2026 15:10:40 +0100 Subject: [PATCH 5/5] Normalize get_from_wrapper failures to return INVALID_HANDLE Per ODBC spec, SQL_INVALID_HANDLE should be returned when a handle argument is invalid (null, wrong type, or corrupted tag). Previously 8 of 16 call sites incorrectly returned SQL_ERROR instead. Also normalizes error messages to the consistent pattern: error!("Failed to get {handle_type} handle: {}", err) Co-Authored-By: Claude Opus 4.6 --- src/odbc/api/sqlallochandle.rs | 8 ++++---- src/odbc/api/sqlconnect.rs | 4 ++-- src/odbc/api/sqldriverconnect.rs | 4 ++-- src/odbc/api/sqlgetdata.rs | 4 ++-- src/odbc/api/sqlgetenvattr.rs | 4 ++-- src/odbc/api/sqlnumresultcols.rs | 4 ++-- src/odbc/api/sqlsetenvattr.rs | 4 ++-- src/odbc/api/sqltables.rs | 4 ++-- 8 files changed, 18 insertions(+), 18 deletions(-) diff --git a/src/odbc/api/sqlallochandle.rs b/src/odbc/api/sqlallochandle.rs index 0db5123..9059cbf 100644 --- a/src/odbc/api/sqlallochandle.rs +++ b/src/odbc/api/sqlallochandle.rs @@ -73,9 +73,9 @@ pub extern "C" fn SQLAllocHandle( match get_from_wrapper(&HandleType::Env, input_handle) { Ok(env) => env, Err(err) => { - error!("Getting environment handle: {}", err); + error!("Failed to get environment handle: {}", err); unsafe { *output_handle = std::ptr::null_mut() } - return SqlReturn::ERROR; + return SqlReturn::INVALID_HANDLE; } }; @@ -91,9 +91,9 @@ pub extern "C" fn SQLAllocHandle( match get_from_wrapper(&HandleType::Dbc, input_handle) { Ok(env) => env, Err(err) => { - info!("Getting connection handle: {}", err); + error!("Failed to get connection handle: {}", err); unsafe { *output_handle = std::ptr::null_mut() } - return SqlReturn::ERROR; + return SqlReturn::INVALID_HANDLE; } }; diff --git a/src/odbc/api/sqlconnect.rs b/src/odbc/api/sqlconnect.rs index a0ccea9..3b8cca3 100644 --- a/src/odbc/api/sqlconnect.rs +++ b/src/odbc/api/sqlconnect.rs @@ -41,8 +41,8 @@ pub extern "C" fn SQLConnectW( match get_from_wrapper(&HandleType::Dbc, connection_handle) { Ok(env) => env, Err(e) => { - error!("{}", e); - return SqlReturn::ERROR; + error!("Failed to get connection handle: {}", e); + return SqlReturn::INVALID_HANDLE; } }; diff --git a/src/odbc/api/sqldriverconnect.rs b/src/odbc/api/sqldriverconnect.rs index 276ee1a..48e93d9 100644 --- a/src/odbc/api/sqldriverconnect.rs +++ b/src/odbc/api/sqldriverconnect.rs @@ -50,8 +50,8 @@ pub extern "C" fn SQLDriverConnectW( match get_from_wrapper(&HandleType::Dbc, connection_handle) { Ok(conn) => conn, Err(e) => { - error!("Error getting connection handle {}", e); - return SqlReturn::ERROR; + error!("Failed to get connection handle: {}", e); + return SqlReturn::INVALID_HANDLE; } }; diff --git a/src/odbc/api/sqlgetdata.rs b/src/odbc/api/sqlgetdata.rs index 5a912b7..acdfb28 100644 --- a/src/odbc/api/sqlgetdata.rs +++ b/src/odbc/api/sqlgetdata.rs @@ -21,8 +21,8 @@ pub extern "C" fn SQLGetData( match get_from_wrapper(&HandleType::Stmt, statement_handle) { Ok(handle) => handle, Err(err) => { - error!("{}", err); - return SqlReturn::ERROR; + error!("Failed to get statement handle: {}", err); + return SqlReturn::INVALID_HANDLE; } }; diff --git a/src/odbc/api/sqlgetenvattr.rs b/src/odbc/api/sqlgetenvattr.rs index 6284707..b166169 100644 --- a/src/odbc/api/sqlgetenvattr.rs +++ b/src/odbc/api/sqlgetenvattr.rs @@ -38,8 +38,8 @@ pub extern "C" fn SQLGetEnvAttr( let env: &mut EnvironmentHandle = match get_from_wrapper(&HandleType::Env, environment_handle) { Ok(env) => env, Err(err) => { - error!("{}", err); - return SqlReturn::ERROR; + error!("Failed to get environment handle: {}", err); + return SqlReturn::INVALID_HANDLE; } }; diff --git a/src/odbc/api/sqlnumresultcols.rs b/src/odbc/api/sqlnumresultcols.rs index ec68fcb..d1c00d3 100644 --- a/src/odbc/api/sqlnumresultcols.rs +++ b/src/odbc/api/sqlnumresultcols.rs @@ -27,8 +27,8 @@ pub unsafe extern "C" fn SQLNumResultCols( match get_from_wrapper(&HandleType::Stmt, statement_handle) { Ok(handle) => handle, Err(err) => { - error!("{}", err); - return SqlReturn::ERROR; + error!("Failed to get statement handle: {}", err); + return SqlReturn::INVALID_HANDLE; } }; diff --git a/src/odbc/api/sqlsetenvattr.rs b/src/odbc/api/sqlsetenvattr.rs index 28d26a0..cec92b1 100644 --- a/src/odbc/api/sqlsetenvattr.rs +++ b/src/odbc/api/sqlsetenvattr.rs @@ -38,8 +38,8 @@ pub fn SQLSetEnvAttr( let env: &mut EnvironmentHandle = match get_from_wrapper(&HandleType::Env, environment_handle) { Ok(env) => env, Err(err) => { - error!("{}", err); - return SqlReturn::ERROR; + error!("Failed to get environment handle: {}", err); + return SqlReturn::INVALID_HANDLE; } }; diff --git a/src/odbc/api/sqltables.rs b/src/odbc/api/sqltables.rs index 0fff081..be78056 100644 --- a/src/odbc/api/sqltables.rs +++ b/src/odbc/api/sqltables.rs @@ -26,8 +26,8 @@ pub extern "C" fn SQLTablesW( match get_from_wrapper(&HandleType::Stmt, statement_handle) { Ok(env) => env, Err(err) => { - error!("{}", err); - return SqlReturn::ERROR; + error!("Failed to get statement handle: {}", err); + return SqlReturn::INVALID_HANDLE; } };