bito-code-review[bot] commented on code in PR #42305:
URL: https://github.com/apache/superset/pull/42305#discussion_r3665261292
##########
superset-frontend/src/middleware/asyncEvent.test.ts:
##########
@@ -130,6 +130,42 @@ describe('asyncEvent middleware', () => {
expect(fetchMock.callHistory.calls(CACHED_DATA_ENDPOINT)).toHaveLength(1);
});
+ test('rejects with an AbortError and cancels the job when the signal
aborts', async () => {
+ const CANCEL_ENDPOINT = 'glob:*/api/v1/async_event/*/cancel';
+ fetchMock.post(CANCEL_ENDPOINT, { status: 200, body: {} });
+
+ const controller = new AbortController();
+ const promise = asyncEvent.waitForAsyncData(
+ asyncPendingEvent,
+ controller.signal,
+ );
+ controller.abort();
+
+ let error: any = null;
+ try {
+ await promise;
+ } catch (err) {
+ error = err;
+ }
+ expect(error?.name).toBe('AbortError');
+ // The cancel POST is fire-and-forget; let its microtask flush.
+ await new Promise(resolve => setTimeout(resolve, 0));
+ expect(fetchMock.callHistory.calls(CANCEL_ENDPOINT)).toHaveLength(1);
+ });
+
+ test('rejects immediately when given an already-aborted signal', async ()
=> {
+ const CANCEL_ENDPOINT = 'glob:*/api/v1/async_event/*/cancel';
+ fetchMock.post(CANCEL_ENDPOINT, { status: 200, body: {} });
+
+ const controller = new AbortController();
+ controller.abort();
+
+ await expect(
+ asyncEvent.waitForAsyncData(asyncPendingEvent, controller.signal),
+ ).rejects.toMatchObject({ name: 'AbortError' });
+ expect(fetchMock.callHistory.calls(CANCEL_ENDPOINT)).toHaveLength(1);
Review Comment:
<!-- Bito Reply -->
The reviewer's suggestion is appropriate. While the test at line 152 does
await a tick, the new test at line 130 also requires this flush to ensure the
fire-and-forget POST request has completed before the assertion runs,
preventing potential race conditions in the test environment.
**superset-frontend/src/middleware/asyncEvent.test.ts**
```
expect(error?.name).toBe('AbortError');
// The cancel POST is fire-and-forget; let its microtask flush.
await new Promise(resolve => setTimeout(resolve, 0));
expect(fetchMock.callHistory.calls(CANCEL_ENDPOINT)).toHaveLength(1);
```
##########
superset/async_events/api.py:
##########
@@ -99,3 +103,69 @@ def events(self) -> Response:
return self.response_401()
return self.response(200, result=events)
+
+ @expose("/<job_id>/cancel", methods=("POST",))
+ @event_logger.log_this
+ @protect()
+ @safe
+ @statsd_metrics
+ @permission_name("list")
+ def cancel(self, job_id: str) -> Response:
+ """Cancel a running async query job.
+ ---
+ post:
+ summary: Cancel a running async query job
+ description: >-
+ Revokes the Celery task backing an in-flight async query. The
+ caller is authorized against the job's original owner (channel and
+ user), both resolved server-side from the request, so a client
+ cannot cancel a job it did not submit.
+ parameters:
+ - in: path
+ name: job_id
+ required: true
+ description: The job ID returned when the async query was submitted
+ schema:
+ type: string
+ responses:
+ 200:
+ description: Job cancelled
+ content:
+ application/json:
+ schema:
+ type: object
+ properties:
+ result:
+ type: object
+ properties:
+ job_id:
+ type: string
+ status:
+ type: string
+ 401:
+ $ref: '#/components/responses/401'
+ 403:
+ $ref: '#/components/responses/403'
+ 404:
+ $ref: '#/components/responses/404'
+ 500:
+ $ref: '#/components/responses/500'
+ """
+ try:
+ async_channel_id =
async_query_manager.parse_channel_id_from_request(
+ request
+ )
+ except AsyncQueryTokenException:
+ return self.response_401()
+
+ try:
+ async_query_manager.cancel_job(job_id, async_channel_id,
get_user_id())
+ except AsyncQueryTokenException:
+ return self.response_403()
+ except AsyncQueryJobException:
+ return self.response_404()
+
+ return self.response(
+ 200,
+ result={"job_id": job_id, "status":
async_query_manager.STATUS_CANCELLED},
+ )
Review Comment:
<!-- Bito Reply -->
The suggestion to add API-level test coverage for the new `cancel` endpoint
is appropriate. While the manager layer is tested, the API layer handles
request parsing, authorization, and exception mapping, which are critical
components that should be verified to prevent regressions in the cancel flow.
--
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]
