You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by jo...@apache.org on 2023/07/11 18:39:10 UTC
[superset] branch master updated: chore(dao): Add explicit ON DELETE CASCADE for ownership (#24628)
This is an automated email from the ASF dual-hosted git repository.
johnbodley pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/superset.git
The following commit(s) were added to refs/heads/master by this push:
new ae00489779 chore(dao): Add explicit ON DELETE CASCADE for ownership (#24628)
ae00489779 is described below
commit ae00489779e55629652e7b704173560a8828fffe
Author: John Bodley <45...@users.noreply.github.com>
AuthorDate: Tue Jul 11 11:39:03 2023 -0700
chore(dao): Add explicit ON DELETE CASCADE for ownership (#24628)
---
UPDATING.md | 3 +-
superset/charts/commands/delete.py | 4 -
superset/daos/chart.py | 1 -
superset/daos/dashboard.py | 1 -
superset/daos/report.py | 26 +-----
superset/examples/birth_names.py | 1 -
superset/migrations/shared/constraints.py | 57 ++++++++++++++
..._add_on_delete_cascade_for_tables_references.py | 92 ++++++++--------------
..._add_on_delete_cascade_for_owners_references.py | 78 ++++++++++++++++++
superset/models/dashboard.py | 10 ++-
superset/models/slice.py | 10 ++-
superset/reports/models.py | 18 ++++-
superset/tables/models.py | 4 +-
tests/integration_tests/charts/api_tests.py | 2 -
tests/integration_tests/charts/commands_tests.py | 2 -
tests/integration_tests/commands_test.py | 7 --
tests/integration_tests/dashboard_tests.py | 1 -
.../integration_tests/dashboards/commands_tests.py | 3 -
tests/integration_tests/datasets/commands_tests.py | 2 -
19 files changed, 201 insertions(+), 121 deletions(-)
diff --git a/UPDATING.md b/UPDATING.md
index 6713cbfe59..abda7ee515 100644
--- a/UPDATING.md
+++ b/UPDATING.md
@@ -24,7 +24,8 @@ assists people when migrating to a new version.
## Next
-- [24488](https://github.com/apache/superset/pull/24488): Augments the foreign key constraints for the `sql_metrics`, `sqlatable_user`, and `table_columns` tables which reference the `tables` table to include an explicit CASCADE ON DELETE to ensure the relevant records are deleted when a dataset is deleted. Scheduled downtime may be advised.
+- [24628]https://github.com/apache/superset/pull/24628): Augments the foreign key constraints for the `dashboard_owner`, `report_schedule_owner`, and `slice_owner` tables to include an explicit CASCADE ON DELETE to ensure the relevant ownership records are deleted when a dataset is deleted. Scheduled downtime may be advised.
+- [24488](https://github.com/apache/superset/pull/24488): Augments the foreign key constraints for the `sql_metrics`, `sqlatable_user`, and `table_columns` tables to include an explicit CASCADE ON DELETE to ensure the relevant records are deleted when a dataset is deleted. Scheduled downtime may be advised.
- [24335](https://github.com/apache/superset/pull/24335): Removed deprecated API `/superset/filter/<datasource_type>/<int:datasource_id>/<column>/`
- [24185](https://github.com/apache/superset/pull/24185): `/api/v1/database/test_connection` and `api/v1/database/validate_parameters` permissions changed from `can_read` to `can_write`. Only Admin user's have access.
- [24232](https://github.com/apache/superset/pull/24232): Enables ENABLE_TEMPLATE_REMOVE_FILTERS, DRILL_TO_DETAIL, DASHBOARD_CROSS_FILTERS by default, marks VERSIONED_EXPORT and ENABLE_TEMPLATE_REMOVE_FILTERS as deprecated.
diff --git a/superset/charts/commands/delete.py b/superset/charts/commands/delete.py
index 6bbceb576f..72d9f9a732 100644
--- a/superset/charts/commands/delete.py
+++ b/superset/charts/commands/delete.py
@@ -47,10 +47,6 @@ class DeleteChartCommand(BaseCommand):
self._model = cast(Slice, self._model)
try:
Dashboard.clear_cache_for_slice(slice_id=self._model_id)
- # Even though SQLAlchemy should in theory delete rows from the association
- # table, sporadically Superset will error because the rows are not deleted.
- # Let's do it manually here to prevent the error.
- self._model.owners = []
ChartDAO.delete(self._model)
except DAODeleteFailedError as ex:
logger.exception(ex.exception)
diff --git a/superset/daos/chart.py b/superset/daos/chart.py
index 6fb257f45a..c8dc216a4d 100644
--- a/superset/daos/chart.py
+++ b/superset/daos/chart.py
@@ -44,7 +44,6 @@ class ChartDAO(BaseDAO[Slice]):
item_ids = [item.id for item in get_iterable(items)]
# bulk delete, first delete related data
for item in get_iterable(items):
- item.owners = []
item.dashboards = []
db.session.merge(item)
# bulk delete itself
diff --git a/superset/daos/dashboard.py b/superset/daos/dashboard.py
index baeac95f2f..69169e1d02 100644
--- a/superset/daos/dashboard.py
+++ b/superset/daos/dashboard.py
@@ -189,7 +189,6 @@ class DashboardDAO(BaseDAO[Dashboard]):
# bulk delete, first delete related data
for item in get_iterable(items):
item.slices = []
- item.owners = []
item.embedded = []
db.session.merge(item)
# bulk delete itself
diff --git a/superset/daos/report.py b/superset/daos/report.py
index b753270122..f4dcbebe9d 100644
--- a/superset/daos/report.py
+++ b/superset/daos/report.py
@@ -36,7 +36,7 @@ from superset.reports.models import (
ReportScheduleType,
ReportState,
)
-from superset.utils.core import get_iterable, get_user_id
+from superset.utils.core import get_user_id
logger = logging.getLogger(__name__)
@@ -95,30 +95,6 @@ class ReportScheduleDAO(BaseDAO[ReportSchedule]):
.all()
)
- @classmethod
- def delete(
- cls,
- items: ReportSchedule | list[ReportSchedule],
- commit: bool = True,
- ) -> None:
- item_ids = [item.id for item in get_iterable(items)]
- try:
- # Clean owners secondary table
- report_schedules = (
- db.session.query(ReportSchedule)
- .filter(ReportSchedule.id.in_(item_ids))
- .all()
- )
- for report_schedule in report_schedules:
- report_schedule.owners = []
- for report_schedule in report_schedules:
- db.session.delete(report_schedule)
- if commit:
- db.session.commit()
- except SQLAlchemyError as ex:
- db.session.rollback()
- raise DAODeleteFailedError(str(ex)) from ex
-
@staticmethod
def validate_unique_creation_method(
dashboard_id: int | None = None, chart_id: int | None = None
diff --git a/superset/examples/birth_names.py b/superset/examples/birth_names.py
index f9bbf8a79b..e91640b190 100644
--- a/superset/examples/birth_names.py
+++ b/superset/examples/birth_names.py
@@ -537,7 +537,6 @@ def create_dashboard(slices: list[Slice]) -> Dashboard:
dash = db.session.query(Dashboard).filter_by(slug="births").first()
if not dash:
dash = Dashboard()
- dash.owners = []
db.session.add(dash)
dash.published = True
diff --git a/superset/migrations/shared/constraints.py b/superset/migrations/shared/constraints.py
new file mode 100644
index 0000000000..df08f841e5
--- /dev/null
+++ b/superset/migrations/shared/constraints.py
@@ -0,0 +1,57 @@
+from __future__ import annotations
+
+from dataclasses import dataclass
+
+from alembic import op
+from sqlalchemy.engine.reflection import Inspector
+
+from superset.utils.core import generic_find_fk_constraint_name
+
+
+@dataclass(frozen=True)
+class ForeignKey:
+ table: str
+ referent_table: str
+ local_cols: list[str]
+ remote_cols: list[str]
+
+ @property
+ def constraint_name(self) -> str:
+ return f"fk_{self.table}_{self.local_cols[0]}_{self.referent_table}"
+
+
+def redefine(
+ foreign_key: ForeignKey,
+ on_delete: str | None = None,
+ on_update: str | None = None,
+) -> None:
+ """
+ Redefine the foreign key constraint to include the ON DELETE and ON UPDATE
+ constructs for cascading purposes.
+
+ :params foreign_key: The foreign key constraint
+ :param ondelete: If set, emit ON DELETE <value> when issuing DDL operations
+ :param onupdate: If set, emit ON UPDATE <value> when issuing DDL operations
+ """
+
+ bind = op.get_bind()
+ insp = Inspector.from_engine(bind)
+ conv = {"fk": "fk_%(table_name)s_%(column_0_name)s_%(referred_table_name)s"}
+
+ with op.batch_alter_table(foreign_key.table, naming_convention=conv) as batch_op:
+ if constraint := generic_find_fk_constraint_name(
+ table=foreign_key.table,
+ columns=set(foreign_key.remote_cols),
+ referenced=foreign_key.referent_table,
+ insp=insp,
+ ):
+ batch_op.drop_constraint(constraint, type_="foreignkey")
+
+ batch_op.create_foreign_key(
+ constraint_name=foreign_key.constraint_name,
+ referent_table=foreign_key.referent_table,
+ local_cols=foreign_key.local_cols,
+ remote_cols=foreign_key.remote_cols,
+ ondelete=on_delete,
+ onupdate=on_update,
+ )
diff --git a/superset/migrations/versions/2023-06-22_13-39_6fbe660cac39_add_on_delete_cascade_for_tables_references.py b/superset/migrations/versions/2023-06-22_13-39_6fbe660cac39_add_on_delete_cascade_for_tables_references.py
index 30de4096d4..0eab2b8bb7 100644
--- a/superset/migrations/versions/2023-06-22_13-39_6fbe660cac39_add_on_delete_cascade_for_tables_references.py
+++ b/superset/migrations/versions/2023-06-22_13-39_6fbe660cac39_add_on_delete_cascade_for_tables_references.py
@@ -21,74 +21,46 @@ Revises: 90139bf715e4
Create Date: 2023-06-22 13:39:47.989373
"""
-from __future__ import annotations
# revision identifiers, used by Alembic.
revision = "6fbe660cac39"
down_revision = "90139bf715e4"
-import sqlalchemy as sa
-from alembic import op
-
-from superset.utils.core import generic_find_fk_constraint_name
-
-
-def migrate(ondelete: str | None) -> None:
- """
- Redefine the foreign key constraints, via a successive DROP and ADD, for all tables
- related to the `tables` table to include the ON DELETE construct for cascading
- purposes.
-
- :param ondelete: If set, emit ON DELETE <value> when issuing DDL for this constraint
- """
-
- bind = op.get_bind()
- insp = sa.engine.reflection.Inspector.from_engine(bind)
-
- conv = {
- "fk": "fk_%(table_name)s_%(column_0_name)s_%(referred_table_name)s",
- }
-
- for table in ("sql_metrics", "table_columns"):
- with op.batch_alter_table(table, naming_convention=conv) as batch_op:
- if constraint := generic_find_fk_constraint_name(
- table=table,
- columns={"id"},
- referenced="tables",
- insp=insp,
- ):
- batch_op.drop_constraint(constraint, type_="foreignkey")
-
- batch_op.create_foreign_key(
- constraint_name=f"fk_{table}_table_id_tables",
- referent_table="tables",
- local_cols=["table_id"],
- remote_cols=["id"],
- ondelete=ondelete,
- )
-
- with op.batch_alter_table("sqlatable_user", naming_convention=conv) as batch_op:
- for table, column in zip(("ab_user", "tables"), ("user_id", "table_id")):
- if constraint := generic_find_fk_constraint_name(
- table="sqlatable_user",
- columns={"id"},
- referenced=table,
- insp=insp,
- ):
- batch_op.drop_constraint(constraint, type_="foreignkey")
-
- batch_op.create_foreign_key(
- constraint_name=f"fk_sqlatable_user_{column}_{table}",
- referent_table=table,
- local_cols=[column],
- remote_cols=["id"],
- ondelete=ondelete,
- )
+from superset.migrations.shared.constraints import ForeignKey, redefine
+
+foreign_keys = [
+ ForeignKey(
+ table="sql_metrics",
+ referent_table="tables",
+ local_cols=["table_id"],
+ remote_cols=["id"],
+ ),
+ ForeignKey(
+ table="table_columns",
+ referent_table="tables",
+ local_cols=["table_id"],
+ remote_cols=["id"],
+ ),
+ ForeignKey(
+ table="sqlatable_user",
+ referent_table="ab_user",
+ local_cols=["user_id"],
+ remote_cols=["id"],
+ ),
+ ForeignKey(
+ table="sqlatable_user",
+ referent_table="tables",
+ local_cols=["table_id"],
+ remote_cols=["id"],
+ ),
+]
def upgrade():
- migrate(ondelete="CASCADE")
+ for foreign_key in foreign_keys:
+ redefine(foreign_key, on_delete="CASCADE")
def downgrade():
- migrate(ondelete=None)
+ for foreign_key in foreign_keys:
+ redefine(foreign_key)
diff --git a/superset/migrations/versions/2023-07-11_15-51_6d05b0a70c89_add_on_delete_cascade_for_owners_references.py b/superset/migrations/versions/2023-07-11_15-51_6d05b0a70c89_add_on_delete_cascade_for_owners_references.py
new file mode 100644
index 0000000000..1303f9b396
--- /dev/null
+++ b/superset/migrations/versions/2023-07-11_15-51_6d05b0a70c89_add_on_delete_cascade_for_owners_references.py
@@ -0,0 +1,78 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""add on delete cascade for owners references
+
+Revision ID: 6d05b0a70c89
+Revises: f92a3124dd66
+Create Date: 2023-07-11 15:51:57.964635
+
+"""
+
+# revision identifiers, used by Alembic.
+revision = "6d05b0a70c89"
+down_revision = "f92a3124dd66"
+
+from superset.migrations.shared.constraints import ForeignKey, redefine
+
+foreign_keys = [
+ ForeignKey(
+ table="dashboard_user",
+ referent_table="ab_user",
+ local_cols=["user_id"],
+ remote_cols=["id"],
+ ),
+ ForeignKey(
+ table="dashboard_user",
+ referent_table="dashboards",
+ local_cols=["dashboard_id"],
+ remote_cols=["id"],
+ ),
+ ForeignKey(
+ table="report_schedule_user",
+ referent_table="ab_user",
+ local_cols=["user_id"],
+ remote_cols=["id"],
+ ),
+ ForeignKey(
+ table="report_schedule_user",
+ referent_table="report_schedule",
+ local_cols=["report_schedule_id"],
+ remote_cols=["id"],
+ ),
+ ForeignKey(
+ table="slice_user",
+ referent_table="ab_user",
+ local_cols=["user_id"],
+ remote_cols=["id"],
+ ),
+ ForeignKey(
+ table="slice_user",
+ referent_table="slices",
+ local_cols=["slice_id"],
+ remote_cols=["id"],
+ ),
+]
+
+
+def upgrade():
+ for foreign_key in foreign_keys:
+ redefine(foreign_key, on_delete="CASCADE")
+
+
+def downgrade():
+ for foreign_key in foreign_keys:
+ redefine(foreign_key)
diff --git a/superset/models/dashboard.py b/superset/models/dashboard.py
index 649c5a499d..e6b91a60d3 100644
--- a/superset/models/dashboard.py
+++ b/superset/models/dashboard.py
@@ -115,8 +115,8 @@ dashboard_user = Table(
"dashboard_user",
metadata,
Column("id", Integer, primary_key=True),
- Column("user_id", Integer, ForeignKey("ab_user.id")),
- Column("dashboard_id", Integer, ForeignKey("dashboards.id")),
+ Column("user_id", Integer, ForeignKey("ab_user.id", ondelete="CASCADE")),
+ Column("dashboard_id", Integer, ForeignKey("dashboards.id", ondelete="CASCADE")),
)
@@ -146,7 +146,11 @@ class Dashboard(Model, AuditMixinNullable, ImportExportMixin):
slices: list[Slice] = relationship(
Slice, secondary=dashboard_slices, backref="dashboards"
)
- owners = relationship(security_manager.user_model, secondary=dashboard_user)
+ owners = relationship(
+ security_manager.user_model,
+ secondary=dashboard_user,
+ passive_deletes=True,
+ )
tags = relationship(
"Tag",
overlaps="objects,tag,tags,tags",
diff --git a/superset/models/slice.py b/superset/models/slice.py
index a6ffb22a08..248f4ee947 100644
--- a/superset/models/slice.py
+++ b/superset/models/slice.py
@@ -58,8 +58,8 @@ slice_user = Table(
"slice_user",
metadata,
Column("id", Integer, primary_key=True),
- Column("user_id", Integer, ForeignKey("ab_user.id")),
- Column("slice_id", Integer, ForeignKey("slices.id")),
+ Column("user_id", Integer, ForeignKey("ab_user.id", ondelete="CASCADE")),
+ Column("slice_id", Integer, ForeignKey("slices.id", ondelete="CASCADE")),
)
logger = logging.getLogger(__name__)
@@ -95,7 +95,11 @@ class Slice( # pylint: disable=too-many-public-methods
last_saved_by = relationship(
security_manager.user_model, foreign_keys=[last_saved_by_fk]
)
- owners = relationship(security_manager.user_model, secondary=slice_user)
+ owners = relationship(
+ security_manager.user_model,
+ secondary=slice_user,
+ passive_deletes=True,
+ )
tags = relationship(
"Tag",
secondary="tagged_object",
diff --git a/superset/reports/models.py b/superset/reports/models.py
index 2cbcbe0daa..a13ded6223 100644
--- a/superset/reports/models.py
+++ b/superset/reports/models.py
@@ -91,9 +91,17 @@ report_schedule_user = Table(
"report_schedule_user",
metadata,
Column("id", Integer, primary_key=True),
- Column("user_id", Integer, ForeignKey("ab_user.id"), nullable=False),
Column(
- "report_schedule_id", Integer, ForeignKey("report_schedule.id"), nullable=False
+ "user_id",
+ Integer,
+ ForeignKey("ab_user.id", ondelete="CASCADE"),
+ nullable=False,
+ ),
+ Column(
+ "report_schedule_id",
+ Integer,
+ ForeignKey("report_schedule.id", ondelete="CASCADE"),
+ nullable=False,
),
UniqueConstraint("user_id", "report_schedule_id"),
)
@@ -132,7 +140,11 @@ class ReportSchedule(Model, AuditMixinNullable, ExtraJSONMixin):
# (Alerts) M-O to database
database_id = Column(Integer, ForeignKey("dbs.id"), nullable=True)
database = relationship(Database, foreign_keys=[database_id])
- owners = relationship(security_manager.user_model, secondary=report_schedule_user)
+ owners = relationship(
+ security_manager.user_model,
+ secondary=report_schedule_user,
+ passive_deletes=True,
+ )
# (Alerts) Stamped last observations
last_eval_dttm = Column(DateTime)
diff --git a/superset/tables/models.py b/superset/tables/models.py
index 0b97414055..a1dc6382ec 100644
--- a/superset/tables/models.py
+++ b/superset/tables/models.py
@@ -53,12 +53,12 @@ table_column_association_table = sa.Table(
Model.metadata, # pylint: disable=no-member
sa.Column(
"table_id",
- sa.ForeignKey("sl_tables.id", ondelete="cascade"),
+ sa.ForeignKey("sl_tables.id", ondelete="CASCADE"),
primary_key=True,
),
sa.Column(
"column_id",
- sa.ForeignKey("sl_columns.id", ondelete="cascade"),
+ sa.ForeignKey("sl_columns.id", ondelete="CASCADE"),
primary_key=True,
),
)
diff --git a/tests/integration_tests/charts/api_tests.py b/tests/integration_tests/charts/api_tests.py
index 6648dc3745..f09662a8de 100644
--- a/tests/integration_tests/charts/api_tests.py
+++ b/tests/integration_tests/charts/api_tests.py
@@ -1528,7 +1528,6 @@ class TestChartApi(SupersetTestCase, ApiOwnersTestCaseMixin, InsertChartMixin):
chart = db.session.query(Slice).filter_by(uuid=chart_config["uuid"]).one()
assert chart.table == dataset
- chart.owners = []
db.session.delete(chart)
db.session.commit()
db.session.delete(dataset)
@@ -1600,7 +1599,6 @@ class TestChartApi(SupersetTestCase, ApiOwnersTestCaseMixin, InsertChartMixin):
dataset = database.tables[0]
chart = db.session.query(Slice).filter_by(uuid=chart_config["uuid"]).one()
- chart.owners = []
db.session.delete(chart)
db.session.commit()
db.session.delete(dataset)
diff --git a/tests/integration_tests/charts/commands_tests.py b/tests/integration_tests/charts/commands_tests.py
index d0a59a3100..911f3bf5da 100644
--- a/tests/integration_tests/charts/commands_tests.py
+++ b/tests/integration_tests/charts/commands_tests.py
@@ -282,8 +282,6 @@ class TestImportChartsCommand(SupersetTestCase):
assert chart.owners == [admin]
- chart.owners = []
- database.owners = []
db.session.delete(chart)
db.session.delete(dataset)
db.session.delete(database)
diff --git a/tests/integration_tests/commands_test.py b/tests/integration_tests/commands_test.py
index e34a040724..86ebdc0951 100644
--- a/tests/integration_tests/commands_test.py
+++ b/tests/integration_tests/commands_test.py
@@ -146,9 +146,6 @@ class TestImportAssetsCommand(SupersetTestCase):
assert dashboard.owners == [self.user]
- dashboard.owners = []
- chart.owners = []
- database.owners = []
db.session.delete(dashboard)
db.session.delete(chart)
db.session.delete(dataset)
@@ -190,10 +187,6 @@ class TestImportAssetsCommand(SupersetTestCase):
chart = dashboard.slices[0]
dataset = chart.table
database = dataset.database
- dashboard.owners = []
-
- chart.owners = []
- database.owners = []
db.session.delete(dashboard)
db.session.delete(chart)
db.session.delete(dataset)
diff --git a/tests/integration_tests/dashboard_tests.py b/tests/integration_tests/dashboard_tests.py
index 6558ccc28e..a8cbd97aa2 100644
--- a/tests/integration_tests/dashboard_tests.py
+++ b/tests/integration_tests/dashboard_tests.py
@@ -213,7 +213,6 @@ class TestDashboard(SupersetTestCase):
hidden_dash.dashboard_title = "Not My Dashboard"
hidden_dash.slug = not_my_dash_slug
hidden_dash.slices = []
- hidden_dash.owners = []
db.session.add(dash)
db.session.add(hidden_dash)
diff --git a/tests/integration_tests/dashboards/commands_tests.py b/tests/integration_tests/dashboards/commands_tests.py
index f9ff4e7dae..7106a978c4 100644
--- a/tests/integration_tests/dashboards/commands_tests.py
+++ b/tests/integration_tests/dashboards/commands_tests.py
@@ -573,9 +573,6 @@ class TestImportDashboardsCommand(SupersetTestCase):
assert dashboard.owners == [admin]
- dashboard.owners = []
- chart.owners = []
- database.owners = []
db.session.delete(dashboard)
db.session.delete(chart)
db.session.delete(dataset)
diff --git a/tests/integration_tests/datasets/commands_tests.py b/tests/integration_tests/datasets/commands_tests.py
index b5956bd5d2..b3b5084e35 100644
--- a/tests/integration_tests/datasets/commands_tests.py
+++ b/tests/integration_tests/datasets/commands_tests.py
@@ -400,7 +400,6 @@ class TestImportDatasetsCommand(SupersetTestCase):
assert column.description is None
assert column.python_date_format is None
- dataset.database.owners = []
db.session.delete(dataset)
db.session.delete(dataset.database)
db.session.commit()
@@ -528,7 +527,6 @@ class TestImportDatasetsCommand(SupersetTestCase):
)
assert len(database.tables) == 1
- database.owners = []
db.session.delete(database.tables[0])
db.session.delete(database)
db.session.commit()