Elastic: prevents datasource from throwing unexpected errors for invalid JSON (#24999)

* Chore: adds error handling to get requests in elasticsearch datasource

* Chore: updates elasticsearch datasource response parsing checks

* Chore: updates elasticsearch non-json errors description

* Chore: removes error catching from query methods to move it to the request method in elasticsearch

* Chore: fixes a typo in elastic response error message

* Chore: moves elasticsearch error handling to request method

* Chore: replaces datasource url in mock elasticsearch datasource

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

@@ -9,6 +9,8 @@ import { TemplateSrv } from 'app/features/templating/template_srv';
 import { DataSourceInstanceSettings } from '@grafana/data';
 import { ElasticsearchOptions, ElasticsearchQuery } from './types';
 
+const ELASTICSEARCH_MOCK_URL = 'http://elasticsearch.local';
+
 jest.mock('@grafana/runtime', () => ({
   ...jest.requireActual('@grafana/runtime'),
   getBackendSrv: () => backendSrv,
@@ -77,7 +79,7 @@ describe('ElasticDatasource', function(this: any) {
   describe('When testing datasource with index pattern', () => {
     beforeEach(() => {
       createDatasource({
-        url: 'http://es.com',
+        url: ELASTICSEARCH_MOCK_URL,
         database: '[asd-]YYYY.MM.DD',
         jsonData: { interval: 'Daily', esVersion: 2 } as ElasticsearchOptions,
       } as DataSourceInstanceSettings<ElasticsearchOptions>);
@@ -93,7 +95,7 @@ describe('ElasticDatasource', function(this: any) {
       ctx.ds.testDatasource();
 
       const today = toUtc().format('YYYY.MM.DD');
-      expect(requestOptions.url).toBe('http://es.com/asd-' + today + '/_mapping');
+      expect(requestOptions.url).toBe(`${ELASTICSEARCH_MOCK_URL}/asd-${today}/_mapping`);
     });
   });
 
@@ -102,7 +104,7 @@ describe('ElasticDatasource', function(this: any) {
 
     beforeEach(async () => {
       createDatasource({
-        url: 'http://es.com',
+        url: ELASTICSEARCH_MOCK_URL,
         database: '[asd-]YYYY.MM.DD',
         jsonData: { interval: 'Daily', esVersion: 2 } as ElasticsearchOptions,
       } as DataSourceInstanceSettings<ElasticsearchOptions>);
@@ -171,7 +173,7 @@ describe('ElasticDatasource', function(this: any) {
   describe('When issuing logs query with interval pattern', () => {
     async function setupDataSource(jsonData?: Partial<ElasticsearchOptions>) {
       createDatasource({
-        url: 'http://es.com',
+        url: ELASTICSEARCH_MOCK_URL,
         database: 'mock-index',
         jsonData: {
           interval: 'Daily',
@@ -236,7 +238,7 @@ describe('ElasticDatasource', function(this: any) {
 
     beforeEach(() => {
       createDatasource({
-        url: 'http://es.com',
+        url: ELASTICSEARCH_MOCK_URL,
         database: 'test',
         jsonData: { esVersion: 2 } as ElasticsearchOptions,
       } as DataSourceInstanceSettings<ElasticsearchOptions>);
@@ -274,10 +276,89 @@ describe('ElasticDatasource', function(this: any) {
     });
   });
 
+  describe('When getting an error on response', () => {
+    const query = {
+      range: {
+        from: toUtc([2020, 1, 1, 10]),
+        to: toUtc([2020, 2, 1, 10]),
+      },
+      targets: [
+        {
+          alias: '$varAlias',
+          bucketAggs: [{ type: 'date_histogram', field: '@timestamp', id: '1' }],
+          metrics: [{ type: 'count', id: '1' }],
+          query: 'escape\\:test',
+        },
+      ],
+    };
+
+    createDatasource({
+      url: ELASTICSEARCH_MOCK_URL,
+      database: '[asd-]YYYY.MM.DD',
+      jsonData: { interval: 'Daily', esVersion: 7 } as ElasticsearchOptions,
+    } as DataSourceInstanceSettings<ElasticsearchOptions>);
+
+    it('should process it properly', async () => {
+      datasourceRequestMock.mockImplementation(() => {
+        return Promise.resolve({
+          data: {
+            took: 1,
+            responses: [
+              {
+                error: {
+                  reason: 'all shards failed',
+                },
+                status: 400,
+              },
+            ],
+          },
+        });
+      });
+
+      const errObject = {
+        data: '{\n    "reason": "all shards failed"\n}',
+        message: 'all shards failed',
+      };
+
+      try {
+        await ctx.ds.query(query);
+      } catch (err) {
+        expect(err).toEqual(errObject);
+      }
+    });
+
+    it('should properly throw an unknown error', async () => {
+      datasourceRequestMock.mockImplementation(() => {
+        return Promise.resolve({
+          data: {
+            took: 1,
+            responses: [
+              {
+                error: {},
+                status: 400,
+              },
+            ],
+          },
+        });
+      });
+
+      const errObject = {
+        data: '{}',
+        message: 'Unknown elastic error response',
+      };
+
+      try {
+        await ctx.ds.query(query);
+      } catch (err) {
+        expect(err).toEqual(errObject);
+      }
+    });
+  });
+
   describe('When getting fields', () => {
     beforeEach(() => {
       createDatasource({
-        url: 'http://es.com',
+        url: ELASTICSEARCH_MOCK_URL,
         database: 'metricbeat',
         jsonData: { esVersion: 50 } as ElasticsearchOptions,
       } as DataSourceInstanceSettings<ElasticsearchOptions>);
@@ -411,7 +492,7 @@ describe('ElasticDatasource', function(this: any) {
     beforeEach(() => {
       createDatasourceWithTime(
         {
-          url: 'http://es.com',
+          url: ELASTICSEARCH_MOCK_URL,
           database: '[asd-]YYYY.MM.DD',
           jsonData: { interval: 'Daily', esVersion: 50 } as ElasticsearchOptions,
         } as DataSourceInstanceSettings<ElasticsearchOptions>,
@@ -429,9 +510,9 @@ describe('ElasticDatasource', function(this: any) {
         .format('YYYY.MM.DD');
 
       datasourceRequestMock.mockImplementation(options => {
-        if (options.url === `http://es.com/asd-${twoDaysBefore}/_mapping`) {
+        if (options.url === `${ELASTICSEARCH_MOCK_URL}/asd-${twoDaysBefore}/_mapping`) {
           return Promise.resolve(basicResponse);
-        } else if (options.url === `http://es.com/asd-${threeDaysBefore}/_mapping`) {
+        } else if (options.url === `${ELASTICSEARCH_MOCK_URL}/asd-${threeDaysBefore}/_mapping`) {
           return Promise.resolve(alternateResponse);
         }
         return Promise.reject({ status: 404 });
@@ -451,7 +532,7 @@ describe('ElasticDatasource', function(this: any) {
         .format('YYYY.MM.DD');
 
       datasourceRequestMock.mockImplementation(options => {
-        if (options.url === `http://es.com/asd-${twoDaysBefore}/_mapping`) {
+        if (options.url === `${ELASTICSEARCH_MOCK_URL}/asd-${twoDaysBefore}/_mapping`) {
           return Promise.resolve(basicResponse);
         }
         return Promise.reject({ status: 500 });
@@ -490,7 +571,7 @@ describe('ElasticDatasource', function(this: any) {
   describe('When getting fields from ES 7.0', () => {
     beforeEach(() => {
       createDatasource({
-        url: 'http://es.com',
+        url: ELASTICSEARCH_MOCK_URL,
         database: 'genuine.es7._mapping.response',
         jsonData: { esVersion: 70 } as ElasticsearchOptions,
       } as DataSourceInstanceSettings<ElasticsearchOptions>);
@@ -643,7 +724,7 @@ describe('ElasticDatasource', function(this: any) {
 
     beforeEach(() => {
       createDatasource({
-        url: 'http://es.com',
+        url: ELASTICSEARCH_MOCK_URL,
         database: 'test',
         jsonData: { esVersion: 5 } as ElasticsearchOptions,
       } as DataSourceInstanceSettings<ElasticsearchOptions>);
@@ -686,7 +767,7 @@ describe('ElasticDatasource', function(this: any) {
 
     beforeEach(() => {
       createDatasource({
-        url: 'http://es.com',
+        url: ELASTICSEARCH_MOCK_URL,
         database: 'test',
         jsonData: { esVersion: 5 } as ElasticsearchOptions,
       } as DataSourceInstanceSettings<ElasticsearchOptions>);
@@ -750,7 +831,7 @@ describe('ElasticDatasource', function(this: any) {
     it('should replace range as integer not string', () => {
       const dataSource = new ElasticDatasource(
         {
-          url: 'http://es.com',
+          url: ELASTICSEARCH_MOCK_URL,
           database: '[asd-]YYYY.MM.DD',
           jsonData: {
             interval: 'Daily',

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

@@ -86,7 +86,17 @@ export class ElasticDatasource extends DataSourceApi<ElasticsearchQuery, Elastic
       };
     }
 
-    return getBackendSrv().datasourceRequest(options);
+    return getBackendSrv()
+      .datasourceRequest(options)
+      .catch((err: any) => {
+        if (err.data && err.data.error) {
+          throw {
+            message: 'Elasticsearch error: ' + err.data.error.reason,
+            error: err.data.error,
+          };
+        }
+        throw err;
+      });
   }
 
   /**
@@ -128,21 +138,10 @@ export class ElasticDatasource extends DataSourceApi<ElasticsearchQuery, Elastic
   }
 
   private post(url: string, data: any) {
-    return this.request('POST', url, data)
+    return this.request('POST', url, data).then((results: any) => {
-      .then((results: any) => {
+      results.data.$$config = results.config;
-        results.data.$$config = results.config;
+      return results.data;
-        return results.data;
+    });
-      })
-      .catch((err: any) => {
-        if (err.data && err.data.error) {
-          throw {
-            message: 'Elasticsearch error: ' + err.data.error.reason,
-            error: err.data.error,
-          };
-        }
-
-        throw err;
-      });
   }
 
   annotationQuery(options: any): Promise<any> {

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

@@ -368,7 +368,7 @@ export class ElasticResponse {
     if (err.root_cause && err.root_cause.length > 0 && err.root_cause[0].reason) {
       result.message = err.root_cause[0].reason;
     } else {
-      result.message = err.reason || 'Unkown elastic error response';
+      result.message = err.reason || 'Unknown elastic error response';
     }
 
     if (response.$$config) {