Restructure SavedObject types internal representation #56378

pgayvallet · 2020-01-30T13:22:05Z

Summary

Main goal of the PR is to introduce the new SavedObjectType type that is an aggregation of current SO schema, mappings and migrations (it will also handle validations) later but this is outside the scope of this PR)

export interface SavedObjectsType {
  name: string;
  hidden: boolean;
  namespaceAgnostic: boolean;
  indexPattern?: string;
  convertToAliasScript?: string;

  mappings: SavedObjectsTypeMappingDefinition;
  migrations?: SavedObjectMigrationMap;
}

Old legacy SO types registration are converted to the new format during the service setup phase.

PR also introduce the new SavedObjectTypeRegistry registry, that superseed the responsibilities of the now deprecated SavedObjectsSchema. It keep the list of all registered types, and is propagated alongs the SO internal services, such as the KibanaMigrator. This avoid passing savedObjectSchemas, savedObjectsMappings and savedObjectsMigrations everywhere and greatly simplifies a lot of internal APIs and calls.

Other things this PR does:

Some plugins were manually instantiating a SavedObjectsSchema to be able to instanciate their own SavedObjectSerializer. as the serializer now relies on a SavedObjectTypeRegistry, and as I did not want the registry to be accessible, I added a createSerializer API to SavedObjectStart to be able to get a ready-to-use serializer.
Rename and document a lot of previously internal types that are now public as SavedObjectSerializer and SavedObjectType are exported and referencing them
Expose a registerType API to saved object internal setup contract. This is only here for testing for now, and can only be made public once Adapt ES-Archiver to use savedObject types defined in the new platform #55860 is resolved.
Get rid of the LegacyConfig usages in the SavedObject codebase. As legacy types are converted during the setup phase, the legacy config is no longer needed on lower levels.

Things this PR does not:

Introduce any new public SO API from core.
Handle the savedObject validations (legacy validations are still used, but are not included in the new SavedObjectType structure

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

~~[ ] This was checked for cross-browser compatibility, including a check against IE11~~
~~[ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support~~
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
~~[ ] This was checked for keyboard-only and screenreader accessibility~~

For maintainers

This was checked for breaking API changes and was labeled appropriately
This includes a feature addition or change that requires a release note and was labeled appropriately

pgayvallet · 2020-01-30T14:40:58Z

src/core/server/saved_objects/migrations/kibana/kibana_migrator.test.ts

-    it('Fails if duplicate mappings are defined', () => {
-      const options = mockOptions();
-      options.savedObjectMappings = [
-        {
-          pluginId: 'aaa',
-          properties: { amap: { type: 'text' } },
-        },


The registry already ensure types are only registered once. Boths tests and Map implementation are ensuring this, the test was obsolete.

pgayvallet · 2020-01-30T14:43:23Z

src/core/server/saved_objects/migrations/types.ts

+export type SavedObjectMigrationFn = (
+  doc: RawSavedObjectDoc,
+  log: SavedObjectsMigrationLogger
+) => RawSavedObjectDoc;

-export const schemaMock = {
-  create: createSchemaMock,
-};
+export interface SavedObjectMigrationMap {
+  [version: string]: SavedObjectMigrationFn;
+}


This is undocumented for now as SavedObjectsType is still an internal type.

pgayvallet · 2020-01-30T14:48:03Z

src/core/server/saved_objects/saved_objects_type_registry.mock.ts

+  mock.isHidden.mockReturnValue(false);
+  mock.isNamespaceAgnostic.mockImplementation((type: string) => type === 'global');
+
+  return mock as any;


I couldn't find how to properly type the mock. TS keep being unhappy about the types private field on SavedObjectTypeRegistry. I didn't want to introduce an interface ONLY to have proper mock typing, so I force-casted.

🤷‍♂ we've done in this in other places. You don't necessarily have to actually export the interface, just use it for the mock and export the mock function. It will be type compatible while still making sure the mock is the correct type.

Issue is not about returning the interface, it's that my mock doesn't respect the jest.Mocked<SavedObjectTypeRegistry> type, as it's does not have the readonly types property. Changing the mock type to jest.Mocked<PublicMethodsOf<SavedObjectTypeRegistry>> fails when using the mock with

Argument of type 'Mocked<Pick<SavedObjectTypeRegistry, "getType" | "registerType" | "getAllTypes" | "isNamespaceAgnostic" | "isHidden" | "getIndex">>' is not assignable to parameter of type 'SavedObjectTypeRegistry'.

The only problem here is that internal mocks aren't type-safe and might be out of sync when SavedObjectTypeRegistry updates the interface. #54906 postponed, probably adding a separate interface is not such a bad idea.

Added the ISavedObjectTypeRegistry interface

pgayvallet · 2020-01-30T14:52:04Z

src/core/server/saved_objects/utils.test.ts

+    const converted = convertLegacyTypes(uiExports, legacyConfig);
+    expect(converted).toMatchSnapshot();


I usually hate snapshot testing, however the structure generated is just too big to manually assert or even using inline snapshots.

pgayvallet · 2020-01-30T14:55:09Z

src/core/server/server.api.md

+// @public (undocumented)
+export interface SavedObjectsComplexFieldMapping {
+    // (undocumented)
+    dynamic?: string;
+    // (undocumented)
+    properties: SavedObjectsMappingProperties;
+    // (undocumented)
+    type?: string;
+}
+
+// @public (undocumented)
+export interface SavedObjectsCoreFieldMapping {
+    // (undocumented)
+    enabled?: boolean;


All these types are made public because they are SavedObjectMapping underlying types. However we are closer to ES definitions than SO here, so I'm not sure documenting them got much value.

Can we link to elasticsearch docs describing the format of the mappings? https://www.elastic.co/guide/en/elasticsearch/reference/current/mapping-types.html
Is it possible to add proper typings for in the future? For example, type is not just a string, but subset of strings.

Can we link to elasticsearch docs describing the format of the mappings?

Yea (thanks for the link). However these type are wrongly defined as public (as I picked the commits from my 'mappings' PR), and are not ATM exposed in any API. I would like to keep all documentation in the next PR where we expose registerType. Would declaring them back to internal for now be acceptable?

Is it possible to add proper typings for in the future?

We do want that. However I would really like to avoid redefining all direct ES types, and I think these types are not defined in the ES library we are using. Maybe this could be resolved when using the new es node library?

However I would really like to avoid redefining all direct ES types, and I think these types are not defined in the ES library we are using. Maybe this could be resolved when using the new es node library?

I didn't find them in elasticsearch-js either. @delvedor are there any plans to add them?

Hello! The new js client does not have type definitions for Elasticsearch's request and response bodies, only for URL and query parameters.
It does offer a solution for fixing this through generics, see the typescript documentation.

Added link to the documentation. The generic does not answer our needs though, as in our part of the SO code, we have no way to be aware of the actual SO type we are working on.

We will have to improve the typings on our side I guess, but this should be done in another PR (We need improvement on pretty much everything in the src/core/server/saved_objects subfolders...)

We need improvement on pretty much everything in the src/core/server/saved_objects subfolders...

Would you mind creating a list of follow-up tasks?

Sure, will add this as a condition to merge this PR.

Created #56856

pgayvallet · 2020-01-30T14:56:05Z

src/es_archiver/lib/indices/kibana_index.js

+  const savedObjectTypes = convertLegacyTypes(uiExports, config);
+  const typeRegistry = new SavedObjectTypeRegistry();
+  savedObjectTypes.forEach(type => typeRegistry.registerType(type));


Adapted the ES-archiver to use new format. #55860 still need to be addressed.

pgayvallet · 2020-01-30T14:57:16Z

src/legacy/server/saved_objects/saved_objects_mixin.js

-  const schema = new SavedObjectsSchema(kbnServer.uiExports.savedObjectSchemas);
+  const schema = new SavedObjectsSchema(convertTypesToLegacySchema(typeRegistry.getAllTypes()));


I wanted to totally get rid of SavedObjectSchema, however the legacy service is still exposing it. Had to keep compatibility by converting back the types to the old schema format.

pgayvallet · 2020-01-30T15:03:18Z

x-pack/plugins/task_manager/server/create_task_manager.ts

-  // as we use this Schema solely to interact with Tasks, we
-  // can initialise it with solely the Tasks schema
-  const serializer = new SavedObjectsSerializer(new SavedObjectsSchema(savedObjectSchemas));
  return new TaskManager({
    taskManagerId: core.uuid.getInstanceUuid(),
    config,
    savedObjectsRepository,
-    serializer,
+    serializer: savedObjectsSerializer,


@elastic/kibana-alerting-services the serializer now requires an internal core type to be instantiated. I exposed an API from core to retrieve a configured serializer, and adapted your call to use it instead.

The createTaskManager method no longer relies on any legacy API, however I did not feel confident on impacting too much you codebase, so I kept the call as is.

You should be good to remove the registerLegacyAPI trick in x-pack/legacy/plugins/task_manager/server/index.ts and x-pack/plugins/task_manager/server/plugin.ts once this lands.

pgayvallet · 2020-02-04T12:39:11Z

@joshdover @restrry I think I addressed all your current comments. PTAL

rudolf

Haven't worked through all the changes, but wanted to give the feedback I have so long.

src/core/server/mocks.ts

rudolf · 2020-02-04T14:33:23Z

src/core/server/saved_objects/migrations/types.ts

-};
+export type SavedObjectMigrationFn = (
+  doc: RawSavedObjectDoc,
+  log: SavedObjectsMigrationLogger


Similar to the SavedObjectsMigrationLogger we could make breaking changes and refactor the existing migrations, but since many migrations are written in javascript and a single uncaught exception can prevent kibana from starting up, this is risky. So I think it's better we keep the API and refactor it post 8.0.0.

src/core/server/server.api.md

kertal

Code LGTM, just took a look at the modified test where KibanaApp is Codeowner.

mistic

The changes to operations team files LGTM

LeeDr · 2020-02-05T14:19:20Z

Old legacy SO types registration are converted to the new format during the service setup phase.

Could you explain when the setup phase happens?

On startup? Linked to migrations?
On saved object import?
When beats setup scripts run and used saved object api to create objects?

Also, we probably want v8.0.0 label on this PR assuming it's going in to master, and then would be backported to 7.x branch.

pgayvallet · 2020-02-05T14:25:58Z

@LeeDr

Could you explain when the setup phase happens?

That would be core's setup phase. During Kibana startup, before plugin setup phase. Not sure to really understand the question?

We are talking about savedObjects types here, not actual SO documents. This is not impacting savedObjects data in any way.

…bjects-structure-change

rudolf · 2020-02-05T14:50:28Z

src/core/server/saved_objects/migrations/types.ts

+ * A map of {@link SavedObjectMigrationFn | migration functions} to be used for a given type.
+ * The map's keys must be valid semver versions.
+ *
+ * Migrations will be executed in order, starting from the lowest matching an higher version that the document


Nit suggestion:

For a given document, only migrations with a higher version number than that of the document will be applied. Migrations are executed in order, starting from the lowest version and ending with the highest one.

rudolf · 2020-02-05T15:03:43Z

src/core/server/saved_objects/saved_objects_type_registry.ts

+  public isNamespaceAgnostic(type: string) {
+    return this.types.get(type)?.namespaceAgnostic ?? false;
+  }
+
+  public isHidden(type: string) {
+    return this.types.get(type)?.hidden ?? false;
+  }
+
+  public getIndex(type: string) {
+    return this.types.get(type)?.indexPattern;
+  }


Maybe you can add a comment to the code?

…bjects-structure-change

kibanamachine · 2020-02-06T08:47:28Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 3c7b2d0

History

💚 Build #24650 succeeded c20039a
💔 Build #24601 failed 252f1de
💚 Build #24585 succeeded 7218b9f
💚 Build #24250 succeeded c1721b1
💚 Build #24243 succeeded 90d1fab

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

* adapt types and tests to prepare for new NP api * rename and export public types * update generated doc * first implementation of registerMappings * adapt es archiver to convert legacy mappings * update generated doc * fix more tests * add unit tests * add legacy-compat unit test * add documentation and examples * Introduce SavedObjectTypeRegistry and SavedObjectType types * add and fix tests * expose createSerializer API and fix usages * remove registerMappings API, add internal registerType * revert changes to migration guide * adapt ES-archiver migrator creation * export serializer-related types * update generated doc * add and use convertTypesToLegacySchema * remove / move to internal some mapping types * fix forEach closure context * add missing docs * fix core path * some nits * fix so_mixin tests * fix integration tests * fix integration tests for real * add documentation on serializer + restructure files and types * nit * add and use the ISavedObjectTypeRegistry interface * Add documentation, deprecates migrationLogger#warning * better typing for SavedObjectsRawDoc._source * nits * update generated doc * remove exposition of SavedObjectsTypeMappingDefinitions, update doc * creates so internal contracts mocks * improve documentation

pgayvallet added 25 commits January 30, 2020 13:25

adapt types and tests to prepare for new NP api

cd8cb8a

rename and export public types

2766384

update generated doc

a384ea6

first implementation of registerMappings

f7f3e9c

adapt es archiver to convert legacy mappings

674f66b

update generated doc

f3e7cdb

fix more tests

5e8645e

add unit tests

3e117de

add legacy-compat unit test

5a36c5d

add documentation and examples

9cbe0f6

Introduce SavedObjectTypeRegistry and SavedObjectType types

35df8c6

add and fix tests

1ac3899

expose createSerializer API and fix usages

0261515

remove registerMappings API, add internal registerType

25bc1cb

revert changes to migration guide

765b0f3

adapt ES-archiver migrator creation

941e18b

export serializer-related types

877abcc

update generated doc

cdf3f11

add and use convertTypesToLegacySchema

c858eff

remove / move to internal some mapping types

044415e

fix forEach closure context

f9ac4e6

add missing docs

8189808

fix core path

62ac23e

some nits

cd2b2ad

fix so_mixin tests

b3ab84f

pgayvallet commented Jan 30, 2020

View reviewed changes

pgayvallet added release_note:skip Skip the PR/issue when compiling release notes v7.7.0 Feature:New Platform Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc labels Jan 30, 2020

pgayvallet added 3 commits February 4, 2020 09:24

better typing for SavedObjectsRawDoc._source

abc1f8d

nits

1623d8e

update generated doc

c1721b1

pgayvallet requested a review from a team February 4, 2020 08:35

rudolf reviewed Feb 4, 2020

View reviewed changes

mshustov approved these changes Feb 5, 2020

View reviewed changes

kertal approved these changes Feb 5, 2020

View reviewed changes

mistic approved these changes Feb 5, 2020

View reviewed changes

pgayvallet mentioned this pull request Feb 5, 2020

[Platform] SavedObject Management #50308

Closed

5 tasks

pgayvallet added the v8.0.0 label Feb 5, 2020

pgayvallet added 3 commits February 5, 2020 15:27

Merge remote-tracking branch 'upstream/master' into kbn-55857-saved-o…

7218b9f

…bjects-structure-change

remove exposition of SavedObjectsTypeMappingDefinitions, update doc

cfe4aba

creates so internal contracts mocks

252f1de

pgayvallet mentioned this pull request Feb 5, 2020

SavedObjects jsdoc improvements and code cleanup #56856

Closed

6 tasks

rudolf reviewed Feb 5, 2020

View reviewed changes

improve documentation

c20039a

joshdover approved these changes Feb 5, 2020

View reviewed changes

Merge remote-tracking branch 'upstream/master' into kbn-55857-saved-o…

3c7b2d0

…bjects-structure-change

pgayvallet merged commit ea7c78d into elastic:master Feb 6, 2020

pgayvallet mentioned this pull request Feb 6, 2020

[7.x] Restructure SavedObject types internal representation (#56378) #56953

Merged

mshustov mentioned this pull request Feb 11, 2020

hits.total type check fails due to ES 7.0 schema updates #56694

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restructure SavedObject types internal representation #56378

Restructure SavedObject types internal representation #56378

pgayvallet commented Jan 30, 2020 •

edited by mshustov

Loading

pgayvallet Jan 30, 2020

pgayvallet Jan 30, 2020

pgayvallet Jan 30, 2020

joshdover Jan 31, 2020

pgayvallet Feb 3, 2020

mshustov Feb 3, 2020

pgayvallet Feb 4, 2020

pgayvallet Jan 30, 2020

pgayvallet Jan 30, 2020

mshustov Feb 3, 2020 •

edited

Loading

pgayvallet Feb 3, 2020

mshustov Feb 3, 2020

delvedor Feb 3, 2020

pgayvallet Feb 4, 2020

mshustov Feb 5, 2020

pgayvallet Feb 5, 2020

pgayvallet Feb 5, 2020

pgayvallet Jan 30, 2020

pgayvallet Jan 30, 2020

pgayvallet Jan 30, 2020

pgayvallet commented Feb 4, 2020

rudolf left a comment

rudolf Feb 4, 2020

kertal left a comment

mistic left a comment

LeeDr commented Feb 5, 2020

pgayvallet commented Feb 5, 2020

rudolf Feb 5, 2020

rudolf Feb 5, 2020

kibanamachine commented Feb 6, 2020

		const converted = convertLegacyTypes(uiExports, legacyConfig);
		expect(converted).toMatchSnapshot();

		const schema = new SavedObjectsSchema(kbnServer.uiExports.savedObjectSchemas);
		const schema = new SavedObjectsSchema(convertTypesToLegacySchema(typeRegistry.getAllTypes()));

Restructure SavedObject types internal representation #56378

Restructure SavedObject types internal representation #56378

Conversation

pgayvallet commented Jan 30, 2020 • edited by mshustov Loading

Summary

Checklist

For maintainers

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mshustov Feb 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgayvallet commented Feb 4, 2020

rudolf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kertal left a comment

Choose a reason for hiding this comment

mistic left a comment

Choose a reason for hiding this comment

LeeDr commented Feb 5, 2020

pgayvallet commented Feb 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibanamachine commented Feb 6, 2020

💚 Build Succeeded

History

pgayvallet commented Jan 30, 2020 •

edited by mshustov

Loading

mshustov Feb 3, 2020 •

edited

Loading