Dashboard: Fix Regression detected in time range variables under the refactorVariablesTimeRange feature flag (#74125)

.betterer.results

@@ -3075,11 +3075,10 @@ exports[`better eslint`] = {
       [0, 0, 0, "Do not use any type assertions.", "7"],
       [0, 0, 0, "Do not use any type assertions.", "8"],
       [0, 0, 0, "Do not use any type assertions.", "9"],
-      [0, 0, 0, "Do not use any type assertions.", "10"],
+      [0, 0, 0, "Unexpected any. Specify a different type.", "10"],
       [0, 0, 0, "Unexpected any. Specify a different type.", "11"],
       [0, 0, 0, "Unexpected any. Specify a different type.", "12"],
-      [0, 0, 0, "Unexpected any. Specify a different type.", "13"],
+      [0, 0, 0, "Unexpected any. Specify a different type.", "13"]
-      [0, 0, 0, "Unexpected any. Specify a different type.", "14"]
     ],
     "public/app/features/variables/state/keyedVariablesReducer.ts:5381": [
       [0, 0, 0, "Unexpected any. Specify a different type.", "0"],

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

@@ -7,6 +7,7 @@ import {
   isEmptyObject,
   LoadingState,
   TimeRange,
+  TypedVariableModel,
   UrlQueryMap,
   UrlQueryValue,
 } from '@grafana/data';
@@ -69,6 +70,7 @@ import {
   toVariablePayload,
 } from '../utils';
 
+import { findVariableNodeInList, isVariableOnTimeRangeConfigured } from './helpers';
 import { toKeyedAction } from './keyedVariablesReducer';
 import { getIfExistsLastKey, getVariable, getVariablesByKey, getVariablesState } from './selectors';
 import {
@@ -669,6 +671,20 @@ const dfs = (node: Node, visited: string[], variables: VariableModel[], variable
   return variablesRefreshTimeRange;
 };
 
+// verify if the output edges of a node are not time range dependent
+const areOuputEdgesNotTimeRange = (node: Node, variables: TypedVariableModel[]) => {
+  return node.outputEdges.every((e) => {
+    const childNode = e.outputNode;
+    if (childNode) {
+      const childVariable = findVariableNodeInList(variables, childNode.name);
+      if (childVariable && childVariable.type === 'query') {
+        return childVariable.refresh !== VariableRefresh.onTimeRangeChanged;
+      }
+    }
+    return true;
+  });
+};
+
 /**
  * This function returns a list of variables that need to be refreshed when the time range changes
  * It follows this logic
@@ -684,24 +700,26 @@ const dfs = (node: Node, visited: string[], variables: VariableModel[], variable
  *       Here, we should traverse the tree using DFS (Depth First Search), as the dependent nodes will be updated in cascade when the parent variable is updated.
  */
 
-export const getVariablesThatNeedRefreshNew = (key: string, state: StoreState): VariableWithOptions[] => {
+export const getVariablesThatNeedRefreshNew = (key: string, state: StoreState): TypedVariableModel[] => {
   const allVariables = getVariablesByKey(key, state);
 
   //create dependency graph
   const g = createGraph(allVariables);
   // create a list of nodes that were visited
   const visitedDfs: string[] = [];
-  const variablesRefreshTimeRange: VariableWithOptions[] = [];
+  const variablesRefreshTimeRange: TypedVariableModel[] = [];
   allVariables.forEach((v) => {
     const node = g.getNode(v.name);
     if (visitedDfs.includes(v.name)) {
       return;
     }
     if (node) {
-      const parentVariableNode = allVariables.find((v) => v.name === node.name) as QueryVariableModel;
+      const parentVariableNode = findVariableNodeInList(allVariables, node.name);
-      const isVariableTimeRange =
+      if (!parentVariableNode) {
-        parentVariableNode && parentVariableNode.refresh === VariableRefresh.onTimeRangeChanged;
+        return;
-      //
+      }
+      const isVariableTimeRange = isVariableOnTimeRangeConfigured(parentVariableNode);
+      // if variable is time range and has no output edges add it to the list of variables that need refresh
       if (isVariableTimeRange && node.outputEdges.length === 0) {
         variablesRefreshTimeRange.push(parentVariableNode);
       }
@@ -716,12 +734,16 @@ export const getVariablesThatNeedRefreshNew = (key: string, state: StoreState):
         dfs(node, visitedDfs, allVariables, variablesRefreshTimeRange);
       }
 
+      // If is variable time range, has outputEdges, but the output edges are not time range configured, it means this
+      // is the top variable that need to be refreshed
+      if (isVariableTimeRange && node.outputEdges.length > 0 && areOuputEdgesNotTimeRange(node, allVariables)) {
+        if (!variablesRefreshTimeRange.includes(parentVariableNode)) {
+          variablesRefreshTimeRange.push(parentVariableNode);
+        }
+      }
+
       // if variable is not time range but has dependents (output edges) visit its dependants and repeat the process
-      if (
+      if (!isVariableTimeRange && node.outputEdges.length > 0) {
-        parentVariableNode &&
-        parentVariableNode.refresh &&
-        parentVariableNode.refresh !== VariableRefresh.onTimeRangeChanged
-      ) {
         dfs(node, visitedDfs, allVariables, variablesRefreshTimeRange);
       }
     }
@@ -755,7 +777,8 @@ export const onTimeRangeUpdated =
     dependencies.templateSrv.updateTimeRange(timeRange);
 
     // approach # 2, get variables that need refresh but use the dependency graph to only update the ones that are affected
-    let variablesThatNeedRefresh: VariableWithOptions[] = [];
+    // TODO: remove the VariableWithOptions type once the feature flag is on GA
+    let variablesThatNeedRefresh: VariableWithOptions[] | TypedVariableModel[] = [];
     if (config.featureToggles.refactorVariablesTimeRange) {
       variablesThatNeedRefresh = getVariablesThatNeedRefreshNew(key, getState());
     } else {

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

@@ -1,6 +1,7 @@
 import { combineReducers } from '@reduxjs/toolkit';
 
 import { TypedVariableModel } from '@grafana/data';
+import { VariableRefresh } from '@grafana/schema';
 import { dashboardReducer } from 'app/features/dashboard/state/reducers';
 
 import { DashboardState, StoreState } from '../../../types';
@@ -161,3 +162,32 @@ export function getPreloadedState(
     },
   };
 }
+
+// function to find a query variable node in the list of all variables
+export function findVariableNodeInList(allVariables: TypedVariableModel[], nodeName: string) {
+  const variableNode = allVariables.find((v) => {
+    return v.name === nodeName;
+  });
+  return variableNode;
+}
+
+/**
+ * Checks if a variable is configured to refresh when the time range changes.
+ *
+ * The function supports three types of variables: 'query', 'datasource', and 'interval'.
+ * Each of these variable types can be configured to refresh based on certain conditions.
+ * For 'query' variables, we offer a UI configuration to set refresh "on time range change."
+ * For 'interval' variables, the default configuration is often set to refresh "on time range change."
+ * For 'datasource' variables, this property is assigned to their model.
+ *
+ * Note: for datasource, It's unclear if provisioned dashboards might come with this default setting for time range.
+ *
+ * @param variable - The variable object with its type and refresh setting
+ * @returns True if the variable should refresh on time range change, otherwise False
+ */
+export function isVariableOnTimeRangeConfigured(variable: TypedVariableModel) {
+  return (
+    (variable.type === 'query' || variable.type === 'datasource' || variable.type === 'interval') &&
+    variable.refresh === VariableRefresh.onTimeRangeChanged
+  );
+}

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

@@ -12,10 +12,11 @@ import { createDashboardModelFixture } from '../../dashboard/state/__fixtures__/
 import { TemplateSrv } from '../../templating/template_srv';
 import { variableAdapters } from '../adapters';
 import { createConstantVariableAdapter } from '../constant/adapter';
+import { createDataSourceVariableAdapter } from '../datasource/adapter';
 import { createIntervalVariableAdapter } from '../interval/adapter';
 import { createIntervalOptions } from '../interval/reducer';
 import { createQueryVariableAdapter } from '../query/adapter';
-import { constantBuilder, intervalBuilder, queryBuilder } from '../shared/testing/builders';
+import { constantBuilder, intervalBuilder, queryBuilder, datasourceBuilder } from '../shared/testing/builders';
 import { VariableRefresh } from '../types';
 import { toKeyedVariableIdentifier, toVariablePayload } from '../utils';
 
@@ -35,6 +36,7 @@ variableAdapters.setInit(() => [
   createIntervalVariableAdapter(),
   createConstantVariableAdapter(),
   createQueryVariableAdapter(),
+  createDataSourceVariableAdapter(),
 ]);
 
 const metricFindQuery = jest
@@ -121,7 +123,7 @@ const getTestContext = (dashboard: DashboardModel) => {
   };
 };
 
-const getTestContextVariables = (dashboard: DashboardModel) => {
+const getTestContextVariables = (dashboard: DashboardModel, customeVariables?: object) => {
   jest.clearAllMocks();
 
   const key = 'key';
@@ -165,6 +167,14 @@ const getTestContextVariables = (dashboard: DashboardModel) => {
     .withRefresh(VariableRefresh.onTimeRangeChanged)
     .build();
 
+  const varQueryWithNoDependentNodes = queryBuilder()
+    .withId('d')
+    .withRootStateKey(key)
+    .withName('queryDNoDependentNodes')
+    .withQuery('test query')
+    .withRefresh(VariableRefresh.onTimeRangeChanged)
+    .build();
+
   const range: TimeRange = {
     from: dateTime(new Date().getTime()).subtract(1, 'minutes'),
     to: dateTime(new Date().getTime()),
@@ -192,6 +202,8 @@ const getTestContextVariables = (dashboard: DashboardModel) => {
       a: { ...queryA },
       b: { ...queryB },
       c: { ...queryC },
+      d: { ...varQueryWithNoDependentNodes },
+      ...customeVariables,
     },
   };
   const preloadedState = {
@@ -349,7 +361,7 @@ describe('when onTimeRangeUpdated is dispatched', () => {
   describe('When onTimeRangeUpdated is dispatched without refactorVariablesTimeRange feature flag ', () => {
     silenceConsoleOutput();
     it('then with old getVariablesThatNeedRefresh we call all of them in pararell', async () => {
-      const { key, preloadedState, range, dependencies } = getTestContextVariables(getDashboardModel());
+      const { key, preloadedState, range, dependencies } = getTestContextVariables(getDashboardModel(), {});
 
       // Spying on timeRangeUpdated action
       const spyTimeRangeUpdated = jest.spyOn(actions, 'timeRangeUpdated');
@@ -382,7 +394,11 @@ describe('when onTimeRangeUpdated is dispatched', () => {
           type: 'query',
           rootStateKey: 'key',
         },
-
+        queryD: {
+          id: 'd',
+          type: 'query',
+          rootStateKey: 'key',
+        },
         interval: {
           id: 'interval-0',
           type: 'interval',
@@ -395,7 +411,7 @@ describe('when onTimeRangeUpdated is dispatched', () => {
       expect(spyTimeRangeUpdated).toHaveBeenCalledWith(expectedVariables.queryC);
       expect(spyTimeRangeUpdated).toHaveBeenCalledWith(expectedVariables.interval);
 
-      expect(spyTimeRangeUpdated).toHaveBeenCalledTimes(4);
+      expect(spyTimeRangeUpdated).toHaveBeenCalledTimes(5);
 
       spyTimeRangeUpdated.mockRestore();
     });
@@ -426,9 +442,8 @@ describe('when onTimeRangeUpdated is dispatched', () => {
       // Based on the algorithm of getVariablesThatNeedRefreshNew, we have the following variables:
       // - queryA: This is a Query variable and has no dependents. According to the algorithm, Query variables without dependents
       //   should be refreshed. Hence, it's expected to be in the refreshed variables list.
-      // - interval: This is an interval variable without any dependents. Similar to queryA, the algorithm specifies that
+      // - queryD and interval: These are variables without any dependents. Similar to queryA, the algorithm specifies that
-      //   variables without dependents should be refreshed when the time range changes. Hence, interval is also expected
+      //   variables without dependents should be refreshed when the time range changes.
-      //   to be refreshed.
       //   Note: Variables 'b' and 'c' are not expected to be in the refreshed variables list even though they're set to
       // refresh on time range change. This is because they have dependencies ('b' and 'c' depend on 'a'). The algorithm
       // optimizes the refreshing process by refreshing only independent variables and those that have dependents.
@@ -440,6 +455,11 @@ describe('when onTimeRangeUpdated is dispatched', () => {
           type: 'query',
           rootStateKey: 'key',
         },
+        queryD: {
+          id: 'd',
+          type: 'query',
+          rootStateKey: 'key',
+        },
         interval: {
           id: 'interval-0',
           type: 'interval',
@@ -449,8 +469,97 @@ describe('when onTimeRangeUpdated is dispatched', () => {
 
       // Asserting that timeRangeUpdated action has been called with the correct arguments
       expect(spyTimeRangeUpdated).toHaveBeenCalledWith(expectedVariables.queryA);
-      expect(spyTimeRangeUpdated).toHaveBeenCalledWith(expectedVariables.interval);
+      expect(spyTimeRangeUpdated).toHaveBeenCalledWith(expectedVariables.queryD);
-      expect(spyTimeRangeUpdated).toHaveBeenCalledTimes(2);
+      expect(spyTimeRangeUpdated).toHaveBeenCalledTimes(3);
+
+      spyTimeRangeUpdated.mockRestore();
+    });
+
+    // query variables can depend on datasource variables, but currently datasource variables do not refresh on time
+    // range change via the UI. This test ensures that query variables that depend on datasource variables are refreshed
+    it('Should ensure query variable is refreshed when dependent on a non-refreshing datasource variable', async () => {
+      const dataSourceVariable = datasourceBuilder()
+        .withId('dsVar')
+        .withRootStateKey('key')
+        .withName('datasource-1')
+        .withOptions('a ds')
+        .withCurrent('a ds')
+        .build();
+
+      const queryE = queryBuilder()
+        .withId('e')
+        .withName('e')
+        .withRootStateKey('key')
+        .withQuery('query ${dsVar}')
+        .withRefresh(VariableRefresh.onTimeRangeChanged)
+        .build();
+
+      const queryF = queryBuilder()
+        .withId('f')
+        .withName('f')
+        .withRootStateKey('key')
+        .withQuery('query ${e}')
+        .withRefresh(VariableRefresh.onTimeRangeChanged)
+        .build();
+
+      const customVariables = {
+        dsVar: dataSourceVariable,
+        queryE,
+        queryF,
+      };
+
+      const { key, preloadedState, range, dependencies } = getTestContextVariables(
+        getDashboardModel(),
+        customVariables
+      );
+
+      // Spying on timeRangeUpdated action
+      const spyTimeRangeUpdated = jest.spyOn(actions, 'timeRangeUpdated');
+
+      // Initiating the Redux testing environment
+      await reduxTester<RootReducerType>({ preloadedState, debug: true })
+        .givenRootReducer(getRootReducer())
+        .whenActionIsDispatched(toKeyedAction(key, variablesInitTransaction({ uid: key })))
+        .whenAsyncActionIsDispatched(onTimeRangeUpdated(key, range, dependencies), true);
+
+      // Defining the variables that are expected to be refreshed
+      // Based on the algorithm of getVariablesThatNeedRefreshNew, we have the following variables:
+      // - queryA, queryD and interval: These are variables with no dependents. According to the algorithm, should be refreshed.
+      // - queryE: This is a query variable with a dependent node (queryF) but also has a dependency on DsVar datasource variable.
+      //   because DSVar is not configured to refresh on time range,this means that queryE is the
+      //   variable that is expected to be refreshed
+      // Note: Variables 'b' and 'c', f are not expected to be in the refreshed variables list even though they're set to
+      // refresh on time range change. This is because they have dependencies ('b' and 'c' depend on 'a').
+
+      const expectedVariables = {
+        queryA: {
+          id: 'a',
+          type: 'query',
+          rootStateKey: 'key',
+        },
+        queryD: {
+          id: 'd',
+          type: 'query',
+          rootStateKey: 'key',
+        },
+        queryE: {
+          id: 'e',
+          type: 'query',
+          rootStateKey: 'key',
+        },
+
+        interval: {
+          id: 'interval-0',
+          type: 'interval',
+          rootStateKey: 'key',
+        },
+      };
+
+      // Asserting that timeRangeUpdated action has been called with the correct arguments
+      expect(spyTimeRangeUpdated).toHaveBeenCalledWith(expectedVariables.queryA);
+      expect(spyTimeRangeUpdated).toHaveBeenCalledWith(expectedVariables.queryD);
+      expect(spyTimeRangeUpdated).toHaveBeenCalledWith(expectedVariables.queryE);
+      expect(spyTimeRangeUpdated).toHaveBeenCalledTimes(4);
 
       spyTimeRangeUpdated.mockRestore();
     });