Variables: Handle variable cancellations better (#43987)

Co-authored-by: Ashley Harrison <ashley.harrison@grafana.com>
Co-authored-by: joshhunt <josh@trtr.co>
Co-authored-by: kay delaney <kay@grafana.com>
Co-authored-by: Alexandra Vargas <alexa1866@gmail.com>>
pull/44034/head
Hugo Häggmark 4 years ago committed by GitHub
parent 11fa9801f2
commit 7ffefc069f
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
  1. 34
      public/app/features/variables/interval/actions.test.ts
  2. 13
      public/app/features/variables/query/VariableQueryRunner.ts
  3. 33
      public/app/features/variables/query/actions.test.ts
  4. 10
      public/app/features/variables/query/actions.ts
  5. 27
      public/app/features/variables/state/actions.ts
  6. 27
      public/app/features/variables/state/onTimeRangeUpdated.test.ts
  7. 26
      public/app/features/variables/state/upgradeLegacyQueries.test.ts
  8. 7
      public/app/features/variables/utils.ts

@ -18,6 +18,8 @@ import { intervalBuilder } from '../shared/testing/builders';
import { updateOptions } from '../state/actions';
import { notifyApp } from '../../../core/actions';
import { silenceConsoleOutput } from '../../../../test/core/utils/silenceConsoleOutput';
import { variablesInitTransaction } from '../state/transactionReducer';
import { afterEach, beforeEach } from '../../../../test/lib/common';
describe('interval actions', () => {
variableAdapters.setInit(() => [createIntervalVariableAdapter()]);
@ -43,7 +45,8 @@ describe('interval actions', () => {
describe('when updateOptions is dispatched but something throws', () => {
silenceConsoleOutput();
it('then an notifyApp action should be dispatched', async () => {
const originalTimeSrv = getTimeSrv();
beforeEach(() => {
const timeSrvMock = ({
timeRange: jest.fn().mockReturnValue({
from: dateTime(new Date()).subtract(1, 'days').toDate(),
@ -54,8 +57,14 @@ describe('interval actions', () => {
},
}),
} as unknown) as TimeSrv;
const originalTimeSrv = getTimeSrv();
setTimeSrv(timeSrvMock);
});
afterEach(() => {
setTimeSrv(originalTimeSrv);
});
it('then an notifyApp action should be dispatched', async () => {
const interval = intervalBuilder()
.withId('0')
.withQuery('1s,1m,1h,1d')
@ -66,6 +75,7 @@ describe('interval actions', () => {
const tester = await reduxTester<RootReducerType>()
.givenRootReducer(getRootReducer())
.whenActionIsDispatched(addVariable(toVariablePayload(interval, { global: false, index: 0, model: interval })))
.whenActionIsDispatched(variablesInitTransaction({ uid: 'a uid' }))
.whenAsyncActionIsDispatched(updateOptions(toVariableIdentifier(interval)), true);
tester.thenDispatchedActionsPredicateShouldEqual((dispatchedActions) => {
@ -91,8 +101,26 @@ describe('interval actions', () => {
return dispatchedActions.length === expectedNumberOfActions;
});
});
setTimeSrv(originalTimeSrv);
describe('but there is no ongoing transaction', () => {
it('then no actions are dispatched', async () => {
const interval = intervalBuilder()
.withId('0')
.withQuery('1s,1m,1h,1d')
.withAuto(true)
.withAutoMin('1xyz') // illegal interval string
.build();
const tester = await reduxTester<RootReducerType>()
.givenRootReducer(getRootReducer())
.whenActionIsDispatched(
addVariable(toVariablePayload(interval, { global: false, index: 0, model: interval }))
)
.whenAsyncActionIsDispatched(updateOptions(toVariableIdentifier(interval)), true);
tester.thenNoActionsWhereDispatched();
});
});
});

@ -1,5 +1,5 @@
import { merge, Observable, of, Subject, throwError, Unsubscribable } from 'rxjs';
import { catchError, filter, finalize, first, mergeMap, takeUntil } from 'rxjs/operators';
import { catchError, filter, finalize, mergeMap, take, takeUntil } from 'rxjs/operators';
import {
CoreApp,
DataQuery,
@ -7,6 +7,7 @@ import {
DataSourceApi,
getDefaultTimeRange,
LoadingState,
PanelData,
ScopedVars,
} from '@grafana/data';
@ -115,12 +116,14 @@ export class VariableQueryRunner {
filter(() => {
// Lets check if we started another batch during the execution of the observable. If so we just want to abort the rest.
const afterUid = getState().templating.transaction.uid;
return beforeUid === afterUid;
}),
first((data) => data.state === LoadingState.Done || data.state === LoadingState.Error),
mergeMap((data) => {
filter((data) => data.state === LoadingState.Done || data.state === LoadingState.Error), // we only care about done or error for now
take(1), // take the first result, using first caused a bug where it in some situations throw an uncaught error because of no results had been received yet
mergeMap((data: PanelData) => {
if (data.state === LoadingState.Error) {
return throwError(data.error);
return throwError(() => data.error);
}
return of(data);
@ -148,7 +151,7 @@ export class VariableQueryRunner {
}
this.updateOptionsResults.next({ identifier, state: LoadingState.Error, error });
return throwError(error);
return throwError(() => error);
}),
finalize(() => {
this.updateOptionsResults.next({ identifier, state: LoadingState.Done });

@ -37,6 +37,7 @@ import { silenceConsoleOutput } from '../../../../test/core/utils/silenceConsole
import { getTimeSrv, setTimeSrv, TimeSrv } from '../../dashboard/services/TimeSrv';
import { setVariableQueryRunner, VariableQueryRunner } from './VariableQueryRunner';
import { setDataSourceSrv } from '@grafana/runtime';
import { variablesInitTransaction } from '../state/transactionReducer';
const mocks: Record<string, any> = {
datasource: {
@ -78,6 +79,22 @@ describe('query actions', () => {
variableAdapters.setInit(() => [createQueryVariableAdapter()]);
describe('when updateQueryVariableOptions is dispatched but there is no ongoing transaction', () => {
it('then correct actions are dispatched', async () => {
const variable = createVariable({ includeAll: false });
const optionsMetrics = [createMetric('A'), createMetric('B')];
mockDatasourceMetrics(variable, optionsMetrics);
const tester = await reduxTester<RootReducerType>()
.givenRootReducer(getRootReducer())
.whenActionIsDispatched(addVariable(toVariablePayload(variable, { global: false, index: 0, model: variable })))
.whenAsyncActionIsDispatched(updateQueryVariableOptions(toVariablePayload(variable)), true);
tester.thenNoActionsWhereDispatched();
});
});
describe('when updateQueryVariableOptions is dispatched for variable without both tags and includeAll', () => {
it('then correct actions are dispatched', async () => {
const variable = createVariable({ includeAll: false });
@ -88,6 +105,7 @@ describe('query actions', () => {
const tester = await reduxTester<RootReducerType>()
.givenRootReducer(getRootReducer())
.whenActionIsDispatched(addVariable(toVariablePayload(variable, { global: false, index: 0, model: variable })))
.whenActionIsDispatched(variablesInitTransaction({ uid: 'a uid' }))
.whenAsyncActionIsDispatched(updateQueryVariableOptions(toVariablePayload(variable)), true);
const option = createOption('A');
@ -110,6 +128,7 @@ describe('query actions', () => {
const tester = await reduxTester<RootReducerType>()
.givenRootReducer(getRootReducer())
.whenActionIsDispatched(addVariable(toVariablePayload(variable, { global: false, index: 0, model: variable })))
.whenActionIsDispatched(variablesInitTransaction({ uid: 'a uid' }))
.whenAsyncActionIsDispatched(updateQueryVariableOptions(toVariablePayload(variable)), true);
const option = createOption(ALL_VARIABLE_TEXT, ALL_VARIABLE_VALUE);
@ -136,6 +155,7 @@ describe('query actions', () => {
const tester = await reduxTester<RootReducerType>()
.givenRootReducer(getRootReducer())
.whenActionIsDispatched(addVariable(toVariablePayload(variable, { global: false, index: 0, model: variable })))
.whenActionIsDispatched(variablesInitTransaction({ uid: 'a uid' }))
.whenActionIsDispatched(setIdInEditor({ id: variable.id }))
.whenAsyncActionIsDispatched(updateQueryVariableOptions(toVariablePayload(variable)), true);
@ -164,6 +184,7 @@ describe('query actions', () => {
const tester = await reduxTester<RootReducerType>()
.givenRootReducer(getRootReducer())
.whenActionIsDispatched(addVariable(toVariablePayload(variable, { global: false, index: 0, model: variable })))
.whenActionIsDispatched(variablesInitTransaction({ uid: 'a uid' }))
.whenActionIsDispatched(setIdInEditor({ id: variable.id }))
.whenAsyncActionIsDispatched(updateQueryVariableOptions(toVariablePayload(variable), 'search'), true);
@ -191,6 +212,7 @@ describe('query actions', () => {
const tester = await reduxTester<RootReducerType>()
.givenRootReducer(getRootReducer())
.whenActionIsDispatched(addVariable(toVariablePayload(variable, { global: false, index: 0, model: variable })))
.whenActionIsDispatched(variablesInitTransaction({ uid: 'a uid' }))
.whenActionIsDispatched(setIdInEditor({ id: variable.id }))
.whenAsyncActionIsDispatched(updateOptions(toVariablePayload(variable)), true);
@ -227,6 +249,7 @@ describe('query actions', () => {
const tester = await reduxTester<RootReducerType>()
.givenRootReducer(getRootReducer())
.whenActionIsDispatched(addVariable(toVariablePayload(variable, { global: false, index: 0, model: variable })))
.whenActionIsDispatched(variablesInitTransaction({ uid: 'a uid' }))
.whenAsyncActionIsDispatched(initQueryVariableEditor(toVariablePayload(variable)), true);
tester.thenDispatchedActionsPredicateShouldEqual((actions) => {
@ -256,6 +279,7 @@ describe('query actions', () => {
const tester = await reduxTester<RootReducerType>()
.givenRootReducer(getRootReducer())
.whenActionIsDispatched(addVariable(toVariablePayload(variable, { global: false, index: 0, model: variable })))
.whenActionIsDispatched(variablesInitTransaction({ uid: 'a uid' }))
.whenAsyncActionIsDispatched(initQueryVariableEditor(toVariablePayload(variable)), true);
tester.thenDispatchedActionsPredicateShouldEqual((actions) => {
@ -284,6 +308,7 @@ describe('query actions', () => {
const tester = await reduxTester<RootReducerType>()
.givenRootReducer(getRootReducer())
.whenActionIsDispatched(addVariable(toVariablePayload(variable, { global: false, index: 0, model: variable })))
.whenActionIsDispatched(variablesInitTransaction({ uid: 'a uid' }))
.whenAsyncActionIsDispatched(initQueryVariableEditor(toVariablePayload(variable)), true);
tester.thenDispatchedActionsPredicateShouldEqual((actions) => {
@ -306,6 +331,7 @@ describe('query actions', () => {
const tester = await reduxTester<RootReducerType>()
.givenRootReducer(getRootReducer())
.whenActionIsDispatched(addVariable(toVariablePayload(variable, { global: false, index: 0, model: variable })))
.whenActionIsDispatched(variablesInitTransaction({ uid: 'a uid' }))
.whenAsyncActionIsDispatched(initQueryVariableEditor(toVariablePayload(variable)), true);
tester.thenDispatchedActionsPredicateShouldEqual((actions) => {
@ -330,6 +356,7 @@ describe('query actions', () => {
const tester = await reduxTester<RootReducerType>()
.givenRootReducer(getRootReducer())
.whenActionIsDispatched(addVariable(toVariablePayload(variable, { global: false, index: 0, model: variable })))
.whenActionIsDispatched(variablesInitTransaction({ uid: 'a uid' }))
.whenAsyncActionIsDispatched(
changeQueryVariableDataSource(toVariablePayload(variable), { uid: 'datasource' }),
true
@ -365,6 +392,7 @@ describe('query actions', () => {
.whenActionIsDispatched(
addVariable(toVariablePayload(variable, { global: false, index: 0, model: variable }))
)
.whenActionIsDispatched(variablesInitTransaction({ uid: 'a uid' }))
.whenAsyncActionIsDispatched(
changeQueryVariableDataSource(toVariablePayload(variable), { uid: 'datasource' }),
true
@ -402,6 +430,7 @@ describe('query actions', () => {
const tester = await reduxTester<RootReducerType>()
.givenRootReducer(getRootReducer())
.whenActionIsDispatched(addVariable(toVariablePayload(variable, { global: false, index: 0, model: variable })))
.whenActionIsDispatched(variablesInitTransaction({ uid: 'a uid' }))
.whenAsyncActionIsDispatched(
changeQueryVariableDataSource(toVariablePayload(variable), { uid: 'datasource' }),
true
@ -436,6 +465,7 @@ describe('query actions', () => {
const tester = await reduxTester<RootReducerType>()
.givenRootReducer(getRootReducer())
.whenActionIsDispatched(addVariable(toVariablePayload(variable, { global: false, index: 0, model: variable })))
.whenActionIsDispatched(variablesInitTransaction({ uid: 'a uid' }))
.whenAsyncActionIsDispatched(changeQueryVariableQuery(toVariablePayload(variable), query, definition), true);
const option = createOption(ALL_VARIABLE_TEXT, ALL_VARIABLE_VALUE);
@ -466,6 +496,7 @@ describe('query actions', () => {
const tester = await reduxTester<RootReducerType>()
.givenRootReducer(getRootReducer())
.whenActionIsDispatched(addVariable(toVariablePayload(variable, { global: false, index: 0, model: variable })))
.whenActionIsDispatched(variablesInitTransaction({ uid: 'a uid' }))
.whenAsyncActionIsDispatched(changeQueryVariableQuery(toVariablePayload(variable), query, definition), true);
const option = createOption(ALL_VARIABLE_TEXT, ALL_VARIABLE_VALUE);
@ -495,6 +526,7 @@ describe('query actions', () => {
const tester = await reduxTester<RootReducerType>()
.givenRootReducer(getRootReducer())
.whenActionIsDispatched(addVariable(toVariablePayload(variable, { global: false, index: 0, model: variable })))
.whenActionIsDispatched(variablesInitTransaction({ uid: 'a uid' }))
.whenAsyncActionIsDispatched(changeQueryVariableQuery(toVariablePayload(variable), query, definition), true);
const option = createOption('A');
@ -521,6 +553,7 @@ describe('query actions', () => {
const tester = await reduxTester<RootReducerType>()
.givenRootReducer(getRootReducer())
.whenActionIsDispatched(addVariable(toVariablePayload(variable, { global: false, index: 0, model: variable })))
.whenActionIsDispatched(variablesInitTransaction({ uid: 'a uid' }))
.whenAsyncActionIsDispatched(changeQueryVariableQuery(toVariablePayload(variable), query, definition), true);
const errorText = 'Query cannot contain a reference to itself. Variable: $' + variable.name;

@ -18,14 +18,20 @@ import { getVariableQueryEditor } from '../editor/getVariableQueryEditor';
import { getVariableQueryRunner } from './VariableQueryRunner';
import { variableQueryObserver } from './variableQueryObserver';
import { QueryVariableEditorState } from './reducer';
import { hasOngoingTransaction } from '../utils';
export const updateQueryVariableOptions = (
identifier: VariableIdentifier,
searchFilter?: string
): ThunkResult<void> => {
return async (dispatch, getState) => {
const variableInState = getVariable<QueryVariableModel>(identifier.id, getState());
try {
if (!hasOngoingTransaction(getState())) {
// we might have cancelled a batch so then variable state is removed
return;
}
const variableInState = getVariable<QueryVariableModel>(identifier.id, getState());
if (getState().templating.editor.id === variableInState.id) {
dispatch(removeVariableEditorError({ errorProp: 'update' }));
}
@ -43,7 +49,7 @@ export const updateQueryVariableOptions = (
});
} catch (err) {
const error = toDataQueryError(err);
if (getState().templating.editor.id === variableInState.id) {
if (getState().templating.editor.id === identifier.id) {
dispatch(addVariableEditorError({ errorProp: 'update', errorText: error.message }));
}

@ -71,7 +71,13 @@ import {
} from './transactionReducer';
import { getBackendSrv } from '../../../core/services/backend_srv';
import { cleanVariables } from './variablesReducer';
import { ensureStringValues, ExtendedUrlQueryMap, getCurrentText, getVariableRefresh } from '../utils';
import {
ensureStringValues,
ExtendedUrlQueryMap,
getCurrentText,
getVariableRefresh,
hasOngoingTransaction,
} from '../utils';
import { store } from 'app/store/store';
import { getDatasourceSrv } from '../../plugins/datasource_srv';
import { cleanEditorState } from '../editor/reducer';
@ -744,14 +750,19 @@ export const updateOptions = (identifier: VariableIdentifier, rethrow = false):
dispatch,
getState
) => {
const variableInState = getVariable(identifier.id, getState());
try {
if (!hasOngoingTransaction(getState())) {
// we might have cancelled a batch so then variable state is removed
return;
}
const variableInState = getVariable(identifier.id, getState());
dispatch(variableStateFetching(toVariablePayload(variableInState)));
await dispatch(upgradeLegacyQueries(toVariableIdentifier(variableInState)));
await variableAdapters.get(variableInState.type).updateOptions(variableInState);
dispatch(completeVariableLoading(identifier));
} catch (error) {
dispatch(variableStateFailed(toVariablePayload(variableInState, { error })));
dispatch(variableStateFailed(toVariablePayload(identifier, { error })));
if (!rethrow) {
console.error(error);
@ -775,6 +786,11 @@ export const createVariableErrorNotification = (
);
export const completeVariableLoading = (identifier: VariableIdentifier): ThunkResult<void> => (dispatch, getState) => {
if (!hasOngoingTransaction(getState())) {
// we might have cancelled a batch so then variable state is removed
return;
}
const variableInState = getVariable(identifier.id, getState());
if (variableInState.state !== LoadingState.Done) {
@ -787,6 +803,11 @@ export function upgradeLegacyQueries(
getDatasourceSrvFunc: typeof getDatasourceSrv = getDatasourceSrv
): ThunkResult<void> {
return async function (dispatch, getState) {
if (!hasOngoingTransaction(getState())) {
// we might have cancelled a batch so then variable state is removed
return;
}
const variable = getVariable<QueryVariableModel>(identifier.id, getState());
if (!isQuery(variable)) {

@ -24,12 +24,11 @@ import { notifyApp } from '../../../core/reducers/appNotification';
import { expect } from '../../../../test/lib/common';
import { TemplatingState } from './reducers';
import { appEvents } from '../../../core/core';
import { variablesInitTransaction } from './transactionReducer';
variableAdapters.setInit(() => [createIntervalVariableAdapter(), createConstantVariableAdapter()]);
const dashboard = new DashboardModel({});
const getTestContext = () => {
const getTestContext = (dashboard: DashboardModel) => {
jest.clearAllMocks();
const interval = intervalBuilder()
@ -60,12 +59,10 @@ const getTestContext = () => {
const dependencies: OnTimeRangeUpdatedDependencies = { templateSrv: templateSrvMock, events: appEvents };
const templateVariableValueUpdatedMock = jest.fn();
const startRefreshMock = jest.fn();
dashboard.templateVariableValueUpdated = templateVariableValueUpdatedMock;
dashboard.startRefresh = startRefreshMock;
const dashboardState = ({
getModel: () => {
dashboard.templateVariableValueUpdated = templateVariableValueUpdatedMock;
dashboard.startRefresh = startRefreshMock;
return dashboard;
},
getModel: () => dashboard,
} as unknown) as DashboardState;
const adapter = variableAdapters.get('interval');
const preloadedState = ({
@ -100,13 +97,15 @@ describe('when onTimeRangeUpdated is dispatched', () => {
updateTimeRangeMock,
templateVariableValueUpdatedMock,
startRefreshMock,
} = getTestContext();
} = getTestContext(getDashboardModel());
const tester = await reduxTester<RootReducerType>({ preloadedState })
.givenRootReducer(getRootReducer())
.whenActionIsDispatched(variablesInitTransaction({ uid: 'a uid' }))
.whenAsyncActionIsDispatched(onTimeRangeUpdated(range, dependencies));
tester.thenDispatchedActionsShouldEqual(
variablesInitTransaction({ uid: 'a uid' }),
variableStateFetching(toVariablePayload({ type: 'interval', id: 'interval-0' })),
createIntervalOptions(toVariablePayload({ type: 'interval', id: 'interval-0' })),
setCurrentVariableValue(
@ -135,10 +134,11 @@ describe('when onTimeRangeUpdated is dispatched', () => {
updateTimeRangeMock,
templateVariableValueUpdatedMock,
startRefreshMock,
} = getTestContext();
} = getTestContext(getDashboardModel());
const base = await reduxTester<RootReducerType>({ preloadedState })
.givenRootReducer(getRootReducer())
.whenActionIsDispatched(variablesInitTransaction({ uid: 'a uid' }))
.whenAsyncActionIsDispatched(setOptionAsCurrent(toVariableIdentifier(interval), interval.options[0], false));
const tester = await base.whenAsyncActionIsDispatched(onTimeRangeUpdated(range, dependencies), true);
@ -173,12 +173,13 @@ describe('when onTimeRangeUpdated is dispatched', () => {
updateTimeRangeMock,
templateVariableValueUpdatedMock,
startRefreshMock,
} = getTestContext();
} = getTestContext(getDashboardModel());
adapter.updateOptions = jest.fn().mockRejectedValue(new Error('Something broke'));
const tester = await reduxTester<RootReducerType>({ preloadedState, debug: true })
.givenRootReducer(getRootReducer())
.whenActionIsDispatched(variablesInitTransaction({ uid: 'a uid' }))
.whenAsyncActionIsDispatched(onTimeRangeUpdated(range, dependencies), true);
tester.thenDispatchedActionsPredicateShouldEqual((dispatchedActions) => {
@ -204,3 +205,7 @@ describe('when onTimeRangeUpdated is dispatched', () => {
});
});
});
function getDashboardModel(): DashboardModel {
return new DashboardModel({ schemaVersion: 9999 }); // ignore any schema migrations
}

@ -5,13 +5,20 @@ import { upgradeLegacyQueries } from './actions';
import { changeVariableProp } from './sharedReducer';
import { thunkTester } from '../../../../test/core/thunk/thunkTester';
import { VariableModel } from '../types';
import { TransactionStatus } from './transactionReducer';
interface Args {
query?: any;
variable?: VariableModel;
datasource?: any;
transactionStatus?: TransactionStatus;
}
function getTestContext({ query = '', variable, datasource }: Args = {}) {
function getTestContext({
query = '',
variable,
datasource,
transactionStatus = TransactionStatus.Fetching,
}: Args = {}) {
variable =
variable ??
queryBuilder()
@ -22,6 +29,7 @@ function getTestContext({ query = '', variable, datasource }: Args = {}) {
.build();
const state = {
templating: {
transaction: { status: transactionStatus },
variables: {
[variable.id]: variable,
},
@ -64,6 +72,22 @@ describe('upgradeLegacyQueries', () => {
expect(get).toHaveBeenCalledTimes(1);
expect(get).toHaveBeenCalledWith({ uid: 'test-data', type: 'test-data' });
});
describe('but there is no ongoing transaction', () => {
it('then it should not dispatch changeVariableProp', async () => {
const { state, identifier, get, getDatasourceSrv } = getTestContext({
query: '*',
transactionStatus: TransactionStatus.NotStarted,
});
const dispatchedActions = await thunkTester(state)
.givenThunk(upgradeLegacyQueries)
.whenThunkIsDispatched(identifier, getDatasourceSrv);
expect(dispatchedActions).toEqual([]);
expect(get).toHaveBeenCalledTimes(0);
});
});
});
describe('when called with a query variable for a standard variable supported data source that has been upgraded', () => {

@ -7,6 +7,9 @@ import { QueryVariableModel, VariableModel, VariableRefresh } from './types';
import { getTimeSrv } from '../dashboard/services/TimeSrv';
import { variableAdapters } from './adapters';
import { safeStringifyValue } from 'app/core/utils/explore';
import { StoreState } from '../../types';
import { getState } from '../../store/store';
import { TransactionStatus } from './state/transactionReducer';
/*
* This regex matches 3 types of variable reference with an optional format specifier
@ -253,3 +256,7 @@ export function ensureStringValues(value: any | any[]): string | string[] {
return '';
}
export function hasOngoingTransaction(state: StoreState = getState()): boolean {
return state.templating.transaction.status !== TransactionStatus.NotStarted;
}

Loading…
Cancel
Save