[Enterprise Search] Request handler refactors/enhancements + update e…

…xisting routes to use shared handler (#76106) (#76232)

* Refactor enterprise_search_request_handler to a class that stores config/log

- So that routes don't have to continuously pass it in - just params that actually change (paths, queries)

+ misc refactors/lint fixes from code review

* Update mocks/dependencies so other tests pass type checks

* Update /api/workplace_search/overview endpoint to use new request handler

* Update /api/app_search/engines route to use new handler

- This required updating the shared handler to accept custom params set by the route, since the engines route transmutes some param names
- createRequest mock also had to be updated to correctly reflect a curried function in order to get tests passing

* DRY out hasValidData to a reusable/cleaner call

* Update shared handler to output specific message for auth errors

- Check for /login URL (indicates auth issue)
- Refactor final catch return to allow being passed messages from previous throw
- Change hasValidData fallback to only log (potentially sensitive?) JSON data to server, not to client
- Minor test cleanup - move error tests to single describe block, minor URL/newline cleanup

* Update handler to pass method & body requests + pass back response code

- This will support future POST requests as well as support (e.g.) 404 responses

Minor test refactors:
- Set up new request defaults (method, empty body) + expected output & convert KibanaAuthHeader to string since they're set so close to one another
- group request pass tests into a describe block, remove response body portion of their tests (since we don't really care about that for those tests)
- group response tests into describe block
- clarify how passed handler params arg overrides request.params

* PR feedback: Update custom params arg to take an object instead of a query string

x-pack/plugins/enterprise_search/server/__mocks__/index.ts

@@ -5,4 +5,9 @@
  */
 
 export { MockRouter } from './router.mock';
-export { mockConfig, mockLogger, mockDependencies } from './routerDependencies.mock';
+export {
+  mockConfig,
+  mockLogger,
+  mockRequestHandler,
+  mockDependencies,
+} from './routerDependencies.mock';

x-pack/plugins/enterprise_search/server/__mocks__/routerDependencies.mock.ts

@@ -9,6 +9,13 @@ import { ConfigType } from '../';
 
 export const mockLogger = loggingSystemMock.createLogger().get();
 
+export const mockRequestHandler = {
+  createRequest: jest.fn(() => () => {}),
+  hasValidData(data: any) {
+    return (this.createRequest as jest.Mock).mock.calls[0][0].hasValidData(data);
+  },
+};
+
 export const mockConfig = {
   enabled: true,
   host: 'http://localhost:3002',
@@ -24,4 +31,5 @@ export const mockDependencies = {
   // Mock router should be handled on a per-test basis
   config: mockConfig,
   log: mockLogger,
+  enterpriseSearchRequestHandler: mockRequestHandler as any,
 };

x-pack/plugins/enterprise_search/server/lib/enterprise_search_request_handler.test.ts

@@ -6,20 +6,24 @@
 
 import { mockConfig, mockLogger } from '../__mocks__';
 
-import { createEnterpriseSearchRequestHandler } from './enterprise_search_request_handler';
+import { EnterpriseSearchRequestHandler } from './enterprise_search_request_handler';
 
 jest.mock('node-fetch');
 // eslint-disable-next-line @typescript-eslint/no-var-requires
 const fetchMock = require('node-fetch') as jest.Mock;
 const { Response } = jest.requireActual('node-fetch');
 
 const responseMock = {
-  ok: jest.fn(),
+  custom: jest.fn(),
   customError: jest.fn(),
 };
-const KibanaAuthHeader = 'Basic 123';
 
-describe('createEnterpriseSearchRequestHandler', () => {
+describe('EnterpriseSearchRequestHandler', () => {
+  const enterpriseSearchRequestHandler = new EnterpriseSearchRequestHandler({
+    config: mockConfig,
+    log: mockLogger,
+  }) as any;
+
   beforeEach(() => {
     jest.clearAllMocks();
     fetchMock.mockReset();
@@ -33,9 +37,7 @@ describe('createEnterpriseSearchRequestHandler', () => {
 
     EnterpriseSearchAPI.mockReturn(responseBody);
 
-    const requestHandler = createEnterpriseSearchRequestHandler({
-      config: mockConfig,
-      log: mockLogger,
+    const requestHandler = enterpriseSearchRequestHandler.createRequest({
       path: '/as/credentials/collection',
     });
 
@@ -47,82 +49,151 @@ describe('createEnterpriseSearchRequestHandler', () => {
     });
 
     EnterpriseSearchAPI.shouldHaveBeenCalledWith(
-      'http://localhost:3002/as/credentials/collection?type=indexed&pageIndex=1'
+      'http://localhost:3002/as/credentials/collection?type=indexed&pageIndex=1',
+      { method: 'GET' }
     );
 
-    expect(responseMock.ok).toHaveBeenCalledWith({
+    expect(responseMock.custom).toHaveBeenCalledWith({
       body: responseBody,
+      statusCode: 200,
     });
   });
 
-  describe('when an API request fails', () => {
-    it('should return 502 with a message', async () => {
-      EnterpriseSearchAPI.mockReturnError();
+  describe('request passing', () => {
+    it('passes route method', async () => {
+      const requestHandler = enterpriseSearchRequestHandler.createRequest({ path: '/api/example' });
 
-      const requestHandler = createEnterpriseSearchRequestHandler({
-        config: mockConfig,
-        log: mockLogger,
-        path: '/as/credentials/collection',
+      await makeAPICall(requestHandler, { route: { method: 'POST' } });
+      EnterpriseSearchAPI.shouldHaveBeenCalledWith('http://localhost:3002/api/example', {
+        method: 'POST',
       });
 
-      await makeAPICall(requestHandler);
+      await makeAPICall(requestHandler, { route: { method: 'DELETE' } });
+      EnterpriseSearchAPI.shouldHaveBeenCalledWith('http://localhost:3002/api/example', {
+        method: 'DELETE',
+      });
+    });
+
+    it('passes request body', async () => {
+      const requestHandler = enterpriseSearchRequestHandler.createRequest({ path: '/api/example' });
+      await makeAPICall(requestHandler, { body: { bodacious: true } });
+
+      EnterpriseSearchAPI.shouldHaveBeenCalledWith('http://localhost:3002/api/example', {
+        body: '{"bodacious":true}',
+      });
+    });
+
+    it('passes custom params set by the handler, which override request params', async () => {
+      const requestHandler = enterpriseSearchRequestHandler.createRequest({
+        path: '/api/example',
+        params: { someQuery: true },
+      });
+      await makeAPICall(requestHandler, { query: { someQuery: false } });
 
       EnterpriseSearchAPI.shouldHaveBeenCalledWith(
-        'http://localhost:3002/as/credentials/collection'
+        'http://localhost:3002/api/example?someQuery=true'
       );
+    });
+  });
 
-      expect(responseMock.customError).toHaveBeenCalledWith({
-        body: 'Error connecting or fetching data from Enterprise Search',
-        statusCode: 502,
-      });
+  describe('response passing', () => {
+    it('returns the response status code from Enterprise Search', async () => {
+      EnterpriseSearchAPI.mockReturn({}, { status: 404 });
+
+      const requestHandler = enterpriseSearchRequestHandler.createRequest({ path: '/api/example' });
+      await makeAPICall(requestHandler);
+
+      EnterpriseSearchAPI.shouldHaveBeenCalledWith('http://localhost:3002/api/example');
+      expect(responseMock.custom).toHaveBeenCalledWith({ body: {}, statusCode: 404 });
     });
+
+    // TODO: It's possible we may also pass back headers at some point
+    // from Enterprise Search, e.g. the x-read-only mode header
   });
 
-  describe('when `hasValidData` fails', () => {
-    it('should return 502 with a message', async () => {
-      const responseBody = {
-        foo: 'bar',
-      };
+  describe('error handling', () => {
+    afterEach(() => {
+      expect(mockLogger.error).toHaveBeenCalledWith(
+        expect.stringContaining('Error connecting to Enterprise Search')
+      );
+    });
+
+    it('returns an error when an API request fails', async () => {
+      EnterpriseSearchAPI.mockReturnError();
+      const requestHandler = enterpriseSearchRequestHandler.createRequest({ path: '/api/failed' });
+
+      await makeAPICall(requestHandler);
+      EnterpriseSearchAPI.shouldHaveBeenCalledWith('http://localhost:3002/api/failed');
 
-      EnterpriseSearchAPI.mockReturn(responseBody);
+      expect(responseMock.customError).toHaveBeenCalledWith({
+        body: 'Error connecting to Enterprise Search: Failed',
+        statusCode: 502,
+      });
+    });
 
-      const requestHandler = createEnterpriseSearchRequestHandler({
-        config: mockConfig,
-        log: mockLogger,
-        path: '/as/credentials/collection',
-        hasValidData: (body?: any) =>
-          Array.isArray(body?.results) && typeof body?.meta?.page?.total_results === 'number',
+    it('returns an error when `hasValidData` fails', async () => {
+      EnterpriseSearchAPI.mockReturn({ results: false });
+      const requestHandler = enterpriseSearchRequestHandler.createRequest({
+        path: '/api/invalid',
+        hasValidData: (body?: any) => Array.isArray(body?.results),
       });
 
       await makeAPICall(requestHandler);
+      EnterpriseSearchAPI.shouldHaveBeenCalledWith('http://localhost:3002/api/invalid');
 
-      EnterpriseSearchAPI.shouldHaveBeenCalledWith(
-        'http://localhost:3002/as/credentials/collection'
+      expect(responseMock.customError).toHaveBeenCalledWith({
+        body: 'Error connecting to Enterprise Search: Invalid data received',
+        statusCode: 502,
+      });
+      expect(mockLogger.debug).toHaveBeenCalledWith(
+        'Invalid data received from <http://localhost:3002/api/invalid>: {"results":false}'
       );
+    });
+
+    it('returns an error when user authentication to Enterprise Search fails', async () => {
+      EnterpriseSearchAPI.mockReturn({}, { url: 'http://localhost:3002/login' });
+      const requestHandler = enterpriseSearchRequestHandler.createRequest({
+        path: '/api/unauthenticated',
+      });
+
+      await makeAPICall(requestHandler);
+      EnterpriseSearchAPI.shouldHaveBeenCalledWith('http://localhost:3002/api/unauthenticated');
 
       expect(responseMock.customError).toHaveBeenCalledWith({
-        body: 'Error connecting or fetching data from Enterprise Search',
+        body: 'Error connecting to Enterprise Search: Cannot authenticate Enterprise Search user',
         statusCode: 502,
       });
     });
   });
+
+  it('has a helper for checking empty objects', async () => {
+    expect(enterpriseSearchRequestHandler.isEmptyObj({})).toEqual(true);
+    expect(enterpriseSearchRequestHandler.isEmptyObj({ empty: false })).toEqual(false);
+  });
 });
 
 const makeAPICall = (handler: Function, params = {}) => {
-  const request = { headers: { authorization: KibanaAuthHeader }, ...params };
+  const request = {
+    headers: { authorization: 'Basic 123' },
+    route: { method: 'GET' },
+    body: {},
+    ...params,
+  };
   return handler(null, request, responseMock);
 };
 
 const EnterpriseSearchAPI = {
   shouldHaveBeenCalledWith(expectedUrl: string, expectedParams = {}) {
     expect(fetchMock).toHaveBeenCalledWith(expectedUrl, {
-      headers: { Authorization: KibanaAuthHeader },
+      headers: { Authorization: 'Basic 123' },
+      method: 'GET',
+      body: undefined,
       ...expectedParams,
     });
   },
-  mockReturn(response: object) {
+  mockReturn(response: object, options?: object) {
     fetchMock.mockImplementation(() => {
-      return Promise.resolve(new Response(JSON.stringify(response)));
+      return Promise.resolve(new Response(JSON.stringify(response), options));
     });
   },
   mockReturnError() {

x-pack/plugins/enterprise_search/server/lib/enterprise_search_request_handler.ts

@@ -7,63 +7,98 @@
 import fetch from 'node-fetch';
 import querystring from 'querystring';
 import {
+  RequestHandler,
   RequestHandlerContext,
   KibanaRequest,
   KibanaResponseFactory,
   Logger,
 } from 'src/core/server';
 import { ConfigType } from '../index';
 
-interface IEnterpriseSearchRequestParams<ResponseBody> {
+interface IConstructorDependencies {
   config: ConfigType;
   log: Logger;
+}
+interface IRequestParams<ResponseBody> {
   path: string;
+  params?: object;
   hasValidData?: (body?: ResponseBody) => boolean;
 }
+export interface IEnterpriseSearchRequestHandler {
+  createRequest(requestParams?: object): RequestHandler<unknown, Readonly<{}>, unknown>;
+}
 
 /**
- * This helper function creates a single standard DRY way of handling
+ * This helper lib creates a single standard DRY way of handling
  * Enterprise Search API requests.
  *
  * This handler assumes that it will essentially just proxy the
  * Enterprise Search API request, so the request body and request
  * parameters are simply passed through.
  */
-export function createEnterpriseSearchRequestHandler<ResponseBody>({
-  config,
-  log,
-  path,
-  hasValidData = () => true,
-}: IEnterpriseSearchRequestParams<ResponseBody>) {
-  return async (
-    _context: RequestHandlerContext,
-    request: KibanaRequest<unknown, Readonly<{}>, unknown>,
-    response: KibanaResponseFactory
-  ) => {
-    try {
-      const enterpriseSearchUrl = config.host as string;
-      const params = request.query ? `?${querystring.stringify(request.query)}` : '';
-      const url = `${encodeURI(enterpriseSearchUrl)}${path}${params}`;
+export class EnterpriseSearchRequestHandler {
+  private enterpriseSearchUrl: string;
+  private log: Logger;
+
+  constructor({ config, log }: IConstructorDependencies) {
+    this.log = log;
+    this.enterpriseSearchUrl = config.host as string;
+  }
+
+  createRequest<ResponseBody>({
+    path,
+    params = {},
+    hasValidData = () => true,
+  }: IRequestParams<ResponseBody>) {
+    return async (
+      _context: RequestHandlerContext,
+      request: KibanaRequest<unknown, Readonly<{}>, unknown>,
+      response: KibanaResponseFactory
+    ) => {
+      try {
+        // Set up API URL
+        const queryParams = { ...request.query, ...params };
+        const queryString = !this.isEmptyObj(queryParams)
+          ? `?${querystring.stringify(queryParams)}`
+          : '';
+        const url = encodeURI(this.enterpriseSearchUrl + path + queryString);
+
+        // Set up API options
+        const { method } = request.route;
+        const headers = { Authorization: request.headers.authorization as string };
+        const body = !this.isEmptyObj(request.body as object)
+          ? JSON.stringify(request.body)
+          : undefined;
+
+        // Call the Enterprise Search API and pass back response to the front-end
+        const apiResponse = await fetch(url, { method, headers, body });
+
+        if (apiResponse.url.endsWith('/login')) {
+          throw new Error('Cannot authenticate Enterprise Search user');
+        }
+
+        const { status } = apiResponse;
+        const json = await apiResponse.json();
 
-      const apiResponse = await fetch(url, {
-        headers: { Authorization: request.headers.authorization as string },
-      });
+        if (hasValidData(json)) {
+          return response.custom({ statusCode: status, body: json });
+        } else {
+          this.log.debug(`Invalid data received from <${url}>: ${JSON.stringify(json)}`);
+          throw new Error('Invalid data received');
+        }
+      } catch (e) {
+        const errorMessage = `Error connecting to Enterprise Search: ${e?.message || e.toString()}`;
 
-      const body = await apiResponse.json();
+        this.log.error(errorMessage);
+        if (e instanceof Error) this.log.debug(e.stack as string);
 
-      if (hasValidData(body)) {
-        return response.ok({ body });
-      } else {
-        throw new Error(`Invalid data received: ${JSON.stringify(body)}`);
+        return response.customError({ statusCode: 502, body: errorMessage });
       }
-    } catch (e) {
-      log.error(`Cannot connect to Enterprise Search: ${e.toString()}`);
-      if (e instanceof Error) log.debug(e.stack as string);
+    };
+  }
 
-      return response.customError({
-        statusCode: 502,
-        body: 'Error connecting or fetching data from Enterprise Search',
-      });
-    }
-  };
+  // Small helper
+  isEmptyObj(obj: object) {
+    return Object.keys(obj).length === 0;
+  }
 }