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

Unable to use Role.import_ in Java CDK #320

Closed
RomainMuller opened this issue Nov 26, 2018 · 9 comments
Closed

Unable to use Role.import_ in Java CDK #320

RomainMuller opened this issue Nov 26, 2018 · 9 comments
Assignees
Labels
bug This issue is a bug. language/java Related to Java bindings

Comments

@RomainMuller
Copy link
Contributor

I'm attempting to call Role.import_(...) in my Java CDK application, but I'm getting a ClassCastException from the CDK library. Apparently the result that's being returned from the Jsii runtime is a Construct which cannot be cast to an IRole:

Exception in thread "main" java.lang.ClassCastException: software.amazon.awscdk.Construct cannot be cast to software.amazon.awscdk.services.iam.IRole
	at software.amazon.awscdk.services.iam.Role.import_(Role.java:24)
	at MyCallingCode

The underlying TypeScript (github) declares that it returns IRole and explicitly returns an ImportedRole, which implements IRole, but also extends Construct (github). While walking through the Java code in the debugger I see that the result from Jsii's toString() shows "ImportedRole", but it's type is Construct.

@RomainMuller RomainMuller added bug This issue is a bug. language/java Related to Java bindings labels Nov 26, 2018
@RomainMuller
Copy link
Contributor Author

The issue appears to be that ImportedRole (what Role.import) returns is not an exported class, which results in the JSII runtime interpreting it as it's exported base class (Construct) when returning the instance over. This causes the Java runtime to then incorrectly type it as Construct instead of IRole. Basically, the interface types are lost at the boundary.

A minimal repro is:

package software.amazon;

import software.amazon.awscdk.App;
import software.amazon.awscdk.Stack;
import software.amazon.awscdk.services.iam.ImportedRoleProps;
import software.amazon.awscdk.services.iam.Role;

public final class Jsii320
{
    public static void main(final String[] args)
    {
        final App app = new App();
        final Stack stack = new Stack(app, "TestStack");
        Role.import_(
            stack,
            "ImportedRole",
            ImportedRoleProps.builder()
                             .withRoleArn("arn:aws:iam:::role/ImportedRole")
                             .build()
        );
        app.run();
    }
}

@rix0rrr
Copy link
Contributor

rix0rrr commented Nov 26, 2018

The Java jsii runtime should be constructing an interace proxy for IRole instead of guessing at a concrete class. Same as what the .NET runtime is doing. Do we have a tracking issue for this yet?

@rix0rrr
Copy link
Contributor

rix0rrr commented Nov 26, 2018

And it should be a compliance test for jsii.

@RomainMuller
Copy link
Contributor Author

RomainMuller commented Nov 26, 2018

I reckon this is the tracking issue for that...

The issue here is really that the jsii-kernel returns an FQN on the concrete class instead of returning that of the declared type of the property.

This could be solved either by having the kernel return an ObjID with the correct type name, or by having the Java runtime ignore the type fragment of the FQN (merely treating it as a hint) and just generating an object proxy if the declared type is an interface.

@rix0rrr
Copy link
Contributor

rix0rrr commented Nov 26, 2018

Fine, but the codegen has the declared type so it could use that as well.

@RomainMuller
Copy link
Contributor Author

Agreed. I'm leaning towards a fix that makes the Java runtime not trust the type part of the ObjID so much, because that's a much easier fix, and also because that'd possibly make some use-cases quite convoluted (same object returned under various different interfaces it implements... ouch).

@rix0rrr
Copy link
Contributor

rix0rrr commented Nov 26, 2018

If we go and trust the declared type completely, there are some edge cases that we can no longer downcast the object even if that would be legal (if jsii would return an exported subtype, or if we passed a subtype from java -> jsii -> java).

We should either:

  • Combine static and runtime types, and so something the likes of:
if (exported(concrete_type))
  return concrete_type;
else
  return declared_type_proxy;
  • Use the reflection API to create a class for the unexported type at runtime.
  • Not care about downcasting.

RomainMuller added a commit that referenced this issue Nov 26, 2018
This test covers the behavior of the JSII runtimes when a method is
declared to return an interface type, and returns an instance of a
private (un-exported) type that implements the interface while extending
an exported type. This has been seen to cause issues in the Java
runtime, for example, as the JSII kernel will return an ObjID with a
type fragment that refers to the exported super-class, and not the
interface type.
@RomainMuller
Copy link
Contributor Author

Adding a compliance test around this issue revealed that .NET/C# is also affected by this problem.

@RomainMuller
Copy link
Contributor Author

PR #321 fixes this issue, 🎉

@RomainMuller RomainMuller self-assigned this Nov 26, 2018
RomainMuller referenced this issue Nov 26, 2018
When an un-exported type that extends an exported type is returned with
an interface as the declared type, the JSII kernel used to return a ref
with the FQN of the exported supertype, instead of correctly wrapping
the instance in a proxy of the interface type as it should have.

---

Adds a test that covers the behavior of the JSII runtimes when a method
is declared to return an interface type, and returns an instance of a
private (un-exported) type that implements the interface while extending
an exported type. This has been seen to cause issues in the Java
runtime, for example, as the JSII kernel will return an ObjID with a
type fragment that refers to the exported super-class, and not the
interface type.

---

Fixes #302
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. language/java Related to Java bindings
Projects
None yet
Development

No branches or pull requests

2 participants