fix(mcp): eager-load dataset.metrics to prevent Excel export DetachedInstanceError#39483
Conversation
…InstanceError get_chart_data with format=excel could fail with "Parent instance SqlaTable is not bound to a Session; lazy load operation of attribute metrics cannot proceed" when the dataset's metrics relationship was accessed after the request-scoped session detached. Pass a subqueryload(Slice.table -> SqlaTable.metrics) query option on ChartDAO lookup so metrics are materialized up front and later detachment is harmless. Mirrors the eager-loading pattern from #39206. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Code Review Agent Run #2bbc04Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
dosubot
Bot
added
change:backend
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| @pytest.fixture | ||
| def mcp_server(): | ||
| from superset.mcp_service.app import mcp | ||
|
|
||
| return mcp |
There was a problem hiding this comment.
Suggestion: Add an explicit return type annotation to the new fixture function. [custom_rule]
Severity Level: Minor
| @pytest.fixture | |
| def mcp_server(): | |
| from superset.mcp_service.app import mcp | |
| return mcp | |
| def mcp_server() -> Any: |
Why it matters? 🤔
The new fixture is defined without a return annotation, which matches the custom typing rule being enforced here. The improved signature adds -> Any, and Any is already imported at the top of the file, so the change is syntactically valid and executable.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/mcp_service/chart/tool/test_get_chart_data.py
**Line:** 863:867
**Comment:**
*Custom Rule: Add an explicit return type annotation to the new fixture 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| @pytest.fixture | ||
| def mock_auth(): | ||
| """Mock MCP auth so Client.call_tool() doesn't need a real admin user.""" | ||
| from contextlib import contextmanager | ||
| from unittest.mock import Mock, patch | ||
|
|
||
| @contextmanager | ||
| def _noop_log_context(*_args: Any, **_kwargs: Any) -> Any: | ||
| yield lambda **_kw: None | ||
|
|
||
| # Also neutralize event_logger.log_context: the default DBEventLogger | ||
| # would otherwise insert a log row referencing our mock user_id and | ||
| # fail a FK constraint against the real users table. | ||
| with ( | ||
| patch("superset.mcp_service.auth.get_user_from_request") as mock_get_user, | ||
| patch( | ||
| "superset.mcp_service.chart.tool.get_chart_data.event_logger.log_context", | ||
| side_effect=_noop_log_context, | ||
| ), | ||
| ): | ||
| user = Mock() | ||
| user.id = 1 | ||
| user.username = "admin" | ||
| mock_get_user.return_value = user | ||
| yield mock_get_user |
There was a problem hiding this comment.
Suggestion: Add an explicit return type annotation to this new fixture function. [custom_rule]
Severity Level: Minor
| @pytest.fixture | |
| def mock_auth(): | |
| """Mock MCP auth so Client.call_tool() doesn't need a real admin user.""" | |
| from contextlib import contextmanager | |
| from unittest.mock import Mock, patch | |
| @contextmanager | |
| def _noop_log_context(*_args: Any, **_kwargs: Any) -> Any: | |
| yield lambda **_kw: None | |
| # Also neutralize event_logger.log_context: the default DBEventLogger | |
| # would otherwise insert a log row referencing our mock user_id and | |
| # fail a FK constraint against the real users table. | |
| with ( | |
| patch("superset.mcp_service.auth.get_user_from_request") as mock_get_user, | |
| patch( | |
| "superset.mcp_service.chart.tool.get_chart_data.event_logger.log_context", | |
| side_effect=_noop_log_context, | |
| ), | |
| ): | |
| user = Mock() | |
| user.id = 1 | |
| user.username = "admin" | |
| mock_get_user.return_value = user | |
| yield mock_get_user | |
| def mock_auth() -> Any: |
Why it matters? 🤔
The new fixture lacks a return annotation, so the suggestion addresses the stated typing rule. Any is already imported, and annotating the fixture as -> Any is syntactically correct and safe for a pytest fixture.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/mcp_service/chart/tool/test_get_chart_data.py
**Line:** 870:894
**Comment:**
*Custom Rule: Add an explicit return type annotation to this new fixture 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| async def test_numeric_id_lookup_passes_metrics_eager_load( | ||
| self, mcp_server, mock_auth | ||
| ): | ||
| """Integer identifier lookup must eager-load Slice.table.metrics.""" | ||
| from unittest.mock import patch | ||
|
|
||
| from fastmcp import Client | ||
|
|
||
| with patch( | ||
| "superset.daos.chart.ChartDAO.find_by_id", return_value=None | ||
| ) as mock_find: | ||
| async with Client(mcp_server) as client: | ||
| await client.call_tool( | ||
| "get_chart_data", | ||
| {"request": {"identifier": 42, "format": "excel"}}, | ||
| ) | ||
|
|
||
| mock_find.assert_called_once() | ||
| call = mock_find.call_args | ||
| assert call.args == (42,) | ||
| query_options = call.kwargs.get("query_options") | ||
| assert query_options is not None, ( | ||
| "Chart lookup must pass query_options for eager-loading." | ||
| ) | ||
| assert len(query_options) == 1 | ||
| load_path = _extract_metrics_load_path(query_options[0]) | ||
| assert load_path == ["table", "metrics"], ( | ||
| f"Expected subqueryload chain 'table' -> 'metrics', got {load_path}" | ||
| ) | ||
|
|
There was a problem hiding this comment.
Suggestion: Add explicit type hints for fixture parameters and return type in this new async test method. [custom_rule]
Severity Level: Minor
| async def test_numeric_id_lookup_passes_metrics_eager_load( | |
| self, mcp_server, mock_auth | |
| ): | |
| """Integer identifier lookup must eager-load Slice.table.metrics.""" | |
| from unittest.mock import patch | |
| from fastmcp import Client | |
| with patch( | |
| "superset.daos.chart.ChartDAO.find_by_id", return_value=None | |
| ) as mock_find: | |
| async with Client(mcp_server) as client: | |
| await client.call_tool( | |
| "get_chart_data", | |
| {"request": {"identifier": 42, "format": "excel"}}, | |
| ) | |
| mock_find.assert_called_once() | |
| call = mock_find.call_args | |
| assert call.args == (42,) | |
| query_options = call.kwargs.get("query_options") | |
| assert query_options is not None, ( | |
| "Chart lookup must pass query_options for eager-loading." | |
| ) | |
| assert len(query_options) == 1 | |
| load_path = _extract_metrics_load_path(query_options[0]) | |
| assert load_path == ["table", "metrics"], ( | |
| f"Expected subqueryload chain 'table' -> 'metrics', got {load_path}" | |
| ) | |
| self, mcp_server: Any, mock_auth: Any | |
| ) -> None: |
Why it matters? 🤔
The method currently leaves the injected fixture parameters untyped and omits a return annotation, which is consistent with the typing rule being targeted. The proposed annotations use Any, which is already available in the module, and -> None is correct for an async test method that does not return a value.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/mcp_service/chart/tool/test_get_chart_data.py
**Line:** 917:946
**Comment:**
*Custom Rule: Add explicit type hints for fixture parameters and return type in this new async test method.
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| @pytest.mark.asyncio | ||
| async def test_uuid_lookup_passes_metrics_eager_load(self, mcp_server, mock_auth): | ||
| """UUID identifier lookup must also eager-load Slice.table.metrics.""" | ||
| from unittest.mock import patch | ||
|
|
||
| from fastmcp import Client | ||
|
|
||
| uuid = "a1b2c3d4-5678-90ab-cdef-1234567890ab" | ||
|
|
||
| with patch( | ||
| "superset.daos.chart.ChartDAO.find_by_id", return_value=None | ||
| ) as mock_find: | ||
| async with Client(mcp_server) as client: | ||
| await client.call_tool( | ||
| "get_chart_data", | ||
| {"request": {"identifier": uuid, "format": "excel"}}, | ||
| ) | ||
|
|
||
| mock_find.assert_called_once() | ||
| call = mock_find.call_args | ||
| assert call.args == (uuid,) | ||
| assert call.kwargs.get("id_column") == "uuid" | ||
| query_options = call.kwargs.get("query_options") | ||
| assert query_options is not None, ( | ||
| "UUID chart lookup must pass query_options for eager-loading." | ||
| ) | ||
| load_path = _extract_metrics_load_path(query_options[0]) | ||
| assert load_path == ["table", "metrics"], ( | ||
| f"Expected subqueryload chain 'table' -> 'metrics', got {load_path}" | ||
| ) | ||
|
|
There was a problem hiding this comment.
Suggestion: Add explicit type hints for fixture parameters and return type in this new async test method. [custom_rule]
Severity Level: Minor
| @pytest.mark.asyncio | |
| async def test_uuid_lookup_passes_metrics_eager_load(self, mcp_server, mock_auth): | |
| """UUID identifier lookup must also eager-load Slice.table.metrics.""" | |
| from unittest.mock import patch | |
| from fastmcp import Client | |
| uuid = "a1b2c3d4-5678-90ab-cdef-1234567890ab" | |
| with patch( | |
| "superset.daos.chart.ChartDAO.find_by_id", return_value=None | |
| ) as mock_find: | |
| async with Client(mcp_server) as client: | |
| await client.call_tool( | |
| "get_chart_data", | |
| {"request": {"identifier": uuid, "format": "excel"}}, | |
| ) | |
| mock_find.assert_called_once() | |
| call = mock_find.call_args | |
| assert call.args == (uuid,) | |
| assert call.kwargs.get("id_column") == "uuid" | |
| query_options = call.kwargs.get("query_options") | |
| assert query_options is not None, ( | |
| "UUID chart lookup must pass query_options for eager-loading." | |
| ) | |
| load_path = _extract_metrics_load_path(query_options[0]) | |
| assert load_path == ["table", "metrics"], ( | |
| f"Expected subqueryload chain 'table' -> 'metrics', got {load_path}" | |
| ) | |
| async def test_uuid_lookup_passes_metrics_eager_load(self, mcp_server: Any, mock_auth: Any) -> None: |
Why it matters? 🤔
This async test method also lacks type annotations for its fixture parameters and return type, so the suggested fix is aligned with the rule. The replacement is valid Python, and Any is already imported in the file, so no additional dependencies are introduced.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/mcp_service/chart/tool/test_get_chart_data.py
**Line:** 947:977
**Comment:**
*Custom Rule: Add explicit type hints for fixture parameters and return type in this new async test method.
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
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #39483 +/- ##
==========================================
- Coverage 64.16% 64.16% -0.01%
==========================================
Files 2592 2592
Lines 138629 138634 +5
Branches 32143 32143
==========================================
+ Hits 88951 88952 +1
- Misses 48146 48150 +4
Partials 1532 1532
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
The `tool/__init__.py` re-exports the `get_chart_data` function from its submodule of the same name, which shadows the module binding in the `tool` package namespace. mock.patch walks the dotted string by getattr()-ing the package namespace, so `superset.mcp_service.chart.tool.get_chart_data.event_logger` resolves `get_chart_data` to the function, then raises AttributeError on `event_logger`, then falls back to `__import__` and fails with ModuleNotFoundError. Obtain the actual module via importlib and patch.object the attribute directly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
msyavuz
force-pushed
the
msyavuz/fix/mcp-excel-export
branch
from
9690753 to
3e2e3da
Compare
Code Review Agent Run #da3022Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
alexandrusoare
left a comment
alexandrusoare
left a comment
There was a problem hiding this comment.
LGTM - the comments from Codeant-AI don't seem relevant
# Conflicts: # superset/mcp_service/chart/tool/get_chart_data.py
msyavuz
force-pushed
the
msyavuz/fix/mcp-excel-export
branch
from
c7cc058 to
36ebb7c
Compare
# Conflicts: # tests/unit_tests/mcp_service/chart/tool/test_get_chart_data.py
…InstanceError (apache#39483) Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Bito Automatic Review Skipped – PR Already Merged |
2 participants
SUMMARY
get_chart_datawithformat="excel"could fail with:The dataset's
metricsrelationship is lazy-loaded. If the request-scoped session detaches betweenChartDAO.find_by_idand later access ofchart.table.metrics, SQLAlchemy has no session to issue the SELECT and raisesDetachedInstanceError. Excel export surfaces this more than CSV/JSON because openpyxl encoding widens the wall-clock window between lookup and any subsequent metrics access.Fix: pass a
subqueryload(Slice.table).subqueryload(SqlaTable.metrics)query option on theChartDAO.find_by_idcalls inget_chart_data, sometricsis materialized at fetch time and later detachment is harmless. Mirrors the eager-loading pattern from #39206 (database relationship) and #38129 (owners/tags onget_chart_info).TESTING INSTRUCTIONS
Automated:
Three new regression tests in
TestChartLookupEagerLoadingassert that the subqueryload chain (table→metrics) is passed asquery_optionson both the numeric-ID and UUID lookup paths, for every format.Manual:
FASTMCP_TRANSPORT=streamable-http python -m superset.mcp_serviceget_chart_datawith{ identifier: <id>, format: "excel", limit: 10 }against a chart whose dataset has saved metrics.ChartDataresponse withformat="excel"and a decodableexcel_database64 blob — no lazy-load error.format="csv"andformat="json"still work unchanged.ADDITIONAL INFORMATION
🤖 Generated with Claude Code