Address code review comments

Co-Authored-By: justing-bq <[email protected]>

Author: alinaliBQ

cpp/src/arrow/flight/sql/odbc/odbc_api.cc

@@ -101,7 +101,6 @@ SQLRETURN SQLGetEnvAttr(SQLHENV env, SQLINTEGER attr, SQLPOINTER value_ptr,
                    << ", value_ptr: " << value_ptr << ", buffer_length: " << buffer_length
                    << ", str_len_ptr: " << static_cast<const void*>(str_len_ptr);
 
-  using driver::odbcabstraction::DriverException;
   using ODBC::ODBCEnvironment;
 
   ODBCEnvironment* environment = reinterpret_cast<ODBCEnvironment*>(env);
@@ -159,7 +158,6 @@ SQLRETURN SQLSetEnvAttr(SQLHENV env, SQLINTEGER attr, SQLPOINTER value_ptr,
   ARROW_LOG(DEBUG) << "SQLSetEnvAttr called with env: " << env << ", attr: " << attr
                    << ", value_ptr: " << value_ptr << ", str_len: " << str_len;
 
-  using driver::odbcabstraction::DriverException;
   using ODBC::ODBCEnvironment;
 
   ODBCEnvironment* environment = reinterpret_cast<ODBCEnvironment*>(env);

cpp/src/arrow/flight/sql/odbc/tests/connection_test.cc

@@ -16,67 +16,137 @@
 // under the License.
 #include "arrow/flight/sql/odbc/tests/odbc_test_suite.h"
 
-#ifdef _WIN32
-#  include <windows.h>
-#endif
+#include "arrow/flight/sql/odbc/odbc_impl/platform.h"
 
 #include <sql.h>
 #include <sqltypes.h>
 #include <sqlucode.h>
 
-#include "gmock/gmock.h"
-#include "gtest/gtest.h"
+#include <gtest/gtest.h>
 
 namespace arrow::flight::sql::odbc {
 
+template <typename T>
+class ConnectionTest : public T {
+ public:
+  using List = std::list<T>;
+};
+
+class ConnectionRemoteTest : public FlightSQLODBCRemoteTestBase {};
+using TestTypes = ::testing::Types<FlightSQLODBCMockTestBase, ConnectionRemoteTest>;
+TYPED_TEST_SUITE(ConnectionTest, TestTypes);
+
 TEST(SQLGetEnvAttr, TestSQLGetEnvAttrODBCVersion) {
-  // ODBC Environment
   SQLHENV env;
 
   SQLINTEGER version;
 
   // Allocate an environment handle
-  SQLRETURN return_env = SQLAllocEnv(&env);
-
-  EXPECT_EQ(SQL_SUCCESS, return_env);
+  ASSERT_EQ(SQL_SUCCESS, SQLAllocEnv(&env));
 
-  SQLRETURN return_get = SQLGetEnvAttr(env, SQL_ATTR_ODBC_VERSION, &version, 0, 0);
+  ASSERT_EQ(SQL_SUCCESS, SQLGetEnvAttr(env, SQL_ATTR_ODBC_VERSION, &version, 0, 0));
 
-  EXPECT_EQ(SQL_SUCCESS, return_get);
+  ASSERT_EQ(SQL_OV_ODBC2, version);
 
-  EXPECT_EQ(SQL_OV_ODBC2, version);
+  ASSERT_EQ(SQL_SUCCESS, SQLFreeEnv(env));
 }
 
 TEST(SQLSetEnvAttr, TestSQLSetEnvAttrODBCVersionValid) {
-  // ODBC Environment
   SQLHENV env;
 
   // Allocate an environment handle
-  SQLRETURN return_env = SQLAllocEnv(&env);
+  ASSERT_EQ(SQL_SUCCESS, SQLAllocEnv(&env));
 
-  EXPECT_EQ(SQL_SUCCESS, return_env);
+  // Attempt to set to supported version
+  ASSERT_EQ(SQL_SUCCESS, SQLSetEnvAttr(env, SQL_ATTR_ODBC_VERSION,
+                                       reinterpret_cast<void*>(SQL_OV_ODBC2), 0));
 
-  // Attempt to set to unsupported version
-  SQLRETURN return_set =
-      SQLSetEnvAttr(env, SQL_ATTR_ODBC_VERSION, reinterpret_cast<void*>(SQL_OV_ODBC2), 0);
+  SQLINTEGER version;
+  // Check ODBC version is set
+  ASSERT_EQ(SQL_SUCCESS, SQLGetEnvAttr(env, SQL_ATTR_ODBC_VERSION, &version, 0, 0));
 
-  EXPECT_EQ(SQL_SUCCESS, return_set);
+  ASSERT_EQ(SQL_OV_ODBC2, version);
+
+  ASSERT_EQ(SQL_SUCCESS, SQLFreeEnv(env));
 }
 
 TEST(SQLSetEnvAttr, TestSQLSetEnvAttrODBCVersionInvalid) {
-  // ODBC Environment
   SQLHENV env;
 
   // Allocate an environment handle
-  SQLRETURN return_env = SQLAllocEnv(&env);
-
-  EXPECT_EQ(SQL_SUCCESS, return_env);
+  ASSERT_EQ(SQL_SUCCESS, SQLAllocEnv(&env));
 
   // Attempt to set to unsupported version
-  SQLRETURN return_set =
-      SQLSetEnvAttr(env, SQL_ATTR_ODBC_VERSION, reinterpret_cast<void*>(1), 0);
+  ASSERT_EQ(SQL_ERROR,
+            SQLSetEnvAttr(env, SQL_ATTR_ODBC_VERSION, reinterpret_cast<void*>(1), 0));
+
+  ASSERT_EQ(SQL_SUCCESS, SQLFreeEnv(env));
+}
+
+TYPED_TEST(ConnectionTest, TestSQLGetEnvAttrOutputNTS) {
+  SQLINTEGER output_nts;
+
+  ASSERT_EQ(SQL_SUCCESS,
+            SQLGetEnvAttr(this->env, SQL_ATTR_OUTPUT_NTS, &output_nts, 0, 0));
+
+  ASSERT_EQ(SQL_TRUE, output_nts);
+}
+
+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
+  SQLINTEGER length;
+  ASSERT_EQ(SQL_SUCCESS,
+            SQLGetEnvAttr(this->env, SQL_ATTR_ODBC_VERSION, nullptr, 0, &length));
+
+  EXPECT_EQ(sizeof(SQLINTEGER), length);
+}
+
+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
+  ASSERT_EQ(SQL_ERROR,
+            SQLGetEnvAttr(this->env, SQL_ATTR_ODBC_VERSION, nullptr, 0, nullptr));
+}
+
+TEST(SQLSetEnvAttr, TestSQLSetEnvAttrOutputNTSValid) {
+  SQLHENV env;
+
+  // Allocate an environment handle
+  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<void*>(SQL_TRUE), 0));
+
+  ASSERT_EQ(SQL_SUCCESS, SQLFreeEnv(env));
+}
+
+TEST(SQLSetEnvAttr, TestSQLSetEnvAttrOutputNTSInvalid) {
+  SQLHENV env;
+
+  // Allocate an environment handle
+  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<void*>(SQL_FALSE), 0));
+
+  ASSERT_EQ(SQL_SUCCESS, SQLFreeEnv(env));
+}
+
+TEST(SQLSetEnvAttr, TestSQLSetEnvAttrNullValuePointer) {
+  SQLHENV env;
+
+  // Allocate an environment handle
+  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));
 
-  EXPECT_EQ(SQL_ERROR, return_set);
+  ASSERT_EQ(SQL_SUCCESS, SQLFreeEnv(env));
 }
 
 }  // namespace arrow::flight::sql::odbc