Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Reporting/Server] register plugin routes synchronously #68976

Merged
merged 15 commits into from
Jun 15, 2020
Merged
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;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moved browserDriverFactory to be a plugin start dep.

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);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Full "plugin setup" now depends on 2 things: the pluginSetup deps, and the async config. That is why this.pluginSetup$.next is called twice in this file.

}

/*
* 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 @@ -995,7 +999,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();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm considering deleting this helper method entirely since the dep can come from getPluginSetupDeps

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

++ on always decreasing API surface area

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will leave it in for now, since it allows us to mock the elasticsearch service pretty well.

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();
Copy link
Member Author

@tsullivan tsullivan Jun 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lazily resolving the config off of reportingCore when needed.

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
Loading