-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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 8 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 |
---|---|---|
|
@@ -26,7 +26,7 @@ import { SavedObjectsMigrationLogger } from './core/migration_logger'; | |
* | ||
* @example | ||
* ```typescript | ||
* const migrateProperty: SavedObjectMigrationFn = (doc, { log }) => { | ||
* const migrateProperty: SavedObjectMigrationFn<MyUnmigratedAttributes, MyMigratedAttributes> = (doc, { log }) => { | ||
* if(doc.attributes.someProp === null) { | ||
* log.warn('Skipping migration'); | ||
* } else { | ||
|
@@ -39,10 +39,10 @@ import { SavedObjectsMigrationLogger } from './core/migration_logger'; | |
* | ||
* @public | ||
*/ | ||
export type SavedObjectMigrationFn = ( | ||
doc: SavedObjectUnsanitizedDoc, | ||
export type SavedObjectMigrationFn<InputAttributes = unknown, MigratedAttributes = unknown> = ( | ||
doc: SavedObjectUnsanitizedDoc<InputAttributes>, | ||
context: SavedObjectMigrationContext | ||
) => SavedObjectUnsanitizedDoc; | ||
) => SavedObjectUnsanitizedDoc<MigratedAttributes>; | ||
Comment on lines
+56
to
+59
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. So, my only interrogation around that is that atm migrations are all mutating and returning the input 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 Which is why I'm wondering if we should add the output properties on the input export type SavedObjectMigrationFn<InputAttributes = unknown, MigratedAttributes = unknown> = (
doc: SavedObjectUnsanitizedDoc<InputAttributes & MigratedAttributes>,
context: SavedObjectMigrationContext
) => SavedObjectUnsanitizedDoc<MigratedAttributes>; Ideally I would have been using Partial, The other option being to decide that migration function should properly spread / construct the resulting migrated object without returning directly the input 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.
Given the high impact of even a small We can leave existing migrations with 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. As a slightly more type-safe solution than 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. SGTM, I'll keep the definition as it is right now then. |
||
|
||
/** | ||
* Migration context provided when invoking a {@link SavedObjectMigrationFn | migration handler} | ||
|
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 |
||||||||||||||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,19 +17,13 @@ | |
* under the License. | ||
*/ | ||
|
||
import { savedObjectsServiceMock } from '../../../../core/server/mocks'; | ||
import { dashboardSavedObjectTypeMigrations as migrations } from './dashboard_migrations'; | ||
import { migrations730 } from './migrations_730'; | ||
import { DashboardDoc700To720, DashboardDoc730ToLatest, DashboardDocPre700 } from '../../common'; | ||
import { RawSavedDashboardPanel730ToLatest } from '../../common'; | ||
|
||
const mockContext = { | ||
log: { | ||
warning: () => {}, | ||
warn: () => {}, | ||
debug: () => {}, | ||
info: () => {}, | ||
}, | ||
}; | ||
const mockContext = savedObjectsServiceMock.createMigrationContext(); | ||
|
||
test('dashboard migration 7.3.0 migrates filters to query on search source', () => { | ||
const doc: DashboardDoc700To720 = { | ||
|
@@ -95,7 +89,7 @@ test('dashboard migration 7.3.0 migrates filters to query on search source when | |
}, | ||
}; | ||
|
||
const doc700: DashboardDoc700To720 = migrations['7.0.0'](doc); | ||
const doc700 = migrations['7.0.0'](doc, mockContext); | ||
Comment on lines
-98
to
+92
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. This migration is now an explicit |
||
const newDoc = migrations['7.3.0'](doc700, mockContext); | ||
|
||
const parsedSearchSource = JSON.parse(newDoc.attributes.kibanaSavedObjectMeta.searchSourceJSON); | ||
|
@@ -127,7 +121,7 @@ test('dashboard migration works when panelsJSON is missing panelIndex', () => { | |
}, | ||
}; | ||
|
||
const doc700: DashboardDoc700To720 = migrations['7.0.0'](doc); | ||
const doc700 = migrations['7.0.0'](doc, mockContext); | ||
const newDoc = migrations['7.3.0'](doc700, mockContext); | ||
|
||
const parsedSearchSource = JSON.parse(newDoc.attributes.kibanaSavedObjectMeta.searchSourceJSON); | ||
|
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.
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.