From 9c1ebfd0689576f416aae8fadf1a3236ea15643a Mon Sep 17 00:00:00 2001 From: JordonPhillips Date: Thu, 15 Jul 2021 13:43:15 +0200 Subject: [PATCH 1/3] Update NodeMapper to handle Trait sourceLocation This updates NodeMapper so that it can properly set source location on traits that use the AbstractTraitBuilder. Unfortunately the fix implemented here is a hack that looks for that specific case, and the same root cause may bite us elsewhere. The true root cause is that our default deserializer is unable to resolve generics. AbstractTraitBuilder generically defines the return value of the setter `sourceLocation` to `B extends AbstractTraitBuilder`. When we use reflection to look at the return type, all we see is `AbstractTraitBuilder`. We *can* also get the type variable, but actually resolving it will take a decent amount of effort as that functionality isn't built in. --- .../model/node/DefaultNodeDeserializers.java | 16 ++++- .../smithy/model/node/NodeMapperTest.java | 61 ++++++++++++++++++- 2 files changed, 74 insertions(+), 3 deletions(-) diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/node/DefaultNodeDeserializers.java b/smithy-model/src/main/java/software/amazon/smithy/model/node/DefaultNodeDeserializers.java index 008c5551eba..f74db673c42 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/node/DefaultNodeDeserializers.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/node/DefaultNodeDeserializers.java @@ -47,6 +47,7 @@ import software.amazon.smithy.model.FromSourceLocation; import software.amazon.smithy.model.SourceLocation; import software.amazon.smithy.model.shapes.ShapeId; +import software.amazon.smithy.model.traits.AbstractTraitBuilder; import software.amazon.smithy.utils.ListUtils; import software.amazon.smithy.utils.MapUtils; import software.amazon.smithy.utils.Pair; @@ -373,8 +374,8 @@ private static boolean isBeanOrBuilderSetter(Method method, Class type, Strin return false; } - // Must either return the target class itself (like a builder) or void. - if (method.getReturnType() != void.class && method.getReturnType() != type) { + + if (!hasSetterReturnType(method, type, propertyName)) { return false; } @@ -391,6 +392,16 @@ private static boolean isBeanOrBuilderSetter(Method method, Class type, Strin return false; } + private static boolean hasSetterReturnType(Method method, Class type, String propertyName) { + // This is a hack to make source location work. Work needs to be done in resolveClassFromType to + // make sourceLocation work without a hack like this. + if (propertyName.equals("sourceLocation") && method.getReturnType() == AbstractTraitBuilder.class) { + return true; + } + // Must either return the target class itself (like a builder) or void. + return method.getReturnType() == void.class || method.getReturnType() == type; + } + private static Class determineParameterizedType(Method setter) { Type genericParameters = setter.getGenericParameterTypes()[0]; Class containingType = setter.getParameterTypes()[0]; @@ -431,6 +442,7 @@ private static Class resolveClassFromType(Type type) { } else if (type instanceof WildcardType) { return resolveClassFromType(((WildcardType) type).getUpperBounds()[0]); } else if (type instanceof TypeVariable) { + // TODO: implement this to enable sourceLocation setting without hacks throw new IllegalArgumentException("TypeVariable targets are not implemented: " + type); } else if (type instanceof GenericArrayType) { throw new IllegalArgumentException("GenericArrayType targets are not implemented: " + type); diff --git a/smithy-model/src/test/java/software/amazon/smithy/model/node/NodeMapperTest.java b/smithy-model/src/test/java/software/amazon/smithy/model/node/NodeMapperTest.java index 0c0b07ee52d..74b6e66be43 100644 --- a/smithy-model/src/test/java/software/amazon/smithy/model/node/NodeMapperTest.java +++ b/smithy-model/src/test/java/software/amazon/smithy/model/node/NodeMapperTest.java @@ -14,7 +14,6 @@ import java.net.URI; import java.net.URL; import java.nio.file.Path; -import java.nio.file.Paths; import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; @@ -34,9 +33,14 @@ import software.amazon.smithy.model.FromSourceLocation; import software.amazon.smithy.model.SourceLocation; import software.amazon.smithy.model.shapes.ShapeId; +import software.amazon.smithy.model.traits.AbstractTrait; +import software.amazon.smithy.model.traits.AbstractTrait.Provider; +import software.amazon.smithy.model.traits.AbstractTraitBuilder; import software.amazon.smithy.model.traits.RangeTrait; +import software.amazon.smithy.model.traits.Trait; import software.amazon.smithy.utils.ListUtils; import software.amazon.smithy.utils.SmithyBuilder; +import software.amazon.smithy.utils.ToSmithyBuilder; public class NodeMapperTest { @Test @@ -1197,6 +1201,19 @@ public void deserializesIntoValueWithSourceLocation() { assertThat(sourceLocation, equalTo(hasSourceLocation.sourceLocation)); } + @Test + public void deserializesIntoTraitWithSourceLocation() { + NodeMapper mapper = new NodeMapper(); + Map mappings = new HashMap<>(); + mappings.put(StringNode.from("foo"), StringNode.from("foo")); + SourceLocation sourceLocation = new SourceLocation("/abc/def"); + Node node = new ObjectNode(mappings, sourceLocation); + SourceLocationBearerTrait trait = mapper.deserialize(node, SourceLocationBearerTrait.class); + + assertThat(trait.getSourceLocation(), equalTo(sourceLocation)); + assertThat(trait.getFoo(), equalTo("foo")); + } + @Test public void doesNotSerializeSourceLocation() { NodeMapper mapper = new NodeMapper(); @@ -1231,6 +1248,48 @@ public String getFoo() { } } + private static class SourceLocationBearerTrait extends AbstractTrait implements ToSmithyBuilder { + public static final ShapeId ID = ShapeId.from("smithy.test#sourceLocationBearer"); + private final String foo; + + public SourceLocationBearerTrait(Builder builder) { + super(ID, builder.getSourceLocation()); + this.foo = builder.foo; + } + + public String getFoo() { + return foo; + } + + @Override + protected Node createNode() { + return new NodeMapper().serialize(this); + } + + @Override + public SmithyBuilder toBuilder() { + return new Builder().sourceLocation(getSourceLocation()); + } + + public static Builder builder() { + return new Builder(); + } + + public static final class Builder extends AbstractTraitBuilder { + private String foo; + + @Override + public SourceLocationBearerTrait build() { + return new SourceLocationBearerTrait(this); + } + + public Builder foo(String foo) { + this.foo = foo; + return this; + } + } + } + @Test public void deserializesEnums() { NodeMapper mapper = new NodeMapper(); From 17aec7355d05424190b28026aca32a7bc079eae0 Mon Sep 17 00:00:00 2001 From: JordonPhillips Date: Fri, 16 Jul 2021 12:44:13 +0200 Subject: [PATCH 2/3] Make NodeMapper pass along source location This updates the serializer for NodeMapper to pass along the source location. --- .../model/node/DefaultNodeSerializers.java | 9 ++++++- .../smithy/model/node/NodeMapperTest.java | 24 ++++++++++++++++--- 2 files changed, 29 insertions(+), 4 deletions(-) diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/node/DefaultNodeSerializers.java b/smithy-model/src/main/java/software/amazon/smithy/model/node/DefaultNodeSerializers.java index f2b61875b5e..98371639dc5 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/node/DefaultNodeSerializers.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/node/DefaultNodeSerializers.java @@ -65,6 +65,7 @@ public Node serialize(ToNode value, Set serializedObjects, NodeMapper ma return null; } + // TODO: make sure every instance of `toNode` is setting this return value.toNode(); } }; @@ -362,7 +363,13 @@ public Node serialize(Object value, Set serializedObjects, NodeMapper ma // multiple times (like in a List). serializedObjects.remove(value); - return new ObjectNode(mappings, SourceLocation.NONE); + // Pass on the source location if it is present. + SourceLocation sourceLocation = SourceLocation.NONE; + if (value instanceof FromSourceLocation) { + sourceLocation = ((FromSourceLocation) value).getSourceLocation(); + } + + return new ObjectNode(mappings, sourceLocation); } private boolean canSerialize(NodeMapper mapper, Node value) { diff --git a/smithy-model/src/test/java/software/amazon/smithy/model/node/NodeMapperTest.java b/smithy-model/src/test/java/software/amazon/smithy/model/node/NodeMapperTest.java index 74b6e66be43..879e955939d 100644 --- a/smithy-model/src/test/java/software/amazon/smithy/model/node/NodeMapperTest.java +++ b/smithy-model/src/test/java/software/amazon/smithy/model/node/NodeMapperTest.java @@ -1215,15 +1215,31 @@ public void deserializesIntoTraitWithSourceLocation() { } @Test - public void doesNotSerializeSourceLocation() { + public void doesNotSerializeSourceLocationAsKey() { NodeMapper mapper = new NodeMapper(); HasSourceLocation hs = new HasSourceLocation(); - hs.setSourceLocation(new SourceLocation("/foo/baz")); + SourceLocation sourceLocation = new SourceLocation("/foo/baz"); + hs.setSourceLocation(sourceLocation); hs.setFoo("hi"); Node result = mapper.serialize(hs); assertThat(result.expectObjectNode().getStringMap(), hasKey("foo")); assertThat(result.expectObjectNode().getStringMap(), not(hasKey("sourceLocation"))); + assertThat(result.getSourceLocation(), equalTo(sourceLocation)); + } + + @Test + public void passesSourceLocationFromTrait() { + SourceLocation sourceLocation = new SourceLocation("/foo/baz"); + SourceLocationBearerTrait trait = SourceLocationBearerTrait.builder() + .sourceLocation(sourceLocation) + .foo("foo") + .build(); + Node result = trait.createNode(); + + assertThat(result.expectObjectNode().getStringMap(), hasKey("foo")); + assertThat(result.expectObjectNode().getStringMap(), not(hasKey("sourceLocation"))); + assertThat(result.getSourceLocation(), equalTo(sourceLocation)); } private static class HasSourceLocation implements FromSourceLocation { @@ -1263,7 +1279,9 @@ public String getFoo() { @Override protected Node createNode() { - return new NodeMapper().serialize(this); + NodeMapper mapper = new NodeMapper(); + mapper.disableToNodeForClass(SourceLocationBearerTrait.class); + return mapper.serialize(this); } @Override From 187b9fd22159d564be85b97d722d6084b2bc31ee Mon Sep 17 00:00:00 2001 From: JordonPhillips Date: Mon, 19 Jul 2021 12:06:27 +0200 Subject: [PATCH 3/3] Check if builder return type is self-assignable This updates the builder detection in the node deserializer to check to see if the builder type is assignable to a given method's return type. This removes the need for a hack enabling source location, but it's still not perfect. Ideally we should attempt to resolve any generic return types. --- .../model/node/DefaultNodeDeserializers.java | 18 ++++-------------- .../smithy/model/node/NodeMapperTest.java | 6 ++++-- 2 files changed, 8 insertions(+), 16 deletions(-) diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/node/DefaultNodeDeserializers.java b/smithy-model/src/main/java/software/amazon/smithy/model/node/DefaultNodeDeserializers.java index f74db673c42..f3dd0585c66 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/node/DefaultNodeDeserializers.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/node/DefaultNodeDeserializers.java @@ -47,7 +47,6 @@ import software.amazon.smithy.model.FromSourceLocation; import software.amazon.smithy.model.SourceLocation; import software.amazon.smithy.model.shapes.ShapeId; -import software.amazon.smithy.model.traits.AbstractTraitBuilder; import software.amazon.smithy.utils.ListUtils; import software.amazon.smithy.utils.MapUtils; import software.amazon.smithy.utils.Pair; @@ -374,8 +373,9 @@ private static boolean isBeanOrBuilderSetter(Method method, Class type, Strin return false; } - - if (!hasSetterReturnType(method, type, propertyName)) { + // Must either return the target class itself (like a builder) or void. + // Ideally we should attempt to resolve any generics and make an assertion of the concrete type. + if (!(method.getReturnType() == void.class || method.getReturnType().isAssignableFrom(type))) { return false; } @@ -392,16 +392,6 @@ private static boolean isBeanOrBuilderSetter(Method method, Class type, Strin return false; } - private static boolean hasSetterReturnType(Method method, Class type, String propertyName) { - // This is a hack to make source location work. Work needs to be done in resolveClassFromType to - // make sourceLocation work without a hack like this. - if (propertyName.equals("sourceLocation") && method.getReturnType() == AbstractTraitBuilder.class) { - return true; - } - // Must either return the target class itself (like a builder) or void. - return method.getReturnType() == void.class || method.getReturnType() == type; - } - private static Class determineParameterizedType(Method setter) { Type genericParameters = setter.getGenericParameterTypes()[0]; Class containingType = setter.getParameterTypes()[0]; @@ -442,7 +432,7 @@ private static Class resolveClassFromType(Type type) { } else if (type instanceof WildcardType) { return resolveClassFromType(((WildcardType) type).getUpperBounds()[0]); } else if (type instanceof TypeVariable) { - // TODO: implement this to enable sourceLocation setting without hacks + // TODO: implement this to enable improved builder detection throw new IllegalArgumentException("TypeVariable targets are not implemented: " + type); } else if (type instanceof GenericArrayType) { throw new IllegalArgumentException("GenericArrayType targets are not implemented: " + type); diff --git a/smithy-model/src/test/java/software/amazon/smithy/model/node/NodeMapperTest.java b/smithy-model/src/test/java/software/amazon/smithy/model/node/NodeMapperTest.java index 879e955939d..3a8847e7992 100644 --- a/smithy-model/src/test/java/software/amazon/smithy/model/node/NodeMapperTest.java +++ b/smithy-model/src/test/java/software/amazon/smithy/model/node/NodeMapperTest.java @@ -1215,7 +1215,7 @@ public void deserializesIntoTraitWithSourceLocation() { } @Test - public void doesNotSerializeSourceLocationAsKey() { + public void serializesSourceLocationFromValue() { NodeMapper mapper = new NodeMapper(); HasSourceLocation hs = new HasSourceLocation(); SourceLocation sourceLocation = new SourceLocation("/foo/baz"); @@ -1224,12 +1224,13 @@ public void doesNotSerializeSourceLocationAsKey() { Node result = mapper.serialize(hs); assertThat(result.expectObjectNode().getStringMap(), hasKey("foo")); + // The sourceLocation needs to be set on the node, not as a key. assertThat(result.expectObjectNode().getStringMap(), not(hasKey("sourceLocation"))); assertThat(result.getSourceLocation(), equalTo(sourceLocation)); } @Test - public void passesSourceLocationFromTrait() { + public void serializesSourceLocationFromTrait() { SourceLocation sourceLocation = new SourceLocation("/foo/baz"); SourceLocationBearerTrait trait = SourceLocationBearerTrait.builder() .sourceLocation(sourceLocation) @@ -1238,6 +1239,7 @@ public void passesSourceLocationFromTrait() { Node result = trait.createNode(); assertThat(result.expectObjectNode().getStringMap(), hasKey("foo")); + // The sourceLocation needs to be set on the node, not as a key. assertThat(result.expectObjectNode().getStringMap(), not(hasKey("sourceLocation"))); assertThat(result.getSourceLocation(), equalTo(sourceLocation)); }