-
Notifications
You must be signed in to change notification settings - Fork 13.5k
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(dao): Add explicit ON DELETE CASCADE for ownership #24628
chore(dao): Add explicit ON DELETE CASCADE for ownership #24628
Conversation
item_ids = [item.id for item in get_iterable(items)] | ||
try: | ||
# Clean owners secondary table | ||
report_schedules = ( |
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'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.
@@ -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 = [] |
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.
No need to initialize this.
|
||
# revision identifiers, used by Alembic. | ||
revision = "6fbe660cac39" | ||
down_revision = "90139bf715e4" | ||
|
||
import sqlalchemy as sa |
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.
Same logic as before, but using the more generalized shared constraints logic.
@@ -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"), |
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.
Using uppercase for consistency.
@@ -282,8 +282,6 @@ def test_import_v1_chart(self, sm_g, utils_g): | |||
|
|||
assert chart.owners == [admin] | |||
|
|||
chart.owners = [] | |||
database.owners = [] |
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.
Databases don't have owners so this line was a red herring.
@@ -146,9 +146,6 @@ def test_import_assets(self): | |||
|
|||
assert dashboard.owners == [self.user] | |||
|
|||
dashboard.owners = [] | |||
chart.owners = [] | |||
database.owners = [] |
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.
See above.
dashboard.owners = [] | ||
|
||
chart.owners = [] | ||
database.owners = [] |
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.
See above.
@@ -573,9 +573,6 @@ def test_import_v1_dashboard(self, sm_g, utils_g): | |||
|
|||
assert dashboard.owners == [admin] | |||
|
|||
dashboard.owners = [] | |||
chart.owners = [] | |||
database.owners = [] |
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.
See above.
@@ -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 = [] |
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.
See above.
@@ -528,7 +527,6 @@ def test_import_v1_dataset_existing_database(self, mock_g): | |||
) | |||
assert len(database.tables) == 1 | |||
|
|||
database.owners = [] |
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.
See above.
38ca4e8
to
8e2b3d8
Compare
Codecov Report
@@ 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
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 8 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
8e2b3d8
to
a2f826b
Compare
|
||
# revision identifiers, used by Alembic. | ||
revision = "6d05b0a70c89" | ||
down_revision = "240d23c7f86f" |
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.
@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.
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.
@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.
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 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.
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.
Done.
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.
LGTM
…_for_owners_references.py to 2023-07-11_15-51_6d05b0a70c89_add_on_delete_cascade_for_owners_references.py
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