Skip to content

Commit

Permalink
[Reporting/Server] register plugin routes synchronously (elastic#68976)…
Browse files Browse the repository at this point in the history
… (elastic#69224)

* register routes synchronously

* back out some refactoring

* comment fix

* fix tests

* register route handler context provider

* Add function level comments in core methods

* fix tests

* revert editor help

* route context is the ReportingStart contract

* Fix reporting job route tests

* Fix generation tests

Co-authored-by: Joel Griffith <[email protected]>
Co-authored-by: Elastic Machine <[email protected]>
# Conflicts:
#	x-pack/plugins/reporting/server/routes/generation.ts

Co-authored-by: Elastic Machine <[email protected]>
  • Loading branch information
tsullivan and elasticmachine authored Jun 16, 2020
1 parent 41e4fcd commit 0228ad7
Show file tree
Hide file tree
Showing 17 changed files with 262 additions and 234 deletions.
113 changes: 75 additions & 38 deletions x-pack/plugins/reporting/server/core.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
*/

import * as Rx from 'rxjs';
import { first, map, mapTo } from 'rxjs/operators';
import { first, map, take } from 'rxjs/operators';
import {
BasePath,
ElasticsearchServiceSetup,
Expand Down Expand Up @@ -33,7 +33,8 @@ export interface ReportingInternalSetup {
security?: SecurityPluginSetup;
}

interface ReportingInternalStart {
export interface ReportingInternalStart {
browserDriverFactory: HeadlessChromiumDriverFactory;
enqueueJob: EnqueueJobFn;
esqueue: ESQueueInstance;
savedObjects: SavedObjectsServiceStart;
Expand All @@ -43,33 +44,83 @@ interface ReportingInternalStart {
export class ReportingCore {
private pluginSetupDeps?: ReportingInternalSetup;
private pluginStartDeps?: ReportingInternalStart;
private browserDriverFactory?: HeadlessChromiumDriverFactory;
private readonly pluginSetup$ = new Rx.ReplaySubject<ReportingInternalSetup>();
private readonly pluginStart$ = new Rx.ReplaySubject<ReportingInternalStart>();
private readonly pluginSetup$ = new Rx.ReplaySubject<boolean>(); // observe async background setupDeps and config each are done
private readonly pluginStart$ = new Rx.ReplaySubject<ReportingInternalStart>(); // observe async background startDeps
private exportTypesRegistry = getExportTypesRegistry();
private config?: ReportingConfig;

constructor(private config: ReportingConfig) {}
constructor() {}

public pluginSetup(reportingSetupDeps: ReportingInternalSetup) {
this.pluginSetupDeps = reportingSetupDeps;
this.pluginSetup$.next(reportingSetupDeps);
/*
* Register setupDeps
*/
public pluginSetup(setupDeps: ReportingInternalSetup) {
this.pluginSetup$.next(true); // trigger the observer
this.pluginSetupDeps = setupDeps; // cache
}

public pluginStart(reportingStartDeps: ReportingInternalStart) {
this.pluginStart$.next(reportingStartDeps);
/*
* Register startDeps
*/
public pluginStart(startDeps: ReportingInternalStart) {
this.pluginStart$.next(startDeps); // trigger the observer
this.pluginStartDeps = startDeps; // cache
}

public pluginHasStarted(): Promise<boolean> {
return this.pluginStart$.pipe(first(), mapTo(true)).toPromise();
/*
* Blocks the caller until setup is done
*/
public async pluginSetsUp(): Promise<boolean> {
// use deps and config as a cached resolver
if (this.pluginSetupDeps && this.config) {
return true;
}
return await this.pluginSetup$.pipe(take(2)).toPromise(); // once for pluginSetupDeps (sync) and twice for config (async)
}

public setBrowserDriverFactory(browserDriverFactory: HeadlessChromiumDriverFactory) {
this.browserDriverFactory = browserDriverFactory;
/*
* Blocks the caller until start is done
*/
public async pluginStartsUp(): Promise<boolean> {
return await this.getPluginStartDeps().then(() => true);
}

/*
* Synchronously checks if all async background setup and startup is completed
*/
public pluginIsStarted() {
return this.pluginSetupDeps != null && this.config != null && this.pluginStartDeps != null;
}

/*
* Internal module dependencies
* Allows config to be set in the background
*/
public setConfig(config: ReportingConfig) {
this.config = config;
this.pluginSetup$.next(true);
}

/*
* Gives synchronous access to the config
*/
public getConfig(): ReportingConfig {
if (!this.config) {
throw new Error('Config is not yet initialized');
}
return this.config;
}

/*
* Gives async access to the startDeps
*/
private async getPluginStartDeps() {
if (this.pluginStartDeps) {
return this.pluginStartDeps;
}

return await this.pluginStart$.pipe(first()).toPromise();
}

public getExportTypesRegistry() {
return this.exportTypesRegistry;
}
Expand All @@ -92,37 +143,23 @@ export class ReportingCore {
.toPromise();
}

public getConfig(): ReportingConfig {
return this.config;
}

public getScreenshotsObservable(): ScreenshotsObservableFn {
const { browserDriverFactory } = this;
if (!browserDriverFactory) {
throw new Error(`"browserDriverFactory" dependency hasn't initialized yet`);
}
return screenshotsObservableFactory(this.config.get('capture'), browserDriverFactory);
public async getScreenshotsObservable(): Promise<ScreenshotsObservableFn> {
const config = this.getConfig();
const { browserDriverFactory } = await this.getPluginStartDeps();
return screenshotsObservableFactory(config.get('capture'), browserDriverFactory);
}

/*
* Gives synchronous access to the setupDeps
*/
public getPluginSetupDeps() {
if (!this.pluginSetupDeps) {
throw new Error(`"pluginSetupDeps" dependencies haven't initialized yet`);
}
return this.pluginSetupDeps;
}

/*
* Outside dependencies
*/

private async getPluginStartDeps() {
if (this.pluginStartDeps) {
return this.pluginStartDeps;
}
return await this.pluginStart$.pipe(first()).toPromise();
}

public async getElasticsearchService() {
public getElasticsearchService() {
return this.getPluginSetupDeps().elasticsearch;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,21 +5,23 @@
*/

import nodeCrypto from '@elastic/node-crypto';
import { IUiSettingsClient, ElasticsearchServiceSetup } from 'kibana/server';
// @ts-ignore
import Puid from 'puid';
import sinon from 'sinon';
import { ReportingConfig, ReportingCore } from '../../../';
import { fieldFormats, UI_SETTINGS } from '../../../../../../../src/plugins/data/server';
import {
CSV_QUOTE_VALUES_SETTING,
CSV_SEPARATOR_SETTING,
} from '../../../../../../../src/plugins/share/server';
import { CancellationToken } from '../../../../common';
import { CSV_BOM_CHARS } from '../../../../common/constants';
import { LevelLogger } from '../../../lib';
import { setFieldFormats } from '../../../services';
import { createMockReportingCore } from '../../../test_helpers';
import { JobDocPayloadDiscoverCsv } from '../types';
import { executeJobFactory } from './execute_job';
import {
CSV_SEPARATOR_SETTING,
CSV_QUOTE_VALUES_SETTING,
} from '../../../../../../../src/plugins/share/server';

const delay = (ms: number) => new Promise((resolve) => setTimeout(() => resolve(), ms));

Expand Down Expand Up @@ -48,8 +50,8 @@ describe('CSV Execute Job', function () {

let clusterStub: any;
let configGetStub: any;
let mockReportingConfig: any;
let mockReportingCore: any;
let mockReportingConfig: ReportingConfig;
let mockReportingCore: ReportingCore;
let callAsCurrentUserStub: any;
let cancellationToken: any;

Expand Down Expand Up @@ -78,9 +80,11 @@ describe('CSV Execute Job', function () {
mockReportingConfig = { get: configGetStub, kbnConfig: { get: configGetStub } };

mockReportingCore = await createMockReportingCore(mockReportingConfig);
mockReportingCore.getUiSettingsServiceFactory = () => Promise.resolve(mockUiSettingsClient);
mockReportingCore.getElasticsearchService = () => Promise.resolve(mockElasticsearch);
mockReportingCore.config = mockReportingConfig;
mockReportingCore.getUiSettingsServiceFactory = () =>
Promise.resolve((mockUiSettingsClient as unknown) as IUiSettingsClient);
mockReportingCore.getElasticsearchService = () =>
mockElasticsearch as ElasticsearchServiceSetup;
mockReportingCore.setConfig(mockReportingConfig);

cancellationToken = new CancellationToken();

Expand Down Expand Up @@ -996,7 +1000,8 @@ describe('CSV Execute Job', function () {
let maxSizeReached: boolean;

beforeEach(async function () {
mockReportingCore.getUiSettingsServiceFactory = () => mockUiSettingsClient;
mockReportingCore.getUiSettingsServiceFactory = () =>
Promise.resolve((mockUiSettingsClient as unknown) as IUiSettingsClient);
configGetStub.withArgs('csv', 'maxSizeBytes').returns(18);

callAsCurrentUserStub.onFirstCall().returns({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ export const executeJobFactory: ExecuteJobFactory<ESQueueWorkerExecuteFn<
job: JobDocPayloadDiscoverCsv,
cancellationToken: any
) {
const elasticsearch = await reporting.getElasticsearchService();
const elasticsearch = reporting.getElasticsearchService();
const jobLogger = logger.clone([jobId]);

const {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ export async function generateCsvSearch(
};

const config = reporting.getConfig();
const elasticsearch = await reporting.getElasticsearchService();
const elasticsearch = reporting.getElasticsearchService();
const { callAsCurrentUser } = elasticsearch.legacy.client.asScoped(req);
const callCluster = (...params: [string, object]) => callAsCurrentUser(...params);
const uiSettings = await getUiSettings(uiConfig);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -84,46 +84,16 @@ test(`passes browserTimezone to generatePdf`, async () => {
await executeJob(
'pdfJobId',
getJobDocPayload({
title: 'PDF Params Timezone Test',
relativeUrl: '/app/kibana#/something',
browserTimezone,
headers: encryptedHeaders,
}),
cancellationToken
);

expect(generatePdfObservable.mock.calls).toMatchInlineSnapshot(`
Array [
Array [
LevelLogger {
"_logger": Object {
"get": [MockFunction],
},
"_tags": Array [
"printable_pdf",
"execute",
"pdfJobId",
],
"warning": [Function],
},
undefined,
Array [
"http://localhost:5601/sbp/app/kibana#/something",
],
"UTC",
Object {
"conditions": Object {
"basePath": "/sbp",
"hostname": "localhost",
"port": 5601,
"protocol": "http",
},
"headers": Object {},
},
undefined,
false,
],
]
`);
const tzParam = generatePdfObservable.mock.calls[0][3];
expect(tzParam).toBe('UTC');
});

test(`returns content_type of application/pdf`, async () => {
Expand Down
12 changes: 5 additions & 7 deletions x-pack/plugins/reporting/server/lib/jobs_query.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,9 @@

import { i18n } from '@kbn/i18n';
import { errors as elasticsearchErrors } from 'elasticsearch';
import { ElasticsearchServiceSetup } from 'kibana/server';
import { get } from 'lodash';
import { ReportingCore } from '../';
import { AuthenticatedUser } from '../../../security/server';
import { ReportingConfig } from '../';
import { JobSource } from '../types';

const esErrors = elasticsearchErrors as Record<string, any>;
Expand Down Expand Up @@ -42,11 +41,8 @@ interface CountAggResult {

const getUsername = (user: AuthenticatedUser | null) => (user ? user.username : false);

export function jobsQueryFactory(
config: ReportingConfig,
elasticsearch: ElasticsearchServiceSetup
) {
const index = config.get('index');
export function jobsQueryFactory(reportingCore: ReportingCore) {
const { elasticsearch } = reportingCore.getPluginSetupDeps();
const { callAsInternalUser } = elasticsearch.legacy.client;

function execQuery(queryType: string, body: QueryBody) {
Expand All @@ -60,6 +56,8 @@ export function jobsQueryFactory(
},
};

const config = reportingCore.getConfig();
const index = config.get('index');
const query = {
index: `${index}-*`,
body: Object.assign(defaultBody[queryType] || {}, body),
Expand Down
3 changes: 2 additions & 1 deletion x-pack/plugins/reporting/server/plugin.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

jest.mock('./browsers/install', () => ({
installBrowser: jest.fn().mockImplementation(() => ({
binaryPath$: {
Expand Down Expand Up @@ -62,10 +63,10 @@ describe('Reporting Plugin', () => {
});

it('logs setup issues', async () => {
initContext.config = null;
const plugin = new ReportingPlugin(initContext);
// @ts-ignore overloading error logger
plugin.logger.error = jest.fn();
coreSetup.elasticsearch = null;
plugin.setup(coreSetup, pluginSetup);

await sleep(5);
Expand Down
Loading

0 comments on commit 0228ad7

Please sign in to comment.