From bbe03800bc25f49965a08b2e14c97d8e8ec3fc65 Mon Sep 17 00:00:00 2001 From: apalumbo Date: Mon, 21 Oct 2019 12:23:42 +0200 Subject: [PATCH] fix(stepfunctions): map state validation fix (#4382) * fix(stepfunctions) Fixed validation for missing iterator for Map state, added synth unit tests * feat(stepfunctions): Added test and validation for maxConcurrency, that has to be a positive integer * refactor(stepfunctions): Refactored Map test to reduce code duplication * refactor(stepfunctions): extracted integer validation outside map, added tests * fix(stepfunctions) Fixed map test and isPositiveInteger --- .../aws-stepfunctions/lib/states/map.ts | 28 +++- .../aws-stepfunctions/test/test.map.ts | 121 ++++++++++++++++++ 2 files changed, 145 insertions(+), 4 deletions(-) diff --git a/packages/@aws-cdk/aws-stepfunctions/lib/states/map.ts b/packages/@aws-cdk/aws-stepfunctions/lib/states/map.ts index 6e6301eb1e6e7..c2a951246439d 100644 --- a/packages/@aws-cdk/aws-stepfunctions/lib/states/map.ts +++ b/packages/@aws-cdk/aws-stepfunctions/lib/states/map.ts @@ -70,6 +70,19 @@ export interface MapProps { readonly maxConcurrency?: number; } +/** + * Returns true if the value passed is a positive integer + * @param value the value ti validate + */ + +export const isPositiveInteger = (value: number) => { + const isFloat = Math.floor(value) !== value; + + const isNotPositiveInteger = value < 0 || value > Number.MAX_SAFE_INTEGER; + + return !isFloat && !isNotPositiveInteger; +}; + /** * Define a Map state in the state machine * @@ -80,7 +93,7 @@ export interface MapProps { export class Map extends State implements INextable { public readonly endStates: INextable[]; - private readonly maxConcurrency?: number; + private readonly maxConcurrency: number | undefined; private readonly itemsPath?: string; constructor(scope: cdk.Construct, id: string, props: MapProps = {}) { @@ -150,10 +163,17 @@ export class Map extends State implements INextable { * Validate this state */ protected validate(): string[] { - if (!!this.iterator) { - return ['Map state must have a non-empty iterator']; + const errors: string[] = []; + + if (this.iteration === undefined) { + errors.push('Map state must have a non-empty iterator'); } - return []; + + if (this.maxConcurrency !== undefined && !isPositiveInteger(this.maxConcurrency)) { + errors.push('maxConcurrency has to be a positive integer'); + } + + return errors; } private renderItemsPath(): any { diff --git a/packages/@aws-cdk/aws-stepfunctions/test/test.map.ts b/packages/@aws-cdk/aws-stepfunctions/test/test.map.ts index ab6fa9e61cbdc..7d0b30757feb3 100644 --- a/packages/@aws-cdk/aws-stepfunctions/test/test.map.ts +++ b/packages/@aws-cdk/aws-stepfunctions/test/test.map.ts @@ -1,6 +1,7 @@ import cdk = require('@aws-cdk/core'); import { Test } from 'nodeunit'; import stepfunctions = require('../lib'); +import { isPositiveInteger } from '../lib'; export = { 'State Machine With Map State'(test: Test) { @@ -42,9 +43,129 @@ export = { }); test.done(); + }, + 'synth is successful'(test: Test) { + + const app = createAppWithMap((stack) => { + const map = new stepfunctions.Map(stack, 'Map State', { + maxConcurrency: 1, + itemsPath: stepfunctions.Data.stringAt('$.inputForMap') + }); + map.iterator(new stepfunctions.Pass(stack, 'Pass State')); + return map; + }); + + app.synth(); + test.done(); + }, + 'fails in synthesis if iterator is missing'(test: Test) { + + const app = createAppWithMap((stack) => { + const map = new stepfunctions.Map(stack, 'Map State', { +             maxConcurrency: 1, + itemsPath: stepfunctions.Data.stringAt('$.inputForMap') + }); + + return map; + }); + +        test.throws(() => { +            app.synth(); +        }, /Map state must have a non-empty iterator/, 'A validation was expected'); + +        test.done(); +    }, + 'fails in synthesis when maxConcurrency is a float'(test: Test) { + + const app = createAppWithMap((stack) => { +            const map = new stepfunctions.Map(stack, 'Map State', { +                maxConcurrency: 1.2, +                itemsPath: stepfunctions.Data.stringAt('$.inputForMap') +            }); +            map.iterator(new stepfunctions.Pass(stack, 'Pass State')); + + return map; + }); + +        test.throws(() => { +            app.synth(); +        }, /maxConcurrency has to be a positive integer/, 'A validation was expected'); + +        test.done(); + +    }, +    'fails in synthesis when maxConcurrency is a negative integer'(test: Test) { + + const app = createAppWithMap((stack) => { +            const map = new stepfunctions.Map(stack, 'Map State', { +                maxConcurrency: -1, +                itemsPath: stepfunctions.Data.stringAt('$.inputForMap') +            }); +            map.iterator(new stepfunctions.Pass(stack, 'Pass State')); + + return map; + }); + +        test.throws(() => { +            app.synth(); +        }, /maxConcurrency has to be a positive integer/, 'A validation was expected'); + +        test.done(); +    }, +    'fails in synthesis when maxConcurrency is too big to be an integer'(test: Test) { + + const app = createAppWithMap((stack) => { +            const map = new stepfunctions.Map(stack, 'Map State', { +             maxConcurrency: Number.MAX_VALUE, +             itemsPath: stepfunctions.Data.stringAt('$.inputForMap') +            }); +            map.iterator(new stepfunctions.Pass(stack, 'Pass State')); + + return map; + }); + +        test.throws(() => { +            app.synth(); +        }, /maxConcurrency has to be a positive integer/, 'A validation was expected'); + +        test.done(); + +    }, + 'isPositiveInteger is false with negative number'(test: Test) { + test.equals(isPositiveInteger(-1), false, '-1 is not a valid positive integer'); +        test.done(); + }, + 'isPositiveInteger is false with decimal number'(test: Test) { + test.equals(isPositiveInteger(1.2), false, '1.2 is not a valid positive integer'); +        test.done(); + }, + 'isPositiveInteger is false with a value greater than safe integer '(test: Test) { + const valueToTest = Number.MAX_SAFE_INTEGER + 1; + test.equals(isPositiveInteger(valueToTest), false, `${valueToTest} is not a valid positive integer`); +        test.done(); + }, + 'isPositiveInteger is true with 0'(test: Test) { + test.equals(isPositiveInteger(0), true, '0 is expected to be a positive integer'); +        test.done(); + }, + 'isPositiveInteger is true with 10'(test: Test) { + test.equals(isPositiveInteger(10), true, '10 is expected to be a positive integer'); +        test.done(); + }, + 'isPositiveInteger is true with max integer value'(test: Test) { + test.equals(isPositiveInteger(Number.MAX_SAFE_INTEGER), true, `${Number.MAX_SAFE_INTEGER} is expected to be a positive integer`); +        test.done(); } }; function render(sm: stepfunctions.IChainable) { return new cdk.Stack().resolve(new stepfunctions.StateGraph(sm.startState, 'Test Graph').toGraphJson()); } + +function createAppWithMap(mapFactory: (stack: cdk.Stack) => stepfunctions.Map) { +    const app = new cdk.App(); + const stack = new cdk.Stack(app, 'my-stack'); + const map = mapFactory(stack); +    new stepfunctions.StateGraph(map, 'Test Graph'); + return app; +}