chore(dao): Add explicit ON DELETE CASCADE for ownership #24628

john-bodley · 2023-07-08T02:50:55Z

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 is somewhat akin to #24488 where explicit ON DELETE CASCADE operations are set for the ownership associations.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

CI.

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: Takes ~ 1 minute for Airbnb's instance w/o load.
Introduces new feature or API
Removes existing feature or API

john-bodley · 2023-07-08T02:52:40Z

superset/daos/report.py

-        item_ids = [item.id for item in get_iterable(items)]
-        try:
-            # Clean owners secondary table
-            report_schedules = (


I'm not really sure why the reports were refetched as opposed to using the items provides. Note after removing the need to remove the owners this logic simply reduced to the BaseDAO.delete method and thus is unnecessary.

john-bodley · 2023-07-08T02:52:59Z

superset/examples/birth_names.py

@@ -535,7 +535,6 @@ def create_dashboard(slices: list[Slice]) -> Dashboard:
    dash = db.session.query(Dashboard).filter_by(slug="births").first()
    if not dash:
        dash = Dashboard()
-        dash.owners = []


No need to initialize this.

john-bodley · 2023-07-08T02:53:34Z

...ations/versions/2023-06-22_13-39_6fbe660cac39_add_on_delete_cascade_for_tables_references.py


 # revision identifiers, used by Alembic.
 revision = "6fbe660cac39"
 down_revision = "90139bf715e4"

-import sqlalchemy as sa


Same logic as before, but using the more generalized shared constraints logic.

john-bodley · 2023-07-08T02:54:11Z

superset/tables/models.py

@@ -53,12 +53,12 @@
    Model.metadata,  # pylint: disable=no-member
    sa.Column(
        "table_id",
-        sa.ForeignKey("sl_tables.id", ondelete="cascade"),
+        sa.ForeignKey("sl_tables.id", ondelete="CASCADE"),


Using uppercase for consistency.

john-bodley · 2023-07-08T02:55:05Z

tests/integration_tests/charts/commands_tests.py

@@ -282,8 +282,6 @@ def test_import_v1_chart(self, sm_g, utils_g):

        assert chart.owners == [admin]

-        chart.owners = []
-        database.owners = []


Databases don't have owners so this line was a red herring.

john-bodley · 2023-07-08T02:55:17Z

tests/integration_tests/commands_test.py

@@ -146,9 +146,6 @@ def test_import_assets(self):

        assert dashboard.owners == [self.user]

-        dashboard.owners = []
-        chart.owners = []
-        database.owners = []


john-bodley · 2023-07-08T02:55:23Z

tests/integration_tests/commands_test.py

-        dashboard.owners = []
-
-        chart.owners = []
-        database.owners = []


john-bodley · 2023-07-08T02:55:30Z

tests/integration_tests/dashboards/commands_tests.py

@@ -573,9 +573,6 @@ def test_import_v1_dashboard(self, sm_g, utils_g):

        assert dashboard.owners == [admin]

-        dashboard.owners = []
-        chart.owners = []
-        database.owners = []


john-bodley · 2023-07-08T02:55:37Z

tests/integration_tests/datasets/commands_tests.py

@@ -400,7 +400,6 @@ def test_import_v1_dataset(self, sm_g, utils_g):
        assert column.description is None
        assert column.python_date_format is None

-        dataset.database.owners = []


john-bodley · 2023-07-08T02:55:42Z

tests/integration_tests/datasets/commands_tests.py

@@ -528,7 +527,6 @@ def test_import_v1_dataset_existing_database(self, mock_g):
        )
        assert len(database.tables) == 1

-        database.owners = []


codecov · 2023-07-08T03:22:47Z

Codecov Report

Merging #24628 (273b32d) into master (c53b249) will increase coverage by 0.08%.
The diff coverage is 31.25%.

❗ Current head 273b32d differs from pull request most recent head 7c045a5. Consider uploading reports for the commit 7c045a5 to get more accurate results

@@            Coverage Diff             @@
##           master   #24628      +/-   ##
==========================================
+ Coverage   68.97%   69.05%   +0.08%     
==========================================
  Files        1907     1908       +1     
  Lines       74153    74168      +15     
  Branches     8182     8177       -5     
==========================================
+ Hits        51148    51219      +71     
+ Misses      20882    20829      -53     
+ Partials     2123     2120       -3

Flag	Coverage Δ
hive	`54.12% <20.58%> (?)`
mysql	`79.42% <35.29%> (?)`
postgres	`79.51% <35.29%> (-0.05%)`	⬇️
presto	`54.02% <20.58%> (-0.02%)`	⬇️
python	`83.49% <35.29%> (+0.16%)`	⬆️
sqlite	`78.07% <35.29%> (-0.05%)`	⬇️
unit	`54.81% <20.58%> (-0.02%)`	⬇️

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

Impacted Files	Coverage Δ
...hart-echarts/src/MixedTimeseries/transformProps.ts	`0.78% <ø> (ø)`
superset-frontend/src/components/Icons/Icon.tsx	`0.00% <ø> (ø)`
...Filters/FilterBar/FilterControls/FilterControl.tsx	`27.69% <0.00%> (-0.44%)`	⬇️
...veFilters/FilterBar/FilterControls/FilterValue.tsx	`5.76% <ø> (+0.10%)`	⬆️
...tersConfigModal/FiltersConfigForm/DefaultValue.tsx	`9.09% <0.00%> (ø)`
...mponents/nativeFilters/FiltersConfigModal/utils.ts	`46.96% <0.00%> (ø)`
...nd/src/dashboard/components/nativeFilters/state.ts	`64.28% <ø> (ø)`
...perset-frontend/src/dashboard/util/crossFilters.ts	`75.00% <ø> (ø)`
.../src/explore/components/ControlPanelsContainer.tsx	`69.76% <0.00%> (ø)`
...rset-frontend/src/explore/components/SaveModal.tsx	`35.13% <ø> (+0.04%)`	⬆️
... and 26 more

... and 8 files with indirect coverage changes

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

EugeneTorap · 2023-07-11T06:02:44Z

...ations/versions/2023-07-07_15-51_6d05b0a70c89_add_on_delete_cascade_for_owners_references.py

+
+# revision identifiers, used by Alembic.
+revision = "6d05b0a70c89"
+down_revision = "240d23c7f86f"


@john-bodley Use the latest f92a3124dd66 revision
Also update Create Date in filename in order to have this file after f92a3124dd66 in migrations/versions folder.

@EugeneTorap I've updated the SHA. AFAIK we typically don't update the date associated with the filename, i.e., there's no guarantee that these revisions are in chronological order, i.e., they merely serves as a guide. cc @craig-rueda for guidance.

I would say that updating the filename is fine. Ideally the filename would be somewhat close to the date that the PR is merged as to keep things somewhat sorted.

…_references.py

craig-rueda

LGTM

…_for_owners_references.py to 2023-07-11_15-51_6d05b0a70c89_add_on_delete_cascade_for_owners_references.py

pull-request-size bot added the size/L label Jul 8, 2023

john-bodley commented Jul 8, 2023

View reviewed changes

john-bodley force-pushed the john-bodley--dao-owners-ondelete-cascade branch from 38ca4e8 to 8e2b3d8 Compare July 8, 2023 02:58

john-bodley marked this pull request as ready for review July 8, 2023 02:59

john-bodley requested a review from a team as a code owner July 8, 2023 02:59

chore(dao): Add explicit ON DELETE CASCADE for ownership

a2f826b

john-bodley force-pushed the john-bodley--dao-owners-ondelete-cascade branch from 8e2b3d8 to a2f826b Compare July 8, 2023 05:39

john-bodley requested a review from michael-s-molina July 10, 2023 16:58

EugeneTorap reviewed Jul 11, 2023

View reviewed changes

Update 2023-07-07_15-51_6d05b0a70c89_add_on_delete_cascade_for_owners…

4cf268c

…_references.py

craig-rueda approved these changes Jul 11, 2023

View reviewed changes

Update and rename 2023-07-07_15-51_6d05b0a70c89_add_on_delete_cascade…

7c045a5

…_for_owners_references.py to 2023-07-11_15-51_6d05b0a70c89_add_on_delete_cascade_for_owners_references.py

john-bodley merged commit ae00489 into apache:master Jul 11, 2023

john-bodley deleted the john-bodley--dao-owners-ondelete-cascade branch July 11, 2023 18:39

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 11, 2023

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

chore(dao): Add explicit ON DELETE CASCADE for ownership (#24628)

96cf640

This was referenced Aug 7, 2023

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

Merged

chore: Add explicit ON DELETE CASCADE for dashboard_slices #24938

Merged

chore: Add explicit ON DELETE CASCADE for embedded_dashboards #24939

Merged

michael-s-molina mentioned this pull request Sep 5, 2023

fix: change constraint alters to non blocking for postgres #25164

Closed

9 tasks

john-bodley mentioned this pull request Sep 15, 2023

fix: Add explicit ON DELETE CASCADE for dashboard_roles #25320

Merged

9 tasks

mistercrunch added 🍒 3.0.0 🍒 3.0.1 🍒 3.0.2 🍒 3.0.3 🍒 3.0.4 🏷️ 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(dao): Add explicit ON DELETE CASCADE for ownership (apache#24628)

787ee4d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(dao): Add explicit ON DELETE CASCADE for ownership #24628

chore(dao): Add explicit ON DELETE CASCADE for ownership #24628

john-bodley commented Jul 8, 2023

john-bodley Jul 8, 2023

john-bodley Jul 8, 2023

john-bodley Jul 8, 2023

john-bodley Jul 8, 2023

john-bodley Jul 8, 2023

john-bodley Jul 8, 2023

john-bodley Jul 8, 2023

john-bodley Jul 8, 2023

john-bodley Jul 8, 2023

john-bodley Jul 8, 2023

codecov bot commented Jul 8, 2023 •

edited

Loading

EugeneTorap Jul 11, 2023

john-bodley Jul 11, 2023

craig-rueda Jul 11, 2023

john-bodley Jul 11, 2023

craig-rueda left a comment

chore(dao): Add explicit ON DELETE CASCADE for ownership #24628

chore(dao): Add explicit ON DELETE CASCADE for ownership #24628

Conversation

john-bodley commented Jul 8, 2023

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

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 Jul 8, 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

Choose a reason for hiding this comment

craig-rueda left a comment

Choose a reason for hiding this comment

codecov bot commented Jul 8, 2023 •

edited

Loading