From 9390d95163bf22e02d22625eeea821e1d7ca8c28 Mon Sep 17 00:00:00 2001 From: "Christiane (Tina) Heiligers" Date: Tue, 23 May 2023 10:32:54 -0700 Subject: [PATCH 01/16] Adds support for model version schema --- .../src/lib/apis/helpers/validation.ts | 39 ++++++++++++++++++- .../src/document_migrator/model_version.ts | 2 + .../src/model_version/schemas.ts | 6 +++ 3 files changed, 45 insertions(+), 2 deletions(-) diff --git a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/helpers/validation.ts b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/helpers/validation.ts index 96224953ba459..82fa8d4fa1904 100644 --- a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/helpers/validation.ts +++ b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/helpers/validation.ts @@ -9,7 +9,10 @@ import type { PublicMethodsOf } from '@kbn/utility-types'; import type { Logger } from '@kbn/logging'; import type { ISavedObjectTypeRegistry } from '@kbn/core-saved-objects-server'; -import { SavedObjectsTypeValidator } from '@kbn/core-saved-objects-base-server-internal'; +import { + SavedObjectsTypeValidator, + modelVersionToVirtualVersion, +} from '@kbn/core-saved-objects-base-server-internal'; import { SavedObjectsErrorHelpers, type SavedObjectSanitizedDoc, @@ -100,10 +103,29 @@ export class ValidationHelper { private getTypeValidator(type: string): SavedObjectsTypeValidator { if (!this.typeValidatorMap[type]) { const savedObjectType = this.registry.getType(type); + + const combinedSchemas = + typeof savedObjectType!.schemas === 'function' + ? savedObjectType!.schemas() + : savedObjectType!.schemas ?? {}; + + const modelVersionsMap = + typeof savedObjectType!.modelVersions === 'function' + ? savedObjectType!.modelVersions() + : savedObjectType!.modelVersions ?? {}; + + Object.entries(modelVersionsMap).reduce((map, [key, modelVersion]) => { + const virtualVersion = modelVersionToVirtualVersion(key); + if (modelVersion.schemas?.create) { + combinedSchemas[virtualVersion] = modelVersion.schemas!.create!; + } + return map; + }, {}); + this.typeValidatorMap[type] = new SavedObjectsTypeValidator({ logger: this.logger.get('type-validator'), type, - validationMap: savedObjectType!.schemas ?? {}, + validationMap: combinedSchemas, defaultVersion: this.kibanaVersion, }); } @@ -122,3 +144,16 @@ export class ValidationHelper { } } } + +/** + ```typescript + * const modelVersionMap: SavedObjectsModelVersionMap = { + * '1': modelVersion1, + * '2': modelVersion2, + * '3': modelVersion3, + * } + * where + * modelVersion = { + changes; [] + } + */ diff --git a/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/document_migrator/model_version.ts b/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/document_migrator/model_version.ts index 5801bfe56f162..4c8a07bffa233 100644 --- a/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/document_migrator/model_version.ts +++ b/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/document_migrator/model_version.ts @@ -22,6 +22,8 @@ import { import { TransformSavedObjectDocumentError } from '../core'; import { type Transform, type TransformFn, TransformType, type TypeVersionSchema } from './types'; +// @TINA this is how we extract the schemas from a model_version for the document migrator +// Question: we need this as a helper, can we move it to core-saved-objects-api-server-internal.src.lib/apis/helpers/validation ?? export const getModelVersionSchemas = ({ typeDefinition, }: { diff --git a/packages/core/saved-objects/core-saved-objects-server/src/model_version/schemas.ts b/packages/core/saved-objects/core-saved-objects-server/src/model_version/schemas.ts index 6ced7b04b5518..792450afa70d7 100644 --- a/packages/core/saved-objects/core-saved-objects-server/src/model_version/schemas.ts +++ b/packages/core/saved-objects/core-saved-objects-server/src/model_version/schemas.ts @@ -7,6 +7,7 @@ */ import type { ObjectType } from '@kbn/config-schema'; +import { SavedObjectsValidationSpec } from '../validation'; /** * The schemas associated with this model version. @@ -29,6 +30,11 @@ export interface SavedObjectsModelVersionSchemaDefinitions { * See {@link SavedObjectModelVersionForwardCompatibilitySchema} for more info. */ forwardCompatibility?: SavedObjectModelVersionForwardCompatibilitySchema; + /** + * The schema applied when creating a document of the current version + * Allows for validating properties using @kbn/config-schema validations + */ + create?: SavedObjectsValidationSpec; } /** From 4361dc6336cfb4f69acede72e2e0cce19dfc980d Mon Sep 17 00:00:00 2001 From: "Christiane (Tina) Heiligers" Date: Thu, 25 May 2023 12:21:19 -0700 Subject: [PATCH 02/16] Adds tests for ValidationHelper.validateObjectForCreate --- .../src/lib/apis/helpers/validation.test.ts | 93 +++++++++++++ .../src/lib/apis/helpers/validation.ts | 19 +-- .../lib/apis/helpers/validation_fixtures.ts | 123 ++++++++++++++++++ .../src/document_migrator/model_version.ts | 2 - 4 files changed, 220 insertions(+), 17 deletions(-) create mode 100644 packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/helpers/validation.test.ts create mode 100644 packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/helpers/validation_fixtures.ts diff --git a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/helpers/validation.test.ts b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/helpers/validation.test.ts new file mode 100644 index 0000000000000..2de3ed4e92062 --- /dev/null +++ b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/helpers/validation.test.ts @@ -0,0 +1,93 @@ +/* + * 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 { loggerMock, type MockedLogger } from '@kbn/logging-mocks'; +import { SavedObjectsType } from '@kbn/core-saved-objects-server'; +import { type SavedObjectSanitizedDoc } from '@kbn/core-saved-objects-server'; +import { ValidationHelper } from './validation'; +import { typedef, typedef1, typedef2 } from './validation_fixtures'; +import { SavedObjectTypeRegistry } from '@kbn/core-saved-objects-base-server-internal'; + +const defaultVersion = '8.8.0'; +const typeA = 'my-typeA'; +const typeB = 'my-typeB'; +const typeC = 'my-typeC'; + +describe('Saved Objects type validation helper', () => { + let helper: ValidationHelper; + let logger: MockedLogger; + let typeRegistry: SavedObjectTypeRegistry; + + const createMockObject = ( + type: string, + attr: Partial + ): SavedObjectSanitizedDoc => ({ + type, + id: 'test-id', + references: [], + attributes: {}, + ...attr, + }); + const registerType = (name: string, parts: Partial) => { + typeRegistry.registerType({ + name, + hidden: false, + namespaceType: 'single', + mappings: { properties: {} }, + ...parts, + }); + }; + beforeEach(() => { + logger = loggerMock.create(); + typeRegistry = new SavedObjectTypeRegistry(); + }); + + afterEach(() => { + jest.resetAllMocks(); + }); + + describe('validation helper', () => { + beforeEach(() => { + helper = new ValidationHelper({ + logger, + registry: typeRegistry, + kibanaVersion: defaultVersion, + }); + registerType(typeA, typedef); + registerType(typeB, typedef1); + registerType(typeC, typedef2); + }); + + it('should work without model versions', () => { + const data = createMockObject(typeA, { attributes: { foo: 'hi', count: 1 } }); + expect(() => helper.validateObjectForCreate(typeA, data)).not.toThrowError(); + }); + + it('should work with model versions when top level schema also includes new added fields', () => { + const data = createMockObject(typeB, { attributes: { foo: 'hi', count: 1 } }); + expect(() => helper.validateObjectForCreate(typeB, data)).not.toThrowError(); + }); + + it('should fail with model versions enabled when top level schema does not include new added fields even though model version create schema does', () => { + const validationError = new Error( + '[attributes.count]: definition for this key is missing: Bad Request' + ); + const data = createMockObject(typeC, { attributes: { foo: 'hi', count: 1 } }); + expect(() => helper.validateObjectForCreate(typeC, data)).toThrowError(validationError); + }); + + it.todo('skips validation without legacy schemas or schema declared for model versions create'); + it.todo( + 'skips validation when schema declared for model versions create but not legacy schemas ' + ); + it.todo( + 'applies validation when schema declared as legacy schemas but not delcared in model versions create' + ); + it.todo('correctly fails validation for incorrect type'); + }); +}); diff --git a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/helpers/validation.ts b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/helpers/validation.ts index 82fa8d4fa1904..a45040dac3ac3 100644 --- a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/helpers/validation.ts +++ b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/helpers/validation.ts @@ -94,7 +94,8 @@ export class ValidationHelper { } const validator = this.getTypeValidator(type); try { - validator.validate(doc, this.kibanaVersion); + const result = validator.validate(doc, this.kibanaVersion); + return result; } catch (error) { throw SavedObjectsErrorHelpers.createBadRequestError(error.message); } @@ -104,7 +105,7 @@ export class ValidationHelper { if (!this.typeValidatorMap[type]) { const savedObjectType = this.registry.getType(type); - const combinedSchemas = + const legacySchemas = typeof savedObjectType!.schemas === 'function' ? savedObjectType!.schemas() : savedObjectType!.schemas ?? {}; @@ -114,6 +115,7 @@ export class ValidationHelper { ? savedObjectType!.modelVersions() : savedObjectType!.modelVersions ?? {}; + const combinedSchemas = { ...legacySchemas }; Object.entries(modelVersionsMap).reduce((map, [key, modelVersion]) => { const virtualVersion = modelVersionToVirtualVersion(key); if (modelVersion.schemas?.create) { @@ -144,16 +146,3 @@ export class ValidationHelper { } } } - -/** - ```typescript - * const modelVersionMap: SavedObjectsModelVersionMap = { - * '1': modelVersion1, - * '2': modelVersion2, - * '3': modelVersion3, - * } - * where - * modelVersion = { - changes; [] - } - */ diff --git a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/helpers/validation_fixtures.ts b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/helpers/validation_fixtures.ts new file mode 100644 index 0000000000000..555af5f6ef6d7 --- /dev/null +++ b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/helpers/validation_fixtures.ts @@ -0,0 +1,123 @@ +/* + * 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 { SavedObjectsType } from '@kbn/core-saved-objects-server'; + +export const typedef: Partial = { + mappings: { + properties: { + foo: { + type: 'keyword', + }, + count: { + type: 'integer', + }, + }, + }, + switchToModelVersionAt: '8.8.0', + schemas: { + '8.7.0': schema.object({ + foo: schema.string(), + }), + '8.8.0': schema.object({ + foo: schema.string(), + count: schema.number(), + }), + }, +}; + +export const typedef1: Partial = { + mappings: { + properties: { + foo: { + type: 'keyword', + }, + count: { + type: 'integer', + }, + }, + }, + switchToModelVersionAt: '8.8.0', + modelVersions: { + '1': { + changes: [ + { + type: 'mappings_addition', + addedMappings: { + count: { + properties: { + count: { + type: 'integer', + }, + }, + }, + }, + }, + ], + schemas: { + create: schema.object({ + foo: schema.string(), + count: schema.number(), + }), + }, + }, + }, + schemas: { + '8.7.0': schema.object({ + foo: schema.string(), + }), + '8.8.0': schema.object({ + foo: schema.string(), + count: schema.number(), + }), + }, +}; + +export const typedef2: Partial = { + mappings: { + properties: { + foo: { + type: 'keyword', + }, + count: { + type: 'integer', + }, + }, + }, + switchToModelVersionAt: '8.8.0', + modelVersions: { + '1': { + changes: [ + { + type: 'mappings_addition', + addedMappings: { + count: { + properties: { + count: { + type: 'integer', + }, + }, + }, + }, + }, + ], + schemas: { + create: schema.object({ + foo: schema.string(), + count: schema.number(), + }), + }, + }, + }, + schemas: { + '8.7.0': schema.object({ + foo: schema.string(), + }), + }, +}; diff --git a/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/document_migrator/model_version.ts b/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/document_migrator/model_version.ts index 4c8a07bffa233..5801bfe56f162 100644 --- a/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/document_migrator/model_version.ts +++ b/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/document_migrator/model_version.ts @@ -22,8 +22,6 @@ import { import { TransformSavedObjectDocumentError } from '../core'; import { type Transform, type TransformFn, TransformType, type TypeVersionSchema } from './types'; -// @TINA this is how we extract the schemas from a model_version for the document migrator -// Question: we need this as a helper, can we move it to core-saved-objects-api-server-internal.src.lib/apis/helpers/validation ?? export const getModelVersionSchemas = ({ typeDefinition, }: { From 1476bc1df9911319c4705a196012c5141d21708e Mon Sep 17 00:00:00 2001 From: "Christiane (Tina) Heiligers" Date: Sun, 28 May 2023 16:48:14 -0700 Subject: [PATCH 03/16] Adds code comments about dependencies and assumptions --- .../src/lib/apis/helpers/validation.test.ts | 13 +------ .../lib/apis/helpers/validation_fixtures.ts | 38 +++++++++---------- .../src/validation/validator.ts | 1 + 3 files changed, 22 insertions(+), 30 deletions(-) diff --git a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/helpers/validation.test.ts b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/helpers/validation.test.ts index 2de3ed4e92062..06856f8f6d538 100644 --- a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/helpers/validation.test.ts +++ b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/helpers/validation.test.ts @@ -13,7 +13,7 @@ import { ValidationHelper } from './validation'; import { typedef, typedef1, typedef2 } from './validation_fixtures'; import { SavedObjectTypeRegistry } from '@kbn/core-saved-objects-base-server-internal'; -const defaultVersion = '8.8.0'; +const defaultVersion = '8.10.0'; const typeA = 'my-typeA'; const typeB = 'my-typeB'; const typeC = 'my-typeC'; @@ -73,21 +73,12 @@ describe('Saved Objects type validation helper', () => { expect(() => helper.validateObjectForCreate(typeB, data)).not.toThrowError(); }); - it('should fail with model versions enabled when top level schema does not include new added fields even though model version create schema does', () => { + it('should fail with model versions enabled, when non-model schema excludes new fields', () => { const validationError = new Error( '[attributes.count]: definition for this key is missing: Bad Request' ); const data = createMockObject(typeC, { attributes: { foo: 'hi', count: 1 } }); expect(() => helper.validateObjectForCreate(typeC, data)).toThrowError(validationError); }); - - it.todo('skips validation without legacy schemas or schema declared for model versions create'); - it.todo( - 'skips validation when schema declared for model versions create but not legacy schemas ' - ); - it.todo( - 'applies validation when schema declared as legacy schemas but not delcared in model versions create' - ); - it.todo('correctly fails validation for incorrect type'); }); }); diff --git a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/helpers/validation_fixtures.ts b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/helpers/validation_fixtures.ts index 555af5f6ef6d7..539c5f13c840d 100644 --- a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/helpers/validation_fixtures.ts +++ b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/helpers/validation_fixtures.ts @@ -20,16 +20,16 @@ export const typedef: Partial = { }, }, }, - switchToModelVersionAt: '8.8.0', schemas: { - '8.7.0': schema.object({ + '8.9.0': schema.object({ foo: schema.string(), }), - '8.8.0': schema.object({ + '8.10.0': schema.object({ foo: schema.string(), count: schema.number(), }), }, + switchToModelVersionAt: '8.10.0', }; export const typedef1: Partial = { @@ -43,7 +43,16 @@ export const typedef1: Partial = { }, }, }, - switchToModelVersionAt: '8.8.0', + schemas: { + '8.9.0': schema.object({ + foo: schema.string(), + }), + '8.10.0': schema.object({ + foo: schema.string(), + count: schema.number(), + }), + }, + switchToModelVersionAt: '8.10.0', modelVersions: { '1': { changes: [ @@ -68,15 +77,6 @@ export const typedef1: Partial = { }, }, }, - schemas: { - '8.7.0': schema.object({ - foo: schema.string(), - }), - '8.8.0': schema.object({ - foo: schema.string(), - count: schema.number(), - }), - }, }; export const typedef2: Partial = { @@ -90,7 +90,12 @@ export const typedef2: Partial = { }, }, }, - switchToModelVersionAt: '8.8.0', + schemas: { + '8.9.0': schema.object({ + foo: schema.string(), + }), + }, + switchToModelVersionAt: '8.10.0', modelVersions: { '1': { changes: [ @@ -115,9 +120,4 @@ export const typedef2: Partial = { }, }, }, - schemas: { - '8.7.0': schema.object({ - foo: schema.string(), - }), - }, }; diff --git a/packages/core/saved-objects/core-saved-objects-base-server-internal/src/validation/validator.ts b/packages/core/saved-objects/core-saved-objects-base-server-internal/src/validation/validator.ts index e028c16f9bd2f..b2eaa0e2aa96d 100644 --- a/packages/core/saved-objects/core-saved-objects-base-server-internal/src/validation/validator.ts +++ b/packages/core/saved-objects/core-saved-objects-base-server-internal/src/validation/validator.ts @@ -51,6 +51,7 @@ export class SavedObjectsTypeValidator { document.typeMigrationVersion ?? document.migrationVersion?.[document.type] ?? this.defaultVersion; + // assume typeMigrationVersion gets updated to the relevant virtual model during write & migration. const schemaVersion = previousVersionWithSchema(this.orderedVersions, docVersion); if (!schemaVersion || !this.validationMap[schemaVersion]) { return; From f69c19a7f5ecc5fcf6e44f4aab51ab4184117531 Mon Sep 17 00:00:00 2001 From: "Christiane (Tina) Heiligers" Date: Mon, 29 May 2023 12:34:43 -0700 Subject: [PATCH 04/16] updates code comments --- .../src/lib/apis/helpers/validation.ts | 1 + .../src/validation/validator.ts | 3 +-- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/helpers/validation.ts b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/helpers/validation.ts index a45040dac3ac3..20e98617c373d 100644 --- a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/helpers/validation.ts +++ b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/helpers/validation.ts @@ -101,6 +101,7 @@ export class ValidationHelper { } } + // adds validation schemas per model to validationMap with virtual model version as key. private getTypeValidator(type: string): SavedObjectsTypeValidator { if (!this.typeValidatorMap[type]) { const savedObjectType = this.registry.getType(type); diff --git a/packages/core/saved-objects/core-saved-objects-base-server-internal/src/validation/validator.ts b/packages/core/saved-objects/core-saved-objects-base-server-internal/src/validation/validator.ts index b2eaa0e2aa96d..fa758cac13fc3 100644 --- a/packages/core/saved-objects/core-saved-objects-base-server-internal/src/validation/validator.ts +++ b/packages/core/saved-objects/core-saved-objects-base-server-internal/src/validation/validator.ts @@ -44,14 +44,13 @@ export class SavedObjectsTypeValidator { this.validationMap = typeof validationMap === 'function' ? validationMap() : validationMap; this.orderedVersions = Object.keys(this.validationMap).sort(Semver.compare); } - + // uses index meta's mappingVersions & docVersions -> virtualVersion to allow keeping track of mixed stack and model versions per type. public validate(document: SavedObjectSanitizedDoc, version?: string): void { const docVersion = version ?? document.typeMigrationVersion ?? document.migrationVersion?.[document.type] ?? this.defaultVersion; - // assume typeMigrationVersion gets updated to the relevant virtual model during write & migration. const schemaVersion = previousVersionWithSchema(this.orderedVersions, docVersion); if (!schemaVersion || !this.validationMap[schemaVersion]) { return; From ca9b8ace50593e526624a0012b4cbf531d392132 Mon Sep 17 00:00:00 2001 From: "Christiane (Tina) Heiligers" Date: Mon, 29 May 2023 14:13:53 -0700 Subject: [PATCH 05/16] improvements --- .../src/lib/apis/helpers/validation.test.ts | 6 +++--- .../src/lib/apis/helpers/validation.ts | 12 +++++------- 2 files changed, 8 insertions(+), 10 deletions(-) diff --git a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/helpers/validation.test.ts b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/helpers/validation.test.ts index 06856f8f6d538..56d25720cf79c 100644 --- a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/helpers/validation.test.ts +++ b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/helpers/validation.test.ts @@ -63,17 +63,17 @@ describe('Saved Objects type validation helper', () => { registerType(typeC, typedef2); }); - it('should work without model versions', () => { + it('should validate objects against stack versions', () => { const data = createMockObject(typeA, { attributes: { foo: 'hi', count: 1 } }); expect(() => helper.validateObjectForCreate(typeA, data)).not.toThrowError(); }); - it('should work with model versions when top level schema also includes new added fields', () => { + it('should validate objects against model versions', () => { const data = createMockObject(typeB, { attributes: { foo: 'hi', count: 1 } }); expect(() => helper.validateObjectForCreate(typeB, data)).not.toThrowError(); }); - it('should fail with model versions enabled, when non-model schema excludes new fields', () => { + it('should fail validation against invalid objects', () => { const validationError = new Error( '[attributes.count]: definition for this key is missing: Bad Request' ); diff --git a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/helpers/validation.ts b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/helpers/validation.ts index 20e98617c373d..343972b74e076 100644 --- a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/helpers/validation.ts +++ b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/helpers/validation.ts @@ -94,30 +94,28 @@ export class ValidationHelper { } const validator = this.getTypeValidator(type); try { - const result = validator.validate(doc, this.kibanaVersion); - return result; + validator.validate(doc, this.kibanaVersion); } catch (error) { throw SavedObjectsErrorHelpers.createBadRequestError(error.message); } } - // adds validation schemas per model to validationMap with virtual model version as key. private getTypeValidator(type: string): SavedObjectsTypeValidator { if (!this.typeValidatorMap[type]) { const savedObjectType = this.registry.getType(type); - const legacySchemas = + const stackVersionSchemas = typeof savedObjectType!.schemas === 'function' ? savedObjectType!.schemas() : savedObjectType!.schemas ?? {}; - const modelVersionsMap = + const modelVersionCreateSchemas = typeof savedObjectType!.modelVersions === 'function' ? savedObjectType!.modelVersions() : savedObjectType!.modelVersions ?? {}; - const combinedSchemas = { ...legacySchemas }; - Object.entries(modelVersionsMap).reduce((map, [key, modelVersion]) => { + const combinedSchemas = { ...stackVersionSchemas }; + Object.entries(modelVersionCreateSchemas).reduce((map, [key, modelVersion]) => { const virtualVersion = modelVersionToVirtualVersion(key); if (modelVersion.schemas?.create) { combinedSchemas[virtualVersion] = modelVersion.schemas!.create!; From 69f80a82d91853e7bcb70c1d532dc9b53966f483 Mon Sep 17 00:00:00 2001 From: "Christiane (Tina) Heiligers" Date: Mon, 29 May 2023 17:11:01 -0700 Subject: [PATCH 06/16] Assert validation against virtual model version --- .../src/lib/apis/helpers/validation.test.ts | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/helpers/validation.test.ts b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/helpers/validation.test.ts index 56d25720cf79c..3e4b975976131 100644 --- a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/helpers/validation.test.ts +++ b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/helpers/validation.test.ts @@ -14,6 +14,7 @@ import { typedef, typedef1, typedef2 } from './validation_fixtures'; import { SavedObjectTypeRegistry } from '@kbn/core-saved-objects-base-server-internal'; const defaultVersion = '8.10.0'; +const modelVirtualVersion = '10.1.0'; const typeA = 'my-typeA'; const typeB = 'my-typeB'; const typeC = 'my-typeC'; @@ -53,27 +54,32 @@ describe('Saved Objects type validation helper', () => { describe('validation helper', () => { beforeEach(() => { - helper = new ValidationHelper({ - logger, - registry: typeRegistry, - kibanaVersion: defaultVersion, - }); registerType(typeA, typedef); registerType(typeB, typedef1); registerType(typeC, typedef2); }); it('should validate objects against stack versions', () => { + helper = new ValidationHelper({ + logger, + registry: typeRegistry, + kibanaVersion: defaultVersion, + }); const data = createMockObject(typeA, { attributes: { foo: 'hi', count: 1 } }); expect(() => helper.validateObjectForCreate(typeA, data)).not.toThrowError(); }); it('should validate objects against model versions', () => { + helper = new ValidationHelper({ + logger, + registry: typeRegistry, + kibanaVersion: modelVirtualVersion, + }); const data = createMockObject(typeB, { attributes: { foo: 'hi', count: 1 } }); expect(() => helper.validateObjectForCreate(typeB, data)).not.toThrowError(); }); - it('should fail validation against invalid objects', () => { + it('should fail validation against invalid objects when version requested does not support a field', () => { const validationError = new Error( '[attributes.count]: definition for this key is missing: Bad Request' ); From bbedd7da97c243e333c1952f4b7048b15bc9cb66 Mon Sep 17 00:00:00 2001 From: "Christiane (Tina) Heiligers" Date: Mon, 29 May 2023 17:11:28 -0700 Subject: [PATCH 07/16] Assert validation against virtual model version --- .../src/lib/apis/helpers/validation.test.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/helpers/validation.test.ts b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/helpers/validation.test.ts index 3e4b975976131..323aa18c8f6ef 100644 --- a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/helpers/validation.test.ts +++ b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/helpers/validation.test.ts @@ -80,6 +80,11 @@ describe('Saved Objects type validation helper', () => { }); it('should fail validation against invalid objects when version requested does not support a field', () => { + helper = new ValidationHelper({ + logger, + registry: typeRegistry, + kibanaVersion: defaultVersion, + }); const validationError = new Error( '[attributes.count]: definition for this key is missing: Bad Request' ); From 8f4ca198b61b03d1857517f47fe848e6be6ab8a0 Mon Sep 17 00:00:00 2001 From: "Christiane (Tina) Heiligers" Date: Wed, 31 May 2023 12:38:27 -0700 Subject: [PATCH 08/16] Add comments --- .../src/validation/validator.test.ts | 2 +- .../src/validation/validator.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/core/saved-objects/core-saved-objects-base-server-internal/src/validation/validator.test.ts b/packages/core/saved-objects/core-saved-objects-base-server-internal/src/validation/validator.test.ts index e552aee1c8976..b1a34b30bdd11 100644 --- a/packages/core/saved-objects/core-saved-objects-base-server-internal/src/validation/validator.test.ts +++ b/packages/core/saved-objects/core-saved-objects-base-server-internal/src/validation/validator.test.ts @@ -93,7 +93,7 @@ describe('Saved Objects type validator', () => { describe('schema selection', () => { beforeEach(() => { validationMap = { - '2.0.0': createStubSpec(), + '2.0.0': createStubSpec(), // version keys have to be valid kibana versions. These are not '2.7.0': createStubSpec(), '3.0.0': createStubSpec(), '3.5.0': createStubSpec(), diff --git a/packages/core/saved-objects/core-saved-objects-base-server-internal/src/validation/validator.ts b/packages/core/saved-objects/core-saved-objects-base-server-internal/src/validation/validator.ts index fa758cac13fc3..38165b732a621 100644 --- a/packages/core/saved-objects/core-saved-objects-base-server-internal/src/validation/validator.ts +++ b/packages/core/saved-objects/core-saved-objects-base-server-internal/src/validation/validator.ts @@ -50,7 +50,7 @@ export class SavedObjectsTypeValidator { version ?? document.typeMigrationVersion ?? document.migrationVersion?.[document.type] ?? - this.defaultVersion; + this.defaultVersion; const schemaVersion = previousVersionWithSchema(this.orderedVersions, docVersion); if (!schemaVersion || !this.validationMap[schemaVersion]) { return; From 0b0b5ae13a2f4aa123fead50a39e4093d474108b Mon Sep 17 00:00:00 2001 From: "Christiane (Tina) Heiligers" Date: Wed, 31 May 2023 16:58:30 -0700 Subject: [PATCH 09/16] adds unit tests for new behavior --- .../src/validation/validator.test.ts | 50 ++++++++++++++++--- .../src/validation/validator.ts | 41 ++++++++++++--- 2 files changed, 76 insertions(+), 15 deletions(-) diff --git a/packages/core/saved-objects/core-saved-objects-base-server-internal/src/validation/validator.test.ts b/packages/core/saved-objects/core-saved-objects-base-server-internal/src/validation/validator.test.ts index b1a34b30bdd11..4d58acae55180 100644 --- a/packages/core/saved-objects/core-saved-objects-base-server-internal/src/validation/validator.test.ts +++ b/packages/core/saved-objects/core-saved-objects-base-server-internal/src/validation/validator.test.ts @@ -96,9 +96,11 @@ describe('Saved Objects type validator', () => { '2.0.0': createStubSpec(), // version keys have to be valid kibana versions. These are not '2.7.0': createStubSpec(), '3.0.0': createStubSpec(), - '3.5.0': createStubSpec(), + '3.5.0': createStubSpec(), // higher than default, should fall back to default '4.0.0': createStubSpec(), '4.3.0': createStubSpec(), + '10.1.0': createStubSpec(), + '11.1.4': createStubSpec(), }; validator = new SavedObjectsTypeValidator({ logger, type, validationMap, defaultVersion }); }); @@ -131,35 +133,67 @@ describe('Saved Objects type validator', () => { }); it('should use the correct schema for documents with typeMigrationVersion', () => { - let data = createMockObject({ typeMigrationVersion: '3.2.0' }); + const data = createMockObject({ typeMigrationVersion: '3.2.0' }); + validator.validate(data); + expect(getCalledVersion()).toEqual('3.0.0'); // falls back to most recent since 3.2.0 + }); + + it('should use the correct schema for documents with typeMigrationVersion higher than default when not a valid virtual model version', () => { + const data = createMockObject({ typeMigrationVersion: '3.5.0' }); // should fall back to most recent since 3.5.0 > default && not a virtual model + validator.validate(data); + expect(getCalledVersion()).toEqual('3.0.0'); + }); + + it('should use the fall back schema for documents with migrationVersion', () => { + let data = createMockObject({ + migrationVersion: { + [type]: '4.6.0', + }, + }); validator.validate(data); expect(getCalledVersion()).toEqual('3.0.0'); jest.clearAllMocks(); - data = createMockObject({ typeMigrationVersion: '3.5.0' }); + data = createMockObject({ + migrationVersion: { + [type]: '3.0.0', + }, + }); validator.validate(data); - expect(getCalledVersion()).toEqual('3.5.0'); + expect(getCalledVersion()).toEqual('3.0.0'); }); - it('should use the correct schema for documents with migrationVersion', () => { + it('should use the correct schema for documents with migrationVersion higher than default when not a valid virtual model version', () => { let data = createMockObject({ migrationVersion: { [type]: '4.6.0', }, }); validator.validate(data); - expect(getCalledVersion()).toEqual('4.3.0'); + expect(getCalledVersion()).toEqual('3.0.0'); jest.clearAllMocks(); data = createMockObject({ migrationVersion: { - [type]: '4.0.0', + [type]: '3.0.0', }, }); validator.validate(data); - expect(getCalledVersion()).toEqual('4.0.0'); + expect(getCalledVersion()).toEqual('3.0.0'); + }); + + it('should use the correct schema for documents with virtualModelVersion', () => { + const data = createMockObject({ typeMigrationVersion: '10.1.0' }); // allowed + validator.validate(data); + expect(getCalledVersion()).toEqual('10.1.0'); + }); + + it('should use the correct schema for documents with with virtualModelVersion higher than default when not a valid virtual model version', () => { + const data = createMockObject({ typeMigrationVersion: '11.1.4' }); // not allowed + validator.validate(data); + expect(getCalledVersion()).toEqual('3.0.0'); }); it('should use the correct schema for documents without a version specified', () => { diff --git a/packages/core/saved-objects/core-saved-objects-base-server-internal/src/validation/validator.ts b/packages/core/saved-objects/core-saved-objects-base-server-internal/src/validation/validator.ts index 38165b732a621..750045bb76d1b 100644 --- a/packages/core/saved-objects/core-saved-objects-base-server-internal/src/validation/validator.ts +++ b/packages/core/saved-objects/core-saved-objects-base-server-internal/src/validation/validator.ts @@ -13,6 +13,7 @@ import type { SavedObjectSanitizedDoc, } from '@kbn/core-saved-objects-server'; import { createSavedObjectSanitizedDocSchema } from './schema'; +import { isVirtualModelVersion } from '../model_version'; /** * Helper class that takes a {@link SavedObjectsValidationMap} and runs validations for a @@ -45,13 +46,39 @@ export class SavedObjectsTypeValidator { this.orderedVersions = Object.keys(this.validationMap).sort(Semver.compare); } // uses index meta's mappingVersions & docVersions -> virtualVersion to allow keeping track of mixed stack and model versions per type. + /** + * want to use the model virtual version if and only if the document has a switchToModelVersion + * check if the type has a switchToModelVersionsAt and if that is equal to the version or the default. + * if version is a valid model virtual versionvalid semver and if switchToModelVersionsAt === version, it is, then the docVersion can be a model version IF the docVersion is a valid modelVersion + * @param document + * @param version + * @returns + */ public validate(document: SavedObjectSanitizedDoc, version?: string): void { - const docVersion = - version ?? - document.typeMigrationVersion ?? - document.migrationVersion?.[document.type] ?? - this.defaultVersion; - const schemaVersion = previousVersionWithSchema(this.orderedVersions, docVersion); + // if a version is specified, use that + let usedVersion = version; + if (!usedVersion) { + const docVersion = + document.typeMigrationVersion ?? document.migrationVersion?.[document.type]; + if (docVersion) { + // retain virtualModelVersion if one was set, otherwise if the latest migration wasn't a model migration, then the most recent migration the doc went through is older that the default and we use the default version. + // only allow validation against versions > default if the version is a valid virtual model version. + usedVersion = + (Semver.gt(docVersion, this.defaultVersion) && isVirtualModelVersion(docVersion)) || + Semver.lt(docVersion, this.defaultVersion) + ? docVersion + : this.defaultVersion; + } else { + usedVersion = this.defaultVersion; + } + } + const schemaVersion = previousVersionWithSchema(this.orderedVersions, usedVersion); + // const docVersion = + // version ?? + // document.typeMigrationVersion ?? + // document.migrationVersion?.[document.type] ?? + // this.defaultVersion; + // const schemaVersion = previousVersionWithSchema(this.orderedVersions, docVersion); if (!schemaVersion || !this.validationMap[schemaVersion]) { return; } @@ -62,7 +89,7 @@ export class SavedObjectsTypeValidator { validationSchema.validate(document); } catch (e) { this.log.warn( - `Error validating object of type [${this.type}] against version [${docVersion}]` + `Error validating object of type [${this.type}] against version [${usedVersion}]` ); throw e; } From ef3e6d686e04fc884941c6d6dfa47025d109d33e Mon Sep 17 00:00:00 2001 From: "Christiane (Tina) Heiligers" Date: Wed, 31 May 2023 17:10:37 -0700 Subject: [PATCH 10/16] Remove option to specify validation version from SOR create and bulkCreate. The version is set within the validator --- .../src/lib/apis/helpers/validation.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/helpers/validation.ts b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/helpers/validation.ts index 343972b74e076..55d56058ce409 100644 --- a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/helpers/validation.ts +++ b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/helpers/validation.ts @@ -94,7 +94,8 @@ export class ValidationHelper { } const validator = this.getTypeValidator(type); try { - validator.validate(doc, this.kibanaVersion); + validator.validate(doc); + // validator.validate(doc, this.kibanaVersion); } catch (error) { throw SavedObjectsErrorHelpers.createBadRequestError(error.message); } From 93f3e54fae886988843b83f2f45eb949629b3101 Mon Sep 17 00:00:00 2001 From: "Christiane (Tina) Heiligers" Date: Thu, 1 Jun 2023 12:12:15 -0700 Subject: [PATCH 11/16] Don't check version against default --- .../src/lib/apis/helpers/validation.ts | 7 ++-- .../src/lib/repository.test.ts | 7 +++- .../src/validation/validator.test.ts | 16 ++++---- .../src/validation/validator.ts | 37 ++++--------------- 4 files changed, 24 insertions(+), 43 deletions(-) diff --git a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/helpers/validation.ts b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/helpers/validation.ts index 55d56058ce409..eeeff682adef8 100644 --- a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/helpers/validation.ts +++ b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/helpers/validation.ts @@ -94,13 +94,14 @@ export class ValidationHelper { } const validator = this.getTypeValidator(type); try { - validator.validate(doc); - // validator.validate(doc, this.kibanaVersion); + // validator.validate(doc); + // this change caused the repo test for create to not throw. + validator.validate(doc, this.kibanaVersion); } catch (error) { throw SavedObjectsErrorHelpers.createBadRequestError(error.message); } } - + // @TINA TODO: https://github.com/elastic/kibana/pull/158527/files#r1210089911 private getTypeValidator(type: string): SavedObjectsTypeValidator { if (!this.typeValidatorMap[type]) { const savedObjectType = this.registry.getType(type); diff --git a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.test.ts b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.test.ts index ab41890d36368..0faf776d4191f 100644 --- a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.test.ts +++ b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.test.ts @@ -853,7 +853,9 @@ describe('SavedObjectsRepository', () => { await bulkCreateError(obj3, true, expectedErrorResult); }); - it(`returns errors for any bulk objects with invalid schemas`, async () => { + it.skip(`returns errors for any bulk objects with invalid schemas`, async () => { + // todo: fix me + // change to validate caused this not to throw when it should const response = getMockBulkCreateResponse([obj3]); client.bulk.mockResponseOnce(response); @@ -3036,7 +3038,8 @@ describe('SavedObjectsRepository', () => { expect(client.create).not.toHaveBeenCalled(); }); - it(`throws when schema validation fails`, async () => { + it.skip(`throws when schema validation fails`, async () => { + // todo: fix await expect( repository.create('dashboard', { title: 123 }) ).rejects.toThrowErrorMatchingInlineSnapshot( diff --git a/packages/core/saved-objects/core-saved-objects-base-server-internal/src/validation/validator.test.ts b/packages/core/saved-objects/core-saved-objects-base-server-internal/src/validation/validator.test.ts index 4d58acae55180..8a3ab836c3d30 100644 --- a/packages/core/saved-objects/core-saved-objects-base-server-internal/src/validation/validator.test.ts +++ b/packages/core/saved-objects/core-saved-objects-base-server-internal/src/validation/validator.test.ts @@ -93,10 +93,10 @@ describe('Saved Objects type validator', () => { describe('schema selection', () => { beforeEach(() => { validationMap = { - '2.0.0': createStubSpec(), // version keys have to be valid kibana versions. These are not + '2.0.0': createStubSpec(), '2.7.0': createStubSpec(), '3.0.0': createStubSpec(), - '3.5.0': createStubSpec(), // higher than default, should fall back to default + '3.5.0': createStubSpec(), '4.0.0': createStubSpec(), '4.3.0': createStubSpec(), '10.1.0': createStubSpec(), @@ -129,22 +129,22 @@ describe('Saved Objects type validator', () => { data = createMockObject({ typeMigrationVersion: '3.5.0' }); validator.validate(data, '4.5.0'); - expect(getCalledVersion()).toEqual('4.3.0'); + expect(getCalledVersion()).toEqual('3.0.0'); }); it('should use the correct schema for documents with typeMigrationVersion', () => { const data = createMockObject({ typeMigrationVersion: '3.2.0' }); validator.validate(data); - expect(getCalledVersion()).toEqual('3.0.0'); // falls back to most recent since 3.2.0 + expect(getCalledVersion()).toEqual('3.0.0'); }); it('should use the correct schema for documents with typeMigrationVersion higher than default when not a valid virtual model version', () => { - const data = createMockObject({ typeMigrationVersion: '3.5.0' }); // should fall back to most recent since 3.5.0 > default && not a virtual model + const data = createMockObject({ typeMigrationVersion: '3.5.0' }); validator.validate(data); expect(getCalledVersion()).toEqual('3.0.0'); }); - it('should use the fall back schema for documents with migrationVersion', () => { + it('should fall back to most recent schema for documents with migrationVersion', () => { let data = createMockObject({ migrationVersion: { [type]: '4.6.0', @@ -185,13 +185,13 @@ describe('Saved Objects type validator', () => { }); it('should use the correct schema for documents with virtualModelVersion', () => { - const data = createMockObject({ typeMigrationVersion: '10.1.0' }); // allowed + const data = createMockObject({ typeMigrationVersion: '10.1.0' }); validator.validate(data); expect(getCalledVersion()).toEqual('10.1.0'); }); it('should use the correct schema for documents with with virtualModelVersion higher than default when not a valid virtual model version', () => { - const data = createMockObject({ typeMigrationVersion: '11.1.4' }); // not allowed + const data = createMockObject({ typeMigrationVersion: '11.1.4' }); validator.validate(data); expect(getCalledVersion()).toEqual('3.0.0'); }); diff --git a/packages/core/saved-objects/core-saved-objects-base-server-internal/src/validation/validator.ts b/packages/core/saved-objects/core-saved-objects-base-server-internal/src/validation/validator.ts index 750045bb76d1b..ae7c822ade0a1 100644 --- a/packages/core/saved-objects/core-saved-objects-base-server-internal/src/validation/validator.ts +++ b/packages/core/saved-objects/core-saved-objects-base-server-internal/src/validation/validator.ts @@ -45,40 +45,17 @@ export class SavedObjectsTypeValidator { this.validationMap = typeof validationMap === 'function' ? validationMap() : validationMap; this.orderedVersions = Object.keys(this.validationMap).sort(Semver.compare); } - // uses index meta's mappingVersions & docVersions -> virtualVersion to allow keeping track of mixed stack and model versions per type. - /** - * want to use the model virtual version if and only if the document has a switchToModelVersion - * check if the type has a switchToModelVersionsAt and if that is equal to the version or the default. - * if version is a valid model virtual versionvalid semver and if switchToModelVersionsAt === version, it is, then the docVersion can be a model version IF the docVersion is a valid modelVersion - * @param document - * @param version - * @returns - */ + public validate(document: SavedObjectSanitizedDoc, version?: string): void { - // if a version is specified, use that let usedVersion = version; - if (!usedVersion) { - const docVersion = - document.typeMigrationVersion ?? document.migrationVersion?.[document.type]; - if (docVersion) { - // retain virtualModelVersion if one was set, otherwise if the latest migration wasn't a model migration, then the most recent migration the doc went through is older that the default and we use the default version. - // only allow validation against versions > default if the version is a valid virtual model version. - usedVersion = - (Semver.gt(docVersion, this.defaultVersion) && isVirtualModelVersion(docVersion)) || - Semver.lt(docVersion, this.defaultVersion) - ? docVersion - : this.defaultVersion; - } else { - usedVersion = this.defaultVersion; - } + const docVersion = document.typeMigrationVersion ?? document.migrationVersion?.[document.type]; + if (docVersion) { + usedVersion = isVirtualModelVersion(docVersion) ? docVersion : this.defaultVersion; + } else { + usedVersion = this.defaultVersion; } const schemaVersion = previousVersionWithSchema(this.orderedVersions, usedVersion); - // const docVersion = - // version ?? - // document.typeMigrationVersion ?? - // document.migrationVersion?.[document.type] ?? - // this.defaultVersion; - // const schemaVersion = previousVersionWithSchema(this.orderedVersions, docVersion); + if (!schemaVersion || !this.validationMap[schemaVersion]) { return; } From fdde7752f9834a10b94f17da8fda4b17e1b84333 Mon Sep 17 00:00:00 2001 From: "Christiane (Tina) Heiligers" Date: Thu, 1 Jun 2023 12:30:35 -0700 Subject: [PATCH 12/16] comments cleanup --- .../src/lib/repository.test.ts | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.test.ts b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.test.ts index 0faf776d4191f..ab41890d36368 100644 --- a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.test.ts +++ b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.test.ts @@ -853,9 +853,7 @@ describe('SavedObjectsRepository', () => { await bulkCreateError(obj3, true, expectedErrorResult); }); - it.skip(`returns errors for any bulk objects with invalid schemas`, async () => { - // todo: fix me - // change to validate caused this not to throw when it should + it(`returns errors for any bulk objects with invalid schemas`, async () => { const response = getMockBulkCreateResponse([obj3]); client.bulk.mockResponseOnce(response); @@ -3038,8 +3036,7 @@ describe('SavedObjectsRepository', () => { expect(client.create).not.toHaveBeenCalled(); }); - it.skip(`throws when schema validation fails`, async () => { - // todo: fix + it(`throws when schema validation fails`, async () => { await expect( repository.create('dashboard', { title: 123 }) ).rejects.toThrowErrorMatchingInlineSnapshot( From 429a70c89410d22f50841f68e165ab87dd5abff4 Mon Sep 17 00:00:00 2001 From: "Christiane (Tina) Heiligers" Date: Thu, 1 Jun 2023 12:48:35 -0700 Subject: [PATCH 13/16] Improve type safety check --- .../src/lib/apis/helpers/validation.ts | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/helpers/validation.ts b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/helpers/validation.ts index eeeff682adef8..11aa553aa8028 100644 --- a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/helpers/validation.ts +++ b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/helpers/validation.ts @@ -94,27 +94,25 @@ export class ValidationHelper { } const validator = this.getTypeValidator(type); try { - // validator.validate(doc); - // this change caused the repo test for create to not throw. validator.validate(doc, this.kibanaVersion); } catch (error) { throw SavedObjectsErrorHelpers.createBadRequestError(error.message); } } - // @TINA TODO: https://github.com/elastic/kibana/pull/158527/files#r1210089911 + private getTypeValidator(type: string): SavedObjectsTypeValidator { if (!this.typeValidatorMap[type]) { const savedObjectType = this.registry.getType(type); const stackVersionSchemas = - typeof savedObjectType!.schemas === 'function' - ? savedObjectType!.schemas() - : savedObjectType!.schemas ?? {}; + typeof savedObjectType?.schemas === 'function' + ? savedObjectType.schemas() + : savedObjectType?.schemas ?? {}; const modelVersionCreateSchemas = - typeof savedObjectType!.modelVersions === 'function' - ? savedObjectType!.modelVersions() - : savedObjectType!.modelVersions ?? {}; + typeof savedObjectType?.modelVersions === 'function' + ? savedObjectType.modelVersions() + : savedObjectType?.modelVersions ?? {}; const combinedSchemas = { ...stackVersionSchemas }; Object.entries(modelVersionCreateSchemas).reduce((map, [key, modelVersion]) => { From 463b9810ec010456036dafe420b48f0208b0d367 Mon Sep 17 00:00:00 2001 From: "Christiane (Tina) Heiligers" Date: Fri, 2 Jun 2023 11:16:05 -0700 Subject: [PATCH 14/16] PR review comments --- .../src/lib/apis/helpers/validation.ts | 4 ++-- .../src/validation/validator.test.ts | 4 ++-- .../src/validation/validator.ts | 9 +++++++-- .../src/model_version/schemas.ts | 2 +- 4 files changed, 12 insertions(+), 7 deletions(-) diff --git a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/helpers/validation.ts b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/helpers/validation.ts index 11aa553aa8028..e9c8c6bcf50cd 100644 --- a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/helpers/validation.ts +++ b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/helpers/validation.ts @@ -94,7 +94,7 @@ export class ValidationHelper { } const validator = this.getTypeValidator(type); try { - validator.validate(doc, this.kibanaVersion); + validator.validate(doc); } catch (error) { throw SavedObjectsErrorHelpers.createBadRequestError(error.message); } @@ -116,8 +116,8 @@ export class ValidationHelper { const combinedSchemas = { ...stackVersionSchemas }; Object.entries(modelVersionCreateSchemas).reduce((map, [key, modelVersion]) => { - const virtualVersion = modelVersionToVirtualVersion(key); if (modelVersion.schemas?.create) { + const virtualVersion = modelVersionToVirtualVersion(key); combinedSchemas[virtualVersion] = modelVersion.schemas!.create!; } return map; diff --git a/packages/core/saved-objects/core-saved-objects-base-server-internal/src/validation/validator.test.ts b/packages/core/saved-objects/core-saved-objects-base-server-internal/src/validation/validator.test.ts index 8a3ab836c3d30..32f17b83a39a7 100644 --- a/packages/core/saved-objects/core-saved-objects-base-server-internal/src/validation/validator.test.ts +++ b/packages/core/saved-objects/core-saved-objects-base-server-internal/src/validation/validator.test.ts @@ -190,10 +190,10 @@ describe('Saved Objects type validator', () => { expect(getCalledVersion()).toEqual('10.1.0'); }); - it('should use the correct schema for documents with with virtualModelVersion higher than default when not a valid virtual model version', () => { + it.only('should use the correct schema for documents with with virtualModelVersion higher than default when not a valid virtual model version', () => { const data = createMockObject({ typeMigrationVersion: '11.1.4' }); validator.validate(data); - expect(getCalledVersion()).toEqual('3.0.0'); + expect(getCalledVersion()).toEqual('3.4.0'); // this should be 10.1.0 }); it('should use the correct schema for documents without a version specified', () => { diff --git a/packages/core/saved-objects/core-saved-objects-base-server-internal/src/validation/validator.ts b/packages/core/saved-objects/core-saved-objects-base-server-internal/src/validation/validator.ts index ae7c822ade0a1..fce389e511371 100644 --- a/packages/core/saved-objects/core-saved-objects-base-server-internal/src/validation/validator.ts +++ b/packages/core/saved-objects/core-saved-objects-base-server-internal/src/validation/validator.ts @@ -46,14 +46,17 @@ export class SavedObjectsTypeValidator { this.orderedVersions = Object.keys(this.validationMap).sort(Semver.compare); } - public validate(document: SavedObjectSanitizedDoc, version?: string): void { - let usedVersion = version; + public validate(document: SavedObjectSanitizedDoc): void { + let usedVersion: string; const docVersion = document.typeMigrationVersion ?? document.migrationVersion?.[document.type]; if (docVersion) { + console.log('docVersion', docVersion); + console.log('defaultVersion', this.defaultVersion); usedVersion = isVirtualModelVersion(docVersion) ? docVersion : this.defaultVersion; } else { usedVersion = this.defaultVersion; } + console.log('usedVersion', usedVersion); const schemaVersion = previousVersionWithSchema(this.orderedVersions, usedVersion); if (!schemaVersion || !this.validationMap[schemaVersion]) { @@ -77,6 +80,8 @@ const previousVersionWithSchema = ( orderedVersions: string[], targetVersion: string ): string | undefined => { + console.log('orderedVersions', orderedVersions); + console.log('targetVersion', targetVersion); for (let i = orderedVersions.length - 1; i >= 0; i--) { const currentVersion = orderedVersions[i]; if (Semver.lte(currentVersion, targetVersion)) { diff --git a/packages/core/saved-objects/core-saved-objects-server/src/model_version/schemas.ts b/packages/core/saved-objects/core-saved-objects-server/src/model_version/schemas.ts index 792450afa70d7..08309b44be4ad 100644 --- a/packages/core/saved-objects/core-saved-objects-server/src/model_version/schemas.ts +++ b/packages/core/saved-objects/core-saved-objects-server/src/model_version/schemas.ts @@ -7,7 +7,7 @@ */ import type { ObjectType } from '@kbn/config-schema'; -import { SavedObjectsValidationSpec } from '../validation'; +import type { SavedObjectsValidationSpec } from '../validation'; /** * The schemas associated with this model version. From 3c25cb2db3920e28e014f47fbaf5ea3a4d9dbb11 Mon Sep 17 00:00:00 2001 From: "Christiane (Tina) Heiligers" Date: Fri, 2 Jun 2023 14:50:01 -0700 Subject: [PATCH 15/16] add test for other conditionals --- .../src/validation/validator.test.ts | 87 ++++++++----------- .../src/validation/validator.ts | 5 -- 2 files changed, 35 insertions(+), 57 deletions(-) diff --git a/packages/core/saved-objects/core-saved-objects-base-server-internal/src/validation/validator.test.ts b/packages/core/saved-objects/core-saved-objects-base-server-internal/src/validation/validator.test.ts index 32f17b83a39a7..b77cca6a46a95 100644 --- a/packages/core/saved-objects/core-saved-objects-base-server-internal/src/validation/validator.test.ts +++ b/packages/core/saved-objects/core-saved-objects-base-server-internal/src/validation/validator.test.ts @@ -51,18 +51,18 @@ describe('Saved Objects type validator', () => { it('should log when a validation fails', () => { const data = createMockObject({ attributes: { foo: false } }); - expect(() => validator.validate(data, '1.0.0')).toThrowError(); + expect(() => validator.validate(data)).toThrowError(); expect(logger.warn).toHaveBeenCalledTimes(1); }); it('should work when given valid values', () => { const data = createMockObject({ attributes: { foo: 'hi' } }); - expect(() => validator.validate(data, '1.0.0')).not.toThrowError(); + expect(() => validator.validate(data)).not.toThrowError(); }); it('should throw an error when given invalid values', () => { const data = createMockObject({ attributes: { foo: false } }); - expect(() => validator.validate(data, '1.0.0')).toThrowErrorMatchingInlineSnapshot( + expect(() => validator.validate(data)).toThrowErrorMatchingInlineSnapshot( `"[attributes.foo]: expected value of type [string] but got [boolean]"` ); }); @@ -71,7 +71,7 @@ describe('Saved Objects type validator', () => { const data = createMockObject({ attributes: { foo: 'hi' } }); // @ts-expect-error Intentionally malformed object data.updated_at = false; - expect(() => validator.validate(data, '1.0.0')).toThrowErrorMatchingInlineSnapshot( + expect(() => validator.validate(data)).toThrowErrorMatchingInlineSnapshot( `"[updated_at]: expected value of type [string] but got [boolean]"` ); }); @@ -86,7 +86,7 @@ describe('Saved Objects type validator', () => { }); const data = createMockObject({ attributes: { foo: 'hi' } }); - expect(() => validator.validate(data, '1.0.0')).not.toThrowError(); + expect(() => validator.validate(data)).not.toThrowError(); }); }); @@ -97,10 +97,8 @@ describe('Saved Objects type validator', () => { '2.7.0': createStubSpec(), '3.0.0': createStubSpec(), '3.5.0': createStubSpec(), - '4.0.0': createStubSpec(), - '4.3.0': createStubSpec(), - '10.1.0': createStubSpec(), - '11.1.4': createStubSpec(), + // we're intentionally leaving out 10.1.0 to test model version selection + '10.2.0': createStubSpec(), }; validator = new SavedObjectsTypeValidator({ logger, type, validationMap, defaultVersion }); }); @@ -120,83 +118,68 @@ describe('Saved Objects type validator', () => { return undefined; }; - it('should use the correct schema when specifying the version', () => { - let data = createMockObject({ typeMigrationVersion: '2.2.0' }); - validator.validate(data, '3.2.0'); - expect(getCalledVersion()).toEqual('3.0.0'); - - jest.clearAllMocks(); - - data = createMockObject({ typeMigrationVersion: '3.5.0' }); - validator.validate(data, '4.5.0'); - expect(getCalledVersion()).toEqual('3.0.0'); - }); - it('should use the correct schema for documents with typeMigrationVersion', () => { - const data = createMockObject({ typeMigrationVersion: '3.2.0' }); + const data = createMockObject({ typeMigrationVersion: '3.0.0' }); validator.validate(data); expect(getCalledVersion()).toEqual('3.0.0'); }); - it('should use the correct schema for documents with typeMigrationVersion higher than default when not a valid virtual model version', () => { + it('should use the correct schema for documents with typeMigrationVersion greater than default version', () => { const data = createMockObject({ typeMigrationVersion: '3.5.0' }); validator.validate(data); expect(getCalledVersion()).toEqual('3.0.0'); }); - it('should fall back to most recent schema for documents with migrationVersion', () => { - let data = createMockObject({ + it('should use the correct schema for documents with migrationVersion', () => { + const data = createMockObject({ migrationVersion: { - [type]: '4.6.0', + [type]: '3.0.0', }, }); validator.validate(data); expect(getCalledVersion()).toEqual('3.0.0'); - jest.clearAllMocks(); + // jest.clearAllMocks(); - data = createMockObject({ - migrationVersion: { - [type]: '3.0.0', - }, - }); - validator.validate(data); - expect(getCalledVersion()).toEqual('3.0.0'); + // data = createMockObject({ + // migrationVersion: { + // [type]: '3.2.0', + // }, + // }); + // validator.validate(data); + // expect(getCalledVersion()).toEqual('3.0.0'); }); - it('should use the correct schema for documents with migrationVersion higher than default when not a valid virtual model version', () => { - let data = createMockObject({ + it('should use the correct schema for documents with migrationVersion higher than default', () => { + const data = createMockObject({ migrationVersion: { [type]: '4.6.0', }, }); validator.validate(data); + // 4.6.0 > 3.3.0 (default), is not a valid virtual model and there aren't migrations for the type in the default version expect(getCalledVersion()).toEqual('3.0.0'); + }); - jest.clearAllMocks(); - - data = createMockObject({ - migrationVersion: { - [type]: '3.0.0', - }, - }); + it("should use the correct schema for documents with virtualModelVersion that isn't registered", () => { + let data = createMockObject({ typeMigrationVersion: '10.1.0' }); validator.validate(data); - expect(getCalledVersion()).toEqual('3.0.0'); - }); + expect(getCalledVersion()).toEqual('3.5.0'); + + jest.clearAllMocks(); - it('should use the correct schema for documents with virtualModelVersion', () => { - const data = createMockObject({ typeMigrationVersion: '10.1.0' }); + data = createMockObject({ typeMigrationVersion: '10.3.0' }); validator.validate(data); - expect(getCalledVersion()).toEqual('10.1.0'); + expect(getCalledVersion()).toEqual('10.2.0'); }); - it.only('should use the correct schema for documents with with virtualModelVersion higher than default when not a valid virtual model version', () => { - const data = createMockObject({ typeMigrationVersion: '11.1.4' }); + it('should use the correct schema for documents with virtualModelVersion that is registered', () => { + const data = createMockObject({ typeMigrationVersion: '10.2.0' }); validator.validate(data); - expect(getCalledVersion()).toEqual('3.4.0'); // this should be 10.1.0 + expect(getCalledVersion()).toEqual('10.2.0'); }); - it('should use the correct schema for documents without a version specified', () => { + it('should use the correct schema for documents without a version', () => { const data = createMockObject({}); validator.validate(data); expect(getCalledVersion()).toEqual('3.0.0'); diff --git a/packages/core/saved-objects/core-saved-objects-base-server-internal/src/validation/validator.ts b/packages/core/saved-objects/core-saved-objects-base-server-internal/src/validation/validator.ts index fce389e511371..e8344de7bc2f0 100644 --- a/packages/core/saved-objects/core-saved-objects-base-server-internal/src/validation/validator.ts +++ b/packages/core/saved-objects/core-saved-objects-base-server-internal/src/validation/validator.ts @@ -50,13 +50,10 @@ export class SavedObjectsTypeValidator { let usedVersion: string; const docVersion = document.typeMigrationVersion ?? document.migrationVersion?.[document.type]; if (docVersion) { - console.log('docVersion', docVersion); - console.log('defaultVersion', this.defaultVersion); usedVersion = isVirtualModelVersion(docVersion) ? docVersion : this.defaultVersion; } else { usedVersion = this.defaultVersion; } - console.log('usedVersion', usedVersion); const schemaVersion = previousVersionWithSchema(this.orderedVersions, usedVersion); if (!schemaVersion || !this.validationMap[schemaVersion]) { @@ -80,8 +77,6 @@ const previousVersionWithSchema = ( orderedVersions: string[], targetVersion: string ): string | undefined => { - console.log('orderedVersions', orderedVersions); - console.log('targetVersion', targetVersion); for (let i = orderedVersions.length - 1; i >= 0; i--) { const currentVersion = orderedVersions[i]; if (Semver.lte(currentVersion, targetVersion)) { From c38b90e68d858de6502e9e595d6de6a694f2d9db Mon Sep 17 00:00:00 2001 From: "Christiane (Tina) Heiligers" Date: Fri, 2 Jun 2023 15:07:39 -0700 Subject: [PATCH 16/16] cleanup --- .../src/validation/validator.test.ts | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/packages/core/saved-objects/core-saved-objects-base-server-internal/src/validation/validator.test.ts b/packages/core/saved-objects/core-saved-objects-base-server-internal/src/validation/validator.test.ts index b77cca6a46a95..37ced142364da 100644 --- a/packages/core/saved-objects/core-saved-objects-base-server-internal/src/validation/validator.test.ts +++ b/packages/core/saved-objects/core-saved-objects-base-server-internal/src/validation/validator.test.ts @@ -138,16 +138,6 @@ describe('Saved Objects type validator', () => { }); validator.validate(data); expect(getCalledVersion()).toEqual('3.0.0'); - - // jest.clearAllMocks(); - - // data = createMockObject({ - // migrationVersion: { - // [type]: '3.2.0', - // }, - // }); - // validator.validate(data); - // expect(getCalledVersion()).toEqual('3.0.0'); }); it('should use the correct schema for documents with migrationVersion higher than default', () => {