Address larger tech debt/TODOs (elastic#8)

* Fix optional chaining TODO

- turns out my local Prettier wasn't up to date, completely my bad

* Fix constants TODO

- adds a common folder/architecture for others to use in the future

* Remove TODO for eslint-disable-line and specify lint rule being skipped

- hopefully that's OK for review, I can't think of any other way to sanely do this without re-architecting the entire file or DDoSing our API

* Add server-side logging to route dependencies

+ add basic example of error catching/logging to Telemetry route
+ [extra] refactor mockResponseFactory name to something slightly easier to read

* Move more Engines Overview API logic/logging to server-side

- handle data validation in the server-side
- wrap server-side API in a try/catch to account for fetch issues
- more correctly return 2xx/4xx statuses and more correctly deal with those responses in the front-end
- Add server info/error/debug logs (addresses TODO)
- Update tests + minor refactors/cleanup
    - remove expectResponseToBe200With helper (since we're now returning multiple response types) and instead make mockResponse var name more readable
    - one-line header auth
    - update tests with example error logs
    - update schema validation for `type` to be an enum of `indexed`/`meta` (more accurately reflecting API)

x-pack/plugins/enterprise_search/common/constants.ts

@@ -0,0 +1,7 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License;
+ * you may not use this file except in compliance with the Elastic License.
+ */
+
+export const ENGINES_PAGE_SIZE = 10;

x-pack/plugins/enterprise_search/public/applications/app_search/components/engine_overview/engine_overview.test.tsx

@@ -52,7 +52,7 @@ describe('EngineOverview', () => {
 
     it('hasNoAccount', async () => {
       const wrapper = await mountWithApiMock({
-        get: () => ({ message: 'no-as-account' }),
+        get: () => Promise.reject({ body: { message: 'no-as-account' } }),
       });
       expect(wrapper.find(NoUserState)).toHaveLength(1);
     });

x-pack/plugins/enterprise_search/public/applications/app_search/components/engine_overview/engine_overview.tsx

@@ -47,34 +47,20 @@ export const EngineOverview: ReactFC<> = () => {
       query: { type, pageIndex },
     });
   };
-  const hasValidData = response => {
-    return (
-      response &&
-      Array.isArray(response.results) &&
-      response.meta &&
-      response.meta.page &&
-      typeof response.meta.page.total_results === 'number'
-    ); // TODO: Move to optional chaining once Prettier has been updated to support it
-  };
-  const hasNoAccountError = response => {
-    return response && response.message === 'no-as-account';
-  };
   const setEnginesData = async (params, callbacks) => {
     try {
       const response = await getEnginesData(params);
-      if (!hasValidData(response)) {
-        if (hasNoAccountError(response)) {
-          return setHasNoAccount(true);
-        }
-        throw new Error('App Search engines response is missing valid data');
-      }
 
       callbacks.setResults(response.results);
       callbacks.setResultsTotal(response.meta.page.total_results);
+
       setIsLoading(false);
     } catch (error) {
-      // TODO - should we be logging errors to telemetry or elsewhere for debugging?
-      setHasErrorConnecting(true);
+      if (error?.body?.message === 'no-as-account') {
+        setHasNoAccount(true);
+      } else {
+        setHasErrorConnecting(true);
+      }
     }
   };
 
@@ -83,16 +69,14 @@ export const EngineOverview: ReactFC<> = () => {
     const callbacks = { setResults: setEngines, setResultsTotal: setEnginesTotal };
 
     setEnginesData(params, callbacks);
-  }, [enginesPage]); // eslint-disable-line
-  // TODO: https://reactjs.org/docs/hooks-faq.html#is-it-safe-to-omit-functions-from-the-list-of-dependencies
+  }, [enginesPage]); // eslint-disable-line react-hooks/exhaustive-deps
 
   useEffect(() => {
     const params = { type: 'meta', pageIndex: metaEnginesPage };
     const callbacks = { setResults: setMetaEngines, setResultsTotal: setMetaEnginesTotal };
 
     setEnginesData(params, callbacks);
-  }, [metaEnginesPage]); // eslint-disable-line
-  // TODO: https://reactjs.org/docs/hooks-faq.html#is-it-safe-to-omit-functions-from-the-list-of-dependencies
+  }, [metaEnginesPage]); // eslint-disable-line react-hooks/exhaustive-deps
 
   if (hasErrorConnecting) return <ErrorState />;
   if (hasNoAccount) return <NoUserState />;

x-pack/plugins/enterprise_search/public/applications/app_search/components/engine_overview/engine_table.tsx

@@ -10,6 +10,8 @@ import { EuiBasicTable, EuiLink } from '@elastic/eui';
 import { sendTelemetry } from '../../../shared/telemetry';
 import { KibanaContext, IKibanaContext } from '../../../index';
 
+import { ENGINES_PAGE_SIZE } from '../../../../../common/constants';
+
 interface IEngineTableProps {
   data: Array<{
     name: string;
@@ -103,7 +105,7 @@ export const EngineTable: ReactFC<IEngineTableProps> = ({
       columns={columns}
       pagination={{
         pageIndex,
-        pageSize: 10, // TODO: pull this out to a constant?
+        pageSize: ENGINES_PAGE_SIZE,
         totalItemCount: totalEngines,
         hidePerPageOptions: true,
       }}

x-pack/plugins/enterprise_search/public/applications/shared/kibana_breadcrumbs/set_breadcrumbs.tsx

@@ -30,7 +30,7 @@ export const SetAppSearchBreadcrumbs: React.FC<ISetBreadcrumbsProps> = ({ text,
 
   useEffect(() => {
     setBreadcrumbs(appSearchBreadcrumbs(history)(crumb));
-  }, []); // eslint-disable-line
+  }, []); // eslint-disable-line react-hooks/exhaustive-deps
 
   return null;
 };

x-pack/plugins/enterprise_search/server/plugin.ts

@@ -11,6 +11,7 @@ import {
   PluginInitializerContext,
   CoreSetup,
   CoreStart,
+  Logger,
   SavedObjectsServiceStart,
 } from 'src/core/server';
 import { UsageCollectionSetup } from 'src/plugins/usage_collection/server';
@@ -30,10 +31,12 @@ export interface ServerConfigType {
 
 export class EnterpriseSearchPlugin implements Plugin {
   private config: Observable<ServerConfigType>;
+  private logger: Logger;
   private savedObjects?: SavedObjectsServiceStart;
 
   constructor(initializerContext: PluginInitializerContext) {
     this.config = initializerContext.config.create<ServerConfigType>();
+    this.logger = initializerContext.logger.get();
   }
 
   public async setup(
@@ -42,7 +45,7 @@ export class EnterpriseSearchPlugin implements Plugin {
   ) {
     const router = http.createRouter();
     const config = await this.config.pipe(first()).toPromise();
-    const dependencies = { router, config };
+    const dependencies = { router, config, log: this.logger };
 
     registerEnginesRoute(dependencies);
 

x-pack/plugins/enterprise_search/server/routes/app_search/engines.test.ts

@@ -6,6 +6,7 @@
 
 import { RequestHandlerContext } from 'kibana/server';
 import { mockRouter, RouterMock } from 'src/core/server/http/router/router.mock';
+import { loggingServiceMock } from 'src/core/server/mocks';
 import { httpServerMock } from 'src/core/server/http/http_server.mocks';
 import { RouteValidatorConfig } from 'src/core/server/http/router/validator';
 
@@ -21,13 +22,15 @@ describe('engine routes', () => {
   describe('GET /api/app_search/engines', () => {
     const AUTH_HEADER = 'Basic 123';
     let router: RouterMock;
-    const mockResponseFactory = httpServerMock.createResponseFactory();
+    const mockResponse = httpServerMock.createResponseFactory();
+    const mockLogger = loggingServiceMock.create().get();
 
     beforeEach(() => {
       jest.resetAllMocks();
       router = mockRouter.create();
       registerEnginesRoute({
         router,
+        log: mockLogger,
         config: {
           host: 'http://localhost:3002',
         },
@@ -38,17 +41,18 @@ describe('engine routes', () => {
       beforeEach(() => {
         AppSearchAPI.shouldBeCalledWith(
           `http://localhost:3002/as/engines/collection?type=indexed&page[current]=1&page[size]=10`,
-          {
-            headers: { Authorization: AUTH_HEADER },
-          }
-        ).andReturn({ name: 'engine1' });
+          { headers: { Authorization: AUTH_HEADER } }
+        ).andReturn({
+          results: [{ name: 'engine1' }],
+          meta: { page: { total_results: 1 } },
+        });
       });
 
       it('should return 200 with a list of engines from the App Search API', async () => {
         await callThisRoute();
 
-        expectResponseToBe200With({
-          body: { name: 'engine1' },
+        expect(mockResponse.ok).toHaveBeenCalledWith({
+          body: { results: [{ name: 'engine1' }], meta: { page: { total_results: 1 } } },
           headers: { 'content-type': 'application/json' },
         });
       });
@@ -58,19 +62,57 @@ describe('engine routes', () => {
       beforeEach(() => {
         AppSearchAPI.shouldBeCalledWith(
           `http://localhost:3002/as/engines/collection?type=indexed&page[current]=1&page[size]=10`,
-          {
-            headers: { Authorization: AUTH_HEADER },
-          }
+          { headers: { Authorization: AUTH_HEADER } }
         ).andReturnRedirect();
       });
 
-      it('should return 200 with a message', async () => {
+      it('should return 403 with a message', async () => {
         await callThisRoute();
 
-        expectResponseToBe200With({
-          body: { message: 'no-as-account' },
-          headers: { 'content-type': 'application/json' },
+        expect(mockResponse.forbidden).toHaveBeenCalledWith({
+          body: 'no-as-account',
+        });
+        expect(mockLogger.info).toHaveBeenCalledWith('No corresponding App Search account found');
+      });
+    });
+
+    describe('when the App Search URL is invalid', () => {
+      beforeEach(() => {
+        AppSearchAPI.shouldBeCalledWith(
+          `http://localhost:3002/as/engines/collection?type=indexed&page[current]=1&page[size]=10`,
+          { headers: { Authorization: AUTH_HEADER } }
+        ).andReturnError();
+      });
+
+      it('should return 404 with a message', async () => {
+        await callThisRoute();
+
+        expect(mockResponse.notFound).toHaveBeenCalledWith({
+          body: 'cannot-connect',
+        });
+        expect(mockLogger.error).toHaveBeenCalledWith('Cannot connect to App Search: Failed');
+        expect(mockLogger.debug).not.toHaveBeenCalled();
+      });
+    });
+
+    describe('when the App Search API returns invalid data', () => {
+      beforeEach(() => {
+        AppSearchAPI.shouldBeCalledWith(
+          `http://localhost:3002/as/engines/collection?type=indexed&page[current]=1&page[size]=10`,
+          { headers: { Authorization: AUTH_HEADER } }
+        ).andReturnInvalidData();
+      });
+
+      it('should return 404 with a message', async () => {
+        await callThisRoute();
+
+        expect(mockResponse.notFound).toHaveBeenCalledWith({
+          body: 'cannot-connect',
         });
+        expect(mockLogger.error).toHaveBeenCalledWith(
+          'Cannot connect to App Search: Error: Invalid data received from App Search: {"foo":"bar"}'
+        );
+        expect(mockLogger.debug).toHaveBeenCalled();
       });
     });
 
@@ -88,7 +130,7 @@ describe('engine routes', () => {
       }
 
       describe('when query is valid', () => {
-        const request = { query: { type: 'indexed', pageIndex: 1 } };
+        const request = { query: { type: 'meta', pageIndex: 5 } };
         itShouldValidate(request);
       });
 
@@ -97,8 +139,8 @@ describe('engine routes', () => {
         itShouldThrow(request);
       });
 
-      describe('type is wrong type', () => {
-        const request = { query: { type: 1, pageIndex: 1 } };
+      describe('type is wrong string', () => {
+        const request = { query: { type: 'invalid', pageIndex: 1 } };
         itShouldThrow(request);
       });
 
@@ -136,14 +178,26 @@ describe('engine routes', () => {
               return Promise.resolve(new Response(JSON.stringify(response)));
             });
           },
+          andReturnInvalidData(response: object) {
+            fetchMock.mockImplementation((url: string, params: object) => {
+              expect(url).toEqual(expectedUrl);
+              expect(params).toEqual(expectedParams);
+
+              return Promise.resolve(new Response(JSON.stringify({ foo: 'bar' })));
+            });
+          },
+          andReturnError(response: object) {
+            fetchMock.mockImplementation((url: string, params: object) => {
+              expect(url).toEqual(expectedUrl);
+              expect(params).toEqual(expectedParams);
+
+              return Promise.reject('Failed');
+            });
+          },
         };
       },
     };
 
-    const expectResponseToBe200With = (response: object) => {
-      expect(mockResponseFactory.ok).toHaveBeenCalledWith(response);
-    };
-
     const callThisRoute = async (
       request = {
         headers: {
@@ -158,7 +212,7 @@ describe('engine routes', () => {
       const [_, handler] = router.get.mock.calls[0];
 
       const context = {} as jest.Mocked<RequestHandlerContext>;
-      await handler(context, httpServerMock.createKibanaRequest(request), mockResponseFactory);
+      await handler(context, httpServerMock.createKibanaRequest(request), mockResponse);
     };
 
     const executeRouteValidation = (data: { query: object }) => {

x-pack/plugins/enterprise_search/server/routes/app_search/engines.ts

@@ -7,38 +7,54 @@
 import fetch from 'node-fetch';
 import { schema } from '@kbn/config-schema';
 
-export function registerEnginesRoute({ router, config }) {
+import { ENGINES_PAGE_SIZE } from '../../../common/constants';
+
+export function registerEnginesRoute({ router, config, log }) {
   router.get(
     {
       path: '/api/app_search/engines',
       validate: {
         query: schema.object({
-          type: schema.string(),
+          type: schema.oneOf([schema.literal('indexed'), schema.literal('meta')]),
           pageIndex: schema.number(),
         }),
       },
     },
     async (context, request, response) => {
-      const appSearchUrl = config.host;
-      const { type, pageIndex } = request.query;
-
-      const url = `${appSearchUrl}/as/engines/collection?type=${type}&page[current]=${pageIndex}&page[size]=10`;
-      const enginesResponse = await fetch(url, {
-        headers: { Authorization: request.headers.authorization },
-      });
-
-      if (enginesResponse.url.endsWith('/login')) {
-        return response.ok({
-          body: { message: 'no-as-account' },
-          headers: { 'content-type': 'application/json' },
+      try {
+        const appSearchUrl = config.host;
+        const { type, pageIndex } = request.query;
+
+        const url = `${appSearchUrl}/as/engines/collection?type=${type}&page[current]=${pageIndex}&page[size]=${ENGINES_PAGE_SIZE}`;
+        const enginesResponse = await fetch(url, {
+          headers: { Authorization: request.headers.authorization },
         });
-      }
 
-      const engines = await enginesResponse.json();
-      return response.ok({
-        body: engines,
-        headers: { 'content-type': 'application/json' },
-      });
+        if (enginesResponse.url.endsWith('/login')) {
+          log.info('No corresponding App Search account found');
+          // Note: Can't use response.unauthorized, Kibana will auto-log out the user
+          return response.forbidden({ body: 'no-as-account' });
+        }
+
+        const engines = await enginesResponse.json();
+        const hasValidData =
+          Array.isArray(engines?.results) && typeof engines?.meta?.page?.total_results === 'number';
+
+        if (hasValidData) {
+          return response.ok({
+            body: engines,
+            headers: { 'content-type': 'application/json' },
+          });
+        } else {
+          // Either a completely incorrect Enterprise Search host URL was configured, or App Search is returning bad data
+          throw new Error(`Invalid data received from App Search: ${JSON.stringify(engines)}`);
+        }
+      } catch (e) {
+        log.error(`Cannot connect to App Search: ${e.toString()}`);
+        if (e instanceof Error) log.debug(e.stack);
+
+        return response.notFound({ body: 'cannot-connect' });
+      }
     }
   );
 }