Elasticsearch: omit query_string filter when no lucene query is provided (#42060)

* Elasticsearch: omit query_string filter when no query is provided

* Fix tests

* optional lucene query

* improve test and types

public/app/plugins/datasource/elasticsearch/datasource.test.ts

@@ -69,8 +69,8 @@ function getTestContext({
   fetchMock.mockImplementation(mockImplementation ?? defaultMock);
 
   const templateSrv: any = {
-    replace: jest.fn((text) => {
+    replace: jest.fn((text?: string) => {
-      if (text.startsWith('$')) {
+      if (text?.startsWith('$')) {
         return `resolvedVariable`;
       } else {
         return text;
@@ -884,7 +884,7 @@ describe('ElasticDatasource', function (this: any) {
     expect((interpolatedQuery.bucketAggs![0] as Filters).settings!.filters![0].query).toBe('resolvedVariable');
   });
 
-  it('should correctly handle empty query strings', () => {
+  it('should correctly handle empty query strings in filters bucket aggregation', () => {
     const { ds } = getTestContext();
     const query: ElasticsearchQuery = {
       refId: 'A',
@@ -895,7 +895,6 @@ describe('ElasticDatasource', function (this: any) {
 
     const interpolatedQuery = ds.interpolateVariablesInQueries([query], {})[0];
 
-    expect(interpolatedQuery.query).toBe('*');
     expect((interpolatedQuery.bucketAggs![0] as Filters).settings!.filters![0].query).toBe('*');
   });
 });

public/app/plugins/datasource/elasticsearch/datasource.ts

@@ -31,7 +31,7 @@ import { IndexPattern } from './index_pattern';
 import { ElasticQueryBuilder } from './query_builder';
 import { defaultBucketAgg, hasMetricOfType } from './query_def';
 import { getTemplateSrv, TemplateSrv } from 'app/features/templating/template_srv';
-import { DataLinkConfig, ElasticsearchOptions, ElasticsearchQuery } from './types';
+import { DataLinkConfig, ElasticsearchOptions, ElasticsearchQuery, TermsQuery } from './types';
 import { RowContextOptions } from '@grafana/ui/src/components/Logs/LogRowContextProvider';
 import { metricAggregationConfig } from './components/QueryEditor/MetricAggregationsEditor/utils';
 import {
@@ -220,7 +220,7 @@ export class ElasticDatasource
     const annotation = options.annotation;
     const timeField = annotation.timeField || '@timestamp';
     const timeEndField = annotation.timeEndField || null;
-    const queryString = annotation.query || '*';
+    const queryString = annotation.query;
     const tagsField = annotation.tagsField || 'tags';
     const textField = annotation.textField || null;
 
@@ -243,8 +243,8 @@ export class ElasticDatasource
       dateRanges.push({ range: rangeEnd });
     }
 
-    const queryInterpolated = this.templateSrv.replace(queryString, {}, 'lucene');
+    const queryInterpolated = this.interpolateLuceneQuery(queryString);
-    const query = {
+    const query: any = {
       bool: {
         filter: [
           {
@@ -253,15 +253,17 @@ export class ElasticDatasource
               minimum_should_match: 1,
             },
           },
-          {
-            query_string: {
-              query: queryInterpolated,
-            },
-          },
         ],
       },
     };
 
+    if (queryInterpolated) {
+      query.bool.filter.push({
+        query_string: {
+          query: queryInterpolated,
+        },
+      });
+    }
     const data: any = {
       query,
       size: 10000,
@@ -361,9 +363,8 @@ export class ElasticDatasource
     );
   }
 
-  private interpolateLuceneQuery(queryString: string, scopedVars: ScopedVars) {
+  private interpolateLuceneQuery(queryString: string, scopedVars?: ScopedVars) {
-    // Elasticsearch queryString should always be '*' if empty string
+    return this.templateSrv.replace(queryString, scopedVars, 'lucene');
-    return this.templateSrv.replace(queryString, scopedVars, 'lucene') || '*';
   }
 
   interpolateVariablesInQueries(queries: ElasticsearchQuery[], scopedVars: ScopedVars): ElasticsearchQuery[] {
@@ -377,7 +378,7 @@ export class ElasticDatasource
             ...bucketAgg.settings,
             filters: bucketAgg.settings?.filters?.map((filter) => ({
               ...filter,
-              query: this.interpolateLuceneQuery(filter.query || '', scopedVars),
+              query: this.interpolateLuceneQuery(filter.query, scopedVars) || '*',
             })),
           },
         };
@@ -646,14 +647,14 @@ export class ElasticDatasource
 
         target.metrics = [];
         // Setting this for metrics queries that are typed as logs
-        queryObj = this.queryBuilder.getLogsQuery(target, limit, adhocFilters, target.query);
+        queryObj = this.queryBuilder.getLogsQuery(target, limit, adhocFilters);
       } else {
         logLimits.push();
         if (target.alias) {
-          target.alias = this.templateSrv.replace(target.alias, options.scopedVars, 'lucene');
+          target.alias = this.interpolateLuceneQuery(target.alias, options.scopedVars);
         }
 
-        queryObj = this.queryBuilder.build(target, adhocFilters, target.query);
+        queryObj = this.queryBuilder.build(target, adhocFilters);
       }
 
       const esQuery = JSON.stringify(queryObj);
@@ -795,7 +796,7 @@ export class ElasticDatasource
     );
   }
 
-  getTerms(queryDef: any, range = getDefaultTimeRange()): Observable<MetricFindValue[]> {
+  getTerms(queryDef: TermsQuery, range = getDefaultTimeRange()): Observable<MetricFindValue[]> {
     const searchType = gte(this.esVersion, '5.0.0') ? 'query_then_fetch' : 'count';
     const header = this.getQueryHeader(searchType, range.from, range.to);
     let esQuery = JSON.stringify(this.queryBuilder.getTermsQuery(queryDef));
@@ -842,13 +843,13 @@ export class ElasticDatasource
     const parsedQuery = JSON.parse(query);
     if (query) {
       if (parsedQuery.find === 'fields') {
-        parsedQuery.type = this.templateSrv.replace(parsedQuery.type, {}, 'lucene');
+        parsedQuery.type = this.interpolateLuceneQuery(parsedQuery.type);
         return lastValueFrom(this.getFields(parsedQuery.type, range));
       }
 
       if (parsedQuery.find === 'terms') {
-        parsedQuery.field = this.templateSrv.replace(parsedQuery.field, {}, 'lucene');
+        parsedQuery.field = this.interpolateLuceneQuery(parsedQuery.field);
-        parsedQuery.query = this.templateSrv.replace(parsedQuery.query || '*', {}, 'lucene');
+        parsedQuery.query = this.interpolateLuceneQuery(parsedQuery.query);
         return lastValueFrom(this.getTerms(parsedQuery, range));
       }
     }
@@ -861,7 +862,7 @@ export class ElasticDatasource
   }
 
   getTagValues(options: any) {
-    return lastValueFrom(this.getTerms({ field: options.key, query: '*' }));
+    return lastValueFrom(this.getTerms({ field: options.key }));
   }
 
   targetContainsTemplate(target: any) {

public/app/plugins/datasource/elasticsearch/query_builder.ts

@@ -16,7 +16,7 @@ import {
   MetricAggregationWithInlineScript,
 } from './components/QueryEditor/MetricAggregationsEditor/aggregations';
 import { defaultBucketAgg, defaultMetricAgg, findMetricById, highlightTags } from './query_def';
-import { ElasticsearchQuery } from './types';
+import { ElasticsearchQuery, TermsQuery } from './types';
 import { convertOrderByToMetricId, getScriptValue } from './utils';
 
 export class ElasticQueryBuilder {
@@ -213,7 +213,7 @@ export class ElasticQueryBuilder {
     }
   }
 
-  build(target: ElasticsearchQuery, adhocFilters?: any, queryString?: string) {
+  build(target: ElasticsearchQuery, adhocFilters?: any) {
     // make sure query has defaults;
     target.metrics = target.metrics || [defaultMetricAgg()];
     target.bucketAggs = target.bucketAggs || [defaultBucketAgg()];
@@ -221,23 +221,27 @@ export class ElasticQueryBuilder {
     let metric: MetricAggregation;
 
     let i, j, pv, nestedAggs;
-    const query = {
+    const query: any = {
       size: 0,
       query: {
         bool: {
-          filter: [
+          filter: [{ range: this.getRangeFilter() }],
-            { range: this.getRangeFilter() },
-            {
-              query_string: {
-                analyze_wildcard: true,
-                query: queryString,
-              },
-            },
-          ],
         },
       },
     };
 
+    if (target.query && target.query !== '') {
+      query.query.bool.filter = [
+        ...query.query.bool.filter,
+        {
+          query_string: {
+            analyze_wildcard: true,
+            query: target.query,
+          },
+        },
+      ];
+    }
+
     this.addAdhocFilters(query, adhocFilters);
 
     // If target doesn't have bucketAggs and type is not raw_document, it is invalid query.
@@ -433,7 +437,7 @@ export class ElasticQueryBuilder {
     return parsedValue;
   }
 
-  getTermsQuery(queryDef: any) {
+  getTermsQuery(queryDef: TermsQuery) {
     const query: any = {
       size: 0,
       query: {
@@ -493,7 +497,7 @@ export class ElasticQueryBuilder {
     return query;
   }
 
-  getLogsQuery(target: ElasticsearchQuery, limit: number, adhocFilters?: any, querystring?: string) {
+  getLogsQuery(target: ElasticsearchQuery, limit: number, adhocFilters?: any) {
     let query: any = {
       size: 0,
       query: {
@@ -509,7 +513,7 @@ export class ElasticQueryBuilder {
       query.query.bool.filter.push({
         query_string: {
           analyze_wildcard: true,
-          query: querystring,
+          query: target.query,
         },
       });
     }
@@ -518,7 +522,7 @@ export class ElasticQueryBuilder {
 
     return {
       ...query,
-      aggs: this.build(target, null, querystring).aggs,
+      aggs: this.build(target, null).aggs,
       highlight: {
         fields: {
           '*': {},

public/app/plugins/datasource/elasticsearch/specs/query_builder.test.ts

@@ -65,8 +65,7 @@ describe('ElasticQueryBuilder', () => {
             metrics: [{ type: 'avg', field: '@value', id: '1' }],
             bucketAggs: [{ type: 'date_histogram', field: '@timestamp', id: '2' }],
           },
-          100,
+          100
-          '1000'
         );
 
         const aggs = query.aggs['2'].aggs;
@@ -91,7 +90,7 @@ describe('ElasticQueryBuilder', () => {
           ],
         };
 
-        const query = builder.build(target, 100, '1000');
+        const query = builder.build(target, 100);
         const firstLevel = query.aggs['2'];
 
         if (gte(builder.esVersion, '6.0.0')) {
@@ -119,8 +118,7 @@ describe('ElasticQueryBuilder', () => {
               { type: 'date_histogram', field: '@timestamp', id: '3' },
             ],
           },
-          100,
+          100
-          '1000'
         );
 
         const firstLevel = query.aggs['2'];
@@ -148,8 +146,7 @@ describe('ElasticQueryBuilder', () => {
               { type: 'date_histogram', field: '@timestamp', id: '3' },
             ],
           },
-          100,
+          100
-          '1000'
         );
 
         expect(query.aggs['2'].terms.order._count).toEqual('asc');
@@ -171,8 +168,7 @@ describe('ElasticQueryBuilder', () => {
               { type: 'date_histogram', field: '@timestamp', id: '3' },
             ],
           },
-          100,
+          100
-          '1000'
         );
 
         const firstLevel = query.aggs['2'];
@@ -197,8 +193,7 @@ describe('ElasticQueryBuilder', () => {
               { type: 'date_histogram', field: '@timestamp', id: '3' },
             ],
           },
-          100,
+          100
-          '1000'
         );
 
         const firstLevel = query.aggs['2'];
@@ -223,8 +218,7 @@ describe('ElasticQueryBuilder', () => {
               { type: 'date_histogram', field: '@timestamp', id: '3' },
             ],
           },
-          100,
+          100
-          '1000'
         );
 
         const firstLevel = query.aggs['2'];
@@ -246,8 +240,7 @@ describe('ElasticQueryBuilder', () => {
               { type: 'date_histogram', field: '@timestamp', id: '3' },
             ],
           },
-          100,
+          100
-          '1000'
         );
 
         const firstLevel = query.aggs['2'];
@@ -273,8 +266,7 @@ describe('ElasticQueryBuilder', () => {
             ],
             bucketAggs: [{ type: 'date_histogram', field: '@timestamp', id: '3' }],
           },
-          100,
+          100
-          '1000'
         );
 
         const firstLevel = query.aggs['3'];
@@ -331,12 +323,6 @@ describe('ElasticQueryBuilder', () => {
                     },
                   },
                 },
-                {
-                  query_string: {
-                    analyze_wildcard: true,
-                    query: undefined,
-                  },
-                },
               ],
             },
           },
@@ -662,10 +648,10 @@ describe('ElasticQueryBuilder', () => {
         expect(query.query.bool.must[0].match_phrase['key1'].query).toBe('value1');
         expect(query.query.bool.must[1].match_phrase['key2'].query).toBe('value2');
         expect(query.query.bool.must_not[0].match_phrase['key2'].query).toBe('value2');
-        expect(query.query.bool.filter[2].range['key3'].lt).toBe('value3');
+        expect(query.query.bool.filter[1].range['key3'].lt).toBe('value3');
-        expect(query.query.bool.filter[3].range['key4'].gt).toBe('value4');
+        expect(query.query.bool.filter[2].range['key4'].gt).toBe('value4');
-        expect(query.query.bool.filter[4].regexp['key5']).toBe('value5');
+        expect(query.query.bool.filter[3].regexp['key5']).toBe('value5');
-        expect(query.query.bool.filter[5].bool.must_not.regexp['key6']).toBe('value6');
+        expect(query.query.bool.filter[4].bool.must_not.regexp['key6']).toBe('value6');
       });
 
       describe('getTermsQuery', () => {
@@ -709,11 +695,49 @@ describe('ElasticQueryBuilder', () => {
           expect(query.aggs['1'].terms.order._key).toBeUndefined();
           expect(query.aggs['1'].terms.order._count).toBe('asc');
         });
+
+        describe('lucene query', () => {
+          it('should add query_string filter when query is not empty', () => {
+            const luceneQuery = 'foo';
+            const query = builder.getTermsQuery({ orderBy: 'doc_count', order: 'asc', query: luceneQuery });
+
+            expect(query.query.bool.filter).toContainEqual({
+              query_string: { analyze_wildcard: true, query: luceneQuery },
+            });
+          });
+
+          it('should not add query_string filter when query is empty', () => {
+            const query = builder.getTermsQuery({ orderBy: 'doc_count', order: 'asc' });
+
+            expect(
+              query.query.bool.filter.find((filter: any) => Object.keys(filter).includes('query_string'))
+            ).toBeFalsy();
+          });
+        });
+      });
+
+      describe('lucene query', () => {
+        it('should add query_string filter when query is not empty', () => {
+          const luceneQuery = 'foo';
+          const query = builder.build({ refId: 'A', query: luceneQuery });
+
+          expect(query.query.bool.filter).toContainEqual({
+            query_string: { analyze_wildcard: true, query: luceneQuery },
+          });
+        });
+
+        it('should not add query_string filter when query is empty', () => {
+          const query = builder.build({ refId: 'A' });
+
+          expect(
+            query.query.bool.filter.find((filter: any) => Object.keys(filter).includes('query_string'))
+          ).toBeFalsy();
+        });
       });
 
       describe('getLogsQuery', () => {
         it('should return query with defaults', () => {
-          const query = builder.getLogsQuery({ refId: 'A' }, 500, null, '*');
+          const query = builder.getLogsQuery({ refId: 'A' }, 500, null);
 
           expect(query.size).toEqual(500);
 
@@ -751,18 +775,23 @@ describe('ElasticQueryBuilder', () => {
           expect(query.aggs).toMatchObject(expectedAggs);
         });
 
-        it('with querystring', () => {
+        describe('lucene query', () => {
-          const query = builder.getLogsQuery({ refId: 'A', query: 'foo' }, 500, null, 'foo');
+          it('should add query_string filter when query is not empty', () => {
+            const luceneQuery = 'foo';
+            const query = builder.getLogsQuery({ refId: 'A', query: luceneQuery }, 500, null);
 
-          const expectedQuery = {
+            expect(query.query.bool.filter).toContainEqual({
-            bool: {
+              query_string: { analyze_wildcard: true, query: luceneQuery },
-              filter: [
+            });
-                { range: { '@timestamp': { gte: '$timeFrom', lte: '$timeTo', format: 'epoch_millis' } } },
+          });
-                { query_string: { analyze_wildcard: true, query: 'foo' } },
+
-              ],
+          it('should not add query_string filter when query is empty', () => {
-            },
+            const query = builder.getLogsQuery({ refId: 'A' }, 500, null);
-          };
+
-          expect(query.query).toEqual(expectedQuery);
+            expect(
+              query.query.bool.filter.find((filter: any) => Object.keys(filter).includes('query_string'))
+            ).toBeFalsy();
+          });
         });
 
         it('with adhoc filters', () => {
@@ -775,7 +804,7 @@ describe('ElasticQueryBuilder', () => {
             { key: 'key5', operator: '=~', value: 'value5' },
             { key: 'key6', operator: '!~', value: 'value6' },
           ];
-          const query = builder.getLogsQuery({ refId: 'A' }, 500, adhocFilters, '*');
+          const query = builder.getLogsQuery({ refId: 'A' }, 500, adhocFilters);
 
           expect(query.query.bool.must[0].match_phrase['key1'].query).toBe('value1');
           expect(query.query.bool.must_not[0].match_phrase['key2'].query).toBe('value2');

public/app/plugins/datasource/elasticsearch/types.ts

@@ -72,6 +72,14 @@ export interface ElasticsearchQuery extends DataQuery {
   timeField?: string;
 }
 
+export interface TermsQuery {
+  query?: string;
+  size?: number;
+  field?: string;
+  order?: 'asc' | 'desc';
+  orderBy?: string;
+}
+
 export type DataLinkConfig = {
   field: string;
   url: string;