Implements getStartServices on server-side

[pgayvallet]

Summary

Fix #54886
Required to migrate plugin's calls for #52071

This adds a new getStartServices to the server-side CoreSetup type for accessing start contracts. This is intended to be used when registering handler during setup phase that will needs to accept start APIs and for which the usages of contexts is overly complicated.

Widely inspired server-side counter part of #50231

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

Dev Docs

Adds a new API to be able to access start dependencies when registering handlers in setup phase.

class MyPlugin implements Plugin {
  setup(core: CoreSetup, plugins: PluginDeps) {
    plugins.usageCollection.registerCollector({
      type: 'MY_TYPE',
      fetch: async () => {
        const [coreStart] = await core.getStartServices();
        const internalRepo = coreStart.savedObjects.createInternalRepository();
        // ...
      },
    });
  }
  start() {}
}

[elasticmachine]

Pinging @elastic/kibana-platform (Team:Platform)

[pgayvallet]

@elastic/kibana-security I changed the type of the CoreStart mock and had to adapt one of your type in some test. That's the only change impacting your owned codebase.

[pgayvallet]

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

kibana/x-pack/plugins/licensing/server/plugin.test.ts

Lines 54 to 55 in 56041f0

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

So I had to compose this custom type instead.

[mshustov]

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()

[pgayvallet]

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

kibana/src/core/server/elasticsearch/elasticsearch_service.mock.ts

Lines 46 to 51 in 2d62ff2

	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

kibana/src/core/server/mocks.ts

Lines 121 to 122 in ea9a7b8

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

vs

kibana/src/core/server/mocks.ts

Lines 131 to 132 in ea9a7b8

	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?

[pgayvallet]

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:

kibana/typings/index.d.ts

Line 32 in eb03fd8

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

kibana/x-pack/typings/index.d.ts

Line 30 in 643f15c

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

[pgayvallet]

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

[pgayvallet]

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.

[pgayvallet]

The method is async, awaiting the returning promise was unnecessary.

[mshustov]

You are right. We use await excessively, even when it's not necessary async () => await do(). It's a matter of convention. Although it's not mentioned in core/conventions.md, so discussable.

[pgayvallet]

The only subtle difference will be the actual place it would throw from if the promise is rejected (I think?)

[mshustov]

I don't see the difference. I have the same output

UNHANDLED PROMISE REJECTION Error: not found
    at mostInner (/Users/mikhailshustov/work/kibana/src.js:21:9)
    at inner (/Users/mikhailshustov/work/kibana/src.js:25:16)
    at outer (/Users/mikhailshustov/work/kibana/src.js:29:16)

for

async function mostInner() {
  throw new Error('not found');
}

async function inner() {
  return await mostInner();
}

async function outer() {
  return await inner();
}

outer();
process.on('unhandledRejection', error => {
  console.error('UNHANDLED PROMISE REJECTION', error);
});

and

function mostInner() {
  new Error('not found');
}

async function inner() {
  return mostInner();
}

async function outer() {
  return await inner();
}

outer();
process.on('unhandledRejection', error => {
  console.error('UNHANDLED PROMISE REJECTION', error);
});

[pgayvallet]

Yea you are right, tried it too this weekend. This is catched at the higher level anyway and gets the full stack in any case

[azasypkin]

Security related changes LGTM

[joshdover]

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

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: ea95905

History

• 💚 Build #21031 succeeded 7e02978

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