Support extending config schemas

[chrisronline]

As discussed with @joshdover and @pgayvallet, it'd be nice to extend a previously defined config schema.

More specifically, given an existing config schema, we should be able to do:

elasticsearch: schema.object({
  ...configSchema,
  somethingNew: schema.string()

or

elasticsearch: configSchema.extends({logFetchCount: schema.number()})

[elasticmachine]

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

[pgayvallet]

elasticsearch: schema.object({
  ...configSchema,
  somethingNew: schema.string()
})

would be ideal, however schema.object is not spreadable, so I doubt we will be able to do so.

The second proposal seems more realistic, by adding the extend/extends method to schema.object type (and only to this one)

elasticsearch: configSchema.extends({logFetchCount: schema.number()})

[mshustov]

...configSchema,

There could be configSchema.getRaw() instead of spread. This approach allows plugins to delete excessive params

[pgayvallet]

There could be configSchema.getRaw() instead of spread. This approach allows plugins to delete excessive params

elasticsearch: schema.object({
  ...configSchema.getRaw(),
  somethingNew: schema.string()
})

would then work.

Notable difference with extends is that it doesn't copy the ObjectTypeOptions, where extends would be able to do. But I think this is more an upside than a downside ?

My other concern is regarding TS typing, but I think

ObjectType<P extends Props = any> extends Type<ObjectResultType<P>>

getRaw(): P (or maybe ObjectResultType<P>)

would 'simply' work.

[chrisronline]

Would it be possible to change the defaultValues of an existing config schema too? As I dive more into the support necessary for stack monitoring, we might have a need to reset the defaultValue for at least one of the configs, specifically elasticsearch.hosts as the configuration is optional for the monitoring configs (so the default value should be an empty string or null or something). We might be able to work around this, but ideally we can support this too.

[mshustov]

@chrisronline that would require some additional work. Not sure we have capacity at the moment. Could you simply copy ES config schema locally for a time being?

I'm also wondering why we cannot place logFetchCount out ot elasticsearch namespace?

[chrisronline]

@restrry Yes I can do that.

I'm also wondering why we cannot place logFetchCount out ot elasticsearch namespace?

We could do this, but I wasn't sure of the effort involved with supporting the override.

[pgayvallet]

#67357 addresses the initial request, by adding a getRaw method to ObjectType, which would allow to create altered ObjectType from the original.

Going further than that, like allowing to edit or create a new schema with altered options, is going to be difficult to implement properly and totally:

We could start by adding a getOption method, either on the root Type class, or on every child class. That would allow to do something like

const origin = schema.object({
   initial: schema.string(),
}, options);

const extended = schema.object(initial.getRaw(), { ...initial.getOptions(), ...additionalOptions })

However the two approaches I tried got some severe limitations:

Adding on the root class

The root Type class has currently no knowledge of it's superclass' property type. Adding getOptions to Type would only be able to return a TypeOption

kibana/packages/kbn-config-schema/src/types/type.ts

Lines 24 to 27 in c4ddd00

	export interface TypeOptions<T> {
	defaultValue?: T | Reference<T> | (() => T);
	validate?: (value: T) => string | void;
	}

This would work to change the defaultValue for example, but not any option defined by the superclass, such as unknowns for ObjectTypeOptions

kibana/packages/kbn-config-schema/src/types/object_type.ts

Lines 60 to 61 in 442b6d7

	export type ObjectTypeOptions<P extends Props = any> = TypeOptions<ObjectResultType<P>> &
	UnknownOptions;

Adding to the child class

We could also add the getOptions to each child class such as ObjectType, StringType and so on. That would allow to return a properly typed option instance for each schema type.

There are a few issue with this:

1/ currently, the schema factories (schema.string, schema.number) are returning generic Type<T> types

kibana/packages/kbn-config-schema/src/index.ts

Lines 77 to 83 in c4ddd00

	function stream(options?: TypeOptions<Stream>): Type<Stream> {
	return new StreamType(options);
	}
	function string(options?: StringOptions): Type<string> {
	return new StringType(options);
	}

Meaning that schema.string().getOptions() would raise an error as Type.getOptions is not defined. This could be fixed for the base types by changing the factories signatures

function string(options?: StringOptions): Type<string> {
  return new StringType(options);
}

would become

function string(options?: StringOptions): StringType {
  return new StringType(options);
}

Structured / Complex types issues

However, the non-basic, structured types such as oneOf or maybe still have some issues with both approaches:

const schema = schema.oneOf([schema.string(), schema.number()]);
schema.getOptions() // what should I return??

properly allowing to 'extend' structured types would require adding specific composition API to each of them.

For example for oneOf, we could imagine something like

const oneOfSchema = schema.oneOf([schema.string(), schema.number()]);
oneOfSchema.getTypes()// would return [instanceof Type<string>, instanceof Type<number>]

// could be used like
schema.oneOf([...oneOfSchema.getTypes(), schema.someOtherType()]);

However once again, the oneOf signature is only working with the Type class

https://github.com/elastic/kibana/blob/c4ddd00540d838add7bdea9faf741ad4f288cca6/packages/kbn-config-schema/src/index.ts#L172-L175\

Meaning that the nested types would not be properly typed:

const oneOfSchema = schema.oneOf([schema.string(), schema.number()]);
oneOfSchema.getTypes()[0].getOptions() // error: property `getOptions` is not defined on type `Type<string>`

Adding a second generic type to Type

Alternative to the previous point that seems to address the structured types options typings.

class Type<V> could evolve to class Type<V, OptionType extends TypeOptions>. That would allow to define the getOptions method only on the Type superclass with proper typing

export class StringType extends Type<string, StringOptions> {...}

function oneOf<A, B, OA, OB>(types: [Type<A, OA>, Type<B, OB>], options?: OA | OB): Type<A | B>;

I think that would work. However these are still heavy changes to the package, to address both the options typing and the structured type issues.

In summary

Extending object schemas properties as done in #67357 is the main request here, and is sufficient for at least #61361.

Any additional enhancement is way more complex, which is why I'm not sure we should really try to go further, as least not without a very solid reason.

[pgayvallet]

I created #68067 as an alternative implementing extends instead of getRaw.

@joshdover

I agree that the extends approach is more elegant. Most significant downside with it is that it would not allow to go further on the schema extension problematic if we want to, as we don't let the consumer access the properties directly (see #59083 (comment)). However as explained in the linked comment, implementing a complete extension API is rather complex, and I'm unsure of the need. So If we think that extending object schema's properties without altering the existing ones if enough, the extends approach is probably better.

Tell me what you think

[joshdover]

I feel that going with extends is more consumer-friendly. I also want to avoid exposing any internals that will make upgrade Joi harder than it needs to be. Especially if we can find other workarounds for future needs.