aminghadersohi opened a new pull request, #43599:
URL: https://github.com/apache/superset/pull/43599
## Why
`extra_form_data` lets a caller override the time grain on a chart query.
That override was
written to the query payload as a top-level `time_grain_sqla` key, but the
time grain is not
a query object field — it is carried inside `extras`, which is how form_data
is translated in
`superset.common.form_data_query_context` (`extras["time_grain_sqla"]`, and
`time_grain_sqla`
is declared on `ChartDataExtrasSchema`, not `ChartDataQueryObjectSchema`).
Because `ChartDataQueryObjectSchema` is configured with `unknown = EXCLUDE`,
the misplaced key
was dropped during load without raising. The caller got the chart's original
grain back with
no indication the override had been ignored.
This is visible as an inconsistency in which `extra_form_data` fields appear
to work: overrides
that happen to map to real query object fields (`time_range`, `granularity`)
apply normally,
while the grain silently does not.
## What
Split the override keys into those that are query object fields and those
that belong in
`extras`, and route the grain into `query["extras"]`, merging rather than
replacing so other
extras (`where`, `having`, relative time bounds) are preserved.
## Blast radius
Limited to the MCP chart tools that merge `extra_form_data` into a query
payload
(`get_chart_data`, `get_chart_sql`). No change to the chart-data REST API or
to query
construction itself — this only moves the value to the field the query
object actually reads.
Charts that do not send a grain override are unaffected.
## How to test
`tests/unit_tests/mcp_service/chart/test_chart_helpers.py`
- `test_merge_extra_form_data_time_grain_override_lands_in_extras` — the
override reaches
`extras`; fails on master.
- `test_merge_extra_form_data_time_grain_preserves_existing_extras` —
existing extras survive.
- Two pre-existing tests asserted the top-level placement and are updated to
assert `extras`;
they encoded the bug.
Verified by reverting the source change with the tests in place: the three
grain cases fail,
the rest pass. Full `tests/unit_tests/mcp_service/chart/` suite: 1333 passed.
## Risk & rollback
Low, and in the direction of correctness: a grain override that was
previously ignored now
takes effect, so a caller sending one will see the grain they asked for.
Anything relying on
the override being dropped would change behavior, but that outcome was the
defect. Straight
revert if needed.
## Review guidance
The whole change is `QUERY_CONTEXT_EXTRA_FORM_DATA_EXTRAS_KEYS` and the
branch in
`merge_form_data_filters_into_query`. Worth confirming the extras merge
cannot clobber
`where`/`having` set earlier in the same function — covered by the second
test.
--
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]
