codeant-ai-for-open-source[bot] commented on code in PR #37516:
URL: https://github.com/apache/superset/pull/37516#discussion_r3579151570
##########
superset/common/query_object.py:
##########
@@ -141,6 +143,7 @@ def __init__( # pylint: disable=too-many-locals,
too-many-arguments
self.applied_time_extras = applied_time_extras or {}
self.apply_fetch_values_predicate = apply_fetch_values_predicate or
False
self.columns = columns or []
+ self.contribution_totals_query_index = contribution_totals_query_index
Review Comment:
**Suggestion:** The new contribution-producer selector is stored on the
object but never propagated into the query’s serialized identity (used by
`to_dict()`/cache-key generation), so requests that differ only by producer
index can hit the same cache entry and return contribution values computed from
the wrong totals query. Include this field in the serialized query identity so
cache keys and query equivalence checks change when the producer index changes.
[cache]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Contribution table charts return incorrect values under caching.
- ⚠️ /api/v1/chart/data responses misrepresent contribution totals.
- ⚠️ Query-level cache ignores contribution_totals_query_index differences.
- ⚠️ Dashboard async queries can serve stale contribution results.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Create a chart using the table viz with contribution post-processing so
the frontend
sets `contribution_totals_query_index` on the primary query (see
`superset-frontend/plugins/plugin-chart-table/src/buildQuery.ts:15-21`, where
`queryPlan[0]` is assigned `contribution_totals_query_index:
queryPlan.length` and a
totals query is pushed).
2. Submit the chart’s form data to `/api/v1/chart/data` so
`ChartDataRestApi.get_data` in
`superset/charts/data/api.py:111-245` builds a JSON query context, validates
it via
`ChartDataQueryContextSchema` (field defined at
`superset/charts/schemas.py:1400-26`), and
constructs a `QueryContext` whose `queries` list contains `QueryObject`
instances.
3. When the query context is built, `QueryObjectFactory` in
`superset/common/query_object_factory.py:80-16` passes
`contribution_totals_query_index`
through `**kwargs` into `QueryObject.__init__`, which stores it on the object
(`superset/common/query_object.py:114-147`, specifically line 146
`self.contribution_totals_query_index = contribution_totals_query_index`).
4. During execution, `QueryContextProcessor._execute_query_plan` in
`superset/common/query_context_processor.py:199-260` calls
`_contribution_plan`, which
reads `query.contribution_totals_query_index` at lines 34-38 to select a
totals producer
for each contribution consumer; `_with_contribution_totals` at
`superset/common/query_context_processor.py:21-31` then injects the chosen
totals into the
consumer query’s `post_processing`.
5. For each query (including the contribution consumer),
`QueryContextProcessor.get_df_payload_result` at
`superset/common/query_context_processor.py:139-167` computes a cache key by
calling
`self.query_cache_key(query_obj)`. `query_cache_key` (lines 144-162 in the
same file)
builds `extra_cache_keys =
datasource.get_extra_cache_keys(query_obj.to_dict())` and then
calls `query_obj.cache_key(...)`, so the cache identity depends on
`QueryObject.to_dict()`
plus extras.
6. Inspect `QueryObject.to_dict()` in
`superset/common/query_object.py:394-420`: it
includes many fields (`columns`, `extras`, `filter`, `is_timeseries`,
`post_processing`,
etc.) but does not include `contribution_totals_query_index`, even though it
is a stored
attribute (class attribute at line 88 and instance assignment at line 146).
7. Inspect `QueryObject.cache_key()` in
`superset/common/query_object.py:430-510`: it
starts from `cache_dict: dict[str, Any] = dict(self.to_dict())` and then
mutates
`cache_dict` (adding `datasource`, `result_type`, `time_range`, sanitized
`post_processing`, `time_offsets`, annotation layers, impersonation keys).
Because
`contribution_totals_query_index` is not in `to_dict()` nor added later, two
`QueryObject`
instances that differ only in `contribution_totals_query_index` produce the
same
`cache_dict` and therefore the same `cache_key`.
8. With caching enabled (non-`CACHE_DISABLED_TIMEOUT` and
`force_cached=False`), make two
`/api/v1/chart/data` requests whose `queries[0]` objects are identical
except for
`contribution_totals_query_index` pointing at different totals queries in
the same context
(this is supported by the generic contribution planner in
`superset/common/query_context_processor.py:19-55`). On the first request,
`QueryCacheManager.get` in `get_df_payload_result` (lines 158-164) misses
and executes the
query with one producer, storing the dataframe with contributions based on
that totals
query under a cache key that omits the index. On the second request,
`QueryCacheManager.get` sees `cache.is_loaded` for the same key (lines
159-164, 182-183)
and returns the cached dataframe without re-executing, so the consumer
query’s response
reuses contribution values computed from the old producer even though
`contribution_totals_query_index` now selects a different totals query,
demonstrating that
index changes are not reflected in cache identity.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=ad47a4c5e9954c1b828e79bee5393950&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=ad47a4c5e9954c1b828e79bee5393950&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/common/query_object.py
**Line:** 146:146
**Comment:**
*Cache: The new contribution-producer selector is stored on the object
but never propagated into the query’s serialized identity (used by
`to_dict()`/cache-key generation), so requests that differ only by producer
index can hit the same cache entry and return contribution values computed from
the wrong totals query. Include this field in the serialized query identity so
cache keys and query equivalence checks change when the producer index changes.
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
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37516&comment_hash=24c4cce8b916597b49d96f5ee13e6828f19b9aaa5fb384fcc617cdece9181266&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37516&comment_hash=24c4cce8b916597b49d96f5ee13e6828f19b9aaa5fb384fcc617cdece9181266&reaction=dislike'>👎</a>
##########
superset/connectors/sqla/models.py:
##########
@@ -2202,7 +2207,7 @@ class and any keys added via `ExtraCache`.
# Add each predicate as a separate cache key component
extra_cache_keys.extend(rls_predicates)
- return list(set(extra_cache_keys))
+ return list(dict.fromkeys(extra_cache_keys))
Review Comment:
**Suggestion:** Deduplicating with `dict.fromkeys` assumes every cache key
is hashable, but `ExtraCache.cache_key_wrapper` accepts `Any` and can add
unhashable values (for example lists/dicts from custom Jinja helpers), which
will raise `TypeError` and break chart-data cache key generation at runtime.
Use a dedupe strategy that handles unhashable entries safely (or normalize keys
before deduping) so cache-key construction cannot crash. [type error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
❌ Chart data endpoint fails for templates using unhashable cache keys.
⚠️ Query caching breaks when Jinja cache_key_wrapper misused.
⚠️ Affected charts cannot load, confusing end users.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure a SQL dataset (SqlaTable) whose SQL or column/metric expression
uses the
Jinja macro `cache_key_wrapper` with an unhashable value, for example `{{
cache_key_wrapper([1, 2]) }}`; this is possible because `cache_key_wrapper`
is exposed to
templates in `superset/jinja_context.py:935-13` and ultimately calls
`ExtraCache.cache_key_wrapper(key: Any)` in
`superset/jinja_context.py:229-28`.
2. When the dataset is queried via a chart (for example through
`/api/v1/chart/data`), a
`QueryContext` is constructed with `datasource` set to the `SqlaTable` and
`queries`
containing the `QueryObject`, as shown in
`superset/common/query_context.py:8-30`; the
chart code then calls `QueryContext.get_df_payload_result()` (or
`get_payload()`), which
delegates to `QueryContextProcessor.get_df_payload_result()` in
`superset/common/query_context_processor.py:140-179`.
3. Inside `QueryContextProcessor.get_df_payload_result`, the processor calls
`self.query_cache_key(query_obj)` at
`superset/common/query_context_processor.py:14-15`;
`query_cache_key` (implemented at
`superset/common/query_context_processor.py:353-361`) in
turn calls `datasource.get_extra_cache_keys(query_obj.to_dict())` where
`datasource` is
the `SqlaTable` instance.
4. The `SqlaTable.get_extra_cache_keys` override in
`superset/connectors/sqla/models.py:2172-2210` starts from `extra_cache_keys
=
super().get_extra_cache_keys(query_obj)` and then appends
`sqla_query.extra_cache_keys`
(built in `superset/models/helpers.py:3360-3375` using a `list[Any]`
collected by Jinja
`ExtraCache` and `cache_key_wrapper`). When it reaches `return
list(dict.fromkeys(extra_cache_keys))` at
`superset/connectors/sqla/models.py:2210`,
Python attempts to hash each element in `extra_cache_keys`; the unhashable
list or dict
added via `cache_key_wrapper` causes `TypeError: unhashable type 'list'` (or
similar), so
cache key generation fails and the chart-data request crashes before hitting
the database,
returning a 500 error.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=92a733c8298c40fbabf59bc873de8c49&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=92a733c8298c40fbabf59bc873de8c49&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/connectors/sqla/models.py
**Line:** 2210:2210
**Comment:**
*Type Error: Deduplicating with `dict.fromkeys` assumes every cache key
is hashable, but `ExtraCache.cache_key_wrapper` accepts `Any` and can add
unhashable values (for example lists/dicts from custom Jinja helpers), which
will raise `TypeError` and break chart-data cache key generation at runtime.
Use a dedupe strategy that handles unhashable entries safely (or normalize keys
before deduping) so cache-key construction cannot crash.
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
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37516&comment_hash=e17d090c02b987dfc6c2b689cf58bb53904951a7aff5d3ac2dcd4e011fbc4d03&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37516&comment_hash=e17d090c02b987dfc6c2b689cf58bb53904951a7aff5d3ac2dcd4e011fbc4d03&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]
