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

kbn/config-schema: Consider maybe properties as optional keys in ObjectType #63838

Merged
merged 4 commits into from
Apr 21, 2020
Merged

kbn/config-schema: Consider maybe properties as optional keys in ObjectType #63838

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3c28c21
consider optional properties as optional keys in ObjectType
pgayvallet Apr 17, 2020
58e2756
fix type on security config
pgayvallet Apr 17, 2020
996cd13
fix ObjectTypeOptions
pgayvallet Apr 17, 2020
c229590
Merge remote-tracking branch 'upstream/master' into kbn-63725-maybe-o…
pgayvallet Apr 20, 2020
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
24 changes: 24 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 @@ -18,6 +18,7 @@
*/

import { schema } from '..';
import { TypeOf } from './object_type';

test('returns value by default', () => {
const type = schema.object({
Expand Down Expand Up @@ -350,3 +351,26 @@ test('unknowns = `ignore` affects only own keys', () => {
})
).toThrowErrorMatchingInlineSnapshot(`"[foo.baz]: definition for this key is missing"`);
});

test('handles optional properties', () => {
const type = schema.object({
required: schema.string(),
optional: schema.maybe(schema.string()),
});

type SchemaType = TypeOf<typeof type>;

let foo: SchemaType = {
required: 'foo',
};
foo = {
required: 'hello',
optional: undefined,
};
foo = {
required: 'hello',
optional: 'bar',
};
Comment on lines +363 to +373
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Until we got proper way to test TS typings, this is the best test I could add. Not even sure it's very relevant, as I can only assert valid assignments and not invalid ones.

Copy link
Contributor

Choose a reason for hiding this comment

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

I added this test to #53762 we can improve it later.


expect(foo).toBeDefined();
});
22 changes: 20 additions & 2 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 @@ -26,9 +26,26 @@ export type Props = Record<string, Type<any>>;

export type TypeOf<RT extends Type<any>> = RT['type'];

type OptionalProperties<Base extends Props> = Pick<
Base,
{
[Key in keyof Base]: undefined extends TypeOf<Base[Key]> ? Key : never;
}[keyof Base]
>;

type RequiredProperties<Base extends Props> = Pick<
Base,
{
[Key in keyof Base]: undefined extends TypeOf<Base[Key]> ? never : Key;
}[keyof Base]
>;

// Because of https://github.com/Microsoft/TypeScript/issues/14041
// this might not have perfect _rendering_ output, but it will be typed.
export type ObjectResultType<P extends Props> = Readonly<{ [K in keyof P]: TypeOf<P[K]> }>;
export type ObjectResultType<P extends Props> = Readonly<
Copy link
Contributor

Choose a reason for hiding this comment

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

maxresdefault

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea, TS magic... TBH I really thought there would be an easier way to type that, but AFAIK there is not, as the ? modifier for an object key(s) cannot be directly inferred / depends on the associated value/property...

{ [K in keyof OptionalProperties<P>]?: TypeOf<P[K]> } &
{ [K in keyof RequiredProperties<P>]: TypeOf<P[K]> }
>;

interface UnknownOptions {
/**
Expand All @@ -41,7 +58,8 @@ interface UnknownOptions {
}

export type ObjectTypeOptions<P extends Props = any> = TypeOptions<
{ [K in keyof P]: TypeOf<P[K]> }
{ [K in keyof OptionalProperties<P>]?: TypeOf<P[K]> } &
{ [K in keyof RequiredProperties<P>]: TypeOf<P[K]> }
> &
UnknownOptions;

Expand Down
6 changes: 3 additions & 3 deletions src/core/server/http/router/validator/validator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,23 +170,23 @@ export class RouteValidator<P = {}, Q = {}, B = {}> {
* @internal
*/
public getParams(data: unknown, namespace?: string): Readonly<P> {
return this.validate(this.config.params, this.options.unsafe?.params, data, namespace);
return this.validate(this.config.params, this.options.unsafe?.params, data, namespace) as P;
Comment on lines -173 to +180
Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, introducing this change in the ObjectResultType caused errors due to RouteValidationSpec and RouteValidationResultType types being different for ObjectType schemas, as the keys mutates. However after spending a lot of time on that (and with @afharo 's help), it appear that this can't really be fixed without a reverse type for ObjectResultType (something like ObjectParamsFromResultType), to properly fix RouteValidationResultType, which is not technically doable.

I think this is still fine because:

  • type inference is still working, and the maybe property keys are correctly considered optionals in the resulting request properties when using in route handlers. It's really only the internal/private methods typing that needs this fix (the return type of validate). it 'works as expected' for a public API point of view.
  • we got good test coverage for this code.

But if someone wants to take another look, please do 😄

}

/**
* Get validated query params
* @internal
*/
public getQuery(data: unknown, namespace?: string): Readonly<Q> {
return this.validate(this.config.query, this.options.unsafe?.query, data, namespace);
return this.validate(this.config.query, this.options.unsafe?.query, data, namespace) as Q;
}

/**
* Get validated body
* @internal
*/
public getBody(data: unknown, namespace?: string): Readonly<B> {
return this.validate(this.config.body, this.options.unsafe?.body, data, namespace);
return this.validate(this.config.body, this.options.unsafe?.body, data, namespace) as B;
}

/**
Expand Down