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

[aws-stepfunctions] recurseObject skips repeated object references even when they're not causing a cycle #14596

Closed
marciocarmona opened this issue May 8, 2021 · 1 comment · Fixed by #14628
Assignees
Labels
@aws-cdk/aws-stepfunctions Related to AWS StepFunctions

Comments

@marciocarmona
Copy link
Contributor

marciocarmona commented May 8, 2021

While writing a StepFunction (LambdaInvoke) I noticed part of my input was just missing from the StateMachine step definition.

Reproduction Steps

For example, if I do this:

        let x = {
            foo: "Hello world"
        }
        let input = {
            value1: x,
            value2: x
        }
        console.log(FieldUtils.renderObject(input))

I get this output:

{ value1: { foo: 'Hello world' }, value2: {} }

What did you expect to happen?

It should have returned:

{ value1: { foo: 'Hello world' }, value2: { foo: 'Hello world' } }

What actually happened?

It returned:

{ value1: { foo: 'Hello world' }, value2: {} }

Environment

  • CDK CLI Version : 1.98.0 (build 79f4512)
  • Framework Version: ?
  • Node.js Version: v12.22.1
  • OS : Linux Red Hat 7.2.1-2
  • Language (Version): TypeScript (4.0.5)

Other

After doing some investigation I found that the issue happens in the FieldUtils.renderObject call, more specifically when it calls the recurseObject function.

The problem seems to happen because the recurseObject pushes all nodes to a visited list as it traverses them to avoid any cycles, however, it doesn't pop them afterwards, so repeated objects are skipped even if they're not in the same subtree, i.e. there's no circular reference.

Code reference:

As a fix the recurseObject should either pop the visited array before returning, or actually send a copy of the visited nodes in the recursive calls, like [...visited, obj].


This is 🐛 Bug Report

@marciocarmona marciocarmona changed the title recurseObject skips repeated object references even when they're not causing a cycle [aws-stepfunctions] recurseObject skips repeated object references even when they're not causing a cycle May 8, 2021
@github-actions github-actions bot added the @aws-cdk/aws-stepfunctions Related to AWS StepFunctions label May 8, 2021
marciocarmona pushed a commit to marciocarmona/aws-cdk that referenced this issue May 11, 2021
…be allowed if not causing a circular reference

fixes aws#14596
@mergify mergify bot closed this as completed in #14628 Jun 7, 2021
mergify bot pushed a commit that referenced this issue Jun 7, 2021
…t a circular reference (#14628)

Popping the current node from `visited` array after finishing visiting it in `recurseObject` method. This allows another reference to the same object to happen on a different object path (currently being swallowed).

Created unit test before making any code changes to verify the failure scenario on the expected behavior before fixing it.

fixes #14596 

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@github-actions
Copy link

github-actions bot commented Jun 7, 2021

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

hollanddd pushed a commit to hollanddd/aws-cdk that referenced this issue Aug 26, 2021
…t a circular reference (aws#14628)

Popping the current node from `visited` array after finishing visiting it in `recurseObject` method. This allows another reference to the same object to happen on a different object path (currently being swallowed).

Created unit test before making any code changes to verify the failure scenario on the expected behavior before fixing it.

fixes aws#14596 

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-stepfunctions Related to AWS StepFunctions
Projects
None yet
2 participants