From c48ca778151f13e0a93c5b02cea124413d2e9665 Mon Sep 17 00:00:00 2001 From: Giordano Ricci Date: Mon, 6 Mar 2023 11:33:39 +0000 Subject: [PATCH] [v9.4.x] Explore: Avoid changing queries twice when importing a query in mixed mode (#64206) Explore: Avoid changing queries twice when importing a query in mixed mode (#63804) * Explore: avoid changing queries twice when importing a query in mixed mode * move logic to thunk * add tests * improve tests (cherry picked from commit 24a4a24a895f8c7a2cfaa12bf03e531b6fcf6d29) --- public/app/features/explore/QueryRows.tsx | 25 +-- .../app/features/explore/state/query.test.ts | 142 +++++++++++++++++- public/app/features/explore/state/query.ts | 31 +++- 3 files changed, 173 insertions(+), 25 deletions(-) diff --git a/public/app/features/explore/QueryRows.tsx b/public/app/features/explore/QueryRows.tsx index da17315b973..b063c96a4ad 100644 --- a/public/app/features/explore/QueryRows.tsx +++ b/public/app/features/explore/QueryRows.tsx @@ -2,7 +2,7 @@ import { createSelector } from '@reduxjs/toolkit'; import React, { useCallback, useMemo } from 'react'; import { CoreApp } from '@grafana/data'; -import { getDataSourceSrv, reportInteraction } from '@grafana/runtime'; +import { reportInteraction } from '@grafana/runtime'; import { DataQuery } from '@grafana/schema'; import { getNextRefIdChar } from 'app/core/utils/query'; import { useDispatch, useSelector } from 'app/types'; @@ -11,7 +11,7 @@ import { ExploreId } from 'app/types/explore'; import { getDatasourceSrv } from '../plugins/datasource_srv'; import { QueryEditorRows } from '../query/components/QueryEditorRows'; -import { runQueries, changeQueriesAction, importQueries } from './state/query'; +import { runQueries, changeQueries } from './state/query'; import { getExploreItemSelector } from './state/selectors'; interface Props { @@ -50,25 +50,10 @@ export const QueryRows = ({ exploreId }: Props) => { }, [dispatch, exploreId]); const onChange = useCallback( - async (newQueries: DataQuery[]) => { - dispatch(changeQueriesAction({ queries: newQueries, exploreId })); - - for (const newQuery of newQueries) { - for (const oldQuery of queries) { - if (newQuery.refId === oldQuery.refId && newQuery.datasource?.type !== oldQuery.datasource?.type) { - const queryDatasource = await getDataSourceSrv().get(newQuery.datasource); - const targetDS = await getDataSourceSrv().get({ uid: newQuery.datasource?.uid }); - dispatch(importQueries(exploreId, queries, queryDatasource, targetDS, newQuery.refId)); - } - } - } - - // if we are removing a query we want to run the remaining ones - if (newQueries.length < queries.length) { - onRunQueries(); - } + (newQueries: DataQuery[]) => { + dispatch(changeQueries({ exploreId, queries: newQueries })); }, - [dispatch, exploreId, onRunQueries, queries] + [dispatch, exploreId] ); const onAddQuery = useCallback( diff --git a/public/app/features/explore/state/query.test.ts b/public/app/features/explore/state/query.test.ts index 4c461a0fd78..219e84cdac3 100644 --- a/public/app/features/explore/state/query.test.ts +++ b/public/app/features/explore/state/query.test.ts @@ -14,7 +14,7 @@ import { RawTimeRange, SupplementaryQueryType, } from '@grafana/data'; -import { DataQuery } from '@grafana/schema'; +import { DataQuery, DataSourceRef } from '@grafana/schema'; import { ExploreId, ExploreItemState, StoreState, ThunkDispatch } from 'app/types'; import { reducerTester } from '../../../../test/core/redux/reducerTester'; @@ -38,7 +38,9 @@ import { scanStopAction, setSupplementaryQueryEnabled, cleanSupplementaryQueryDataProviderAction, + changeQueries, } from './query'; +import * as actions from './query'; import { makeExplorePaneState } from './utils'; const { testRange, defaultInitialState } = createDefaultInitialState(); @@ -54,10 +56,10 @@ const datasources: DataSourceApi[] = [ } as DataSourceApi, { name: 'testDs2', - type: 'postgres', + type: 'mysql', uid: 'ds2', getRef: () => { - return { type: 'postgres', uid: 'ds2' }; + return { type: 'mysql', uid: 'ds2' }; }, } as DataSourceApi, ]; @@ -77,7 +79,15 @@ jest.mock('@grafana/runtime', () => ({ }), getDataSourceSrv: () => { return { - get: (uid?: string) => datasources.find((ds) => ds.uid === uid) || datasources[0], + get: (ref?: DataSourceRef | string) => { + if (!ref) { + return datasources[0]; + } + + return ( + datasources.find((ds) => (typeof ref === 'string' ? ds.uid === ref : ds.uid === ref.uid)) || datasources[0] + ); + }, }; }, })); @@ -197,6 +207,130 @@ describe('running queries', () => { }); }); +describe('changeQueries', () => { + // Due to how spyOn works (it removes `type`, `match` and `toString` from the spied function, on which we rely on in the reducer), + // we are repeating the following tests twice, once to chck the resulting state and once to check that the correct actions are dispatched. + describe('calls the correct actions', () => { + afterEach(() => { + jest.restoreAllMocks(); + }); + it('should import queries when datasource is changed', async () => { + jest.spyOn(actions, 'importQueries'); + jest.spyOn(actions, 'changeQueriesAction'); + + const originalQueries = [{ refId: 'A', datasource: datasources[0].getRef() }]; + + const { dispatch } = configureStore({ + ...defaultInitialState, + explore: { + [ExploreId.left]: { + ...defaultInitialState.explore[ExploreId.left], + datasourceInstance: datasources[0], + queries: originalQueries, + }, + }, + } as unknown as Partial); + + await dispatch( + changeQueries({ + queries: [{ refId: 'A', datasource: datasources[1].getRef() }], + exploreId: ExploreId.left, + }) + ); + + expect(actions.changeQueriesAction).not.toHaveBeenCalled(); + expect(actions.importQueries).toHaveBeenCalledWith( + ExploreId.left, + originalQueries, + datasources[0], + datasources[1], + originalQueries[0].refId + ); + }); + + it('should not import queries when datasource is not changed', async () => { + jest.spyOn(actions, 'importQueries'); + jest.spyOn(actions, 'changeQueriesAction'); + + const { dispatch } = configureStore({ + ...defaultInitialState, + explore: { + [ExploreId.left]: { + ...defaultInitialState.explore[ExploreId.left], + datasourceInstance: datasources[0], + queries: [{ refId: 'A', datasource: datasources[0].getRef() }], + }, + }, + } as unknown as Partial); + + await dispatch( + changeQueries({ + queries: [{ refId: 'A', datasource: datasources[0].getRef(), queryType: 'someValue' }], + exploreId: ExploreId.left, + }) + ); + + expect(actions.changeQueriesAction).toHaveBeenCalled(); + expect(actions.importQueries).not.toHaveBeenCalled(); + }); + }); + + describe('correctly modifies the state', () => { + it('should import queries when datasource is changed', async () => { + const originalQueries = [{ refId: 'A', datasource: datasources[0].getRef() }]; + + const { dispatch, getState } = configureStore({ + ...defaultInitialState, + explore: { + [ExploreId.left]: { + ...defaultInitialState.explore[ExploreId.left], + datasourceInstance: datasources[0], + queries: originalQueries, + }, + }, + } as unknown as Partial); + + await dispatch( + changeQueries({ + queries: [{ refId: 'A', datasource: datasources[1].getRef() }], + exploreId: ExploreId.left, + }) + ); + + expect(getState().explore[ExploreId.left].queries[0]).toHaveProperty('refId', 'A'); + expect(getState().explore[ExploreId.left].queries[0]).toHaveProperty('datasource', datasources[1].getRef()); + }); + + it('should not import queries when datasource is not changed', async () => { + const { dispatch, getState } = configureStore({ + ...defaultInitialState, + explore: { + [ExploreId.left]: { + ...defaultInitialState.explore[ExploreId.left], + datasourceInstance: datasources[0], + queries: [{ refId: 'A', datasource: datasources[0].getRef() }], + }, + }, + } as unknown as Partial); + + await dispatch( + changeQueries({ + queries: [{ refId: 'A', datasource: datasources[0].getRef(), queryType: 'someValue' }], + exploreId: ExploreId.left, + }) + ); + + expect(getState().explore[ExploreId.left].queries[0]).toHaveProperty('refId', 'A'); + expect(getState().explore[ExploreId.left].queries[0]).toHaveProperty('datasource', datasources[0].getRef()); + expect(getState().explore[ExploreId.left].queries[0]).toEqual({ + refId: 'A', + datasource: datasources[0].getRef(), + queryType: 'someValue', + }); + }); + }); +}); + describe('importing queries', () => { describe('when importing queries between the same type of data source', () => { it('remove datasource property from all of the queries', async () => { diff --git a/public/app/features/explore/state/query.ts b/public/app/features/explore/state/query.ts index 531ebeb5374..3a24a45c655 100644 --- a/public/app/features/explore/state/query.ts +++ b/public/app/features/explore/state/query.ts @@ -37,7 +37,7 @@ import { CorrelationData } from 'app/features/correlations/useCorrelations'; import { getTimeZone } from 'app/features/profile/state/selectors'; import { MIXED_DATASOURCE_NAME } from 'app/plugins/datasource/mixed/MixedDataSource'; import { store } from 'app/store/store'; -import { ExploreItemState, ExplorePanelData, ThunkDispatch, ThunkResult } from 'app/types'; +import { createAsyncThunk, ExploreItemState, ExplorePanelData, ThunkDispatch, ThunkResult } from 'app/types'; import { ExploreId, ExploreState, QueryOptions, SupplementaryQueries } from 'app/types/explore'; import { notifyApp } from '../../../core/actions'; @@ -291,6 +291,35 @@ const getImportableQueries = async ( return addDatasourceToQueries(targetDataSource, queriesOut); }; +export const changeQueries = createAsyncThunk( + 'explore/changeQueries', + async ({ queries, exploreId }, { getState, dispatch }) => { + let queriesImported = false; + const oldQueries = getState().explore[exploreId]!.queries; + + for (const newQuery of queries) { + for (const oldQuery of oldQueries) { + if (newQuery.refId === oldQuery.refId && newQuery.datasource?.type !== oldQuery.datasource?.type) { + const queryDatasource = await getDataSourceSrv().get(oldQuery.datasource); + const targetDS = await getDataSourceSrv().get({ uid: newQuery.datasource?.uid }); + await dispatch(importQueries(exploreId, oldQueries, queryDatasource, targetDS, newQuery.refId)); + queriesImported = true; + } + } + } + + // Importing queries changes the same state, therefore if we are importing queries we don't want to change the state again + if (!queriesImported) { + dispatch(changeQueriesAction({ queries, exploreId })); + } + + // if we are removing a query we want to run the remaining ones + if (queries.length < queries.length) { + dispatch(runQueries(exploreId)); + } + } +); + /** * Import queries from previous datasource if possible eg Loki and Prometheus have similar query language so the * labels part can be reused to get similar data.