From aae76d797b6ac71df9dd6c9d7e0d352295b2885d Mon Sep 17 00:00:00 2001 From: Marcus Andersson Date: Wed, 1 Apr 2020 18:05:16 +0200 Subject: [PATCH] TemplateVariable: moved templateSrv dependency from reducer to prevent side effect issues. --- .../features/variables/query/actions.test.ts | 31 ++++++--- .../app/features/variables/query/actions.ts | 17 ++++- .../features/variables/query/reducer.test.ts | 59 +++++++++-------- .../app/features/variables/query/reducer.ts | 21 ++++-- .../variables/state/processVariable.test.ts | 64 +++++++++++++------ 5 files changed, 128 insertions(+), 64 deletions(-) diff --git a/public/app/features/variables/query/actions.test.ts b/public/app/features/variables/query/actions.test.ts index 31c1c476c97..26748a1dc3f 100644 --- a/public/app/features/variables/query/actions.test.ts +++ b/public/app/features/variables/query/actions.test.ts @@ -46,6 +46,10 @@ jest.mock('../../plugins/plugin_loader', () => ({ importDataSourcePlugin: () => mocks.pluginLoader.importDataSourcePlugin(), })); +jest.mock('../../templating/template_srv', () => ({ + replace: jest.fn().mockReturnValue(''), +})); + describe('query actions', () => { variableAdapters.setInit(() => [createQueryVariableAdapter()]); @@ -63,12 +67,13 @@ describe('query actions', () => { .whenAsyncActionIsDispatched(updateQueryVariableOptions(toVariablePayload(variable)), true); const option = createOption(ALL_VARIABLE_TEXT, ALL_VARIABLE_VALUE); + const update = { results: optionsMetrics, templatedRegex: '' }; tester.thenDispatchedActionsPredicateShouldEqual(actions => { const [updateOptions, updateTags, setCurrentAction] = actions; const expectedNumberOfActions = 3; - expect(updateOptions).toEqual(updateVariableOptions(toVariablePayload(variable, optionsMetrics))); + expect(updateOptions).toEqual(updateVariableOptions(toVariablePayload(variable, update))); expect(updateTags).toEqual(updateVariableTags(toVariablePayload(variable, tagsMetrics))); expect(setCurrentAction).toEqual(setCurrentVariableValue(toVariablePayload(variable, { option }))); return actions.length === expectedNumberOfActions; @@ -90,12 +95,13 @@ describe('query actions', () => { .whenAsyncActionIsDispatched(updateQueryVariableOptions(toVariablePayload(variable)), true); const option = createOption('A'); + const update = { results: optionsMetrics, templatedRegex: '' }; tester.thenDispatchedActionsPredicateShouldEqual(actions => { const [updateOptions, updateTags, setCurrentAction] = actions; const expectedNumberOfActions = 3; - expect(updateOptions).toEqual(updateVariableOptions(toVariablePayload(variable, optionsMetrics))); + expect(updateOptions).toEqual(updateVariableOptions(toVariablePayload(variable, update))); expect(updateTags).toEqual(updateVariableTags(toVariablePayload(variable, tagsMetrics))); expect(setCurrentAction).toEqual(setCurrentVariableValue(toVariablePayload(variable, { option }))); return actions.length === expectedNumberOfActions; @@ -116,12 +122,13 @@ describe('query actions', () => { .whenAsyncActionIsDispatched(updateQueryVariableOptions(toVariablePayload(variable)), true); const option = createOption('A'); + const update = { results: optionsMetrics, templatedRegex: '' }; tester.thenDispatchedActionsPredicateShouldEqual(actions => { const [updateOptions, setCurrentAction] = actions; const expectedNumberOfActions = 2; - expect(updateOptions).toEqual(updateVariableOptions(toVariablePayload(variable, optionsMetrics))); + expect(updateOptions).toEqual(updateVariableOptions(toVariablePayload(variable, update))); expect(setCurrentAction).toEqual(setCurrentVariableValue(toVariablePayload(variable, { option }))); return actions.length === expectedNumberOfActions; }); @@ -141,12 +148,13 @@ describe('query actions', () => { .whenAsyncActionIsDispatched(updateQueryVariableOptions(toVariablePayload(variable)), true); const option = createOption(ALL_VARIABLE_TEXT, ALL_VARIABLE_VALUE); + const update = { results: optionsMetrics, templatedRegex: '' }; tester.thenDispatchedActionsPredicateShouldEqual(actions => { const [updateOptions, setCurrentAction] = actions; const expectedNumberOfActions = 2; - expect(updateOptions).toEqual(updateVariableOptions(toVariablePayload(variable, optionsMetrics))); + expect(updateOptions).toEqual(updateVariableOptions(toVariablePayload(variable, update))); expect(setCurrentAction).toEqual(setCurrentVariableValue(toVariablePayload(variable, { option }))); return actions.length === expectedNumberOfActions; }); @@ -167,13 +175,14 @@ describe('query actions', () => { .whenAsyncActionIsDispatched(updateQueryVariableOptions(toVariablePayload(variable)), true); const option = createOption(ALL_VARIABLE_TEXT, ALL_VARIABLE_VALUE); + const update = { results: optionsMetrics, templatedRegex: '' }; tester.thenDispatchedActionsPredicateShouldEqual(actions => { const [clearErrors, updateOptions, setCurrentAction] = actions; const expectedNumberOfActions = 3; expect(clearErrors).toEqual(removeVariableEditorError({ errorProp: 'update' })); - expect(updateOptions).toEqual(updateVariableOptions(toVariablePayload(variable, optionsMetrics))); + expect(updateOptions).toEqual(updateVariableOptions(toVariablePayload(variable, update))); expect(setCurrentAction).toEqual(setCurrentVariableValue(toVariablePayload(variable, { option }))); return actions.length === expectedNumberOfActions; }); @@ -399,6 +408,7 @@ describe('query actions', () => { .whenAsyncActionIsDispatched(changeQueryVariableQuery(toVariablePayload(variable), query, definition), true); const option = createOption(ALL_VARIABLE_TEXT, ALL_VARIABLE_VALUE); + const update = { results: optionsMetrics, templatedRegex: '' }; tester.thenDispatchedActionsPredicateShouldEqual(actions => { const [clearError, changeQuery, changeDefinition, updateOptions, updateTags, setOption] = actions; @@ -411,7 +421,7 @@ describe('query actions', () => { expect(changeDefinition).toEqual( changeVariableProp(toVariablePayload(variable, { propName: 'definition', propValue: definition })) ); - expect(updateOptions).toEqual(updateVariableOptions(toVariablePayload(variable, optionsMetrics))); + expect(updateOptions).toEqual(updateVariableOptions(toVariablePayload(variable, update))); expect(updateTags).toEqual(updateVariableTags(toVariablePayload(variable, tagsMetrics))); expect(setOption).toEqual(setCurrentVariableValue(toVariablePayload(variable, { option }))); @@ -436,6 +446,7 @@ describe('query actions', () => { .whenAsyncActionIsDispatched(changeQueryVariableQuery(toVariablePayload(variable), query, definition), true); const option = createOption(ALL_VARIABLE_TEXT, ALL_VARIABLE_VALUE); + const update = { results: optionsMetrics, templatedRegex: '' }; tester.thenDispatchedActionsPredicateShouldEqual(actions => { const [clearError, changeQuery, changeDefinition, updateOptions, setOption] = actions; @@ -448,7 +459,7 @@ describe('query actions', () => { expect(changeDefinition).toEqual( changeVariableProp(toVariablePayload(variable, { propName: 'definition', propValue: definition })) ); - expect(updateOptions).toEqual(updateVariableOptions(toVariablePayload(variable, optionsMetrics))); + expect(updateOptions).toEqual(updateVariableOptions(toVariablePayload(variable, update))); expect(setOption).toEqual(setCurrentVariableValue(toVariablePayload(variable, { option }))); return actions.length === expectedNumberOfActions; @@ -471,6 +482,7 @@ describe('query actions', () => { .whenAsyncActionIsDispatched(changeQueryVariableQuery(toVariablePayload(variable), query, definition), true); const option = createOption('A'); + const update = { results: optionsMetrics, templatedRegex: '' }; tester.thenDispatchedActionsPredicateShouldEqual(actions => { const [clearError, changeQuery, changeDefinition, updateOptions, setOption] = actions; @@ -483,7 +495,7 @@ describe('query actions', () => { expect(changeDefinition).toEqual( changeVariableProp(toVariablePayload(variable, { propName: 'definition', propValue: definition })) ); - expect(updateOptions).toEqual(updateVariableOptions(toVariablePayload(variable, optionsMetrics))); + expect(updateOptions).toEqual(updateVariableOptions(toVariablePayload(variable, update))); expect(setOption).toEqual(setCurrentVariableValue(toVariablePayload(variable, { option }))); return actions.length === expectedNumberOfActions; @@ -559,14 +571,13 @@ function createOption(text: string, value?: string) { const metric = createMetric(text); return { ...metric, - value: value ?? metric.value, + value: value ?? metric.text, selected: false, }; } function createMetric(value: string) { return { - value: value, text: value, }; } diff --git a/public/app/features/variables/query/actions.ts b/public/app/features/variables/query/actions.ts index 9fcb3eeebc4..89121c64834 100644 --- a/public/app/features/variables/query/actions.ts +++ b/public/app/features/variables/query/actions.ts @@ -1,9 +1,9 @@ import { AppEvents, DataSourcePluginMeta, DataSourceSelectItem } from '@grafana/data'; - import { validateVariableSelectionState } from '../state/actions'; import { QueryVariableModel, VariableRefresh } from '../../templating/types'; import { ThunkResult } from '../../../types'; import { getDatasourceSrv } from '../../plugins/datasource_srv'; +import templateSrv from '../../templating/template_srv'; import { getTimeSrv } from '../../dashboard/services/TimeSrv'; import appEvents from '../../../core/app_events'; import { importDataSourcePlugin } from '../../plugins/plugin_loader'; @@ -36,7 +36,8 @@ export const updateQueryVariableOptions = ( } const results = await dataSource.metricFindQuery(variableInState.query, queryOptions); - await dispatch(updateVariableOptions(toVariablePayload(variableInState, results))); + const templatedRegex = getTemplatedRegex(variableInState); + await dispatch(updateVariableOptions(toVariablePayload(variableInState, { results, templatedRegex }))); if (variableInState.useTags) { const tagResults = await dataSource.metricFindQuery(variableInState.tagsQuery, queryOptions); @@ -113,3 +114,15 @@ export const changeQueryVariableQuery = ( dispatch(changeVariableProp(toVariablePayload(identifier, { propName: 'definition', propValue: definition }))); await variableAdapters.get(identifier.type).updateOptions(variableInState); }; + +const getTemplatedRegex = (variable: QueryVariableModel): string => { + if (!variable) { + return ''; + } + + if (!variable.regex) { + return ''; + } + + return templateSrv.replace(variable.regex, {}, 'regex'); +}; diff --git a/public/app/features/variables/query/reducer.test.ts b/public/app/features/variables/query/reducer.test.ts index b3ff0224b2e..bcb6c4c4df0 100644 --- a/public/app/features/variables/query/reducer.test.ts +++ b/public/app/features/variables/query/reducer.test.ts @@ -1,11 +1,12 @@ import { reducerTester } from '../../../../test/core/redux/reducerTester'; import { queryVariableReducer, updateVariableOptions, updateVariableTags } from './reducer'; -import { QueryVariableModel, VariableOption } from '../../templating/types'; +import { QueryVariableModel } from '../../templating/types'; import cloneDeep from 'lodash/cloneDeep'; import { VariablesState } from '../state/variablesReducer'; import { getVariableTestContext } from '../state/helpers'; import { toVariablePayload } from '../state/types'; import { createQueryVariableAdapter } from './adapter'; +import { MetricFindValue } from '@grafana/data'; describe('queryVariableReducer', () => { const adapter = createQueryVariableAdapter(); @@ -13,11 +14,10 @@ describe('queryVariableReducer', () => { describe('when updateVariableOptions is dispatched and includeAll is true', () => { it('then state should be correct', () => { const { initialState } = getVariableTestContext(adapter, { includeAll: true }); - const options: VariableOption[] = [ - { text: 'A', value: 'A', selected: false }, - { text: 'B', value: 'B', selected: false }, - ]; - const payload = toVariablePayload({ id: '0', type: 'query' }, options); + const metrics = [createMetric('A'), createMetric('B')]; + const update = { results: metrics, templatedRegex: '' }; + const payload = toVariablePayload({ id: '0', type: 'query' }, update); + reducerTester() .givenReducer(queryVariableReducer, cloneDeep(initialState)) .whenActionIsDispatched(updateVariableOptions(payload)) @@ -38,11 +38,10 @@ describe('queryVariableReducer', () => { describe('when updateVariableOptions is dispatched and includeAll is false', () => { it('then state should be correct', () => { const { initialState } = getVariableTestContext(adapter, { includeAll: false }); - const options: VariableOption[] = [ - { text: 'A', value: 'A', selected: false }, - { text: 'B', value: 'B', selected: false }, - ]; - const payload = toVariablePayload({ id: '0', type: 'query' }, options); + const metrics = [createMetric('A'), createMetric('B')]; + const update = { results: metrics, templatedRegex: '' }; + const payload = toVariablePayload({ id: '0', type: 'query' }, update); + reducerTester() .givenReducer(queryVariableReducer, cloneDeep(initialState)) .whenActionIsDispatched(updateVariableOptions(payload)) @@ -62,7 +61,9 @@ describe('queryVariableReducer', () => { describe('when updateVariableOptions is dispatched and includeAll is true and payload is an empty array', () => { it('then state should be correct', () => { const { initialState } = getVariableTestContext(adapter, { includeAll: true }); - const payload = toVariablePayload({ id: '0', type: 'query' }, []); + const update = { results: [] as MetricFindValue[], templatedRegex: '' }; + const payload = toVariablePayload({ id: '0', type: 'query' }, update); + reducerTester() .givenReducer(queryVariableReducer, cloneDeep(initialState)) .whenActionIsDispatched(updateVariableOptions(payload)) @@ -79,7 +80,9 @@ describe('queryVariableReducer', () => { describe('when updateVariableOptions is dispatched and includeAll is false and payload is an empty array', () => { it('then state should be correct', () => { const { initialState } = getVariableTestContext(adapter, { includeAll: false }); - const payload = toVariablePayload({ id: '0', type: 'query' }, []); + const update = { results: [] as MetricFindValue[], templatedRegex: '' }; + const payload = toVariablePayload({ id: '0', type: 'query' }, update); + reducerTester() .givenReducer(queryVariableReducer, cloneDeep(initialState)) .whenActionIsDispatched(updateVariableOptions(payload)) @@ -95,12 +98,12 @@ describe('queryVariableReducer', () => { describe('when updateVariableOptions is dispatched and includeAll is true and regex is set', () => { it('then state should be correct', () => { - const { initialState } = getVariableTestContext(adapter, { includeAll: true, regex: '/.*(a).*/i' }); - const options: VariableOption[] = [ - { text: 'A', value: 'A', selected: false }, - { text: 'B', value: 'B', selected: false }, - ]; - const payload = toVariablePayload({ id: '0', type: 'query' }, options); + const regex = '/.*(a).*/i'; + const { initialState } = getVariableTestContext(adapter, { includeAll: true, regex }); + const metrics = [createMetric('A'), createMetric('B')]; + const update = { results: metrics, templatedRegex: regex }; + const payload = toVariablePayload({ id: '0', type: 'query' }, update); + reducerTester() .givenReducer(queryVariableReducer, cloneDeep(initialState)) .whenActionIsDispatched(updateVariableOptions(payload)) @@ -119,12 +122,12 @@ describe('queryVariableReducer', () => { describe('when updateVariableOptions is dispatched and includeAll is false and regex is set', () => { it('then state should be correct', () => { - const { initialState } = getVariableTestContext(adapter, { includeAll: false, regex: '/.*(a).*/i' }); - const options: VariableOption[] = [ - { text: 'A', value: 'A', selected: false }, - { text: 'B', value: 'B', selected: false }, - ]; - const payload = toVariablePayload({ id: '0', type: 'query' }, options); + const regex = '/.*(a).*/i'; + const { initialState } = getVariableTestContext(adapter, { includeAll: false, regex }); + const metrics = [createMetric('A'), createMetric('B')]; + const update = { results: metrics, templatedRegex: regex }; + const payload = toVariablePayload({ id: '0', type: 'query' }, update); + reducerTester() .givenReducer(queryVariableReducer, cloneDeep(initialState)) .whenActionIsDispatched(updateVariableOptions(payload)) @@ -159,3 +162,9 @@ describe('queryVariableReducer', () => { }); }); }); + +function createMetric(value: string) { + return { + text: value, + }; +} diff --git a/public/app/features/variables/query/reducer.ts b/public/app/features/variables/query/reducer.ts index 6643ae5e9e2..8a86248d6fd 100644 --- a/public/app/features/variables/query/reducer.ts +++ b/public/app/features/variables/query/reducer.ts @@ -1,6 +1,6 @@ import { createSlice, PayloadAction } from '@reduxjs/toolkit'; import _ from 'lodash'; -import { DataSourceApi, DataSourceSelectItem, stringToJsRegex } from '@grafana/data'; +import { DataSourceApi, DataSourceSelectItem, stringToJsRegex, MetricFindValue } from '@grafana/data'; import { QueryVariableModel, @@ -10,7 +10,7 @@ import { VariableSort, VariableTag, } from '../../templating/types'; -import templateSrv from '../../templating/template_srv'; + import { ALL_VARIABLE_TEXT, ALL_VARIABLE_VALUE, @@ -24,6 +24,11 @@ import { ComponentType } from 'react'; import { VariableQueryProps } from '../../../types'; import { initialVariablesState, VariablesState } from '../state/variablesReducer'; +interface VariableOptionsUpdate { + templatedRegex: string; + results: MetricFindValue[]; +} + export interface QueryVariableEditorState { VariableQueryEditor: ComponentType | null; dataSources: DataSourceSelectItem[]; @@ -94,8 +99,9 @@ const metricNamesToVariableValues = (variableRegEx: string, sort: VariableSort, let options: VariableOption[] = []; if (variableRegEx) { - regex = stringToJsRegex(templateSrv.replace(variableRegEx, {}, 'regex')); + regex = stringToJsRegex(variableRegEx); } + for (i = 0; i < metricNames.length; i++) { const item = metricNames[i]; let text = item.text === undefined || item.text === null ? item.value : item.text; @@ -132,15 +138,16 @@ export const queryVariableSlice = createSlice({ name: 'templating/query', initialState: initialVariablesState, reducers: { - updateVariableOptions: (state: VariablesState, action: PayloadAction>) => { - const results = action.payload.data; + updateVariableOptions: (state: VariablesState, action: PayloadAction>) => { + const { results, templatedRegex } = action.payload.data; const instanceState = getInstanceState(state, action.payload.id); - const { regex, includeAll, sort } = instanceState; - const options = metricNamesToVariableValues(regex, sort, results); + const { includeAll, sort } = instanceState; + const options = metricNamesToVariableValues(templatedRegex, sort, results); if (includeAll) { options.unshift({ text: ALL_VARIABLE_TEXT, value: ALL_VARIABLE_VALUE, selected: false }); } + if (!options.length) { options.push({ text: NONE_VARIABLE_TEXT, value: NONE_VARIABLE_VALUE, isNone: true, selected: false }); } diff --git a/public/app/features/variables/state/processVariable.test.ts b/public/app/features/variables/state/processVariable.test.ts index a61c41b1bed..186a77d4292 100644 --- a/public/app/features/variables/state/processVariable.test.ts +++ b/public/app/features/variables/state/processVariable.test.ts @@ -176,11 +176,17 @@ describe('processVariable', () => { await tester.thenDispatchedActionsShouldEqual( updateVariableOptions( - toVariablePayload({ type: 'query', id: 'queryNoDepends' }, [ - { value: 'A', text: 'A' }, - { value: 'B', text: 'B' }, - { value: 'C', text: 'C' }, - ]) + toVariablePayload( + { type: 'query', id: 'queryNoDepends' }, + { + results: [ + { value: 'A', text: 'A' }, + { value: 'B', text: 'B' }, + { value: 'C', text: 'C' }, + ], + templatedRegex: '', + } + ) ), setCurrentVariableValue( toVariablePayload( @@ -236,11 +242,17 @@ describe('processVariable', () => { await tester.thenDispatchedActionsShouldEqual( updateVariableOptions( - toVariablePayload({ type: 'query', id: 'queryNoDepends' }, [ - { value: 'A', text: 'A' }, - { value: 'B', text: 'B' }, - { value: 'C', text: 'C' }, - ]) + toVariablePayload( + { type: 'query', id: 'queryNoDepends' }, + { + results: [ + { value: 'A', text: 'A' }, + { value: 'B', text: 'B' }, + { value: 'C', text: 'C' }, + ], + templatedRegex: '', + } + ) ), setCurrentVariableValue( toVariablePayload( @@ -305,11 +317,17 @@ describe('processVariable', () => { await tester.thenDispatchedActionsShouldEqual( updateVariableOptions( - toVariablePayload({ type: 'query', id: 'queryDependsOnCustom' }, [ - { value: 'AA', text: 'AA' }, - { value: 'AB', text: 'AB' }, - { value: 'AC', text: 'AC' }, - ]) + toVariablePayload( + { type: 'query', id: 'queryDependsOnCustom' }, + { + results: [ + { value: 'AA', text: 'AA' }, + { value: 'AB', text: 'AB' }, + { value: 'AC', text: 'AC' }, + ], + templatedRegex: '', + } + ) ), setCurrentVariableValue( toVariablePayload( @@ -375,11 +393,17 @@ describe('processVariable', () => { await tester.thenDispatchedActionsShouldEqual( updateVariableOptions( - toVariablePayload({ type: 'query', id: 'queryDependsOnCustom' }, [ - { value: 'AA', text: 'AA' }, - { value: 'AB', text: 'AB' }, - { value: 'AC', text: 'AC' }, - ]) + toVariablePayload( + { type: 'query', id: 'queryDependsOnCustom' }, + { + results: [ + { value: 'AA', text: 'AA' }, + { value: 'AB', text: 'AB' }, + { value: 'AC', text: 'AC' }, + ], + templatedRegex: '', + } + ) ), setCurrentVariableValue( toVariablePayload(