Skip to content

Commit

Permalink
fix(stepfunctions): non-object arguments to recurseObject are incorre…
Browse files Browse the repository at this point in the history
…ctly treated as objects (#14631)

This doesn't actually fix the issue #12935 as currently the Json paths won't be resolved for Lambda steps where the `Resource` is the Lambda ARN and not `arn:aws:states:::lambda:invoke`, but it at least fixes the issue for Text inputs when `payloadResponseOnly: true` and will avoid the same error from happening again if the `recurseObject` is called with a value that's not an object.

Ideally the `TaskInput.value` field should be changed to `{ [key: string]: any } | string` here to ensure the type check when sending the value to methods like `FieldUtils.renderObject`:
https://github.com/aws/aws-cdk/blob/master/packages/@aws-cdk/aws-stepfunctions/lib/input.ts#L65

Or even better the `TaskInput` should be made generic like:
```
export class TaskInput<T extends InputType> {
  ...
  private constructor(public readonly type: T, public readonly value: ValueType[T]) {}
}

type ValueType = {
  [InputType.OBJECT]: { [key: string]: any },
  [InputType.TEXT]: string
}
```
However, any of the changes above wouldn't be backwards compatible and could break not only internal references in the `aws-cdk` but also on any customer packages using the CDK.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
marciocarmona authored Jul 23, 2021
1 parent cb6e7c9 commit e133bca
Show file tree
Hide file tree
Showing 2 changed files with 40 additions and 2 deletions.
7 changes: 6 additions & 1 deletion packages/@aws-cdk/aws-stepfunctions/lib/json-path.ts
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,12 @@ interface FieldHandlers {
}

export function recurseObject(obj: object | undefined, handlers: FieldHandlers, visited: object[] = []): object | undefined {
if (obj === undefined) { return undefined; }
// If the argument received is not actually an object (string, number, boolean, undefined, ...) or null
// just return it as is as there's nothing to be rendered. This should only happen in the original call to
// recurseObject as any recursive calls to it are checking for typeof value === 'object' && value !== null
if (typeof obj !== 'object' || obj === null) {
return obj;
}

// Avoiding infinite recursion
if (visited.includes(obj)) { return {}; }
Expand Down
35 changes: 34 additions & 1 deletion packages/@aws-cdk/aws-stepfunctions/test/fields.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import '@aws-cdk/assert-internal/jest';
import { FieldUtils, JsonPath } from '../lib';
import { FieldUtils, JsonPath, TaskInput } from '../lib';

describe('Fields', () => {
const jsonPathValidationErrorMsg = /exactly '\$', '\$\$', start with '\$.', start with '\$\$.' or start with '\$\['/;
Expand Down Expand Up @@ -153,6 +153,39 @@ describe('Fields', () => {
.toStrictEqual(['$.listField', '$.numField', '$.stringField']);
});

test('rendering a non-object value should just return itself', () => {
expect(
FieldUtils.renderObject(TaskInput.fromText('Hello World').value),
).toEqual(
'Hello World',
);
expect(
FieldUtils.renderObject('Hello World' as any),
).toEqual(
'Hello World',
);
expect(
FieldUtils.renderObject(null as any),
).toEqual(
null,
);
expect(
FieldUtils.renderObject(3.14 as any),
).toEqual(
3.14,
);
expect(
FieldUtils.renderObject(true as any),
).toEqual(
true,
);
expect(
FieldUtils.renderObject(undefined),
).toEqual(
undefined,
);
});

test('repeated object references at different tree paths should not be considered as recursions', () => {
const repeatedObject = {
field: JsonPath.stringAt('$.stringField'),
Expand Down

0 comments on commit e133bca

Please sign in to comment.