add generic typings for SavedObjectMigrationFn #63943

pgayvallet · 2020-04-20T08:39:13Z

Summary

Add generic types for input and output attributes on SavedObjectMigrationFn to allow plugin implementations to have some type checking on their migrations. Attributes types defaults to unknown
Adapt existing usages of SavedObjectMigrationFn to add <any, any> typings.

Checklist

Documentation was added for features that require explanation or tutorials

rudolf · 2020-04-20T12:59:50Z

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

@@ -39,10 +39,10 @@ import { SavedObjectsMigrationLogger } from './core/migration_logger';
 *
 * @public
 */
-export type SavedObjectMigrationFn = (
-  doc: SavedObjectUnsanitizedDoc,
+export type SavedObjectMigrationFn<InputProps = any, MigratedProps = any> = (


This would probably require updating the types of many migrations, but is it possible to default to unknown instead of any? I think that would encourage more plugins to type their migrations and make it explicit that their migration is dangerously untyped.

What do you think about calling these generic arguments InputAttributes and MigratedAttributes since they define the attributes property?

This would probably require updating the types of many migrations, but is it possible to default to unknown instead of any

I wanted to avoid impacting plugin code with that change TBH, therefor the defaulting any. But it may be better to still do it now to encourage yet-to-migrate SO migrations to be explicitly typed.

What do you think about calling these generic arguments InputAttributes and MigratedAttributes since they define the attributes property?

Indeed better. I always mix up attributes and props terminology for SOs.

…ion-fn-generics

pgayvallet · 2020-04-21T07:06:15Z

x-pack/plugins/lens/server/migrations.ts

-export const migrations: Record<string, SavedObjectMigrationFn> = {
+// eslint-disable-next-line @typescript-eslint/no-explicit-any
+export const migrations: Record<string, SavedObjectMigrationFn<any, any>> = {


Caused by

kibana/.eslintrc.js

Lines 736 to 741 in 40f8222

{

files: ['x-pack/plugins/lens/**/*.{ts,tsx}'],

rules: {

'@typescript-eslint/no-explicit-any': 'error',

},

},

pgayvallet · 2020-04-21T07:21:05Z

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

+export type SavedObjectMigrationFn<InputAttributes = unknown, MigratedAttributes = unknown> = (
+  doc: SavedObjectUnsanitizedDoc<InputAttributes>,
  context: SavedObjectMigrationContext
-) => SavedObjectUnsanitizedDoc;
+) => SavedObjectUnsanitizedDoc<MigratedAttributes>;


So, my only interrogation around that is that atm migrations are all mutating and returning the input doc object, causing usage of these introduced generics difficult in existing migrations.

I.E

export const migrateToKibana660: SavedObjectMigrationFn<any, any> = doc => { if (!doc.attributes.hasOwnProperty('disabledFeatures')) { doc.attributes.disabledFeatures = []; } return doc; };

Meaning that

type InputAttrs = { } type OutputAttrs = { disabledFeatures: SomeType[]; } export const migrateToKibana660: SavedObjectMigrationFn<InputAttrs, OutputAttrs> = doc => { if (!doc.attributes.hasOwnProperty('disabledFeatures')) { doc.attributes.disabledFeatures = []; } return doc; };

Would fail to compile with something like property disabledFeatures does not exists on type InputAttrs on the doc.attributes.disabledFeatures = []; line.

Which is why I'm wondering if we should add the output properties on the input doc:

export type SavedObjectMigrationFn<InputAttributes = unknown, MigratedAttributes = unknown> = ( doc: SavedObjectUnsanitizedDoc<InputAttributes & MigratedAttributes>, context: SavedObjectMigrationContext ) => SavedObjectUnsanitizedDoc<MigratedAttributes>;

Ideally I would have been using Partial, doc: SavedObjectUnsanitizedDoc<InputAttributes & Partial<MigratedAttributes>>, however the given example still fails with disabledFeatures is optional on type SavedObjectUnsanitizedDoc<InputAttributes & Partial<MigratedAttributes>> but required on type SavedObjectUnsanitizedDoc<MigratedAttributes> when returning the doc.

The other option being to decide that migration function should properly spread / construct the resulting migrated object without returning directly the input doc. @rudolf wdyt?

The other option being to decide that migration function should properly spread / construct the resulting migrated object without returning directly the input doc.

Given the high impact of even a small cannot read property x of undefined bug in a migration I'd say it validates the bit of extra effort and memory to construct a new output object 👍

We can leave existing migrations with any as they are, but then encourage new migrations to be written in this way. We should update our documentation to show this pattern and probably add this to the developer release notes to create some more visibility so that teams don't stumble on this.

As a slightly more type-safe solution than any, teams can still use InputAttributes & MigratedAttributes as their InputAttributes generic paramater.

SGTM, I'll keep the definition as it is right now then.

…ion-fn-generics

jportner

With this in mind:

We can leave existing migrations with any as they are, but then encourage new migrations to be written in this way

Security changes LGTM 👍

rudolf · 2020-04-24T11:57:08Z

It causes some type errors in core, but we should remove the index signature

kibana/src/core/server/saved_objects/serialization/types.ts

Line 63 in 97d1685

[rootProp: string]: any;

Done in #64434

alexwizp

Code LGTM! Thank you

…ion-fn-generics

pgayvallet · 2020-04-28T08:49:57Z

src/plugins/dashboard/server/saved_objects/dashboard_migrations.ts

-  '7.0.0': flow<(doc: SavedObjectUnsanitizedDoc) => DashboardDoc700To720>(migrations700),
-  '7.3.0': flow<SavedObjectMigrationFn>(migrations730),
+  '6.7.2': flow<SavedObjectMigrationFn<any, any>>(migrateMatchAllQuery),
+  '7.0.0': flow<SavedObjectMigrationFn<any, DashboardDoc700To720['attributes']>>(migrations700),


@elastic/kibana-app DashboardAttributesTo720 exists, but is not exported from bwc/types. I used DashboardDoc700To720['attributes'] instead.

pgayvallet · 2020-04-28T08:51:01Z

src/plugins/dashboard/server/saved_objects/migrations_730.test.ts

-  const doc700: DashboardDoc700To720 = migrations['7.0.0'](doc);
+  const doc700 = migrations['7.0.0'](doc, mockContext);


This migration is now an explicit SavedObjectMigrationFn so the second parameter is now needed in the signature, so I had to adapt the tests.

streamich

AppArch code changes look fine to me.

Only question: do we need all those any? Would it be a big ask to put in the right types, even if they don't completely define the saved object but only the part necessary for the migration?

cc @stacey-gammon as she has opinion about anys.

streamich · 2020-04-28T10:58:41Z

Alternatively, maybe we can create an issue to fix those anys and tag Team:AppArch and Team:KibanaApp.

rudolf

Happy for this to be merged once we've updated the tsdocs code sample.

rudolf · 2020-04-28T12:23:53Z

src/plugins/dashboard/server/saved_objects/dashboard_migrations.ts

@@ -62,7 +62,7 @@ function migrateIndexPattern(doc: DashboardDoc700To720) {
  doc.attributes.kibanaSavedObjectMeta.searchSourceJSON = JSON.stringify(searchSource);
 }

-const migrations700: SavedObjectMigrationFn = (doc): DashboardDoc700To720 => {
+const migrations700: SavedObjectMigrationFn<any, any> = (doc): DashboardDoc700To720 => {


Suggested change

const migrations700: SavedObjectMigrationFn<any, any> = (doc): DashboardDoc700To720 => {

const migrations700: SavedObjectMigrationFn<DashboardDoc700To720['attributes'], DashboardDoc700To720['attributes']> = doc => {

rudolf · 2020-04-28T12:25:04Z

src/plugins/dashboard/server/saved_objects/dashboard_migrations.ts

+  '6.7.2': flow<SavedObjectMigrationFn<any, any>>(migrateMatchAllQuery),
+  '7.0.0': flow<SavedObjectMigrationFn<any, DashboardDoc700To720['attributes']>>(migrations700),
+  '7.3.0': flow<SavedObjectMigrationFn<any, any>>(migrations730),


Suggested change

'6.7.2': flow<SavedObjectMigrationFn<any, any>>(migrateMatchAllQuery),

'7.0.0': flow<SavedObjectMigrationFn<any, DashboardDoc700To720['attributes']>>(migrations700),

'7.3.0': flow<SavedObjectMigrationFn<any, any>>(migrations730),

'6.7.2': flow<typeof migrateMatchAllQuery>(migrateMatchAllQuery),

'7.0.0': flow<typeof migrations700>(migrations700),

'7.3.0': flow<SavedObjectMigrationFn<any, any>>(migrations730),

We can't follow the same pattern with migrations730 because it's not typed as a SavedObjectMigrationFn.

rudolf · 2020-04-28T12:32:19Z

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

@@ -26,7 +26,7 @@ import { SavedObjectsMigrationLogger } from './core/migration_logger';
 *
 * @example
 * ```typescript
- * const migrateProperty: SavedObjectMigrationFn = (doc, { log }) => {
+ * const migrateProperty: SavedObjectMigrationFn<MyUnmigratedAttributes, MyMigratedAttributes> = (doc, { log }) => {


we should update the example to show how to construct a copy so that the output is typed different to the input. Since this is only for type safety we don't need to make a deep copy, so it might be worth adding a comment to the example to highlight this.

rudolf · 2020-04-28T12:35:40Z

@streamich This doesn't weaken any types, it only makes it explicit that some migration function have been relying on any all along. I agree that it would be good to create a follow-up issue with a task for each of the teams to update their migrations' type safety.

streamich · 2020-04-28T15:10:33Z

I think just one issue should be enough. The reason I suggested to tag both teams is because it appears that AppArch is codeowners of some of these changes, but those migrations might be owned KibanaApp soon instead.

pgayvallet · 2020-04-29T07:30:06Z

I agree that it would be good to create a follow-up issue with a task for each of the teams to update their migrations' type safety.

I will create a follow-up issue with the list of files I had to add <any, any> in and ping the respective owners.

flash1293

Kibana app changes LGTM, thanks for improving the types! There is room for more improvement (getting rid of the anys), but those can be addressed separately - definitely out of scope for this change. I like Rudolfs suggestions.

…ion-fn-generics

pgayvallet · 2020-04-29T08:26:35Z

Created #64748

pgayvallet · 2020-04-29T14:34:55Z

retest

wylieconlon

LGTM from Kibana app side.

…ion-fn-generics

kibanamachine · 2020-04-30T08:02:13Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 4799896

History

💚 Build #44208 succeeded 9e8ebf7
💚 Build #43665 succeeded 8a03805
💔 Build #43661 failed 05912ff
💚 Build #42085 succeeded a0ad3fb
💚 Build #41796 succeeded b769c2a

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

* add generic typings for SavedObjectMigrationFn * change default attributes type to unknown * update generated doc * adapt new calls * update generated doc * update migration example * fix merge conflicts

* master: (42 commits) [Ingest] Allow aggent to send metadata compliant with ECS (elastic#64452) [Endpoint] Remove todos, urls to issues (elastic#64833) [Uptime] Remove hard coded value for monitor states histograms (elastic#64396) Feature/send feedback link (elastic#64845) [ML] Moving get filters capability to admin (elastic#64879) Remove edit alert button from alerts list (elastic#64643) [EPM] Handle constant_keyword type in KB index patterns and ES index templates (elastic#64876) [ML] Disable data frame anaylics clone button based on permission (elastic#64830) Dashboard url generator to preserve saved filters from destination dashboard (elastic#64767) add generic typings for SavedObjectMigrationFn (elastic#63943) Allow to define and update a defaultPath for applications (elastic#64498) [Event Log] add rel=primary to saved objects for query targets (elastic#64615) [Lens] Use a size of 5 for first string field in visualization (elastic#64726) [SIEM][Lists] Removes plugin dependencies, adds more unit tests, fixes more TypeScript types [Ingest] Edit datasource UI (elastic#64727) [Lens] Bind all time fields to the time picker (elastic#63874) [Lens] Use suggestion system in chart switcher for subtypes (elastic#64613) Improve alpha messaging (elastic#64692) [Ingest] Allow to enable monitoring of elastic agent (elastic#63598) [Metrics UI] Fix alerting when a filter query is present (elastic#64575) ...

add generic typings for SavedObjectMigrationFn

b769c2a

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

pgayvallet requested a review from a team as a code owner April 20, 2020 08:39

rudolf reviewed Apr 20, 2020

View reviewed changes

pgayvallet added 3 commits April 21, 2020 08:56

change default attributes type to unknown

b97d4c1

update generated doc

4374ec8

Merge remote-tracking branch 'upstream/master' into kbn-XXX-so-migrat…

2504a55

…ion-fn-generics

pgayvallet requested a review from a team April 21, 2020 07:02

pgayvallet requested review from a team as code owners April 21, 2020 07:02

pgayvallet commented Apr 21, 2020

View reviewed changes

Merge remote-tracking branch 'upstream/master' into kbn-XXX-so-migrat…

a0ad3fb

…ion-fn-generics

jportner approved these changes Apr 22, 2020

View reviewed changes

pgayvallet mentioned this pull request Apr 24, 2020

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

Closed

7 tasks

alexwizp approved these changes Apr 27, 2020

View reviewed changes

pgayvallet added 3 commits April 28, 2020 09:51

Merge remote-tracking branch 'upstream/master' into kbn-XXX-so-migrat…

2614f61

…ion-fn-generics

adapt new calls

05912ff

update generated doc

8a03805

pgayvallet commented Apr 28, 2020

View reviewed changes

streamich approved these changes Apr 28, 2020

View reviewed changes

rudolf approved these changes Apr 28, 2020

View reviewed changes

flash1293 approved these changes Apr 29, 2020

View reviewed changes

pgayvallet added 2 commits April 29, 2020 09:43

Merge remote-tracking branch 'upstream/master' into kbn-XXX-so-migrat…

fa962a7

…ion-fn-generics

update migration example

bb25186

pgayvallet mentioned this pull request Apr 29, 2020

Add attributes types in SavedObjects migration #64748

Closed

9 tasks

wylieconlon approved these changes Apr 29, 2020

View reviewed changes

pgayvallet added 3 commits April 29, 2020 21:24

Merge remote-tracking branch 'upstream/master' into kbn-XXX-so-migrat…

9e8ebf7

…ion-fn-generics

Merge remote-tracking branch 'upstream/master' into kbn-XXX-so-migrat…

cb373e9

…ion-fn-generics

fix merge conflicts

4799896

pgayvallet merged commit 6e26913 into elastic:master Apr 30, 2020

pgayvallet mentioned this pull request Apr 30, 2020

[7.x] add generic typings for SavedObjectMigrationFn (#63943) #64874

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add generic typings for SavedObjectMigrationFn #63943

add generic typings for SavedObjectMigrationFn #63943

pgayvallet commented Apr 20, 2020 •

edited

Loading

rudolf Apr 20, 2020

pgayvallet Apr 21, 2020

pgayvallet Apr 21, 2020

pgayvallet Apr 21, 2020

rudolf Apr 21, 2020

rudolf Apr 21, 2020 •

edited

Loading

pgayvallet Apr 21, 2020

jportner left a comment

rudolf commented Apr 24, 2020 •

edited

Loading

alexwizp left a comment

pgayvallet Apr 28, 2020

pgayvallet Apr 28, 2020

streamich left a comment

streamich commented Apr 28, 2020

rudolf left a comment

rudolf Apr 28, 2020

rudolf Apr 28, 2020

rudolf Apr 28, 2020

rudolf commented Apr 28, 2020

streamich commented Apr 28, 2020 •

edited

Loading

pgayvallet commented Apr 29, 2020

flash1293 left a comment

pgayvallet commented Apr 29, 2020

pgayvallet commented Apr 29, 2020

wylieconlon left a comment

kibanamachine commented Apr 30, 2020

	{
	files: ['x-pack/plugins/lens/*/.{ts,tsx}'],
	rules: {
	'@typescript-eslint/no-explicit-any': 'error',
	},
	},

		const doc700: DashboardDoc700To720 = migrations['7.0.0'](doc);
		const doc700 = migrations['7.0.0'](doc, mockContext);

	const migrations700: SavedObjectMigrationFn<any, any> = (doc): DashboardDoc700To720 => {
	const migrations700: SavedObjectMigrationFn<DashboardDoc700To720['attributes'], DashboardDoc700To720['attributes']> = doc => {

add generic typings for SavedObjectMigrationFn #63943

add generic typings for SavedObjectMigrationFn #63943

Conversation

pgayvallet commented Apr 20, 2020 • edited Loading

Summary

Checklist

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

rudolf Apr 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jportner left a comment

Choose a reason for hiding this comment

rudolf commented Apr 24, 2020 • edited Loading

alexwizp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

streamich left a comment

Choose a reason for hiding this comment

streamich commented Apr 28, 2020

rudolf left a 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

rudolf commented Apr 28, 2020

streamich commented Apr 28, 2020 • edited Loading

pgayvallet commented Apr 29, 2020

flash1293 left a comment

Choose a reason for hiding this comment

pgayvallet commented Apr 29, 2020

pgayvallet commented Apr 29, 2020

wylieconlon left a comment

Choose a reason for hiding this comment

kibanamachine commented Apr 30, 2020

💚 Build Succeeded

History

pgayvallet commented Apr 20, 2020 •

edited

Loading

rudolf Apr 21, 2020 •

edited

Loading

rudolf commented Apr 24, 2020 •

edited

Loading

streamich commented Apr 28, 2020 •

edited

Loading