fix(mcp): generate durable explore permalink URL instead of ephemeral form_data_key#40773
fix(mcp): generate durable explore permalink URL instead of ephemeral form_data_key#40773aminghadersohi wants to merge 5 commits into
Conversation
… form_data_key The `generate_explore_link` MCP tool previously returned a URL like `/explore/?form_data_key=<key>` backed by Redis cache (~24h expiry). When the key expired the URL became unreachable, preventing LLMs from iteratively building and sharing charts. This commit changes the URL strategy: 1. Try `CreateExplorePermalinkCommand` first (DB-backed key-value store, does not expire) → `/explore/p/<key>/` 2. Fall back to `MCPCreateFormDataCommand` (Redis cache) on permalink failure → `/explore/?form_data_key=<key>` 3. Fall back to plain dataset URL on both failing. The tool response now includes a `permalink_key` field (populated when the durable permalink path is used) alongside the existing `form_data_key` (populated only in the Redis-fallback case). `extract_permalink_key_from_url()` added to `chart_helpers.py`. Tests updated to mock `CreateExplorePermalinkCommand.run` by default and cover both the permalink path and each fallback tier.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #40773 +/- ##
==========================================
- Coverage 64.19% 63.97% -0.22%
==========================================
Files 2666 2648 -18
Lines 143991 143092 -899
Branches 33108 32947 -161
==========================================
- Hits 92428 91543 -885
+ Misses 49950 49940 -10
+ Partials 1613 1609 -4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…urce - Move extract_permalink_key_from_url from chart_helpers to url_utils and reimplement via urlparse path parsing instead of regex - Fix generate_explore_link tool to use dataset.id (resolved numeric ID) instead of request.dataset_id (may be UUID string) when building the datasource field in the returned form_data dict - Add unit tests for extract_permalink_key_from_url in test_url_utils.py
Code Review Agent Run #d40bd6Actionable 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 |
rusackas
left a comment
rusackas
left a comment
There was a problem hiding this comment.
LGTM, but the permalink-creation except is very broad (TypeError/AttributeError/KeyError/ValueError alongside the expected ExplorePermalinkCreateFailedError/SQLAlchemyError) — it could mask a genuine bug behind a silent fallback, but it's a deliberate resilience path with debug logging.
…modes Address review feedback that the permalink-creation except was too broad. CreateExplorePermalinkCommand already wraps its internal failures into ExplorePermalinkCreateFailedError, so catch only that and SQLAlchemyError in the permalink and outer fallback blocks. Programming errors (TypeError, AttributeError, KeyError, ValueError) now surface to the tool handler instead of being silently masked behind a fallback URL.
Code Review Agent Run #648f3bActionable 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 |
After narrowing the explore-link exception handling, the durable-permalink path no longer swallows the AttributeError raised by check_chart_access accessing g.user outside a request context. Patch CreateExplorePermalinkCommand.run to fail explicitly so the test deterministically exercises the form_data_key fallback, matching the test's stated intent.
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| state = {"formData": form_data_with_datasource} | ||
| permalink_key = CreateExplorePermalinkCommand(state=state).run() | ||
| return f"{base_url}/explore/p/{permalink_key}/" |
There was a problem hiding this comment.
Suggestion: Returning a permalink URL by default breaks existing callers that still rely on a form_data_key URL contract. update_chart_preview immediately parses the generated URL with extract_form_data_key_from_url and now fails with "missing form_data_key" for the normal success path, and generate_chart preview mode similarly loses its unsaved-state key. Keep permalink generation for the explore tool path, but preserve/optionally force form_data_key generation for shared callers that require it (or update those callers to consume permalink keys end-to-end before changing this shared helper default). [api mismatch]
Severity Level: Critical 🚨
- ❌ update_chart_preview tool returns PreviewError for valid preview updates.
- ⚠️ generate_chart preview responses no longer expose form_data_key.
- ⚠️ LLM workflows cannot reuse cached preview state keys.Steps of Reproduction ✅
1. Invoke the MCP `generate_chart` tool in preview mode by calling
`generate_chart(request={... save_chart: false ...})` as documented in
`superset/mcp_service/app.py:102-114` and implemented in
`superset/mcp_service/chart/tool/generate_chart.py:81-161`.
2. In the preview-only branch of `generate_chart` at
`superset/mcp_service/chart/tool/generate_chart.py:527-535`, observe that it imports
`generate_explore_link` and calls `explore_url = generate_explore_link(request.dataset_id,
form_data)` (line 530), then immediately calls `form_data_key =
extract_form_data_key_from_url(explore_url)` (lines 533-534) to populate the response's
`form_data_key`.
3. The shared helper `generate_explore_link` used above is implemented in
`superset/mcp_service/chart/chart_utils.py:160-116`; on the normal success path it now
executes `state = {"formData": form_data_with_datasource}` and `permalink_key =
CreateExplorePermalinkCommand(state=state).run()` at `chart_utils.py:217-218`, then
returns `f"{base_url}/explore/p/{permalink_key}/"` at `chart_utils.py:219` — a permalink
URL with **no** `form_data_key` query parameter, so `extract_form_data_key_from_url` in
`superset/mcp_service/chart/chart_helpers.py:19-28` returns `None`, causing
`generate_chart` preview responses to lose the previously-populated unsaved preview
`form_data_key`.
4. Independently, call the MCP `update_chart_preview` tool defined at
`superset/mcp_service/chart/tool/update_chart_preview.py:115-33` with a valid `dataset_id`
and `config`. After validation, the function generates a new explore URL via `explore_url
= generate_explore_link(request.dataset_id, new_form_data)` at
`update_chart_preview.py:244`, then immediately runs `new_form_data_key =
extract_form_data_key_from_url(explore_url)` at `update_chart_preview.py:247` and checks
`if not new_form_data_key:` returning an error payload with `error_type: "PreviewError"`
and message `"Failed to generate preview: missing form_data_key"` at
`update_chart_preview.py:248-156`. Because the shared `generate_explore_link` now returns
a `/explore/p/<permalink_key>/` URL without a `form_data_key` under normal conditions,
this error path is taken even when permalink generation succeeds, breaking the normal
success flow for `update_chart_preview` and any clients that follow the preview-first
workflow described in `superset/mcp_service/app.py:102-114`.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:** superset/mcp_service/chart/chart_utils.py
**Line:** 217:219
**Comment:**
*Api Mismatch: Returning a permalink URL by default breaks existing callers that still rely on a `form_data_key` URL contract. `update_chart_preview` immediately parses the generated URL with `extract_form_data_key_from_url` and now fails with `"missing form_data_key"` for the normal success path, and `generate_chart` preview mode similarly loses its unsaved-state key. Keep permalink generation for the explore tool path, but preserve/optionally force `form_data_key` generation for shared callers that require it (or update those callers to consume permalink keys end-to-end before changing this shared helper default).
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
|
The flagged issue is correct. The To resolve this, you can update Would you like me to implement this change and check the rest of the comments on this PR? superset/mcp_service/chart/chart_utils.py |
SUMMARY
The
generate_explore_linkMCP tool was returning URLs like/explore/?form_data_key=<key>where the key is stored in Redis cache and expires in ~24h. This caused two problems for LLM-based workflows:form_data_keyis opaque — an LLM building charts iteratively cannot read back the current configuration from the URL to revise it.This fix changes the URL strategy to use Superset's explore permalink mechanism, which stores chart state in the key-value DB table (not Redis) and does not expire. The resulting URL is
/explore/p/<key>/— bookmarkable, shareable, and reconstructible.Fallback chain:
CreateExplorePermalinkCommand(DB-backed, durable) →/explore/p/<key>/MCPCreateFormDataCommand(Redis-backed, ephemeral) on permalink failure/explore/?datasource_type=table&datasource_id=<id>) on both failingResponse schema change: adds
permalink_key(populated for durable URLs) alongside the existingform_data_key(now only populated in the Redis-fallback case).BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before:
http://host/explore/?form_data_key=L-6rhlg-CSY— expires in ~24h, config not in URLAfter:
http://host/explore/p/1OkEh0GK5e9lFKVMCGqEG1/— permanent, DB-backed permalinkTESTING INSTRUCTIONS
generate_explore_linkMCP tool with a dataset ID and chart config/explore/p/<key>/formatpytest tests/unit_tests/mcp_service/explore/tool/test_generate_explore_link.py -x -qADDITIONAL INFORMATION