Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion superset/charts/data/dashboard_filter_context.py
Original file line number Diff line number Diff line change
Expand Up @@ -306,7 +306,7 @@ def get_dashboard_filter_context(
return context


def apply_dashboard_filter_context(
def apply_dashboard_filter_context( # noqa: C901
query_context: dict[str, Any],
extra_form_data: dict[str, Any],
) -> None:
Expand All @@ -332,6 +332,17 @@ def apply_dashboard_filter_context(
for key in EXTRA_FORM_DATA_OVERRIDE_EXTRA_KEYS:
if key in extra_form_data:
extras[key] = extra_form_data[key]

# EXTRA_FORM_DATA_OVERRIDE_EXTRA_KEYS is originally used with form_data objects,
# not query_context objects. form_data objects expect time_grain_sqla as a
# top-level key, but query_context objects expect it as an extra key.
if custom_time_grain := extra_form_data.get("time_grain_sqla"):
extras["time_grain_sqla"] = custom_time_grain
Comment on lines +339 to +340

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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:** 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
👍 | 👎

# get_time_grain() resolves grain from the first adhoc column (columns[0])
columns = query.get("columns") or []
if columns and isinstance(columns[0], dict):
columns[0]["timeGrain"] = custom_time_grain

if extras:
query["extras"] = extras

Expand Down
96 changes: 96 additions & 0 deletions tests/unit_tests/charts/test_chart_data_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,102 @@ def test_apply_dashboard_filter_context_does_not_duplicate_filters(
assert ExtraCache().filter_values("country") == ["USA"]


def test_apply_dashboard_filter_context_applies_time_grain_to_extras() -> None:
"""
A dashboard time-grain filter must land in ``query["extras"]``, where
get_time_grain() reads it for charts that have no adhoc x-axis column.
"""
query_context_json: dict[str, Any] = {
"queries": [{"extras": {"time_grain_sqla": "P1D", "having": "", "where": ""}}],
}

apply_dashboard_filter_context(query_context_json, {"time_grain_sqla": "P1M"})

assert query_context_json["queries"][0]["extras"]["time_grain_sqla"] == "P1M"


def test_apply_dashboard_filter_context_overrides_x_axis_time_grain() -> None:
"""
For charts with an adhoc X-Axis, the dashboard grain must override the
BASE_AXIS column's ``timeGrain`` (which get_time_grain() reads before
falling back to extras), mirroring the frontend's normalizeTimeColumn.
"""
query_context_json: dict[str, Any] = {
"queries": [
{
"columns": [
{
"timeGrain": "P1D",
"columnType": "BASE_AXIS",
"sqlExpression": "order_date",
}
],
"extras": {"time_grain_sqla": "P1D"},
}
],
}

apply_dashboard_filter_context(query_context_json, {"time_grain_sqla": "P1Y"})

query = query_context_json["queries"][0]
assert query["columns"][0]["timeGrain"] == "P1Y"
assert query["extras"]["time_grain_sqla"] == "P1Y"


def test_apply_dashboard_filter_context_grain_targets_first_adhoc_column() -> None:
"""
The grain override must land on ``columns[0]`` to match frontend logic.
"""
query_context_json: dict[str, Any] = {
"queries": [
{
"columns": [
{"timeGrain": "P1D", "sqlExpression": "order_date"},
{"columnType": "BASE_AXIS", "sqlExpression": "other"},
],
"extras": {},
}
],
}

apply_dashboard_filter_context(query_context_json, {"time_grain_sqla": "P1Y"})

columns = query_context_json["queries"][0]["columns"]
assert columns[0]["timeGrain"] == "P1Y" # the column get_time_grain reads
assert "timeGrain" not in columns[1] # the BASE_AXIS-tagged one is untouched


def test_apply_dashboard_filter_context_keeps_grain_when_no_grain_filter() -> None:
"""
When the dashboard applies a non-grain filter (e.g. a value filter), the
chart's own x-axis ``timeGrain`` must be preserved -- not wiped -- since no
dashboard grain was provided.
"""
query_context_json: dict[str, Any] = {
"queries": [
{
"columns": [
{
"timeGrain": "P1M",
"columnType": "BASE_AXIS",
"sqlExpression": "order_date",
}
],
"extras": {"time_grain_sqla": "P1M"},
}
],
}

# extra_form_data carries a value filter but NO time_grain_sqla
apply_dashboard_filter_context(
query_context_json,
{"filters": [{"col": "country", "op": "IN", "val": ["US"]}]},
)

query = query_context_json["queries"][0]
assert query["columns"][0]["timeGrain"] == "P1M"


def _extract_filename(form_value: str) -> str | None:
"""Run _extract_export_params_from_request with a form filename value."""
from superset.charts.data.api import ChartDataRestApi
Expand Down
Loading