craig-rueda commented on code in PR #24488:
URL: https://github.com/apache/superset/pull/24488#discussion_r1242480042
##########
superset/daos/dataset.py:
##########
@@ -361,25 +362,24 @@ def create_metric(
"""
return DatasetMetricDAO.create(properties, commit=commit)
- @staticmethod
- def bulk_delete(models: Optional[list[SqlaTable]], commit: bool = True) ->
None:
+ @classmethod
+ def bulk_delete(
+ cls, models: Optional[list[SqlaTable]], commit: bool = True
+ ) -> None:
item_ids = [model.id for model in models] if models else []
- # bulk delete, first delete related data
- if models:
- for model in models:
- model.owners = []
- db.session.merge(model)
-
db.session.query(SqlMetric).filter(SqlMetric.table_id.in_(item_ids)).delete(
- synchronize_session="fetch"
- )
- db.session.query(TableColumn).filter(
- TableColumn.table_id.in_(item_ids)
- ).delete(synchronize_session="fetch")
# bulk delete itself
try:
db.session.query(SqlaTable).filter(SqlaTable.id.in_(item_ids)).delete(
synchronize_session="fetch"
)
+
+ if models:
+ 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)
Review Comment:
Ideally, DAO's should be super light, only concerned with interfacing with
the DB. They shouldn't have dependencies upwards (to the mid-tier), which can
lead to circular dependencies as basically all mid-tier logic will at some
point need a DAO. This can obviously be worked around using some late-binding
technique.
From a semantic POV, the command is just asking to "please delete these
records". Calling the SM to do additional work here is a side effect, which
ultimately makes this code less reusable for lower level operations.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
