From c276d4011f86353cd409544459406fc435a0424c Mon Sep 17 00:00:00 2001 From: "Alina (Xi) Li" Date: Wed, 8 Oct 2025 13:20:02 -0700 Subject: [PATCH 01/11] Extract SQLDriverConnect implementation * SQLDriverConnect requires SQLGetInfo to be implemented for tests to pass, because driver manager requires SQLGetInfo Co-Authored-By: alinalibq --- cpp/src/arrow/flight/sql/odbc/odbc.def | 3 +- cpp/src/arrow/flight/sql/odbc/odbc_api.cc | 148 ++++++++++++++- .../flight/sql/odbc/odbc_impl/CMakeLists.txt | 4 +- .../sql/odbc/odbc_impl/attribute_utils.h | 26 +-- .../odbc/odbc_impl/config/configuration.cc | 16 +- .../sql/odbc/odbc_impl/config/configuration.h | 9 +- .../sql/odbc/odbc_impl/encoding_utils.h | 26 ++- .../odbc/odbc_impl/flight_sql_auth_method.cc | 12 +- .../odbc/odbc_impl/flight_sql_connection.cc | 6 +- .../odbc/odbc_impl/flight_sql_connection.h | 7 + .../odbc_impl/flight_sql_connection_test.cc | 40 ++-- .../arrow/flight/sql/odbc/odbc_impl/main.cc | 14 +- .../sql/odbc/odbc_impl/odbc_connection.cc | 103 +++------- .../sql/odbc/odbc_impl/odbc_connection.h | 8 +- .../sql/odbc/odbc_impl/spi/connection.h | 4 +- .../flight/sql/odbc/odbc_impl/system_dsn.cc | 117 +----------- .../flight/sql/odbc/odbc_impl/system_dsn.h | 4 + .../odbc_impl/ui/dsn_configuration_window.cc | 11 +- .../sql/odbc/odbc_impl/win_system_dsn.cc | 176 ++++++++++++++++++ .../flight/sql/odbc/tests/connection_test.cc | 3 +- 20 files changed, 457 insertions(+), 280 deletions(-) create mode 100644 cpp/src/arrow/flight/sql/odbc/odbc_impl/win_system_dsn.cc diff --git a/cpp/src/arrow/flight/sql/odbc/odbc.def b/cpp/src/arrow/flight/sql/odbc/odbc.def index a8191ff662b..8ba5b3fff78 100644 --- a/cpp/src/arrow/flight/sql/odbc/odbc.def +++ b/cpp/src/arrow/flight/sql/odbc/odbc.def @@ -17,8 +17,7 @@ LIBRARY arrow_flight_sql_odbc EXPORTS - ; GH-46574 TODO enable DSN window - ; ConfigDSNW + ConfigDSNW SQLAllocConnect SQLAllocEnv SQLAllocHandle diff --git a/cpp/src/arrow/flight/sql/odbc/odbc_api.cc b/cpp/src/arrow/flight/sql/odbc/odbc_api.cc index 01780f0efe2..8d156964e5b 100644 --- a/cpp/src/arrow/flight/sql/odbc/odbc_api.cc +++ b/cpp/src/arrow/flight/sql/odbc/odbc_api.cc @@ -31,6 +31,11 @@ #include "arrow/flight/sql/odbc/odbc_impl/spi/connection.h" #include "arrow/util/logging.h" +#if defined _WIN32 || defined _WIN64 +// For displaying DSN Window +# include "arrow/flight/sql/odbc/odbc_impl/system_dsn.h" +#endif + namespace arrow::flight::sql::odbc { SQLRETURN SQLAllocHandle(SQLSMALLINT type, SQLHANDLE parent, SQLHANDLE* result) { ARROW_LOG(DEBUG) << "SQLAllocHandle called with type: " << type @@ -722,6 +727,21 @@ SQLRETURN SQLSetConnectAttr(SQLHDBC conn, SQLINTEGER attr, SQLPOINTER value_ptr, return SQL_INVALID_HANDLE; } +// Load properties from the given DSN. The properties loaded do _not_ overwrite existing +// entries in the properties. +void LoadPropertiesFromDSN(const std::string& dsn, + Connection::ConnPropertyMap& properties) { + arrow::flight::sql::odbc::config::Configuration config; + config.LoadDsn(dsn); + Connection::ConnPropertyMap dsn_properties = config.GetProperties(); + for (auto& [key, value] : dsn_properties) { + auto prop_iter = properties.find(key); + if (prop_iter == properties.end()) { + properties.emplace(std::make_pair(std::move(key), std::move(value))); + } + } +} + SQLRETURN SQLDriverConnect(SQLHDBC conn, SQLHWND window_handle, SQLWCHAR* in_connection_string, SQLSMALLINT in_connection_string_len, @@ -740,13 +760,73 @@ SQLRETURN SQLDriverConnect(SQLHDBC conn, SQLHWND window_handle, << out_connection_string_buffer_len << ", out_connection_string_len: " << static_cast(out_connection_string_len) << ", driver_completion: " << driver_completion; + // GH-46449 TODO: Implement FILEDSN and SAVEFILE keywords according to the spec // GH-46560 TODO: Copy connection string properly in SQLDriverConnect according to the // spec - // GH-46574 TODO: Implement SQLDriverConnect - return SQL_INVALID_HANDLE; + using arrow::flight::sql::odbc::Connection; + using arrow::flight::sql::odbc::DriverException; + using ODBC::ODBCConnection; + + return ODBCConnection::ExecuteWithDiagnostics(conn, SQL_ERROR, [=]() { + ODBCConnection* connection = reinterpret_cast(conn); + std::string connection_string = + ODBC::SqlWcharToString(in_connection_string, in_connection_string_len); + Connection::ConnPropertyMap properties; + std::string dsn = ODBCConnection::GetDsnIfExists(connection_string); + if (!dsn.empty()) { + LoadPropertiesFromDSN(dsn, properties); + } + ODBCConnection::GetPropertiesFromConnString(connection_string, properties); + + std::vector missing_properties; + + // GH-46448 TODO: Implement SQL_DRIVER_COMPLETE_REQUIRED in SQLDriverConnect according + // to the spec +#if defined _WIN32 || defined _WIN64 + // Load the DSN window according to driver_completion + if (driver_completion == SQL_DRIVER_PROMPT) { + // Load DSN window before first attempt to connect + arrow::flight::sql::odbc::config::Configuration config; + if (!DisplayConnectionWindow(window_handle, config, properties)) { + return static_cast(SQL_NO_DATA); + } + connection->Connect(dsn, properties, missing_properties); + } else if (driver_completion == SQL_DRIVER_COMPLETE || + driver_completion == SQL_DRIVER_COMPLETE_REQUIRED) { + try { + connection->Connect(dsn, properties, missing_properties); + } catch (const DriverException&) { + // If first connection fails due to missing attributes, load + // the DSN window and try to connect again + if (!missing_properties.empty()) { + arrow::flight::sql::odbc::config::Configuration config; + missing_properties.clear(); + + if (!DisplayConnectionWindow(window_handle, config, properties)) { + return static_cast(SQL_NO_DATA); + } + connection->Connect(dsn, properties, missing_properties); + } else { + throw; + } + } + } else { + // Default case: attempt connection without showing DSN window + connection->Connect(dsn, properties, missing_properties); + } +#else + // Attempt connection without loading DSN window on macOS/Linux + connection->Connect(dsn, properties, missing_properties); +#endif + // Copy connection string to out_connection_string after connection attempt + return ODBC::GetStringAttribute(true, connection_string, false, out_connection_string, + out_connection_string_buffer_len, + out_connection_string_len, + connection->GetDiagnostics()); + }); } SQLRETURN SQLConnect(SQLHDBC conn, SQLWCHAR* dsn_name, SQLSMALLINT dsn_name_len, @@ -759,14 +839,50 @@ SQLRETURN SQLConnect(SQLHDBC conn, SQLWCHAR* dsn_name, SQLSMALLINT dsn_name_len, << ", user_name_len: " << user_name_len << ", password: " << static_cast(password) << ", password_len: " << password_len; - // GH-46574 TODO: Implement SQLConnect - return SQL_INVALID_HANDLE; + + using arrow::flight::sql::odbc::FlightSqlConnection; + using arrow::flight::sql::odbc::config::Configuration; + using ODBC::ODBCConnection; + + using ODBC::SqlWcharToString; + + return ODBCConnection::ExecuteWithDiagnostics(conn, SQL_ERROR, [=]() { + ODBCConnection* connection = reinterpret_cast(conn); + std::string dsn = SqlWcharToString(dsn_name, dsn_name_len); + + Configuration config; + config.LoadDsn(dsn); + + if (user_name) { + std::string uid = SqlWcharToString(user_name, user_name_len); + config.Emplace(FlightSqlConnection::UID, std::move(uid)); + } + + if (password) { + std::string pwd = SqlWcharToString(password, password_len); + config.Emplace(FlightSqlConnection::PWD, std::move(pwd)); + } + + std::vector missing_properties; + + connection->Connect(dsn, config.GetProperties(), missing_properties); + + return SQL_SUCCESS; + }); } SQLRETURN SQLDisconnect(SQLHDBC conn) { ARROW_LOG(DEBUG) << "SQLDisconnect called with conn: " << conn; - // GH-46574 TODO: Implement SQLDisconnect - return SQL_INVALID_HANDLE; + + using ODBC::ODBCConnection; + + return ODBCConnection::ExecuteWithDiagnostics(conn, SQL_ERROR, [=]() { + ODBCConnection* connection = reinterpret_cast(conn); + + connection->Disconnect(); + + return SQL_SUCCESS; + }); } SQLRETURN SQLGetInfo(SQLHDBC conn, SQLUSMALLINT info_type, SQLPOINTER info_value_ptr, @@ -776,8 +892,24 @@ SQLRETURN SQLGetInfo(SQLHDBC conn, SQLUSMALLINT info_type, SQLPOINTER info_value << ", info_value_ptr: " << info_value_ptr << ", buf_len: " << buf_len << ", string_length_ptr: " << static_cast(string_length_ptr); - // GH-47709 TODO: Implement SQLGetInfo - return SQL_INVALID_HANDLE; + + // GH-47709 TODO: Update SQLGetInfo implementation and add tests for SQLGetInfo + using ODBC::ODBCConnection; + + return ODBCConnection::ExecuteWithDiagnostics(conn, SQL_ERROR, [=]() { + ODBCConnection* connection = reinterpret_cast(conn); + + // Set character type to be Unicode by default + const bool is_unicode = true; + + if (!info_value_ptr && !string_length_ptr) { + return static_cast(SQL_ERROR); + } + + connection->GetInfo(info_type, info_value_ptr, buf_len, string_length_ptr, + is_unicode); + return static_cast(SQL_SUCCESS); + }); } SQLRETURN SQLGetStmtAttr(SQLHSTMT stmt, SQLINTEGER attribute, SQLPOINTER value_ptr, diff --git a/cpp/src/arrow/flight/sql/odbc/odbc_impl/CMakeLists.txt b/cpp/src/arrow/flight/sql/odbc/odbc_impl/CMakeLists.txt index b232577ee37..8f09fccd71d 100644 --- a/cpp/src/arrow/flight/sql/odbc/odbc_impl/CMakeLists.txt +++ b/cpp/src/arrow/flight/sql/odbc/odbc_impl/CMakeLists.txt @@ -124,7 +124,9 @@ if(WIN32) ui/dsn_configuration_window.h ui/window.cc ui/window.h - system_dsn.cc) + win_system_dsn.cc + system_dsn.cc + system_dsn.h) endif() target_link_libraries(arrow_odbc_spi_impl PUBLIC arrow_flight_sql_shared diff --git a/cpp/src/arrow/flight/sql/odbc/odbc_impl/attribute_utils.h b/cpp/src/arrow/flight/sql/odbc/odbc_impl/attribute_utils.h index 7baea759ede..fcf5d5a81d9 100644 --- a/cpp/src/arrow/flight/sql/odbc/odbc_impl/attribute_utils.h +++ b/cpp/src/arrow/flight/sql/odbc/odbc_impl/attribute_utils.h @@ -17,16 +17,15 @@ #pragma once -#include -#include -#include #include #include #include #include #include - -#include +#include "arrow/flight/sql/odbc/odbc_impl/diagnostics.h" +#include "arrow/flight/sql/odbc/odbc_impl/encoding_utils.h" +#include "arrow/flight/sql/odbc/odbc_impl/exceptions.h" +#include "arrow/flight/sql/odbc/odbc_impl/platform.h" namespace ODBC { @@ -48,12 +47,12 @@ inline void GetAttribute(T attribute_value, SQLPOINTER output, O output_size, } template -inline SQLRETURN GetAttributeUTF8(const std::string& attribute_value, SQLPOINTER output, - O output_size, O* output_len_ptr) { +inline SQLRETURN GetAttributeUTF8(const std::string_view& attribute_value, + SQLPOINTER output, O output_size, O* output_len_ptr) { if (output) { size_t output_len_before_null = std::min(static_cast(attribute_value.size()), static_cast(output_size - 1)); - memcpy(output, attribute_value.c_str(), output_len_before_null); + std::memcpy(output, attribute_value.data(), output_len_before_null); reinterpret_cast(output)[output_len_before_null] = '\0'; } @@ -68,8 +67,8 @@ inline SQLRETURN GetAttributeUTF8(const std::string& attribute_value, SQLPOINTER } template -inline SQLRETURN GetAttributeUTF8(const std::string& attribute_value, SQLPOINTER output, - O output_size, O* output_len_ptr, +inline SQLRETURN GetAttributeUTF8(const std::string_view& attribute_value, + SQLPOINTER output, O output_size, O* output_len_ptr, Diagnostics& diagnostics) { SQLRETURN result = GetAttributeUTF8(attribute_value, output, output_size, output_len_ptr); @@ -80,7 +79,7 @@ inline SQLRETURN GetAttributeUTF8(const std::string& attribute_value, SQLPOINTER } template -inline SQLRETURN GetAttributeSQLWCHAR(const std::string& attribute_value, +inline SQLRETURN GetAttributeSQLWCHAR(const std::string_view& attribute_value, bool is_length_in_bytes, SQLPOINTER output, O output_size, O* output_len_ptr) { size_t length = ConvertToSqlWChar( @@ -104,7 +103,7 @@ inline SQLRETURN GetAttributeSQLWCHAR(const std::string& attribute_value, } template -inline SQLRETURN GetAttributeSQLWCHAR(const std::string& attribute_value, +inline SQLRETURN GetAttributeSQLWCHAR(const std::string_view& attribute_value, bool is_length_in_bytes, SQLPOINTER output, O output_size, O* output_len_ptr, Diagnostics& diagnostics) { @@ -117,7 +116,8 @@ inline SQLRETURN GetAttributeSQLWCHAR(const std::string& attribute_value, } template -inline SQLRETURN GetStringAttribute(bool is_unicode, const std::string& attribute_value, +inline SQLRETURN GetStringAttribute(bool is_unicode, + const std::string_view& attribute_value, bool is_length_in_bytes, SQLPOINTER output, O output_size, O* output_len_ptr, Diagnostics& diagnostics) { diff --git a/cpp/src/arrow/flight/sql/odbc/odbc_impl/config/configuration.cc b/cpp/src/arrow/flight/sql/odbc/odbc_impl/config/configuration.cc index cdb889f0567..c72fcbfbbf1 100644 --- a/cpp/src/arrow/flight/sql/odbc/odbc_impl/config/configuration.cc +++ b/cpp/src/arrow/flight/sql/odbc/odbc_impl/config/configuration.cc @@ -151,11 +151,11 @@ void Configuration::LoadDsn(const std::string& dsn) { void Configuration::Clear() { this->properties_.clear(); } bool Configuration::IsSet(const std::string_view& key) const { - return 0 != this->properties_.count(key); + return 0 != this->properties_.count(std::string(key)); } const std::string& Configuration::Get(const std::string_view& key) const { - const auto itr = this->properties_.find(key); + const auto itr = this->properties_.find(std::string(key)); if (itr == this->properties_.cend()) { static const std::string empty(""); return empty; @@ -171,7 +171,15 @@ void Configuration::Set(const std::string_view& key, const std::wstring& wvalue) void Configuration::Set(const std::string_view& key, const std::string& value) { const std::string copy = boost::trim_copy(value); if (!copy.empty()) { - this->properties_[key] = value; + this->properties_[std::string(key)] = value; + } +} + +void Configuration::Emplace(const std::string_view& key, std::string&& value) { + const std::string copy = boost::trim_copy(value); + if (!copy.empty()) { + this->properties_.emplace( + std::make_pair(std::move(std::string(key)), std::move(value))); } } @@ -182,7 +190,7 @@ const Connection::ConnPropertyMap& Configuration::GetProperties() const { std::vector Configuration::GetCustomKeys() const { Connection::ConnPropertyMap copy_props(properties_); for (auto& key : FlightSqlConnection::ALL_KEYS) { - copy_props.erase(key); + copy_props.erase(std::string(key)); } std::vector keys; boost::copy(copy_props | boost::adaptors::map_keys, std::back_inserter(keys)); diff --git a/cpp/src/arrow/flight/sql/odbc/odbc_impl/config/configuration.h b/cpp/src/arrow/flight/sql/odbc/odbc_impl/config/configuration.h index 77d07b1420a..56d8bac6173 100644 --- a/cpp/src/arrow/flight/sql/odbc/odbc_impl/config/configuration.h +++ b/cpp/src/arrow/flight/sql/odbc/odbc_impl/config/configuration.h @@ -46,13 +46,6 @@ class Configuration { */ ~Configuration(); - /** - * Convert configure to connect string. - * - * @return Connect string. - */ - std::string ToConnectString() const; - void LoadDefaults(); void LoadDsn(const std::string& dsn); @@ -61,7 +54,7 @@ class Configuration { const std::string& Get(const std::string_view& key) const; void Set(const std::string_view& key, const std::wstring& wvalue); void Set(const std::string_view& key, const std::string& value); - + void Emplace(const std::string_view& key, std::string&& value); /** * Get properties map. */ diff --git a/cpp/src/arrow/flight/sql/odbc/odbc_impl/encoding_utils.h b/cpp/src/arrow/flight/sql/odbc/odbc_impl/encoding_utils.h index a5cc3a6f4c8..5c65eedd6fa 100644 --- a/cpp/src/arrow/flight/sql/odbc/odbc_impl/encoding_utils.h +++ b/cpp/src/arrow/flight/sql/odbc/odbc_impl/encoding_utils.h @@ -16,7 +16,6 @@ // under the License. #pragma once - #include "arrow/flight/sql/odbc/odbc_impl/encoding.h" #include "arrow/flight/sql/odbc/odbc_impl/platform.h" @@ -40,15 +39,15 @@ using arrow::flight::sql::odbc::WcsToUtf8; // Return the number of bytes required for the conversion. template -inline size_t ConvertToSqlWChar(const std::string& str, SQLWCHAR* buffer, +inline size_t ConvertToSqlWChar(const std::string_view& str, SQLWCHAR* buffer, SQLLEN buffer_size_in_bytes) { thread_local std::vector wstr; Utf8ToWcs(str.data(), str.size(), &wstr); SQLLEN value_length_in_bytes = wstr.size(); if (buffer) { - memcpy(buffer, wstr.data(), - std::min(static_cast(wstr.size()), buffer_size_in_bytes)); + std::memcpy(buffer, wstr.data(), + std::min(static_cast(wstr.size()), buffer_size_in_bytes)); // Write a NUL terminator if (buffer_size_in_bytes >= @@ -67,7 +66,7 @@ inline size_t ConvertToSqlWChar(const std::string& str, SQLWCHAR* buffer, return value_length_in_bytes; } -inline size_t ConvertToSqlWChar(const std::string& str, SQLWCHAR* buffer, +inline size_t ConvertToSqlWChar(const std::string_view& str, SQLWCHAR* buffer, SQLLEN buffer_size_in_bytes) { switch (GetSqlWCharSize()) { case sizeof(char16_t): @@ -86,7 +85,7 @@ inline size_t ConvertToSqlWChar(const std::string& str, SQLWCHAR* buffer, /// \param[in] msg_len Number of characters in wchar_msg /// \return wchar_msg in std::string format inline std::string SqlWcharToString(SQLWCHAR* wchar_msg, SQLINTEGER msg_len = SQL_NTS) { - if (msg_len == 0 || !wchar_msg || wchar_msg[0] == 0) { + if (!wchar_msg || wchar_msg[0] == 0 || msg_len == 0) { return std::string(); } @@ -101,4 +100,19 @@ inline std::string SqlWcharToString(SQLWCHAR* wchar_msg, SQLINTEGER msg_len = SQ return std::string(utf8_str.begin(), utf8_str.end()); } +inline std::string SqlStringToString(const unsigned char* sql_str, + int32_t sql_str_len = SQL_NTS) { + std::string res; + + const char* sql_str_c = reinterpret_cast(sql_str); + + if (!sql_str) return res; + + if (sql_str_len == SQL_NTS) + res.assign(sql_str_c); + else if (sql_str_len > 0) + res.assign(sql_str_c, sql_str_len); + + return res; +} } // namespace ODBC diff --git a/cpp/src/arrow/flight/sql/odbc/odbc_impl/flight_sql_auth_method.cc b/cpp/src/arrow/flight/sql/odbc/odbc_impl/flight_sql_auth_method.cc index bdf7f71589c..5bfa22dcb98 100644 --- a/cpp/src/arrow/flight/sql/odbc/odbc_impl/flight_sql_auth_method.cc +++ b/cpp/src/arrow/flight/sql/odbc/odbc_impl/flight_sql_auth_method.cc @@ -142,22 +142,22 @@ std::unique_ptr FlightSqlAuthMethod::FromProperties( const std::unique_ptr& client, const Connection::ConnPropertyMap& properties) { // Check if should use user-password authentication - auto it_user = properties.find(FlightSqlConnection::USER); + auto it_user = properties.find(std::string(FlightSqlConnection::USER)); if (it_user == properties.end()) { // The Microsoft OLE DB to ODBC bridge provider (MSDASQL) will write // "User ID" and "Password" properties instead of mapping // to ODBC compliant UID/PWD keys. - it_user = properties.find(FlightSqlConnection::USER_ID); + it_user = properties.find(std::string(FlightSqlConnection::USER_ID)); } - auto it_password = properties.find(FlightSqlConnection::PASSWORD); - auto it_token = properties.find(FlightSqlConnection::TOKEN); + auto it_password = properties.find(std::string(FlightSqlConnection::PASSWORD)); + auto it_token = properties.find(std::string(FlightSqlConnection::TOKEN)); if (it_user == properties.end() || it_password == properties.end()) { // Accept UID/PWD as aliases for User/Password. These are suggested as // standard properties in the documentation for SQLDriverConnect. - it_user = properties.find(FlightSqlConnection::UID); - it_password = properties.find(FlightSqlConnection::PWD); + it_user = properties.find(std::string(FlightSqlConnection::UID)); + it_password = properties.find(std::string(FlightSqlConnection::PWD)); } if (it_user != properties.end() || it_password != properties.end()) { const std::string& user = it_user != properties.end() ? it_user->second : ""; diff --git a/cpp/src/arrow/flight/sql/odbc/odbc_impl/flight_sql_connection.cc b/cpp/src/arrow/flight/sql/odbc/odbc_impl/flight_sql_connection.cc index 479a72f3fea..7615e10623e 100644 --- a/cpp/src/arrow/flight/sql/odbc/odbc_impl/flight_sql_connection.cc +++ b/cpp/src/arrow/flight/sql/odbc/odbc_impl/flight_sql_connection.cc @@ -99,7 +99,7 @@ inline std::string GetCerts() { return ""; } #endif -const std::set BUILT_IN_PROPERTIES = { +const std::set BUILT_IN_PROPERTIES = { FlightSqlConnection::HOST, FlightSqlConnection::PORT, FlightSqlConnection::USER, @@ -118,7 +118,7 @@ const std::set BUILT_IN_PROPERTIES Connection::ConnPropertyMap::const_iterator TrackMissingRequiredProperty( const std::string_view& property, const Connection::ConnPropertyMap& properties, std::vector& missing_attr) { - auto prop_iter = properties.find(property); + auto prop_iter = properties.find(std::string(property)); if (properties.end() == prop_iter) { missing_attr.push_back(property); } @@ -138,7 +138,7 @@ std::shared_ptr LoadFlightSslConfigs( .value_or(SYSTEM_TRUST_STORE_DEFAULT); auto trusted_certs_iterator = - conn_property_map.find(FlightSqlConnection::TRUSTED_CERTS); + conn_property_map.find(std::string(FlightSqlConnection::TRUSTED_CERTS)); auto trusted_certs = trusted_certs_iterator != conn_property_map.end() ? trusted_certs_iterator->second : ""; diff --git a/cpp/src/arrow/flight/sql/odbc/odbc_impl/flight_sql_connection.h b/cpp/src/arrow/flight/sql/odbc/odbc_impl/flight_sql_connection.h index 6219bb287e4..6a35e59a6df 100644 --- a/cpp/src/arrow/flight/sql/odbc/odbc_impl/flight_sql_connection.h +++ b/cpp/src/arrow/flight/sql/odbc/odbc_impl/flight_sql_connection.h @@ -28,6 +28,13 @@ namespace arrow::flight::sql::odbc { +/// \brief Case insensitive comparator that takes string_view +struct CaseInsensitiveComparatorStrView { + bool operator()(const std::string_view& s1, const std::string_view& s2) const { + return boost::lexicographical_compare(s1, s2, boost::is_iless()); + } +}; + class FlightSqlSslConfig; /// \brief Create an instance of the FlightSqlSslConfig class, from the properties passed diff --git a/cpp/src/arrow/flight/sql/odbc/odbc_impl/flight_sql_connection_test.cc b/cpp/src/arrow/flight/sql/odbc/odbc_impl/flight_sql_connection_test.cc index a42d0198527..d14486d7508 100644 --- a/cpp/src/arrow/flight/sql/odbc/odbc_impl/flight_sql_connection_test.cc +++ b/cpp/src/arrow/flight/sql/odbc/odbc_impl/flight_sql_connection_test.cc @@ -33,7 +33,7 @@ TEST(AttributeTests, SetAndGetAttribute) { EXPECT_TRUE(first_value); - EXPECT_EQ(boost::get(*first_value), static_cast(200)); + EXPECT_EQ(static_cast(200), boost::get(*first_value)); connection.SetAttribute(Connection::CONNECTION_TIMEOUT, static_cast(300)); @@ -41,7 +41,7 @@ TEST(AttributeTests, SetAndGetAttribute) { connection.GetAttribute(Connection::CONNECTION_TIMEOUT); EXPECT_TRUE(change_value); - EXPECT_EQ(boost::get(*change_value), static_cast(300)); + EXPECT_EQ(static_cast(300), boost::get(*change_value)); connection.Close(); } @@ -65,10 +65,12 @@ TEST(MetadataSettingsTest, StringColumnLengthTest) { const int32_t expected_string_column_length = 100000; const Connection::ConnPropertyMap properties = { - {FlightSqlConnection::HOST, std::string("localhost")}, // expect not used - {FlightSqlConnection::PORT, std::string("32010")}, // expect not used - {FlightSqlConnection::USE_ENCRYPTION, std::string("false")}, // expect not used - {FlightSqlConnection::STRING_COLUMN_LENGTH, + {std::string(FlightSqlConnection::HOST), + std::string("localhost")}, // expect not used + {std::string(FlightSqlConnection::PORT), std::string("32010")}, // expect not used + {std::string(FlightSqlConnection::USE_ENCRYPTION), + std::string("false")}, // expect not used + {std::string(FlightSqlConnection::STRING_COLUMN_LENGTH), std::to_string(expected_string_column_length)}, }; @@ -86,10 +88,10 @@ TEST(MetadataSettingsTest, UseWideCharTest) { connection.SetClosed(false); const Connection::ConnPropertyMap properties1 = { - {FlightSqlConnection::USE_WIDE_CHAR, std::string("true")}, + {std::string(FlightSqlConnection::USE_WIDE_CHAR), std::string("true")}, }; const Connection::ConnPropertyMap properties2 = { - {FlightSqlConnection::USE_WIDE_CHAR, std::string("false")}, + {std::string(FlightSqlConnection::USE_WIDE_CHAR), std::string("false")}, }; EXPECT_EQ(true, connection.GetUseWideChar(properties1)); @@ -101,9 +103,9 @@ TEST(MetadataSettingsTest, UseWideCharTest) { TEST(BuildLocationTests, ForTcp) { std::vector missing_attr; Connection::ConnPropertyMap properties = { - {FlightSqlConnection::HOST, std::string("localhost")}, - {FlightSqlConnection::PORT, std::string("32010")}, - {FlightSqlConnection::USE_ENCRYPTION, std::string("false")}, + {std::string(FlightSqlConnection::HOST), std::string("localhost")}, + {std::string(FlightSqlConnection::PORT), std::string("32010")}, + {std::string(FlightSqlConnection::USE_ENCRYPTION), std::string("false")}, }; const std::shared_ptr& ssl_config = @@ -113,8 +115,8 @@ TEST(BuildLocationTests, ForTcp) { FlightSqlConnection::BuildLocation(properties, missing_attr, ssl_config); const Location& actual_location2 = FlightSqlConnection::BuildLocation( { - {FlightSqlConnection::HOST, std::string("localhost")}, - {FlightSqlConnection::PORT, std::string("32011")}, + {std::string(FlightSqlConnection::HOST), std::string("localhost")}, + {std::string(FlightSqlConnection::PORT), std::string("32011")}, }, missing_attr, ssl_config); @@ -127,9 +129,9 @@ TEST(BuildLocationTests, ForTcp) { TEST(BuildLocationTests, ForTls) { std::vector missing_attr; Connection::ConnPropertyMap properties = { - {FlightSqlConnection::HOST, std::string("localhost")}, - {FlightSqlConnection::PORT, std::string("32010")}, - {FlightSqlConnection::USE_ENCRYPTION, std::string("1")}, + {std::string(FlightSqlConnection::HOST), std::string("localhost")}, + {std::string(FlightSqlConnection::PORT), std::string("32010")}, + {std::string(FlightSqlConnection::USE_ENCRYPTION), std::string("1")}, }; const std::shared_ptr& ssl_config = @@ -139,9 +141,9 @@ TEST(BuildLocationTests, ForTls) { FlightSqlConnection::BuildLocation(properties, missing_attr, ssl_config); Connection::ConnPropertyMap second_properties = { - {FlightSqlConnection::HOST, std::string("localhost")}, - {FlightSqlConnection::PORT, std::string("32011")}, - {FlightSqlConnection::USE_ENCRYPTION, std::string("1")}, + {std::string(FlightSqlConnection::HOST), std::string("localhost")}, + {std::string(FlightSqlConnection::PORT), std::string("32011")}, + {std::string(FlightSqlConnection::USE_ENCRYPTION), std::string("1")}, }; const std::shared_ptr& second_ssl_config = diff --git a/cpp/src/arrow/flight/sql/odbc/odbc_impl/main.cc b/cpp/src/arrow/flight/sql/odbc/odbc_impl/main.cc index 8f649311e9d..4d9277a0cc9 100644 --- a/cpp/src/arrow/flight/sql/odbc/odbc_impl/main.cc +++ b/cpp/src/arrow/flight/sql/odbc/odbc_impl/main.cc @@ -43,7 +43,7 @@ using arrow::flight::sql::odbc::Statement; void TestBindColumn(const std::shared_ptr& connection) { const std::shared_ptr& statement = connection->CreateStatement(); - statement->Execute("SELECT IncidntNum, Category FROM \"@dremio\".Test LIMIT 10"); + statement->Execute("SELECT IncidntNum, Category FROM \"@apache\".Test LIMIT 10"); const std::shared_ptr& result_set = statement->GetResultSet(); @@ -105,7 +105,7 @@ void TestBindColumnBigInt(const std::shared_ptr& connection) { " SELECT CONVERT_TO_INTEGER(IncidntNum, 1, 1, 0) AS IncidntNum, " "Category\n" " FROM (\n" - " SELECT IncidntNum, Category FROM \"@dremio\".Test LIMIT 10\n" + " SELECT IncidntNum, Category FROM \"@apache\".Test LIMIT 10\n" " ) nested_0\n" ") nested_0"); @@ -202,11 +202,11 @@ int main() { driver.CreateConnection(arrow::flight::sql::odbc::OdbcVersion::V_3); Connection::ConnPropertyMap properties = { - {FlightSqlConnection::HOST, std::string("automaster.drem.io")}, - {FlightSqlConnection::PORT, std::string("32010")}, - {FlightSqlConnection::USER, std::string("dremio")}, - {FlightSqlConnection::PASSWORD, std::string("dremio123")}, - {FlightSqlConnection::USE_ENCRYPTION, std::string("false")}, + {std::string(FlightSqlConnection::HOST), std::string("automaster.apache")}, + {std::string(FlightSqlConnection::PORT), std::string("32010")}, + {std::string(FlightSqlConnection::USER), std::string("apache")}, + {std::string(FlightSqlConnection::PASSWORD), std::string("apache123")}, + {std::string(FlightSqlConnection::USE_ENCRYPTION), std::string("false")}, }; std::vector missing_attr; connection->Connect(properties, missing_attr); diff --git a/cpp/src/arrow/flight/sql/odbc/odbc_impl/odbc_connection.cc b/cpp/src/arrow/flight/sql/odbc/odbc_impl/odbc_connection.cc index c0a55840d56..cc183e5e6b5 100644 --- a/cpp/src/arrow/flight/sql/odbc/odbc_impl/odbc_connection.cc +++ b/cpp/src/arrow/flight/sql/odbc/odbc_impl/odbc_connection.cc @@ -53,57 +53,7 @@ namespace { // characters such as semi-colons and equals signs. NOTE: This can be optimized to be // built statically. const boost::xpressive::sregex CONNECTION_STR_REGEX( - boost::xpressive::sregex::compile("([^=;]+)=({.+}|[^=;]+|[^;])")); - -// Load properties from the given DSN. The properties loaded do _not_ overwrite existing -// entries in the properties. -void loadPropertiesFromDSN(const std::string& dsn, - Connection::ConnPropertyMap& properties) { - const size_t BUFFER_SIZE = 1024 * 10; - std::vector output_buffer; - output_buffer.resize(BUFFER_SIZE, '\0'); - SQLSetConfigMode(ODBC_BOTH_DSN); - - CONVERT_WIDE_STR(const std::wstring wdsn, dsn); - - SQLGetPrivateProfileString(wdsn.c_str(), NULL, L"", &output_buffer[0], BUFFER_SIZE, - L"odbc.ini"); - - // The output buffer holds the list of keys in a series of NUL-terminated strings. - // The series is terminated with an empty string (eg a NUL-terminator terminating the - // last key followed by a NUL terminator after). - std::vector keys; - size_t pos = 0; - while (pos < BUFFER_SIZE) { - std::wstring wkey(&output_buffer[pos]); - if (wkey.empty()) { - break; - } - size_t len = wkey.size(); - - // Skip over Driver or DSN keys. - if (!boost::iequals(wkey, L"DSN") && !boost::iequals(wkey, L"Driver")) { - keys.emplace_back(std::move(wkey)); - } - pos += len + 1; - } - - for (auto& wkey : keys) { - output_buffer.clear(); - output_buffer.resize(BUFFER_SIZE, '\0'); - SQLGetPrivateProfileString(wdsn.c_str(), wkey.data(), L"", &output_buffer[0], - BUFFER_SIZE, L"odbc.ini"); - - std::wstring wvalue = std::wstring(&output_buffer[0]); - CONVERT_UTF8_STR(const std::string value, wvalue); - CONVERT_UTF8_STR(const std::string key, std::wstring(wkey)); - auto propIter = properties.find(key); - if (propIter == properties.end()) { - properties.emplace(std::make_pair(std::move(key), std::move(value))); - } - } -} - + boost::xpressive::sregex::compile("([^=;]+)=({.+}|[^;]+|[^;])")); } // namespace // Public @@ -734,39 +684,43 @@ void ODBCConnection::DropDescriptor(ODBCDescriptor* desc) { // Public Static // =================================================================================== -std::string ODBCConnection::GetPropertiesFromConnString( +std::string ODBCConnection::GetDsnIfExists(const std::string& conn_str) { + const int groups[] = {1, 2}; // CONNECTION_STR_REGEX has two groups. key: 1, value: 2 + boost::xpressive::sregex_token_iterator regex_iter(conn_str.begin(), conn_str.end(), + CONNECTION_STR_REGEX, groups), + end; + + // First key in connection string should be either dsn or driver + auto it = regex_iter; + std::string key = *regex_iter; + std::string value = *++regex_iter; + + // Strip wrapping curly braces. + if (value.size() >= 2 && value[0] == '{' && value[value.size() - 1] == '}') { + value = value.substr(1, value.size() - 2); + } + + if (boost::iequals(key, "DSN")) { + return value; + } else if (boost::iequals(key, "Driver")) { + return std::string(""); + } else { + throw DriverException( + "Connection string is faulty. The first key should be DSN or Driver.", "HY000"); + } +} + +void ODBCConnection::GetPropertiesFromConnString( const std::string& conn_str, Connection::ConnPropertyMap& properties) { const int groups[] = {1, 2}; // CONNECTION_STR_REGEX has two groups. key: 1, value: 2 boost::xpressive::sregex_token_iterator regex_iter(conn_str.begin(), conn_str.end(), CONNECTION_STR_REGEX, groups), end; - bool is_dsn_first = false; - bool is_driver_first = false; - std::string dsn; for (auto it = regex_iter; end != regex_iter; ++regex_iter) { std::string key = *regex_iter; std::string value = *++regex_iter; - // If the DSN shows up before driver key, load settings from the DSN. - // Only load values from the DSN once regardless of how many times the DSN - // key shows up. - if (boost::iequals(key, "DSN")) { - if (!is_driver_first) { - if (!is_dsn_first) { - is_dsn_first = true; - loadPropertiesFromDSN(value, properties); - dsn.swap(value); - } - } - continue; - } else if (boost::iequals(key, "Driver")) { - if (!is_dsn_first) { - is_driver_first = true; - } - continue; - } - // Strip wrapping curly braces. if (value.size() >= 2 && value[0] == '{' && value[value.size() - 1] == '}') { value = value.substr(1, value.size() - 2); @@ -776,5 +730,4 @@ std::string ODBCConnection::GetPropertiesFromConnString( // including over entries in the DSN. properties[key] = std::move(value); } - return dsn; } diff --git a/cpp/src/arrow/flight/sql/odbc/odbc_impl/odbc_connection.h b/cpp/src/arrow/flight/sql/odbc/odbc_impl/odbc_connection.h index 8157c2f5f94..6f793cd0111 100644 --- a/cpp/src/arrow/flight/sql/odbc/odbc_impl/odbc_connection.h +++ b/cpp/src/arrow/flight/sql/odbc/odbc_impl/odbc_connection.h @@ -75,8 +75,12 @@ class ODBCConnection : public ODBCHandle { inline bool IsOdbc2Connection() const { return is_2x_connection_; } - /// @return the DSN or empty string if Driver was used. - static std::string GetPropertiesFromConnString( + /// @return the DSN or an empty string if the DSN is not found or is found after the + /// driver + static std::string GetDsnIfExists(const std::string& conn_str); + + /// Read properties from connection string, but does not read values from DSN + static void GetPropertiesFromConnString( const std::string& conn_str, arrow::flight::sql::odbc::Connection::ConnPropertyMap& properties); diff --git a/cpp/src/arrow/flight/sql/odbc/odbc_impl/spi/connection.h b/cpp/src/arrow/flight/sql/odbc/odbc_impl/spi/connection.h index e24af6c3dd7..ec0e3e727ee 100644 --- a/cpp/src/arrow/flight/sql/odbc/odbc_impl/spi/connection.h +++ b/cpp/src/arrow/flight/sql/odbc/odbc_impl/spi/connection.h @@ -32,13 +32,13 @@ namespace arrow::flight::sql::odbc { /// \brief Case insensitive comparator struct CaseInsensitiveComparator { - bool operator()(const std::string_view& s1, const std::string_view& s2) const { + bool operator()(const std::string& s1, const std::string& s2) const { return boost::lexicographical_compare(s1, s2, boost::is_iless()); } }; // PropertyMap is case-insensitive for keys. -typedef std::map PropertyMap; +typedef std::map PropertyMap; class Statement; diff --git a/cpp/src/arrow/flight/sql/odbc/odbc_impl/system_dsn.cc b/cpp/src/arrow/flight/sql/odbc/odbc_impl/system_dsn.cc index 75501ac8dd4..fd77fbf50f7 100644 --- a/cpp/src/arrow/flight/sql/odbc/odbc_impl/system_dsn.cc +++ b/cpp/src/arrow/flight/sql/odbc/odbc_impl/system_dsn.cc @@ -17,18 +17,11 @@ #include "arrow/flight/sql/odbc/odbc_impl/system_dsn.h" -// platform.h includes windows.h, so it needs to be included -// before winuser.h -#include "arrow/flight/sql/odbc/odbc_impl/platform.h" - -#include -#include #include "arrow/flight/sql/odbc/odbc_impl/config/configuration.h" -#include "arrow/flight/sql/odbc/odbc_impl/config/connection_string_parser.h" -#include "arrow/flight/sql/odbc/odbc_impl/exceptions.h" #include "arrow/flight/sql/odbc/odbc_impl/flight_sql_connection.h" #include "arrow/flight/sql/odbc/odbc_impl/ui/dsn_configuration_window.h" #include "arrow/flight/sql/odbc/odbc_impl/ui/window.h" +#include "arrow/flight/sql/odbc/odbc_impl/util.h" #include "arrow/result.h" #include "arrow/util/utf8.h" @@ -38,41 +31,6 @@ namespace arrow::flight::sql::odbc { using config::Configuration; -using config::ConnectionStringParser; -using config::DsnConfigurationWindow; -using config::Result; -using config::Window; - -bool DisplayConnectionWindow(void* window_parent, Configuration& config) { - HWND hwnd_parent = (HWND)window_parent; - - if (!hwnd_parent) return true; - - try { - Window parent(hwnd_parent); - DsnConfigurationWindow window(&parent, config); - - window.Create(); - - window.Show(); - window.Update(); - - return ProcessMessages(window) == Result::OK; - } catch (const DriverException& err) { - std::stringstream buf; - buf << "SQL State: " << err.GetSqlState() << ", Message: " << err.GetMessageText() - << ", Code: " << err.GetNativeError(); - std::wstring wmessage = - arrow::util::UTF8ToWideString(buf.str()).ValueOr(L"Error during load DSN"); - MessageBox(NULL, wmessage.c_str(), L"Error!", MB_ICONEXCLAMATION | MB_OK); - - std::wstring wmessage_text = arrow::util::UTF8ToWideString(err.GetMessageText()) - .ValueOr(L"Error during load DSN"); - SQLPostInstallerError(err.GetNativeError(), wmessage_text.c_str()); - } - - return false; -} void PostError(DWORD error_code, LPCWSTR error_msg) { MessageBox(NULL, error_msg, L"Error!", MB_ICONEXCLAMATION | MB_OK); @@ -167,77 +125,4 @@ bool RegisterDsn(const Configuration& config, LPCWSTR driver) { return true; } - -BOOL INSTAPI ConfigDSNW(HWND hwnd_parent, WORD req, LPCWSTR wdriver, - LPCWSTR wattributes) { - Configuration config; - ConnectionStringParser parser(config); - - auto attributes_result = arrow::util::WideStringToUTF8(std::wstring(wattributes)); - if (!attributes_result.status().ok()) { - PostArrowUtilError(attributes_result.status()); - return FALSE; - } - std::string attributes = attributes_result.ValueOrDie(); - - parser.ParseConfigAttributes(attributes.c_str()); - - switch (req) { - case ODBC_ADD_DSN: { - config.LoadDefaults(); - if (!DisplayConnectionWindow(hwnd_parent, config) || !RegisterDsn(config, wdriver)) - return FALSE; - - break; - } - - case ODBC_CONFIG_DSN: { - const std::string& dsn = config.Get(FlightSqlConnection::DSN); - auto wdsn_result = arrow::util::UTF8ToWideString(dsn); - if (!wdsn_result.status().ok()) { - PostArrowUtilError(wdsn_result.status()); - return FALSE; - } - std::wstring wdsn = wdsn_result.ValueOrDie(); - if (!SQLValidDSN(wdsn.c_str())) return FALSE; - - Configuration loaded(config); - try { - loaded.LoadDsn(dsn); - } catch (const DriverException& err) { - std::string error_msg = err.GetMessageText(); - std::wstring werror_msg = - arrow::util::UTF8ToWideString(error_msg).ValueOr(L"Error during DSN load"); - - PostError(err.GetNativeError(), werror_msg.c_str()); - return FALSE; - } - - if (!DisplayConnectionWindow(hwnd_parent, loaded) || !UnregisterDsn(wdsn.c_str()) || - !RegisterDsn(loaded, wdriver)) - return FALSE; - - break; - } - - case ODBC_REMOVE_DSN: { - const std::string& dsn = config.Get(FlightSqlConnection::DSN); - auto wdsn_result = arrow::util::UTF8ToWideString(dsn); - if (!wdsn_result.status().ok()) { - PostArrowUtilError(wdsn_result.status()); - return FALSE; - } - std::wstring wdsn = wdsn_result.ValueOrDie(); - if (!SQLValidDSN(wdsn.c_str()) || !UnregisterDsn(wdsn)) return FALSE; - - break; - } - - default: - return FALSE; - } - - return TRUE; -} - } // namespace arrow::flight::sql::odbc diff --git a/cpp/src/arrow/flight/sql/odbc/odbc_impl/system_dsn.h b/cpp/src/arrow/flight/sql/odbc/odbc_impl/system_dsn.h index 32d17af6753..78dbd51c2e2 100644 --- a/cpp/src/arrow/flight/sql/odbc/odbc_impl/system_dsn.h +++ b/cpp/src/arrow/flight/sql/odbc/odbc_impl/system_dsn.h @@ -19,6 +19,7 @@ #include "arrow/flight/sql/odbc/odbc_impl/platform.h" #include "arrow/flight/sql/odbc/odbc_impl/config/configuration.h" +#include "arrow/result.h" namespace arrow::flight::sql::odbc { @@ -65,4 +66,7 @@ bool RegisterDsn(const Configuration& config, LPCWSTR driver); */ bool UnregisterDsn(const std::wstring& dsn); +void PostError(DWORD error_code, LPCWSTR error_msg); + +void PostArrowUtilError(arrow::Status error_status); } // namespace arrow::flight::sql::odbc diff --git a/cpp/src/arrow/flight/sql/odbc/odbc_impl/ui/dsn_configuration_window.cc b/cpp/src/arrow/flight/sql/odbc/odbc_impl/ui/dsn_configuration_window.cc index 3f49690daad..0432836a16f 100644 --- a/cpp/src/arrow/flight/sql/odbc/odbc_impl/ui/dsn_configuration_window.cc +++ b/cpp/src/arrow/flight/sql/odbc/odbc_impl/ui/dsn_configuration_window.cc @@ -18,9 +18,9 @@ #include "arrow/result.h" #include "arrow/util/utf8.h" -#include "arrow/flight/sql/odbc/odbc_impl/ui/dsn_configuration_window.h" - #include "arrow/flight/sql/odbc/odbc_impl/flight_sql_connection.h" +#include "arrow/flight/sql/odbc/odbc_impl/ui/add_property_window.h" +#include "arrow/flight/sql/odbc/odbc_impl/ui/dsn_configuration_window.h" #include "arrow/flight/sql/odbc/odbc_impl/util.h" #include @@ -30,16 +30,13 @@ #include #include -#include "arrow/flight/sql/odbc/odbc_impl/ui/add_property_window.h" - #define COMMON_TAB 0 #define ADVANCED_TAB 1 namespace arrow::flight::sql::odbc { namespace { std::string TestConnection(const config::Configuration& config) { - std::unique_ptr flight_sql_conn( - new FlightSqlConnection(OdbcVersion::V_3)); + std::unique_ptr flight_sql_conn(new FlightSqlConnection(V_3)); std::vector missing_properties; flight_sql_conn->Connect(config.GetProperties(), missing_properties); @@ -250,6 +247,7 @@ int DsnConfigurationWindow::CreateEncryptionSettingsGroup(int pos_x, int pos_y, std::string val = config_.Get(FlightSqlConnection::USE_ENCRYPTION); + // Enable encryption default value is true const bool enable_encryption = util::AsBool(val).value_or(true); labels_.push_back(CreateLabel(label_pos_x, row_pos, LABEL_WIDTH, ROW_HEIGHT, L"Use Encryption:", ChildId::ENABLE_ENCRYPTION_LABEL)); @@ -275,6 +273,7 @@ int DsnConfigurationWindow::CreateEncryptionSettingsGroup(int pos_x, int pos_y, val = config_.Get(FlightSqlConnection::USE_SYSTEM_TRUST_STORE).c_str(); + // System trust store default value is true const bool use_system_cert_store = util::AsBool(val).value_or(true); labels_.push_back(CreateLabel(label_pos_x, row_pos, LABEL_WIDTH, 2 * ROW_HEIGHT, L"Use System Certificate Store:", diff --git a/cpp/src/arrow/flight/sql/odbc/odbc_impl/win_system_dsn.cc b/cpp/src/arrow/flight/sql/odbc/odbc_impl/win_system_dsn.cc new file mode 100644 index 00000000000..2ea9a2451c2 --- /dev/null +++ b/cpp/src/arrow/flight/sql/odbc/odbc_impl/win_system_dsn.cc @@ -0,0 +1,176 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +#include "arrow/flight/sql/odbc/odbc_impl/system_dsn.h" + +// platform.h includes windows.h, so it needs to be included +// before winuser.h +#include "arrow/flight/sql/odbc/odbc_impl/platform.h" + +#include +#include + +#include "arrow/result.h" +#include "arrow/util/utf8.h" + +#include "arrow/flight/sql/odbc/odbc_impl/config/configuration.h" +#include "arrow/flight/sql/odbc/odbc_impl/config/connection_string_parser.h" +#include "arrow/flight/sql/odbc/odbc_impl/exceptions.h" +#include "arrow/flight/sql/odbc/odbc_impl/flight_sql_connection.h" +#include "arrow/flight/sql/odbc/odbc_impl/ui/dsn_configuration_window.h" +#include "arrow/flight/sql/odbc/odbc_impl/ui/window.h" +#include "arrow/util/logging.h" + +#include +#include +#include +#include + +namespace arrow::flight::sql::odbc { +using config::Configuration; +using config::ConnectionStringParser; +using config::DsnConfigurationWindow; +using config::Result; +using config::Window; +bool DisplayConnectionWindow(void* window_parent, Configuration& config) { + HWND hwnd_parent = (HWND)window_parent; + + if (!hwnd_parent) return true; + + try { + Window parent(hwnd_parent); + DsnConfigurationWindow window(&parent, config); + + window.Create(); + + window.Show(); + window.Update(); + + return ProcessMessages(window) == Result::OK; + } catch (const DriverException& err) { + std::stringstream buf; + buf << "SQL State: " << err.GetSqlState() << ", Message: " << err.GetMessageText() + << ", Code: " << err.GetNativeError(); + std::wstring wmessage = + arrow::util::UTF8ToWideString(buf.str()).ValueOr(L"Error during load DSN"); + MessageBox(NULL, wmessage.c_str(), L"Error!", MB_ICONEXCLAMATION | MB_OK); + + std::wstring wmessage_text = arrow::util::UTF8ToWideString(err.GetMessageText()) + .ValueOr(L"Error during load DSN"); + SQLPostInstallerError(err.GetNativeError(), wmessage_text.c_str()); + } + + return false; +} + +bool DisplayConnectionWindow(void* window_parent, Configuration& config, + Connection::ConnPropertyMap& properties) { + for (const auto& [key, value] : properties) { + config.Set(key, value); + } + + if (DisplayConnectionWindow(window_parent, config)) { + properties = config.GetProperties(); + return true; + } else { + ARROW_LOG(INFO) << "Dialog is cancelled by user"; + return false; + } +} +} // namespace arrow::flight::sql::odbc + +BOOL INSTAPI ConfigDSNW(HWND hwnd_parent, WORD req, LPCWSTR wdriver, + LPCWSTR wattributes) { + using arrow::flight::sql::odbc::DisplayConnectionWindow; + using arrow::flight::sql::odbc::DriverException; + using arrow::flight::sql::odbc::FlightSqlConnection; + using arrow::flight::sql::odbc::PostArrowUtilError; + using arrow::flight::sql::odbc::PostError; + using arrow::flight::sql::odbc::RegisterDsn; + using arrow::flight::sql::odbc::UnregisterDsn; + using arrow::flight::sql::odbc::config::Configuration; + using arrow::flight::sql::odbc::config::ConnectionStringParser; + + Configuration config; + ConnectionStringParser parser(config); + + auto attributes_result = arrow::util::WideStringToUTF8(std::wstring(wattributes)); + if (!attributes_result.status().ok()) { + PostArrowUtilError(attributes_result.status()); + return FALSE; + } + std::string attributes = attributes_result.ValueOrDie(); + + parser.ParseConfigAttributes(attributes.c_str()); + + switch (req) { + case ODBC_ADD_DSN: { + config.LoadDefaults(); + if (!DisplayConnectionWindow(hwnd_parent, config) || !RegisterDsn(config, wdriver)) + return FALSE; + + break; + } + + case ODBC_CONFIG_DSN: { + const std::string& dsn = config.Get(FlightSqlConnection::DSN); + auto wdsn_result = arrow::util::UTF8ToWideString(dsn); + if (!wdsn_result.status().ok()) { + PostArrowUtilError(wdsn_result.status()); + return FALSE; + } + std::wstring wdsn = wdsn_result.ValueOrDie(); + if (!SQLValidDSN(wdsn.c_str())) return FALSE; + + Configuration loaded(config); + try { + loaded.LoadDsn(dsn); + } catch (const DriverException& err) { + std::string error_msg = err.GetMessageText(); + std::wstring werror_msg = + arrow::util::UTF8ToWideString(error_msg).ValueOr(L"Error during DSN load"); + + PostError(err.GetNativeError(), werror_msg.c_str()); + return FALSE; + } + + if (!DisplayConnectionWindow(hwnd_parent, loaded) || !UnregisterDsn(wdsn.c_str()) || + !RegisterDsn(loaded, wdriver)) + return FALSE; + + break; + } + + case ODBC_REMOVE_DSN: { + const std::string& dsn = config.Get(FlightSqlConnection::DSN); + auto wdsn_result = arrow::util::UTF8ToWideString(dsn); + if (!wdsn_result.status().ok()) { + PostArrowUtilError(wdsn_result.status()); + return FALSE; + } + std::wstring wdsn = wdsn_result.ValueOrDie(); + if (!SQLValidDSN(wdsn.c_str()) || !UnregisterDsn(wdsn)) return FALSE; + + break; + } + + default: + return FALSE; + } + + return TRUE; +} diff --git a/cpp/src/arrow/flight/sql/odbc/tests/connection_test.cc b/cpp/src/arrow/flight/sql/odbc/tests/connection_test.cc index c5646b42bef..602de323a39 100644 --- a/cpp/src/arrow/flight/sql/odbc/tests/connection_test.cc +++ b/cpp/src/arrow/flight/sql/odbc/tests/connection_test.cc @@ -153,8 +153,7 @@ TEST(SQLSetEnvAttr, TestSQLSetEnvAttrODBCVersionInvalid) { ASSERT_EQ(SQL_SUCCESS, SQLFreeEnv(env)); } -// GH-46574 TODO: enable TestSQLGetEnvAttrOutputNTS which requires connection support -TYPED_TEST(ConnectionTest, DISABLED_TestSQLGetEnvAttrOutputNTS) { +TYPED_TEST(ConnectionTest, TestSQLGetEnvAttrOutputNTS) { SQLINTEGER output_nts; ASSERT_EQ(SQL_SUCCESS, From 6f296a87a77bb36d2cd863166ce1f3f29b507582 Mon Sep 17 00:00:00 2001 From: "Alina (Xi) Li" Date: Mon, 27 Oct 2025 16:27:29 -0700 Subject: [PATCH 02/11] Add connection tests and refactoring Co-authored-by: justing-bq --- .../flight/sql/odbc/tests/connection_test.cc | 330 +++++++++++++++--- .../flight/sql/odbc/tests/odbc_test_suite.cc | 72 ++-- .../flight/sql/odbc/tests/odbc_test_suite.h | 47 ++- 3 files changed, 371 insertions(+), 78 deletions(-) diff --git a/cpp/src/arrow/flight/sql/odbc/tests/connection_test.cc b/cpp/src/arrow/flight/sql/odbc/tests/connection_test.cc index 602de323a39..2e115253ad4 100644 --- a/cpp/src/arrow/flight/sql/odbc/tests/connection_test.cc +++ b/cpp/src/arrow/flight/sql/odbc/tests/connection_test.cc @@ -14,6 +14,7 @@ // KIND, either express or implied. See the License for the // specific language governing permissions and limitations // under the License. + #include "arrow/flight/sql/odbc/tests/odbc_test_suite.h" #include "arrow/flight/sql/odbc/odbc_impl/platform.h" @@ -29,34 +30,31 @@ namespace arrow::flight::sql::odbc { template class ConnectionTest : public T {}; -// GH-46574 TODO: add remote server test cases using `ConnectionRemoteTest` -class ConnectionRemoteTest : public FlightSQLODBCRemoteTestBase {}; -using TestTypes = ::testing::Types; +using TestTypes = + ::testing::Types; TYPED_TEST_SUITE(ConnectionTest, TestTypes); -TEST(SQLAllocHandle, TestSQLAllocHandleEnv) { - SQLHENV env; - - // Allocate an environment handle - ASSERT_EQ(SQL_SUCCESS, SQLAllocHandle(SQL_HANDLE_ENV, SQL_NULL_HANDLE, &env)); - - ASSERT_NE(env, nullptr); - - // Free an environment handle - ASSERT_EQ(SQL_SUCCESS, SQLFreeHandle(SQL_HANDLE_ENV, env)); -} +template +class ConnectionHandleTest : public T {}; -TEST(SQLAllocEnv, TestSQLAllocEnv) { - SQLHENV env; +class ConnectionRemoteTest : public FlightSQLOdbcHandleRemoteTestBase {}; +using TestTypesHandle = + ::testing::Types; +TYPED_TEST_SUITE(ConnectionHandleTest, TestTypesHandle); +TEST(ODBCHandles, TestSQLAllocAndFreeEnv) { // Allocate an environment handle + SQLHENV env; ASSERT_EQ(SQL_SUCCESS, SQLAllocEnv(&env)); - // Free an environment handle + // Check for valid handle + ASSERT_NE(nullptr, env); + + // Free environment handle ASSERT_EQ(SQL_SUCCESS, SQLFreeEnv(env)); } -TEST(SQLAllocHandle, TestSQLAllocHandleConnect) { +TEST(ODBCHandles, TestSQLAllocAndFreeHandleConnect) { SQLHENV env; SQLHDBC conn; @@ -66,14 +64,17 @@ TEST(SQLAllocHandle, TestSQLAllocHandleConnect) { // Allocate a connection using alloc handle ASSERT_EQ(SQL_SUCCESS, SQLAllocHandle(SQL_HANDLE_DBC, env, &conn)); - // Free a connection handle + // Check for valid handle + ASSERT_NE(nullptr, conn); + + // Free the created connection using free handle ASSERT_EQ(SQL_SUCCESS, SQLFreeHandle(SQL_HANDLE_DBC, conn)); - // Free an environment handle + // Free environment handle ASSERT_EQ(SQL_SUCCESS, SQLFreeHandle(SQL_HANDLE_ENV, env)); } -TEST(SQLAllocConnect, TestSQLAllocHandleConnect) { +TEST(ODBCHandles, TestSQLAllocAndFreeConnect) { SQLHENV env; SQLHDBC conn; @@ -83,14 +84,17 @@ TEST(SQLAllocConnect, TestSQLAllocHandleConnect) { // Allocate a connection using alloc handle ASSERT_EQ(SQL_SUCCESS, SQLAllocConnect(env, &conn)); - // Free a connection handle + // Check for valid handle + ASSERT_NE(nullptr, conn); + + // Free the created connection using free connect ASSERT_EQ(SQL_SUCCESS, SQLFreeConnect(conn)); - // Free an environment handle + // Free environment handle ASSERT_EQ(SQL_SUCCESS, SQLFreeEnv(env)); } -TEST(SQLFreeHandle, TestFreeNullHandles) { +TEST(ODBCHandles, TestFreeNullHandles) { SQLHENV env = NULL; SQLHDBC conn = NULL; SQLHSTMT stmt = NULL; @@ -108,7 +112,6 @@ TEST(SQLFreeHandle, TestFreeNullHandles) { TEST(SQLGetEnvAttr, TestSQLGetEnvAttrODBCVersion) { SQLHENV env; - SQLINTEGER version; // Allocate an environment handle @@ -118,38 +121,33 @@ TEST(SQLGetEnvAttr, TestSQLGetEnvAttrODBCVersion) { ASSERT_EQ(SQL_OV_ODBC2, version); + // Free environment handle ASSERT_EQ(SQL_SUCCESS, SQLFreeEnv(env)); } TEST(SQLSetEnvAttr, TestSQLSetEnvAttrODBCVersionValid) { - SQLHENV env; - // Allocate an environment handle + SQLHENV env; ASSERT_EQ(SQL_SUCCESS, SQLAllocEnv(&env)); - // Attempt to set to supported version + // Attempt to set to unsupported version ASSERT_EQ(SQL_SUCCESS, SQLSetEnvAttr(env, SQL_ATTR_ODBC_VERSION, reinterpret_cast(SQL_OV_ODBC2), 0)); - SQLINTEGER version; - // Check ODBC version is set - ASSERT_EQ(SQL_SUCCESS, SQLGetEnvAttr(env, SQL_ATTR_ODBC_VERSION, &version, 0, 0)); - - ASSERT_EQ(SQL_OV_ODBC2, version); - + // Free environment handle ASSERT_EQ(SQL_SUCCESS, SQLFreeEnv(env)); } TEST(SQLSetEnvAttr, TestSQLSetEnvAttrODBCVersionInvalid) { - SQLHENV env; - // Allocate an environment handle + SQLHENV env; ASSERT_EQ(SQL_SUCCESS, SQLAllocEnv(&env)); // Attempt to set to unsupported version ASSERT_EQ(SQL_ERROR, SQLSetEnvAttr(env, SQL_ATTR_ODBC_VERSION, reinterpret_cast(1), 0)); + // Free environment handle ASSERT_EQ(SQL_SUCCESS, SQLFreeEnv(env)); } @@ -164,8 +162,7 @@ TYPED_TEST(ConnectionTest, TestSQLGetEnvAttrOutputNTS) { TYPED_TEST(ConnectionTest, DISABLED_TestSQLGetEnvAttrGetLength) { // Test is disabled because call to SQLGetEnvAttr is handled by the driver manager on - // Windows. Windows driver manager ignores the length pointer. - // This test case can be potentially used on macOS/Linux + // Windows. This test case can be potentially used on macOS/Linux SQLINTEGER length; ASSERT_EQ(SQL_SUCCESS, SQLGetEnvAttr(this->env, SQL_ATTR_ODBC_VERSION, nullptr, 0, &length)); @@ -175,48 +172,289 @@ TYPED_TEST(ConnectionTest, DISABLED_TestSQLGetEnvAttrGetLength) { TYPED_TEST(ConnectionTest, DISABLED_TestSQLGetEnvAttrNullValuePointer) { // Test is disabled because call to SQLGetEnvAttr is handled by the driver manager on - // Windows. The Windows driver manager doesn't error out when null pointer is passed. - // This test case can be potentially used on macOS/Linux + // Windows. This test case can be potentially used on macOS/Linux ASSERT_EQ(SQL_ERROR, SQLGetEnvAttr(this->env, SQL_ATTR_ODBC_VERSION, nullptr, 0, nullptr)); } TEST(SQLSetEnvAttr, TestSQLSetEnvAttrOutputNTSValid) { - SQLHENV env; - // Allocate an environment handle + SQLHENV env; ASSERT_EQ(SQL_SUCCESS, SQLAllocEnv(&env)); // Attempt to set to output nts to supported version ASSERT_EQ(SQL_SUCCESS, SQLSetEnvAttr(env, SQL_ATTR_OUTPUT_NTS, reinterpret_cast(SQL_TRUE), 0)); + // Free environment handle ASSERT_EQ(SQL_SUCCESS, SQLFreeEnv(env)); } TEST(SQLSetEnvAttr, TestSQLSetEnvAttrOutputNTSInvalid) { - SQLHENV env; - // Allocate an environment handle + SQLHENV env; ASSERT_EQ(SQL_SUCCESS, SQLAllocEnv(&env)); // Attempt to set to output nts to unsupported false ASSERT_EQ(SQL_ERROR, SQLSetEnvAttr(env, SQL_ATTR_OUTPUT_NTS, reinterpret_cast(SQL_FALSE), 0)); + // Free environment handle ASSERT_EQ(SQL_SUCCESS, SQLFreeEnv(env)); } TEST(SQLSetEnvAttr, TestSQLSetEnvAttrNullValuePointer) { - SQLHENV env; - // Allocate an environment handle + SQLHENV env; ASSERT_EQ(SQL_SUCCESS, SQLAllocEnv(&env)); // Attempt to set using bad data pointer ASSERT_EQ(SQL_ERROR, SQLSetEnvAttr(env, SQL_ATTR_ODBC_VERSION, nullptr, 0)); + // Free environment handle ASSERT_EQ(SQL_SUCCESS, SQLFreeEnv(env)); } +TYPED_TEST(ConnectionHandleTest, TestSQLDriverConnect) { + // Connect string + std::string connect_str = this->GetConnectionString(); + ASSERT_OK_AND_ASSIGN(std::wstring wconnect_str, + arrow::util::UTF8ToWideString(connect_str)); + std::vector connect_str0(wconnect_str.begin(), wconnect_str.end()); + + SQLWCHAR out_str[kOdbcBufferSize] = L""; + SQLSMALLINT out_str_len; + + // Connecting to ODBC server. + ASSERT_EQ(SQL_SUCCESS, + SQLDriverConnect(this->conn, NULL, &connect_str0[0], + static_cast(connect_str0.size()), out_str, + kOdbcBufferSize, &out_str_len, SQL_DRIVER_NOPROMPT)); + + // Check that out_str has same content as connect_str + std::string out_connection_string = ODBC::SqlWcharToString(out_str, out_str_len); + Connection::ConnPropertyMap out_properties; + Connection::ConnPropertyMap in_properties; + ODBC::ODBCConnection::GetPropertiesFromConnString(out_connection_string, + out_properties); + ODBC::ODBCConnection::GetPropertiesFromConnString(connect_str, in_properties); + ASSERT_TRUE(CompareConnPropertyMap(out_properties, in_properties)); + + // Disconnect from ODBC + ASSERT_EQ(SQL_SUCCESS, SQLDisconnect(this->conn)); +} + +#if defined _WIN32 || defined _WIN64 +TYPED_TEST(ConnectionHandleTest, TestSQLDriverConnectDsn) { + // Connect string + std::string connect_str = this->GetConnectionString(); + + // Write connection string content into a DSN, + // must succeed before continuing + ASSERT_TRUE(WriteDSN(connect_str)); + + std::string dsn(kTestDsn); + ASSERT_OK_AND_ASSIGN(std::wstring wdsn, arrow::util::UTF8ToWideString(dsn)); + + // Update connection string to use DSN to connect + connect_str = std::string("DSN=") + std::string(kTestDsn) + + std::string(";driver={Apache Arrow Flight SQL ODBC Driver};"); + ASSERT_OK_AND_ASSIGN(std::wstring wconnect_str, + arrow::util::UTF8ToWideString(connect_str)); + std::vector connect_str0(wconnect_str.begin(), wconnect_str.end()); + + SQLWCHAR out_str[kOdbcBufferSize] = L""; + SQLSMALLINT out_str_len; + + // Connecting to ODBC server. + ASSERT_EQ(SQL_SUCCESS, + SQLDriverConnect(this->conn, NULL, &connect_str0[0], + static_cast(connect_str0.size()), out_str, + kOdbcBufferSize, &out_str_len, SQL_DRIVER_NOPROMPT)); + + // Remove DSN + ASSERT_TRUE(UnregisterDsn(wdsn)); + + // Disconnect from ODBC + ASSERT_EQ(SQL_SUCCESS, SQLDisconnect(this->conn)); +} + +TYPED_TEST(ConnectionHandleTest, TestSQLConnect) { + // Connect string + std::string connect_str = this->GetConnectionString(); + + // Write connection string content into a DSN, + // must succeed before continuing + std::string uid(""), pwd(""); + ASSERT_TRUE(WriteDSN(connect_str)); + + std::string dsn(kTestDsn); + ASSERT_OK_AND_ASSIGN(std::wstring wdsn, arrow::util::UTF8ToWideString(dsn)); + ASSERT_OK_AND_ASSIGN(std::wstring wuid, arrow::util::UTF8ToWideString(uid)); + ASSERT_OK_AND_ASSIGN(std::wstring wpwd, arrow::util::UTF8ToWideString(pwd)); + std::vector dsn0(wdsn.begin(), wdsn.end()); + std::vector uid0(wuid.begin(), wuid.end()); + std::vector pwd0(wpwd.begin(), wpwd.end()); + + // Connecting to ODBC server. Empty uid and pwd should be ignored. + ASSERT_EQ(SQL_SUCCESS, + SQLConnect(this->conn, dsn0.data(), static_cast(dsn0.size()), + uid0.data(), static_cast(uid0.size()), pwd0.data(), + static_cast(pwd0.size()))); + + // Remove DSN + ASSERT_TRUE(UnregisterDsn(wdsn)); + + // Disconnect from ODBC + ASSERT_EQ(SQL_SUCCESS, SQLDisconnect(this->conn)); +} + +TEST_F(ConnectionRemoteTest, TestSQLConnectInputUidPwd) { + // Connect string + std::string connect_str = GetConnectionString(); + + // Retrieve valid uid and pwd, assumes TEST_CONNECT_STR contains uid and pwd + Connection::ConnPropertyMap properties; + ODBC::ODBCConnection::GetPropertiesFromConnString(connect_str, properties); + std::string uid_key("uid"); + std::string pwd_key("pwd"); + std::string uid = properties[uid_key]; + std::string pwd = properties[pwd_key]; + + // Write connection string content without uid and pwd into a DSN, + // must succeed before continuing + properties.erase(uid_key); + properties.erase(pwd_key); + ASSERT_TRUE(WriteDSN(properties)); + + std::string dsn(kTestDsn); + ASSERT_OK_AND_ASSIGN(std::wstring wdsn, arrow::util::UTF8ToWideString(dsn)); + ASSERT_OK_AND_ASSIGN(std::wstring wuid, arrow::util::UTF8ToWideString(uid)); + ASSERT_OK_AND_ASSIGN(std::wstring wpwd, arrow::util::UTF8ToWideString(pwd)); + std::vector dsn0(wdsn.begin(), wdsn.end()); + std::vector uid0(wuid.begin(), wuid.end()); + std::vector pwd0(wpwd.begin(), wpwd.end()); + + // Connecting to ODBC server. + ASSERT_EQ(SQL_SUCCESS, + SQLConnect(this->conn, dsn0.data(), static_cast(dsn0.size()), + uid0.data(), static_cast(uid0.size()), pwd0.data(), + static_cast(pwd0.size()))); + + // Remove DSN + ASSERT_TRUE(UnregisterDsn(wdsn)); + + // Disconnect from ODBC + ASSERT_EQ(SQL_SUCCESS, SQLDisconnect(this->conn)); +} + +TEST_F(ConnectionRemoteTest, TestSQLConnectInvalidUid) { + // Connect string + std::string connect_str = GetConnectionString(); + + // Retrieve valid uid and pwd, assumes TEST_CONNECT_STR contains uid and pwd + Connection::ConnPropertyMap properties; + ODBC::ODBCConnection::GetPropertiesFromConnString(connect_str, properties); + std::string uid = properties[std::string("uid")]; + std::string pwd = properties[std::string("pwd")]; + + // Append invalid uid to connection string + connect_str += std::string("uid=non_existent_id;"); + + // Write connection string content into a DSN, + // must succeed before continuing + ASSERT_TRUE(WriteDSN(connect_str)); + + std::string dsn(kTestDsn); + ASSERT_OK_AND_ASSIGN(std::wstring wdsn, arrow::util::UTF8ToWideString(dsn)); + ASSERT_OK_AND_ASSIGN(std::wstring wuid, arrow::util::UTF8ToWideString(uid)); + ASSERT_OK_AND_ASSIGN(std::wstring wpwd, arrow::util::UTF8ToWideString(pwd)); + std::vector dsn0(wdsn.begin(), wdsn.end()); + std::vector uid0(wuid.begin(), wuid.end()); + std::vector pwd0(wpwd.begin(), wpwd.end()); + + // Connecting to ODBC server. + // UID specified in DSN will take precedence, + // so connection still fails despite passing valid uid in SQLConnect call + ASSERT_EQ(SQL_ERROR, + SQLConnect(this->conn, dsn0.data(), static_cast(dsn0.size()), + uid0.data(), static_cast(uid0.size()), pwd0.data(), + static_cast(pwd0.size()))); + + VerifyOdbcErrorState(SQL_HANDLE_DBC, this->conn, kErrorState28000); + + // Remove DSN + ASSERT_TRUE(UnregisterDsn(wdsn)); +} + +TEST_F(ConnectionRemoteTest, TestSQLConnectDSNPrecedence) { + // Connect string + std::string connect_str = GetConnectionString(); + + // Write connection string content into a DSN, + // must succeed before continuing + + // Pass incorrect uid and password to SQLConnect, they will be ignored. + // Assumes TEST_CONNECT_STR contains uid and pwd + std::string uid("non_existent_id"), pwd("non_existent_password"); + ASSERT_TRUE(WriteDSN(connect_str)); + + std::string dsn(kTestDsn); + ASSERT_OK_AND_ASSIGN(std::wstring wdsn, arrow::util::UTF8ToWideString(dsn)); + ASSERT_OK_AND_ASSIGN(std::wstring wuid, arrow::util::UTF8ToWideString(uid)); + ASSERT_OK_AND_ASSIGN(std::wstring wpwd, arrow::util::UTF8ToWideString(pwd)); + std::vector dsn0(wdsn.begin(), wdsn.end()); + std::vector uid0(wuid.begin(), wuid.end()); + std::vector pwd0(wpwd.begin(), wpwd.end()); + + // Connecting to ODBC server. + ASSERT_EQ(SQL_SUCCESS, + SQLConnect(this->conn, dsn0.data(), static_cast(dsn0.size()), + uid0.data(), static_cast(uid0.size()), pwd0.data(), + static_cast(pwd0.size()))); + + // Remove DSN + ASSERT_TRUE(UnregisterDsn(wdsn)); + + // Disconnect from ODBC + ASSERT_EQ(SQL_SUCCESS, SQLDisconnect(this->conn)); +} + +#endif + +TEST_F(ConnectionRemoteTest, TestSQLDriverConnectInvalidUid) { + // Invalid connect string + std::string connect_str = GetInvalidConnectionString(); + + ASSERT_OK_AND_ASSIGN(std::wstring wconnect_str, + arrow::util::UTF8ToWideString(connect_str)); + std::vector connect_str0(wconnect_str.begin(), wconnect_str.end()); + + SQLWCHAR out_str[kOdbcBufferSize]; + SQLSMALLINT out_str_len; + + // Connecting to ODBC server. + ASSERT_EQ(SQL_ERROR, + SQLDriverConnect(this->conn, NULL, &connect_str0[0], + static_cast(connect_str0.size()), out_str, + kOdbcBufferSize, &out_str_len, SQL_DRIVER_NOPROMPT)); + + VerifyOdbcErrorState(SQL_HANDLE_DBC, this->conn, kErrorState28000); + + std::string out_connection_string = ODBC::SqlWcharToString(out_str, out_str_len); + ASSERT_TRUE(out_connection_string.empty()); +} + +TYPED_TEST(ConnectionHandleTest, TestSQLDisconnectWithoutConnection) { + // Attempt to disconnect without a connection, expect to fail + ASSERT_EQ(SQL_ERROR, SQLDisconnect(this->conn)); + + // Expect ODBC driver manager to return error state + VerifyOdbcErrorState(SQL_HANDLE_DBC, this->conn, kErrorState08003); +} + +TYPED_TEST(ConnectionTest, TestConnect) { + // Verifies connect and disconnect works on its own +} + } // namespace arrow::flight::sql::odbc diff --git a/cpp/src/arrow/flight/sql/odbc/tests/odbc_test_suite.cc b/cpp/src/arrow/flight/sql/odbc/tests/odbc_test_suite.cc index fccb5525759..751450ceffd 100644 --- a/cpp/src/arrow/flight/sql/odbc/tests/odbc_test_suite.cc +++ b/cpp/src/arrow/flight/sql/odbc/tests/odbc_test_suite.cc @@ -28,7 +28,7 @@ namespace arrow::flight::sql::odbc { -void FlightSQLODBCRemoteTestBase::AllocEnvConnHandles(SQLINTEGER odbc_ver) { +void ODBCRemoteTestBase::AllocEnvConnHandles(SQLINTEGER odbc_ver) { // Allocate an environment handle ASSERT_EQ(SQL_SUCCESS, SQLAllocEnv(&env)); @@ -41,13 +41,13 @@ void FlightSQLODBCRemoteTestBase::AllocEnvConnHandles(SQLINTEGER odbc_ver) { ASSERT_EQ(SQL_SUCCESS, SQLAllocHandle(SQL_HANDLE_DBC, env, &conn)); } -void FlightSQLODBCRemoteTestBase::Connect(SQLINTEGER odbc_ver) { +void ODBCRemoteTestBase::Connect(SQLINTEGER odbc_ver) { ASSERT_NO_FATAL_FAILURE(AllocEnvConnHandles(odbc_ver)); std::string connect_str = GetConnectionString(); ASSERT_NO_FATAL_FAILURE(ConnectWithString(connect_str)); } -void FlightSQLODBCRemoteTestBase::ConnectWithString(std::string connect_str) { +void ODBCRemoteTestBase::ConnectWithString(std::string connect_str) { // Connect string std::vector connect_str0(connect_str.begin(), connect_str.end()); @@ -65,7 +65,7 @@ void FlightSQLODBCRemoteTestBase::ConnectWithString(std::string connect_str) { ASSERT_EQ(SQL_SUCCESS, SQLAllocHandle(SQL_HANDLE_STMT, conn, &stmt)); } -void FlightSQLODBCRemoteTestBase::Disconnect() { +void ODBCRemoteTestBase::Disconnect() { // Close statement EXPECT_EQ(SQL_SUCCESS, SQLFreeHandle(SQL_HANDLE_STMT, stmt)); @@ -73,6 +73,10 @@ void FlightSQLODBCRemoteTestBase::Disconnect() { EXPECT_EQ(SQL_SUCCESS, SQLDisconnect(conn)) << GetOdbcErrorMessage(SQL_HANDLE_DBC, conn); + FreeEnvConnHandles(); +} + +void ODBCRemoteTestBase::FreeEnvConnHandles() { // Free connection handle EXPECT_EQ(SQL_SUCCESS, SQLFreeHandle(SQL_HANDLE_DBC, conn)); @@ -80,20 +84,20 @@ void FlightSQLODBCRemoteTestBase::Disconnect() { EXPECT_EQ(SQL_SUCCESS, SQLFreeHandle(SQL_HANDLE_ENV, env)); } -std::string FlightSQLODBCRemoteTestBase::GetConnectionString() { +std::string ODBCRemoteTestBase::GetConnectionString() { std::string connect_str = arrow::internal::GetEnvVar(kTestConnectStr.data()).ValueOrDie(); return connect_str; } -std::string FlightSQLODBCRemoteTestBase::GetInvalidConnectionString() { +std::string ODBCRemoteTestBase::GetInvalidConnectionString() { std::string connect_str = GetConnectionString(); // Append invalid uid to connection string connect_str += std::string("uid=non_existent_id;"); return connect_str; } -std::wstring FlightSQLODBCRemoteTestBase::GetQueryAllDataTypes() { +std::wstring ODBCRemoteTestBase::GetQueryAllDataTypes() { std::wstring wsql = LR"( SELECT -- Numeric types @@ -144,11 +148,14 @@ std::wstring FlightSQLODBCRemoteTestBase::GetQueryAllDataTypes() { return wsql; } -void FlightSQLODBCRemoteTestBase::SetUp() { +void ODBCRemoteTestBase::SetUp() { if (arrow::internal::GetEnvVar(kTestConnectStr.data()).ValueOr("").empty()) { GTEST_SKIP() << "Skipping test: kTestConnectStr not set"; } +} +void FlightSQLODBCRemoteTestBase::SetUp() { + ODBCRemoteTestBase::SetUp(); this->Connect(); connected_ = true; } @@ -161,14 +168,18 @@ void FlightSQLODBCRemoteTestBase::TearDown() { } void FlightSQLOdbcV2RemoteTestBase::SetUp() { - if (arrow::internal::GetEnvVar(kTestConnectStr.data()).ValueOr("").empty()) { - GTEST_SKIP() << "Skipping test: kTestConnectStr not set"; - } - + ODBCRemoteTestBase::SetUp(); this->Connect(SQL_OV_ODBC2); connected_ = true; } +void FlightSQLOdbcHandleRemoteTestBase::SetUp() { + ODBCRemoteTestBase::SetUp(); + this->AllocEnvConnHandles(); +} + +void FlightSQLOdbcHandleRemoteTestBase::TearDown() { this->FreeEnvConnHandles(); } + std::string FindTokenInCallHeaders(const CallHeaders& incoming_headers) { // Lambda function to compare characters without case sensitivity. auto char_compare = [](const char& char1, const char& char2) { @@ -209,7 +220,7 @@ Status MockServerMiddlewareFactory::StartCall( return Status::OK(); } -std::string FlightSQLODBCMockTestBase::GetConnectionString() { +std::string ODBCMockTestBase::GetConnectionString() { std::string connect_str( "driver={Apache Arrow Flight SQL ODBC Driver};HOST=localhost;port=" + std::to_string(port) + ";token=" + std::string(kTestToken) + @@ -217,14 +228,14 @@ std::string FlightSQLODBCMockTestBase::GetConnectionString() { return connect_str; } -std::string FlightSQLODBCMockTestBase::GetInvalidConnectionString() { +std::string ODBCMockTestBase::GetInvalidConnectionString() { std::string connect_str = GetConnectionString(); // Append invalid token to connection string connect_str += std::string("token=invalid_token;"); return connect_str; } -std::wstring FlightSQLODBCMockTestBase::GetQueryAllDataTypes() { +std::wstring ODBCMockTestBase::GetQueryAllDataTypes() { std::wstring wsql = LR"( SELECT -- Numeric types @@ -273,7 +284,7 @@ std::wstring FlightSQLODBCMockTestBase::GetQueryAllDataTypes() { return wsql; } -void FlightSQLODBCMockTestBase::CreateTestTables() { +void ODBCMockTestBase::CreateTestTables() { ASSERT_OK(server_->ExecuteSql(R"( CREATE TABLE TestTable ( id INTEGER PRIMARY KEY AUTOINCREMENT, @@ -286,7 +297,7 @@ void FlightSQLODBCMockTestBase::CreateTestTables() { )")); } -void FlightSQLODBCMockTestBase::CreateTableAllDataType() { +void ODBCMockTestBase::CreateTableAllDataType() { // Limitation on mock SQLite server: // Only int64, float64, binary, and utf8 Arrow Types are supported by // SQLiteFlightSqlServer::Impl::DoGetTables @@ -308,7 +319,7 @@ void FlightSQLODBCMockTestBase::CreateTableAllDataType() { )")); } -void FlightSQLODBCMockTestBase::CreateUnicodeTable() { +void ODBCMockTestBase::CreateUnicodeTable() { std::string unicode_sql = arrow::util::WideStringToUTF8( LR"( CREATE TABLE 数据( @@ -322,7 +333,7 @@ void FlightSQLODBCMockTestBase::CreateUnicodeTable() { ASSERT_OK(server_->ExecuteSql(unicode_sql)); } -void FlightSQLODBCMockTestBase::Initialize() { +void ODBCMockTestBase::SetUp() { ASSERT_OK_AND_ASSIGN(auto location, Location::ForGrpcTcp("0.0.0.0", 0)); arrow::flight::FlightServerOptions options(location); options.auth_handler = std::make_unique(); @@ -338,25 +349,40 @@ void FlightSQLODBCMockTestBase::Initialize() { } void FlightSQLODBCMockTestBase::SetUp() { - this->Initialize(); + ODBCMockTestBase::SetUp(); this->Connect(); connected_ = true; } +void ODBCMockTestBase::TearDown() { + ASSERT_OK(server_->Shutdown()); + ASSERT_OK(server_->Wait()); +} + void FlightSQLODBCMockTestBase::TearDown() { if (connected_) { this->Disconnect(); connected_ = false; } - ASSERT_OK(server_->Shutdown()); + ODBCMockTestBase::TearDown(); } void FlightSQLOdbcV2MockTestBase::SetUp() { - this->Initialize(); + ODBCMockTestBase::SetUp(); this->Connect(SQL_OV_ODBC2); connected_ = true; } +void FlightSQLOdbcHandleMockTestBase::SetUp() { + ODBCMockTestBase::SetUp(); + this->AllocEnvConnHandles(); +} + +void FlightSQLOdbcHandleMockTestBase::TearDown() { + this->FreeEnvConnHandles(); + ODBCMockTestBase::TearDown(); +} + bool CompareConnPropertyMap(Connection::ConnPropertyMap map1, Connection::ConnPropertyMap map2) { if (map1.size() != map2.size()) return false; @@ -411,7 +437,7 @@ std::string GetOdbcErrorMessage(SQLSMALLINT handle_type, SQLHANDLE handle) { return res; } -// TODO: once RegisterDsn is implemented in Mac and Linux, the following can be +// GH-47822 TODO: once RegisterDsn is implemented in Mac and Linux, the following can be // re-enabled. #if defined _WIN32 bool WriteDSN(std::string connection_str) { diff --git a/cpp/src/arrow/flight/sql/odbc/tests/odbc_test_suite.h b/cpp/src/arrow/flight/sql/odbc/tests/odbc_test_suite.h index e35e6c38f85..8f3cebe7819 100644 --- a/cpp/src/arrow/flight/sql/odbc/tests/odbc_test_suite.h +++ b/cpp/src/arrow/flight/sql/odbc/tests/odbc_test_suite.h @@ -42,14 +42,16 @@ static constexpr std::string_view kTestDsn = "Apache Arrow Flight SQL Test DSN"; namespace arrow::flight::sql::odbc { /// \brief Base test fixture for running tests against a remote server. -/// Each test file running remote server tests should define a -/// fixture inheriting from this base fixture. /// The connection string for connecting to this server is defined /// in the ARROW_FLIGHT_SQL_ODBC_CONN environment variable. -class FlightSQLODBCRemoteTestBase : public ::testing::Test { +/// Note that this fixture does not handle the driver's connection/disconnection +/// during SetUp/Teardown. +class ODBCRemoteTestBase : public ::testing::Test { public: /// \brief Allocate environment and connection handles void AllocEnvConnHandles(SQLINTEGER odbc_ver = SQL_OV_ODBC3); + /// \brief Free environment and connection handles + void FreeEnvConnHandles(); /// \brief Connect to Arrow Flight SQL server using connection string defined in /// environment variable "ARROW_FLIGHT_SQL_ODBC_CONN", allocate statement handle. /// Connects using ODBC Ver 3 by default @@ -75,6 +77,16 @@ class FlightSQLODBCRemoteTestBase : public ::testing::Test { /** ODBC Statement. */ SQLHSTMT stmt = 0; + protected: + void SetUp() override; +}; + +/// \brief Base test fixture for running tests against a remote server. +/// Each test file running remote server tests should define a +/// fixture inheriting from this base fixture. +/// The connection string for connecting to this server is defined +/// in the ARROW_FLIGHT_SQL_ODBC_CONN environment variable. +class FlightSQLODBCRemoteTestBase : public ODBCRemoteTestBase { protected: void SetUp() override; @@ -91,6 +103,12 @@ class FlightSQLOdbcV2RemoteTestBase : public FlightSQLODBCRemoteTestBase { void SetUp() override; }; +class FlightSQLOdbcHandleRemoteTestBase : public FlightSQLODBCRemoteTestBase { + protected: + void SetUp() override; + void TearDown() override; +}; + static constexpr std::string_view kAuthorizationHeader = "authorization"; static constexpr std::string_view kBearerPrefix = "Bearer "; static constexpr std::string_view kTestToken = "t0k3n"; @@ -129,9 +147,7 @@ class MockServerMiddlewareFactory : public ServerMiddlewareFactory { }; /// \brief Base test fixture for running tests against a mock server. -/// Each test file running mock server tests should define a -/// fixture inheriting from this base fixture. -class FlightSQLODBCMockTestBase : public FlightSQLODBCRemoteTestBase { +class ODBCMockTestBase : public FlightSQLODBCRemoteTestBase { // Sets up a mock server for each test case public: /// \brief Get connection string for mock server @@ -152,16 +168,23 @@ class FlightSQLODBCMockTestBase : public FlightSQLODBCRemoteTestBase { int port; protected: - void Initialize(); - void SetUp() override; void TearDown() override; - private: std::shared_ptr server_; }; +/// \brief Base test fixture for running tests against a mock server. +/// Each test file running mock server tests should define a +/// fixture inheriting from this base fixture. +class FlightSQLODBCMockTestBase : public ODBCMockTestBase { + protected: + void SetUp() override; + + void TearDown() override; +}; + /// \brief Base test fixture for running ODBC V2 tests against a mock server. /// Each test file running mock server ODBC V2 tests should define a /// fixture inheriting from this base fixture. @@ -170,6 +193,12 @@ class FlightSQLOdbcV2MockTestBase : public FlightSQLODBCMockTestBase { void SetUp() override; }; +class FlightSQLOdbcHandleMockTestBase : public FlightSQLODBCMockTestBase { + protected: + void SetUp() override; + void TearDown() override; +}; + /** ODBC read buffer size. */ static constexpr int kOdbcBufferSize = 1024; From 6d260ad98dc936feece2d6879b501572ce145132 Mon Sep 17 00:00:00 2001 From: "Alina (Xi) Li" Date: Tue, 28 Oct 2025 15:29:15 -0700 Subject: [PATCH 03/11] Port test fixture changes Co-authored-by: justing-bq --- cpp/src/arrow/flight/sql/odbc/odbc_api.cc | 4 +- .../flight/sql/odbc/tests/connection_test.cc | 39 ++++++++++++------- .../flight/sql/odbc/tests/odbc_test_suite.cc | 21 +++++++++- .../flight/sql/odbc/tests/odbc_test_suite.h | 4 ++ 4 files changed, 51 insertions(+), 17 deletions(-) diff --git a/cpp/src/arrow/flight/sql/odbc/odbc_api.cc b/cpp/src/arrow/flight/sql/odbc/odbc_api.cc index 8d156964e5b..65facfe8062 100644 --- a/cpp/src/arrow/flight/sql/odbc/odbc_api.cc +++ b/cpp/src/arrow/flight/sql/odbc/odbc_api.cc @@ -31,7 +31,7 @@ #include "arrow/flight/sql/odbc/odbc_impl/spi/connection.h" #include "arrow/util/logging.h" -#if defined _WIN32 || defined _WIN64 +#if defined _WIN32 // For displaying DSN Window # include "arrow/flight/sql/odbc/odbc_impl/system_dsn.h" #endif @@ -785,7 +785,7 @@ SQLRETURN SQLDriverConnect(SQLHDBC conn, SQLHWND window_handle, // GH-46448 TODO: Implement SQL_DRIVER_COMPLETE_REQUIRED in SQLDriverConnect according // to the spec -#if defined _WIN32 || defined _WIN64 +#if defined _WIN32 // Load the DSN window according to driver_completion if (driver_completion == SQL_DRIVER_PROMPT) { // Load DSN window before first attempt to connect diff --git a/cpp/src/arrow/flight/sql/odbc/tests/connection_test.cc b/cpp/src/arrow/flight/sql/odbc/tests/connection_test.cc index 2e115253ad4..3eff87a5778 100644 --- a/cpp/src/arrow/flight/sql/odbc/tests/connection_test.cc +++ b/cpp/src/arrow/flight/sql/odbc/tests/connection_test.cc @@ -162,7 +162,8 @@ TYPED_TEST(ConnectionTest, TestSQLGetEnvAttrOutputNTS) { TYPED_TEST(ConnectionTest, DISABLED_TestSQLGetEnvAttrGetLength) { // Test is disabled because call to SQLGetEnvAttr is handled by the driver manager on - // Windows. This test case can be potentially used on macOS/Linux + // Windows. Windows driver manager ignores the length pointer. + // This test case can be potentially used on macOS/Linux SQLINTEGER length; ASSERT_EQ(SQL_SUCCESS, SQLGetEnvAttr(this->env, SQL_ATTR_ODBC_VERSION, nullptr, 0, &length)); @@ -172,7 +173,8 @@ TYPED_TEST(ConnectionTest, DISABLED_TestSQLGetEnvAttrGetLength) { TYPED_TEST(ConnectionTest, DISABLED_TestSQLGetEnvAttrNullValuePointer) { // Test is disabled because call to SQLGetEnvAttr is handled by the driver manager on - // Windows. This test case can be potentially used on macOS/Linux + // Windows. The Windows driver manager doesn't error out when null pointer is passed. + // This test case can be potentially used on macOS/Linux ASSERT_EQ(SQL_ERROR, SQLGetEnvAttr(this->env, SQL_ATTR_ODBC_VERSION, nullptr, 0, nullptr)); } @@ -229,7 +231,8 @@ TYPED_TEST(ConnectionHandleTest, TestSQLDriverConnect) { ASSERT_EQ(SQL_SUCCESS, SQLDriverConnect(this->conn, NULL, &connect_str0[0], static_cast(connect_str0.size()), out_str, - kOdbcBufferSize, &out_str_len, SQL_DRIVER_NOPROMPT)); + kOdbcBufferSize, &out_str_len, SQL_DRIVER_NOPROMPT)) + << GetOdbcErrorMessage(SQL_HANDLE_DBC, this->conn); // Check that out_str has same content as connect_str std::string out_connection_string = ODBC::SqlWcharToString(out_str, out_str_len); @@ -241,10 +244,11 @@ TYPED_TEST(ConnectionHandleTest, TestSQLDriverConnect) { ASSERT_TRUE(CompareConnPropertyMap(out_properties, in_properties)); // Disconnect from ODBC - ASSERT_EQ(SQL_SUCCESS, SQLDisconnect(this->conn)); + ASSERT_EQ(SQL_SUCCESS, SQLDisconnect(this->conn)) + << GetOdbcErrorMessage(SQL_HANDLE_DBC, this->conn); } -#if defined _WIN32 || defined _WIN64 +#if defined _WIN32 TYPED_TEST(ConnectionHandleTest, TestSQLDriverConnectDsn) { // Connect string std::string connect_str = this->GetConnectionString(); @@ -270,13 +274,15 @@ TYPED_TEST(ConnectionHandleTest, TestSQLDriverConnectDsn) { ASSERT_EQ(SQL_SUCCESS, SQLDriverConnect(this->conn, NULL, &connect_str0[0], static_cast(connect_str0.size()), out_str, - kOdbcBufferSize, &out_str_len, SQL_DRIVER_NOPROMPT)); + kOdbcBufferSize, &out_str_len, SQL_DRIVER_NOPROMPT)) + << GetOdbcErrorMessage(SQL_HANDLE_DBC, this->conn); // Remove DSN ASSERT_TRUE(UnregisterDsn(wdsn)); // Disconnect from ODBC - ASSERT_EQ(SQL_SUCCESS, SQLDisconnect(this->conn)); + ASSERT_EQ(SQL_SUCCESS, SQLDisconnect(this->conn)) + << GetOdbcErrorMessage(SQL_HANDLE_DBC, this->conn); } TYPED_TEST(ConnectionHandleTest, TestSQLConnect) { @@ -300,13 +306,15 @@ TYPED_TEST(ConnectionHandleTest, TestSQLConnect) { ASSERT_EQ(SQL_SUCCESS, SQLConnect(this->conn, dsn0.data(), static_cast(dsn0.size()), uid0.data(), static_cast(uid0.size()), pwd0.data(), - static_cast(pwd0.size()))); + static_cast(pwd0.size()))) + << GetOdbcErrorMessage(SQL_HANDLE_DBC, this->conn); // Remove DSN ASSERT_TRUE(UnregisterDsn(wdsn)); // Disconnect from ODBC - ASSERT_EQ(SQL_SUCCESS, SQLDisconnect(this->conn)); + ASSERT_EQ(SQL_SUCCESS, SQLDisconnect(this->conn)) + << GetOdbcErrorMessage(SQL_HANDLE_DBC, this->conn); } TEST_F(ConnectionRemoteTest, TestSQLConnectInputUidPwd) { @@ -339,13 +347,15 @@ TEST_F(ConnectionRemoteTest, TestSQLConnectInputUidPwd) { ASSERT_EQ(SQL_SUCCESS, SQLConnect(this->conn, dsn0.data(), static_cast(dsn0.size()), uid0.data(), static_cast(uid0.size()), pwd0.data(), - static_cast(pwd0.size()))); + static_cast(pwd0.size()))) + << GetOdbcErrorMessage(SQL_HANDLE_DBC, conn); // Remove DSN ASSERT_TRUE(UnregisterDsn(wdsn)); // Disconnect from ODBC - ASSERT_EQ(SQL_SUCCESS, SQLDisconnect(this->conn)); + ASSERT_EQ(SQL_SUCCESS, SQLDisconnect(this->conn)) + << GetOdbcErrorMessage(SQL_HANDLE_DBC, conn); } TEST_F(ConnectionRemoteTest, TestSQLConnectInvalidUid) { @@ -411,13 +421,15 @@ TEST_F(ConnectionRemoteTest, TestSQLConnectDSNPrecedence) { ASSERT_EQ(SQL_SUCCESS, SQLConnect(this->conn, dsn0.data(), static_cast(dsn0.size()), uid0.data(), static_cast(uid0.size()), pwd0.data(), - static_cast(pwd0.size()))); + static_cast(pwd0.size()))) + << GetOdbcErrorMessage(SQL_HANDLE_DBC, conn); // Remove DSN ASSERT_TRUE(UnregisterDsn(wdsn)); // Disconnect from ODBC - ASSERT_EQ(SQL_SUCCESS, SQLDisconnect(this->conn)); + ASSERT_EQ(SQL_SUCCESS, SQLDisconnect(this->conn)) + << GetOdbcErrorMessage(SQL_HANDLE_DBC, conn); } #endif @@ -456,5 +468,4 @@ TYPED_TEST(ConnectionHandleTest, TestSQLDisconnectWithoutConnection) { TYPED_TEST(ConnectionTest, TestConnect) { // Verifies connect and disconnect works on its own } - } // namespace arrow::flight::sql::odbc diff --git a/cpp/src/arrow/flight/sql/odbc/tests/odbc_test_suite.cc b/cpp/src/arrow/flight/sql/odbc/tests/odbc_test_suite.cc index 751450ceffd..f2e3e5da720 100644 --- a/cpp/src/arrow/flight/sql/odbc/tests/odbc_test_suite.cc +++ b/cpp/src/arrow/flight/sql/odbc/tests/odbc_test_suite.cc @@ -150,12 +150,17 @@ std::wstring ODBCRemoteTestBase::GetQueryAllDataTypes() { void ODBCRemoteTestBase::SetUp() { if (arrow::internal::GetEnvVar(kTestConnectStr.data()).ValueOr("").empty()) { + skipping_test_ = true; GTEST_SKIP() << "Skipping test: kTestConnectStr not set"; } } void FlightSQLODBCRemoteTestBase::SetUp() { ODBCRemoteTestBase::SetUp(); + if (skipping_test_) { + return; + } + this->Connect(); connected_ = true; } @@ -169,16 +174,30 @@ void FlightSQLODBCRemoteTestBase::TearDown() { void FlightSQLOdbcV2RemoteTestBase::SetUp() { ODBCRemoteTestBase::SetUp(); + if (skipping_test_) { + return; + } + this->Connect(SQL_OV_ODBC2); connected_ = true; } void FlightSQLOdbcHandleRemoteTestBase::SetUp() { ODBCRemoteTestBase::SetUp(); + if (skipping_test_) { + return; + } + this->AllocEnvConnHandles(); + allocated_ = true; } -void FlightSQLOdbcHandleRemoteTestBase::TearDown() { this->FreeEnvConnHandles(); } +void FlightSQLOdbcHandleRemoteTestBase::TearDown() { + if (allocated_) { + this->FreeEnvConnHandles(); + allocated_ = false; + } +} std::string FindTokenInCallHeaders(const CallHeaders& incoming_headers) { // Lambda function to compare characters without case sensitivity. diff --git a/cpp/src/arrow/flight/sql/odbc/tests/odbc_test_suite.h b/cpp/src/arrow/flight/sql/odbc/tests/odbc_test_suite.h index 8f3cebe7819..e043a459f0a 100644 --- a/cpp/src/arrow/flight/sql/odbc/tests/odbc_test_suite.h +++ b/cpp/src/arrow/flight/sql/odbc/tests/odbc_test_suite.h @@ -79,6 +79,8 @@ class ODBCRemoteTestBase : public ::testing::Test { protected: void SetUp() override; + + bool skipping_test_ = false; }; /// \brief Base test fixture for running tests against a remote server. @@ -107,6 +109,8 @@ class FlightSQLOdbcHandleRemoteTestBase : public FlightSQLODBCRemoteTestBase { protected: void SetUp() override; void TearDown() override; + + bool allocated_ = false; }; static constexpr std::string_view kAuthorizationHeader = "authorization"; From fec523c121cffb190cef3c4a70e256f0e9893cf7 Mon Sep 17 00:00:00 2001 From: "Alina (Xi) Li" Date: Tue, 28 Oct 2025 16:50:36 -0700 Subject: [PATCH 04/11] Fix connectivity issues from headers --- .../arrow/flight/sql/odbc/odbc_impl/flight_sql_connection.cc | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/cpp/src/arrow/flight/sql/odbc/odbc_impl/flight_sql_connection.cc b/cpp/src/arrow/flight/sql/odbc/odbc_impl/flight_sql_connection.cc index 7615e10623e..0cc7e56b247 100644 --- a/cpp/src/arrow/flight/sql/odbc/odbc_impl/flight_sql_connection.cc +++ b/cpp/src/arrow/flight/sql/odbc/odbc_impl/flight_sql_connection.cc @@ -161,6 +161,8 @@ void FlightSqlConnection::Connect(const ConnPropertyMap& properties, std::unique_ptr flight_client; ThrowIfNotOK(FlightClient::Connect(location, client_options).Value(&flight_client)); + PopulateMetadataSettings(properties); + PopulateCallOptions(properties); std::unique_ptr auth_method = FlightSqlAuthMethod::FromProperties(flight_client, properties); @@ -175,9 +177,6 @@ void FlightSqlConnection::Connect(const ConnPropertyMap& properties, info_.SetProperty(SQL_USER_NAME, auth_method->GetUser()); attribute_[CONNECTION_DEAD] = static_cast(SQL_FALSE); - - PopulateMetadataSettings(properties); - PopulateCallOptions(properties); } catch (...) { attribute_[CONNECTION_DEAD] = static_cast(SQL_TRUE); sql_client_.reset(); From 3d95eee9cc4956f4bc0679add9f0c83d88842fb6 Mon Sep 17 00:00:00 2001 From: "Alina (Xi) Li" Date: Wed, 29 Oct 2025 14:11:45 -0700 Subject: [PATCH 05/11] Attempt to fix segfault error Iirc, there is segfault error associated with `SQLGetStmtAttrW` Attempt to fix segfault error by adding impl for `SQLSetConnectAttr` Part-fix remove `using` that isn't needed --- cpp/src/arrow/flight/sql/odbc/odbc_api.cc | 15 +++++++++------ .../flight/sql/odbc/tests/odbc_test_suite.cc | 8 ++++---- 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/cpp/src/arrow/flight/sql/odbc/odbc_api.cc b/cpp/src/arrow/flight/sql/odbc/odbc_api.cc index 65facfe8062..685977167db 100644 --- a/cpp/src/arrow/flight/sql/odbc/odbc_api.cc +++ b/cpp/src/arrow/flight/sql/odbc/odbc_api.cc @@ -723,8 +723,15 @@ SQLRETURN SQLSetConnectAttr(SQLHDBC conn, SQLINTEGER attr, SQLPOINTER value_ptr, ARROW_LOG(DEBUG) << "SQLSetConnectAttrW called with conn: " << conn << ", attr: " << attr << ", value_ptr: " << value_ptr << ", value_len: " << value_len; - // GH-47708 TODO: Implement SQLSetConnectAttr - return SQL_INVALID_HANDLE; + // GH-47708 TODO: Add tests for SQLSetConnectAttr + using ODBC::ODBCConnection; + + return ODBCConnection::ExecuteWithDiagnostics(conn, SQL_ERROR, [=]() { + const bool is_unicode = true; + ODBCConnection* connection = reinterpret_cast(conn); + connection->SetConnectAttr(attr, value_ptr, value_len, is_unicode); + return SQL_SUCCESS; + }); } // Load properties from the given DSN. The properties loaded do _not_ overwrite existing @@ -766,8 +773,6 @@ SQLRETURN SQLDriverConnect(SQLHDBC conn, SQLHWND window_handle, // GH-46560 TODO: Copy connection string properly in SQLDriverConnect according to the // spec - using arrow::flight::sql::odbc::Connection; - using arrow::flight::sql::odbc::DriverException; using ODBC::ODBCConnection; return ODBCConnection::ExecuteWithDiagnostics(conn, SQL_ERROR, [=]() { @@ -840,8 +845,6 @@ SQLRETURN SQLConnect(SQLHDBC conn, SQLWCHAR* dsn_name, SQLSMALLINT dsn_name_len, << ", password: " << static_cast(password) << ", password_len: " << password_len; - using arrow::flight::sql::odbc::FlightSqlConnection; - using arrow::flight::sql::odbc::config::Configuration; using ODBC::ODBCConnection; using ODBC::SqlWcharToString; diff --git a/cpp/src/arrow/flight/sql/odbc/tests/odbc_test_suite.cc b/cpp/src/arrow/flight/sql/odbc/tests/odbc_test_suite.cc index f2e3e5da720..eb6c60b9762 100644 --- a/cpp/src/arrow/flight/sql/odbc/tests/odbc_test_suite.cc +++ b/cpp/src/arrow/flight/sql/odbc/tests/odbc_test_suite.cc @@ -61,13 +61,13 @@ void ODBCRemoteTestBase::ConnectWithString(std::string connect_str) { kOdbcBufferSize, &out_str_len, SQL_DRIVER_NOPROMPT)) << GetOdbcErrorMessage(SQL_HANDLE_DBC, conn); - // Allocate a statement using alloc handle - ASSERT_EQ(SQL_SUCCESS, SQLAllocHandle(SQL_HANDLE_STMT, conn, &stmt)); + // GH-47710: TODO Allocate a statement using alloc handle + // ASSERT_EQ(SQL_SUCCESS, SQLAllocHandle(SQL_HANDLE_STMT, conn, &stmt)); } void ODBCRemoteTestBase::Disconnect() { - // Close statement - EXPECT_EQ(SQL_SUCCESS, SQLFreeHandle(SQL_HANDLE_STMT, stmt)); + // GH-47710: TODO Close statement + // EXPECT_EQ(SQL_SUCCESS, SQLFreeHandle(SQL_HANDLE_STMT, stmt)); // Disconnect from ODBC EXPECT_EQ(SQL_SUCCESS, SQLDisconnect(conn)) From c23ff578dfb2d0c69bba9a35296cb93a6e53efdb Mon Sep 17 00:00:00 2001 From: "Alina (Xi) Li" Date: Wed, 5 Nov 2025 15:28:58 -0800 Subject: [PATCH 06/11] Remove ODBC from MinGW build See https://github.com/apache/arrow/issues/48019 --- .github/workflows/cpp.yml | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/.github/workflows/cpp.yml b/.github/workflows/cpp.yml index bb9571042c3..aed25b4748c 100644 --- a/.github/workflows/cpp.yml +++ b/.github/workflows/cpp.yml @@ -310,7 +310,7 @@ jobs: ARROW_DATASET: ON ARROW_FLIGHT: ON ARROW_FLIGHT_SQL: ON - ARROW_FLIGHT_SQL_ODBC: ON + ARROW_FLIGHT_SQL_ODBC: OFF ARROW_GANDIVA: ON ARROW_GCS: ON ARROW_HDFS: OFF @@ -389,10 +389,6 @@ jobs: PIPX_BASE_PYTHON: ${{ steps.python-install.outputs.python-path }} run: | ci/scripts/install_gcs_testbench.sh default - - name: Register Flight SQL ODBC Driver - shell: cmd - run: | - call "cpp\src\arrow\flight\sql\odbc\tests\install_odbc.cmd" ${{ github.workspace }}\build\cpp\%ARROW_BUILD_TYPE%\libarrow_flight_sql_odbc.dll - name: Test shell: msys2 {0} run: | From 2f88ba409bf3b8a49451d0f5c2aafde98cd05dea Mon Sep 17 00:00:00 2001 From: justing-bq <62349012+justing-bq@users.noreply.github.com> Date: Fri, 7 Nov 2025 12:41:39 -0800 Subject: [PATCH 07/11] Transparent Comparator --- .../sql/odbc/odbc_impl/attribute_utils.h | 15 ++++---- .../odbc/odbc_impl/config/configuration.cc | 21 ++++++----- .../sql/odbc/odbc_impl/config/configuration.h | 10 +++--- .../sql/odbc/odbc_impl/encoding_utils.h | 4 +-- .../odbc/odbc_impl/flight_sql_auth_method.cc | 12 +++---- .../odbc/odbc_impl/flight_sql_connection.cc | 11 ++++-- .../odbc/odbc_impl/flight_sql_connection.h | 7 ---- .../odbc_impl/flight_sql_connection_test.cc | 36 +++++++++---------- .../arrow/flight/sql/odbc/odbc_impl/main.cc | 10 +++--- .../sql/odbc/odbc_impl/spi/connection.h | 6 ++-- .../flight/sql/odbc/odbc_impl/system_dsn.cc | 2 +- .../arrow/flight/sql/odbc/odbc_impl/util.cc | 8 ++--- .../arrow/flight/sql/odbc/odbc_impl/util.h | 4 +-- .../flight/sql/odbc/tests/connection_test.cc | 6 ++-- 14 files changed, 75 insertions(+), 77 deletions(-) diff --git a/cpp/src/arrow/flight/sql/odbc/odbc_impl/attribute_utils.h b/cpp/src/arrow/flight/sql/odbc/odbc_impl/attribute_utils.h index fcf5d5a81d9..f163545f17e 100644 --- a/cpp/src/arrow/flight/sql/odbc/odbc_impl/attribute_utils.h +++ b/cpp/src/arrow/flight/sql/odbc/odbc_impl/attribute_utils.h @@ -47,8 +47,8 @@ inline void GetAttribute(T attribute_value, SQLPOINTER output, O output_size, } template -inline SQLRETURN GetAttributeUTF8(const std::string_view& attribute_value, - SQLPOINTER output, O output_size, O* output_len_ptr) { +inline SQLRETURN GetAttributeUTF8(std::string_view attribute_value, SQLPOINTER output, + O output_size, O* output_len_ptr) { if (output) { size_t output_len_before_null = std::min(static_cast(attribute_value.size()), static_cast(output_size - 1)); @@ -67,8 +67,8 @@ inline SQLRETURN GetAttributeUTF8(const std::string_view& attribute_value, } template -inline SQLRETURN GetAttributeUTF8(const std::string_view& attribute_value, - SQLPOINTER output, O output_size, O* output_len_ptr, +inline SQLRETURN GetAttributeUTF8(std::string_view attribute_value, SQLPOINTER output, + O output_size, O* output_len_ptr, Diagnostics& diagnostics) { SQLRETURN result = GetAttributeUTF8(attribute_value, output, output_size, output_len_ptr); @@ -79,7 +79,7 @@ inline SQLRETURN GetAttributeUTF8(const std::string_view& attribute_value, } template -inline SQLRETURN GetAttributeSQLWCHAR(const std::string_view& attribute_value, +inline SQLRETURN GetAttributeSQLWCHAR(std::string_view attribute_value, bool is_length_in_bytes, SQLPOINTER output, O output_size, O* output_len_ptr) { size_t length = ConvertToSqlWChar( @@ -103,7 +103,7 @@ inline SQLRETURN GetAttributeSQLWCHAR(const std::string_view& attribute_value, } template -inline SQLRETURN GetAttributeSQLWCHAR(const std::string_view& attribute_value, +inline SQLRETURN GetAttributeSQLWCHAR(std::string_view attribute_value, bool is_length_in_bytes, SQLPOINTER output, O output_size, O* output_len_ptr, Diagnostics& diagnostics) { @@ -116,8 +116,7 @@ inline SQLRETURN GetAttributeSQLWCHAR(const std::string_view& attribute_value, } template -inline SQLRETURN GetStringAttribute(bool is_unicode, - const std::string_view& attribute_value, +inline SQLRETURN GetStringAttribute(bool is_unicode, std::string_view attribute_value, bool is_length_in_bytes, SQLPOINTER output, O output_size, O* output_len_ptr, Diagnostics& diagnostics) { diff --git a/cpp/src/arrow/flight/sql/odbc/odbc_impl/config/configuration.cc b/cpp/src/arrow/flight/sql/odbc/odbc_impl/config/configuration.cc index c72fcbfbbf1..7eea7fe9ded 100644 --- a/cpp/src/arrow/flight/sql/odbc/odbc_impl/config/configuration.cc +++ b/cpp/src/arrow/flight/sql/odbc/odbc_impl/config/configuration.cc @@ -35,7 +35,7 @@ static const char DEFAULT_USE_CERT_STORE[] = TRUE_STR; static const char DEFAULT_DISABLE_CERT_VERIFICATION[] = FALSE_STR; namespace { -std::string ReadDsnString(const std::string& dsn, const std::string_view& key, +std::string ReadDsnString(const std::string& dsn, std::string_view key, const std::string& dflt = "") { CONVERT_WIDE_STR(const std::wstring wdsn, dsn); CONVERT_WIDE_STR(const std::wstring wkey, key); @@ -150,12 +150,12 @@ void Configuration::LoadDsn(const std::string& dsn) { void Configuration::Clear() { this->properties_.clear(); } -bool Configuration::IsSet(const std::string_view& key) const { - return 0 != this->properties_.count(std::string(key)); +bool Configuration::IsSet(std::string_view key) const { + return 0 != this->properties_.count(key); } -const std::string& Configuration::Get(const std::string_view& key) const { - const auto itr = this->properties_.find(std::string(key)); +const std::string& Configuration::Get(std::string_view key) const { + const auto itr = this->properties_.find(key); if (itr == this->properties_.cend()) { static const std::string empty(""); return empty; @@ -163,23 +163,22 @@ const std::string& Configuration::Get(const std::string_view& key) const { return itr->second; } -void Configuration::Set(const std::string_view& key, const std::wstring& wvalue) { +void Configuration::Set(std::string_view key, const std::wstring& wvalue) { CONVERT_UTF8_STR(const std::string value, wvalue); Set(key, value); } -void Configuration::Set(const std::string_view& key, const std::string& value) { +void Configuration::Set(std::string_view key, const std::string& value) { const std::string copy = boost::trim_copy(value); if (!copy.empty()) { - this->properties_[std::string(key)] = value; + this->properties_[key] = value; } } -void Configuration::Emplace(const std::string_view& key, std::string&& value) { +void Configuration::Emplace(std::string_view key, std::string&& value) { const std::string copy = boost::trim_copy(value); if (!copy.empty()) { - this->properties_.emplace( - std::make_pair(std::move(std::string(key)), std::move(value))); + this->properties_.emplace(std::make_pair(key, std::move(value))); } } diff --git a/cpp/src/arrow/flight/sql/odbc/odbc_impl/config/configuration.h b/cpp/src/arrow/flight/sql/odbc/odbc_impl/config/configuration.h index 56d8bac6173..0390a57e52f 100644 --- a/cpp/src/arrow/flight/sql/odbc/odbc_impl/config/configuration.h +++ b/cpp/src/arrow/flight/sql/odbc/odbc_impl/config/configuration.h @@ -50,11 +50,11 @@ class Configuration { void LoadDsn(const std::string& dsn); void Clear(); - bool IsSet(const std::string_view& key) const; - const std::string& Get(const std::string_view& key) const; - void Set(const std::string_view& key, const std::wstring& wvalue); - void Set(const std::string_view& key, const std::string& value); - void Emplace(const std::string_view& key, std::string&& value); + bool IsSet(std::string_view key) const; + const std::string& Get(std::string_view key) const; + void Set(std::string_view key, const std::wstring& wvalue); + void Set(std::string_view key, const std::string& value); + void Emplace(std::string_view key, std::string&& value); /** * Get properties map. */ diff --git a/cpp/src/arrow/flight/sql/odbc/odbc_impl/encoding_utils.h b/cpp/src/arrow/flight/sql/odbc/odbc_impl/encoding_utils.h index 5c65eedd6fa..aac11f4e231 100644 --- a/cpp/src/arrow/flight/sql/odbc/odbc_impl/encoding_utils.h +++ b/cpp/src/arrow/flight/sql/odbc/odbc_impl/encoding_utils.h @@ -39,7 +39,7 @@ using arrow::flight::sql::odbc::WcsToUtf8; // Return the number of bytes required for the conversion. template -inline size_t ConvertToSqlWChar(const std::string_view& str, SQLWCHAR* buffer, +inline size_t ConvertToSqlWChar(std::string_view str, SQLWCHAR* buffer, SQLLEN buffer_size_in_bytes) { thread_local std::vector wstr; Utf8ToWcs(str.data(), str.size(), &wstr); @@ -66,7 +66,7 @@ inline size_t ConvertToSqlWChar(const std::string_view& str, SQLWCHAR* buffer, return value_length_in_bytes; } -inline size_t ConvertToSqlWChar(const std::string_view& str, SQLWCHAR* buffer, +inline size_t ConvertToSqlWChar(std::string_view str, SQLWCHAR* buffer, SQLLEN buffer_size_in_bytes) { switch (GetSqlWCharSize()) { case sizeof(char16_t): diff --git a/cpp/src/arrow/flight/sql/odbc/odbc_impl/flight_sql_auth_method.cc b/cpp/src/arrow/flight/sql/odbc/odbc_impl/flight_sql_auth_method.cc index 5bfa22dcb98..bdf7f71589c 100644 --- a/cpp/src/arrow/flight/sql/odbc/odbc_impl/flight_sql_auth_method.cc +++ b/cpp/src/arrow/flight/sql/odbc/odbc_impl/flight_sql_auth_method.cc @@ -142,22 +142,22 @@ std::unique_ptr FlightSqlAuthMethod::FromProperties( const std::unique_ptr& client, const Connection::ConnPropertyMap& properties) { // Check if should use user-password authentication - auto it_user = properties.find(std::string(FlightSqlConnection::USER)); + auto it_user = properties.find(FlightSqlConnection::USER); if (it_user == properties.end()) { // The Microsoft OLE DB to ODBC bridge provider (MSDASQL) will write // "User ID" and "Password" properties instead of mapping // to ODBC compliant UID/PWD keys. - it_user = properties.find(std::string(FlightSqlConnection::USER_ID)); + it_user = properties.find(FlightSqlConnection::USER_ID); } - auto it_password = properties.find(std::string(FlightSqlConnection::PASSWORD)); - auto it_token = properties.find(std::string(FlightSqlConnection::TOKEN)); + auto it_password = properties.find(FlightSqlConnection::PASSWORD); + auto it_token = properties.find(FlightSqlConnection::TOKEN); if (it_user == properties.end() || it_password == properties.end()) { // Accept UID/PWD as aliases for User/Password. These are suggested as // standard properties in the documentation for SQLDriverConnect. - it_user = properties.find(std::string(FlightSqlConnection::UID)); - it_password = properties.find(std::string(FlightSqlConnection::PWD)); + it_user = properties.find(FlightSqlConnection::UID); + it_password = properties.find(FlightSqlConnection::PWD); } if (it_user != properties.end() || it_password != properties.end()) { const std::string& user = it_user != properties.end() ? it_user->second : ""; diff --git a/cpp/src/arrow/flight/sql/odbc/odbc_impl/flight_sql_connection.cc b/cpp/src/arrow/flight/sql/odbc/odbc_impl/flight_sql_connection.cc index 0cc7e56b247..e18a58d069f 100644 --- a/cpp/src/arrow/flight/sql/odbc/odbc_impl/flight_sql_connection.cc +++ b/cpp/src/arrow/flight/sql/odbc/odbc_impl/flight_sql_connection.cc @@ -99,6 +99,13 @@ inline std::string GetCerts() { return ""; } #endif +// Case insensitive comparator that takes string_view +struct CaseInsensitiveComparatorStrView { + bool operator()(std::string_view s1, std::string_view s2) const { + return boost::lexicographical_compare(s1, s2, boost::is_iless()); + } +}; + const std::set BUILT_IN_PROPERTIES = { FlightSqlConnection::HOST, FlightSqlConnection::PORT, @@ -116,9 +123,9 @@ const std::set BUILT_IN_PROP FlightSqlConnection::USE_WIDE_CHAR}; Connection::ConnPropertyMap::const_iterator TrackMissingRequiredProperty( - const std::string_view& property, const Connection::ConnPropertyMap& properties, + std::string_view property, const Connection::ConnPropertyMap& properties, std::vector& missing_attr) { - auto prop_iter = properties.find(std::string(property)); + auto prop_iter = properties.find(property); if (properties.end() == prop_iter) { missing_attr.push_back(property); } diff --git a/cpp/src/arrow/flight/sql/odbc/odbc_impl/flight_sql_connection.h b/cpp/src/arrow/flight/sql/odbc/odbc_impl/flight_sql_connection.h index 6a35e59a6df..6219bb287e4 100644 --- a/cpp/src/arrow/flight/sql/odbc/odbc_impl/flight_sql_connection.h +++ b/cpp/src/arrow/flight/sql/odbc/odbc_impl/flight_sql_connection.h @@ -28,13 +28,6 @@ namespace arrow::flight::sql::odbc { -/// \brief Case insensitive comparator that takes string_view -struct CaseInsensitiveComparatorStrView { - bool operator()(const std::string_view& s1, const std::string_view& s2) const { - return boost::lexicographical_compare(s1, s2, boost::is_iless()); - } -}; - class FlightSqlSslConfig; /// \brief Create an instance of the FlightSqlSslConfig class, from the properties passed diff --git a/cpp/src/arrow/flight/sql/odbc/odbc_impl/flight_sql_connection_test.cc b/cpp/src/arrow/flight/sql/odbc/odbc_impl/flight_sql_connection_test.cc index d14486d7508..a3fdcfc0387 100644 --- a/cpp/src/arrow/flight/sql/odbc/odbc_impl/flight_sql_connection_test.cc +++ b/cpp/src/arrow/flight/sql/odbc/odbc_impl/flight_sql_connection_test.cc @@ -65,12 +65,10 @@ TEST(MetadataSettingsTest, StringColumnLengthTest) { const int32_t expected_string_column_length = 100000; const Connection::ConnPropertyMap properties = { - {std::string(FlightSqlConnection::HOST), - std::string("localhost")}, // expect not used - {std::string(FlightSqlConnection::PORT), std::string("32010")}, // expect not used - {std::string(FlightSqlConnection::USE_ENCRYPTION), - std::string("false")}, // expect not used - {std::string(FlightSqlConnection::STRING_COLUMN_LENGTH), + {FlightSqlConnection::HOST, "localhost"}, // expect not used + {FlightSqlConnection::PORT, "32010"}, // expect not used + {FlightSqlConnection::USE_ENCRYPTION, "false"}, // expect not used + {FlightSqlConnection::STRING_COLUMN_LENGTH, std::to_string(expected_string_column_length)}, }; @@ -88,10 +86,10 @@ TEST(MetadataSettingsTest, UseWideCharTest) { connection.SetClosed(false); const Connection::ConnPropertyMap properties1 = { - {std::string(FlightSqlConnection::USE_WIDE_CHAR), std::string("true")}, + {FlightSqlConnection::USE_WIDE_CHAR, "true"}, }; const Connection::ConnPropertyMap properties2 = { - {std::string(FlightSqlConnection::USE_WIDE_CHAR), std::string("false")}, + {FlightSqlConnection::USE_WIDE_CHAR, "false"}, }; EXPECT_EQ(true, connection.GetUseWideChar(properties1)); @@ -103,9 +101,9 @@ TEST(MetadataSettingsTest, UseWideCharTest) { TEST(BuildLocationTests, ForTcp) { std::vector missing_attr; Connection::ConnPropertyMap properties = { - {std::string(FlightSqlConnection::HOST), std::string("localhost")}, - {std::string(FlightSqlConnection::PORT), std::string("32010")}, - {std::string(FlightSqlConnection::USE_ENCRYPTION), std::string("false")}, + {FlightSqlConnection::HOST, "localhost"}, + {FlightSqlConnection::PORT, "32010"}, + {FlightSqlConnection::USE_ENCRYPTION, "false"}, }; const std::shared_ptr& ssl_config = @@ -115,8 +113,8 @@ TEST(BuildLocationTests, ForTcp) { FlightSqlConnection::BuildLocation(properties, missing_attr, ssl_config); const Location& actual_location2 = FlightSqlConnection::BuildLocation( { - {std::string(FlightSqlConnection::HOST), std::string("localhost")}, - {std::string(FlightSqlConnection::PORT), std::string("32011")}, + {FlightSqlConnection::HOST, "localhost"}, + {FlightSqlConnection::PORT, "32011"}, }, missing_attr, ssl_config); @@ -129,9 +127,9 @@ TEST(BuildLocationTests, ForTcp) { TEST(BuildLocationTests, ForTls) { std::vector missing_attr; Connection::ConnPropertyMap properties = { - {std::string(FlightSqlConnection::HOST), std::string("localhost")}, - {std::string(FlightSqlConnection::PORT), std::string("32010")}, - {std::string(FlightSqlConnection::USE_ENCRYPTION), std::string("1")}, + {FlightSqlConnection::HOST, "localhost"}, + {FlightSqlConnection::PORT, "32010"}, + {FlightSqlConnection::USE_ENCRYPTION, "1"}, }; const std::shared_ptr& ssl_config = @@ -141,9 +139,9 @@ TEST(BuildLocationTests, ForTls) { FlightSqlConnection::BuildLocation(properties, missing_attr, ssl_config); Connection::ConnPropertyMap second_properties = { - {std::string(FlightSqlConnection::HOST), std::string("localhost")}, - {std::string(FlightSqlConnection::PORT), std::string("32011")}, - {std::string(FlightSqlConnection::USE_ENCRYPTION), std::string("1")}, + {FlightSqlConnection::HOST, "localhost"}, + {FlightSqlConnection::PORT, "32011"}, + {FlightSqlConnection::USE_ENCRYPTION, "1"}, }; const std::shared_ptr& second_ssl_config = diff --git a/cpp/src/arrow/flight/sql/odbc/odbc_impl/main.cc b/cpp/src/arrow/flight/sql/odbc/odbc_impl/main.cc index 4d9277a0cc9..63687f80920 100644 --- a/cpp/src/arrow/flight/sql/odbc/odbc_impl/main.cc +++ b/cpp/src/arrow/flight/sql/odbc/odbc_impl/main.cc @@ -202,11 +202,11 @@ int main() { driver.CreateConnection(arrow::flight::sql::odbc::OdbcVersion::V_3); Connection::ConnPropertyMap properties = { - {std::string(FlightSqlConnection::HOST), std::string("automaster.apache")}, - {std::string(FlightSqlConnection::PORT), std::string("32010")}, - {std::string(FlightSqlConnection::USER), std::string("apache")}, - {std::string(FlightSqlConnection::PASSWORD), std::string("apache123")}, - {std::string(FlightSqlConnection::USE_ENCRYPTION), std::string("false")}, + {FlightSqlConnection::HOST, "automaster.apache"}, + {FlightSqlConnection::PORT, "32010"}, + {FlightSqlConnection::USER, "apache"}, + {FlightSqlConnection::PASSWORD, "apache123"}, + {FlightSqlConnection::USE_ENCRYPTION, "false"}, }; std::vector missing_attr; connection->Connect(properties, missing_attr); diff --git a/cpp/src/arrow/flight/sql/odbc/odbc_impl/spi/connection.h b/cpp/src/arrow/flight/sql/odbc/odbc_impl/spi/connection.h index ec0e3e727ee..29c10bf42be 100644 --- a/cpp/src/arrow/flight/sql/odbc/odbc_impl/spi/connection.h +++ b/cpp/src/arrow/flight/sql/odbc/odbc_impl/spi/connection.h @@ -32,13 +32,15 @@ namespace arrow::flight::sql::odbc { /// \brief Case insensitive comparator struct CaseInsensitiveComparator { - bool operator()(const std::string& s1, const std::string& s2) const { + using is_transparent = std::true_type; + + bool operator()(std::string_view s1, std::string_view s2) const { return boost::lexicographical_compare(s1, s2, boost::is_iless()); } }; // PropertyMap is case-insensitive for keys. -typedef std::map PropertyMap; +typedef std::map PropertyMap; class Statement; diff --git a/cpp/src/arrow/flight/sql/odbc/odbc_impl/system_dsn.cc b/cpp/src/arrow/flight/sql/odbc/odbc_impl/system_dsn.cc index fd77fbf50f7..468f05e4cf4 100644 --- a/cpp/src/arrow/flight/sql/odbc/odbc_impl/system_dsn.cc +++ b/cpp/src/arrow/flight/sql/odbc/odbc_impl/system_dsn.cc @@ -96,7 +96,7 @@ bool RegisterDsn(const Configuration& config, LPCWSTR driver) { const auto& map = config.GetProperties(); for (auto it = map.begin(); it != map.end(); ++it) { - const std::string_view& key = it->first; + std::string_view key = it->first; if (boost::iequals(FlightSqlConnection::DSN, key) || boost::iequals(FlightSqlConnection::DRIVER, key)) { continue; diff --git a/cpp/src/arrow/flight/sql/odbc/odbc_impl/util.cc b/cpp/src/arrow/flight/sql/odbc/odbc_impl/util.cc index df6aff9cfa7..59ee7dda565 100644 --- a/cpp/src/arrow/flight/sql/odbc/odbc_impl/util.cc +++ b/cpp/src/arrow/flight/sql/odbc/odbc_impl/util.cc @@ -1108,8 +1108,8 @@ boost::optional AsBool(const std::string& value) { } boost::optional AsBool(const Connection::ConnPropertyMap& conn_property_map, - const std::string_view& property_name) { - auto extracted_property = conn_property_map.find(std::string(property_name)); + std::string_view property_name) { + auto extracted_property = conn_property_map.find(property_name); if (extracted_property != conn_property_map.end()) { return AsBool(extracted_property->second); @@ -1120,8 +1120,8 @@ boost::optional AsBool(const Connection::ConnPropertyMap& conn_property_ma boost::optional AsInt32(int32_t min_value, const Connection::ConnPropertyMap& conn_property_map, - const std::string_view& property_name) { - auto extracted_property = conn_property_map.find(std::string(property_name)); + std::string_view property_name) { + auto extracted_property = conn_property_map.find(property_name); if (extracted_property != conn_property_map.end()) { const int32_t string_column_length = std::stoi(extracted_property->second); diff --git a/cpp/src/arrow/flight/sql/odbc/odbc_impl/util.h b/cpp/src/arrow/flight/sql/odbc/odbc_impl/util.h index 8197f741d1e..c17e77e7de8 100644 --- a/cpp/src/arrow/flight/sql/odbc/odbc_impl/util.h +++ b/cpp/src/arrow/flight/sql/odbc/odbc_impl/util.h @@ -144,7 +144,7 @@ boost::optional AsBool(const std::string& value); /// \param property_name the name of the property that will be looked up. /// \return the parsed valued. boost::optional AsBool(const Connection::ConnPropertyMap& conn_property_map, - const std::string_view& property_name); + std::string_view property_name); /// Looks up for a value inside the ConnPropertyMap and then try to parse it. /// In case it does not find or it cannot parse, the default value will be returned. @@ -156,7 +156,7 @@ boost::optional AsBool(const Connection::ConnPropertyMap& conn_property_ma /// std::out_of_range exception from std::stoi boost::optional AsInt32(int32_t min_value, const Connection::ConnPropertyMap& conn_property_map, - const std::string_view& property_name); + std::string_view property_name); } // namespace util } // namespace arrow::flight::sql::odbc diff --git a/cpp/src/arrow/flight/sql/odbc/tests/connection_test.cc b/cpp/src/arrow/flight/sql/odbc/tests/connection_test.cc index 3eff87a5778..531250b69b8 100644 --- a/cpp/src/arrow/flight/sql/odbc/tests/connection_test.cc +++ b/cpp/src/arrow/flight/sql/odbc/tests/connection_test.cc @@ -365,11 +365,11 @@ TEST_F(ConnectionRemoteTest, TestSQLConnectInvalidUid) { // Retrieve valid uid and pwd, assumes TEST_CONNECT_STR contains uid and pwd Connection::ConnPropertyMap properties; ODBC::ODBCConnection::GetPropertiesFromConnString(connect_str, properties); - std::string uid = properties[std::string("uid")]; - std::string pwd = properties[std::string("pwd")]; + std::string uid = properties["uid"]; + std::string pwd = properties["pwd"]; // Append invalid uid to connection string - connect_str += std::string("uid=non_existent_id;"); + connect_str += "uid=non_existent_id;"; // Write connection string content into a DSN, // must succeed before continuing From 58cb05a1add4908b4a40043c23dd46925cfa0daf Mon Sep 17 00:00:00 2001 From: justing-bq <62349012+justing-bq@users.noreply.github.com> Date: Fri, 7 Nov 2025 13:01:29 -0800 Subject: [PATCH 08/11] Address remaining feedback --- cpp/src/arrow/flight/sql/odbc/odbc_api.cc | 18 ++++++++++-------- .../sql/odbc/odbc_impl/attribute_utils.h | 1 + .../flight/sql/odbc/odbc_impl/encoding_utils.h | 2 +- .../sql/odbc/odbc_impl/odbc_connection.cc | 4 ++-- .../sql/odbc/odbc_impl/odbc_connection.h | 6 +++--- .../flight/sql/odbc/odbc_impl/system_dsn.h | 2 +- 6 files changed, 18 insertions(+), 15 deletions(-) diff --git a/cpp/src/arrow/flight/sql/odbc/odbc_api.cc b/cpp/src/arrow/flight/sql/odbc/odbc_api.cc index 685977167db..a028e063b34 100644 --- a/cpp/src/arrow/flight/sql/odbc/odbc_api.cc +++ b/cpp/src/arrow/flight/sql/odbc/odbc_api.cc @@ -34,7 +34,7 @@ #if defined _WIN32 // For displaying DSN Window # include "arrow/flight/sql/odbc/odbc_impl/system_dsn.h" -#endif +#endif // defined(_WIN32) namespace arrow::flight::sql::odbc { SQLRETURN SQLAllocHandle(SQLSMALLINT type, SQLHANDLE parent, SQLHANDLE* result) { @@ -780,9 +780,11 @@ SQLRETURN SQLDriverConnect(SQLHDBC conn, SQLHWND window_handle, std::string connection_string = ODBC::SqlWcharToString(in_connection_string, in_connection_string_len); Connection::ConnPropertyMap properties; - std::string dsn = ODBCConnection::GetDsnIfExists(connection_string); - if (!dsn.empty()) { - LoadPropertiesFromDSN(dsn, properties); + std::string dsn_value = ""; + std::optional dsn = ODBCConnection::GetDsnIfExists(connection_string); + if (dsn.has_value()) { + dsn_value = dsn.value(); + LoadPropertiesFromDSN(dsn_value, properties); } ODBCConnection::GetPropertiesFromConnString(connection_string, properties); @@ -798,11 +800,11 @@ SQLRETURN SQLDriverConnect(SQLHDBC conn, SQLHWND window_handle, if (!DisplayConnectionWindow(window_handle, config, properties)) { return static_cast(SQL_NO_DATA); } - connection->Connect(dsn, properties, missing_properties); + connection->Connect(dsn_value, properties, missing_properties); } else if (driver_completion == SQL_DRIVER_COMPLETE || driver_completion == SQL_DRIVER_COMPLETE_REQUIRED) { try { - connection->Connect(dsn, properties, missing_properties); + connection->Connect(dsn_value, properties, missing_properties); } catch (const DriverException&) { // If first connection fails due to missing attributes, load // the DSN window and try to connect again @@ -813,14 +815,14 @@ SQLRETURN SQLDriverConnect(SQLHDBC conn, SQLHWND window_handle, if (!DisplayConnectionWindow(window_handle, config, properties)) { return static_cast(SQL_NO_DATA); } - connection->Connect(dsn, properties, missing_properties); + connection->Connect(dsn_value, properties, missing_properties); } else { throw; } } } else { // Default case: attempt connection without showing DSN window - connection->Connect(dsn, properties, missing_properties); + connection->Connect(dsn_value, properties, missing_properties); } #else // Attempt connection without loading DSN window on macOS/Linux diff --git a/cpp/src/arrow/flight/sql/odbc/odbc_impl/attribute_utils.h b/cpp/src/arrow/flight/sql/odbc/odbc_impl/attribute_utils.h index f163545f17e..8c5eae59f7e 100644 --- a/cpp/src/arrow/flight/sql/odbc/odbc_impl/attribute_utils.h +++ b/cpp/src/arrow/flight/sql/odbc/odbc_impl/attribute_utils.h @@ -27,6 +27,7 @@ #include "arrow/flight/sql/odbc/odbc_impl/exceptions.h" #include "arrow/flight/sql/odbc/odbc_impl/platform.h" +// GH-48083 TODO: replace `namespace ODBC` with `namespace arrow::flight::sql::odbc` namespace ODBC { using arrow::flight::sql::odbc::Diagnostics; diff --git a/cpp/src/arrow/flight/sql/odbc/odbc_impl/encoding_utils.h b/cpp/src/arrow/flight/sql/odbc/odbc_impl/encoding_utils.h index aac11f4e231..f68c4b08342 100644 --- a/cpp/src/arrow/flight/sql/odbc/odbc_impl/encoding_utils.h +++ b/cpp/src/arrow/flight/sql/odbc/odbc_impl/encoding_utils.h @@ -85,7 +85,7 @@ inline size_t ConvertToSqlWChar(std::string_view str, SQLWCHAR* buffer, /// \param[in] msg_len Number of characters in wchar_msg /// \return wchar_msg in std::string format inline std::string SqlWcharToString(SQLWCHAR* wchar_msg, SQLINTEGER msg_len = SQL_NTS) { - if (!wchar_msg || wchar_msg[0] == 0 || msg_len == 0) { + if (msg_len == 0 || !wchar_msg || wchar_msg[0] == 0) { return std::string(); } diff --git a/cpp/src/arrow/flight/sql/odbc/odbc_impl/odbc_connection.cc b/cpp/src/arrow/flight/sql/odbc/odbc_impl/odbc_connection.cc index cc183e5e6b5..ead2beada4b 100644 --- a/cpp/src/arrow/flight/sql/odbc/odbc_impl/odbc_connection.cc +++ b/cpp/src/arrow/flight/sql/odbc/odbc_impl/odbc_connection.cc @@ -684,7 +684,7 @@ void ODBCConnection::DropDescriptor(ODBCDescriptor* desc) { // Public Static // =================================================================================== -std::string ODBCConnection::GetDsnIfExists(const std::string& conn_str) { +std::optional ODBCConnection::GetDsnIfExists(const std::string& conn_str) { const int groups[] = {1, 2}; // CONNECTION_STR_REGEX has two groups. key: 1, value: 2 boost::xpressive::sregex_token_iterator regex_iter(conn_str.begin(), conn_str.end(), CONNECTION_STR_REGEX, groups), @@ -703,7 +703,7 @@ std::string ODBCConnection::GetDsnIfExists(const std::string& conn_str) { if (boost::iequals(key, "DSN")) { return value; } else if (boost::iequals(key, "Driver")) { - return std::string(""); + return std::nullopt; } else { throw DriverException( "Connection string is faulty. The first key should be DSN or Driver.", "HY000"); diff --git a/cpp/src/arrow/flight/sql/odbc/odbc_impl/odbc_connection.h b/cpp/src/arrow/flight/sql/odbc/odbc_impl/odbc_connection.h index 6f793cd0111..2e5ab57ad49 100644 --- a/cpp/src/arrow/flight/sql/odbc/odbc_impl/odbc_connection.h +++ b/cpp/src/arrow/flight/sql/odbc/odbc_impl/odbc_connection.h @@ -23,6 +23,7 @@ #include #include #include +#include #include namespace ODBC { @@ -75,9 +76,8 @@ class ODBCConnection : public ODBCHandle { inline bool IsOdbc2Connection() const { return is_2x_connection_; } - /// @return the DSN or an empty string if the DSN is not found or is found after the - /// driver - static std::string GetDsnIfExists(const std::string& conn_str); + /// \return an optional DSN + static std::optional GetDsnIfExists(const std::string& conn_str); /// Read properties from connection string, but does not read values from DSN static void GetPropertiesFromConnString( diff --git a/cpp/src/arrow/flight/sql/odbc/odbc_impl/system_dsn.h b/cpp/src/arrow/flight/sql/odbc/odbc_impl/system_dsn.h index 78dbd51c2e2..5d23c3dfcaf 100644 --- a/cpp/src/arrow/flight/sql/odbc/odbc_impl/system_dsn.h +++ b/cpp/src/arrow/flight/sql/odbc/odbc_impl/system_dsn.h @@ -19,7 +19,7 @@ #include "arrow/flight/sql/odbc/odbc_impl/platform.h" #include "arrow/flight/sql/odbc/odbc_impl/config/configuration.h" -#include "arrow/result.h" +#include "arrow/status.h" namespace arrow::flight::sql::odbc { From e06a5e096238bd07f4e298ce29ddc9cb7d0d4634 Mon Sep 17 00:00:00 2001 From: "Alina (Xi) Li" Date: Mon, 10 Nov 2025 15:07:28 -0800 Subject: [PATCH 09/11] Add braces --- cpp/src/arrow/flight/sql/odbc/odbc_impl/encoding_utils.h | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/cpp/src/arrow/flight/sql/odbc/odbc_impl/encoding_utils.h b/cpp/src/arrow/flight/sql/odbc/odbc_impl/encoding_utils.h index f68c4b08342..66e5c3bf0d8 100644 --- a/cpp/src/arrow/flight/sql/odbc/odbc_impl/encoding_utils.h +++ b/cpp/src/arrow/flight/sql/odbc/odbc_impl/encoding_utils.h @@ -106,12 +106,15 @@ inline std::string SqlStringToString(const unsigned char* sql_str, const char* sql_str_c = reinterpret_cast(sql_str); - if (!sql_str) return res; + if (!sql_str) { + return res; + } - if (sql_str_len == SQL_NTS) + if (sql_str_len == SQL_NTS) { res.assign(sql_str_c); - else if (sql_str_len > 0) + } else if (sql_str_len > 0) { res.assign(sql_str_c, sql_str_len); + } return res; } From cfcc956e5052811377c411b0d5dfa789638897d3 Mon Sep 17 00:00:00 2001 From: "Alina (Xi) Li" Date: Mon, 10 Nov 2025 15:37:48 -0800 Subject: [PATCH 10/11] Change map to use std::string std::string_view doesn't work well with std::map because once the std::string_view object is out of scope, the key is no longer valid. The map then has undefined behavior. Enabling transparent comparing only helps with the `find` function, not inserts. For inserts we still need to use a std::string constructor. Ran ODBC tests on MSVC locally and they passed. --- .../odbc/odbc_impl/config/configuration.cc | 2 +- .../odbc_impl/flight_sql_connection_test.cc | 34 +++++++++---------- .../arrow/flight/sql/odbc/odbc_impl/main.cc | 10 +++--- .../sql/odbc/odbc_impl/spi/connection.h | 2 +- 4 files changed, 24 insertions(+), 24 deletions(-) diff --git a/cpp/src/arrow/flight/sql/odbc/odbc_impl/config/configuration.cc b/cpp/src/arrow/flight/sql/odbc/odbc_impl/config/configuration.cc index 7eea7fe9ded..df61f1247c7 100644 --- a/cpp/src/arrow/flight/sql/odbc/odbc_impl/config/configuration.cc +++ b/cpp/src/arrow/flight/sql/odbc/odbc_impl/config/configuration.cc @@ -171,7 +171,7 @@ void Configuration::Set(std::string_view key, const std::wstring& wvalue) { void Configuration::Set(std::string_view key, const std::string& value) { const std::string copy = boost::trim_copy(value); if (!copy.empty()) { - this->properties_[key] = value; + this->properties_[std::string(key)] = value; } } diff --git a/cpp/src/arrow/flight/sql/odbc/odbc_impl/flight_sql_connection_test.cc b/cpp/src/arrow/flight/sql/odbc/odbc_impl/flight_sql_connection_test.cc index a3fdcfc0387..9c9b0f8f3c1 100644 --- a/cpp/src/arrow/flight/sql/odbc/odbc_impl/flight_sql_connection_test.cc +++ b/cpp/src/arrow/flight/sql/odbc/odbc_impl/flight_sql_connection_test.cc @@ -65,10 +65,10 @@ TEST(MetadataSettingsTest, StringColumnLengthTest) { const int32_t expected_string_column_length = 100000; const Connection::ConnPropertyMap properties = { - {FlightSqlConnection::HOST, "localhost"}, // expect not used - {FlightSqlConnection::PORT, "32010"}, // expect not used - {FlightSqlConnection::USE_ENCRYPTION, "false"}, // expect not used - {FlightSqlConnection::STRING_COLUMN_LENGTH, + {std::string(FlightSqlConnection::HOST), "localhost"}, // expect not used + {std::string(FlightSqlConnection::PORT), "32010"}, // expect not used + {std::string(FlightSqlConnection::USE_ENCRYPTION), "false"}, // expect not used + {std::string(FlightSqlConnection::STRING_COLUMN_LENGTH), std::to_string(expected_string_column_length)}, }; @@ -86,10 +86,10 @@ TEST(MetadataSettingsTest, UseWideCharTest) { connection.SetClosed(false); const Connection::ConnPropertyMap properties1 = { - {FlightSqlConnection::USE_WIDE_CHAR, "true"}, + {std::string(FlightSqlConnection::USE_WIDE_CHAR), "true"}, }; const Connection::ConnPropertyMap properties2 = { - {FlightSqlConnection::USE_WIDE_CHAR, "false"}, + {std::string(FlightSqlConnection::USE_WIDE_CHAR), "false"}, }; EXPECT_EQ(true, connection.GetUseWideChar(properties1)); @@ -101,9 +101,9 @@ TEST(MetadataSettingsTest, UseWideCharTest) { TEST(BuildLocationTests, ForTcp) { std::vector missing_attr; Connection::ConnPropertyMap properties = { - {FlightSqlConnection::HOST, "localhost"}, - {FlightSqlConnection::PORT, "32010"}, - {FlightSqlConnection::USE_ENCRYPTION, "false"}, + {std::string(FlightSqlConnection::HOST), "localhost"}, + {std::string(FlightSqlConnection::PORT), "32010"}, + {std::string(FlightSqlConnection::USE_ENCRYPTION), "false"}, }; const std::shared_ptr& ssl_config = @@ -113,8 +113,8 @@ TEST(BuildLocationTests, ForTcp) { FlightSqlConnection::BuildLocation(properties, missing_attr, ssl_config); const Location& actual_location2 = FlightSqlConnection::BuildLocation( { - {FlightSqlConnection::HOST, "localhost"}, - {FlightSqlConnection::PORT, "32011"}, + {std::string(FlightSqlConnection::HOST), "localhost"}, + {std::string(FlightSqlConnection::PORT), "32011"}, }, missing_attr, ssl_config); @@ -127,9 +127,9 @@ TEST(BuildLocationTests, ForTcp) { TEST(BuildLocationTests, ForTls) { std::vector missing_attr; Connection::ConnPropertyMap properties = { - {FlightSqlConnection::HOST, "localhost"}, - {FlightSqlConnection::PORT, "32010"}, - {FlightSqlConnection::USE_ENCRYPTION, "1"}, + {std::string(FlightSqlConnection::HOST), "localhost"}, + {std::string(FlightSqlConnection::PORT), "32010"}, + {std::string(FlightSqlConnection::USE_ENCRYPTION), "1"}, }; const std::shared_ptr& ssl_config = @@ -139,9 +139,9 @@ TEST(BuildLocationTests, ForTls) { FlightSqlConnection::BuildLocation(properties, missing_attr, ssl_config); Connection::ConnPropertyMap second_properties = { - {FlightSqlConnection::HOST, "localhost"}, - {FlightSqlConnection::PORT, "32011"}, - {FlightSqlConnection::USE_ENCRYPTION, "1"}, + {std::string(FlightSqlConnection::HOST), "localhost"}, + {std::string(FlightSqlConnection::PORT), "32011"}, + {std::string(FlightSqlConnection::USE_ENCRYPTION), "1"}, }; const std::shared_ptr& second_ssl_config = diff --git a/cpp/src/arrow/flight/sql/odbc/odbc_impl/main.cc b/cpp/src/arrow/flight/sql/odbc/odbc_impl/main.cc index 63687f80920..3336e0160e1 100644 --- a/cpp/src/arrow/flight/sql/odbc/odbc_impl/main.cc +++ b/cpp/src/arrow/flight/sql/odbc/odbc_impl/main.cc @@ -202,11 +202,11 @@ int main() { driver.CreateConnection(arrow::flight::sql::odbc::OdbcVersion::V_3); Connection::ConnPropertyMap properties = { - {FlightSqlConnection::HOST, "automaster.apache"}, - {FlightSqlConnection::PORT, "32010"}, - {FlightSqlConnection::USER, "apache"}, - {FlightSqlConnection::PASSWORD, "apache123"}, - {FlightSqlConnection::USE_ENCRYPTION, "false"}, + {std::string(FlightSqlConnection::HOST), "automaster.apache"}, + {std::string(FlightSqlConnection::PORT), "32010"}, + {std::string(FlightSqlConnection::USER), "apache"}, + {std::string(FlightSqlConnection::PASSWORD), "apache123"}, + {std::string(FlightSqlConnection::USE_ENCRYPTION), "false"}, }; std::vector missing_attr; connection->Connect(properties, missing_attr); diff --git a/cpp/src/arrow/flight/sql/odbc/odbc_impl/spi/connection.h b/cpp/src/arrow/flight/sql/odbc/odbc_impl/spi/connection.h index 29c10bf42be..7a8243e7859 100644 --- a/cpp/src/arrow/flight/sql/odbc/odbc_impl/spi/connection.h +++ b/cpp/src/arrow/flight/sql/odbc/odbc_impl/spi/connection.h @@ -40,7 +40,7 @@ struct CaseInsensitiveComparator { }; // PropertyMap is case-insensitive for keys. -typedef std::map PropertyMap; +typedef std::map PropertyMap; class Statement; From f37d7e3c6d78b6ec1767233607a58deeeeb383d7 Mon Sep 17 00:00:00 2001 From: "Alina (Xi) Li" Date: Wed, 12 Nov 2025 13:17:44 -0800 Subject: [PATCH 11/11] Empty commit to trigger CI for