bito-code-review[bot] commented on code in PR #44552:
URL: https://github.com/apache/superset/pull/44552#discussion_r4078642368
##########
superset-frontend/src/explore/components/ExploreViewContainer/index.tsx:
##########
@@ -584,9 +585,26 @@ function ExploreViewContainer(props:
ExploreViewContainerProps) {
);
function onStop() {
- if (props.chart && props.chart.queryController) {
+ // Abort the in-flight HTTP request so the UI stops waiting immediately.
+ if (props.chart?.queryController) {
props.chart.queryController.abort();
}
+
+ // Aborting only drops the response; the database keeps executing the
query.
+ // Ask the backend to cancel it too. The backend resolves `client_id`
within
+ // the requesting user's own in-flight queries, so this can only ever
cancel
+ // our own query. A falsy `stopped` just means there was nothing running to
+ // cancel (or the engine has no cancel support), which is not an error.
+ const clientId = props.chart?.latestQueryId;
+ if (clientId) {
+ SupersetClient.post({
+ endpoint: '/api/v1/chart/data/stop',
+ body: JSON.stringify({ client_id: clientId }),
+ headers: { 'Content-Type': 'application/json' },
+ }).catch(() => {
+ props.addDangerToast(t('Failed to stop query.'));
+ });
+ }
Review Comment:
<!-- Bito Reply -->
The suggestion to extract a shared `stopQuery` helper is appropriate, as it
promotes code reuse and ensures consistent handling of CSRF headers across the
application. Regarding the error handling, logging the error before displaying
the toast is a good practice that improves observability for server-side
failures.
##########
tests/unit_tests/tasks/test_query_cancel.py:
##########
@@ -205,3 +206,208 @@ def test_capture_is_noop_without_a_database() -> None:
with _capture_query_cancellation(qc):
assert _cancel_id_sink.get() is None
get_context.assert_not_called()
+
+
+# --- synchronous Explore cancellation registry ----------------------------
+
+
+class _FakeCache:
+ """Minimal dict-backed stand-in for ``cache_manager.cache``.
+
+ Real storage (rather than a MagicMock) so the user-scoping of the registry
+ keys is genuinely exercised end to end.
+ """
+
+ def __init__(self) -> None:
+ self.store: dict[str, object] = {}
+
+ def set(self, key: str, value: object, timeout: int | None = None) -> None:
+ self.store[key] = value
+
+ def get(self, key: str) -> object | None:
+ return self.store.get(key)
+
+ def delete(self, key: str) -> None:
+ self.store.pop(key, None)
+
+
+@contextmanager
+def _registry_env(user_id: int | None, cache: "_FakeCache"):
+ """Run with a given current user and a shared fake cache backend."""
+ # ``cache_manager.cache`` is a read-only property; swap the backing
attribute.
+ with (
+ patch("superset.utils.core.get_user_id", return_value=user_id),
+ patch("superset.extensions.cache_manager._cache", cache),
+ patch("superset.tasks.query_cancel._registry_ttl", return_value=60),
+ ):
+ yield
+
+
+def _run_cancellable(client_id, database, cache, user_id,
cancel_id="engine-1"):
+ """Drive one cancellable query, returning whether a handle was live inside
it."""
+ from superset.tasks.query_cancel import cancellable_chart_query
+
+ with _registry_env(user_id, cache):
+ with patch(
+ "superset.tasks.query_cancel.capture_cancel_query_id",
+ return_value=cancel_id,
+ ):
+ with cancellable_chart_query(client_id, database):
+ # Stands in for Database._execute_sql_with_mutation_and_logging
+ # handing the live cursor to the active sink before executing.
+ notify_cursor(MagicMock())
+ inside = dict(cache.store)
Review Comment:
<!-- Bito Reply -->
The reviewer's suggestion to annotate local variables in the test file is a
stylistic preference. Since the project does not currently enforce explicit
type annotations for inferable local variables in tests, you are not required
to apply this change. You may choose to ignore this suggestion if it does not
align with the existing codebase conventions.
--
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]
