diff --git a/x-pack/plugins/enterprise_search/common/constants.ts b/x-pack/plugins/enterprise_search/common/constants.ts new file mode 100644 index 0000000000000..c134131caba75 --- /dev/null +++ b/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; diff --git a/x-pack/plugins/enterprise_search/public/applications/app_search/components/engine_overview/engine_overview.test.tsx b/x-pack/plugins/enterprise_search/public/applications/app_search/components/engine_overview/engine_overview.test.tsx index dd3effce21957..8f707fe57bde7 100644 --- a/x-pack/plugins/enterprise_search/public/applications/app_search/components/engine_overview/engine_overview.test.tsx +++ b/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); }); diff --git a/x-pack/plugins/enterprise_search/public/applications/app_search/components/engine_overview/engine_overview.tsx b/x-pack/plugins/enterprise_search/public/applications/app_search/components/engine_overview/engine_overview.tsx index 8c3c6d61c89d8..d87c36cd9b9d6 100644 --- a/x-pack/plugins/enterprise_search/public/applications/app_search/components/engine_overview/engine_overview.tsx +++ b/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 ; if (hasNoAccount) return ; diff --git a/x-pack/plugins/enterprise_search/public/applications/app_search/components/engine_overview/engine_table.tsx b/x-pack/plugins/enterprise_search/public/applications/app_search/components/engine_overview/engine_table.tsx index 8db8538e82788..e138bade11c15 100644 --- a/x-pack/plugins/enterprise_search/public/applications/app_search/components/engine_overview/engine_table.tsx +++ b/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 = ({ columns={columns} pagination={{ pageIndex, - pageSize: 10, // TODO: pull this out to a constant? + pageSize: ENGINES_PAGE_SIZE, totalItemCount: totalEngines, hidePerPageOptions: true, }} diff --git a/x-pack/plugins/enterprise_search/public/applications/shared/kibana_breadcrumbs/set_breadcrumbs.tsx b/x-pack/plugins/enterprise_search/public/applications/shared/kibana_breadcrumbs/set_breadcrumbs.tsx index 19ba890e0af0d..aaa54febcc20b 100644 --- a/x-pack/plugins/enterprise_search/public/applications/shared/kibana_breadcrumbs/set_breadcrumbs.tsx +++ b/x-pack/plugins/enterprise_search/public/applications/shared/kibana_breadcrumbs/set_breadcrumbs.tsx @@ -30,7 +30,7 @@ export const SetAppSearchBreadcrumbs: React.FC = ({ text, useEffect(() => { setBreadcrumbs(appSearchBreadcrumbs(history)(crumb)); - }, []); // eslint-disable-line + }, []); // eslint-disable-line react-hooks/exhaustive-deps return null; }; diff --git a/x-pack/plugins/enterprise_search/server/plugin.ts b/x-pack/plugins/enterprise_search/server/plugin.ts index d9e5ce79537e6..f93fab18ab90e 100644 --- a/x-pack/plugins/enterprise_search/server/plugin.ts +++ b/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; + private logger: Logger; private savedObjects?: SavedObjectsServiceStart; constructor(initializerContext: PluginInitializerContext) { this.config = initializerContext.config.create(); + 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); diff --git a/x-pack/plugins/enterprise_search/server/routes/app_search/engines.test.ts b/x-pack/plugins/enterprise_search/server/routes/app_search/engines.test.ts index 608d79f0cdbcf..02408544a8315 100644 --- a/x-pack/plugins/enterprise_search/server/routes/app_search/engines.test.ts +++ b/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; - await handler(context, httpServerMock.createKibanaRequest(request), mockResponseFactory); + await handler(context, httpServerMock.createKibanaRequest(request), mockResponse); }; const executeRouteValidation = (data: { query: object }) => { diff --git a/x-pack/plugins/enterprise_search/server/routes/app_search/engines.ts b/x-pack/plugins/enterprise_search/server/routes/app_search/engines.ts index 8455b01aa4354..3a474dc58e4dd 100644 --- a/x-pack/plugins/enterprise_search/server/routes/app_search/engines.ts +++ b/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' }); + } } ); } diff --git a/x-pack/plugins/enterprise_search/server/routes/app_search/telemetry.test.ts b/x-pack/plugins/enterprise_search/server/routes/app_search/telemetry.test.ts index 02fc3f63f402a..db98b95d500ab 100644 --- a/x-pack/plugins/enterprise_search/server/routes/app_search/telemetry.test.ts +++ b/x-pack/plugins/enterprise_search/server/routes/app_search/telemetry.test.ts @@ -6,6 +6,7 @@ import { mockRouter, RouterMock } from 'src/core/server/http/router/router.mock'; import { savedObjectsServiceMock } from 'src/core/server/saved_objects/saved_objects_service.mock'; +import { loggingServiceMock } from 'src/core/server/mocks'; import { httpServerMock } from 'src/core/server/http/http_server.mocks'; import { registerTelemetryRoute } from './telemetry'; @@ -17,7 +18,8 @@ import { incrementUICounter } from '../../collectors/app_search/telemetry'; describe('App Search Telemetry API', () => { let router: RouterMock; - const mockResponseFactory = httpServerMock.createResponseFactory(); + const mockResponse = httpServerMock.createResponseFactory(); + const mockLogger = loggingServiceMock.create().get(); beforeEach(() => { jest.resetAllMocks(); @@ -25,6 +27,7 @@ describe('App Search Telemetry API', () => { registerTelemetryRoute({ router, getSavedObjectsService: () => savedObjectsServiceMock.create(), + log: mockLogger, }); }); @@ -40,16 +43,17 @@ describe('App Search Telemetry API', () => { uiAction: 'ui_viewed', metric: 'setup_guide', }); - expect(mockResponseFactory.ok).toHaveBeenCalledWith({ body: successResponse }); + expect(mockResponse.ok).toHaveBeenCalledWith({ body: successResponse }); }); it('throws an error when incrementing fails', async () => { - incrementUICounter.mockImplementation(jest.fn(() => Promise.reject())); + incrementUICounter.mockImplementation(jest.fn(() => Promise.reject('Failed'))); await callThisRoute('put', { body: { action: 'error', metric: 'error' } }); expect(incrementUICounter).toHaveBeenCalled(); - expect(mockResponseFactory.internalError).toHaveBeenCalled(); + expect(mockLogger.error).toHaveBeenCalled(); + expect(mockResponse.internalError).toHaveBeenCalled(); }); describe('validates', () => { @@ -96,7 +100,7 @@ describe('App Search Telemetry API', () => { const [_, handler] = router[method].mock.calls[0]; const context = {}; - await handler(context, httpServerMock.createKibanaRequest(request), mockResponseFactory); + await handler(context, httpServerMock.createKibanaRequest(request), mockResponse); }; const executeRouteValidation = request => { diff --git a/x-pack/plugins/enterprise_search/server/routes/app_search/telemetry.ts b/x-pack/plugins/enterprise_search/server/routes/app_search/telemetry.ts index 3eabe1f19c5ff..6b7657a384e9f 100644 --- a/x-pack/plugins/enterprise_search/server/routes/app_search/telemetry.ts +++ b/x-pack/plugins/enterprise_search/server/routes/app_search/telemetry.ts @@ -8,7 +8,7 @@ import { schema } from '@kbn/config-schema'; import { incrementUICounter } from '../../collectors/app_search/telemetry'; -export function registerTelemetryRoute({ router, getSavedObjectsService }) { +export function registerTelemetryRoute({ router, getSavedObjectsService, log }) { router.put( { path: '/api/app_search/telemetry', @@ -35,6 +35,7 @@ export function registerTelemetryRoute({ router, getSavedObjectsService }) { }), }); } catch (e) { + log.error(`App Search UI telemetry error: ${e instanceof Error ? e.stack : e.toString()}`); return response.internalError({ body: 'App Search UI telemetry failed' }); } }