Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Datasource: Fix - apply default query also to queries in new panels #59625

Merged
merged 3 commits into from Dec 5, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
23 changes: 22 additions & 1 deletion public/app/core/utils/query.test.ts
@@ -1,6 +1,10 @@
import { DataQuery } from '@grafana/data';

import { getNextRefIdChar } from './query';
import { getNextRefIdChar, queryIsEmpty } from './query';

export interface TestQuery extends DataQuery {
name?: string;
}

function dataQueryHelper(ids: string[]): DataQuery[] {
return ids.map((letter) => {
Expand Down Expand Up @@ -29,3 +33,20 @@ describe('Get next refId char', () => {
expect(getNextRefIdChar(singleExtendedDataQuery)).toEqual('AA');
});
});

describe('queryIsEmpty', () => {
it('should return true if query only includes props that are defined in the DataQuery interface', () => {
const testQuery: DataQuery = { refId: 'A' };
expect(queryIsEmpty(testQuery)).toEqual(true);
});

it('should return true if query only includes props that are defined in the DataQuery interface and a label prop', () => {
const testQuery: DataQuery & { label: string } = { refId: 'A', label: '' };
expect(queryIsEmpty(testQuery)).toEqual(true);
});

it('should return false if query only includes props that are not defined in the DataQuery interface', () => {
const testQuery: TestQuery = { refId: 'A', name: 'test' };
expect(queryIsEmpty(testQuery)).toEqual(false);
});
});
17 changes: 17 additions & 0 deletions public/app/core/utils/query.ts
Expand Up @@ -9,6 +9,23 @@ export const getNextRefIdChar = (queries: DataQuery[]): string => {
}
};

// This function checks if the query has defined properties beyond those defined in the DataQuery interface.
export function queryIsEmpty(query: DataQuery): boolean {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check does not feel future proof as it doesn't take new props into consideration. Any ideas of how to make this more robust?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this not the same as just doing Object.keys(query).length === 0 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently this function is checking if the query has prop that is not defined in the DataQuery interface. So basically if it the data source has called onChange for the given query.

const dataQueryProps = ['refId', 'hide', 'key', 'queryType', 'datasource'];

for (const key in query) {
// label is not a DataQuery prop, but it's defined in the query when called from the QueryRunner.
if (key === 'label') {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also seems like the query runner adds this label prop to the query. I have no idea where this is coming from.

continue;
}
if (!dataQueryProps.includes(key)) {
return false;
}
}

return true;
}

export function addQuery(queries: DataQuery[], query?: Partial<DataQuery>, datasource?: DataSourceRef): DataQuery[] {
const q = query || {};
q.refId = getNextRefIdChar(queries);
Expand Down
8 changes: 6 additions & 2 deletions public/app/features/query/components/QueryGroup.tsx
Expand Up @@ -17,7 +17,7 @@ import { Button, CustomScrollbar, HorizontalGroup, InlineFormLabel, Modal, style
import { PluginHelp } from 'app/core/components/PluginHelp/PluginHelp';
import config from 'app/core/config';
import { backendSrv } from 'app/core/services/backend_srv';
import { addQuery } from 'app/core/utils/query';
import { addQuery, queryIsEmpty } from 'app/core/utils/query';
import { dataSource as expressionDatasource } from 'app/features/expressions/ExpressionDatasource';
import { DashboardQueryEditor, isSharedDashboardQuery } from 'app/plugins/datasource/dashboard';
import { QueryGroupDataSource, QueryGroupOptions } from 'app/types';
Expand Down Expand Up @@ -96,7 +96,11 @@ export class QueryGroup extends PureComponent<Props, State> {
const dsSettings = this.dataSourceSrv.getInstanceSettings(options.dataSource);
const defaultDataSource = await this.dataSourceSrv.get();
const datasource = ds.getRef();
const queries = options.queries.map((q) => (q.datasource ? q : { ...q, datasource }));
const queries = options.queries.map((q) => ({
...(queryIsEmpty(q) && ds?.getDefaultQuery?.(CoreApp.PanelEditor)),
datasource,
...q,
}));
this.setState({
queries,
dataSource: ds,
Expand Down
7 changes: 7 additions & 0 deletions public/app/features/query/state/runRequest.ts
Expand Up @@ -6,6 +6,7 @@ import { catchError, map, mapTo, share, takeUntil, tap } from 'rxjs/operators';
// Utils & Services
// Types
import {
CoreApp,
DataFrame,
DataQueryError,
DataQueryRequest,
Expand All @@ -23,6 +24,7 @@ import {
import { toDataQueryError } from '@grafana/runtime';
import { isExpressionReference } from '@grafana/runtime/src/utils/DataSourceWithBackend';
import { backendSrv } from 'app/core/services/backend_srv';
import { queryIsEmpty } from 'app/core/utils/query';
import { dataSource as expressionDatasource } from 'app/features/expressions/ExpressionDatasource';
import { ExpressionQuery } from 'app/features/expressions/types';

Expand Down Expand Up @@ -174,6 +176,11 @@ export function callQueryMethod(
request: DataQueryRequest,
queryFunction?: typeof datasource.query
) {
// If the datasource has defined a default query, make sure it's applied if the query is empty
request.targets = request.targets.map((t) =>
queryIsEmpty(t) ? { ...datasource?.getDefaultQuery?.(CoreApp.PanelEditor), ...t } : t
);

// If its a public datasource, just return the result. Expressions will be handled on the backend.
if (datasource.type === 'public-ds') {
return from(datasource.query(request));
Expand Down
51 changes: 51 additions & 0 deletions public/app/features/query/state/updateQueries.test.ts
Expand Up @@ -5,6 +5,7 @@ import {
DataSourceWithQueryImportSupport,
} from '@grafana/data';
import { ExpressionDatasourceRef } from '@grafana/runtime/src/utils/DataSourceWithBackend';
import { TestQuery } from 'app/core/utils/query.test';

import { updateQueries } from './updateQueries';

Expand Down Expand Up @@ -41,6 +42,9 @@ const newUidSameTypeDS = {
} as DataSourceApi;

describe('updateQueries', () => {
afterEach(() => {
jest.clearAllMocks();
});
it('Should update all queries except expression query when changing data source with same type', async () => {
const updated = await updateQueries(
newUidSameTypeDS,
Expand Down Expand Up @@ -111,6 +115,53 @@ describe('updateQueries', () => {
expect(updated[0].datasource).toEqual({ type: 'new-type', uid: 'new-uid' });
});

it('Should clear queries and get default query from ds when changing type', async () => {
newUidDS.getDefaultQuery = jest.fn().mockReturnValue({ test: 'default-query1' } as Partial<TestQuery>);
const updated = await updateQueries(
newUidDS,
'new-uid',
[
{
refId: 'A',
datasource: {
uid: 'old-uid',
type: 'old-type',
},
},
{
refId: 'B',
datasource: {
uid: 'old-uid',
type: 'old-type',
},
},
],
oldUidDS
);

expect(newUidDS.getDefaultQuery).toHaveBeenCalled();
expect(updated as TestQuery[]).toEqual([
{
datasource: { type: 'new-type', uid: 'new-uid' },
refId: 'A',
test: 'default-query1',
},
]);
});

it('Should return default query from ds when changing type and no new queries exist', async () => {
newUidDS.getDefaultQuery = jest.fn().mockReturnValue({ test: 'default-query2' } as Partial<TestQuery>);
const updated = await updateQueries(newUidDS, 'new-uid', [], oldUidDS);
expect(newUidDS.getDefaultQuery).toHaveBeenCalled();
expect(updated as TestQuery[]).toEqual([
{
datasource: { type: 'new-type', uid: 'new-uid' },
refId: 'A',
test: 'default-query2',
},
]);
});

it('Should preserve query data source when changing to mixed', async () => {
const updated = await updateQueries(
mixedDS,
Expand Down
7 changes: 4 additions & 3 deletions public/app/features/query/state/updateQueries.ts
@@ -1,4 +1,4 @@
import { DataQuery, DataSourceApi, hasQueryExportSupport, hasQueryImportSupport } from '@grafana/data';
import { CoreApp, DataQuery, DataSourceApi, hasQueryExportSupport, hasQueryImportSupport } from '@grafana/data';
import { isExpressionReference } from '@grafana/runtime/src/utils/DataSourceWithBackend';

export async function updateQueries(
Expand All @@ -9,6 +9,7 @@ export async function updateQueries(
): Promise<DataQuery[]> {
let nextQueries = queries;
const datasource = { type: nextDS.type, uid: nextDSUidOrVariableExpression };
const DEFAULT_QUERY = { ...nextDS?.getDefaultQuery?.(CoreApp.PanelEditor), datasource, refId: 'A' };

// we are changing data source type
if (currentDS?.meta.id !== nextDS.meta.id) {
Expand All @@ -27,12 +28,12 @@ export async function updateQueries(
}
// Otherwise clear queries
else {
return [{ refId: 'A', datasource }];
return [DEFAULT_QUERY];
}
}

if (nextQueries.length === 0) {
return [{ refId: 'A', datasource }];
return [DEFAULT_QUERY];
}

// Set data source on all queries except expression queries
Expand Down