Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: add 4xx error codes where applicable #21627

Merged
merged 1 commit into from
Oct 3, 2022
+31 −15
Merged

chore: add 4xx error codes where applicable #21627

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 16 additions & 2 deletions superset/reports/commands/exceptions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,7 @@ def __init__(self) -> None:


class ReportScheduleInvalidError(CommandInvalidError):
status = 422
message = _("Report Schedule parameters are invalid.")


Expand All @@ -135,6 +136,7 @@ class ReportScheduleUpdateFailedError(CreateFailedError):


class ReportScheduleNotFoundError(CommandException):
status = 404
message = _("Report Schedule not found.")


Expand Down Expand Up @@ -163,10 +165,12 @@ class ReportScheduleExecuteUnexpectedError(CommandException):


class ReportSchedulePreviousWorkingError(CommandException):
status = 429
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: idk if this is a too many request fit here but i also don't what other code fits

Copy link
Member Author

@eschutho eschutho Sep 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I agree. I wrote the same thing in the summary. I feel like ideally this wouldn't be an error.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this will ever generate an HTTP response, this is only used on execute.py that always run on the workers

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @dpgaspar. The goal is to help log the user errors vs system errors more granularly. Right now they are all getting logged as one event. Do you think that the status codes is a viable approach for logging purposes?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

error_type seems more appropriate to me, similar to:

superset/superset/exceptions.py

Line 118 in 383313b

class SupersetSyntaxErrorException(SupersetErrorsException):

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @dpgaspar! @john-bodley, @betodealmeida and I were talking about error_types as well and concluded that errors should not be raised, only exceptions, and that we can include errors with an exception, but not raise them. Do you have thoughts on that @dpgaspar?

message = _("Report Schedule is still working, refusing to re-compute.")


class ReportScheduleWorkingTimeoutError(CommandException):
status = 408
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here, please recheck

message = _("Report Schedule reached a working timeout.")


Expand All @@ -188,20 +192,22 @@ class ReportScheduleCreationMethodUniquenessValidationError(CommandException):


class AlertQueryMultipleRowsError(CommandException):

status = 422
message = _("Alert query returned more than one row.")


class AlertValidatorConfigError(CommandException):

status = 422
message = _("Alert validator config error.")


class AlertQueryMultipleColumnsError(CommandException):
status = 422
message = _("Alert query returned more than one column.")


class AlertQueryInvalidTypeError(CommandException):
status = 422
message = _("Alert query returned a non-number value.")


Expand All @@ -210,30 +216,37 @@ class AlertQueryError(CommandException):


class AlertQueryTimeout(CommandException):
status = 408
message = _("A timeout occurred while executing the query.")


class ReportScheduleScreenshotTimeout(CommandException):
status = 408
message = _("A timeout occurred while taking a screenshot.")


class ReportScheduleCsvTimeout(CommandException):
status = 408
message = _("A timeout occurred while generating a csv.")


class ReportScheduleDataFrameTimeout(CommandException):
status = 408
message = _("A timeout occurred while generating a dataframe.")


class ReportScheduleAlertGracePeriodError(CommandException):
status = 429
message = _("Alert fired during grace period.")


class ReportScheduleAlertEndGracePeriodError(CommandException):
status = 429
message = _("Alert ended grace period.")


class ReportScheduleNotificationError(CommandException):
status = 429
message = _("Alert on grace period")


Expand All @@ -250,6 +263,7 @@ class ReportScheduleUnexpectedError(CommandException):


class ReportScheduleForbiddenError(ForbiddenError):
status = 403
message = _("Changing this report is forbidden")


Expand Down
23 changes: 12 additions & 11 deletions superset/reports/commands/execute.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@
ReportScheduleDataFrameTimeout,
ReportScheduleExecuteUnexpectedError,
ReportScheduleNotFoundError,
ReportScheduleNotificationError,
ReportSchedulePreviousWorkingError,
ReportScheduleScreenshotFailedError,
ReportScheduleScreenshotTimeout,
Expand Down Expand Up @@ -399,9 +398,9 @@ def _send(
"""
Sends a notification to all recipients

:raises: ReportScheduleNotificationError
:raises: NotificationError
"""
notification_errors = []
notification_errors: List[NotificationError] = []
for recipient in recipients:
notification = create_notification(recipient, notification_content)
try:
Expand All @@ -415,15 +414,17 @@ def _send(
notification.send()
except NotificationError as ex:
# collect notification errors but keep processing them
notification_errors.append(str(ex))
notification_errors.append(ex)
if notification_errors:
raise ReportScheduleNotificationError(";".join(notification_errors))
# raise errors separately so that we can utilize error status codes
for error in notification_errors:
raise error

def send(self) -> None:
"""
Creates the notification content and sends them to all recipients

:raises: ReportScheduleNotificationError
:raises: NotificationError
"""
notification_content = self._get_notification_content()
self._send(notification_content, self._report_schedule.recipients)
Expand All @@ -432,7 +433,7 @@ def send_error(self, name: str, message: str) -> None:
"""
Creates and sends a notification for an error, to all recipients

:raises: ReportScheduleNotificationError
:raises: NotificationError
"""
header_data = self._get_log_data()
header_data["error_text"] = message
Expand Down Expand Up @@ -526,7 +527,7 @@ def next(self) -> None:
return
self.send()
self.update_report_schedule_and_log(ReportState.SUCCESS)
except CommandException as first_ex:
except Exception as first_ex:
Copy link
Member

@pkdotson pkdotson Oct 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you changing these to more general exceptions because the CommandException does not return the full trace?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rather because all of the exceptions here may not inherit from CommandException since I'm raising them all individually.

self.update_report_schedule_and_log(
ReportState.ERROR, error_message=str(first_ex)
)
Expand All @@ -542,7 +543,7 @@ def next(self) -> None:
ReportState.ERROR,
error_message=REPORT_SCHEDULE_ERROR_NOTIFICATION_MARKER,
)
except CommandException as second_ex:
except Exception as second_ex: # pylint: disable=broad-except
self.update_report_schedule_and_log(
ReportState.ERROR, error_message=str(second_ex)
)
Expand Down Expand Up @@ -599,7 +600,7 @@ def next(self) -> None:
if not AlertCommand(self._report_schedule).run():
self.update_report_schedule_and_log(ReportState.NOOP)
return
except CommandException as ex:
except Exception as ex:
self.send_error(
f"Error occurred for {self._report_schedule.type}:"
f" {self._report_schedule.name}",
Expand All @@ -614,7 +615,7 @@ def next(self) -> None:
try:
self.send()
self.update_report_schedule_and_log(ReportState.SUCCESS)
except CommandException as ex:
except Exception as ex: # pylint: disable=broad-except
self.update_report_schedule_and_log(
ReportState.ERROR, error_message=str(ex)
)
Expand Down
5 changes: 3 additions & 2 deletions tests/integration_tests/reports/commands_tests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,10 +39,10 @@
ReportScheduleCsvFailedError,
ReportScheduleCsvTimeout,
ReportScheduleNotFoundError,
ReportScheduleNotificationError,
ReportSchedulePreviousWorkingError,
ReportScheduleScreenshotFailedError,
ReportScheduleScreenshotTimeout,
ReportScheduleUnexpectedError,
ReportScheduleWorkingTimeoutError,
)
from superset.reports.commands.execute import AsyncExecuteReportScheduleCommand
Expand Down Expand Up @@ -113,6 +113,7 @@ def assert_log(state: str, error_message: Optional[str] = None):

if state == ReportState.ERROR:
# On error we send an email
print(logs)
assert len(logs) == 3
else:
assert len(logs) == 2
Expand Down Expand Up @@ -1321,7 +1322,7 @@ def test_email_dashboard_report_fails(
screenshot_mock.return_value = SCREENSHOT_FILE
email_mock.side_effect = SMTPException("Could not connect to SMTP XPTO")

with pytest.raises(ReportScheduleNotificationError):
with pytest.raises(ReportScheduleUnexpectedError):
AsyncExecuteReportScheduleCommand(
TEST_ID, create_report_email_dashboard.id, datetime.utcnow()
).run()
Expand Down