codeant-ai-for-open-source[bot] commented on code in PR #40099:
URL: https://github.com/apache/superset/pull/40099#discussion_r3238422367
##########
superset/mcp_service/chart/chart_helpers.py:
##########
@@ -69,6 +69,141 @@ def get_cached_form_data(form_data_key: str) -> str | None:
return None
+def resolve_datasource_engine(datasource_id: Any, datasource_type: str) -> str:
+ """Return the datasource engine name, or ``"base"`` if it cannot be
resolved."""
+ if not isinstance(datasource_id, (int, str)):
+ return "base"
+ try:
+ from superset.daos.datasource import DatasourceDAO
+ from superset.utils.core import DatasourceType
+
+ datasource = DatasourceDAO.get_datasource(
+ datasource_type=DatasourceType(datasource_type),
+ database_id_or_uuid=datasource_id,
+ )
+ return datasource.database.db_engine_spec.engine
+ except Exception: # noqa: BLE001
+ logger.debug("Could not resolve engine for datasource %s",
datasource_id)
+ return "base"
+
+
+def prepare_form_data_for_query(
+ form_data: dict[str, Any],
+ datasource_id: Any,
+ datasource_type: str,
+ extra_form_data: dict[str, Any] | None = None,
+ datasource_engine: str | None = None,
+) -> None:
+ """Normalize form_data filters before building a QueryObject payload.
+
+ Explore and legacy viz query construction merge dashboard/native filter
payloads
+ and split adhoc filters into the concrete ``filters``/``where``/``having``
+ fields consumed by QueryObject. MCP tools that build query payloads
directly
+ must perform the same normalization before calling QueryContextFactory.
+
+ Mutates ``form_data`` in place.
+ """
+ from superset.utils.core import (
+ convert_legacy_filters_into_adhoc,
+ form_data_to_adhoc,
+ merge_extra_filters,
+ simple_filter_to_adhoc,
+ split_adhoc_filters_into_base_filters,
+ )
+
+ if isinstance(form_data.get("adhoc_filters"), list):
+ adhoc_filters = [
+ *(
+ form_data_to_adhoc(form_data, clause)
+ for clause in ("having", "where")
+ if form_data.get(clause)
+ ),
+ *(
+ simple_filter_to_adhoc(filter_, "where")
+ for filter_ in form_data.get("filters") or []
+ if filter_ is not None
+ ),
+ *form_data["adhoc_filters"],
+ ]
+ form_data["adhoc_filters"] = adhoc_filters
+
+ if extra_form_data:
+ form_data["extra_form_data"] = extra_form_data
Review Comment:
**Suggestion:** This overwrites any `extra_form_data` already present in
cached `form_data` instead of merging it, so existing dashboard/native filter
payload can be silently dropped whenever request-level `extra_form_data` is
provided. Merge request extras into the existing `form_data["extra_form_data"]`
additively so both cached and request filters are preserved before
normalization. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ MCP get_chart_data ignores cached dashboard/native filters with
overrides.
- ⚠️ LLM analyses see data inconsistent with Explore visualization filters.
- ⚠️ Fallback query construction drops extra_form_data-based temporal
restrictions.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In the standard Explore flow, when a chart is rendered with
dashboard/native filters,
the view layer populates `form_data["extra_form_data"]` with those filters,
as seen in
`superset/viz.py:1638-1652` where `_apply_layer_filtering()` copies
`self.form_data["extra_form_data"]` into the per-layer `form_data` dict.
2. That Explore `form_data` (including `extra_form_data` from step 1) is
cached under a
`form_data_key` via the Explore caching mechanism used by
`GetFormDataCommand`, which
`get_cached_form_data()` in
`superset/mcp_service/chart/chart_helpers.py:54-69` later
reads back for MCP tools.
3. An MCP client calls the `get_chart_data` tool at
`superset/mcp_service/chart/tool/get_chart_data.py:99-120` with both
`request.form_data_key` (pointing to cached `form_data` that already has
`extra_form_data`) and a non-empty `request.extra_form_data` containing
additional
request-level filters; the unsaved-state branch at
`get_chart_data.py:243-331` loads
`cached_form_data_dict` and then invokes
`prepare_form_data_for_query(cached_form_data_dict, datasource_id,
datasource_type,
request.extra_form_data)` at lines 326-331.
4. Inside `prepare_form_data_for_query()` in
`superset/mcp_service/chart/chart_helpers.py:90-137`, the check at line 130
(`if
extra_form_data:`) and assignment at line 131 (`form_data["extra_form_data"]
=
extra_form_data`) overwrite any existing `form_data["extra_form_data"]` from
the cached
Explore state before `merge_extra_filters(form_data)` is called, so when the
query context
is built in `get_chart_data.py:333-351` the dashboard/native filters encoded
in the
original `extra_form_data` are silently dropped and only the request-level
`extra_form_data` predicates are applied.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset%2Fmcp_service%2Fchart%2Fchart_helpers.py%0A%2A%2ALine%3A%2A%2A%20130%3A131%0A%2A%2AComment%3A%2A%2A%0A%09%2ALogic%20Error%3A%20This%20overwrites%20any%20%60extra_form_data%60%20already%20present%20in%20cached%20%60form_data%60%20instead%20of%20merging%20it%2C%20so%20existing%20dashboard%2Fnative%20filter%20payload%20can%20be%20silently%20dropped%20whenever%20request-level%20%60extra_form_data%60%20is%20provided.%20Merge%20request%20extras%20into%20the%20existing%20%60form_data%5B%22extra_form_data%22%5D%60%20additively%20so%20both%20cached%20and%20request%20filters%20are%20preserved%20before%20normalization.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce
%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset%2Fmcp_service%2Fchart%2Fchart_helpers.py%0A%2A%2ALine%3A%2A%2A%20130%3A131%0A%2A%2AComment%3A%2A%2A%0A%09%2ALogic%20Error%3A%20This%20overwrites%20any%20%60extra_form_data%60%20already%20present%20in%20cached%20%60form_data%60%20instead%20of%20merging%20it%2C%20so%20existing%20dashboard%2Fnative%20filter%20payload%20can%20be%20silently%20dropped%20whenever%20request-level%20%60extra_form_data%60%20is%20provided.%20Merge%20request%20extras%20into%20the%20existing%20%60form_data%5B%22ext
ra_form_data%22%5D%60%20additively%20so%20both%20cached%20and%20request%20filters%20are%20preserved%20before%20normalization.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/chart/chart_helpers.py
**Line:** 130:131
**Comment:**
*Logic Error: This overwrites any `extra_form_data` already present in
cached `form_data` instead of merging it, so existing dashboard/native filter
payload can be silently dropped whenever request-level `extra_form_data` is
provided. Merge request extras into the existing `form_data["extra_form_data"]`
additively so both cached and request filters are preserved before
normalization.
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
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40099&comment_hash=084f0813a0c53474695c029e998d1d1e7181a4ef9624ca46e9869cea64cbb4c0&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40099&comment_hash=084f0813a0c53474695c029e998d1d1e7181a4ef9624ca46e9869cea64cbb4c0&reaction=dislike'>👎</a>
--
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]
