Variables: Fixes issue with upgrading legacy queries (#29375)

* Variables: Fixes issue with upgrading legacy queries

* Tests: adds tests for upgradeLegacyQueries

public/app/features/variables/query/actions.ts

@@ -1,4 +1,4 @@
-import { DataQuery, DataSourceApi, DataSourcePluginMeta, DataSourceSelectItem } from '@grafana/data';
+import { DataSourcePluginMeta, DataSourceSelectItem } from '@grafana/data';
 import { toDataQueryError } from '@grafana/runtime';
 
 import { updateOptions } from '../state/actions';
@@ -9,7 +9,6 @@ import { getVariable } from '../state/selectors';
 import { addVariableEditorError, changeVariableEditorExtended, removeVariableEditorError } from '../editor/reducer';
 import { changeVariableProp } from '../state/sharedReducer';
 import { toVariableIdentifier, toVariablePayload, VariableIdentifier } from '../state/types';
-import { hasLegacyVariableSupport, hasStandardVariableSupport } from '../guard';
 import { getVariableQueryEditor } from '../editor/getVariableQueryEditor';
 import { Subscription } from 'rxjs';
 import { getVariableQueryRunner } from './VariableQueryRunner';
@@ -26,7 +25,6 @@ export const updateQueryVariableOptions = (
         dispatch(removeVariableEditorError({ errorProp: 'update' }));
       }
       const datasource = await getDatasourceSrv().get(variableInState.datasource ?? '');
-      dispatch(upgradeLegacyQueries(identifier, datasource));
 
       // we need to await the result from variableQueryRunner before moving on otherwise variables dependent on this
       // variable will have the wrong current value as input
@@ -158,35 +156,3 @@ export function flattenQuery(query: any): any {
 
   return flattened;
 }
-
-export function upgradeLegacyQueries(identifier: VariableIdentifier, datasource: DataSourceApi): ThunkResult<void> {
-  return function(dispatch, getState) {
-    if (hasLegacyVariableSupport(datasource)) {
-      return;
-    }
-
-    if (!hasStandardVariableSupport(datasource)) {
-      return;
-    }
-
-    const variable = getVariable<QueryVariableModel>(identifier.id, getState());
-    if (isDataQuery(variable.query)) {
-      return;
-    }
-
-    const query = {
-      refId: `${datasource.name}-${identifier.id}-Variable-Query`,
-      query: variable.query,
-    };
-
-    dispatch(changeVariableProp(toVariablePayload(identifier, { propName: 'query', propValue: query })));
-  };
-}
-
-function isDataQuery(query: any): query is DataQuery {
-  if (!query) {
-    return false;
-  }
-
-  return query.hasOwnProperty('refId') && typeof query.refId === 'string';
-}

public/app/features/variables/shared/testing/optionsVariableBuilder.ts

@@ -28,7 +28,7 @@ export class OptionsVariableBuilder<T extends VariableWithOptions> extends Varia
     return this;
   }
 
-  withQuery(query: string) {
+  withQuery(query: any) {
     this.variable.query = query;
     return this;
   }

public/app/features/variables/shared/testing/queryVariableBuilder.ts

@@ -11,4 +11,9 @@ export class QueryVariableBuilder<T extends QueryVariableModel> extends Datasour
     this.variable.tagsQuery = tagsQuery;
     return this;
   }
+
+  withDatasource(datasource: string) {
+    this.variable.datasource = datasource;
+    return this;
+  }
 }

public/app/features/variables/state/actions.ts

@@ -1,6 +1,6 @@
 import angular from 'angular';
 import castArray from 'lodash/castArray';
-import { LoadingState, TimeRange, UrlQueryMap, UrlQueryValue } from '@grafana/data';
+import { DataQuery, LoadingState, TimeRange, UrlQueryMap, UrlQueryValue } from '@grafana/data';
 
 import {
   DashboardVariableModel,
@@ -39,7 +39,7 @@ import {
 import { contextSrv } from 'app/core/services/context_srv';
 import { getTemplateSrv, TemplateSrv } from '../../templating/template_srv';
 import { alignCurrentWithMulti } from '../shared/multiOptions';
-import { isMulti } from '../guard';
+import { hasLegacyVariableSupport, hasStandardVariableSupport, isMulti, isQuery } from '../guard';
 import { getTimeSrv } from 'app/features/dashboard/services/TimeSrv';
 import { DashboardModel } from 'app/features/dashboard/state';
 import { createErrorNotification } from '../../../core/copy/appNotification';
@@ -54,6 +54,7 @@ import { cleanVariables } from './variablesReducer';
 import isEqual from 'lodash/isEqual';
 import { getCurrentText, getVariableRefresh } from '../utils';
 import { store } from 'app/store/store';
+import { getDatasourceSrv } from '../../plugins/datasource_srv';
 
 // process flow queryVariable
 // thunk => processVariables
@@ -99,8 +100,11 @@ export const initDashboardTemplating = (list: VariableModel[]): ThunkResult<void
 
     getTemplateSrv().updateTimeRange(getTimeSrv().timeRange());
 
-    for (let index = 0; index < getVariables(getState()).length; index++) {
-      dispatch(variableStateNotStarted(toVariablePayload(getVariables(getState())[index])));
+    const variables = getVariables(getState());
+    for (let index = 0; index < variables.length; index++) {
+      const variable = variables[index];
+      dispatch(upgradeLegacyQueries(toVariableIdentifier(variable)));
+      dispatch(variableStateNotStarted(toVariablePayload(variable)));
     }
   };
 };
@@ -668,3 +672,45 @@ export const completeVariableLoading = (identifier: VariableIdentifier): ThunkRe
     dispatch(variableStateCompleted(toVariablePayload(variableInState)));
   }
 };
+
+export function upgradeLegacyQueries(
+  identifier: VariableIdentifier,
+  getDatasourceSrvFunc: typeof getDatasourceSrv = getDatasourceSrv
+): ThunkResult<void> {
+  return async function(dispatch, getState) {
+    const variable = getVariable<QueryVariableModel>(identifier.id, getState());
+
+    if (!isQuery(variable)) {
+      return;
+    }
+
+    const datasource = await getDatasourceSrvFunc().get(variable.datasource ?? '');
+
+    if (hasLegacyVariableSupport(datasource)) {
+      return;
+    }
+
+    if (!hasStandardVariableSupport(datasource)) {
+      return;
+    }
+
+    if (isDataQueryType(variable.query)) {
+      return;
+    }
+
+    const query = {
+      refId: `${datasource.name}-${identifier.id}-Variable-Query`,
+      query: variable.query,
+    };
+
+    dispatch(changeVariableProp(toVariablePayload(identifier, { propName: 'query', propValue: query })));
+  };
+}
+
+function isDataQueryType(query: any): query is DataQuery {
+  if (!query) {
+    return false;
+  }
+
+  return query.hasOwnProperty('refId') && typeof query.refId === 'string';
+}

public/app/features/variables/state/upgradeLegacyQueries.test.ts

@@ -0,0 +1,155 @@
+import { customBuilder, queryBuilder } from '../shared/testing/builders';
+import { VariableSupportType } from '@grafana/data';
+import { toVariableIdentifier } from './types';
+import { upgradeLegacyQueries } from './actions';
+import { changeVariableProp } from './sharedReducer';
+import { thunkTester } from '../../../../test/core/thunk/thunkTester';
+import { VariableModel } from '../types';
+
+interface Args {
+  query?: any;
+  variable?: VariableModel;
+  datasource?: any;
+}
+function getTestContext({ query = '', variable, datasource }: Args = {}) {
+  variable =
+    variable ??
+    queryBuilder()
+      .withId('query')
+      .withName('query')
+      .withQuery(query)
+      .withDatasource('test-data')
+      .build();
+  const state = {
+    templating: {
+      variables: {
+        [variable.id]: variable,
+      },
+    },
+  };
+  datasource = datasource ?? {
+    name: 'TestData',
+    metricFindQuery: () => undefined,
+    variables: { getType: () => VariableSupportType.Standard, toDataQuery: () => undefined },
+  };
+  const get = jest.fn().mockResolvedValue(datasource);
+  const getDatasourceSrv = jest.fn().mockReturnValue({ get });
+  const identifier = toVariableIdentifier(variable);
+
+  return { state, get, getDatasourceSrv, identifier };
+}
+
+describe('upgradeLegacyQueries', () => {
+  describe('when called with a query variable for a standard variable supported data source that has not been upgraded', () => {
+    it('then it should dispatch changeVariableProp', async () => {
+      const { state, identifier, get, getDatasourceSrv } = getTestContext({ query: '*' });
+
+      const dispatchedActions = await thunkTester(state)
+        .givenThunk(upgradeLegacyQueries)
+        .whenThunkIsDispatched(identifier, getDatasourceSrv);
+
+      expect(dispatchedActions).toEqual([
+        changeVariableProp({
+          type: 'query',
+          id: 'query',
+          data: {
+            propName: 'query',
+            propValue: {
+              refId: 'TestData-query-Variable-Query',
+              query: '*',
+            },
+          },
+        }),
+      ]);
+      expect(get).toHaveBeenCalledTimes(1);
+      expect(get).toHaveBeenCalledWith('test-data');
+    });
+  });
+
+  describe('when called with a query variable for a standard variable supported data source that has been upgraded', () => {
+    it('then it should not dispatch any actions', async () => {
+      const { state, identifier, get, getDatasourceSrv } = getTestContext({ query: { refId: 'A' } });
+
+      const dispatchedActions = await thunkTester(state)
+        .givenThunk(upgradeLegacyQueries)
+        .whenThunkIsDispatched(identifier, getDatasourceSrv);
+
+      expect(dispatchedActions).toEqual([]);
+      expect(get).toHaveBeenCalledTimes(1);
+      expect(get).toHaveBeenCalledWith('test-data');
+    });
+  });
+
+  describe('when called with a query variable for a legacy variable supported data source', () => {
+    it('then it should not dispatch any actions', async () => {
+      const datasource = {
+        name: 'TestData',
+        metricFindQuery: () => undefined,
+      };
+      const { state, identifier, get, getDatasourceSrv } = getTestContext({ datasource });
+
+      const dispatchedActions = await thunkTester(state)
+        .givenThunk(upgradeLegacyQueries)
+        .whenThunkIsDispatched(identifier, getDatasourceSrv);
+
+      expect(dispatchedActions).toEqual([]);
+      expect(get).toHaveBeenCalledTimes(1);
+      expect(get).toHaveBeenCalledWith('test-data');
+    });
+  });
+
+  describe('when called with a query variable for a custom variable supported data source', () => {
+    it('then it should not dispatch any actions', async () => {
+      const datasource = {
+        name: 'TestData',
+        metricFindQuery: () => undefined,
+        variables: { getType: () => VariableSupportType.Custom, query: () => undefined, editor: {} },
+      };
+      const { state, identifier, get, getDatasourceSrv } = getTestContext({ datasource });
+
+      const dispatchedActions = await thunkTester(state)
+        .givenThunk(upgradeLegacyQueries)
+        .whenThunkIsDispatched(identifier, getDatasourceSrv);
+
+      expect(dispatchedActions).toEqual([]);
+      expect(get).toHaveBeenCalledTimes(1);
+      expect(get).toHaveBeenCalledWith('test-data');
+    });
+  });
+
+  describe('when called with a query variable for a datasource variable supported data source', () => {
+    it('then it should not dispatch any actions', async () => {
+      const datasource = {
+        name: 'TestData',
+        metricFindQuery: () => undefined,
+        variables: { getType: () => VariableSupportType.Datasource },
+      };
+      const { state, identifier, get, getDatasourceSrv } = getTestContext({ datasource });
+
+      const dispatchedActions = await thunkTester(state)
+        .givenThunk(upgradeLegacyQueries)
+        .whenThunkIsDispatched(identifier, getDatasourceSrv);
+
+      expect(dispatchedActions).toEqual([]);
+      expect(get).toHaveBeenCalledTimes(1);
+      expect(get).toHaveBeenCalledWith('test-data');
+    });
+  });
+
+  describe('when called with a custom variable', () => {
+    it('then it should not dispatch any actions', async () => {
+      const variable = customBuilder()
+        .withId('custom')
+        .withName('custom')
+        .build();
+      const { state, identifier, get, getDatasourceSrv } = getTestContext({ variable });
+
+      const dispatchedActions = await thunkTester(state)
+        .givenThunk(upgradeLegacyQueries)
+        .whenThunkIsDispatched(identifier, getDatasourceSrv);
+
+      expect(dispatchedActions).toEqual([]);
+      expect(get).toHaveBeenCalledTimes(0);
+    });
+  });
+});