add SavedObject export hooks #87807
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,63 @@ | ||
| /* | ||
| * 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 { SavedObject } from '../../../types'; | ||
| import { KibanaRequest } from '../../http'; | ||
| import { SavedObjectsTypeExportHook, SavedObjectsExportContext } from './types'; | ||
|
|
||
| interface ApplyExportHooksOptions { | ||
| objects: SavedObject[]; | ||
| request: KibanaRequest; | ||
| exportHooks: Record<string, SavedObjectsTypeExportHook>; | ||
| } | ||
|
|
||
| // TODO: doc + add tests. | ||
| export const applyExportHooks = async ({ | ||
| objects, | ||
| request, | ||
| exportHooks, | ||
| }: ApplyExportHooksOptions): Promise<SavedObject[]> => { | ||
| const context = createContext(request); | ||
| const byType = splitByType(objects); | ||
|
|
||
| let finalObjects: SavedObject[] = []; | ||
| for (const [type, typeObjs] of Object.entries(byType)) { | ||
| const typeHook = exportHooks[type]; | ||
| if (typeHook) { | ||
| finalObjects = [...finalObjects, ...(await typeHook(typeObjs, context))]; | ||
| } else { | ||
| finalObjects = [...finalObjects, ...typeObjs]; | ||
| } | ||
| } | ||
|
|
||
| return finalObjects; | ||
| }; | ||
|
There was a problem hiding this comment. This is the core of the feature The hook is defined by export type SavedObjectsTypeExportHook = <T = unknown>(
objects: Array<SavedObject<T>>,
context: SavedObjectsExportContext
) => SavedObject[] | Promise<SavedObject[]>;Which allows to both:
Note that is also allows to filter / exclude objects from the export. This is probably something we do not want, but it seems still alright, and limiting that with a more structured API would need a better structure than just returning a list of SO from the hook. Does that look alright? There was a problem hiding this comment. When I think of a "hook" I think of a callback that gets called for every model/object, like a "pre-save hook". What do you think about calling this There was a problem hiding this comment.
I do like the simplicity of this API shape. Instead of solving the filtering caveat with the shape of the return type, any reason we shouldn't prevent accidental filtering at runtime by verifying that all object IDs that were passed in were also in the array that was returned?
+1 on not naming this concept "hook". We've had requests in the past for on-save and on-delete hooks and I worry this would confuse developers. There was a problem hiding this comment.
I agree. Will rename to
Yea, we can do that. This will cause the export to fail, but I guess this would be detected during development time and is still better than doing nothing |
||
|
|
||
| const createContext = (request: KibanaRequest): SavedObjectsExportContext => { | ||
| return { | ||
| request, | ||
| }; | ||
| }; | ||
|
|
||
| const splitByType = (objects: SavedObject[]): Record<string, SavedObject[]> => { | ||
| return objects.reduce((memo, obj) => { | ||
| memo[obj.type] = [...(memo[obj.type] ?? []), obj]; | ||
| return memo; | ||
| }, {} as Record<string, SavedObject[]>); | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,8 +27,10 @@ import { | |
| SavedObjectExportBaseOptions, | ||
| SavedObjectsExportByObjectOptions, | ||
| SavedObjectsExportByTypeOptions, | ||
| SavedObjectsTypeExportHook, | ||
| } from './types'; | ||
| import { SavedObjectsExportError } from './errors'; | ||
| import { applyExportHooks } from './apply_export_hooks'; | ||
|
|
||
| /** | ||
| * @public | ||
|
|
@@ -40,16 +42,20 @@ export type ISavedObjectsExporter = PublicMethodsOf<SavedObjectsExporter>; | |
| */ | ||
| export class SavedObjectsExporter { | ||
| readonly #savedObjectsClient: SavedObjectsClientContract; | ||
| readonly #exportHooks: Record<string, SavedObjectsTypeExportHook>; | ||
| readonly #exportSizeLimit: number; | ||
|
|
||
| constructor({ | ||
| savedObjectsClient, | ||
| exportHooks, | ||
| exportSizeLimit, | ||
| }: { | ||
| savedObjectsClient: SavedObjectsClientContract; | ||
| exportHooks: Record<string, SavedObjectsTypeExportHook>; | ||
| exportSizeLimit: number; | ||
| }) { | ||
| this.#savedObjectsClient = savedObjectsClient; | ||
| this.#exportHooks = exportHooks; | ||
| this.#exportSizeLimit = exportSizeLimit; | ||
| } | ||
|
|
||
|
|
@@ -63,6 +69,7 @@ export class SavedObjectsExporter { | |
| public async exportByTypes(options: SavedObjectsExportByTypeOptions) { | ||
| const objects = await this.fetchByTypes(options); | ||
| return this.processObjects(objects, { | ||
| request: options.request, | ||
| includeReferencesDeep: options.includeReferencesDeep, | ||
| excludeExportDetails: options.excludeExportDetails, | ||
| namespace: options.namespace, | ||
|
|
@@ -82,6 +89,7 @@ export class SavedObjectsExporter { | |
| } | ||
| const objects = await this.fetchByObjects(options); | ||
| return this.processObjects(objects, { | ||
| request: options.request, | ||
| includeReferencesDeep: options.includeReferencesDeep, | ||
| excludeExportDetails: options.excludeExportDetails, | ||
| namespace: options.namespace, | ||
|
|
@@ -91,6 +99,7 @@ export class SavedObjectsExporter { | |
| private async processObjects( | ||
| savedObjects: SavedObject[], | ||
| { | ||
| request, | ||
| excludeExportDetails = false, | ||
| includeReferencesDeep = false, | ||
| namespace, | ||
|
|
@@ -99,6 +108,12 @@ export class SavedObjectsExporter { | |
| let exportedObjects: Array<SavedObject<unknown>>; | ||
| let missingReferences: SavedObjectsExportResultDetails['missingReferences'] = []; | ||
|
|
||
| savedObjects = await applyExportHooks({ | ||
|
There was a problem hiding this comment.
There was a problem hiding this comment. Maybe you could also add a comment to the code, because it's not so obvious from the existing code why we did this sort in the first place. There was a problem hiding this comment. Would be great, however as the hooks mutate subsets of the exported objects, I'm not really sure how to do this. I guess we could create a
Does that sound alright? There was a problem hiding this comment. I just realized that types using the export hooks would not have ever been exported before, so for version control it won't really matter (it's not impossible for
There was a problem hiding this comment.
The hooks handling logic still changes the position of types not having registered hooks, as it needs to split the exported objects per type and then reconstruct a result array (see the implementation https://github.com/elastic/kibana/pull/87807/files#diff-d6563061094fb926296a7960b2c76ec062d04e63e22646fb48975ab03dde6fe5). So we still need to find a way to reorder all the objects if we want to be compliant with #29747 I think? |
||
| objects: savedObjects, | ||
| request, | ||
| exportHooks: this.#exportHooks, | ||
| }); | ||
|
|
||
| if (includeReferencesDeep) { | ||
| const fetchResult = await fetchNestedDependencies( | ||
| savedObjects, | ||
|
|
||
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.
nit: instead of having a dedicated
registerExportHook, should we make it part of the existingSavedObjectsTypeManagementDefinitioninterface since this already hasimportableAndExportable?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.
Yea, we came with the same conclusion with @rudolf in #84980 (comment). I will adapt that, as I did in #87996