-
Notifications
You must be signed in to change notification settings - Fork 14.3k
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
Changes from all commits
b295654
ab9f197
5c5e18d
2194ea3
296c198
72dadf9
7169c6a
d73f9d2
b8e8a24
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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__) | ||
|
@@ -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)] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See my previous comment. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
@@ -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): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Per the PR description this method actually should handle bulk deletion correctly. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are other custom |
||
security_manager.dataset_after_delete(mapper, connection, item) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this logic is still valid to be removed? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 ( | ||
|
There was a problem hiding this comment.
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
.There was a problem hiding this comment.
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:
There was a problem hiding this comment.
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 theBaseDAO.delete
method should be suffice.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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