codeant-ai-for-open-source[bot] commented on code in PR #38910:
URL: https://github.com/apache/superset/pull/38910#discussion_r3000851752
##########
superset/common/query_context_processor.py:
##########
@@ -385,6 +385,27 @@ def get_payload(
def get_cache_timeout(self) -> int:
if cache_timeout_rv := self._query_context.get_cache_timeout():
Review Comment:
**Suggestion:** The cache timeout check uses a truthiness test, so a valid
timeout value of 0 seconds will be treated as "no timeout" and incorrectly fall
back to other defaults instead of honoring the explicitly configured zero
timeout; compare against None instead to distinguish "unset" from "zero".
[logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Chart-level cache_timeout=0 cannot be honored in practice.
- ⚠️ Priority chain in QueryContext.get_cache_timeout effectively bypassed
for zero.
- ⚠️ May lead to longer-than-expected caching for configured charts.
```
</details>
```suggestion
cache_timeout_rv = self._query_context.get_cache_timeout()
if cache_timeout_rv is not None:
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure a chart's cache timeout to 0 seconds via the Charts API or UI,
which writes
`cache_timeout = 0` to the `Slice` model (field exposed in
`superset/charts/schemas.py:82-84` and stored on the `Slice` object used by
`QueryContextFactory` at `superset/common/query_context_factory.py:22-27`).
2. Build a query payload for that chart (e.g., via the Explore UI or a test
using
`ChartDataQueryContextSchema().load(payload)` as in
`superset/tests/integration_tests/query_context_tests.py:60-68`), so that
`QueryContext`
is created with `slice_` pointing to the chart and `custom_cache_timeout`
left as `None`.
3. When `QueryContext.get_cache_timeout()` is called at
`superset/common/query_context.py:12-30`, it sees `self.custom_cache_timeout
is None` and
`self.slice_.cache_timeout` is `0`, so it returns `0` as the chart-level
timeout according
to the documented priority order.
4. `QueryContextProcessor.get_cache_timeout()` at
`superset/common/query_context_processor.py:385-416` evaluates `if
cache_timeout_rv :=
self._query_context.get_cache_timeout():` with `cache_timeout_rv == 0`,
which is falsy, so
it does not return 0 and instead falls through to
`DATA_CACHE_CONFIG["CACHE_DEFAULT_TIMEOUT"]` or `CACHE_DEFAULT_TIMEOUT`,
causing the
effective cache timeout in the response (used in `get_df_payload()` and
surfaced to
`/api/v1/chart/data`) to ignore the chart's explicit 0 configuration.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/common/query_context_processor.py
**Line:** 386:386
**Comment:**
*Logic Error: The cache timeout check uses a truthiness test, so a
valid timeout value of 0 seconds will be treated as "no timeout" and
incorrectly fall back to other defaults instead of honoring the explicitly
configured zero timeout; compare against None instead to distinguish "unset"
from "zero".
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%2F38910&comment_hash=7bd08c702bd80ab298287168133d836de0279628a4c43cace7141dab66abbb3c&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38910&comment_hash=7bd08c702bd80ab298287168133d836de0279628a4c43cace7141dab66abbb3c&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]
