Skip to content

Commit fd5334a

Browse files
alinaliBQalinaliBQ
committed
Work on code review comments
- rebased onto master Use RAII helper for allocating and freeing env/conn handles
1 parent ef178ca commit fd5334a

File tree

4 files changed

+97
-112
lines changed

4 files changed

+97
-112
lines changed

‎cpp/src/arrow/flight/sql/odbc/tests/CMakeLists.txt‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@ add_arrow_test(flight_sql_odbc_test
3434
SOURCES
3535
odbc_test_suite.cc
3636
odbc_test_suite.h
37+
errors_test.cc
3738
connection_test.cc
3839
# Enable Protobuf cleanup after test execution
3940
# GH-46889: move protobuf_test_util to a more common location

‎cpp/src/arrow/flight/sql/odbc/tests/errors_test.cc‎

Lines changed: 56 additions & 108 deletions
Original file line numberDiff line numberDiff line change
@@ -27,54 +27,42 @@
2727
namespace arrow::flight::sql::odbc {
2828

2929
template <typename T>
30-
class ErrorsTest : public T {
31-
public:
32-
using List = std::list<T>;
33-
};
30+
class ErrorsTest : public T {};
3431

3532
using TestTypes =
3633
::testing::Types<FlightSQLODBCMockTestBase, FlightSQLODBCRemoteTestBase>;
3734
TYPED_TEST_SUITE(ErrorsTest, TestTypes);
3835

3936
template <typename T>
40-
class ErrorsOdbcV2Test : public T {
41-
public:
42-
using List = std::list<T>;
43-
};
37+
class ErrorsOdbcV2Test : public T {};
4438

4539
using TestTypesOdbcV2 =
4640
::testing::Types<FlightSQLOdbcV2MockTestBase, FlightSQLOdbcV2RemoteTestBase>;
4741
TYPED_TEST_SUITE(ErrorsOdbcV2Test, TestTypesOdbcV2);
4842

49-
TYPED_TEST(ErrorsTest, TestSQLGetDiagFieldWForConnectFailure) {
50-
// ODBC Environment
51-
SQLHENV env;
52-
SQLHDBC conn;
53-
54-
// Allocate an environment handle
55-
ASSERT_EQ(SQL_SUCCESS, SQLAllocEnv(&env));
56-
57-
ASSERT_EQ(SQL_SUCCESS,
58-
SQLSetEnvAttr(env, SQL_ATTR_ODBC_VERSION, (void*)SQL_OV_ODBC3, 0));
43+
template <typename T>
44+
class ErrorsHandleTest : public T {};
5945

60-
// Allocate a connection using alloc handle
61-
ASSERT_EQ(SQL_SUCCESS, SQLAllocHandle(SQL_HANDLE_DBC, env, &conn));
46+
using TestTypesHandle = ::testing::Types<FlightSQLOdbcEnvConnHandleMockTestBase,
47+
FlightSQLOdbcEnvConnHandleRemoteTestBase>;
48+
TYPED_TEST_SUITE(ErrorsHandleTest, TestTypesHandle);
6249

50+
TYPED_TEST(ErrorsHandleTest, TestSQLGetDiagFieldWForConnectFailure) {
6351
// Invalid connect string
6452
std::string connect_str = this->GetInvalidConnectionString();
6553

6654
ASSERT_OK_AND_ASSIGN(std::wstring wconnect_str,
6755
arrow::util::UTF8ToWideString(connect_str));
6856
std::vector<SQLWCHAR> connect_str0(wconnect_str.begin(), wconnect_str.end());
6957

70-
SQLWCHAR out_str[ODBC_BUFFER_SIZE];
58+
SQLWCHAR out_str[kOdbcBufferSize];
7159
SQLSMALLINT out_str_len;
7260

7361
// Connecting to ODBC server.
7462
ASSERT_EQ(SQL_ERROR,
75-
SQLDriverConnect(conn, NULL, &connect_str0[0],
63+
SQLDriverConnect(this->conn, NULL, &connect_str0[0],
7664
static_cast<SQLSMALLINT>(connect_str0.size()), out_str,
77-
ODBC_BUFFER_SIZE, &out_str_len, SQL_DRIVER_NOPROMPT));
65+
kOdbcBufferSize, &out_str_len, SQL_DRIVER_NOPROMPT));
7866

7967
// Retrieve all supported header level and record level data
8068
SQLSMALLINT HEADER_LEVEL = 0;
@@ -85,25 +73,25 @@ TYPED_TEST(ErrorsTest, TestSQLGetDiagFieldWForConnectFailure) {
8573
SQLSMALLINT diag_number_length;
8674

8775
EXPECT_EQ(SQL_SUCCESS,
88-
SQLGetDiagField(SQL_HANDLE_DBC, conn, HEADER_LEVEL, SQL_DIAG_NUMBER,
76+
SQLGetDiagField(SQL_HANDLE_DBC, this->conn, HEADER_LEVEL, SQL_DIAG_NUMBER,
8977
&diag_number, sizeof(SQLINTEGER), &diag_number_length));
9078

9179
EXPECT_EQ(1, diag_number);
9280

9381
// SQL_DIAG_SERVER_NAME
94-
SQLWCHAR server_name[ODBC_BUFFER_SIZE];
82+
SQLWCHAR server_name[kOdbcBufferSize];
9583
SQLSMALLINT server_name_length;
9684

9785
EXPECT_EQ(SQL_SUCCESS,
98-
SQLGetDiagField(SQL_HANDLE_DBC, conn, RECORD_1, SQL_DIAG_SERVER_NAME,
86+
SQLGetDiagField(SQL_HANDLE_DBC, this->conn, RECORD_1, SQL_DIAG_SERVER_NAME,
9987
server_name, ODBC_BUFFER_SIZE, &server_name_length));
10088

10189
// SQL_DIAG_MESSAGE_TEXT
102-
SQLWCHAR message_text[ODBC_BUFFER_SIZE];
90+
SQLWCHAR message_text[kOdbcBufferSize];
10391
SQLSMALLINT message_text_length;
10492

10593
EXPECT_EQ(SQL_SUCCESS,
106-
SQLGetDiagField(SQL_HANDLE_DBC, conn, RECORD_1, SQL_DIAG_MESSAGE_TEXT,
94+
SQLGetDiagField(SQL_HANDLE_DBC, this->conn, RECORD_1, SQL_DIAG_MESSAGE_TEXT,
10795
message_text, ODBC_BUFFER_SIZE, &message_text_length));
10896

10997
EXPECT_GT(message_text_length, 100);
@@ -113,8 +101,8 @@ TYPED_TEST(ErrorsTest, TestSQLGetDiagFieldWForConnectFailure) {
113101
SQLSMALLINT diag_native_length;
114102

115103
EXPECT_EQ(SQL_SUCCESS,
116-
SQLGetDiagField(SQL_HANDLE_DBC, conn, RECORD_1, SQL_DIAG_NATIVE, &diag_native,
117-
sizeof(diag_native), &diag_native_length));
104+
SQLGetDiagField(SQL_HANDLE_DBC, this->conn, RECORD_1, SQL_DIAG_NATIVE,
105+
&diag_native, sizeof(diag_native), &diag_native_length));
118106

119107
EXPECT_EQ(200, diag_native);
120108

@@ -123,34 +111,18 @@ TYPED_TEST(ErrorsTest, TestSQLGetDiagFieldWForConnectFailure) {
123111
SQLWCHAR sql_state[sql_state_size];
124112
SQLSMALLINT sql_state_length;
125113

126-
EXPECT_EQ(SQL_SUCCESS,
127-
SQLGetDiagField(SQL_HANDLE_DBC, conn, RECORD_1, SQL_DIAG_SQLSTATE, sql_state,
128-
sql_state_size * arrow::flight::sql::odbc::GetSqlWCharSize(),
129-
&sql_state_length));
114+
EXPECT_EQ(
115+
SQL_SUCCESS,
116+
SQLGetDiagField(SQL_HANDLE_DBC, this->conn, RECORD_1, SQL_DIAG_SQLSTATE, sql_state,
117+
sql_state_size * arrow::flight::sql::odbc::GetSqlWCharSize(),
118+
&sql_state_length));
130119

131120
EXPECT_EQ(std::wstring(L"28000"), std::wstring(sql_state));
132-
133-
// Free connection handle
134-
EXPECT_EQ(SQL_SUCCESS, SQLFreeHandle(SQL_HANDLE_DBC, conn));
135-
136-
// Free environment handle
137-
EXPECT_EQ(SQL_SUCCESS, SQLFreeHandle(SQL_HANDLE_ENV, env));
138121
}
139122

140-
TYPED_TEST(ErrorsTest, DISABLED_TestSQLGetDiagFieldWForConnectFailureNTS) {
123+
TYPED_TEST(ErrorsHandleTest, DISABLED_TestSQLGetDiagFieldWForConnectFailureNTS) {
141124
// Test is disabled because driver manager on Windows does not pass through SQL_NTS
142125
// This test case can be potentially used on macOS/Linux
143-
SQLHENV env;
144-
SQLHDBC conn;
145-
146-
// Allocate an environment handle
147-
ASSERT_EQ(SQL_SUCCESS, SQLAllocEnv(&env));
148-
149-
ASSERT_EQ(SQL_SUCCESS,
150-
SQLSetEnvAttr(env, SQL_ATTR_ODBC_VERSION, (void*)SQL_OV_ODBC3, 0));
151-
152-
// Allocate a connection using alloc handle
153-
ASSERT_EQ(SQL_SUCCESS, SQLAllocHandle(SQL_HANDLE_DBC, env, &conn));
154126

155127
// Invalid connect string
156128
std::string connect_str = this->GetInvalidConnectionString();
@@ -159,35 +131,29 @@ TYPED_TEST(ErrorsTest, DISABLED_TestSQLGetDiagFieldWForConnectFailureNTS) {
159131
arrow::util::UTF8ToWideString(connect_str));
160132
std::vector<SQLWCHAR> connect_str0(wconnect_str.begin(), wconnect_str.end());
161133

162-
SQLWCHAR out_str[ODBC_BUFFER_SIZE];
134+
SQLWCHAR out_str[kOdbcBufferSize];
163135
SQLSMALLINT out_str_len;
164136

165137
// Connecting to ODBC server.
166138
ASSERT_EQ(SQL_ERROR,
167-
SQLDriverConnect(conn, NULL, &connect_str0[0],
139+
SQLDriverConnect(this->conn, NULL, &connect_str0[0],
168140
static_cast<SQLSMALLINT>(connect_str0.size()), out_str,
169-
ODBC_BUFFER_SIZE, &out_str_len, SQL_DRIVER_NOPROMPT));
141+
kOdbcBufferSize, &out_str_len, SQL_DRIVER_NOPROMPT));
170142

171143
// Retrieve all supported header level and record level data
172144
SQLSMALLINT RECORD_1 = 1;
173145

174146
// SQL_DIAG_MESSAGE_TEXT SQL_NTS
175-
SQLWCHAR message_text[ODBC_BUFFER_SIZE];
147+
SQLWCHAR message_text[kOdbcBufferSize];
176148
SQLSMALLINT message_text_length;
177149

178-
message_text[ODBC_BUFFER_SIZE - 1] = '\0';
150+
message_text[kOdbcBufferSize - 1] = '\0';
179151

180152
ASSERT_EQ(SQL_SUCCESS,
181-
SQLGetDiagField(SQL_HANDLE_DBC, conn, RECORD_1, SQL_DIAG_MESSAGE_TEXT,
153+
SQLGetDiagField(SQL_HANDLE_DBC, this->conn, RECORD_1, SQL_DIAG_MESSAGE_TEXT,
182154
message_text, SQL_NTS, &message_text_length));
183155

184156
EXPECT_GT(message_text_length, 100);
185-
186-
// Free connection handle
187-
ASSERT_EQ(SQL_SUCCESS, SQLFreeHandle(SQL_HANDLE_DBC, conn));
188-
189-
// Free environment handle
190-
ASSERT_EQ(SQL_SUCCESS, SQLFreeHandle(SQL_HANDLE_ENV, env));
191157
}
192158

193159
TYPED_TEST(ErrorsTest, TestSQLGetDiagFieldWForDescriptorFailureFromDriverManager) {
@@ -214,20 +180,20 @@ TYPED_TEST(ErrorsTest, TestSQLGetDiagFieldWForDescriptorFailureFromDriverManager
214180
EXPECT_EQ(1, diag_number);
215181

216182
// SQL_DIAG_SERVER_NAME
217-
SQLWCHAR server_name[ODBC_BUFFER_SIZE];
183+
SQLWCHAR server_name[kOdbcBufferSize];
218184
SQLSMALLINT server_name_length;
219185

220186
EXPECT_EQ(SQL_SUCCESS,
221187
SQLGetDiagField(SQL_HANDLE_DESC, descriptor, RECORD_1, SQL_DIAG_SERVER_NAME,
222-
server_name, ODBC_BUFFER_SIZE, &server_name_length));
188+
server_name, kOdbcBufferSize, &server_name_length));
223189

224190
// SQL_DIAG_MESSAGE_TEXT
225-
SQLWCHAR message_text[ODBC_BUFFER_SIZE];
191+
SQLWCHAR message_text[kOdbcBufferSize];
226192
SQLSMALLINT message_text_length;
227193

228194
EXPECT_EQ(SQL_SUCCESS,
229195
SQLGetDiagField(SQL_HANDLE_DESC, descriptor, RECORD_1, SQL_DIAG_MESSAGE_TEXT,
230-
message_text, ODBC_BUFFER_SIZE, &message_text_length));
196+
message_text, kOdbcBufferSize, &message_text_length));
231197

232198
EXPECT_GT(message_text_length, 100);
233199

@@ -267,12 +233,12 @@ TYPED_TEST(ErrorsTest, TestSQLGetDiagRecForDescriptorFailureFromDriverManager) {
267233

268234
SQLWCHAR sql_state[6];
269235
SQLINTEGER native_error;
270-
SQLWCHAR message[ODBC_BUFFER_SIZE];
236+
SQLWCHAR message[kOdbcBufferSize];
271237
SQLSMALLINT message_length;
272238

273239
ASSERT_EQ(SQL_SUCCESS,
274240
SQLGetDiagRec(SQL_HANDLE_DESC, descriptor, 1, sql_state, &native_error,
275-
message, ODBC_BUFFER_SIZE, &message_length));
241+
message, kOdbcBufferSize, &message_length));
276242

277243
EXPECT_GT(message_length, 60);
278244

@@ -281,76 +247,58 @@ TYPED_TEST(ErrorsTest, TestSQLGetDiagRecForDescriptorFailureFromDriverManager) {
281247
// API not implemented error from driver manager
282248
EXPECT_EQ(std::wstring(L"IM001"), std::wstring(sql_state));
283249

284-
EXPECT_TRUE(!std::wstring(message).empty());
250+
EXPECT_FALSE(std::wstring(message).empty());
285251

286252
// Free descriptor handle
287253
EXPECT_EQ(SQL_SUCCESS, SQLFreeHandle(SQL_HANDLE_DESC, descriptor));
288254
}
289255

290-
TYPED_TEST(ErrorsTest, TestSQLGetDiagRecForConnectFailure) {
291-
// ODBC Environment
292-
SQLHENV env;
293-
SQLHDBC conn;
294-
295-
// Allocate an environment handle
296-
ASSERT_EQ(SQL_SUCCESS, SQLAllocEnv(&env));
297-
298-
ASSERT_EQ(SQL_SUCCESS,
299-
SQLSetEnvAttr(env, SQL_ATTR_ODBC_VERSION, (void*)SQL_OV_ODBC3, 0));
300-
301-
// Allocate a connection using alloc handle
302-
ASSERT_EQ(SQL_SUCCESS, SQLAllocHandle(SQL_HANDLE_DBC, env, &conn));
303-
256+
TYPED_TEST(ErrorsHandleTest, TestSQLGetDiagRecForConnectFailure) {
304257
// Invalid connect string
305258
std::string connect_str = this->GetInvalidConnectionString();
306259

307260
ASSERT_OK_AND_ASSIGN(std::wstring wconnect_str,
308261
arrow::util::UTF8ToWideString(connect_str));
309262
std::vector<SQLWCHAR> connect_str0(wconnect_str.begin(), wconnect_str.end());
310263

311-
SQLWCHAR out_str[ODBC_BUFFER_SIZE];
264+
SQLWCHAR out_str[kOdbcBufferSize];
312265
SQLSMALLINT out_str_len;
313266

314267
// Connecting to ODBC server.
315268
ASSERT_EQ(SQL_ERROR,
316-
SQLDriverConnect(conn, NULL, &connect_str0[0],
269+
SQLDriverConnect(this->conn, NULL, &connect_str0[0],
317270
static_cast<SQLSMALLINT>(connect_str0.size()), out_str,
318-
ODBC_BUFFER_SIZE, &out_str_len, SQL_DRIVER_NOPROMPT));
271+
kOdbcBufferSize, &out_str_len, SQL_DRIVER_NOPROMPT));
319272

320273
SQLWCHAR sql_state[6];
321274
SQLINTEGER native_error;
322-
SQLWCHAR message[ODBC_BUFFER_SIZE];
275+
SQLWCHAR message[kOdbcBufferSize];
323276
SQLSMALLINT message_length;
324-
ASSERT_EQ(SQL_SUCCESS, SQLGetDiagRec(SQL_HANDLE_DBC, conn, 1, sql_state, &native_error,
325-
message, ODBC_BUFFER_SIZE, &message_length));
277+
ASSERT_EQ(SQL_SUCCESS,
278+
SQLGetDiagRec(SQL_HANDLE_DBC, this->conn, 1, sql_state, &native_error,
279+
message, ODBC_BUFFER_SIZE, &message_length));
326280

327281
EXPECT_GT(message_length, 120);
328282

329283
EXPECT_EQ(200, native_error);
330284

331285
EXPECT_EQ(std::wstring(L"28000"), std::wstring(sql_state));
332286

333-
EXPECT_TRUE(!std::wstring(message).empty());
334-
335-
// Free connection handle
336-
ASSERT_EQ(SQL_SUCCESS, SQLFreeHandle(SQL_HANDLE_DBC, conn));
337-
338-
// Free environment handle
339-
ASSERT_EQ(SQL_SUCCESS, SQLFreeHandle(SQL_HANDLE_ENV, env));
287+
EXPECT_FALSE(std::wstring(message).empty());
340288
}
341289

342290
TYPED_TEST(ErrorsTest, TestSQLGetDiagRecInputData) {
343291
// SQLGetDiagRec does not post diagnostic records for itself.
344292

345293
SQLWCHAR sql_state[6];
346294
SQLINTEGER native_error;
347-
SQLWCHAR message[ODBC_BUFFER_SIZE];
295+
SQLWCHAR message[kOdbcBufferSize];
348296
SQLSMALLINT message_length;
349297

350298
// Pass invalid record number
351299
EXPECT_EQ(SQL_ERROR,
352300
SQLGetDiagRec(SQL_HANDLE_DBC, this->conn, 0, sql_state, &native_error,
353-
message, ODBC_BUFFER_SIZE, &message_length));
301+
message, kOdbcBufferSize, &message_length));
354302

355303
// Pass valid record number with null inputs
356304
EXPECT_EQ(SQL_NO_DATA, SQLGetDiagRec(SQL_HANDLE_DBC, this->conn, 1, 0, 0, 0, 0, 0));
@@ -398,7 +346,7 @@ TYPED_TEST(ErrorsTest, TestSQLErrorEnvErrorFromDriverManager) {
398346
// Function sequence error state from driver manager
399347
EXPECT_EQ(std::wstring(L"HY010"), std::wstring(sql_state));
400348

401-
EXPECT_TRUE(!std::wstring(message).empty());
349+
EXPECT_FALSE(std::wstring(message).empty());
402350
}
403351

404352
TYPED_TEST(ErrorsTest, TestSQLErrorConnError) {
@@ -426,7 +374,7 @@ TYPED_TEST(ErrorsTest, TestSQLErrorConnError) {
426374
// optional feature not supported error state
427375
EXPECT_EQ(std::wstring(L"HYC00"), std::wstring(sql_state));
428376

429-
EXPECT_TRUE(!std::wstring(message).empty());
377+
EXPECT_FALSE(std::wstring(message).empty());
430378
}
431379

432380
TYPED_TEST(ErrorsTest, TestSQLErrorStmtError) {
@@ -454,7 +402,7 @@ TYPED_TEST(ErrorsTest, TestSQLErrorStmtError) {
454402

455403
EXPECT_EQ(std::wstring(L"HY000"), std::wstring(sql_state));
456404

457-
EXPECT_TRUE(!std::wstring(message).empty());
405+
EXPECT_FALSE(std::wstring(message).empty());
458406
}
459407

460408
TYPED_TEST(ErrorsTest, TestSQLErrorStmtWarning) {
@@ -490,7 +438,7 @@ TYPED_TEST(ErrorsTest, TestSQLErrorStmtWarning) {
490438
// Verify string truncation warning is reported
491439
EXPECT_EQ(std::wstring(L"01004"), std::wstring(sql_state));
492440

493-
EXPECT_TRUE(!std::wstring(message).empty());
441+
EXPECT_FALSE(std::wstring(message).empty());
494442
}
495443

496444
TYPED_TEST(ErrorsOdbcV2Test, TestSQLErrorEnvErrorODBCVer2FromDriverManager) {
@@ -517,7 +465,7 @@ TYPED_TEST(ErrorsOdbcV2Test, TestSQLErrorEnvErrorODBCVer2FromDriverManager) {
517465
// Function sequence error state from driver manager
518466
EXPECT_EQ(std::wstring(L"S1010"), std::wstring(sql_state));
519467

520-
EXPECT_TRUE(!std::wstring(message).empty());
468+
EXPECT_FALSE(std::wstring(message).empty());
521469
}
522470

523471
TYPED_TEST(ErrorsOdbcV2Test, TestSQLErrorConnErrorODBCVer2) {
@@ -543,7 +491,7 @@ TYPED_TEST(ErrorsOdbcV2Test, TestSQLErrorConnErrorODBCVer2) {
543491
// optional feature not supported error state. Driver Manager maps state to S1C00
544492
EXPECT_EQ(std::wstring(L"S1C00"), std::wstring(sql_state));
545493

546-
EXPECT_TRUE(!std::wstring(message).empty());
494+
EXPECT_FALSE(std::wstring(message).empty());
547495
}
548496

549497
TYPED_TEST(ErrorsOdbcV2Test, TestSQLErrorStmtErrorODBCVer2) {
@@ -572,7 +520,7 @@ TYPED_TEST(ErrorsOdbcV2Test, TestSQLErrorStmtErrorODBCVer2) {
572520
// Driver Manager maps error state to S1000
573521
EXPECT_EQ(std::wstring(L"S1000"), std::wstring(sql_state));
574522

575-
EXPECT_TRUE(!std::wstring(message).empty());
523+
EXPECT_FALSE(std::wstring(message).empty());
576524
}
577525

578526
TYPED_TEST(ErrorsOdbcV2Test, TestSQLErrorStmtWarningODBCVer2) {
@@ -608,7 +556,7 @@ TYPED_TEST(ErrorsOdbcV2Test, TestSQLErrorStmtWarningODBCVer2) {
608556
// Verify string truncation warning is reported
609557
EXPECT_EQ(std::wstring(L"01004"), std::wstring(sql_state));
610558

611-
EXPECT_TRUE(!std::wstring(message).empty());
559+
EXPECT_FALSE(std::wstring(message).empty());
612560
}
613561

614562
} // namespace arrow::flight::sql::odbc

0 commit comments

Comments
 (0)