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

[uiSettings] always use the latest config document to create the new one #159649

Merged
merged 8 commits into from
Jun 19, 2023
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import { Subject, Observable, firstValueFrom, of } from 'rxjs';
import { filter, take, switchMap } from 'rxjs/operators';
import type { Logger } from '@kbn/logging';
import { stripVersionQualifier } from '@kbn/std';
import type { ServiceStatus } from '@kbn/core-status-common';
import type { CoreContext, CoreService } from '@kbn/core-base-server-internal';
import type { DocLinksServiceStart } from '@kbn/core-doc-links-server';
Expand Down Expand Up @@ -106,9 +107,7 @@ export class SavedObjectsService

constructor(private readonly coreContext: CoreContext) {
this.logger = coreContext.logger.get('savedobjects-service');
this.kibanaVersion = SavedObjectsService.stripVersionQualifier(
this.coreContext.env.packageInfo.version
);
this.kibanaVersion = stripVersionQualifier(this.coreContext.env.packageInfo.version);
}

public async setup(setupDeps: SavedObjectsSetupDeps): Promise<InternalSavedObjectsServiceSetup> {
Expand Down Expand Up @@ -384,12 +383,4 @@ export class SavedObjectsService
nodeRoles: nodeInfo.roles,
});
}

/**
* Coerce a semver-like string (x.y.z-SNAPSHOT) or prerelease version (x.y.z-alpha)
* to regular semver (x.y.z).
*/
private static stripVersionQualifier(version: string) {
return version.split('-')[0];
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@
"@kbn/utils",
"@kbn/core-http-router-server-internal",
"@kbn/logging-mocks",
"@kbn/std",
],
"exclude": [
"target/**/*",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,59 @@ describe('getUpgradeableConfig', () => {
expect(result).toEqual(savedConfig);
});

it('uses the latest config when multiple are found', async () => {
const savedObjectsClient = savedObjectsClientMock.create();
savedObjectsClient.find.mockResolvedValue({
saved_objects: [
{ id: '7.2.0', attributes: 'foo' },
Copy link
Contributor

Choose a reason for hiding this comment

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

Could there ever be a scenario where we have an invalid version value in the id field? If so, would be nice to have test coverage for it. If not, ignore this!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In theory, there shouldn't, but life alway finds a way, so I guess adding a test wouldn't hurt

{ id: '7.3.0', attributes: 'foo' },
],
} as SavedObjectsFindResponse);

const result = await getUpgradeableConfig({
savedObjectsClient,
version: '7.5.0',
type: 'config',
});
expect(result!.id).toBe('7.3.0');
});

it('uses the latest config when multiple are found with rc qualifier', async () => {
const savedObjectsClient = savedObjectsClientMock.create();
savedObjectsClient.find.mockResolvedValue({
saved_objects: [
{ id: '7.2.0', attributes: 'foo' },
{ id: '7.3.0', attributes: 'foo' },
{ id: '7.5.0-rc1', attributes: 'foo' },
],
} as SavedObjectsFindResponse);

const result = await getUpgradeableConfig({
savedObjectsClient,
version: '7.5.0',
type: 'config',
});
expect(result!.id).toBe('7.5.0-rc1');
});

it('ignores documents with malformed ids', async () => {
const savedObjectsClient = savedObjectsClientMock.create();
savedObjectsClient.find.mockResolvedValue({
saved_objects: [
{ id: 'not-a-semver', attributes: 'foo' },
{ id: '7.2.0', attributes: 'foo' },
{ id: '7.3.0', attributes: 'foo' },
],
} as SavedObjectsFindResponse);

const result = await getUpgradeableConfig({
savedObjectsClient,
version: '7.5.0',
type: 'config',
});
expect(result!.id).toBe('7.3.0');
});

it('finds saved config with RC version === Kibana version', async () => {
const savedConfig = { id: '7.5.0-rc1', attributes: 'foo' };
const savedObjectsClient = savedObjectsClientMock.create();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,11 @@
* Side Public License, v 1.
*/

import type { SavedObjectsClientContract } from '@kbn/core-saved-objects-api-server';
import Semver from 'semver';
import type {
SavedObjectsClientContract,
SavedObjectsFindResult,
} from '@kbn/core-saved-objects-api-server';
import type { ConfigAttributes } from '../saved_objects';
import { isConfigVersionUpgradeable } from './is_config_version_upgradeable';

Expand Down Expand Up @@ -47,11 +51,26 @@ export async function getUpgradeableConfig({
});

// try to find a config that we can upgrade
const findResult = savedConfigs.find((savedConfig) =>
const matchingResults = savedConfigs.filter((savedConfig) =>
isConfigVersionUpgradeable(savedConfig.id, version)
);
if (findResult) {
return { id: findResult.id, attributes: findResult.attributes };
const mostRecentConfig = getMostRecentConfig(matchingResults);
if (mostRecentConfig) {
return { id: mostRecentConfig.id, attributes: mostRecentConfig.attributes };
}
Comment on lines +54 to 60
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The root change. The code was making the assumption that sorting by buildNum was sufficient, but this field is indexed as keyword, not number, which cause the order to be wrong in some situations.

E.g

["9517", "16602", "63240"].sort()
>> (3) ['16602', '63240', '9517']

So we're now manually comparing the version of each matching document to make sure that the most recent is used.

return null;
}

const getMostRecentConfig = (
results: Array<SavedObjectsFindResult<UpgradeableConfigAttributes>>
): SavedObjectsFindResult<UpgradeableConfigAttributes> | undefined => {
return results.reduce<SavedObjectsFindResult<UpgradeableConfigAttributes> | undefined>(
(mostRecent, current) => {
if (!mostRecent) {
return current;
}
return Semver.gt(mostRecent.id, current.id) ? mostRecent : current;
},
undefined
);
};
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import { firstValueFrom, Observable } from 'rxjs';
import { mapToObject } from '@kbn/std';

import type { Logger } from '@kbn/logging';
import { stripVersionQualifier } from '@kbn/std';
import type { CoreContext, CoreService } from '@kbn/core-base-server-internal';
import type { InternalHttpServiceSetup } from '@kbn/core-http-server-internal';
import type { SavedObjectsClientContract } from '@kbn/core-saved-objects-api-server';
Expand All @@ -32,6 +33,7 @@ export interface SetupDeps {
http: InternalHttpServiceSetup;
savedObjects: InternalSavedObjectsServiceSetup;
}

type ClientType<T> = T extends 'global'
? UiSettingsGlobalClient
: T extends 'namespace'
Expand Down Expand Up @@ -109,7 +111,7 @@ export class UiSettingsService
const isNamespaceScope = scope === 'namespace';
const options = {
type: (isNamespaceScope ? 'config' : 'config-global') as 'config' | 'config-global',
id: version,
id: stripVersionQualifier(version),
Comment on lines 113 to +114
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For consistency with how we're treating qualified versions (e.g X.Y.Z-SNAPSHOT or X.Y.Z-RC1) in other parts of core, especially in SO code, we're now stripping qualifiers when providing the version to the uiSettings client.

Note that this isn't directly related to the fix, but it was trivial enough to include it there.

buildNum,
savedObjectsClient,
defaults: isNamespaceScope
Expand Down
1 change: 1 addition & 0 deletions packages/kbn-std/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,3 +29,4 @@ export {
} from './src/iteration';
export { ensureDeepObject } from './src/ensure_deep_object';
export { Semaphore } from './src/semaphore';
export { stripVersionQualifier } from './src/strip_version_qualifier';
15 changes: 15 additions & 0 deletions packages/kbn-std/src/strip_version_qualifier.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

/**
* Coerce a semver-like string (x.y.z-SNAPSHOT) or prerelease version (x.y.z-alpha)
* to regular semver (x.y.z).
*/
export function stripVersionQualifier(version: string): string {
return version.split('-')[0];
}
Comment on lines +13 to +15
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 from the SO service to be reused elsewhere

3 changes: 2 additions & 1 deletion x-pack/test/functional/apps/spaces/enter_space.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
* 2.0.
*/

import { stripVersionQualifier } from '@kbn/std';
import { FtrProviderContext } from '../../ftr_provider_context';

export default function enterSpaceFunctionalTests({
Expand Down Expand Up @@ -32,7 +33,7 @@ export default function enterSpaceFunctionalTests({
{ space: 'another-space' }
);
const config = await kibanaServer.savedObjects.get({
id: await kibanaServer.version.get(),
id: stripVersionQualifier(await kibanaServer.version.get()),
type: 'config',
Comment on lines 35 to 37
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only FTR test that was doing direct config access by ID using the SO client. Had to adapt for the version change described in previous comment.

});
await kibanaServer.savedObjects.update({
Expand Down