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

Python None maps to JS/TS undefined instead of null #3999

Closed
beck3905 opened this issue Mar 8, 2023 · 2 comments · Fixed by aws/aws-cdk#24593
Closed

Python None maps to JS/TS undefined instead of null #3999

beck3905 opened this issue Mar 8, 2023 · 2 comments · Fixed by aws/aws-cdk#24593
Labels
bug This issue is a bug. needs-triage This issue or PR still needs to be triaged.

Comments

@beck3905
Copy link

beck3905 commented Mar 8, 2023

Describe the bug

It seems that JSII may be interpreting a Python None value or NoneType object as undefined rather than null. I am working with Step Functions state machines in the CDK in Python and when I use result_path=JsonPath.DISCARD this gets set to "ResultPath": None in the resulting state machine definition as a Python dictionary. But then either calling stack.resolve or just during the resolution process during cdk synth the ResultPath property get's removed entirely. I can see here where resolve removed properties with undefined value. https://github.com/aws/aws-cdk/blob/main/packages/%40aws-cdk/core/lib/private/resolve.ts#L222. But when converting from code to JSON, null is a valid value, which Python's None value usually maps to.

Expected Behavior

When creating a Step Function state with result_path=JsonPath.DISCARD I expect the resulting state machine definition to include "ResultPath": null.

Current Behavior

Currently, if I create a state with result_path=JsonPath.DISCARD the resulting state machine definition does not include the ResultPath field at all.

Reproduction Steps

from aws_cdk import Stack
from aws_cdk.aws_stepfunctions import JsonPath, Map, Pass, StateGraph
from constructs import Construct


class CdkMapStack(Stack):
    def __init__(self, scope: Construct, construct_id: str, **kwargs) -> None:
        super().__init__(scope, construct_id, **kwargs)

        map_state = Map(
            self,
            "Map",
            max_concurrency=1,
            items_path=JsonPath.string_at("$.inputForMap"),
            parameters={"foo": "foo", "bar": JsonPath.string_at("$.bar")},
            result_path=JsonPath.DISCARD,
        )
        map_state.iterator(Pass(self, "Pass State"))

        print(map_state.to_state_json(), file=open("./cdk.out/map-state.json", "w"))

        graph = StateGraph(map_state.start_state, "Test Graph")

        graph_json = graph.to_graph_json()

        print(
            graph_json,
            file=open("./cdk.out/state-graph.json", "w"),
        )

        print(
            self.resolve(graph_json),
            file=open("./cdk.out/state-graph-resolved.json", "w"),
        )

Run cdk synth with the stack above. Inspect the 3 files created in the cdk.out directory. The map-state.json and state-graph.json files will show the "ResultPath": null, but the state-graph-resolved.json file will not include the ResultPath field on the Map state at all.

Possible Solution

I'm not entirely sure of a solution as in some contexts it makes sense for Python None values to map to undefined and other times it makes sense for it to map to null. I don't believe Python has a way to distinguish the 2. Perhaps the CDK or JSII need an enum or constant to represent null in those contexts where null is valid and then None will always be treated as undefined.

Additional Information/Context

I have noticed this on multiple state types now, but both related to a Map state. I've found this behavior on both the Map state itself and when I add result_path=JsonPath.DISCARD to a task state within the Map state iterator. Neither had the ResultPath field in the synthesized/resolved state machine definition.

SDK version used

2.68.0 (build 25fda51)

Environment details (OS name and version, etc.)

Mac OS 13.2.1, Python 3.9.6

@beck3905 beck3905 added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Mar 8, 2023
@RomainMuller
Copy link
Contributor

Jsii APIs cannot distinguish between null and undefined by design, as most languages don't have a distinction between the two. The fix for this issue lies in the CDK, where a special JsonNull value needs to be used.

@github-actions
Copy link
Contributor

⚠️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.

mergify bot pushed a commit to aws/aws-cdk that referenced this issue Mar 13, 2023
If any part of a state's JSON representation is `null`, that value will be replaced by `undefined` when jsii sends data to the other language, resulting in a change of semantics.

Multi-language APIs cannot differentiate between `null` and `undefined` as non-JS languages typically fail to distinguish between them... In order to address that, a `JsonNull` value was added which serializes to `null` (via Javascript's standard `toJSON` method), which must be used in such cases where `null` may need to cross the language boundary.

The `JsonPath.DISCARD` value is now a string-token representation of the `JsonNull` instance.

Fixes #14639
Fixes aws/jsii#3999

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
homakk pushed a commit to homakk/aws-cdk that referenced this issue Mar 28, 2023
If any part of a state's JSON representation is `null`, that value will be replaced by `undefined` when jsii sends data to the other language, resulting in a change of semantics.

Multi-language APIs cannot differentiate between `null` and `undefined` as non-JS languages typically fail to distinguish between them... In order to address that, a `JsonNull` value was added which serializes to `null` (via Javascript's standard `toJSON` method), which must be used in such cases where `null` may need to cross the language boundary.

The `JsonPath.DISCARD` value is now a string-token representation of the `JsonNull` instance.

Fixes aws#14639
Fixes aws/jsii#3999

----

*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
bug This issue is a bug. needs-triage This issue or PR still needs to be triaged.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants