codeant-ai-for-open-source[bot] commented on code in PR #37516:
URL: https://github.com/apache/superset/pull/37516#discussion_r3657287983
##########
superset/charts/data/api.py:
##########
@@ -435,7 +472,10 @@ def _run_async(
# but only if we're not forcing a refresh.
if not form_data.get("force"):
with contextlib.suppress(ChartDataCacheLoadError):
- result = command.run(force_cached=True)
+ with chart_timing_phase("query"):
Review Comment:
**Suggestion:** The cache-hit execution now raises
`ChartDataQueryFailedError` when a cached query payload contains an error, but
this exception is not suppressed or handled in `_run_async`. As a result, an
error stored in the cache can escape the request and produce an internal server
error instead of returning the expected chart error response or falling back to
asynchronous execution. Handle this exception explicitly in the cache-hit path.
[error handling]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Async cache-hit requests can return unhandled server errors.
- ⚠️ Cached chart failures bypass standard 400 handling.
- ⚠️ Users may receive inconsistent errors for identical queries.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Enable `GLOBAL_ASYNC_QUERIES` and send a normal JSON, full-result request
to `GET
/<int:pk>/data/` or `POST /data/`, handled by `ChartDataRestApi.get_data()`
at
`superset/charts/data/api.py:102` or `ChartDataRestApi.data()` at
`superset/charts/data/api.py:287`.
2. Use a request whose query context satisfies the async conditions in
`superset/charts/data/api.py:263-269` or
`superset/charts/data/api.py:371-377`, including
`_supports_async_execution(query_context)`, and whose result is already
represented by a
cache entry.
3. The request enters `ChartDataRestApi._run_async()` at
`superset/charts/data/api.py:462`; the cache-hit branch calls
`command.execute(ChartDataExecutionOptions(force_cached=True))` at
`superset/charts/data/api.py:475-478`.
4. If the cached query payload represents a failed query,
`command.execute()` raises
`ChartDataQueryFailedError`. The surrounding `contextlib.suppress()` only
suppresses
`ChartDataCacheLoadError` at `superset/charts/data/api.py:473-475`, so the
query failure
escapes `_run_async()` instead of reaching the existing
`ChartDataQueryFailedError`
response handling in `_get_data_response()` at
`superset/charts/data/api.py:647-650`,
producing an unhandled server error.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=552803c8e96a4534af49defe30d1f461&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=552803c8e96a4534af49defe30d1f461&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/charts/data/api.py
**Line:** 475:484
**Comment:**
*Error Handling: The cache-hit execution now raises
`ChartDataQueryFailedError` when a cached query payload contains an error, but
this exception is not suppressed or handled in `_run_async`. As a result, an
error stored in the cache can escape the request and produce an internal server
error instead of returning the expected chart error response or falling back to
asynchronous execution. Handle this exception explicitly in the cache-hit path.
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=93c60e9dc4a02cbf7de6ad0ecb91f2140b8ada4b62d0939e045cdfe4edd5c72b&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37516&comment_hash=93c60e9dc4a02cbf7de6ad0ecb91f2140b8ada4b62d0939e045cdfe4edd5c72b&reaction=dislike'>👎</a>
##########
superset/common/query_context_processor.py:
##########
@@ -396,9 +475,362 @@ def get_payload(
},
self.get_cache_timeout(),
)
- return_value["cache_key"] = cache_key # type: ignore
+ return QueryContextExecutionResult(
+ queries=query_results,
+ cache_key=cache_key,
+ context_cache_write_outcome=context_cache_write_outcome,
+ )
- return return_value
+ def _execute_query_plan(
+ self,
+ force_cached: bool,
+ materialize: bool,
+ ) -> tuple[QueryDataResult, ...]:
+ """Execute one dependency-aware plan for every response mode."""
+
+ contribution_plan = self._contribution_plan()
+ requested_result_types = {
+ query_idx: get_effective_result_type(self._query_context, query)
+ for query_idx, query in enumerate(self._query_context.queries)
+ }
+ contribution_dependencies = self._acquire_contribution_dependencies(
+ contribution_plan,
+ requested_result_types,
+ force_cached,
+ materialize,
+ )
+ output_acquisitions = dict(contribution_dependencies.reusable_outputs)
+ query_results: dict[int, QueryDataResult] = {}
+
+ for query_idx, query_obj in enumerate(self._query_context.queries):
+ if query_idx in output_acquisitions:
+ continue
+ result_type = requested_result_types[query_idx]
+ if not is_data_result_type(result_type):
+ query_results[query_idx] = (
+ get_query_results_with_timing(
+ result_type,
+ self._query_context,
+ query_obj,
+ force_cached,
+ )
+ if materialize
+ else get_query_results_cache_only(
+ result_type,
+ self._query_context,
+ query_obj,
+ )
+ )
+ continue
+
+ totals_idx =
contribution_dependencies.consumer_to_producer.get(query_idx)
+ totals = (
+ contribution_dependencies.totals_by_producer.get(totals_idx)
+ if totals_idx is not None
+ else None
+ )
+ if totals_idx is not None and (
+ totals is None
+ or not self._totals_support_consumer(query_obj, totals.values)
+ ):
+ query_results[query_idx] = self._dependency_failed_result(
+ _("Contribution totals query failed")
+ )
+ continue
+ execution_query = (
+ self._with_contribution_totals(
+ query_obj,
+ totals.values,
+ )
+ if totals is not None
+ else query_obj
+ )
+ acquired = acquire_query_data(
+ result_type,
+ self._query_context,
+ execution_query,
+ force_cached if materialize else False,
+ detect_currency_value=materialize,
+ cache_key_extra=(
+ {"contribution_totals_cache_key": totals.cache_key}
+ if totals is not None
+ else None
+ ),
+ )
+ if acquired is not None:
+ output_acquisitions[query_idx] = acquired
+ continue
+ raise QueryObjectValidationError(
+ _("Data-backed result type did not produce a dataframe")
+ )
+
+ for query_idx, acquired in output_acquisitions.items():
+ query_results[query_idx] = (
+ materialize_acquired_query(self._query_context, acquired)
+ if materialize
+ else cache_acquired_query(acquired)
+ )
+ return tuple(
+ query_results[query_idx]
+ for query_idx in range(len(self._query_context.queries))
+ )
+
+ def _acquire_contribution_dependencies(
+ self,
+ contribution_plan: dict[int, int],
+ requested_result_types: dict[int, ChartDataResultType],
+ force_cached: bool,
+ materialize: bool,
+ ) -> _ContributionDependencies:
+ """Acquire dataframe producers needed by data-backed consumers."""
+
+ data_plan: dict[int, int] = {
+ consumer_idx: producer_idx
+ for consumer_idx, producer_idx in contribution_plan.items()
+ if is_data_result_type(requested_result_types[consumer_idx])
+ }
+ reusable_outputs: dict[int, AcquiredQuery] = {}
+ totals: dict[int, _ContributionTotals] = {}
+ for producer_idx in sorted(set(data_plan.values())):
+ producer = self._query_context.queries[producer_idx]
+ requested_result_type = requested_result_types[producer_idx]
+ dependency_query = self._totals_query(producer_idx)
+ reuse_for_output = (
+ requested_result_type in _REUSABLE_CONTRIBUTION_RESULT_TYPES
+ and self._same_acquisition_identity(producer, dependency_query)
+ )
+ if reuse_for_output:
+ dependency_query = producer
+ dependency_result_type = requested_result_type
+ else:
+ dependency_query.result_type = ChartDataResultType.FULL
+ dependency_result_type = ChartDataResultType.FULL
+ acquired = acquire_query_data(
Review Comment:
**Suggestion:** This assignment mutates the original producer query object
stored in `self._query_context.queries`. When the producer is not reusable, its
result type is permanently changed to `FULL`, so the later response-processing
loop can return a full dataframe payload instead of the producer's requested
result type and can also affect subsequent execution of the same query context.
Set the result type only on a copied query. [state inconsistency]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Contribution query responses can use an unintended full-data result
format.
- ⚠️ Reused query contexts retain mutated producer result-type state.
- ⚠️ Response timing and materialization can diverge from requested result
types.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Send a chart-data request through the chart data API at
`superset/charts/data/api.py`,
which constructs a `QueryContext` and executes
`QueryContextProcessor.get_payload_result()` in
`superset/common/query_context_processor.py`.
2. Use a query context containing a contribution consumer whose producer
query is not
reusable, causing `_acquire_contribution_dependencies()` at
`superset/common/query_context_processor.py:596-608` to select the
producer's totals query
and enter the `else` branch.
3. At `superset/common/query_context_processor.py:606`, the code assigns
`dependency_query.result_type = ChartDataResultType.FULL`; when the
dependency query is
the original producer object, this mutates the object stored in
`self._query_context.queries`.
4. The later loop at `superset/common/query_context_processor.py:505-576`
still uses the
result type captured before the mutation, but passes the mutated query
object into
`acquire_query_data()` at `superset/common/query_context_processor.py:548`;
the producer
can therefore be executed or materialized with inconsistent result-type
state, and the
same query context retains the unintended `FULL` value for subsequent use.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=f849e83ebbbd4e0e93ba19f5779d077a&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=f849e83ebbbd4e0e93ba19f5779d077a&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_context_processor.py
**Line:** 606:608
**Comment:**
*State Inconsistency: This assignment mutates the original producer
query object stored in `self._query_context.queries`. When the producer is not
reusable, its result type is permanently changed to `FULL`, so the later
response-processing loop can return a full dataframe payload instead of the
producer's requested result type and can also affect subsequent execution of
the same query context. Set the result type only on a copied query.
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=9ac75d51e97db7f2b73ccd276bf1975586cadb928a84fa17ffda4d20b432fb84&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37516&comment_hash=9ac75d51e97db7f2b73ccd276bf1975586cadb928a84fa17ffda4d20b432fb84&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]
