From 62fb9740d3cf93d5b2fec9fb3b1ce351404a58bc Mon Sep 17 00:00:00 2001 From: Arash Date: Fri, 20 Aug 2021 12:18:31 -0400 Subject: [PATCH 01/11] backend creation method validation --- superset/models/dashboard.py | 4 +--- superset/models/slice.py | 2 +- superset/reports/commands/create.py | 17 ++++++++++++++++- superset/reports/commands/exceptions.py | 12 ++++++++++++ superset/reports/dao.py | 18 ++++++++++++++++++ 5 files changed, 48 insertions(+), 5 deletions(-) diff --git a/superset/models/dashboard.py b/superset/models/dashboard.py index d74efa72cde8f..8a9abc8ce2777 100644 --- a/superset/models/dashboard.py +++ b/superset/models/dashboard.py @@ -131,9 +131,7 @@ def copy_dashboard( ) -class Dashboard( # pylint: disable=too-many-instance-attributes - Model, AuditMixinNullable, ImportExportMixin -): +class Dashboard(Model, AuditMixinNullable, ImportExportMixin): """The dashboard object!""" diff --git a/superset/models/slice.py b/superset/models/slice.py index 310343a1f0a26..40573396cfa45 100644 --- a/superset/models/slice.py +++ b/superset/models/slice.py @@ -53,7 +53,7 @@ logger = logging.getLogger(__name__) -class Slice( # pylint: disable=too-many-instance-attributes,too-many-public-methods +class Slice( # pylint: disable=too-many-instance-attributes, too-many-public-methods Model, AuditMixinNullable, ImportExportMixin ): """A slice is essentially a report or a view on data""" diff --git a/superset/reports/commands/create.py b/superset/reports/commands/create.py index 23ed27e89c440..f14773bea2fea 100644 --- a/superset/reports/commands/create.py +++ b/superset/reports/commands/create.py @@ -25,12 +25,13 @@ from superset.commands.base import CreateMixin from superset.dao.exceptions import DAOCreateFailedError from superset.databases.dao import DatabaseDAO -from superset.models.reports import ReportScheduleType +from superset.models.reports import ReportCreationMethodType, ReportScheduleType from superset.reports.commands.base import BaseReportScheduleCommand from superset.reports.commands.exceptions import ( DatabaseNotFoundValidationError, ReportScheduleAlertRequiredDatabaseValidationError, ReportScheduleCreateFailedError, + ReportScheduleCreationMethodUniquenessValidationError, ReportScheduleInvalidError, ReportScheduleNameUniquenessValidationError, ReportScheduleRequiredTypeValidationError, @@ -59,6 +60,10 @@ def validate(self) -> None: owner_ids: Optional[List[int]] = self._properties.get("owners") name = self._properties.get("name", "") report_type = self._properties.get("type") + creation_method = self._properties.get("creation_method") + chart_id = self._properties.get("chart_id") + dashboard_id = self._properties.get("dashboard_id") + user_id = self._actor.id # Validate type is required if not report_type: @@ -84,6 +89,16 @@ def validate(self) -> None: # Validate chart or dashboard relations self.validate_chart_dashboard(exceptions) + # Validate that each chart or dashboard only has one report with + # the respective creation method. + if ( + creation_method != ReportCreationMethodType.ALERTS_REPORTS + and not ReportScheduleDAO.validate_unique_creation_method( + user_id, dashboard_id, chart_id + ) + ): + exceptions.append(ReportScheduleCreationMethodUniquenessValidationError) + if "validator_config_json" in self._properties: self._properties["validator_config_json"] = json.dumps( self._properties["validator_config_json"] diff --git a/superset/reports/commands/exceptions.py b/superset/reports/commands/exceptions.py index dfe2402da0449..ce53436afba8b 100644 --- a/superset/reports/commands/exceptions.py +++ b/superset/reports/commands/exceptions.py @@ -137,6 +137,18 @@ def __init__(self) -> None: super().__init__([_("Name must be unique")], field_name="name") +class ReportScheduleCreationMethodUniquenessValidationError(ValidationError): + """ + Marshmallow validation error for Report Schedule with creation method charts + or dashboards already existing on a report. + """ + + def __init__(self) -> None: + super().__init__( + [_("Resource already has an attached report")], field_name="creation_method" + ) + + class AlertQueryMultipleRowsError(CommandException): message = _("Alert query returned more then one row.") diff --git a/superset/reports/dao.py b/superset/reports/dao.py index ce69ba004c470..6b68da3dafba9 100644 --- a/superset/reports/dao.py +++ b/superset/reports/dao.py @@ -113,6 +113,24 @@ def bulk_delete( db.session.rollback() raise DAODeleteFailedError(str(ex)) from ex + @staticmethod + def validate_unique_creation_method( + user_id: int, dashboard_id: Optional[int] = None, chart_id: Optional[int] = None + ) -> bool: + """ + Validate if the user already has a chart or dashboard + with a report attached form the self subscribe reports + """ + query = db.session.query(ReportSchedule).filter( + ReportSchedule.created_by_fk == user_id + ) + if dashboard_id is not None: + query = query.filter(ReportSchedule.dashboard_id == dashboard_id) + + if chart_id is not None: + query = query.filter(ReportSchedule.chart_id == chart_id) + return query.one_or_none() + @staticmethod def validate_update_uniqueness( name: str, report_type: str, report_schedule_id: Optional[int] = None From 95a720758151fb0d49a8e825dcc5f8588cdb0c64 Mon Sep 17 00:00:00 2001 From: Arash Date: Tue, 24 Aug 2021 17:01:37 -0400 Subject: [PATCH 02/11] added tests --- superset/models/dashboard.py | 4 +- superset/models/slice.py | 2 +- superset/reports/commands/base.py | 1 - superset/reports/commands/exceptions.py | 3 +- superset/reports/dao.py | 3 +- tests/integration_tests/reports/api_tests.py | 93 +++++++++++++++++++- 6 files changed, 98 insertions(+), 8 deletions(-) diff --git a/superset/models/dashboard.py b/superset/models/dashboard.py index 3b15841d7011e..11fc26d2c9af8 100644 --- a/superset/models/dashboard.py +++ b/superset/models/dashboard.py @@ -129,7 +129,9 @@ def copy_dashboard( ) -class Dashboard(Model, AuditMixinNullable, ImportExportMixin): +class Dashboard( # pylint: disable=too-many-instance-attributes + Model, AuditMixinNullable, ImportExportMixin +): """The dashboard object!""" __tablename__ = "dashboards" diff --git a/superset/models/slice.py b/superset/models/slice.py index 28473d53d14ec..3d00502d4c2d5 100644 --- a/superset/models/slice.py +++ b/superset/models/slice.py @@ -53,7 +53,7 @@ logger = logging.getLogger(__name__) -class Slice( # pylint: disable=too-many-public-methods +class Slice( # pylint: disable=too-many-public-methods, too-many-instance-attributes Model, AuditMixinNullable, ImportExportMixin ): """A slice is essentially a report or a view on data""" diff --git a/superset/reports/commands/base.py b/superset/reports/commands/base.py index cc26b81e5f298..3582767ef65f2 100644 --- a/superset/reports/commands/base.py +++ b/superset/reports/commands/base.py @@ -58,7 +58,6 @@ def validate_chart_dashboard( return if creation_method == ReportCreationMethodType.DASHBOARDS and not dashboard_id: - print("I am here") exceptions.append(DashboardNotSavedValidationError()) return diff --git a/superset/reports/commands/exceptions.py b/superset/reports/commands/exceptions.py index f937bdba7e265..fa1af12d6ce7b 100644 --- a/superset/reports/commands/exceptions.py +++ b/superset/reports/commands/exceptions.py @@ -175,7 +175,8 @@ class ReportScheduleCreationMethodUniquenessValidationError(ValidationError): def __init__(self) -> None: super().__init__( - [_("Resource already has an attached report")], field_name="creation_method" + [_("Resource already has an attached report.")], + field_name="creation_method", ) diff --git a/superset/reports/dao.py b/superset/reports/dao.py index 24e3a79fb5cf1..7d8297285ccd4 100644 --- a/superset/reports/dao.py +++ b/superset/reports/dao.py @@ -128,7 +128,8 @@ def validate_unique_creation_method( if query is not None and chart_id is not None: query = query.filter(ReportSchedule.chart_id == chart_id) - return query.one_or_none() + print(query) + return not db.session.query(query.exists()).scalar() @staticmethod def validate_update_uniqueness( diff --git a/tests/integration_tests/reports/api_tests.py b/tests/integration_tests/reports/api_tests.py index 87341c8aaf46c..6647bf2ed24a6 100644 --- a/tests/integration_tests/reports/api_tests.py +++ b/tests/integration_tests/reports/api_tests.py @@ -784,15 +784,102 @@ def test_no_dashboard_report_schedule_schema(self): uri = "api/v1/report/" rv = self.client.post(uri, json=report_schedule_data) data = json.loads(rv.data.decode("utf-8")) - print(rv) assert rv.status_code == 422 - print(data) - print(data["message"]) assert ( data["message"]["dashboard"] == "Please save your dashboard first, then try creating a new email report." ) + @pytest.mark.usefixtures( + "load_birth_names_dashboard_with_slices", "create_report_schedules" + ) + def test_create_multiple_creation_method_report_schedule_charts(self): + """ + ReportSchedule Api: Test create multiple reports with the same creation method + """ + self.login(username="admin") + chart = db.session.query(Slice).first() + dashboard = db.session.query(Dashboard).first() + example_db = get_example_database() + report_schedule_data = { + "type": ReportScheduleType.REPORT, + "name": "name4", + "description": "description", + "creation_method": ReportCreationMethodType.CHARTS, + "crontab": "0 9 * * *", + "working_timeout": 3600, + "chart": chart.id, + } + uri = "api/v1/report/" + rv = self.client.post(uri, json=report_schedule_data) + data = json.loads(rv.data.decode("utf-8")) + print(data) + assert rv.status_code == 201 + + # this second time it should receive an error because the chart has an attached report + # with the same creation method from the same user. + report_schedule_data = { + "type": ReportScheduleType.REPORT, + "name": "name5", + "description": "description", + "creation_method": ReportCreationMethodType.CHARTS, + "crontab": "0 9 * * *", + "working_timeout": 3600, + "chart": chart.id, + } + uri = "api/v1/report/" + rv = self.client.post(uri, json=report_schedule_data) + data = json.loads(rv.data.decode("utf-8")) + assert rv.status_code == 422 + assert data["message"]["creation_method"] == [ + "Resource already has an attached report." + ] + + @pytest.mark.usefixtures( + "load_birth_names_dashboard_with_slices", "create_report_schedules" + ) + def test_create_multiple_creation_method_report_schedule_dashboards(self): + """ + ReportSchedule Api: Test create multiple reports with the same creation method + """ + self.login(username="admin") + chart = db.session.query(Slice).first() + dashboard = db.session.query(Dashboard).first() + example_db = get_example_database() + report_schedule_data = { + "type": ReportScheduleType.REPORT, + "name": "name4", + "description": "description", + "creation_method": ReportCreationMethodType.DASHBOARDS, + "crontab": "0 9 * * *", + "working_timeout": 3600, + "dashboard": dashboard.id, + } + uri = "api/v1/report/" + rv = self.client.post(uri, json=report_schedule_data) + data = json.loads(rv.data.decode("utf-8")) + print(data) + assert rv.status_code == 201 + + # this second time it should receive an error because the dashboard has an attached report + # with the same creation method from the same user. + report_schedule_data = { + "type": ReportScheduleType.REPORT, + "name": "name5", + "description": "description", + "creation_method": ReportCreationMethodType.DASHBOARDS, + "crontab": "0 9 * * *", + "working_timeout": 3600, + "dashboard": dashboard.id, + } + uri = "api/v1/report/" + rv = self.client.post(uri, json=report_schedule_data) + data = json.loads(rv.data.decode("utf-8")) + assert rv.status_code == 422 + assert data["message"]["creation_method"] == [ + "Resource already has an attached report." + ] + @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") def test_create_report_schedule_chart_dash_validation(self): """ From 603c674d8847f6877daba390578465e0c824fd53 Mon Sep 17 00:00:00 2001 From: AAfghahi <48933336+AAfghahi@users.noreply.github.com> Date: Tue, 24 Aug 2021 18:09:08 -0400 Subject: [PATCH 03/11] Update superset/reports/dao.py Co-authored-by: Beto Dealmeida --- superset/reports/dao.py | 1 - 1 file changed, 1 deletion(-) diff --git a/superset/reports/dao.py b/superset/reports/dao.py index 7d8297285ccd4..ba15c95c7297c 100644 --- a/superset/reports/dao.py +++ b/superset/reports/dao.py @@ -128,7 +128,6 @@ def validate_unique_creation_method( if query is not None and chart_id is not None: query = query.filter(ReportSchedule.chart_id == chart_id) - print(query) return not db.session.query(query.exists()).scalar() @staticmethod From 15c6ed2d729546634459c1637f9f1693d9cf7cd3 Mon Sep 17 00:00:00 2001 From: AAfghahi <48933336+AAfghahi@users.noreply.github.com> Date: Tue, 24 Aug 2021 18:09:23 -0400 Subject: [PATCH 04/11] Update superset/reports/dao.py Co-authored-by: Beto Dealmeida --- superset/reports/dao.py | 1 + 1 file changed, 1 insertion(+) diff --git a/superset/reports/dao.py b/superset/reports/dao.py index ba15c95c7297c..bb56cd7b79582 100644 --- a/superset/reports/dao.py +++ b/superset/reports/dao.py @@ -128,6 +128,7 @@ def validate_unique_creation_method( if query is not None and chart_id is not None: query = query.filter(ReportSchedule.chart_id == chart_id) + return not db.session.query(query.exists()).scalar() @staticmethod From 455bdc609d3a2789e4b9259c025a86dd06476c6f Mon Sep 17 00:00:00 2001 From: AAfghahi <48933336+AAfghahi@users.noreply.github.com> Date: Tue, 24 Aug 2021 18:09:29 -0400 Subject: [PATCH 05/11] Update tests/integration_tests/reports/api_tests.py Co-authored-by: Beto Dealmeida --- tests/integration_tests/reports/api_tests.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/integration_tests/reports/api_tests.py b/tests/integration_tests/reports/api_tests.py index 6647bf2ed24a6..801673ecc50ab 100644 --- a/tests/integration_tests/reports/api_tests.py +++ b/tests/integration_tests/reports/api_tests.py @@ -858,7 +858,6 @@ def test_create_multiple_creation_method_report_schedule_dashboards(self): uri = "api/v1/report/" rv = self.client.post(uri, json=report_schedule_data) data = json.loads(rv.data.decode("utf-8")) - print(data) assert rv.status_code == 201 # this second time it should receive an error because the dashboard has an attached report From 6b5692d34b5e70353dd1dc56c2ef87b9f4fd9655 Mon Sep 17 00:00:00 2001 From: AAfghahi <48933336+AAfghahi@users.noreply.github.com> Date: Tue, 24 Aug 2021 18:09:33 -0400 Subject: [PATCH 06/11] Update tests/integration_tests/reports/api_tests.py Co-authored-by: Beto Dealmeida --- tests/integration_tests/reports/api_tests.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/integration_tests/reports/api_tests.py b/tests/integration_tests/reports/api_tests.py index 801673ecc50ab..cfebb8b19b399 100644 --- a/tests/integration_tests/reports/api_tests.py +++ b/tests/integration_tests/reports/api_tests.py @@ -813,7 +813,6 @@ def test_create_multiple_creation_method_report_schedule_charts(self): uri = "api/v1/report/" rv = self.client.post(uri, json=report_schedule_data) data = json.loads(rv.data.decode("utf-8")) - print(data) assert rv.status_code == 201 # this second time it should receive an error because the chart has an attached report From 9eac7ebf16b335a712fe66fe87f981fd53fffebf Mon Sep 17 00:00:00 2001 From: AAfghahi <48933336+AAfghahi@users.noreply.github.com> Date: Tue, 24 Aug 2021 18:09:48 -0400 Subject: [PATCH 07/11] Update superset/reports/dao.py Co-authored-by: Beto Dealmeida --- superset/reports/dao.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/superset/reports/dao.py b/superset/reports/dao.py index bb56cd7b79582..c9ebd112b7d9e 100644 --- a/superset/reports/dao.py +++ b/superset/reports/dao.py @@ -126,7 +126,7 @@ def validate_unique_creation_method( if query is not None and dashboard_id is not None: query = query.filter(ReportSchedule.dashboard_id == dashboard_id) - if query is not None and chart_id is not None: + if chart_id is not None: query = query.filter(ReportSchedule.chart_id == chart_id) return not db.session.query(query.exists()).scalar() From 7b37ab4a4ad9c84ba8e2585dc16062ae4a62127a Mon Sep 17 00:00:00 2001 From: AAfghahi <48933336+AAfghahi@users.noreply.github.com> Date: Tue, 24 Aug 2021 18:09:55 -0400 Subject: [PATCH 08/11] Update superset/reports/dao.py Co-authored-by: Beto Dealmeida --- superset/reports/dao.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/superset/reports/dao.py b/superset/reports/dao.py index c9ebd112b7d9e..11947a349a650 100644 --- a/superset/reports/dao.py +++ b/superset/reports/dao.py @@ -123,7 +123,7 @@ def validate_unique_creation_method( """ query = db.session.query(ReportSchedule).filter_by(created_by_fk=user_id) - if query is not None and dashboard_id is not None: + if dashboard_id is not None: query = query.filter(ReportSchedule.dashboard_id == dashboard_id) if chart_id is not None: From 2e510f307e4eb54e4f3bcce70d077270b33762d4 Mon Sep 17 00:00:00 2001 From: AAfghahi <48933336+AAfghahi@users.noreply.github.com> Date: Tue, 24 Aug 2021 18:10:02 -0400 Subject: [PATCH 09/11] Update superset/reports/commands/create.py Co-authored-by: Beto Dealmeida --- superset/reports/commands/create.py | 1 + 1 file changed, 1 insertion(+) diff --git a/superset/reports/commands/create.py b/superset/reports/commands/create.py index dcee3f274d2e4..48a1b0fef425c 100644 --- a/superset/reports/commands/create.py +++ b/superset/reports/commands/create.py @@ -64,6 +64,7 @@ def validate(self) -> None: chart_id = self._properties.get("chart") dashboard_id = self._properties.get("dashboard") user_id = self._actor.id + # Validate type is required if not report_type: exceptions.append(ReportScheduleRequiredTypeValidationError()) From 7de1eef8a462b2042a5f7d1eec829d2cec9e2a67 Mon Sep 17 00:00:00 2001 From: AAfghahi <48933336+AAfghahi@users.noreply.github.com> Date: Tue, 24 Aug 2021 18:11:27 -0400 Subject: [PATCH 10/11] Update superset/reports/commands/exceptions.py Co-authored-by: Beto Dealmeida --- superset/reports/commands/exceptions.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/superset/reports/commands/exceptions.py b/superset/reports/commands/exceptions.py index fa1af12d6ce7b..db1273f1721b8 100644 --- a/superset/reports/commands/exceptions.py +++ b/superset/reports/commands/exceptions.py @@ -174,6 +174,8 @@ class ReportScheduleCreationMethodUniquenessValidationError(ValidationError): """ def __init__(self) -> None: + status = 409 + super().__init__( [_("Resource already has an attached report.")], field_name="creation_method", From 148c09cf7c9320130734ca47f407bd86350d848c Mon Sep 17 00:00:00 2001 From: Arash Date: Tue, 24 Aug 2021 18:12:07 -0400 Subject: [PATCH 11/11] revisions --- superset/reports/api.py | 1 - superset/reports/commands/create.py | 4 +- superset/reports/commands/exceptions.py | 16 ++----- tests/integration_tests/reports/api_tests.py | 44 ++++++++++++++++---- 4 files changed, 41 insertions(+), 24 deletions(-) diff --git a/superset/reports/api.py b/superset/reports/api.py index 3a049c024594a..1a5907fba701b 100644 --- a/superset/reports/api.py +++ b/superset/reports/api.py @@ -271,7 +271,6 @@ def delete(self, pk: int) -> Response: @expose("/", methods=["POST"]) @protect() - @safe @statsd_metrics @permission_name("post") def post(self) -> Response: diff --git a/superset/reports/commands/create.py b/superset/reports/commands/create.py index 48a1b0fef425c..f4d4610ee8a20 100644 --- a/superset/reports/commands/create.py +++ b/superset/reports/commands/create.py @@ -64,7 +64,7 @@ def validate(self) -> None: chart_id = self._properties.get("chart") dashboard_id = self._properties.get("dashboard") user_id = self._actor.id - + # Validate type is required if not report_type: exceptions.append(ReportScheduleRequiredTypeValidationError()) @@ -97,7 +97,7 @@ def validate(self) -> None: user_id, dashboard_id, chart_id ) ): - exceptions.append(ReportScheduleCreationMethodUniquenessValidationError()) + raise ReportScheduleCreationMethodUniquenessValidationError() if "validator_config_json" in self._properties: self._properties["validator_config_json"] = json.dumps( diff --git a/superset/reports/commands/exceptions.py b/superset/reports/commands/exceptions.py index db1273f1721b8..27fcd1eef0ea7 100644 --- a/superset/reports/commands/exceptions.py +++ b/superset/reports/commands/exceptions.py @@ -167,19 +167,9 @@ def __init__(self) -> None: super().__init__([_("Name must be unique")], field_name="name") -class ReportScheduleCreationMethodUniquenessValidationError(ValidationError): - """ - Marshmallow validation error for Report Schedule with creation method charts - or dashboards already existing on a report. - """ - - def __init__(self) -> None: - status = 409 - - super().__init__( - [_("Resource already has an attached report.")], - field_name="creation_method", - ) +class ReportScheduleCreationMethodUniquenessValidationError(CommandException): + status = 409 + message = "Resource already has an attached report." class AlertQueryMultipleRowsError(CommandException): diff --git a/tests/integration_tests/reports/api_tests.py b/tests/integration_tests/reports/api_tests.py index cfebb8b19b399..7526d11cdbb38 100644 --- a/tests/integration_tests/reports/api_tests.py +++ b/tests/integration_tests/reports/api_tests.py @@ -829,10 +829,24 @@ def test_create_multiple_creation_method_report_schedule_charts(self): uri = "api/v1/report/" rv = self.client.post(uri, json=report_schedule_data) data = json.loads(rv.data.decode("utf-8")) - assert rv.status_code == 422 - assert data["message"]["creation_method"] == [ - "Resource already has an attached report." - ] + assert rv.status_code == 409 + assert data == { + "errors": [ + { + "message": "Resource already has an attached report.", + "error_type": "GENERIC_COMMAND_ERROR", + "level": "warning", + "extra": { + "issue_codes": [ + { + "code": 1010, + "message": "Issue 1010 - Superset encountered an error while running a command.", + } + ] + }, + } + ] + } @pytest.mark.usefixtures( "load_birth_names_dashboard_with_slices", "create_report_schedules" @@ -873,10 +887,24 @@ def test_create_multiple_creation_method_report_schedule_dashboards(self): uri = "api/v1/report/" rv = self.client.post(uri, json=report_schedule_data) data = json.loads(rv.data.decode("utf-8")) - assert rv.status_code == 422 - assert data["message"]["creation_method"] == [ - "Resource already has an attached report." - ] + assert rv.status_code == 409 + assert data == { + "errors": [ + { + "message": "Resource already has an attached report.", + "error_type": "GENERIC_COMMAND_ERROR", + "level": "warning", + "extra": { + "issue_codes": [ + { + "code": 1010, + "message": "Issue 1010 - Superset encountered an error while running a command.", + } + ] + }, + } + ] + } @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") def test_create_report_schedule_chart_dash_validation(self):