chore(dao): Condense delete/bulk-delete operations #24466

john-bodley · 2023-06-20T21:26:49Z

SUMMARY

Per SIP-92 Proposal for restructuring the Python code base #24331 organized all the DAOs to be housed within a shared folder—the result of which highlighted numerous inconsistencies, repetition, and inefficiencies.

This PR (one of many smaller bitesized PRs) condenses the single (delete) and bulk deletion (bulk_delete) logic into a single (delete) method which removes a bunch of copypasta and helps to standardize the code.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Has associated issue: [SIP-92] Proposal for restructuring the Python code base #20630
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

john-bodley · 2023-06-20T21:27:27Z

superset/daos/annotation.py

Copypasta except the BaseDAO is defined as a classmethod as opposed to a staticmethod.

These comments are very helpful while reviewing. Thank you.

john-bodley · 2023-06-20T21:27:57Z

superset/daos/annotation.py

Copypasta except the BaseDAO is defined as a classmethod as opposed to a staticmethod.

Nice cleanup!

john-bodley · 2023-06-20T21:29:21Z

superset/daos/css.py

Copypasta except the BaseDAO is defined as a classmethod as opposed to a staticmethod.

codecov · 2023-06-20T21:37:17Z

Codecov Report

Merging #24466 (f1fae05) into master (aa6870b) will decrease coverage by 10.75%.
The diff coverage is 47.80%.

❗ Current head f1fae05 differs from pull request most recent head 8d32885. Consider uploading reports for the commit 8d32885 to get more accurate results

@@             Coverage Diff             @@
##           master   #24466       +/-   ##
===========================================
- Coverage   69.08%   58.33%   -10.75%     
===========================================
  Files        1906     1906               
  Lines       74156    74107       -49     
  Branches     8161     8165        +4     
===========================================
- Hits        51232    43234     -7998     
- Misses      20805    28750     +7945     
- Partials     2119     2123        +4

Flag	Coverage Δ
hive	`54.14% <43.38%> (+0.20%)`	⬆️
mysql	`?`
postgres	`?`
presto	`54.04% <43.38%> (+0.20%)`	⬆️
python	`61.10% <52.20%> (-22.38%)`	⬇️
sqlite	`?`
unit	`54.79% <52.20%> (+0.09%)`	⬆️

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

Impacted Files	Coverage Δ
...et-ui-chart-controls/src/components/labelUtils.tsx	`100.00% <ø> (ø)`
superset-frontend/src/components/Tooltip/index.tsx	`100.00% <ø> (ø)`
...ses/DatabaseModal/DatabaseConnectionForm/index.tsx	`81.81% <ø> (ø)`
...otation_layers/annotations/commands/bulk_delete.py	`50.00% <0.00%> (-37.50%)`	⬇️
superset/annotation_layers/commands/bulk_delete.py	`46.15% <0.00%> (-38.47%)`	⬇️
superset/charts/commands/bulk_delete.py	`44.44% <0.00%> (-46.99%)`	⬇️
superset/css_templates/commands/bulk_delete.py	`50.00% <0.00%> (-37.50%)`	⬇️
superset/daos/annotation.py	`56.00% <ø> (-31.24%)`	⬇️
superset/dashboards/api.py	`51.28% <0.00%> (-41.29%)`	⬇️
superset/dashboards/commands/bulk_delete.py	`44.44% <0.00%> (-47.23%)`	⬇️
... and 37 more

... and 267 files with indirect coverage changes

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

john-bodley · 2023-06-21T05:33:07Z

superset/annotation_layers/annotations/commands/delete.py

No need to return the object we're deleting.

john-bodley · 2023-06-21T16:28:32Z

superset/dashboards/api.py

I'm somewhat surprised that #24465 didn't pick this up.

john-bodley · 2023-06-21T17:27:38Z

superset/annotation_layers/annotations/commands/bulk_delete.py

This logic is temporary. It should be cleaned up when we refactor the commands and enforce that validate doesn't augment any properties.

john-bodley · 2023-06-21T17:28:50Z

superset/daos/dataset.py

This is more accurate, i.e., the DatasetColumnDAO rather than the DatasetDAO should be deleting the column.

john-bodley · 2023-06-21T17:30:21Z

superset/datasets/metrics/commands/delete.py

DRY. The validate method already raises a DatasetMetricNotFoundError if the metric is not found.

john-bodley · 2023-06-21T17:31:18Z

superset/daos/base.py

items seems to be a more accurate/consistent than models, i.e., we're deleting database items/instances as opposed to models.

john-bodley · 2023-06-22T04:22:20Z

superset/daos/dataset.py

The bulk_deletion logic differs from the delete logic in the sense that the former first deletes the relationships (which in actuality may not be explicitly required) before deleting the dataset, whereas the later only deletes the datasets (which in theory should delete all the records with an explicit relationship).

michael-s-molina

LGTM

michael-s-molina · 2023-07-06T12:26:37Z

superset/daos/annotation.py

These comments are very helpful while reviewing. Thank you.

michael-s-molina · 2023-07-06T12:26:58Z

superset/daos/annotation.py

Nice cleanup!

superset/daos/base.py

michael-s-molina · 2023-07-06T12:51:53Z

Thank you for the nice cleanup @john-bodley!

Co-authored-by: Michael S. Molina <[email protected]>

Co-authored-by: Michael S. Molina <[email protected]> (cherry picked from commit 7409289)

Co-authored-by: Michael S. Molina <[email protected]>

pull-request-size bot added the size/L label Jun 20, 2023

john-bodley commented Jun 20, 2023

View reviewed changes

john-bodley requested review from dpgaspar, michael-s-molina and villebro June 20, 2023 21:30

john-bodley force-pushed the john-bodley--dao-bulk-delete branch 4 times, most recently from 2208e64 to 51dc939 Compare June 21, 2023 05:32

john-bodley commented Jun 21, 2023

View reviewed changes

superset/annotation_layers/annotations/commands/delete.py Outdated

Copy link

Member Author

john-bodley Jun 21, 2023

Choose a reason for hiding this comment

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

No need to return the object we're deleting.

john-bodley force-pushed the john-bodley--dao-bulk-delete branch 5 times, most recently from 5b99d4b to 8b63521 Compare June 21, 2023 16:24

john-bodley commented Jun 21, 2023

View reviewed changes

superset/dashboards/api.py Outdated

Copy link

Member Author

john-bodley Jun 21, 2023

Choose a reason for hiding this comment

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

I'm somewhat surprised that #24465 didn't pick this up.

john-bodley force-pushed the john-bodley--dao-bulk-delete branch 2 times, most recently from 02e8b79 to cbb621d Compare June 21, 2023 17:26

john-bodley commented Jun 21, 2023

View reviewed changes

john-bodley force-pushed the john-bodley--dao-bulk-delete branch 7 times, most recently from ee29647 to 3620631 Compare June 22, 2023 03:58

john-bodley commented Jun 22, 2023

View reviewed changes

john-bodley force-pushed the john-bodley--dao-bulk-delete branch from 3620631 to aa543ae Compare June 22, 2023 04:54

john-bodley changed the title ~~chore(dao): Condense delete/bulk-delete~~ chore(dao): Condense delete/bulk-delete operations Jun 22, 2023

john-bodley mentioned this pull request Jun 23, 2023

chore(dao): Add explicit ON DELETE CASCADE when deleting datasets #24488

Merged

9 tasks

john-bodley requested a review from betodealmeida June 23, 2023 14:16

john-bodley force-pushed the john-bodley--dao-bulk-delete branch from aa543ae to 14664d8 Compare June 28, 2023 23:29

chore(dao): Condense delete/bulk-delete

6aaeed0

john-bodley force-pushed the john-bodley--dao-bulk-delete branch from 14664d8 to 6aaeed0 Compare June 30, 2023 03:26

michael-s-molina approved these changes Jul 6, 2023

View reviewed changes

Update superset/daos/base.py

8d32885

Co-authored-by: Michael S. Molina <[email protected]>

john-bodley merged commit 7409289 into apache:master Jul 6, 2023

john-bodley mentioned this pull request Jul 6, 2023

chore(command): Condense delete/bulk-delete operations #24607

Merged

9 tasks

michael-s-molina added the v3.0 Label added by the release manager to track PRs to be included in the 3.0 branch label Jul 12, 2023

michael-s-molina added a commit that referenced this pull request Jul 26, 2023

chore(dao): Condense delete/bulk-delete operations (#24466)

71eff95

Co-authored-by: Michael S. Molina <[email protected]> (cherry picked from commit 7409289)

john-bodley mentioned this pull request Nov 1, 2023

[SIP-99D] Proposal for consolidating Command/DAO create, update, and upsert operations #25830

Closed

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

chore(dao): Condense delete/bulk-delete operations (apache#24466)

c4672be

Co-authored-by: Michael S. Molina <[email protected]>

chore(dao): Condense delete/bulk-delete operations #24466

chore(dao): Condense delete/bulk-delete operations #24466

Uh oh!

Conversation

john-bodley commented Jun 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Jun 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

john-bodley Jun 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michael-s-molina left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

michael-s-molina commented Jul 6, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

john-bodley commented Jun 20, 2023 •

edited

Loading

codecov bot commented Jun 20, 2023 •

edited

Loading

john-bodley Jun 21, 2023 •

edited

Loading