Skip to content

Commit

Permalink
[NP] Remove observables from es internal contract (elastic#54556)
Browse files Browse the repository at this point in the history
* request context always uses the latest es client

* update integration tests

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
  • Loading branch information
2 people authored and chrisronline committed Jan 13, 2020
1 parent ed9b805 commit 9db404e
Show file tree
Hide file tree
Showing 7 changed files with 22 additions and 60 deletions.
2 changes: 0 additions & 2 deletions src/core/server/elasticsearch/elasticsearch_service.mock.ts
Original file line number Diff line number Diff line change
Expand Up @@ -74,8 +74,6 @@ const createInternalSetupContractMock = () => {
legacy: {
config$: new BehaviorSubject({} as ElasticsearchConfig),
},
adminClient$: new BehaviorSubject(createClusterClientMock()),
dataClient$: new BehaviorSubject(createClusterClientMock()),
};
setupContract.adminClient.asScoped.mockReturnValue(createScopedClusterClientMock());
setupContract.dataClient.asScoped.mockReturnValue(createScopedClusterClientMock());
Expand Down
40 changes: 1 addition & 39 deletions src/core/server/elasticsearch/elasticsearch_service.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import { first } from 'rxjs/operators';

import { MockClusterClient } from './elasticsearch_service.test.mocks';

import { BehaviorSubject, combineLatest } from 'rxjs';
import { BehaviorSubject } from 'rxjs';
import { Env } from '../config';
import { getEnvOptions } from '../config/__mocks__/env';
import { CoreContext } from '../core_context';
Expand Down Expand Up @@ -91,44 +91,6 @@ describe('#setup', () => {
expect(mockDataClusterClientInstance.callAsInternalUser).toHaveBeenCalledTimes(1);
});

it('returns data and admin client observables as a part of the contract', async () => {
const mockAdminClusterClientInstance = { close: jest.fn() };
const mockDataClusterClientInstance = { close: jest.fn() };
MockClusterClient.mockImplementationOnce(
() => mockAdminClusterClientInstance
).mockImplementationOnce(() => mockDataClusterClientInstance);

const setupContract = await elasticsearchService.setup(deps);

const [esConfig, adminClient, dataClient] = await combineLatest(
setupContract.legacy.config$,
setupContract.adminClient$,
setupContract.dataClient$
)
.pipe(first())
.toPromise();

expect(adminClient).toBe(mockAdminClusterClientInstance);
expect(dataClient).toBe(mockDataClusterClientInstance);

expect(MockClusterClient).toHaveBeenCalledTimes(2);
expect(MockClusterClient).toHaveBeenNthCalledWith(
1,
esConfig,
expect.objectContaining({ context: ['elasticsearch', 'admin'] }),
undefined
);
expect(MockClusterClient).toHaveBeenNthCalledWith(
2,
esConfig,
expect.objectContaining({ context: ['elasticsearch', 'data'] }),
expect.any(Function)
);

expect(mockAdminClusterClientInstance.close).not.toHaveBeenCalled();
expect(mockDataClusterClientInstance.close).not.toHaveBeenCalled();
});

describe('#createClient', () => {
it('allows to specify config properties', async () => {
const setupContract = await elasticsearchService.setup(deps);
Expand Down
2 changes: 0 additions & 2 deletions src/core/server/elasticsearch/elasticsearch_service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -152,8 +152,6 @@ export class ElasticsearchService implements CoreService<InternalElasticsearchSe
return {
legacy: { config$: clients$.pipe(map(clients => clients.config)) },

adminClient$,
dataClient$,
adminClient,
dataClient,

Expand Down
3 changes: 0 additions & 3 deletions src/core/server/elasticsearch/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,4 @@ export interface InternalElasticsearchServiceSetup extends ElasticsearchServiceS
readonly legacy: {
readonly config$: Observable<ElasticsearchConfig>;
};

readonly adminClient$: Observable<IClusterClient>;
readonly dataClient$: Observable<IClusterClient>;
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,11 @@
* specific language governing permissions and limitations
* under the License.
*/
import { elasticsearchServiceMock } from '../../elasticsearch/elasticsearch_service.mock';

export const clusterClientMock = jest.fn();
jest.doMock('../../elasticsearch/scoped_cluster_client', () => ({
ScopedClusterClient: clusterClientMock,
ScopedClusterClient: clusterClientMock.mockImplementation(function() {
return elasticsearchServiceMock.createScopedClusterClient();
}),
}));
22 changes: 15 additions & 7 deletions src/core/server/http/integration_tests/core_services.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ describe('http service', () => {
const { http } = await root.setup();
const { registerAuth } = http;

await registerAuth((req, res, toolkit) => {
registerAuth((req, res, toolkit) => {
return toolkit.authenticated({ responseHeaders: authResponseHeader });
});

Expand All @@ -157,7 +157,7 @@ describe('http service', () => {
const { http } = await root.setup();
const { registerAuth } = http;

await registerAuth((req, res, toolkit) => {
registerAuth((req, res, toolkit) => {
return toolkit.authenticated({ responseHeaders: authResponseHeader });
});

Expand Down Expand Up @@ -222,12 +222,15 @@ describe('http service', () => {
const { http } = await root.setup();
const { registerAuth, createRouter } = http;

await registerAuth((req, res, toolkit) =>
toolkit.authenticated({ requestHeaders: authHeaders })
);
registerAuth((req, res, toolkit) => toolkit.authenticated({ requestHeaders: authHeaders }));

const router = createRouter('/new-platform');
router.get({ path: '/', validate: false }, (context, req, res) => res.ok());
router.get({ path: '/', validate: false }, async (context, req, res) => {
// it forces client initialization since the core creates them lazily.
await context.core.elasticsearch.adminClient.callAsCurrentUser('ping');
await context.core.elasticsearch.dataClient.callAsCurrentUser('ping');
return res.ok();
});

await root.start();

Expand All @@ -247,7 +250,12 @@ describe('http service', () => {
const { createRouter } = http;

const router = createRouter('/new-platform');
router.get({ path: '/', validate: false }, (context, req, res) => res.ok());
router.get({ path: '/', validate: false }, async (context, req, res) => {
// it forces client initialization since the core creates them lazily.
await context.core.elasticsearch.adminClient.callAsCurrentUser('ping');
await context.core.elasticsearch.dataClient.callAsCurrentUser('ping');
return res.ok();
});

await root.start();

Expand Down
8 changes: 2 additions & 6 deletions src/core/server/server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
* under the License.
*/

import { take } from 'rxjs/operators';
import { Type } from '@kbn/config-schema';

import {
Expand Down Expand Up @@ -216,9 +215,6 @@ export class Server {
coreId,
'core',
async (context, req, res): Promise<RequestHandlerContext['core']> => {
// it consumes elasticsearch observables to provide the same client throughout the context lifetime.
const adminClient = await coreSetup.elasticsearch.adminClient$.pipe(take(1)).toPromise();
const dataClient = await coreSetup.elasticsearch.dataClient$.pipe(take(1)).toPromise();
const savedObjectsClient = coreSetup.savedObjects.getScopedClient(req);
const uiSettingsClient = coreSetup.uiSettings.asScopedToClient(savedObjectsClient);

Expand All @@ -230,8 +226,8 @@ export class Server {
client: savedObjectsClient,
},
elasticsearch: {
adminClient: adminClient.asScoped(req),
dataClient: dataClient.asScoped(req),
adminClient: coreSetup.elasticsearch.adminClient.asScoped(req),
dataClient: coreSetup.elasticsearch.dataClient.asScoped(req),
},
uiSettings: {
client: uiSettingsClient,
Expand Down

0 comments on commit 9db404e

Please sign in to comment.