TemplateVariable: moved templateSrv dependency from reducer to prevent side effect issues.

aae76d79 · Marcus Andersson · GitHub · 69259d62 · aae76d79 · aae76d79
Commit aae76d79 authored Apr 01, 2020 by Marcus Andersson Committed by GitHub Apr 01, 2020
5 changed files
--- 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,
  };
 }
--- a/public/app/features/variables/query/actions.ts
+++ b/public/app/features/variables/query/actions.ts
 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');
+};
--- a/public/app/features/variables/query/reducer.test.ts
+++ b/public/app/features/variables/query/reducer.test.ts
 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[] = [
+      const metrics = [createMetric('A'), createMetric('B')];
-        { text: 'A', value: 'A', selected: false },
+      const update = { results: metrics, templatedRegex: '' };
-        { text: 'B', value: 'B', selected: false },
+      const payload = toVariablePayload({ id: '0', type: 'query' }, update);
-      ];
-      const payload = toVariablePayload({ id: '0', type: 'query' }, options);
      reducerTester<VariablesState>()
        .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[] = [
+      const metrics = [createMetric('A'), createMetric('B')];
-        { text: 'A', value: 'A', selected: false },
+      const update = { results: metrics, templatedRegex: '' };
-        { text: 'B', value: 'B', selected: false },
+      const payload = toVariablePayload({ id: '0', type: 'query' }, update);
-      ];
-      const payload = toVariablePayload({ id: '0', type: 'query' }, options);
      reducerTester<VariablesState>()
        .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<VariablesState>()
        .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<VariablesState>()
        .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 regex = '/.*(a).*/i';
-      const options: VariableOption[] = [
+      const { initialState } = getVariableTestContext(adapter, { includeAll: true, regex });
-        { text: 'A', value: 'A', selected: false },
+      const metrics = [createMetric('A'), createMetric('B')];
-        { text: 'B', value: 'B', selected: false },
+      const update = { results: metrics, templatedRegex: regex };
-      ];
+      const payload = toVariablePayload({ id: '0', type: 'query' }, update);
-      const payload = toVariablePayload({ id: '0', type: 'query' }, options);
      reducerTester<VariablesState>()
        .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 regex = '/.*(a).*/i';
-      const options: VariableOption[] = [
+      const { initialState } = getVariableTestContext(adapter, { includeAll: false, regex });
-        { text: 'A', value: 'A', selected: false },
+      const metrics = [createMetric('A'), createMetric('B')];
-        { text: 'B', value: 'B', selected: false },
+      const update = { results: metrics, templatedRegex: regex };
-      ];
+      const payload = toVariablePayload({ id: '0', type: 'query' }, update);
-      const payload = toVariablePayload({ id: '0', type: 'query' }, options);
      reducerTester<VariablesState>()
        .givenReducer(queryVariableReducer, cloneDeep(initialState))
        .whenActionIsDispatched(updateVariableOptions(payload))
@@ -159,3 +162,9 @@ describe('queryVariableReducer', () => {
    });
  });
 });
+function createMetric(value: string) {
+  return {
+    text: value,
+  };
+}
--- a/public/app/features/variables/query/reducer.ts
+++ b/public/app/features/variables/query/reducer.ts
 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<VariableQueryProps> | 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<VariablePayload<any[]>>) => {
+    updateVariableOptions: (state: VariablesState, action: PayloadAction<VariablePayload<VariableOptionsUpdate>>) => {
-      const results = action.payload.data;
+      const { results, templatedRegex } = action.payload.data;
      const instanceState = getInstanceState<QueryVariableModel>(state, action.payload.id);
-      const { regex, includeAll, sort } = instanceState;
+      const { includeAll, sort } = instanceState;
-      const options = metricNamesToVariableValues(regex, sort, results);
+      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 });
      }

--- 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' }, [
+                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' }, [
+                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' }, [
+                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' }, [
+                toVariablePayload(
+                  { type: 'query', id: 'queryDependsOnCustom' },
+                  {
+                    results: [
                      { value: 'AA', text: 'AA' },
                      { value: 'AB', text: 'AB' },
                      { value: 'AC', text: 'AC' },
-                ])
+                    ],
+                    templatedRegex: '',
+                  }
+                )
              ),
              setCurrentVariableValue(
                toVariablePayload(