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

Author: apalumbo

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 {

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