[Reporting] server side code clean up (#106940)

* clean up the enqueue job function * clean up the screenshots observable * clean up authorized user pre routing * clean up get_user * fix download job response handlers * clean up jobs query factory repetition * clean up setup deps made available from plugin.ts * update test for screenshots observable * Revert "clean up setup deps made available from plugin.ts" This reverts commit 91de680. * revert renames * minor rename * fix test after rename Co-authored-by: Kibana Machine <[email protected]>
elastic · Aug 11, 2021 · e4e22ab · e4e22ab
1 parent c0395c9
commit e4e22ab
Show file tree

Hide file tree

Showing 19 changed files with 370 additions and 434 deletions.
diff --git a/x-pack/plugins/reporting/server/core.ts b/x-pack/plugins/reporting/server/core.ts
@@ -29,7 +29,6 @@ import { ReportingConfig, ReportingSetup } from './';
 import { HeadlessChromiumDriverFactory } from './browsers/chromium/driver_factory';
 import { ReportingConfigType } from './config';
 import { checkLicense, getExportTypesRegistry, LevelLogger } from './lib';
-import { screenshotsObservableFactory, ScreenshotsObservableFn } from './lib/screenshots';
 import { ReportingStore } from './lib/store';
 import { ExecuteReportTask, MonitorReportsTask, ReportTaskParams } from './lib/tasks';
 import { ReportingPluginRouter } from './types';
@@ -237,12 +236,6 @@ export class ReportingCore {
       .toPromise();
   }
 
-  public async getScreenshotsObservable(): Promise<ScreenshotsObservableFn> {
-    const config = this.getConfig();
-    const { browserDriverFactory } = await this.getPluginStartDeps();
-    return screenshotsObservableFactory(config.get('capture'), browserDriverFactory);
-  }
-
   public getEnableScreenshotMode() {
     const { screenshotMode } = this.getPluginSetupDeps();
     return screenshotMode.setScreenshotModeEnabled;

diff --git a/x-pack/plugins/reporting/server/export_types/png/lib/generate_png.ts b/x-pack/plugins/reporting/server/export_types/png/lib/generate_png.ts
@@ -11,7 +11,7 @@ import { finalize, map, tap } from 'rxjs/operators';
 import { ReportingCore } from '../../../';
 import { LevelLogger } from '../../../lib';
 import { LayoutParams, PreserveLayout } from '../../../lib/layouts';
-import { ScreenshotResults } from '../../../lib/screenshots';
+import { getScreenshots$, ScreenshotResults } from '../../../lib/screenshots';
 import { ConditionalHeaders } from '../../common';
 
 function getBase64DecodedSize(value: string) {
@@ -24,7 +24,9 @@ function getBase64DecodedSize(value: string) {
 }
 
 export async function generatePngObservableFactory(reporting: ReportingCore) {
-  const getScreenshots = await reporting.getScreenshotsObservable();
+  const config = reporting.getConfig();
+  const captureConfig = config.get('capture');
+  const { browserDriverFactory } = await reporting.getPluginStartDeps();
 
   return function generatePngObservable(
     logger: LevelLogger,
@@ -43,7 +45,7 @@ export async function generatePngObservableFactory(reporting: ReportingCore) {
 
     const apmScreenshots = apmTrans?.startSpan('screenshots_pipeline', 'setup');
     let apmBuffer: typeof apm.currentSpan;
-    const screenshots$ = getScreenshots({
+    const screenshots$ = getScreenshots$(captureConfig, browserDriverFactory, {
       logger,
       urls: [url],
       conditionalHeaders,

diff --git a/x-pack/plugins/reporting/server/export_types/printable_pdf/lib/generate_pdf.ts b/x-pack/plugins/reporting/server/export_types/printable_pdf/lib/generate_pdf.ts
@@ -11,7 +11,7 @@ import { mergeMap } from 'rxjs/operators';
 import { ReportingCore } from '../../../';
 import { LevelLogger } from '../../../lib';
 import { createLayout, LayoutParams } from '../../../lib/layouts';
-import { ScreenshotResults } from '../../../lib/screenshots';
+import { getScreenshots$, ScreenshotResults } from '../../../lib/screenshots';
 import { ConditionalHeaders } from '../../common';
 import { PdfMaker } from './pdf';
 import { getTracker } from './tracker';
@@ -29,7 +29,7 @@ const getTimeRange = (urlScreenshots: ScreenshotResults[]) => {
 export async function generatePdfObservableFactory(reporting: ReportingCore) {
   const config = reporting.getConfig();
   const captureConfig = config.get('capture');
-  const getScreenshots = await reporting.getScreenshotsObservable();
+  const { browserDriverFactory } = await reporting.getPluginStartDeps();
 
   return function generatePdfObservable(
     logger: LevelLogger,
@@ -48,7 +48,7 @@ export async function generatePdfObservableFactory(reporting: ReportingCore) {
     tracker.endLayout();
 
     tracker.startScreenshots();
-    const screenshots$ = getScreenshots({
+    const screenshots$ = getScreenshots$(captureConfig, browserDriverFactory, {
       logger,
       urls,
       conditionalHeaders,

diff --git a/x-pack/plugins/reporting/server/lib/enqueue_job.test.ts b/x-pack/plugins/reporting/server/lib/enqueue_job.test.ts
@@ -16,7 +16,7 @@ import {
 } from '../test_helpers';
 import { ReportingRequestHandlerContext } from '../types';
 import { ExportTypesRegistry, ReportingStore } from './';
-import { enqueueJobFactory } from './enqueue_job';
+import { enqueueJob } from './enqueue_job';
 import { Report } from './store';
 
 describe('Enqueue Job', () => {
@@ -72,13 +72,14 @@ describe('Enqueue Job', () => {
   });
 
   it('returns a Report object', async () => {
-    const enqueueJob = enqueueJobFactory(mockReporting, logger);
     const report = await enqueueJob(
+      mockReporting,
+      ({} as unknown) as KibanaRequest,
+      ({} as unknown) as ReportingRequestHandlerContext,
+      false,
       'printablePdf',
       mockBaseParams,
-      false,
-      ({} as unknown) as ReportingRequestHandlerContext,
-      ({} as unknown) as KibanaRequest
+      logger
     );
 
     const { _id, created_at: _created_at, ...snapObj } = report;
@@ -117,14 +118,15 @@ describe('Enqueue Job', () => {
   });
 
   it('provides a default kibana version field for older POST URLs', async () => {
-    const enqueueJob = enqueueJobFactory(mockReporting, logger);
     mockBaseParams.version = undefined;
     const report = await enqueueJob(
+      mockReporting,
+      ({} as unknown) as KibanaRequest,
+      ({} as unknown) as ReportingRequestHandlerContext,
+      false,
       'printablePdf',
       mockBaseParams,
-      false,
-      ({} as unknown) as ReportingRequestHandlerContext,
-      ({} as unknown) as KibanaRequest
+      logger
     );
 
     const { _id, created_at: _created_at, ...snapObj } = report;

diff --git a/x-pack/plugins/reporting/server/lib/enqueue_job.ts b/x-pack/plugins/reporting/server/lib/enqueue_job.ts
@@ -12,64 +12,53 @@ import { BaseParams, ReportingUser } from '../types';
 import { checkParamsVersion, LevelLogger } from './';
 import { Report } from './store';
 
-export type EnqueueJobFn = (
+export async function enqueueJob(
+  reporting: ReportingCore,
+  request: KibanaRequest,
+  context: ReportingRequestHandlerContext,
+  user: ReportingUser,
   exportTypeId: string,
   jobParams: BaseParams,
-  user: ReportingUser,
-  context: ReportingRequestHandlerContext,
-  request: KibanaRequest
-) => Promise<Report>;
-
-export function enqueueJobFactory(
-  reporting: ReportingCore,
   parentLogger: LevelLogger
-): EnqueueJobFn {
+): Promise<Report> {
   const logger = parentLogger.clone(['createJob']);
-  return async function enqueueJob(
-    exportTypeId: string,
-    jobParams: BaseParams,
-    user: ReportingUser,
-    context: ReportingRequestHandlerContext,
-    request: KibanaRequest
-  ) {
-    const exportType = reporting.getExportTypesRegistry().getById(exportTypeId);
+  const exportType = reporting.getExportTypesRegistry().getById(exportTypeId);
 
-    if (exportType == null) {
-      throw new Error(`Export type ${exportTypeId} does not exist in the registry!`);
-    }
+  if (exportType == null) {
+    throw new Error(`Export type ${exportTypeId} does not exist in the registry!`);
+  }
 
-    if (!exportType.createJobFnFactory) {
-      throw new Error(`Export type ${exportTypeId} is not an async job type!`);
-    }
+  if (!exportType.createJobFnFactory) {
+    throw new Error(`Export type ${exportTypeId} is not an async job type!`);
+  }
 
-    const [createJob, store] = await Promise.all([
-      exportType.createJobFnFactory(reporting, logger.clone([exportType.id])),
-      reporting.getStore(),
-    ]);
+  const [createJob, store] = await Promise.all([
+    exportType.createJobFnFactory(reporting, logger.clone([exportType.id])),
+    reporting.getStore(),
+  ]);
 
-    jobParams.version = checkParamsVersion(jobParams, logger);
-    const job = await createJob!(jobParams, context, request);
+  jobParams.version = checkParamsVersion(jobParams, logger);
+  const job = await createJob!(jobParams, context, request);
 
-    // 1. Add the report to ReportingStore to show as pending
-    const report = await store.addReport(
-      new Report({
-        jobtype: exportType.jobType,
-        created_by: user ? user.username : false,
-        payload: job,
-        meta: {
-          objectType: jobParams.objectType,
-          layout: jobParams.layout?.id,
-        },
-      })
-    );
-    logger.debug(`Successfully stored pending job: ${report._index}/${report._id}`);
+  // 1. Add the report to ReportingStore to show as pending
+  const report = await store.addReport(
+    new Report({
+      jobtype: exportType.jobType,
+      created_by: user ? user.username : false,
+      payload: job,
+      meta: {
+        objectType: jobParams.objectType,
+        layout: jobParams.layout?.id,
+      },
+    })
+  );
+  logger.debug(`Successfully stored pending job: ${report._index}/${report._id}`);
 
-    // 2. Schedule the report with Task Manager
-    const task = await reporting.scheduleTask(report.toReportTaskJSON());
-    logger.info(
-      `Scheduled ${exportType.name} reporting task. Task ID: task:${task.id}. Report ID: ${report._id}`
-    );
+  // 2. Schedule the report with Task Manager
+  const task = await reporting.scheduleTask(report.toReportTaskJSON());
+  logger.info(
+    `Scheduled ${exportType.name} reporting task. Task ID: task:${task.id}. Report ID: ${report._id}`
+  );
 
-    return report;
-  };
+  return report;
 }
diff --git a/x-pack/plugins/reporting/server/lib/screenshots/index.ts b/x-pack/plugins/reporting/server/lib/screenshots/index.ts
@@ -5,12 +5,11 @@
  * 2.0.
  */
 
-import * as Rx from 'rxjs';
 import { LevelLogger } from '../';
 import { ConditionalHeaders } from '../../export_types/common';
 import { LayoutInstance } from '../layouts';
 
-export { screenshotsObservableFactory } from './observable';
+export { getScreenshots$ } from './observable';
 
 export interface ScreenshotObservableOpts {
   logger: LevelLogger;
@@ -55,11 +54,3 @@ export interface ScreenshotResults {
   error?: Error;
   elementsPositionAndAttributes?: ElementsPositionAndAttribute[]; // NOTE: for testing
 }
-
-export type ScreenshotsObservableFn = ({
-  logger,
-  urls,
-  conditionalHeaders,
-  layout,
-  browserTimezone,
-}: ScreenshotObservableOpts) => Rx.Observable<ScreenshotResults[]>;
diff --git a/x-pack/plugins/reporting/server/lib/screenshots/observable.test.ts b/x-pack/plugins/reporting/server/lib/screenshots/observable.test.ts
@@ -32,7 +32,7 @@ import {
 } from '../../test_helpers';
 import { ElementsPositionAndAttribute } from './';
 import * as contexts from './constants';
-import { screenshotsObservableFactory } from './observable';
+import { getScreenshots$ } from './';
 
 /*
  * Mocks
@@ -67,8 +67,7 @@ describe('Screenshot Observable Pipeline', () => {
   });
 
   it('pipelines a single url into screenshot and timeRange', async () => {
-    const getScreenshots$ = screenshotsObservableFactory(captureConfig, mockBrowserDriverFactory);
-    const result = await getScreenshots$({
+    const result = await getScreenshots$(captureConfig, mockBrowserDriverFactory, {
       logger,
       urls: ['/welcome/home/start/index.htm'],
       conditionalHeaders: {} as ConditionalHeaders,
@@ -128,8 +127,7 @@ describe('Screenshot Observable Pipeline', () => {
     });
 
     // test
-    const getScreenshots$ = screenshotsObservableFactory(captureConfig, mockBrowserDriverFactory);
-    const result = await getScreenshots$({
+    const result = await getScreenshots$(captureConfig, mockBrowserDriverFactory, {
       logger,
       urls: ['/welcome/home/start/index2.htm', '/welcome/home/start/index.php3?page=./home.php'],
       conditionalHeaders: {} as ConditionalHeaders,
@@ -227,9 +225,8 @@ describe('Screenshot Observable Pipeline', () => {
       });
 
       // test
-      const getScreenshots$ = screenshotsObservableFactory(captureConfig, mockBrowserDriverFactory);
       const getScreenshot = async () => {
-        return await getScreenshots$({
+        return await getScreenshots$(captureConfig, mockBrowserDriverFactory, {
           logger,
           urls: [
             '/welcome/home/start/index2.htm',
@@ -322,9 +319,8 @@ describe('Screenshot Observable Pipeline', () => {
       });
 
       // test
-      const getScreenshots$ = screenshotsObservableFactory(captureConfig, mockBrowserDriverFactory);
       const getScreenshot = async () => {
-        return await getScreenshots$({
+        return await getScreenshots$(captureConfig, mockBrowserDriverFactory, {
           logger,
           urls: ['/welcome/home/start/index.php3?page=./home.php3'],
           conditionalHeaders: {} as ConditionalHeaders,
@@ -354,50 +350,46 @@ describe('Screenshot Observable Pipeline', () => {
       });
       mockLayout.getViewport = () => null;
 
-      // test
-      const getScreenshots$ = screenshotsObservableFactory(captureConfig, mockBrowserDriverFactory);
-      const getScreenshot = async () => {
-        return await getScreenshots$({
-          logger,
-          urls: ['/welcome/home/start/index.php3?page=./home.php3'],
-          conditionalHeaders: {} as ConditionalHeaders,
-          layout: mockLayout,
-          browserTimezone: 'UTC',
-        }).toPromise();
-      };
+      const screenshots = await getScreenshots$(captureConfig, mockBrowserDriverFactory, {
+        logger,
+        urls: ['/welcome/home/start/index.php3?page=./home.php3'],
+        conditionalHeaders: {} as ConditionalHeaders,
+        layout: mockLayout,
+        browserTimezone: 'UTC',
+      }).toPromise();
 
-      await expect(getScreenshot()).resolves.toMatchInlineSnapshot(`
-              Array [
-                Object {
-                  "elementsPositionAndAttributes": Array [
-                    Object {
-                      "attributes": Object {},
-                      "position": Object {
-                        "boundingClientRect": Object {
-                          "height": 1200,
-                          "left": 0,
-                          "top": 0,
-                          "width": 1800,
-                        },
-                        "scroll": Object {
-                          "x": 0,
-                          "y": 0,
-                        },
-                      },
-                    },
-                  ],
-                  "error": undefined,
-                  "screenshots": Array [
-                    Object {
-                      "base64EncodedData": "allyourBase64",
-                      "description": undefined,
-                      "title": undefined,
-                    },
-                  ],
-                  "timeRange": undefined,
+      expect(screenshots).toMatchInlineSnapshot(`
+        Array [
+          Object {
+            "elementsPositionAndAttributes": Array [
+              Object {
+                "attributes": Object {},
+                "position": Object {
+                  "boundingClientRect": Object {
+                    "height": 1200,
+                    "left": 0,
+                    "top": 0,
+                    "width": 1800,
+                  },
+                  "scroll": Object {
+                    "x": 0,
+                    "y": 0,
+                  },
                 },
-              ]
-            `);
+              },
+            ],
+            "error": undefined,
+            "screenshots": Array [
+              Object {
+                "base64EncodedData": "allyourBase64",
+                "description": undefined,
+                "title": undefined,
+              },
+            ],
+            "timeRange": undefined,
+          },
+        ]
+      `);
     });
   });
 });