Closed reste85 closed 3 months ago
mrgrain
commented
3 months ago 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
commented
3 months ago @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
commented
3 months ago @reste85: I think in this case we want
IValidationSettingsto 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 toValidationSettingsis a breaking change – I don't know if we could maybe maintain TypeScript compatibility at least by exporting an@deprecatedtype 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
commented
3 months ago 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
)In this line, you are converting a behavioral interface to a struct. That works in TS, because duck-typing. But it fails in jsii.
And here you are doing the opposite, but this happens to work because that part of the code never crosses the jsii boundary:
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
commented
3 months ago 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:self._mode.value to convert the enum to the string value that the low level construct wantsimport 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
commented
3 months ago @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!
github-actions[bot]
commented
3 months ago 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.
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
PolicyStoreconstruct. 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 providingvalidation_settingsargument 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 elementsExpected Behavior
Create a
PolicyStorewithout problems and with the minimum required parametersCurrent Behavior
Deserialization error:
Reproduction Steps
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:
PolicyStore, more specifically the error happens related to entering fields for validation_settings propertySDK 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