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

Struct multi-inheritance generates wrong Python code #2653

Closed
1 of 4 tasks
skinny85 opened this issue Mar 4, 2021 · 1 comment · Fixed by #2654
Closed
1 of 4 tasks

Struct multi-inheritance generates wrong Python code #2653

skinny85 opened this issue Mar 4, 2021 · 1 comment · Fixed by #2654
Assignees
Labels
bug This issue is a bug. effort/small Small work item – less than a day of effort language/python Related to Python bindings module/pacmak Issues affecting the `jsii-pacmak` module p1

Comments

@skinny85
Copy link
Contributor

skinny85 commented Mar 4, 2021

🐛 Bug Report

Affected Languages

  • TypeScript or Javascript
  • Python
  • Java
  • .NET (C#, F#, ...)

General Information

  • JSII Version: 1.23.0
  • Platform: all of them

What is the problem?

After merging aws/aws-cdk#13356 in the CDK repo, JSII generates the following code for the forAsset static method in the @aws-cdk/aws-lambda module:

    @jsii.member(jsii_name="fromAsset") # type: ignore[misc]
    @builtins.classmethod
    def from_asset(
        cls,
        path: builtins.str,
        *,
        readers: typing.Optional[typing.List[aws_cdk.aws_iam.IGrantable]] = None,
        source_hash: typing.Optional[builtins.str] = None,
        exclude: typing.Optional[typing.List[builtins.str]] = None,
        follow: typing.Optional[aws_cdk.assets.FollowMode] = None,
        ignore_mode: typing.Optional[aws_cdk.core.IgnoreMode] = None,
        exclude: typing.Optional[typing.List[builtins.str]] = None,
        follow_symlinks: typing.Optional[aws_cdk.core.SymlinkFollowMode] = None,
        ignore_mode: typing.Optional[aws_cdk.core.IgnoreMode] = None,
        asset_hash: typing.Optional[builtins.str] = None,
        asset_hash_type: typing.Optional[aws_cdk.core.AssetHashType] = None,
        bundling: typing.Optional[aws_cdk.core.BundlingOptions] = None,
    ) -> "AssetCode":
        '''Loads the function code from a local disk path.

        :param path: Either a directory with the Lambda code bundle or a .zip file.
        :param readers: (experimental) A list of principals that should be able to read this asset from S3. You can use ``asset.grantRead(principal)`` to grant read permissions later. Default: - No principals that can read file asset.
        :param source_hash: (deprecated) Custom hash to use when identifying the specific version of the asset. For consistency, this custom hash will be SHA256 hashed and encoded as hex. The resulting hash will be the asset hash. NOTE: the source hash is used in order to identify a specific revision of the asset, and used for optimizing and caching deployment activities related to this asset such as packaging, uploading to Amazon S3, etc. If you chose to customize the source hash, you will need to make sure it is updated every time the source changes, or otherwise it is possible that some deployments will not be invalidated. Default: - automatically calculate source hash based on the contents of the source file or directory.
        :param exclude: (deprecated) Glob patterns to exclude from the copy. Default: nothing is excluded
        :param follow: (deprecated) A strategy for how to handle symlinks. Default: Never
        :param ignore_mode: (deprecated) The ignore behavior to use for exclude patterns. Default: - GLOB for file assets, DOCKER or GLOB for docker assets depending on whether the '
        :param exclude: Glob patterns to exclude from the copy. Default: - nothing is excluded
        :param follow_symlinks: A strategy for how to handle symlinks. Default: SymlinkFollowMode.NEVER
        :param ignore_mode: The ignore behavior to use for exclude patterns. Default: IgnoreMode.GLOB
        :param asset_hash: Specify a custom hash for this asset. If ``assetHashType`` is set it must be set to ``AssetHashType.CUSTOM``. For consistency, this custom hash will be SHA256 hashed and encoded as hex. The resulting hash will be the asset hash. NOTE: the hash is used in order to identify a specific revision of the asset, and used for optimizing and caching deployment activities related to this asset such as packaging, uploading to Amazon S3, etc. If you chose to customize the hash, you will need to make sure it is updated every time the asset changes, or otherwise it is possible that some deployments will not be invalidated. Default: - based on ``assetHashType``
        :param asset_hash_type: Specifies the type of hash to calculate for this asset. If ``assetHash`` is configured, this option must be ``undefined`` or ``AssetHashType.CUSTOM``. Default: - the default is ``AssetHashType.SOURCE``, but if ``assetHash`` is explicitly specified this value defaults to ``AssetHashType.CUSTOM``.
        :param bundling: (experimental) Bundle the asset by executing a command in a Docker container. The asset path will be mounted at ``/asset-input``. The Docker container is responsible for putting content at ``/asset-output``. The content at ``/asset-output`` will be zipped and used as the final asset. Default: - uploaded as-is to S3 if the asset is a regular file or a .zip file, archived into a .zip file and uploaded to S3 otherwise
        '''
        options = aws_cdk.aws_s3_assets.AssetOptions(
            readers=readers,
            source_hash=source_hash,
            exclude=exclude,
            follow=follow,
            ignore_mode=ignore_mode,
            exclude=exclude,
            follow_symlinks=follow_symlinks,
            ignore_mode=ignore_mode,
            asset_hash=asset_hash,
            asset_hash_type=asset_hash_type,
            bundling=bundling,
        )

        return typing.cast("AssetCode", jsii.sinvoke(cls, "fromAsset", [path, options]))

This is clearly wrong - the exclude and ignore_mode keyword args are duplicated.

I suspect that's because the argument to fromAsset(), AssetOptions from the @aws-cdk/aws-s3-assets module, is currently defined as:

export interface AssetOptions extends assets.CopyOptions, cdk.FileCopyOptions, cdk.AssetOptions {
  // ...

Where both assets.CopyOptions and cdk.FileCopyOptions define the properties exclude and ignoreMode (of the same types, string[] and cdk.IgnoreMode, respectively).

Verbose Log

@skinny85 skinny85 added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Mar 4, 2021
RomainMuller added a commit that referenced this issue Mar 5, 2021
When a struct field is inherited from more than one parent (this could
be twice from the same type - in the case of a diamond shape; or from
two distinct parents that declare the same field), the lifted keyword
arguments in python would be duplicated for this field.

This is because the method that performs the keyword argument lifting
did not perform name-based de-duplication, and operates directly on the
"raw" assembly (whereby it must traverse the inheritance tree itself,
as opposed to the Go generator which uses `jsii-reflect` and has the
field collection done by that library).

Added the necessary de-duplication logic and confirmed the produced code
now looks correct using the exact same test harness as I had introduced
in #2650.

Fixes #2653
RomainMuller added a commit that referenced this issue Mar 5, 2021
When a struct field is inherited from more than one parent (this could
be twice from the same type - in the case of a diamond shape; or from
two distinct parents that declare the same field), the lifted keyword
arguments in python would be duplicated for this field.

This is because the method that performs the keyword argument lifting
did not perform name-based de-duplication, and operates directly on the
"raw" assembly (whereby it must traverse the inheritance tree itself,
as opposed to the Go generator which uses `jsii-reflect` and has the
field collection done by that library).

Added the necessary de-duplication logic and confirmed the produced code
now looks correct using the exact same test harness as I had introduced
in #2650.

Fixes #2653
@RomainMuller RomainMuller added effort/small Small work item – less than a day of effort language/python Related to Python bindings module/pacmak Issues affecting the `jsii-pacmak` module p1 and removed needs-triage This issue or PR still needs to be triaged. labels Mar 5, 2021
@RomainMuller RomainMuller self-assigned this Mar 5, 2021
RomainMuller added a commit that referenced this issue Mar 18, 2021
When a struct field is inherited from more than one parent (this could
be twice from the same type - in the case of a diamond shape; or from
two distinct parents that declare the same field), the lifted keyword
arguments in python would be duplicated for this field.

This is because the method that performs the keyword argument lifting
did not perform name-based de-duplication, and operates directly on the
"raw" assembly (whereby it must traverse the inheritance tree itself,
as opposed to the Go generator which uses `jsii-reflect` and has the
field collection done by that library).

Fixes #2653
@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.

skinny85 added a commit to skinny85/aws-cdk that referenced this issue Mar 25, 2021
This is a re-submit of the PR aws#13356,
which had to be reverted because of JSII issue aws/jsii#2653.
Since that issue has been fixed in JSII version `1.26.0`,
which is what we currently use,
re-introduce the changes from that PR.
mergify bot pushed a commit to aws/aws-cdk that referenced this issue Mar 29, 2021
This is a re-submit of the PR #13356,
which had to be reverted because of JSII issue aws/jsii#2653.
Since that issue has been fixed in JSII version `1.26.0`,
which is what we currently use,
re-introduce the changes from that PR.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
hollanddd pushed a commit to hollanddd/aws-cdk that referenced this issue Mar 31, 2021
This is a re-submit of the PR aws#13356,
which had to be reverted because of JSII issue aws/jsii#2653.
Since that issue has been fixed in JSII version `1.26.0`,
which is what we currently use,
re-introduce the changes from that PR.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
hollanddd pushed a commit to hollanddd/aws-cdk that referenced this issue Aug 26, 2021
This is a re-submit of the PR aws#13356,
which had to be reverted because of JSII issue aws/jsii#2653.
Since that issue has been fixed in JSII version `1.26.0`,
which is what we currently use,
re-introduce the changes from that PR.

----

*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. effort/small Small work item – less than a day of effort language/python Related to Python bindings module/pacmak Issues affecting the `jsii-pacmak` module p1
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants