Skip to content

Commit

Permalink
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Address larger tech debt/TODOs (#8)
Browse files Browse the repository at this point in the history
* 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)
Constance authored and cee-chen committed Jun 16, 2020
1 parent 1f0ab9d commit 069500f
Showing 10 changed files with 147 additions and 76 deletions.
7 changes: 7 additions & 0 deletions x-pack/plugins/enterprise_search/common/constants.ts
Original file line number Diff line number Diff line change
@@ -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;
Original file line number Diff line number Diff line change
@@ -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);
});
Original file line number Diff line number Diff line change
@@ -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 />;
Original file line number Diff line number Diff line change
@@ -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,
}}
Original file line number Diff line number Diff line change
@@ -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;
};
5 changes: 4 additions & 1 deletion x-pack/plugins/enterprise_search/server/plugin.ts
Original file line number Diff line number Diff line change
@@ -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);

Original file line number Diff line number Diff line change
@@ -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 }) => {
Original file line number Diff line number Diff line change
@@ -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' });
}
}
);
}
Original file line number Diff line number Diff line change
@@ -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,14 +18,16 @@ 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();
router = mockRouter.create();
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 => {
Original file line number Diff line number Diff line change
@@ -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' });
}
}

0 comments on commit 069500f

Please sign in to comment.