session.query(Model).get(id) is deprecated under SQLAlchemy 2.0 in
favor of session.get(Model, id). Migrate the 9 remaining call sites
(flagged by review) and update the corresponding test mocks that
asserted against the old session.query(...).get(...) call shape.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
`generate_dashboard()` re-fetches with eager-loaded relationships via
`subqueryload(Dashboard.slices).subqueryload(Slice.editors)` (and
similarly for `.tags`, `Dashboard.editors`, `Dashboard.tags`), using a
`Dashboard` class resolved through a deferred `from
superset.models.dashboard import Dashboard` import inside the function --
deferred specifically so `@patch("superset.models.dashboard.Dashboard")`
in tests can substitute it.
`test_generate_dashboard_refetches_via_dao` and
`test_generate_dashboard_restricted_user_redacts_chart_datasource_name`
both patch the whole `Dashboard` class to control its constructor
(`mock_dashboard_cls.return_value = dashboard`), which leaves
`Dashboard.slices`/`.editors`/`.tags` as unconfigured auto-mocks. Passing
those to `subqueryload()` now raises `sqlalchemy.exc.ArgumentError:
Wildcard token cannot be followed by another entity` -- `ArgumentError`
subclasses `SQLAlchemyError`, which `generate_dashboard`'s re-fetch
already catches and treats as "re-fetch failed, return minimal
response", so both tests silently got the fallback response instead of
the real one they were asserting against.
Confirmed via a side-by-side instrumented run against SQLAlchemy 1.4.54:
the exact same mock chain there raised no error at all -- 1.4 didn't
eagerly validate `subqueryload()`'s argument type, so an unconfigured
MagicMock silently worked. This is a genuine SQLAlchemy 2.0 strictness
change in loader-option construction, but it doesn't affect real
production code (where `Dashboard` is never mocked and `.slices` is
always a real `InstrumentedAttribute`) -- it only breaks these two
tests' mocks, which were never fully specified to begin with.
Capture the real `Dashboard` class in a module-level `_RealDashboard`
import (before any per-test `@patch` can shadow it) and copy its
`slices`/`editors`/`tags` relationship attributes onto the mocked class
in `_setup_generate_dashboard_mocks`, so `subqueryload()` sees genuine
mapped attributes while `Dashboard(...)` construction stays mocked. Ran
the full 64-test file afterward, including the sibling
`..._refetch_sqlalchemy_error_rollback` test (which forces a real
`SQLAlchemyError` via `find_by_id.side_effect` and still correctly hits
the fallback path) -- no regressions.
test_get_sql_results_oauth2's `query = mocker.MagicMock(select_as_cta=False,
database=database)` left `limit` and `select_as_cta_used` as unconfigured
auto-mocks. `execute_sql_statements` unconditionally calls `apply_limit`
before ever reaching the mocked OAuth2 error, and `apply_limit` does
`query.limit > sql_max_row` once `SQLLAB_CTAS_NO_LIMIT` is false -- an
unconfigured MagicMock has no real `__gt__`, so this raised `TypeError:
'>' not supported between instances of 'MagicMock' and 'int'` instead of
ever reaching the OAuth2 path.
Root-caused with a side-by-side instrumented run against master pinned to
SQLAlchemy 1.4.54 + Flask-SQLAlchemy 2.5.1 (byte-identical sql_lab.py and
test file): there, `flask.current_app` inside `apply_limit` resolved to
an unrelated, already-initialized `superset.app` singleton (config
SQLLAB_CTAS_NO_LIMIT=True) instead of this test's own fixture app
(SQLLAB_CTAS_NO_LIMIT=False), so `apply_limit` took its
`select_as_cta_used and sqllab_ctas_no_limit` early-return path before
ever touching `query.limit` -- masking the gap by accident. Under
Flask-SQLAlchemy 3.x, `current_app` correctly resolves to the test's own
pushed app context, so the early return no longer fires and the
underspecified mock's real gap is exposed. This is not a SQLAlchemy 2.0
semantic break in application code; it's an under-specified test double
that only ever worked because of unrelated cross-test app-identity
bleed in the old environment.
Set `limit=None` and `select_as_cta_used=False` on the mock, matching
the real `Query` model's actual column defaults (`Column(Integer)` is
nullable/None; `Column(Boolean, default=False)`), so `apply_limit`
behaves the same way it would for a genuine freshly-queried `Query` row.
With the mock now correctly reaching the OAuth2 path inside the right
app context, the generated redirect URL correctly reflects this test's
own `SERVER_NAME=example.com` fixture config rather than Flask's
"localhost" default that leaked in from the wrong app previously --
update the two hardcoded `http://localhost/...` expectations to
`http://example.com/...` to match.
SQLAlchemy 2.0's declarative mapper now orders a model's mapped columns
with the class's own Column attributes first, followed by mixin-provided
columns (AuditMixinNullable's created_on/changed_on/created_by_fk/
changed_by_fk and the uuid mixin column) in mixin-declaration order,
appended at the end rather than interleaved at the front. This shifted
where `dbs.uuid`, `dbs.created_on`, `dbs.changed_on` (and the matching
`ssh_tunnels_1.*` aliases from the joined SSHTunnel) land in the compiled
SELECT list. SQLAlchemy 2.0 also renders the concatenation expression on
the left of an IN clause with an explicit parenthesized group.
Verified the actual compiled query has the identical column set (just
reordered) and an equivalent WHERE clause (same expression, extra
parens), then updated the hardcoded expected string in
test_database_filter to match.
shillelagh.exceptions.ProgrammingError is wrapped by SQLAlchemy's own
StatementError machinery, which appends a "Background on this error at:
https://sqlalche.me/e/<major><minor>/<code>" hint derived from the
running SQLAlchemy version. Under 1.4 that was .../e/14/f405; under 2.0
it's .../e/20/f405. test_dml and test_allowed_dbs hardcoded the 1.4 URL
in their expected exception strings; update both to the 2.0 path.
Critical regression: SQLAlchemy 2.0 changed URL.__str__() to always
substitute "***" for the password rather than rendering it verbatim
(SQLAlchemy 1.4's str(URL) rendered the real value). Every
build_sqlalchemy_uri() implementation across the db_engine_specs
(base/Postgres+MySQL+etc, ClickHouse, Databricks x2, Snowflake,
Databend, Couchbase) built a URL with the user's real password and
returned str(url) - which is exactly the string
superset/databases/schemas.py's pre-load hook writes into
data["sqlalchemy_uri"] when a database is created or edited via the
parameterized connection form. Under SQLAlchemy 2.0 that stores the
literal password "***" instead of the real one, breaking every new
connection made that way. Switched all of these to
render_as_string(hide_password=False), which is the 2.0-native way to
get the real, unmasked URL string.
Two more instances of the same str(URL) regression in
superset/models/core.py, both with real functional impact:
- Database.set_sqlalchemy_uri() intentionally replaces the real
password with Superset's own PASSWORD_MASK sentinel
("X" * 10, not a secret) before storing self.sqlalchemy_uri, so a
later edit can compare conn.password != PASSWORD_MASK to detect
"the user didn't touch the password field, keep the existing one."
str(conn) under 2.0 was substituting its own "***" for that
sentinel, so the stored URI no longer round-tripped to
PASSWORD_MASK - it round-tripped to the meaningless literal "***",
breaking password-preservation on every database edit.
- The per-process SQLAlchemy engine cache (superset/models/core.py,
_ENGINE_CACHE) keys on str(sqlalchemy_url) specifically so that a
password rotation naturally invalidates the cached engine (the
module comment states this explicitly). Under 2.0, str(url) always
masks to the same "***" regardless of the real password, so
rotating a database's password would silently keep reusing the old,
now-wrong cached engine/connection pool for the life of the worker
process.
Also fixed a separate, unrelated 1.4->2.0 break in
superset/db_engine_specs/duckdb.py: two build_sqlalchemy_uri variants
called the raw URL(...) constructor, which SQLAlchemy 2.0 turned into
a strict NamedTuple requiring username/password/host/port to be
passed explicitly (they used to default to None). That raised
"URL.__new__() missing 4 required positional arguments" outright.
Switched both to URL.create(), which keeps those optional.
Test-side: model_tests.py/db_engine_specs test files that asserted
str(uri) == "<scheme>://user:realpassword@host/..." were relying on
the old unmasked str() behavior; switched them to
uri.render_as_string(hide_password=False) to keep verifying the real
underlying value rather than relaxing what's being checked.
Verified locally (sqlite): tests/integration_tests/db_engine_specs/
and tests/unit_tests/db_engine_specs/ - the password-masking and
duckdb URL() failures are gone (18 -> 12 remaining, unrelated:
mysqlclient not importable on this Mac, 5 bigquery test_fetch_data
failures, and a where_latest_partition literal-rendering cluster
across hive/presto/trino, tracked separately).
