diff --git a/UPDATING.md b/UPDATING.md index 6713cbfe59bb5..abda7ee5150db 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////` - [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 6bbceb576f2ec..72d9f9a732dd1 100644 --- a/superset/charts/commands/delete.py +++ b/superset/charts/commands/delete.py @@ -47,10 +47,6 @@ def run(self) -> None: 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 6fb257f45a98e..c8dc216a4d2f2 100644 --- a/superset/daos/chart.py +++ b/superset/daos/chart.py @@ -44,7 +44,6 @@ def delete(cls, items: Slice | list[Slice], commit: bool = True) -> None: 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 baeac95f2fb5a..69169e1d02b3d 100644 --- a/superset/daos/dashboard.py +++ b/superset/daos/dashboard.py @@ -189,7 +189,6 @@ def delete(cls, items: Dashboard | list[Dashboard], commit: bool = True) -> None # 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 b753270122644..f4dcbebe9d8e8 100644 --- a/superset/daos/report.py +++ b/superset/daos/report.py @@ -36,7 +36,7 @@ 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 @@ def find_by_database_ids(database_ids: list[int]) -> list[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 8b09aa4bf224a..4ea47b3528cae 100644 --- a/superset/examples/birth_names.py +++ b/superset/examples/birth_names.py @@ -535,7 +535,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 0000000000000..df08f841e5a91 --- /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 when issuing DDL operations + :param onupdate: If set, emit ON UPDATE 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 30de4096d42dc..0eab2b8bb7632 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 @@ 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 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 0000000000000..1303f9b396932 --- /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 649c5a499d8da..e6b91a60d333e 100644 --- a/superset/models/dashboard.py +++ b/superset/models/dashboard.py @@ -115,8 +115,8 @@ def copy_dashboard(_mapper: Mapper, connection: Connection, target: Dashboard) - "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 a6ffb22a087a0..248f4ee947e7d 100644 --- a/superset/models/slice.py +++ b/superset/models/slice.py @@ -58,8 +58,8 @@ "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 2cbcbe0daab4e..a13ded6223743 100644 --- a/superset/reports/models.py +++ b/superset/reports/models.py @@ -91,9 +91,17 @@ class ReportSourceFormat(str, enum.Enum): "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 0b97414055a62..a1dc6382ecbb2 100644 --- a/superset/tables/models.py +++ b/superset/tables/models.py @@ -53,12 +53,12 @@ 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 68b2b55809bac..dc1cce2173e5c 100644 --- a/tests/integration_tests/charts/api_tests.py +++ b/tests/integration_tests/charts/api_tests.py @@ -1503,7 +1503,6 @@ def test_import_chart(self): 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) @@ -1575,7 +1574,6 @@ def test_import_chart_overwrite(self): 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 d0a59a3100439..911f3bf5daa4c 100644 --- a/tests/integration_tests/charts/commands_tests.py +++ b/tests/integration_tests/charts/commands_tests.py @@ -282,8 +282,6 @@ def test_import_v1_chart(self, sm_g, utils_g): 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 e34a04072403c..86ebdc0951032 100644 --- a/tests/integration_tests/commands_test.py +++ b/tests/integration_tests/commands_test.py @@ -146,9 +146,6 @@ def test_import_assets(self): 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 @@ def test_import_v1_dashboard_overwrite(self): 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 6558ccc28e21f..a8cbd97aa2f02 100644 --- a/tests/integration_tests/dashboard_tests.py +++ b/tests/integration_tests/dashboard_tests.py @@ -213,7 +213,6 @@ def test_users_can_view_own_dashboard(self): 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 f9ff4e7dae170..7106a978c4170 100644 --- a/tests/integration_tests/dashboards/commands_tests.py +++ b/tests/integration_tests/dashboards/commands_tests.py @@ -573,9 +573,6 @@ def test_import_v1_dashboard(self, sm_g, utils_g): 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 b5956bd5d247b..b3b5084e35cb0 100644 --- a/tests/integration_tests/datasets/commands_tests.py +++ b/tests/integration_tests/datasets/commands_tests.py @@ -400,7 +400,6 @@ def test_import_v1_dataset(self, sm_g, utils_g): 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 @@ def test_import_v1_dataset_existing_database(self, mock_g): ) assert len(database.tables) == 1 - database.owners = [] db.session.delete(database.tables[0]) db.session.delete(database) db.session.commit()