Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion superset/commands/importers/v1/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -361,7 +361,7 @@ def safe_insert_dashboard_chart_relationships(
# Get existing relationships only for dashboards being updated
dashboard_ids = {dashboard_id for dashboard_id, _ in dashboard_chart_ids}
existing_relationships = db.session.execute(
select([dashboard_slices.c.dashboard_id, dashboard_slices.c.slice_id]).where(
select(dashboard_slices.c.dashboard_id, dashboard_slices.c.slice_id).where(
dashboard_slices.c.dashboard_id.in_(dashboard_ids)
)
).fetchall()
Expand Down
76 changes: 29 additions & 47 deletions superset/common/tags.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,9 @@ def add_types_to_charts(

charts = (
select(
[
tag.c.id.label("tag_id"),
slices.c.id.label("object_id"),
literal(ObjectType.chart.name).label("object_type"),
]
tag.c.id.label("tag_id"),
slices.c.id.label("object_id"),
literal(ObjectType.chart.name).label("object_type"),
)
.select_from(
join(
Expand Down Expand Up @@ -64,11 +62,9 @@ def add_types_to_dashboards(

dashboards = (
select(
[
tag.c.id.label("tag_id"),
dashboard_table.c.id.label("object_id"),
literal(ObjectType.dashboard.name).label("object_type"),
]
tag.c.id.label("tag_id"),
dashboard_table.c.id.label("object_id"),
literal(ObjectType.dashboard.name).label("object_type"),
)
.select_from(
join(
Expand Down Expand Up @@ -96,11 +92,9 @@ def add_types_to_saved_queries(

saved_queries = (
select(
[
tag.c.id.label("tag_id"),
saved_query.c.id.label("object_id"),
literal(ObjectType.query.name).label("object_type"),
]
tag.c.id.label("tag_id"),
saved_query.c.id.label("object_id"),
literal(ObjectType.query.name).label("object_type"),
)
.select_from(
join(
Expand Down Expand Up @@ -128,11 +122,9 @@ def add_types_to_datasets(

datasets = (
select(
[
tag.c.id.label("tag_id"),
tables.c.id.label("object_id"),
literal(ObjectType.dataset.name).label("object_type"),
]
tag.c.id.label("tag_id"),
tables.c.id.label("object_id"),
literal(ObjectType.dataset.name).label("object_type"),
)
.select_from(
join(
Expand Down Expand Up @@ -238,11 +230,9 @@ def add_owners_to_charts(

charts = (
select(
[
tag.c.id.label("tag_id"),
slices.c.id.label("object_id"),
literal(ObjectType.chart.name).label("object_type"),
]
tag.c.id.label("tag_id"),
slices.c.id.label("object_id"),
literal(ObjectType.chart.name).label("object_type"),
)
.select_from(
join(
Expand Down Expand Up @@ -274,11 +264,9 @@ def add_owners_to_dashboards(

dashboards = (
select(
[
tag.c.id.label("tag_id"),
dashboard_table.c.id.label("object_id"),
literal(ObjectType.dashboard.name).label("object_type"),
]
tag.c.id.label("tag_id"),
dashboard_table.c.id.label("object_id"),
literal(ObjectType.dashboard.name).label("object_type"),
)
.select_from(
join(
Expand Down Expand Up @@ -310,11 +298,9 @@ def add_owners_to_saved_queries(

saved_queries = (
select(
[
tag.c.id.label("tag_id"),
saved_query.c.id.label("object_id"),
literal(ObjectType.query.name).label("object_type"),
]
tag.c.id.label("tag_id"),
saved_query.c.id.label("object_id"),
literal(ObjectType.query.name).label("object_type"),
)
.select_from(
join(
Expand Down Expand Up @@ -346,11 +332,9 @@ def add_owners_to_datasets(

datasets = (
select(
[
tag.c.id.label("tag_id"),
tables.c.id.label("object_id"),
literal(ObjectType.dataset.name).label("object_type"),
]
tag.c.id.label("tag_id"),
tables.c.id.label("object_id"),
literal(ObjectType.dataset.name).label("object_type"),
)
.select_from(
join(
Expand Down Expand Up @@ -440,7 +424,7 @@ def add_owners(metadata: MetaData) -> None:
columns = ["tag_id", "object_id", "object_type"]

# create a custom tag for each user
ids = select([users.c.id])
ids = select(users.c.id)
insert = tag.insert()
for (id_,) in db.session.execute(ids):
with contextlib.suppress(IntegrityError): # already exists
Expand Down Expand Up @@ -478,18 +462,16 @@ def add_favorites(metadata: MetaData) -> None:
columns = ["tag_id", "object_id", "object_type"]

# create a custom tag for each user
ids = select([users.c.id])
ids = select(users.c.id)
insert = tag.insert()
for (id_,) in db.session.execute(ids):
with contextlib.suppress(IntegrityError): # already exists
db.session.execute(insert, name=f"favorited_by:{id_}", type=TagType.type)
favstars = (
select(
[
tag.c.id.label("tag_id"),
favstar.c.obj_id.label("object_id"),
func.lower(favstar.c.class_name).label("object_type"),
]
tag.c.id.label("tag_id"),
favstar.c.obj_id.label("object_id"),
func.lower(favstar.c.class_name).label("object_type"),
)
.select_from(
join(
Expand Down
6 changes: 2 additions & 4 deletions superset/connectors/sqla/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -1695,11 +1695,9 @@ def adhoc_column_to_sqla( # pylint: disable=too-many-locals
# for those we fall back to LIMIT 1.
tbl, _unused_cte = self.get_from_clause(template_processor)
if self.db_engine_spec.type_probe_needs_row:
qry = sa.select([sqla_column]).limit(1).select_from(tbl)
qry = sa.select(sqla_column).limit(1).select_from(tbl)
else:
qry = (
sa.select([sqla_column]).where(sa.false()).select_from(tbl)
)
qry = sa.select(sqla_column).where(sa.false()).select_from(tbl)
sql = self.database.compile_sqla_query(
qry,
catalog=self.catalog,
Expand Down
4 changes: 2 additions & 2 deletions superset/extensions/metadb.py
Original file line number Diff line number Diff line change
Expand Up @@ -361,7 +361,7 @@ def _build_sql(
"""
Build SQLAlchemy query object.
"""
query = select([self._table])
query = select(self._table)

for column_name, filter_ in bounds.items():
column = self._table.c[column_name]
Expand Down Expand Up @@ -445,7 +445,7 @@ def insert_row(self, row: Row) -> int:
if self._rowid:
return result.inserted_primary_key[0]

query = select([func.count()]).select_from(self._table)
query = select(func.count()).select_from(self._table)
return connection.execute(query).scalar()

@check_dml
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ def upgrade():
clusters = sa.Table("clusters", metadata, autoload=True)

statement = datasources.update().values(
cluster_id=sa.select([clusters.c.id])
cluster_id=sa.select(clusters.c.id)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: This subquery is still chained with .as_scalar() in the following line, which is removed in SQLAlchemy 2.x for Select objects. The migration will fail at runtime when building the update statement; switch this subquery to scalar_subquery() so upgrade runs correctly under SQLAlchemy 2. [possible bug]

Severity Level: Critical 🚨
- ❌ Database upgrade fails when executing migration e96dbf2cfef0 upgrade().
- ⚠️ Superset cannot start until migrations complete successfully.
Steps of Reproduction ✅
1. Install Superset with this PR applied and SQLAlchemy 2.x as the `sqlalchemy` dependency
(per the PR goal to bump SQLAlchemy to version 2).

2. Ensure the database is at a revision earlier than `e96dbf2cfef0` so that this migration
must run (new database or older Superset instance).

3. Run database migrations using the standard Alembic environment at
`superset/migrations/env.py:64-85` (offline) or `superset/migrations/env.py:87-137`
(online), typically via the `superset db upgrade` command referenced in
`superset/app.py:175-177`.

4. When Alembic processes revision `e96dbf2cfef0` it calls `upgrade()` at
`superset/migrations/versions/2020-01-08_01-17_e96dbf2cfef0_datasource_cluster_fk.py:38`.
Inside this function, the update statement is built at lines 51-55 using
`cluster_id=sa.select(clusters.c.id).where(...).as_scalar()`. Under SQLAlchemy 2.x
`sa.select()` returns a `Select` object without an `as_scalar()` method, so the call at
line 54 raises `AttributeError: 'Select' object has no attribute 'as_scalar'`, causing the
migration (and thus the overall upgrade) to fail.

Fix in Cursor | Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** superset/migrations/versions/2020-01-08_01-17_e96dbf2cfef0_datasource_cluster_fk.py
**Line:** 52:52
**Comment:**
	*Possible Bug: This subquery is still chained with `.as_scalar()` in the following line, which is removed in SQLAlchemy 2.x for `Select` objects. The migration will fail at runtime when building the update statement; switch this subquery to `scalar_subquery()` so upgrade runs correctly under SQLAlchemy 2.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
👍 | 👎

.where(datasources.c.cluster_name == clusters.c.cluster_name)
.as_scalar()
)
Expand Down Expand Up @@ -91,7 +91,7 @@ def downgrade():
clusters = sa.Table("clusters", metadata, autoload=True)

statement = datasources.update().values(
cluster_name=sa.select([clusters.c.cluster_name])
cluster_name=sa.select(clusters.c.cluster_name)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: The downgrade statement also keeps using .as_scalar() after this select, which is not supported in SQLAlchemy 2.x and will break rollback execution. Replace the scalar conversion with scalar_subquery() to keep downgrade functional. [possible bug]

Severity Level: Major ⚠️
- ❌ Database downgrade to before e96dbf2cfef0 raises runtime error.
- ⚠️ Rollback procedures relying on Alembic downgrade become unreliable.
Steps of Reproduction ✅
1. Start from a Superset instance whose database has already been migrated past revision
`e96dbf2cfef0` (so `upgrade()` in this file has run previously and `cluster_id` exists).

2. Invoke a database downgrade to a revision earlier than `e96dbf2cfef0` using the Alembic
environment in `superset/migrations/env.py:64-85` or `87-137` (e.g., via a `superset db
downgrade <rev>` or equivalent Alembic command wired through Flask-Migrate).

3. Alembic loads revision `e96dbf2cfef0` from
`superset/migrations/versions/2020-01-08_01-17_e96dbf2cfef0_datasource_cluster_fk.py` and
calls its `downgrade()` function at line 80 as part of the downgrade path.

4. In `downgrade()`, the rollback data migration builds an update at lines 93-97 using
`cluster_name=sa.select(clusters.c.cluster_name).where(...).as_scalar()`. Under SQLAlchemy
2.x the `Select` returned by `sa.select()` no longer has `as_scalar()`, so the method call
at line 96 raises `AttributeError`, aborting the downgrade before it can restore
`cluster_name` and drop `cluster_id`.

Fix in Cursor | Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** superset/migrations/versions/2020-01-08_01-17_e96dbf2cfef0_datasource_cluster_fk.py
**Line:** 94:94
**Comment:**
	*Possible Bug: The downgrade statement also keeps using `.as_scalar()` after this `select`, which is not supported in SQLAlchemy 2.x and will break rollback execution. Replace the scalar conversion with `scalar_subquery()` to keep downgrade functional.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
👍 | 👎

.where(datasources.c.cluster_id == clusters.c.id)
.as_scalar()
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ def upgrade():
)

rlsf = sa.Table("row_level_security_filters", metadata, autoload=True)
filter_ids = sa.select([rlsf.c.id, rlsf.c.table_id])
filter_ids = sa.select(rlsf.c.id, rlsf.c.table_id)

for row in bind.execute(filter_ids):
move_table_id = rls_filter_tables.insert().values(
Expand Down Expand Up @@ -85,7 +85,7 @@ def downgrade():
rls_filter_tables = sa.Table("rls_filter_tables", metadata, autoload=True)
rls_filter_roles = sa.Table("rls_filter_roles", metadata, autoload=True)

filter_tables = sa.select([rls_filter_tables.c.rls_filter_id]).group_by(
filter_tables = sa.select(rls_filter_tables.c.rls_filter_id).group_by(
rls_filter_tables.c.rls_filter_id
)

Expand All @@ -95,7 +95,7 @@ def downgrade():
filter_params = dict(bind.execute(filter_query).fetchone())
origin_id = filter_params.pop("id", None)
table_ids = bind.execute(
sa.select([rls_filter_tables.c.table_id]).where(
sa.select(rls_filter_tables.c.table_id).where(
rls_filter_tables.c.rls_filter_id == row["rls_filter_id"]
)
).fetchall()
Expand Down
Loading
Loading