Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Make config-schema extensible for handling of unknown fields #156214

Merged
merged 12 commits into from
May 5, 2023
Merged

Make config-schema extensible for handling of unknown fields #156214

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
0302a5d
Initial commit
mikecote Apr 19, 2023
262805f
Cleanup code
mikecote Apr 28, 2023
f3cd265
Update POC
mikecote Apr 28, 2023
ac50762
Merge branch 'main' into config-schema/poc-extendsDeep
kibanamachine May 1, 2023
0be20c9
Merge branch 'main' of github.com:elastic/kibana into config-schema/p…
mikecote May 2, 2023
db1c624
Add tests
mikecote May 2, 2023
8d5abe3
Merge branch 'config-schema/poc-extendsDeep' of github.com:mikecote/k…
mikecote May 2, 2023
fc31e32
Set updated schema into variables
mikecote May 3, 2023
532f5ee
[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…
kibanamachine May 3, 2023
0f1357b
Share OptionsForUnknowns
mikecote May 3, 2023
132cfed
Merge branch 'config-schema/poc-extendsDeep' of github.com:mikecote/k…
mikecote May 3, 2023
dedf230
Merge branch 'main' into config-schema/poc-extendsDeep
kibanamachine May 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions packages/kbn-config-schema/src/types/array_type.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,3 +188,30 @@ describe('#maxSize', () => {
).toThrowErrorMatchingInlineSnapshot(`"array size is [2], but cannot be greater than [1]"`);
});
});

describe('#extendsDeep', () => {
const type = schema.arrayOf(
schema.object({
foo: schema.string(),
})
);

test('objects with unknown attributes are kept when extending with unknowns=allow', () => {
const allowSchema = type.extendsDeep({ unknowns: 'allow' });
const result = allowSchema.validate([{ foo: 'test', bar: 'test' }]);
expect(result).toEqual([{ foo: 'test', bar: 'test' }]);
});

test('objects with unknown attributes are dropped when extending with unknowns=ignore', () => {
const ignoreSchema = type.extendsDeep({ unknowns: 'ignore' });
const result = ignoreSchema.validate([{ foo: 'test', bar: 'test' }]);
expect(result).toEqual([{ foo: 'test' }]);
});

test('objects with unknown attributes fail validation when extending with unknowns=forbid', () => {
const forbidSchema = type.extendsDeep({ unknowns: 'forbid' });
expect(() =>
forbidSchema.validate([{ foo: 'test', bar: 'test' }])
).toThrowErrorMatchingInlineSnapshot(`"[0.bar]: definition for this key is missing"`);
});
});
11 changes: 10 additions & 1 deletion packages/kbn-config-schema/src/types/array_type.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,17 @@

import typeDetect from 'type-detect';
import { internals } from '../internals';
import { Type, TypeOptions } from './type';
import { Type, TypeOptions, ExtendsDeepOptions } from './type';

export type ArrayOptions<T> = TypeOptions<T[]> & {
minSize?: number;
maxSize?: number;
};

export class ArrayType<T> extends Type<T[]> {
private readonly arrayType: Type<T>;
private readonly arrayOptions: ArrayOptions<T>;

constructor(type: Type<T>, options: ArrayOptions<T> = {}) {
let schema = internals.array().items(type.getSchema().optional()).sparse(false);

Expand All @@ -28,6 +31,12 @@ export class ArrayType<T> extends Type<T[]> {
}

super(schema, options);
this.arrayType = type;
this.arrayOptions = options;
}

public extendsDeep(options: ExtendsDeepOptions) {
return new ArrayType(this.arrayType.extendsDeep(options), this.arrayOptions);
}

protected handleError(type: string, { limit, reason, value }: Record<string, any>) {
Expand Down
81 changes: 81 additions & 0 deletions packages/kbn-config-schema/src/types/conditional_type.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -395,3 +395,84 @@ describe('#validate', () => {
});
});
});

describe('#extendsDeep', () => {
describe('#equalType', () => {
const type = schema.object({
foo: schema.string(),
test: schema.conditional(
schema.siblingRef('foo'),
'test',
schema.object({
bar: schema.string(),
}),
schema.string()
),
});

test('objects with unknown attributes are kept when extending with unknowns=allow', () => {
const result = type
.extendsDeep({ unknowns: 'allow' })
Comment on lines +414 to +415
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(unrelated to this line) I wonder if that extendsDeep will be sufficient covering our 'eviction schema' needs. Like, isn't there going to be scenarios were teams would need a more finely grained configuration, e.g ignore for some props, allow for some others and so on?

I guess in that case, they can still just manually redefine the schema (if such scenario even make sense)

Copy link
Contributor Author

@mikecote mikecote May 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can only explain from how I plan to use this but feel free to propose changes. In the example of Task Manager, we plan to evict unknown properties when reading from the task.state object while preventing tasks from running if task.params contains unknown properties (ex: properties created by a newer Kibana version).

We would build two different task schema objects to satisfy the needs:

  • schema for read would ignore unknowns for task.state while forbid unknowns for task.params
  • schema for write would forbid unknowns for task.state and task.params

Since we have control of the params schema by task type and the state schema by task type, we can intercept those and only apply .extendsDeep(...) as needed before building the task read/write schema for a given task type.

.validate({ foo: 'test', test: { bar: 'test', baz: 'test' } });
expect(result).toEqual({
foo: 'test',
test: { bar: 'test', baz: 'test' },
});
});

test('objects with unknown attributes are dropped when extending with unknowns=ignore', () => {
const result = type
.extendsDeep({ unknowns: 'ignore' })
.validate({ foo: 'test', test: { bar: 'test', baz: 'test' } });
expect(result).toEqual({
foo: 'test',
test: { bar: 'test' },
});
});
test('objects with unknown attributes fail validation when extending with unknowns=forbid', () => {
expect(() =>
type
.extendsDeep({ unknowns: 'forbid' })
.validate({ foo: 'test', test: { bar: 'test', baz: 'test' } })
).toThrowErrorMatchingInlineSnapshot(`"[test.baz]: definition for this key is missing"`);
});
});

describe('#notEqualType', () => {
const type = schema.object({
foo: schema.string(),
test: schema.conditional(
schema.siblingRef('foo'),
'test',
schema.string(),
schema.object({
bar: schema.string(),
})
),
});

test('objects with unknown attributes are kept when extending with unknowns=allow', () => {
const allowSchema = type.extendsDeep({ unknowns: 'allow' });
const result = allowSchema.validate({ foo: 'not-test', test: { bar: 'test', baz: 'test' } });
expect(result).toEqual({
foo: 'not-test',
test: { bar: 'test', baz: 'test' },
});
});

test('objects with unknown attributes are dropped when extending with unknowns=ignore', () => {
const ignoreSchema = type.extendsDeep({ unknowns: 'ignore' });
const result = ignoreSchema.validate({ foo: 'not-test', test: { bar: 'test', baz: 'test' } });
expect(result).toEqual({
foo: 'not-test',
test: { bar: 'test' },
});
});
test('objects with unknown attributes fail validation when extending with unknowns=forbid', () => {
const forbidSchema = type.extendsDeep({ unknowns: 'forbid' });
expect(() =>
forbidSchema.validate({ foo: 'not-test', test: { bar: 'test', baz: 'test' } })
).toThrowErrorMatchingInlineSnapshot(`"[test.baz]: definition for this key is missing"`);
});
});
});
23 changes: 22 additions & 1 deletion packages/kbn-config-schema/src/types/conditional_type.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,17 @@
import typeDetect from 'type-detect';
import { internals } from '../internals';
import { Reference } from '../references';
import { Type, TypeOptions } from './type';
import { ExtendsDeepOptions, Type, TypeOptions } from './type';

export type ConditionalTypeValue = string | number | boolean | object | null;

export class ConditionalType<A extends ConditionalTypeValue, B, C> extends Type<B | C> {
private readonly leftOperand: Reference<A>;
private readonly rightOperand: Reference<A> | A | Type<unknown>;
private readonly equalType: Type<B>;
private readonly notEqualType: Type<C>;
private readonly options?: TypeOptions<B | C>;

constructor(
leftOperand: Reference<A>,
rightOperand: Reference<A> | A | Type<unknown>,
Expand All @@ -31,6 +37,21 @@ export class ConditionalType<A extends ConditionalTypeValue, B, C> extends Type<
});

super(schema, options);
this.leftOperand = leftOperand;
this.rightOperand = rightOperand;
this.equalType = equalType;
this.notEqualType = notEqualType;
this.options = options;
}

public extendsDeep(options: ExtendsDeepOptions) {
return new ConditionalType(
this.leftOperand,
this.rightOperand,
this.equalType.extendsDeep(options),
this.notEqualType.extendsDeep(options),
this.options
);
}

protected handleError(type: string, { value }: Record<string, any>) {
Expand Down
25 changes: 25 additions & 0 deletions packages/kbn-config-schema/src/types/map_of_type.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,3 +185,28 @@ test('error preserves full path', () => {
`"[grandParentKey.parentKey.ab]: expected value of type [number] but got [string]"`
);
});

describe('#extendsDeep', () => {
describe('#keyType', () => {
const type = schema.mapOf(schema.string(), schema.object({ foo: schema.string() }));

test('objects with unknown attributes are kept when extending with unknowns=allow', () => {
const allowSchema = type.extendsDeep({ unknowns: 'allow' });
const result = allowSchema.validate({ key: { foo: 'test', bar: 'test' } });
expect(result.get('key')).toEqual({ foo: 'test', bar: 'test' });
});

test('objects with unknown attributes are dropped when extending with unknowns=ignore', () => {
const ignoreSchema = type.extendsDeep({ unknowns: 'ignore' });
const result = ignoreSchema.validate({ key: { foo: 'test', bar: 'test' } });
expect(result.get('key')).toEqual({ foo: 'test' });
});

test('objects with unknown attributes fail validation when extending with unknowns=forbid', () => {
const forbidSchema = type.extendsDeep({ unknowns: 'forbid' });
expect(() =>
forbidSchema.validate({ key: { foo: 'test', bar: 'test' } })
).toThrowErrorMatchingInlineSnapshot(`"[key.bar]: definition for this key is missing"`);
});
});
});
17 changes: 16 additions & 1 deletion packages/kbn-config-schema/src/types/map_type.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,15 @@
import typeDetect from 'type-detect';
import { SchemaTypeError, SchemaTypesError } from '../errors';
import { internals } from '../internals';
import { Type, TypeOptions } from './type';
import { Type, TypeOptions, ExtendsDeepOptions } from './type';

export type MapOfOptions<K, V> = TypeOptions<Map<K, V>>;

export class MapOfType<K, V> extends Type<Map<K, V>> {
private readonly keyType: Type<K>;
private readonly valueType: Type<V>;
private readonly mapOptions: MapOfOptions<K, V>;

constructor(keyType: Type<K>, valueType: Type<V>, options: MapOfOptions<K, V> = {}) {
const defaultValue = options.defaultValue;
const schema = internals.map().entries(keyType.getSchema(), valueType.getSchema());
Expand All @@ -26,6 +30,17 @@ export class MapOfType<K, V> extends Type<Map<K, V>> {
// default value instead.
defaultValue: defaultValue instanceof Map ? () => defaultValue : defaultValue,
});
this.keyType = keyType;
this.valueType = valueType;
this.mapOptions = options;
}

public extendsDeep(options: ExtendsDeepOptions) {
return new MapOfType(
this.keyType.extendsDeep(options),
this.valueType.extendsDeep(options),
this.mapOptions
);
}

protected handleError(
Expand Down
23 changes: 23 additions & 0 deletions packages/kbn-config-schema/src/types/maybe_type.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,3 +95,26 @@ describe('maybe + object', () => {
expect(type.validate({})).toEqual({});
});
});

describe('#extendsDeep', () => {
const type = schema.maybe(schema.object({ foo: schema.string() }));

test('objects with unknown attributes are kept when extending with unknowns=allow', () => {
const allowSchema = type.extendsDeep({ unknowns: 'allow' });
const result = allowSchema.validate({ foo: 'test', bar: 'test' });
expect(result).toEqual({ foo: 'test', bar: 'test' });
});

test('objects with unknown attributes are dropped when extending with unknowns=ignore', () => {
const ignoreSchema = type.extendsDeep({ unknowns: 'ignore' });
const result = ignoreSchema.validate({ foo: 'test', bar: 'test' });
expect(result).toEqual({ foo: 'test' });
});

test('objects with unknown attributes fail validation when extending with unknowns=forbid', () => {
const forbidSchema = type.extendsDeep({ unknowns: 'forbid' });
expect(() =>
forbidSchema.validate({ foo: 'test', bar: 'test' })
).toThrowErrorMatchingInlineSnapshot(`"[bar]: definition for this key is missing"`);
});
});
9 changes: 8 additions & 1 deletion packages/kbn-config-schema/src/types/maybe_type.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,22 @@
* Side Public License, v 1.
*/

import { Type } from './type';
import { Type, ExtendsDeepOptions } from './type';

export class MaybeType<V> extends Type<V | undefined> {
private readonly maybeType: Type<V>;

constructor(type: Type<V>) {
super(
type
.getSchema()
.optional()
.default(() => undefined)
);
this.maybeType = type;
}

public extendsDeep(options: ExtendsDeepOptions) {
return new MaybeType(this.maybeType.extendsDeep(options));
}
}
23 changes: 23 additions & 0 deletions packages/kbn-config-schema/src/types/object_type.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -563,3 +563,26 @@ test('returns schema structure', () => {
{ path: ['nested', 'uri'], type: 'string' },
]);
});

describe('#extendsDeep', () => {
const type = schema.object({ test: schema.object({ foo: schema.string() }) });

test('objects with unknown attributes are kept when extending with unknowns=allow', () => {
const allowSchema = type.extendsDeep({ unknowns: 'allow' });
const result = allowSchema.validate({ test: { foo: 'test', bar: 'test' } });
expect(result).toEqual({ test: { foo: 'test', bar: 'test' } });
});

test('objects with unknown attributes are dropped when extending with unknowns=ignore', () => {
const ignoreSchema = type.extendsDeep({ unknowns: 'ignore' });
const result = ignoreSchema.validate({ test: { foo: 'test', bar: 'test' } });
expect(result).toEqual({ test: { foo: 'test' } });
});

test('objects with unknown attributes fail validation when extending with unknowns=forbid', () => {
const forbidSchema = type.extendsDeep({ unknowns: 'forbid' });
expect(() =>
forbidSchema.validate({ test: { foo: 'test', bar: 'test' } })
).toThrowErrorMatchingInlineSnapshot(`"[test.bar]: definition for this key is missing"`);
});
});
29 changes: 21 additions & 8 deletions packages/kbn-config-schema/src/types/object_type.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
import type { AnySchema } from 'joi';
import typeDetect from 'type-detect';
import { internals } from '../internals';
import { Type, TypeOptions } from './type';
import { Type, TypeOptions, ExtendsDeepOptions, OptionsForUnknowns } from './type';
import { ValidationError } from '../errors';

export type Props = Record<string, Type<any>>;
Expand Down Expand Up @@ -60,13 +60,7 @@ type ExtendedObjectTypeOptions<P extends Props, NP extends NullableProps> = Obje
>;

interface UnknownOptions {
/**
* Options for dealing with unknown keys:
* - allow: unknown keys will be permitted
* - ignore: unknown keys will not fail validation, but will be stripped out
* - forbid (default): unknown keys will fail validation
*/
unknowns?: 'allow' | 'ignore' | 'forbid';
unknowns?: OptionsForUnknowns;
}

export type ObjectTypeOptions<P extends Props = any> = TypeOptions<ObjectResultType<P>> &
Expand Down Expand Up @@ -181,6 +175,25 @@ export class ObjectType<P extends Props = any> extends Type<ObjectResultType<P>>
return new ObjectType(extendedProps, extendedOptions);
}

public extendsDeep(options: ExtendsDeepOptions) {
const extendedProps = Object.entries(this.props).reduce((memo, [key, value]) => {
if (value !== null && value !== undefined) {
return {
Comment on lines +178 to +181
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(thinking out loud here) I'm overall fine with the recursive approach, I can understand that it would be complicated / tedious to manually redefine everything.

Now, if this extendsDeep approach looks fine to me as an internal API to perform the mutation, I wonder if that's what we want as the public API.

I was more thinking of something like:

const type = schema.object({ foo: schema.string() });
const savedObject = { foo: 'test', bar: 'test' };
// ... later
type.validate(savedObject, { unknowns: 'ignore' });

Now, technically, internally we don't want to create a new schema everytime validate is called with this option. But if we know that we will only be exposing this single 'schema mutation' option to validate, we could store the mutated schema internally.

However, the validate signature is already crowded with 'useless' stuff as second and third parameters:

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

Line 86 in 27ff7d3

public validate(value: any, context: Record<string, any> = {}, namespace?: string): V {

So I don't think it would be that easy to adapt it to have a good DX, as it would look like

type.validate(savedObject, {}, undefined, { unknowns: 'ignore' });

So in summary, your approach is probably the most pragmatic one.

...memo,
[key]: value.extendsDeep(options),
};
}
return memo;
}, {} as P);

const extendedOptions: ObjectTypeOptions<P> = {
...this.options,
...(options.unknowns ? { unknowns: options.unknowns } : {}),
};

return new ObjectType(extendedProps, extendedOptions);
}

protected handleError(type: string, { reason, value }: Record<string, any>) {
switch (type) {
case 'any.required':
Expand Down
Loading