[Reporting/NP Migration] Remove server.expose of ExportTypeRegistry #50973

tsullivan · 2019-11-18T21:24:42Z

Summary

This PR prepares the Reporting plugin for New Platform further, by taking away the export type registry from being "exposed" on the server. Previously, this was done as a utility to access a singleton. Now, the code follows the traditional flow of passing down the object to modules that depend on it.

Some of the modules that depend on export type registry were loaded with oncePerServer, which made the exported modules forced to be functions that could only receive the server object as a parameter. Since that doesn't work with the main idea behind the code change, there are unrelated modules that had to change by no longer being loaded with oncePerServer.

This PR is related to the dependent PRs:

The main changes of this PR is to pass the browserDriverFactory and exportTypesRegistry objects to the module that need them, instead of making those objects available throughout the plugin using server.expose.

Other cleanup-related work:

replaced all the export_type/server/index.ts files with export_type/index.ts. The new changes expose an export type definition object, which are imported by the export type registry class, and consumed in its getExportTypesRegistryFn.
- Why: it was a weird pattern to pass a register function into a server function to have the export defs registered.
removed common/export_types_registry and moved its code to server/lib/export_types_registry
- Why: previously the server/lib registry code would scan a directory for available export types. Now each export type definition is explicitly imported. The scan code is no longer needed and the registry code should be consolidated in server/lib/

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

~~[ ] This was checked for cross-browser compatibility, including a check against IE11~~
~~[ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support~~
~~[ ] Documentation was added for features that require explanation or tutorials~~
Unit or functional tests were updated or added to match the most common scenarios
~~[ ] This was checked for keyboard-only and screenreader accessibility~~

elasticmachine · 2019-11-18T21:34:55Z

Pinging @elastic/kibana-stack-services (Team:Stack Services)

… server.expose

tsullivan · 2019-12-03T21:31:05Z

x-pack/legacy/plugins/reporting/common/export_types_registry.js

@@ -1,64 +0,0 @@
-/*


Moved this out to replace server/lib/export_types_registry

tsullivan · 2019-12-03T21:31:57Z

x-pack/legacy/plugins/reporting/export_types/csv/server/index.ts

@@ -1,22 +0,0 @@
-/*


replaced all the export_type/server/index.ts files with export_type/index.ts

tsullivan · 2019-12-03T21:47:42Z

x-pack/legacy/plugins/reporting/server/lib/export_types_registry.ts

 }

-export const exportTypesRegistryFactory = oncePerServer(exportTypesRegistryFn);
+// FIXME: is this the best way to return a singleton?


Maybe there is no need to memoize this, since exportTypesRegistry is consumed in 1 place

Why not instanciate ExportTypesRegistry in the module and export the singleton then? Maybe a little harder for tests though.

x-pack/legacy/plugins/reporting/export_types/printable_pdf/server/lib/generate_pdf.ts

x-pack/legacy/plugins/reporting/export_types/csv/index.ts

…unctions.

elasticmachine · 2019-12-05T01:06:59Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 226a34b

…option

elasticmachine · 2019-12-05T21:02:07Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: a50757d

elasticmachine · 2019-12-05T23:17:48Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 7870e26

tsullivan · 2019-12-09T17:24:00Z

@elasticmachine merge upstream

…tform-shim-part-iv

tsullivan · 2019-12-10T16:36:21Z

@elasticmachine merge upstream

joelgriffith · 2019-12-10T16:48:46Z

x-pack/legacy/plugins/reporting/common/constants.ts

@@ -50,3 +50,9 @@ export const PNG_JOB_TYPE = 'PNG';
 export const CSV_JOB_TYPE = 'csv';
 export const CSV_FROM_SAVEDOBJECT_JOB_TYPE = 'csv_from_savedobject';
 export const USES_HEADLESS_JOB_TYPES = [PDF_JOB_TYPE, PNG_JOB_TYPE];
+


elasticmachine · 2019-12-10T17:51:51Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: c8fb7cd

History

💚 Build #14297 succeeded 716cf4c

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

tsullivan · 2019-12-10T21:32:09Z

@elastic/kibana-platform would you be able to lend someone to give a 2nd review on this PR?

As it has a lot of refactoring, there is some context required, but the main part of the changes are to support getting rid of our server.expose usage.

pgayvallet

Don't have all the functional context, but PR does what is described and it seems to be going in the right direction for NP migration.

Some comments:

pgayvallet · 2019-12-11T15:11:10Z

x-pack/legacy/plugins/reporting/common/constants.ts

+export const LICENSE_TYPE_TRIAL = 'trial';
+export const LICENSE_TYPE_BASIC = 'basic';
+export const LICENSE_TYPE_STANDARD = 'standard';
+export const LICENSE_TYPE_GOLD = 'gold';
+export const LICENSE_TYPE_PLATINUM = 'platinum';


Should probably use LicenseType from x-pack/plugins/licensing/server instead or redeclaring them

pgayvallet · 2019-12-11T15:16:18Z

x-pack/legacy/plugins/reporting/server/lib/export_types_registry.ts

+// => ExportTypeDefinition<T, U, V, W>
+
+export class ExportTypesRegistry {
+  private _map: Map<string, ExportTypeDefinition<any, any, any, any>> = new Map();


NIT: should probably be unknown instead of any

pgayvallet · 2019-12-11T15:20:29Z

x-pack/legacy/plugins/reporting/server/lib/export_types_registry.ts

 }

-export const exportTypesRegistryFactory = oncePerServer(exportTypesRegistryFn);
+// FIXME: is this the best way to return a singleton?


Why not instanciate ExportTypesRegistry in the module and export the singleton then? Maybe a little harder for tests though.

pgayvallet · 2019-12-11T15:25:08Z

x-pack/legacy/plugins/reporting/server/routes/generation.ts

+  const config = server.config();
+  const DOWNLOAD_BASE_URL = config.get('server.basePath') + `${API_BASE_URL}/jobs/download`;


You could access this from NP with core.http.basePath.serverBasePath instead of relying on the legacy config.

pgayvallet · 2019-12-11T15:28:56Z

x-pack/legacy/plugins/reporting/server/routes/generation.ts

+  function handleError(exportTypeId: string, err: Error) {
+    if (err instanceof esErrors['401']) {
+      return boom.unauthorized(`Sorry, you aren't authenticated`);
+    }
+    if (err instanceof esErrors['403']) {
+      return boom.forbidden(`Sorry, you are not authorized to create ${exportTypeId} reports`);
+    }
+    if (err instanceof esErrors['404']) {
+      return boom.boomify(err, { statusCode: 404 });
+    }
+    return err;
+  }


Info for later: Boom errors are not handled by default by NP router. You will have to user router.handleLegacyErrors when you'll migrate your routes (or use proper NP response errors)

tsullivan · 2019-12-11T19:11:46Z

Thanks for the insightful feedback, @pgayvallet ! I think your comments should come in as changes in forthcoming PRs

…lastic#50973) * [Reporting/NPMigration] typescriptify ExportTypeRegistry, remove from server.expose * Minor routes registration cleanup * move the ETR test file * Re-pack the route registration, reduce LOC changes * add EnqueueJobFn type * Fix usage collector test * remove a throw error used for development/debugging * fix imports error * Fix execute job tests * wip test fixes * test fixes for real * fix more tests * fix diffs * Add TODOs about the ExportTypesRegistry.register unwrap the factory functions. * really make headlessbrowserdriver required as an execute job factory option * fix tests * Use constants for license type keywords

…50973) (#52813) * [Reporting/NPMigration] typescriptify ExportTypeRegistry, remove from server.expose * Minor routes registration cleanup * move the ETR test file * Re-pack the route registration, reduce LOC changes * add EnqueueJobFn type * Fix usage collector test * remove a throw error used for development/debugging * fix imports error * Fix execute job tests * wip test fixes * test fixes for real * fix more tests * fix diffs * Add TODOs about the ExportTypesRegistry.register unwrap the factory functions. * really make headlessbrowserdriver required as an execute job factory option * fix tests * Use constants for license type keywords

tsullivan · 2020-01-03T23:41:05Z

#53898

tsullivan added Feature:NP Migration (Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead release_note:skip Skip the PR/issue when compiling release notes Team:Stack Services labels Nov 18, 2019

tsullivan added v7.6.0 v8.0.0 labels Nov 18, 2019

This comment has been minimized.

Sign in to view

tsullivan changed the title ~~[Reporting] Remove server.expose of ExportTypeRegistry~~ [WIP] [Reporting] Remove server.expose of ExportTypeRegistry Nov 18, 2019

This was referenced Nov 19, 2019

[Reporting/np-k8] Remove several oncePerServer usages #50997

Merged

[Reporting] Convert Export Type Definitions to Typescript #51643

Merged

tsullivan force-pushed the reporting/new-platform-shim-part-iv branch 5 times, most recently from 0365826 to 47efc80 Compare November 27, 2019 04:05

This comment has been minimized.

Sign in to view

tsullivan force-pushed the reporting/new-platform-shim-part-iv branch from 47efc80 to 53320f1 Compare December 2, 2019 19:30

This comment has been minimized.

Sign in to view

tsullivan force-pushed the reporting/new-platform-shim-part-iv branch from 53320f1 to 44166c6 Compare December 3, 2019 00:26

This comment has been minimized.

Sign in to view

tsullivan force-pushed the reporting/new-platform-shim-part-iv branch from 0d7f588 to 446a9f8 Compare December 3, 2019 19:45

This comment has been minimized.

Sign in to view

[Reporting/NPMigration] typescriptify ExportTypeRegistry, remove from…

723ab00

… server.expose

tsullivan force-pushed the reporting/new-platform-shim-part-iv branch from 446a9f8 to 723ab00 Compare December 3, 2019 21:28

tsullivan changed the title ~~[WIP] [Reporting] Remove server.expose of ExportTypeRegistry~~ [Reporting/NP Migration] Remove server.expose of ExportTypeRegistry Dec 3, 2019

tsullivan commented Dec 3, 2019

View reviewed changes

Minor routes registration cleanup

ed80fea

joelgriffith reviewed Dec 4, 2019

View reviewed changes

x-pack/legacy/plugins/reporting/export_types/printable_pdf/server/lib/generate_pdf.ts Show resolved Hide resolved

x-pack/legacy/plugins/reporting/export_types/csv/index.ts Outdated Show resolved Hide resolved

tsullivan added 2 commits December 4, 2019 15:54

Merge branch 'master' into reporting/new-platform-shim-part-iv

2482657

Add TODOs about the ExportTypesRegistry.register unwrap the factory f…

226a34b

…unctions.

really make headlessbrowserdriver required as an execute job factory …

c411021

…option

This comment has been minimized.

Sign in to view

fix tests

a50757d

tsullivan requested a review from joelgriffith December 5, 2019 21:44

Use constants for license type keywords

7870e26

Merge remote-tracking branch 'upstream/master' into reporting/new-pla…

716cf4c

…tform-shim-part-iv

tsullivan requested a review from Bargs December 10, 2019 00:30

Merge branch 'master' into reporting/new-platform-shim-part-iv

c8fb7cd

joelgriffith reviewed Dec 10, 2019

View reviewed changes

joelgriffith approved these changes Dec 10, 2019

View reviewed changes

tsullivan requested a review from a team December 10, 2019 21:31

pgayvallet approved these changes Dec 11, 2019

View reviewed changes

tsullivan merged commit 711b44b into elastic:master Dec 11, 2019

tsullivan deleted the reporting/new-platform-shim-part-iv branch December 11, 2019 19:42

tsullivan mentioned this pull request Dec 11, 2019

[7.x] [Reporting/NP Migration] Remove server.expose of ExportTypeRegistry (#50973) #52813

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Reporting/NP Migration] Remove server.expose of ExportTypeRegistry #50973

[Reporting/NP Migration] Remove server.expose of ExportTypeRegistry #50973

tsullivan commented Nov 18, 2019 •

edited

Loading

elasticmachine commented Nov 18, 2019

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

tsullivan Dec 3, 2019

tsullivan Dec 3, 2019

tsullivan Dec 3, 2019

pgayvallet Dec 11, 2019

elasticmachine commented Dec 5, 2019

This comment has been minimized.

elasticmachine commented Dec 5, 2019

elasticmachine commented Dec 5, 2019

tsullivan commented Dec 9, 2019

tsullivan commented Dec 10, 2019

joelgriffith Dec 10, 2019

elasticmachine commented Dec 10, 2019

tsullivan commented Dec 10, 2019

pgayvallet left a comment

pgayvallet Dec 11, 2019

pgayvallet Dec 11, 2019

pgayvallet Dec 11, 2019

pgayvallet Dec 11, 2019

pgayvallet Dec 11, 2019

tsullivan commented Dec 11, 2019

tsullivan commented Jan 3, 2020

		const config = server.config();
		const DOWNLOAD_BASE_URL = config.get('server.basePath') + `${API_BASE_URL}/jobs/download`;

[Reporting/NP Migration] Remove server.expose of ExportTypeRegistry #50973

[Reporting/NP Migration] Remove server.expose of ExportTypeRegistry #50973

Conversation

tsullivan commented Nov 18, 2019 • edited Loading

Summary

Checklist

elasticmachine commented Nov 18, 2019

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Dec 5, 2019

💚 Build Succeeded

This comment has been minimized.

elasticmachine commented Dec 5, 2019

💚 Build Succeeded

elasticmachine commented Dec 5, 2019

💚 Build Succeeded

tsullivan commented Dec 9, 2019

tsullivan commented Dec 10, 2019

Choose a reason for hiding this comment

elasticmachine commented Dec 10, 2019

💚 Build Succeeded

History

tsullivan commented Dec 10, 2019

pgayvallet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsullivan commented Dec 11, 2019

tsullivan commented Jan 3, 2020

tsullivan commented Nov 18, 2019 •

edited

Loading