justinpark commented on PR #43349: URL: https://github.com/apache/superset/pull/43349#issuecomment-5359868938
> **Review — `fix(mcp): honor use_cache and cache_timeout in get_chart_data`** > > Verdict: **Correct and mergeable on the merits** — small, well-scoped diff, explanatory commit, and a real parametrized test. I'm posting a comment rather than approving only because the reviewer thread on `get_chart_data.py` (codeant, `refreshed` metadata) is still marked unresolved; the finding it raised is in fact already addressed in commit 2 — please resolve the thread. > > **1. codeant's `refreshed` finding — already fixed (commit 2), thread is stale.** Both `get_cache_status_from_result(...)` calls now pass `request.force_refresh`, not `effective_force`: > > * `get_chart_data.py:806-807` > * `get_chart_data.py:1128-1129` > > So `cache_status.refreshed` reflects only an explicit `force_refresh=True`; a `use_cache=False, force_refresh=False` request no longer misreports `refreshed=True`. `effective_force` is retained only for query execution. The concern is resolved; the thread is outdated (anchored to the pre-commit-2 line). > > **2. Completeness — the `effective_force` / `request.force_refresh` split is deliberate and complete.** (not blocking) > > * Cache **bypass** uses `effective_force` at all four `force=` sites: `575`, `672`, `677`, `1072`. > * Response **metadata** uses `request.force_refresh` at both `get_cache_status_from_result` sites: `807`, `1129`. > * No remaining `force_refresh` / `use_cache` site was missed. This is correctly _not_ a replace-all. > > **3. `use_cache` direction is safe — calling it out explicitly.** (non-issue) The `use_cache` half only makes `force` True _more_ often, so the cache is consulted _less_. That direction can only cost extra queries; it cannot serve stale or cross-tenant data. No tenancy concern. > > **4. `cache_timeout` is unbounded.** (non-blocking, minor) `GetChartDataRequest.cache_timeout` (via `QueryCacheControl`) has no `ge=`/`le=` and no validator. It threads through as `custom_cache_timeout` into `QueryContext` and is returned first (highest priority) by `QueryContextProcessor.get_cache_timeout()` — no config ceiling clamps it. A client can set a very large TTL (pinning a chart's cached result) or a negative value. > > * Not a privilege issue: this is at parity with the existing core `/api/v1/chart/data` (`ChartDataQueryContextSchema.custom_cache_timeout` is likewise unbounded), so any principal with chart-data access already has this capability. > * Consistency nit: `GenerateChartRequest` already validates `cache_timeout >= 0`; `GetChartDataRequest` does not. Consider the same non-negative check (and optionally an upper bound) for defense-in-depth. > > **5. Tests.** (non-blocking) The new parametrized test pins the wiring well — including the negative leg `(use_cache=True, force_refresh=False) → force=False`, which is what distinguishes correct behavior from over-forcing — and asserts `custom_cache_timeout` threads through. It exercises `_query_from_form_data` only and does not assert the `refreshed` metadata semantics that commit 2 fixed; a small follow-up test there would lock in codeant's fix. > > **6. Minor.** `effective_force = request.force_refresh or not request.use_cache` is duplicated at `363` and `1064` — could be one shared helper. Not a blocker. > > Nothing here blocks merge; the only open item is resolving the stale codeant thread. Addressed all of them -- 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]
