iop
/
superset
4
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

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

This commit is contained in:
John Bodley 2023-07-11 11:39:03 -07:00 committed by GitHub
parent b4d08aecdb
commit ae00489779
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
19 changed files with 200 additions and 120 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

3
UPDATING.md
View File

@ -24,7 +24,8 @@ assists people when migrating to a new version.
## Next
- [24488](https://github.com/apache/superset/pull/24488): Augments the foreign key constraints for the `sql_metrics`, `sqlatable_user`, and `table_columns` tables which reference the `tables` table to include an explicit CASCADE ON DELETE to ensure the relevant records are deleted when a dataset is deleted. Scheduled downtime may be advised.
- [24628]https://github.com/apache/superset/pull/24628): Augments the foreign key constraints for the `dashboard_owner`, `report_schedule_owner`, and `slice_owner` tables to include an explicit CASCADE ON DELETE to ensure the relevant ownership records are deleted when a dataset is deleted. Scheduled downtime may be advised.
- [24488](https://github.com/apache/superset/pull/24488): Augments the foreign key constraints for the `sql_metrics`, `sqlatable_user`, and `table_columns` tables to include an explicit CASCADE ON DELETE to ensure the relevant records are deleted when a dataset is deleted. Scheduled downtime may be advised.
- [24335](https://github.com/apache/superset/pull/24335): Removed deprecated API `/superset/filter/<datasource_type>/<int:datasource_id>/<column>/`
- [24185](https://github.com/apache/superset/pull/24185): `/api/v1/database/test_connection` and `api/v1/database/validate_parameters` permissions changed from `can_read` to `can_write`. Only Admin user's have access.
- [24232](https://github.com/apache/superset/pull/24232): Enables ENABLE_TEMPLATE_REMOVE_FILTERS, DRILL_TO_DETAIL, DASHBOARD_CROSS_FILTERS by default, marks VERSIONED_EXPORT and ENABLE_TEMPLATE_REMOVE_FILTERS as deprecated.

4
superset/charts/commands/delete.py
View File

@ -47,10 +47,6 @@ class DeleteChartCommand(BaseCommand):
self._model = cast(Slice, self._model)
try:
Dashboard.clear_cache_for_slice(slice_id=self._model_id)
# Even though SQLAlchemy should in theory delete rows from the association
# table, sporadically Superset will error because the rows are not deleted.
# Let's do it manually here to prevent the error.
self._model.owners = []
ChartDAO.delete(self._model)
except DAODeleteFailedError as ex:
logger.exception(ex.exception)

1
superset/daos/chart.py
View File

@ -44,7 +44,6 @@ class ChartDAO(BaseDAO[Slice]):
item_ids = [item.id for item in get_iterable(items)]
# bulk delete, first delete related data
for item in get_iterable(items):
item.owners = []
item.dashboards = []
db.session.merge(item)
# bulk delete itself

1
superset/daos/dashboard.py
View File

@ -189,7 +189,6 @@ class DashboardDAO(BaseDAO[Dashboard]):
# bulk delete, first delete related data
for item in get_iterable(items):
item.slices = []
item.owners = []
item.embedded = []
db.session.merge(item)
# bulk delete itself

26
superset/daos/report.py
View File

@ -36,7 +36,7 @@ from superset.reports.models import (
ReportScheduleType,
ReportState,
)
from superset.utils.core import get_iterable, get_user_id
from superset.utils.core import get_user_id
logger = logging.getLogger(__name__)
@ -95,30 +95,6 @@ class ReportScheduleDAO(BaseDAO[ReportSchedule]):
.all()
)
@classmethod
def delete(
cls,
items: ReportSchedule | list[ReportSchedule],
commit: bool = True,
) -> None:
item_ids = [item.id for item in get_iterable(items)]
try:
# Clean owners secondary table
report_schedules = (
db.session.query(ReportSchedule)
.filter(ReportSchedule.id.in_(item_ids))
.all()
)
for report_schedule in report_schedules:
report_schedule.owners = []
for report_schedule in report_schedules:
db.session.delete(report_schedule)
if commit:
db.session.commit()
except SQLAlchemyError as ex:
db.session.rollback()
raise DAODeleteFailedError(str(ex)) from ex
@staticmethod
def validate_unique_creation_method(
dashboard_id: int | None = None, chart_id: int | None = None

1
superset/examples/birth_names.py
View File

@ -537,7 +537,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 = []
db.session.add(dash)
dash.published = True

57
superset/migrations/shared/constraints.py Normal file
View File

@ -0,0 +1,57 @@
from __future__ import annotations
from dataclasses import dataclass
from alembic import op
from sqlalchemy.engine.reflection import Inspector
from superset.utils.core import generic_find_fk_constraint_name
@dataclass(frozen=True)
class ForeignKey:
table: str
referent_table: str
local_cols: list[str]
remote_cols: list[str]
@property
def constraint_name(self) -> str:
return f"fk_{self.table}_{self.local_cols[0]}_{self.referent_table}"
def redefine(
foreign_key: ForeignKey,
on_delete: str | None = None,
on_update: str | None = None,
) -> None:
"""
Redefine the foreign key constraint to include the ON DELETE and ON UPDATE
constructs for cascading purposes.
:params foreign_key: The foreign key constraint
:param ondelete: If set, emit ON DELETE <value> when issuing DDL operations
:param onupdate: If set, emit ON UPDATE <value> when issuing DDL operations
"""
bind = op.get_bind()
insp = Inspector.from_engine(bind)
conv = {"fk": "fk_%(table_name)s_%(column_0_name)s_%(referred_table_name)s"}
with op.batch_alter_table(foreign_key.table, naming_convention=conv) as batch_op:
if constraint := generic_find_fk_constraint_name(
table=foreign_key.table,
columns=set(foreign_key.remote_cols),
referenced=foreign_key.referent_table,
insp=insp,
):
batch_op.drop_constraint(constraint, type_="foreignkey")
batch_op.create_foreign_key(
constraint_name=foreign_key.constraint_name,
referent_table=foreign_key.referent_table,
local_cols=foreign_key.local_cols,
remote_cols=foreign_key.remote_cols,
ondelete=on_delete,
onupdate=on_update,
)

90
superset/migrations/versions/2023-06-22_13-39_6fbe660cac39_add_on_delete_cascade_for_tables_references.py
View File

@ -21,74 +21,46 @@ Revises: 90139bf715e4
Create Date: 2023-06-22 13:39:47.989373
"""
from __future__ import annotations
# revision identifiers, used by Alembic.
revision = "6fbe660cac39"
down_revision = "90139bf715e4"
import sqlalchemy as sa
from alembic import op
from superset.migrations.shared.constraints import ForeignKey, redefine
from superset.utils.core import generic_find_fk_constraint_name
def migrate(ondelete: str | None) -> None:
"""
Redefine the foreign key constraints, via a successive DROP and ADD, for all tables
related to the `tables` table to include the ON DELETE construct for cascading
purposes.
:param ondelete: If set, emit ON DELETE <value> when issuing DDL for this constraint
"""
bind = op.get_bind()
insp = sa.engine.reflection.Inspector.from_engine(bind)
conv = {
"fk": "fk_%(table_name)s_%(column_0_name)s_%(referred_table_name)s",
}
for table in ("sql_metrics", "table_columns"):
with op.batch_alter_table(table, naming_convention=conv) as batch_op:
if constraint := generic_find_fk_constraint_name(
table=table,
columns={"id"},
referenced="tables",
insp=insp,
):
batch_op.drop_constraint(constraint, type_="foreignkey")
batch_op.create_foreign_key(
constraint_name=f"fk_{table}_table_id_tables",
referent_table="tables",
local_cols=["table_id"],
remote_cols=["id"],
ondelete=ondelete,
)
with op.batch_alter_table("sqlatable_user", naming_convention=conv) as batch_op:
for table, column in zip(("ab_user", "tables"), ("user_id", "table_id")):
if constraint := generic_find_fk_constraint_name(
table="sqlatable_user",
columns={"id"},
referenced=table,
insp=insp,
):
batch_op.drop_constraint(constraint, type_="foreignkey")
batch_op.create_foreign_key(
constraint_name=f"fk_sqlatable_user_{column}_{table}",
referent_table=table,
local_cols=[column],
remote_cols=["id"],
ondelete=ondelete,
)
foreign_keys = [
ForeignKey(
table="sql_metrics",
referent_table="tables",
local_cols=["table_id"],
remote_cols=["id"],
),
ForeignKey(
table="table_columns",
referent_table="tables",
local_cols=["table_id"],
remote_cols=["id"],
),
ForeignKey(
table="sqlatable_user",
referent_table="ab_user",
local_cols=["user_id"],
remote_cols=["id"],
),
ForeignKey(
table="sqlatable_user",
referent_table="tables",
local_cols=["table_id"],
remote_cols=["id"],
),
]
def upgrade():
migrate(ondelete="CASCADE")
for foreign_key in foreign_keys:
redefine(foreign_key, on_delete="CASCADE")
def downgrade():
migrate(ondelete=None)
for foreign_key in foreign_keys:
redefine(foreign_key)

78
superset/migrations/versions/2023-07-11_15-51_6d05b0a70c89_add_on_delete_cascade_for_owners_references.py Normal file
View File

@ -0,0 +1,78 @@
# Licensed to the Apache Software Foundation (ASF) under one
# or more contributor license agreements. See the NOTICE file
# distributed with this work for additional information
# regarding copyright ownership. The ASF licenses this file
# to you under the Apache License, Version 2.0 (the
# "License"); you may not use this file except in compliance
# with the License. You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing,
# software distributed under the License is distributed on an
# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
"""add on delete cascade for owners references
Revision ID: 6d05b0a70c89
Revises: f92a3124dd66
Create Date: 2023-07-11 15:51:57.964635
"""
# revision identifiers, used by Alembic.
revision = "6d05b0a70c89"
down_revision = "f92a3124dd66"
from superset.migrations.shared.constraints import ForeignKey, redefine
foreign_keys = [
ForeignKey(
table="dashboard_user",
referent_table="ab_user",
local_cols=["user_id"],
remote_cols=["id"],
),
ForeignKey(
table="dashboard_user",
referent_table="dashboards",
local_cols=["dashboard_id"],
remote_cols=["id"],
),
ForeignKey(
table="report_schedule_user",
referent_table="ab_user",
local_cols=["user_id"],
remote_cols=["id"],
),
ForeignKey(
table="report_schedule_user",
referent_table="report_schedule",
local_cols=["report_schedule_id"],
remote_cols=["id"],
),
ForeignKey(
table="slice_user",
referent_table="ab_user",
local_cols=["user_id"],
remote_cols=["id"],
),
ForeignKey(
table="slice_user",
referent_table="slices",
local_cols=["slice_id"],
remote_cols=["id"],
),
]
def upgrade():
for foreign_key in foreign_keys:
redefine(foreign_key, on_delete="CASCADE")
def downgrade():
for foreign_key in foreign_keys:
redefine(foreign_key)

10
superset/models/dashboard.py
View File

@ -115,8 +115,8 @@ dashboard_user = Table(
"dashboard_user",
metadata,
Column("id", Integer, primary_key=True),
Column("user_id", Integer, ForeignKey("ab_user.id")),
Column("dashboard_id", Integer, ForeignKey("dashboards.id")),
Column("user_id", Integer, ForeignKey("ab_user.id", ondelete="CASCADE")),
Column("dashboard_id", Integer, ForeignKey("dashboards.id", ondelete="CASCADE")),
)
@ -146,7 +146,11 @@ class Dashboard(Model, AuditMixinNullable, ImportExportMixin):
slices: list[Slice] = relationship(
Slice, secondary=dashboard_slices, backref="dashboards"
)
owners = relationship(security_manager.user_model, secondary=dashboard_user)
owners = relationship(
security_manager.user_model,
secondary=dashboard_user,
passive_deletes=True,
)
tags = relationship(
"Tag",
overlaps="objects,tag,tags,tags",

10
superset/models/slice.py
View File

@ -58,8 +58,8 @@ slice_user = Table(
"slice_user",
metadata,
Column("id", Integer, primary_key=True),
Column("user_id", Integer, ForeignKey("ab_user.id")),
Column("slice_id", Integer, ForeignKey("slices.id")),
Column("user_id", Integer, ForeignKey("ab_user.id", ondelete="CASCADE")),
Column("slice_id", Integer, ForeignKey("slices.id", ondelete="CASCADE")),
)
logger = logging.getLogger(__name__)
@ -95,7 +95,11 @@ class Slice( # pylint: disable=too-many-public-methods
last_saved_by = relationship(
security_manager.user_model, foreign_keys=[last_saved_by_fk]
)
owners = relationship(security_manager.user_model, secondary=slice_user)
owners = relationship(
security_manager.user_model,
secondary=slice_user,
passive_deletes=True,
)
tags = relationship(
"Tag",
secondary="tagged_object",

18
superset/reports/models.py
View File

@ -91,9 +91,17 @@ report_schedule_user = Table(
"report_schedule_user",
metadata,
Column("id", Integer, primary_key=True),
Column("user_id", Integer, ForeignKey("ab_user.id"), nullable=False),
Column(
"report_schedule_id", Integer, ForeignKey("report_schedule.id"), nullable=False
"user_id",
Integer,
ForeignKey("ab_user.id", ondelete="CASCADE"),
nullable=False,
),
Column(
"report_schedule_id",
Integer,
ForeignKey("report_schedule.id", ondelete="CASCADE"),
nullable=False,
),
UniqueConstraint("user_id", "report_schedule_id"),
)
@ -132,7 +140,11 @@ class ReportSchedule(Model, AuditMixinNullable, ExtraJSONMixin):
# (Alerts) M-O to database
database_id = Column(Integer, ForeignKey("dbs.id"), nullable=True)
database = relationship(Database, foreign_keys=[database_id])
owners = relationship(security_manager.user_model, secondary=report_schedule_user)
owners = relationship(
security_manager.user_model,
secondary=report_schedule_user,
passive_deletes=True,
)
# (Alerts) Stamped last observations
last_eval_dttm = Column(DateTime)

4
superset/tables/models.py
View File

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

2
tests/integration_tests/charts/api_tests.py
View File

@ -1528,7 +1528,6 @@ class TestChartApi(SupersetTestCase, ApiOwnersTestCaseMixin, InsertChartMixin):
chart = db.session.query(Slice).filter_by(uuid=chart_config["uuid"]).one()
assert chart.table == dataset
chart.owners = []
db.session.delete(chart)
db.session.commit()
db.session.delete(dataset)
@ -1600,7 +1599,6 @@ class TestChartApi(SupersetTestCase, ApiOwnersTestCaseMixin, InsertChartMixin):
dataset = database.tables[0]
chart = db.session.query(Slice).filter_by(uuid=chart_config["uuid"]).one()
chart.owners = []
db.session.delete(chart)
db.session.commit()
db.session.delete(dataset)

2
tests/integration_tests/charts/commands_tests.py
View File

@ -282,8 +282,6 @@ class TestImportChartsCommand(SupersetTestCase):
assert chart.owners == [admin]
chart.owners = []
database.owners = []
db.session.delete(chart)
db.session.delete(dataset)
db.session.delete(database)

7
tests/integration_tests/commands_test.py
View File

@ -146,9 +146,6 @@ class TestImportAssetsCommand(SupersetTestCase):
assert dashboard.owners == [self.user]
dashboard.owners = []
chart.owners = []
database.owners = []
db.session.delete(dashboard)
db.session.delete(chart)
db.session.delete(dataset)
@ -190,10 +187,6 @@ class TestImportAssetsCommand(SupersetTestCase):
chart = dashboard.slices[0]
dataset = chart.table
database = dataset.database
dashboard.owners = []
chart.owners = []
database.owners = []
db.session.delete(dashboard)
db.session.delete(chart)
db.session.delete(dataset)

1
tests/integration_tests/dashboard_tests.py
View File

@ -213,7 +213,6 @@ class TestDashboard(SupersetTestCase):
hidden_dash.dashboard_title = "Not My Dashboard"
hidden_dash.slug = not_my_dash_slug
hidden_dash.slices = []
hidden_dash.owners = []
db.session.add(dash)
db.session.add(hidden_dash)

3
tests/integration_tests/dashboards/commands_tests.py
View File

@ -573,9 +573,6 @@ class TestImportDashboardsCommand(SupersetTestCase):
assert dashboard.owners == [admin]
dashboard.owners = []
chart.owners = []
database.owners = []
db.session.delete(dashboard)
db.session.delete(chart)
db.session.delete(dataset)

2
tests/integration_tests/datasets/commands_tests.py
View File

@ -400,7 +400,6 @@ class TestImportDatasetsCommand(SupersetTestCase):
assert column.description is None
assert column.python_date_format is None
dataset.database.owners = []
db.session.delete(dataset)
db.session.delete(dataset.database)
db.session.commit()
@ -528,7 +527,6 @@ class TestImportDatasetsCommand(SupersetTestCase):
)
assert len(database.tables) == 1
database.owners = []
db.session.delete(database.tables[0])
db.session.delete(database)
db.session.commit()