fix(java): invalid collections returned with non-class elements #1197

RomainMuller · 2020-01-14T15:08:53Z

The Java runtime for jsii used to not be able to necessarily hold the
type information necessary to correctly process the contents of
collections, as the generic information is lost.

In order to address this, introduced a new NativeType<T> class that is
able to carry:

The actual declared type of the value (via generic type parameter)
The raw type of the value (reified)
The correct JavaType to pass to Jackson to obtain the correct result

It also preserves the specific behavior around de-serialization of
JsiiObject sub-classes as well as jsii interfaces, correctly
requesting de-serialization as the JsiiObject class, and in a second
phase transform this into the correct value type as needed.

Introduced a new test that validates the List<T> and Map<String, T>
values received from node are deserialized to instances with the
appropriate content type.

Fixes #1196

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Commit Message

fix(java): invalid collections returned with non-class elements (#1197)

The Java runtime for jsii used to not be able to necessarily hold the
type information necessary to correctly process the contents of
collections, as the generic information is lost.

In order to address this, introduced a new NativeType<T> class that is
able to carry:

The actual declared type of the value (via generic type parameter)
The raw type of the value (reified)
The correct JavaType to pass to Jackson to obtain the correct result

It also preserves the specific behavior around de-serialization of
JsiiObject sub-classes as well as jsii interfaces, correctly
requesting de-serialization as the JsiiObject class, and in a second
phase transform this into the correct value type as needed.

Introduced a new test that validates the List<T> and Map<String, T>
values received from node are deserialized to instances with the
appropriate content type.

Fixes #1196

RomainMuller · 2020-01-14T15:09:40Z

Labelled with do-not-merge as I'm pondering whether the old code paths need to be preserved in order to ensure proper interoperability with code generated against older versions of the Java runtime.

That shouldn't prevent reviewing the current change (the backwards compatibility can be restored by adding overloads for the previous API style, which would leverage the new API transparently - it would however conserve the previously broken behavior).

aws-cdk-automation · 2020-01-14T15:19:12Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2020-01-14T15:21:15Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

rix0rrr · 2020-01-14T15:54:15Z

To help me review, since this change is ungodly:

This was flagged as a regression (since jsii 0.17 if I'm reading the breadcrumb trail directly). Can you explain succinctly what the changes were that caused this breakage (starting from the wire protocol changes, if any) and what the approach for the fix is?

To the uncultured plebeian that I am, it would seem that if the wire protocol says:

[ { "jsii$ref": "my.MyObject@1" }]

It should be easy enough to construct a List with a MyObject in it?

It's probably trickier than that but I need it explained to me like I'm 5 :).

RomainMuller · 2020-01-15T14:12:26Z

I significantly reduced the amount of regression test file changes by preserving the previous API (and having it delegate into the new one); and only switching generated code to the new API when the older one was unsafe.

To answer @rix0rrr's question, here's what happened:

In feat(kernel): annotate implemented interfaces on "ObjRef"s #825, the Object ID generation was changed, specifically around interfaces.
- The format of Object IDs generally speaking is <type-fqn>@<some-number>.
- Prior to feat(kernel): annotate implemented interfaces on "ObjRef"s #825, the <type-fqn> part could be an interface if the value was exiting JavaScript through an interface-typed method or property.
- After feat(kernel): annotate implemented interfaces on "ObjRef"s #825, the <type-fqn> part is required to be a class name, or Object (pure interface implementations, and by-reference structs). Necessary interface declarations are passed on a separate field of the ObjRef.
This means that since then, first pass hydration in Java cannot yield an interface at all... Obtaining an interface view of an object requires on additional step (JsiiObject.asInterfaceProxy(Class<?>)).
This additional step is done for single values, but was missed from Lists and Maps; because the only signal that is passed to the deserializer was List.class or Map.class, which yields no information about the expected element type.
Add to this that Java does not allow providing customized implicit conversions (the ways C# does), nor creating a dynamic proxy object with a base class other than Object (the way it can be done in Python), and you get the broken behavior.

Hopefully this helps clarify the problem...

Now - to the approach for solving this: I introduced a way to represent complex types to provide directions to the deserializer, in the form of the NativeType class. This is a Jackson (our JSON handling tool of choice) friendly way to express the expected type to be deserialized, including any specializations such as element type.

aws-cdk-automation · 2020-01-15T14:12:30Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2020-01-15T14:14:35Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

eladb · 2020-01-15T21:38:42Z

I am not sure I fully understand if this is a breaking change. I don't think we can afford breaking changes neither of the manifest nor of the runtime protocol.

RomainMuller · 2020-01-16T14:00:17Z

@eladb - I don't think this changes the runtime protocol. The only part that may be "broken" here is technically not working correctly in the current state of things. The only one thing which I think may still need "easing in" (so new runtime works with code generated by the previous generator) is with how the "old API" was modified to delegate into the "new API", which will throw if one attempts to use an api that returns a List or a Map (those could have returned collection with "illegal" items in them).

I think I can work out a way to make the old behavior preserved, so older generated code keeps behaving the same as it currently does (still broken when it was, but not failing more).

I can then attempt to run a number of scenarios using the current CDK libraries with the new Java runtime ahead of the class path to validate it is indeed backwards-compatible?

I'd add unit tests to assert persistence of the older behavior, but I'd be skeptical as to the coverage of such (new) tests being sufficient...

rix0rrr · 2020-01-17T09:54:02Z

This means that since then, first pass hydration in Java cannot yield an interface at all...

This part I don't get. It used to be:

{ "jsii$ref": "my.MyInterface@1" }

And now it's:

{ "jsii$ref": "Object@1", "jsii$interfaces": ["my.MyInterface"] }

We can restore the "old" behavior by picking the first interface from the associated list and instantiating the proxy class for that type, right?

I guess we're doing all this work so that if there are 2 interfaces in the wire list, we'll pick the one that the current method has been declared to return? (Without looking at the wire data, looks like, just assuming the types will work out as declared?)

I am slightly concerned about all the introspection happening on every JSII call; every call constructs a set of new NativeType objects, which could be cached because they're always the same. But I guess this is not the most expensive work happening on jsii calls :)

aws-cdk-automation · 2020-01-20T09:02:09Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2020-01-20T09:02:20Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

This will allow ensuring the fix is not "critically" backwards incompatible with older generated code. For instance, that old code will continue to "work" (including the current bug) instead of throwing some new exception (IllegalArgument, IllegalState, ...).

The Java runtime for jsii used to not be able to necessarily hold the type information necessary to correctly process the contents of collections, as the generic information is lost. In order to address this, introduced a new `NativeType<T>` class that is able to carry: 1. The actual declared type of the value (via generic type parameter) 2. The raw type of the value (reified) 3. The correct JavaType to pass to Jackson to obtain the correct result It also preserves the specific behavior around de-serialization of `JsiiObject` sub-classes as well as jsii interfaces, correctly requesting de-serialization as the `JsiiObject` class, and in a second phase `transform` this into the correct value type as needed. Introduced a new test that validates the `List<T>` and `Map<String, T>` values received from `node` are deserialized to instances with the appropriate content type. Fixes #1196

aws-cdk-automation · 2020-01-20T11:22:08Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2020-01-20T11:24:31Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

RomainMuller · 2020-01-20T11:31:02Z

I added a test of the previous behavior with respects to List and Map, in order to ensure the new code path does not cause older generated code to crash. I also fixed the new code path so that... it... does not crash on such code paths.

…llections

aws-cdk-automation · 2020-01-27T11:28:49Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2020-01-27T11:28:56Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

eladb

Curious, how come this issue did not occur with .NET? Was the modeling of native types different there?

eladb · 2020-01-27T14:08:01Z

packages/@jsii/java-runtime/pom.xml.t.js

-        <!-- https://mvnrepository.com/artifact/org.mockito/mockito-core -->
+        <!-- https://mvnrepository.com/artifact/org.mockito/mockito-junit-jupiter -->


Why was this changed?

eladb · 2020-01-27T14:11:07Z

packages/@jsii/java-runtime/project/src/main/java/software/amazon/jsii/NativeType.java

+import java.util.Map;
+import java.util.stream.Collectors;
+
+public abstract class NativeType<T> {


Missing documentation

eladb · 2020-01-27T14:11:23Z

packages/@jsii/java-runtime/project/src/main/java/software/amazon/jsii/NativeType.java

+import java.util.stream.Collectors;
+
+public abstract class NativeType<T> {
+    protected static final JavaType[] NO_TYPE_PARAMS = {};


Missing docs

mergify · 2020-01-27T14:16:49Z

Thank you for contributing! ❤️ I will now look into making sure the PR is up-to-date, then proceed to try and merge it!

mergify · 2020-01-27T14:16:49Z

Thank you for contributing! ❤️ I will now look into making sure the PR is up-to-date, then proceed to try and merge it!

RomainMuller requested review from MrArnoldPalmer and a team as code owners January 14, 2020 15:08

RomainMuller self-assigned this Jan 14, 2020

RomainMuller added the pr/do-not-merge This PR should not be merged at this time. label Jan 14, 2020

RomainMuller and others added 3 commits January 20, 2020 11:54

Make change less invasive by preserving previous API

ecedc20

RomainMuller force-pushed the rmuller/fix-java-collections branch from cd152f0 to ecedc20 Compare January 20, 2020 11:12

RomainMuller removed the pr/do-not-merge This PR should not be merged at this time. label Jan 20, 2020

SomayaB added the contribution/core This is a PR that came from AWS. label Jan 21, 2020

Merge remote-tracking branch 'origin/master' into rmuller/fix-java-co…

5f4b475

…llections

RomainMuller removed the request for review from MrArnoldPalmer January 27, 2020 11:19

eladb approved these changes Jan 27, 2020

View reviewed changes

mergify bot added pr/ready-to-merge This PR is ready to be merged. labels Jan 27, 2020

mergify bot merged commit bbc2302 into master Jan 27, 2020

mergify bot deleted the rmuller/fix-java-collections branch January 27, 2020 14:18

mergify bot removed the pr/ready-to-merge This PR is ready to be merged. label Jan 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(java): invalid collections returned with non-class elements #1197

fix(java): invalid collections returned with non-class elements #1197

RomainMuller commented Jan 14, 2020 •

edited

Loading

RomainMuller commented Jan 14, 2020 •

edited

Loading

aws-cdk-automation commented Jan 14, 2020

aws-cdk-automation commented Jan 14, 2020

rix0rrr commented Jan 14, 2020

RomainMuller commented Jan 15, 2020

aws-cdk-automation commented Jan 15, 2020

aws-cdk-automation commented Jan 15, 2020

eladb commented Jan 15, 2020

RomainMuller commented Jan 16, 2020

rix0rrr commented Jan 17, 2020

aws-cdk-automation commented Jan 20, 2020

aws-cdk-automation commented Jan 20, 2020

aws-cdk-automation commented Jan 20, 2020

aws-cdk-automation commented Jan 20, 2020

RomainMuller commented Jan 20, 2020

aws-cdk-automation commented Jan 27, 2020

aws-cdk-automation commented Jan 27, 2020

eladb left a comment

eladb Jan 27, 2020

eladb Jan 27, 2020

eladb Jan 27, 2020

mergify bot commented Jan 27, 2020

mergify bot commented Jan 27, 2020

		<!-- https://mvnrepository.com/artifact/org.mockito/mockito-core -->
		<!-- https://mvnrepository.com/artifact/org.mockito/mockito-junit-jupiter -->

fix(java): invalid collections returned with non-class elements #1197

fix(java): invalid collections returned with non-class elements #1197

Conversation

RomainMuller commented Jan 14, 2020 • edited Loading

Commit Message

RomainMuller commented Jan 14, 2020 • edited Loading

aws-cdk-automation commented Jan 14, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Jan 14, 2020

AWS CodeBuild CI Report

rix0rrr commented Jan 14, 2020

RomainMuller commented Jan 15, 2020

aws-cdk-automation commented Jan 15, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Jan 15, 2020

AWS CodeBuild CI Report

eladb commented Jan 15, 2020

RomainMuller commented Jan 16, 2020

rix0rrr commented Jan 17, 2020

aws-cdk-automation commented Jan 20, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Jan 20, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Jan 20, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Jan 20, 2020

AWS CodeBuild CI Report

RomainMuller commented Jan 20, 2020

aws-cdk-automation commented Jan 27, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Jan 27, 2020

AWS CodeBuild CI Report

eladb left a comment

Choose a reason for hiding this comment

eladb Jan 27, 2020

Choose a reason for hiding this comment

eladb Jan 27, 2020

Choose a reason for hiding this comment

eladb Jan 27, 2020

Choose a reason for hiding this comment

mergify bot commented Jan 27, 2020

mergify bot commented Jan 27, 2020

RomainMuller commented Jan 14, 2020 •

edited

Loading

RomainMuller commented Jan 14, 2020 •

edited

Loading