From 6aaeed0ed7f0b9848659f5781c36ea2ec4f4626f Mon Sep 17 00:00:00 2001 From: John Bodley Date: Tue, 20 Jun 2023 14:24:29 -0700 Subject: [PATCH 1/2] chore(dao): Condense delete/bulk-delete --- .../annotations/commands/bulk_delete.py | 5 +- .../annotations/commands/delete.py | 7 +- .../annotation_layers/commands/bulk_delete.py | 5 +- superset/annotation_layers/commands/delete.py | 7 +- superset/charts/commands/bulk_delete.py | 4 +- superset/charts/commands/delete.py | 6 +- .../css_templates/commands/bulk_delete.py | 5 +- superset/daos/annotation.py | 31 --------- superset/daos/base.py | 50 ++++++++------ superset/daos/chart.py | 21 +++--- superset/daos/css.py | 23 +------ superset/daos/dashboard.py | 37 +++++----- superset/daos/dataset.py | 67 +++++++++++-------- superset/daos/query.py | 18 +---- superset/daos/report.py | 38 ++++++----- superset/dashboards/api.py | 2 +- superset/dashboards/commands/bulk_delete.py | 5 +- superset/dashboards/commands/delete.py | 6 +- superset/dashboards/filter_sets/api.py | 4 +- .../dashboards/filter_sets/commands/delete.py | 6 +- superset/databases/commands/delete.py | 6 +- .../databases/ssh_tunnel/commands/delete.py | 7 +- superset/datasets/columns/commands/delete.py | 11 ++- superset/datasets/commands/bulk_delete.py | 2 +- superset/datasets/commands/delete.py | 4 +- superset/datasets/metrics/commands/delete.py | 11 ++- .../saved_queries/commands/bulk_delete.py | 5 +- superset/reports/commands/bulk_delete.py | 5 +- superset/reports/commands/delete.py | 7 +- .../commands/bulk_delete.py | 4 +- tests/integration_tests/datasets/api_tests.py | 4 +- 31 files changed, 171 insertions(+), 242 deletions(-) diff --git a/superset/annotation_layers/annotations/commands/bulk_delete.py b/superset/annotation_layers/annotations/commands/bulk_delete.py index 2e0c53808f4fb..153f93aa6fc1e 100644 --- a/superset/annotation_layers/annotations/commands/bulk_delete.py +++ b/superset/annotation_layers/annotations/commands/bulk_delete.py @@ -36,9 +36,10 @@ def __init__(self, model_ids: list[int]): def run(self) -> None: self.validate() + assert self._models + try: - AnnotationDAO.bulk_delete(self._models) - return None + AnnotationDAO.delete(self._models) except DAODeleteFailedError as ex: logger.exception(ex.exception) raise AnnotationBulkDeleteFailedError() from ex diff --git a/superset/annotation_layers/annotations/commands/delete.py b/superset/annotation_layers/annotations/commands/delete.py index 2af01f57f4708..bbd7f39dd6958 100644 --- a/superset/annotation_layers/annotations/commands/delete.py +++ b/superset/annotation_layers/annotations/commands/delete.py @@ -17,8 +17,6 @@ import logging from typing import Optional -from flask_appbuilder.models.sqla import Model - from superset.annotation_layers.annotations.commands.exceptions import ( AnnotationDeleteFailedError, AnnotationNotFoundError, @@ -36,16 +34,15 @@ def __init__(self, model_id: int): self._model_id = model_id self._model: Optional[Annotation] = None - def run(self) -> Model: + def run(self) -> None: self.validate() assert self._model try: - annotation = AnnotationDAO.delete(self._model) + AnnotationDAO.delete(self._model) except DAODeleteFailedError as ex: logger.exception(ex.exception) raise AnnotationDeleteFailedError() from ex - return annotation def validate(self) -> None: # Validate/populate model exists diff --git a/superset/annotation_layers/commands/bulk_delete.py b/superset/annotation_layers/commands/bulk_delete.py index e4696065a6f31..a227fc3266f5a 100644 --- a/superset/annotation_layers/commands/bulk_delete.py +++ b/superset/annotation_layers/commands/bulk_delete.py @@ -37,9 +37,10 @@ def __init__(self, model_ids: list[int]): def run(self) -> None: self.validate() + assert self._models + try: - AnnotationLayerDAO.bulk_delete(self._models) - return None + AnnotationLayerDAO.delete(self._models) except DAODeleteFailedError as ex: logger.exception(ex.exception) raise AnnotationLayerBulkDeleteFailedError() from ex diff --git a/superset/annotation_layers/commands/delete.py b/superset/annotation_layers/commands/delete.py index 1af4242dce762..14f53272f13f3 100644 --- a/superset/annotation_layers/commands/delete.py +++ b/superset/annotation_layers/commands/delete.py @@ -17,8 +17,6 @@ import logging from typing import Optional -from flask_appbuilder.models.sqla import Model - from superset.annotation_layers.commands.exceptions import ( AnnotationLayerDeleteFailedError, AnnotationLayerDeleteIntegrityError, @@ -37,16 +35,15 @@ def __init__(self, model_id: int): self._model_id = model_id self._model: Optional[AnnotationLayer] = None - def run(self) -> Model: + def run(self) -> None: self.validate() assert self._model try: - annotation_layer = AnnotationLayerDAO.delete(self._model) + AnnotationLayerDAO.delete(self._model) except DAODeleteFailedError as ex: logger.exception(ex.exception) raise AnnotationLayerDeleteFailedError() from ex - return annotation_layer def validate(self) -> None: # Validate/populate model exists diff --git a/superset/charts/commands/bulk_delete.py b/superset/charts/commands/bulk_delete.py index 964c40d8129af..0555992e24ce1 100644 --- a/superset/charts/commands/bulk_delete.py +++ b/superset/charts/commands/bulk_delete.py @@ -43,8 +43,10 @@ def __init__(self, model_ids: list[int]): def run(self) -> None: self.validate() + assert self._models + try: - ChartDAO.bulk_delete(self._models) + ChartDAO.delete(self._models) except DeleteFailedError as ex: logger.exception(ex.exception) raise ChartBulkDeleteFailedError() from ex diff --git a/superset/charts/commands/delete.py b/superset/charts/commands/delete.py index 184e9f8e1a16b..6bbceb576f2ec 100644 --- a/superset/charts/commands/delete.py +++ b/superset/charts/commands/delete.py @@ -17,7 +17,6 @@ import logging from typing import cast, Optional -from flask_appbuilder.models.sqla import Model from flask_babel import lazy_gettext as _ from superset import security_manager @@ -43,7 +42,7 @@ def __init__(self, model_id: int): self._model_id = model_id self._model: Optional[Slice] = None - def run(self) -> Model: + def run(self) -> None: self.validate() self._model = cast(Slice, self._model) try: @@ -52,11 +51,10 @@ def run(self) -> Model: # table, sporadically Superset will error because the rows are not deleted. # Let's do it manually here to prevent the error. self._model.owners = [] - chart = ChartDAO.delete(self._model) + ChartDAO.delete(self._model) except DAODeleteFailedError as ex: logger.exception(ex.exception) raise ChartDeleteFailedError() from ex - return chart def validate(self) -> None: # Validate/populate model exists diff --git a/superset/css_templates/commands/bulk_delete.py b/superset/css_templates/commands/bulk_delete.py index c676e9eed8402..ef13a1c8e226f 100644 --- a/superset/css_templates/commands/bulk_delete.py +++ b/superset/css_templates/commands/bulk_delete.py @@ -36,9 +36,10 @@ def __init__(self, model_ids: list[int]): def run(self) -> None: self.validate() + assert self._models + try: - CssTemplateDAO.bulk_delete(self._models) - return None + CssTemplateDAO.delete(self._models) except DAODeleteFailedError as ex: logger.exception(ex.exception) raise CssTemplateBulkDeleteFailedError() from ex diff --git a/superset/daos/annotation.py b/superset/daos/annotation.py index 2df336647a1eb..1dd992241f5d1 100644 --- a/superset/daos/annotation.py +++ b/superset/daos/annotation.py @@ -17,10 +17,7 @@ import logging from typing import Optional, Union -from sqlalchemy.exc import SQLAlchemyError - from superset.daos.base import BaseDAO -from superset.daos.exceptions import DAODeleteFailedError from superset.extensions import db from superset.models.annotations import Annotation, AnnotationLayer @@ -28,19 +25,6 @@ class AnnotationDAO(BaseDAO[Annotation]): - @staticmethod - def bulk_delete(models: Optional[list[Annotation]], commit: bool = True) -> None: - item_ids = [model.id for model in models] if models else [] - try: - db.session.query(Annotation).filter(Annotation.id.in_(item_ids)).delete( - synchronize_session="fetch" - ) - if commit: - db.session.commit() - except SQLAlchemyError as ex: - db.session.rollback() - raise DAODeleteFailedError() from ex - @staticmethod def validate_update_uniqueness( layer_id: int, short_descr: str, annotation_id: Optional[int] = None @@ -63,21 +47,6 @@ def validate_update_uniqueness( class AnnotationLayerDAO(BaseDAO[AnnotationLayer]): - @staticmethod - def bulk_delete( - models: Optional[list[AnnotationLayer]], commit: bool = True - ) -> None: - item_ids = [model.id for model in models] if models else [] - try: - db.session.query(AnnotationLayer).filter( - AnnotationLayer.id.in_(item_ids) - ).delete(synchronize_session="fetch") - if commit: - db.session.commit() - except SQLAlchemyError as ex: - db.session.rollback() - raise DAODeleteFailedError() from ex - @staticmethod def has_annotations(model_id: Union[int, list[int]]) -> bool: if isinstance(model_id, list): diff --git a/superset/daos/base.py b/superset/daos/base.py index c0758f51dd9f3..2d7dfe20f457d 100644 --- a/superset/daos/base.py +++ b/superset/daos/base.py @@ -14,7 +14,9 @@ # KIND, either express or implied. See the License for the # specific language governing permissions and limitations # under the License. -from typing import Any, Generic, get_args, Optional, TypeVar, Union +from __future__ import annotations + +from typing import Any, Generic, get_args, TypeVar from flask_appbuilder.models.filters import BaseFilter from flask_appbuilder.models.sqla import Model @@ -29,6 +31,7 @@ DAOUpdateFailedError, ) from superset.extensions import db +from superset.utils.core import get_iterable T = TypeVar("T", bound=Model) # pylint: disable=invalid-name @@ -38,12 +41,12 @@ class BaseDAO(Generic[T]): Base DAO, implement base CRUD sqlalchemy operations """ - model_cls: Optional[type[Model]] = None + model_cls: type[Model] | None = None """ Child classes need to state the Model class so they don't need to implement basic create, update and delete methods """ - base_filter: Optional[BaseFilter] = None + base_filter: BaseFilter | None = None """ Child classes can register base filtering to be applied to all filter methods """ @@ -57,10 +60,10 @@ def __init_subclass__(cls) -> None: # pylint: disable=arguments-differ @classmethod def find_by_id( cls, - model_id: Union[str, int], + model_id: str | int, session: Session = None, skip_base_filter: bool = False, - ) -> Optional[Model]: + ) -> Model | None: """ Find a model by id, if defined applies `base_filter` """ @@ -81,7 +84,7 @@ def find_by_id( @classmethod def find_by_ids( cls, - model_ids: Union[list[str], list[int]], + model_ids: list[str] | list[int], session: Session = None, skip_base_filter: bool = False, ) -> list[T]: @@ -114,7 +117,7 @@ def find_all(cls) -> list[T]: return query.all() @classmethod - def find_one_or_none(cls, **filter_by: Any) -> Optional[T]: + def find_one_or_none(cls, **filter_by: Any) -> T | None: """ Get the first that fit the `base_filter` """ @@ -180,25 +183,28 @@ def update(cls, model: T, properties: dict[str, Any], commit: bool = True) -> T: return model @classmethod - def delete(cls, model: T, commit: bool = True) -> T: + def delete(cls, items: T | list[T], commit: bool = True) -> None: """ - Generic delete a model - :raises: DAODeleteFailedError + Delete the specified items(s) including their associated relationships. + + Note that bulk deletion via `delete` is not invoked in the base class as this + does not dispatch the ORM `after_delete` event which may be required to augment + additional records loosely defined via implicit relationships. Instead ORM + objects are deleted one-by-one via `Session.delete`. + + Subclasses may invoke bulk deletion but are responsible for instrumenting any + post-deletion logic. + + :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.delete(model) - if commit: - db.session.commit() - except SQLAlchemyError as ex: # pragma: no cover - db.session.rollback() - raise DAODeleteFailedError(exception=ex) from ex - return model - @classmethod - def bulk_delete(cls, models: list[T], commit: bool = True) -> None: try: - for model in models: - cls.delete(model, False) + for item in get_iterable(items): + db.session.delete(item) + if commit: db.session.commit() except SQLAlchemyError as ex: diff --git a/superset/daos/chart.py b/superset/daos/chart.py index 1a13965022221..6fb257f45a98e 100644 --- a/superset/daos/chart.py +++ b/superset/daos/chart.py @@ -15,9 +15,11 @@ # specific language governing permissions and limitations # under the License. # pylint: disable=arguments-renamed +from __future__ import annotations + import logging from datetime import datetime -from typing import Optional, TYPE_CHECKING +from typing import TYPE_CHECKING from sqlalchemy.exc import SQLAlchemyError @@ -26,7 +28,7 @@ from superset.extensions import db from superset.models.core import FavStar, FavStarClassName from superset.models.slice import Slice -from superset.utils.core import get_user_id +from superset.utils.core import get_iterable, get_user_id if TYPE_CHECKING: from superset.connectors.base.models import BaseDatasource @@ -37,15 +39,14 @@ class ChartDAO(BaseDAO[Slice]): base_filter = ChartFilter - @staticmethod - def bulk_delete(models: Optional[list[Slice]], commit: bool = True) -> None: - item_ids = [model.id for model in models] if models else [] + @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 - if models: - for model in models: - model.owners = [] - model.dashboards = [] - db.session.merge(model) + for item in get_iterable(items): + item.owners = [] + item.dashboards = [] + db.session.merge(item) # bulk delete itself try: db.session.query(Slice).filter(Slice.id.in_(item_ids)).delete( diff --git a/superset/daos/css.py b/superset/daos/css.py index 3a1cbe8fda148..df52de37e3acb 100644 --- a/superset/daos/css.py +++ b/superset/daos/css.py @@ -14,30 +14,9 @@ # KIND, either express or implied. See the License for the # specific language governing permissions and limitations # under the License. -import logging -from typing import Optional - -from sqlalchemy.exc import SQLAlchemyError - from superset.daos.base import BaseDAO -from superset.daos.exceptions import DAODeleteFailedError -from superset.extensions import db from superset.models.core import CssTemplate -logger = logging.getLogger(__name__) - class CssTemplateDAO(BaseDAO[CssTemplate]): - @staticmethod - def bulk_delete(models: Optional[list[CssTemplate]], commit: bool = True) -> None: - item_ids = [model.id for model in models] if models else [] - try: - db.session.query(CssTemplate).filter(CssTemplate.id.in_(item_ids)).delete( - synchronize_session="fetch" - ) - if commit: - db.session.commit() - except SQLAlchemyError as ex: - if commit: - db.session.rollback() - raise DAODeleteFailedError() from ex + pass diff --git a/superset/daos/dashboard.py b/superset/daos/dashboard.py index 1650711d50188..baeac95f2fb5a 100644 --- a/superset/daos/dashboard.py +++ b/superset/daos/dashboard.py @@ -14,10 +14,12 @@ # KIND, either express or implied. See the License for the # specific language governing permissions and limitations # under the License. +from __future__ import annotations + import json import logging from datetime import datetime -from typing import Any, Optional, Union +from typing import Any from flask import g from flask_appbuilder.models.sqla import Model @@ -43,7 +45,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_user_id +from superset.utils.core import get_iterable, get_user_id from superset.utils.dashboard_filter_scopes_converter import copy_filter_scopes logger = logging.getLogger(__name__) @@ -53,7 +55,7 @@ class DashboardDAO(BaseDAO[Dashboard]): base_filter = DashboardAccessFilter @classmethod - def get_by_id_or_slug(cls, id_or_slug: Union[int, str]) -> Dashboard: + def get_by_id_or_slug(cls, id_or_slug: int | str) -> Dashboard: if is_uuid(id_or_slug): # just get dashboard if it's uuid dashboard = Dashboard.get(id_or_slug) @@ -88,9 +90,7 @@ def get_charts_for_dashboard(id_or_slug: str) -> list[Slice]: return DashboardDAO.get_by_id_or_slug(id_or_slug).slices @staticmethod - def get_dashboard_changed_on( - id_or_slug_or_dashboard: Union[str, Dashboard] - ) -> datetime: + def get_dashboard_changed_on(id_or_slug_or_dashboard: str | Dashboard) -> datetime: """ Get latest changed datetime for a dashboard. @@ -108,7 +108,7 @@ def get_dashboard_changed_on( @staticmethod def get_dashboard_and_slices_changed_on( # pylint: disable=invalid-name - id_or_slug_or_dashboard: Union[str, Dashboard] + id_or_slug_or_dashboard: str | Dashboard, ) -> datetime: """ Get latest changed datetime for a dashboard. The change could be a dashboard @@ -134,7 +134,7 @@ def get_dashboard_and_slices_changed_on( # pylint: disable=invalid-name @staticmethod def get_dashboard_and_datasets_changed_on( # pylint: disable=invalid-name - id_or_slug_or_dashboard: Union[str, Dashboard] + id_or_slug_or_dashboard: str | Dashboard, ) -> datetime: """ Get latest changed datetime for a dashboard. The change could be a dashboard @@ -166,7 +166,7 @@ def validate_slug_uniqueness(slug: str) -> bool: return not db.session.query(dashboard_query.exists()).scalar() @staticmethod - def validate_update_slug_uniqueness(dashboard_id: int, slug: Optional[str]) -> bool: + def validate_update_slug_uniqueness(dashboard_id: int, slug: str | None) -> bool: if slug is not None: dashboard_query = db.session.query(Dashboard).filter( Dashboard.slug == slug, Dashboard.id != dashboard_id @@ -183,16 +183,15 @@ def update_charts_owners(model: Dashboard, commit: bool = True) -> Dashboard: db.session.commit() return model - @staticmethod - def bulk_delete(models: Optional[list[Dashboard]], commit: bool = True) -> None: - item_ids = [model.id for model in models] if models else [] + @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 - if models: - for model in models: - model.slices = [] - model.owners = [] - model.embedded = [] - db.session.merge(model) + for item in get_iterable(items): + item.slices = [] + item.owners = [] + item.embedded = [] + db.session.merge(item) # bulk delete itself try: db.session.query(Dashboard).filter(Dashboard.id.in_(item_ids)).delete( @@ -208,7 +207,7 @@ def bulk_delete(models: Optional[list[Dashboard]], commit: bool = True) -> None: def set_dash_metadata( # pylint: disable=too-many-locals dashboard: Dashboard, data: dict[Any, Any], - old_to_new_slice_ids: Optional[dict[int, int]] = None, + old_to_new_slice_ids: dict[int, int] | None = None, commit: bool = False, ) -> Dashboard: new_filter_scopes = {} diff --git a/superset/daos/dataset.py b/superset/daos/dataset.py index 74b7fa50eb1e7..716fcd9a057a6 100644 --- a/superset/daos/dataset.py +++ b/superset/daos/dataset.py @@ -14,8 +14,10 @@ # KIND, either express or implied. See the License for the # specific language governing permissions and limitations # under the License. +from __future__ import annotations + import logging -from typing import Any, Optional +from typing import Any from sqlalchemy.exc import SQLAlchemyError @@ -26,7 +28,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 +from superset.utils.core import DatasourceType, get_iterable from superset.views.base import DatasourceFilter logger = logging.getLogger(__name__) @@ -36,7 +38,7 @@ class DatasetDAO(BaseDAO[SqlaTable]): # pylint: disable=too-many-public-methods base_filter = DatasourceFilter @staticmethod - def get_database_by_id(database_id: int) -> Optional[Database]: + def get_database_by_id(database_id: int) -> Database | None: try: return db.session.query(Database).filter_by(id=database_id).one_or_none() except SQLAlchemyError as ex: # pragma: no cover @@ -68,7 +70,7 @@ def get_related_objects(database_id: int) -> dict[str, Any]: @staticmethod def validate_table_exists( - database: Database, table_name: str, schema: Optional[str] + database: Database, table_name: str, schema: str | None ) -> bool: try: database.get_table(table_name, schema=schema) @@ -80,9 +82,9 @@ def validate_table_exists( @staticmethod def validate_uniqueness( database_id: int, - schema: Optional[str], + schema: str | None, name: str, - dataset_id: Optional[int] = None, + dataset_id: int | None = None, ) -> bool: dataset_query = db.session.query(SqlaTable).filter( SqlaTable.table_name == name, @@ -287,9 +289,7 @@ def update_metrics( db.session.commit() @classmethod - def find_dataset_column( - cls, dataset_id: int, column_id: int - ) -> Optional[TableColumn]: + def find_dataset_column(cls, dataset_id: int, column_id: int) -> TableColumn | None: # We want to apply base dataset filters dataset = DatasetDAO.find_by_id(dataset_id) if not dataset: @@ -319,16 +319,14 @@ def create_column( return DatasetColumnDAO.create(properties, commit=commit) @classmethod - def delete_column(cls, model: TableColumn, commit: bool = True) -> TableColumn: + def delete_column(cls, model: TableColumn, commit: bool = True) -> None: """ Deletes a Dataset column """ - return cls.delete(model, commit=commit) + DatasetColumnDAO.delete(model, commit=commit) @classmethod - def find_dataset_metric( - cls, dataset_id: int, metric_id: int - ) -> Optional[SqlMetric]: + def find_dataset_metric(cls, dataset_id: int, metric_id: int) -> SqlMetric | None: # We want to apply base dataset filters dataset = DatasetDAO.find_by_id(dataset_id) if not dataset: @@ -336,11 +334,11 @@ def find_dataset_metric( return db.session.query(SqlMetric).get(metric_id) @classmethod - def delete_metric(cls, model: SqlMetric, commit: bool = True) -> SqlMetric: + def delete_metric(cls, model: SqlMetric, commit: bool = True) -> None: """ Deletes a Dataset metric """ - return cls.delete(model, commit=commit) + DatasetMetricDAO.delete(model, commit=commit) @classmethod def update_metric( @@ -363,22 +361,33 @@ def create_metric( return DatasetMetricDAO.create(properties, commit=commit) @classmethod - def bulk_delete( - cls, models: Optional[list[SqlaTable]], commit: bool = True + def delete( + cls, + items: SqlaTable | list[SqlaTable], + commit: bool = True, ) -> None: - item_ids = [model.id for model in models] if models else [] - # bulk delete itself + """ + 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_ids)).delete( - synchronize_session="fetch" - ) + db.session.query(SqlaTable).filter( + SqlaTable.id.in_(item.id for item in get_iterable(items)) + ).delete(synchronize_session="fetch") - if models: - connection = db.session.connection() - mapper = next(iter(cls.model_cls.registry.mappers)) # type: ignore + connection = db.session.connection() + mapper = next(iter(cls.model_cls.registry.mappers)) # type: ignore - for model in models: - security_manager.dataset_after_delete(mapper, connection, model) + for item in get_iterable(items): + security_manager.dataset_after_delete(mapper, connection, item) if commit: db.session.commit() @@ -388,7 +397,7 @@ def bulk_delete( raise ex @staticmethod - def get_table_by_name(database_id: int, table_name: str) -> Optional[SqlaTable]: + def get_table_by_name(database_id: int, table_name: str) -> SqlaTable | None: return ( db.session.query(SqlaTable) .filter_by(database_id=database_id, table_name=table_name) diff --git a/superset/daos/query.py b/superset/daos/query.py index 80b5d1ad4ed49..ea7c82cc34dba 100644 --- a/superset/daos/query.py +++ b/superset/daos/query.py @@ -16,14 +16,11 @@ # under the License. import logging from datetime import datetime -from typing import Any, Optional, Union - -from sqlalchemy.exc import SQLAlchemyError +from typing import Any, Union from superset import sql_lab from superset.common.db_query_status import QueryStatus from superset.daos.base import BaseDAO -from superset.daos.exceptions import DAODeleteFailedError from superset.exceptions import QueryNotFoundException, SupersetCancelQueryException from superset.extensions import db from superset.models.sql_lab import Query, SavedQuery @@ -105,16 +102,3 @@ def stop_query(client_id: str) -> None: class SavedQueryDAO(BaseDAO[SavedQuery]): base_filter = SavedQueryFilter - - @staticmethod - def bulk_delete(models: Optional[list[SavedQuery]], commit: bool = True) -> None: - item_ids = [model.id for model in models] if models else [] - try: - db.session.query(SavedQuery).filter(SavedQuery.id.in_(item_ids)).delete( - synchronize_session="fetch" - ) - if commit: - db.session.commit() - except SQLAlchemyError as ex: - db.session.rollback() - raise DAODeleteFailedError() from ex diff --git a/superset/daos/report.py b/superset/daos/report.py index 70a87a6454110..b753270122644 100644 --- a/superset/daos/report.py +++ b/superset/daos/report.py @@ -14,10 +14,12 @@ # KIND, either express or implied. See the License for the # specific language governing permissions and limitations # under the License. +from __future__ import annotations + import json import logging from datetime import datetime -from typing import Any, Optional +from typing import Any from flask_appbuilder import Model from sqlalchemy.exc import SQLAlchemyError @@ -34,7 +36,7 @@ ReportScheduleType, ReportState, ) -from superset.utils.core import get_user_id +from superset.utils.core import get_iterable, get_user_id logger = logging.getLogger(__name__) @@ -93,11 +95,13 @@ def find_by_database_ids(database_ids: list[int]) -> list[ReportSchedule]: .all() ) - @staticmethod - def bulk_delete( - models: Optional[list[ReportSchedule]], commit: bool = True + @classmethod + def delete( + cls, + items: ReportSchedule | list[ReportSchedule], + commit: bool = True, ) -> None: - item_ids = [model.id for model in models] if models else [] + item_ids = [item.id for item in get_iterable(items)] try: # Clean owners secondary table report_schedules = ( @@ -117,7 +121,7 @@ def bulk_delete( @staticmethod def validate_unique_creation_method( - dashboard_id: Optional[int] = None, chart_id: Optional[int] = None + dashboard_id: int | None = None, chart_id: int | None = None ) -> bool: """ Validate if the user already has a chart or dashboard @@ -135,7 +139,7 @@ def validate_unique_creation_method( @staticmethod def validate_update_uniqueness( - name: str, report_type: ReportScheduleType, expect_id: Optional[int] = None + name: str, report_type: ReportScheduleType, expect_id: int | None = None ) -> bool: """ Validate if this name and type is unique. @@ -218,7 +222,7 @@ def update( raise DAOCreateFailedError(str(ex)) from ex @staticmethod - def find_active(session: Optional[Session] = None) -> list[ReportSchedule]: + def find_active(session: Session | None = None) -> list[ReportSchedule]: """ Find all active reports. If session is passed it will be used instead of the default `db.session`, this is useful when on a celery worker session context @@ -231,8 +235,8 @@ def find_active(session: Optional[Session] = None) -> list[ReportSchedule]: @staticmethod def find_last_success_log( report_schedule: ReportSchedule, - session: Optional[Session] = None, - ) -> Optional[ReportExecutionLog]: + session: Session | None = None, + ) -> ReportExecutionLog | None: """ Finds last success execution log for a given report """ @@ -250,8 +254,8 @@ def find_last_success_log( @staticmethod def find_last_entered_working_log( report_schedule: ReportSchedule, - session: Optional[Session] = None, - ) -> Optional[ReportExecutionLog]: + session: Session | None = None, + ) -> ReportExecutionLog | None: """ Finds last success execution log for a given report """ @@ -270,8 +274,8 @@ def find_last_entered_working_log( @staticmethod def find_last_error_notification( report_schedule: ReportSchedule, - session: Optional[Session] = None, - ) -> Optional[ReportExecutionLog]: + session: Session | None = None, + ) -> ReportExecutionLog | None: """ Finds last error email sent """ @@ -307,9 +311,9 @@ def find_last_error_notification( def bulk_delete_logs( model: ReportSchedule, from_date: datetime, - session: Optional[Session] = None, + session: Session | None = None, commit: bool = True, - ) -> Optional[int]: + ) -> int | None: session = session or db.session try: row_count = ( diff --git a/superset/dashboards/api.py b/superset/dashboards/api.py index 3b8b70e1678d2..781ad6f6c77f7 100644 --- a/superset/dashboards/api.py +++ b/superset/dashboards/api.py @@ -1364,7 +1364,7 @@ def delete_embedded(self, dashboard: Dashboard) -> Response: $ref: '#/components/responses/500' """ for embedded in dashboard.embedded: - DashboardDAO.delete(embedded) + EmbeddedDashboardDAO.delete(embedded) return self.response(200, message="OK") @expose("//copy/", methods=("POST",)) diff --git a/superset/dashboards/commands/bulk_delete.py b/superset/dashboards/commands/bulk_delete.py index 4802c9f101764..707f0d722ab11 100644 --- a/superset/dashboards/commands/bulk_delete.py +++ b/superset/dashboards/commands/bulk_delete.py @@ -43,9 +43,10 @@ def __init__(self, model_ids: list[int]): def run(self) -> None: self.validate() + assert self._models + try: - DashboardDAO.bulk_delete(self._models) - return None + DashboardDAO.delete(self._models) except DeleteFailedError as ex: logger.exception(ex.exception) raise DashboardBulkDeleteFailedError() from ex diff --git a/superset/dashboards/commands/delete.py b/superset/dashboards/commands/delete.py index 1f5eb4ae3b69c..13b92977dfc30 100644 --- a/superset/dashboards/commands/delete.py +++ b/superset/dashboards/commands/delete.py @@ -17,7 +17,6 @@ import logging from typing import Optional -from flask_appbuilder.models.sqla import Model from flask_babel import lazy_gettext as _ from superset import security_manager @@ -42,16 +41,15 @@ def __init__(self, model_id: int): self._model_id = model_id self._model: Optional[Dashboard] = None - def run(self) -> Model: + def run(self) -> None: self.validate() assert self._model try: - dashboard = DashboardDAO.delete(self._model) + DashboardDAO.delete(self._model) except DAODeleteFailedError as ex: logger.exception(ex.exception) raise DashboardDeleteFailedError() from ex - return dashboard def validate(self) -> None: # Validate/populate model exists diff --git a/superset/dashboards/filter_sets/api.py b/superset/dashboards/filter_sets/api.py index 11291b91cc5a6..f735404cd4574 100644 --- a/superset/dashboards/filter_sets/api.py +++ b/superset/dashboards/filter_sets/api.py @@ -374,8 +374,8 @@ def delete(self, dashboard_id: int, pk: int) -> Response: $ref: '#/components/responses/500' """ try: - changed_model = DeleteFilterSetCommand(dashboard_id, pk).run() - return self.response(200, id=changed_model.id) + DeleteFilterSetCommand(dashboard_id, pk).run() + return self.response(200, id=dashboard_id) except ValidationError as error: return self.response_400(message=error.messages) except FilterSetNotFoundError: diff --git a/superset/dashboards/filter_sets/commands/delete.py b/superset/dashboards/filter_sets/commands/delete.py index c058354245204..1ae5f7cda3013 100644 --- a/superset/dashboards/filter_sets/commands/delete.py +++ b/superset/dashboards/filter_sets/commands/delete.py @@ -16,8 +16,6 @@ # under the License. import logging -from flask_appbuilder.models.sqla import Model - from superset.daos.dashboard import FilterSetDAO from superset.daos.exceptions import DAODeleteFailedError from superset.dashboards.filter_sets.commands.base import BaseFilterSetCommand @@ -35,12 +33,12 @@ def __init__(self, dashboard_id: int, filter_set_id: int): super().__init__(dashboard_id) self._filter_set_id = filter_set_id - def run(self) -> Model: + def run(self) -> None: self.validate() assert self._filter_set try: - return FilterSetDAO.delete(self._filter_set, commit=True) + FilterSetDAO.delete(self._filter_set) except DAODeleteFailedError as err: raise FilterSetDeleteFailedError(str(self._filter_set_id), "") from err diff --git a/superset/databases/commands/delete.py b/superset/databases/commands/delete.py index 95d212e2903e9..b7a86d6ef1512 100644 --- a/superset/databases/commands/delete.py +++ b/superset/databases/commands/delete.py @@ -17,7 +17,6 @@ import logging from typing import Optional -from flask_appbuilder.models.sqla import Model from flask_babel import lazy_gettext as _ from superset.commands.base import BaseCommand @@ -40,16 +39,15 @@ def __init__(self, model_id: int): self._model_id = model_id self._model: Optional[Database] = None - def run(self) -> Model: + def run(self) -> None: self.validate() assert self._model try: - database = DatabaseDAO.delete(self._model) + DatabaseDAO.delete(self._model) except DAODeleteFailedError as ex: logger.exception(ex.exception) raise DatabaseDeleteFailedError() from ex - return database def validate(self) -> None: # Validate/populate model exists diff --git a/superset/databases/ssh_tunnel/commands/delete.py b/superset/databases/ssh_tunnel/commands/delete.py index 375c496f2a488..04d6e68338901 100644 --- a/superset/databases/ssh_tunnel/commands/delete.py +++ b/superset/databases/ssh_tunnel/commands/delete.py @@ -17,8 +17,6 @@ import logging from typing import Optional -from flask_appbuilder.models.sqla import Model - from superset import is_feature_enabled from superset.commands.base import BaseCommand from superset.daos.database import SSHTunnelDAO @@ -38,17 +36,16 @@ def __init__(self, model_id: int): self._model_id = model_id self._model: Optional[SSHTunnel] = None - def run(self) -> Model: + def run(self) -> None: if not is_feature_enabled("SSH_TUNNELING"): raise SSHTunnelingNotEnabledError() self.validate() assert self._model try: - ssh_tunnel = SSHTunnelDAO.delete(self._model) + SSHTunnelDAO.delete(self._model) except DAODeleteFailedError as ex: raise SSHTunnelDeleteFailedError() from ex - return ssh_tunnel def validate(self) -> None: # Validate/populate model exists diff --git a/superset/datasets/columns/commands/delete.py b/superset/datasets/columns/commands/delete.py index 6ff8c21d7da59..d377141e131ad 100644 --- a/superset/datasets/columns/commands/delete.py +++ b/superset/datasets/columns/commands/delete.py @@ -17,8 +17,6 @@ import logging from typing import Optional -from flask_appbuilder.models.sqla import Model - from superset import security_manager from superset.commands.base import BaseCommand from superset.connectors.sqla.models import TableColumn @@ -40,13 +38,12 @@ def __init__(self, dataset_id: int, model_id: int): self._model_id = model_id self._model: Optional[TableColumn] = None - def run(self) -> Model: + def run(self) -> None: self.validate() + assert self._model + try: - if not self._model: - raise DatasetColumnNotFoundError() - column = DatasetDAO.delete_column(self._model) - return column + DatasetDAO.delete_column(self._model) except DAODeleteFailedError as ex: logger.exception(ex.exception) raise DatasetColumnDeleteFailedError() from ex diff --git a/superset/datasets/commands/bulk_delete.py b/superset/datasets/commands/bulk_delete.py index 6937dd20b75bf..48515bbc6aa00 100644 --- a/superset/datasets/commands/bulk_delete.py +++ b/superset/datasets/commands/bulk_delete.py @@ -42,7 +42,7 @@ def run(self) -> None: assert self._models try: - DatasetDAO.bulk_delete(self._models) + DatasetDAO.delete(self._models) except DeleteFailedError as ex: logger.exception(ex.exception) raise DatasetBulkDeleteFailedError() from ex diff --git a/superset/datasets/commands/delete.py b/superset/datasets/commands/delete.py index 1c2147b9594af..3e7fc7d5a3178 100644 --- a/superset/datasets/commands/delete.py +++ b/superset/datasets/commands/delete.py @@ -17,8 +17,6 @@ import logging from typing import Optional -from flask_appbuilder.models.sqla import Model - from superset import security_manager from superset.commands.base import BaseCommand from superset.connectors.sqla.models import SqlaTable @@ -39,7 +37,7 @@ def __init__(self, model_id: int): self._model_id = model_id self._model: Optional[SqlaTable] = None - def run(self) -> Model: + def run(self) -> None: self.validate() assert self._model diff --git a/superset/datasets/metrics/commands/delete.py b/superset/datasets/metrics/commands/delete.py index 5e7b2144c0810..2e1f0897b9112 100644 --- a/superset/datasets/metrics/commands/delete.py +++ b/superset/datasets/metrics/commands/delete.py @@ -17,8 +17,6 @@ import logging from typing import Optional -from flask_appbuilder.models.sqla import Model - from superset import security_manager from superset.commands.base import BaseCommand from superset.connectors.sqla.models import SqlMetric @@ -40,13 +38,12 @@ def __init__(self, dataset_id: int, model_id: int): self._model_id = model_id self._model: Optional[SqlMetric] = None - def run(self) -> Model: + def run(self) -> None: self.validate() + assert self._model + try: - if not self._model: - raise DatasetMetricNotFoundError() - column = DatasetDAO.delete_metric(self._model) - return column + DatasetDAO.delete_metric(self._model) except DAODeleteFailedError as ex: logger.exception(ex.exception) raise DatasetMetricDeleteFailedError() from ex diff --git a/superset/queries/saved_queries/commands/bulk_delete.py b/superset/queries/saved_queries/commands/bulk_delete.py index ba01bd456a4b1..4e68f6b79d08f 100644 --- a/superset/queries/saved_queries/commands/bulk_delete.py +++ b/superset/queries/saved_queries/commands/bulk_delete.py @@ -36,9 +36,10 @@ def __init__(self, model_ids: list[int]): def run(self) -> None: self.validate() + assert self._models + try: - SavedQueryDAO.bulk_delete(self._models) - return None + SavedQueryDAO.delete(self._models) except DAODeleteFailedError as ex: logger.exception(ex.exception) raise SavedQueryBulkDeleteFailedError() from ex diff --git a/superset/reports/commands/bulk_delete.py b/superset/reports/commands/bulk_delete.py index a3644d9fb4027..3e3df3d100c67 100644 --- a/superset/reports/commands/bulk_delete.py +++ b/superset/reports/commands/bulk_delete.py @@ -39,9 +39,10 @@ def __init__(self, model_ids: list[int]): def run(self) -> None: self.validate() + assert self._models + try: - ReportScheduleDAO.bulk_delete(self._models) - return None + ReportScheduleDAO.delete(self._models) except DAODeleteFailedError as ex: logger.exception(ex.exception) raise ReportScheduleBulkDeleteFailedError() from ex diff --git a/superset/reports/commands/delete.py b/superset/reports/commands/delete.py index f52d96f7f5b8a..4f9be93f8a22d 100644 --- a/superset/reports/commands/delete.py +++ b/superset/reports/commands/delete.py @@ -17,8 +17,6 @@ import logging from typing import Optional -from flask_appbuilder.models.sqla import Model - from superset import security_manager from superset.commands.base import BaseCommand from superset.daos.exceptions import DAODeleteFailedError @@ -39,16 +37,15 @@ def __init__(self, model_id: int): self._model_id = model_id self._model: Optional[ReportSchedule] = None - def run(self) -> Model: + def run(self) -> None: self.validate() assert self._model try: - report_schedule = ReportScheduleDAO.delete(self._model) + ReportScheduleDAO.delete(self._model) except DAODeleteFailedError as ex: logger.exception(ex.exception) raise ReportScheduleDeleteFailedError() from ex - return report_schedule def validate(self) -> None: # Validate/populate model exists diff --git a/superset/row_level_security/commands/bulk_delete.py b/superset/row_level_security/commands/bulk_delete.py index f180d0b2b810d..f0c8dddabcfca 100644 --- a/superset/row_level_security/commands/bulk_delete.py +++ b/superset/row_level_security/commands/bulk_delete.py @@ -37,9 +37,7 @@ def __init__(self, model_ids: list[int]): def run(self) -> None: self.validate() try: - RLSDAO.bulk_delete(self._models) - - return None + RLSDAO.delete(self._models) except DAODeleteFailedError as ex: logger.exception(ex.exception) raise RuleBulkDeleteFailedError() from ex diff --git a/tests/integration_tests/datasets/api_tests.py b/tests/integration_tests/datasets/api_tests.py index d31b8f0b28ed1..97c4800478850 100644 --- a/tests/integration_tests/datasets/api_tests.py +++ b/tests/integration_tests/datasets/api_tests.py @@ -1591,7 +1591,7 @@ def test_delete_dataset_column_not_owned(self): assert rv.status_code == 403 @pytest.mark.usefixtures("create_datasets") - @patch("superset.daos.dataset.DatasetDAO.delete") + @patch("superset.daos.dataset.DatasetColumnDAO.delete") def test_delete_dataset_column_fail(self, mock_dao_delete): """ Dataset API: Test delete dataset column @@ -1671,7 +1671,7 @@ def test_delete_dataset_metric_not_owned(self): assert rv.status_code == 403 @pytest.mark.usefixtures("create_datasets") - @patch("superset.daos.dataset.DatasetDAO.delete") + @patch("superset.daos.dataset.DatasetMetricDAO.delete") def test_delete_dataset_metric_fail(self, mock_dao_delete): """ Dataset API: Test delete dataset metric From 8d32885f4c43b676dd4c5b0c08d130caa76ea21e Mon Sep 17 00:00:00 2001 From: John Bodley <4567245+john-bodley@users.noreply.github.com> Date: Thu, 6 Jul 2023 08:46:43 -0700 Subject: [PATCH 2/2] Update superset/daos/base.py Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com> --- superset/daos/base.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/superset/daos/base.py b/superset/daos/base.py index 2d7dfe20f457d..a92e4813b389f 100644 --- a/superset/daos/base.py +++ b/superset/daos/base.py @@ -185,7 +185,7 @@ def update(cls, model: T, properties: dict[str, Any], commit: bool = True) -> T: @classmethod def delete(cls, items: T | list[T], commit: bool = True) -> None: """ - Delete the specified items(s) including their associated relationships. + Delete the specified item(s) including their associated relationships. Note that bulk deletion via `delete` is not invoked in the base class as this does not dispatch the ORM `after_delete` event which may be required to augment