codeant-ai-for-open-source[bot] commented on code in PR #41133:
URL: https://github.com/apache/superset/pull/41133#discussion_r3429878766
##########
superset-frontend/src/dashboard/components/menu/DownloadMenuItems/index.tsx:
##########
@@ -68,8 +71,19 @@ export const useDownloadMenuItems = (
} = props;
const { addDangerToast, addSuccessToast } = useToasts();
+ const dataMask = useSelector((state: RootState) => state.dataMask);
const SCREENSHOT_NODE_SELECTOR = '.dashboard';
+ const buildActiveDataMask = (): Record<string, { extraFormData: object }> =>
+ Object.entries(dataMask || {}).reduce<
+ Record<string, { extraFormData: object }>
+ >((acc, [id, mask]) => {
+ if (id.startsWith(NATIVE_FILTER_PREFIX)) {
+ acc[id] = { extraFormData: mask?.extraFormData ?? {} };
+ }
Review Comment:
**Suggestion:** The active mask payload is restricted to IDs starting with
`NATIVE_FILTER_PREFIX`, which creates a client/server contract mismatch for
dashboards whose native filter IDs don't use that prefix (for example
older/imported IDs). In those cases the frontend drops valid active filters, so
backend export falls back to saved defaults instead of the live filter state.
Build the payload from actual native filter IDs in state/config rather than
hard-prefix filtering. [logic error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Excel export ignores some active dashboard native filters.
- ❌ Back-end filter context falls back to saved defaults.
- ⚠️ Users see exported data inconsistent with on-screen charts.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open a dashboard whose native filter IDs match the backend-native config
but do not
start with `NATIVE_FILTER_PREFIX` (for example, a dashboard created or
imported with
custom or legacy filter IDs). In the frontend Redux state at `dataMask` (type
`RootState['dataMask']` from `superset-frontend/src/dashboard/types`),
ensure there is at
least one entry representing a native filter where the `id` key does not
start with
`NATIVE_FILTER_PREFIX` but does correspond to a native filter defined in the
dashboard
metadata.
2. In the running UI, apply that native filter so that its `dataMask` entry
has a
non-empty `extraFormData` (this is how live filter state is captured for
queries and
exports). Confirm via Redux devtools or logging that
`state.dataMask[id].extraFormData` is
populated for the non-prefixed `id`.
3. Trigger the new "Export Data to Excel" action from the download menu,
which calls
`useDownloadMenuItems` in
`superset-frontend/src/dashboard/components/menu/DownloadMenuItems/index.tsx`.
The
`onExportXlsx` handler invokes `buildActiveDataMask`, which currently does:
- Iterates `Object.entries(dataMask || {})`.
- For each `[id, mask]`, only includes it in the payload when
`id.startsWith(NATIVE_FILTER_PREFIX)`, assigning `acc[id] = {
extraFormData:
mask?.extraFormData ?? {} }`.
- As a result, any active native filter whose `id` does not start with
`NATIVE_FILTER_PREFIX` is dropped entirely from `active_data_mask`.
4. On the backend, the Excel export endpoint (implemented in the dashboards
API and Celery
task stack) calls `get_dashboard_filter_context(..., active_data_mask=...)`
in
`superset/charts/data/dashboard_filter_context.py`, which relies on the
filter IDs to
match. Because the non-prefixed native filter is missing from
`active_data_mask`,
`_resolve_filter_extra_form_data` falls back to
`_extract_filter_extra_form_data` and uses
the saved default instead of the current live value. The resulting Excel
export and any
chart-data query executed for the export therefore reflect stale/default
filter values
rather than the active ones the user set.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=b685d8c751684ae38fb26e7dc004337d&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=b685d8c751684ae38fb26e7dc004337d&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/dashboard/components/menu/DownloadMenuItems/index.tsx
**Line:** 81:83
**Comment:**
*Logic Error: The active mask payload is restricted to IDs starting
with `NATIVE_FILTER_PREFIX`, which creates a client/server contract mismatch
for dashboards whose native filter IDs don't use that prefix (for example
older/imported IDs). In those cases the frontend drops valid active filters, so
backend export falls back to saved defaults instead of the live filter state.
Build the payload from actual native filter IDs in state/config rather than
hard-prefix filtering.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41133&comment_hash=0137cec7d17c7b84c95bd3c7e06c6d64bea80d9ff3ab7993b720352d9e7edfc2&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41133&comment_hash=0137cec7d17c7b84c95bd3c7e06c6d64bea80d9ff3ab7993b720352d9e7edfc2&reaction=dislike'>👎</a>
##########
superset/charts/data/dashboard_filter_context.py:
##########
@@ -314,3 +349,50 @@ def get_dashboard_filter_context(
)
return context
+
+
+def apply_extra_form_data_to_query_context_json(
+ json_body: dict[str, Any],
+ extra_form_data: dict[str, Any],
+) -> None:
+ """
+ Merge a dashboard filter context's ``extra_form_data`` into a raw query
+ context JSON body, mutating ``json_body`` in place.
+
+ For each query it appends scoped native filters (flagged ``isExtra``),
+ overrides ``extras`` keys (e.g. ``relative_start``) and regular mappings
+ (e.g. ``time_range``), and exposes the full ``extra_form_data`` so Jinja
+ templating can resolve it in ``get_sqla_query``. Shared by the chart data
+ API (GET) and the dashboard Excel export task.
+
+ :param json_body: The query context JSON body (mutated in place)
+ :param extra_form_data: The merged extra_form_data to apply
+ """
+ if not extra_form_data:
+ return
+
+ efd = extra_form_data
+ extra_filters = efd.get("filters", [])
+
+ for query in json_body.get("queries", []):
+ if extra_filters:
+ existing = query.get("filters") or []
+ query["filters"] = existing + [
+ {**f, "isExtra": True} for f in extra_filters
+ ]
Review Comment:
**Suggestion:** The query-context merge only appends `filters`, but ignores
other appendable dashboard filter payload keys such as `adhoc_filters` (and
interactive keys) even though they are part of merged `extra_form_data`. As a
result, exports/GET chart-data paths that rely on those keys will silently drop
active dashboard filters and return incorrect data. Extend this merge to handle
all `EXTRA_FORM_DATA_APPEND_KEYS` (not just `filters`) when mutating each
query. [incomplete implementation]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Dashboard adhoc filters omitted from Excel export queries.
- ❌ Other appendable extra_form_data keys silently ignored.
- ⚠️ Exported chart data diverges from on-screen filtered results.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure a dashboard-native filter that injects its logic via
`extra_form_data` keys
other than simple `filters`, for example:
- An `adhoc_filters` clause (e.g. a non-time WHERE filter).
- Or another appendable key defined in `EXTRA_FORM_DATA_APPEND_KEYS`
(such as
interactive filter-related lists), so that `extra_form_data` includes
`{"adhoc_filters": [...]} ` or similar. This configuration is part of the
dashboard's
native filter JSON and is merged server-side using
`EXTRA_FORM_DATA_APPEND_KEYS` in
`superset/charts/data/dashboard_filter_context.py`.
2. In the dashboard UI, apply that filter so the merged dashboard filter
context contains
the expected `extra_form_data` (including `adhoc_filters` or other
appendable keys). The
query-context APIs (GET /api/v1/chart/data) and the new Excel export task
both rely on
`apply_extra_form_data_to_query_context_json` in
`superset/charts/data/dashboard_filter_context.py` to apply this
`extra_form_data` to each
query in the query context JSON.
3. Trigger an Excel export for the dashboard using the new
`/api/v1/dashboard/<pk>/export_xlsx/` endpoint. The backend builds a
`json_body` (query
context) for each chart and passes it with the merged `extra_form_data` into
`apply_extra_form_data_to_query_context_json`. Inside this function, the
current
implementation:
- Assigns `efd = extra_form_data` and `extra_filters = efd.get("filters",
[])`.
- Iterates over `json_body["queries"]` and, if `extra_filters` is
non-empty, appends
them to `query["filters"]` with `isExtra: True`.
- It does **not** handle other appendable keys like `adhoc_filters` or
any additional
keys listed in `EXTRA_FORM_DATA_APPEND_KEYS`, even though they are
present in
`extra_form_data`.
4. Observe the SQL executed for the exported workbook's charts (e.g. via
database query
logs or Superset's query history). The appended `filters` are present, but
any
`adhoc_filters` or other appendable clauses from the dashboard-native filter
are missing
from each query, meaning those active filter conditions are silently dropped
for the
export/GET chart-data path. The resulting Excel data (and any chart-data API
responses
going through this helper) will not include the expected active filter
constraints.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=61ff8cbc774740aa9ca36a158850dc46&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=61ff8cbc774740aa9ca36a158850dc46&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/charts/data/dashboard_filter_context.py
**Line:** 374:382
**Comment:**
*Incomplete Implementation: The query-context merge only appends
`filters`, but ignores other appendable dashboard filter payload keys such as
`adhoc_filters` (and interactive keys) even though they are part of merged
`extra_form_data`. As a result, exports/GET chart-data paths that rely on those
keys will silently drop active dashboard filters and return incorrect data.
Extend this merge to handle all `EXTRA_FORM_DATA_APPEND_KEYS` (not just
`filters`) when mutating each query.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41133&comment_hash=09d3e6705a7f6fe276b18806383e2a6a74887ccb0f0945063c0017ae6f0b6e10&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41133&comment_hash=09d3e6705a7f6fe276b18806383e2a6a74887ccb0f0945063c0017ae6f0b6e10&reaction=dislike'>👎</a>
##########
superset/dashboards/api.py:
##########
@@ -1372,6 +1382,97 @@ def export_as_example(self, pk: int) -> Response:
response.set_cookie(token, "done", max_age=600)
return response
+ @expose("/<pk>/export_xlsx/", methods=("POST",))
+ @protect()
+ @safe
+ @permission_name("export")
+ @statsd_metrics
+ @event_logger.log_this_with_context(
+ action=lambda self, *args, **kwargs:
f"{self.__class__.__name__}.export_xlsx",
+ log_to_statsd=False,
+ )
+ def export_xlsx(self, pk: int) -> WerkzeugResponse:
+ """Export all of a dashboard's chart data to an Excel workbook (async).
+ ---
+ post:
+ summary: Export dashboard chart data to Excel
+ description: >-
+ Enqueues an async task that writes each chart's data to its own
+ worksheet, uploads the .xlsx to S3, and emails the requesting user
a
+ pre-signed download link. Returns immediately with a job id.
+ parameters:
+ - in: path
+ schema:
+ type: integer
+ name: pk
+ description: The dashboard id
+ requestBody:
+ content:
+ application/json:
+ schema:
+ $ref: '#/components/schemas/DashboardExportXlsxPostSchema'
+ responses:
+ 202:
+ description: Export task accepted
+ content:
+ application/json:
+ schema:
+ $ref:
'#/components/schemas/DashboardExportXlsxResponseSchema'
+ 400:
+ $ref: '#/components/responses/400'
+ 401:
+ $ref: '#/components/responses/401'
+ 403:
+ $ref: '#/components/responses/403'
+ 404:
+ $ref: '#/components/responses/404'
+ 500:
+ $ref: '#/components/responses/500'
+ 501:
+ description: Excel export is not configured on this server
+ """
+ if not current_app.config["EXCEL_EXPORT_S3_BUCKET"]:
+ return self.response(
+ 501, message="Excel export is not configured on this server."
+ )
+ try:
+ # Tolerate an empty/non-JSON body (e.g. a POST with no
Content-Type);
+ # request.json would otherwise raise 415.
+ payload = DashboardExportXlsxPostSchema().load(
+ request.get_json(silent=True) or {}
Review Comment:
**Suggestion:** The payload fallback uses `or {}`, which converts any falsy
JSON value (like `[]`, `false`, or `0`) into `{}` and bypasses schema type
validation. That makes malformed request bodies incorrectly succeed and enqueue
exports instead of returning `400`. Only treat `None` (no/invalid JSON body) as
empty input, and let other JSON types flow into schema validation so invalid
types are rejected. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ DashboardRestApi.export_xlsx accepts structurally invalid JSON request
bodies.
- ⚠️ Clients sending bad payloads receive 202 instead of 400 errors.
- ⚠️ Malformed exports may run with unintended empty filter state.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. The Excel export endpoint is implemented in
`DashboardRestApi.export_xlsx` at
`superset/dashboards/api.py:1385-1474`, exposed as
`@expose("/<pk>/export_xlsx/",
methods=("POST",))` and included in `include_route_methods` with
`resource_name =
"dashboard"` (`superset/dashboards/api.py:236-263`), so it is reachable via
the dashboard
REST API.
2. The request body for this endpoint is parsed in `export_xlsx` using
`payload =
DashboardExportXlsxPostSchema().load(request.get_json(silent=True) or {})`
(`superset/dashboards/api.py:1439-1445`); the schema
`DashboardExportXlsxPostSchema` at
`superset/dashboards/schemas.py:628-637` defines a single optional field
`active_data_mask` as a `fields.Dict(...)` with `load_default=dict`.
3. Send an HTTP `POST` to the dashboard export endpoint (e.g.
`/api/v1/dashboard/1/export_xlsx/`) with `Content-Type: application/json`
and a **falsy
non-object JSON body**, such as `[]` or `false`. In Flask,
`request.get_json(silent=True)`
returns the parsed JSON value (`[]` or `False`) rather than `None`; because
the code uses
`or {}`, this falsy value is replaced with `{}` before being passed to
`.load(...)`.
4. `DashboardExportXlsxPostSchema().load({})` succeeds and returns a payload
with
`active_data_mask` defaulted to `{}`, so no `ValidationError` is raised and
`export_xlsx`
continues to enqueue the Celery task via
`export_dashboard_excel.apply_async(...)`
(`superset/dashboards/api.py:1465-1473`) and returns `202`. Without the `or
{}` fallback,
passing `[]`/`false`/`0` into `.load(...)` would cause Marshmallow to raise
a validation
error for the `fields.Dict` field, and `export_xlsx` would correctly return
`400` via the
`except ValidationError` block at `superset/dashboards/api.py:1445-1447`.
The current code
therefore silently accepts structurally invalid but falsy JSON bodies
instead of rejecting
them.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=ddea4787bbfb4598a3d79c5a7f4f4878&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=ddea4787bbfb4598a3d79c5a7f4f4878&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/dashboards/api.py
**Line:** 1441:1442
**Comment:**
*Api Mismatch: The payload fallback uses `or {}`, which converts any
falsy JSON value (like `[]`, `false`, or `0`) into `{}` and bypasses schema
type validation. That makes malformed request bodies incorrectly succeed and
enqueue exports instead of returning `400`. Only treat `None` (no/invalid JSON
body) as empty input, and let other JSON types flow into schema validation so
invalid types are rejected.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41133&comment_hash=1d30977671ca492742372840c0e3b1ab89bb345f5fd57e44345102d917348bf6&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41133&comment_hash=1d30977671ca492742372840c0e3b1ab89bb345f5fd57e44345102d917348bf6&reaction=dislike'>👎</a>
--
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]
