Added device flow, native error handling to ODBC, Powershell installer#26
Closed
dprophet wants to merge 1 commit intotrinodb:mainfrom
Closed
Added device flow, native error handling to ODBC, Powershell installer#26dprophet wants to merge 1 commit intotrinodb:mainfrom
dprophet wants to merge 1 commit intotrinodb:mainfrom
Conversation
Reviewer's GuideThis PR adds full support for OAuth2 device flow authentication, implements a native ODBC error‐mapping layer with chunked diagnostics, and ships a PowerShell script to install the development ODBC driver. Class diagram for new and updated authentication providers (Device Flow)classDiagram
class AuthConfig {
}
class TokenCacheAuthProviderBase {
}
class DeviceCredAuthConfig {
+oidcDiscoveryUrl: string
+clientId: string
+clientSecret: string
+scope: string
+grantType: string
+tokenEndpoint: string
+obtainAccessToken(curl, responseData, responseHeaderData): string
}
AuthConfig <|-- TokenCacheAuthProviderBase
TokenCacheAuthProviderBase <|-- DeviceCredAuthConfig
class "getDeviceFlowAuthProvider()" {
}
"getDeviceFlowAuthProvider()" ..> DeviceCredAuthConfig
class ConnectionConfig {
+tokenEndpoint: string
+grantType: string
+authMethod: ApiAuthMethod
+authConfigPtr: unique_ptr<AuthConfig>
}
ConnectionConfig --> AuthConfig
class DriverConfig {
+tokenEndpoint: string
+grantType: string
+getTokenEndpoint(): string
+setTokenEndpoint(tokenEndpoint: string)
+getGrantType(): string
+setGrantType(grantType: string)
}
class ApiAuthMethod {
AM_NO_AUTH
AM_EXTERNAL_AUTH
AM_CLIENT_CRED_AUTH
AM_DEVICE_FLOW
}
ConnectionConfig --> ApiAuthMethod
Class diagram for TrinoOdbcErrorHandler and error mappingclassDiagram
class TrinoOdbcErrorHandler {
+FromTrinoJson(err: json, queryId: string): OdbcError
+LookupEntryByName(errorName: string): Entry*
+ReloadMappingFromJson(path: string, error_out: string*): bool
+SetConfigDirectory(dir: string)
+GetEffectiveConfigPath(): string
+OdbcErrorToString(err: OdbcError, include_stack: bool): string
}
class TrinoOdbcErrorHandler_OdbcError {
+ret: SQLRETURN
+sqlstate: string
+native: SQLINTEGER
+message: string
+description: string
+stack: vector<string>
+lineNumber: optional<int>
+columnNumber: optional<int>
+queryId: string
}
class TrinoOdbcErrorHandler_Entry {
+trino_code: int
+trino_name: string
+sqlstate: string
+description: string
}
TrinoOdbcErrorHandler ..> TrinoOdbcErrorHandler_OdbcError
TrinoOdbcErrorHandler ..> TrinoOdbcErrorHandler_Entry
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/trinoAPIWrapper/trinoQuery.cpp:197-198` </location>
<code_context>
CURLcode res = curl_easy_perform(curl);
+ if (res != CURLE_OK) {
+ WriteLog(LL_ERROR, std::string("CURL error: ") + curl_easy_strerror(res));
+ }
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider propagating CURL errors to the caller, not just logging.
If the caller needs to handle network errors, return an error code or set an error state instead of only logging.
Suggested implementation:
```cpp
CURLcode res = curl_easy_perform(curl);
if (res != CURLE_OK) {
WriteLog(LL_ERROR, std::string("CURL error: ") + curl_easy_strerror(res));
return res; // Propagate error to caller
}
```
You will need to ensure that the containing function's return type is `CURLcode` (or some error-propagating type), and that callers handle the returned error code appropriately. If the function currently returns `void`, change its signature to `CURLcode`. If you prefer to use exceptions, you could throw a custom exception instead of returning.
</issue_to_address>
### Comment 2
<location> `src/trinoAPIWrapper/trinoQuery.cpp:486-487` </location>
<code_context>
+ }
+}
+
+const TrinoOdbcErrorHandler::OdbcError& TrinoQuery::getError() const {
+ return odbcError.value();
+};
</code_context>
<issue_to_address>
**issue:** getError() will throw if odbcError is not set.
Document or enforce that getError() should only be called when hasError() returns true to prevent exceptions.
</issue_to_address>
### Comment 3
<location> `src/trinoAPIWrapper/connectionConfig.cpp:66-68` </location>
<code_context>
+
this->curl = nullptr;
+ if (hostname.empty()) {
+ throw std::invalid_argument("hostname");
+ }
+
</code_context>
<issue_to_address>
**suggestion:** Throwing std::invalid_argument for empty hostname is appropriate, but consider including more context in the error message.
Adding the invalid hostname value to the exception message will make debugging easier.
```suggestion
if (hostname.empty()) {
throw std::invalid_argument("Invalid hostname: value is empty");
}
```
</issue_to_address>
### Comment 4
<location> `src/trinoAPIWrapper/authProvider/deviceFlowAuthProvider.cpp:104-105` </location>
<code_context>
+
+ std::string command = "start " + verificationUriComplete;
+
+ int result = std::system(command.c_str());
+ if (result != 0) {
+ WriteLog(LL_ERROR, " Failed to open the browser. Command: " + command);
+ }
</code_context>
<issue_to_address>
**issue:** Using std::system to launch a browser is platform-specific and may fail silently.
Please use a cross-platform approach for opening URLs, or clearly document the Windows-only support. Also, review the security risks of using std::system.
</issue_to_address>
### Comment 5
<location> `src/trinoAPIWrapper/authProvider/deviceFlowAuthProvider.cpp:157-160` </location>
<code_context>
+ } else if (error == "slow_down") {
+ WriteLog(LL_DEBUG,
+ " Server requested slower polling, increasing interval...");
+ interval += 5; // Adjust interval as per server's suggestion
+ } else {
+ WriteLog(LL_ERROR, " Polling failed with error: " + error);
</code_context>
<issue_to_address>
**suggestion:** Increasing polling interval by a fixed amount may not match server expectations.
Check if the response includes an 'interval' value and use it when available.
```suggestion
} else if (error == "slow_down") {
WriteLog(LL_DEBUG,
" Server requested slower polling.");
if (pollingResponse.contains("interval")) {
int newInterval = pollingResponse["interval"];
WriteLog(LL_DEBUG, " Server suggested interval: " + std::to_string(newInterval));
interval = newInterval;
} else {
WriteLog(LL_DEBUG, " No interval provided by server, increasing by 5 seconds.");
interval += 5; // Fallback if server does not provide interval
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+197
to
+198
| if (res != CURLE_OK) { | ||
| WriteLog(LL_ERROR, std::string("CURL error: ") + curl_easy_strerror(res)); |
There was a problem hiding this comment.
suggestion (bug_risk): Consider propagating CURL errors to the caller, not just logging.
If the caller needs to handle network errors, return an error code or set an error state instead of only logging.
Suggested implementation:
CURLcode res = curl_easy_perform(curl);
if (res != CURLE_OK) {
WriteLog(LL_ERROR, std::string("CURL error: ") + curl_easy_strerror(res));
return res; // Propagate error to caller
}
You will need to ensure that the containing function's return type is CURLcode (or some error-propagating type), and that callers handle the returned error code appropriately. If the function currently returns void, change its signature to CURLcode. If you prefer to use exceptions, you could throw a custom exception instead of returning.
Comment on lines
+66
to
+68
| if (hostname.empty()) { | ||
| throw std::invalid_argument("hostname"); | ||
| } |
There was a problem hiding this comment.
suggestion: Throwing std::invalid_argument for empty hostname is appropriate, but consider including more context in the error message.
Adding the invalid hostname value to the exception message will make debugging easier.
Suggested change
| if (hostname.empty()) { | |
| throw std::invalid_argument("hostname"); | |
| } | |
| if (hostname.empty()) { | |
| throw std::invalid_argument("Invalid hostname: value is empty"); | |
| } |
Comment on lines
+104
to
+105
| int result = std::system(command.c_str()); | ||
| if (result != 0) { |
There was a problem hiding this comment.
issue: Using std::system to launch a browser is platform-specific and may fail silently.
Please use a cross-platform approach for opening URLs, or clearly document the Windows-only support. Also, review the security risks of using std::system.
Comment on lines
+157
to
+160
| } else if (error == "slow_down") { | ||
| WriteLog(LL_DEBUG, | ||
| " Server requested slower polling, increasing interval..."); | ||
| interval += 5; // Adjust interval as per server's suggestion |
There was a problem hiding this comment.
suggestion: Increasing polling interval by a fixed amount may not match server expectations.
Check if the response includes an 'interval' value and use it when available.
Suggested change
| } else if (error == "slow_down") { | |
| WriteLog(LL_DEBUG, | |
| " Server requested slower polling, increasing interval..."); | |
| interval += 5; // Adjust interval as per server's suggestion | |
| } else if (error == "slow_down") { | |
| WriteLog(LL_DEBUG, | |
| " Server requested slower polling."); | |
| if (pollingResponse.contains("interval")) { | |
| int newInterval = pollingResponse["interval"]; | |
| WriteLog(LL_DEBUG, " Server suggested interval: " + std::to_string(newInterval)); | |
| interval = newInterval; | |
| } else { | |
| WriteLog(LL_DEBUG, " No interval provided by server, increasing by 5 seconds."); | |
| interval += 5; // Fallback if server does not provide interval | |
| } |
* Initial checkin of device flow (aka device grant) * Better error reporting back to ODBC interface * Powershell installer for development ODBC driver.
Contributor
Author
|
I am closing this PR as I want to do it from a feature branch under our org, not from the main branch |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This checkin includes the following
Summary by Sourcery
Add device flow authentication, implement native ODBC error mapping and reporting, and provide a PowerShell installer for the development ODBC driver.
New Features:
Enhancements:
Build: