SNOW-3234503 ODBC Connection initialization#662
SNOW-3234503 ODBC Connection initialization#662sfc-gh-daniszewski wants to merge 1 commit intomainfrom
Conversation
Description Testing
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
There was a problem hiding this comment.
Pull request overview
Adds initial ODBC connection initialization coverage by implementing DSN-backed SQLConnect and introducing SQLBrowseConnect support, along with new end-to-end tests and a Gherkin feature definition.
Changes:
- Implement
SQLConnect{,W}by routing through the existing connection pipeline and adding DSN config loading. - Add
SQLBrowseConnect{,W}with per-connection accumulation of browse-connect attributes andSQL_NEED_DATAtemplating. - Add new e2e tests (and feature definition) covering DSN connect, unknown DSN SQLSTATE, and browse-connect template behavior.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
tests/definitions/odbc/connection/sql_connect.feature |
New feature scenarios for SQLConnect/SQLBrowseConnect behaviors. |
odbc/src/c_api.rs |
Updates SQLDriverConnect{,W} parameters and exports SQLBrowseConnect{,W} C entrypoints. |
odbc/src/api/types/odbc_types.rs |
Adds browse_connect_attrs state to the connection handle. |
odbc/src/api/mod.rs |
Registers new dsn module. |
odbc/src/api/handle_allocation.rs |
Initializes new browse_connect_attrs field. |
odbc/src/api/error.rs |
Adds DsnNotFound error variant and maps it to SQLSTATE IM002. |
odbc/src/api/dsn.rs |
Introduces DSN config loading (unix odbc.ini parsing; Windows stub). |
odbc/src/api/connection.rs |
Implements SQLConnect and SQLBrowseConnect logic; adds DSN merge + completed-string generation. |
odbc_tests/tests/e2e/connection/sql_connect.cpp |
New Catch2 e2e tests for DSN connect, unknown DSN, and browse-connect template. |
odbc_tests/tests/e2e/CMakeLists.txt |
Adds the new e2e test target. |
| write_string_bytes::<E>( | ||
| &completed, | ||
| out_connection_string, | ||
| out_buffer_length, | ||
| out_string_length, |
There was a problem hiding this comment.
SQLDriverConnect{,W} buffer-length parameters are in character units (and pcbConnStrOut is a character count), but this uses write_string_bytes, which treats buffer_length / *out_string_length as bytes and will under-fill/truncate and report the wrong length for the wide (W) variant. Consider using write_string_chars here (or a dedicated helper that follows the SQLDriverConnect length semantics).
| if !has_server { | ||
| tracing::debug!("browse_connect: insufficient attrs, returning NEED_DATA"); | ||
| write_string_bytes::<E>( | ||
| BROWSE_CONNECT_TEMPLATE, | ||
| out_connection_string, | ||
| out_buffer_length, | ||
| out_string_length, | ||
| None, |
There was a problem hiding this comment.
SQLBrowseConnect{,W} uses write_string_bytes for the output template / completed connection string, but ODBC defines out_buffer_length and *out_string_length for this API in character units. This will misreport lengths and truncate early for the wide (W) variant. Use write_string_chars (or an API-specific helper) to match the expected length units.
|
|
||
| pub fn load(dsn_name: &str) -> Option<HashMap<String, String>> { | ||
| // DSN entries live at: | ||
| // HKCU\SOFTWARE\ODBC\ODBC.INI\<dsn> (user DSN) | ||
| // HKLM\SOFTWARE\ODBC\ODBC.INI\<dsn> (system DSN) | ||
| // | ||
| // TODO: implement Windows registry lookup | ||
| tracing::warn!( | ||
| "dsn: Windows registry DSN loading not yet implemented for {:?}", | ||
| dsn_name | ||
| ); | ||
| None | ||
| } |
There was a problem hiding this comment.
The Windows DSN loader is currently a stub that always returns None (and only logs a warning). With SQLConnect now delegating through DSN resolution, DSN-based connections will always fail on Windows (and the new e2e SQLConnect-via-DSN scenario is likely to fail in Windows CI). Implement the registry lookup (HKCU/HKLM ...\ODBC.INI\<dsn>) or gate the SQLConnect DSN path on cfg(windows) until it’s supported.
| pub fn load(dsn_name: &str) -> Option<HashMap<String, String>> { | |
| // DSN entries live at: | |
| // HKCU\SOFTWARE\ODBC\ODBC.INI\<dsn> (user DSN) | |
| // HKLM\SOFTWARE\ODBC\ODBC.INI\<dsn> (system DSN) | |
| // | |
| // TODO: implement Windows registry lookup | |
| tracing::warn!( | |
| "dsn: Windows registry DSN loading not yet implemented for {:?}", | |
| dsn_name | |
| ); | |
| None | |
| } | |
| use winreg::enums::{HKEY_CURRENT_USER, HKEY_LOCAL_MACHINE, HKEY}; | |
| use winreg::RegKey; | |
| pub fn load(dsn_name: &str) -> Option<HashMap<String, String>> { | |
| // DSN entries live at: | |
| // HKCU\SOFTWARE\ODBC\ODBC.INI\<dsn> (user DSN) | |
| // HKLM\SOFTWARE\ODBC\ODBC.INI\<dsn> (system DSN) | |
| let subkey = format!(r"SOFTWARE\\ODBC\\ODBC.INI\\{dsn_name}"); | |
| tracing::debug!("dsn: loading Windows registry config for {:?}", dsn_name); | |
| // Prefer user DSN (HKCU) over system DSN (HKLM), mirroring typical ODBC behaviour. | |
| if let Some(attrs) = load_from_hive(HKEY_CURRENT_USER, &subkey) { | |
| return Some(attrs); | |
| } | |
| if let Some(attrs) = load_from_hive(HKEY_LOCAL_MACHINE, &subkey) { | |
| return Some(attrs); | |
| } | |
| tracing::debug!("dsn: {:?} not found in Windows registry", dsn_name); | |
| None | |
| } | |
| fn load_from_hive(root: HKEY, subkey: &str) -> Option<HashMap<String, String>> { | |
| let hive = RegKey::predef(root); | |
| let key = match hive.open_subkey(subkey) { | |
| Ok(k) => k, | |
| Err(e) => { | |
| tracing::trace!( | |
| "dsn: Windows registry key {:?}\\{:?} not accessible: {}", | |
| root, | |
| subkey, | |
| e | |
| ); | |
| return None; | |
| } | |
| }; | |
| let mut attrs: HashMap<String, String> = HashMap::new(); | |
| // Enumerate all values under the DSN key and read them as strings. | |
| for value_result in key.enum_values() { | |
| let (name, _) = match value_result { | |
| Ok(pair) => pair, | |
| Err(e) => { | |
| tracing::trace!("dsn: failed to enumerate registry value: {}", e); | |
| continue; | |
| } | |
| }; | |
| match key.get_value::<String, _>(&name) { | |
| Ok(val) => { | |
| attrs.insert(name.to_uppercase(), val); | |
| } | |
| Err(e) => { | |
| tracing::trace!( | |
| "dsn: failed to read registry value {:?} from {:?}: {}", | |
| name, | |
| subkey, | |
| e | |
| ); | |
| } | |
| } | |
| } | |
| if attrs.is_empty() { | |
| None | |
| } else { | |
| Some(attrs) | |
| } | |
| } |
| ) -> OdbcResult<String> { | ||
| if ptr.is_null() { | ||
| return Ok(String::new()); | ||
| } |
There was a problem hiding this comment.
read_optional_string calls E::read_string for any non-null pointer. E::read_string currently treats non-SQL_NTS lengths <= 0 as invalid, so callers passing an explicit empty string with length = 0 (a common pattern for optional SQLConnect args) will get InvalidBufferLength instead of an empty value. Consider treating length == 0 as Ok(String::new()) in read_optional_string.
| } | |
| } | |
| if length == 0 { | |
| return Ok(String::new()); | |
| } |
| // Attempt the connection with the accumulated attributes. | ||
| match driver_connect_core(connection_handle, &conn_str) { | ||
| Ok(completed) => { | ||
| // Reset accumulated state on success. | ||
| conn_from_handle(connection_handle) | ||
| .browse_connect_attrs | ||
| .clear(); | ||
| write_string_bytes::<E>( | ||
| &completed, | ||
| out_connection_string, | ||
| out_buffer_length, | ||
| out_string_length, | ||
| None, | ||
| ); | ||
| Ok(BrowseConnectOutcome::Connected) | ||
| } | ||
| Err(e) => Err(e), | ||
| } |
There was a problem hiding this comment.
browse_connect_attrs is documented as being cleared when the browse-connect loop completes (success or reset), but the current implementation only clears it on success. If driver_connect_core returns an error (or if the caller wants to restart the browse-connect sequence), the accumulated attributes will persist and can leak into later attempts. Consider clearing the map on error and/or explicitly handling a reset condition (e.g., empty input) to match the documented behavior.
Description
This pull request implements comprehensive ODBC connection functionality by adding support for
SQLConnect,SQLBrowseConnect, and DSN (Data Source Name) configuration loading.Key Changes
DSN Configuration Support
dsn.rsmodule with platform-specific DSN loadingodbc.inifiles ($ODBCINI,~/.odbc.ini,/etc/odbc.ini)Enhanced Connection Functions
SQLConnectto connect via DSN with optional explicit credentialsSQLBrowseConnectwith stateful attribute accumulation and connection templatesSQLDriverConnectto support DSN references and return completed connection stringsConnection String Processing
DRIVER,DSN,DESCRIPTION,SSL,LOCALE, andTRACINGparameter filteringError Handling
DsnNotFoundSnafuerror type with appropriate SQLSTATEIM002State Management
browse_connect_attrsfield toConnectionstruct for SQLBrowseConnect stateTesting
SQLConnectwith DSN-based connections and unknown DSN error handlingSQLBrowseConnecttests for connection template generation andSQL_NEED_DATAresponses