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.

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

Implements getStartServices on server-side #55156

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
<!-- Do not edit this file. It is automatically generated by API Documenter. -->

[Home](./index.md) &gt; [kibana-plugin-server](./kibana-plugin-server.md) &gt; [CoreSetup](./kibana-plugin-server.coresetup.md) &gt; [getStartServices](./kibana-plugin-server.coresetup.getstartservices.md)

## CoreSetup.getStartServices() method

Allows plugins to get access to APIs available in start inside async handlers. Promise will not resolve until Core and plugin dependencies have completed `start`<!-- -->. This should only be used inside handlers registered during `setup` that will only be executed after `start` lifecycle.

<b>Signature:</b>

```typescript
getStartServices(): Promise<[CoreStart, TPluginsStart]>;
```
<b>Returns:</b>

`Promise<[CoreStart, TPluginsStart]>`

58 changes: 32 additions & 26 deletions docs/development/core/server/kibana-plugin-server.coresetup.md
Original file line number Diff line number Diff line change
@@ -1,26 +1,32 @@
<!-- Do not edit this file. It is automatically generated by API Documenter. -->

[Home](./index.md) &gt; [kibana-plugin-server](./kibana-plugin-server.md) &gt; [CoreSetup](./kibana-plugin-server.coresetup.md)

## CoreSetup interface

Context passed to the plugins `setup` method.

<b>Signature:</b>

```typescript
export interface CoreSetup
```

## Properties

| Property | Type | Description |
| --- | --- | --- |
| [capabilities](./kibana-plugin-server.coresetup.capabilities.md) | <code>CapabilitiesSetup</code> | [CapabilitiesSetup](./kibana-plugin-server.capabilitiessetup.md) |
| [context](./kibana-plugin-server.coresetup.context.md) | <code>ContextSetup</code> | [ContextSetup](./kibana-plugin-server.contextsetup.md) |
| [elasticsearch](./kibana-plugin-server.coresetup.elasticsearch.md) | <code>ElasticsearchServiceSetup</code> | [ElasticsearchServiceSetup](./kibana-plugin-server.elasticsearchservicesetup.md) |
| [http](./kibana-plugin-server.coresetup.http.md) | <code>HttpServiceSetup</code> | [HttpServiceSetup](./kibana-plugin-server.httpservicesetup.md) |
| [savedObjects](./kibana-plugin-server.coresetup.savedobjects.md) | <code>SavedObjectsServiceSetup</code> | [SavedObjectsServiceSetup](./kibana-plugin-server.savedobjectsservicesetup.md) |
| [uiSettings](./kibana-plugin-server.coresetup.uisettings.md) | <code>UiSettingsServiceSetup</code> | [UiSettingsServiceSetup](./kibana-plugin-server.uisettingsservicesetup.md) |
| [uuid](./kibana-plugin-server.coresetup.uuid.md) | <code>UuidServiceSetup</code> | [UuidServiceSetup](./kibana-plugin-server.uuidservicesetup.md) |

<!-- Do not edit this file. It is automatically generated by API Documenter. -->

[Home](./index.md) &gt; [kibana-plugin-server](./kibana-plugin-server.md) &gt; [CoreSetup](./kibana-plugin-server.coresetup.md)

## CoreSetup interface

Context passed to the plugins `setup` method.

<b>Signature:</b>

```typescript
export interface CoreSetup<TPluginsStart extends object = object>
```

## Properties

| Property | Type | Description |
| --- | --- | --- |
| [capabilities](./kibana-plugin-server.coresetup.capabilities.md) | <code>CapabilitiesSetup</code> | [CapabilitiesSetup](./kibana-plugin-server.capabilitiessetup.md) |
| [context](./kibana-plugin-server.coresetup.context.md) | <code>ContextSetup</code> | [ContextSetup](./kibana-plugin-server.contextsetup.md) |
| [elasticsearch](./kibana-plugin-server.coresetup.elasticsearch.md) | <code>ElasticsearchServiceSetup</code> | [ElasticsearchServiceSetup](./kibana-plugin-server.elasticsearchservicesetup.md) |
| [http](./kibana-plugin-server.coresetup.http.md) | <code>HttpServiceSetup</code> | [HttpServiceSetup](./kibana-plugin-server.httpservicesetup.md) |
| [savedObjects](./kibana-plugin-server.coresetup.savedobjects.md) | <code>SavedObjectsServiceSetup</code> | [SavedObjectsServiceSetup](./kibana-plugin-server.savedobjectsservicesetup.md) |
| [uiSettings](./kibana-plugin-server.coresetup.uisettings.md) | <code>UiSettingsServiceSetup</code> | [UiSettingsServiceSetup](./kibana-plugin-server.uisettingsservicesetup.md) |
| [uuid](./kibana-plugin-server.coresetup.uuid.md) | <code>UuidServiceSetup</code> | [UuidServiceSetup](./kibana-plugin-server.uuidservicesetup.md) |

## Methods

| Method | Description |
| --- | --- |
| [getStartServices()](./kibana-plugin-server.coresetup.getstartservices.md) | Allows plugins to get access to APIs available in start inside async handlers. Promise will not resolve until Core and plugin dependencies have completed <code>start</code>. This should only be used inside handlers registered during <code>setup</code> that will only be executed after <code>start</code> lifecycle. |

9 changes: 8 additions & 1 deletion src/core/server/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -283,7 +283,7 @@ export interface RequestHandlerContext {
*
* @public
*/
export interface CoreSetup {
export interface CoreSetup<TPluginsStart extends object = object> {
/** {@link CapabilitiesSetup} */
capabilities: CapabilitiesSetup;
/** {@link ContextSetup} */
Expand All @@ -298,6 +298,13 @@ export interface CoreSetup {
uiSettings: UiSettingsServiceSetup;
/** {@link UuidServiceSetup} */
uuid: UuidServiceSetup;
/**
* Allows plugins to get access to APIs available in start inside async handlers.
* Promise will not resolve until Core and plugin dependencies have completed `start`.
* This should only be used inside handlers registered during `setup` that will only be executed
* after `start` lifecycle.
*/
getStartServices(): Promise<[CoreStart, TPluginsStart]>;
}

/**
Expand Down
12 changes: 7 additions & 5 deletions src/core/server/legacy/legacy_service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -256,6 +256,12 @@ export class LegacyService implements CoreService {
startDeps: LegacyServiceStartDeps,
legacyPlugins: LegacyPlugins
) {
const coreStart: CoreStart = {
capabilities: startDeps.core.capabilities,
savedObjects: { getScopedClient: startDeps.core.savedObjects.getScopedClient },
uiSettings: { asScopedToClient: startDeps.core.uiSettings.asScopedToClient },
};

const coreSetup: CoreSetup = {
capabilities: setupDeps.core.capabilities,
context: setupDeps.core.context,
Expand Down Expand Up @@ -291,11 +297,7 @@ export class LegacyService implements CoreService {
uuid: {
getInstanceUuid: setupDeps.core.uuid.getInstanceUuid,
},
};
const coreStart: CoreStart = {
capabilities: startDeps.core.capabilities,
savedObjects: { getScopedClient: startDeps.core.savedObjects.getScopedClient },
uiSettings: { asScopedToClient: startDeps.core.uiSettings.asScopedToClient },
getStartServices: () => Promise.resolve([coreStart, startDeps.plugins]),
};

// eslint-disable-next-line @typescript-eslint/no-var-requires
Expand Down
7 changes: 6 additions & 1 deletion src/core/server/mocks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,8 @@ function pluginInitializerContextMock<T>(config: T = {} as T) {
return mock;
}

type CoreSetupMockType = MockedKeys<CoreSetup> & jest.Mocked<Pick<CoreSetup, 'getStartServices'>>;

Comment on lines +89 to +90
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 had a hard time adding getStartServices to the CoreSetup mock. MockedKeys does not properly handles functions and resulted in the mock not being compatible with the concrete implementation. Using jest.Mocked or DeeplyMockedKeys both resulted on errors on tests that were overriding read-only properties (that seems to be preserved by both but not by MockedKeys), such as

const coreSetup = coreMock.createSetup();
coreSetup.elasticsearch.dataClient = dataClient;

So I had to compose this custom type instead.

Copy link
Contributor

@mshustov mshustov Jan 17, 2020

Choose a reason for hiding this comment

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

It sounds like something we will face in the future again when marking more API points as readonly. We need to define a common pattern for configuring mocks at any deep level. Re-writing a property is the most obvious way, but create this incompatibility. Options:

  • a mock factory accepts an object configuring mocks at any deep coreMock.createSetup({ elasticsearch: { dataClient: ... } }); That would quick grow with the number of nested objects.
  • provide configurable mock factory (as a builder pattern, for example) coreMock.get().withElasticsearchDataClient(...).withSomethingElse().build()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After a quick look, this is not the first time this occurs (the exemple is actually the root cause if this specific issue)

type MockedElasticSearchServiceSetup = jest.Mocked<
ElasticsearchServiceSetup & {
adminClient: jest.Mocked<IClusterClient>;
dataClient: jest.Mocked<IClusterClient>;
}
>;

We are also inconsistent in the types we actually return from mocks. Some are explicitly mock instance, some are not

function createCoreStartMock() {
const mock: MockedKeys<CoreStart> = {

vs

function createInternalCoreSetupMock() {
const setupDeps: InternalCoreSetup = {

That would quick grow with the number of nested objects.

It does. however the (manually coded) factory pattern present the same (if not bigger) problem in term of maintenance. I'm not the biggest TS expert, but can't we have an utility type that would remove readonly from the mock definition while preserving compatibility with the concrete type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hum, I was trying things and was not understanding why I couldnt override core.elasticsearch.dataClient from inside core... And I just saw that the MockedKeys type is not the same between src and x-pack... This explain why this is possible in the licensing plugin tests:

type MockedKeys<T> = { [P in keyof T]: jest.Mocked<T[P]> };

type MockedKeys<T> = { [P in keyof T]: jest.Mocked<Writable<T[P]>> };

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is outside of the scope of this PR. Created #55189 to continue the discussion

function createCoreSetupMock() {
const httpService = httpServiceMock.createSetupContract();
const httpMock: jest.Mocked<CoreSetup['http']> = {
Expand All @@ -105,14 +107,17 @@ function createCoreSetupMock() {
const uiSettingsMock = {
register: uiSettingsServiceMock.createSetupContract().register,
};
const mock: MockedKeys<CoreSetup> = {
const mock: CoreSetupMockType = {
capabilities: capabilitiesServiceMock.createSetupContract(),
context: contextServiceMock.createSetupContract(),
elasticsearch: elasticsearchServiceMock.createSetup(),
http: httpMock,
savedObjects: savedObjectsServiceMock.createSetupContract(),
uiSettings: uiSettingsMock,
uuid: uuidServiceMock.createSetupContract(),
getStartServices: jest
.fn<Promise<[ReturnType<typeof createCoreStartMock>, object]>, []>()
.mockResolvedValue([createCoreStartMock(), {}]),
};

return mock;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
/*
* Licensed to Elasticsearch B.V. under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch B.V. licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

export const mockPackage = new Proxy(
{ raw: { __dirname: '/tmp' } as any },
{ get: (obj, prop) => obj.raw[prop] }
);
jest.mock('../../../../core/server/utils/package_json', () => ({ pkg: mockPackage }));

export const mockDiscover = jest.fn();
jest.mock('../discovery/plugins_discovery', () => ({ discover: mockDiscover }));
167 changes: 167 additions & 0 deletions src/core/server/plugins/integration_tests/plugins_service.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,167 @@
/*
* Licensed to Elasticsearch B.V. under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch B.V. licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

import { mockPackage, mockDiscover } from './plugins_service.test.mocks';

import { join } from 'path';
Comment on lines +20 to +22
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are very similar to the service's unit test. However the difference is that in this file, we do not mock the underlying plugin system, and call the service's setup and start with actual mocked dependencies.


import { PluginsService } from '../plugins_service';
import { ConfigPath, ConfigService, Env } from '../../config';
import { getEnvOptions } from '../../config/__mocks__/env';
import { BehaviorSubject, from } from 'rxjs';
import { rawConfigServiceMock } from '../../config/raw_config_service.mock';
import { config } from '../plugins_config';
import { loggingServiceMock } from '../../logging/logging_service.mock';
import { coreMock } from '../../mocks';
import { Plugin } from '../types';
import { PluginWrapper } from '../plugin';

describe('PluginsService', () => {
const logger = loggingServiceMock.create();
let pluginsService: PluginsService;

const createPlugin = (
id: string,
{
path = id,
disabled = false,
version = 'some-version',
requiredPlugins = [],
optionalPlugins = [],
kibanaVersion = '7.0.0',
configPath = [path],
server = true,
ui = true,
}: {
path?: string;
disabled?: boolean;
version?: string;
requiredPlugins?: string[];
optionalPlugins?: string[];
kibanaVersion?: string;
configPath?: ConfigPath;
server?: boolean;
ui?: boolean;
}
): PluginWrapper => {
return new PluginWrapper({
path,
manifest: {
id,
version,
configPath: `${configPath}${disabled ? '-disabled' : ''}`,
kibanaVersion,
requiredPlugins,
optionalPlugins,
server,
ui,
},
opaqueId: Symbol(id),
initializerContext: { logger } as any,
});
};

beforeEach(async () => {
mockPackage.raw = {
branch: 'feature-v1',
version: 'v1',
build: {
distributable: true,
number: 100,
sha: 'feature-v1-build-sha',
},
};

const env = Env.createDefault(getEnvOptions());
const config$ = new BehaviorSubject<Record<string, any>>({
plugins: {
initialize: true,
},
});
const rawConfigService = rawConfigServiceMock.create({ rawConfig$: config$ });
const configService = new ConfigService(rawConfigService, env, logger);
await configService.setSchema(config.path, config.schema);

pluginsService = new PluginsService({
coreId: Symbol('core'),
env,
logger,
configService,
});
});

it("properly resolves `getStartServices` in plugin's lifecycle", async () => {
expect.assertions(5);

const pluginPath = 'plugin-path';

mockDiscover.mockReturnValue({
error$: from([]),
plugin$: from([
createPlugin('plugin-id', {
path: pluginPath,
configPath: 'path',
}),
]),
});

let startDependenciesResolved = false;
let contextFromStart: any = null;
let contextFromStartService: any = null;

const pluginInitializer = () =>
({
setup: async (coreSetup, deps) => {
coreSetup.getStartServices().then(([core, plugins]) => {
startDependenciesResolved = true;
contextFromStartService = { core, plugins };
});
},
start: async (core, plugins) => {
contextFromStart = { core, plugins };
await new Promise(resolve => setTimeout(resolve, 10));
expect(startDependenciesResolved).toBe(false);
},
} as Plugin);

jest.doMock(
join(pluginPath, 'server'),
() => ({
plugin: pluginInitializer,
}),
{
virtual: true,
}
);

await pluginsService.discover();

const setupDeps = coreMock.createInternalSetup();
await pluginsService.setup(setupDeps);

expect(startDependenciesResolved).toBe(false);

const startDeps = coreMock.createInternalStart();
await pluginsService.start(startDeps);

expect(startDependenciesResolved).toBe(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be worth verifying that the resolved dependencies are the objects you expect them to be.

expect(contextFromStart!.core).toEqual(contextFromStartService!.core);
expect(contextFromStart!.plugins).toEqual(contextFromStartService!.plugins);
});
});
37 changes: 37 additions & 0 deletions src/core/server/plugins/plugin.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,43 @@ test('`start` calls plugin.start with context and dependencies', async () => {
expect(mockPluginInstance.start).toHaveBeenCalledWith(context, deps);
});

test("`start` resolves `startDependencies` Promise after plugin's start", async () => {
expect.assertions(2);

const manifest = createPluginManifest();
const opaqueId = Symbol();
const plugin = new PluginWrapper({
path: 'plugin-with-initializer-path',
manifest,
opaqueId,
initializerContext: createPluginInitializerContext(coreContext, opaqueId, manifest),
});
const startContext = { any: 'thing' } as any;
const pluginDeps = { someDep: 'value' };

let startDependenciesResolved = false;

const mockPluginInstance = {
setup: jest.fn(),
start: async () => {
// delay to ensure startDependencies is not resolved until after the plugin instance's start resolves.
await new Promise(resolve => setTimeout(resolve, 10));
expect(startDependenciesResolved).toBe(false);
},
};
mockPluginInitializer.mockReturnValue(mockPluginInstance);

await plugin.setup({} as any, {} as any);

const startDependenciesCheck = plugin.startDependencies.then(resolvedStartDeps => {
startDependenciesResolved = true;
expect(resolvedStartDeps).toEqual([startContext, pluginDeps]);
});

await plugin.start(startContext, pluginDeps);
await startDependenciesCheck;
});

test('`stop` fails if plugin is not set up', async () => {
const manifest = createPluginManifest();
const opaqueId = Symbol();
Expand Down
Loading