EnxDev commented on code in PR #44082:
URL: https://github.com/apache/superset/pull/44082#discussion_r4110664924
##########
superset/dashboards/api.py:
##########
@@ -1854,14 +1874,10 @@ def export_xlsx(self, pk: int) -> WerkzeugResponse:
$ref: '#/components/responses/404'
500:
$ref: '#/components/responses/500'
- 501:
- description: Excel export is not configured on this server
"""
- storage_config = current_app.config["EXPORT_STORAGE"]
- if not storage_config.get("bucket") or storage_config.get("backend")
is None:
- return self.response(
- 501, message="Excel export is not configured on this server."
- )
+ # Keep the request guards and two delivery paths together.
+ # Resolve the path once so its checks and delivery cannot diverge.
+ queued = is_export_storage_configured()
Review Comment:
Good catch. With `CELERY_CONFIG = None` a complete storage config still
queued the export on a broker nobody runs. In 9d482b7 the queue decision goes
through `is_background_export_available()`, which also requires
`CELERY_CONFIG`. When Celery is off, the export falls back to the direct
download and logs one warning per process. The bootstrap flag uses the same
check, so image export stays hidden in that setup too. Unit tests in
`test_excel_export_storage.py` cover it, plus
`test_export_xlsx_without_celery_downloads_directly` on the API.
I didn't add a live worker check. `control.ping()` costs a broker round trip
and a timeout on every export and every page load (the flag is in the bootstrap
payload), and a reachable broker still doesn't prove a worker is consuming. SQL
Lab async, thumbnails and reports make the same assumption. If a job is never
picked up, the UI's polling deadline reports that the export is taking longer
than expected. The docs and UPDATING now say so.
##########
superset-frontend/src/dashboard/components/menu/DownloadMenuItems/index.tsx:
##########
@@ -348,19 +360,26 @@ export const useDownloadMenuItems = (
addInfoToast(t('An export for this dashboard is already in
progress.'));
}
} catch (error) {
- // status comes from the response (Partial<SupersetClientResponse>),
which
- // the union type does not expose uniformly; read it via a narrow cast.
- const { status } = (await getClientErrorObject(error)) as {
+ // The client error union does not expose response fields uniformly.
+ const { status, message } = (await getClientErrorObject(error)) as {
status?: number;
+ message?: string;
};
if (unmountedRef.current) {
return;
}
- if (status === 501) {
- addDangerToast(t('Excel export is not configured on this server.'));
+ // Show actionable client errors; keep server errors generic.
+ if (message && status && status >= 400 && status < 500) {
Review Comment:
Applied in 9d482b7. The toast only uses `message` when it is a string, so a
field-error dict from the schema `ValidationError` falls through to the generic
toast. I also typed `message` as `unknown` so the cast no longer claims it is a
string. The new test `Export Data to Excel keeps a field-error 400 generic`
covers it.
--
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]
