From 13bf29f9cda324a45ef766bb8c1d697cecc2f82b Mon Sep 17 00:00:00 2001 From: Pierre Gayvallet Date: Thu, 6 Oct 2022 12:20:04 +0200 Subject: [PATCH] Add integration test tracking changes performed on all SO types migration metadata (#142306) * create empty test helper package * create extractor * create hash extraction logic * fix pkg names * actually create the test * updating README * [CI] Auto-commit changed files from 'node scripts/generate codeowners' * [CI] Auto-commit changed files from 'node scripts/eslint --no-cache --fix' * documentation draft * add more fields and update snapshot * update review doc * update review documentation * more feedback * updating snapshot Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com> --- .github/CODEOWNERS | 1 + package.json | 2 + packages/BUILD.bazel | 2 + .../BUILD.bazel | 111 ++++++ .../README.md | 3 + .../index.ts | 11 + .../jest.config.js | 13 + .../kibana.jsonc | 7 + .../package.json | 8 + .../src/extract_migration_info.test.ts | 161 +++++++++ .../src/extract_migration_info.ts | 50 +++ .../src/get_migration_hash.test.ts | 336 ++++++++++++++++++ .../src/get_migration_hash.ts | 33 ++ .../tsconfig.json | 17 + .../docs/kib_core_reviewing_so_type_pr.mdx | 84 +++++ .../migrations/check_registered_types.test.ts | 150 ++++++++ yarn.lock | 8 + 17 files changed, 997 insertions(+) create mode 100644 packages/core/test-helpers/core-test-helpers-so-type-serializer/BUILD.bazel create mode 100644 packages/core/test-helpers/core-test-helpers-so-type-serializer/README.md create mode 100644 packages/core/test-helpers/core-test-helpers-so-type-serializer/index.ts create mode 100644 packages/core/test-helpers/core-test-helpers-so-type-serializer/jest.config.js create mode 100644 packages/core/test-helpers/core-test-helpers-so-type-serializer/kibana.jsonc create mode 100644 packages/core/test-helpers/core-test-helpers-so-type-serializer/package.json create mode 100644 packages/core/test-helpers/core-test-helpers-so-type-serializer/src/extract_migration_info.test.ts create mode 100644 packages/core/test-helpers/core-test-helpers-so-type-serializer/src/extract_migration_info.ts create mode 100644 packages/core/test-helpers/core-test-helpers-so-type-serializer/src/get_migration_hash.test.ts create mode 100644 packages/core/test-helpers/core-test-helpers-so-type-serializer/src/get_migration_hash.ts create mode 100644 packages/core/test-helpers/core-test-helpers-so-type-serializer/tsconfig.json create mode 100644 src/core/server/docs/kib_core_reviewing_so_type_pr.mdx create mode 100644 src/core/server/integration_tests/saved_objects/migrations/check_registered_types.test.ts diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index d64b7f9af1ab..2bdca7d7f825 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -833,6 +833,7 @@ packages/core/status/core-status-server-internal @elastic/kibana-core packages/core/status/core-status-server-mocks @elastic/kibana-core packages/core/test-helpers/core-test-helpers-deprecations-getters @elastic/kibana-core packages/core/test-helpers/core-test-helpers-http-setup-browser @elastic/kibana-core +packages/core/test-helpers/core-test-helpers-so-type-serializer @elastic/kibana-core packages/core/theme/core-theme-browser @elastic/kibana-core packages/core/theme/core-theme-browser-internal @elastic/kibana-core packages/core/theme/core-theme-browser-mocks @elastic/kibana-core diff --git a/package.json b/package.json index b50ef4d57b18..805bf5cdaa72 100644 --- a/package.json +++ b/package.json @@ -295,6 +295,7 @@ "@kbn/core-status-server-mocks": "link:bazel-bin/packages/core/status/core-status-server-mocks", "@kbn/core-test-helpers-deprecations-getters": "link:bazel-bin/packages/core/test-helpers/core-test-helpers-deprecations-getters", "@kbn/core-test-helpers-http-setup-browser": "link:bazel-bin/packages/core/test-helpers/core-test-helpers-http-setup-browser", + "@kbn/core-test-helpers-so-type-serializer": "link:bazel-bin/packages/core/test-helpers/core-test-helpers-so-type-serializer", "@kbn/core-theme-browser": "link:bazel-bin/packages/core/theme/core-theme-browser", "@kbn/core-theme-browser-internal": "link:bazel-bin/packages/core/theme/core-theme-browser-internal", "@kbn/core-theme-browser-mocks": "link:bazel-bin/packages/core/theme/core-theme-browser-mocks", @@ -1025,6 +1026,7 @@ "@types/kbn__core-status-server-mocks": "link:bazel-bin/packages/core/status/core-status-server-mocks/npm_module_types", "@types/kbn__core-test-helpers-deprecations-getters": "link:bazel-bin/packages/core/test-helpers/core-test-helpers-deprecations-getters/npm_module_types", "@types/kbn__core-test-helpers-http-setup-browser": "link:bazel-bin/packages/core/test-helpers/core-test-helpers-http-setup-browser/npm_module_types", + "@types/kbn__core-test-helpers-so-type-serializer": "link:bazel-bin/packages/core/test-helpers/core-test-helpers-so-type-serializer/npm_module_types", "@types/kbn__core-theme-browser": "link:bazel-bin/packages/core/theme/core-theme-browser/npm_module_types", "@types/kbn__core-theme-browser-internal": "link:bazel-bin/packages/core/theme/core-theme-browser-internal/npm_module_types", "@types/kbn__core-theme-browser-mocks": "link:bazel-bin/packages/core/theme/core-theme-browser-mocks/npm_module_types", diff --git a/packages/BUILD.bazel b/packages/BUILD.bazel index 97b7064f4cd7..aae46a111c05 100644 --- a/packages/BUILD.bazel +++ b/packages/BUILD.bazel @@ -160,6 +160,7 @@ filegroup( "//packages/core/status/core-status-server-mocks:build", "//packages/core/test-helpers/core-test-helpers-deprecations-getters:build", "//packages/core/test-helpers/core-test-helpers-http-setup-browser:build", + "//packages/core/test-helpers/core-test-helpers-so-type-serializer:build", "//packages/core/theme/core-theme-browser:build", "//packages/core/theme/core-theme-browser-internal:build", "//packages/core/theme/core-theme-browser-mocks:build", @@ -500,6 +501,7 @@ filegroup( "//packages/core/status/core-status-server-mocks:build_types", "//packages/core/test-helpers/core-test-helpers-deprecations-getters:build_types", "//packages/core/test-helpers/core-test-helpers-http-setup-browser:build_types", + "//packages/core/test-helpers/core-test-helpers-so-type-serializer:build_types", "//packages/core/theme/core-theme-browser:build_types", "//packages/core/theme/core-theme-browser-internal:build_types", "//packages/core/theme/core-theme-browser-mocks:build_types", diff --git a/packages/core/test-helpers/core-test-helpers-so-type-serializer/BUILD.bazel b/packages/core/test-helpers/core-test-helpers-so-type-serializer/BUILD.bazel new file mode 100644 index 000000000000..714250a907fb --- /dev/null +++ b/packages/core/test-helpers/core-test-helpers-so-type-serializer/BUILD.bazel @@ -0,0 +1,111 @@ +load("@npm//@bazel/typescript:index.bzl", "ts_config") +load("@build_bazel_rules_nodejs//:index.bzl", "js_library") +load("//src/dev/bazel:index.bzl", "jsts_transpiler", "pkg_npm", "pkg_npm_types", "ts_project") + +PKG_DIRNAME = "core-test-helpers-so-type-serializer" +PKG_REQUIRE_NAME = "@kbn/core-test-helpers-so-type-serializer" + +SOURCE_FILES = glob( + [ + "**/*.ts", + ], + exclude = [ + "**/*.config.js", + "**/*.mock.*", + "**/*.test.*", + "**/*.stories.*", + "**/__snapshots__/**", + "**/integration_tests/**", + "**/mocks/**", + "**/scripts/**", + "**/storybook/**", + "**/test_fixtures/**", + "**/test_helpers/**", + ], +) + +SRCS = SOURCE_FILES + +filegroup( + name = "srcs", + srcs = SRCS, +) + +NPM_MODULE_EXTRA_FILES = [ + "package.json", +] + +RUNTIME_DEPS = [ + "@npm//semver", + "//packages/kbn-std", +] + +TYPES_DEPS = [ + "@npm//@types/node", + "@npm//@types/jest", + "@npm//@types/semver", + "//packages/kbn-std:npm_module_types", + "//packages/core/saved-objects/core-saved-objects-common:npm_module_types", + "//packages/core/saved-objects/core-saved-objects-server:npm_module_types", +] + +jsts_transpiler( + name = "target_node", + srcs = SRCS, + build_pkg_name = package_name(), +) + +ts_config( + name = "tsconfig", + src = "tsconfig.json", + deps = [ + "//:tsconfig.base.json", + "//:tsconfig.bazel.json", + ], +) + +ts_project( + name = "tsc_types", + args = ['--pretty'], + srcs = SRCS, + deps = TYPES_DEPS, + declaration = True, + declaration_map = True, + emit_declaration_only = True, + out_dir = "target_types", + tsconfig = ":tsconfig", +) + +js_library( + name = PKG_DIRNAME, + srcs = NPM_MODULE_EXTRA_FILES, + deps = RUNTIME_DEPS + [":target_node"], + package_name = PKG_REQUIRE_NAME, + visibility = ["//visibility:public"], +) + +pkg_npm( + name = "npm_module", + deps = [":" + PKG_DIRNAME], +) + +filegroup( + name = "build", + srcs = [":npm_module"], + visibility = ["//visibility:public"], +) + +pkg_npm_types( + name = "npm_module_types", + srcs = SRCS, + deps = [":tsc_types"], + package_name = PKG_REQUIRE_NAME, + tsconfig = ":tsconfig", + visibility = ["//visibility:public"], +) + +filegroup( + name = "build_types", + srcs = [":npm_module_types"], + visibility = ["//visibility:public"], +) diff --git a/packages/core/test-helpers/core-test-helpers-so-type-serializer/README.md b/packages/core/test-helpers/core-test-helpers-so-type-serializer/README.md new file mode 100644 index 000000000000..d562272b9649 --- /dev/null +++ b/packages/core/test-helpers/core-test-helpers-so-type-serializer/README.md @@ -0,0 +1,3 @@ +# @kbn/core-test-helpers-so-type-serializer + +Utility package for savedObjects integration tests. diff --git a/packages/core/test-helpers/core-test-helpers-so-type-serializer/index.ts b/packages/core/test-helpers/core-test-helpers-so-type-serializer/index.ts new file mode 100644 index 000000000000..100acc28a200 --- /dev/null +++ b/packages/core/test-helpers/core-test-helpers-so-type-serializer/index.ts @@ -0,0 +1,11 @@ +/* + * 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. + */ + +export type { SavedObjectTypeMigrationInfo } from './src/extract_migration_info'; +export { extractMigrationInfo } from './src/extract_migration_info'; +export { getMigrationHash } from './src/get_migration_hash'; diff --git a/packages/core/test-helpers/core-test-helpers-so-type-serializer/jest.config.js b/packages/core/test-helpers/core-test-helpers-so-type-serializer/jest.config.js new file mode 100644 index 000000000000..66287aba24c2 --- /dev/null +++ b/packages/core/test-helpers/core-test-helpers-so-type-serializer/jest.config.js @@ -0,0 +1,13 @@ +/* + * 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. + */ + +module.exports = { + preset: '@kbn/test/jest_node', + rootDir: '../../../..', + roots: ['/packages/core/test-helpers/core-test-helpers-so-type-serializer'], +}; diff --git a/packages/core/test-helpers/core-test-helpers-so-type-serializer/kibana.jsonc b/packages/core/test-helpers/core-test-helpers-so-type-serializer/kibana.jsonc new file mode 100644 index 000000000000..4a4a765bbc51 --- /dev/null +++ b/packages/core/test-helpers/core-test-helpers-so-type-serializer/kibana.jsonc @@ -0,0 +1,7 @@ +{ + "type": "shared-common", + "id": "@kbn/core-test-helpers-so-type-serializer", + "owner": "@elastic/kibana-core", + "runtimeDeps": [], + "typeDeps": [], +} diff --git a/packages/core/test-helpers/core-test-helpers-so-type-serializer/package.json b/packages/core/test-helpers/core-test-helpers-so-type-serializer/package.json new file mode 100644 index 000000000000..95de844e70ed --- /dev/null +++ b/packages/core/test-helpers/core-test-helpers-so-type-serializer/package.json @@ -0,0 +1,8 @@ +{ + "name": "@kbn/core-test-helpers-so-type-serializer", + "private": true, + "version": "1.0.0", + "main": "./target_node/index.js", + "author": "Kibana Core", + "license": "SSPL-1.0 OR Elastic License 2.0" +} diff --git a/packages/core/test-helpers/core-test-helpers-so-type-serializer/src/extract_migration_info.test.ts b/packages/core/test-helpers/core-test-helpers-so-type-serializer/src/extract_migration_info.test.ts new file mode 100644 index 000000000000..47620aee90b3 --- /dev/null +++ b/packages/core/test-helpers/core-test-helpers-so-type-serializer/src/extract_migration_info.test.ts @@ -0,0 +1,161 @@ +/* + * 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. + */ + +import { schema } from '@kbn/config-schema'; +import type { SavedObjectsType } from '@kbn/core-saved-objects-server'; +import { extractMigrationInfo } from './extract_migration_info'; + +const createType = (parts: Partial): SavedObjectsType => ({ + name: 'test-type', + hidden: false, + namespaceType: 'multiple', + mappings: { properties: {} }, + ...parts, +}); + +const dummyMigration = jest.fn(); +const dummySchema = schema.object({}); + +describe('extractMigrationInfo', () => { + describe('simple fields', () => { + it('returns the `name` from the SO type', () => { + const type = createType({ name: 'my-type' }); + const output = extractMigrationInfo(type); + expect(output.name).toEqual('my-type'); + }); + + it('returns the `namespaceType` from the SO type', () => { + const type = createType({ namespaceType: 'multiple-isolated' }); + const output = extractMigrationInfo(type); + expect(output.namespaceType).toEqual('multiple-isolated'); + }); + + it('returns the `convertToMultiNamespaceTypeVersion` from the SO type', () => { + const type = createType({ convertToMultiNamespaceTypeVersion: '6.6.6' }); + const output = extractMigrationInfo(type); + expect(output.convertToMultiNamespaceTypeVersion).toEqual('6.6.6'); + }); + + it('returns the `convertToAliasScript` from the SO type', () => { + const type = createType({ convertToAliasScript: 'some_value' }); + const output = extractMigrationInfo(type); + expect(output.convertToAliasScript).toEqual('some_value'); + }); + + it('returns true for `hasExcludeOnUpgrade` if the SO type specifies `excludeOnUpgrade`', () => { + expect( + extractMigrationInfo(createType({ excludeOnUpgrade: jest.fn() })).hasExcludeOnUpgrade + ).toEqual(true); + expect( + extractMigrationInfo(createType({ excludeOnUpgrade: undefined })).hasExcludeOnUpgrade + ).toEqual(false); + }); + }); + + describe('migrations', () => { + it('returns the versions with registered migrations, sorted asc', () => { + const type = createType({ + migrations: { + '8.3.3': dummyMigration, + '7.17.7': dummyMigration, + '8.0.2': dummyMigration, + }, + }); + + const output = extractMigrationInfo(type); + + expect(output.migrationVersions).toEqual(['7.17.7', '8.0.2', '8.3.3']); + }); + + it('supports migration provider functions', () => { + const type = createType({ + migrations: () => ({ + '8.3.3': dummyMigration, + '7.17.7': dummyMigration, + '8.0.2': dummyMigration, + }), + }); + + const output = extractMigrationInfo(type); + + expect(output.migrationVersions).toEqual(['7.17.7', '8.0.2', '8.3.3']); + }); + + it('returns an empty list when migrations are not defined', () => { + const type = createType({ + migrations: undefined, + }); + + const output = extractMigrationInfo(type); + + expect(output.migrationVersions).toEqual([]); + }); + }); + + describe('schemas', () => { + it('returns the versions with registered schemas, sorted asc', () => { + const type = createType({ + schemas: { + '8.3.2': dummySchema, + '7.15.2': dummySchema, + '8.1.2': dummySchema, + }, + }); + + const output = extractMigrationInfo(type); + + expect(output.schemaVersions).toEqual(['7.15.2', '8.1.2', '8.3.2']); + }); + + it('supports schema provider functions', () => { + const type = createType({ + schemas: () => ({ + '8.3.2': dummySchema, + '7.15.2': dummySchema, + '8.1.2': dummySchema, + }), + }); + + const output = extractMigrationInfo(type); + + expect(output.schemaVersions).toEqual(['7.15.2', '8.1.2', '8.3.2']); + }); + + it('returns an empty list when schemas are not defined', () => { + const type = createType({ + schemas: undefined, + }); + + const output = extractMigrationInfo(type); + + expect(output.schemaVersions).toEqual([]); + }); + }); + + describe('mappings', () => { + it('returns a flattened version of the mappings', () => { + const type = createType({ + mappings: { + dynamic: false, + properties: { + description: { type: 'text' }, + hits: { type: 'integer', index: false, doc_values: false }, + }, + }, + }); + const output = extractMigrationInfo(type); + expect(output.mappings).toEqual({ + dynamic: false, + 'properties.description.type': 'text', + 'properties.hits.doc_values': false, + 'properties.hits.index': false, + 'properties.hits.type': 'integer', + }); + }); + }); +}); diff --git a/packages/core/test-helpers/core-test-helpers-so-type-serializer/src/extract_migration_info.ts b/packages/core/test-helpers/core-test-helpers-so-type-serializer/src/extract_migration_info.ts new file mode 100644 index 000000000000..c2f1c0b7aa9a --- /dev/null +++ b/packages/core/test-helpers/core-test-helpers-so-type-serializer/src/extract_migration_info.ts @@ -0,0 +1,50 @@ +/* + * 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. + */ + +import { compare as semverCompare } from 'semver'; +import { getFlattenedObject } from '@kbn/std'; +import type { SavedObjectsNamespaceType } from '@kbn/core-saved-objects-common'; +import type { SavedObjectsType } from '@kbn/core-saved-objects-server'; + +export interface SavedObjectTypeMigrationInfo { + name: string; + namespaceType: SavedObjectsNamespaceType; + convertToAliasScript?: string; + convertToMultiNamespaceTypeVersion?: string; + migrationVersions: string[]; + schemaVersions: string[]; + mappings: Record; + hasExcludeOnUpgrade: boolean; +} + +/** + * Extract all migration-relevant informations bound to given type in a serializable format. + * + * @param soType + */ +export const extractMigrationInfo = (soType: SavedObjectsType): SavedObjectTypeMigrationInfo => { + const migrationMap = + typeof soType.migrations === 'function' ? soType.migrations() : soType.migrations; + const migrationVersions = Object.keys(migrationMap ?? {}); + migrationVersions.sort(semverCompare); + + const schemaMap = typeof soType.schemas === 'function' ? soType.schemas() : soType.schemas; + const schemaVersions = Object.keys(schemaMap ?? {}); + schemaVersions.sort(semverCompare); + + return { + name: soType.name, + namespaceType: soType.namespaceType, + convertToAliasScript: soType.convertToAliasScript, + convertToMultiNamespaceTypeVersion: soType.convertToMultiNamespaceTypeVersion, + migrationVersions, + schemaVersions, + mappings: getFlattenedObject(soType.mappings ?? {}), + hasExcludeOnUpgrade: !!soType.excludeOnUpgrade, + }; +}; diff --git a/packages/core/test-helpers/core-test-helpers-so-type-serializer/src/get_migration_hash.test.ts b/packages/core/test-helpers/core-test-helpers-so-type-serializer/src/get_migration_hash.test.ts new file mode 100644 index 000000000000..2ac9e04172e8 --- /dev/null +++ b/packages/core/test-helpers/core-test-helpers-so-type-serializer/src/get_migration_hash.test.ts @@ -0,0 +1,336 @@ +/* + * 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. + */ + +import { schema } from '@kbn/config-schema'; +import type { SavedObjectsType } from '@kbn/core-saved-objects-server'; +import { getMigrationHash } from './get_migration_hash'; + +const createType = (parts: Partial = {}): SavedObjectsType => ({ + name: 'test-type', + hidden: false, + namespaceType: 'multiple', + mappings: { + dynamic: false, + properties: { + description: { type: 'text' }, + hits: { type: 'integer', index: false, doc_values: false }, + }, + }, + ...parts, +}); + +describe('getMigrationHash', () => { + it('returns the same hash for the exact same simple type', () => { + const type = createType(); + expect(getMigrationHash(type)).toEqual(getMigrationHash(type)); + }); + + describe('simple fields', () => { + it('returns different hashes if `name` changes', () => { + expect(getMigrationHash(createType({ name: 'typeA' }))).not.toEqual( + getMigrationHash(createType({ name: 'typeB' })) + ); + }); + it('returns different hashes if `namespaceType` changes', () => { + expect(getMigrationHash(createType({ namespaceType: 'single' }))).not.toEqual( + getMigrationHash(createType({ namespaceType: 'multiple' })) + ); + }); + it('returns different hashes if `convertToMultiNamespaceTypeVersion` changes', () => { + expect( + getMigrationHash(createType({ convertToMultiNamespaceTypeVersion: undefined })) + ).not.toEqual(getMigrationHash(createType({ convertToMultiNamespaceTypeVersion: '6.6.6' }))); + }); + it('returns different hashes if `convertToAliasScript` changes', () => { + expect(getMigrationHash(createType({ convertToAliasScript: undefined }))).not.toEqual( + getMigrationHash(createType({ convertToAliasScript: 'some_script' })) + ); + }); + it('returns different hashes if `excludeOnUpgrade` is defined or not', () => { + expect(getMigrationHash(createType({ excludeOnUpgrade: undefined }))).not.toEqual( + getMigrationHash(createType({ excludeOnUpgrade: jest.fn() })) + ); + }); + }); + + describe('migrations', () => { + it('returns same hash if same migration versions are registered', () => { + const typeA = createType({ + migrations: { + '7.17.1': jest.fn(), + '8.4.2': jest.fn(), + }, + }); + const typeB = createType({ + migrations: { + '7.17.1': jest.fn(), + '8.4.2': jest.fn(), + }, + }); + + expect(getMigrationHash(typeA)).toEqual(getMigrationHash(typeB)); + }); + + it('returns same hash if same migration versions are registered in different order', () => { + const typeA = createType({ + migrations: { + '9.1.3': jest.fn(), + '7.17.1': jest.fn(), + '8.4.2': jest.fn(), + }, + }); + const typeB = createType({ + migrations: { + '8.4.2': jest.fn(), + '9.1.3': jest.fn(), + '7.17.1': jest.fn(), + }, + }); + + expect(getMigrationHash(typeA)).toEqual(getMigrationHash(typeB)); + }); + + it('returns same hash if same migration versions are registered using record + function', () => { + const typeA = createType({ + migrations: { + '9.1.3': jest.fn(), + '7.17.1': jest.fn(), + '8.4.2': jest.fn(), + }, + }); + const typeB = createType({ + migrations: () => ({ + '8.4.2': jest.fn(), + '9.1.3': jest.fn(), + '7.17.1': jest.fn(), + }), + }); + + expect(getMigrationHash(typeA)).toEqual(getMigrationHash(typeB)); + }); + + it('returns different hashes if different migration versions are registered', () => { + const typeA = createType({ + migrations: { + '7.17.1': jest.fn(), + '8.4.2': jest.fn(), + }, + }); + const typeB = createType({ + migrations: { + '7.17.69': jest.fn(), + '42.0.0': jest.fn(), + }, + }); + + expect(getMigrationHash(typeA)).not.toEqual(getMigrationHash(typeB)); + }); + }); + describe('schemas', () => { + it('returns same hash if same schema versions are registered', () => { + const typeA = createType({ + schemas: { + '7.17.1': schema.object({}), + '8.4.2': schema.object({}), + }, + }); + const typeB = createType({ + schemas: { + '7.17.1': schema.object({}), + '8.4.2': schema.object({}), + }, + }); + + expect(getMigrationHash(typeA)).toEqual(getMigrationHash(typeB)); + }); + + it('returns same hash if same schema versions are registered in different order', () => { + const typeA = createType({ + schemas: { + '9.1.3': schema.object({}), + '7.17.1': schema.object({}), + '8.4.2': schema.object({}), + }, + }); + const typeB = createType({ + schemas: { + '8.4.2': schema.object({}), + '9.1.3': schema.object({}), + '7.17.1': schema.object({}), + }, + }); + + expect(getMigrationHash(typeA)).toEqual(getMigrationHash(typeB)); + }); + + it('returns same hash if same schema versions are registered using record + function', () => { + const typeA = createType({ + schemas: { + '9.1.3': schema.object({}), + '7.17.1': schema.object({}), + '8.4.2': schema.object({}), + }, + }); + const typeB = createType({ + schemas: () => ({ + '8.4.2': schema.object({}), + '9.1.3': schema.object({}), + '7.17.1': schema.object({}), + }), + }); + + expect(getMigrationHash(typeA)).toEqual(getMigrationHash(typeB)); + }); + + it('returns different hashes if different schema versions are registered', () => { + const typeA = createType({ + schemas: { + '7.17.1': schema.object({}), + '8.4.2': schema.object({}), + }, + }); + const typeB = createType({ + schemas: { + '7.17.69': schema.object({}), + '42.0.0': schema.object({}), + }, + }); + + expect(getMigrationHash(typeA)).not.toEqual(getMigrationHash(typeB)); + }); + }); + + describe('mappings', () => { + it('returns same hash for the same mappings', () => { + const typeA = createType({ + mappings: { + dynamic: false, + properties: { + description: { type: 'text' }, + hits: { type: 'integer', index: false, doc_values: false }, + }, + }, + }); + const typeB = createType({ + mappings: { + dynamic: false, + properties: { + description: { type: 'text' }, + hits: { type: 'integer', index: false, doc_values: false }, + }, + }, + }); + + expect(getMigrationHash(typeA)).toEqual(getMigrationHash(typeB)); + }); + + it('returns same hash for the same mappings in different order', () => { + const typeA = createType({ + mappings: { + dynamic: false, + properties: { + hits: { type: 'integer', index: false, doc_values: false }, + description: { type: 'text' }, + }, + }, + }); + const typeB = createType({ + mappings: { + properties: { + description: { type: 'text' }, + hits: { index: false, type: 'integer', doc_values: false }, + }, + dynamic: false, + }, + }); + + expect(getMigrationHash(typeA)).toEqual(getMigrationHash(typeB)); + }); + + it('returns different hashes for different mappings (removing nested property)', () => { + const typeA = createType({ + mappings: { + dynamic: false, + properties: { + description: { type: 'text' }, + hits: { type: 'integer', index: false, doc_values: false }, + }, + }, + }); + const typeB = createType({ + mappings: { + dynamic: false, + properties: { + description: { type: 'text' }, + hits: { type: 'integer', doc_values: false }, + }, + }, + }); + + expect(getMigrationHash(typeA)).not.toEqual(getMigrationHash(typeB)); + }); + + it('returns different hashes for different mappings (adding nested property)', () => { + const typeA = createType({ + mappings: { + dynamic: false, + properties: { + description: { type: 'text' }, + hits: { type: 'integer', index: false, doc_values: false }, + }, + }, + }); + const typeB = createType({ + mappings: { + dynamic: false, + properties: { + description: { type: 'text', boost: 42 }, + hits: { type: 'integer', index: false, doc_values: false }, + }, + }, + }); + + expect(getMigrationHash(typeA)).not.toEqual(getMigrationHash(typeB)); + }); + + it('returns different hashes for different mappings (removing top-level property)', () => { + const typeA = createType({ + mappings: { + dynamic: false, + properties: { + description: { type: 'text' }, + hits: { type: 'integer', index: false, doc_values: false }, + }, + }, + }); + const typeB = createType({ + mappings: { + properties: { + description: { type: 'text' }, + hits: { type: 'integer', index: false, doc_values: false }, + }, + }, + }); + + expect(getMigrationHash(typeA)).not.toEqual(getMigrationHash(typeB)); + }); + }); + + describe('ignored fields', () => { + it('returns same hash if `hidden` changes', () => { + expect(getMigrationHash(createType({ hidden: false }))).toEqual( + getMigrationHash(createType({ hidden: true })) + ); + }); + it('returns same hash if `management` changes', () => { + expect(getMigrationHash(createType({ management: undefined }))).toEqual( + getMigrationHash(createType({ management: { visibleInManagement: false } })) + ); + }); + }); +}); diff --git a/packages/core/test-helpers/core-test-helpers-so-type-serializer/src/get_migration_hash.ts b/packages/core/test-helpers/core-test-helpers-so-type-serializer/src/get_migration_hash.ts new file mode 100644 index 000000000000..7e23ec35bb9e --- /dev/null +++ b/packages/core/test-helpers/core-test-helpers-so-type-serializer/src/get_migration_hash.ts @@ -0,0 +1,33 @@ +/* + * 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. + */ + +import { createHash } from 'crypto'; +import type { SavedObjectsType } from '@kbn/core-saved-objects-server'; +import { extractMigrationInfo } from './extract_migration_info'; + +type SavedObjectTypeMigrationHash = string; + +export const getMigrationHash = (soType: SavedObjectsType): SavedObjectTypeMigrationHash => { + const migInfo = extractMigrationInfo(soType); + + const hash = createHash('sha1'); + + const hashParts = [ + migInfo.name, + migInfo.namespaceType, + migInfo.convertToAliasScript ?? 'none', + migInfo.hasExcludeOnUpgrade, + migInfo.convertToMultiNamespaceTypeVersion ?? 'none', + migInfo.migrationVersions.join(','), + migInfo.schemaVersions.join(','), + JSON.stringify(migInfo.mappings, Object.keys(migInfo.mappings).sort()), + ]; + const hashFeed = hashParts.join('-'); + + return hash.update(hashFeed).digest('hex'); +}; diff --git a/packages/core/test-helpers/core-test-helpers-so-type-serializer/tsconfig.json b/packages/core/test-helpers/core-test-helpers-so-type-serializer/tsconfig.json new file mode 100644 index 000000000000..71bb40fe57f3 --- /dev/null +++ b/packages/core/test-helpers/core-test-helpers-so-type-serializer/tsconfig.json @@ -0,0 +1,17 @@ +{ + "extends": "../../../../tsconfig.bazel.json", + "compilerOptions": { + "declaration": true, + "declarationMap": true, + "emitDeclarationOnly": true, + "outDir": "target_types", + "stripInternal": false, + "types": [ + "jest", + "node" + ] + }, + "include": [ + "**/*.ts", + ] +} diff --git a/src/core/server/docs/kib_core_reviewing_so_type_pr.mdx b/src/core/server/docs/kib_core_reviewing_so_type_pr.mdx new file mode 100644 index 000000000000..95dc46610954 --- /dev/null +++ b/src/core/server/docs/kib_core_reviewing_so_type_pr.mdx @@ -0,0 +1,84 @@ +--- +id: kibCoreReviewingSoPr +slug: /kibana-dev-docs/review/reviewing-so-pr +title: Reviewing SavedObject PRs +description: How to review PRs that changes savedObjects registration +date: 2022-09-30 +tags: ['kibana','dev', 'contributor', 'api docs'] +--- + +# Reviewing PRs that change Saved Object types + +## How does automatic review assignment work when SO types are changed? + +PRs modifying / adding / deleting any SO type registration will be flagged by the integration +test located at `src/core/server/integration_tests/saved_objects/migrations/check_registered_types.test.ts`. + +This test will fail any time a change is performed on an SO type registration that could risk having an impact on +upgrades/migrations, and will force the PR's author to update the test's snapshot, which will trigger a review +from the Core team. + +## What and how to review? + +Reviews will be triggered when one, or more, of these scenarios occur: +- a new type was added (registered) +- a type was removed (is no longer registered) +- a new migration function was added +- a mapping change was performed +- a new validation schema was added +- an `excludeOnUpgrade` function was added or removed + +Note: reviews will **not** automatically be triggered in these scenarios: +- an existing migration function is changed +- an existing validation schema is changed +- an existing `excludeOnUpgrade` function is changed + +### A new type was added + +We have another integration test detecting this scenario (`src/core/server/integration_tests/saved_objects/migrations/type_registrations.test.ts`) + +In that scenario, we should: +- check the initial mappings of the type to spot potential issues: + - fields being defined explicitly in the mappings but not directly used for search + - overall amount of fields is high + - use of `dynamic: true` (this can lead to mapping explosions) +- check if the type is registered as `hidden: true` and encourage to do so otherwise. + - this avoids polluting the global SO HTTP APIs with another type, and instead requires plugin developers to build + their own HTTP APIs to access this type of SO if they truly need to. + +### A type was removed + +The integration test mentioned in the previous section also detects those scenarios. + +Here, we need to check: +- that `REMOVED_TYPES` (`packages/core/saved-objects/core-saved-objects-migration-server-internal/src/core/unused_types.ts`) was properly updated + - (but, again, the integration test will fail otherwise) +- that no other existing SO types may be referencing this type + - can't really be automated, will likely need to ask the owning team directly + +### A new migration function was added + +- Review the migration function + - owners are supposed to do it, but an additional review never hurts + - make sure they are typing their migration function using `SavedObjectMigrationFn` and supplying arguments for the generics + e.g. `SavedObjectMigrationFn`. +- Make sure that the migration function is properly tested + - by unit tests + - and, ideally, by integration tests using 'real' data +- If the migration function is moving/creating/deleting/mutating fields, make sure that the type's mappings and/or schemas were updated accordingly +- Please refer to [the migration section of our testing docs](https://github.com/elastic/kibana/blob/main/dev_docs/tutorials/testing_plugins.mdx#L796) for more details + +### A mapping change was performed + +- Make sure a migration function was added to reflect the changes: + - If a field was removed, ensure the migration function always removes the field from all documents + - If a field type was changed from e.g. `text` to `long`, make sure the migration function guarantees that all documents have compatible fields + - If a text type was changed to a keyword, make sure the text won't exceed Elasticsearch's 32k keyword length limit by e.g. specifying: `ignore_above: 256` +- If the migration function is present, refer to previous section. +- If the type is registering validation schemas, make sure a new schema was added reflecting the changes to the model. + +### A new validation schema was added + +- Make sure the associated mapping changes were performed, and that a migration function was added accordingly. +- Ideally schemas are validated with unit tests as well, especially for more complex ones. +- Refer to prior sections to see what to check. \ No newline at end of file diff --git a/src/core/server/integration_tests/saved_objects/migrations/check_registered_types.test.ts b/src/core/server/integration_tests/saved_objects/migrations/check_registered_types.test.ts new file mode 100644 index 000000000000..d7704db87476 --- /dev/null +++ b/src/core/server/integration_tests/saved_objects/migrations/check_registered_types.test.ts @@ -0,0 +1,150 @@ +/* + * 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. + */ + +import type { ISavedObjectTypeRegistry } from '@kbn/core-saved-objects-server'; +import { getMigrationHash } from '@kbn/core-test-helpers-so-type-serializer'; +import { Root } from '../../../root'; +import * as kbnTestServer from '../../../../test_helpers/kbn_server'; + +describe('checking migration metadata changes on all registered SO types', () => { + let esServer: kbnTestServer.TestElasticsearchUtils; + let root: Root; + let typeRegistry: ISavedObjectTypeRegistry; + + beforeAll(async () => { + const { startES } = kbnTestServer.createTestServers({ + adjustTimeout: (t: number) => jest.setTimeout(t), + }); + + esServer = await startES(); + root = kbnTestServer.createRootWithCorePlugins({}, { oss: false }); + await root.preboot(); + await root.setup(); + const coreStart = await root.start(); + typeRegistry = coreStart.savedObjects.getTypeRegistry(); + }); + + afterAll(async () => { + if (root) { + await root.shutdown(); + } + if (esServer) { + await esServer.stop(); + } + }); + + // This test is meant to fail when any change is made in registered types that could potentially impact the SO migration. + // Just update the snapshot by running this test file via jest_integration with `-u` and push the update. + // The intent is to trigger a code review from the Core team to review the SO type changes. + it('detecting migration related changes in registered types', () => { + const allTypes = typeRegistry.getAllTypes(); + + const hashMap = allTypes.reduce((map, type) => { + map[type.name] = getMigrationHash(type); + return map; + }, {} as Record); + + expect(hashMap).toMatchInlineSnapshot(` + Object { + "action": "7858e6d5a9f231bf23f6f2e57328eb0095b26735", + "action_task_params": "bbd38cbfd74bf6713586fe078e3fa92db2234299", + "alert": "48461f3375d9ba22882ea23a318b62a5b0921a9b", + "api_key_pending_invalidation": "9b4bc1235337da9a87ef05a1d1f4858b2a3b77c6", + "apm-indices": "ceb0870f3a74e2ffc3a1cd3a3c73af76baca0999", + "apm-server-schema": "2bfd2998d3873872e1366458ce553def85418f91", + "apm-service-group": "07ecbf25ee4828d2b686abc98656b6665831d1a0", + "apm-telemetry": "abaa1e9469e6e0bad76938309f0ac4c66b528d58", + "app_search_telemetry": "7fc4fc08852bf0924ee29942bb394fda9aa8954d", + "application_usage_daily": "6e645e0b60ef3af2e8fde80963c2a4f09a190d61", + "application_usage_totals": "b2af3577dcd50bfae492b166a7804f69e2cc41dc", + "canvas-element": "5f32b99ba6ff9c1f17cc093591b975be65a27b9b", + "canvas-workpad": "b60252414fb6159a14f9febf98dbe41e5a8bf199", + "canvas-workpad-template": "c371cad0a8d61385f4782cab9a9063d3cf241ee0", + "cases": "7ff5ce930146a2d6fc8fbf536ce2ee16e9df296f", + "cases-comments": "8cfbad4ede637305eb6fb79db680f03dc0ce5ec4", + "cases-configure": "1afc414f5563a36e4612fa269193d3ed7277c7bd", + "cases-connector-mappings": "4b16d440af966e5d6e0fa33368bfa15d987a4b69", + "cases-telemetry": "16e261e7378a72acd0806f18df92525dd1da4f37", + "cases-user-actions": "3973dfcaacbe6ae147d7331699cfc25d2a27ca30", + "config": "e3f0408976dbdd453641f5699927b28b188f6b8c", + "connector_token": "fa5301aa5a2914795d3b1b82d0a49939444009da", + "core-usage-stats": "f40a213da2c597b0de94e364a4326a5a1baa4ca9", + "csp-rule-template": "3679c5f2431da8153878db79c78a4e695357fb61", + "csp_rule": "d2bb53ea5d2bdfba1a835ad8956dfcd2b2c32e19", + "dashboard": "0b0842b6aa40c125d64233fd81cee11080580dc2", + "endpoint:user-artifact": "f94c250a52b30d0a2d32635f8b4c5bdabd1e25c0", + "endpoint:user-artifact-manifest": "8c14d49a385d5d1307d956aa743ec78de0b2be88", + "enterprise_search_telemetry": "fafcc8318528d34f721c42d1270787c52565bad5", + "epm-packages": "c4c39f20d6bcfff40994813ee0f2bab01d34b646", + "epm-packages-assets": "9fd3d6726ac77369249e9a973902c2cd615fc771", + "event_loop_delays_daily": "d2ed39cf669577d90921c176499908b4943fb7bd", + "exception-list": "fe8cc004fd2742177cdb9300f4a67689463faf9c", + "exception-list-agnostic": "49fae8fcd1967cc4be45ba2a2c66c4afbc1e341b", + "file": "280f28bd48b3ad1f1a9f84c6c0ae6dd5ed1179da", + "file-upload-usage-collection-telemetry": "8478924cf0057bd90df737155b364f98d05420a5", + "fileShare": "3f88784b041bb8728a7f40763a08981828799a75", + "fleet-preconfiguration-deletion-record": "7b28f200513c28ae774f1b7d7d7906954e3c6e16", + "graph-workspace": "3342f2cd561afdde8f42f5fb284bf550dee8ebb5", + "guided-onboarding-guide-state": "561db8d481b131a2bbf46b1e534d6ce960255135", + "index-pattern": "48e77ca393c254e93256f11a7cdc0232dd754c08", + "infrastructure-monitoring-log-view": "e2c78c1076bd35e57d7c5fa1b410e5c126d12327", + "infrastructure-ui-source": "7c8dbbc0a608911f1b683a944f4a65383f6153ed", + "ingest-agent-policies": "5d728f483dc3b14dcfa6bbad95c2024d2da68890", + "ingest-download-sources": "1e69dabd6db5e320fe08c5bda8f35f29bafc6b54", + "ingest-outputs": "29b867bf7bfd28b1e17c84697dce5c6d078f9705", + "ingest-package-policies": "e8707a8c7821ea085e67c2d213e24efa56307393", + "ingest_manager_settings": "bb71f20e36a9ac3a2e46d9345e2caa96e7bf8c22", + "inventory-view": "bc2bd1e7ec7c186159447ab228d269f22bd39056", + "kql-telemetry": "29544cd7d3b767c5399878efae6bd724d24c03fd", + "legacy-url-alias": "7172dfd54f2e0c89fe263fd7095519b2d826a930", + "lens": "08769c789ad6d1b8a4d0cffebc9d9bb08bf01ad9", + "lens-ui-telemetry": "df2844565c9e18fed2bdb1f6cc3aadd58cf1e45b", + "map": "00ca6c4cf46ae59f70f1436262eb9f457b45eb14", + "maps-telemetry": "5adbde35bd50ec2b8e9ea5b96d4d9f886e31ecfb", + "metrics-explorer-view": "09e56993352b8ee678e88f71e4410d9aeee72f3a", + "ml-job": "2836da98a81bd220db61c0549e8e28da7a876cb2", + "ml-module": "95055522c8406afa67a554690a43506f6c040744", + "ml-trained-model": "e39dd10b2da827e194ddcaaf3db141ad1daf0201", + "monitoring-telemetry": "af508cea8e22edaa909e462069390650fbbf01b7", + "osquery-manager-usage-metric": "fbe3cbea25a96e2ca522ca436878e0162c94dcc2", + "osquery-pack": "afb3b46c5e23fc24ad438e9c4317ff37e4e5164a", + "osquery-pack-asset": "32421669c87c49dfabd4d3957f044e5eb7f7fb20", + "osquery-saved-query": "7b213b4b7a3e59350e99c50e8df9948662ed493a", + "query": "4640ef356321500a678869f24117b7091a911cb6", + "sample-data-telemetry": "8b10336d9efae6f3d5593c4cc89fb4abcdf84e04", + "search": "e7ba25ea37cb36b622db42c9590c6d8dfc838801", + "search-session": "ba383309da68a15be3765977f7a44c84f0ec7964", + "search-telemetry": "beb3fc25488c753f2a6dcff1845d667558712b66", + "security-rule": "e0dfdba5d66139d0300723b2e6672993cd4a11f3", + "security-solution-signals-migration": "e65933e32926e0ca385415bd44fc6da0b6d3d419", + "siem-detection-engine-rule-actions": "d4b5934c0c0e4ccdf509a41000eb0bee07be0c28", + "siem-detection-engine-rule-execution-info": "b92d51db7b7d591758d3e85892a91064aff01ff8", + "siem-ui-timeline": "95474f10662802e2f9ea068b45bf69212a2f5842", + "siem-ui-timeline-note": "08c71dc0b8b8018a67e80beb4659a078404c223d", + "siem-ui-timeline-pinned-event": "e2697b38751506c7fce6e8b7207a830483dc4283", + "space": "c4a0acce1bd4b9cce85154f2a350624a53111c59", + "spaces-usage-stats": "922d3235bbf519e3fb3b260e27248b1df8249b79", + "synthetics-monitor": "cffb4dfe9e0a36755a226d5cf983c21aac2b5b1e", + "synthetics-privates-locations": "dd00385f4a27ef062c3e57312eeb3799872fa4af", + "tag": "39413f4578cc2128c9a0fda97d0acd1c8862c47a", + "task": "ef53d0f070bd54957b8fe22fae3b1ff208913f76", + "telemetry": "9142dc5f18123fb6e6a9083db04e5becbfde94fd", + "ui-metric": "2fb66ccdee2d1fad52547964421629c5a485c38f", + "upgrade-assistant-ml-upgrade-operation": "408120d386c04ab25fe64a03937597aa0438c10d", + "upgrade-assistant-reindex-operation": "d9e18b3d9578ecabf09a297296dcf7e36b2481fd", + "upgrade-assistant-telemetry": "a0c80933a9f8b50a2590d19e1d1e5f97d28f7104", + "uptime-dynamic-settings": "9de35c5aeaef915c5bc3c5b1632c33fb0f6f1c55", + "uptime-synthetics-api-key": "df9d8418ddc210d832a069a0fb796f73e63d1082", + "url": "d66c1f26ed23a392be3617a8444d713571f58380", + "usage-counters": "33e2081a52215293041da1100e6602fb553ff446", + "visualization": "f45d06858a5634c9ed0367e11eb44f7f7dde0be2", + "workplace_search_telemetry": "45bd03e12b060c08381b0fd325d939f80d08c914", + } + `); + }); +}); diff --git a/yarn.lock b/yarn.lock index ebe3af38fa5e..d79a8afad43b 100644 --- a/yarn.lock +++ b/yarn.lock @@ -3388,6 +3388,10 @@ version "0.0.0" uid "" +"@kbn/core-test-helpers-so-type-serializer@link:bazel-bin/packages/core/test-helpers/core-test-helpers-so-type-serializer": + version "0.0.0" + uid "" + "@kbn/core-theme-browser-internal@link:bazel-bin/packages/core/theme/core-theme-browser-internal": version "0.0.0" uid "" @@ -7548,6 +7552,10 @@ version "0.0.0" uid "" +"@types/kbn__core-test-helpers-so-type-serializer@link:bazel-bin/packages/core/test-helpers/core-test-helpers-so-type-serializer/npm_module_types": + version "0.0.0" + uid "" + "@types/kbn__core-theme-browser-internal@link:bazel-bin/packages/core/theme/core-theme-browser-internal/npm_module_types": version "0.0.0" uid ""