Variables: Ensures all variable values are strings (#31942)

* Variables: Ensures all variable values are strings * Chore: remove redundant typings * Chore: fixes tests
5 years ago · 31ab1a4afe
parent 16027770c8
commit 31ab1a4afe
8 changed files with 75 additions and 24 deletions
--- a/public/app/features/variables/state/actions.test.ts
+++ b/public/app/features/variables/state/actions.test.ts
@ -60,8 +60,7 @@ import { expect } from '../../../../test/lib/common';
 import { ConstantVariableModel, VariableRefresh } from '../types';
 import { updateVariableOptions } from '../query/reducer';
 import { setVariableQueryRunner, VariableQueryRunner } from '../query/VariableQueryRunner';
-import { setDataSourceSrv } from '@grafana/runtime';
-import { LocationState } from 'app/types';
+import { setDataSourceSrv, setLocationService } from '@grafana/runtime';

 variableAdapters.setInit(() => [
  createQueryVariableAdapter(),
@ -146,8 +145,9 @@ describe('shared actions', () => {
      const list = [query, constant, datasource, custom, textbox];
      const preloadedState = {
        templating: ({} as unknown) as TemplatingState,
-        location: ({ query: {} } as unknown) as LocationState,
      };
+      const locationService: any = { getSearchObject: () => ({}) };
+      setLocationService(locationService);

      const tester = await reduxTester<TemplatingReducerType>({ preloadedState })
        .givenRootReducer(getTemplatingRootReducer())
@ -203,9 +203,10 @@ describe('shared actions', () => {

      const list = [stats, substats];
      const query = { orgId: '1', 'var-stats': 'response', 'var-substats': ALL_VARIABLE_TEXT };
+      const locationService: any = { getSearchObject: () => query };
+      setLocationService(locationService);
      const preloadedState = {
        templating: ({} as unknown) as TemplatingState,
-        location: ({ query } as unknown) as LocationState,
      };

      const tester = await reduxTester<TemplatingReducerType>({ preloadedState })
@ -223,6 +224,9 @@ describe('shared actions', () => {
          toVariablePayload(stats, { option: { text: ALL_VARIABLE_TEXT, value: ALL_VARIABLE_VALUE, selected: false } })
        ),
        variableStateCompleted(toVariablePayload(stats)),
+        setCurrentVariableValue(
+          toVariablePayload(stats, { option: { text: ['response'], value: ['response'], selected: false } })
+        ),
        variableStateFetching(toVariablePayload(substats)),
        updateVariableOptions(
          toVariablePayload(substats, { results: [{ text: '200' }, { text: '500' }], templatedRegex: '' })
@ -232,7 +236,12 @@ describe('shared actions', () => {
            option: { text: [ALL_VARIABLE_TEXT], value: [ALL_VARIABLE_VALUE], selected: true },
          })
        ),
-        variableStateCompleted(toVariablePayload(substats))
+        variableStateCompleted(toVariablePayload(substats)),
+        setCurrentVariableValue(
+          toVariablePayload(substats, {
+            option: { text: [ALL_VARIABLE_TEXT], value: [ALL_VARIABLE_VALUE], selected: false },
+          })
+        )
      );
    });
  });
--- a/public/app/features/variables/state/actions.ts
+++ b/public/app/features/variables/state/actions.ts
@ -52,7 +52,7 @@ import {
 import { getBackendSrv } from '../../../core/services/backend_srv';
 import { cleanVariables } from './variablesReducer';
 import isEqual from 'lodash/isEqual';
-import { getCurrentText, getVariableRefresh } from '../utils';
+import { ensureStringValues, getCurrentText, getVariableRefresh } from '../utils';
 import { store } from 'app/store/store';
 import { getDatasourceSrv } from '../../plugins/datasource_srv';
 import { cleanEditorState } from '../editor/reducer';
@ -247,7 +247,8 @@ export const processVariable = (

    const urlValue = queryParams['var-' + variable.name];
    if (urlValue !== void 0) {
-      await variableAdapters.get(variable.type).setValueFromUrl(variable, urlValue ?? '');
+      const stringUrlValue = ensureStringValues(urlValue);
+      await variableAdapters.get(variable.type).setValueFromUrl(variable, stringUrlValue);
      return;
    }

@ -283,6 +284,7 @@ export const setOptionFromUrl = (
  urlValue: UrlQueryValue
 ): ThunkResult<Promise<void>> => {
  return async (dispatch, getState) => {
+    const stringUrlValue = ensureStringValues(urlValue);
    const variable = getVariable(identifier.id, getState());
    if (getVariableRefresh(variable) !== VariableRefresh.never) {
      // updates options
@ -296,23 +298,22 @@ export const setOptionFromUrl = (
    }
    // Simple case. Value in url matches existing options text or value.
    let option = variableFromState.options.find((op) => {
-      return op.text === urlValue || op.value === urlValue;
+      return op.text === stringUrlValue || op.value === stringUrlValue;
    });

    if (!option && isMulti(variableFromState)) {
-      if (variableFromState.allValue && urlValue === variableFromState.allValue) {
+      if (variableFromState.allValue && stringUrlValue === variableFromState.allValue) {
        option = { text: ALL_VARIABLE_TEXT, value: ALL_VARIABLE_VALUE, selected: false };
      }
    }

    if (!option) {
-      let defaultText = urlValue as string | string[];
-      const defaultValue = urlValue as string | string[];
+      let defaultText = stringUrlValue;
+      const defaultValue = stringUrlValue;

-      if (Array.isArray(urlValue)) {
+      if (Array.isArray(stringUrlValue)) {
        // Multiple values in the url. We construct text as a list of texts from all matched options.
-        const urlValueArray = urlValue as string[];
-        defaultText = urlValueArray.reduce((acc: string[], item: string) => {
+        defaultText = stringUrlValue.reduce((acc: string[], item: string) => {
          const foundOption = variableFromState.options.find((o) => o.value === item);
          if (!foundOption) {
            // @ts-ignore according to strict null errors this can never happen
@ -569,8 +570,9 @@ export const templateVarsChangedInUrl = (vars: UrlQueryMap): ThunkResult<void> =
 };

 const isVariableUrlValueDifferentFromCurrent = (variable: VariableModel, urlValue: any): boolean => {
+  const stringUrlValue = ensureStringValues(urlValue);
  // lodash isEqual handles array of value equality checks as well
-  return !isEqual(variableAdapters.get(variable.type).getValueForUrl(variable), urlValue);
+  return !isEqual(variableAdapters.get(variable.type).getValueForUrl(variable), stringUrlValue);
 };

 const getQueryWithVariables = (getState: () => StoreState): UrlQueryMap => {
--- a/public/app/features/variables/state/onTimeRangeUpdated.test.ts
+++ b/public/app/features/variables/state/onTimeRangeUpdated.test.ts
@ -65,7 +65,6 @@ const getTestContext = () => {
  const adapter = variableAdapters.get('interval');
  const preloadedState = ({
    dashboard,
-    location: { query: '' },
    templating: ({
      variables: {
        'interval-0': { ...interval },
--- a/public/app/features/variables/state/setOptionFromUrl.test.ts
+++ b/public/app/features/variables/state/setOptionFromUrl.test.ts
@ -17,15 +17,15 @@ describe('when setOptionFromUrl is dispatched with a custom variable (no refresh
    ${['B']}      | ${false} | ${'B'}
    ${'X'}        | ${false} | ${'X'}
    ${''}         | ${false} | ${''}
-    ${null}       | ${false} | ${null}
-    ${undefined}  | ${false} | ${undefined}
+    ${null}       | ${false} | ${''}
+    ${undefined}  | ${false} | ${''}
    ${'B'}        | ${true}  | ${['B']}
    ${['B']}      | ${true}  | ${['B']}
    ${'X'}        | ${true}  | ${['X']}
    ${''}         | ${true}  | ${['']}
    ${['A', 'B']} | ${true}  | ${['A', 'B']}
-    ${null}       | ${true}  | ${[null]}
-    ${undefined}  | ${true}  | ${[undefined]}
+    ${null}       | ${true}  | ${['']}
+    ${undefined}  | ${true}  | ${['']}
  `('and urlValue is $urlValue then correct actions are dispatched', async ({ urlValue, expected, isMulti }) => {
    const custom = customBuilder().withId('0').withMulti(isMulti).withOptions('A', 'B', 'C').withCurrent('A').build();

--- a/public/app/features/variables/state/sharedReducer.ts
+++ b/public/app/features/variables/state/sharedReducer.ts
@ -9,6 +9,7 @@ import { variableAdapters } from '../adapters';
 import { changeVariableNameSucceeded } from '../editor/reducer';
 import { initialVariablesState, VariablesState } from './variablesReducer';
 import { isQuery } from '../guard';
+import { ensureStringValues } from '../utils';

 const sharedReducerSlice = createSlice({
  name: 'templating/shared',
@ -121,10 +122,12 @@ const sharedReducerSlice = createSlice({
      }

      const instanceState = getInstanceState<VariableWithOptions>(state, action.payload.id);
-      const current = { ...action.payload.data.option };
+      const { option } = action.payload.data;
+      const current = { ...option, text: ensureStringValues(option?.text), value: ensureStringValues(option?.value) };

      instanceState.current = current;
      instanceState.options = instanceState.options.map((option) => {
+        option.value = ensureStringValues(option.value);
        let selected = false;
        if (Array.isArray(current.value)) {
          for (let index = 0; index < current.value.length; index++) {
--- a/public/app/features/variables/textbox/actions.ts
+++ b/public/app/features/variables/textbox/actions.ts
@ -7,6 +7,7 @@ import { toVariableIdentifier, toVariablePayload, VariableIdentifier } from '../
 import { setOptionFromUrl } from '../state/actions';
 import { UrlQueryValue } from '@grafana/data';
 import { changeVariableProp } from '../state/sharedReducer';
+import { ensureStringValues } from '../utils';

 export const updateTextBoxVariableOptions = (identifier: VariableIdentifier): ThunkResult<void> => {
  return async (dispatch, getState) => {
@ -23,7 +24,8 @@ export const setTextBoxVariableOptionsFromUrl = (
 ): ThunkResult<void> => async (dispatch, getState) => {
  const variableInState = getVariable<TextBoxVariableModel>(identifier.id, getState());

-  dispatch(changeVariableProp(toVariablePayload(variableInState, { propName: 'query', propValue: urlValue })));
+  const stringUrlValue = ensureStringValues(urlValue);
+  dispatch(changeVariableProp(toVariablePayload(variableInState, { propName: 'query', propValue: stringUrlValue })));

-  await dispatch(setOptionFromUrl(toVariableIdentifier(variableInState), urlValue));
+  await dispatch(setOptionFromUrl(toVariableIdentifier(variableInState), stringUrlValue));
 };
--- a/public/app/features/variables/utils.test.ts
+++ b/public/app/features/variables/utils.test.ts
@ -1,4 +1,4 @@
-import { findTemplateVarChanges, getCurrentText, getVariableRefresh, isAllVariable } from './utils';
+import { ensureStringValues, findTemplateVarChanges, getCurrentText, getVariableRefresh, isAllVariable } from './utils';
 import { VariableRefresh } from './types';
 import { UrlQueryMap } from '@grafana/data';

@ -157,3 +157,19 @@ describe('findTemplateVarChanges', () => {
    expect(findTemplateVarChanges(a, b)!['var-test']).toEqual(['test']);
  });
 });
+
+describe('ensureStringValues', () => {
+  it.each`
+    value              | expected
+    ${null}            | ${''}
+    ${undefined}       | ${''}
+    ${{}}              | ${''}
+    ${{ current: {} }} | ${''}
+    ${1}               | ${'1'}
+    ${[1, 2]}          | ${['1', '2']}
+    ${'1'}             | ${'1'}
+    ${['1', '2']}      | ${['1', '2']}
+  `('when called with value:$value then result should be:$expected', ({ value, expected }) => {
+    expect(ensureStringValues(value)).toEqual(expected);
+  });
+});
--- a/public/app/features/variables/utils.ts
+++ b/public/app/features/variables/utils.ts
@ -223,3 +223,23 @@ export function findTemplateVarChanges(query: UrlQueryMap, old: UrlQueryMap): Ur
  }
  return count ? changes : undefined;
 }
+
+export function ensureStringValues(value: any | any[]): string | string[] {
+  if (Array.isArray(value)) {
+    return value.map(String);
+  }
+
+  if (value === null || value === undefined) {
+    return '';
+  }
+
+  if (typeof value === 'number') {
+    return value.toString(10);
+  }
+
+  if (typeof value === 'string') {
+    return value;
+  }
+
+  return '';
+}