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

jfrag1 · 2023-08-05T00:10:56Z

SUMMARY

This change shouldn't have any impact on the behavior of the application outside of maybe a small performance hit when deleting many objects at once. Note additionally that this is the default method of deletion used by the BaseDAO.

The motivation behind the change is that at Preset we'd like the ability to utilize the after_delete sqlalchemy listener hooks for these objects, and these aren't triggered for bulk deletes.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Has associated issue:
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

codecov · 2023-08-05T00:19:56Z

Codecov Report

Merging #24894 (e52cff5) into master (7397ab3) will increase coverage by 0.03%.
Report is 4 commits behind head on master.
The diff coverage is 100.00%.

❗ Current head e52cff5 differs from pull request most recent head 72dadf9. Consider uploading reports for the commit 72dadf9 to get more accurate results

@@            Coverage Diff             @@
##           master   #24894      +/-   ##
==========================================
+ Coverage   68.96%   68.99%   +0.03%     
==========================================
  Files        1906     1906              
  Lines       74122    74107      -15     
  Branches     8208     8208              
==========================================
+ Hits        51116    51132      +16     
+ Misses      20883    20852      -31     
  Partials     2123     2123

Flag	Coverage Δ
hive	`54.18% <20.00%> (+0.01%)`	⬆️
mysql	`79.21% <100.00%> (+<0.01%)`	⬆️
postgres	`79.31% <100.00%> (?)`
presto	`54.08% <20.00%> (+0.01%)`	⬆️
python	`83.37% <100.00%> (+0.08%)`	⬆️
sqlite	`77.91% <100.00%> (+0.02%)`	⬆️
unit	`55.06% <20.00%> (+0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
superset/daos/chart.py	`92.72% <100.00%> (ø)`
superset/daos/dashboard.py	`96.68% <100.00%> (ø)`
superset/daos/dataset.py	`93.38% <100.00%> (+2.21%)`	⬆️

... and 9 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

jfrag1 · 2023-08-05T01:27:29Z

tests/integration_tests/datasets/api_tests.py

        datasets = (
            db.session.query(SqlaTable)
            .filter(SqlaTable.table_name.in_(self.fixture_tables_names))
            .all()
        )
        assert datasets == []
+        for dataset in deleted_datasets:
+            setattr(dataset, "_deleted", True)


This test started failing since the cleanup was trying to delete already-deleted datasets. This was fine for some reason before with the bulk-deletes.

john-bodley · 2023-08-07T16:42:23Z

superset/daos/chart.py

@@ -41,16 +41,14 @@ class ChartDAO(BaseDAO[Slice]):

    @classmethod
    def delete(cls, items: Slice | list[Slice], commit: bool = True) -> None:
-        item_ids = [item.id for item in get_iterable(items)]


@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.

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)

@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.

@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.

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

john-bodley · 2023-08-07T16:42:33Z

superset/daos/dashboard.py

@@ -185,17 +185,15 @@ def update_charts_owners(model: Dashboard, commit: bool = True) -> Dashboard:

    @classmethod
    def delete(cls, items: Dashboard | list[Dashboard], commit: bool = True) -> None:
-        item_ids = [item.id for item in get_iterable(items)]


See my previous comment.

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

john-bodley · 2023-08-07T16:44:53Z

superset/daos/dataset.py

-            connection = db.session.connection()
-            mapper = next(iter(cls.model_cls.registry.mappers))  # type: ignore
-
-            for item in get_iterable(items):


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

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

john-bodley · 2023-08-07T16:45:56Z

tests/integration_tests/datasets/api_tests.py

        datasets = (
            db.session.query(SqlaTable)
            .filter(SqlaTable.table_name.in_(self.fixture_tables_names))
            .all()
        )
        assert datasets == []
+        for dataset in deleted_datasets:
+            setattr(dataset, "_deleted", True)


This approach doesn't see right, i.e., we shouldn't need to be setting/getting hidden attributes. If I were you, I would re-evaluate the failing tests and see what the issue is. Maybe the results weren't flushed or committed?

Updated with a better method of checking whether the object was deleted

zephyring · 2023-08-07T16:54:38Z

superset/daos/dataset.py

-            mapper = next(iter(cls.model_cls.registry.mappers))  # type: ignore
-
-            for item in get_iterable(items):
-                security_manager.dataset_after_delete(mapper, connection, item)


this logic is still valid to be removed?

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

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

…pache#24894) (cherry picked from commit 4a59a26)

…pache#24894)

Change to single delete method

b295654

pull-request-size bot added the size/S label Aug 5, 2023

Fix test

ab9f197

pull-request-size bot added size/M and removed size/S labels Aug 5, 2023

jfrag1 added 2 commits August 4, 2023 18:24

Remove unnecessary override of delete in DatasetDAO

5c5e18d

CFix test

2194ea3

jfrag1 commented Aug 5, 2023

View reviewed changes

Fix lint

296c198

john-bodley reviewed Aug 7, 2023

View reviewed changes

zephyring reviewed Aug 7, 2023

View reviewed changes

Better method of checking deletion status in tests

72dadf9

jfrag1 requested a review from john-bodley August 7, 2023 17:27

jfrag1 added 3 commits August 15, 2023 10:19

Fix merge conflicts

7169c6a

Fall back to base dao delete for charts

d73f9d2

Fix lint

b8e8a24

zephyring approved these changes Aug 15, 2023

View reviewed changes

john-bodley approved these changes Aug 17, 2023

View reviewed changes

eschutho merged commit 4a59a26 into apache:master Aug 18, 2023

eschutho deleted the jack/do-single-deletes-rather-than-bulk-in-daos branch August 18, 2023 00:00

michael-s-molina added v3.0 Label added by the release manager to track PRs to be included in the 3.0 branch and removed v3.0 Label added by the release manager to track PRs to be included in the 3.0 branch labels Aug 18, 2023

jinghua-qa pushed a commit to preset-io/superset that referenced this pull request Aug 18, 2023

chore: Update DAOs to use singular deletion method instead of bulk (a…

a44def8

…pache#24894) (cherry picked from commit 4a59a26)

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 3.1.0 labels Mar 8, 2024

vinothkumar66 pushed a commit to vinothkumar66/superset that referenced this pull request Nov 11, 2024

chore: Update DAOs to use singular deletion method instead of bulk (a…

92dbdfa

…pache#24894)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jfrag1 commented Aug 5, 2023

codecov bot commented Aug 5, 2023 •

edited

Loading

jfrag1 Aug 5, 2023

john-bodley Aug 7, 2023

jfrag1 Aug 7, 2023

john-bodley Aug 7, 2023 •

edited

Loading

john-bodley Aug 9, 2023 •

edited

Loading

jfrag1 Aug 15, 2023

john-bodley Aug 7, 2023

jfrag1 Aug 7, 2023

john-bodley Aug 7, 2023

jfrag1 Aug 7, 2023

john-bodley Aug 7, 2023

jfrag1 Aug 7, 2023

zephyring Aug 7, 2023

zephyring Aug 7, 2023

jfrag1 Aug 7, 2023

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

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

Conversation

jfrag1 commented Aug 5, 2023

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Aug 5, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

john-bodley Aug 7, 2023 • edited Loading

Choose a reason for hiding this comment

john-bodley Aug 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 5, 2023 •

edited

Loading

john-bodley Aug 7, 2023 •

edited

Loading

john-bodley Aug 9, 2023 •

edited

Loading