gabotorresruiz commented on code in PR #43974:
URL: https://github.com/apache/superset/pull/43974#discussion_r3992845991
##########
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:
Just a small question, not a blocker: are we certain
`connect_args["database"]` actually wins over an `odbc_connect` embedded
`Database=` at the driver level? What I could verify is that SQLAlchemy uses
`odbc_connect` as the entire connection string and passes `connect_args` on as
extra `pyodbc.connect()` kwargs, which pyodbc appends to the end of that
string. The ODBC `SQLDriverConnect` spec says a driver uses the first
occurrence of a repeated keyword, which would make the `odbc_connect` value
win, the opposite of the order encoded here and in
`test_get_catalog_from_engine_params_connect_args_wins_over_odbc_connect`. I
could not test against a live SQL Server, so you may well know better.
It only matters when an admin configures both sources with different
databases, which is already a contradictory setup. But since this feeds an
authorization decision, cheap insurance would be to return `None` whenever both
sources are present and disagree, so the normalization simply never fires and
we keep the pre-existing denial:
```python
if odbc_database and connect_args_database and odbc_database !=
connect_args_database:
return None
```
Happy to dig into the actual msodbcsql behavior with you if you think it is
worth pinning down.
--
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]
