[Reporting/Server] register plugin routes synchronously (elastic#68976)

* 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
tsullivan · Jun 15, 2020 · 4717af8 · 4717af8
1 parent 86aa4f2
commit 4717af8
Show file tree

Hide file tree

Showing 17 changed files with 262 additions and 234 deletions.
diff --git a/x-pack/plugins/reporting/server/core.ts b/x-pack/plugins/reporting/server/core.ts
@@ -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,
@@ -33,7 +33,8 @@ export interface ReportingInternalSetup {
   security?: SecurityPluginSetup;
 }
 
-interface ReportingInternalStart {
+export interface ReportingInternalStart {
+  browserDriverFactory: HeadlessChromiumDriverFactory;
   enqueueJob: EnqueueJobFn;
   esqueue: ESQueueInstance;
   savedObjects: SavedObjectsServiceStart;
@@ -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;
   }
@@ -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;
   }
 

diff --git a/x-pack/plugins/reporting/server/export_types/csv/server/execute_job.test.ts b/x-pack/plugins/reporting/server/export_types/csv/server/execute_job.test.ts
@@ -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));
 
@@ -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;
 
@@ -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();
 
@@ -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({

diff --git a/x-pack/plugins/reporting/server/export_types/csv/server/execute_job.ts b/x-pack/plugins/reporting/server/export_types/csv/server/execute_job.ts
@@ -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 {

diff --git a/...gins/reporting/server/export_types/csv_from_savedobject/server/lib/generate_csv_search.ts b/...gins/reporting/server/export_types/csv_from_savedobject/server/lib/generate_csv_search.ts
@@ -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);

diff --git a/x-pack/plugins/reporting/server/export_types/printable_pdf/server/execute_job/index.test.ts b/x-pack/plugins/reporting/server/export_types/printable_pdf/server/execute_job/index.test.ts
@@ -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 () => {

diff --git a/x-pack/plugins/reporting/server/lib/jobs_query.ts b/x-pack/plugins/reporting/server/lib/jobs_query.ts
@@ -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>;
@@ -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) {
@@ -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),

diff --git a/x-pack/plugins/reporting/server/plugin.test.ts b/x-pack/plugins/reporting/server/plugin.test.ts
@@ -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$: {
@@ -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);