diff --git a/.betterer.results b/.betterer.results index 9f5c15df513..0055fe606a3 100644 --- a/.betterer.results +++ b/.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.", "14"] + [0, 0, 0, "Unexpected any. Specify a different type.", "13"] ], "public/app/features/variables/state/keyedVariablesReducer.ts:5381": [ [0, 0, 0, "Unexpected any. Specify a different type.", "0"], diff --git a/public/app/features/variables/state/actions.ts b/public/app/features/variables/state/actions.ts index 0ac422a8725..5feea4c65dc 100644 --- a/public/app/features/variables/state/actions.ts +++ b/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 isVariableTimeRange = - parentVariableNode && parentVariableNode.refresh === VariableRefresh.onTimeRangeChanged; - // + const parentVariableNode = findVariableNodeInList(allVariables, node.name); + if (!parentVariableNode) { + 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 ( - parentVariableNode && - parentVariableNode.refresh && - parentVariableNode.refresh !== VariableRefresh.onTimeRangeChanged - ) { + if (!isVariableTimeRange && node.outputEdges.length > 0) { 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 { diff --git a/public/app/features/variables/state/helpers.ts b/public/app/features/variables/state/helpers.ts index f1d296117bf..90f3057e093 100644 --- a/public/app/features/variables/state/helpers.ts +++ b/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 + ); +} diff --git a/public/app/features/variables/state/onTimeRangeUpdated.test.ts b/public/app/features/variables/state/onTimeRangeUpdated.test.ts index 8c641ce359b..40b6c9ab037 100644 --- a/public/app/features/variables/state/onTimeRangeUpdated.test.ts +++ b/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,10 +442,9 @@ 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 - // variables without dependents should be refreshed when the time range changes. Hence, interval is also expected - // to be refreshed. - // Note: Variables 'b' and 'c' are not expected to be in the refreshed variables list even though they're set to + // - 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. + // 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. // Once 'a' is refreshed, it will trigger a cascading refresh of 'b' and 'c'. @@ -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).toHaveBeenCalledTimes(2); + expect(spyTimeRangeUpdated).toHaveBeenCalledWith(expectedVariables.queryD); + 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({ 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(); });