bito-code-review[bot] commented on code in PR #43176:
URL: https://github.com/apache/superset/pull/43176#discussion_r4068195260
##########
tests/unit_tests/mcp_service/chart/test_compile.py:
##########
@@ -766,3 +771,59 @@ def test_aggregation_ambiguity_returns_validation_errors()
-> None:
)
assert len(errors) == 1
assert errors[0].error_code == "AMBIGUOUS_DATASET_REFERENCE"
+
+
+def test_compile_chart_exposes_jinja_context() -> None:
+ """Compile checks publish the same Jinja inputs used during chart
execution."""
+ from flask import current_app
+
+ from superset.common.query_object import QueryObject
+ from superset.mcp_service.chart.compile import _compile_chart
+ from tests.unit_tests.charts.data.form_data_test import (
+ assert_request_dependent_jinja_macros,
+ )
+
+ query = QueryObject(
+ filters=[{"col": "region", "op": "IN", "val": ["North"]}],
+ time_range="Last week",
+ )
+ query_context = SimpleNamespace(
+ queries=[query],
+ form_data={"url_params": {"tenant": "acme"}},
+ )
+ observed: dict[str, bool] = {}
+
+ class ChartDataCommand:
+ def __init__(self, qc: object) -> None:
+ self.query_context = qc
+
+ def validate(self) -> None:
+ pass
+
+ def run(self) -> dict[str, object]:
+ assert_request_dependent_jinja_macros()
+ observed["ran"] = True
+ return {"queries": [{"data": [{"region": "North"}]}]}
+
+ with (
+ current_app.test_request_context(),
+ patch(
+ "superset.common.query_context_factory.QueryContextFactory.create",
+ return_value=query_context,
+ ),
+ patch(
+ "superset.commands.chart.data.get_data_command.ChartDataCommand",
+ ChartDataCommand,
+ ),
+ ):
+ result = _compile_chart(
+ {
+ "metrics": ["count"],
+ "url_params": {"tenant": "acme"},
+ "time_range": "Last week",
+ },
+ dataset_id=7,
+ )
+
+ assert result.success is True
+ assert observed["ran"] is True
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Test asserts helper, not compile wiring</b></div>
<div id="fix">
`run()` only calls `assert_request_dependent_jinja_macros()`, so the test
proves the helper works (already covered by
`test_query_context_form_data_supports_request_dependent_jinja_macros`) but
never asserts what `_compile_chart` published via `set_query_context_form_data`
(e.g. `g.form_data['datasource']`). Asserting the published state would test
this wiring, not re-test the helper.
</div>
</div>
<small><i>Code Review Run #528d96</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/tool/test_get_chart_preview.py:
##########
@@ -556,6 +556,90 @@ def run(self) -> dict[str, Any]:
assert query["filters"] == [{"col": "gender", "op": "==", "val":
"boy"}]
assert "adhoc_filters" not in query
+ def test_table_preview_exposes_jinja_context(
+ self,
+ monkeypatch: pytest.MonkeyPatch,
+ ) -> None:
+ """Chart preview queries expose the same Jinja inputs as
get_chart_data."""
+ from flask import current_app
+
+ from superset.common.query_object import QueryObject
+ from tests.unit_tests.charts.data.form_data_test import (
+ assert_request_dependent_jinja_macros,
+ )
+
+ get_data_command_module = importlib.import_module(
+ "superset.commands.chart.data.get_data_command"
+ )
+ query = QueryObject(
+ filters=[{"col": "region", "op": "IN", "val": ["North"]}],
+ time_range="Last week",
+ columns=["region"],
+ metrics=["count"],
+ )
+ query_context = SimpleNamespace(
+ queries=[query],
+ form_data={"url_params": {"tenant": "acme"}},
+ )
+ observed: dict[str, bool] = {}
+
+ class ChartDataCommand:
+ def __init__(self, qc: object) -> None:
+ self.query_context = qc
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Cryptic stub param qc</b></div>
<div id="fix">
The new test's `ChartDataCommand.__init__` names its parameter `qc`, while
the five sibling stubs of the same class in this file (e.g. in
`test_table_preview_converts_saved_adhoc_filters_to_query_filters` and
`test_table_preview_uses_singular_metric`) all use `query_context`. The
two-letter abbreviation is cryptic and breaks naming consistency across the
duplicated stub pattern; renaming keeps the family uniform.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
def __init__(self, query_context: object) -> None:
self.query_context = query_context
````
</div>
</details>
</div>
<small><i>Code Review Run #528d96</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/tool/test_get_chart_preview.py:
##########
@@ -556,6 +556,90 @@ def run(self) -> dict[str, Any]:
assert query["filters"] == [{"col": "gender", "op": "==", "val":
"boy"}]
assert "adhoc_filters" not in query
+ def test_table_preview_exposes_jinja_context(
+ self,
+ monkeypatch: pytest.MonkeyPatch,
+ ) -> None:
+ """Chart preview queries expose the same Jinja inputs as
get_chart_data."""
+ from flask import current_app
+
+ from superset.common.query_object import QueryObject
+ from tests.unit_tests.charts.data.form_data_test import (
+ assert_request_dependent_jinja_macros,
+ )
+
+ get_data_command_module = importlib.import_module(
+ "superset.commands.chart.data.get_data_command"
+ )
+ query = QueryObject(
+ filters=[{"col": "region", "op": "IN", "val": ["North"]}],
+ time_range="Last week",
+ columns=["region"],
+ metrics=["count"],
+ )
+ query_context = SimpleNamespace(
+ queries=[query],
+ form_data={"url_params": {"tenant": "acme"}},
+ )
+ observed: dict[str, bool] = {}
+
+ class ChartDataCommand:
+ def __init__(self, qc: object) -> None:
+ self.query_context = qc
+
+ def validate(self) -> None:
+ pass
+
+ def run(self) -> dict[str, Any]:
+ assert_request_dependent_jinja_macros()
+ observed["ran"] = True
+ return {
+ "queries": [
+ {
+ "data": [{"region": "North"}],
+ "colnames": ["region"],
+ "rowcount": 1,
+ }
+ ]
+ }
+
+ preview_module = importlib.import_module(
+ "superset.mcp_service.chart.tool.get_chart_preview"
+ )
+ monkeypatch.setattr(
+ preview_module,
+ "build_query_context_from_form_data",
+ lambda *args, **kwargs: query_context,
+ )
+ monkeypatch.setattr(
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Untyped lambda in monkeypatch</b></div>
<div id="fix">
The monkeypatch uses an untyped lambda (`lambda *args, **kwargs:
query_context`). BITO rule 13350 asks that untyped lambdas be extracted into
named helper functions with explicit type hints for parameters and return
values. A typed `def _fake_build_query_context(*args: Any, **kwargs: Any) ->
SimpleNamespace` defined above the `monkeypatch.setattr` call satisfies the
rule and documents the stub's contract.
</div>
</div>
<small><i>Code Review Run #528d96</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/charts/data/form_data_test.py:
##########
@@ -15,11 +15,58 @@
# specific language governing permissions and limitations
# under the License.
-from typing import Any
+from types import SimpleNamespace
+from typing import Any, cast
from flask import current_app, g
-from superset.charts.data.form_data import set_form_data
+from superset.charts.data.form_data import (
+ set_form_data,
+ set_query_context_form_data,
+)
+from superset.common.query_object import QueryObject
+from superset.constants import NO_TIME_RANGE
+from superset.jinja_context import ExtraCache, get_dataset_id_from_context
+
+
+def _jinja_query_context(
+ *,
+ filters: list[dict[str, Any]] | None = None,
+ time_range: str = "Last week",
+ url_params: dict[str, str] | None = None,
+) -> SimpleNamespace:
+ """Build a QueryContext-shaped object from a real QueryObject."""
+ query = QueryObject(
+ filters=cast(Any, filters or [{"col": "region", "op": "IN", "val":
["North"]}]),
+ time_range=time_range,
+ )
+ return SimpleNamespace(
+ queries=[query],
+ form_data={"url_params": url_params or {"tenant": "acme"}},
+ )
+
+
+def assert_request_dependent_jinja_macros(
+ *,
+ expected_filter_col: str = "region",
+ expected_filter_val: str = "North",
+ expected_url_param: str | None = "tenant",
+ expected_url_value: str = "acme",
+ expected_time_range: str | None = "Last week",
+ expected_dataset_id: int = 7,
+) -> None:
+ """Assert Jinja macros resolve the same inputs as a chart-data API
request."""
+ extra_cache = ExtraCache()
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing local type annotations</b></div>
<div id="fix">
BITO adaptive rule 13153 requires explicit type annotations on all test-file
locals, even when inferable. The new locals `extra_cache` (line 59),
`query_context` (lines 84, 111, 124, 143), and `query` (lines 98, 120, 135) are
unannotated. Add e.g. `query: QueryObject` and `query_context: SimpleNamespace`
to satisfy the org-mandated typing standard.
</div>
</div>
<small><i>Code Review Run #528d96</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/tool/test_get_chart_preview.py:
##########
@@ -556,6 +556,90 @@ def run(self) -> dict[str, Any]:
assert query["filters"] == [{"col": "gender", "op": "==", "val":
"boy"}]
assert "adhoc_filters" not in query
+ def test_table_preview_exposes_jinja_context(
+ self,
+ monkeypatch: pytest.MonkeyPatch,
+ ) -> None:
+ """Chart preview queries expose the same Jinja inputs as
get_chart_data."""
+ from flask import current_app
+
+ from superset.common.query_object import QueryObject
+ from tests.unit_tests.charts.data.form_data_test import (
+ assert_request_dependent_jinja_macros,
+ )
+
+ get_data_command_module = importlib.import_module(
+ "superset.commands.chart.data.get_data_command"
+ )
+ query = QueryObject(
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing local type annotations</b></div>
<div id="fix">
Several locals in the new test (`query`, `query_context`, `chart`,
`get_data_command_module`, `preview_module`) lack explicit type annotations,
while `observed: dict[str, bool]` on line 584 is annotated. BITO rule 13153
asks for explicit annotations on all test-file locals even when inferable;
annotating these keeps the new test consistent with the rule and with the
annotated `observed`/`captured_query_contexts` variables.
</div>
</div>
<small><i>Code Review Run #528d96</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/tool/test_get_chart_data.py:
##########
@@ -762,6 +762,84 @@ def run(self) -> dict[str, Any]:
assert queries[1]["metrics"] == ["sum__profit"]
assert queries[1]["row_limit"] == 99
+ @pytest.mark.asyncio
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Redundant asyncio marker</b></div>
<div id="fix">
`pytest.ini` sets `asyncio_mode = auto`, so every async test is collected
without a marker; the sibling tests in `TestUnsavedChartDataQueryConstruction`
(e.g. `test_gauge_preserves_sort_order_and_validates_saved_metric_output`)
still carry the decorator, but new code should not copy it. Drop
`@pytest.mark.asyncio` here.
</div>
</div>
<small><i>Code Review Run #528d96</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/tool/test_get_chart_data.py:
##########
@@ -762,6 +762,84 @@ def run(self) -> dict[str, Any]:
assert queries[1]["metrics"] == ["sum__profit"]
assert queries[1]["row_limit"] == 99
+ @pytest.mark.asyncio
+ async def test_form_data_key_path_exposes_jinja_context(
+ self,
+ monkeypatch: pytest.MonkeyPatch,
+ ) -> None:
+ """Unsaved-chart execution publishes the same Jinja inputs as
get_chart_data."""
+ from flask import current_app
+
+ from superset.common.query_object import QueryObject
+ from tests.unit_tests.charts.data.form_data_test import (
+ assert_request_dependent_jinja_macros,
+ )
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Inline imports violate rule 12745</b></div>
<div id="fix">
BITO.md rule 12745 requires module-level imports unless a documented
circular dependency exists. `current_app`, `QueryObject`, and
`assert_request_dependent_jinja_macros` have no circularity here — the file
already imports `superset.mcp_service.chart.tool.get_chart_data` at module
scope (lines 42-49). Move these three imports to the top; the
`importlib.import_module` calls can stay since they support monkeypatching.
</div>
</div>
<small><i>Code Review Run #528d96</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]
