fix(dashboard-filter): Consider dashboard filters to charts not declared in the dashboard position#40774
Conversation
…red in the dashboard position
dosubot
Bot
added
api:charts
Code Review Agent Run #576395Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #40774 +/- ##
==========================================
- Coverage 64.03% 64.03% -0.01%
==========================================
Files 2663 2663
Lines 143619 143623 +4
Branches 33030 33032 +2
==========================================
+ Hits 91973 91975 +2
- Misses 50044 50046 +2
Partials 1602 1602
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| if _find_chart_layout_item(chart_id, position_json) is not None: | ||
| return False |
There was a problem hiding this 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]
Severity Level: Major ⚠️
- ❌ ChartData API 500 when dashboard layout JSON is list.
- ⚠️ Affects dashboards with malformed or legacy position_json data.Steps of Reproduction ✅
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.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
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 fixThere was a problem hiding this comment.
position_json is passed as an argument to this method, typed as dict[str, Any]. If it's not a dict we def have a bigger problem.
|
The flagged issue is correct. The function To resolve this, you should add a type guard to ensure Here is the corrected implementation for def _is_filter_in_scope_for_chart(
filter_config: dict[str, Any],
chart_id: int,
position_json: dict[str, Any],
) -> bool:
# ... (existing code)
# Normalize position_json to a dict if it is not one
layout = position_json if isinstance(position_json, dict) else {}
if chart_layout_item := _find_chart_layout_item(chart_id, layout):
parents: list[str] = chart_layout_item.get("parents", [])
return any(parent in root_path for parent in parents)
# ... (rest of the function)I have checked the PR comments and there are no other pending review comments to address. superset/charts/data/dashboard_filter_context.py |
Vitor-Avila
deleted the
fix/handle-dashboard-filters-charts-not-in-position
branch
2 participants
SUMMARY
When saving/updating a chart, it's possible to also add it to a dashboard. When the chart is added to a dashboard this way, it's not formally added to the dashboard's
position_json.This PR fixes the logic in
_is_filter_in_scope_for_chartto also consider charts in this scenario. This fix is needed for:/api/v1/chart/datawith thefilters_dashboard_idURL param.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
No UI changes.
TESTING INSTRUCTIONS
Unit tests added.
ADDITIONAL INFORMATION