codeant-ai-for-open-source[bot] commented on code in PR #38934:
URL: https://github.com/apache/superset/pull/38934#discussion_r3009573202
##########
superset/mcp_service/dashboard/tool/add_chart_to_existing_dashboard.py:
##########
@@ -435,25 +435,60 @@ def add_chart_to_existing_dashboard(
# Re-fetch the dashboard with eager-loaded relationships to avoid
# "Instance is not bound to a Session" errors when serializing
- # chart .tags and .owners.
+ # chart .tags and .owners. The preceding command.run() commit may
+ # invalidate the session in multi-tenant environments; on failure,
+ # return a minimal response using only scalar attributes that are
+ # already loaded — relationship fields (owners, tags, slices) would
+ # trigger lazy-loading on the same dead session.
from sqlalchemy.orm import subqueryload
- from superset.daos.dashboard import DashboardDAO
from superset.models.dashboard import Dashboard
from superset.models.slice import Slice
- updated_dashboard = (
- DashboardDAO.find_by_id(
+ try:
+ updated_dashboard = (
+ DashboardDAO.find_by_id(
+ updated_dashboard.id,
+ query_options=[
+
subqueryload(Dashboard.slices).subqueryload(Slice.owners),
+
subqueryload(Dashboard.slices).subqueryload(Slice.tags),
+ subqueryload(Dashboard.owners),
+ subqueryload(Dashboard.tags),
+ ],
+ )
+ or updated_dashboard
+ )
+ except SQLAlchemyError:
+ logger.warning(
+ "Re-fetch of dashboard %s failed; returning minimal response",
updated_dashboard.id,
- query_options=[
- subqueryload(Dashboard.slices).subqueryload(Slice.owners),
- subqueryload(Dashboard.slices).subqueryload(Slice.tags),
- subqueryload(Dashboard.owners),
- subqueryload(Dashboard.tags),
- ],
+ exc_info=True,
+ )
+ try:
+ db.session.rollback() # pylint:
disable=consider-using-transaction
+ except SQLAlchemyError:
+ logger.warning(
+ "Database rollback failed during dashboard re-fetch error
handling",
+ exc_info=True,
+ )
+ dashboard_url = (
+
f"{get_superset_base_url()}/superset/dashboard/{updated_dashboard.id}/"
+ )
+ position_info = {
+ "row": row_key,
+ "chart_key": chart_key,
+ "row_key": row_key,
+ }
+ return AddChartToDashboardResponse(
+ dashboard=DashboardInfo(
+ id=updated_dashboard.id,
+ dashboard_title=updated_dashboard.dashboard_title,
+ url=dashboard_url,
+ ),
Review Comment:
**Suggestion:** In the fallback path where the dashboard re-fetch fails, the
minimal `DashboardInfo` response omits `chart_count` (and `published`), so
clients will see a chart_count of 0 even though a chart was successfully added;
this is a logic error and can be fixed by populating `chart_count` from the
already-materialized `all_chart_objects` list and including `published` from
the existing `updated_dashboard` object, both of which are safe scalar data and
consistent with the main success path. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ MCP dashboard clients misreport chart counts after re-fetch failures.
- ⚠️ Clients relying on published flag see null despite published.
```
</details>
```suggestion
chart_count=len(all_chart_objects),
published=updated_dashboard.published,
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. From an MCP client, invoke the `add_chart_to_existing_dashboard` tool
(entrypoint
`add_chart_to_existing_dashboard` in
`superset/mcp_service/dashboard/tool/add_chart_to_existing_dashboard.py:327-588`)
with a
valid `dashboard_id` and `chart_id` so that validation succeeds and the code
reaches the
update block and then the re-fetch block around lines 430–491 (see re-fetch
try/except in
`add_chart_to_existing_dashboard.py` read via tools).
2. Let `UpdateDashboardCommand.run()` complete successfully in the
`mcp.add_chart_to_dashboard.db_write` context so that `updated_dashboard` is
a committed
ORM instance and `all_chart_objects = list(dashboard.slices) + [new_chart]`
has been
constructed in-memory just before the re-fetch (lines just above the
re-fetch block in
`add_chart_to_existing_dashboard.py`).
3. Cause the re-fetch to fail so that the `except SQLAlchemyError:` path
executes at
`superset/mcp_service/dashboard/tool/add_chart_to_existing_dashboard.py:32-42`
of the
snippet we inspected: for example, in a unit test, patch
`DashboardDAO.find_by_id` (used
in the re-fetch at lines 19–29 of the same block) to raise
`SQLAlchemyError`, or in a real
environment, reproduce a PostgreSQL SSL connection drop so that the session
is poisoned
and the re-fetch query fails.
4. Observe the returned `AddChartToDashboardResponse` (schema in
`superset/mcp_service/dashboard/schemas.py:410-423`): its `dashboard` field
is a
`DashboardInfo` constructed by the minimal path at lines 482–491, which only
sets `id`,
`dashboard_title`, and `url`. Because `DashboardInfo.chart_count` defaults
to `0` and
`published` defaults to `None` (`schemas.py:290-310`), the client will see
`dashboard.chart_count == 0` and `dashboard.published is None` even though a
chart was
actually added and `updated_dashboard.published` is a valid boolean. In the
normal success
path (eager-load succeeds), `dashboard_info` sets
`chart_count=len(updated_dashboard.slices)` and
`published=updated_dashboard.published`
(lines 70–77, 509–511), demonstrating that the fallback path is inconsistent
and returns
incorrect metadata.
```
</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:** 487:487
**Comment:**
*Logic Error: In the fallback path where the dashboard re-fetch fails,
the minimal `DashboardInfo` response omits `chart_count` (and `published`), so
clients will see a chart_count of 0 even though a chart was successfully added;
this is a logic error and can be fixed by populating `chart_count` from the
already-materialized `all_chart_objects` list and including `published` from
the existing `updated_dashboard` object, both of which are safe scalar data and
consistent with the main success path.
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%2F38934&comment_hash=9abcd1cd451895e563ce246815e8da6866736fa3e27689c6e3e398a2ae90b477&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38934&comment_hash=9abcd1cd451895e563ce246815e8da6866736fa3e27689c6e3e398a2ae90b477&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]
