bito-code-review[bot] commented on code in PR #42481: URL: https://github.com/apache/superset/pull/42481#discussion_r3676185433
########## superset/migrations/versions/2026-07-24_00-00_f3a8c1d2e9b7_add_report_retry_state_columns.py: ########## @@ -0,0 +1,84 @@ +# 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_report_retry_columns + +Revision ID: f3a8c1d2e9b7 +Revises: e5f6a7b8c9d0 +Create Date: 2026-07-24 00:00:00.000000 + +""" + +import logging + +import sqlalchemy as sa +from alembic import op + +from superset.migrations.shared.utils import get_table_column + +logger = logging.getLogger("alembic.env") + +# revision identifiers, used by Alembic. +revision = "f3a8c1d2e9b7" +down_revision = "d3b9a1f6c204" Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Inconsistent Alembic revision chain</b></div> <div id="fix"> The `down_revision` points to `d3b9a1f6c204` but `Revises` states `e5f6a7b8c9d0`. Both migrations exist and both Revises `e5f6a7b8c9d0`, creating a branching tree rather than a linear chain. Alembic will fail with 'Multiple head migrations are detected' when running migrations. Please reconcile the dependency order. </div> </div> <small><i>Code Review Run #d51984</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them ########## tests/integration_tests/reports/commands_tests.py: ########## @@ -2667,3 +2667,428 @@ def test__send_with_server_errors(notification_mock, logger_mock): logger_mock.warning.assert_called_with( "SupersetError(message='', error_type=<SupersetErrorType.REPORT_NOTIFICATION_ERROR: 'REPORT_NOTIFICATION_ERROR'>, level=<ErrorLevel.ERROR: 'error'>, extra=None)" # noqa: E501 ) + + +# --------------------------------------------------------------------------- +# Retry tests +# --------------------------------------------------------------------------- + + [email protected]("load_birth_names_dashboard_with_slices") +@patch("superset.commands.report.execute.ReportNotTriggeredErrorState._schedule_retry") +@patch("superset.reports.notifications.email.send_email_smtp") +@patch("superset.utils.screenshots.ChartScreenshot.get_screenshot") +def test_retry_on_failure_schedules_retry( + screenshot_mock: Mock, + email_mock: Mock, + schedule_retry_mock: Mock, +) -> None: + """ + ExecuteReport Command: when retry_on_failure is enabled and the report fails, + the state transitions to RETRYING and a retry task is enqueued with the + correct exponential-backoff delay. + """ + chart = db.session.query(Slice).first() + report_schedule = create_report_notification( + email_target="[email protected]", + chart=chart, + retry_on_failure=True, + retry_max_attempts=3, + retry_notify_owners=False, + retry_notify_recipients=False, + ) + try: + screenshot_mock.side_effect = Exception("screenshot failed") + + # Should NOT re-raise (retry path exits cleanly) + AsyncExecuteReportScheduleCommand( + TEST_ID, report_schedule.id, datetime.utcnow() + ).run() + + db.session.refresh(report_schedule) + assert report_schedule.last_state == ReportState.RETRYING + assert report_schedule.retry_attempt == 1 + # Verify delay: base=60, attempt=1 → min(60 * 2^1, 3600) = 120 + schedule_retry_mock.assert_called_once_with(120) + # No error email should be sent on the first failure (notification is + # sent after a *retry* fails, not after the original failure) + email_mock.assert_not_called() + finally: + cleanup_report_schedule(report_schedule) + + [email protected]("load_birth_names_dashboard_with_slices") +@patch("superset.commands.report.execute.ReportNotTriggeredErrorState._schedule_retry") +@patch("superset.commands.report.execute.BaseReportState.send_retry_notification") +@patch("superset.utils.screenshots.ChartScreenshot.get_screenshot") +def test_retry_exhausted_transitions_to_error( + screenshot_mock: Mock, + retry_notification_mock: Mock, + schedule_retry_mock: Mock, +) -> None: + """ + ExecuteReport Command: when all retries are exhausted the state transitions + to ERROR, the retry counter is reset, and the retry notification is sent + for the final attempt. + """ + chart = db.session.query(Slice).first() + report_schedule = create_report_notification( + email_target="[email protected]", + chart=chart, + retry_on_failure=True, + retry_max_attempts=2, + retry_notify_owners=False, + retry_notify_recipients=False, + ) + # Pre-set retry_attempt to the max so the next execution exhausts retries. + # Use the same timestamp for both so _is_retry_window_stale() returns False. + # Truncate microseconds — MySQL DateTime columns drop them, which would make + # the round-tripped value differ from the in-memory one. + scheduled_dttm = datetime.now(tz=timezone.utc).replace(tzinfo=None, microsecond=0) + report_schedule.retry_attempt = 2 + report_schedule.retry_scheduled_dttm = scheduled_dttm + db.session.commit() + + try: + screenshot_mock.side_effect = Exception("screenshot failed") + + with pytest.raises(Exception, match="screenshot failed"): + AsyncExecuteReportScheduleCommand( + TEST_ID, report_schedule.id, scheduled_dttm + ).run() + + db.session.refresh(report_schedule) + assert report_schedule.last_state == ReportState.ERROR + # Counter is reset after exhaustion + assert report_schedule.retry_attempt == 0 + # No further retry should have been scheduled + schedule_retry_mock.assert_not_called() + # Retry notification sent for the exhausted attempt (attempt 2 of 2) + # The error message is wrapped by the screenshot layer, so use ANY. + retry_notification_mock.assert_called_once_with(2, 2, ANY) + finally: + cleanup_report_schedule(report_schedule) + + [email protected]("load_birth_names_dashboard_with_slices") +@patch("superset.commands.report.execute.ReportNotTriggeredErrorState._schedule_retry") +@patch("superset.commands.report.execute.BaseReportState.send_final_failure_report") +@patch("superset.utils.screenshots.ChartScreenshot.get_screenshot") +def test_send_failed_reports_sends_to_recipients( + screenshot_mock: Mock, + final_failure_mock: Mock, + schedule_retry_mock: Mock, +) -> None: + """ + ExecuteReport Command: when send_failed_reports is True and all retries are + exhausted, send_final_failure_report is called with the error message. + """ + chart = db.session.query(Slice).first() + report_schedule = create_report_notification( + email_target="[email protected]", + chart=chart, + retry_on_failure=True, + retry_max_attempts=1, + send_failed_reports=True, + retry_notify_owners=False, + retry_notify_recipients=False, + ) + scheduled_dttm = datetime.now(tz=timezone.utc).replace(tzinfo=None, microsecond=0) + report_schedule.retry_attempt = 1 + report_schedule.retry_scheduled_dttm = scheduled_dttm + db.session.commit() + + try: + screenshot_mock.side_effect = Exception("screenshot failed") + + with pytest.raises(Exception, match="screenshot failed"): + AsyncExecuteReportScheduleCommand( + TEST_ID, report_schedule.id, scheduled_dttm + ).run() + + # send_final_failure_report should have been called + final_failure_mock.assert_called_once_with(ANY) + schedule_retry_mock.assert_not_called() + finally: + cleanup_report_schedule(report_schedule) + + [email protected]("load_birth_names_dashboard_with_slices") +@patch("superset.commands.report.execute.ReportNotTriggeredErrorState._schedule_retry") +@patch("superset.utils.screenshots.ChartScreenshot.get_screenshot") +def test_retrying_state_schedules_another_retry( + screenshot_mock: Mock, + schedule_retry_mock: Mock, +) -> None: + """ + ExecuteReport Command: a schedule with last_state=RETRYING is routed to + ReportNotTriggeredErrorState, which increments the counter and schedules + another retry with the correct delay. + """ + chart = db.session.query(Slice).first() Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Duplicate test setup code blocks</b></div> <div id="fix"> Test setup code is duplicated in tests/integration_tests/reports/commands_tests.py at lines 2828-2839 and 2995-3006. Create a shared fixture or helper function for this common test initialization pattern to eliminate duplication. </div> </div> <small><i>Code Review Run #d51984</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them ########## superset/reports/schemas.py: ########## @@ -278,6 +278,35 @@ class ReportSchedulePostSchema(Schema): required=False, dump_default=None, ) + retry_on_failure = fields.Boolean( + metadata={"description": _("Enable automatic retries on report failure")}, + load_default=False, + ) + retry_max_attempts = fields.Integer( + metadata={ + "description": _("Maximum number of retry attempts (1–10)"), + "example": 3, + }, + load_default=3, + required=False, + validate=[Range(min=1, max=10, error=_("Must be between 1 and 10"))], + ) + send_failed_reports = fields.Boolean( + metadata={ + "description": _( + "Send the failed report to all recipients after retries are exhausted" + ) + }, + load_default=False, + ) + retry_notify_owners = fields.Boolean( + metadata={"description": _("Notify report owners on each retry attempt")}, + load_default=True, + ) Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Missing unit tests for retry config fields</b></div> <div id="fix"> The diff adds 5 new retry fields (`retry_on_failure`, `retry_max_attempts`, `send_failed_reports`, `retry_notify_owners`, `retry_notify_recipients`) to both schemas, but `schemas_test.py` contains zero tests for any of them. New schema fields require test coverage to prevent regression. </div> </div> <small><i>Code Review Run #d51984</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them ########## superset/reports/schemas.py: ########## @@ -278,6 +278,35 @@ class ReportSchedulePostSchema(Schema): required=False, dump_default=None, ) + retry_on_failure = fields.Boolean( + metadata={"description": _("Enable automatic retries on report failure")}, + load_default=False, + ) + retry_max_attempts = fields.Integer( + metadata={ + "description": _("Maximum number of retry attempts (1–10)"), + "example": 3, + }, + load_default=3, + required=False, + validate=[Range(min=1, max=10, error=_("Must be between 1 and 10"))], + ) + send_failed_reports = fields.Boolean( + metadata={ + "description": _( + "Send the failed report to all recipients after retries are exhausted" + ) + }, + load_default=False, + ) + retry_notify_owners = fields.Boolean( + metadata={"description": _("Notify report owners on each retry attempt")}, + load_default=True, + ) + retry_notify_recipients = fields.Boolean( + metadata={"description": _("Notify report recipients on each retry attempt")}, + load_default=False, Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Duplicate validation method in schemas</b></div> <div id="fix"> The custom_width validation method is duplicated in superset/reports/schemas.py at lines 308-329 and 514-535. Consider extracting the validation logic into a shared helper method to improve maintainability and reduce code duplication. </div> </div> <small><i>Code Review Run #d51984</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
