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

Migrate /translations route to core #83280

Merged
merged 3 commits into from
Nov 16, 2020
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
5 changes: 5 additions & 0 deletions src/core/server/i18n/i18n_service.test.mocks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,3 +26,8 @@ export const initTranslationsMock = jest.fn();
jest.doMock('./init_translations', () => ({
initTranslations: initTranslationsMock,
}));

export const registerRoutesMock = jest.fn();
jest.doMock('./routes', () => ({
registerRoutes: registerRoutesMock,
}));
26 changes: 22 additions & 4 deletions src/core/server/i18n/i18n_service.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,18 @@
* under the License.
*/

import { getKibanaTranslationFilesMock, initTranslationsMock } from './i18n_service.test.mocks';
import {
getKibanaTranslationFilesMock,
initTranslationsMock,
registerRoutesMock,
} from './i18n_service.test.mocks';

import { BehaviorSubject } from 'rxjs';
import { I18nService } from './i18n_service';

import { configServiceMock } from '../config/mocks';
import { mockCoreContext } from '../core_context.mock';
import { httpServiceMock } from '../http/http_service.mock';

const getConfigService = (locale = 'en') => {
const configService = configServiceMock.create();
Expand All @@ -41,21 +46,24 @@ const getConfigService = (locale = 'en') => {
describe('I18nService', () => {
let service: I18nService;
let configService: ReturnType<typeof configServiceMock.create>;
let http: ReturnType<typeof httpServiceMock.createInternalSetupContract>;

beforeEach(() => {
jest.clearAllMocks();
configService = getConfigService();

const coreContext = mockCoreContext.create({ configService });
service = new I18nService(coreContext);

http = httpServiceMock.createInternalSetupContract();
});

describe('#setup', () => {
it('calls `getKibanaTranslationFiles` with the correct parameters', async () => {
getKibanaTranslationFilesMock.mockResolvedValue([]);

const pluginPaths = ['/pathA', '/pathB'];
await service.setup({ pluginPaths });
await service.setup({ pluginPaths, http });

expect(getKibanaTranslationFilesMock).toHaveBeenCalledTimes(1);
expect(getKibanaTranslationFilesMock).toHaveBeenCalledWith('en', pluginPaths);
Expand All @@ -65,17 +73,27 @@ describe('I18nService', () => {
const translationFiles = ['/path/to/file', 'path/to/another/file'];
getKibanaTranslationFilesMock.mockResolvedValue(translationFiles);

await service.setup({ pluginPaths: [] });
await service.setup({ pluginPaths: [], http });

expect(initTranslationsMock).toHaveBeenCalledTimes(1);
expect(initTranslationsMock).toHaveBeenCalledWith('en', translationFiles);
});

it('calls `registerRoutesMock` with the correct parameters', async () => {
await service.setup({ pluginPaths: [], http });

expect(registerRoutesMock).toHaveBeenCalledTimes(1);
expect(registerRoutesMock).toHaveBeenCalledWith({
locale: 'en',
router: expect.any(Object),
});
});

it('returns accessors for locale and translation files', async () => {
const translationFiles = ['/path/to/file', 'path/to/another/file'];
getKibanaTranslationFilesMock.mockResolvedValue(translationFiles);

const { getLocale, getTranslationFiles } = await service.setup({ pluginPaths: [] });
const { getLocale, getTranslationFiles } = await service.setup({ pluginPaths: [], http });

expect(getLocale()).toEqual('en');
expect(getTranslationFiles()).toEqual(translationFiles);
Expand Down
8 changes: 7 additions & 1 deletion src/core/server/i18n/i18n_service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,14 @@ import { take } from 'rxjs/operators';
import { Logger } from '../logging';
import { IConfigService } from '../config';
import { CoreContext } from '../core_context';
import { InternalHttpServiceSetup } from '../http';
import { config as i18nConfigDef, I18nConfigType } from './i18n_config';
import { getKibanaTranslationFiles } from './get_kibana_translation_files';
import { initTranslations } from './init_translations';
import { registerRoutes } from './routes';

interface SetupDeps {
http: InternalHttpServiceSetup;
pluginPaths: string[];
}

Expand Down Expand Up @@ -53,7 +56,7 @@ export class I18nService {
this.configService = coreContext.configService;
}

public async setup({ pluginPaths }: SetupDeps): Promise<I18nServiceSetup> {
public async setup({ pluginPaths, http }: SetupDeps): Promise<I18nServiceSetup> {
const i18nConfig = await this.configService
.atPath<I18nConfigType>(i18nConfigDef.path)
.pipe(take(1))
Expand All @@ -67,6 +70,9 @@ export class I18nService {
this.log.debug(`Using translation files: [${translationFiles.join(', ')}]`);
await initTranslations(locale, translationFiles);

const router = http.createRouter('');
registerRoutes({ router, locale });

return {
getLocale: () => locale,
getTranslationFiles: () => translationFiles,
Expand Down
25 changes: 25 additions & 0 deletions src/core/server/i18n/routes/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
/*
* 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 { IRouter } from '../../http';
import { registerTranslationsRoute } from './translations';

export const registerRoutes = ({ router, locale }: { router: IRouter; locale: string }) => {
registerTranslationsRoute(router, locale);
};
69 changes: 69 additions & 0 deletions src/core/server/i18n/routes/translations.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
/*
* 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 { createHash } from 'crypto';
import { i18n } from '@kbn/i18n';
import { schema } from '@kbn/config-schema';
import { IRouter } from '../../http';

interface TranslationCache {
translations: string;
hash: string;
}

export const registerTranslationsRoute = (router: IRouter, locale: string) => {
let translationCache: TranslationCache;

router.get(
{
path: '/translations/{locale}.json',
validate: {
params: schema.object({
locale: schema.string(),
}),
},
options: {
authRequired: false,
},
},
(ctx, req, res) => {
if (req.params.locale.toLowerCase() !== locale.toLowerCase()) {
return res.notFound({
body: `Unknown locale: ${req.params.locale}`,
});
}
if (!translationCache) {
const translations = JSON.stringify(i18n.getTranslation());
const hash = createHash('sha1').update(translations).digest('hex');
translationCache = {
translations,
hash,
};
}
return res.ok({
headers: {
'content-type': 'application/json',
'cache-control': 'must-revalidate',
etag: translationCache.hash,
},
body: translationCache.translations,
Comment on lines +60 to +65
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed with @restrry, we ideally should use cache-control instead of etag here in dist mode to avoid an unnecessary roundtrip. However as this requires some refactoring to be able to add a path prefix,suffix or query param to invalidate the request when the kibana version is bumped (as we do with static assets), it will be done at a later time. I will create an issue for that before the current PR got merged.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created #83409

});
}
);
};
6 changes: 3 additions & 3 deletions src/core/server/server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -131,9 +131,6 @@ export class Server {
await ensureValidConfiguration(this.configService, legacyConfigSetup);
}

// setup i18n prior to any other service, to have translations ready
const i18nServiceSetup = await this.i18n.setup({ pluginPaths });

const contextServiceSetup = this.context.setup({
// We inject a fake "legacy plugin" with dependencies on every plugin so that legacy plugins:
// 1) Can access context from any KP plugin
Expand All @@ -149,6 +146,9 @@ export class Server {
context: contextServiceSetup,
});

// setup i18n prior to any other service, to have translations ready
const i18nServiceSetup = await this.i18n.setup({ http: httpSetup, pluginPaths });
Comment on lines +149 to +150
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had to move this after the http service setup to register the route. I could move i18n initialization in another method (like we did for plugin.discover) to keep the i18n setup before context and http, but I don't think this is really necessary


const capabilitiesSetup = this.capabilities.setup({ http: httpSetup });

const elasticsearchServiceSetup = await this.elasticsearch.setup({
Expand Down
32 changes: 0 additions & 32 deletions src/legacy/ui/ui_render/ui_render_mixin.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,7 @@
* under the License.
*/

import { createHash } from 'crypto';
import Boom from '@hapi/boom';
import { i18n } from '@kbn/i18n';
import * as UiSharedDeps from '@kbn/ui-shared-deps';
import { KibanaRequest } from '../../../core/server';
import { AppBootstrap } from './bootstrap';
Expand All @@ -37,36 +35,6 @@ import { getApmConfig } from '../apm';
* @param {KbnServer['config']} config
*/
export function uiRenderMixin(kbnServer, server, config) {
const translationsCache = { translations: null, hash: null };
server.route({
path: '/translations/{locale}.json',
method: 'GET',
config: { auth: false },
handler(request, h) {
// Kibana server loads translations only for a single locale
// that is specified in `i18n.locale` config value.
const { locale } = request.params;
if (i18n.getLocale() !== locale.toLowerCase()) {
throw Boom.notFound(`Unknown locale: ${locale}`);
}

// Stringifying thousands of labels and calculating hash on the resulting
// string can be expensive so it makes sense to do it once and cache.
if (translationsCache.translations == null) {
translationsCache.translations = JSON.stringify(i18n.getTranslation());
translationsCache.hash = createHash('sha1')
.update(translationsCache.translations)
.digest('hex');
}

return h
.response(translationsCache.translations)
.header('cache-control', 'must-revalidate')
.header('content-type', 'application/json')
.etag(translationsCache.hash);
},
});

const authEnabled = !!server.auth.settings.default;
server.route({
path: '/bootstrap.js',
Expand Down
55 changes: 55 additions & 0 deletions test/api_integration/apis/core/compression.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
/*
* 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 expect from '@kbn/expect';
import { FtrProviderContext } from '../../ftr_provider_context';

export default function ({ getService }: FtrProviderContext) {
const supertest = getService('supertest');

describe('compression', () => {
it(`uses compression when there isn't a referer`, async () => {
Comment on lines +22 to +26
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Extracted / converted to TS from test/api_integration/apis/core/index.js

await supertest
.get('/app/kibana')
.set('accept-encoding', 'gzip')
.then((response) => {
expect(response.header).to.have.property('content-encoding', 'gzip');
});
});

it(`uses compression when there is a whitelisted referer`, async () => {
await supertest
.get('/app/kibana')
.set('accept-encoding', 'gzip')
.set('referer', 'https://some-host.com')
.then((response) => {
expect(response.header).to.have.property('content-encoding', 'gzip');
});
});

it(`doesn't use compression when there is a non-whitelisted referer`, async () => {
await supertest
.get('/app/kibana')
.set('accept-encoding', 'gzip')
.set('referer', 'https://other.some-host.com')
.then((response) => {
expect(response.header).not.to.have.property('content-encoding');
});
});
});
}
56 changes: 0 additions & 56 deletions test/api_integration/apis/core/index.js

This file was deleted.

Loading