From bac84a3aac6a9f565aae9cda1820cd8ada014d63 Mon Sep 17 00:00:00 2001 From: Daniel Vaz Gaspar Date: Thu, 26 Nov 2020 08:45:49 +0000 Subject: [PATCH] fix: delete chart, dashboards, dbs with assoc reports (#11801) * fix: delete chart or dashboards with assoc reports * database constraint to reports and tests * add tests for dashboards and database * fix exceptions default text --- superset/charts/commands/bulk_delete.py | 10 +++ superset/charts/commands/delete.py | 10 +++ superset/charts/commands/exceptions.py | 10 ++- superset/dashboards/commands/bulk_delete.py | 10 +++ superset/dashboards/commands/delete.py | 10 +++ superset/dashboards/commands/exceptions.py | 8 +++ superset/databases/commands/delete.py | 11 ++++ superset/databases/commands/exceptions.py | 4 ++ superset/reports/dao.py | 48 ++++++++++++++ superset/reports/schemas.py | 2 +- tests/charts/api_tests.py | 69 +++++++++++++++++++- tests/dashboards/api_tests.py | 71 +++++++++++++++++++++ tests/databases/api_tests.py | 45 +++++++++++++ 13 files changed, 304 insertions(+), 4 deletions(-) diff --git a/superset/charts/commands/bulk_delete.py b/superset/charts/commands/bulk_delete.py index de1af113d..463f17bc0 100644 --- a/superset/charts/commands/bulk_delete.py +++ b/superset/charts/commands/bulk_delete.py @@ -18,9 +18,11 @@ import logging from typing import List, Optional from flask_appbuilder.security.sqla.models import User +from flask_babel import lazy_gettext as _ from superset.charts.commands.exceptions import ( ChartBulkDeleteFailedError, + ChartBulkDeleteFailedReportsExistError, ChartForbiddenError, ChartNotFoundError, ) @@ -29,6 +31,7 @@ from superset.commands.base import BaseCommand from superset.commands.exceptions import DeleteFailedError from superset.exceptions import SupersetSecurityException from superset.models.slice import Slice +from superset.reports.dao import ReportScheduleDAO from superset.views.base import check_ownership logger = logging.getLogger(__name__) @@ -53,6 +56,13 @@ class BulkDeleteChartCommand(BaseCommand): self._models = ChartDAO.find_by_ids(self._model_ids) if not self._models or len(self._models) != len(self._model_ids): raise ChartNotFoundError() + # Check there are no associated ReportSchedules + reports = ReportScheduleDAO.find_by_chart_ids(self._model_ids) + if reports: + report_names = [report.name for report in reports] + raise ChartBulkDeleteFailedReportsExistError( + _("There are associated alerts or reports: %s" % ",".join(report_names)) + ) # Check ownership for model in self._models: try: diff --git a/superset/charts/commands/delete.py b/superset/charts/commands/delete.py index c6392a905..5b33a5041 100644 --- a/superset/charts/commands/delete.py +++ b/superset/charts/commands/delete.py @@ -19,9 +19,11 @@ from typing import Optional from flask_appbuilder.models.sqla import Model from flask_appbuilder.security.sqla.models import User +from flask_babel import lazy_gettext as _ from superset.charts.commands.exceptions import ( ChartDeleteFailedError, + ChartDeleteFailedReportsExistError, ChartForbiddenError, ChartNotFoundError, ) @@ -31,6 +33,7 @@ from superset.dao.exceptions import DAODeleteFailedError from superset.exceptions import SupersetSecurityException from superset.models.dashboard import Dashboard from superset.models.slice import Slice +from superset.reports.dao import ReportScheduleDAO from superset.views.base import check_ownership logger = logging.getLogger(__name__) @@ -57,6 +60,13 @@ class DeleteChartCommand(BaseCommand): self._model = ChartDAO.find_by_id(self._model_id) if not self._model: raise ChartNotFoundError() + # Check there are no associated ReportSchedules + reports = ReportScheduleDAO.find_by_chart_id(self._model_id) + if reports: + report_names = [report.name for report in reports] + raise ChartDeleteFailedReportsExistError( + _("There are associated alerts or reports: %s" % ",".join(report_names)) + ) # Check ownership try: check_ownership(self._model) diff --git a/superset/charts/commands/exceptions.py b/superset/charts/commands/exceptions.py index dd146d790..51b5ca84f 100644 --- a/superset/charts/commands/exceptions.py +++ b/superset/charts/commands/exceptions.py @@ -78,13 +78,21 @@ class ChartDeleteFailedError(DeleteFailedError): message = _("Chart could not be deleted.") +class ChartDeleteFailedReportsExistError(ChartDeleteFailedError): + message = _("There are associated alerts or reports") + + class ChartForbiddenError(ForbiddenError): message = _("Changing this chart is forbidden") -class ChartBulkDeleteFailedError(CreateFailedError): +class ChartBulkDeleteFailedError(DeleteFailedError): message = _("Charts could not be deleted.") +class ChartBulkDeleteFailedReportsExistError(ChartBulkDeleteFailedError): + message = _("There are associated alerts or reports") + + class ChartImportError(ImportFailedError): message = _("Import chart failed for an unknown reason") diff --git a/superset/dashboards/commands/bulk_delete.py b/superset/dashboards/commands/bulk_delete.py index cb2bab62e..27787cc61 100644 --- a/superset/dashboards/commands/bulk_delete.py +++ b/superset/dashboards/commands/bulk_delete.py @@ -18,17 +18,20 @@ import logging from typing import List, Optional from flask_appbuilder.security.sqla.models import User +from flask_babel import lazy_gettext as _ from superset.commands.base import BaseCommand from superset.commands.exceptions import DeleteFailedError from superset.dashboards.commands.exceptions import ( DashboardBulkDeleteFailedError, + DashboardBulkDeleteFailedReportsExistError, DashboardForbiddenError, DashboardNotFoundError, ) from superset.dashboards.dao import DashboardDAO from superset.exceptions import SupersetSecurityException from superset.models.dashboard import Dashboard +from superset.reports.dao import ReportScheduleDAO from superset.views.base import check_ownership logger = logging.getLogger(__name__) @@ -54,6 +57,13 @@ class BulkDeleteDashboardCommand(BaseCommand): self._models = DashboardDAO.find_by_ids(self._model_ids) if not self._models or len(self._models) != len(self._model_ids): raise DashboardNotFoundError() + # Check there are no associated ReportSchedules + reports = ReportScheduleDAO.find_by_dashboard_ids(self._model_ids) + if reports: + report_names = [report.name for report in reports] + raise DashboardBulkDeleteFailedReportsExistError( + _("There are associated alerts or reports: %s" % ",".join(report_names)) + ) # Check ownership for model in self._models: try: diff --git a/superset/dashboards/commands/delete.py b/superset/dashboards/commands/delete.py index 08d0fcb1d..795288a84 100644 --- a/superset/dashboards/commands/delete.py +++ b/superset/dashboards/commands/delete.py @@ -19,17 +19,20 @@ from typing import Optional from flask_appbuilder.models.sqla import Model from flask_appbuilder.security.sqla.models import User +from flask_babel import lazy_gettext as _ from superset.commands.base import BaseCommand from superset.dao.exceptions import DAODeleteFailedError from superset.dashboards.commands.exceptions import ( DashboardDeleteFailedError, + DashboardDeleteFailedReportsExistError, DashboardForbiddenError, DashboardNotFoundError, ) from superset.dashboards.dao import DashboardDAO from superset.exceptions import SupersetSecurityException from superset.models.dashboard import Dashboard +from superset.reports.dao import ReportScheduleDAO from superset.views.base import check_ownership logger = logging.getLogger(__name__) @@ -55,6 +58,13 @@ class DeleteDashboardCommand(BaseCommand): self._model = DashboardDAO.find_by_id(self._model_id) if not self._model: raise DashboardNotFoundError() + # Check there are no associated ReportSchedules + reports = ReportScheduleDAO.find_by_dashboard_id(self._model_id) + if reports: + report_names = [report.name for report in reports] + raise DashboardDeleteFailedReportsExistError( + _("There are associated alerts or reports: %s" % ",".join(report_names)) + ) # Check ownership try: check_ownership(self._model) diff --git a/superset/dashboards/commands/exceptions.py b/superset/dashboards/commands/exceptions.py index cc645da4b..03413aa25 100644 --- a/superset/dashboards/commands/exceptions.py +++ b/superset/dashboards/commands/exceptions.py @@ -53,6 +53,10 @@ class DashboardBulkDeleteFailedError(CreateFailedError): message = _("Dashboards could not be deleted.") +class DashboardBulkDeleteFailedReportsExistError(DashboardBulkDeleteFailedError): + message = _("There are associated alerts or reports") + + class DashboardUpdateFailedError(UpdateFailedError): message = _("Dashboard could not be updated.") @@ -61,6 +65,10 @@ class DashboardDeleteFailedError(DeleteFailedError): message = _("Dashboard could not be deleted.") +class DashboardDeleteFailedReportsExistError(DashboardDeleteFailedError): + message = _("There are associated alerts or reports") + + class DashboardForbiddenError(ForbiddenError): message = _("Changing this Dashboard is forbidden") diff --git a/superset/databases/commands/delete.py b/superset/databases/commands/delete.py index 5ea8de373..5f002dbfb 100644 --- a/superset/databases/commands/delete.py +++ b/superset/databases/commands/delete.py @@ -19,16 +19,19 @@ from typing import Optional from flask_appbuilder.models.sqla import Model from flask_appbuilder.security.sqla.models import User +from flask_babel import lazy_gettext as _ from superset.commands.base import BaseCommand from superset.dao.exceptions import DAODeleteFailedError from superset.databases.commands.exceptions import ( DatabaseDeleteDatasetsExistFailedError, DatabaseDeleteFailedError, + DatabaseDeleteFailedReportsExistError, DatabaseNotFoundError, ) from superset.databases.dao import DatabaseDAO from superset.models.core import Database +from superset.reports.dao import ReportScheduleDAO logger = logging.getLogger(__name__) @@ -53,6 +56,14 @@ class DeleteDatabaseCommand(BaseCommand): self._model = DatabaseDAO.find_by_id(self._model_id) if not self._model: raise DatabaseNotFoundError() + # Check there are no associated ReportSchedules + reports = ReportScheduleDAO.find_by_database_id(self._model_id) + + if reports: + report_names = [report.name for report in reports] + raise DatabaseDeleteFailedReportsExistError( + _("There are associated alerts or reports: %s" % ",".join(report_names)) + ) # Check if there are datasets for this database if self._model.tables: raise DatabaseDeleteDatasetsExistFailedError() diff --git a/superset/databases/commands/exceptions.py b/superset/databases/commands/exceptions.py index 48c63a2c9..b8b1c67f0 100644 --- a/superset/databases/commands/exceptions.py +++ b/superset/databases/commands/exceptions.py @@ -113,6 +113,10 @@ class DatabaseDeleteFailedError(DeleteFailedError): message = _("Database could not be deleted.") +class DatabaseDeleteFailedReportsExistError(DatabaseDeleteFailedError): + message = _("There are associated alerts or reports") + + class DatabaseSecurityUnsafeError(DBSecurityException): message = _("Stopped an unsafe database connection") diff --git a/superset/reports/dao.py b/superset/reports/dao.py index 6081fc8ef..aaa0c3919 100644 --- a/superset/reports/dao.py +++ b/superset/reports/dao.py @@ -38,6 +38,54 @@ logger = logging.getLogger(__name__) class ReportScheduleDAO(BaseDAO): model_cls = ReportSchedule + @staticmethod + def find_by_chart_id(chart_id: int) -> List[ReportSchedule]: + return ( + db.session.query(ReportSchedule) + .filter(ReportSchedule.chart_id == chart_id) + .all() + ) + + @staticmethod + def find_by_chart_ids(chart_ids: List[int]) -> List[ReportSchedule]: + return ( + db.session.query(ReportSchedule) + .filter(ReportSchedule.chart_id.in_(chart_ids)) + .all() + ) + + @staticmethod + def find_by_dashboard_id(dashboard_id: int) -> List[ReportSchedule]: + return ( + db.session.query(ReportSchedule) + .filter(ReportSchedule.dashboard_id == dashboard_id) + .all() + ) + + @staticmethod + def find_by_dashboard_ids(dashboard_ids: List[int]) -> List[ReportSchedule]: + return ( + db.session.query(ReportSchedule) + .filter(ReportSchedule.dashboard_id.in_(dashboard_ids)) + .all() + ) + + @staticmethod + def find_by_database_id(database_id: int) -> List[ReportSchedule]: + return ( + db.session.query(ReportSchedule) + .filter(ReportSchedule.database_id == database_id) + .all() + ) + + @staticmethod + def find_by_database_ids(database_ids: List[int]) -> List[ReportSchedule]: + return ( + db.session.query(ReportSchedule) + .filter(ReportSchedule.database_id.in_(database_ids)) + .all() + ) + @staticmethod def bulk_delete( models: Optional[List[ReportSchedule]], commit: bool = True diff --git a/superset/reports/schemas.py b/superset/reports/schemas.py index 7613278ad..a03b3394c 100644 --- a/superset/reports/schemas.py +++ b/superset/reports/schemas.py @@ -136,7 +136,7 @@ class ReportSchedulePostSchema(Schema): crontab = fields.String( description=crontab_description, validate=[validate_crontab, Length(1, 50)], - example="*/5 * * * * *", + example="*/5 * * * *", allow_none=False, required=True, ) diff --git a/tests/charts/api_tests.py b/tests/charts/api_tests.py index 4e44bcc43..39abb6ced 100644 --- a/tests/charts/api_tests.py +++ b/tests/charts/api_tests.py @@ -38,6 +38,7 @@ from superset.connectors.connector_registry import ConnectorRegistry from superset.extensions import db, security_manager from superset.models.core import Database, FavStar, FavStarClassName from superset.models.dashboard import Dashboard +from superset.models.reports import ReportSchedule, ReportScheduleType from superset.models.slice import Slice from superset.utils import core as utils from tests.base_api_tests import ApiOwnersTestCaseMixin @@ -117,6 +118,26 @@ class TestChartApi(SupersetTestCase, ApiOwnersTestCaseMixin): db.session.delete(fav_chart) db.session.commit() + @pytest.fixture() + def create_chart_with_report(self): + with self.create_app().app_context(): + admin = self.get_user("admin") + chart = self.insert_chart(f"chart_report", [admin.id], 1) + report_schedule = ReportSchedule( + type=ReportScheduleType.REPORT, + name="report_with_chart", + crontab="* * * * *", + chart=chart, + ) + db.session.commit() + + yield chart + + # rollback changes + db.session.delete(report_schedule) + db.session.delete(chart) + db.session.commit() + def test_delete_chart(self): """ Chart API: Test delete @@ -174,6 +195,26 @@ class TestChartApi(SupersetTestCase, ApiOwnersTestCaseMixin): rv = self.delete_assert_metric(uri, "delete") self.assertEqual(rv.status_code, 404) + @pytest.mark.usefixtures("create_chart_with_report") + def test_delete_chart_with_report(self): + """ + Chart API: Test delete with associated report + """ + self.login(username="admin") + chart = ( + db.session.query(Slice) + .filter(Slice.slice_name == "chart_report") + .one_or_none() + ) + uri = f"api/v1/chart/{chart.id}" + rv = self.client.delete(uri) + response = json.loads(rv.data.decode("utf-8")) + self.assertEqual(rv.status_code, 422) + expected_response = { + "message": "There are associated alerts or reports: report_with_chart" + } + self.assertEqual(response, expected_response) + def test_delete_bulk_charts_not_found(self): """ Chart API: Test delete bulk not found @@ -181,11 +222,35 @@ class TestChartApi(SupersetTestCase, ApiOwnersTestCaseMixin): max_id = db.session.query(func.max(Slice.id)).scalar() chart_ids = [max_id + 1, max_id + 2] self.login(username="admin") - argument = chart_ids - uri = f"api/v1/chart/?q={prison.dumps(argument)}" + uri = f"api/v1/chart/?q={prison.dumps(chart_ids)}" rv = self.delete_assert_metric(uri, "bulk_delete") self.assertEqual(rv.status_code, 404) + @pytest.mark.usefixtures("create_chart_with_report", "create_charts") + def test_bulk_delete_chart_with_report(self): + """ + Chart API: Test bulk delete with associated report + """ + self.login(username="admin") + chart_with_report = ( + db.session.query(Slice.id) + .filter(Slice.slice_name == "chart_report") + .one_or_none() + ) + + charts = db.session.query(Slice.id).filter(Slice.slice_name.like("name%")).all() + chart_ids = [chart.id for chart in charts] + chart_ids.append(chart_with_report.id) + + uri = f"api/v1/chart/?q={prison.dumps(chart_ids)}" + rv = self.client.delete(uri) + response = json.loads(rv.data.decode("utf-8")) + self.assertEqual(rv.status_code, 422) + expected_response = { + "message": "There are associated alerts or reports: report_with_chart" + } + self.assertEqual(response, expected_response) + def test_delete_chart_admin_not_owned(self): """ Chart API: Test admin delete not owned diff --git a/tests/dashboards/api_tests.py b/tests/dashboards/api_tests.py index 24d461d1d..aba79f9b9 100644 --- a/tests/dashboards/api_tests.py +++ b/tests/dashboards/api_tests.py @@ -33,6 +33,7 @@ from sqlalchemy import and_ from superset import db, security_manager from superset.models.dashboard import Dashboard from superset.models.core import FavStar, FavStarClassName +from superset.models.reports import ReportSchedule, ReportScheduleType from superset.models.slice import Slice from superset.views.base import generate_download_headers @@ -121,6 +122,28 @@ class TestDashboardApi(SupersetTestCase, ApiOwnersTestCaseMixin): db.session.delete(fav_dashboard) db.session.commit() + @pytest.fixture() + def create_dashboard_with_report(self): + with self.create_app().app_context(): + admin = self.get_user("admin") + dashboard = self.insert_dashboard( + f"dashboard_report", "dashboard_report", [admin.id] + ) + report_schedule = ReportSchedule( + type=ReportScheduleType.REPORT, + name="report_with_dashboard", + crontab="* * * * *", + dashboard=dashboard, + ) + db.session.commit() + + yield dashboard + + # rollback changes + db.session.delete(report_schedule) + db.session.delete(dashboard) + db.session.commit() + def test_get_dashboard(self): """ Dashboard API: Test get dashboard @@ -493,6 +516,26 @@ class TestDashboardApi(SupersetTestCase, ApiOwnersTestCaseMixin): rv = self.client.delete(uri) self.assertEqual(rv.status_code, 404) + @pytest.mark.usefixtures("create_dashboard_with_report") + def test_delete_dashboard_with_report(self): + """ + Dashboard API: Test delete with associated report + """ + self.login(username="admin") + dashboard = ( + db.session.query(Dashboard.id) + .filter(Dashboard.dashboard_title == "dashboard_report") + .one_or_none() + ) + uri = f"api/v1/dashboard/{dashboard.id}" + rv = self.client.delete(uri) + response = json.loads(rv.data.decode("utf-8")) + self.assertEqual(rv.status_code, 422) + expected_response = { + "message": "There are associated alerts or reports: report_with_dashboard" + } + self.assertEqual(response, expected_response) + def test_delete_bulk_dashboards_not_found(self): """ Dashboard API: Test delete bulk not found @@ -504,6 +547,34 @@ class TestDashboardApi(SupersetTestCase, ApiOwnersTestCaseMixin): rv = self.client.delete(uri) self.assertEqual(rv.status_code, 404) + @pytest.mark.usefixtures("create_dashboard_with_report", "create_dashboards") + def test_bulk_delete_dashboard_with_report(self): + """ + Dashboard API: Test bulk delete with associated report + """ + self.login(username="admin") + dashboard_with_report = ( + db.session.query(Dashboard.id) + .filter(Dashboard.dashboard_title == "dashboard_report") + .one_or_none() + ) + dashboards = ( + db.session.query(Dashboard) + .filter(Dashboard.dashboard_title.like("title%")) + .all() + ) + + dashboard_ids = [dashboard.id for dashboard in dashboards] + dashboard_ids.append(dashboard_with_report.id) + uri = f"api/v1/dashboard/?q={prison.dumps(dashboard_ids)}" + rv = self.client.delete(uri) + response = json.loads(rv.data.decode("utf-8")) + self.assertEqual(rv.status_code, 422) + expected_response = { + "message": "There are associated alerts or reports: report_with_dashboard" + } + self.assertEqual(response, expected_response) + def test_delete_dashboard_admin_not_owned(self): """ Dashboard API: Test admin delete not owned diff --git a/tests/databases/api_tests.py b/tests/databases/api_tests.py index 9a290e159..6e2dbef80 100644 --- a/tests/databases/api_tests.py +++ b/tests/databases/api_tests.py @@ -30,6 +30,7 @@ from sqlalchemy.sql import func from superset import db, security_manager from superset.connectors.sqla.models import SqlaTable from superset.models.core import Database +from superset.models.reports import ReportSchedule, ReportScheduleType from superset.utils.core import get_example_database, get_main_database from tests.base_tests import SupersetTestCase from tests.fixtures.certificates import ssl_certificate @@ -66,6 +67,30 @@ class TestDatabaseApi(SupersetTestCase): db.session.commit() return database + @pytest.fixture() + def create_database_with_report(self): + with self.create_app().app_context(): + example_db = get_example_database() + database = self.insert_database( + "database_with_report", + example_db.sqlalchemy_uri_decrypted, + expose_in_sqllab=True, + ) + report_schedule = ReportSchedule( + type=ReportScheduleType.ALERT, + name="report_with_database", + crontab="* * * * *", + database=database, + ) + db.session.add(report_schedule) + db.session.commit() + yield database + + # rollback changes + db.session.delete(report_schedule) + db.session.delete(database) + db.session.commit() + def test_get_items(self): """ Database API: Test get items @@ -486,6 +511,26 @@ class TestDatabaseApi(SupersetTestCase): rv = self.delete_assert_metric(uri, "delete") self.assertEqual(rv.status_code, 422) + @pytest.mark.usefixtures("create_database_with_report") + def test_delete_database_with_report(self): + """ + Database API: Test delete with associated report + """ + self.login(username="admin") + database = ( + db.session.query(Database) + .filter(Database.database_name == "database_with_report") + .one_or_none() + ) + uri = f"api/v1/database/{database.id}" + rv = self.client.delete(uri) + response = json.loads(rv.data.decode("utf-8")) + self.assertEqual(rv.status_code, 422) + expected_response = { + "message": "There are associated alerts or reports: report_with_database" + } + self.assertEqual(response, expected_response) + def test_get_table_metadata(self): """ Database API: Test get table metadata info