add SavedObject export hooks by pgayvallet · Pull Request #87807 · elastic/kibana

pgayvallet · 2021-01-11T09:37:18Z

Summary

This PR adds per-type export transformation during the SO export generation. Type owners can now register a transformer function using the new SavedObjectType.onExport property when registering their type via registerType.

The transformers allow to perform any number of the following operations:

Mutate the exported objects associated with the transform
Add additional objects to the list of exported objects

API usage examples

Mutating some of the exported objects attributes

savedObjects.registerType({
  name: 'my-type',
  // [...]
  management: {
    importableAndExportable: true,
    onExport: (ctx, objs) => {
      return objs.map((obj) => ({
        ...obj,
        attributes: {
          ...obj.attributes,
          enabled: false,
        },
      }));
    },
  },
});

Adding additional objects to the export

const savedObjectStartContractPromise = getStartServices().then(
    ([{ savedObjects: savedObjectsStart }]) => savedObjectsStart
);

savedObjects.registerType({
  name: 'my-type',
  // [...]
  management: {
    importableAndExportable: true,
    onExport: async (ctx, objs) => {
      const { getScopedClient } = await savedObjectStartContractPromise;
      const client = getScopedClient(ctx.request);
      const objRefs = objs.map(({ id, type }) => ({ id, type }));
      const depResponse = await client.find({
        type: 'my-dep-type',
        hasReference: objRefs,
      });
      return [...objs, ...depResponse.saved_objects];
    },
  },
});

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

pgayvallet

This is a POC of the proposed implementation of #84980,

@joshdover @kobelb @rudolf and overall @elastic/kibana-core: opinions and feedback would be appreciated before I go further.

pgayvallet · 2021-01-11T13:25:11Z

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

+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;
+};


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:

update exported SOs and return updated versions

add additional objects to the export

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?

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 SavedObjectsExportTransform and registerExportTransform?

Note that is also allows to filter / exclude objects from the export. This is probably something we do not want

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?

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 SavedObjectsExportTransform and registerExportTransform?

+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.

What do you think about calling this SavedObjectsExportTransform

I agree. Will rename to transform instead

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?

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

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

pgayvallet · 2021-01-11T13:28:04Z

src/core/server/saved_objects/saved_objects_service.ts

   * ```
   */
  registerType: (type: SavedObjectsType) => void;
+
+  /**
+   * TODO: documentation
+   */
+  registerExportHook: (type: string, exportHook: SavedObjectsTypeExportHook) => void;


As discussed in #84980 (comment), The hooks are registered via the setup contract, but using a distinct API instead of being included in the type when registering them via registerType. See the comment on #84980 for the (still opened to discussion) reasoning behind that decision.

Note: after discussion starting at #84980 (comment), registerExportHook will be removed in favor of registering the transform via SavedObjectType.onExport

pgayvallet · 2021-01-11T13:29:51Z

src/core/server/saved_objects/saved_objects_service.ts

+      registerExportHook: (type, hook) => {
+        if (this.started) {
+          throw new Error('cannot call `registerExportHook` after service startup.');
+        }
+        // TODO
+        this.exportHooks.set(type, hook);
+      },


As registerExportHook is dissociated from registerType, it is theoretically possible to register multiple hooks for the same type. However this would create a lot of complexity (order of execution, recursivity...), so I think we should just forbid that and throw in that case?

We should probably validate that the type has management.importableAndExportable: true if it specifies an export hook.

kobelb

Just one nit, but besides that, I like it 🥇

kobelb · 2021-01-13T01:42:51Z

src/core/server/legacy/legacy_service.ts

        setClientFactoryProvider: setupDeps.core.savedObjects.setClientFactoryProvider,
        addClientWrapper: setupDeps.core.savedObjects.addClientWrapper,
        registerType: setupDeps.core.savedObjects.registerType,
+        registerExportHook: setupDeps.core.savedObjects.registerExportHook,


nit: instead of having a dedicated registerExportHook, should we make it part of the existing SavedObjectsTypeManagementDefinition interface since this already has importableAndExportable?

Yea, we came with the same conclusion with @rudolf in #84980 (comment). I will adapt that, as I did in #87996

rudolf · 2021-01-13T16:12:16Z

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

    let exportedObjects: Array<SavedObject<unknown>>;
    let missingReferences: SavedObjectsExportResultDetails['missingReferences'] = [];

+    savedObjects = await applyExportHooks({


applyExportHooks will change the sorting order. The new sorting order will be consistent, but it would be nice if we could keep the sorting order the same as previous versions because of #29747.

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.

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 type:id list of the exported objects before calling the hooks, and then try to reorder the objects based on their position on this type:id list.

objects mutated or untouched would preserve their position

objects added would be appended at the end of the exported list

Does that sound alright?

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 hidden: false objects to also adopt export hooks, but probably unlikely). However, plugins implementing an exportHook might not know/forget to use a stable sort order. So I think it's best if we:

keep the current sort in fetchByTypes

also sort all objects returned by an export hook (it doesn't really matter which stable sort we use, we could just use id to be consistent, in 8.0.0 it would be nice to rather sort by a new created_at field so that new objects are always at the end of the list, but that would require resorting all objects after export hooks were applied).

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

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?

pgayvallet · 2021-01-14T14:17:39Z

Thanks guys for the feedback. As everyone seems alright with the overall implementation, I will continue in this direction.

…rt-hooks

pgayvallet

Self review

pgayvallet · 2021-01-18T13:27:22Z

src/core/public/public.api.md

 import { ConfigPath } from '@kbn/config';
+import { DetailedPeerCertificate } from 'tls';


Even with the changes I did in src/core/server/types.ts to remove some exports that were only server-side, it seems that we are leaking server-side types to the public definition. This seems caused by the addition of request: KibanaRequest to SavedObjectExportBaseOptions, however I don't see this type exported from server/types or imported anywhere from server/index... If someone got better eyes that I do...

pgayvallet · 2021-01-18T13:31:02Z

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

+const assertValidTransform = (transformedObjects: SavedObject[], initialKeys: string[]) => {
+  const transformedKeys = transformedObjects.map(getObjKey);
+  const missingKeys: string[] = [];
+  initialKeys.forEach((initialKey) => {
+    if (!transformedKeys.includes(initialKey)) {
+      missingKeys.push(initialKey);
+    }
+  });
+  if (missingKeys.length) {
+    throw SavedObjectsExportError.invalidTransformError(missingKeys);
+  }
+};


I did not check that objects added to the export are all importableAndExportable: true. With current implementation you can export 'non-exportable' objects, and then when trying to import them back, it will fail because the types are not importable. However this highly depends on #82027, and I think this should be addressed at the same time.

Should I still add importableAndExportable check during assertValidTransform in this PR?

+1 For leaving out the validation since we will anyway support importing these objects in the same release.

pgayvallet · 2021-01-18T13:45:12Z

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

+export const getPreservedOrderComparator = (objects: SavedObject[]): SavedObjectComparator => {
+  const orderedKeys = objects.map(getObjKey);
+  return (a: SavedObject, b: SavedObject) => {
+    const indexA = orderedKeys.indexOf(getObjKey(a));


As discussed in #87807 (comment), the export hooks can alter the order of the exported objects, even when no transform are registered for the exported types. For exportByType, the objects were ordered by ID, so we are just re-doing that after executing the transformations. For exportById, the order was the same as the list of the objects to export. This is what this getPreservedOrderComparator is used for.

pgayvallet · 2021-01-18T13:47:22Z

src/core/server/types.ts

-export * from './saved_objects/types';
+export type {
+  SavedObjectsImportResponse,
+  SavedObjectsImportSuccess,
+  SavedObjectsImportConflictError,


We were exporting more than necessary to the client-side (such as SavedObjectsType or SavedObjectsTypeManagementDefinition), so I switched to explicit export instead of *

pgayvallet · 2021-01-18T13:48:59Z

test/plugin_functional/plugins/saved_object_export_hooks/server/plugin.ts

+        onExport: async (ctx, objs) => {
+          const { getScopedClient } = await savedObjectStartContractPromise;
+          const client = getScopedClient(ctx.request);
+          const objRefs = objs.map(({ id, type }) => ({ id, type }));
+          const depResponse = await client.find({
+            type: 'test-export-add-dep',
+            hasReference: objRefs,
+          });
+          return [...objs, ...depResponse.saved_objects];
+        },


This (naive) test example should be very close to what we will need to do with case and case-comment

elasticmachine · 2021-01-18T15:33:00Z

Pinging @elastic/kibana-core (Team:Core)

rudolf

Did a quick pass and this was the only thing that stood out to me

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

…rt-hooks

test/plugin_functional/test_suites/saved_objects_management/export_transform.ts

rudolf

I think it's worth adding an integration test for transform failures, but otherwise 👍

docs/development/core/server/kibana-plugin-core-server.savedobjectsexporttransform.md

src/core/server/saved_objects/types.ts

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

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

jportner

Spaces changes LGTM.

Did not test locally but I took a high level pass on the Core changes too, nothing stood out to me there.

…rt-hooks

kibanamachine · 2021-01-21T13:33:37Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 912da12

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`core`	447	445	-2

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`core`	563.8KB	561.6KB	-2.3KB

History

💚 Build #100265 succeeded a2c380a
💚 Build #100248 succeeded 9ac648f
💚 Build #99872 succeeded 9cee831
💚 Build #99409 succeeded 417aff9
💚 Build #99402 succeeded 3887dbd

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

* initial POC * fix spaces UT * address POC feedback, add tests for applyExportTransforms * add sorting for transforms * add type validation in SOTR * add FTR tests * update documentation * add explicit so type export for client-side * update generated doc * add exporter test * update license headers * update generated doc * fix so import... imports * update generated doc * nits * update generated doc * rename test plugins * adding FTR tests on export failures

* add SavedObject export hooks (#87807) * initial POC * fix spaces UT * address POC feedback, add tests for applyExportTransforms * add sorting for transforms * add type validation in SOTR * add FTR tests * update documentation * add explicit so type export for client-side * update generated doc * add exporter test * update license headers * update generated doc * fix so import... imports * update generated doc * nits * update generated doc * rename test plugins * adding FTR tests on export failures * fix data for 7.x

pgayvallet added 2 commits January 11, 2021 10:36

initial POC

d9de67f

fix spaces UT

53996ee

pgayvallet commented Jan 11, 2021

View reviewed changes

pgayvallet mentioned this pull request Jan 11, 2021

Saved-object export custom logic #84980

Closed

pgayvallet added Feature:Saved Objects release_note:skip Skip the PR/issue when compiling release notes Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// v7.12.0 v8.0.0 labels Jan 12, 2021

kobelb reviewed Jan 13, 2021

View reviewed changes

rudolf reviewed Jan 13, 2021

View reviewed changes

pgayvallet added 10 commits January 15, 2021 09:47

Merge remote-tracking branch 'upstream/master' into kbn-87438-so-expo…

a3702f6

…rt-hooks

address POC feedback, add tests for applyExportTransforms

af9523f

add sorting for transforms

0ba9aea

add type validation in SOTR

067c3f7

add FTR tests

cd4ac17

Merge remote-tracking branch 'upstream/master' into kbn-87438-so-expo…

12871c4

…rt-hooks

update documentation

f36534f

add explicit so type export for client-side

df1eb4d

update generated doc

3887dbd

add exporter test

417aff9

pgayvallet commented Jan 18, 2021

View reviewed changes

pgayvallet marked this pull request as ready for review January 18, 2021 15:32

pgayvallet requested review from a team as code owners January 18, 2021 15:32

pgayvallet requested review from joshdover and rudolf January 18, 2021 15:35

botelastic bot added the Feature:Embedding Embedding content via iFrame label Jan 19, 2021

rudolf reviewed Jan 19, 2021

View reviewed changes

src/core/server/saved_objects/export/apply_export_transforms.ts Show resolved Hide resolved

pgayvallet added 3 commits January 20, 2021 07:56

Merge remote-tracking branch 'upstream/master' into kbn-87438-so-expo…

ed2fc4b

…rt-hooks

update license headers

5434e0f

update generated doc

9cee831

rudolf reviewed Jan 20, 2021

View reviewed changes

test/plugin_functional/test_suites/saved_objects_management/export_transform.ts Show resolved Hide resolved

rudolf approved these changes Jan 20, 2021

View reviewed changes

joshdover reviewed Jan 20, 2021

View reviewed changes

joshdover approved these changes Jan 20, 2021

View reviewed changes

jportner approved these changes Jan 20, 2021

View reviewed changes

pgayvallet added 7 commits January 21, 2021 09:24

Merge remote-tracking branch 'upstream/master' into kbn-87438-so-expo…

c1f49a2

…rt-hooks

fix so import... imports

4f292e9

update generated doc

9ac648f

nits

7c6ee4a

update generated doc

a2c380a

rename test plugins

1a33426

adding FTR tests on export failures

912da12

pgayvallet merged commit 477d0bb into elastic:master Jan 21, 2021

pgayvallet mentioned this pull request Jan 21, 2021

[7.x] add SavedObject export hooks (#87807) #88973

Merged

joshdover mentioned this pull request Feb 4, 2021

[Saved Objects] Support type changing and document splitting #87900

Closed

rudolf mentioned this pull request Feb 15, 2021

switch from esArchiver to saved objects API for .kibana data in functional tests #89368

Closed

This was referenced Feb 22, 2021

Import and export Cases using saved-object management #82533

Closed

Import and export Detection rules using saved-object management #82537

Closed

Import and export Timelines using saved-object management #82526

Closed

		import { ConfigPath } from '@kbn/config';
		import { DetailedPeerCertificate } from 'tls';

Conversation

pgayvallet commented Jan 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

API usage examples

Checklist

Uh oh!

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

pgayvallet Jan 15, 2021 • 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!

Uh oh!

pgayvallet Jan 11, 2021 • 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!

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

Choose a reason for hiding this comment

Uh oh!

pgayvallet Jan 13, 2021 • 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!

rudolf Jan 14, 2021 • 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!

pgayvallet Jan 14, 2021 • 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!

pgayvallet commented Jan 14, 2021

Uh oh!

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

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!

elasticmachine commented Jan 18, 2021

Uh oh!

rudolf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rudolf left a comment

Choose a reason for hiding this comment

pgayvallet commented Jan 11, 2021 •

edited

Loading

pgayvallet Jan 15, 2021 •

edited

Loading

pgayvallet Jan 11, 2021 •

edited

Loading

pgayvallet Jan 13, 2021 •

edited

Loading

rudolf Jan 14, 2021 •

edited

Loading

pgayvallet Jan 14, 2021 •

edited

Loading