Fix conn string parsing with unsupported options #60
Conversation
This file contains 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
staticlibs
added a commit
to staticlibs/duckdb-odbc
that referenced
this pull request
Feb 16, 2025
When the ODBC data source is used from Excel/Power Query, additional options may be added to connection string automatically without showing them to user. It was discovered in duckdb#58 that `UID` and `PWD` options are added when user has credentials registered for this DSN in Excel. DuckDB driver does not recognize these options and currently cannot process them properly (see duckdb#59 for details). However the driver knows about possible `trusted_connection` Excel/Power Query option and ignores it successfully. It is proposed to also add `UID` and `PWD` to the existing ignore list. In the light of duckdb#60, that fixes the handling for unsupported options, this change is not strictly necessary, but it still may be better to consistently ignore all unneeded Excel/Power Query options and to not register diagnostic messages for them. Testing: existing Excel/Power Query ignore list test is extended to include new options. Fixes: duckdb#58
The ODBC specification does not mandate how the driver should deal with unsupported options specified by client in the connection string. When encountering an unsupported option, DuckDB driver registers a diagnostic message in a format: 'Invalid keyword: <option_name>', that can be fetched by client using SQLGetDiagRec function, and returns SQL_SUCCESS_WITH_INFO instead of SQL_SUCCESS. This is a reasonable approach, but there is inconsistent error handling in its implementation in Connect::ParseInputStr. It only checks for SQL_SUCCESS and treats SQL_SUCCESS_WITH_INFO as an error. This causes the early exit from this function that prevents subsequent options in the connection string to be processed. It also prevents the DSN name to be set on the connection (even when the DSN option comes before the unsupported one). To fix this it is proposed to check for both SQL_SUCCESS and SQL_SUCCESS_WITH_INFO and only do the early exit if some other error code is returned by FindKeyValPair routine. Testing: tests are included for a common option and for the DSN option. Fixes: duckdb#59
staticlibs
force-pushed
the
conn_str_unsupported_options
branch
from
db53862 to
2094239
Compare
staticlibs
added a commit
to staticlibs/duckdb-odbc
that referenced
this pull request
Feb 19, 2025
When the ODBC data source is used from Excel/Power Query, additional options may be added to connection string automatically without showing them to user. It was discovered in duckdb#58 that `UID` and `PWD` options are added when user has credentials registered for this DSN in Excel. DuckDB driver does not recognize these options and currently cannot process them properly (see duckdb#59 for details). However the driver knows about possible `trusted_connection` Excel/Power Query option and ignores it successfully. It is proposed to also add `UID` and `PWD` to the existing ignore list. In the light of duckdb#60, that fixes the handling for unsupported options, this change is not strictly necessary, but it still may be better to consistently ignore all unneeded Excel/Power Query options and to not register diagnostic messages for them. Testing: existing Excel/Power Query ignore list test is extended to include new options. Fixes: duckdb#58
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The ODBC specification does not mandate how the driver should deal with unsupported options specified by client in the connection string (link).
When encountering an unsupported option, DuckDB driver registers a diagnostic message in a format:
Invalid keyword: <option_name>, that can be fetched by client usingSQLGetDiagRecfunction, and returnsSQL_SUCCESS_WITH_INFOinstead ofSQL_SUCCESS.This is a reasonable approach, but there is inconsistent error handling in its implementation in Connect::ParseInputStr. It only checks for
SQL_SUCCESSand treatsSQL_SUCCESS_WITH_INFOas an error. This causes the early exit from this function that prevents subsequent options in the connection string to be processed. It also prevents the DSN name to be set on the connection (even when the DSN option comes before the unsupported one).To fix this it is proposed to check for both
SQL_SUCCESSandSQL_SUCCESS_WITH_INFOand only do the early exit if some other error code is returned by FindKeyValPair routine.Testing: tests are included for a common option and for the DSN option.
Fixes: #59