codeant-ai-for-open-source[bot] commented on code in PR #43464:
URL: https://github.com/apache/superset/pull/43464#discussion_r3844884739
##########
tests/unit_tests/tasks/test_async_queries.py:
##########
@@ -64,6 +67,70 @@ def test_load_chart_data_into_cache_with_error(
)
[email protected]("superset.tasks.async_queries.security_manager")
[email protected]("superset.tasks.async_queries.async_query_manager")
[email protected]("superset.tasks.async_queries.ChartDataQueryContextSchema")
+def test_load_chart_data_into_cache_with_query_failed_error_does_not_reraise(
+ mock_query_context_schema_cls, mock_async_query_manager,
mock_security_manager
+):
+ """
+ ChartDataQueryFailedError maps to a 400 in the synchronous chart/data
+ endpoint (see ChartDataRestApi._get_data_response) - an expected,
+ client-facing validation failure (e.g. a chart still referencing columns
+ a customer has since dropped from the dataset), not an application bug.
+ The task must still report it to the client via update_job, but must not
+ re-raise it - that would surface it a second time as an unhandled Celery
+ task exception.
+ """
+ from superset.tasks.async_queries import load_chart_data_into_cache
+
+ job_metadata = {"user_id": 1}
+ form_data: dict[str, Any] = {}
+ err_message = "Columns missing in dataset: ['foo']"
+ err = ChartDataQueryFailedError(_(err_message))
+
+ mock_security_manager.get_user_by_id.return_value = mock.MagicMock()
+ mock_async_query_manager.STATUS_ERROR = "error"
+ mock_query_context_schema_cls.return_value.load.side_effect = err
+
+ # Should not raise.
+ load_chart_data_into_cache(job_metadata, form_data)
Review Comment:
**Suggestion:** The regression test raises `ChartDataQueryFailedError` from
`ChartDataQueryContextSchema.load`, so it never executes
`ChartDataCommand.run()` or the production logic that converts query payload
errors into this exception. This can pass even if the real chart-data failure
path is not caught correctly; mock `ChartDataCommand.run` to raise the
exception after a valid query context is created, as the cache-load test does.
[possible bug]
<details>
<summary><b>Severity Level:</b> Minor ๐งน</summary>
```mdx
- โ ๏ธ Unit regression test does not cover `ChartDataCommand.run`.
- โ ๏ธ Incorrect exception handling could regress undetected.
- โ ๏ธ Async chart-data error behavior remains weakly protected.
```
</details>
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/tasks/test_async_queries.py
**Line:** 94:97
**Comment:**
*Possible Bug: The regression test raises `ChartDataQueryFailedError`
from `ChartDataQueryContextSchema.load`, so it never executes
`ChartDataCommand.run()` or the production logic that converts query payload
errors into this exception. This can pass even if the real chart-data failure
path is not caught correctly; mock `ChartDataCommand.run` to raise the
exception after a valid query context is created, as the cache-load test does.
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%2F43464&comment_hash=be7da8741149ba5671a62a3f423d6a3f6bd806ae5b75de97a8608499241ae7cd&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43464&comment_hash=be7da8741149ba5671a62a3f423d6a3f6bd806ae5b75de97a8608499241ae7cd&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]
