[Reporting] Define shims of legacy dependencies by tsullivan · Pull Request #54082 · elastic/kibana

tsullivan · 2020-01-07T00:31:05Z

Summary

Part of #53898

This PR defines serverFacade and requestFacade objects that use bare-bones types of the legacy dependencies

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~~

For maintainers

This was checked for breaking API changes and was labeled appropriately
This includes a feature addition or change that requires a release note and was labeled appropriately

elasticmachine · 2020-01-07T00:31:08Z

Pinging @elastic/kibana-reporting-services (Team:Reporting Services)

tsullivan · 2020-01-07T00:40:43Z

x-pack/legacy/plugins/reporting/types.d.ts

+  headers: Legacy.Request['headers'];
+  params: Legacy.Request['params'];
+  payload: JobParamPostPayload | GenerateExportTypePayload;
+  query: ListQuery & GenerateQuery;


not sure why this isn't ListQuery | GenerateQuery

tsullivan · 2020-01-07T17:15:32Z

@elasticmachine merge upstream

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

tsullivan · 2020-01-07T21:04:53Z

@elasticmachine merge upstream

tsullivan · 2020-01-14T21:46:37Z

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

+import { ServerFacade } from '../../types';
+
+export function getUserFactory(server: ServerFacade) {
+  return async (request: Legacy.Request) => {


This function takes a Legacy request because it is called from middleware. That means we get into this code before we get into any request handlers which is where we pick the properties for RequestFacade.

@pgayvallet @restrry this means that I don't actually need to call request.getRawRequest - doing so made all the code break since this isn't an actual RequestFacade :)

ddf8fac added that method back, but for a different usage

Just thinking loudly: why not call this function from a route handler? NP routing doesn't provide a pre-middleware mechanism at the moment.

NP routing doesn't provide a pre-middleware mechanism at the moment.

Hm, if that is the case it will be a major blocker for Reporting migration.

@rudolf can you confirm whether we will be able to register routing middleware in the new platform?

@rudolf can you confirm whether we will be able to register routing middleware in the new platform?

ATM this is not something we are planing to provide.

Hm, if that is the case it will be a major blocker for Reporting migration.

Correct me if I'm wrong, but the whole pre-routing mechanism can easily be replaced by some wrapper around the actual route handlers. The usage would be really close to what you currently have and should not be that big of a change

Looking at your getRouteConfigFactoryReportingPre method and usages in x-pack/legacy/plugins/reporting/server/routes/generate_from_jobparams.ts, the pre-routing can be moved to an higher level. This would be something similar as what we are going in src/core/server/http/router/error_wrapper.ts

const getRouteConfig = () => { const getOriginalRouteConfig: GetRouteConfigFactoryFn = getRouteConfigFactoryReportingPre( server ); const routeConfigFactory: RouteConfigFactory = getOriginalRouteConfig( ({ params: { exportType } }) => exportType ); return { ...routeConfigFactory, validate: { params: Joi.object({ [...] }, }; }; // generate report server.route({ path: `${BASE_GENERATE}/{exportType}`, method: 'POST', options: getRouteConfig(), handler: async (legacyRequest: Legacy.Request, h: ReportingResponseToolkit) => { [...] return response; }, });

would become something like (also adapted to NP router format)

const getRouteConfig = () => { const getOriginalRouteConfig: GetRouteConfigFactoryFn = getRouteConfigFactoryReportingPre( server ); const routeConfigFactory: RouteConfigFactory = getOriginalRouteConfig( ({ params: { exportType } }) => exportType ); return { ...routeConfigFactory, validate: { params: schema.object({ [...] }, }; }; const routeConfig = getRouteConfig() router.post( { path: `${BASE_GENERATE}/{exportType}`, options: routeConfig.options validate: routeConfig.validate, }, routeConfig.performPreAuth(async (ctx, req, res, preAuthResult) => { const request = makeRequestFacade(legacyRequest, preAuthResults); .... }) );

The performPreAuth generated from getRouteConfigFactoryReportingPre would need some adaption from the existing code to execute all pre-handlers and either returns a response error if one throws or execute the underlying handler with an additional parameter containing the resolved objects from the pre handlers (basically what HAPI was doing itself before), but that doesn't sound like big changes, the actual pre-hook handlers would remains unchanged.

Please ping me if you want me to go a little more in-details with that.

I'm ++ on any solution that will allow us to move further, and I don't see any problems with your proposals. I think there are a lot of ways to achieve the goal that the code is doing and pre-routing is just one of them.

tsullivan · 2020-01-15T21:36:12Z

@elasticmachine merge upstream

tsullivan · 2020-01-16T16:32:46Z

@elasticmachine merge upstream

mshustov · 2020-01-16T18:12:06Z

x-pack/legacy/plugins/reporting/types.d.ts

+  management: {
+    jobTypes: any;
+  };
+  user: any;


btw Security exports AuthenticatedUser type

kibana/x-pack/plugins/security/server/index.ts

Line 28 in 02344aa

export { AuthenticatedUser } from '../common/model';

I'm approve of this message

Hm, when I tried that, I get an ESLint error:
@kbn/eslint/no-restricted-paths: Unexpected path "../../../plugins/security/server" imported in restricted zone. Server modules cannot be imported into client modules or shared modules.

joelgriffith · 2020-01-16T19:21:49Z

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

      return await server.plugins.security.getUser(request);
    } catch (err) {
-      server.log(['reporting', 'getUser', 'debug'], err);
+      server.log(['reporting', 'getUser', 'error'], err);


Nice catch here.

joelgriffith · 2020-01-16T19:22:13Z

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

      throw new TypeError('This function expects to be called with a single argument');
    }

-    if (!server || typeof server.expose !== 'function') {


joelgriffith · 2020-01-16T19:23:10Z

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

    options: getRouteConfig(),
-    handler: async (request: RequestFacade, h: ReportingResponseToolkit) => {
+    handler: async (originalRequest: Legacy.Request, h: ReportingResponseToolkit) => {
+      const request = makeRequestFacade(originalRequest);


Do we want to keep with the legacy naming convention here? originalRequest to legacyRequest?

joelgriffith

LGTM! Saw the note below on Type's we can use for the user, which I'm a +1 on.

kibanamachine · 2020-01-16T21:54:29Z

💚 Build Succeeded

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

History

💚 Build #20877 succeeded 9d446a1
💔 Build #20746 failed b4dcefb
💔 Build #20707 failed ddf8fac
💔 Build #20356 failed 15ab24b
💔 Build #20343 failed cb40fa3

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

* simplify serverfacade definition * simplify requestfacade definition * use the shim * makeRequestFacade * requestFacade * import sorting * originalServer * reduce loc change * remove consolelog * hacks to fix tests * ServerFacade in index * Cosmetic * remove field from serverfacade * add raw to the request * fix types * add fieldFormatServiceFactory to legacy * Pass the complete request object to sec plugin * Fix test * fix test 2 * getUser takes a legacy request * add unit test for new lib * add getRawRequest to pass to saved objects method * update test snapshot * leave a TODO comment for type import * variable rename for legacy id Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

…t-of-legacy * 'master' of github.com:elastic/kibana: (142 commits) [Vis] Move Timelion Vis to vis_type_timelion (elastic#52069) Deprecate `chrome.navlinks.update` and add documentation (elastic#54893) [ML] Single Metric Viewer: Fix time bounds with custom strings. (elastic#55045) [Vis: Default editor] EUIficate and Reactify the sidebar (elastic#49864) [Mappings editor] Fix cannot set boolean value for "null_value" param (elastic#55015) [SIEM] Adds support for apm-* to the network map (elastic#54876) [Reporting] Define shims of legacy dependencies (elastic#54082) Resolver is overflow: hidden to prevent obscured elements from showing up (elastic#55076) Upgraded EUI to 18.2.1 (elastic#55090) [Maps] Support styles on agg fields with _of_ in name (elastic#54965) Remove xpack_main requirement, it's no longer in use (elastic#55060) Fix Snapshots Policies Alignment Issue in IE11 (elastic#54866) first rule cuts (elastic#54990) [DOCS] Adds geocentroid note to coordinate map (elastic#54389) [Canvas] Fixes the Copy Post Url link (elastic#54831) Fixes bugs with full screen filters (elastic#54792) [ML] Fix decoding in the URL state (elastic#54915) Remove redundant `x-pack/typings`. (elastic#55042) [SIEM][Detection Engine] Adds critical missing status route to prepackaged rules Generate legacy vars when rendering all applications (elastic#54768) ... # Conflicts: # x-pack/plugins/translations/translations/ja-JP.json # x-pack/plugins/translations/translations/zh-CN.json

* simplify serverfacade definition * simplify requestfacade definition * use the shim * makeRequestFacade * requestFacade * import sorting * originalServer * reduce loc change * remove consolelog * hacks to fix tests * ServerFacade in index * Cosmetic * remove field from serverfacade * add raw to the request * fix types * add fieldFormatServiceFactory to legacy * Pass the complete request object to sec plugin * Fix test * fix test 2 * getUser takes a legacy request * add unit test for new lib * add getRawRequest to pass to saved objects method * update test snapshot * leave a TODO comment for type import * variable rename for legacy id Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

* upstream/master: (83 commits) [Reporting] Fix map tiles not loading by using Chrome's Remote Protocol (elastic#55137) [Data Plugin] combine autocomplete provider and suggestions provider (elastic#54451) resolves elastic#53038 - remove references to specific license levels (elastic#53858) highlighting rules should still know about url parts when in sql state (elastic#55200) [Metric] convert mocha tests to jest (elastic#54054) [skip-ci] Update vector styling docs for 7.6 UI changes and new features (elastic#55087) Fix enable API to schedule task after alert is updated (elastic#55095) chore(NA): add 7.6 branch to the list of backport branches (elastic#54998) Convert tests to jest in vis_type_timeseries/public & common folders (elastic#55023) [ML] Accessibility fix for structural markup on table rows (elastic#55075) [Mappings editor] include/exclude fields only support custom options (elastic#54949) [Vis] Move Timelion Vis to vis_type_timelion (elastic#52069) Deprecate `chrome.navlinks.update` and add documentation (elastic#54893) [ML] Single Metric Viewer: Fix time bounds with custom strings. (elastic#55045) [Vis: Default editor] EUIficate and Reactify the sidebar (elastic#49864) [Mappings editor] Fix cannot set boolean value for "null_value" param (elastic#55015) [SIEM] Adds support for apm-* to the network map (elastic#54876) [Reporting] Define shims of legacy dependencies (elastic#54082) Resolver is overflow: hidden to prevent obscured elements from showing up (elastic#55076) Upgraded EUI to 18.2.1 (elastic#55090) ...

* [Reporting] Define shims of legacy dependencies (#54082) * simplify serverfacade definition * simplify requestfacade definition * use the shim * makeRequestFacade * requestFacade * import sorting * originalServer * reduce loc change * remove consolelog * hacks to fix tests * ServerFacade in index * Cosmetic * remove field from serverfacade * add raw to the request * fix types * add fieldFormatServiceFactory to legacy * Pass the complete request object to sec plugin * Fix test * fix test 2 * getUser takes a legacy request * add unit test for new lib * add getRawRequest to pass to saved objects method * update test snapshot * leave a TODO comment for type import * variable rename for legacy id Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> * update legacy to fix ts errors Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

tsullivan added review zDeprecated Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Feature:NP Migration v7.6.0 Team:Reporting Services labels Jan 7, 2020

tsullivan changed the title ~~Reporting/shim plugin~~ [Reporting] Define shims of legacy dependencies Jan 7, 2020

tsullivan mentioned this pull request Jan 7, 2020

[Reporting] New Platform Migration #53898

Closed

19 tasks

tsullivan requested a review from joelgriffith January 7, 2020 00:33

tsullivan added 5 commits January 6, 2020 17:34

simplify serverfacade definition

7315513

simplify requestfacade definition

d954ef0

makeRequestFacade

bb65245

requestFacade

79a9288

use the shim

c931081

tsullivan force-pushed the reporting/shim-plugin branch from a5cdf86 to c931081 Compare January 7, 2020 00:34

tsullivan added 4 commits January 6, 2020 17:35

import sorting

62dc408

originalServer

78a2ea1

reduce loc change

3e71f57

remove consolelog

956ae17

tsullivan commented Jan 7, 2020

View reviewed changes

hacks to fix tests

e4761ff

Merge branch 'master' into reporting/shim-plugin

48ec88c

tsullivan commented Jan 7, 2020

View reviewed changes

x-pack/legacy/plugins/reporting/server/lib/get_user.ts Outdated Show resolved Hide resolved

ServerFacade in index

44b714c

elasticmachine and others added 2 commits January 7, 2020 13:04

Merge branch 'master' into reporting/shim-plugin

4192c34

Cosmetic

4e7e448

tsullivan added 3 commits January 14, 2020 13:59

Fix test

17b3b10

fix test 2

f7609d1

getUser takes a legacy request

cb40fa3

tsullivan commented Jan 14, 2020

View reviewed changes

add unit test for new lib

15ab24b

elasticmachine and others added 2 commits January 15, 2020 16:36

Merge branch 'master' into reporting/shim-plugin

37f6c3e

add getRawRequest to pass to saved objects method

ddf8fac

tsullivan force-pushed the reporting/shim-plugin branch from aee133f to ddf8fac Compare January 15, 2020 22:05

update test snapshot

b4dcefb

Merge branch 'master' into reporting/shim-plugin

9d446a1

tsullivan mentioned this pull request Jan 16, 2020

[Reporting/Migration] ReportingSetup, LegacySetup #54198

Merged

mshustov reviewed Jan 16, 2020

View reviewed changes

mshustov approved these changes Jan 16, 2020

View reviewed changes

joelgriffith reviewed Jan 16, 2020

View reviewed changes

joelgriffith approved these changes Jan 16, 2020

View reviewed changes

tsullivan added 2 commits January 16, 2020 13:34

leave a TODO comment for type import

25e30da

variable rename for legacy id

5378584

tsullivan merged commit cfbd095 into elastic:master Jan 16, 2020

tsullivan deleted the reporting/shim-plugin branch January 16, 2020 22:15

tsullivan mentioned this pull request Jan 16, 2020

[7.x] [Reporting] Define shims of legacy dependencies (#54082) #55134

Merged

Conversation

tsullivan commented Jan 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

For maintainers

Uh oh!

elasticmachine commented Jan 7, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tsullivan commented Jan 7, 2020

Uh oh!

Uh oh!

tsullivan commented Jan 7, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pgayvallet Jan 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tsullivan commented Jan 15, 2020

Uh oh!

tsullivan commented Jan 16, 2020

Uh oh!

mshustov Jan 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joelgriffith left a comment

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Jan 16, 2020

💚 Build Succeeded

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

tsullivan commented Jan 7, 2020 •

edited

Loading

pgayvallet Jan 21, 2020 •

edited

Loading

mshustov Jan 16, 2020 •

edited

Loading