codeant-ai-for-open-source[bot] commented on code in PR #40774:
URL: https://github.com/apache/superset/pull/40774#discussion_r3358144319
##########
superset/charts/data/dashboard_filter_context.py:
##########
@@ -78,13 +78,21 @@ def _is_filter_in_scope_for_chart(
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.
+ 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. Also considers charts that were added to the dashboard
+ but were not instantiated in the layout.
"""
if (charts_in_scope := filter_config.get("chartsInScope")) is not None:
- return chart_id in charts_in_scope
+ if chart_id in charts_in_scope:
+ return True
+
+ # If the chart is found in position_json and not in chartsInScope,
+ # it was explicitly excluded by the filter scope config.
+ if _find_chart_layout_item(chart_id, position_json) is not None:
+ return False
Review Comment:
**Suggestion:** The new fallback path calls `_find_chart_layout_item` before
validating that `position_json` is a dictionary. If `dashboard.position_json`
deserializes to a non-dict JSON type (for example `[]` from malformed/legacy
data), `_find_chart_layout_item` will call `.values()` on that object and raise
an `AttributeError`, breaking filter context resolution. Add a type guard
(`isinstance(position_json, dict)`) before this lookup or normalize non-dict
inputs to `{}`. [type error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ ChartData API 500 when dashboard layout JSON is list.
- ⚠️ Affects dashboards with malformed or legacy position_json data.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Prepare a dashboard whose `position_json` column contains a non-dict JSON
value, e.g.
the literal string `"[]"`. This can be done by directly updating the
`dashboards.position_json` field in the database or by importing legacy
data. Other parts
of the codebase (e.g. `superset/views/utils.py:366-371` and
`superset/mcp_service/chart/chart_helpers.py:50-56`) explicitly guard
`json.loads(dashboard.position_json)` with `isinstance(..., dict)`,
indicating such
malformed/legacy values are expected in practice.
2. Call the Chart Data API endpoint for a chart on that dashboard: `GET
/api/v1/chart/data/<chart_id>` with query parameter
`filters_dashboard_id=<dashboard_id>`.
This hits `superset/charts/data/api.py:183-196`, where
`filters_dashboard_id` is parsed
and `get_dashboard_filter_context` is invoked (lines 27-32 in the snippet,
corresponding
to `api.py:183-196`).
3. Inside `get_dashboard_filter_context`
(`superset/charts/data/dashboard_filter_context.py:35-67`), the code loads
metadata and
layout as:
- `metadata = json.loads(dashboard.json_metadata or "{}")` (line 61)
- `native_filter_config = metadata.get("native_filter_configuration",
[])` (lines
62-64)
- `position_json: dict[str, Any] = json.loads(dashboard.position_json or
"{}")` (line
66)
No `isinstance(position_json, dict)` check is performed here, so when
`dashboard.position_json == "[]"`, `position_json` becomes a Python list.
4. For each native filter, `_is_filter_in_scope_for_chart` is called with
this list-valued
`position_json` (`dashboard_filter_context.py:70-76`). Inside
`_is_filter_in_scope_for_chart` (`lines 75-107`), when `chartsInScope` is
present and the
chart is not in it, or later when evaluating layout-based scope, the
function calls
`_find_chart_layout_item(chart_id, position_json)` at lines 94 and 104.
`_find_chart_layout_item` (`lines 114-120`) immediately executes `for item in
position_json.values():`, but since `position_json` is a list, this raises
`AttributeError: 'list' object has no attribute 'values'`. This propagates
up and causes
the `/api/v1/chart/data` request with `filters_dashboard_id` to fail with a
500 error,
instead of gracefully treating the layout as empty. Adding an
`isinstance(position_json,
dict)` guard (or normalizing non-dict to `{}`) before these calls prevents
the crash.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=ed057894caaf47389c6be651378d4bec&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=ed057894caaf47389c6be651378d4bec&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:** 94:95
**Comment:**
*Type Error: The new fallback path calls `_find_chart_layout_item`
before validating that `position_json` is a dictionary. If
`dashboard.position_json` deserializes to a non-dict JSON type (for example
`[]` from malformed/legacy data), `_find_chart_layout_item` will call
`.values()` on that object and raise an `AttributeError`, breaking filter
context resolution. Add a type guard (`isinstance(position_json, dict)`) before
this lookup or normalize non-dict inputs to `{}`.
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%2F40774&comment_hash=d0814a5349f7cddf633acbd0ed01f040969f567ef679c2914dc6725dff1edf37&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40774&comment_hash=d0814a5349f7cddf633acbd0ed01f040969f567ef679c2914dc6725dff1edf37&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]
