fix(mcp): honor use_cache and cache_timeout in get_chart_data - #43349
fix(mcp): honor use_cache and cache_timeout in get_chart_data#43349justinpark wants to merge 2 commits into
Conversation
get_chart_data's docstring advertises "Cache control: use_cache, force_refresh, cache_timeout", but only force_refresh was ever wired into the QueryContext's force flag. use_cache=False was silently ignored (the tool would still read cache), and cache_timeout was never applied at all. Compute an effective_force = force_refresh or not use_cache and use it everywhere the tool builds/patches a QueryContext, and thread cache_timeout through as QueryContextFactory's existing custom_cache_timeout parameter (also exposed as a top-level field on the saved query_context JSON via ChartDataQueryContextSchema).
dosubot
Bot
added
the
infra:caching
Code Review Agent Run #8099d9Actionable 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 |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| cache_status = get_cache_status_from_result( | ||
| query_result, force_refresh=request.force_refresh | ||
| query_result, force_refresh=effective_force | ||
| ) |
There was a problem hiding this comment.
Suggestion: The refreshed response field is documented and implemented as indicating a force refresh, but passing effective_force makes every use_cache=False, force_refresh=False request report refreshed=True. This conflates cache bypassing with an explicit refresh and produces an incorrect cache status for callers; pass the original request.force_refresh when constructing response metadata while retaining effective_force for query execution. [cache]
Severity Level: Minor 🧹
- ⚠️ `ChartData.cache_status.refreshed` misrepresents cache-bypass requests.
- ⚠️ MCP clients cannot distinguish bypassing cache from explicit refresh.
- ⚠️ Saved and unsaved chart responses expose inconsistent request semantics.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/mcp_service/chart/tool/get_chart_data.py
**Line:** 1128:1130
**Comment:**
*Cache: The `refreshed` response field is documented and implemented as indicating a force refresh, but passing `effective_force` makes every `use_cache=False, force_refresh=False` request report `refreshed=True`. This conflates cache bypassing with an explicit refresh and produces an incorrect cache status for callers; pass the original `request.force_refresh` when constructing response metadata while retaining `effective_force` for query execution.
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. Using Would you like me to fetch all other comments on this PR to validate and implement fixes for them as well? superset/mcp_service/chart/tool/get_chart_data.py |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #43349 +/- ##
=======================================
Coverage 66.73% 66.73%
=======================================
Files 2876 2876
Lines 164201 164205 +4
Branches 37887 37888 +1
=======================================
+ Hits 109577 109580 +3
- Misses 52467 52469 +2
+ Partials 2157 2156 -1
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:
|
…ed field get_chart_data used effective_force (which also folds in use_cache=False) when building the refreshed cache_status field, so any use_cache=False request incorrectly reported refreshed=True. effective_force is still used for actual query execution to bypass the cache. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Code Review Agent Run #331fa3Actionable 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 |
1 participant
SUMMARY
get_chart_data's docstring advertisesCache control: use_cache, force_refresh, cache_timeout, but onlyforce_refreshwas actually wired into theQueryContext'sforceflag:use_cache=Falsewas silently ignored — the tool would still read from cache even when a caller explicitly asked it not to.cache_timeoutwas never applied anywhere, despite being a documented, logged request field.This fixes both by:
effective_force = force_refresh or not use_cacheonce per call path, and using it everywhere the tool builds or patches aQueryContext(the cached-form_data path, the no-saved-query_context fallback path, the saved-query_contextpatch path, and the unsaved-chart-only_query_from_form_datapath), instead offorce=request.force_refresh.cache_timeoutthrough asQueryContextFactory's existingcustom_cache_timeoutparameter (already supported byQueryContext/QueryContextFactoryand byChartDataQueryContextSchemaas a top-level field) — added as a new optional param onbuild_query_context_from_form_datainchart_helpers.pyand passed through at every call site.No behavior changes for existing callers that don't set
use_cache/cache_timeout: defaults areuse_cache=True,cache_timeout=None, soeffective_forcereduces toforce_refreshas before.TESTING INSTRUCTIONS
test_query_from_form_data_use_cache_false_bypasses_cache(parametrized overuse_cache/force_refreshcombinations) totests/unit_tests/mcp_service/chart/tool/test_get_chart_data.py, asserting theforceandcustom_cache_timeoutkwargs passed tobuild_query_context_from_form_data.ruff check/ruff format --check/python -m py_compilepass on all changed files.Connection.rollback()missing) that reproduces identically onmasterbefore this change.ADDITIONAL INFORMATION