Decouple saved object management from SavedObjectLoader

[flash1293]

In saved object management the "edit saved object" form as well as the legacy import are still tied by the saved object loader respectively the saved object class from the saved_objects plugin.

To be able to get rid of legacy structures, the saved object management API should be decoupled to make it possible to incrementally migrate away from the saved object class.

Currently the registry looks like this:

kibana/src/plugins/saved_objects_management/public/services/service_registry.ts

Lines 22 to 26 in 358d139

	export interface SavedObjectsManagementServiceRegistryEntry {
	id: string;
	service: SavedObjectLoader;
	title: string;
	}

A decoupled API could look like this

export interface SavedObjectsManagementServiceRegistryEntry {
  id: string;
  fields: Array<{ name: string; type: 'text' | 'number' | 'boolean' | 'json'; defaultValue?: unknown; }>;
  importDocument: (doc: SavedObjectsRawDoc, id: string; migrationVersion: string) => Promise<void>;
  title: string;
}

This requires some changes in the legacy import and in the way the fields are put together for the form:

kibana/src/plugins/saved_objects_management/public/lib/resolve_saved_objects.ts

Line 33 in bf04235

async function getSavedObject(doc: SavedObjectsRawDoc, services: SavedObjectLoader[]) {

kibana/src/plugins/saved_objects_management/public/lib/resolve_saved_objects.ts

Line 133 in bf04235

await obj.applyESResp({

kibana/src/plugins/saved_objects_management/public/lib/create_field_list.ts

Line 92 in bf04235

const addFieldsFromClass = function (

To keep the existing implementations till they can get refactored accordingly, a shim function should be introduced as well to turn a saved object loader into the required format:

function savedObjectLoaderToSOMEntry(savedObjectLoader: SavedObjectLoader): SavedObjectsManagementServiceRegistryEntry;

[elasticmachine]

Pinging @elastic/kibana-platform (Team:Platform)

[pgayvallet]

So, as @flash1293 already described in the issue, we are mostly using the SO loaders in SOM for two things:

We are currently using the SO loaders in SOM for two things:

1. Loading SOs for the legacy import

kibana/src/plugins/saved_objects_management/public/lib/resolve_saved_objects.ts

Lines 268 to 274 in 9450248

	export async function resolveSavedObjects(
	savedObjects: SavedObjectsRawDoc[],
	overwriteAll: boolean,
	services: SavedObjectLoader[],
	indexPatterns: IndexPatternsContract,
	confirmModalPromise: OverlayStart['openConfirm']
	) {

kibana/src/plugins/saved_objects_management/public/lib/resolve_saved_objects.ts

Lines 35 to 45 in 9450248

	async function getSavedObject(doc: SavedObjectsRawDoc, services: SavedObjectLoader[]) {
	const service = services.find((s) => s.type === doc._type);
	if (!service) {
	return;
	}
	const obj = await service.get();
	obj.id = doc._id;
	obj.migrationVersion = doc._migrationVersion;
	return obj;
	}

I think these usages could be easily replaced by generic APIs provided by the client side savedObjects core service.

2. Reading the mappings of the types to generate the fields to display in the edition view

kibana/src/plugins/saved_objects_management/public/lib/create_field_list.ts

Lines 41 to 43 in 6c62c68

	if (service && (service as any).Class) {
	addFieldsFromClass((service as any).Class, fields);
	}

This one is slightly more complicated to remove or adapt, and we have multiple options here:

1. Use a new API for the client-side mappings

As @flash1293 suggested, we could have something similar to

export interface SavedObjectsManagementServiceRegistryEntry {
  id: string;
  fields: Array<{ name: string; type: 'text' | 'number' | 'boolean' | 'json'; defaultValue?: unknown; }>;
  importDocument: (doc: SavedObjectsRawDoc, id: string; migrationVersion: string) => Promise<void>;
  title: string;
}

Note that we are currently only using the SO loaders provided / exposed by type owners

kibana/src/plugins/saved_objects_management/public/register_services.ts

Lines 30 to 44 in 15c0644

	if (dashboard) {
	registry.register({
	id: 'savedDashboards',
	title: 'dashboards',
	service: dashboard.getSavedDashboardLoader(),
	});
	}
	if (visualizations) {
	registry.register({
	id: 'savedVisualizations',
	title: 'visualizations',
	service: visualizations.savedVisualizationsLoader,
	});
	}

so doing so would require that this API is already in place when we start addressing this issue

As a side-note, I don't think we'll need this importDocument API. We could just use the SO service instead for that. Or is there any specific logic that would require to use type-specific handlers to import a given document @flash1293?

2. Change the current edition view with a plain JSON editor

We could replace the current view with a 'plain' JSON editor. we would just have two fields

• references
• attributes

This have some notable upsides:

• This simplifies the SOM edition page implementation a lot
• All types would be inspectable / editable out of the box

The most notable con is that, even if it was already quite easy to break stuff by editing saved objects from this page, it would be even more, as we wouldn't even be validating the allowed fields in the attributes field, or their type (it's easy to type number: "12" instead of number: 12 for example.

I still think this would be a good alternative to what we currently have. @timroes @joshdover wdyt?

[flash1293]

As a side-note, I don't think we'll need this importDocument API. We could just use the SO service instead for that. Or is there any specific logic that would require to use type-specific handlers to import a given document?

I haven't looked at this code in a while but I think the way legacy jsons are structured it does require custom logic to unwrap them and turn them into "real" saved objects. Does someone have legacy JSONs to test? :) Edit: Actually, I found some I think you are right - we can have a generic helper to migrate the structure.

The most notable con is that, even if it was already quite easy to break stuff by editing saved objects from this page, it would be even more, as we wouldn't even be validating the allowed fields in the attributes field, or their type (it's easy to type number: "12" instead of number: 12 for example.

Another IMHO large con I want to bring up is that in a lot of places we are storing serialized JSON as a string field. If this page would become a regular JSON editor, the user would have to deal with JSON like this : myAttribute: "{\"a\":{\"b\": \"thevalue\"}}" - super hard to edit manually.

What about implementing the service to register editable fields, but just falling back to a full json editor? Seems like this would give us the best of both worlds.

[pgayvallet]

Does someone have legacy JSONs to test

From a quick look, legacy import/export is just working with raw SOs. I guess accessing the attributes given the type should be doable.

kibana/src/plugins/legacy_export/server/lib/export/collect_references_deep.ts

Lines 29 to 52 in 36b66a8

	export async function collectReferencesDeep(
	savedObjectClient: SavedObjectsClientContract,
	objects: ObjectsToCollect[]
	): Promise<SavedObject[]> {
	let result: SavedObject[] = [];
	const queue = [...objects];
	while (queue.length !== 0) {
	const itemsToGet = queue.splice(0, MAX_BULK_GET_SIZE);
	const { saved_objects: savedObjects } = await savedObjectClient.bulkGet(itemsToGet);
	result = result.concat(savedObjects);
	for (const { references = [] } of savedObjects) {
	for (const reference of references) {
	const isDuplicate = queue
	.concat(result)
	.some((obj) => obj.type === reference.type && obj.id === reference.id);
	if (isDuplicate) {
	continue;
	}
	queue.push({ type: reference.type, id: reference.id });
	}
	}
	}
	return result;
	}

kibana/src/plugins/legacy_export/server/lib/import/import_dashboards.ts

Lines 22 to 39 in 68dc4e8

	export async function importDashboards(
	savedObjectsClient: SavedObjectsClientContract,
	objects: SavedObject[],
	{ overwrite, exclude }: { overwrite: boolean; exclude: string[] }
	) {
	// The server assumes that documents with no migrationVersion are up to date.
	// That assumption enables Kibana and other API consumers to not have to build
	// up migrationVersion prior to creating new objects. But it means that imports
	// need to set migrationVersion to something other than undefined, so that imported
	// docs are not seen as automatically up-to-date.
	const docs = objects
	.filter((item) => !exclude.includes(item.type))
	// filter out any document version, if present
	.map(({ version, ...doc }) => ({ ...doc, migrationVersion: doc.migrationVersion || {} }));
	const results = await savedObjectsClient.bulkCreate(docs, { overwrite });
	return { objects: results.saved_objects };
	}

we can have a generic helper to migrate the structure.

Would you mind sharing where?

Another IMHO large con I want to bring up is that in a lot of places we are storing serialized JSON as a string field. If this page would become a regular JSON editor, the user would have to deal with JSON like this : myAttribute: "{"a":{"b": "thevalue"}}" - super hard to edit manually.

That's a very valid point we overlooked with @timroes during our sync discussion yesteday

What about implementing the service to register editable fields, but just falling back to a full json editor? Seems like this would give us the best of both worlds.

That could be an option. It would also allow us to change SOM without the requirement that all SO loader providers migrate to the new API before or at the same time.

As a side note: currently, the dependency between SOM and the loader providers is reversed, as the are registering the SO 'services' from SOM itself:

kibana/src/plugins/saved_objects_management/kibana.json

Line 7 in d4b2a51

"optionalPlugins": ["dashboard", "visualizations", "discover", "home", "savedObjectsTaggingOss"],

If we are going to clean SOM from loaders, I feel that now will be a good time to also return to dependency to its correct direction (e.g dashboard -> soManagement instead of the current opposite). I suspect we might encounter cyclic dependencies by doing so though.

[flash1293]

Would you mind sharing where?

Not sure whether you are asking for the place for this helper to live or the legacy exports.
The exports are here: https://gist.github.com/flash1293/0ea158f94dd165647c560d1b90a5ed72
The helper basically being a simple pure function I would expect to be part of the saved object management plugin.

If we are going to clean SOM from loaders, I feel that now will be a good time to also return to dependency to its correct direction (e.g dashboard -> soManagement instead of the current opposite). I suspect we might encounter cyclic dependencies by doing so though.

+1 on reversing this dependency

[pgayvallet]

we can have a generic helper to migrate the structure

Not sure whether you are asking for the place for this helper to live or the legacy exports.

Sorry, I read we already have. Yes, doing the conversion should (well, you never know with SOs, but still) be trivial

[pgayvallet]

So, after taking a quick look, regarding the legacy import, there is actually way more logic than I thought in the underlying calls to the loaders/SavedObject class.

The most blocking thing I discovered is the way we handle searchSource in legacy import, and the associated logic to display the index-pattern conflicts.

Took me some time to follow the cascade, but:

This is the code that collect the index-pattern conflict errors:

kibana/src/plugins/saved_objects_management/public/lib/resolve_saved_objects.ts

Lines 339 to 351 in 9450248

	try {
	if (await importDocument(obj, otherDoc, overwriteAll)) {
	importedObjectCount++;
	}
	} catch (error) {
	const isIndexPatternNotFound =
	error.constructor.name === 'SavedObjectNotFound' &&
	error.savedObjectType === 'index-pattern';
	if (isIndexPatternNotFound && obj.savedSearchId) {
	conflictedSavedObjectsLinkedToSavedSearches.push(obj);
	} else if (isIndexPatternNotFound) {
	conflictedIndexPatterns.push({ obj, doc: otherDoc });
	} else {

Which calls SavedObject.applyESResp

kibana/src/plugins/saved_objects_management/public/lib/resolve_saved_objects.ts

Lines 136 to 140 in 9450248

	async function importDocument(obj: SavedObject, doc: SavedObjectsRawDoc, overwriteAll: boolean) {
	await obj.applyESResp({
	references: doc._references || [],
	...cloneDeep(doc),
	});

Which does a LOT of internal logic, and mostly calls dependencies.search.searchSource.create, which can return a [error name=SavedObjectNotFound savedObjectType=index-pattern], which is what is used to detect that an object is referencing a missing index pattern.

kibana/src/plugins/saved_objects/public/saved_object/helpers/apply_es_resp.ts

Lines 70 to 85 in d4b2a51

	if (meta.searchSourceJSON) {
	try {
	let searchSourceValues = parseSearchSourceJSON(meta.searchSourceJSON);
	if (config.searchSource) {
	searchSourceValues = injectSearchSourceReferences(
	searchSourceValues as any,
	resp.references
	);
	savedObject.searchSource = await dependencies.search.searchSource.create(
	searchSourceValues
	);
	} else {
	savedObject.searchSourceFields = searchSourceValues;
	}
	} catch (error) {

So in short: getting rid of SO loaders for legacy import would require to recode most of the logic contained in applyESResp.

This is more work than anticipated tbh. Overall, I feel like the easiest solution (by a very large margin) would be to just drop the legacy import in master (only), which would naturally get rid of the associated SO loader usages. But @timroes @flash1293 I feel like this will not be considered an acceptable solution?

The alternative will be to recode / copy / adapt all we need from the saved_objects plugin, but this is going to be tedious, long and error-prone.

[flash1293]

@pgayvallet So if I understand correctly the issue is the check whether an index pattern actually exists? I think we can work around that in a generic way without relying on applyESResp - there should be a references array in the legacy json which we probably can use to check whether there are missing index patterns as well without having to parse the search source. We just have to make sure to resolve the entries in the legacy json in the right order (like the current code is doing - first index patterns, then saved searches, then visualizations, then dashboards)

[pgayvallet]

there should be a references array in the legacy json which we probably can use to check whether there are missing index patterns as well without having to parse the search source

I don't think it should. I think it only may have that references array. Legacy import is supporting very old versions (6.8? older?) and references were not yet a thing at this time. This is what the logic in applyESResp is transforming if I understand it correctly. We are basically doing client-side migration of the object...

[flash1293]

Good point @pgayvallet , I missed those really old legacy exports :) .

We chatted about this offline and IMHO the best option right now is to just leave this code in place until 8.0. It would be possible to implement an intermediary solution now, then remove it in 8.0, but as this code is rarely interacted with I think it's not worth the extra effort; maintaining it for a few more minors is less work.

[TinaHeiligers]

Added impact:low since the remaining work on this is to handle tech debt of removing what should be unused code.
(no longer support legacy imports & edit view has changed to a read-only view).