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

fix(stepfunctions): renderObject ignores explicit null #17098

Closed
wants to merge 5 commits into from
Closed

fix(stepfunctions): renderObject ignores explicit null #17098

wants to merge 5 commits into from

Conversation

kaizencc
Copy link
Contributor

@kaizencc kaizencc commented Oct 21, 2021

Fixes #16253.

The stepfunctions module has a function renderObject that deep renders a JSON object to expand JSON path fields. Previously, renderObject handled null values and undefined values the same. However, null and undefined have different meanings in Typescript and it is possible for a user to want to pass an explicit null into the JSON path. For example:

FieldUtils.renderObject({
  reference1: {nullParameter: null},
  reference2: {notNullParameter: 'not null'},
})

Previously would result in:

{
  reference2: {notNullParameter: 'not null'}
}

which completely ignores the null parameter. Now, this results in:

{
  reference1: {nullParameter: null},
  reference2: {notNullParameter: 'not null'}
}

which is the expected behavior.

This PR builds on what was started in #16721.


There is also a bug (I believe) in how we specify notificationProperty for GlueStartJobRun and RunGlueJobTask in aws-stepfunctions-tasks. The default value is specified as null and not undefined, which I think is typically how we specify non-existent properties in Typescript.

I am not sure if notificationProperty is some sort of exception to this rule ❗, but I have looked at the documentation for it and nothing seems to suggest that the null value was intentional. It was introduced in this PR as well as this one.


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@gitpod-io
Copy link

gitpod-io bot commented Oct 21, 2021

@github-actions github-actions bot added the @aws-cdk/aws-stepfunctions Related to AWS StepFunctions label Oct 21, 2021
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Oct 21, 2021
@kaizencc kaizencc requested a review from a team October 21, 2021 17:16
Copy link
Contributor

@rix0rrr rix0rrr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, null and undefined have different meanings in Typescript and it is possible for a user to want to pass an explicit null into the JSON path.

Yes but no. There is a difference between null and undefined in JavaScript, but there is not a difference between them in jsii (because Java does not have this distinction, Python does not have this distinction, etc).

So we can not rely on the fact that they are two different values in our TypeScript code.

The only correct way to deal with this is to change the API in such a way that the user has another ability to specify a null (maybe an additional property, maybe a magic value) -- and it would be better to name it after the effect of specifying a null, something like DROP_STATE or something (not necessarily a great name, but I just want to indicate that it shouldn't be SPECIFY_NULL: that doesn't tell anybody anything).

@kaizencc
Copy link
Contributor Author

kaizencc commented Nov 8, 2021

Sounds like a plan. I'm actually going to p2 this fix and leave it hanging for now. It doesn't really seem like that big of a miss now that the limitations are explained. I might circle back to it if it seems like more customers are providing +1's to the original issue: #16253.

@kaizencc kaizencc added effort/small Small work item – less than a day of effort p2 labels Nov 8, 2021
@kaizencc kaizencc removed their assignment Feb 17, 2022
@kaizencc kaizencc closed this Feb 17, 2022
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
  • Commit ID: ec4beb7
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@krish00back
Copy link

I am facing this issue.

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 contribution/core This is a PR that came from AWS. effort/small Small work item – less than a day of effort p2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(stepfunctions-tasks): LambdaInvoke - explicit null is not passed to lambda when using
5 participants