codeant-ai-for-open-source[bot] commented on code in PR #38638:
URL: https://github.com/apache/superset/pull/38638#discussion_r2938043612
##########
tests/integration_tests/charts/data/api_tests.py:
##########
@@ -1810,3 +1810,200 @@ def test_chart_data_subquery_allowed(
rv = test_client.post(CHART_DATA_URI, json=physical_query_context)
assert rv.status_code == status_code
+
+
+class TestGetChartDataWithDashboardFilter(BaseTestChartDataApi):
Review Comment:
**Suggestion:** This new test class uses the same birth-names fixture/query
flow that is explicitly skipped elsewhere in this file due to DuckDB
incompatibility, but it is missing the same class-level skip marker. In CI
environments using DuckDB, these tests can fail even when the feature code is
correct. Add the same skip guard used by adjacent chart data flow test classes.
[logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ DuckDB chart-data CI lane fails on new tests.
- ⚠️ Dashboard-filter feature validation blocked by fixture incompatibility.
```
</details>
```suggestion
@pytest.mark.chart_data_flow
@pytest.mark.skip(
reason=(
"TODO: Fix test class to work with DuckDB example data format. "
"Birth names fixture conflicts with new example data structure."
)
)
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run integration tests in the DuckDB chart-data environment (the same
environment
already acknowledged as broken in this file via class-level skips at
`tests/integration_tests/charts/data/api_tests.py:168-173` and `:1165-1169`
with reason
`"Birth names fixture conflicts with new example data structure."`).
2. Execute any test from `TestGetChartDataWithDashboardFilter` (class
declaration at
`tests/integration_tests/charts/data/api_tests.py:1815`), e.g.
`test_get_data_with_dashboard_filter_context` at `:1851`.
3. The test path uses the same birth-names fixture chain
(`@pytest.mark.usefixtures("load_birth_names_dashboard_with_slices")` at
`:1849`, `:1892`,
`:1937`, etc.) and the same `"Genders"` slice setup in
`_setup_chart_with_query_context`
(`:1818-1820`).
4. Because this class lacks the existing DuckDB skip guard while adjacent
chart-data
classes have it, pytest executes this known-incompatible fixture path and
the test fails
in DuckDB runs before validating the new dashboard-filter behavior.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/integration_tests/charts/data/api_tests.py
**Line:** 1815:1815
**Comment:**
*Logic Error: This new test class uses the same birth-names
fixture/query flow that is explicitly skipped elsewhere in this file due to
DuckDB incompatibility, but it is missing the same class-level skip marker. In
CI environments using DuckDB, these tests can fail even when the feature code
is correct. Add the same skip guard used by adjacent chart data flow test
classes.
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38638&comment_hash=1ab822b99f62fc57e4b8f47e9a5681f2c6510c2721d672c2c605e59debf296b4&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38638&comment_hash=1ab822b99f62fc57e4b8f47e9a5681f2c6510c2721d672c2c605e59debf296b4&reaction=dislike'>👎</a>
##########
superset/charts/data/api.py:
##########
@@ -156,6 +180,53 @@ def get_data( # noqa: C901
json_body["result_type"] = request.args.get("type",
ChartDataResultType.FULL)
json_body["force"] = request.args.get("force")
+ # Apply dashboard filter context when filters_dashboard_id is provided
+ dashboard_filter_context: DashboardFilterContext | None = None
+ filters_dashboard_id = request.args.get("filters_dashboard_id",
type=int)
+ if filters_dashboard_id is not None:
Review Comment:
**Suggestion:** Invalid `filters_dashboard_id` values are silently treated
as if the parameter was not provided because `request.args.get(..., type=int)`
returns `None` on conversion failure. This causes incorrect 200 responses
without applying filters instead of a client error; explicitly validate parsing
and return 400 when the parameter is present but non-integer. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ GET chart data silently ignores malformed dashboard filter ID.
- ⚠️ Clients receive unfiltered results without explicit error.
```
</details>
```suggestion
filters_dashboard_id: int | None = None
if "filters_dashboard_id" in request.args:
try:
filters_dashboard_id =
int(request.args["filters_dashboard_id"])
except (TypeError, ValueError):
return self.response_400(
message=_("`filters_dashboard_id` must be an integer")
)
if filters_dashboard_id is not None:
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Hit GET chart data endpoint `/<int:pk>/data/`
(`superset/charts/data/api.py:81`,
`get_data` at `:89`) with malformed query param:
`/api/v1/chart/<id>/data/?filters_dashboard_id=abc`.
2. In `get_data`, parsing uses `request.args.get("filters_dashboard_id",
type=int)`
(`superset/charts/data/api.py:185`), which yields `None` for bad integer
input; branch at
`:186` is skipped.
3. Because the dashboard filter branch is skipped,
`get_dashboard_filter_context(...)`
(`:188-191`) is never called, so no validation/security/default-filter
application occurs.
4. Endpoint still returns normal 200 data path; `dashboard_filters` metadata
is only
attached when context exists (`superset/charts/data/api.py:534-535`), so
client gets
silent unfiltered response instead of input error.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/charts/data/api.py
**Line:** 185:186
**Comment:**
*Logic Error: Invalid `filters_dashboard_id` values are silently
treated as if the parameter was not provided because `request.args.get(...,
type=int)` returns `None` on conversion failure. This causes incorrect 200
responses without applying filters instead of a client error; explicitly
validate parsing and return 400 when the parameter is present but non-integer.
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38638&comment_hash=61085592bfe9ca44b74574045a51a76f287c7985f2abd6f6b8b38bab55a3be76&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38638&comment_hash=61085592bfe9ca44b74574045a51a76f287c7985f2abd6f6b8b38bab55a3be76&reaction=dislike'>👎</a>
##########
superset/charts/data/api.py:
##########
@@ -156,6 +180,53 @@ def get_data( # noqa: C901
json_body["result_type"] = request.args.get("type",
ChartDataResultType.FULL)
json_body["force"] = request.args.get("force")
+ # Apply dashboard filter context when filters_dashboard_id is provided
+ dashboard_filter_context: DashboardFilterContext | None = None
+ filters_dashboard_id = request.args.get("filters_dashboard_id",
type=int)
+ if filters_dashboard_id is not None:
+ try:
+ dashboard_filter_context = get_dashboard_filter_context(
+ dashboard_id=filters_dashboard_id,
+ chart_id=pk,
+ )
+ except ValueError as error:
+ return self.response_400(message=str(error))
+ except SupersetSecurityException:
+ return self.response_403()
+
+ if dashboard_filter_context.extra_form_data:
+ efd = dashboard_filter_context.extra_form_data
+ extra_filters = efd.get("filters", [])
+
+ for query in json_body.get("queries", []):
+ if extra_filters:
+ existing = query.get("filters", [])
+ query["filters"] = existing + [
+ {**f, "isExtra": True} for f in extra_filters
+ ]
Review Comment:
**Suggestion:** `filters` can be `None` in query objects (schema allows
null), so concatenating it with a list raises a `TypeError` at runtime when
dashboard filters are applied. Normalize missing/null filters to an empty list
before concatenation. [null pointer]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ GET chart data crashes when saved query filters null.
- ⚠️ Dashboard default filters cannot be applied reliably.
```
</details>
```suggestion
existing = query.get("filters") or []
query["filters"] = existing + [
{**f, "isExtra": True} for f in extra_filters
]
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Use chart save/update flow where `query_context` is accepted as raw JSON
string
(`ChartPostSchema.query_context` in `superset/charts/schemas.py:213`; update
persists
properties via `ChartDAO.update` in `superset/commands/chart/update.py:73`)
and store a
query object containing `"filters": null`.
2. Call GET chart data endpoint with dashboard filter application enabled
(`superset/charts/data/api.py:81`, `:185-186`) and a dashboard whose
defaults produce
`extra_form_data.filters` (`:199-203`).
3. Loop over saved query context (`superset/charts/data/api.py:201`)
executes `existing =
query.get("filters", [])` (`:203`), returning `None` because key exists with
null.
4. Next line performs `None + list` (`:204-206`), raising `TypeError` before
request
validation/response handling, causing server error on this endpoint path.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/charts/data/api.py
**Line:** 203:206
**Comment:**
*Null Pointer: `filters` can be `None` in query objects (schema allows
null), so concatenating it with a list raises a `TypeError` at runtime when
dashboard filters are applied. Normalize missing/null filters to an empty list
before concatenation.
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38638&comment_hash=f726c98f1d17cb82eae8dd233d4323d39bc82e3c3f92a39bee253b522a1848c0&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38638&comment_hash=f726c98f1d17cb82eae8dd233d4323d39bc82e3c3f92a39bee253b522a1848c0&reaction=dislike'>👎</a>
##########
superset/charts/data/api.py:
##########
@@ -156,6 +180,53 @@ def get_data( # noqa: C901
json_body["result_type"] = request.args.get("type",
ChartDataResultType.FULL)
json_body["force"] = request.args.get("force")
+ # Apply dashboard filter context when filters_dashboard_id is provided
+ dashboard_filter_context: DashboardFilterContext | None = None
+ filters_dashboard_id = request.args.get("filters_dashboard_id",
type=int)
+ if filters_dashboard_id is not None:
+ try:
+ dashboard_filter_context = get_dashboard_filter_context(
+ dashboard_id=filters_dashboard_id,
+ chart_id=pk,
+ )
+ except ValueError as error:
+ return self.response_400(message=str(error))
+ except SupersetSecurityException:
+ return self.response_403()
+
+ if dashboard_filter_context.extra_form_data:
+ efd = dashboard_filter_context.extra_form_data
+ extra_filters = efd.get("filters", [])
+
+ for query in json_body.get("queries", []):
+ if extra_filters:
+ existing = query.get("filters", [])
+ query["filters"] = existing + [
+ {**f, "isExtra": True} for f in extra_filters
+ ]
+
+ extras = query.get("extras", {})
+ for key in EXTRA_FORM_DATA_OVERRIDE_EXTRA_KEYS:
+ if key in efd:
+ extras[key] = efd[key]
Review Comment:
**Suggestion:** `extras` can be `None` in query objects (schema allows
null), and assigning keys to `None` raises a runtime exception when extra
form-data overrides are present. Coerce null `extras` to a dict before writing
override keys. [null pointer]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ GET chart data fails when query extras is null.
- ⚠️ Time/extras overrides from dashboard defaults are lost.
```
</details>
```suggestion
extras = query.get("extras") or {}
for key in EXTRA_FORM_DATA_OVERRIDE_EXTRA_KEYS:
if key in efd:
extras[key] = efd[key]
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Persist a chart `query_context` containing `"extras": null` (allowed
nullable query
object fields: `extras` in `superset/charts/schemas.py:1337-1340`; chart
query_context
accepted as JSON string at `superset/charts/schemas.py:213`).
2. Invoke GET `/api/v1/chart/<id>/data/?filters_dashboard_id=<dashboard_id>`
so dashboard
context is loaded (`superset/charts/data/api.py:185-191`) and
`extra_form_data` is present
(`:197-199`).
3. Ensure `efd` includes any override key iterated by
`EXTRA_FORM_DATA_OVERRIDE_EXTRA_KEYS`
(`superset/charts/data/api.py:209-211`), then
mutation path runs.
4. `extras = query.get("extras", {})` returns `None` when key exists as null
(`:208`), and
assignment `extras[key] = ...` at `:211` raises `TypeError`, breaking
request execution.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/charts/data/api.py
**Line:** 208:211
**Comment:**
*Null Pointer: `extras` can be `None` in query objects (schema allows
null), and assigning keys to `None` raises a runtime exception when extra
form-data overrides are present. Coerce null `extras` to a dict before writing
override keys.
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38638&comment_hash=b271a2b1e8d4b1f3efdae349d6633d05afc89b76860a268b5396ee7ed9315780&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38638&comment_hash=b271a2b1e8d4b1f3efdae349d6633d05afc89b76860a268b5396ee7ed9315780&reaction=dislike'>👎</a>
##########
superset/charts/data/dashboard_filter_context.py:
##########
@@ -0,0 +1,288 @@
+# 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.
+from __future__ import annotations
+
+import logging
+from dataclasses import dataclass, field
+from enum import Enum
+from typing import Any
+
+from flask_babel import gettext as _
+
+from superset import db, security_manager
+from superset.models.dashboard import Dashboard
+from superset.utils import json
+
+logger = logging.getLogger(__name__)
+
+CHART_TYPE = "CHART"
+
+
+class DashboardFilterStatus(str, Enum):
+ APPLIED = "applied"
+ NOT_APPLIED = "not_applied"
+ NOT_APPLIED_USES_DEFAULT_TO_FIRST_ITEM_PREQUERY = (
+ "not_applied_uses_default_to_first_item_prequery"
+ )
+
+
+@dataclass
+class DashboardFilterInfo:
+ id: str
+ name: str
+ status: DashboardFilterStatus
+ column: str | None = None
+
+
+@dataclass
+class DashboardFilterContext:
+ extra_form_data: dict[str, Any] = field(default_factory=dict)
+ filters: list[DashboardFilterInfo] = field(default_factory=list)
+
+ def to_dict(self) -> dict[str, Any]:
+ return {
+ "filters": [
+ {
+ "id": f.id,
+ "name": f.name,
+ "status": f.status.value,
+ **({"column": f.column} if f.column else {}),
+ }
+ for f in self.filters
+ ],
+ }
+
+
+def _is_filter_in_scope_for_chart(
+ filter_config: dict[str, Any],
+ chart_id: int,
+ position_json: dict[str, Any],
+) -> bool:
+ """
+ Determines whether a native filter applies to a given chart. When
+ chartsInScope is present on the filter config, uses that directly.
+ Otherwise falls back to scope.rootPath and scope.excluded with
+ the dashboard layout.
+ """
+ if charts_in_scope := filter_config.get("chartsInScope"):
+ return chart_id in charts_in_scope
Review Comment:
**Suggestion:** The scope check treats an empty `chartsInScope` list as if
the field were missing and falls back to `rootPath`, which can incorrectly
apply a filter to charts that should receive no filter. Handle the presence of
`chartsInScope` explicitly so `[]` means "in scope for no charts". [logic error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Scoped-out filters can still affect chart SQL.
- ⚠️ Dashboard filter metadata reports misleading applied status.
- ⚠️ GET chart data differs from frontend scope semantics.
```
</details>
```suggestion
if "chartsInScope" in filter_config:
charts_in_scope = filter_config.get("chartsInScope")
if charts_in_scope is not None:
return chart_id in charts_in_scope
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Call GET
`api/v1/chart/<chart_id>/data/?filters_dashboard_id=<dashboard_id>`
(entrypoint in `superset/charts/data/api.py:185-190`), which invokes
`get_dashboard_filter_context(...)`.
2. Ensure dashboard metadata contains a native filter with explicit
`"chartsInScope": []`
and default `scope.rootPath` (this field is persisted when present; see
helper behavior in
`tests/unit_tests/charts/test_dashboard_filter_context.py:101-103`).
3. Execution reaches `_is_filter_in_scope_for_chart`
(`superset/charts/data/dashboard_filter_context.py:70-97`); line 81 treats
`[]` as falsy,
so it skips explicit scope and falls back to `rootPath` logic.
4. For charts under `ROOT_ID`, line 96 returns True and filter is treated
in-scope, so
defaults are merged (`dashboard_filter_context.py:274-277`) and applied to
chart query
unexpectedly.
```
</details>
<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:** 81:82
**Comment:**
*Logic Error: The scope check treats an empty `chartsInScope` list as
if the field were missing and falls back to `rootPath`, which can incorrectly
apply a filter to charts that should receive no filter. Handle the presence of
`chartsInScope` explicitly so `[]` means "in scope for no charts".
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38638&comment_hash=26265dd75eeb82280bd39234d819b5b54778e4e2b1580e341c50f257e6c7ddba&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38638&comment_hash=26265dd75eeb82280bd39234d819b5b54778e4e2b1580e341c50f257e6c7ddba&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]
