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

Typescript union types are not correctly resolved in Java #3935

Open
RoKish opened this issue May 27, 2020 · 7 comments
Open

Typescript union types are not correctly resolved in Java #3935

RoKish opened this issue May 27, 2020 · 7 comments
Labels
bug This issue is a bug. closed-for-staleness effort/large Large work item – several weeks of effort jsii language/java Related to Java bindings p1

Comments

@RoKish
Copy link

RoKish commented May 27, 2020

In Typescript CloudFormationStackArtifact#getAssets returns AssetMetadataEntry[], where AssetMetadataEntry = FileAssetMetadataEntry | ContainerImageAssetMetadataEntry.
In Java this method returns a List<Object>, which is, actually, a list of JSII objects. The JSII objects are supposed to reference FileAssetMetadataEntry and ContainerImageAssetMetadataEntry objects from JSII runtime, however the reference type is always FileAssetMetadataEntry:

final Object asset = stackArtifact.getAssets().get(0);
// the reference interface will always be FileAssetMetadataEntry even though it's actually ContainerImageAssetMetadataEntry
JsiiEngine.getInstance().nativeToObjRef(asset).getInterfaces() 

The issue is also actual for software.amazon.awscdk.cloudassembly.schema.MissingContext#getProps. It is supposed to return ContextQueryProperties which is a union type in Typescript, however, the JSII object reference is always of type AmiContextQuery (looks like it always takes the first type in the union type definition as both FileAssetMetadataEntry and AmiContextQuery are defined first.

Reproduction Steps

  1. Define a container image asset in your stack
  2. Synthesize the templates
  3. Try to access repositoryName of the asset:
Object asset = stackArtifact.getAssets().get(0);
JsiiEngine jsiiEngine = JsiiEngine.getInstance();
JsiiObjectRef objectRef = jsiiEngine.nativeToObjRef(asset);
JsiiClient client = jsiiEngine.getClient();
String packaging = client.getPropertyValue(objectRef, "packaging").asText();
assert packaging.equals("container-image");
String repositoryName = client.getPropertyValue(objectRef, "repositoryName").asText(); //causes an exception

Error Log

Error: Type Object or interface(s) @aws-cdk/cloud-assembly-schema.FileAssetMetadataEntry doesn't have a property 'repositoryName'
    at Kernel._typeInfoForProperty (/private/var/folders/g6/wh2_ynfn4nb9wcpv5vjfphww0000gn/T/jsii-java-runtime14150496997154827187/jsii-runtime.js:8205:19)
    at Kernel.get (/private/var/folders/g6/wh2_ynfn4nb9wcpv5vjfphww0000gn/T/jsii-java-runtime14150496997154827187/jsii-runtime.js:7642:25)
    at KernelHost.processRequest (/private/var/folders/g6/wh2_ynfn4nb9wcpv5vjfphww0000gn/T/jsii-java-runtime14150496997154827187/jsii-runtime.js:7388:28)
    at KernelHost.run (/private/var/folders/g6/wh2_ynfn4nb9wcpv5vjfphww0000gn/T/jsii-java-runtime14150496997154827187/jsii-runtime.js:7328:14)
    at Immediate._onImmediate (/private/var/folders/g6/wh2_ynfn4nb9wcpv5vjfphww0000gn/T/jsii-java-runtime14150496997154827187/jsii-runtime.js:7331:37)
    at processImmediate (internal/timers.js:456:21)

Environment

  • CLI Version : 1.41.0
  • Framework Version: 1.41.0
  • OS : macOs Catalina
  • Language : Java
@RoKish RoKish added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels May 27, 2020
@SomayaB SomayaB added language/java Related to Java bindings jsii labels May 27, 2020
@RomainMuller RomainMuller added p1 needs-reproduction This issue needs reproduction. and removed needs-triage This issue or PR still needs to be triaged. labels May 28, 2020
@RomainMuller
Copy link
Contributor

Hey!

Thanks for reporting. That looks weird indeed... I'll need to come up with a jsii-level repro to clarify the situation here...

@RoKish
Copy link
Author

RoKish commented May 28, 2020

Thank you! Just in case, I've created a repo reproducing the issue: https://github.com/RoKish/cdk-union-type-bug.

@RomainMuller
Copy link
Contributor

Thanks for that - this is super helpful.

This issue is actually pretty complex... There is a lack of runtime-available information and the jsii kernel (JS side) is doing its best here... The problem is while I can come up with a solution that possibly fixes this particular instance, a fix that solves the issue generally is a lot more difficult to achieve.


Thinking though some of the options to make this a little nicer (basically, giving you as a user more control over what you can do), it appears the best way would be to surface some Union model out instead of trying to be clever:

  1. An ugly way is to offer a Union class that has a boolean isInstance(Class<?> clazz) as well as an <T> T asType(Class<T> class) throws ClassCastException method.
  2. A less ugly solution would be to code-generate a distinct class for each union we return, with directed methods for each candidate type (boolean isFileAssetMetadataEntry(), FileAssetMetadataEntry asFileAssetMetadataEntry(), ...)
    • The challenge is with finding a way to name those types that does not risk conflicting with another user-defined type. For example in your case here that'd be something like StackArtifact.getAsset$Result
    • We'd also possibly generate tons of duplicated code (which is probably not the biggest problem here)

@RomainMuller RomainMuller added effort/large Large work item – several weeks of effort and removed needs-reproduction This issue needs reproduction. labels Aug 11, 2020
@RomainMuller RomainMuller removed their assignment Jun 21, 2021
@github-actions
Copy link
Contributor

This issue has not received any attention in 1 year. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.

@github-actions github-actions bot added closing-soon This issue will automatically close in 4 days unless further comments are made. closed-for-staleness and removed closing-soon This issue will automatically close in 4 days unless further comments are made. labels Jun 21, 2022
@matusfaro
Copy link

Issue is still present and makes parsing of manifest from Java not possible.

From an ArtifactManifest object, the property properties in java-land is a JsiiObject which after attempting to use as a AssetManifestProperties throws an error since it incorrectly thinks it should be AwsCloudFormationStackProperties.

To reproduce error:

AssemblyManifest assemblyManifest = Manifest.loadAssemblyManifest("manifest.json"));
assemblyManifest.getArtifacts().values().stream()
         // Filter out only asset artifacts
         .filter(artifactManifest -> ArtifactType.ASSET_MANIFEST.equals(artifactManifest.getType()))
         // Expect the properties to be of type AssetManifestProperties
         .map(artifactManifest -> Kernel.get(artifactManifest, "properties", AssetManifestProperties.class))

Throws:

Caused by: software.amazon.jsii.JsiiException: Type Object or interface(s) @aws-cdk/cloud-assembly-schema.AwsCloudFormationStackProperties, @aws-cdk/cloud-assembly-schema.AwsCloudFormationStackProperties doesn't have a property 'file'
Error: Type Object or interface(s) @aws-cdk/cloud-assembly-schema.AwsCloudFormationStackProperties, @aws-cdk/cloud-assembly-schema.AwsCloudFormationStackProperties doesn't have a property 'file'
    at exports.Kernel._typeInfoForProperty (/private/var/folders/15/6r443qw917d9y61l_sw28wfc0000gn/T/jsii-java-runtime8967244983856758059/lib/program.js:5413:27)
    at exports.Kernel.get (/private/var/folders/15/6r443qw917d9y61l_sw28wfc0000gn/T/jsii-java-runtime8967244983856758059/lib/program.js:5020:96)
    at exports.KernelHost.processRequest (/private/var/folders/15/6r443qw917d9y61l_sw28wfc0000gn/T/jsii-java-runtime8967244983856758059/lib/program.js:6083:36)
    at exports.KernelHost.run (/private/var/folders/15/6r443qw917d9y61l_sw28wfc0000gn/T/jsii-java-runtime8967244983856758059/lib/program.js:6057:48)
    at Immediate._onImmediate (/private/var/folders/15/6r443qw917d9y61l_sw28wfc0000gn/T/jsii-java-runtime8967244983856758059/lib/program.js:6058:46)
    at process.processImmediate (node:internal/timers:471:21)
    at software.amazon.jsii.JsiiRuntime.processErrorResponse (JsiiRuntime.java:124)
    at software.amazon.jsii.JsiiRuntime.requestResponse (JsiiRuntime.java:95)
    at software.amazon.jsii.JsiiClient.getPropertyValue (JsiiClient.java:112)
    at software.amazon.jsii.Kernel.get (Kernel.java:72)
    at software.amazon.awscdk.cloudassembly.schema.AssetManifestProperties$Jsii$Proxy.<init> (AssetManifestProperties.java:144)
    at jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0 (Native Method)
    at jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance (NativeConstructorAccessorImpl.java:77)
    at jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance (DelegatingConstructorAccessorImpl.java:45)
    at java.lang.reflect.Constructor.newInstanceWithCaller (Constructor.java:499)
    at java.lang.reflect.Constructor.newInstance (Constructor.java:480)
    at software.amazon.jsii.JsiiObject.asInterfaceProxy (JsiiObject.java:377)
    at software.amazon.jsii.NativeType$SimpleNativeType.transform (NativeType.java:157)
    at software.amazon.jsii.JsiiObjectMapper.treeToValue (JsiiObjectMapper.java:47)
    at software.amazon.jsii.Kernel.get (Kernel.java:73)
...

@RomainMuller
Copy link
Contributor

I'll mention here that we have an open RFC about the type unions problem => aws/aws-cdk-rfcs#193

@RomainMuller RomainMuller reopened this Aug 29, 2022
@TheRealAmazonKendra
Copy link
Contributor

This is a jsii issue so I'm moving this to that repo.

@TheRealAmazonKendra TheRealAmazonKendra transferred this issue from aws/aws-cdk Jan 27, 2023
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. closed-for-staleness effort/large Large work item – several weeks of effort jsii language/java Related to Java bindings p1
Projects
None yet
Development

No branches or pull requests

5 participants