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 generated construct failure in deserialization - AVP L2 CDK Constructs #4593

Closed
reste85 opened this issue Aug 1, 2024 · 8 comments
Closed
Labels
bug This issue is a bug. needs-triage This issue or PR still needs to be triaged.

Comments

@reste85
Copy link

reste85 commented Aug 1, 2024

Describe the bug

Hello everyone,
I'm one of the authors of https://github.com/cdklabs/cdk-verified-permissions, which are a set of L2 CDK Constructs for Amazon Verified Permissions. I'm experiencing an error while using that constructs library in Python especially the PolicyStore construct. You can see the definition of that construct here: https://github.com/cdklabs/cdk-verified-permissions/blob/main/src/policy-store.ts. The main point is that when i try to create a PolicyStore providing validation_settings argument as Python dict i receive a deserialization error. It seems that the property is not recognized even if i'm passing a dictionary containing all the required elements

Expected Behavior

Create a PolicyStore without problems and with the minimum required parameters

Current Behavior

Deserialization error:

jsii.errors.JavaScriptError: 
  @jsii/kernel.SerializationError: Passed to parameter props of new @cdklabs/cdk-verified-permissions.PolicyStore: Unable to deserialize value as @cdklabs/cdk-verified-permissions.PolicyStoreProps | undefined
  ├── 🛑 Failing value is an object
  │      { '$jsii.struct': [Object] }
  ╰── 🔍 Failure reason(s):
      ╰─ Key 'validationSettings': Unable to deserialize value as @cdklabs/cdk-verified-permissions.IValidationSettings
          ├── 🛑 Failing value is an object
          │      { '$jsii.map': [Object] }
          ╰── 🔍 Failure reason(s):
              ╰─ Value does not have the "$jsii.byref" key
      at Object.process (/private/var/folders/dw/ht_ktppj05g2t91hp33zvjr00000gr/T/tmpjqea4h9e/lib/program.js:11477:19)
      at Kernel._Kernel_toSandbox (/private/var/folders/dw/ht_ktppj05g2t91hp33zvjr00000gr/T/tmpjqea4h9e/lib/program.js:10455:25)
      at /private/var/folders/dw/ht_ktppj05g2t91hp33zvjr00000gr/T/tmpjqea4h9e/lib/program.js:10471:38
      at Array.map (<anonymous>)
      at Kernel._Kernel_boxUnboxParameters (/private/var/folders/dw/ht_ktppj05g2t91hp33zvjr00000gr/T/tmpjqea4h9e/lib/program.js:10471:23)
      at Kernel._Kernel_toSandboxValues (/private/var/folders/dw/ht_ktppj05g2t91hp33zvjr00000gr/T/tmpjqea4h9e/lib/program.js:10459:101)
      at Kernel._Kernel_create (/private/var/folders/dw/ht_ktppj05g2t91hp33zvjr00000gr/T/tmpjqea4h9e/lib/program.js:10119:115)
      at Kernel.create (/private/var/folders/dw/ht_ktppj05g2t91hp33zvjr00000gr/T/tmpjqea4h9e/lib/program.js:9790:93)
      at KernelHost.processRequest (/private/var/folders/dw/ht_ktppj05g2t91hp33zvjr00000gr/T/tmpjqea4h9e/lib/program.js:11707:36)
      at KernelHost.run (/private/var/folders/dw/ht_ktppj05g2t91hp33zvjr00000gr/T/tmpjqea4h9e/lib/program.js:11667:22)

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "cdk-verified-permissions-l2-test-python/app.py", line 10, in <module>
    CdkVerifiedPermissionsL2TestPythonStack(app, "CdkVerifiedPermissionsL2TestPythonStack",
  File "cdk-verified-permissions-l2-test-python/.venv/lib/python3.12/site-packages/jsii/_runtime.py", line 118, in __call__
    inst = super(JSIIMeta, cast(JSIIMeta, cls)).__call__(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "cdk-verified-permissions-l2-test-python/cdk_verified_permissions_l2_test_python/cdk_verified_permissions_l2_test_python_stack.py", line 21, in __init__
    test = avp.PolicyStore(
           ^^^^^^^^^^^^^^^^
  File "cdk-verified-permissions-l2-test-python/.venv/lib/python3.12/site-packages/jsii/_runtime.py", line 118, in __call__
    inst = super(JSIIMeta, cast(JSIIMeta, cls)).__call__(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "cdk-verified-permissions-l2-test-python/.venv/lib/python3.12/site-packages/cdklabs/cdk_verified_permissions/__init__.py", line 2333, in __init__
    jsii.create(self.__class__, self, [scope, id, props])
  File "/dk-verified-permissions-l2-test-python/.venv/lib/python3.12/site-packages/jsii/_kernel/__init__.py", line 334, in create
    response = self.provider.create(
               ^^^^^^^^^^^^^^^^^^^^^
  File "cdk-verified-permissions-l2-test-python/.venv/lib/python3.12/site-packages/jsii/_kernel/providers/process.py", line 365, in create
    return self._process.send(request, CreateResponse)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "cdk-verified-permissions-l2-test-python/.venv/lib/python3.12/site-packages/jsii/_kernel/providers/process.py", line 342, in send
    raise RuntimeError(resp.error) from JavaScriptError(resp.stack)
RuntimeError: Passed to parameter props of new @cdklabs/cdk-verified-permissions.PolicyStore: Unable to deserialize value as @cdklabs/cdk-verified-permissions.PolicyStoreProps | undefined
├── 🛑 Failing value is an object
│      { '$jsii.struct': [Object] }
╰── 🔍 Failure reason(s):
    ╰─ Key 'validationSettings': Unable to deserialize value as @cdklabs/cdk-verified-permissions.IValidationSettings
        ├── 🛑 Failing value is an object
        │      { '$jsii.map': [Object] }
        ╰── 🔍 Failure reason(s):
            ╰─ Value does not have the "$jsii.byref" key

Reproduction Steps

  • Create a new CDK Python Project
  • Install cdklabs.cdk-verified-permissions 0.0.5 as dependency
  • Try to deploy a PolicyStore with the following code:
from aws_cdk import (
    Stack,
)
from constructs import Construct
from cdklabs import cdk_verified_permissions as avp


class CdkVerifiedPermissionsL2TestPythonStack(Stack):

    def __init__(self, scope: Construct, construct_id: str, **kwargs) -> None:
        super().__init__(scope, construct_id, **kwargs)
        vs: avp.IValidationSettings = {
            "mode": avp.ValidationSettingsMode.OFF
        }
        test = avp.PolicyStore(
            scope, "PolicyStore", validation_settings=vs
        )

Possible Solution

No response

Additional Information/Context

I know that this could depends by the code in the construct library. But the behaviour seems very strange (especially due to the fact that we're seeing passing dictionaries as properties in Python for other CDK L2 Constructs and there are no problems at all) and we would like to understand if it is something related to our code (so we need to fix it) or if it's due to a potential bug in JSII.

Please note:

SDK version used

constructs compiled with jsii-compiler 5.2 and also 5.4

Environment details (OS name and version, etc.)

Local Machine MacOS Ventura, Python version 3.12.4

@reste85 reste85 added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Aug 1, 2024
@mrgrain
Copy link
Contributor

mrgrain commented Aug 7, 2024

As discussed, the error is because IValidationSettings is defined as a behavioral interface, but the code is trying to give it a data-only struct.

It needs use @jsii.implements instead.
Reference: https://aws.github.io/jsii/user-guides/lib-user/language-specific/python/

@lucajone
Copy link

lucajone commented Aug 7, 2024

@reste85: I think in this case we want IValidationSettings to be treated as a struct, rather than a behavioural interface (ref https://aws.github.io/jsii/specification/2-type-system/#interfaces-structs). Renaming the interface to ValidationSettings is a breaking change – I don't know if we could maybe maintain TypeScript compatibility at least by exporting an @deprecated type alias?

@mrgrain
Copy link
Contributor

mrgrain commented Aug 7, 2024

@reste85: I think in this case we want IValidationSettings to be treated as a struct, rather than a behavioural interface (ref https://aws.github.io/jsii/specification/2-type-system/#interfaces-structs). Renaming the interface to ValidationSettings is a breaking change – I don't know if we could maybe maintain TypeScript compatibility at least by exporting an @deprecated type alias?

If that's the case you could add a @struct tag (reference) without the need to rename. It might however be confusing for users and technically changing to a struct is a breaking change for your Python et. al. users as well.

@mrgrain
Copy link
Contributor

mrgrain commented Aug 7, 2024

The following code does work from a jsii perspective, but fails due to a bug in the package code:

import jsii
from aws_cdk import (
    Stack,
)
from constructs import Construct
from cdklabs import cdk_verified_permissions as avp


@jsii.implements(avp.IValidationSettings)
class ValidationSettings:

    def __init__(self, mode):
        self.mode = mode

class CdkPythonStack(Stack):

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

        vs: avp.IValidationSettings = ValidationSettings(mode=avp.ValidationSettingsMode.OFF)
        test = avp.PolicyStore(
            self, "PolicyStore", validation_settings=vs
        )

@mrgrain
Copy link
Contributor

mrgrain commented Aug 7, 2024

In this line, you are converting a behavioral interface to a struct. That works in TS, because duck-typing. But it fails in jsii.

https://github.com/cdklabs/cdk-verified-permissions/blob/ed9c058c298305159dccae234eaae0ef3a0cc345/src/policy-store.ts#L349

And here you are doing the opposite, but this happens to work because that part of the code never crosses the jsii boundary:

https://github.com/cdklabs/cdk-verified-permissions/blob/ed9c058c298305159dccae234eaae0ef3a0cc345/src/policy-store.ts#L334-L336


Sorry jsii doesn't warn you about this. If it can, it probably should. You might also want to look into integration testing your jsii packages.

@mrgrain
Copy link
Contributor

mrgrain commented Aug 7, 2024

Don't do this, but for any one interested you can actually make this work. You "just" have to implement the conversion on the Python side as well.

Key parts here are:

  • @jsii.implements(cdk_avp.CfnPolicyStore.ValidationSettingsProperty)
  • def mode(self) -> builtins.str:
  • And self._mode.value to convert the enum to the string value that the low level construct wants
import jsii
import builtins
import typing
from aws_cdk import (
    Stack,
    aws_verifiedpermissions as cdk_avp
)
from constructs import Construct
from cdklabs import cdk_verified_permissions as avp


@jsii.implements(avp.IValidationSettings)
@jsii.implements(cdk_avp.CfnPolicyStore.ValidationSettingsProperty)
class ValidationSettings:

    def __init__(self, mode):
        self._mode = mode

    @builtins.property
    def mode(self) -> builtins.str:
        result = self._mode.value
        assert result is not None, "Required property 'mode' is missing"
        return typing.cast(builtins.str, result)        

class CdkPythonStack(Stack):

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

        vs: avp.IValidationSettings = ValidationSettings(mode=avp.ValidationSettingsMode.OFF)
        test = avp.PolicyStore(
            self, "PolicyStore", validation_settings=vs
        )

@reste85
Copy link
Author

reste85 commented Aug 7, 2024

@mrgrain and @lucajone thanks a lot for all the infos.
I do think we should avoid behavioural interfaces for properties to give developers a greater experience.

Going to introduce a breaking change, we're still in alpha also for this kind of problems.

Thanks again!

@reste85 reste85 closed this as completed Aug 7, 2024
Copy link
Contributor

github-actions bot commented Aug 7, 2024

This issue is now closed. Comments on closed issues are hard for our team to see.
If you need more assistance, please open a new issue that references this one.

github-merge-queue bot pushed a commit to cdklabs/cdk-verified-permissions that referenced this issue Aug 7, 2024
Removing I notation from props interfaces, in order to be considered as
struct by JSII. This solves Python problems of our Constructs that have
been addressed thanks to the JSII team in the following issue
aws/jsii#4593

BREAKING CHANGE: this introduces breaking changes since some property
classes of constructs have been changed
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

No branches or pull requests

3 participants