Merge remote-tracking branch 'upstream/master' into lens/error-telemetry

src/core/server/saved_objects/service/lib/aggregations/aggs_types/bucket_aggs.ts

@@ -49,7 +49,7 @@ export const bucketAggsSchemas: Record<string, ObjectType> = {
   histogram: s.object({
     field: s.maybe(s.string()),
     interval: s.maybe(s.number()),
-    min_doc_count: s.maybe(s.number()),
+    min_doc_count: s.maybe(s.number({ min: 1 })),
     extended_bounds: s.maybe(
       s.object({
         min: s.number(),
@@ -78,7 +78,7 @@ export const bucketAggsSchemas: Record<string, ObjectType> = {
     include: s.maybe(s.oneOf([s.string(), s.arrayOf(s.string())])),
     execution_hint: s.maybe(s.string()),
     missing: s.maybe(s.number()),
-    min_doc_count: s.maybe(s.number()),
+    min_doc_count: s.maybe(s.number({ min: 1 })),
     size: s.maybe(s.number()),
     show_term_doc_count_error: s.maybe(s.boolean()),
     order: s.maybe(s.oneOf([s.literal('asc'), s.literal('desc')])),

src/core/server/saved_objects/service/lib/aggregations/aggs_types/schemas.test.ts

@@ -0,0 +1,43 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License
+ * 2.0 and the Server Side Public License, v 1; you may not use this file except
+ * in compliance with, at your election, the Elastic License 2.0 or the Server
+ * Side Public License, v 1.
+ */
+
+import { bucketAggsSchemas } from './bucket_aggs';
+
+describe('bucket aggregation schemas', () => {
+  describe('terms aggregation schema', () => {
+    const schema = bucketAggsSchemas.terms;
+
+    it('passes validation when using `1` for `min_doc_count`', () => {
+      expect(() => schema.validate({ min_doc_count: 1 })).not.toThrow();
+    });
+
+    // see https://www.elastic.co/guide/en/elasticsearch/reference/current/search-aggregations-bucket-terms-aggregation.html#_minimum_document_count_4
+    // Setting min_doc_count=0 will also return buckets for terms that didn’t match any hit,
+    // bypassing any filtering perform via `filter` or `query`
+    // causing a potential security issue as we can return values from other spaces.
+    it('throws an error when using `0` for `min_doc_count`', () => {
+      expect(() => schema.validate({ min_doc_count: 0 })).toThrowErrorMatchingInlineSnapshot(
+        `"[min_doc_count]: Value must be equal to or greater than [1]."`
+      );
+    });
+  });
+
+  describe('histogram aggregation schema', () => {
+    const schema = bucketAggsSchemas.histogram;
+
+    it('passes validation when using `1` for `min_doc_count`', () => {
+      expect(() => schema.validate({ min_doc_count: 1 })).not.toThrow();
+    });
+
+    it('throws an error when using `0` for `min_doc_count`', () => {
+      expect(() => schema.validate({ min_doc_count: 0 })).toThrowErrorMatchingInlineSnapshot(
+        `"[min_doc_count]: Value must be equal to or greater than [1]."`
+      );
+    });
+  });
+});

x-pack/plugins/enterprise_search/public/applications/shared/schema/schema_errors_accordion.scss

@@ -0,0 +1,20 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License
+ * 2.0; you may not use this file except in compliance with the Elastic License
+ * 2.0.
+ */
+
+.schemaFieldError {
+  border-top: 1px solid $euiColorLightShade;
+
+  &:last-child {
+    border-bottom: 1px solid $euiColorLightShade;
+  }
+
+  // Something about the EuiFlexGroup being inside a button collapses the row of items.
+  // This wrapper div was injected by EUI and had 'with: auto' on it.
+  .euiIEFlexWrapFix {
+    width: 100%;
+  }
+}

x-pack/plugins/enterprise_search/public/applications/shared/schema/schema_errors_accordion.test.tsx

@@ -11,7 +11,7 @@ import { shallow } from 'enzyme';
 
 import { EuiAccordion, EuiTableRow } from '@elastic/eui';
 
-import { EuiLinkTo } from '../react_router_helpers';
+import { EuiButtonEmptyTo } from '../react_router_helpers';
 
 import { SchemaErrorsAccordion } from './schema_errors_accordion';
 
@@ -40,12 +40,12 @@ describe('SchemaErrorsAccordion', () => {
 
     expect(wrapper.find(EuiAccordion)).toHaveLength(1);
     expect(wrapper.find(EuiTableRow)).toHaveLength(2);
-    expect(wrapper.find(EuiLinkTo)).toHaveLength(0);
+    expect(wrapper.find(EuiButtonEmptyTo)).toHaveLength(0);
   });
 
   it('renders document buttons', () => {
     const wrapper = shallow(<SchemaErrorsAccordion {...props} itemId="123" getRoute={jest.fn()} />);
 
-    expect(wrapper.find(EuiLinkTo)).toHaveLength(2);
+    expect(wrapper.find(EuiButtonEmptyTo)).toHaveLength(2);
   });
 });

x-pack/plugins/enterprise_search/public/applications/shared/schema/schema_errors_accordion.tsx

@@ -9,6 +9,7 @@ import React from 'react';
 
 import {
   EuiAccordion,
+  EuiButton,
   EuiFlexGroup,
   EuiFlexItem,
   EuiTable,
@@ -19,10 +20,12 @@ import {
   EuiTableRowCell,
 } from '@elastic/eui';
 
-import { EuiLinkTo } from '../react_router_helpers';
+import { EuiButtonEmptyTo } from '../react_router_helpers';
 
 import { TruncatedContent } from '../truncate';
 
+import './schema_errors_accordion.scss';
+
 import {
   ERROR_TABLE_ID_HEADER,
   ERROR_TABLE_ERROR_HEADER,
@@ -60,14 +63,19 @@ export const SchemaErrorsAccordion: React.FC<ISchemaErrorsAccordionProps> = ({
         <EuiFlexGroup alignItems="center" justifyContent="spaceBetween" gutterSize="none">
           <EuiFlexItem grow={false}>
             <EuiFlexGroup alignItems="center" gutterSize="xl">
-              <EuiFlexItem className="field-error__field-name">
-                <TruncatedContent content={fieldName} length={32} />
+              <EuiFlexItem>
+                <strong>
+                  <TruncatedContent content={fieldName} length={32} />
+                </strong>
               </EuiFlexItem>
-              <EuiFlexItem className="field-error__field-type">{schema[fieldName]}</EuiFlexItem>
+              <EuiFlexItem>{schema[fieldName]}</EuiFlexItem>
             </EuiFlexGroup>
           </EuiFlexItem>
           <EuiFlexItem grow={false}>
-            <span className="field-error__control button">{ERROR_TABLE_REVIEW_CONTROL}</span>
+            {/* href is needed here because a button cannot be nested in a button or console will error and EuiAccordion uses a button to wrap this. */}
+            <EuiButton size="s" href="#">
+              {ERROR_TABLE_REVIEW_CONTROL}
+            </EuiButton>
           </EuiFlexItem>
         </EuiFlexGroup>
       );
@@ -76,12 +84,12 @@ export const SchemaErrorsAccordion: React.FC<ISchemaErrorsAccordionProps> = ({
         <EuiAccordion
           key={fieldNameIndex}
           id={`accordion${fieldNameIndex}`}
-          className="euiAccordionForm field-error"
+          className="schemaFieldError"
           buttonClassName="euiAccordionForm__button field-error__header"
           buttonContent={accordionHeader}
           paddingSize="xl"
         >
-          <EuiTable>
+          <EuiTable tableLayout="auto">
             <EuiTableHeader>
               <EuiTableHeaderCell>{ERROR_TABLE_ID_HEADER}</EuiTableHeaderCell>
               <EuiTableHeaderCell>{ERROR_TABLE_ERROR_HEADER}</EuiTableHeaderCell>
@@ -93,34 +101,21 @@ export const SchemaErrorsAccordion: React.FC<ISchemaErrorsAccordionProps> = ({
                 const documentPath = getRoute && itemId ? getRoute(itemId, error.external_id) : '';
 
                 const viewButton = showViewButton && (
-                  <EuiTableRowCell className="field-error-document__actions">
-                    <EuiLinkTo
-                      className="euiButtonEmpty euiButtonEmpty--primary euiButtonEmpty--xSmall"
-                      to={documentPath}
-                    >
-                      <span className="euiButtonEmpty__content">
-                        <span className="euiButtonEmpty__text">{ERROR_TABLE_VIEW_LINK}</span>
-                      </span>
-                    </EuiLinkTo>
+                  <EuiTableRowCell>
+                    <EuiButtonEmptyTo to={documentPath}>{ERROR_TABLE_VIEW_LINK}</EuiButtonEmptyTo>
                   </EuiTableRowCell>
                 );
 
                 return (
-                  <EuiTableRow
-                    key={`schema-change-document-error-${fieldName}-${errorIndex} field-error-document`}
-                  >
-                    <EuiTableRowCell className="field-error-document__id">
-                      <div className="data-type--id">
-                        <TruncatedContent
-                          tooltipType="title"
-                          content={error.external_id}
-                          length={22}
-                        />
-                      </div>
-                    </EuiTableRowCell>
-                    <EuiTableRowCell className="field-error-document__field-content">
-                      {error.error}
+                  <EuiTableRow key={`schema-change-document-error-${fieldName}-${errorIndex}`}>
+                    <EuiTableRowCell truncateText>
+                      <TruncatedContent
+                        tooltipType="title"
+                        content={error.external_id}
+                        length={22}
+                      />
                     </EuiTableRowCell>
+                    <EuiTableRowCell>{error.error}</EuiTableRowCell>
                     {showViewButton ? viewButton : <EuiTableRowCell />}
                   </EuiTableRow>
                 );

x-pack/plugins/enterprise_search/public/applications/workplace_search/views/content_sources/components/schema/schema_change_errors.tsx

@@ -10,8 +10,6 @@ import { useParams } from 'react-router-dom';
 
 import { useActions, useValues } from 'kea';
 
-import { EuiSpacer } from '@elastic/eui';
-
 import { SchemaErrorsAccordion } from '../../../../../shared/schema/schema_errors_accordion';
 import { ViewContentHeader } from '../../../../components/shared/view_content_header';
 
@@ -32,16 +30,13 @@ export const SchemaChangeErrors: React.FC = () => {
   }, []);
 
   return (
-    <div>
+    <>
       <ViewContentHeader title={SCHEMA_ERRORS_HEADING} />
-      <EuiSpacer size="xl" />
-      <main>
-        <SchemaErrorsAccordion
-          fieldCoercionErrors={fieldCoercionErrors}
-          schema={serverSchema}
-          itemId={sourceId}
-        />
-      </main>
-    </div>
+      <SchemaErrorsAccordion
+        fieldCoercionErrors={fieldCoercionErrors}
+        schema={serverSchema}
+        itemId={sourceId}
+      />
+    </>
   );
 };

x-pack/plugins/enterprise_search/public/applications/workplace_search/views/content_sources/source_logic.test.ts

@@ -33,6 +33,7 @@ describe('SourceLogic', () => {
     flashAPIErrors,
     setSuccessMessage,
     setQueuedSuccessMessage,
+    setErrorMessage,
   } = mockFlashMessageHelpers;
   const { navigateToUrl } = mockKibanaValues;
   const { mount, getListeners } = new LogicMounter(SourceLogic);
@@ -204,6 +205,19 @@ describe('SourceLogic', () => {
 
         expect(navigateToUrl).toHaveBeenCalledWith(NOT_FOUND_PATH);
       });
+
+      it('renders error messages passed in success response from server', async () => {
+        const errors = ['ERROR'];
+        const promise = Promise.resolve({
+          ...contentSource,
+          errors,
+        });
+        http.get.mockReturnValue(promise);
+        SourceLogic.actions.initializeSource(contentSource.id);
+        await promise;
+
+        expect(setErrorMessage).toHaveBeenCalledWith(errors);
+      });
     });
 
     describe('initializeFederatedSummary', () => {

x-pack/plugins/enterprise_search/public/applications/workplace_search/views/content_sources/source_logic.ts

@@ -13,6 +13,7 @@ import { DEFAULT_META } from '../../../shared/constants';
 import {
   flashAPIErrors,
   setSuccessMessage,
+  setErrorMessage,
   setQueuedSuccessMessage,
   clearFlashMessages,
 } from '../../../shared/flash_messages';
@@ -148,6 +149,11 @@ export const SourceLogic = kea<MakeLogicType<SourceValues, SourceActions>>({
         if (response.isFederatedSource) {
           actions.initializeFederatedSummary(sourceId);
         }
+        if (response.errors) {
+          setErrorMessage(response.errors);
+        } else {
+          clearFlashMessages();
+        }
       } catch (e) {
         if (e.response.status === 404) {
           KibanaLogic.values.navigateToUrl(NOT_FOUND_PATH);

x-pack/plugins/enterprise_search/public/applications/workplace_search/views/content_sources/source_router.test.tsx

@@ -8,6 +8,8 @@
 import '../../../__mocks__/shallow_useeffect.mock';
 
 import { setMockValues, setMockActions } from '../../../__mocks__';
+import { mockLocation } from '../../../__mocks__/react_router_history.mock';
+import { unmountHandler } from '../../../__mocks__/shallow_useeffect.mock';
 import { contentSources } from '../../__mocks__/content_sources.mock';
 
 import React from 'react';
@@ -30,6 +32,7 @@ import { SourceRouter } from './source_router';
 
 describe('SourceRouter', () => {
   const initializeSource = jest.fn();
+  const resetSourceState = jest.fn();
   const contentSource = contentSources[1];
   const customSource = contentSources[0];
   const mockValues = {
@@ -40,10 +43,11 @@ describe('SourceRouter', () => {
   beforeEach(() => {
     setMockActions({
       initializeSource,
+      resetSourceState,
     });
     setMockValues({ ...mockValues });
     (useParams as jest.Mock).mockImplementationOnce(() => ({
-      sourceId: '1',
+      sourceId: contentSource.id,
     }));
   });
 
@@ -114,4 +118,22 @@ describe('SourceRouter', () => {
       NAV.DISPLAY_SETTINGS,
     ]);
   });
+
+  describe('reset state', () => {
+    it('does not reset state when switching between source tree views', () => {
+      mockLocation.pathname = `/sources/${contentSource.id}`;
+      shallow(<SourceRouter />);
+      unmountHandler();
+
+      expect(resetSourceState).not.toHaveBeenCalled();
+    });
+
+    it('resets state when leaving source tree', () => {
+      mockLocation.pathname = '/home';
+      shallow(<SourceRouter />);
+      unmountHandler();
+
+      expect(resetSourceState).toHaveBeenCalled();
+    });
+  });
 });

x-pack/plugins/enterprise_search/public/applications/workplace_search/views/content_sources/source_router.tsx

@@ -6,7 +6,8 @@
  */
 
 import React, { useEffect } from 'react';
-import { Route, Switch, useParams } from 'react-router-dom';
+
+import { Route, Switch, useLocation, useParams } from 'react-router-dom';
 
 import { useActions, useValues } from 'kea';
 import moment from 'moment';
@@ -47,14 +48,18 @@ import { SourceLogic } from './source_logic';
 
 export const SourceRouter: React.FC = () => {
   const { sourceId } = useParams() as { sourceId: string };
+  const { pathname } = useLocation();
   const { initializeSource, resetSourceState } = useActions(SourceLogic);
   const { contentSource, dataLoading } = useValues(SourceLogic);
   const { isOrganization } = useValues(AppLogic);
 
   useEffect(() => {
     initializeSource(sourceId);
-    return resetSourceState;
-  }, []);
+    return () => {
+      // We only want to reset the state when leaving the source section. Otherwise there is an unwanted flash of UI.
+      if (!pathname.includes(sourceId)) resetSourceState();
+    };
+  }, [pathname]);
 
   if (dataLoading) return <Loading />;