Skip to content

Commit

Permalink
fix(java): remove Jackson confusion with certain patterns (#1070)
Browse files Browse the repository at this point in the history
(Reintroduce #987).

Upon deserializing (`ObjectMapper#treeToValue`), Jackson will first
validate the `valueType` for whether ti can be deserialized with the
"standard" bean deserializer. This will bail out if there are multiple
ambiguous setters for a given property (they are ambiguous if they have
parameters of non-trivial types, such as `List`, some class, ...).

In order to remove the problem, this changes the deserialization path so
that instead of asking for deserialization into the needed instance type
directly, `JsiiObject` will be requested instead when the declared type
is a sub-class of that. Since such types are passed by-reference, the
custom de-serializer modifier will correctly determine the "right" class
to use (the declared type, or a subtype of it), and return this... But
Jackson will not get the chance to be confused or unhappy about that
type's structure.

This was the root cause of aws/aws-cdk#4080
  • Loading branch information
rix0rrr authored and mergify[bot] committed Nov 26, 2019
1 parent fea0f0a commit 9eacbe3
Show file tree
Hide file tree
Showing 17 changed files with 770 additions and 9 deletions.
26 changes: 24 additions & 2 deletions packages/jsii-calc/lib/compliance.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2332,10 +2332,10 @@ export class JsonFormatter {
return () => 'boom';
}

public static anyDate(): any {
public static anyDate(): any {
return new Date('2019-11-18T13:01:20.515Z');
}

public static anyNumber(): any {
return 123;
}
Expand Down Expand Up @@ -2374,3 +2374,25 @@ export class JsonFormatter {

private constructor() { }
}

/**
* This tries to confuse Jackson by having overloaded property setters.
*
* @see https://github.com/aws/aws-cdk/issues/4080
*/
export class ConfusingToJackson {
public static makeInstance(): ConfusingToJackson {
return new ConfusingToJackson();
}

public static makeStructInstance(): ConfusingToJacksonStruct {
return {};
}

public unionProperty?: Array<IFriendly | AbstractClass> | IFriendly;

private constructor() { }
}
export interface ConfusingToJacksonStruct {
readonly unionProperty?: Array<IFriendly | AbstractClass> | IFriendly;
}
145 changes: 144 additions & 1 deletion packages/jsii-calc/test/assembly.jsii
Original file line number Diff line number Diff line change
Expand Up @@ -2221,6 +2221,149 @@
}
]
},
"jsii-calc.ConfusingToJackson": {
"assembly": "jsii-calc",
"docs": {
"see": "https://github.com/aws/aws-cdk/issues/4080",
"stability": "experimental",
"summary": "This tries to confuse Jackson by having overloaded property setters."
},
"fqn": "jsii-calc.ConfusingToJackson",
"kind": "class",
"locationInModule": {
"filename": "lib/compliance.ts",
"line": 2383
},
"methods": [
{
"docs": {
"stability": "experimental"
},
"locationInModule": {
"filename": "lib/compliance.ts",
"line": 2384
},
"name": "makeInstance",
"returns": {
"type": {
"fqn": "jsii-calc.ConfusingToJackson"
}
},
"static": true
},
{
"docs": {
"stability": "experimental"
},
"locationInModule": {
"filename": "lib/compliance.ts",
"line": 2388
},
"name": "makeStructInstance",
"returns": {
"type": {
"fqn": "jsii-calc.ConfusingToJacksonStruct"
}
},
"static": true
}
],
"name": "ConfusingToJackson",
"properties": [
{
"docs": {
"stability": "experimental"
},
"locationInModule": {
"filename": "lib/compliance.ts",
"line": 2392
},
"name": "unionProperty",
"optional": true,
"type": {
"union": {
"types": [
{
"fqn": "@scope/jsii-calc-lib.IFriendly"
},
{
"collection": {
"elementtype": {
"union": {
"types": [
{
"fqn": "@scope/jsii-calc-lib.IFriendly"
},
{
"fqn": "jsii-calc.AbstractClass"
}
]
}
},
"kind": "array"
}
}
]
}
}
}
]
},
"jsii-calc.ConfusingToJacksonStruct": {
"assembly": "jsii-calc",
"datatype": true,
"docs": {
"stability": "experimental"
},
"fqn": "jsii-calc.ConfusingToJacksonStruct",
"kind": "interface",
"locationInModule": {
"filename": "lib/compliance.ts",
"line": 2396
},
"name": "ConfusingToJacksonStruct",
"properties": [
{
"abstract": true,
"docs": {
"stability": "experimental"
},
"immutable": true,
"locationInModule": {
"filename": "lib/compliance.ts",
"line": 2397
},
"name": "unionProperty",
"optional": true,
"type": {
"union": {
"types": [
{
"fqn": "@scope/jsii-calc-lib.IFriendly"
},
{
"collection": {
"elementtype": {
"union": {
"types": [
{
"fqn": "@scope/jsii-calc-lib.IFriendly"
},
{
"fqn": "jsii-calc.AbstractClass"
}
]
}
},
"kind": "array"
}
}
]
}
}
}
]
},
"jsii-calc.ConstructorPassesThisOut": {
"assembly": "jsii-calc",
"docs": {
Expand Down Expand Up @@ -11525,5 +11668,5 @@
}
},
"version": "0.20.8",
"fingerprint": "N6rN2lItGrVvvguYWKJffMgN+NaFLr5GQp2a1fmrbx0="
"fingerprint": "s++IK3929zHRbg2MXmnn2A+PdvZnMVvPwaarJuUeBNM="
}
Original file line number Diff line number Diff line change
Expand Up @@ -1034,7 +1034,7 @@ public void VariadicCallbacksAreHandledCorrectly()
}

[Fact(DisplayName = Prefix + nameof(ReturnSubclassThatImplementsInterface976))]
public void ReturnSubclassThatImplementsInterface976()
public void ReturnSubclassThatImplementsInterface976()
{
var obj = SomeTypeJsii976.ReturnReturn();
Assert.Equal(333, obj.Foo);
Expand Down Expand Up @@ -1291,12 +1291,24 @@ public void StructsAreUndecoratedOntheWayToKernel()
{
var json = JsonFormatter.Stringify(new StructB {RequiredString = "Bazinga!", OptionalBoolean = false});
var actual = JObject.Parse(json);

var expected = new JObject();
expected.Add("RequiredString", "Bazinga!");
expected.Add("OptionalBoolean", false);

Assert.Equal(expected, actual);
}

[Fact(DisplayName = Prefix + nameof(CanObtainReferenceWithOverloadedSetters))]
public void CanObtainReferenceWithOverloadedSetters()
{
Assert.NotNull(ConfusingToJackson.MakeInstance());
}

[Fact(DisplayName = Prefix + nameof(CanObtainStructReferenceWithOverloadedSetters))]
public void CanObtainStructReferenceWithOverloadedSetters()
{
Assert.NotNull(ConfusingToJackson.MakeStructInstance());
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -1584,4 +1584,14 @@ public void structsAreUndecoratedOntheWayToKernel() throws IOException {

assertEquals(expected, actual);
}

@Test
public void canObtainReferenceWithOverloadedSetter() {
assertNotNull(ConfusingToJackson.makeInstance());
}

@Test
public void canObtainStructReferenceWithOverloadedSetter() {
assertNotNull(ConfusingToJackson.makeStructInstance());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -50,15 +50,22 @@ public static <T> T treeToValue(final JsonNode tree, final Class<T> valueType) {
return null;
}
try {
final T result = INSTANCE.treeToValue(tree, valueType);
// If the needed type is a sub-class of JsiiObject, we'll be receiving it by-reference, so we can ask Jackson to
// de-serialize a JsiiObject instead of the actual type; and we'll still get the correct instance type. This
// avoids running into problems because of Jackson not liking the structure of a particular class (it will
// validate that before attempting any deserialization operation, and I don't know how to mute this behavior).
final Class<?> deserType = JsiiObject.class.isAssignableFrom(valueType)
? JsiiObject.class
: valueType;
final Object result = INSTANCE.treeToValue(tree, deserType);
if (result != null && valueType.isInterface() && result instanceof JsiiObject) {
// The result type does not implement the interface, returning the proxy instead!
if (!valueType.isAssignableFrom(result.getClass()) && valueType.isAnnotationPresent(Jsii.Proxy.class)) {
final Jsii.Proxy proxyAnnotation = valueType.getAnnotation(Jsii.Proxy.class);
return (T)((JsiiObject) result).asInterfaceProxy(proxyAnnotation.value());
}
}
return result;
return (T)result;
} catch (final JsonProcessingException jpe) {
throw new JsiiException(jpe);
}
Expand Down
Loading

0 comments on commit 9eacbe3

Please sign in to comment.