-
Notifications
You must be signed in to change notification settings - Fork 0
SQLError implementation in macOS #142
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Test `TestSQLErrorConnError` now passing locally Disable `SQLGetConnectOption` on macOS. Still need to check if SQLGetConnectOption is needed on macOS Excel. Move implementation to odbc_api.cc
verified on Windows
| // TODO: verify that `SQLGetConnectOption` is not required by Excel. | ||
| #ifndef __APPLE__ | ||
| TYPED_TEST(ErrorsOdbcV2Test, TestSQLErrorConnError) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still need to verify if SQLGetConnectOption is required by latest version of Excel. Will remove this todo comment in future PRs.
For this PR, we are checking in the SQLErrors changes.
| // Function sequence error state from driver manager | ||
| EXPECT_EQ(std::wstring(L"S1010"), std::wstring(sql_state)); | ||
| #ifdef _WIN32 | ||
| // Windows Driver Manager returns S1010 | ||
| EXPECT_EQ(SqlWcharToString(sql_state), kErrorStateS1010); | ||
| #else | ||
| // unix Driver Manager returns HY010 | ||
| EXPECT_EQ(SqlWcharToString(sql_state), kErrorStateHY010); | ||
| #endif // _WIN32 | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am surprised that Windows Driver Manager (DM) returns S1010, but I ran the original TestSQLErrorEnvErrorFromDriverManager test on windows and S1010 is what DM returns.
| sql_state_size * GetSqlWCharSize(), &sql_state_length)); | ||
|
|
||
| EXPECT_EQ(std::wstring(L"28000"), std::wstring(sql_state)); | ||
| EXPECT_EQ(SqlWcharToString(sql_state), kErrorState28000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously we had EXPECT_EQ(expected, actual) and now you've swapped the order around. Can we do EXPECT_EQ(kErrorState28000, SqlWcharToString(sql_state))?
Likewise for the rest of this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I have swapped back. The swap was accidental as I forgot the original intended order
| return SQL_ERROR; | ||
| } | ||
|
|
||
| #if defined(__APPLE__) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we also need a windows version of SQLError?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can rely on the driver manager for Windows to provide an implementation of SQLError, it is less error prone.
Driver Managers are supposed to handle ODBC 2.0 APIs like SQLError by mapping to ODBC 3.0 APIs, but macOS Driver Manager is missing on the functionality so we have to implement SQLError for macOS.
Implement SQLError for macOS only, as macOS driver manager does not do the mapping for SQLError. On Windows we use the SQLError mapping by the driver manager.
Test command with tests passing on intel mac