codeant-ai-for-open-source[bot] commented on code in PR #38859:
URL: https://github.com/apache/superset/pull/38859#discussion_r2991123306
##########
tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_generation.py:
##########
@@ -1218,58 +1244,53 @@ def test_long_title_is_truncated(self):
class TestDashboardSerializationEagerLoading:
- """Tests for eager loading fix in dashboard serialization paths."""
+ """Tests for eager loading fix in dashboard serialization paths.
- @patch("superset.daos.dashboard.DashboardDAO.find_by_id")
- def test_generate_dashboard_refetches_via_dao(self, mock_find_by_id):
- """generate_dashboard re-fetches dashboard via DashboardDAO.find_by_id
+ The re-fetch uses db.session.query(Dashboard) instead of
+ DashboardDAO.find_by_id() to avoid "Can't reconnect until invalid
+ transaction is rolled back" errors in multi-tenant environments.
+ """
+
+ def test_generate_dashboard_refetches_via_session_query(self):
+ """generate_dashboard re-fetches dashboard via db.session.query()
with eager-loaded slice relationships before serialization."""
refetched_dashboard = _mock_dashboard()
refetched_chart = _mock_chart(id=1, slice_name="Refetched Chart")
refetched_dashboard.slices = [refetched_chart]
- mock_find_by_id.return_value = refetched_dashboard
-
- from superset.daos.dashboard import DashboardDAO
-
- result = (
- DashboardDAO.find_by_id(1, query_options=["dummy"]) or
_mock_dashboard()
+ mock_query = MagicMock()
+ mock_query.options.return_value.filter.return_value.first.return_value
= (
+ refetched_dashboard
)
+ result = mock_query.options().filter().first() or _mock_dashboard()
+
assert result is refetched_dashboard
- mock_find_by_id.assert_called_once_with(1, query_options=["dummy"])
- @patch("superset.daos.dashboard.DashboardDAO.find_by_id")
- def test_add_chart_refetches_dashboard_via_dao(self, mock_find_by_id):
+ def test_add_chart_refetches_dashboard_via_session_query(self):
"""add_chart_to_existing_dashboard re-fetches dashboard via
- DashboardDAO.find_by_id with eager-loaded slice relationships."""
+ db.session.query() with eager-loaded slice relationships."""
original_dashboard = _mock_dashboard()
refetched_dashboard = _mock_dashboard()
refetched_dashboard.slices = [_mock_chart()]
- mock_find_by_id.return_value = refetched_dashboard
-
- from superset.daos.dashboard import DashboardDAO
-
- result = (
- DashboardDAO.find_by_id(original_dashboard.id,
query_options=["dummy"])
- or original_dashboard
+ mock_query = MagicMock()
+ mock_query.options.return_value.filter.return_value.first.return_value
= (
+ refetched_dashboard
)
+ result = mock_query.options().filter().first() or original_dashboard
+
assert result is refetched_dashboard
- @patch("superset.daos.dashboard.DashboardDAO.find_by_id")
- def test_add_chart_falls_back_on_refetch_failure(self, mock_find_by_id):
+ def test_add_chart_falls_back_on_refetch_failure(self):
"""add_chart_to_existing_dashboard falls back to original dashboard
- if DashboardDAO.find_by_id returns None."""
+ if db.session.query().first() returns None."""
original_dashboard = _mock_dashboard()
- mock_find_by_id.return_value = None
- from superset.daos.dashboard import DashboardDAO
+ mock_query = MagicMock()
+ mock_query.options.return_value.filter.return_value.first.return_value
= None
- result = (
- DashboardDAO.find_by_id(original_dashboard.id,
query_options=["dummy"])
- or original_dashboard
- )
+ result = mock_query.options().filter().first() or original_dashboard
assert result is original_dashboard
Review Comment:
**Suggestion:** The fallback test is also disconnected from production code
and only checks Python `or` semantics on mocks, so it does not verify fallback
behavior in the real tool. Run the tool with refetch returning `None` and
assert the response still uses the updated dashboard. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Fallback-to-updated-dashboard behavior remains untested.
- ⚠️ None-refetch regressions may break dashboard serialization.
```
</details>
```suggestion
@patch("superset.commands.dashboard.update.UpdateDashboardCommand")
@patch("superset.daos.dashboard.DashboardDAO.find_by_id")
@patch("superset.db.session")
@pytest.mark.asyncio()
async def test_add_chart_falls_back_on_refetch_failure(
self, mock_db_session, mock_find_dashboard, mock_update_command,
mcp_server
):
"""add_chart_to_existing_dashboard should return updated dashboard
when refetch is None."""
dashboard = _mock_dashboard(id=8, title="Fallback Dashboard")
dashboard.slices = []
dashboard.position_json = "{}"
mock_find_dashboard.return_value = dashboard
mock_db_session.get.return_value = _mock_chart(id=33,
slice_name="Fallback Chart")
updated_dashboard = _mock_dashboard(id=8, title="Fallback Dashboard")
updated_dashboard.slices = [_mock_chart(id=33, slice_name="Fallback
Chart")]
mock_update_command.return_value.run.return_value = updated_dashboard
refetch = mock_db_session.query.return_value.options.return_value
refetch.filter.return_value.first.return_value = None
async with Client(mcp_server) as client:
result = await client.call_tool(
"add_chart_to_existing_dashboard",
{"request": {"dashboard_id": 8, "chart_id": 33}},
)
assert result.structured_content["error"] is None
assert result.structured_content["dashboard"]["id"] == 8
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Execute `test_add_chart_falls_back_on_refetch_failure` at
`tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_generation.py:1286-1296`.
2. It only checks Python `or` on a `MagicMock` return (`None or
original_dashboard`) and
never exercises tool code.
3. Actual fallback is implemented in
`add_chart_to_existing_dashboard.py:443` as `(...
.first()) or updated_dashboard`.
4. If that fallback regresses, downstream serialization uses
`updated_dashboard.id` at
`add_chart_to_existing_dashboard.py:454`; this test still passes because it
never reaches
production flow.
```
</details>
<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/dashboard/tool/test_dashboard_generation.py
**Line:** 1286:1296
**Comment:**
*Logic Error: The fallback test is also disconnected from production
code and only checks Python `or` semantics on mocks, so it does not verify
fallback behavior in the real tool. Run the tool with refetch returning `None`
and assert the response still uses the updated dashboard.
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38859&comment_hash=103cd4330456da7faa4b7d1aa28fdfd2141efa52e0d827de5fdd9fc6bc10e96f&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38859&comment_hash=103cd4330456da7faa4b7d1aa28fdfd2141efa52e0d827de5fdd9fc6bc10e96f&reaction=dislike'>👎</a>
##########
superset/mcp_service/dashboard/tool/add_chart_to_existing_dashboard.py:
##########
@@ -324,6 +324,7 @@ def add_chart_to_existing_dashboard(
"""
try:
from superset.commands.dashboard.update import UpdateDashboardCommand
Review Comment:
**Suggestion:** `SQLAlchemyError` is referenced in the outer `except` tuple
but is only imported later in the `try` block. If any earlier statement raises,
Python evaluates the `except` tuple with an uninitialized local and raises
`UnboundLocalError`, masking the real failure. Import `SQLAlchemyError` before
any potentially failing operations in the `try`. [possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ add_chart_to_existing_dashboard can fail with internal server error.
- ⚠️ Original CommandException is masked by UnboundLocalError.
```
</details>
```suggestion
from sqlalchemy.exc import SQLAlchemyError
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Start MCP service where tools are registered from
`superset/mcp_service/app.py:408-413`, including
`add_chart_to_existing_dashboard`.
2. Invoke the tool through normal MCP flow (same path used in tests via
`Client.call_tool("add_chart_to_existing_dashboard", ...)` at
`tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_generation.py:582`).
3. In `add_chart_to_existing_dashboard()`
(`superset/mcp_service/dashboard/tool/add_chart_to_existing_dashboard.py`),
execution
reaches `updated_dashboard = command.run()` at line 426.
4. `UpdateDashboardCommand.run()` is transaction-wrapped at
`superset/commands/dashboard/update.py:58` and can raise
`DashboardUpdateFailedError`
(`superset/commands/dashboard/exceptions.py:57`), which is a
`CommandException` subtype.
5. If that exception is raised before line 435, Python evaluates the outer
`except
(CommandException, SQLAlchemyError, KeyError, ValueError)` at line 510, but
`SQLAlchemyError` is still an uninitialized local (imported later at line
435), causing
`UnboundLocalError` and masking the original command failure.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset/mcp_service/dashboard/tool/add_chart_to_existing_dashboard.py
**Line:** 326:326
**Comment:**
*Possible Bug: `SQLAlchemyError` is referenced in the outer `except`
tuple but is only imported later in the `try` block. If any earlier statement
raises, Python evaluates the `except` tuple with an uninitialized local and
raises `UnboundLocalError`, masking the real failure. Import `SQLAlchemyError`
before any potentially failing operations in the `try`.
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38859&comment_hash=1fd9dab1a05d0102f38636f94ac41e1a07f3be566f08cceb609275e22a86d95e&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38859&comment_hash=1fd9dab1a05d0102f38636f94ac41e1a07f3be566f08cceb609275e22a86d95e&reaction=dislike'>👎</a>
##########
superset/mcp_service/chart/tool/generate_chart.py:
##########
@@ -713,24 +714,25 @@ async def generate_chart( # noqa: C901
if request.save_chart and chart:
from sqlalchemy.orm import joinedload
- from superset.daos.chart import ChartDAO
+ from superset import db
from superset.mcp_service.chart.schemas import
serialize_chart_object
from superset.models.slice import Slice
# Re-fetch with eager-loaded relationships to avoid detached
# instance errors when serialize_chart_object accesses .tags
- # and .owners. Use joinedload (single JOIN query) since we
- # are fetching a single chart.
- chart = (
- ChartDAO.find_by_id(
- chart.id,
- query_options=[
- joinedload(Slice.owners),
- joinedload(Slice.tags),
- ],
- )
- or chart
- )
+ # and .owners. Use a direct db.session.query() instead of
+ # ChartDAO.find_by_id() because the preceding commit may
+ # invalidate the session in multi-tenant environments, causing
+ # "Can't reconnect until invalid transaction is rolled back".
+ try:
+ chart = (
+ db.session.query(Slice)
+ .options(joinedload(Slice.owners), joinedload(Slice.tags))
+ .filter(Slice.id == chart.id)
+ .first()
+ ) or chart
+ except SQLAlchemyError:
+ pass
Review Comment:
**Suggestion:** Swallowing `SQLAlchemyError` with `pass` leaves the
SQLAlchemy session in a failed transaction state, so subsequent ORM access in
the same request can still fail with reconnect/transaction errors. Roll back
the session when the re-fetch fails, then continue with the fallback object.
[possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ `generate_chart` save flow can fail post-commit.
- ⚠️ Fallback chart serialization may still hit invalid session.
```
</details>
```suggestion
except SQLAlchemyError as ex:
db.session.rollback()
logger.warning(
"Chart re-fetch failed, using existing chart object:
%s", ex
)
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Invoke `generate_chart` with `save_chart=True` (tool is exposed via
`superset/mcp_service/app.py:176-188`; save flow in
`superset/mcp_service/chart/tool/generate_chart.py`).
2. After chart creation (`CreateChartCommand.run` at
`superset/commands/chart/create.py:46-51`), response-building runs the
re-fetch query at
`generate_chart.py:727-733`.
3. If that query raises `SQLAlchemyError`, current code catches it and does
`pass`
(`generate_chart.py:734-735`) without `db.session.rollback()`, leaving the
same session
potentially failed.
4. Code then serializes the fallback chart via `serialize_chart_object`
(`superset/mcp_service/chart/schemas.py:273+`), which accesses lazy
relationships
`chart.tags` and `chart.owners` (`schemas.py:55-67`) and can require DB
access.
5. With a failed session, subsequent ORM access can raise
transaction/reconnect SQLAlchemy
errors; outer handler at `generate_chart.py:791` catches `SQLAlchemyError`
and returns a
failure response even though chart creation already succeeded.
```
</details>
<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/generate_chart.py
**Line:** 734:735
**Comment:**
*Possible Bug: Swallowing `SQLAlchemyError` with `pass` leaves the
SQLAlchemy session in a failed transaction state, so subsequent ORM access in
the same request can still fail with reconnect/transaction errors. Roll back
the session when the re-fetch fails, then continue with the fallback object.
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38859&comment_hash=0cb55b4293e114118566e318ed7030d816db3d1565342e116d303318c7f240bb&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38859&comment_hash=0cb55b4293e114118566e318ed7030d816db3d1565342e116d303318c7f240bb&reaction=dislike'>👎</a>
##########
superset/mcp_service/dashboard/tool/generate_dashboard.py:
##########
@@ -339,21 +339,26 @@ def generate_dashboard(
error="Failed to create dashboard due to a database
error.",
)
- # Re-fetch with eager-loaded relationships for serialization
- from superset.daos.dashboard import DashboardDAO
-
- dashboard = (
- DashboardDAO.find_by_id(
- dashboard.id,
- query_options=[
+ # Re-fetch with eager-loaded relationships for serialization.
+ # Use a direct db.session.query() instead of DashboardDAO.find_by_id()
+ # because the preceding commit may invalidate the session in
+ # multi-tenant environments, causing "Can't reconnect until invalid
+ # transaction is rolled back". A fresh query on the current session
+ # avoids that problem.
+ try:
+ dashboard = (
+ db.session.query(Dashboard)
+ .options(
subqueryload(Dashboard.slices).subqueryload(Slice.owners),
subqueryload(Dashboard.slices).subqueryload(Slice.tags),
subqueryload(Dashboard.owners),
subqueryload(Dashboard.tags),
- ],
- )
- or dashboard
- )
+ )
+ .filter(Dashboard.id == dashboard.id)
+ .first()
+ ) or dashboard
+ except SQLAlchemyError:
+ pass
Review Comment:
**Suggestion:** The re-fetch error handler swallows `SQLAlchemyError`
without rolling back the failed transaction. When the query fails due to an
invalid transaction state, the session remains unusable and later attribute
access can raise `PendingRollbackError`, causing this flow to fail even though
the dashboard was already committed. Roll back the session in this `except`
block before continuing with the fallback object. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ generate_dashboard may fail after successful dashboard commit.
- ⚠️ Users may retry, creating duplicate dashboards unintentionally.
```
</details>
```suggestion
except SQLAlchemyError as ex:
db.session.rollback()
logger.warning(
"Dashboard re-fetch failed; returning committed object: %s",
ex,
exc_info=True,
)
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Call MCP tool `generate_dashboard` (tool entrypoint at
`superset/mcp_service/dashboard/tool/generate_dashboard.py:192`; also
invoked in tests via
`client.call_tool("generate_dashboard", ...)` at
`tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_generation.py:195`).
2. In normal flow, dashboard creation commits successfully at
`generate_dashboard.py:328`,
then code immediately performs a re-fetch query at
`generate_dashboard.py:349-359`.
3. If that re-fetch raises `SQLAlchemyError` (the PR itself documents this
invalid-transaction scenario in comments at
`generate_dashboard.py:343-347`), current code
catches it at `generate_dashboard.py:360-361` and does nothing (`pass`),
leaving failed
transaction state unhandled.
4. Function then serializes ORM fields (`dashboard.created_by_name` at
`generate_dashboard.py:377`; `dashboard.tags` at
`generate_dashboard.py:389`). These can
trigger lazy-load queries (relationship-backed properties in
`superset/models/helpers.py:592-600` and `superset/models/dashboard.py:153`).
5. With session still pending rollback, subsequent ORM access can raise
`PendingRollbackError` (confirmed subclass of `SQLAlchemyError`), which is
caught by outer
handler at `generate_dashboard.py:411`, returning failure even though
dashboard was
already committed.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/dashboard/tool/generate_dashboard.py
**Line:** 360:361
**Comment:**
*Logic Error: The re-fetch error handler swallows `SQLAlchemyError`
without rolling back the failed transaction. When the query fails due to an
invalid transaction state, the session remains unusable and later attribute
access can raise `PendingRollbackError`, causing this flow to fail even though
the dashboard was already committed. Roll back the session in this `except`
block before continuing with the fallback object.
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38859&comment_hash=0b07bb977f8fc6aa772d0c7532e701904cafaeaf7e6b26c263ce40ebaa5098fb&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38859&comment_hash=0b07bb977f8fc6aa772d0c7532e701904cafaeaf7e6b26c263ce40ebaa5098fb&reaction=dislike'>👎</a>
##########
tests/unit_tests/mcp_service/chart/tool/test_generate_chart.py:
##########
@@ -410,38 +410,31 @@ def
test_serialize_chart_object_fails_on_detached_instance(self):
with pytest.raises(DetachedInstanceError):
serialize_chart_object(chart)
- @patch("superset.daos.chart.ChartDAO.find_by_id")
- def test_generate_chart_refetches_via_dao(self, mock_find_by_id):
- """The serialization path re-fetches the chart via ChartDAO.find_by_id
- with joinedload query_options for owners and tags."""
+ def test_generate_chart_refetches_via_session_query(self):
+ """The serialization path re-fetches the chart via db.session.query()
+ with joinedload options for owners and tags, avoiding DAO calls that
+ fail in multi-tenant with invalidated sessions."""
refetched_chart = _make_mock_chart()
refetched_chart.tags = [Mock(id=1, name="tag1", type="custom")]
refetched_chart.tags[0].description = ""
- mock_find_by_id.return_value = refetched_chart
-
- from superset.daos.chart import ChartDAO
-
- chart = (
- ChartDAO.find_by_id(42, query_options=["dummy_option"])
- or _make_mock_chart()
+ mock_query = MagicMock()
+ mock_query.options.return_value.filter.return_value.first.return_value
= (
+ refetched_chart
)
+ chart = mock_query.options().filter().first() or _make_mock_chart()
+
assert chart is refetched_chart
Review Comment:
**Suggestion:** This test does not execute the `generate_chart` code path it
claims to verify; it only evaluates a locally mocked chain, so it will still
pass even if production code stops using eager-loaded `db.session.query(...)`.
Replace this with assertions that at least validate `options(...)` is called
with eager-load arguments so the test can actually fail when eager loading is
removed. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Eager-loading regressions can pass unit tests unnoticed.
- ⚠️ `generate_chart` serialization path not actually exercised.
```
</details>
```suggestion
mock_query.options.return_value.filter.return_value.first.return_value =
refetched_chart
owners_joinedload = Mock(name="owners_joinedload")
tags_joinedload = Mock(name="tags_joinedload")
chart = (
mock_query.options(owners_joinedload,
tags_joinedload).filter("slice_id_filter").first()
or _make_mock_chart()
)
mock_query.options.assert_called_once_with(owners_joinedload,
tags_joinedload)
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Inspect `test_generate_chart_refetches_via_session_query` in
`tests/unit_tests/mcp_service/chart/tool/test_generate_chart.py:413-429`; it
only executes
`mock_query.options().filter().first()` and never calls `generate_chart()`.
2. Verify imports in the same file: `from
superset.mcp_service.chart.tool.generate_chart
import (_compile_chart, CompileResult)`
(`.../test_generate_chart.py:36-39`), so the
tested function is not even imported.
3. Inspect real eager-loading logic in
`superset/mcp_service/chart/tool/generate_chart.py:722-733`, where
production calls
`db.session.query(Slice).options(joinedload(Slice.owners),
joinedload(Slice.tags))...`.
4. Because the test never reaches that code path, a production regression in
eager loading
would not fail this test; the current assertion is effectively validating
only local mock
wiring, not behavior.
```
</details>
<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_generate_chart.py
**Line:** 422:428
**Comment:**
*Logic Error: This test does not execute the `generate_chart` code path
it claims to verify; it only evaluates a locally mocked chain, so it will still
pass even if production code stops using eager-loaded `db.session.query(...)`.
Replace this with assertions that at least validate `options(...)` is called
with eager-load arguments so the test can actually fail when eager loading is
removed.
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38859&comment_hash=fceaeeb222899ef4c38ef11dc7dfb32aa181dafe677524c1203f4ae9544084e9&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38859&comment_hash=fceaeeb222899ef4c38ef11dc7dfb32aa181dafe677524c1203f4ae9544084e9&reaction=dislike'>👎</a>
##########
tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_generation.py:
##########
@@ -1218,58 +1244,53 @@ def test_long_title_is_truncated(self):
class TestDashboardSerializationEagerLoading:
- """Tests for eager loading fix in dashboard serialization paths."""
+ """Tests for eager loading fix in dashboard serialization paths.
- @patch("superset.daos.dashboard.DashboardDAO.find_by_id")
- def test_generate_dashboard_refetches_via_dao(self, mock_find_by_id):
- """generate_dashboard re-fetches dashboard via DashboardDAO.find_by_id
+ The re-fetch uses db.session.query(Dashboard) instead of
+ DashboardDAO.find_by_id() to avoid "Can't reconnect until invalid
+ transaction is rolled back" errors in multi-tenant environments.
+ """
+
+ def test_generate_dashboard_refetches_via_session_query(self):
+ """generate_dashboard re-fetches dashboard via db.session.query()
with eager-loaded slice relationships before serialization."""
refetched_dashboard = _mock_dashboard()
refetched_chart = _mock_chart(id=1, slice_name="Refetched Chart")
refetched_dashboard.slices = [refetched_chart]
- mock_find_by_id.return_value = refetched_dashboard
-
- from superset.daos.dashboard import DashboardDAO
-
- result = (
- DashboardDAO.find_by_id(1, query_options=["dummy"]) or
_mock_dashboard()
+ mock_query = MagicMock()
+ mock_query.options.return_value.filter.return_value.first.return_value
= (
+ refetched_dashboard
)
+ result = mock_query.options().filter().first() or _mock_dashboard()
+
assert result is refetched_dashboard
Review Comment:
**Suggestion:** This test never calls `generate_dashboard`; it only
exercises a `MagicMock` chain, so it will pass even if the real re-fetch logic
is broken. Replace it with an actual tool invocation and assert that the
`db.session.query(...).options(...)` path is used. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ CI misses generate_dashboard eager-load path regressions.
- ⚠️ Multi-tenant session-fix behavior unverified by this test.
```
</details>
```suggestion
@patch("superset.models.dashboard.Dashboard")
@patch("superset.db.session")
@pytest.mark.asyncio()
async def test_generate_dashboard_refetches_via_session_query(
self, mock_db_session, mock_dashboard_cls, mcp_server
):
"""generate_dashboard should execute session-query re-fetch before
serialization."""
charts = [_mock_chart(id=1, slice_name="Refetched Chart")]
dashboard = _mock_dashboard(id=101, title="Refetch Dashboard")
_setup_generate_dashboard_mocks(
mock_db_session, Mock(), mock_dashboard_cls, charts, dashboard
)
request = {"chart_ids": [1], "dashboard_title": "Refetch Dashboard"}
async with Client(mcp_server) as client:
result = await client.call_tool("generate_dashboard",
{"request": request})
assert result.structured_content["error"] is None
mock_db_session.query.return_value.options.assert_called_once()
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run `pytest` for `test_generate_dashboard_refetches_via_session_query` in
`tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_generation.py:1254-1268`.
2. Observe the test only executes `mock_query.options().filter().first()`
and never
invokes MCP `Client.call_tool("generate_dashboard", ...)`.
3. Compare with production path `generate_dashboard()` at
`superset/mcp_service/dashboard/tool/generate_dashboard.py:192`, where
re-fetch actually
happens at `:343-351` using
`db.session.query(Dashboard).options(...).filter(...).first()`.
4. Because the test is disconnected from `generate_dashboard()`, it will
pass even if the
real re-fetch block regresses; this is a real false-positive test.
```
</details>
<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/dashboard/tool/test_dashboard_generation.py
**Line:** 1254:1268
**Comment:**
*Logic Error: This test never calls `generate_dashboard`; it only
exercises a `MagicMock` chain, so it will pass even if the real re-fetch logic
is broken. Replace it with an actual tool invocation and assert that the
`db.session.query(...).options(...)` path is used.
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38859&comment_hash=2b03bd6f2906257911abb7548d4edbad6e2a56c5c74b328d6f037fb5911d690f&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38859&comment_hash=2b03bd6f2906257911abb7548d4edbad6e2a56c5c74b328d6f037fb5911d690f&reaction=dislike'>👎</a>
##########
tests/unit_tests/mcp_service/chart/tool/test_generate_chart.py:
##########
@@ -410,38 +410,31 @@ def
test_serialize_chart_object_fails_on_detached_instance(self):
with pytest.raises(DetachedInstanceError):
serialize_chart_object(chart)
- @patch("superset.daos.chart.ChartDAO.find_by_id")
- def test_generate_chart_refetches_via_dao(self, mock_find_by_id):
- """The serialization path re-fetches the chart via ChartDAO.find_by_id
- with joinedload query_options for owners and tags."""
+ def test_generate_chart_refetches_via_session_query(self):
+ """The serialization path re-fetches the chart via db.session.query()
+ with joinedload options for owners and tags, avoiding DAO calls that
+ fail in multi-tenant with invalidated sessions."""
refetched_chart = _make_mock_chart()
refetched_chart.tags = [Mock(id=1, name="tag1", type="custom")]
refetched_chart.tags[0].description = ""
- mock_find_by_id.return_value = refetched_chart
-
- from superset.daos.chart import ChartDAO
-
- chart = (
- ChartDAO.find_by_id(42, query_options=["dummy_option"])
- or _make_mock_chart()
+ mock_query = MagicMock()
+ mock_query.options.return_value.filter.return_value.first.return_value
= (
+ refetched_chart
)
+ chart = mock_query.options().filter().first() or _make_mock_chart()
+
assert chart is refetched_chart
- mock_find_by_id.assert_called_once_with(42,
query_options=["dummy_option"])
- @patch("superset.daos.chart.ChartDAO.find_by_id")
- def test_generate_chart_falls_back_to_original_on_refetch_failure(
- self, mock_find_by_id
- ):
- """Falls back to original chart if ChartDAO.find_by_id returns None."""
+ def test_generate_chart_falls_back_to_original_on_refetch_failure(self):
+ """Falls back to original chart if db.session.query().first()
+ returns None."""
original_chart = _make_mock_chart()
- mock_find_by_id.return_value = None
- from superset.daos.chart import ChartDAO
+ mock_query = MagicMock()
+ mock_query.options.return_value.filter.return_value.first.return_value
= None
- chart = (
- ChartDAO.find_by_id(original_chart.id, query_options=[]) or
original_chart
- )
+ chart = mock_query.options().filter().first() or original_chart
Review Comment:
**Suggestion:** The fallback test only covers a `None` return and still does
not exercise a real refetch failure path; if the refetch raises
`SQLAlchemyError` (which production handles), this test gives no protection.
Make the mock raise `SQLAlchemyError` and assert fallback to the original
chart. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ SQLAlchemy exception fallback path lacks test coverage.
- ⚠️ Multi-tenant session regressions may evade CI detection.
```
</details>
```suggestion
from sqlalchemy.exc import SQLAlchemyError
mock_query = MagicMock()
mock_query.options.return_value.filter.return_value.first.side_effect = (
SQLAlchemyError("session failure")
)
try:
chart = mock_query.options("owners_joinedload",
"tags_joinedload").filter("slice_id_filter").first()
except SQLAlchemyError:
chart = original_chart
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Inspect fallback logic in production `generate_chart` at
`superset/mcp_service/chart/tool/generate_chart.py:727-756`;
`db.session.query(...).first()` is wrapped in `try/except SQLAlchemyError`,
then falls
back to existing `chart`.
2. Inspect `test_generate_chart_falls_back_to_original_on_refetch_failure` in
`tests/unit_tests/mcp_service/chart/tool/test_generate_chart.py:430-440`; it
only tests
`first()` returning `None`.
3. No test in this file simulates `first()` raising `SQLAlchemyError`
(search result shows
no such exception usage in this test module).
4. Therefore, the exception fallback branch in production remains
unverified; regressions
in that branch can ship while tests still pass.
```
</details>
<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_generate_chart.py
**Line:** 435:438
**Comment:**
*Logic Error: The fallback test only covers a `None` return and still
does not exercise a real refetch failure path; if the refetch raises
`SQLAlchemyError` (which production handles), this test gives no protection.
Make the mock raise `SQLAlchemyError` and assert fallback to the original chart.
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38859&comment_hash=7a77b9a30269d5379b6f24a83fc717d3f8baa6b539f2f83de2ac1cb6e20870f7&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38859&comment_hash=7a77b9a30269d5379b6f24a83fc717d3f8baa6b539f2f83de2ac1cb6e20870f7&reaction=dislike'>👎</a>
##########
tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_generation.py:
##########
@@ -1218,58 +1244,53 @@ def test_long_title_is_truncated(self):
class TestDashboardSerializationEagerLoading:
- """Tests for eager loading fix in dashboard serialization paths."""
+ """Tests for eager loading fix in dashboard serialization paths.
- @patch("superset.daos.dashboard.DashboardDAO.find_by_id")
- def test_generate_dashboard_refetches_via_dao(self, mock_find_by_id):
- """generate_dashboard re-fetches dashboard via DashboardDAO.find_by_id
+ The re-fetch uses db.session.query(Dashboard) instead of
+ DashboardDAO.find_by_id() to avoid "Can't reconnect until invalid
+ transaction is rolled back" errors in multi-tenant environments.
+ """
+
+ def test_generate_dashboard_refetches_via_session_query(self):
+ """generate_dashboard re-fetches dashboard via db.session.query()
with eager-loaded slice relationships before serialization."""
refetched_dashboard = _mock_dashboard()
refetched_chart = _mock_chart(id=1, slice_name="Refetched Chart")
refetched_dashboard.slices = [refetched_chart]
- mock_find_by_id.return_value = refetched_dashboard
-
- from superset.daos.dashboard import DashboardDAO
-
- result = (
- DashboardDAO.find_by_id(1, query_options=["dummy"]) or
_mock_dashboard()
+ mock_query = MagicMock()
+ mock_query.options.return_value.filter.return_value.first.return_value
= (
+ refetched_dashboard
)
+ result = mock_query.options().filter().first() or _mock_dashboard()
+
assert result is refetched_dashboard
- mock_find_by_id.assert_called_once_with(1, query_options=["dummy"])
- @patch("superset.daos.dashboard.DashboardDAO.find_by_id")
- def test_add_chart_refetches_dashboard_via_dao(self, mock_find_by_id):
+ def test_add_chart_refetches_dashboard_via_session_query(self):
"""add_chart_to_existing_dashboard re-fetches dashboard via
- DashboardDAO.find_by_id with eager-loaded slice relationships."""
+ db.session.query() with eager-loaded slice relationships."""
original_dashboard = _mock_dashboard()
refetched_dashboard = _mock_dashboard()
refetched_dashboard.slices = [_mock_chart()]
- mock_find_by_id.return_value = refetched_dashboard
-
- from superset.daos.dashboard import DashboardDAO
-
- result = (
- DashboardDAO.find_by_id(original_dashboard.id,
query_options=["dummy"])
- or original_dashboard
+ mock_query = MagicMock()
+ mock_query.options.return_value.filter.return_value.first.return_value
= (
+ refetched_dashboard
)
+ result = mock_query.options().filter().first() or original_dashboard
+
assert result is refetched_dashboard
Review Comment:
**Suggestion:** This test does not execute
`add_chart_to_existing_dashboard`; it only asserts `MagicMock` behavior and
cannot catch regressions in the actual re-fetch implementation. Convert it into
a real tool call and verify the eager-load query path runs. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ add_chart eager-loading query path lacks direct test.
- ⚠️ Refetch regressions can slip through unit CI.
```
</details>
```suggestion
@patch("superset.commands.dashboard.update.UpdateDashboardCommand")
@patch("superset.daos.dashboard.DashboardDAO.find_by_id")
@patch("superset.db.session")
@pytest.mark.asyncio()
async def test_add_chart_refetches_dashboard_via_session_query(
self, mock_db_session, mock_find_dashboard, mock_update_command,
mcp_server
):
"""add_chart_to_existing_dashboard should execute session-query
re-fetch."""
original_dashboard = _mock_dashboard(id=7, title="Existing")
original_dashboard.slices = [_mock_chart(id=10)]
original_dashboard.position_json = "{}"
mock_find_dashboard.return_value = original_dashboard
mock_db_session.get.return_value = _mock_chart(id=20,
slice_name="Added")
updated_dashboard = _mock_dashboard(id=7, title="Existing")
updated_dashboard.slices = [_mock_chart(id=10), _mock_chart(id=20)]
mock_update_command.return_value.run.return_value = updated_dashboard
refetch = mock_db_session.query.return_value.options.return_value
refetch.filter.return_value.first.return_value = updated_dashboard
async with Client(mcp_server) as client:
result = await client.call_tool(
"add_chart_to_existing_dashboard",
{"request": {"dashboard_id": 7, "chart_id": 20}},
)
assert result.structured_content["error"] is None
mock_db_session.query.return_value.options.assert_called_once()
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run `test_add_chart_refetches_dashboard_via_session_query` at
`tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_generation.py:1270-1284`.
2. Confirm it only evaluates a local `MagicMock` chain and does not call
`add_chart_to_existing_dashboard()` through MCP client.
3. Real logic lives in
`superset/mcp_service/dashboard/tool/add_chart_to_existing_dashboard.py:318`,
with
re-fetch at `:431-444`
(`db.session.query(Dashboard).options(...).filter(...).first()`).
4. Since the test never enters that function, regressions in eager-load
query path are
undetectable by this test.
```
</details>
<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/dashboard/tool/test_dashboard_generation.py
**Line:** 1270:1284
**Comment:**
*Logic Error: This test does not execute
`add_chart_to_existing_dashboard`; it only asserts `MagicMock` behavior and
cannot catch regressions in the actual re-fetch implementation. Convert it into
a real tool call and verify the eager-load query path runs.
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38859&comment_hash=bce6bbb8f75d2584ece184be1b66fc74029a73889f3a8e1699d25b74afbb7bcd&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38859&comment_hash=bce6bbb8f75d2584ece184be1b66fc74029a73889f3a8e1699d25b74afbb7bcd&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]
