[SOR] Adds support for validation schema with models #158527

TinaHeiligers · 2023-05-26T02:57:55Z

saved objects convert to using models. Ideally, type owners should still be able to validate objects against an expected model version schema for creating and editing their objects.

This PR adds support for optionally declaring model.schema.create, with which to validate against a config.schema.
Validation schemas for models are mapped to their virtual model version (model version '1' -> virtual version 10.1.0) in the validationMap.

...saved-objects/core-saved-objects-api-server-internal/src/lib/apis/helpers/validation.test.ts

TinaHeiligers

Draft early stages of adding tests

...-objects/core-saved-objects-migration-server-internal/src/document_migrator/model_version.ts

...saved-objects/core-saved-objects-api-server-internal/src/lib/apis/helpers/validation.test.ts

elasticmachine · 2023-05-29T23:00:28Z

Pinging @elastic/kibana-core (Team:Core)

TinaHeiligers · 2023-05-29T23:01:12Z

@elasticmachine merge upstream

afharo

LGTM. I just added a few nits.

I didn't approve because I'd like to rely on SO gurus for this 😇

...core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/helpers/validation.ts

packages/core/saved-objects/core-saved-objects-base-server-internal/src/validation/validator.ts

TinaHeiligers · 2023-05-30T17:26:34Z

We need a different way to target the validation version on write since we can't just take the latest stack version for documents that are already on 10.x.0.

Proposed solutions: don't pass version as an option to validate from SOR create and bulkCreate APIs and leave the (now unused) version option in the logic chain for targeting a version or, since option.version won't be used anymore, keep it simple and remove the option.

Since we already know issues will arise and, since it's going to be unused anyway, I'm going with removing the option altogether. There's no point in carting something along "just in case". We can add it back if we need to.

TinaHeiligers · 2023-05-31T17:42:31Z

@elasticmachine merge upstream

…reate. The version is set within the validator

TinaHeiligers

initial draft self review: comments to myself and todos

.../core/saved-objects/core-saved-objects-base-server-internal/src/validation/validator.test.ts

packages/core/saved-objects/core-saved-objects-base-server-internal/src/validation/validator.ts

...core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/helpers/validation.ts

TinaHeiligers

Self review 2

packages/core/saved-objects/core-saved-objects-base-server-internal/src/validation/validator.ts

TinaHeiligers · 2023-06-01T22:01:19Z

packages/core/saved-objects/core-saved-objects-base-server-internal/src/validation/validator.ts

-      document.migrationVersion?.[document.type] ??
-      this.defaultVersion;
-    const schemaVersion = previousVersionWithSchema(this.orderedVersions, docVersion);
+    let usedVersion = version;


Even though the version option is (or should be) unused now, keeping the behavior allows us to reuse it if needed.

I'm open to removing the option if someone objects.

Correct me if I'm wrong, but in current state, the version option is effectively unused (given usedVersion is always replaced L52-L56), right?

I'm fine either way (keeping it or removing it), but if we do keep it, then we need the version parameter to be used if specified (it doesn't make sense to keep an option ignored by the implementation), and to adapt the call to validate to no longer pass the option, here:

kibana/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/helpers/validation.ts

Lines 93 to 95 in 3b6b7ad

try {

validator.validate(doc, this.kibanaVersion);

} catch (error) {

You're entirely right, version isn't used anymore and it doesn't make sense to keep dead code around. I will remove it.

pgayvallet

Great work, looking good to me!

Just asked a few questions for clarification before approval:

pgayvallet · 2023-06-02T11:11:31Z

packages/core/saved-objects/core-saved-objects-server/src/model_version/schemas.ts

 */

 import type { ObjectType } from '@kbn/config-schema';
+import { SavedObjectsValidationSpec } from '../validation';


NIT: import type

done https://github.com/elastic/kibana/pull/158527/files#diff-430c626c4c3a1f4ce40c0ca9d40a141c835e61f0dafa10a8e451ba072a674c20R10

pgayvallet · 2023-06-02T11:16:07Z

packages/core/saved-objects/core-saved-objects-base-server-internal/src/validation/validator.ts

-      document.migrationVersion?.[document.type] ??
-      this.defaultVersion;
-    const schemaVersion = previousVersionWithSchema(this.orderedVersions, docVersion);
+    let usedVersion = version;


Correct me if I'm wrong, but in current state, the version option is effectively unused (given usedVersion is always replaced L52-L56), right?

I'm fine either way (keeping it or removing it), but if we do keep it, then we need the version parameter to be used if specified (it doesn't make sense to keep an option ignored by the implementation), and to adapt the call to validate to no longer pass the option, here:

kibana/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/helpers/validation.ts

Lines 93 to 95 in 3b6b7ad

try {

validator.validate(doc, this.kibanaVersion);

} catch (error) {

pgayvallet · 2023-06-02T11:31:23Z

...core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/helpers/validation.ts

+        const virtualVersion = modelVersionToVirtualVersion(key);
+        if (modelVersion.schemas?.create) {
+          combinedSchemas[virtualVersion] = modelVersion.schemas!.create!;
+        }


NIT: modelVersionToVirtualVersion can be done within the if block to avoid calling it when not needed

if (modelVersion.schemas?.create) { const virtualVersion = modelVersionToVirtualVersion(key); combinedSchemas[virtualVersion] = modelVersion.schemas!.create!; }

done: https://github.com/elastic/kibana/pull/158527/files#diff-a6987e7e73107abdbc32cc43871a76dfec7cc99dfc2e9fc815cd338e4de0c457R119-R122

pgayvallet · 2023-06-02T11:39:18Z

.../core/saved-objects/core-saved-objects-base-server-internal/src/validation/validator.test.ts

+    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' });
+      validator.validate(data);
+      expect(getCalledVersion()).toEqual('3.0.0');
    });


NIT: atm, 11.1.4 is an 'invalid' version for any document to have, given there's no reason to have a major superior to 10, so I wouldn't add tests about it tbh. (also, why does it use the 3.0.0 schema and not the 10.1.0 one?)

also, why does it use the 3.0.0 schema and not the 10.1.0 one?

The docVersion is 11.1.4and the default version is 3.2.0. Because 11.1.4 isn't a valid virtual mode version, the conditional:

usedVersion = isVirtualModelVersion(docVersion) ? docVersion : this.defaultVersion;

implements the false path: usedVersion = this.defaultVersion, and previousVersionWithSchema returns 3.0.0.

Hence 3.0.0 and not 10.1.0.

pgayvallet · 2023-06-02T11:41:30Z

.../core/saved-objects/core-saved-objects-base-server-internal/src/validation/validator.test.ts

+    it('should use the correct schema for documents with virtualModelVersion', () => {
+      const data = createMockObject({ typeMigrationVersion: '10.1.0' });
+      validator.validate(data);
+      expect(getCalledVersion()).toEqual('10.1.0');
+    });


NIT: Instead of registering a 10.1.0 schema, I would register one of 10.2.0 and then assert that

a document with version 10.1.0 uses schema 4.3.0

a document with version 10.2.0 uses schema 10.2.0

a document with version 10.3.0 uses schema 10.2.0

a document with version 10.1.0 uses schema 4.3.0

The default version is 3.3.0 in the tests, so the schema that will be used is 3.0.0

TinaHeiligers · 2023-06-02T21:53:50Z

@elasticmachine merge upstream

TinaHeiligers · 2023-06-02T22:12:01Z

@pgayvallet I've answered your questions and handled the changes. Could you please take another look?

TinaHeiligers · 2023-06-04T17:45:56Z

@elasticmachine merge upstream

kibana-ci · 2023-06-04T18:42:42Z

💚 Build Succeeded

Buildkite Build
Commit: 8f4ff4b

Metrics [docs]

Unknown metric groups

API count

id	before	after	diff
`@kbn/core-saved-objects-server`	534	535	+1

ESLint disabled line counts

id	before	after	diff
`enterpriseSearch`	19	21	+2
`securitySolution`	414	418	+4
total			+6

Total ESLint disabled count

id	before	after	diff
`enterpriseSearch`	20	22	+2
`securitySolution`	498	502	+4
total			+6

History

💚 Build #132309 succeeded c38b90e
💚 Build #131900 succeeded 429a70c
💔 Build #131561 failed ef3e6d6
💛 Build #131438 was flaky 3c405e8
💚 Build #130870 succeeded bbedd7d
💛 Build #130865 was flaky c59a391

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

pgayvallet

LGTM

pgayvallet · 2023-06-05T06:18:43Z

.../core/saved-objects/core-saved-objects-base-server-internal/src/validation/validator.test.ts

+      // 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');


NIT: remove commented code

should already be done

TinaHeiligers requested a review from pgayvallet May 26, 2023 02:58

TinaHeiligers commented May 26, 2023

View reviewed changes

...saved-objects/core-saved-objects-api-server-internal/src/lib/apis/helpers/validation.test.ts Outdated Show resolved Hide resolved

TinaHeiligers commented May 26, 2023

View reviewed changes

pgayvallet reviewed May 26, 2023

View reviewed changes

...-objects/core-saved-objects-migration-server-internal/src/document_migrator/model_version.ts Outdated Show resolved Hide resolved

...saved-objects/core-saved-objects-api-server-internal/src/lib/apis/helpers/validation.test.ts Outdated Show resolved Hide resolved

TinaHeiligers changed the title ~~[WIP] Adds support for model version schema~~ [SOR] Adds support for validation schema with models May 29, 2023

TinaHeiligers requested a review from pgayvallet May 29, 2023 01:21

TinaHeiligers added 5 commits May 29, 2023 12:06

Adds support for model version schema

9390d95

Adds tests for ValidationHelper.validateObjectForCreate

4361dc6

Adds code comments about dependencies and assumptions

1476bc1

updates code comments

f69c19a

improvements

ca9b8ac

TinaHeiligers force-pushed the kbn-152994-SO-model-version-support-schema branch from 0ad3f3e to ca9b8ac Compare May 29, 2023 21:33

TinaHeiligers marked this pull request as ready for review May 29, 2023 23:00

TinaHeiligers requested a review from a team as a code owner May 29, 2023 23:00

TinaHeiligers removed the request for review from pgayvallet May 29, 2023 23:00

kibanamachine and others added 3 commits May 29, 2023 19:01

Merge branch 'main' into kbn-152994-SO-model-version-support-schema

c59a391

Assert validation against virtual model version

69f80a8

Assert validation against virtual model version

bbedd7d

afharo reviewed May 30, 2023

View reviewed changes

...core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/helpers/validation.ts Show resolved Hide resolved

packages/core/saved-objects/core-saved-objects-base-server-internal/src/validation/validator.ts Outdated Show resolved Hide resolved

TinaHeiligers marked this pull request as draft May 30, 2023 17:48

kibanamachine and others added 4 commits May 31, 2023 13:42

Merge branch 'main' into kbn-152994-SO-model-version-support-schema

3c405e8

Add comments

8f4ca19

adds unit tests for new behavior

0b0b5ae

Remove option to specify validation version from SOR create and bulkC…

ef3e6d6

…reate. The version is set within the validator

TinaHeiligers commented Jun 1, 2023

View reviewed changes

TinaHeiligers added 3 commits June 1, 2023 12:50

Don't check version against default

93f3e54

comments cleanup

fdde775

Improve type safety check

429a70c

TinaHeiligers commented Jun 1, 2023

View reviewed changes

...core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/helpers/validation.ts Show resolved Hide resolved

TinaHeiligers commented Jun 1, 2023

View reviewed changes

TinaHeiligers marked this pull request as ready for review June 1, 2023 22:02

pgayvallet reviewed Jun 2, 2023

View reviewed changes

TinaHeiligers added 2 commits June 2, 2023 11:16

PR review comments

463b981

add test for other conditionals

3c25cb2

kibanamachine and others added 2 commits June 2, 2023 17:53

Merge branch 'main' into kbn-152994-SO-model-version-support-schema

ee9d035

cleanup

c38b90e

Merge branch 'main' into kbn-152994-SO-model-version-support-schema

8f4ff4b

pgayvallet approved these changes Jun 5, 2023

View reviewed changes

TinaHeiligers merged commit fd068da into elastic:main Jun 5, 2023

TinaHeiligers deleted the kbn-152994-SO-model-version-support-schema branch June 5, 2023 13:11

	try {
	validator.validate(doc, this.kibanaVersion);
	} catch (error) {

[SOR] Adds support for validation schema with models #158527

[SOR] Adds support for validation schema with models #158527

Uh oh!

Conversation

TinaHeiligers commented May 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

TinaHeiligers left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

elasticmachine commented May 29, 2023

Uh oh!

TinaHeiligers commented May 29, 2023

Uh oh!

afharo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

TinaHeiligers commented May 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TinaHeiligers commented May 31, 2023

Uh oh!

TinaHeiligers left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TinaHeiligers left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pgayvallet left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TinaHeiligers Jun 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TinaHeiligers commented Jun 2, 2023

Uh oh!

TinaHeiligers commented Jun 2, 2023

Uh oh!

TinaHeiligers commented Jun 4, 2023

Uh oh!

kibana-ci commented Jun 4, 2023

💚 Build Succeeded

Metrics [docs]

API count

ESLint disabled line counts

TinaHeiligers commented May 26, 2023 •

edited

Loading

TinaHeiligers commented May 30, 2023 •

edited

Loading

TinaHeiligers Jun 2, 2023 •

edited

Loading