fix(chart API): Consider time grain filters with the filters_dashboard_id param#41290
Conversation
Code Review Agent Run #73fb3dActionable Suggestions - 0Additional Suggestions - 1
Review 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 |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| if custom_time_grain := extra_form_data.get("time_grain_sqla"): | ||
| extras["time_grain_sqla"] = custom_time_grain |
There was a problem hiding this comment.
Suggestion: The time-grain override is gated by a truthiness check, so a provided falsy value (for example an explicit empty/null-like value used to clear an existing grain) is ignored instead of being applied. This leaves stale chart-level grain values in extras/columns and makes dashboard overrides non-deterministic. Check for key presence (or is not None) rather than truthiness when deciding whether to apply the override. [incorrect condition logic]
Severity Level: Major ⚠️
- ❌ Dashboard time-grain clear operations silently fail.
- ⚠️ Chart and dashboard grains become inconsistent.
- ⚠️ Future falsy time-grain overrides behave non-deterministically.Steps of Reproduction ✅
1. The chart data GET endpoint `/api/v1/chart/<pk>/data` builds `json_body` and, when
`filters_dashboard_id` is present, calls `get_dashboard_filter_context()` and then
`apply_dashboard_filter_context(json_body, efd)` at `superset/charts/data/api.py:14-29`.
2. `get_dashboard_filter_context()` in
`superset/charts/data/dashboard_filter_context.py:18-47` merges native filters'
`extra_form_data` into a single dict; if a time-grain native filter is configured to clear
the grain it can yield `extra_form_data={"time_grain_sqla": ""}` or `{"time_grain_sqla":
None}`.
3. `apply_dashboard_filter_context()` at
`superset/charts/data/dashboard_filter_context.py:64-86` executes `if custom_time_grain :=
extra_form_data.get("time_grain_sqla"):` (line 339 in the PR hunk); for a falsy override
like `""` or `None`, `custom_time_grain` is falsy so the block is skipped and
`extras["time_grain_sqla"]` is left at the prior chart-level value.
4. The resulting `query_context` (built later via
`_create_query_context_from_form(json_body)` in `superset/charts/data/api.py:38-40`) still
carries the old `extras["time_grain_sqla"]`, so any backend consumer (e.g.
`QueryObjectHelper.get_time_grain()` in `superset/models/helpers.py:2011-22` for charts
without adhoc x-axis) sees the stale grain and the dashboard attempt to clear or override
the grain is ignored.
(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:** 339:340
**Comment:**
*Incorrect Condition Logic: The time-grain override is gated by a truthiness check, so a provided falsy value (for example an explicit empty/null-like value used to clear an existing grain) is ignored instead of being applied. This leaves stale chart-level grain values in `extras`/columns and makes dashboard overrides non-deterministic. Check for key presence (or `is not None`) rather than truthiness when deciding whether to apply the override.
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
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #41290 +/- ##
==========================================
- Coverage 64.34% 64.33% -0.02%
==========================================
Files 2653 2653
Lines 144989 145031 +42
Branches 33456 33463 +7
==========================================
+ Hits 93295 93299 +4
- Misses 50011 50046 +35
- Partials 1683 1686 +3
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:
|
There was a problem hiding this comment.
Pull request overview
Fixes the /api/v1/chart/<pk>/data handling of the filters_dashboard_id parameter so that dashboard-applied time grain filters correctly override the chart’s query context (both in query.extras.time_grain_sqla and the X-Axis adhoc column’s timeGrain), aligning backend behavior with frontend normalization logic.
Changes:
- Update
apply_dashboard_filter_contextto propagatetime_grain_sqlaintoquery["extras"]and into theBASE_AXIScolumn’stimeGrain. - Add unit tests to validate time grain override behavior (extras-only, X-Axis override, and no-grain-filter preservation).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| superset/charts/data/dashboard_filter_context.py | Ensures dashboard time grain overrides are applied to query context extras and X-Axis column configuration. |
| tests/unit_tests/charts/test_chart_data_api.py | Adds regression tests covering dashboard time grain override scenarios. |
|
Bito Automatic Review Skipped – PR Already Merged |
SUMMARY
The
filters_dashboard_idURL param for the/api/v1/chart/<pk>/dataAPI was not considering time grain filters applied in the dashboard. This PR fixes the issue.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
No UI changes.
TESTING INSTRUCTIONS
Test coverage added. For manual testing:
cleaned_sales_datadataset. Useorder_dateas the X-Axis with Time Grain set to Day, and any metric.GET /api/v1/chart/{{ChartID}}/data?filters_dahsboard_id={{DashboardID}}and confirm the time grain applied is coming from the dashboard (not the chart's default value).ADDITIONAL INFORMATION