Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[RFC] Handler interface #36509

Merged
merged 6 commits into from
Jun 18, 2019
Merged

[RFC] Handler interface #36509

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2b95f61
RFC: handler interface
epixa May 11, 2019
5ac8665
Add a typing pattern
joshdover May 23, 2019
13427ac
Cleanup and clarify language and examples
joshdover May 28, 2019
5e4fab9
Add additional exampes of where handler context would be useful
joshdover May 30, 2019
18e1301
Add context creation section
joshdover Jun 12, 2019
571d609
Context execution order update
joshdover Jun 17, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 38 additions & 22 deletions rfcs/text/0003_handler_interface.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ services that are not necessarily known to the service owner.

```js
// services can register context providers to route handlers
http.registerContext('myApi', request => ({ getId() { return request.params.myApiId } }));
http.registerContext('myApi', (context, request) => ({ getId() { return request.params.myApiId } }));

http.router.route({
method: 'GET',
Expand Down Expand Up @@ -44,7 +44,7 @@ taskManager.registerTaskDefinitions({
createTaskRunner(context) {
return {
async run() {
const docs = await context.elasticsearch.search();
const docs = await context.core.elasticsearch.search();
doSomethingWithDocs(docs);
}
}
Expand All @@ -56,7 +56,10 @@ taskManager.registerTaskDefinitions({
application.registerApp({
id: 'myApp',
mount(context, domElement) {
ReactDOM.render(<MyApp overlaysService={context.overlays} />, domElement);
ReactDOM.render(
<MyApp overlaysService={context.core.overlays} />,
domElement
);
return () => ReactDOM.unmountComponentAtNode(domElement);
}
});
Expand All @@ -65,7 +68,7 @@ application.registerApp({
alerting.registerType({
id: 'myAlert',
async execute(context, params, state) {
const indexPatterns = await context.savedObjects.find('indexPattern');
const indexPatterns = await context.core.savedObjects.find('indexPattern');
// use index pattern to search
}
})
Expand Down Expand Up @@ -116,11 +119,11 @@ their handlers extensible.

```ts
interface Context {
core: unknown;
core: Record<string, unknown>;
[contextName: string]: unknown;
}

type Handler = (context: Partial<Context>, ...args: unknown[]) => Promise<unknown>;
type Handler = (context: Context, ...args: unknown[]) => Promise<unknown>;
```

- `args` in this example is specific to the handler type, for instance in a
Expand All @@ -133,7 +136,10 @@ type Handler = (context: Partial<Context>, ...args: unknown[]) => Promise<unknow
## Registering new contexts

```ts
type ContextProvider<T extends keyof Context> = (...args: unknown[]) => Promise<Context[T]>;
type ContextProvider<T extends keyof Context> = (
context: Partial<Context>,
...args: unknown[]
) => Promise<Context[T]>;

interface HandlerService {
registerContext<T extends keyof Context>(contextName: T, provider: ContextProvider<T>): void;
Expand All @@ -159,21 +165,24 @@ providers is merged into a single object where each key of the object is the
name of the context provider and the value is the return value of the provider.
Key facts about context providers:

- **Context providers cannot access context from other providers.** They should
be fully self-contained and not dependent on other contexts. The order that
they execute is not guaranteed.
- **Context providers are executed in registration order.** Providers are
registered during the setup phase, which happens in topological dependency
order, which will cause the context providers to execute in the same order.
Providers can leverage this property to rely on the context of dependencies to
be present during the execution of its own providers. All context registered
by Core will be present during all plugin context provider executions.
- **Context providers may be executed with the different arguments from
handlers** Each service owner should define what arguments are available to
handlers.** Each service owner should define what arguments are available to
context providers, however the context itself should never be an argument (see
point above).
- **Context providers cannot takeover the handler execution.** Context providers
cannot "intercept" handlers and return a different response. This is different
than traditional middleware. It should be noted that throwing an exception
will be bubbled up to the calling code and will prevent the handler from
will be bubbled up to the calling code and may prevent the handler from
getting executed at all. How the service owner handles that exception is
service-specific.
- **Values returned by context providers are expected to be valid for the entire
scope of the handler.**
execution scope of the handler.**

Here's a simple example of how a service owner could construct a context and
execute a handler:
Expand All @@ -184,7 +193,7 @@ const contextProviders = new Map()<string, ContextProvider<unknown>>;
async function executeHandler(handler, request, toolkit) {
const newContext = {};
for (const [contextName, provider] of contextProviders.entries()) {
newContext[contextName] = await provider(request, toolkit);
newContext[contextName] = await provider(newContext, request, toolkit);
}

return handler(context, request, toolkit);
Expand All @@ -194,7 +203,7 @@ async function executeHandler(handler, request, toolkit) {
## End to end example

```js
http.router.registerRequestContext('elasticsearch', async request => {
http.router.registerRequestContext('elasticsearch', async (context, request) => {
const client = await core.elasticsearch.client$.toPromise();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's an implementation detail, but imo important don't forget to make context extension lazy (as a getter?) and execute this function every time when someone reads from context.[extension point] otherwise context may have stale version of the client.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think context needs to be generated immediately before invoking the handler. The idea is that context does not change once its corresponding handler begins executing, so even if the ES client were to update at the service level, a handler that has already started executing would continue to use the original client until it finishes executing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joshdover We should call this out specifically in the RFC because it's an important detail.

return client.child({
headers: { authorization: request.headers.authorization },
Expand All @@ -204,7 +213,7 @@ http.router.registerRequestContext('elasticsearch', async request => {
http.router.route({
path: '/foo',
async routeHandler(context) {
context.elasticsearch.search(); // === callWithRequest(request, 'search')
context.core.elasticsearch.search(); // === callWithRequest(request, 'search')
},
});
```
Expand All @@ -230,7 +239,7 @@ interface HttpSetup {

registerRequestContext<T extends keyof RequestContext>(
contextName: T,
provider: (request: Request) => RequestContext[T] | Promise<RequestContext[T]>
provider: (context: Partial<RequestContext>, request: Request) => RequestContext[T] | Promise<RequestContext[T]>
): void;

// ...
Expand Down Expand Up @@ -258,7 +267,7 @@ declare module "../../core/server" {
class MyPlugin {
setup(core) {
// This will be type-safe!
core.http.registerRequestContext('myPlugin', (request) => ({
core.http.registerRequestContext('myPlugin', (context, request) => ({
getFoo() { return 'foo!' }
}))
joshdover marked this conversation as resolved.
Show resolved Hide resolved
}
Expand All @@ -267,15 +276,17 @@ class MyPlugin {

# Drawbacks

- Since the context properties that are present changes if plugins are disabled,
- Since the context properties that are present change if plugins are disabled,
they are all marked as optional properties which makes consuming the context
type awkward. We can expose types at the core and plugin level, but consumers
of those types might need to define which properties are present manually to
match their required plugin dependencies. Example:
```ts
type RequiredDependencies = 'data' | 'timepicker';
type OptionalDependencies = 'telemetry';
type MyPluginContext = Pick<RequestContext, 'core'> & Pick<RequestContext, RequiredDependencies> & Pick<Partial<RequestContext>, OptionalDependencies>;
type MyPluginContext = Pick<RequestContext, 'core'> &
Pick<RequestContext, RequiredDependencies> &
Pick<Partial<RequestContext>, OptionalDependencies>;
// => { core: {}, data: Data, timepicker: Timepicker, telemetry?: Telemetry };
```
This could even be provided as a generic type:
Expand All @@ -297,9 +308,14 @@ class MyPlugin {
necessarily associate similar patterns elsewhere as the same set of problems.
- "Chicken and egg" questions will arise around where context providers should be
registered. For example, does the `http` service invoke its
registerCapabilities for `elasticsearch`, or does the `elasticsearch` service
invoke `http.registerCapabilities`, or does core itself register the
registerRequestContext for `elasticsearch`, or does the `elasticsearch` service
invoke `http.registerRequestContext`, or does core itself register the
provider so neither service depends directly on the other.
- The existence of plugins that a given plugin does not depend on may leak
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this RFC is about a broad concept rather than a specific feature, I'm fine proceeding with this RFC as-is, but this detail is an important problem to address for implementations. Realistically, I think this means that core must provide plugin developers a mechanism to build a context since by nature a plugin won't even know about plugins that it does not depend on, so they can't properly construct the context themselves.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. We may actually need to expose a service to plugins that allows them to offer context registration and construction. This service would handle the dependency management aspect of constructing context.

This needs to be built before any plugins begin offering context in their handlers.

through the context object. This becomes a problem if a plugin uses any
context properties provided by a plugin that it does not depend on and that
plugin gets disabled in production. This can be solved by service owners, but
may need to be reimplemented for each one.

# Alternatives

Expand Down