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

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

Merged
merged 1 commit into from
Jan 8, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
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);
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);
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;
}

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();
});
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', () => {
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();

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