Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(cancel-query-request): added abort query request functionality after submitting a query #18387

Merged
merged 7 commits into from
Jun 16, 2020
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

### Features

1. [18387](https://github.com/influxdata/influxdb/pull/18387): Integrate query cancellation after queries have been submitted

## v2.0.0-beta.12 [2020-06-12]

### Features
Expand Down
20 changes: 19 additions & 1 deletion ui/jestSetup.ts
Original file line number Diff line number Diff line change
@@ -1,13 +1,31 @@
import {cleanup} from '@testing-library/react'
import 'intersection-observer'
import MutationObserver from 'mutation-observer'
import fetchMock from 'jest-fetch-mock'

// global vars
process.env.API_PREFIX = 'http://example.com/'

declare global {
interface Window {
flushAllPromises: () => Promise<any>
MutationObserver: MutationObserver
}
}

// Adds MutationObserver as a polyfill for testing
window.MutationObserver = MutationObserver

window.flushAllPromises = async () => {
return new Promise(resolve => setImmediate(resolve))
}

// mocks and stuff
fetchMock.enableMocks()
jest.mock('honeybadger-js', () => () => null)

process.env.API_PREFIX = '/'
// cleans up state between @testing-library/react tests
afterEach(() => {
cleanup()
fetchMock.resetMocks()
})
5 changes: 3 additions & 2 deletions ui/src/shared/apis/query.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,8 @@ export interface RunQueryErrorResult {
export const runQuery = (
orgID: string,
query: string,
extern?: File
extern?: File,
abortController?: AbortController
): CancelBox<RunQueryResult> => {
const url = `${API_BASE_PATH}api/v2/query?${new URLSearchParams({orgID})}`

Expand All @@ -51,7 +52,7 @@ export const runQuery = (
dialect: {annotations: ['group', 'datatype', 'default']},
}

const controller = new AbortController()
const controller = abortController || new AbortController()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think you should cut this since it's duplicated in runQuery and the result from there is passed in here. caveat emptor to anyone using this method - they'll need to pass in an abort controller - that's fine, since you're the only one using it right now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good eyes, I totally didn't see that. Will do


const request = fetch(url, {
method: 'POST',
Expand Down
5 changes: 5 additions & 0 deletions ui/src/shared/copy/notifications.ts
Original file line number Diff line number Diff line change
Expand Up @@ -529,6 +529,11 @@ export const resourceLimitReached = (resourceName: string): Notification => ({
type: 'resourceLimitReached',
})

export const queryCancelRequest = (): Notification => ({
...defaultSuccessNotification,
message: `Cancelling query...`,
})

export const taskNotCreated = (additionalMessage: string): Notification => ({
...defaultErrorNotification,
message: `Failed to create new task: ${additionalMessage}`,
Expand Down
25 changes: 16 additions & 9 deletions ui/src/timeMachine/actions/queries.ts
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,10 @@ const isFromBucket = (node: Node) => {
)
}

export const executeQueries = () => async (dispatch, getState: GetState) => {
export const executeQueries = (abortController?: AbortController) => async (
dispatch,
getState: GetState
) => {
reportSimpleQueryPerformanceEvent('executeQueries function start')

const state = getState()
Expand Down Expand Up @@ -160,9 +163,8 @@ export const executeQueries = () => async (dispatch, getState: GetState) => {

const extern = buildVarsOption(variableAssignments)

return runQuery(orgID, text, extern)
return runQuery(orgID, text, extern, abortController)
})

const results = await Promise.all(pendingResults.map(r => r.promise))
reportSimpleQueryPerformanceEvent('executeQueries queries end')

Expand Down Expand Up @@ -209,13 +211,15 @@ export const executeQueries = () => async (dispatch, getState: GetState) => {
setQueryResults(RemoteDataState.Done, files, duration, null, statuses)
)
reportSimpleQueryPerformanceEvent('executeQueries function start')
} catch (e) {
if (e.name === 'CancellationError') {
return results
} catch (error) {
if (error.name === 'CancellationError' || error.name === 'AbortError') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This showed up cause of testing! There are situations where the request can be canceled before the other the catch in runQuery has a chance to catch it, so it would wind up here without properly being converted into a CancellationError

dispatch(setQueryResults(RemoteDataState.Done, null, null))
return
}

console.error(e)
dispatch(setQueryResults(RemoteDataState.Error, null, null, e.message))
console.error(error)
dispatch(setQueryResults(RemoteDataState.Error, null, null, error.message))
}
}

Expand All @@ -227,9 +231,12 @@ const saveDraftQueries = (): SaveDraftQueriesAction => ({
type: 'SAVE_DRAFT_QUERIES',
})

export const saveAndExecuteQueries = () => dispatch => {
export const saveAndExecuteQueries = (
abortController?: AbortController
) => dispatch => {
dispatch(saveDraftQueries())
dispatch(executeQueries())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you dispatch(setQueryToLoading) here instead of as part of the private handleClick = (): void => { method in submit button?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds great!

dispatch(setQueryResults(RemoteDataState.Loading, [], null))
dispatch(executeQueries(abortController))
}

export const executeCheckQuery = () => async (dispatch, getState: GetState) => {
Expand Down
202 changes: 202 additions & 0 deletions ui/src/timeMachine/components/SubmitQueryButton.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,202 @@
// Libraries
import React from 'react'
import {mocked} from 'ts-jest/utils'
import {fireEvent} from '@testing-library/react'

import {renderWithRedux} from 'src/mockState'
import {RemoteDataState} from 'src/types'

declare global {
interface Window {
TextDecoder: any
}
}

class FakeTextDecoder {
decode() {
return ''
}
}

window.TextDecoder = FakeTextDecoder

jest.mock('src/external/parser', () => {
return {
parse: jest.fn(() => {
return {
type: 'File',
package: {
name: {
name: 'fake',
type: 'Identifier',
},
type: 'PackageClause',
},
imports: [],
body: [],
}
}),
}
})

jest.mock('src/variables/actions/thunks', () => {
return {
hydrateVariables: jest.fn(() => {
return (_dispatch, _getState) => {
return Promise.resolve()
}
}),
}
})

import SubmitQueryButton from 'src/timeMachine/components/SubmitQueryButton'

const stateOverride = {
timeMachines: {
activeTimeMachineID: 'veo',
timeMachines: {
veo: {
draftQueries: [
{
text: `from(bucket: "apps")
|> range(start: v.timeRangeStart, stop: v.timeRangeStop)
|> filter(fn: (r) => r["_measurement"] == "rum")
|> filter(fn: (r) => r["_field"] == "domInteractive")
|> map(fn: (r) => ({r with _value: r._value / 1000.0}))
|> group()`,
},
],
activeQueryIndex: 0,
queryResults: {
status: RemoteDataState.NotStarted,
},
view: {
properties: {
queries: [{text: 'draftstate'}],
},
},
},
},
},
}

describe('TimeMachine.Components.SubmitQueryButton', () => {
beforeEach(() => {
jest.useFakeTimers()
})
afterEach(() => {
jest.useRealTimers()
})

it('it changes the Submit button to Cancel when the request is in flight, then back to Submit after the request has resolved', async () => {
const fakeReader = {
cancel: jest.fn(),
read: jest.fn(() => {
return Promise.resolve({
done: true,
})
}),
}

const fakeResponse = {
status: 200,
body: {
getReader: () => fakeReader,
},
}

const expectedMockedFetchCall = {
method: 'POST',
headers: {'Content-Type': 'application/json', 'Accept-Encoding': 'gzip'},
body: JSON.stringify({
query: stateOverride.timeMachines.timeMachines.veo.draftQueries[0].text,
extern: {
type: 'File',
package: null,
imports: null,
body: [
{
type: 'OptionStatement',
assignment: {
type: 'VariableAssignment',
id: {type: 'Identifier', name: 'v'},
init: {
type: 'ObjectExpression',
properties: [
{
type: 'Property',
key: {type: 'Identifier', name: 'timeRangeStart'},
value: {
type: 'UnaryExpression',
operator: '-',
argument: {
type: 'DurationLiteral',
values: [{magnitude: 1, unit: 'h'}],
},
},
},
{
type: 'Property',
key: {type: 'Identifier', name: 'timeRangeStop'},
value: {
type: 'CallExpression',
callee: {type: 'Identifier', name: 'now'},
},
},
],
},
},
},
],
},
dialect: {annotations: ['group', 'datatype', 'default']},
}),
signal: new AbortController().signal,
}

mocked(fetch).mockImplementation(() => {
return Promise.resolve(fakeResponse)
})
const {getByTitle} = renderWithRedux(<SubmitQueryButton />, s => ({
...s,
...stateOverride,
}))

fireEvent.click(getByTitle('Submit'))
expect(getByTitle('Cancel')).toBeTruthy()
await window.flushAllPromises()

expect(mocked(fetch)).toHaveBeenCalledWith(
'http://example.com/api/v2/query?orgID=orgid',
expectedMockedFetchCall
)
expect(getByTitle('Submit')).toBeTruthy()
})

it("cancels the query after submission if the query hasn't finished and resolved", async () => {
mocked(fetchMock).mockResponse(() => {
return new Promise((resolve, _reject) => {
setTimeout(() => {
resolve('')
}, 3000)
})
})

const {getByTitle} = renderWithRedux(<SubmitQueryButton />, s => ({
...s,
...stateOverride,
}))
const SubmitBtn = getByTitle('Submit')
fireEvent.click(SubmitBtn)

const CancelBtn = getByTitle('Cancel')
fireEvent.click(CancelBtn)
await window.flushAllPromises()

const {type, value: error} = mocked(fetch).mock.results[0] as any
expect(type).toBe('throw')
expect(error.name).toBe('AbortError')

expect(getByTitle('Submit')).toBeTruthy()
})
})
Loading