Skip to content

Commit

Permalink
[Reporting] Clean up any usage, reorganize server route files (#110740
Browse files Browse the repository at this point in the history
)

* refactor/reflatten server routes

* fix import

* fix any usage in server/lib

* clean up unused parameter

* remove any in server/browsers

* refactor handle request function into a class

* more cleanup
  • Loading branch information
tsullivan authored Sep 2, 2021
1 parent b0a0dc2 commit e6faa58
Show file tree
Hide file tree
Showing 29 changed files with 380 additions and 264 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,8 @@ export class HeadlessChromiumDriverFactory {
{ viewport, browserTimezone }: { viewport: ViewportConfig; browserTimezone?: string },
pLogger: LevelLogger
): Rx.Observable<{ driver: HeadlessChromiumDriver; exit$: Rx.Observable<never> }> {
return Rx.Observable.create(async (observer: InnerSubscriber<any, any>) => {
// FIXME: 'create' is deprecated
return Rx.Observable.create(async (observer: InnerSubscriber<unknown, unknown>) => {
const logger = pLogger.clone(['browser-driver']);
logger.info(`Creating browser page driver`);

Expand Down
56 changes: 45 additions & 11 deletions x-pack/plugins/reporting/server/config/ui_settings.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,23 +49,57 @@ test('throws validation error if provided with data over max size', () => {
});

test('throws validation error if provided with non-image data', () => {
const invalidErrorMatcher = /try a different image/;

expect(() => PdfLogoSchema.validate('')).toThrowError(invalidErrorMatcher);
expect(() => PdfLogoSchema.validate(true)).toThrow(invalidErrorMatcher);
expect(() => PdfLogoSchema.validate(false)).toThrow(invalidErrorMatcher);
expect(() => PdfLogoSchema.validate({})).toThrow(invalidErrorMatcher);
expect(() => PdfLogoSchema.validate([])).toThrow(invalidErrorMatcher);
expect(() => PdfLogoSchema.validate(0)).toThrow(invalidErrorMatcher);
expect(() => PdfLogoSchema.validate(0x00f)).toThrow(invalidErrorMatcher);
expect(() => PdfLogoSchema.validate('')).toThrowErrorMatchingInlineSnapshot(`
"types that failed validation:
- [0]: Sorry, that file will not work. Please try a different image file.
- [1]: expected value to equal [null]"
`);
expect(() => PdfLogoSchema.validate(true)).toThrowErrorMatchingInlineSnapshot(`
"types that failed validation:
- [0]: expected value of type [string] but got [boolean]
- [1]: expected value to equal [null]"
`);
expect(() => PdfLogoSchema.validate(false)).toThrowErrorMatchingInlineSnapshot(`
"types that failed validation:
- [0]: expected value of type [string] but got [boolean]
- [1]: expected value to equal [null]"
`);
expect(() => PdfLogoSchema.validate({})).toThrowErrorMatchingInlineSnapshot(`
"types that failed validation:
- [0]: expected value of type [string] but got [Object]
- [1]: expected value to equal [null]"
`);
expect(() => PdfLogoSchema.validate([])).toThrowErrorMatchingInlineSnapshot(`
"types that failed validation:
- [0]: expected value of type [string] but got [Array]
- [1]: expected value to equal [null]"
`);
expect(() => PdfLogoSchema.validate(0)).toThrowErrorMatchingInlineSnapshot(`
"types that failed validation:
- [0]: expected value of type [string] but got [number]
- [1]: expected value to equal [null]"
`);
expect(() => PdfLogoSchema.validate(0x00f)).toThrowErrorMatchingInlineSnapshot(`
"types that failed validation:
- [0]: expected value of type [string] but got [number]
- [1]: expected value to equal [null]"
`);

const csvString =
`data:text/csv;base64,Il9pZCIsIl9pbmRleCIsIl9zY29yZSIsIl90eXBlIiwiZm9vLmJhciIsImZvby5iYXIua2V5d29yZCIKZjY1QU9IZ0J5bFZmWW04W` +
`TRvb1EsYmVlLDEsIi0iLGJheixiYXoKbks1QU9IZ0J5bFZmWW04WTdZcUcsYmVlLDEsIi0iLGJvbyxib28K`;
expect(() => PdfLogoSchema.validate(csvString)).toThrow(invalidErrorMatcher);
expect(() => PdfLogoSchema.validate(csvString)).toThrowErrorMatchingInlineSnapshot(`
"types that failed validation:
- [0]: Sorry, that file will not work. Please try a different image file.
- [1]: expected value to equal [null]"
`);

const scriptString =
`data:application/octet-stream;base64,QEVDSE8gT0ZGCldFRUtPRllSLkNPTSB8IEZJTkQgIlRoaXMgaXMiID4gVEVNUC5CQV` +
`QKRUNITz5USElTLkJBVCBTRVQgV0VFSz0lJTMKQ0FMTCBURU1QLkJBVApERUwgIFRFTVAuQkFUCkRFTCAgVEhJUy5CQVQKRUNITyBXZWVrICVXRUVLJQo=`;
expect(() => PdfLogoSchema.validate(scriptString)).toThrow(invalidErrorMatcher);
expect(() => PdfLogoSchema.validate(scriptString)).toThrowErrorMatchingInlineSnapshot(`
"types that failed validation:
- [0]: Sorry, that file will not work. Please try a different image file.
- [1]: expected value to equal [null]"
`);
});
8 changes: 5 additions & 3 deletions x-pack/plugins/reporting/server/config/ui_settings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ const maxLogoSizeInBase64 = kbToBase64Length(200);
const dataurlRegex = /^data:([a-z]+\/[a-z0-9-+.]+)(;[a-z-]+=[a-z0-9-]+)?(;([a-z0-9]+))?,/;
const imageTypes = ['image/svg+xml', 'image/jpeg', 'image/png', 'image/gif'];

const isImageData = (str: any): boolean => {
const isImageData = (str: string) => {
const matches = str.match(dataurlRegex);

if (!matches) {
Expand All @@ -33,7 +33,7 @@ const isImageData = (str: any): boolean => {
return true;
};

const validatePdfLogoBase64String = (str: any) => {
const validatePdfLogoBase64String = (str: string) => {
if (typeof str !== 'string' || !isImageData(str)) {
return i18n.translate('xpack.reporting.uiSettings.validate.customLogo.badFile', {
defaultMessage: `Sorry, that file will not work. Please try a different image file.`,
Expand All @@ -46,7 +46,9 @@ const validatePdfLogoBase64String = (str: any) => {
}
};

export const PdfLogoSchema = schema.nullable(schema.any({ validate: validatePdfLogoBase64String }));
export const PdfLogoSchema = schema.nullable(
schema.string({ validate: validatePdfLogoBase64String })
);

export function registerUiSettings(core: CoreSetup<object, unknown>) {
core.uiSettings.register({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,10 @@ export class PdfMaker {
this._addContents(contents);
}

addImage(image: Buffer, opts = { title: '', description: '' }) {
addImage(
image: Buffer,
opts: { title?: string; description?: string } = { title: '', description: '' }
) {
const size = this._layout.getPdfImageSize();
const img = {
image: `data:image/png;base64,${image.toString('base64')}`,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,8 +73,8 @@ export async function generatePdfObservableFactory(reporting: ReportingCore) {
tracker.startAddImage();
tracker.endAddImage();
pdfOutput.addImage(screenshot.data, {
title: screenshot.title,
description: screenshot.description,
title: screenshot.title ?? undefined,
description: screenshot.description ?? undefined,
});
});
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -84,8 +84,8 @@ export async function generatePdfObservableFactory(reporting: ReportingCore) {
tracker.startAddImage();
tracker.endAddImage();
pdfOutput.addImage(screenshot.data, {
title: screenshot.title,
description: screenshot.description,
title: screenshot.title ?? undefined,
description: screenshot.description ?? undefined,
});
});
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ export const getElementPositionAndAttributes = async (
},
attributes: Object.keys(attributes).reduce((result: AttributesMap, key) => {
const attribute = attributes[key];
(result as any)[key] = element.getAttribute(attribute);
result[key] = element.getAttribute(attribute);
return result;
}, {} as AttributesMap),
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,10 @@
import { HeadlessChromiumDriver } from '../../browsers';
import {
createMockBrowserDriverFactory,
createMockConfig,
createMockConfigSchema,
createMockLayoutInstance,
createMockLevelLogger,
createMockReportingCore,
} from '../../test_helpers';
import { LayoutInstance } from '../layouts';
import { getScreenshots } from './get_screenshots';

describe('getScreenshots', () => {
Expand All @@ -35,17 +32,12 @@ describe('getScreenshots', () => {
},
];

let layout: LayoutInstance;
let logger: ReturnType<typeof createMockLevelLogger>;
let browser: jest.Mocked<HeadlessChromiumDriver>;

beforeEach(async () => {
const schema = createMockConfigSchema();
const config = createMockConfig(schema);
const captureConfig = config.get('capture');
const core = await createMockReportingCore(schema);
const core = await createMockReportingCore(createMockConfigSchema());

layout = createMockLayoutInstance(captureConfig);
logger = createMockLevelLogger();

await createMockBrowserDriverFactory(core, logger, {
Expand All @@ -71,7 +63,7 @@ describe('getScreenshots', () => {
});

it('should return screenshots', async () => {
await expect(getScreenshots(browser, layout, elementsPositionAndAttributes, logger)).resolves
await expect(getScreenshots(browser, elementsPositionAndAttributes, logger)).resolves
.toMatchInlineSnapshot(`
Array [
Object {
Expand Down Expand Up @@ -117,7 +109,7 @@ describe('getScreenshots', () => {
});

it('should forward elements positions', async () => {
await getScreenshots(browser, layout, elementsPositionAndAttributes, logger);
await getScreenshots(browser, elementsPositionAndAttributes, logger);

expect(browser.screenshot).toHaveBeenCalledTimes(2);
expect(browser.screenshot).toHaveBeenNthCalledWith(
Expand All @@ -134,7 +126,7 @@ describe('getScreenshots', () => {
browser.screenshot.mockResolvedValue(Buffer.from(''));

await expect(
getScreenshots(browser, layout, elementsPositionAndAttributes, logger)
getScreenshots(browser, elementsPositionAndAttributes, logger)
).rejects.toBeInstanceOf(Error);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,10 @@
import { i18n } from '@kbn/i18n';
import { LevelLogger, startTrace } from '../';
import { HeadlessChromiumDriver } from '../../browsers';
import { LayoutInstance } from '../layouts';
import { ElementsPositionAndAttribute, Screenshot } from './';

export const getScreenshots = async (
browser: HeadlessChromiumDriver,
layout: LayoutInstance,
elementsPositionAndAttributes: ElementsPositionAndAttribute[],
logger: LevelLogger
): Promise<Screenshot[]> => {
Expand Down
6 changes: 3 additions & 3 deletions x-pack/plugins/reporting/server/lib/screenshots/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ export interface ScreenshotObservableOpts {
}

export interface AttributesMap {
[key: string]: any;
[key: string]: string | null;
}

export interface ElementPosition {
Expand All @@ -45,8 +45,8 @@ export interface ElementsPositionAndAttribute {

export interface Screenshot {
data: Buffer;
title: string;
description: string;
title: string | null;
description: string | null;
}

export interface ScreenshotResults {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ export function getScreenshots$(
const elements = data.elementsPositionAndAttributes
? data.elementsPositionAndAttributes
: getDefaultElementPosition(layout.getViewport(1));
const screenshots = await getScreenshots(driver, layout, elements, logger);
const screenshots = await getScreenshots(driver, elements, logger);
const { timeRange, error: setupError } = data;
return {
timeRange,
Expand Down
4 changes: 2 additions & 2 deletions x-pack/plugins/reporting/server/lib/tasks/execute_report.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,8 @@ interface TaskExecutor extends Pick<ExportTypeDefinition, 'jobContentEncoding'>
jobExecutor: RunTaskFn<BasePayload>;
}

function isOutput(output: any): output is CompletedReportOutput {
return output?.size != null;
function isOutput(output: CompletedReportOutput | Error): output is CompletedReportOutput {
return (output as CompletedReportOutput).size != null;
}

function reportFromTask(task: ReportTaskParams) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ describe('POST /diagnose/screenshot', () => {
toPromise: () => (resp instanceof Error ? Promise.reject(resp) : Promise.resolve(resp)),
}),
}));
(generatePngObservableFactory as any).mockResolvedValue(generateMock);
(generatePngObservableFactory as jest.Mock).mockResolvedValue(generateMock);
};

const config = createMockConfigSchema({ queue: { timeout: 120000 } });
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,16 +5,16 @@
* 2.0.
*/

import { Writable } from 'stream';
import { schema } from '@kbn/config-schema';
import { KibanaRequest } from 'src/core/server';
import { ReportingCore } from '../';
import { runTaskFnFactory } from '../export_types/csv_searchsource_immediate/execute_job';
import { JobParamsDownloadCSV } from '../export_types/csv_searchsource_immediate/types';
import { LevelLogger as Logger } from '../lib';
import { TaskRunResult } from '../lib/tasks';
import { authorizedUserPreRouting } from './lib/authorized_user_pre_routing';
import { HandlerErrorFunction } from './types';
import { Writable } from 'stream';
import { ReportingCore } from '../../';
import { runTaskFnFactory } from '../../export_types/csv_searchsource_immediate/execute_job';
import { JobParamsDownloadCSV } from '../../export_types/csv_searchsource_immediate/types';
import { LevelLogger as Logger } from '../../lib';
import { TaskRunResult } from '../../lib/tasks';
import { authorizedUserPreRouting } from '../lib/authorized_user_pre_routing';
import { RequestHandler } from '../lib/request_handler';

const API_BASE_URL_V1 = '/api/reporting/v1';
const API_BASE_GENERATE_V1 = `${API_BASE_URL_V1}/generate`;
Expand All @@ -32,7 +32,6 @@ export type CsvFromSavedObjectRequest = KibanaRequest<unknown, unknown, JobParam
*/
export function registerGenerateCsvFromSavedObjectImmediate(
reporting: ReportingCore,
handleError: HandlerErrorFunction,
parentLogger: Logger
) {
const setupDeps = reporting.getPluginSetupDeps();
Expand Down Expand Up @@ -64,9 +63,10 @@ export function registerGenerateCsvFromSavedObjectImmediate(
},
authorizedUserPreRouting(
reporting,
async (_user, context, req: CsvFromSavedObjectRequest, res) => {
async (user, context, req: CsvFromSavedObjectRequest, res) => {
const logger = parentLogger.clone(['csv_searchsource_immediate']);
const runTaskFn = runTaskFnFactory(reporting, logger);
const requestHandler = new RequestHandler(reporting, user, context, req, res, logger);

try {
let buffer = Buffer.from('');
Expand Down Expand Up @@ -107,7 +107,7 @@ export function registerGenerateCsvFromSavedObjectImmediate(
});
} catch (err) {
logger.error(err);
return handleError(res, err);
return requestHandler.handleError(err);
}
}
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,19 +7,16 @@

import { schema } from '@kbn/config-schema';
import rison from 'rison-node';
import { ReportingCore } from '../';
import { API_BASE_URL } from '../../common/constants';
import { BaseParams } from '../types';
import { authorizedUserPreRouting } from './lib/authorized_user_pre_routing';
import { HandlerErrorFunction, HandlerFunction } from './types';
import { ReportingCore } from '../..';
import { API_BASE_URL } from '../../../common/constants';
import { LevelLogger } from '../../lib';
import { BaseParams } from '../../types';
import { authorizedUserPreRouting } from '../lib/authorized_user_pre_routing';
import { RequestHandler } from '../lib/request_handler';

const BASE_GENERATE = `${API_BASE_URL}/generate`;

export function registerGenerateFromJobParams(
reporting: ReportingCore,
handler: HandlerFunction,
handleError: HandlerErrorFunction
) {
export function registerJobGenerationRoutes(reporting: ReportingCore, logger: LevelLogger) {
const setupDeps = reporting.getPluginSetupDeps();
const { router } = setupDeps;

Expand Down Expand Up @@ -62,7 +59,6 @@ export function registerGenerateFromJobParams(
});
}

const { exportType } = req.params;
let jobParams;

try {
Expand All @@ -80,10 +76,12 @@ export function registerGenerateFromJobParams(
});
}

const requestHandler = new RequestHandler(reporting, user, context, req, res, logger);

try {
return await handler(user, exportType, jobParams, context, req, res);
return await requestHandler.handleGenerateRequest(req.params.exportType, jobParams);
} catch (err) {
return handleError(res, err);
return requestHandler.handleError(err);
}
})
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,15 +12,15 @@ import { of } from 'rxjs';
import { ElasticsearchClient } from 'kibana/server';
import { setupServer } from 'src/core/server/test_utils';
import supertest from 'supertest';
import { ReportingCore } from '..';
import { ExportTypesRegistry } from '../lib/export_types_registry';
import { createMockLevelLogger, createMockReportingCore } from '../test_helpers';
import { ReportingCore } from '../..';
import { ExportTypesRegistry } from '../../lib/export_types_registry';
import { createMockLevelLogger, createMockReportingCore } from '../../test_helpers';
import {
createMockConfigSchema,
createMockPluginSetup,
} from '../test_helpers/create_mock_reportingplugin';
import { registerJobGenerationRoutes } from './generation';
import type { ReportingRequestHandlerContext } from '../types';
} from '../../test_helpers/create_mock_reportingplugin';
import type { ReportingRequestHandlerContext } from '../../types';
import { registerJobGenerationRoutes } from './generate_from_jobparams';

type SetupServerReturn = UnwrapPromise<ReturnType<typeof setupServer>>;

Expand Down
Loading

0 comments on commit e6faa58

Please sign in to comment.