Add SavedObject management section registration in core by pgayvallet · Pull Request #59291 · elastic/kibana

pgayvallet · 2020-03-04T10:34:58Z

Summary

Step 1/3 of #50308
Fix #59349

Allow plugins to register their types management sections using the registerType API.
This should be the last part of SO registration, allowing plugins to totally drop legacy regarding SO types.
Also remove the code handling the config type as a special one. the config (aka settings aka uiSettings aka advancedSettings) type is now registered from core as any other one.

Checklist

Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios

elasticmachine · 2020-03-04T10:35:21Z

Pinging @elastic/kibana-platform (Team:Platform)

pgayvallet · 2020-03-04T10:43:55Z

src/core/server/index.ts

    savedObjects: {
      client: SavedObjectsClientContract;
+      typeRegistry: ISavedObjectTypeRegistry;
    };


This is going to be used for the next step of the migration (migrate the server routes in new plugin). I could retrieve the registry using getStartServices, but I thought it made sense to expose it there. Tell me if I should remove it.

Makes sense to me to have it here 👍

pgayvallet · 2020-03-04T10:45:40Z

src/core/server/saved_objects/saved_objects_service.ts

+    const getImportableAndExportableTypes = () => {
+      return this.typeRegistry
+        .getAllTypes()
+        .map(type => type.name)
+        .filter(type => this.typeRegistry.isImportableAndExportable(type));
+    };


Same 'issue' than with the migrator, The (NP registered) types are not registered until the end of the setup phase, I now need an accessor to retrieve them instead of raw values.

Couldn't these routes use the context.savedObjects.typeRegistry to get this data? If so, we could just add a method to the type registry that does this filtering.

Good catch. I actually did that before adding the typeRegistry to the context. The routes should use the context instead.

Hum, actually, the types are needed outside of the handler:

const typeSchema = schema.string({ validate: (type: string) => { if (!getSupportedTypes().includes(type)) { return `${type} is not exportable`; } }, });

Guess I can move the validation in the handler's body instead.

…gement-registration-in-core

joshdover · 2020-03-05T17:24:01Z

ack: will review today

…gement-registration-in-core

pgayvallet · 2020-03-06T12:43:57Z

I adapted the so management service to be aware of NP types by using the registry. This causes functional test failures caused by the issue logged here: #59349

I guess I will have to make the changes regarding the config SO type in this PR.

…o type.

pgayvallet · 2020-03-06T13:27:26Z

src/legacy/server/kbn_server.d.ts

      scopedTutorialContextFactory: (...args: any[]) => any
    ) => void;
-    savedObjectsManagement(): SavedObjectsManagement;
+    getSavedObjectsManagement(): SavedObjectsManagement;


Even if not used, the d.ts definition was wrong.

pgayvallet · 2020-03-06T13:29:00Z

src/core/server/saved_objects/saved_objects_service.ts

+    this.typeRegistry.registerType(configSavedObjectType);
+


I'm registering the config type from the so service itself. @restrry do you think it make more sense to have the type declaration in src/core/server/ui_settings and having the uiSettings service performs the registration instead?

yeah, I think we should. I was planning to do it on the final cleanup when we move ui settings definitions to NP https://github.com/elastic/kibana/blob/7e087633d26dfebe5cf262bbfde2cd4c29770464/src/legacy/core_plugins/kibana/ui_setting_defaults.js
but it can be done now. do you want me to create an issue?

Can do in this PR, moving the type to the ui_settings folder should be trivial

mshustov

some minors. LGTM

mshustov · 2020-03-06T13:18:00Z

src/core/server/saved_objects/types.ts

+   * The icon name to display in the management table.
+   * If not defined, no icon will be displayed.
+   */
+  icon?: string;


is it icon name or icon url? Applications configure URL for icon and name(?) for euiIconType

kibana/src/core/public/application/types.ts

Lines 117 to 123 in 48a33ab

euiIconType?: string;

/**

* A URL to an image file used as an icon. Used as a fallback

* if `euiIconType` is not provided.

*/

icon?: string;

It's an EUI icon name.

I.E

kibana/src/legacy/core_plugins/kibana/index.js

Lines 180 to 181 in 4000d4e

dashboard: {

icon: 'dashboardApp',

Now is the good time to rename any of the management property. Shall I rename icon to euiIconType ?

(minor) Also my description is wrong. A default apps icon will actually be used if not present

kibana/src/legacy/core_plugins/kibana/public/management/sections/objects/components/objects_table/components/table/table.js

Lines 184 to 188 in 8e9a8a8

<EuiIcon

aria-label={getSavedObjectLabel(type)}

type={object.meta.icon || 'apps'}

size="s"

/>

Now is the good time to rename any of the management property. Shall I rename icon to euiIconType ?

I prefer euiIconType personally to make it explicit that it is not the same as the icon option on the AppBase type.

mshustov · 2020-03-06T13:18:06Z

src/core/server/saved_objects/types.ts

+   * Function returning the url to use to redirect to this object from the management section.
+   * If not defined, redirecting to the object will not be allowed.
+   */
+  getInAppUrl?: (savedObject: SavedObject<any>) => { path: string; uiCapabilitiesPath: string };


could you add comments what returned values mean? uiCapabilitiesPath doesn't say much

mshustov · 2020-03-06T13:25:23Z

src/core/server/saved_objects/types.ts

+
+/**
+ * @internal
+ * @deprecated


optional nit: would be good to provide a reference to SavedObjectsTypeManagementDefinition as recommended substitution

mshustov · 2020-03-06T13:41:56Z

src/core/server/saved_objects/routes/export.ts

+        .getImportableAndExportableTypes()
+        .map(t => t.name);
+      if (types) {
+        const invalidTypes = types.filter(t => !supportedTypes.includes(t));


Optional: it would be easier to test if we extract this logic in a separate function out of the route handler.

mshustov · 2020-03-06T13:43:45Z

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


-        expect(typeRegistryInstanceMock.registerType).toHaveBeenCalledTimes(1);
+        // the config type is also registered during setup
+        expect(typeRegistryInstanceMock.registerType).toHaveBeenCalledTimes(internalTypesCount + 1);


is this comment related to internalTypesCount and should be placed near https://github.com/elastic/kibana/pull/59291/files#diff-ae6a02eb060bb735014b535756384724R40 ?

It is. But as I'm going to move the config registration to ui_settings, this internalTypesCount is going to disappear from this file.

mshustov · 2020-03-06T13:46:45Z

src/core/server/saved_objects/saved_objects_service.ts

+    this.typeRegistry.registerType(configSavedObjectType);
+


yeah, I think we should. I was planning to do it on the final cleanup when we move ui settings definitions to NP https://github.com/elastic/kibana/blob/7e087633d26dfebe5cf262bbfde2cd4c29770464/src/legacy/core_plugins/kibana/ui_setting_defaults.js
but it can be done now. do you want me to create an issue?

rudolf · 2020-03-06T16:42:26Z

src/core/server/saved_objects/routes/export.ts

+          return res.badRequest({
+            body: {
+              message: `Trying to export object(s) with non-exportable types: ${invalidObjects
+                .map(obj => `${obj.type}-${obj.id}`)


Do we have any precedence for the formatting of a list of objects in our API's? I would have expected a : instead of an - like ${obj.type}:${obj.id} but if we're using the dash somewhere else it might be better to stay consintent.

No you are right, : is what should be used here. We dont use the dash anywhere.

resolutions[`${type}:${id}`] = overwrite;

rudolf · 2020-03-06T17:10:34Z

src/core/server/saved_objects/so_types/config.ts

+  hidden: false,
+  namespaceAgnostic: false,
+  mappings: {
+    dynamic: true as any, // TODO: check if can be switched to false, else adapt the type to allow true


I'm not sure if we use it, but with dynamic: false we wouldn't be able to do telemetry on configuration objects which might be useful information.

I'm too afraid to touch that value anyway, this will stay true.

Now the question is: do we want to allow plugins to register types with dynamic=true mappings? As josh suggested, we could have our own internal type representation that allow true where it's not allowed in the public SavedObjectType, however this means a lot of changes for something that can be force-casted anyway if plugins really wants to do it, so I'm leaning to just change false | 'strict' to boolean | strict. @joshdover wdyt?

I'm not sure if we use it, but with dynamic: false we wouldn't be able to do telemetry on configuration objects which might be useful information.

I don't really think this has to be implemented as a ES query even if we did have telemetry like this. We only have one of these objects per space, per release version. I'm not sure how many spaces we support, but I can't imagine a customer ever having so many that doing this calculation in-memory would be a problem.

That said, it may not be worth doing this change right now.

so I'm leaning to just change false | 'strict' to boolean | strict

I lean towards just keeping it the way you have it (force casted) so as to not encourage plugins to use dynamic: true since it is probably never what we want or need. If a legitimate use case comes up, we can change this.

Ok, changing back to force-casting with explanatory comment

rudolf · 2020-03-06T17:11:24Z

src/core/server/saved_objects/types.ts

   */
  migrations?: SavedObjectMigrationMap;
+  /**
+   * An optional {@link SavedObjectsTypeManagementDefinition | management section} definition for the type.


Suggested change

* An optional {@link SavedObjectsTypeManagementDefinition | management section} definition for the type.

* An optional {@link SavedObjectsTypeManagementDefinition | saved objects management section} definition for the type.

rudolf · 2020-03-06T17:15:31Z

src/core/server/saved_objects/types.ts

+  /**
+   * Function returning the url to use to redirect to the edition page of this object.
+   * If not defined, edition will not be allowed.
+   */


Suggested change

/**

* Function returning the url to use to redirect to the edition page of this object.

* If not defined, edition will not be allowed.

*/

/**

* Function returning the url to use to redirect to the editing page of this object.

* If not defined, editing will not be allowed.

*/

…gement-registration-in-core

pgayvallet · 2020-03-09T14:10:03Z

retest

…gement-registration-in-core

… config type

kibanamachine · 2020-03-10T09:41:22Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 12db03e

History

💚 Build #31973 succeeded 347c6f3
💚 Build #31753 succeeded 0a45d03
💚 Build #31654 succeeded ef5b438
💛 Build #31473 was flaky c18b248
💔 Build #31429 failed b130e59

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

* add management section to SavedObjectsType * adapt import/export routes to get types accessor * add documentation * update generated doc * update migration guide * use request context to access exportable types * update generated doc * adapt SavedObjectsManagement to use the registry * stop magical tricks about the config type, register it as any other so type. * fix FTR assertions * fix so_mixin tests * register the `config` type from the uiSettings service * nits and comments * update generated doc * remove true from dynamic property definition, use force-cast back for config type * remove obsolete test comment

* master: [Logs / Metrics UI] Link handling / stop page reloads (elastic#58478) Add SavedObject management section registration in core (elastic#59291)

) * add management section to SavedObjectsType * adapt import/export routes to get types accessor * add documentation * update generated doc * update migration guide * use request context to access exportable types * update generated doc * adapt SavedObjectsManagement to use the registry * stop magical tricks about the config type, register it as any other so type. * fix FTR assertions * fix so_mixin tests * register the `config` type from the uiSettings service * nits and comments * update generated doc * remove true from dynamic property definition, use force-cast back for config type * remove obsolete test comment

* master: Add a retry to dashboard test for a sometimes slow async operation (elastic#59600) [Endpoint] Sample data generator for endpoint app (elastic#58936) [Vis Editor] Refactoring metrics axes (elastic#59135) [DOCS] Changed Discover app to Discover (elastic#59769) [Metrics Alerts] Add support for search query and groupBy on alerts (elastic#59388) Enhancement - EUICodeEditor for Visualize JSON (elastic#58679) [ML] Transforms: Data grid fixes. (elastic#59538) [SIEM] Fix and consolidate handling of error responses in the client (elastic#59438) [Maps] convert tooltip classes to typescript (elastic#59589) [ML] Functional tests - re-activate date_nanos test (elastic#59649) Move canvas to use NP Expressions service (elastic#58387) Update misc dependencies (elastic#59542) [Unit Testing] Configure react-testing-library queries to use Kibana's data-test-subj instead of default data-testid (elastic#59445) [Console] Remove unused code (elastic#59554) [Logs / Metrics UI] Link handling / stop page reloads (elastic#58478) Add SavedObject management section registration in core (elastic#59291)

pgayvallet added 5 commits March 2, 2020 15:59

add management section to SavedObjectsType

9928a93

adapt import/export routes to get types accessor

19de765

add documentation

9babf8d

update generated doc

48c18ac

update migration guide

9e22cec

pgayvallet added the Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// label Mar 4, 2020

pgayvallet mentioned this pull request Mar 4, 2020

[Platform] SavedObject Management #50308

Closed

5 tasks

pgayvallet added Feature:New Platform release_note:skip Skip the PR/issue when compiling release notes v7.7.0 v8.0.0 labels Mar 4, 2020

pgayvallet commented Mar 4, 2020

View reviewed changes

pgayvallet marked this pull request as ready for review March 4, 2020 10:49

pgayvallet requested a review from a team as a code owner March 4, 2020 10:49

pgayvallet added 2 commits March 4, 2020 12:09

Merge remote-tracking branch 'upstream/master' into kbn-50308-so-mana…

fda0bfa

…gement-registration-in-core

Merge remote-tracking branch 'upstream/master' into kbn-50308-so-mana…

026eea6

…gement-registration-in-core

This was referenced Mar 4, 2020

[App Arch] Saved object migrations from kibana plugin to new platform #59044

Merged

Saved Objects config type export broken on master #59349

Closed

pgayvallet added 4 commits March 6, 2020 11:23

use request context to access exportable types

d8451dc

Merge remote-tracking branch 'upstream/master' into kbn-50308-so-mana…

d62c942

…gement-registration-in-core

update generated doc

b504359

adapt SavedObjectsManagement to use the registry

579c3ca

stop magical tricks about the config type, register it as any other s…

4000d4e

…o type.

pgayvallet commented Mar 6, 2020

View reviewed changes

mshustov approved these changes Mar 6, 2020

View reviewed changes

pgayvallet added 2 commits March 6, 2020 15:34

fix FTR assertions

b130e59

fix so_mixin tests

c18b248

rudolf reviewed Mar 6, 2020

View reviewed changes

pgayvallet added 5 commits March 9, 2020 08:22

Merge remote-tracking branch 'upstream/master' into kbn-50308-so-mana…

ef5b438

…gement-registration-in-core

register the config type from the uiSettings service

66c2324

nits and comments

926a900

update generated doc

f632b08

Merge remote-tracking branch 'upstream/master' into kbn-50308-so-mana…

0a45d03

…gement-registration-in-core

pgayvallet added 3 commits March 10, 2020 08:16

Merge remote-tracking branch 'upstream/master' into kbn-50308-so-mana…

41564ea

…gement-registration-in-core

remove true from dynamic property definition, use force-cast back for…

347c6f3

… config type

remove obsolete test comment

12db03e

rudolf approved these changes Mar 10, 2020

View reviewed changes

pgayvallet merged commit f1272b5 into elastic:master Mar 10, 2020

pgayvallet mentioned this pull request Mar 10, 2020

[7.x] Add SavedObject management section registration in core (#59291) #59731

Merged

alexwizp mentioned this pull request Mar 26, 2020

move visualizations plugin to new platform #60403

Merged

	euiIconType?: string;

	/**
	* A URL to an image file used as an icon. Used as a fallback
	* if `euiIconType` is not provided.
	*/
	icon?: string;

	<EuiIcon
	aria-label={getSavedObjectLabel(type)}
	type={object.meta.icon \|\| 'apps'}
	size="s"
	/>

	* An optional {@link SavedObjectsTypeManagementDefinition \| management section} definition for the type.
	* An optional {@link SavedObjectsTypeManagementDefinition \| saved objects management section} definition for the type.

Conversation

pgayvallet commented Mar 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Uh oh!

elasticmachine commented Mar 4, 2020

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!

joshdover commented Mar 5, 2020

Uh oh!

pgayvallet commented Mar 6, 2020

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!

mshustov 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!

mshustov Mar 6, 2020 • 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!

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!

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!

pgayvallet commented Mar 9, 2020

Uh oh!

kibanamachine commented Mar 10, 2020

💚 Build Succeeded

History

Uh oh!

Reviewers

Assignees

Labels

pgayvallet commented Mar 4, 2020 •

edited

Loading

mshustov Mar 6, 2020 •

edited

Loading