Skip to content
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
2 changes: 1 addition & 1 deletion packages/core/i18n/core-i18n-server-internal/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,5 +6,5 @@
* Side Public License, v 1.
*/

export type { I18nConfigType } from './src';
export type { I18nConfigType, InternalI18nServicePreboot } from './src';
export { config, I18nService } from './src';
Original file line number Diff line number Diff line change
Expand Up @@ -37,12 +37,13 @@ describe('I18nService', () => {
let configService: ReturnType<typeof configServiceMock.create>;
let httpPreboot: ReturnType<typeof httpServiceMock.createInternalPrebootContract>;
let httpSetup: ReturnType<typeof httpServiceMock.createInternalSetupContract>;
let coreContext: ReturnType<typeof mockCoreContext.create>;

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

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

httpPreboot = httpServiceMock.createInternalPrebootContract();
Expand Down Expand Up @@ -73,13 +74,15 @@ describe('I18nService', () => {
expect(initTranslationsMock).toHaveBeenCalledWith('en', translationFiles);
});

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

expect(registerRoutesMock).toHaveBeenCalledTimes(1);
expect(registerRoutesMock).toHaveBeenCalledWith({
locale: 'en',
router: expect.any(Object),
isDist: coreContext.env.packageInfo.dist,
translationHash: expect.any(String),
});
});
});
Expand Down Expand Up @@ -114,13 +117,15 @@ describe('I18nService', () => {
expect(initTranslationsMock).toHaveBeenCalledWith('en', translationFiles);
});

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

expect(registerRoutesMock).toHaveBeenCalledTimes(1);
expect(registerRoutesMock).toHaveBeenCalledWith({
locale: 'en',
router: expect.any(Object),
isDist: coreContext.env.packageInfo.dist,
translationHash: expect.any(String),
});
});

Expand Down
36 changes: 29 additions & 7 deletions packages/core/i18n/core-i18n-server-internal/src/i18n_service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@
*/

import { firstValueFrom } from 'rxjs';
import { createHash } from 'crypto';
import { i18n, Translation } from '@kbn/i18n';
import type { Logger } from '@kbn/logging';
import type { IConfigService } from '@kbn/config';
import type { CoreContext } from '@kbn/core-base-server-internal';
Expand All @@ -30,29 +32,42 @@ export interface SetupDeps {
pluginPaths: string[];
}

export interface InternalI18nServicePreboot {
getTranslationHash(): string;
}

export class I18nService {
private readonly log: Logger;
private readonly configService: IConfigService;

constructor(coreContext: CoreContext) {
constructor(private readonly coreContext: CoreContext) {
this.log = coreContext.logger.get('i18n');
this.configService = coreContext.configService;
}

public async preboot({ pluginPaths, http }: PrebootDeps) {
const { locale } = await this.initTranslations(pluginPaths);
http.registerRoutes('', (router) => registerRoutes({ router, locale }));
public async preboot({ pluginPaths, http }: PrebootDeps): Promise<InternalI18nServicePreboot> {
const { locale, translationHash } = await this.initTranslations(pluginPaths);
const { dist: isDist } = this.coreContext.env.packageInfo;
http.registerRoutes('', (router) =>
registerRoutes({ router, locale, isDist, translationHash })
);

return {
getTranslationHash: () => translationHash,
};
}

public async setup({ pluginPaths, http }: SetupDeps): Promise<I18nServiceSetup> {
const { locale, translationFiles } = await this.initTranslations(pluginPaths);
const { locale, translationFiles, translationHash } = await this.initTranslations(pluginPaths);

const router = http.createRouter('');
registerRoutes({ router, locale });
const { dist: isDist } = this.coreContext.env.packageInfo;
registerRoutes({ router, locale, isDist, translationHash });

return {
getLocale: () => locale,
getTranslationFiles: () => translationFiles,
getTranslationHash: () => translationHash,
};
}

Expand All @@ -69,6 +84,13 @@ export class I18nService {
this.log.debug(`Using translation files: [${translationFiles.join(', ')}]`);
await initTranslations(locale, translationFiles);

return { locale, translationFiles };
const translationHash = getTranslationHash(i18n.getTranslation());

return { locale, translationFiles, translationHash };
}
}

const getTranslationHash = (translations: Translation) => {
const serialized = JSON.stringify(translations);
return createHash('sha256').update(serialized).digest('hex').slice(0, 12);
};
1 change: 1 addition & 0 deletions packages/core/i18n/core-i18n-server-internal/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,4 @@
export { config } from './i18n_config';
export type { I18nConfigType } from './i18n_config';
export { I18nService } from './i18n_service';
export type { InternalI18nServicePreboot } from './i18n_service';
14 changes: 12 additions & 2 deletions packages/core/i18n/core-i18n-server-internal/src/routes/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,16 @@
import type { IRouter } from '@kbn/core-http-server';
import { registerTranslationsRoute } from './translations';

export const registerRoutes = ({ router, locale }: { router: IRouter; locale: string }) => {
registerTranslationsRoute(router, locale);
export const registerRoutes = ({
router,
locale,
isDist,
translationHash,
}: {
router: IRouter;
locale: string;
isDist: boolean;
translationHash: string;
}) => {
registerTranslationsRoute({ router, locale, isDist, translationHash });
};
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,27 @@ import { registerTranslationsRoute } from './translations';
describe('registerTranslationsRoute', () => {
test('registers route with expected options', () => {
const router = mockRouter.create();
registerTranslationsRoute(router, 'en');
expect(router.get).toHaveBeenCalledTimes(1);
registerTranslationsRoute({
router,
locale: 'en',
isDist: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

I may have missed it, but is it worth having a jest integration style test for this "isDist" Boolean and how it affects response headers?

translationHash: 'XXXX',
});
expect(router.get).toHaveBeenCalledTimes(2);
expect(router.get).toHaveBeenNthCalledWith(
1,
expect.objectContaining({ options: { access: 'public', authRequired: false } }),
expect.objectContaining({
path: '/translations/{locale}.json',
options: { access: 'public', authRequired: false },
}),
expect.any(Function)
);
expect(router.get).toHaveBeenNthCalledWith(
2,
expect.objectContaining({
path: '/translations/XXXX/{locale}.json',
options: { access: 'public', authRequired: false },
}),
expect.any(Function)
);
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,54 +6,81 @@
* Side Public License, v 1.
*/

import { createHash } from 'crypto';
import { i18n } from '@kbn/i18n';
import { schema } from '@kbn/config-schema';
import type { IRouter } from '@kbn/core-http-server';

const MINUTE = 60;
const HOUR = 60 * MINUTE;
const DAY = 24 * HOUR;

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

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

router.get(
{
path: '/translations/{locale}.json',
validate: {
params: schema.object({
locale: schema.string(),
}),
},
options: {
access: 'public',
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,
['/translations/{locale}.json', `/translations/${translationHash}/{locale}.json`].forEach(
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 kept the old route for two reasons:

  • functional tests against the endpoint (it's a pain to retrieve the translation hash from the API integration tests...)
  • BWC: some integrations/customers might be reaching this endpoint, you never know

Copy link
Member

Choose a reason for hiding this comment

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

This path is not a public API so i dont think we should worry about custom integrations especially since i dont see any use case for this.. Maybe worth creating an issue to remove it later on in a separate PR

(routePath) => {
router.get(
{
path: routePath,
validate: {
params: schema.object({
locale: schema.string(),
}),
},
options: {
access: 'public',
authRequired: false,
},
},
body: translationCache.translations,
});
(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());
translationCache = {
translations,
hash: translationHash,
};
}

let headers: Record<string, string>;
if (isDist) {
headers = {
'content-type': 'application/json',
'cache-control': `public, max-age=${365 * DAY}, immutable`,
};
} else {
headers = {
'content-type': 'application/json',
'cache-control': 'must-revalidate',
etag: translationCache.hash,
};
Comment on lines +65 to +75
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 kept the etag cache control for non-distribution build, even if I'm not sure how useful this is, given the translation hash will change when translations are added/removed/changed.

}

return res.ok({
headers,
body: translationCache.translations,
});
}
);
}
);
};
Original file line number Diff line number Diff line change
Expand Up @@ -7,17 +7,29 @@
*/

import type { PublicMethodsOf } from '@kbn/utility-types';
import type { I18nService } from '@kbn/core-i18n-server-internal';
import type { I18nService, InternalI18nServicePreboot } from '@kbn/core-i18n-server-internal';
import type { I18nServiceSetup } from '@kbn/core-i18n-server';

const createSetupContractMock = () => {
const mock: jest.Mocked<I18nServiceSetup> = {
getLocale: jest.fn(),
getTranslationFiles: jest.fn(),
getTranslationHash: jest.fn(),
};

mock.getLocale.mockReturnValue('en');
mock.getTranslationFiles.mockReturnValue([]);
mock.getTranslationHash.mockReturnValue('MOCK_HASH');

return mock;
};

const createInternalPrebootMock = () => {
const mock: jest.Mocked<InternalI18nServicePreboot> = {
getTranslationHash: jest.fn(),
};

mock.getTranslationHash.mockReturnValue('MOCK_HASH');

return mock;
};
Expand All @@ -38,4 +50,5 @@ const createMock = () => {
export const i18nServiceMock = {
create: createMock,
createSetupContract: createSetupContractMock,
createInternalPrebootContract: createInternalPrebootMock,
};
5 changes: 5 additions & 0 deletions packages/core/i18n/core-i18n-server/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,4 +19,9 @@ export interface I18nServiceSetup {
* Return the absolute paths to translation files currently in use.
*/
getTranslationFiles(): string[];

/**
* Returns the hash generated from the current translations.
*/
getTranslationHash(): string;
}
Loading