gabotorresruiz commented on code in PR #43974:
URL: https://github.com/apache/superset/pull/43974#discussion_r4020329977
##########
superset/db_engine_specs/mssql.py:
##########
@@ -194,6 +195,82 @@ def fetch_data(
# Lists of `pyodbc.Row` need to be unpacked further
return cls.pyodbc_rows_to_tuples(data)
+ @classmethod
+ def get_catalog_from_engine_params(
+ cls,
+ sqlalchemy_uri: URL,
+ connect_args: dict[str, Any],
+ ) -> str | None:
+ """
+ Resolve the database from genuine, statically-configured connection
+ settings only: an explicit ``connect_args["database"]``, a
+ ``Database=``/``Initial Catalog=`` entry embedded in the documented
+ ``odbc_connect`` connection-string query parameter, or the URL's own
+ database segment.
+
+ The check order mirrors ``MSDialect_pyodbc.create_connect_args``'s
+ actual precedence, not just plausibility:
+
+ - ``connect_args`` is always appended, as extra keyword arguments, to
+ whatever connection string SQLAlchemy hands to ``pyodbc.connect()``
+ -- regardless of whether that string came from the URL or from
+ ``odbc_connect`` -- so a duplicate key there wins over both.
+ - When ``odbc_connect`` is present, SQLAlchemy uses it as the *entire*
+ connection string and never looks at the URL's host/database
+ segments at all, so it must be checked before falling back to
+ ``sqlalchemy_uri.database``.
+
+ Returns None when none of these statically state a database -- e.g. a
+ host/DSN-only URI that relies on the SQL login's server-side default
+ database. That default is only known to SQL Server itself, at connect
+ time; resolving it would require a live query, which this method
+ deliberately does not perform.
+ """
+ if isinstance(database := connect_args.get("database"), str) and
database:
Review Comment:
Thanks @Ujjwaljain16 for pinning this down against the real driver, that
settles it far better than my spec reading did. My approval stands.
I verified the delta independently on this branch:
* The reordered hook matches how the positional connection string is really
built. I checked `PyODBCConnector.create_connect_args` on SQLAlchemy 2.0.52,
which is exactly what `requirements/base.txt` pins: `odbc_connect` becomes the
entire string, a host URL with a database bakes `Database=<db>` in
positionally, and a host-only URL becomes a `dsn=` string with no `Database=`
at all, so the `connect_args` fallback really is what the driver connects to in
that last case.
* All three affected suites pass at `d2a2327` (283 tests). Running the
updated test files against the previous commit `723e6b2` fails exactly the six
precedence tests, including the new `raise_for_access` level conflict test, so
the coverage genuinely pins this fix.
* Adversarial probes all keep the pre-existing denial: repeated
`odbc_connect` query params, non-string `connect_args["database"]` values, a
bare `Database` token without `=`, an empty `Database=` alongside a conflicting
`connect_args`, and an unclosed brace value all resolve to `None` or to a value
that can never match a parsed catalog.
Good catch on `Initial Catalog=` being an OLEDB/ADO.NET keyword. Dropping it
also closes a subtle over-grant in the version I approved: had the driver
silently ignored it and connected to the login's default database, the old
parser reported a database the connection never used, and the normalization
could have fired on it.
One tiny leftover in the same contradictory-config family, not a blocker and
fine as a follow-up or a wontfix: SQLAlchemy also accepts the database as a URL
query parameter. On 2.0.52,
`mssql+pyodbc://u:p@host?database=realdb&driver=...` builds `Database=realdb`
into the positional string while `URL.database` stays `None`, so with a
conflicting `connect_args["database"]` the hook falls through and reports the
`connect_args` value the driver never uses. Superset only documents the path
form, so I would not hold the PR on it; if you want cheap insurance, checking
`sqlalchemy_uri.query.get("database")` before the `connect_args` fallback would
cover it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
