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

pgayvallet · 2020-04-17T11:57:57Z

Summary

Adapt ObjectResultType to consider that undefined-able child properties are optional keys.

Checklist

Unit or functional tests were updated or added to match the most common scenarios

pgayvallet · 2020-04-17T12:01:25Z

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

+  let foo: SchemaType = {
+    required: 'foo',
+  };
+  foo = {
+    required: 'hello',
+    optional: undefined,
+  };
+  foo = {
+    required: 'hello',
+    optional: 'bar',
+  };


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.

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

pgayvallet · 2020-04-17T12:52:18Z

src/core/server/http/router/validator/validator.ts

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


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 😄

elasticmachine · 2020-04-17T17:15:34Z

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

…bj-keys-nightmare

kibanamachine · 2020-04-20T08:12:37Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: c229590

History

💛 Build #41555 was flaky 996cd13
💚 Build #41546 succeeded 58e2756
💔 Build #41523 failed 3c28c21

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

afharo

Awesome job! And great catch with the optional keys!

mshustov · 2020-04-20T14:42:52Z

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

+  let foo: SchemaType = {
+    required: 'foo',
+  };
+  foo = {
+    required: 'hello',
+    optional: undefined,
+  };
+  foo = {
+    required: 'hello',
+    optional: 'bar',
+  };


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

mshustov · 2020-04-20T14:45:26Z

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

 // 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<


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

…ctType (elastic#63838) * consider optional properties as optional keys in ObjectType * fix type on security config * fix ObjectTypeOptions

* master: (30 commits) Migrate vis_type_table to kibana/new platform (elastic#63105) Enable include/exclude in Terms agg for numeric fields (elastic#59425) follow conventions for saved object definitions (elastic#63571) [Docs]Adds saved object key setting to load balancing kib instances (elastic#63935) kbn/config-schema: Consider maybe properties as optional keys in ObjectType (elastic#63838) Fix page layouts, clean up unused code (elastic#63992) [SIEM] Adds recursive exact key checks for validation and formatter [Maps] Migrate remaining maps client files to NP (except routi… (elastic#63859) [Maps] Do not fetch geo_shape from docvalues (elastic#63997) Vega doc changes (elastic#63889) [Metrics UI] Reorganize file layout for Metrics UI (elastic#60049) Add sub-menus to Resolver node (for 75% zoom) (elastic#63476) [FTR] Add test suite metrics tracking/output (elastic#62515) [ML] Fixing single metric viewer page padding (elastic#63839) [Discover] Allow user to generate a report after saving a modified saved search (elastic#63623) [Reporting] Config flag to escape formula CSV values (elastic#63645) [Metrics UI] Remove remaining field filtering (elastic#63398) [Maps] fix date labels (elastic#63909) [TSVB] Use default Kibana palette for split series (elastic#62241) Ingest overview page (elastic#63214) ...

…ctType (#63838) (#64036) * consider optional properties as optional keys in ObjectType * fix type on security config * fix ObjectTypeOptions

…t-node-pipelines * 'master' of github.com:elastic/kibana: (32 commits) Move authz lib out of snapshot restore (#63947) Migrate vis_type_table to kibana/new platform (#63105) Enable include/exclude in Terms agg for numeric fields (#59425) follow conventions for saved object definitions (#63571) [Docs]Adds saved object key setting to load balancing kib instances (#63935) kbn/config-schema: Consider maybe properties as optional keys in ObjectType (#63838) Fix page layouts, clean up unused code (#63992) [SIEM] Adds recursive exact key checks for validation and formatter [Maps] Migrate remaining maps client files to NP (except routi… (#63859) [Maps] Do not fetch geo_shape from docvalues (#63997) Vega doc changes (#63889) [Metrics UI] Reorganize file layout for Metrics UI (#60049) Add sub-menus to Resolver node (for 75% zoom) (#63476) [FTR] Add test suite metrics tracking/output (#62515) [ML] Fixing single metric viewer page padding (#63839) [Discover] Allow user to generate a report after saving a modified saved search (#63623) [Reporting] Config flag to escape formula CSV values (#63645) [Metrics UI] Remove remaining field filtering (#63398) [Maps] fix date labels (#63909) [TSVB] Use default Kibana palette for split series (#62241) ...

…bana into ingest-node-pipelines/privileges * 'feature/ingest-node-pipelines' of github.com:elastic/kibana: (34 commits) Move authz lib out of snapshot restore (elastic#63947) Migrate vis_type_table to kibana/new platform (elastic#63105) Enable include/exclude in Terms agg for numeric fields (elastic#59425) follow conventions for saved object definitions (elastic#63571) [Docs]Adds saved object key setting to load balancing kib instances (elastic#63935) kbn/config-schema: Consider maybe properties as optional keys in ObjectType (elastic#63838) Fix page layouts, clean up unused code (elastic#63992) [SIEM] Adds recursive exact key checks for validation and formatter [Maps] Migrate remaining maps client files to NP (except routi… (elastic#63859) [Maps] Do not fetch geo_shape from docvalues (elastic#63997) Vega doc changes (elastic#63889) [Metrics UI] Reorganize file layout for Metrics UI (elastic#60049) Add sub-menus to Resolver node (for 75% zoom) (elastic#63476) [FTR] Add test suite metrics tracking/output (elastic#62515) [Ingest pipelines] Delete pipeline (elastic#63635) [ML] Fixing single metric viewer page padding (elastic#63839) [Discover] Allow user to generate a report after saving a modified saved search (elastic#63623) [Reporting] Config flag to escape formula CSV values (elastic#63645) [Metrics UI] Remove remaining field filtering (elastic#63398) [Maps] fix date labels (elastic#63909) ... # Conflicts: # x-pack/legacy/plugins/uptime/public/components/monitor/ml/index.ts # x-pack/legacy/plugins/uptime/public/components/overview/index.ts # x-pack/plugins/ingest_pipelines/public/application/index.tsx # x-pack/plugins/ingest_pipelines/server/routes/api/index.ts # x-pack/plugins/ingest_pipelines/server/routes/index.ts

…bana into pipeline-editor-part-mvp-2 * 'feature/ingest-node-pipelines' of github.com:elastic/kibana: (34 commits) [Ingest Node Pipelines] Clone Pipeline (elastic#64049) Move authz lib out of snapshot restore (elastic#63947) Migrate vis_type_table to kibana/new platform (elastic#63105) Enable include/exclude in Terms agg for numeric fields (elastic#59425) follow conventions for saved object definitions (elastic#63571) [Docs]Adds saved object key setting to load balancing kib instances (elastic#63935) kbn/config-schema: Consider maybe properties as optional keys in ObjectType (elastic#63838) Fix page layouts, clean up unused code (elastic#63992) [SIEM] Adds recursive exact key checks for validation and formatter [Maps] Migrate remaining maps client files to NP (except routi… (elastic#63859) [Maps] Do not fetch geo_shape from docvalues (elastic#63997) Vega doc changes (elastic#63889) [Metrics UI] Reorganize file layout for Metrics UI (elastic#60049) Add sub-menus to Resolver node (for 75% zoom) (elastic#63476) [FTR] Add test suite metrics tracking/output (elastic#62515) [Ingest pipelines] Delete pipeline (elastic#63635) [ML] Fixing single metric viewer page padding (elastic#63839) [Discover] Allow user to generate a report after saving a modified saved search (elastic#63623) [Reporting] Config flag to escape formula CSV values (elastic#63645) [Metrics UI] Remove remaining field filtering (elastic#63398) ...

consider optional properties as optional keys in ObjectType

3c28c21

pgayvallet commented Apr 17, 2020

View reviewed changes

pgayvallet added 2 commits April 17, 2020 15:14

fix type on security config

58e2756

fix ObjectTypeOptions

996cd13

pgayvallet added this to Pending Review in kibana-core [DEPRECATED] via automation Apr 17, 2020

pgayvallet added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.8.0 v8.0.0 labels Apr 17, 2020

pgayvallet added the release_note:skip Skip the PR/issue when compiling release notes label Apr 17, 2020

pgayvallet marked this pull request as ready for review April 17, 2020 17:15

pgayvallet requested a review from a team as a code owner April 17, 2020 17:15

Merge remote-tracking branch 'upstream/master' into kbn-63725-maybe-o…

c229590

…bj-keys-nightmare

afharo reviewed Apr 20, 2020

View reviewed changes

mshustov approved these changes Apr 20, 2020

View reviewed changes

pgayvallet merged commit f6f610d into elastic:master Apr 21, 2020

kibana-core [DEPRECATED] automation moved this from Pending Review to Done (7.8) Apr 21, 2020

pgayvallet mentioned this pull request Apr 21, 2020

[7.x] kbn/config-schema: Consider maybe properties as optional keys in ObjectType (#63838) #64036

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

pgayvallet commented Apr 17, 2020 •

edited

pgayvallet Apr 17, 2020

mshustov Apr 20, 2020

pgayvallet Apr 17, 2020

elasticmachine commented Apr 17, 2020

kibanamachine commented Apr 20, 2020

afharo left a comment

mshustov Apr 20, 2020

mshustov Apr 20, 2020

pgayvallet Apr 21, 2020

		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;

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

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

Conversation

pgayvallet commented Apr 17, 2020 • edited

Summary

Checklist

pgayvallet Apr 17, 2020

Choose a reason for hiding this comment

mshustov Apr 20, 2020

Choose a reason for hiding this comment

pgayvallet Apr 17, 2020

Choose a reason for hiding this comment

elasticmachine commented Apr 17, 2020

kibanamachine commented Apr 20, 2020

💚 Build Succeeded

History

afharo left a comment

Choose a reason for hiding this comment

mshustov Apr 20, 2020

Choose a reason for hiding this comment

mshustov Apr 20, 2020

Choose a reason for hiding this comment

pgayvallet Apr 21, 2020

Choose a reason for hiding this comment

pgayvallet commented Apr 17, 2020 •

edited