geido opened a new pull request, #44004:
URL: https://github.com/apache/superset/pull/44004
### SUMMARY
`get_chart_data` fetches the chart `Slice`, then crosses many `await` points
and nested `event_logger.log_context(...)` blocks. Exiting those blocks commits
the DB session (`DBEventLogger.log()` → `db.session.commit()`), which expires
every loaded ORM attribute. A later attribute read on the chart then raises
`DetachedInstanceError` (`Instance <Slice at 0x...> is not bound to a Session;
attribute refresh operation cannot proceed`). The tool's broad `except (...,
SQLAlchemyError, ...)` handler swallows it, so the caller gets a confusing
internal-session error instead of the chart data.
The fix calls `db.session.refresh(chart)` immediately after the lookup,
while the session is still live, so all column values are loaded into the
instance upfront and a later expiry has nothing left to lazily fetch:
```python
if chart is not None:
db.session.refresh(chart)
guest_dashboard_id = guest_scope.guest_dashboard_id(chart)
```
That is the entire production change (plus the `db` import). This is **the
same fix already applied to the sibling `get_chart_preview` tool** in #39921 —
`get_chart_data` never received the equivalent.
**Design notes**
- **The eager-load added for Excel export is preserved.** `refresh()`
expires attributes, so the obvious concern is whether it undoes the
`subqueryload(Slice.table).subqueryload(SqlaTable.metrics)` eager-load added in
#39483. Verified empirically against the pinned SQLAlchemy 2.0.52 with a mapped
model: `Session.refresh()` reloads already-eagerly-loaded relationships using
their original loader strategy, and `chart.table.metrics` remains reachable
after the instance is detached. The Excel export path is unaffected.
- **The call is intentionally unguarded**, matching `get_chart_preview`.
`find_chart_by_identifier` only ever returns a persistent `Slice` from
`ChartDAO.find_by_id` or `None`, so there is no production path where
`refresh()` would receive a non-persistent object.
- **No change to the existing exception handling.** This prevents reaching
that handler for this call site rather than altering it.
- Seven existing tests stood in a `SimpleNamespace` for the chart;
SQLAlchemy rejects `refresh()` on an unmapped object, so they now take a
`stub_session_refresh` fixture. This is a test-double artifact only.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Not applicable — no UI change.
### TESTING INSTRUCTIONS
Two regression tests are added in a `TestDetachedInstanceError` class
mirroring the one in `test_get_chart_preview.py`:
1. `test_session_refresh_called_after_chart_load` — `db.session.refresh()`
is called exactly once with the chart returned by the lookup.
2. `test_session_refresh_precedes_chart_attribute_access` — the refresh runs
**before** anything reads a chart attribute, which is the ordering property
that makes the fix work. A refresh placed after the first attribute read would
leave that read exposed to an expired session.
```bash
pytest tests/unit_tests/mcp_service/chart/tool/test_get_chart_data.py -q
pytest tests/unit_tests/mcp_service/ -q
```
The tests were written first and confirmed to fail without the production
change:
```
AssertionError: db.session.refresh() must be called exactly once with the
chart
loaded by find_chart_by_identifier; got []
AssertionError: db.session.refresh() was never called
assert 'refresh' in ['attr:id', 'attr:slice_name', 'attr:viz_type', ...]
```
To reconfirm they are not vacuous, stash only the production file and re-run
the class — both tests fail again.
**Results on this branch**
| Run | Result |
| --- | --- |
| `test_get_chart_data.py` | 109 passed |
| `tests/unit_tests/mcp_service/` | 3701 passed, 1 failed |
| `pre-commit run` (mypy, ruff, pylint, all hooks) | all pass |
The single failure is
`test_mcp_e2e_smoke.py::test_tools_call_health_check_over_real_asgi_transport`,
confirmed to fail identically on unmodified `master` — pre-existing and
unrelated to this change.
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in
[SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
🤖 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: [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]
