[Core] Explicit typings for request handler context #88718

mshustov · 2021-01-19T17:38:58Z

Summary

Part of #57588
This PR removes interface augmentation pattern for RequestHandlerContext type.
If a plugin extends RequestHandlerContext with a custom property or accesses a property defined by a dependency, it should declare a context type explicitly.

Before:

declare module 'src/core/server' {
  interface RequestHandlerContext {
    myPlugin?: MyPluginApi;
  }
}

const router = http.createRouter();
http.registerRouteHandlerContext('my-plugin', async (context, req, res) => {...});

After:

export interface MyPluginRequestHandlerContext extends RequestHandlerContext {
  myPlugin: MyPluginApi;
}
const router = http.createRouter<MyPluginRequestHandlerContext>();
http.registerRouteHandlerContext<MyPluginRequestHandlerContext, 'my-plugin'>(
  'my-plugin',
  async (context, req, res) => {...}
);

The benefits of the approach:

avoids global type pollution
it's easier to grasp the request handler context interface
request handler context interface can be adjusted to reflect plugin required & optional dependencies

Plugin API Changes

Whenever Kibana needs to get access to data saved in elasticsearch, it should perform a check whether an end-user has access to the data.
On the server-side, API requiring impersonation with an incoming request, are exposed via context argument of request handler:

const router = core.http.createRouter();
router.get(
  { path: '/api/my-plugin/', validate: … },
  async (context, req, res) => {}
)

Starting from the current version your plugin should declare an interface of this context parameter explicitly.
Before:

declare module 'src/core/server' {
  interface RequestHandlerContext {
    myPlugin?: MyPluginApi;
  }
}

const router = http.createRouter();
http.registerRouteHandlerContext('my-plugin', async (context, req, res) => {...});

After:

export interface MyPluginRequestHandlerContext extends RequestHandlerContext {
  myPlugin: MyPluginApi;
}
const router = http.createRouter<MyPluginRequestHandlerContext>();
http.registerRouteHandlerContext<MyPluginRequestHandlerContext, 'my-plugin'>(
  'my-plugin',
  async (context, req, res) => {...}
);

Context implementation will be simplified in follow-up.

justinkambic

Changes to Uptime app LGTM.

chrisronline

LGTM for stack monitoring

streamich

AppServices changes LGTM.

rylnd

Lists and Detections changes LGTM! I'd like to get (and have requested) approval from someone(s) more familiar with the Cases and Endpoint code, though.

XavierM

LGTM for Case and timeline

cnasikas

LGTM for Case and Timeline

cnasikas · 2021-01-20T20:16:54Z

x-pack/plugins/case/server/types.ts

+  case: CaseRequestContext;
+  actions: ActionsApiRequestHandlerContext;
+  // TODO: Remove when triggers_ui do not import case's types.
+  // PR https://github.com/elastic/kibana/pull/84587.


Super nit -> The comments should be removed right? To my understanding it is required to declare all plugin dependencies context and we should not remove the securitySolution, right?

@cnasikas no idea, this comment was copied from the old declaration https://github.com/elastic/kibana/pull/88718/files/6334f55c5e4a8af3cc93d10345cb2340e14fc437#diff-e0b5bc10c412de3c7f41b1647bfefd4c32bf449eb235c1b257e8eac0daf6da80L18

x-pack/plugins/apm/server/routes/typings.ts

paul-tavares

LGTM from a Security Solution Endpoint Management standpoint

x-pack/plugins/security_solution/server/plugin.ts

jloleysens

ES UI changes look good to me!

x-pack/plugins/cross_cluster_replication/server/types.ts

afharo

Code LGTM! I think we'll need a follow-up to fix the @ts-expect-error bits. But I think we can move on for now to unblock the TS refs efforts.

afharo · 2021-01-21T10:07:01Z

src/core/server/context/container/context.ts

    {
-      provider: IContextProvider<THandler, keyof HandlerContextType<THandler>>;
+      provider: IContextProvider<any, any>;


Super-NIT: I think this generalization of the types makes TS lose the references to THandler. And that is causing the arity problems down the line. IMO, we can revisit these types later to unblock the TS References migrations.

pgayvallet

LGTM. That was something!

pgayvallet · 2021-01-21T10:17:51Z

src/core/server/http/http_service.ts

@@ -11,6 +11,7 @@ import { first, map } from 'rxjs/operators';
 import { Server } from '@hapi/hapi';
 import { pick } from '@kbn/std';

+import type { RequestHandlerContext } from 'src/core/server';


NIT: can we use relative import here?

we can, but RequestHandlerContext declared in 'src/core/server/index.ts'. ~~I'd prefer we move it to http in a separate PR~~ not sure what is the best place to keep it, RequestHandlerContext is Kibana application-specific type, not related to http service

pgayvallet · 2021-01-21T10:23:46Z

src/core/server/http/router/router.ts

@@ -341,10 +349,11 @@ export type RequestHandler<
  P = unknown,
  Q = unknown,
  B = unknown,
+  Context extends RequestHandlerContext = RequestHandlerContext,


Some plugins are declaring their handlers outside of the route registration, so the handler's generics are not inferred

e.g

const handler = ... router.get(config, handler);

pgayvallet · 2021-01-21T10:37:09Z

src/plugins/telemetry/server/routes/telemetry_opt_in_stats.test.ts

-      body: mockOptInStatus,
+      body: '["mock_opt_in_hashed_value"]',


NIT: why the change?

fetch body doesn't accept string[], but string. after the fix, the test reflects the change.

pgayvallet · 2021-01-21T10:39:01Z

src/plugins/telemetry/server/routes/telemetry_opt_in_stats.ts

@@ -35,7 +35,7 @@ export async function sendTelemetryOptInStatus(

  await fetch(optInStatusUrl, {
    method: 'post',
-    body: optInStatus,
+    body: JSON.stringify(optInStatus),


Probably because of the removed ts-ignore?

kibanamachine · 2021-01-21T11:47:05Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 3185d54

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`core`	447	446	-1

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`core`	563.8KB	558.4KB	-5.4KB

History

💛 Build #99936 was flaky 6334f55
💚 Build #99919 succeeded 30c9c49
💔 Build #99905 failed 73cfc08
💔 Build #99667 failed bb08a9f

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

* move context to server part. couple with RequestHandlerContext Context implementation will be simplified in follow-up. * adopt core code * adopt bfetch code * adopt data code * adopt search examples * adopt vis_type_timelion * adopt vis_type_timeseries * adopt plugin functional tests * adopt actions * adopt alerting plugin * adopt APM plugin * adopt beats_management * adopt case plugin * adopt cross_cluster_replication * adopt data_enhanced * adopt event_log * adopt global_search * adopt index_management * adopt infra * adopt licensing * adopt lists * adopt logstash * adopt reporting * adopt observability * adopt monitoring * adopt rollup * adopt so tagging * adopt security * adopt security_solutions * adopt watcher * adopt uptime * adopt spaces * adopt snapshot_restore * adopt features changes * mute error when null used to extend context * update docs * small cleanup * add type safety for return type * refactor registerRouteHandlerContext type * update docs * update license header * update docs * fix type error. fetch body does not accept array of strings * fix telemetry test * remove unnecessary ts-ignore * address comments * update docs # Conflicts: # docs/development/plugins/data/server/kibana-plugin-plugins-data-server.plugin.start.md # src/plugins/data/server/server.api.md # x-pack/plugins/monitoring/server/plugin.ts

mshustov added 30 commits January 19, 2021 15:37

move context to server part. couple with RequestHandlerContext

bb2cd94

Context implementation will be simplified in follow-up.

adopt core code

58387da

adopt bfetch code

3e90e00

adopt data code

4554253

adopt search examples

e9eaf0a

adopt vis_type_timelion

3348748

adopt vis_type_timeseries

ccfcdca

adopt plugin functional tests

c800642

adopt actions

33c45b2

adopt alerting plugin

258e3c1

adopt APM plugin

b8016bc

adopt beats_management

b3b0f1b

adopt case plugin

0f6f622

adopt cross_cluster_replication

dfc44f6

adopt data_enhanced

bedd6d0

adopt event_log

274620d

adopt global_search

3cec9b2

adopt index_management

f635ed7

adopt infra

adab1bc

adopt licensing

96b4960

adopt lists

036f3dd

adopt logstash

5aa208e

adopt reporting

929efc2

adopt observability

02172f1

adopt monitoring

f148f70

adopt rollup

44aa3ec

adopt so tagging

9a0bb86

adopt security

75027b6

adopt security_solutions

0ea1243

adopt watcher

42d36f8

justinkambic approved these changes Jan 20, 2021

View reviewed changes

chrisronline approved these changes Jan 20, 2021

View reviewed changes

streamich approved these changes Jan 20, 2021

View reviewed changes

rylnd approved these changes Jan 20, 2021

View reviewed changes

XavierM approved these changes Jan 20, 2021

View reviewed changes

cnasikas approved these changes Jan 20, 2021

View reviewed changes

smith requested changes Jan 20, 2021

View reviewed changes

x-pack/plugins/apm/server/routes/typings.ts Outdated Show resolved Hide resolved

paul-tavares approved these changes Jan 20, 2021

View reviewed changes

paul-tavares reviewed Jan 20, 2021

View reviewed changes

x-pack/plugins/security_solution/server/plugin.ts Outdated Show resolved Hide resolved

wylieconlon added v8.0.0 and removed 8.0.0 labels Jan 20, 2021

jloleysens approved these changes Jan 21, 2021

View reviewed changes

x-pack/plugins/cross_cluster_replication/server/types.ts Outdated Show resolved Hide resolved

mshustov added 3 commits January 21, 2021 10:25

Merge branch 'master' into explicit-request-hanlder-context

f389948

address comments

f2750b8

update docs

3185d54

afharo approved these changes Jan 21, 2021

View reviewed changes

pgayvallet approved these changes Jan 21, 2021

View reviewed changes

mshustov requested a review from smith January 21, 2021 13:54

smith approved these changes Jan 21, 2021

View reviewed changes

mshustov merged commit b3a9754 into elastic:master Jan 21, 2021

mshustov deleted the explicit-request-hanlder-context branch January 21, 2021 14:20

mshustov mentioned this pull request Jan 21, 2021

[7.x] [Core] Explicit typings for request handler context (#88718) #88975

Merged

mshustov added the backported label Jan 21, 2021

mshustov mentioned this pull request Jan 24, 2021

Add type argument for RouteHandlerContext #57588

Closed

This was referenced Jan 26, 2021

[data.search] Add search session methods to search service contract #87966

Merged

[data.search] Allow search response to follow new hits format #88115

Merged

mshustov mentioned this pull request Mar 4, 2021

[Core] Simplify context typings #93579

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Core] Explicit typings for request handler context #88718

[Core] Explicit typings for request handler context #88718

mshustov commented Jan 19, 2021 •

edited

Loading

justinkambic left a comment

chrisronline left a comment

streamich left a comment

rylnd left a comment

XavierM left a comment

cnasikas left a comment

cnasikas Jan 20, 2021 •

edited

Loading

mshustov Jan 21, 2021

paul-tavares left a comment

jloleysens left a comment

afharo left a comment

afharo Jan 21, 2021

pgayvallet left a comment

pgayvallet Jan 21, 2021

mshustov Jan 21, 2021 •

edited

Loading

pgayvallet Jan 21, 2021

pgayvallet Jan 21, 2021

mshustov Jan 21, 2021

pgayvallet Jan 21, 2021

kibanamachine commented Jan 21, 2021

[Core] Explicit typings for request handler context #88718

[Core] Explicit typings for request handler context #88718

Conversation

mshustov commented Jan 19, 2021 • edited Loading

Summary

Plugin API Changes

justinkambic left a comment

Choose a reason for hiding this comment

chrisronline left a comment

Choose a reason for hiding this comment

streamich left a comment

Choose a reason for hiding this comment

rylnd left a comment

Choose a reason for hiding this comment

XavierM left a comment

Choose a reason for hiding this comment

cnasikas left a comment

Choose a reason for hiding this comment

cnasikas Jan 20, 2021 • edited Loading

Choose a reason for hiding this comment

mshustov Jan 21, 2021

Choose a reason for hiding this comment

paul-tavares left a comment

Choose a reason for hiding this comment

jloleysens left a comment

Choose a reason for hiding this comment

afharo left a comment

Choose a reason for hiding this comment

afharo Jan 21, 2021

Choose a reason for hiding this comment

pgayvallet left a comment

Choose a reason for hiding this comment

pgayvallet Jan 21, 2021

Choose a reason for hiding this comment

mshustov Jan 21, 2021 • edited Loading

Choose a reason for hiding this comment

pgayvallet Jan 21, 2021

Choose a reason for hiding this comment

pgayvallet Jan 21, 2021

Choose a reason for hiding this comment

mshustov Jan 21, 2021

Choose a reason for hiding this comment

pgayvallet Jan 21, 2021

Choose a reason for hiding this comment

kibanamachine commented Jan 21, 2021

💚 Build Succeeded

Metrics [docs]

Module Count

Page load bundle

History

mshustov commented Jan 19, 2021 •

edited

Loading

cnasikas Jan 20, 2021 •

edited

Loading

mshustov Jan 21, 2021 •

edited

Loading