Address feedback.

elastic · Feb 15, 2019 · 1c73449 · 1c73449
1 parent d29bcf4
commit 1c73449
Show file tree

Hide file tree

Showing 3 changed files with 78 additions and 60 deletions.
diff --git a/src/legacy/ui/public/visualize/loader/embedded_visualize_handler.ts b/src/legacy/ui/public/visualize/loader/embedded_visualize_handler.ts
@@ -46,6 +46,9 @@ import {
 } from './types';
 import { queryGeohashBounds } from './utils';
 
+import { i18n } from '@kbn/i18n';
+import { toastNotifications } from 'ui/notify';
+
 interface EmbeddedVisualizeHandlerParams extends VisualizeLoaderParams {
   Private: IPrivate;
   queryFilter: any;
@@ -428,12 +431,47 @@ export class EmbeddedVisualizeHandler {
     this.dataLoaderParams.inspectorAdapters = this.inspectorAdapters;
 
     this.vis.filters = { timeRange: this.dataLoaderParams.timeRange };
+    this.vis.requestError = undefined;
+    this.vis.showRequestError = false;
+
+    return this.dataLoader
+      .fetch(this.dataLoaderParams)
+      .then(data => {
+        // Pipeline responses never throw errors, so we need to check for
+        // `type: 'error'`, and then throw so it can be caught below.
+        // TODO: We should revisit this after we have fully migrated
+        // to the new expression pipeline infrastructure.
+        if (data && data.type === 'error') {
+          throw data.error;
+        }
 
-    return this.dataLoader.fetch(this.dataLoaderParams).then(data => {
-      if (data && data.value) {
-        this.dataSubject.next(data.value);
-      }
-      return data;
+        if (data && data.value) {
+          this.dataSubject.next(data.value);
+        }
+        return data;
+      })
+      .catch(this.handleDataLoaderError);
+  };
+
+  /**
+   * When dataLoader returns an error, we need to make sure it surfaces in the UI.
+   *
+   * TODO: Eventually we should add some custom error messages for issues that are
+   * frequently encountered by users.
+   */
+  private handleDataLoaderError = (error: any): void => {
+    // TODO: come up with a general way to cancel execution of pipeline expressions.
+    this.dataLoaderParams.searchSource.cancelQueued();
+
+    this.vis.requestError = error;
+    this.vis.showRequestError =
+      error.type && ['NO_OP_SEARCH_STRATEGY', 'UNSUPPORTED_QUERY'].includes(error.type);
+
+    toastNotifications.addDanger({
+      title: i18n.translate('kbn.visualize.loader.dataLoaderError', {
+        defaultMessage: 'Error in visualization',
+      }),
+      text: error.message,
     });
   };
 

diff --git a/src/legacy/ui/public/visualize/loader/pipeline_data_loader.ts b/src/legacy/ui/public/visualize/loader/pipeline_data_loader.ts
@@ -18,18 +18,15 @@
  */
 
 import { RequestHandlerParams, Vis } from '../../vis';
-import { handleLoaderError } from './errors';
 import { buildPipeline, runPipeline } from './pipeline_helpers';
 
 export class PipelineDataLoader {
   constructor(private readonly vis: Vis) {}
 
   public async fetch(params: RequestHandlerParams): Promise<any> {
-    this.vis.requestError = undefined;
-    this.vis.showRequestError = false;
     this.vis.pipelineExpression = buildPipeline(this.vis, params);
 
-    const pipelineResponse = await runPipeline(
+    return await runPipeline(
       this.vis.pipelineExpression,
       {},
       {
@@ -43,11 +40,5 @@ export class PipelineDataLoader {
         inspectorAdapters: params.inspectorAdapters,
       }
     );
-
-    if (pipelineResponse.type === 'error') {
-      return handleLoaderError(params, this.vis, pipelineResponse.error);
-    }
-
-    return pipelineResponse;
   }
 }
diff --git a/src/legacy/ui/public/visualize/loader/visualize_data_loader.ts b/src/legacy/ui/public/visualize/loader/visualize_data_loader.ts
@@ -34,7 +34,6 @@ import {
 import { VisResponseData } from './types';
 
 import { decorateVisObject } from 'ui/visualize/loader/pipeline_helpers/build_pipeline';
-import { handleLoaderError } from './errors';
 
 function getHandler<T extends RequestHandler | ResponseHandler>(
   from: Array<{ name: string; handler: T }>,
@@ -68,53 +67,43 @@ export class VisualizeDataLoader {
   }
 
   public async fetch(params: RequestHandlerParams): Promise<VisResponseData | void> {
-    this.vis.filters = { timeRange: params.timeRange };
-    this.vis.requestError = undefined;
-    this.vis.showRequestError = false;
-
     // add necessary params to vis object (dimensions, bucket, metric, etc)
     decorateVisObject(this.vis, { timeRange: params.timeRange });
 
-    try {
-      // searchSource is only there for courier request handler
-      const requestHandlerResponse = await this.requestHandler({
-        partialRows: this.vis.params.partialRows || this.vis.type.requiresPartialRows,
-        isHierarchical: this.vis.isHierarchical(),
-        visParams: this.vis.params,
-        ...params,
-        filters: params.filters
-          ? params.filters.filter(filter => !filter.meta.disabled)
-          : undefined,
-      });
-
-      // No need to call the response handler when there have been no data nor has been there changes
-      // in the vis-state (response handler does not depend on uiStat
-      const canSkipResponseHandler =
-        this.previousRequestHandlerResponse &&
-        this.previousRequestHandlerResponse === requestHandlerResponse &&
-        this.previousVisState &&
-        isEqual(this.previousVisState, this.vis.getState());
-
-      this.previousVisState = this.vis.getState();
-      this.previousRequestHandlerResponse = requestHandlerResponse;
-
-      if (!canSkipResponseHandler) {
-        this.visData = await Promise.resolve(
-          this.responseHandler(requestHandlerResponse, this.vis.params.dimensions)
-        );
-      }
-
-      return {
-        as: 'visualization',
-        value: {
-          visType: this.vis.type.name,
-          visData: this.visData,
-          visConfig: this.vis.params,
-          params: {},
-        },
-      };
-    } catch (error) {
-      return handleLoaderError(params, this.vis, error);
+    // searchSource is only there for courier request handler
+    const requestHandlerResponse = await this.requestHandler({
+      partialRows: this.vis.params.partialRows || this.vis.type.requiresPartialRows,
+      isHierarchical: this.vis.isHierarchical(),
+      visParams: this.vis.params,
+      ...params,
+      filters: params.filters ? params.filters.filter(filter => !filter.meta.disabled) : undefined,
+    });
+
+    // No need to call the response handler when there have been no data nor has been there changes
+    // in the vis-state (response handler does not depend on uiStat
+    const canSkipResponseHandler =
+      this.previousRequestHandlerResponse &&
+      this.previousRequestHandlerResponse === requestHandlerResponse &&
+      this.previousVisState &&
+      isEqual(this.previousVisState, this.vis.getState());
+
+    this.previousVisState = this.vis.getState();
+    this.previousRequestHandlerResponse = requestHandlerResponse;
+
+    if (!canSkipResponseHandler) {
+      this.visData = await Promise.resolve(
+        this.responseHandler(requestHandlerResponse, this.vis.params.dimensions)
+      );
     }
+
+    return {
+      as: 'visualization',
+      value: {
+        visType: this.vis.type.name,
+        visData: this.visData,
+        visConfig: this.vis.params,
+        params: {},
+      },
+    };
   }
 }