You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "msyavuz (via GitHub)" <gi...@apache.org> on 2026/04/24 08:13:20 UTC
[PR] feat(mcp): include applied dashboard filters in get_chart_info [superset]
msyavuz opened a new pull request, #39620:
URL: https://github.com/apache/superset/pull/39620
### SUMMARY
Adds optional `dashboard_id` to the MCP `get_chart_info` tool. When provided, the response's `filters.dashboard_filters` lists dashboard-level native filters in scope for the chart, each with `id`, `name`, `filter_type`, `column`, `operator`, `value`, and `status`. The chart's own query-native filters (adhoc/extra/time_range/where/having) were already returned under `filters` — this fills in the missing dashboard-scoped side so MCP consumers can reason about the filter state a chart is actually rendered with on a dashboard.
Scope resolution reuses the existing helpers in `superset.charts.data.dashboard_filter_context` (`_is_filter_in_scope_for_chart`, `_extract_filter_extra_form_data`, `_get_filter_target_column`) so behavior matches the frontend/back-end rendering path (handles `chartsInScope`, `scope.rootPath`/`excluded`, `defaultToFirstItem`, DIVIDER entries). Permalink-state (user-applied values) is intentionally out of scope for this PR — values come from each filter's default data mask, matching the saved dashboard view.
Error paths: caller without dashboard access → `ChartError(DashboardNotAccessible)`; chart not on the dashboard / dashboard missing → `ChartError(ChartNotOnDashboard)`.
Closes [sc-101887](https://app.shortcut.com/preset/story/101887).
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — MCP tool response shape change only.
**Before** (call with `{"identifier": 123}` on a chart whose dashboard has a `country IN (US)` native filter targeting it):
```json
{"filters": {"adhoc_filters": [...], "time_range": "No filter", "dashboard_filters": []}}
```
**After** (same chart, with `{"identifier": 123, "dashboard_id": 45}`):
```json
{"filters": {"adhoc_filters": [...], "time_range": "No filter",
"dashboard_filters": [
{"id": "NATIVE_FILTER-abc", "name": "Country", "filter_type": "filter_select",
"column": "country", "operator": "IN", "value": ["US"], "status": "applied"}
]}}
```
### TESTING INSTRUCTIONS
Unit tests:
```
pytest tests/unit_tests/mcp_service/chart/tool/test_get_chart_info.py
```
Manual (MCP client):
1. Pick a chart on a dashboard where at least one native filter targets it (e.g. filter on `country` with default `["US"]`).
2. Call `get_chart_info` with just `{"identifier": <chart_id>}` — confirm `filters.dashboard_filters` is empty.
3. Call again with `{"identifier": <chart_id>, "dashboard_id": <dashboard_id>}` — confirm each in-scope filter appears with column/operator/value and `status: "applied"`.
4. Add a filter with `defaultToFirstItem: true` — confirm it shows `status: "not_applied_uses_default_to_first_item_prequery"` and no value.
5. Exclude the chart from a filter's scope — confirm that filter is absent.
6. Pass a `dashboard_id` for a dashboard the chart is not on — expect `ChartError(ChartNotOnDashboard)`.
7. Pass a `dashboard_id` for a dashboard the caller can't access — expect `ChartError(DashboardNotAccessible)`.
### ADDITIONAL INFORMATION
- [x] Has associated issue: [sc-101887](https://app.shortcut.com/preset/story/101887)
- [ ] Required feature flags: No — always-on MCP response enrichment.
- [ ] Changes UI: No — backend/MCP only.
- [ ] Includes DB Migration: No.
- [x] Introduces new feature or API: Yes — adds `dashboard_id` request field and `dashboard_filters` response field to the `get_chart_info` MCP tool (additive; existing callers unaffected).
- [ ] Removes existing feature or API: No.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): include applied dashboard filters in get_chart_info [superset]
Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz merged PR #39620:
URL: https://github.com/apache/superset/pull/39620
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): include applied dashboard filters in get_chart_info [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #39620:
URL: https://github.com/apache/superset/pull/39620#issuecomment-4523943984
**Bito Automatic Review Skipped – PR Already Merged**<br/><br/>Bito scheduled an automatic review for this pull request, but the review was skipped because this PR was merged before the review could be run.<br/>No action is needed if you didn't intend to review it. To get a review, you can type `/review` in a comment and save it
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): include applied dashboard filters in get_chart_info [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #39620:
URL: https://github.com/apache/superset/pull/39620#issuecomment-4311677564
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): include applied dashboard filters in get_chart_info [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #39620:
URL: https://github.com/apache/superset/pull/39620#discussion_r3136366151
##########
tests/unit_tests/mcp_service/chart/tool/test_get_chart_info.py:
##########
@@ -0,0 +1,262 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+"""
+Unit tests for get_chart_info MCP tool: dashboard-filter resolution path.
+"""
+
+from unittest.mock import MagicMock, patch
+
+import pytest
+
+from superset.mcp_service.chart.chart_helpers import (
+ _resolve_filter_operator_and_value,
+ build_applied_dashboard_filters,
+)
+from superset.mcp_service.chart.schemas import GetChartInfoRequest
+
+
+class TestGetChartInfoRequestSchema:
Review Comment:
**Suggestion:** Add a class docstring describing the purpose of this newly introduced test class. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The class has no docstring immediately under the class definition.
If the custom rule requires docstrings for new classes, this is a genuine violation.
</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%20tests%2Funit_tests%2Fmcp_service%2Fchart%2Ftool%2Ftest_get_chart_info.py%0A%2A%2ALine%3A%2A%2A%2033%3A33%0A%2A%2AComment%3A%2A%2A%0A%09%2ACustom%20Rule%3A%20Add%20a%20class%20docstring%20describing%20the%20purpose%20of%20this%20newly%20introduced%20test%20class.%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%20tests%2Funit_tests%2Fmcp_service%2Fchart%2Ftool%2Ftest_get_chart_info.py%0A%2A%2ALine%3A%2A%2A%2033%3A33%0A%2A%2AComment%3A%2A%2A%0A%09%2ACustom%20Rule%3A%20Add%20a%20class%20docstring%20describing%20the%20purpose%20of%20this%20newly%20introduced%20test%20class.%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:** tests/unit_tests/mcp_service/chart/tool/test_get_chart_info.py
**Line:** 33:33
**Comment:**
*Custom Rule: Add a class docstring describing the purpose of this newly introduced test class.
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%2F39620&comment_hash=a1dc2f8b3e82726e704bfba6b0f4c4c2909ab01f2ee6c0ba738eda6839441fc1&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39620&comment_hash=a1dc2f8b3e82726e704bfba6b0f4c4c2909ab01f2ee6c0ba738eda6839441fc1&reaction=dislike'>👎</a>
##########
tests/unit_tests/mcp_service/chart/tool/test_get_chart_info.py:
##########
@@ -0,0 +1,262 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+"""
+Unit tests for get_chart_info MCP tool: dashboard-filter resolution path.
+"""
+
+from unittest.mock import MagicMock, patch
+
+import pytest
+
+from superset.mcp_service.chart.chart_helpers import (
+ _resolve_filter_operator_and_value,
+ build_applied_dashboard_filters,
+)
+from superset.mcp_service.chart.schemas import GetChartInfoRequest
+
+
+class TestGetChartInfoRequestSchema:
+ def test_dashboard_id_optional(self):
+ request = GetChartInfoRequest(identifier=1)
+ assert request.dashboard_id is None
+
+ def test_dashboard_id_accepted(self):
+ request = GetChartInfoRequest(identifier=1, dashboard_id=42)
+ assert request.dashboard_id == 42
+
+
Review Comment:
**Suggestion:** Add a class docstring explaining the behavior covered by this newly added test class. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This newly added test class also lacks a docstring.
The suggestion accurately points out the missing documentation if that is the enforced rule.
</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%20tests%2Funit_tests%2Fmcp_service%2Fchart%2Ftool%2Ftest_get_chart_info.py%0A%2A%2ALine%3A%2A%2A%2042%3A42%0A%2A%2AComment%3A%2A%2A%0A%09%2ACustom%20Rule%3A%20Add%20a%20class%20docstring%20explaining%20the%20behavior%20covered%20by%20this%20newly%20added%20test%20class.%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%20tests%2Funit_tests%2Fmcp_service%2Fchart%2Ftool%2Ftest_get_chart_info.py%0A%2A%2ALine%3A%2A%2A%2042%3A42%0A%2A%2AComment%3A%2A%2A%0A%09%2ACustom%20Rule%3A%20Add%20a%20class%20docstring%20explaining%20the%20behavior%20covered%20by%20this%20newly%20added%20test%20class.%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:** tests/unit_tests/mcp_service/chart/tool/test_get_chart_info.py
**Line:** 42:42
**Comment:**
*Custom Rule: Add a class docstring explaining the behavior covered by this newly added test class.
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%2F39620&comment_hash=c9e322b07bec6968fe6ba190ecc0d0c2aae50261a4b298d4b1caabb3570658fb&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39620&comment_hash=c9e322b07bec6968fe6ba190ecc0d0c2aae50261a4b298d4b1caabb3570658fb&reaction=dislike'>👎</a>
##########
tests/unit_tests/mcp_service/chart/tool/test_get_chart_info.py:
##########
@@ -0,0 +1,262 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+"""
+Unit tests for get_chart_info MCP tool: dashboard-filter resolution path.
+"""
+
+from unittest.mock import MagicMock, patch
+
+import pytest
+
+from superset.mcp_service.chart.chart_helpers import (
+ _resolve_filter_operator_and_value,
+ build_applied_dashboard_filters,
+)
+from superset.mcp_service.chart.schemas import GetChartInfoRequest
+
+
+class TestGetChartInfoRequestSchema:
+ def test_dashboard_id_optional(self):
Review Comment:
**Suggestion:** Add a function docstring describing the test intent for this newly added test function. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This test function has no docstring. If the rule requires docstrings on newly added functions, the violation is real.
</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%20tests%2Funit_tests%2Fmcp_service%2Fchart%2Ftool%2Ftest_get_chart_info.py%0A%2A%2ALine%3A%2A%2A%2034%3A34%0A%2A%2AComment%3A%2A%2A%0A%09%2ACustom%20Rule%3A%20Add%20a%20function%20docstring%20describing%20the%20test%20intent%20for%20this%20newly%20added%20test%20function.%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](http
s://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%20tests%2Funit_tests%2Fmcp_service%2Fchart%2Ftool%2Ftest_get_chart_info.py%0A%2A%2ALine%3A%2A%2A%2034%3A34%0A%2A%2AComment%3A%2A%2A%0A%09%2ACustom%20Rule%3A%20Add%20a%20function%20docstring%20describing%20the%20test%20intent%20for%20this%20newly%20added%20test%20function.%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:** tests/unit_tests/mcp_service/chart/tool/test_get_chart_info.py
**Line:** 34:34
**Comment:**
*Custom Rule: Add a function docstring describing the test intent for this newly added test function.
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%2F39620&comment_hash=a001613cb0e4afcf46a65c25be6220b80cdaf139089ccf156f620a2c7409189e&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39620&comment_hash=a001613cb0e4afcf46a65c25be6220b80cdaf139089ccf156f620a2c7409189e&reaction=dislike'>👎</a>
##########
tests/unit_tests/mcp_service/chart/tool/test_get_chart_info.py:
##########
@@ -0,0 +1,262 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+"""
+Unit tests for get_chart_info MCP tool: dashboard-filter resolution path.
+"""
+
+from unittest.mock import MagicMock, patch
+
+import pytest
+
+from superset.mcp_service.chart.chart_helpers import (
+ _resolve_filter_operator_and_value,
+ build_applied_dashboard_filters,
+)
+from superset.mcp_service.chart.schemas import GetChartInfoRequest
+
+
+class TestGetChartInfoRequestSchema:
+ def test_dashboard_id_optional(self):
+ request = GetChartInfoRequest(identifier=1)
+ assert request.dashboard_id is None
+
+ def test_dashboard_id_accepted(self):
+ request = GetChartInfoRequest(identifier=1, dashboard_id=42)
+ assert request.dashboard_id == 42
+
+
+class TestResolveFilterOperatorAndValue:
+ def test_matches_adhoc_filter_by_subject(self):
+ efd = {
+ "adhoc_filters": [
+ {
+ "subject": "country",
+ "operator": "IN",
+ "comparator": ["US", "CA"],
+ }
+ ]
+ }
+ assert _resolve_filter_operator_and_value(efd, "country") == (
+ "IN",
+ ["US", "CA"],
+ )
+
+ def test_matches_legacy_filter_by_col(self):
+ efd = {"filters": [{"col": "state", "op": "==", "val": "NY"}]}
+ assert _resolve_filter_operator_and_value(efd, "state") == ("==", "NY")
+
+ def test_time_range_when_no_column(self):
+ efd = {"time_range": "Last 7 days"}
+ assert _resolve_filter_operator_and_value(efd, None) == (
+ "TIME_RANGE",
+ "Last 7 days",
+ )
+
+ def test_column_not_in_extra_form_data(self):
+ efd = {
+ "adhoc_filters": [{"subject": "other", "operator": "==", "comparator": 1}]
+ }
+ assert _resolve_filter_operator_and_value(efd, "country") == (None, None)
+
+ def test_none_extra_form_data(self):
+ assert _resolve_filter_operator_and_value(None, "country") == (None, None)
+
+ def test_ignores_non_dict_entries(self):
+ efd = {
+ "adhoc_filters": ["not-a-dict", None],
+ "filters": [42, "foo"],
+ }
+ assert _resolve_filter_operator_and_value(efd, "country") == (None, None)
+
+
+class TestBuildAppliedDashboardFilters:
+ """The helper validates access, checks chart-on-dashboard, iterates
+ native filters, resolves scope, and maps each to AppliedDashboardFilter."""
+
+ def _make_dashboard(self, json_metadata=None, position_json=None, slice_ids=None):
+ dashboard = MagicMock()
+ dashboard.json_metadata = json_metadata or "{}"
+ dashboard.position_json = position_json or "{}"
+ dashboard.slices = [MagicMock(id=sid) for sid in (slice_ids or [])]
+ return dashboard
+
+ def test_chart_not_on_dashboard_raises(self):
+ dashboard = self._make_dashboard(slice_ids=[2, 3])
+ with (
+ patch("superset.db") as mock_db,
+ patch("superset.security_manager"),
+ ):
+ mock_db.session.query.return_value.filter_by.return_value.one_or_none.return_value = dashboard # noqa: E501
+ with pytest.raises(ValueError, match="not on dashboard"):
+ build_applied_dashboard_filters(dashboard_id=10, chart_id=1)
+
+ def test_dashboard_not_found_raises(self):
+ with (
+ patch("superset.db") as mock_db,
+ patch("superset.security_manager"),
+ ):
+ mock_db.session.query.return_value.filter_by.return_value.one_or_none.return_value = None # noqa: E501
+ with pytest.raises(ValueError, match="not found"):
+ build_applied_dashboard_filters(dashboard_id=10, chart_id=1)
+
+ def test_in_scope_filter_with_static_default(self):
+ native_filter = {
+ "id": "NATIVE_FILTER-1",
+ "name": "Country",
+ "type": "NATIVE_FILTER",
+ "filterType": "filter_select",
+ "chartsInScope": [1],
+ "targets": [{"column": {"name": "country"}, "datasetId": 7}],
+ "defaultDataMask": {
+ "filterState": {"value": ["US"]},
+ "extraFormData": {
+ "adhoc_filters": [
+ {
+ "subject": "country",
+ "operator": "IN",
+ "comparator": ["US"],
+ }
+ ]
+ },
+ },
+ }
+ dashboard = self._make_dashboard(
+ json_metadata='{"native_filter_configuration": %s}'
+ % _json(native_filter_list=[native_filter]),
+ slice_ids=[1],
+ )
+
+ with (
+ patch("superset.db") as mock_db,
+ patch("superset.security_manager"),
+ ):
+ mock_db.session.query.return_value.filter_by.return_value.one_or_none.return_value = dashboard # noqa: E501
+ result = build_applied_dashboard_filters(dashboard_id=10, chart_id=1)
+
+ assert len(result) == 1
+ flt = result[0]
+ assert flt.id == "NATIVE_FILTER-1"
+ assert flt.name == "Country"
+ assert flt.filter_type == "filter_select"
+ assert flt.column == "country"
+ assert flt.operator == "IN"
+ assert flt.value == ["US"]
+ assert flt.status == "applied"
+
+ def test_excluded_chart_filter_skipped(self):
+ native_filter = {
+ "id": "NATIVE_FILTER-1",
+ "name": "Region",
+ "type": "NATIVE_FILTER",
+ "filterType": "filter_select",
+ "chartsInScope": [2, 3], # chart 1 excluded
+ "targets": [{"column": {"name": "region"}, "datasetId": 7}],
+ "defaultDataMask": {
+ "filterState": {"value": ["NA"]},
+ "extraFormData": {
+ "filters": [{"col": "region", "op": "==", "val": "NA"}]
+ },
+ },
+ }
+ dashboard = self._make_dashboard(
+ json_metadata='{"native_filter_configuration": %s}'
+ % _json(native_filter_list=[native_filter]),
+ slice_ids=[1],
+ )
+
+ with (
+ patch("superset.db") as mock_db,
+ patch("superset.security_manager"),
+ ):
+ mock_db.session.query.return_value.filter_by.return_value.one_or_none.return_value = dashboard # noqa: E501
+ result = build_applied_dashboard_filters(dashboard_id=10, chart_id=1)
+
+ assert result == []
+
+ def test_default_to_first_item_marks_prequery(self):
+ native_filter = {
+ "id": "NATIVE_FILTER-1",
+ "name": "Region",
+ "type": "NATIVE_FILTER",
+ "filterType": "filter_select",
+ "chartsInScope": [1],
+ "targets": [{"column": {"name": "region"}, "datasetId": 7}],
+ "controlValues": {"defaultToFirstItem": True},
+ "defaultDataMask": {},
+ }
+ dashboard = self._make_dashboard(
+ json_metadata='{"native_filter_configuration": %s}'
+ % _json(native_filter_list=[native_filter]),
+ slice_ids=[1],
+ )
+
+ with (
+ patch("superset.db") as mock_db,
+ patch("superset.security_manager"),
+ ):
+ mock_db.session.query.return_value.filter_by.return_value.one_or_none.return_value = dashboard # noqa: E501
+ result = build_applied_dashboard_filters(dashboard_id=10, chart_id=1)
+
+ assert len(result) == 1
+ assert result[0].status == "not_applied_uses_default_to_first_item_prequery"
+ assert result[0].operator is None
+ assert result[0].value is None
+
+ def test_divider_entry_skipped(self):
+ divider = {
+ "id": "DIVIDER-1",
+ "name": "Section header",
+ "type": "DIVIDER",
+ }
+ dashboard = self._make_dashboard(
+ json_metadata='{"native_filter_configuration": %s}'
+ % _json(native_filter_list=[divider]),
+ slice_ids=[1],
+ )
+
+ with (
+ patch("superset.db") as mock_db,
+ patch("superset.security_manager"),
+ ):
+ mock_db.session.query.return_value.filter_by.return_value.one_or_none.return_value = dashboard # noqa: E501
+ result = build_applied_dashboard_filters(dashboard_id=10, chart_id=1)
+
+ assert result == []
+
+ def test_no_native_filters_returns_empty_list(self):
+ dashboard = self._make_dashboard(
+ json_metadata="{}",
+ slice_ids=[1],
+ )
+
+ with (
+ patch("superset.db") as mock_db,
+ patch("superset.security_manager"),
+ ):
+ mock_db.session.query.return_value.filter_by.return_value.one_or_none.return_value = dashboard # noqa: E501
+ result = build_applied_dashboard_filters(dashboard_id=10, chart_id=1)
+
+ assert result == []
+
+
+def _json(native_filter_list):
Review Comment:
**Suggestion:** Add explicit type annotations to this new module-level helper function, including both the input parameter type and return type. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This new module-level helper has an untyped parameter and no return annotation.
That matches the suggested typing issue, so the violation is present in the file.
</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%20tests%2Funit_tests%2Fmcp_service%2Fchart%2Ftool%2Ftest_get_chart_info.py%0A%2A%2ALine%3A%2A%2A%20257%3A257%0A%2A%2AComment%3A%2A%2A%0A%09%2ACustom%20Rule%3A%20Add%20explicit%20type%20annotations%20to%20this%20new%20module-level%20helper%20function%2C%20including%20both%20the%20input%20parameter%20type%20and%20return%20type.%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%20implemen
t%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%20tests%2Funit_tests%2Fmcp_service%2Fchart%2Ftool%2Ftest_get_chart_info.py%0A%2A%2ALine%3A%2A%2A%20257%3A257%0A%2A%2AComment%3A%2A%2A%0A%09%2ACustom%20Rule%3A%20Add%20explicit%20type%20annotations%20to%20this%20new%20module-level%20helper%20function%2C%20including%20both%20the%20input%20parameter%20type%20and%20return%20type.%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%20valid
ate%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:** tests/unit_tests/mcp_service/chart/tool/test_get_chart_info.py
**Line:** 257:257
**Comment:**
*Custom Rule: Add explicit type annotations to this new module-level helper function, including both the input parameter type and return type.
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%2F39620&comment_hash=7b7359f36db380811766ac9313f74d5e94afde2d32d45f7e63a18a06b719a27f&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39620&comment_hash=7b7359f36db380811766ac9313f74d5e94afde2d32d45f7e63a18a06b719a27f&reaction=dislike'>👎</a>
##########
tests/unit_tests/mcp_service/chart/tool/test_get_chart_info.py:
##########
@@ -0,0 +1,262 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+"""
+Unit tests for get_chart_info MCP tool: dashboard-filter resolution path.
+"""
+
+from unittest.mock import MagicMock, patch
+
+import pytest
+
+from superset.mcp_service.chart.chart_helpers import (
+ _resolve_filter_operator_and_value,
+ build_applied_dashboard_filters,
+)
+from superset.mcp_service.chart.schemas import GetChartInfoRequest
+
+
+class TestGetChartInfoRequestSchema:
+ def test_dashboard_id_optional(self):
+ request = GetChartInfoRequest(identifier=1)
+ assert request.dashboard_id is None
+
+ def test_dashboard_id_accepted(self):
+ request = GetChartInfoRequest(identifier=1, dashboard_id=42)
+ assert request.dashboard_id == 42
+
+
+class TestResolveFilterOperatorAndValue:
+ def test_matches_adhoc_filter_by_subject(self):
+ efd = {
+ "adhoc_filters": [
+ {
+ "subject": "country",
+ "operator": "IN",
+ "comparator": ["US", "CA"],
+ }
+ ]
+ }
+ assert _resolve_filter_operator_and_value(efd, "country") == (
+ "IN",
+ ["US", "CA"],
+ )
+
+ def test_matches_legacy_filter_by_col(self):
+ efd = {"filters": [{"col": "state", "op": "==", "val": "NY"}]}
+ assert _resolve_filter_operator_and_value(efd, "state") == ("==", "NY")
+
+ def test_time_range_when_no_column(self):
+ efd = {"time_range": "Last 7 days"}
+ assert _resolve_filter_operator_and_value(efd, None) == (
+ "TIME_RANGE",
+ "Last 7 days",
+ )
+
+ def test_column_not_in_extra_form_data(self):
+ efd = {
+ "adhoc_filters": [{"subject": "other", "operator": "==", "comparator": 1}]
+ }
+ assert _resolve_filter_operator_and_value(efd, "country") == (None, None)
+
+ def test_none_extra_form_data(self):
+ assert _resolve_filter_operator_and_value(None, "country") == (None, None)
+
+ def test_ignores_non_dict_entries(self):
+ efd = {
+ "adhoc_filters": ["not-a-dict", None],
+ "filters": [42, "foo"],
+ }
+ assert _resolve_filter_operator_and_value(efd, "country") == (None, None)
+
+
+class TestBuildAppliedDashboardFilters:
+ """The helper validates access, checks chart-on-dashboard, iterates
+ native filters, resolves scope, and maps each to AppliedDashboardFilter."""
+
+ def _make_dashboard(self, json_metadata=None, position_json=None, slice_ids=None):
Review Comment:
**Suggestion:** Add explicit type annotations for all parameters and the return type of this helper method to satisfy typing requirements for newly added methods. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The helper is newly added and currently has no type annotations for its parameters or return type.
If the rule requires explicit typing for new helpers, this is a real violation.
</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%20tests%2Funit_tests%2Fmcp_service%2Fchart%2Ftool%2Ftest_get_chart_info.py%0A%2A%2ALine%3A%2A%2A%2091%3A91%0A%2A%2AComment%3A%2A%2A%0A%09%2ACustom%20Rule%3A%20Add%20explicit%20type%20annotations%20for%20all%20parameters%20and%20the%20return%20type%20of%20this%20helper%20method%20to%20satisfy%20typing%20requirements%20for%20newly%20added%20methods.%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%20correc
tness%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%20tests%2Funit_tests%2Fmcp_service%2Fchart%2Ftool%2Ftest_get_chart_info.py%0A%2A%2ALine%3A%2A%2A%2091%3A91%0A%2A%2AComment%3A%2A%2A%0A%09%2ACustom%20Rule%3A%20Add%20explicit%20type%20annotations%20for%20all%20parameters%20and%20the%20return%20type%20of%20this%20helper%20method%20to%20satisfy%20typing%20requirements%20for%20newly%20added%20methods.%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%2
0then%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:** tests/unit_tests/mcp_service/chart/tool/test_get_chart_info.py
**Line:** 91:91
**Comment:**
*Custom Rule: Add explicit type annotations for all parameters and the return type of this helper method to satisfy typing requirements for newly added methods.
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%2F39620&comment_hash=d22a4acb9d16a1c6d4e87ad17eb66300c740de348fbfea589aff928b42249f1e&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39620&comment_hash=d22a4acb9d16a1c6d4e87ad17eb66300c740de348fbfea589aff928b42249f1e&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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): include applied dashboard filters in get_chart_info [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #39620:
URL: https://github.com/apache/superset/pull/39620#issuecomment-4311692582
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | d2cf7c9e59ea92b7f91e37ba55cfe10b4e58110e |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/69eb26203574f70008726c73 |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-39620--superset-docs-preview.netlify.app](https://deploy-preview-39620--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">📱</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-39620--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): include applied dashboard filters in get_chart_info [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #39620:
URL: https://github.com/apache/superset/pull/39620#issuecomment-4311728877
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/39620?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `17.39130%` with `76 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 63.70%. Comparing base ([`72d39be`](https://app.codecov.io/gh/apache/superset/commit/72d39bea857b929246cd9dd74f8480c173da0cd4?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`d2cf7c9`](https://app.codecov.io/gh/apache/superset/commit/d2cf7c9e59ea92b7f91e37ba55cfe10b4e58110e?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 28 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/39620?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [superset/mcp\_service/chart/chart\_helpers.py](https://app.codecov.io/gh/apache/superset/pull/39620?src=pr&el=tree&filepath=superset%2Fmcp_service%2Fchart%2Fchart_helpers.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvbWNwX3NlcnZpY2UvY2hhcnQvY2hhcnRfaGVscGVycy5weQ==) | 8.62% | [53 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/39620?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/mcp\_service/chart/tool/get\_chart\_info.py](https://app.codecov.io/gh/apache/superset/pull/39620?src=pr&el=tree&filepath=superset%2Fmcp_service%2Fchart%2Ftool%2Fget_chart_info.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvbWNwX3NlcnZpY2UvY2hhcnQvdG9vbC9nZXRfY2hhcnRfaW5mby5weQ==) | 13.04% | [20 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/39620?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/mcp\_service/chart/schemas.py](https://app.codecov.io/gh/apache/superset/pull/39620?src=pr&el=tree&filepath=superset%2Fmcp_service%2Fchart%2Fschemas.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvbWNwX3NlcnZpY2UvY2hhcnQvc2NoZW1hcy5weQ==) | 72.72% | [3 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/39620?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #39620 +/- ##
==========================================
- Coverage 64.57% 63.70% -0.88%
==========================================
Files 2564 2565 +1
Lines 133558 133752 +194
Branches 31031 31077 +46
==========================================
- Hits 86250 85201 -1049
- Misses 45816 47041 +1225
- Partials 1492 1510 +18
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/39620/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/39620/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `39.79% <17.39%> (-0.09%)` | :arrow_down: |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/39620/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/39620/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/39620/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `41.57% <17.39%> (-0.09%)` | :arrow_down: |
| [python](https://app.codecov.io/gh/apache/superset/pull/39620/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `60.14% <17.39%> (-1.96%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/39620/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `60.00% <17.39%> (-0.15%)` | :arrow_down: |
| [unit](https://app.codecov.io/gh/apache/superset/pull/39620/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/superset/pull/39620?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): include applied dashboard filters in get_chart_info [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #39620:
URL: https://github.com/apache/superset/pull/39620#discussion_r3136383568
##########
superset/mcp_service/chart/tool/get_chart_info.py:
##########
@@ -74,6 +79,34 @@ def _build_unsaved_chart_info(form_data_key: str) -> ChartInfo | ChartError:
)
+async def _attach_dashboard_filters(
+ result: ChartInfo, dashboard_id: int, ctx: Context
+) -> ChartError | None:
+ """Resolve dashboard-scoped native filters and attach them to result.filters.
+
+ Returns a ChartError to surface to the caller on validation / access
+ failures, or None on success (including the no-filters case).
+ """
+ if not result.id:
+ return None
+ with event_logger.log_context(action="mcp.get_chart_info.dashboard_filters"):
+ try:
+ dashboard_filters = build_applied_dashboard_filters(dashboard_id, result.id)
+ except ValueError as exc:
+ await ctx.warning("Chart not on dashboard: %s" % (str(exc),))
+ return ChartError(error=str(exc), error_type="ChartNotOnDashboard")
+ except SupersetSecurityException as exc:
Review Comment:
**🟠 Architect Review — HIGH**
All ValueError cases from build_applied_dashboard_filters (including "Dashboard {id} not found" and "Chart {id} is not on dashboard {id}") are caught and returned as ChartError(error_type="ChartNotOnDashboard"), so a missing dashboard is incorrectly surfaced as a "ChartNotOnDashboard" error.
**Suggestion:** Inspect the ValueError message or raise distinct exception types from build_applied_dashboard_filters so that _attach_dashboard_filters can return different ChartError.error_type values for "dashboard not found" (e.g. "DashboardNotFound") versus "chart not on dashboard" ("ChartNotOnDashboard"), and adjust the log messages accordingly.
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt=This%20is%20an%20%2A%2AArchitect%20%2F%20Logical%20Review%2A%2A%20comment%20left%20during%20a%20code%20review.%20These%20reviews%20are%20first-class%2C%20important%20findings%20%E2%80%94%20not%20optional%20suggestions.%20Do%20NOT%20dismiss%20this%20as%20a%20%27big%20architectural%20change%27%20just%20because%20the%20title%20says%20architect%20review%3B%20most%20of%20these%20can%20be%20resolved%20with%20a%20small%2C%20localized%20fix%20once%20the%20intent%20is%20understood.%0A%0A%2A%2APath%3A%2A%2A%20superset%2Fmcp_service%2Fchart%2Ftool%2Fget_chart_info.py%0A%2A%2ALine%3A%2A%2A%2095%3A98%0A%2A%2AComment%3A%2A%2A%0A%09%2AHIGH%3A%20All%20ValueError%20cases%20from%20build_applied_dashboard_filters%20%28including%20%22Dashboard%20%7Bid%7D%20not%20found%22%20and%20%22Chart%20%7Bid%7D%20is%20not%20on%20dashboard%20%7Bid%7D%22%29%20are%20caught%20and%20returned%20as%20ChartError%28error_type%3D%22ChartNotOnDashboard%22%
29%2C%20so%20a%20missing%20dashboard%20is%20incorrectly%20surfaced%20as%20a%20%22ChartNotOnDashboard%22%20error.%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.%0AIf%20a%20suggested%20approach%20is%20provided%20above%2C%20use%20it%20as%20the%20authoritative%20instruction.%20If%20no%20explicit%20code%20suggestion%20is%20given%2C%20you%20MUST%20still%20draft%20and%20apply%20your%20own%20minimal%2C%20localized%20fix%20%E2%80%94%20do%20not%20punt%20back%20with%20%27no%20suggestion%20provided%2C%20review%20manually%27.%20Keep%20the%20change%20as%20small%20as%20possible%3A%20add%20a%20guard%20clause%2C%20gate%20on%20a%20loading%20state%2C%20reorder%20an%20await%2C%20wrap%20in%20a%20conditional%2C%20etc.%20Do%20not%20refactor%20surrounding%20code%20or%20expand%20scope%20beyond%20the%20finding.%0AOnce%20fix%20is%20implemented%2C%20als
o%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%20an%20%2A%2AArchitect%20%2F%20Logical%20Review%2A%2A%20comment%20left%20during%20a%20code%20review.%20These%20reviews%20are%20first-class%2C%20important%20findings%20%E2%80%94%20not%20optional%20suggestions.%20Do%20NOT%20dismiss%20this%20as%20a%20%27big%20architectural%20change%27%20just%20because%20the%20title%20says%20architect%20review%3B%20most%20of%20these%20can%20be%20resolved%20with%20a%20small%2C%20localized%20fix%20once%20the%20intent%20is%20understood.%0A%0A%2A%2APath%3A%2A%2A%20superset%2Fmcp_service%2Fchart%2Ftool%2Fget_chart_info.py%0A%2A%2ALine%3A%2A%2A%2095%3A98%0A%2A%2AComment%3A%2A%2A%0
A%09%2AHIGH%3A%20All%20ValueError%20cases%20from%20build_applied_dashboard_filters%20%28including%20%22Dashboard%20%7Bid%7D%20not%20found%22%20and%20%22Chart%20%7Bid%7D%20is%20not%20on%20dashboard%20%7Bid%7D%22%29%20are%20caught%20and%20returned%20as%20ChartError%28error_type%3D%22ChartNotOnDashboard%22%29%2C%20so%20a%20missing%20dashboard%20is%20incorrectly%20surfaced%20as%20a%20%22ChartNotOnDashboard%22%20error.%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.%0AIf%20a%20suggested%20approach%20is%20provided%20above%2C%20use%20it%20as%20the%20authoritative%20instruction.%20If%20no%20explicit%20code%20suggestion%20is%20given%2C%20you%20MUST%20still%20draft%20and%20apply%20your%20own%20minimal%2C%20localized%20fix%20%E2%80%94%20do%20not%20punt%20back%20with%20%27no%20suggestion%20provided%2C%20review%20manually%27.%20Keep%20the%
20change%20as%20small%20as%20possible%3A%20add%20a%20guard%20clause%2C%20gate%20on%20a%20loading%20state%2C%20reorder%20an%20await%2C%20wrap%20in%20a%20conditional%2C%20etc.%20Do%20not%20refactor%20surrounding%20code%20or%20expand%20scope%20beyond%20the%20finding.%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 an **Architect / Logical Review** comment left during a code review. These reviews are first-class, important findings — not optional suggestions. Do NOT dismiss this as a 'big architectural change' just because the title says architect review; most of these can be resolved with a small, localized fix once the intent is understood.
**Path:** superset/mcp_service/chart/tool/get_chart_info.py
**Line:** 95:98
**Comment:**
*HIGH: All ValueError cases from build_applied_dashboard_filters (including "Dashboard {id} not found" and "Chart {id} is not on dashboard {id}") are caught and returned as ChartError(error_type="ChartNotOnDashboard"), so a missing dashboard is incorrectly surfaced as a "ChartNotOnDashboard" error.
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.
If a suggested approach is provided above, use it as the authoritative instruction. If no explicit code suggestion is given, you MUST still draft and apply your own minimal, localized fix — do not punt back with 'no suggestion provided, review manually'. Keep the change as small as possible: add a guard clause, gate on a loading state, reorder an await, wrap in a conditional, etc. Do not refactor surrounding code or expand scope beyond the finding.
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>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): include applied dashboard filters in get_chart_info [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #39620:
URL: https://github.com/apache/superset/pull/39620#discussion_r3136386982
##########
superset/mcp_service/chart/tool/get_chart_info.py:
##########
@@ -74,6 +79,34 @@ def _build_unsaved_chart_info(form_data_key: str) -> ChartInfo | ChartError:
)
+async def _attach_dashboard_filters(
+ result: ChartInfo, dashboard_id: int, ctx: Context
+) -> ChartError | None:
+ """Resolve dashboard-scoped native filters and attach them to result.filters.
+
+ Returns a ChartError to surface to the caller on validation / access
+ failures, or None on success (including the no-filters case).
+ """
+ if not result.id:
+ return None
+ with event_logger.log_context(action="mcp.get_chart_info.dashboard_filters"):
+ try:
+ dashboard_filters = build_applied_dashboard_filters(dashboard_id, result.id)
+ except ValueError as exc:
+ await ctx.warning("Chart not on dashboard: %s" % (str(exc),))
+ return ChartError(error=str(exc), error_type="ChartNotOnDashboard")
Review Comment:
**Suggestion:** All `ValueError` exceptions are mapped to `ChartNotOnDashboard`, but `build_applied_dashboard_filters` also raises `ValueError` for a missing dashboard, so callers get the wrong error type and misleading warning text. Handle dashboard-not-found separately so clients can distinguish missing dashboard vs chart-scope mismatch. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ get_chart_info mislabels missing dashboards as ChartNotOnDashboard.
- ⚠️ MCP clients cannot distinguish dashboard-missing vs chart-scope errors.
- ⚠️ Warning text conflicts with underlying ValueError message.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Call the MCP tool `get_chart_info(request, ctx)`
(`superset/mcp_service/chart/tool/get_chart_info.py:148-150`) with a valid chart
identifier and a `dashboard_id` that does not exist in the database (for example, 99999).
2. Inside `get_chart_info`, after successfully loading and validating the chart
(`superset/mcp_service/chart/tool/get_chart_info.py:219-272`), the `if
request.dashboard_id:` block at
`superset/mcp_service/chart/tool/get_chart_info.py:274-277` calls
`_attach_dashboard_filters(result, request.dashboard_id, ctx)`.
3. `_attach_dashboard_filters` in
`superset/mcp_service/chart/tool/get_chart_info.py:82-107` calls
`build_applied_dashboard_filters(dashboard_id, result.id)` at line 94, which is
implemented in `superset/mcp_service/chart/chart_helpers.py:7-19,31-40`; when the
dashboard is missing, `build_applied_dashboard_filters` raises `ValueError(f"Dashboard
{dashboard_id} not found")` at `chart_helpers.py:31-33`.
4. That `ValueError` is caught by `except ValueError as exc:` at
`get_chart_info.py:95-97`, which logs a warning `"Chart not on dashboard: %s"` and returns
`ChartError(error=str(exc), error_type="ChartNotOnDashboard")`, conflating "dashboard not
found" with "chart is not on dashboard" despite the helper's documented distinction in
`chart_helpers.py:12-18`; clients cannot distinguish a missing dashboard from a
chart-scoping problem.
```
</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%2Ftool%2Fget_chart_info.py%0A%2A%2ALine%3A%2A%2A%2095%3A97%0A%2A%2AComment%3A%2A%2A%0A%09%2ALogic%20Error%3A%20All%20%60ValueError%60%20exceptions%20are%20mapped%20to%20%60ChartNotOnDashboard%60%2C%20but%20%60build_applied_dashboard_filters%60%20also%20raises%20%60ValueError%60%20for%20a%20missing%20dashboard%2C%20so%20callers%20get%20the%20wrong%20error%20type%20and%20misleading%20warning%20text.%20Handle%20dashboard-not-found%20separately%20so%20clients%20can%20distinguish%20missing%20dashboard%20vs%20chart-scope%20mismatch.%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%20comment
s%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%2Ftool%2Fget_chart_info.py%0A%2A%2ALine%3A%2A%2A%2095%3A97%0A%2A%2AComment%3A%2A%2A%0A%09%2ALogic%20Error%3A%20All%20%60ValueError%60%20exceptions%20are%20mapped%20to%20%60ChartNotOnDashboard%60%2C%20but%20%60build_applied_dashboard_filters%60%20also%20raises%20%60ValueError%60%20for%20a%20missing%20dashboard%2C%20so%20callers%20get%20the%20wrong%20error%20type%20and%20misleading%20warning%20text.%20Handle%20dashboard-not-found%20separately%20so%20clients%20can%20distinguish%20missing%20dashboard%20vs%20chart-scope%20mismat
ch.%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/tool/get_chart_info.py
**Line:** 95:97
**Comment:**
*Logic Error: All `ValueError` exceptions are mapped to `ChartNotOnDashboard`, but `build_applied_dashboard_filters` also raises `ValueError` for a missing dashboard, so callers get the wrong error type and misleading warning text. Handle dashboard-not-found separately so clients can distinguish missing dashboard vs chart-scope mismatch.
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%2F39620&comment_hash=54e4db7569a1c27747d40e73efa8a0d3d5084599133de8982a3f44f083c421e6&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39620&comment_hash=54e4db7569a1c27747d40e73efa8a0d3d5084599133de8982a3f44f083c421e6&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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): include applied dashboard filters in get_chart_info [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #39620:
URL: https://github.com/apache/superset/pull/39620#issuecomment-4312107387
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): include applied dashboard filters in get_chart_info [superset]
Posted by "alexandrusoare (via GitHub)" <gi...@apache.org>.
alexandrusoare commented on code in PR #39620:
URL: https://github.com/apache/superset/pull/39620#discussion_r3137112188
##########
superset/mcp_service/chart/chart_helpers.py:
##########
@@ -79,3 +84,118 @@ def extract_form_data_key_from_url(url: str | None) -> str | None:
parsed = urlparse(url)
values = parse_qs(parsed.query).get("form_data_key", [])
return values[0] if values else None
+
+
+def _match_adhoc_by_subject(
+ adhoc_filters: Any, column: str | None
+) -> tuple[str | None, Any] | None:
+ if not column or not isinstance(adhoc_filters, list):
+ return None
+ for af in adhoc_filters:
+ if isinstance(af, dict) and af.get("subject") == column:
+ return af.get("operator"), af.get("comparator")
+ return None
+
+
+def _match_legacy_by_col(
+ legacy_filters: Any, column: str | None
+) -> tuple[str | None, Any] | None:
+ if not column or not isinstance(legacy_filters, list):
+ return None
+ for f in legacy_filters:
+ if isinstance(f, dict) and f.get("col") == column:
+ return f.get("op"), f.get("val")
+ return None
+
+
+def _resolve_filter_operator_and_value(
+ extra_form_data: dict[str, Any] | None,
+ column: str | None,
+) -> tuple[str | None, Any]:
+ """Pull operator and value for a dashboard filter from its
+ default extra_form_data, matching on target column where applicable."""
+ if not extra_form_data:
+ return None, None
+
+ if match := _match_adhoc_by_subject(extra_form_data.get("adhoc_filters"), column):
+ return match
+ if match := _match_legacy_by_col(extra_form_data.get("filters"), column):
+ return match
+ # Temporal filters contribute time_range with no target column
+ if time_range := extra_form_data.get("time_range"):
+ return "TIME_RANGE", time_range
+ return None, None
+
+
+def build_applied_dashboard_filters(
+ dashboard_id: int, chart_id: int
+) -> list[AppliedDashboardFilter]:
+ """Resolve dashboard-level native filters in scope for a chart.
+
+ Validates that the dashboard exists, the caller has access, and the chart
+ is on the dashboard. Returns one AppliedDashboardFilter per non-DIVIDER
+ native filter whose scope includes the chart, populated with the filter's
+ default operator and value.
+
+ Raises DashboardNotFoundError if the dashboard is missing,
+ ChartNotOnDashboardError if the chart is not on it, and
+ SupersetSecurityException if the caller cannot access the dashboard.
+ """
+ # Local imports avoid circular deps at module load
+ from superset import db, security_manager
+ from superset.charts.data.dashboard_filter_context import (
+ _extract_filter_extra_form_data,
+ _get_filter_target_column,
+ _is_filter_in_scope_for_chart,
+ )
+ from superset.commands.dashboard.exceptions import DashboardNotFoundError
+ from superset.mcp_service.chart.schemas import AppliedDashboardFilter
+ from superset.models.dashboard import Dashboard
+ from superset.utils import json
Review Comment:
I see that we import a lot at function level, do we need all of them, or can we move some of them to the module level?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org