bito-code-review[bot] commented on code in PR #43176:
URL: https://github.com/apache/superset/pull/43176#discussion_r4142154727
##########
superset/charts/data/form_data.py:
##########
@@ -31,33 +31,69 @@ def set_form_data(form_data: dict[str, Any]) -> None:
g.form_data = form_data
+def _as_form_data_dict(value: Any) -> dict[str, Any]:
+ return value if isinstance(value, dict) else {}
+
+
+def _as_query_list(value: Any) -> list[Any]:
+ if isinstance(value, (list, tuple)):
+ return list(value)
+ return []
+
+
def _serialize_query(
query: QueryObject,
form_data: dict[str, Any],
-) -> dict[str, Any]:
- """Serialize query fields consumed by the Jinja form-data fallback."""
- query_data = dict(query.to_dict())
- query_data["filters"] = query.filter
- if query.time_range is not None:
- query_data["time_range"] = query.time_range
+ time_range: str | None = None,
+) -> dict[str, Any] | None:
+ """Serialize query fields consumed by the Jinja form-data fallback.
+
+ Incomplete stubs (unit-test doubles without ``to_dict``) are skipped so
+ callers can still publish datasource context for Jinja without requiring a
+ full ``QueryObject``.
+
+ ``time_range`` is an optional overlay for callers that deliberately leave
+ ``QueryObject.time_range`` unset (tabular queries, so relative ranges keep
+ ``from_dttm``/``to_dttm`` in the cache key). Chart and async callers omit
+ it so ``get_time_filter()`` matches the chart-data API: a TEMPORAL_RANGE
+ filter alone is not a published time range.
+ """
+ to_dict = getattr(query, "to_dict", None)
+ if not callable(to_dict):
+ return None
+
+ query_data = dict(to_dict())
+ filters = getattr(query, "filter", None)
+ query_data["filters"] = filters
+ resolved = time_range
+ if resolved is None:
+ obj_range = getattr(query, "time_range", None)
+ if isinstance(obj_range, str):
+ resolved = obj_range
+ if resolved is not None:
+ query_data["time_range"] = resolved
if url_params := form_data.get("url_params"):
query_data["url_params"] = url_params
return query_data
def set_query_context_form_data(
query_context: QueryContext,
- datasource_id: int,
+ datasource_id: int | str,
datasource_type: str,
+ time_range: str | None = None,
) -> None:
"""Expose a programmatically-created query like a chart data API
request."""
- form_data = query_context.form_data or {}
+ form_data = _as_form_data_dict(getattr(query_context, "form_data", None))
+ queries = _as_query_list(getattr(query_context, "queries", None))
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Misleading QueryContext annotation</b></div>
<div id="fix">
Signature declares `QueryContext`, but
`_as_form_data_dict(getattr(query_context, `form_data`, None))` accepts any
object — tests pass `object()` (form_data_test.py:153). The nominal annotation
misleads callers, and the `getattr` defaults turn a renamed
`form_data`/`queries` field into silently empty Jinja context instead of a loud
error. Prefer direct attribute access or a structural Protocol matching what is
actually accepted.
</div>
</div>
<details>
<summary><b>Citations</b></summary>
<ul>
<li>
Rule Violated: <a
href="https://github.com/apache/superset/blob/bd6d88f/.cursor/rules/dev-standard.mdc#L28";>dev-standard.mdc:28</a>
</li>
</ul>
</details>
<small><i>Code Review Run #540efc</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset/charts/data/form_data.py:
##########
@@ -31,33 +31,69 @@ def set_form_data(form_data: dict[str, Any]) -> None:
g.form_data = form_data
+def _as_form_data_dict(value: Any) -> dict[str, Any]:
+ return value if isinstance(value, dict) else {}
+
+
+def _as_query_list(value: Any) -> list[Any]:
+ if isinstance(value, (list, tuple)):
+ return list(value)
+ return []
+
+
def _serialize_query(
query: QueryObject,
form_data: dict[str, Any],
-) -> dict[str, Any]:
- """Serialize query fields consumed by the Jinja form-data fallback."""
- query_data = dict(query.to_dict())
- query_data["filters"] = query.filter
- if query.time_range is not None:
- query_data["time_range"] = query.time_range
+ time_range: str | None = None,
+) -> dict[str, Any] | None:
+ """Serialize query fields consumed by the Jinja form-data fallback.
+
+ Incomplete stubs (unit-test doubles without ``to_dict``) are skipped so
+ callers can still publish datasource context for Jinja without requiring a
+ full ``QueryObject``.
+
+ ``time_range`` is an optional overlay for callers that deliberately leave
+ ``QueryObject.time_range`` unset (tabular queries, so relative ranges keep
+ ``from_dttm``/``to_dttm`` in the cache key). Chart and async callers omit
+ it so ``get_time_filter()`` matches the chart-data API: a TEMPORAL_RANGE
+ filter alone is not a published time range.
+ """
+ to_dict = getattr(query, "to_dict", None)
+ if not callable(to_dict):
+ return None
+
+ query_data = dict(to_dict())
+ filters = getattr(query, "filter", None)
+ query_data["filters"] = filters
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Dead defensive branch for stubs</b></div>
<div id="fix">
`getattr(query, `to_dict`, None)` and `getattr(query, `filter`, None)`
differ from direct access only for stubs; every production caller passes
`QueryObject`, which defines both, so the `None` return and the `filters: None`
publication are production-unreachable branches that also mask attribute
renames as silently wrong Jinja form data. Consider direct access, handling
stubs in tests.
</div>
</div>
<small><i>Code Review Run #540efc</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset/mcp_service/chart/compile.py:
##########
@@ -94,6 +94,7 @@ def _compile_chart(
Returns a :class:`CompileResult` with ``success=True`` when the
query executes cleanly.
"""
+ from superset.charts.data.form_data import set_query_context_form_data
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Inline import violates guideline</b></div>
<div id="fix">
This function-local import lacks the circular-dependency justification or
explanatory comment that BITO.md rule 12745 requires for inline imports. Unlike
the deferred `ChartDataCommand` imports beside it,
`superset.charts.data.form_data` imports only `flask.g` at runtime, and sibling
tools (`get_chart_data.py`, `get_chart_preview.py`, `get_chart_sql.py`) import
it at module level. Move it to the module imports.
</div>
</div>
<small><i>Code Review Run #540efc</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
tests/unit_tests/mcp_service/chart/test_preview_utils.py:
##########
@@ -398,3 +398,80 @@ def test_vega_preview_y_axis_fallback_accepts_decimal() ->
None:
)
assert encoding["y"]["field"] == "revenue"
+
+
+def test_generate_preview_from_form_data_exposes_jinja_context(
+ monkeypatch: pytest.MonkeyPatch,
+) -> None:
+ """Unsaved-chart previews expose the same Jinja inputs as execution."""
+ from types import SimpleNamespace
+ from typing import Any
+ from unittest.mock import MagicMock
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Redundant duplicate import</b></div>
<div id="fix">
`MagicMock` is already imported at module level (line 26: `from
unittest.mock import MagicMock, Mock, patch`); re-importing it inside the test
shadows the identical name and adds a second binding to keep in sync. The other
function-local imports here (`SimpleNamespace`, `current_app`, `QueryObject`,
the form_data_test helper) have no module-level counterpart, so only this one
is redundant. Drop line 409.
</div>
</div>
<small><i>Code Review Run #540efc</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset/common/tabular_query.py:
##########
@@ -397,6 +402,28 @@ def build_query_dict(
return query_dict
+def _time_range_from_filters(filters: Any) -> str | None:
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Confusable helper names</b></div>
<div id="fix">
`_time_range_from_filters` (extracts a range from filters) and the
pre-existing `_time_range_filters` (builds filters from a range) differ by one
word while performing opposite transformations, and both sit in this module's
time-filter flow: `build_query_dict` calls the latter, `execute_tabular_query`
the former. A distinct name such as `_temporal_range_in_filters` would prevent
mix-ups.
</div>
</div>
<small><i>Code Review Run #540efc</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
