From 4a59a265fba36fbaa59d5952ad28dace71a612b7 Mon Sep 17 00:00:00 2001 From: Jack Fragassi Date: Thu, 17 Aug 2023 17:00:33 -0700 Subject: [PATCH] chore: Update DAOs to use singular deletion method instead of bulk (#24894) --- superset/daos/chart.py | 19 +-------- superset/daos/dashboard.py | 16 +------- superset/daos/dataset.py | 39 +------------------ tests/integration_tests/datasets/api_tests.py | 5 ++- 4 files changed, 7 insertions(+), 72 deletions(-) diff --git a/superset/daos/chart.py b/superset/daos/chart.py index f82239bfc..7eae38cb0 100644 --- a/superset/daos/chart.py +++ b/superset/daos/chart.py @@ -20,14 +20,12 @@ import logging 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 @@ -38,21 +36,6 @@ logger = logging.getLogger(__name__) class ChartDAO(BaseDAO[Slice]): base_filter = ChartFilter - @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 - # bulk delete itself - try: - db.session.query(Slice).filter(Slice.id.in_(item_ids)).delete( - synchronize_session="fetch" - ) - 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] diff --git a/superset/daos/dashboard.py b/superset/daos/dashboard.py index 1b62041cc..e6fe6ff25 100644 --- a/superset/daos/dashboard.py +++ b/superset/daos/dashboard.py @@ -23,7 +23,6 @@ from typing import Any 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.dashboard import Dashboard, id_or_slug_filter 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__) @@ -190,19 +189,6 @@ class DashboardDAO(BaseDAO[Dashboard]): db.session.commit() return model - @classmethod - def delete(cls, items: Dashboard | list[Dashboard], commit: bool = True) -> None: - item_ids = [item.id for item in get_iterable(items)] - try: - db.session.query(Dashboard).filter(Dashboard.id.in_(item_ids)).delete( - synchronize_session="fetch" - ) - if commit: - db.session.commit() - except SQLAlchemyError as ex: - db.session.rollback() - raise ex - @staticmethod def set_dash_metadata( # pylint: disable=too-many-locals dashboard: Dashboard, diff --git a/superset/daos/dataset.py b/superset/daos/dataset.py index 57b5a0b46..8f12fcca9 100644 --- a/superset/daos/dataset.py +++ b/superset/daos/dataset.py @@ -21,14 +21,13 @@ from typing import Any 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 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__) @@ -309,42 +308,6 @@ class DatasetDAO(BaseDAO[SqlaTable]): return None return db.session.query(SqlMetric).get(metric_id) - @classmethod - def delete( - cls, - items: SqlaTable | list[SqlaTable], - commit: bool = True, - ) -> None: - """ - 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 - :see: https://docs.sqlalchemy.org/en/latest/orm/queryguide/dml.html - """ - - 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) - - 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 ( diff --git a/tests/integration_tests/datasets/api_tests.py b/tests/integration_tests/datasets/api_tests.py index 8884b1171..2c6850e5b 100644 --- a/tests/integration_tests/datasets/api_tests.py +++ b/tests/integration_tests/datasets/api_tests.py @@ -25,6 +25,7 @@ from zipfile import is_zipfile, ZipFile import prison import pytest import yaml +from sqlalchemy import inspect from sqlalchemy.orm import joinedload from sqlalchemy.sql import func @@ -153,7 +154,9 @@ class TestDatasetApi(SupersetTestCase): # rollback changes for dataset in datasets: - db.session.delete(dataset) + state = inspect(dataset) + if not state.was_deleted: + db.session.delete(dataset) db.session.commit() @staticmethod