This is an automated email from the ASF dual-hosted git repository. jli pushed a commit to branch fix-report-put-database-500 in repository https://gitbox.apache.org/repos/asf/superset.git
commit c78624b20ec9cd760d788ebd96f4b4fa3fa5a546 Author: Joe Li <[email protected]> AuthorDate: Wed Feb 18 17:27:16 2026 -0800 fix(reports): validate database field on PUT report schedule PUT /api/v1/report/{id} with {"database": N} on a Report-type schedule returned 500 instead of a validation error. The POST schema had this check but the PUT path lacked it. - Add ReportScheduleDatabaseNotAllowedValidationError exception - Add command-level validation in UpdateReportScheduleCommand enforcing both invariants: Reports must not have a database, Alerts must have one - Add allow_none=True on ReportSchedulePutSchema.database to allow clearing via PUT (consistent with dashboard field) - Use explicit null check (database_id is not None) for DB existence validation to handle edge cases Co-Authored-By: Claude Opus 4.6 <[email protected]> --- superset/commands/report/exceptions.py | 12 ++ superset/commands/report/update.py | 22 +++- superset/reports/schemas.py | 2 +- tests/integration_tests/reports/api_tests.py | 167 ++++++++++++++++++++++++++- 4 files changed, 199 insertions(+), 4 deletions(-) diff --git a/superset/commands/report/exceptions.py b/superset/commands/report/exceptions.py index 27966e6f09e..b51c2d1cd1f 100644 --- a/superset/commands/report/exceptions.py +++ b/superset/commands/report/exceptions.py @@ -39,6 +39,18 @@ class DatabaseNotFoundValidationError(ValidationError): super().__init__(_("Database does not exist"), field_name="database") +class ReportScheduleDatabaseNotAllowedValidationError(ValidationError): + """ + Marshmallow validation error for database reference on a Report type schedule + """ + + def __init__(self) -> None: + super().__init__( + _("Database reference is not allowed on a report"), + field_name="database", + ) + + class DashboardNotFoundValidationError(ValidationError): """ Marshmallow validation error for dashboard does not exist diff --git a/superset/commands/report/update.py b/superset/commands/report/update.py index abae62cadd2..3bf51ee8f36 100644 --- a/superset/commands/report/update.py +++ b/superset/commands/report/update.py @@ -26,6 +26,8 @@ from superset.commands.base import UpdateMixin from superset.commands.report.base import BaseReportScheduleCommand from superset.commands.report.exceptions import ( DatabaseNotFoundValidationError, + ReportScheduleAlertRequiredDatabaseValidationError, + ReportScheduleDatabaseNotAllowedValidationError, ReportScheduleForbiddenError, ReportScheduleInvalidError, ReportScheduleNameUniquenessValidationError, @@ -98,8 +100,26 @@ class UpdateReportScheduleCommand(UpdateMixin, BaseReportScheduleCommand): ) ) + # Validate database is not allowed on Report type + if report_type == ReportScheduleType.REPORT: + if "database" in self._properties: + has_database = self._properties["database"] is not None + else: + has_database = self._model.database_id is not None + if has_database: + exceptions.append(ReportScheduleDatabaseNotAllowedValidationError()) + + # Validate Alert has a database + if report_type == ReportScheduleType.ALERT: + if "database" in self._properties: + has_database = self._properties["database"] is not None + else: + has_database = self._model.database_id is not None + if not has_database: + exceptions.append(ReportScheduleAlertRequiredDatabaseValidationError()) + # Validate if DB exists (for alerts) - if report_type == ReportScheduleType.ALERT and database_id: + if report_type == ReportScheduleType.ALERT and database_id is not None: if not (database := DatabaseDAO.find_by_id(database_id)): exceptions.append(DatabaseNotFoundValidationError()) self._properties["database"] = database diff --git a/superset/reports/schemas.py b/superset/reports/schemas.py index 76108f976ff..055ebc75d24 100644 --- a/superset/reports/schemas.py +++ b/superset/reports/schemas.py @@ -334,7 +334,7 @@ class ReportSchedulePutSchema(Schema): metadata={"description": creation_method_description}, ) dashboard = fields.Integer(required=False, allow_none=True) - database = fields.Integer(required=False) + database = fields.Integer(required=False, allow_none=True) owners = fields.List( fields.Integer(metadata={"description": owners_description}), required=False ) diff --git a/tests/integration_tests/reports/api_tests.py b/tests/integration_tests/reports/api_tests.py index 41edf717020..9a7b5cf7b20 100644 --- a/tests/integration_tests/reports/api_tests.py +++ b/tests/integration_tests/reports/api_tests.py @@ -18,6 +18,7 @@ """Unit tests for Superset""" from datetime import datetime, timedelta +from typing import Any from unittest.mock import patch import pytz @@ -1392,7 +1393,7 @@ class TestReportSchedulesApi(SupersetTestCase): ) assert report_schedule.type == ReportScheduleType.ALERT previous_cron = report_schedule.crontab - update_payload = { + update_payload: dict[str, Any] = { "crontab": "5,10 * * * *", } with patch.dict( @@ -1410,6 +1411,7 @@ class TestReportSchedulesApi(SupersetTestCase): # Test report minimum interval update_payload["crontab"] = "5,8 * * * *" update_payload["type"] = ReportScheduleType.REPORT + update_payload["database"] = None uri = f"api/v1/report/{report_schedule.id}" rv = self.put_assert_metric(uri, update_payload, "put") assert rv.status_code == 200 @@ -1424,6 +1426,7 @@ class TestReportSchedulesApi(SupersetTestCase): # Undo changes update_payload["crontab"] = previous_cron update_payload["type"] = ReportScheduleType.ALERT + update_payload["database"] = get_example_database().id uri = f"api/v1/report/{report_schedule.id}" rv = self.put_assert_metric(uri, update_payload, "put") assert rv.status_code == 200 @@ -1441,7 +1444,7 @@ class TestReportSchedulesApi(SupersetTestCase): .one_or_none() ) assert report_schedule.type == ReportScheduleType.ALERT - update_payload = { + update_payload: dict[str, Any] = { "crontab": "5,10 * * * *", } with patch.dict( @@ -1468,6 +1471,7 @@ class TestReportSchedulesApi(SupersetTestCase): # Exceed report minimum interval update_payload["crontab"] = "5,8 * * * *" update_payload["type"] = ReportScheduleType.REPORT + update_payload["database"] = None uri = f"api/v1/report/{report_schedule.id}" rv = self.put_assert_metric(uri, update_payload, "put") assert rv.status_code == 422 @@ -1607,6 +1611,165 @@ class TestReportSchedulesApi(SupersetTestCase): data = json.loads(rv.data.decode("utf-8")) assert data == {"message": {"chart": "Choose a chart or dashboard not both"}} + @pytest.mark.usefixtures("create_report_schedules") + def test_update_report_schedule_database_not_allowed_on_report(self): + """ + ReportSchedule API: Test update report schedule rejects database on Report type + """ + self.login(ADMIN_USERNAME) + example_db = get_example_database() + + # Create a Report-type schedule (name1 is an Alert, so create one) + report_schedule = ( + db.session.query(ReportSchedule) + .filter(ReportSchedule.name == "name1") + .one_or_none() + ) + # Change to Report type first (clearing database) + uri = f"api/v1/report/{report_schedule.id}" + rv = self.put_assert_metric( + uri, + {"type": ReportScheduleType.REPORT, "database": None}, + "put", + ) + assert rv.status_code == 200 + + # Test 1: Report + database (no type in payload) → 422 + rv = self.put_assert_metric(uri, {"database": example_db.id}, "put") + assert rv.status_code == 422 + data = json.loads(rv.data.decode("utf-8")) + assert data == { + "message": {"database": "Database reference is not allowed on a report"} + } + + # Test 2: Report + database + explicit type=Report → 422 + rv = self.put_assert_metric( + uri, + {"type": ReportScheduleType.REPORT, "database": example_db.id}, + "put", + ) + assert rv.status_code == 422 + data = json.loads(rv.data.decode("utf-8")) + assert data == { + "message": {"database": "Database reference is not allowed on a report"} + } + + @pytest.mark.usefixtures("create_report_schedules") + def test_update_alert_schedule_database_allowed(self): + """ + ReportSchedule API: Test update alert schedule accepts database + """ + self.login(ADMIN_USERNAME) + example_db = get_example_database() + + report_schedule = ( + db.session.query(ReportSchedule) + .filter(ReportSchedule.name == "name2") + .one_or_none() + ) + assert report_schedule.type == ReportScheduleType.ALERT + + # Test 3: Alert + database (no type in payload) → 200 + uri = f"api/v1/report/{report_schedule.id}" + rv = self.put_assert_metric(uri, {"database": example_db.id}, "put") + assert rv.status_code == 200 + + @pytest.mark.usefixtures("create_report_schedules") + def test_update_report_schedule_type_transitions(self): + """ + ReportSchedule API: Test type transitions with database validation + """ + self.login(ADMIN_USERNAME) + example_db = get_example_database() + + report_schedule = ( + db.session.query(ReportSchedule) + .filter(ReportSchedule.name == "name3") + .one_or_none() + ) + assert report_schedule.type == ReportScheduleType.ALERT + assert report_schedule.database_id is not None + uri = f"api/v1/report/{report_schedule.id}" + + # Test 4: Alert + database update (same type) → 200 + rv = self.put_assert_metric( + uri, + {"database": example_db.id}, + "put", + ) + assert rv.status_code == 200 + + # Test 5: Alert → Report + database → 422 + rv = self.put_assert_metric( + uri, + { + "type": ReportScheduleType.REPORT, + "database": example_db.id, + }, + "put", + ) + assert rv.status_code == 422 + data = json.loads(rv.data.decode("utf-8")) + assert data == { + "message": {"database": "Database reference is not allowed on a report"} + } + + # Test 6: Alert → Report without clearing database → 422 + rv = self.put_assert_metric(uri, {"type": ReportScheduleType.REPORT}, "put") + assert rv.status_code == 422 + data = json.loads(rv.data.decode("utf-8")) + assert data == { + "message": {"database": "Database reference is not allowed on a report"} + } + + # Test 7: Alert → Report with database: null (explicit clear) → 200 + rv = self.put_assert_metric( + uri, + {"type": ReportScheduleType.REPORT, "database": None}, + "put", + ) + assert rv.status_code == 200 + + # Now schedule is a Report with no database. + # Test 8: Report → Alert without providing database → 422 + rv = self.put_assert_metric( + uri, + {"type": ReportScheduleType.ALERT}, + "put", + ) + assert rv.status_code == 422 + data = json.loads(rv.data.decode("utf-8")) + assert data == {"message": {"database": "Database is required for alerts"}} + + # Test 9: Report → Alert with database → 200 (valid transition) + rv = self.put_assert_metric( + uri, + {"type": ReportScheduleType.ALERT, "database": example_db.id}, + "put", + ) + assert rv.status_code == 200 + + @pytest.mark.usefixtures("create_report_schedules") + def test_update_alert_schedule_database_null_rejected(self): + """ + ReportSchedule API: Test alert schedule rejects null database + """ + self.login(ADMIN_USERNAME) + + report_schedule = ( + db.session.query(ReportSchedule) + .filter(ReportSchedule.name == "name2") + .one_or_none() + ) + assert report_schedule.type == ReportScheduleType.ALERT + uri = f"api/v1/report/{report_schedule.id}" + + # Test 8: Alert + database: null → 422 + rv = self.put_assert_metric(uri, {"database": None}, "put") + assert rv.status_code == 422 + data = json.loads(rv.data.decode("utf-8")) + assert data == {"message": {"database": "Database is required for alerts"}} + @pytest.mark.usefixtures( "load_birth_names_dashboard_with_slices", "create_report_schedules" )
