EnxDev commented on code in PR #44082: URL: https://github.com/apache/superset/pull/44082#discussion_r4045114225
########## superset/dashboards/excel_export/sync_budget.py: ########## @@ -0,0 +1,107 @@ +# 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. +"""Plan and size dashboard Excel exports served in the HTTP response.""" + +from __future__ import annotations + +import logging +from dataclasses import dataclass +from typing import Any + +from celery.exceptions import SoftTimeLimitExceeded +from flask import current_app + +from superset.dashboards.excel_export.layout import get_charts_in_layout_order +from superset.dashboards.excel_export.workbook import ( + resolve_query_context, + ResolvedQueryContexts, +) + +logger = logging.getLogger(__name__) + + +@dataclass(frozen=True) +class InlineExportPlan: + """Queries planned for a direct download and their row budget.""" + + #: Resolved query contexts by chart id. ``None`` marks a skipped chart. + query_contexts: ResolvedQueryContexts + #: Combined row limit, or ``None`` when any query has no finite limit. + requested_rows: int | None + #: Configured limit for direct downloads. + max_rows: int + + @property + def fits_row_budget(self) -> bool: + """Return whether the export can run during the request.""" + return self.requested_rows is not None and self.requested_rows <= self.max_rows + + +def _finite_row_limit(query: Any) -> int | None: + """Return a safe upper bound for one query's result rows.""" + if not isinstance(query, dict): + return None + # Grouping sets do not apply row_limit and may fan out into several queries. + if query.get("grouping_sets"): + return None + columns = query.get("columns") + metrics = query.get("metrics") + if ( + columns == [] + and isinstance(metrics, list) + and metrics + and not query.get("is_timeseries") + ): + # A metric query with no grouping columns returns one aggregate row. + return 1 Review Comment: Good catch — the shortcut assumed every metric aggregates, which Custom SQL does not have to. With `columns: []` the builder emits no GROUP BY (the non-aggregate guard in `get_sqla_query` only fires under `groupby_all_columns`), so `SELECT amount ... LIMIT 50000` really does return 50,000 rows for a query I was budgeting as one. The shortcut now requires proof: a query counts as one row only when it groups nothing, is not a timeseries, and *every* metric provably collapses rows — a `SIMPLE` adhoc metric (the column wrapped in its aggregate), or Custom SQL / a saved dataset metric whose expression parses to an aggregate in the dataset's own dialect. Anything unprovable — non-aggregate SQL, a templated expression, an unmodelled function, a metric missing from the dataset, an unreadable dataset — keeps its effective row limit. To keep proving the KPI case rather than regressing it, saved metric names are resolved against the chart's dataset, so `["count"]` is still worth one row. `has_aggregate` grew a `fail_open` parameter for this: its existing caller rejects non-aggregates and must not block what it cannot parse, while sizing a query must not grant the one-row discount on a guess. Unit tests cover each metric form on both sides. Fixed in 8db370e6b5. ########## superset/dashboards/api.py: ########## @@ -1797,20 +1811,29 @@ def export_xlsx(self, pk: int) -> WerkzeugResponse: except SupersetSecurityException: return self.response_403() - # Email delivery is the only result channel, so an account with an email - # address is required; embedded guest users are excluded in this version. + # Both delivery paths require a non-guest account with an email address. if isinstance(g.user, GuestUser) or not getattr(g.user, "email", None): return self.response_400( message="Excel export requires an account with an email address." ) if not dashboard.slices: return self.response_400(message="Dashboard has no charts to export.") - # Throttle: one concurrent export per user+dashboard. Acquire a shared, - # atomic distributed lock (Redis when configured, the metadata DB - # otherwise) so the guard works across the web server and workers and is - # not a no-op under the default cache. The task releases it when it - # settles; the TTL is the backstop if that release is ever lost. + active_data_mask = payload.get("active_data_mask", {}) + mode = payload.get("mode", "data") + + if not queued and mode == EXPORT_MODE_IMAGES: + # Webdriver rendering is too slow and unbounded for a web request. + return self.response_400( + message=( + "Exporting images to Excel runs in the background. " + "Configure EXCEL_EXPORT_S3_BUCKET to use it, or export " + "the dashboard's data instead." + ) + ) + + # Allow one export per user and dashboard across web and worker processes. + # The TTL releases the lock if normal cleanup fails. lock_params = export_lock_params(g.user.id, dashboard.id) try: AcquireDistributedLock( Review Comment: Valid. `AcquireDistributedLock` mints a per-acquisition token precisely so release can compare-and-delete, and discarding it meant an export that outran `EXPORT_LOCK_TTL_SECONDS` would delete whatever lock was held when it finished — including a newer request's. The acquisition is now kept and its token passed to every release: the planning-failure path, the enqueue-failure path, the inline `finally`, and the Celery task (threaded through the task kwargs, defaulting to `None` so tasks enqueued before this change still release as they did). This matches the existing pattern in `superset/tasks/locks.py`. Fixed in 8db370e6b5. -- 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]
