From b295654e949aab2bdc592ae7539286d55d1fbdb7 Mon Sep 17 00:00:00 2001 From: Jack Fragassi Date: Fri, 4 Aug 2023 17:05:30 -0700 Subject: [PATCH 1/8] Change to single delete method --- superset/daos/chart.py | 6 ++---- superset/daos/dashboard.py | 6 ++---- superset/daos/dataset.py | 13 +------------ 3 files changed, 5 insertions(+), 20 deletions(-) diff --git a/superset/daos/chart.py b/superset/daos/chart.py index c8dc216a4d2f2..b4b027f523973 100644 --- a/superset/daos/chart.py +++ b/superset/daos/chart.py @@ -41,16 +41,14 @@ class ChartDAO(BaseDAO[Slice]): @classmethod 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.dashboards = [] db.session.merge(item) # bulk delete itself try: - db.session.query(Slice).filter(Slice.id.in_(item_ids)).delete( - synchronize_session="fetch" - ) + for item in get_iterable(items): + db.session.delete(item) if commit: db.session.commit() except SQLAlchemyError as ex: diff --git a/superset/daos/dashboard.py b/superset/daos/dashboard.py index 69169e1d02b3d..ee443bdf5449f 100644 --- a/superset/daos/dashboard.py +++ b/superset/daos/dashboard.py @@ -185,7 +185,6 @@ def update_charts_owners(model: Dashboard, commit: bool = True) -> Dashboard: @classmethod def delete(cls, items: Dashboard | list[Dashboard], 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.slices = [] @@ -193,9 +192,8 @@ def delete(cls, items: Dashboard | list[Dashboard], commit: bool = True) -> None db.session.merge(item) # bulk delete itself try: - db.session.query(Dashboard).filter(Dashboard.id.in_(item_ids)).delete( - synchronize_session="fetch" - ) + for item in get_iterable(items): + db.session.delete(item) if commit: db.session.commit() except SQLAlchemyError as ex: diff --git a/superset/daos/dataset.py b/superset/daos/dataset.py index b5d32f02af851..09d3929c82dd5 100644 --- a/superset/daos/dataset.py +++ b/superset/daos/dataset.py @@ -369,9 +369,6 @@ def delete( """ Delete the specified items(s) including their associated relationships. - Note that bulk deletion via `delete` does not dispatch the `after_delete` event - and thus the ORM event listener callback needs to be invoked manually. - :param items: The item(s) to delete :param commit: Whether to commit the transaction :raises DAODeleteFailedError: If the deletion failed @@ -379,16 +376,8 @@ def delete( """ try: - db.session.query(SqlaTable).filter( - SqlaTable.id.in_(item.id for item in get_iterable(items)) - ).delete(synchronize_session="fetch") - - connection = db.session.connection() - mapper = next(iter(cls.model_cls.registry.mappers)) # type: ignore - for item in get_iterable(items): - security_manager.dataset_after_delete(mapper, connection, item) - + db.session.delete(item) if commit: db.session.commit() except SQLAlchemyError as ex: From ab9f197ee3e43abb1cf5c79d6e79be04f3971206 Mon Sep 17 00:00:00 2001 From: Jack Fragassi Date: Fri, 4 Aug 2023 18:22:36 -0700 Subject: [PATCH 2/8] Fix test --- superset/daos/dataset.py | 1 - tests/integration_tests/datasets/api_tests.py | 5 ++++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/superset/daos/dataset.py b/superset/daos/dataset.py index 09d3929c82dd5..9641eadd7fb28 100644 --- a/superset/daos/dataset.py +++ b/superset/daos/dataset.py @@ -21,7 +21,6 @@ from sqlalchemy.exc import SQLAlchemyError -from superset import security_manager from superset.connectors.sqla.models import SqlaTable, SqlMetric, TableColumn from superset.daos.base import BaseDAO from superset.extensions import db diff --git a/tests/integration_tests/datasets/api_tests.py b/tests/integration_tests/datasets/api_tests.py index 027002507aece..1d28f452a99bd 100644 --- a/tests/integration_tests/datasets/api_tests.py +++ b/tests/integration_tests/datasets/api_tests.py @@ -153,7 +153,8 @@ def create_datasets(self): # rollback changes for dataset in datasets: - db.session.delete(dataset) + if not hasattr(dataset, "_deleted"): + db.session.delete(dataset) db.session.commit() @staticmethod @@ -1711,6 +1712,8 @@ def test_bulk_delete_dataset_items(self): assert rv.status_code == 200 expected_response = {"message": f"Deleted {len(datasets)} datasets"} assert data == expected_response + for dataset in datasets: + setattr(dataset, "_deleted", True) datasets = ( db.session.query(SqlaTable) .filter(SqlaTable.table_name.in_(self.fixture_tables_names)) From 5c5e18dba7ee4b38734dadcef463db862819c159 Mon Sep 17 00:00:00 2001 From: Jack Fragassi Date: Fri, 4 Aug 2023 18:24:14 -0700 Subject: [PATCH 3/8] Remove unnecessary override of delete in DatasetDAO --- superset/daos/dataset.py | 25 ------------------------- 1 file changed, 25 deletions(-) diff --git a/superset/daos/dataset.py b/superset/daos/dataset.py index 9641eadd7fb28..e81cb024fdbbc 100644 --- a/superset/daos/dataset.py +++ b/superset/daos/dataset.py @@ -359,31 +359,6 @@ def create_metric( """ return DatasetMetricDAO.create(properties, commit=commit) - @classmethod - def delete( - cls, - items: SqlaTable | list[SqlaTable], - commit: bool = True, - ) -> None: - """ - Delete the specified items(s) including their associated relationships. - - :param items: The item(s) to delete - :param commit: Whether to commit the transaction - :raises DAODeleteFailedError: If the deletion failed - :see: https://docs.sqlalchemy.org/en/latest/orm/queryguide/dml.html - """ - - try: - for item in get_iterable(items): - db.session.delete(item) - if commit: - db.session.commit() - except SQLAlchemyError as ex: - if commit: - db.session.rollback() - raise ex - @staticmethod def get_table_by_name(database_id: int, table_name: str) -> SqlaTable | None: return ( From 2194ea35bc3261c188e1d2e6ea4c76c9b0eeff20 Mon Sep 17 00:00:00 2001 From: Jack Fragassi Date: Fri, 4 Aug 2023 18:26:26 -0700 Subject: [PATCH 4/8] CFix test --- tests/integration_tests/datasets/api_tests.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/integration_tests/datasets/api_tests.py b/tests/integration_tests/datasets/api_tests.py index 1d28f452a99bd..a9d53014033c9 100644 --- a/tests/integration_tests/datasets/api_tests.py +++ b/tests/integration_tests/datasets/api_tests.py @@ -1712,14 +1712,15 @@ def test_bulk_delete_dataset_items(self): assert rv.status_code == 200 expected_response = {"message": f"Deleted {len(datasets)} datasets"} assert data == expected_response - for dataset in datasets: - setattr(dataset, "_deleted", True) + deleted_datasets = datasets datasets = ( db.session.query(SqlaTable) .filter(SqlaTable.table_name.in_(self.fixture_tables_names)) .all() ) assert datasets == [] + for dataset in deleted_datasets: + setattr(dataset, "_deleted", True) # Assert permissions get cleaned for view_menu_name in view_menu_names: assert security_manager.find_view_menu(view_menu_name) is None From 296c198a412fb581e31d861ae7da6afb1e44d104 Mon Sep 17 00:00:00 2001 From: Jack Fragassi Date: Fri, 4 Aug 2023 18:32:46 -0700 Subject: [PATCH 5/8] Fix lint --- superset/daos/dataset.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/superset/daos/dataset.py b/superset/daos/dataset.py index e81cb024fdbbc..08279565c8978 100644 --- a/superset/daos/dataset.py +++ b/superset/daos/dataset.py @@ -27,7 +27,7 @@ from superset.models.core import Database from superset.models.dashboard import Dashboard from superset.models.slice import Slice -from superset.utils.core import DatasourceType, get_iterable +from superset.utils.core import DatasourceType from superset.views.base import DatasourceFilter logger = logging.getLogger(__name__) From 72dadf984c38fbc6a38e326abc0e13daa0f89f03 Mon Sep 17 00:00:00 2001 From: Jack Fragassi Date: Mon, 7 Aug 2023 10:01:17 -0700 Subject: [PATCH 6/8] Better method of checking deletion status in tests --- tests/integration_tests/datasets/api_tests.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/tests/integration_tests/datasets/api_tests.py b/tests/integration_tests/datasets/api_tests.py index a9d53014033c9..5e0c23ced6053 100644 --- a/tests/integration_tests/datasets/api_tests.py +++ b/tests/integration_tests/datasets/api_tests.py @@ -25,6 +25,7 @@ import prison import pytest import yaml +from sqlalchemy import inspect from sqlalchemy.orm import joinedload from sqlalchemy.sql import func @@ -153,7 +154,8 @@ def create_datasets(self): # rollback changes for dataset in datasets: - if not hasattr(dataset, "_deleted"): + state = inspect(dataset) + if not state.was_deleted: db.session.delete(dataset) db.session.commit() @@ -1712,15 +1714,12 @@ def test_bulk_delete_dataset_items(self): assert rv.status_code == 200 expected_response = {"message": f"Deleted {len(datasets)} datasets"} assert data == expected_response - deleted_datasets = datasets datasets = ( db.session.query(SqlaTable) .filter(SqlaTable.table_name.in_(self.fixture_tables_names)) .all() ) assert datasets == [] - for dataset in deleted_datasets: - setattr(dataset, "_deleted", True) # Assert permissions get cleaned for view_menu_name in view_menu_names: assert security_manager.find_view_menu(view_menu_name) is None From d73f9d20bd89c0c5e7dfb5c1eacd1e7562ea018c Mon Sep 17 00:00:00 2001 From: Jack Fragassi Date: Tue, 15 Aug 2023 10:22:37 -0700 Subject: [PATCH 7/8] Fall back to base dao delete for charts --- superset/daos/chart.py | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/superset/daos/chart.py b/superset/daos/chart.py index d716bf48b40b8..9abb05b3249ad 100644 --- a/superset/daos/chart.py +++ b/superset/daos/chart.py @@ -38,19 +38,6 @@ class ChartDAO(BaseDAO[Slice]): base_filter = ChartFilter - @classmethod - def delete(cls, items: Slice | list[Slice], commit: bool = True) -> None: - # bulk delete, first delete related data - # bulk delete itself - try: - for item in get_iterable(items): - db.session.delete(item) - if commit: - db.session.commit() - except SQLAlchemyError as ex: - db.session.rollback() - raise ex - @staticmethod def favorited_ids(charts: list[Slice]) -> list[FavStar]: ids = [chart.id for chart in charts] From b8e8a249fb88d88f6913aac646cddb8a5287b4c1 Mon Sep 17 00:00:00 2001 From: Jack Fragassi Date: Tue, 15 Aug 2023 10:41:54 -0700 Subject: [PATCH 8/8] Fix lint --- superset/daos/chart.py | 4 +--- superset/daos/dashboard.py | 3 +-- superset/daos/dataset.py | 2 +- 3 files changed, 3 insertions(+), 6 deletions(-) diff --git a/superset/daos/chart.py b/superset/daos/chart.py index 9abb05b3249ad..7eae38cb0ecad 100644 --- a/superset/daos/chart.py +++ b/superset/daos/chart.py @@ -20,14 +20,12 @@ from datetime import datetime from typing import TYPE_CHECKING -from sqlalchemy.exc import SQLAlchemyError - from superset.charts.filters import ChartFilter from superset.daos.base import BaseDAO from superset.extensions import db from superset.models.core import FavStar, FavStarClassName from superset.models.slice import Slice -from superset.utils.core import get_iterable, get_user_id +from superset.utils.core import get_user_id if TYPE_CHECKING: from superset.connectors.base.models import BaseDatasource diff --git a/superset/daos/dashboard.py b/superset/daos/dashboard.py index 9abd4e47dbea5..e6fe6ff25d6c8 100644 --- a/superset/daos/dashboard.py +++ b/superset/daos/dashboard.py @@ -23,7 +23,6 @@ from flask import g from flask_appbuilder.models.sqla.interface import SQLAInterface -from sqlalchemy.exc import SQLAlchemyError from superset import is_feature_enabled, security_manager from superset.daos.base import BaseDAO @@ -48,7 +47,7 @@ from superset.models.embedded_dashboard import EmbeddedDashboard from superset.models.filter_set import FilterSet from superset.models.slice import Slice -from superset.utils.core import get_iterable, get_user_id +from superset.utils.core import get_user_id from superset.utils.dashboard_filter_scopes_converter import copy_filter_scopes logger = logging.getLogger(__name__) diff --git a/superset/daos/dataset.py b/superset/daos/dataset.py index 3d21b4745173f..8f12fcca9375a 100644 --- a/superset/daos/dataset.py +++ b/superset/daos/dataset.py @@ -33,7 +33,7 @@ logger = logging.getLogger(__name__) -class DatasetDAO(BaseDAO[SqlaTable]): # pylint: disable=too-many-public-methods +class DatasetDAO(BaseDAO[SqlaTable]): base_filter = DatasourceFilter @staticmethod