Made code generation more resilient against bad method names.

When generating models that would collide with existing methods or use reserved words, rename the generated methods to end with 'Value' (e.g. ConfirmProductInstanceResult.return -> ConfirmProductInstanceResult.returnValue).

Author: millems

codegen/src/main/java/software/amazon/awssdk/codegen/AddShapes.java

@@ -138,7 +138,7 @@ private MemberModel generateMemberModel(String c2jMemberName, Member c2jMemberDe
                                             String protocol, Shape parentShape,
                                             Map<String, Shape> allC2jShapes) {
         String c2jShapeName = c2jMemberDefinition.getShape();
-        Shape c2jShape = allC2jShapes.get(c2jShapeName);
+        Shape shape = allC2jShapes.get(c2jShapeName);
         String variableName = getNamingStrategy().getVariableName(c2jMemberName);
         String variableType = getTypeUtils().getJavaDataType(allC2jShapes, c2jShapeName);
         String variableDeclarationType = getTypeUtils()
@@ -163,17 +163,17 @@ private MemberModel generateMemberModel(String c2jMemberName, Member c2jMemberDe
                                          .withDocumentation(c2jMemberDefinition.getDocumentation()))
                    .withSetterModel(new VariableModel(variableName, variableType, variableDeclarationType))
                    .withGetterModel(new ReturnTypeModel(variableType))
-                   .withTimestampFormat(resolveTimestampFormat(c2jMemberDefinition, c2jShape))
+                   .withTimestampFormat(resolveTimestampFormat(c2jMemberDefinition, shape))
                    .withJsonValue(c2jMemberDefinition.getJsonValue());
         memberModel.setDocumentation(c2jMemberDefinition.getDocumentation());
         memberModel.setDeprecated(c2jMemberDefinition.isDeprecated());
         memberModel
-                .withFluentGetterMethodName(namingStrategy.getFluentGetterMethodName(c2jMemberName, c2jShape))
-                .withFluentEnumGetterMethodName(namingStrategy.getFluentEnumGetterMethodName(c2jMemberName, c2jShape))
-                .withFluentSetterMethodName(namingStrategy.getFluentSetterMethodName(c2jMemberName, c2jShape))
-                .withFluentEnumSetterMethodName(namingStrategy.getFluentEnumSetterMethodName(c2jMemberName, c2jShape))
-                .withBeanStyleGetterMethodName(namingStrategy.getBeanStyleGetterMethodName(c2jMemberName))
-                .withBeanStyleSetterMethodName(namingStrategy.getBeanStyleSetterMethodName(c2jMemberName));
+                .withFluentGetterMethodName(namingStrategy.getFluentGetterMethodName(c2jMemberName, parentShape, shape))
+                .withFluentEnumGetterMethodName(namingStrategy.getFluentEnumGetterMethodName(c2jMemberName, parentShape, shape))
+                .withFluentSetterMethodName(namingStrategy.getFluentSetterMethodName(c2jMemberName, parentShape, shape))
+                .withFluentEnumSetterMethodName(namingStrategy.getFluentEnumSetterMethodName(c2jMemberName, parentShape, shape))
+                .withBeanStyleGetterMethodName(namingStrategy.getBeanStyleGetterMethodName(c2jMemberName, parentShape, shape))
+                .withBeanStyleSetterMethodName(namingStrategy.getBeanStyleSetterMethodName(c2jMemberName, parentShape, shape));
         memberModel.setIdempotencyToken(c2jMemberDefinition.isIdempotencyToken());
         memberModel.setEventPayload(c2jMemberDefinition.isEventPayload());
         memberModel.setEventHeader(c2jMemberDefinition.isEventHeader());
@@ -195,7 +195,7 @@ private MemberModel generateMemberModel(String c2jMemberName, Member c2jMemberDe
 
         String payload = parentShape.getPayload();
 
-        boolean shapeIsStreaming = c2jShape.isStreaming();
+        boolean shapeIsStreaming = shape.isStreaming();
         boolean memberIsStreaming = c2jMemberDefinition.isStreaming();
         boolean payloadIsStreaming = shapeIsStreaming || memberIsStreaming;
 

codegen/src/main/java/software/amazon/awssdk/codegen/internal/Constant.java

@@ -66,7 +66,7 @@ public final class Constant {
 
     public static final String FAULT_CLASS_SUFFIX = "Fault";
 
-    public static final String VARIABLE_NAME_SUFFIX = "Value";
+    public static final String CONFLICTING_NAME_SUFFIX = "Value";
 
     public static final String AUTHORIZER_NAME_PREFIX = "I";
 

codegen/src/main/java/software/amazon/awssdk/codegen/naming/DefaultNamingStrategy.java

@@ -17,11 +17,11 @@
 
 import static java.util.stream.Collectors.joining;
 import static software.amazon.awssdk.codegen.internal.Constant.AUTHORIZER_NAME_PREFIX;
+import static software.amazon.awssdk.codegen.internal.Constant.CONFLICTING_NAME_SUFFIX;
 import static software.amazon.awssdk.codegen.internal.Constant.EXCEPTION_CLASS_SUFFIX;
 import static software.amazon.awssdk.codegen.internal.Constant.FAULT_CLASS_SUFFIX;
 import static software.amazon.awssdk.codegen.internal.Constant.REQUEST_CLASS_SUFFIX;
 import static software.amazon.awssdk.codegen.internal.Constant.RESPONSE_CLASS_SUFFIX;
-import static software.amazon.awssdk.codegen.internal.Constant.VARIABLE_NAME_SUFFIX;
 import static software.amazon.awssdk.codegen.internal.Utils.unCapitalize;
 
 import java.util.Arrays;
@@ -50,16 +50,41 @@ public class DefaultNamingStrategy implements NamingStrategy {
 
     private static final Set<String> RESERVED_KEYWORDS;
 
+    private static final Set<String> RESERVED_EXCEPTION_METHOD_NAMES;
+
+    private static final Set<Object> RESERVED_STRUCTURE_METHOD_NAMES;
+
     static {
-        Set<String> keywords = new HashSet<>();
-        Collections.addAll(keywords,
+        Set<String> reservedJavaKeywords = new HashSet<>();
+        Collections.addAll(reservedJavaKeywords,
                            "abstract", "assert", "boolean", "break", "byte", "case", "catch", "char", "class",
-                           "continue", "default", "do", "double", "else", "enum", "extends", "final", "finally", "float", "for",
-                           "if", "implements", "import", "instanceof", "int", "interface", "long", "native", "new", "package",
-                           "private", "protected", "public", "return", "short", "static", "strictfp", "super", "switch",
-                           "synchronized", "this", "throw", "throws", "transient", "try", "void", "volatile", "while", "true",
-                           "null", "false", "const", "goto");
-        RESERVED_KEYWORDS = Collections.unmodifiableSet(keywords);
+                           "const", "continue", "default", "do", "double", "else", "enum", "extends", "false", "final",
+                           "finally", "float", "for", "goto", "if", "implements", "import", "instanceof", "int",
+                           "interface", "long", "native", "new", "null", "package", "private", "protected", "public",
+                           "return", "short", "static", "strictfp", "super", "switch", "synchronized", "this", "throw",
+                           "throws", "transient", "true", "try", "void", "volatile", "while");
+        RESERVED_KEYWORDS = Collections.unmodifiableSet(reservedJavaKeywords);
+
+
+        Set<String> reservedJavaMethodNames = new HashSet<>();
+        Collections.addAll(reservedJavaMethodNames,
+                           "equals", "finalize", "getClass", "hashCode", "notify", "notifyAll", "toString", "wait");
+
+        Set<String> reserveJavaPojoMethodNames = new HashSet<>(reservedJavaMethodNames);
+        Collections.addAll(reserveJavaPojoMethodNames,
+                           "builder", "sdkFields", "toBuilder");
+
+        Set<String> reservedExceptionMethodNames = new HashSet<>(reserveJavaPojoMethodNames);
+        Collections.addAll(reservedExceptionMethodNames,
+                           "awsErrorDetails", "cause", "fillInStackTrace", "getCause", "getLocalizedMessage",
+                           "getMessage", "getStackTrace", "getSuppressed", "isClockSkewException", "isThrottlingException",
+                           "printStackTrace", "requestId", "retryable", "serializableBuilderClass", "statusCode");
+        RESERVED_EXCEPTION_METHOD_NAMES = Collections.unmodifiableSet(reservedExceptionMethodNames);
+
+        Set<String> reservedStructureMethodNames = new HashSet<>(reserveJavaPojoMethodNames);
+        Collections.addAll(reservedStructureMethodNames,
+                           "overrideConfiguration", "sdkHttpResponse");
+        RESERVED_STRUCTURE_METHOD_NAMES = Collections.unmodifiableSet(reservedStructureMethodNames);
     }
 
     private final ServiceModel serviceModel;
@@ -189,11 +214,15 @@ public String getResponseClassName(String operationName) {
 
     @Override
     public String getVariableName(String name) {
-        if (isJavaKeyword(name)) {
-            return unCapitalize(name + VARIABLE_NAME_SUFFIX);
-        } else {
-            return unCapitalize(name);
+        // Exclude keywords because they will not compile, and exclude reserved method names because they're frequently
+        // used for local variable names.
+        if (RESERVED_KEYWORDS.contains(name) ||
+            RESERVED_STRUCTURE_METHOD_NAMES.contains(name) ||
+            RESERVED_EXCEPTION_METHOD_NAMES.contains(name)) {
+            return unCapitalize(name + CONFLICTING_NAME_SUFFIX);
         }
+
+        return unCapitalize(name);
     }
 
     @Override
@@ -236,9 +265,11 @@ public String getAuthorizerClassName(String shapeName) {
     }
 
     @Override
-    public String getFluentGetterMethodName(String memberName, Shape shape) {
+    public String getFluentGetterMethodName(String memberName, Shape parentShape, Shape shape) {
         String getterMethodName = Utils.unCapitalize(memberName);
 
+        getterMethodName = rewriteInvalidMemberName(getterMethodName, parentShape);
+
         if (Utils.isOrContainsEnumShape(shape, serviceModel.getShapes())) {
             getterMethodName += "AsString";
 
@@ -251,33 +282,34 @@ public String getFluentGetterMethodName(String memberName, Shape shape) {
     }
 
     @Override
-    public String getFluentEnumGetterMethodName(String memberName, Shape shape) {
+    public String getFluentEnumGetterMethodName(String memberName, Shape parentShape, Shape shape) {
         if (!Utils.isOrContainsEnumShape(shape, serviceModel.getShapes())) {
             return null;
         }
 
-        return Utils.unCapitalize(memberName);
-    }
-
-    @Override
-    public String getBeanStyleGetterMethodName(String memberName) {
-        return String.format("get%s", Utils.capitalize(memberName));
+        String getterMethodName = Utils.unCapitalize(memberName);
+        getterMethodName = rewriteInvalidMemberName(getterMethodName, parentShape);
+        return getterMethodName;
     }
 
     @Override
-    public String getSetterMethodName(String memberName) {
-        return Utils.unCapitalize(memberName);
+    public String getBeanStyleGetterMethodName(String memberName, Shape parentShape, Shape c2jShape) {
+        String fluentGetterMethodName = getFluentGetterMethodName(memberName, parentShape, c2jShape);
+        return String.format("get%s", Utils.capitalize(fluentGetterMethodName));
     }
 
     @Override
-    public String getBeanStyleSetterMethodName(String memberName) {
-        return String.format("set%s", Utils.capitalize(memberName));
+    public String getBeanStyleSetterMethodName(String memberName, Shape parentShape, Shape c2jShape) {
+        String fluentSetterMethodName = getFluentSetterMethodName(memberName, parentShape, c2jShape);
+        return String.format("set%s", Utils.capitalize(fluentSetterMethodName));
     }
 
     @Override
-    public String getFluentSetterMethodName(String memberName, Shape shape) {
+    public String getFluentSetterMethodName(String memberName, Shape parentShape, Shape shape) {
         String setterMethodName = Utils.unCapitalize(memberName);
 
+        setterMethodName = rewriteInvalidMemberName(setterMethodName, parentShape);
+
         if (Utils.isOrContainsEnumShape(shape, serviceModel.getShapes()) &&
             (Utils.isListShape(shape) || Utils.isMapShape(shape))) {
 
@@ -288,19 +320,37 @@ public String getFluentSetterMethodName(String memberName, Shape shape) {
     }
 
     @Override
-    public String getFluentEnumSetterMethodName(String memberName, Shape shape) {
+    public String getFluentEnumSetterMethodName(String memberName, Shape parentShape, Shape shape) {
         if (!Utils.isOrContainsEnumShape(shape, serviceModel.getShapes())) {
             return null;
         }
 
-        return Utils.unCapitalize(memberName);
+        String setterMethodName = Utils.unCapitalize(memberName);
+        setterMethodName = rewriteInvalidMemberName(setterMethodName, parentShape);
+        return setterMethodName;
     }
 
     @Override
     public String getSdkFieldFieldName(MemberModel memberModel) {
         return screamCase(memberModel.getName()) + "_FIELD";
     }
 
+    private String rewriteInvalidMemberName(String memberName, Shape parentShape) {
+        if (isJavaKeyword(memberName) || isDisallowedNameForShape(memberName, parentShape)) {
+            return Utils.unCapitalize(memberName + CONFLICTING_NAME_SUFFIX);
+        }
+
+        return memberName;
+    }
+
+    private boolean isDisallowedNameForShape(String name, Shape parentShape) {
+        if (parentShape.isException()) {
+            return RESERVED_EXCEPTION_METHOD_NAMES.contains(name);
+        } else {
+            return RESERVED_STRUCTURE_METHOD_NAMES.contains(name);
+        }
+    }
+
     private String[] splitOnWordBoundaries(String toSplit) {
         String result = toSplit;
 

codegen/src/main/java/software/amazon/awssdk/codegen/naming/NamingStrategy.java

@@ -105,45 +105,39 @@ public interface NamingStrategy {
      * @param shape The shape associated with the member.
      * @return Name of the getter method for a model class member.
      */
-    String getFluentGetterMethodName(String memberName, Shape shape);
+    String getFluentGetterMethodName(String memberName, Shape parentShape, Shape shape);
 
     /**
      * @param memberName The full member to get the name for.
      * @param shape The shape associated with the member.
      * @return Name of the getter method for an enum model class member.
      */
-    String getFluentEnumGetterMethodName(String memberName, Shape shape);
+    String getFluentEnumGetterMethodName(String memberName, Shape parentShape, Shape shape);
 
     /**
      * @param memberName Member name to name getter for.
      * @return Name of the JavaBean getter method for model class member.
      */
-    String getBeanStyleGetterMethodName(String memberName);
-
-    /**
-     * @param memberName Member name to name setter for.
-     * @return Name of the setter method for a model class member.
-     */
-    String getSetterMethodName(String memberName);
+    String getBeanStyleGetterMethodName(String memberName, Shape parentShape, Shape c2jShape);
 
     /**
      * @param memberName Member name to name setter for.
      * @return Name of the JavaBean setter method for model class member.
      */
-    String getBeanStyleSetterMethodName(String memberName);
+    String getBeanStyleSetterMethodName(String memberName, Shape parentShape, Shape c2jShape);
 
     /**
      * @param memberName Member name to name fluent setter for.
      * @return Appropriate name to use for fluent setter method (i.e. withFoo) for a model class member.
      */
-    String getFluentSetterMethodName(String memberName, Shape shape);
+    String getFluentSetterMethodName(String memberName, Shape parentShape, Shape shape);
 
     /**
      * @param memberName The full member to get the name for.
      * @param shape The shape associated with the member.
      * @return Name of the getter method for an enum model class member.
      */
-    String getFluentEnumSetterMethodName(String memberName, Shape shape);
+    String getFluentEnumSetterMethodName(String memberName, Shape parentShape, Shape shape);
 
     /**
      * Stuttering is intentional, returns the name of the {@link SdkField} field.

codegen/src/main/java/software/amazon/awssdk/codegen/poet/model/AbstractMemberSetters.java

@@ -124,33 +124,36 @@ protected CodeBlock copySetterBuilderBody() {
 
     protected CodeBlock beanCopySetterBody() {
         if (memberModel.isSdkBytesType()) {
-            return sdkBytesGetter();
+            return sdkBytesSetter();
         }
         if (memberModel.isList() && memberModel.getListModel().getListMemberModel().isSdkBytesType()) {
-            return sdkBytesListGetter();
+            return sdkBytesListSetter();
         }
         if (memberModel.isMap() && memberModel.getMapModel().getValueModel().isSdkBytesType()) {
-            return sdkBytesMapValueGetter();
+            return sdkBytesMapValueSetter();
         }
 
         return copySetterBuilderBody();
     }
 
-    private CodeBlock sdkBytesGetter() {
-        return CodeBlock.of("$1N($1N == null ? null : $2T.fromByteBuffer($1N));",
-                            memberModel.getVariable().getVariableName(), SdkBytes.class);
+    private CodeBlock sdkBytesSetter() {
+        return CodeBlock.of("$1N($2N == null ? null : $3T.fromByteBuffer($2N));",
+                            memberModel.getFluentSetterMethodName(), fieldName(),
+                            SdkBytes.class);
     }
 
-    private CodeBlock sdkBytesListGetter() {
-        return CodeBlock.of("$1N($1N == null ? null : $1N.stream().map($2T::fromByteBuffer).collect($3T.toList()));",
-                            memberModel.getVariable().getVariableName(), SdkBytes.class, Collectors.class);
+    private CodeBlock sdkBytesListSetter() {
+        return CodeBlock.of("$1N($2N == null ? null : $2N.stream().map($3T::fromByteBuffer).collect($4T.toList()));",
+                            memberModel.getFluentSetterMethodName(), fieldName(),
+                            SdkBytes.class, Collectors.class);
     }
 
-    private CodeBlock sdkBytesMapValueGetter() {
-        return CodeBlock.of("$1N($1N == null ? null : " +
-                            "$1N.entrySet().stream()" +
-                            ".collect($3T.toMap(e -> e.getKey(), e -> $2T.fromByteBuffer(e.getValue()))));",
-                            memberModel.getVariable().getVariableName(), SdkBytes.class, Collectors.class);
+    private CodeBlock sdkBytesMapValueSetter() {
+        return CodeBlock.of("$1N($2N == null ? null : " +
+                            "$2N.entrySet().stream()" +
+                            ".collect($4T.toMap(e -> e.getKey(), e -> $3T.fromByteBuffer(e.getValue()))));",
+                            memberModel.getFluentSetterMethodName(), fieldName(),
+                            SdkBytes.class, Collectors.class);
     }
 
     protected ParameterSpec memberAsParameter() {

codegen/src/main/java/software/amazon/awssdk/codegen/poet/model/ListSetters.java

@@ -179,16 +179,18 @@ private MethodSpec fluentEnumVarargToListSetter(String methodName,
 
 
     private CodeBlock varargToListSetterBody() {
-        return CodeBlock.of("$1L($2T.asList($3L));", memberModel().getFluentSetterMethodName(), Arrays.class, fieldName());
+        return CodeBlock.of("$1L($2T.asList($3L));",
+                            memberModel().getFluentSetterMethodName(), Arrays.class, fieldName());
     }
 
     private CodeBlock consumerBuilderVarargSetterBody() {
-        return CodeBlock.of("$1L($2T.of($1L).map(c -> $3T.builder().applyMutation(c).build()).collect($4T.toList()));",
-                            fieldName(), Stream.class, listElementType(), Collectors.class);
+        return CodeBlock.of("$1L($3T.of($2L).map(c -> $4T.builder().applyMutation(c).build()).collect($5T.toList()));",
+                            memberModel().getFluentSetterMethodName(), fieldName(),
+                            Stream.class, listElementType(), Collectors.class);
     }
 
     private CodeBlock enumVarargToListSetterBody() {
-        return CodeBlock.of("$1L($2T.asList($1L));", fieldName(), Arrays.class);
+        return CodeBlock.of("$1L($3T.asList($2L));", memberModel().getFluentEnumSetterMethodName(), fieldName(), Arrays.class);
     }
 
     private MemberModel elementModel() {

codegen/src/main/java/software/amazon/awssdk/codegen/poet/model/NonCollectionSetters.java

@@ -130,7 +130,7 @@ private MethodSpec fluentConsumerFluentSetter(TypeName returnType) {
 
     private CodeBlock enumToStringAssignmentBody() {
         return CodeBlock.builder()
-                        .addStatement("this.$N($N.toString())", fieldName(), fieldName())
+                        .addStatement("this.$N($N.toString())", memberModel().getFluentSetterMethodName(), fieldName())
                         .build();
     }