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
108 changes: 55 additions & 53 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,65 +44,36 @@ 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>(); // for pluginHasSetup
private readonly pluginStart$ = new Rx.ReplaySubject<ReportingInternalStart>(); // for getPluginStartDeps
private exportTypesRegistry = getExportTypesRegistry();
private config?: ReportingConfig;

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

public pluginSetup(reportingSetupDeps: ReportingInternalSetup) {
this.pluginSetupDeps = reportingSetupDeps;
this.pluginSetup$.next(reportingSetupDeps);
this.pluginSetup$.next(true);
}

public pluginStart(reportingStartDeps: ReportingInternalStart) {
this.pluginStart$.next(reportingStartDeps);
}

public pluginHasStarted(): Promise<boolean> {
return this.pluginStart$.pipe(first(), mapTo(true)).toPromise();
}

public setBrowserDriverFactory(browserDriverFactory: HeadlessChromiumDriverFactory) {
this.browserDriverFactory = browserDriverFactory;
public setConfig(config: ReportingConfig) {
this.config = config;
this.pluginSetup$.next(true);
}

/*
* Internal module dependencies
* High-level module dependencies
*/
public getExportTypesRegistry() {
return this.exportTypesRegistry;
}

public async getEsqueue() {
return (await this.getPluginStartDeps()).esqueue;
}

public async getEnqueueJob() {
return (await this.getPluginStartDeps()).enqueueJob;
}

public async getLicenseInfo() {
const { licensing } = this.getPluginSetupDeps();
return await licensing.license$
.pipe(
map((license) => checkLicense(this.getExportTypesRegistry(), license)),
first()
)
.toPromise();
}

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

public getScreenshotsObservable(): ScreenshotsObservableFn {
const { browserDriverFactory } = this;
if (!browserDriverFactory) {
throw new Error(`"browserDriverFactory" dependency hasn't initialized yet`);
if (!this.config) {
throw new Error('Config is not yet initialized');
}
return screenshotsObservableFactory(this.config.get('capture'), browserDriverFactory);
return this.config;
}

public getPluginSetupDeps() {
Expand All @@ -111,29 +83,59 @@ export class ReportingCore {
return this.pluginSetupDeps;
}

/*
* Outside dependencies
*/

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

public async getElasticsearchService() {
return this.getPluginSetupDeps().elasticsearch;
public async pluginIsSetup(): 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 async pluginIsStarted(): Promise<boolean> {
return await this.getPluginStartDeps().then(() => true);
}

/*
* Downstream dependencies
*/

public getExportTypesRegistry() {
return this.exportTypesRegistry;
}

public async getSavedObjectsClient(fakeRequest: KibanaRequest) {
const { savedObjects } = await this.getPluginStartDeps();
return savedObjects.getScopedClient(fakeRequest) as SavedObjectsClientContract;
}

public async getUiSettingsServiceFactory(savedObjectsClient: SavedObjectsClientContract) {
const { uiSettings: uiSettingsService } = await this.getPluginStartDeps();
const scopedUiSettingsService = uiSettingsService.asScopedToClient(savedObjectsClient);
return scopedUiSettingsService;
public async getEsqueue() {
return (await this.getPluginStartDeps()).esqueue;
}

public async getEnqueueJob() {
return (await this.getPluginStartDeps()).enqueueJob;
}

public async getLicenseInfo() {
const { licensing } = this.getPluginSetupDeps();
return await licensing.license$
.pipe(
map((license) => checkLicense(this.getExportTypesRegistry(), license)),
first()
)
.toPromise();
}

public async getScreenshotsObservable(): Promise<ScreenshotsObservableFn> {
const config = this.getConfig();
const { browserDriverFactory } = await this.getPluginStartDeps();
return screenshotsObservableFactory(config.get('capture'), browserDriverFactory);
}
}
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
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
1 change: 1 addition & 0 deletions 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
Loading