-
Notifications
You must be signed in to change notification settings - Fork 8.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add generic typings for SavedObjectMigrationFn #63943
Changes from all commits
b769c2a
b97d4c1
4374ec8
2504a55
a0ad3fb
2614f61
05912ff
8a03805
fa962a7
bb25186
9e8ebf7
cb373e9
4799896
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,43 @@ | ||
/* | ||
* Licensed to Elasticsearch B.V. under one or more contributor | ||
* license agreements. See the NOTICE file distributed with | ||
* this work for additional information regarding copyright | ||
* ownership. Elasticsearch B.V. licenses this file to you under | ||
* the Apache License, Version 2.0 (the "License"); you may | ||
* not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, | ||
* software distributed under the License is distributed on an | ||
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
* KIND, either express or implied. See the License for the | ||
* specific language governing permissions and limitations | ||
* under the License. | ||
*/ | ||
|
||
import { SavedObjectMigrationContext } from './types'; | ||
import { SavedObjectsMigrationLogger } from './core'; | ||
|
||
const createLoggerMock = (): jest.Mocked<SavedObjectsMigrationLogger> => { | ||
const mock = { | ||
debug: jest.fn(), | ||
info: jest.fn(), | ||
warning: jest.fn(), | ||
warn: jest.fn(), | ||
}; | ||
|
||
return mock; | ||
}; | ||
|
||
const createContextMock = (): jest.Mocked<SavedObjectMigrationContext> => { | ||
const mock = { | ||
log: createLoggerMock(), | ||
}; | ||
return mock; | ||
}; | ||
|
||
export const migrationMocks = { | ||
createContext: createContextMock, | ||
}; |
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -19,7 +19,7 @@ | |||||||||||||
|
||||||||||||||
import { get, flow } from 'lodash'; | ||||||||||||||
|
||||||||||||||
import { SavedObjectMigrationFn, SavedObjectUnsanitizedDoc } from 'kibana/server'; | ||||||||||||||
import { SavedObjectMigrationFn } from 'kibana/server'; | ||||||||||||||
import { migrations730 } from './migrations_730'; | ||||||||||||||
import { migrateMatchAllQuery } from './migrate_match_all_query'; | ||||||||||||||
import { DashboardDoc700To720 } from '../../common'; | ||||||||||||||
|
@@ -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 => { | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||
// Set new "references" attribute | ||||||||||||||
doc.references = doc.references || []; | ||||||||||||||
|
||||||||||||||
|
@@ -111,7 +111,7 @@ export const dashboardSavedObjectTypeMigrations = { | |||||||||||||
* in that version. So we apply this twice, once with 6.7.2 and once with 7.0.1 while the backport to 6.7 | ||||||||||||||
* only contained the 6.7.2 migration and not the 7.0.1 migration. | ||||||||||||||
*/ | ||||||||||||||
'6.7.2': flow<SavedObjectMigrationFn>(migrateMatchAllQuery), | ||||||||||||||
'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), | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @elastic/kibana-app |
||||||||||||||
'7.3.0': flow<SavedObjectMigrationFn<any, any>>(migrations730), | ||||||||||||||
Comment on lines
+114
to
+116
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
We can't follow the same pattern with |
||||||||||||||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Meaning that
Would fail to compile with something like
property disabledFeatures does not exists on type InputAttrs
on thedoc.attributes.disabledFeatures = [];
line.Which is why I'm wondering if we should add the output properties on the input
doc
:Ideally I would have been using Partial,
doc: SavedObjectUnsanitizedDoc<InputAttributes & Partial<MigratedAttributes>>
, however the given example still fails withdisabledFeatures 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?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a slightly more type-safe solution than
any
, teams can still useInputAttributes & MigratedAttributes
as theirInputAttributes
generic paramater.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGTM, I'll keep the definition as it is right now then.