Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: Update DAOs to use singular deletion method instead of bulk #24894

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 1 addition & 18 deletions superset/daos/chart.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -38,21 +36,6 @@
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)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jfrag1 if we're removing the bulk delete logic, i.e., deleting outside of the ORM, then I think there's merit in removing the entire function and falling back to BaseDAO.delete.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't do this here because of this logic which is specific to charts:

for item in get_iterable(items):
    item.dashboards = []
    db.session.merge(item)

Copy link
Member

@john-bodley john-bodley Aug 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jfrag1 the reason this logic is required is likely because for non SQLAlchemy ORM operations, i.e., bulk deletion, the ON DELETE CASCADE isn't configured. If one were to simply remove the bulk deletion logic then the BaseDAO.delete method should be suffice.

Copy link
Member

@john-bodley john-bodley Aug 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jfrag1 I authored #24938 and #24939 which should help provide some clarity, i.e., that said code block is redundant as we're leaning on the database schema.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated the chart/dashboard DAOs to fall back to BaseDAO.delete

# 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]
Expand Down
16 changes: 1 addition & 15 deletions superset/daos/dashboard.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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__)
Expand Down Expand Up @@ -190,19 +189,6 @@ def update_charts_owners(model: Dashboard, commit: bool = True) -> 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)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my previous comment.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here, there's some logic which is specific to dashboards.

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,
Expand Down
41 changes: 2 additions & 39 deletions superset/daos/dataset.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,20 +21,19 @@

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__)


class DatasetDAO(BaseDAO[SqlaTable]): # pylint: disable=too-many-public-methods
class DatasetDAO(BaseDAO[SqlaTable]):
base_filter = DatasourceFilter

@staticmethod
Expand Down Expand Up @@ -309,42 +308,6 @@ def find_dataset_metric(cls, dataset_id: int, metric_id: int) -> SqlMetric | Non
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):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per the PR description this method actually should handle bulk deletion correctly.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are other custom after_delete listeners we'd like to trigger outside of just security_manager.dataset_after_delete

security_manager.dataset_after_delete(mapper, connection, item)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this logic is still valid to be removed?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should there any test failing if we remove this? I think we should add a test here to catch if
the vm is removed once dataset is deleted

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, when doing singular deletes this is triggered via this listener: /~https://github.com/apache/superset/blob/master/superset/connectors/sqla/models.py#L1585. The logic was only there since this was not triggered via the bulk delete method


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 (
Expand Down
5 changes: 4 additions & 1 deletion tests/integration_tests/datasets/api_tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import prison
import pytest
import yaml
from sqlalchemy import inspect
from sqlalchemy.orm import joinedload
from sqlalchemy.sql import func

Expand Down Expand Up @@ -153,7 +154,9 @@ def create_datasets(self):

# 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
Expand Down