bito-code-review[bot] commented on code in PR #44552:
URL: https://github.com/apache/superset/pull/44552#discussion_r4078498284
##########
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:
<div>
<div id="suggestion">
<div id="issue"><b>CSRF token missing on POST</b></div>
<div id="fix">
`SupersetClient.post` here bypasses the CSRF token that
`SupersetClient.configure` injects via `postHeaders`/`csrfHeader` —
`chartAction.ts` and `postStopQuery` (SqlLab) always send it, so this request
can 403 on CSRF validation. It also duplicates the stop-request logic that
exists in `chartAction.ts`/`sqlLab.ts`. Extract a shared `stopQuery` helper and
reuse it here.
</div>
</div>
<div id="suggestion">
<div id="issue"><b>Swallowed stop-request error</b></div>
<div id="fix">
The `.catch` swallows the actual error object and shows only a generic
toast, so server-side failures (403/500/network) leave no diagnostic trace in
the console or logger. `logging` is already imported in this file (line 53) —
log the error before showing the toast.
</div>
</div>
<small><i>Code Review Run #1ec09f</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
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)
+ return inside
+
+
+def test_registry_key_is_scoped_by_user() -> None:
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing test docstrings</b></div>
<div id="fix">
Nine new test functions have no docstring:
`test_registry_key_is_scoped_by_user`,
`..._publishes_then_discards_the_handle`, the three `is_noop` tests,
`..._publishes_nothing_when_engine_has_no_cancel_id`,
`..._cancels_its_own_query`, and the two `returns_false` tests. BITO.md rule
12148 requires a docstring per new test and says inline comments do not
substitute.
</div>
</div>
<small><i>Code Review Run #1ec09f</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
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:
<div>
<div id="suggestion">
<div id="issue"><b>Unannotated test locals</b></div>
<div id="fix">
Locals in the new section are unannotated: `inside` here, plus `cache`,
`database`, and `victim_handle` throughout the new test bodies. BITO.md rule
13153 requires explicit annotations on all test-file locals even when
inferable. Annotate e.g. `inside: dict[str, object]`, `cache: _FakeCache`,
`database: MagicMock`, `victim_handle: dict[str, object]`.
</div>
</div>
<small><i>Code Review Run #1ec09f</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
