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.

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

Update schemas boolean, byteSize, and duration to coerce strings #54177

Merged
7 changes: 5 additions & 2 deletions packages/kbn-config-schema/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,9 @@ __Usage:__
const valueSchema = schema.boolean({ defaultValue: false });
```

__Notes:__
* The `schema.boolean()` also supports a string as input if it equals `'true'` or `'false'` (case-insensitive).

#### `schema.literal()`

Validates input data as a [string](https://www.typescriptlang.org/docs/handbook/advanced-types.html#string-literal-types), [numeric](https://www.typescriptlang.org/docs/handbook/advanced-types.html#numeric-literal-types) or boolean literal.
Expand Down Expand Up @@ -397,7 +400,7 @@ const valueSchema = schema.byteSize({ min: '3kb' });
```

__Notes:__
* The string value for `schema.byteSize()` and its options supports the following prefixes: `b`, `kb`, `mb`, `gb` and `tb`.
* The string value for `schema.byteSize()` and its options supports the following optional suffixes: `b`, `kb`, `mb`, `gb` and `tb`. The default suffix is `b`.
* The number value is treated as a number of bytes and hence should be a positive integer, e.g. `100` is equal to `'100b'`.
* Currently you cannot specify zero bytes with a string format and should use number `0` instead.

Expand All @@ -417,7 +420,7 @@ const valueSchema = schema.duration({ defaultValue: '70ms' });
```

__Notes:__
* The string value for `schema.duration()` supports the following prefixes: `ms`, `s`, `m`, `h`, `d`, `w`, `M` and `Y`.
* The string value for `schema.duration()` supports the following optional suffixes: `ms`, `s`, `m`, `h`, `d`, `w`, `M` and `Y`. The default suffix is `ms`.
* The number value is treated as a number of milliseconds and hence should be a positive integer, e.g. `100` is equal to `'100ms'`.

#### `schema.conditional()`
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 5 additions & 5 deletions packages/kbn-config-schema/src/byte_size_value/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,10 @@
import { ByteSizeValue } from '.';

describe('parsing units', () => {
test('number string (bytes)', () => {
expect(ByteSizeValue.parse('123').getValueInBytes()).toBe(123);
});

test('bytes', () => {
expect(ByteSizeValue.parse('123b').getValueInBytes()).toBe(123);
});
Expand All @@ -37,12 +41,8 @@ describe('parsing units', () => {
expect(ByteSizeValue.parse('1gb').getValueInBytes()).toBe(1073741824);
});

test('throws an error when no unit specified', () => {
expect(() => ByteSizeValue.parse('123')).toThrowError('could not parse byte size value');
});

test('throws an error when unsupported unit specified', () => {
expect(() => ByteSizeValue.parse('1tb')).toThrowError('could not parse byte size value');
expect(() => ByteSizeValue.parse('1tb')).toThrowErrorMatchingSnapshot();
});
});

Expand Down
14 changes: 9 additions & 5 deletions packages/kbn-config-schema/src/byte_size_value/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,14 @@ export class ByteSizeValue {
public static parse(text: string): ByteSizeValue {
const match = /([1-9][0-9]*)(b|kb|mb|gb)/.exec(text);
if (!match) {
throw new Error(
`could not parse byte size value [${text}]. Value must be a safe positive integer.`
);
const number = Number(text);
jportner marked this conversation as resolved.
Show resolved Hide resolved
if (typeof number !== 'number' || isNaN(number)) {
throw new Error(
`Failed to parse [${text}] as byte value. Value must be either number of bytes, or follow the format <count>[b|kb|mb|gb] ` +
`(e.g., '1024kb', '200mb', '1gb'), where the number is a safe positive integer.`
);
}
return new ByteSizeValue(number);
}

const value = parseInt(match[1], 0);
Expand All @@ -49,8 +54,7 @@ export class ByteSizeValue {
constructor(private readonly valueInBytes: number) {
if (!Number.isSafeInteger(valueInBytes) || valueInBytes < 0) {
throw new Error(
`Value in bytes is expected to be a safe positive integer, ` +
`but provided [${valueInBytes}]`
`Value in bytes is expected to be a safe positive integer, but provided [${valueInBytes}].`
);
}
}
Expand Down
15 changes: 9 additions & 6 deletions packages/kbn-config-schema/src/duration/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,14 @@ const timeFormatRegex = /^(0|[1-9][0-9]*)(ms|s|m|h|d|w|M|Y)$/;
function stringToDuration(text: string) {
const result = timeFormatRegex.exec(text);
if (!result) {
throw new Error(
`Failed to parse [${text}] as time value. ` +
`Format must be <count>[ms|s|m|h|d|w|M|Y] (e.g. '70ms', '5s', '3d', '1Y')`
);
const number = Number(text);
jportner marked this conversation as resolved.
Show resolved Hide resolved
if (typeof number !== 'number' || isNaN(number)) {
throw new Error(
`Failed to parse [${text}] as time value. Value must be a duration in milliseconds, or follow the format ` +
`<count>[ms|s|m|h|d|w|M|Y] (e.g. '70ms', '5s', '3d', '1Y'), where the duration is a safe positive integer.`
);
}
return numberToDuration(number);
}

const count = parseInt(result[1], 0);
Expand All @@ -40,8 +44,7 @@ function stringToDuration(text: string) {
function numberToDuration(numberMs: number) {
if (!Number.isSafeInteger(numberMs) || numberMs < 0) {
throw new Error(
`Failed to parse [${numberMs}] as time value. ` +
`Value should be a safe positive integer number.`
`Value in milliseconds is expected to be a safe positive integer, but provided [${numberMs}].`
);
}

Expand Down
18 changes: 17 additions & 1 deletion packages/kbn-config-schema/src/internals/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,23 @@ export const internals = Joi.extend([
base: Joi.boolean(),
coerce(value: any, state: State, options: ValidationOptions) {
// If value isn't defined, let Joi handle default value if it's defined.
if (value !== undefined && typeof value !== 'boolean') {
if (value === undefined) {
return value;
}

// Allow strings 'true' and 'false' to be coerced to booleans (case-insensitive).

// From Joi docs on `Joi.boolean`:
// > Generates a schema object that matches a boolean data type. Can also
// > be called via bool(). If the validation convert option is on
// > (enabled by default), a string (either "true" or "false") will be
// converted to a boolean if specified.
if (typeof value === 'string') {
const normalized = value.toLowerCase();
value = normalized === 'true' ? true : normalized === 'false' ? false : value;
jportner marked this conversation as resolved.
Show resolved Hide resolved
}

if (typeof value !== 'boolean') {
return this.createError('boolean.base', { value }, state, options);
}

Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

15 changes: 15 additions & 0 deletions packages/kbn-config-schema/src/types/boolean_type.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,17 @@ test('returns value by default', () => {
expect(schema.boolean().validate(true)).toBe(true);
});

test('handles boolean strings', () => {
expect(schema.boolean().validate('true')).toBe(true);
expect(schema.boolean().validate('TRUE')).toBe(true);
expect(schema.boolean().validate('True')).toBe(true);
expect(schema.boolean().validate('TrUe')).toBe(true);
expect(schema.boolean().validate('false')).toBe(false);
expect(schema.boolean().validate('FALSE')).toBe(false);
expect(schema.boolean().validate('False')).toBe(false);
expect(schema.boolean().validate('FaLse')).toBe(false);
});

test('is required by default', () => {
expect(() => schema.boolean().validate(undefined)).toThrowErrorMatchingSnapshot();
});
Expand All @@ -49,4 +60,8 @@ test('returns error when not boolean', () => {
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();
Comment on lines 60 to +66
Copy link
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Member

Choose a reason for hiding this comment

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

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

});
24 changes: 22 additions & 2 deletions packages/kbn-config-schema/src/types/byte_size_type.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,15 @@ import { ByteSizeValue } from '../byte_size_value';
const { byteSize } = schema;

test('returns value by default', () => {
expect(byteSize().validate('123b')).toMatchSnapshot();
expect(byteSize().validate('123b')).toEqual(new ByteSizeValue(123));
});

test('handles numeric strings', () => {
expect(byteSize().validate('123')).toEqual(new ByteSizeValue(123));
});

test('handles numbers', () => {
expect(byteSize().validate(123)).toEqual(new ByteSizeValue(123));
});

test('is required by default', () => {
Expand Down Expand Up @@ -51,6 +59,14 @@ describe('#defaultValue', () => {
).toMatchSnapshot();
});

test('can be a string-formatted number', () => {
expect(
byteSize({
defaultValue: '1024',
}).validate(undefined)
).toMatchSnapshot();
});

test('can be a number', () => {
expect(
byteSize({
Expand Down Expand Up @@ -88,7 +104,7 @@ describe('#max', () => {
});
});

test('returns error when not string or positive safe integer', () => {
test('returns error when not valid string or positive safe integer', () => {
expect(() => byteSize().validate(-123)).toThrowErrorMatchingSnapshot();

expect(() => byteSize().validate(NaN)).toThrowErrorMatchingSnapshot();
Expand All @@ -100,4 +116,8 @@ test('returns error when not string or positive safe integer', () => {
expect(() => byteSize().validate([1, 2, 3])).toThrowErrorMatchingSnapshot();

expect(() => byteSize().validate(/abc/)).toThrowErrorMatchingSnapshot();

expect(() => byteSize().validate('123foo')).toThrowErrorMatchingSnapshot();

expect(() => byteSize().validate('123 456')).toThrowErrorMatchingSnapshot();
});
24 changes: 22 additions & 2 deletions packages/kbn-config-schema/src/types/duration_type.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,15 @@ import { schema } from '..';
const { duration, object, contextRef, siblingRef } = schema;

test('returns value by default', () => {
expect(duration().validate('123s')).toMatchSnapshot();
expect(duration().validate('123s')).toEqual(momentDuration(123000));
});

test('handles numeric string', () => {
expect(duration().validate('123000')).toEqual(momentDuration(123000));
});

test('handles number', () => {
expect(duration().validate(123000)).toEqual(momentDuration(123000));
});

test('is required by default', () => {
Expand Down Expand Up @@ -51,6 +59,14 @@ describe('#defaultValue', () => {
).toMatchSnapshot();
});

test('can be a string-formatted number', () => {
jportner marked this conversation as resolved.
Show resolved Hide resolved
expect(
duration({
defaultValue: '600',
}).validate(undefined)
).toMatchSnapshot();
});

test('can be a number', () => {
expect(
duration({
Expand Down Expand Up @@ -124,7 +140,7 @@ Object {
});
});

test('returns error when not string or non-safe positive integer', () => {
test('returns error when not valid string or non-safe positive integer', () => {
expect(() => duration().validate(-123)).toThrowErrorMatchingSnapshot();

expect(() => duration().validate(NaN)).toThrowErrorMatchingSnapshot();
Expand All @@ -136,4 +152,8 @@ test('returns error when not string or non-safe positive integer', () => {
expect(() => duration().validate([1, 2, 3])).toThrowErrorMatchingSnapshot();

expect(() => duration().validate(/abc/)).toThrowErrorMatchingSnapshot();

expect(() => duration().validate('123foo')).toThrowErrorMatchingSnapshot();
jportner marked this conversation as resolved.
Show resolved Hide resolved

expect(() => duration().validate('123 456')).toThrowErrorMatchingSnapshot();
});
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ describe('params validation', () => {
);

expect(() => {
validateParams(actionType, { refresh: 'true' });
validateParams(actionType, { refresh: 'foo' });
}).toThrowErrorMatchingInlineSnapshot(
`"error validating action params: [refresh]: expected value of type [boolean] but got [string]"`
);
Expand Down