Skip to content
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

Incorrect connection string parsing when unsupported options are present #59

Open
staticlibs opened this issue Feb 16, 2025 · 0 comments · May be fixed by #60
Open

Incorrect connection string parsing when unsupported options are present #59

staticlibs opened this issue Feb 16, 2025 · 0 comments · May be fixed by #60

Comments

@staticlibs
Copy link
Contributor

When unsupported options are included in the connection string, the driver aborts the parsing and does not process the (supported) options that are specified later in the connection string.

For example, with a driver registered in odbcinst.ini like this:

[DuckDB Driver]
Description=DuckDB Driver
Driver=/path/to/libduckdb_odbc.so

The allow_unsigned_extensions option can processed correctly:

Python 3.12.8 (main, Dec  6 2024, 00:00:00) [GCC 14.2.1 20240912 (Red Hat 14.2.1-3)] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import pyodbc
>>> print(pyodbc.connect("Driver={DuckDB Driver};allow_unsigned_extensions=true;").cursor().execute("select current_setting('allow_unsigned_extensions')").fetchone())
('true',)

But it won't be set if some unsupported option precedes it:

>>> print(pyodbc.connect("foo=bar;Driver={DuckDB Driver};unsupported_option_1=value_1;allow_unsigned_extensions=true;").cursor().execute("select current_setting('allow_unsigned_extensions')").fetchone())
('false',)

Besides that, the DSN name is ignored even when it is specified before the unsupported option, with the odbc.ini like this:

[DuckDB]
Driver = DuckDB Driver
database = /path/to/test1.db
>>> print(pyodbc.connect("DSN=DuckDB;").cursor().execute("select current_catalog()").fetchone())
('test1',)
>>> print(pyodbc.connect("DSN=DuckDB;unsupported_option_1=value_1;").cursor().execute("select current_catalog()").fetchone())
('memory',)
staticlibs added a commit to staticlibs/duckdb-odbc that referenced this issue Feb 16, 2025
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 staticlibs linked a pull request Feb 16, 2025 that will close this issue
staticlibs added a commit to staticlibs/duckdb-odbc that referenced this issue 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
staticlibs added a commit to staticlibs/duckdb-odbc that referenced this issue Feb 19, 2025
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 added a commit to staticlibs/duckdb-odbc that referenced this issue 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
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant