Added device flow, native error handling to ODBC, Powershell installer #27
Conversation
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
* Initial checkin of device flow (aka device grant) * Better error reporting back to ODBC interface * Powershell installer for development ODBC driver.
Reviewer's GuideThis PR adds support for OIDC Device Flow by extending configuration and wiring in a new auth provider, implements comprehensive native error handling via a new TrinoOdbcErrorHandler integrated into TrinoQuery and ODBC entry points, and introduces a PowerShell script to automate ODBC driver installation. Class diagram for new and updated authentication providers (Device Flow)classDiagram
class AuthConfig
class TokenCacheAuthProviderBase
class DeviceCredAuthConfig {
+std::string oidcDiscoveryUrl
+std::string clientId
+std::string clientSecret
+std::string scope
+std::string grantType
+std::string tokenEndpoint
+obtainAccessToken(CURL*, std::string*, std::map<std::string, std::string>*) std::string
}
AuthConfig <|-- TokenCacheAuthProviderBase
TokenCacheAuthProviderBase <|-- DeviceCredAuthConfig
class "getDeviceFlowAuthProvider()" {
}
DeviceCredAuthConfig <.. "getDeviceFlowAuthProvider()" : creates
Class diagram for TrinoOdbcErrorHandler integrationclassDiagram
class TrinoOdbcErrorHandler {
+OdbcError FromTrinoJson(json, std::string)
+const Entry* LookupEntryByName(std::string)
+bool ReloadMappingFromJson(std::string, std::string*)
+void SetConfigDirectory(std::string)
+std::string GetEffectiveConfigPath()
+std::string OdbcErrorToString(OdbcError, bool)
--
class OdbcError {
+SQLRETURN ret
+std::string sqlstate
+SQLINTEGER native
+std::string message
+std::string description
+std::vector<std::string> stack
+std::optional<int> lineNumber
+std::optional<int> columnNumber
+std::string queryId
}
class Entry {
+int trino_code
+std::string trino_name
+std::string sqlstate
+std::string description
}
}
TrinoOdbcErrorHandler o-- OdbcError
TrinoOdbcErrorHandler o-- 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 - here's some feedback:
- In deviceFlowAuthProvider.cpp, the curl_slist* headers created for POST requests aren’t freed after use—add curl_slist_free_all(headers) to prevent memory leaks.
TrinoQuery::parseTrinoErroris implemented but never used—either hook it into error handling or remove the dead code to keep the class focused.- When registering/unregistering the disconnect callback via std::bind you’re creating two distinct functors—store the bound function object in a member so unregisterDisconnectCallback receives the exact same callable.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In deviceFlowAuthProvider.cpp, the curl_slist* headers created for POST requests aren’t freed after use—add curl_slist_free_all(headers) to prevent memory leaks.
- `TrinoQuery::parseTrinoError` is implemented but never used—either hook it into error handling or remove the dead code to keep the class focused.
- When registering/unregistering the disconnect callback via std::bind you’re creating two distinct functors—store the bound function object in a member so unregisterDisconnectCallback receives the exact same callable.
## Individual Comments
### Comment 1
<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 (bug_risk):** Calling value() on std::optional without checking may throw.
getError() should check if odbcError contains a value before calling value(), or handle the case where it is not set to avoid exceptions.
</issue_to_address>
### Comment 2
<location> `src/driver/handles/connHandle.cpp:24-26` </location>
<code_context>
return this->connectionConfig->getTrinoServerVersion();
}
+void Connection::checkInputs(DriverConfig config) {
+ if (config.getHostname().empty()) {
+ throw std::invalid_argument("hostname cannot be empty");
+ } else if (!config.getPortNum()) {
+ throw std::invalid_argument("port cannot be empty");
+ }
+}
+
</code_context>
<issue_to_address>
**suggestion:** checkInputs throws for falsy port, which may allow zero.
If zero is not a valid port, this logic is fine. Otherwise, use 'port <= 0' for clarity.
```suggestion
} else if (config.getPortNum() <= 0) {
throw std::invalid_argument("port must be greater than zero");
}
```
</issue_to_address>
### Comment 3
<location> `src/driver/numResultCols.cpp:19-20` </location>
<code_context>
" Requesting RecNumber: " + std::to_string(RecNumber));
- return SQL_NO_DATA;
+
+ if (statement->trinoQuery->hasError()) {
+ TrinoOdbcErrorHandler::OdbcError odbcErr =
+ statement->trinoQuery->getError();
</code_context>
<issue_to_address>
**suggestion:** Returning SQL_ERROR for any TrinoQuery error may be too broad.
Evaluate if specific TrinoQuery errors should map to different SQL return codes, such as SQL_SUCCESS_WITH_INFO for non-critical issues.
</issue_to_address>
### Comment 4
<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):** CURL error is logged but not propagated.
Consider updating the code to propagate the CURL error to the caller, either by returning an error code or updating the object's state, to enable proper error handling.
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 the error to the caller
}
```
You will need to:
1. Update the function signature to return `CURLcode` (or an appropriate error type).
2. Update all callers of this function to handle the returned error code.
3. If this code is inside a constructor or a void function, consider updating the design to allow error propagation (e.g., by returning a status or throwing an exception).
</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
+24
to
+26
| } else if (!config.getPortNum()) { | ||
| throw std::invalid_argument("port cannot be empty"); | ||
| } |
There was a problem hiding this comment.
suggestion: checkInputs throws for falsy port, which may allow zero.
If zero is not a valid port, this logic is fine. Otherwise, use 'port <= 0' for clarity.
Suggested change
| } else if (!config.getPortNum()) { | |
| throw std::invalid_argument("port cannot be empty"); | |
| } | |
| } else if (config.getPortNum() <= 0) { | |
| throw std::invalid_argument("port must be greater than zero"); | |
| } |
rileymcdowell
requested changes
Oct 13, 2025
There was a problem hiding this comment.
Very nice. I left some comments for a few minor style things. The functionality here all looks good.
Please also take a look at the clang-format CICD output. I think you just need to run clang-format on the whole codebase and it should auto-resolve the issues for you.
| const json& getRowAtIndex(int64_t) const; | ||
|
|
||
| void setQueryId(const std::string& id); | ||
| const std::string& getQueryId() const; // <-- Add this getter |
There was a problem hiding this comment.
I suspect this comment was leftover from development time.
| if (response_json.contains("error")) { | ||
| this->error = true; | ||
| std::string errorDetails; | ||
| // errorDetails = parseTrinoError(response_json["error"]); |
There was a problem hiding this comment.
Can we swap this (and the header) to trinoOdbcErrorHandler.*** to keep them consistent with the other files in the repo?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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
Enable OIDC device flow authentication, enhance Trino-to-ODBC error mapping, and provide a PowerShell installer for developer convenience
New Features:
Enhancements:
Build: