aminghadersohi opened a new pull request, #42576:
URL: https://github.com/apache/superset/pull/42576
### SUMMARY
`get_chart_preview` (MCP tool) fails on common chart types when generating
`ascii`/`table` previews:
- On `echarts_timeseries_line` charts, it can raise an unhandled
`AttributeError: 'NoneType' object has no attribute 'copy'`, because
`resolve_metrics()` used `form_data.get("metrics", [])`, which only applies the
`[]` default when the key is *absent* — if `form_data["metrics"]` is explicitly
`null` (as happens for some saved/migrated chart configs), `None` flows
downstream instead of an empty list.
- On `big_number` (and `big_number_total`/`pop_kpi`) charts saved without
the singular `metric` field, the preview falls through to the query engine and
surfaces a generic, confusing failure instead of a clear explanation. Some
saved configs also store the metric under the plural `metrics` key even for
these single-metric chart types, silently losing the metric entirely.
- A query result whose `"data"` field is explicitly `null` (rather than
absent) was treated as present, which could propagate `None` into ASCII
rendering.
This PR:
- Hardens `resolve_metrics()` / `resolve_metrics_and_groupby()` in
`chart_helpers.py` against explicit `null` values, and adds a plural-`metrics`
fallback for single-metric chart types.
- Adds a `_query_context_has_fields()` pre-flight check in
`ASCIIPreviewStrategy` and `TablePreviewStrategy` (`get_chart_preview.py`) that
short-circuits with a clear `NoQueryFields` error when a chart has no metrics
or columns to query, instead of hitting `ChartDataCommand`.
- Guards against an explicitly-`null` `"data"` field in the query result.
- Broadens `generate_ascii_chart()`'s exception handling to also catch
`AttributeError` as defense-in-depth for the ASCII/bar/line/scatter renderers.
### TESTING INSTRUCTIONS
Added unit tests covering the None-safety gaps and the new short-circuit
behavior:
- `tests/unit_tests/mcp_service/chart/test_chart_helpers.py`:
`resolve_metrics`/`resolve_metrics_and_groupby` with explicit `None` values and
the plural-`metrics` fallback for `big_number`-family chart types.
- `tests/unit_tests/mcp_service/chart/tool/test_get_chart_preview.py`:
`_query_context_has_fields`/`_no_query_fields_error` unit tests, plus two
regression tests — a `big_number` chart with no metric returns a
`NoQueryFields` `ChartError` without ever calling `ChartDataCommand`, and an
ASCII preview with an explicitly-`null` query result `"data"` renders a
graceful "No data" message instead of crashing.
```
pytest tests/unit_tests/mcp_service/chart/tool/test_get_chart_preview.py \
tests/unit_tests/mcp_service/chart/test_chart_helpers.py \
tests/unit_tests/mcp_service/chart/test_ascii_charts.py -q
# 147 passed
```
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in
[SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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]
