Update schemas boolean, byteSize, and duration to coerce strings by jportner · Pull Request #54177 · elastic/kibana

jportner · 2020-01-07T18:43:27Z

Summary

This PR updates the boolean, byteSize, and duration schemas in the Kibana Platform to coerce string values.

boolean: now accepts 'true' and 'false' (case-insensitive).
byteSize: now accepts number strings; assumes any string that doesn't have a unit ('b', 'kb', 'mb', etc.) must be bytes.
duration: now accepts number strings; assumes any string that doesn't have a time format ('ms', 'h', 'd', etc.) must be milliseconds.

Fixes: #54172

elasticmachine · 2020-01-07T18:43:29Z

Pinging @elastic/kibana-security (Team:Security)

jportner · 2020-01-07T21:27:10Z

Renamed this PR to reflect the expanded scope.

Regarding boolean:

The Joi.boolean() API allows 'true' and 'false' by default (case-insensitive).
There is nowhere in Kibana that uses boolean.falsy, boolean.sensitive, boolean.truthy, or the validation convert option -- so it doesn't make sense to add support for any of that. This PR simply changes the Kibana Platform boolean schema to act like the default Joi.boolean() does.

pgayvallet

LGTM for platform changes

Some nits and comments:

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

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

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

pgayvallet · 2020-01-08T07:13:22Z

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

  expect(() => schema.boolean().validate([1, 2, 3])).toThrowErrorMatchingSnapshot();

  expect(() => schema.boolean().validate('abc')).toThrowErrorMatchingSnapshot();
+
+  expect(() => schema.boolean().validate(0)).toThrowErrorMatchingSnapshot();
+
+  expect(() => schema.boolean().validate('no')).toThrowErrorMatchingSnapshot();


Note to self: error messages are short, should replace with inline snapshots in the whole types tests to increase tests readability. (out of the scope of the PR)

Yeah, when we wrote this library we didn't use inline snapshots in Kibana yet (or it wasn't supported by that version of Jest, can't recall already).

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

azasypkin · 2020-01-08T08:49:06Z

ACK: Reviewing...

azasypkin

LGTM, thanks! Just a couple of test related nits and note about updating Readme file.

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

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

azasypkin · 2020-01-08T10:00:06Z

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

  expect(() => schema.boolean().validate([1, 2, 3])).toThrowErrorMatchingSnapshot();

  expect(() => schema.boolean().validate('abc')).toThrowErrorMatchingSnapshot();
+
+  expect(() => schema.boolean().validate(0)).toThrowErrorMatchingSnapshot();
+
+  expect(() => schema.boolean().validate('no')).toThrowErrorMatchingSnapshot();


Yeah, when we wrote this library we didn't use inline snapshots in Kibana yet (or it wasn't supported by that version of Jest, can't recall already).

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

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

kibanamachine · 2020-01-08T19:46:11Z

💚 Build Succeeded

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

History

💚 Build #18776 succeeded 5fd48d8
💚 Build #18756 succeeded c90d6cf
💚 Build #18577 succeeded 89f8964
💔 Build #18554 failed ff2da17
💚 Build #18513 succeeded 056c597

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

…stic#54177) * Update Duration to coerce number strings to numbers (in millis) * Coerce in a way that's consistent with kbn-config-schema * Update ByteSizeValue to coerce strings to numbers * Update Boolean to coerce strings to boolean values * Fix Jest test * Address PR review feedback * Whoops * Whoops 2 * Whoops 3

) (#54299) * Update Duration to coerce number strings to numbers (in millis) * Coerce in a way that's consistent with kbn-config-schema * Update ByteSizeValue to coerce strings to numbers * Update Boolean to coerce strings to boolean values * Fix Jest test * Address PR review feedback * Whoops * Whoops 2 * Whoops 3

* master: (23 commits) [Vis: Default editor] Reactify the timelion editor (elastic#52990) [Discover] fix histogram min interval (elastic#53979) [Telemetry] [Monitoring] Only retry fetching usage once monito… (elastic#54309) [docs][APM] Add runtime index config documentation (elastic#53907) [SIEM] Detection engine timeline (elastic#53783) Filter scripted fields preview field list to source fields (elastic#53826) Management - New platform api (elastic#52579) Reset region and Account when switching inventory (elastic#54287) [SIEM] [Case] Case workflow api schema (elastic#51535) Code coverage setup on CI (elastic#49003) [ML] DF Analytics Results: adds link to docs (elastic#54189) Update schemas boolean, byteSize, and duration to coerce strings (elastic#54177) [Metrics UI] Pass relevant shouldAllowEdit capabilities into SettingsPage (elastic#49781) [Canvas] Fixes bugs with autoplay and refresh (elastic#53149) [ML] DF Analytics Classification: ensure confusion matrix can be fetched (elastic#53629) Fix Vega react eslint errors (elastic#54259) Remove non existing codeowners (elastic#54274) use correct type (elastic#54244) [Dashboard] Removing 100% as dshDashboardViewport height (elastic#54263) add `examples/` to no-restricted-path config (elastic#54252) ...

Update Duration to coerce number strings to numbers (in millis)

8d1bd5e

jportner added chore Team:Security Platform Security: Auth, Users, Roles, Spaces, Audit Logging, etc t// v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.6.0 labels Jan 7, 2020

jportner requested a review from azasypkin January 7, 2020 18:43

jportner requested a review from a team as a code owner January 7, 2020 18:43

jportner added 3 commits January 7, 2020 14:08

Coerce in a way that's consistent with kbn-config-schema

056c597

Update ByteSizeValue to coerce strings to numbers

0e8cea9

Update Boolean to coerce strings to boolean values

ff2da17

jportner changed the title ~~Update Duration to coerce number strings to numbers (in millis)~~ Update schemas Boolean, ByteSizeValue, and Duration to coerce strings Jan 7, 2020

jportner changed the title ~~Update schemas Boolean, ByteSizeValue, and Duration to coerce strings~~ Update schemas boolean, byteSize, and duration to coerce strings Jan 7, 2020

Fix Jest test

89f8964

pgayvallet approved these changes Jan 8, 2020

View reviewed changes

azasypkin approved these changes Jan 8, 2020

View reviewed changes

jportner added 5 commits January 8, 2020 09:40

Merge branch 'master' into issue-54172-duration-string-validation

d3b4d69

Address PR review feedback

c90d6cf

Whoops

4312f73

Whoops 2

5fd48d8

Whoops 3

afed31c

jportner merged commit bbe700d into elastic:master Jan 8, 2020

jportner mentioned this pull request Jan 8, 2020

[7.x] Update schemas boolean, byteSize, and duration to coerce strings (#54177) #54299

Merged

jportner added the backported label Jan 8, 2020

jportner deleted the issue-54172-duration-string-validation branch January 8, 2020 21:08

Conversation

jportner commented Jan 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

elasticmachine commented Jan 7, 2020

Uh oh!

jportner commented Jan 7, 2020

Uh oh!

pgayvallet left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pgayvallet Jan 8, 2020

Choose a reason for hiding this comment

Uh oh!

azasypkin Jan 8, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

azasypkin commented Jan 8, 2020

Uh oh!

azasypkin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

azasypkin Jan 8, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kibanamachine commented Jan 8, 2020

💚 Build Succeeded

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jportner commented Jan 7, 2020 •

edited

Loading