Explore: Fix issue when some query errors were not shown (#32212)

* Remove getFirstNonQueryRowSpecificError

* Move some error handling to separate component

* Update public/app/features/explore/ErrorContainer.tsx

Co-authored-by: Giordano Ricci <gio.ricci@grafana.com>

* Add comments

* More explicit expects

* Update tests with proper expects

Co-authored-by: Giordano Ricci <gio.ricci@grafana.com>

public/app/core/utils/explore.test.ts

@@ -2,7 +2,6 @@ import {
   buildQueryTransaction,
   clearHistory,
   DEFAULT_RANGE,
-  getFirstQueryErrorWithoutRefId,
   getRefIds,
   getValueWithRefId,
   hasNonEmptyQuery,
@@ -14,7 +13,7 @@ import {
   getTimeRangeFromUrl,
 } from './explore';
 import store from 'app/core/store';
-import { DataQueryError, dateTime, ExploreUrlState, LogsSortOrder } from '@grafana/data';
+import { dateTime, ExploreUrlState, LogsSortOrder } from '@grafana/data';
 import { RefreshPicker } from '@grafana/ui';
 import { serializeStateToUrlParam } from '@grafana/data/src/utils/url';
 
@@ -323,40 +322,6 @@ describe('getTimeRangeFromUrl', () => {
   });
 });
 
-describe('getFirstQueryErrorWithoutRefId', () => {
-  describe('when called with a null value', () => {
-    it('then it should return undefined', () => {
-      const errors: DataQueryError[] | undefined = undefined;
-      const result = getFirstQueryErrorWithoutRefId(errors);
-
-      expect(result).toBeUndefined();
-    });
-  });
-
-  describe('when called with an array with only refIds', () => {
-    it('then it should return undefined', () => {
-      const errors: DataQueryError[] = [{ refId: 'A' }, { refId: 'B' }];
-      const result = getFirstQueryErrorWithoutRefId(errors);
-
-      expect(result).toBeUndefined();
-    });
-  });
-
-  describe('when called with an array with and without refIds', () => {
-    it('then it should return undefined', () => {
-      const errors: DataQueryError[] = [
-        { refId: 'A' },
-        { message: 'A message' },
-        { refId: 'B' },
-        { message: 'B message' },
-      ];
-      const result = getFirstQueryErrorWithoutRefId(errors);
-
-      expect(result).toBe(errors[1]);
-    });
-  });
-});
-
 describe('getRefIds', () => {
   describe('when called with a null value', () => {
     it('then it should return empty array', () => {

public/app/core/utils/explore.ts

@@ -5,7 +5,6 @@ import { Unsubscribable } from 'rxjs';
 import {
   CoreApp,
   DataQuery,
-  DataQueryError,
   DataQueryRequest,
   DataSourceApi,
   dateMath,
@@ -422,14 +421,6 @@ export const getValueWithRefId = (value?: any): any => {
   return undefined;
 };
 
-export const getFirstQueryErrorWithoutRefId = (errors?: DataQueryError[]): DataQueryError | undefined => {
-  if (!errors) {
-    return undefined;
-  }
-
-  return errors.filter((error) => (error && error.refId ? false : true))[0];
-};
-
 export const getRefIds = (value: any): string[] => {
   if (!value) {
     return [];
@@ -479,11 +470,6 @@ export function getIntervals(range: TimeRange, lowLimit?: string, resolution?: n
   return rangeUtil.calculateInterval(range, resolution, lowLimit);
 }
 
-export const getFirstNonQueryRowSpecificError = (queryErrors?: DataQueryError[]): DataQueryError | undefined => {
-  const refId = getValueWithRefId(queryErrors);
-  return refId ? undefined : getFirstQueryErrorWithoutRefId(queryErrors);
-};
-
 export const copyStringToClipboard = (string: string) => {
   const el = document.createElement('textarea');
   el.value = string;

public/app/features/explore/ErrorContainer.tsx

@@ -11,7 +11,7 @@ export const ErrorContainer: FunctionComponent<ErrorContainerProps> = (props) =>
   const { queryError } = props;
   const showError = queryError ? true : false;
   const duration = showError ? 100 : 10;
-  const message = queryError ? queryError.message : null;
+  const message = queryError?.message || queryError?.data?.message || 'Unknown error';
 
   return (
     <FadeIn in={showError} duration={duration}>

public/app/features/explore/Explore.test.tsx

@@ -1,6 +1,5 @@
 import React from 'react';
 import { DataSourceApi, LoadingState, toUtc, DataQueryError, DataQueryRequest, CoreApp } from '@grafana/data';
-import { getFirstNonQueryRowSpecificError } from 'app/core/utils/explore';
 import { ExploreId } from 'app/types/explore';
 import { shallow } from 'enzyme';
 import { Explore, ExploreProps } from './Explore';
@@ -85,17 +84,6 @@ const dummyProps: ExploreProps = {
   splitOpen: (() => {}) as any,
 };
 
-const setupErrors = (hasRefId?: boolean) => {
-  return [
-    {
-      message: 'Error message',
-      status: '400',
-      statusText: 'Bad Request',
-      refId: hasRefId ? 'A' : '',
-    },
-  ];
-};
-
 describe('Explore', () => {
   it('should render component', () => {
     const wrapper = shallow(<Explore {...dummyProps} />);
@@ -107,21 +95,4 @@ describe('Explore', () => {
     expect(wrapper.find(SecondaryActions)).toHaveLength(1);
     expect(wrapper.find(SecondaryActions).props().addQueryRowButtonHidden).toBe(false);
   });
-
-  it('should filter out a query-row-specific error when looking for non-query-row-specific errors', async () => {
-    const queryErrors = setupErrors(true);
-    const queryError = getFirstNonQueryRowSpecificError(queryErrors);
-    expect(queryError).toBeUndefined();
-  });
-
-  it('should not filter out a generic error when looking for non-query-row-specific errors', async () => {
-    const queryErrors = setupErrors();
-    const queryError = getFirstNonQueryRowSpecificError(queryErrors);
-    expect(queryError).toEqual({
-      message: 'Error message',
-      status: '400',
-      statusText: 'Bad Request',
-      refId: '',
-    });
-  });
 });

public/app/features/explore/Explore.tsx

@@ -31,17 +31,15 @@ import { updateTimeRange } from './state/time';
 import { scanStopAction, addQueryRow, modifyQueries, setQueries, scanStart } from './state/query';
 import { ExploreId, ExploreItemState } from 'app/types/explore';
 import { StoreState } from 'app/types';
-import { getFirstNonQueryRowSpecificError } from 'app/core/utils/explore';
 import { ExploreToolbar } from './ExploreToolbar';
 import { NoDataSourceCallToAction } from './NoDataSourceCallToAction';
 import { getTimeZone } from '../profile/state/selectors';
-import { ErrorContainer } from './ErrorContainer';
-//TODO:unification
 import { TraceView } from './TraceView/TraceView';
 import { SecondaryActions } from './SecondaryActions';
 import { FILTER_FOR_OPERATOR, FILTER_OUT_OPERATOR, FilterItem } from '@grafana/ui/src/components/Table/types';
 import { ExploreGraphNGPanel } from './ExploreGraphNGPanel';
 import { NodeGraphContainer } from './NodeGraphContainer';
+import { ResponseErrorContainer } from './ResponseErrorContainer';
 
 const getStyles = stylesFactory((theme: GrafanaTheme) => {
   return {
@@ -315,12 +313,6 @@ export class Explore extends React.PureComponent<ExploreProps, ExploreState> {
     const { openDrawer } = this.state;
     const styles = getStyles(theme);
     const showPanels = queryResponse && queryResponse.state !== LoadingState.NotStarted;
-
-    // gets an error without a refID, so non-query-row-related error, like a connection error
-    const queryErrors =
-      queryResponse.state === LoadingState.Error && queryResponse.error ? [queryResponse.error] : undefined;
-    const queryError = getFirstNonQueryRowSpecificError(queryErrors);
-
     const showRichHistory = openDrawer === ExploreDrawer.RichHistory;
     const showQueryInspector = openDrawer === ExploreDrawer.QueryInspector;
 
@@ -344,7 +336,7 @@ export class Explore extends React.PureComponent<ExploreProps, ExploreState> {
                 onClickQueryInspectorButton={this.toggleShowQueryInspector}
               />
             </div>
-            <ErrorContainer queryError={queryError} />
+            <ResponseErrorContainer exploreId={exploreId} />
             <AutoSizer onResize={this.onResize} disableHeight>
               {({ width }) => {
                 if (width === 0) {

public/app/features/explore/QueryRow.tsx

@@ -177,6 +177,8 @@ export class QueryRow extends PureComponent<QueryRowProps, QueryRowState> {
 
     const canToggleEditorModes = has(datasourceInstance, 'components.QueryCtrl.prototype.toggleEditorMode');
     const isNotStarted = queryResponse.state === LoadingState.NotStarted;
+
+    // We show error without refId in ResponseErrorContainer so this condition needs to match se we don't loose errors.
     const queryErrors = queryResponse.error && queryResponse.error.refId === query.refId ? [queryResponse.error] : [];
 
     return (

public/app/features/explore/ResponseErrorContainer.test.tsx

@@ -0,0 +1,56 @@
+import React from 'react';
+import { configureStore } from '../../store/configureStore';
+import { ResponseErrorContainer } from './ResponseErrorContainer';
+import { Provider } from 'react-redux';
+import { render, screen } from '@testing-library/react';
+import { ExploreId } from '../../types';
+import { DataQueryError, LoadingState } from '@grafana/data';
+
+describe('ResponseErrorContainer', () => {
+  it('shows error message if it does not contain refId', async () => {
+    setup({
+      message: 'test error',
+    });
+    expect(screen.getByText('test error')).toBeInTheDocument();
+  });
+
+  it('shows error.data.message if error.message does not exist', async () => {
+    setup({
+      data: {
+        message: 'test error',
+      },
+    });
+    expect(screen.getByText('test error')).toBeInTheDocument();
+  });
+
+  it('does not show error if there is refID', async () => {
+    setup({
+      refId: 'someId',
+      message: 'test error',
+    });
+    expect(screen.queryByText('test error')).not.toBeInTheDocument();
+  });
+
+  it('does not show error if there is refID', async () => {
+    setup({
+      refId: 'someId',
+      message: 'test error',
+    });
+    expect(screen.queryByText('test error')).not.toBeInTheDocument();
+  });
+});
+
+function setup(error: DataQueryError) {
+  const store = configureStore();
+  store.getState().explore[ExploreId.left].queryResponse = {
+    timeRange: {} as any,
+    series: [],
+    state: LoadingState.Error,
+    error,
+  };
+  render(
+    <Provider store={store}>
+      <ResponseErrorContainer exploreId={ExploreId.left} />
+    </Provider>
+  );
+}

public/app/features/explore/ResponseErrorContainer.tsx

@@ -0,0 +1,21 @@
+import React from 'react';
+import { useSelector } from 'react-redux';
+import { ExploreId, StoreState } from '../../types';
+import { LoadingState } from '@grafana/data';
+import { ErrorContainer } from './ErrorContainer';
+
+interface Props {
+  exploreId: ExploreId;
+}
+export function ResponseErrorContainer(props: Props) {
+  const queryResponse = useSelector((state: StoreState) => state.explore[props.exploreId]?.queryResponse);
+
+  // Only show error if it does not have refId. Otherwise let query row to handle it so this condition has to be matched
+  // with QueryRow.tsx so we don't loose errors.
+  const queryError =
+    queryResponse?.state === LoadingState.Error && queryResponse?.error && !queryResponse.error.refId
+      ? queryResponse.error
+      : undefined;
+
+  return <ErrorContainer queryError={queryError} />;
+}

public/app/features/explore/__snapshots__/Explore.test.tsx.snap

@@ -26,7 +26,9 @@ exports[`Explore should render component 1`] = `
         richHistoryButtonActive={false}
       />
     </div>
-    <ErrorContainer />
+    <ResponseErrorContainer
+      exploreId="left"
+    />
     <AutoSizer
       disableHeight={true}
       disableWidth={false}