groovy-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From pa...@apache.org
Subject groovy git commit: GROOVY-8186: Builder ExternalStrategy constructors have trouble with private fields (closes #547)
Date Tue, 29 Aug 2017 06:19:01 GMT
Repository: groovy
Updated Branches:
  refs/heads/GROOVY_2_5_X a2e635f9b -> bab4be2cf


GROOVY-8186: Builder ExternalStrategy constructors have trouble with private fields (closes
#547)


Project: http://git-wip-us.apache.org/repos/asf/groovy/repo
Commit: http://git-wip-us.apache.org/repos/asf/groovy/commit/bab4be2c
Tree: http://git-wip-us.apache.org/repos/asf/groovy/tree/bab4be2c
Diff: http://git-wip-us.apache.org/repos/asf/groovy/diff/bab4be2c

Branch: refs/heads/GROOVY_2_5_X
Commit: bab4be2cfc716a94cb09dd629b59ff570a4c59ef
Parents: a2e635f
Author: paulk <paulk@asert.com.au>
Authored: Tue May 23 14:17:45 2017 +1000
Committer: paulk <paulk@asert.com.au>
Committed: Tue Aug 29 16:18:51 2017 +1000

----------------------------------------------------------------------
 src/main/groovy/transform/builder/Builder.java  | 14 ++++++-
 .../transform/builder/DefaultStrategy.java      | 39 +++++++-----------
 .../transform/builder/ExternalStrategy.java     | 41 +------------------
 .../transform/builder/InitializerStrategy.java  |  1 +
 .../transform/builder/SimpleStrategy.java       |  1 +
 .../transform/BuilderASTTransformation.java     | 43 ++++++++++++++++++++
 .../transform/BuilderTransformTest.groovy       | 19 +++++++++
 7 files changed, 93 insertions(+), 65 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/groovy/blob/bab4be2c/src/main/groovy/transform/builder/Builder.java
----------------------------------------------------------------------
diff --git a/src/main/groovy/transform/builder/Builder.java b/src/main/groovy/transform/builder/Builder.java
index 127273c..8293f1f 100644
--- a/src/main/groovy/transform/builder/Builder.java
+++ b/src/main/groovy/transform/builder/Builder.java
@@ -60,8 +60,6 @@ import static org.codehaus.groovy.transform.BuilderASTTransformation.BuilderStra
  * </pre>
  * so you might not find value in using the builder transform at all. But if you need Java
integration or in some cases improved type safety, the {@code @Builder} transform might prove
very useful.
  *
- * @author Marcin Grzejszczak
- * @author Paul King
  * @see groovy.transform.builder.SimpleStrategy
  * @see groovy.transform.builder.ExternalStrategy
  * @see groovy.transform.builder.DefaultStrategy
@@ -147,4 +145,16 @@ public @interface Builder {
      * @since 2.5.0
      */
     boolean allNames() default false;
+
+    /**
+     * Whether to include all properties (as per the JavaBean spec) in the generated builder.
+     * Groovy recognizes any field-like definitions with no explicit visibility as property
definitions
+     * and always includes them in the {@code @Builder} generated classes. Groovy also treats
any explicitly created getXxx() or isYyy()
+     * methods as property getters as per the JavaBean specification. Old versions of Groovy
did not.
+     * So set this flag to false for the old behavior or if you want to explicitly exclude
such properties.
+     * Currently only supported by DefaultStrategy and ExternalStrategy.
+     *
+     * @since 2.5.0
+     */
+    boolean allProperties() default true;
 }

http://git-wip-us.apache.org/repos/asf/groovy/blob/bab4be2c/src/main/groovy/transform/builder/DefaultStrategy.java
----------------------------------------------------------------------
diff --git a/src/main/groovy/transform/builder/DefaultStrategy.java b/src/main/groovy/transform/builder/DefaultStrategy.java
index 3c4b048..c1f1ca6 100644
--- a/src/main/groovy/transform/builder/DefaultStrategy.java
+++ b/src/main/groovy/transform/builder/DefaultStrategy.java
@@ -54,7 +54,6 @@ import static org.codehaus.groovy.ast.tools.GenericsUtils.createGenericsSpec;
 import static org.codehaus.groovy.ast.tools.GenericsUtils.extractSuperClassGenerics;
 import static org.codehaus.groovy.ast.tools.GenericsUtils.newClass;
 import static org.codehaus.groovy.transform.AbstractASTTransformation.getMemberStringValue;
-import static org.codehaus.groovy.transform.AbstractASTTransformation.shouldSkipUndefinedAware;
 import static org.codehaus.groovy.transform.BuilderASTTransformation.NO_EXCEPTIONS;
 import static org.codehaus.groovy.transform.BuilderASTTransformation.NO_PARAMS;
 import static org.objectweb.asm.Opcodes.ACC_PRIVATE;
@@ -199,20 +198,21 @@ public class DefaultStrategy extends BuilderASTTransformation.AbstractBuilderStr
         createBuilderFactoryMethod(anno, buildee, builder);
         List<FieldNode> fields = getFields(transform, anno, buildee);
         boolean allNames = transform.memberHasValue(anno, "allNames", true);
-        List<FieldNode> filteredFields = selectFieldsFromExistingClass(fields, includes,
excludes, allNames);
-        for (FieldNode fieldNode : filteredFields) {
-            ClassNode correctedType = getCorrectedType(buildee, fieldNode);
-            String fieldName = fieldNode.getName();
+        boolean allProperties = !transform.memberHasValue(anno, "allProperties", false);
+        List<PropertyInfo> props = getPropertyInfos(transform, anno, buildee, excludes,
includes, allNames, allProperties);
+        for (PropertyInfo pi : props) {
+            ClassNode correctedType = getCorrectedType(buildee, pi.getType(), builder);
+            String fieldName = pi.getName();
             builder.addField(createFieldCopy(buildee, fieldName, correctedType));
             builder.addMethod(createBuilderMethodForProp(builder, new PropertyInfo(fieldName,
correctedType), getPrefix(anno)));
         }
-        builder.addMethod(createBuildMethod(anno, buildee, filteredFields));
+        builder.addMethod(createBuildMethod(anno, buildee, props));
     }
 
-    private static ClassNode getCorrectedType(ClassNode buildee, FieldNode fieldNode) {
-        Map<String,ClassNode> genericsSpec = createGenericsSpec(fieldNode.getDeclaringClass());
-        extractSuperClassGenerics(fieldNode.getType(), buildee, genericsSpec);
-        return correctToGenericsSpecRecurse(genericsSpec, fieldNode.getType());
+    private static ClassNode getCorrectedType(ClassNode buildee, ClassNode fieldType, ClassNode
declaringClass) {
+        Map<String,ClassNode> genericsSpec = createGenericsSpec(declaringClass);
+        extractSuperClassGenerics(fieldType, buildee, genericsSpec);
+        return correctToGenericsSpecRecurse(genericsSpec, fieldType);
     }
 
     private static void createBuilderFactoryMethod(AnnotationNode anno, ClassNode buildee,
ClassNode builder) {
@@ -254,10 +254,10 @@ public class DefaultStrategy extends BuilderASTTransformation.AbstractBuilderStr
         return new MethodNode(builderMethodName, PUBLIC_STATIC, builder, NO_PARAMS, NO_EXCEPTIONS,
body);
     }
 
-    private static MethodNode createBuildMethod(AnnotationNode anno, ClassNode buildee, List<FieldNode>
fields) {
+    private static MethodNode createBuildMethod(AnnotationNode anno, ClassNode buildee, List<PropertyInfo>
props) {
         String buildMethodName = getMemberStringValue(anno, "buildMethodName", "build");
         final BlockStatement body = new BlockStatement();
-        body.addStatement(returnS(initializeInstance(buildee, fields, body)));
+        body.addStatement(returnS(initializeInstance(buildee, props, body)));
         return new MethodNode(buildMethodName, ACC_PUBLIC, newClass(buildee), NO_PARAMS,
NO_EXCEPTIONS, body);
     }
 
@@ -282,20 +282,11 @@ public class DefaultStrategy extends BuilderASTTransformation.AbstractBuilderStr
         return new FieldNode(fieldName, ACC_PRIVATE, fieldType, buildee, DEFAULT_INITIAL_VALUE);
     }
 
-    private static List<FieldNode> selectFieldsFromExistingClass(List<FieldNode>
fieldNodes, List<String> includes, List<String> excludes, boolean allNames) {
-        List<FieldNode> fields = new ArrayList<FieldNode>();
-        for (FieldNode fNode : fieldNodes) {
-            if (shouldSkipUndefinedAware(fNode.getName(), excludes, includes, allNames))
continue;
-            fields.add(fNode);
-        }
-        return fields;
-    }
-
-    private static Expression initializeInstance(ClassNode buildee, List<FieldNode>
fields, BlockStatement body) {
+    private static Expression initializeInstance(ClassNode buildee, List<PropertyInfo>
props, BlockStatement body) {
         Expression instance = varX("_the" + buildee.getNameWithoutPackage(), buildee);
         body.addStatement(declS(instance, ctorX(buildee)));
-        for (FieldNode field : fields) {
-            body.addStatement(stmt(assignX(propX(instance, field.getName()), varX(field))));
+        for (PropertyInfo pi : props) {
+            body.addStatement(stmt(assignX(propX(instance, pi.getName()), varX(pi.getName(),
pi.getType()))));
         }
         return instance;
     }

http://git-wip-us.apache.org/repos/asf/groovy/blob/bab4be2c/src/main/groovy/transform/builder/ExternalStrategy.java
----------------------------------------------------------------------
diff --git a/src/main/groovy/transform/builder/ExternalStrategy.java b/src/main/groovy/transform/builder/ExternalStrategy.java
index 9596c19..29b6971 100644
--- a/src/main/groovy/transform/builder/ExternalStrategy.java
+++ b/src/main/groovy/transform/builder/ExternalStrategy.java
@@ -21,19 +21,13 @@ package groovy.transform.builder;
 import groovy.transform.Undefined;
 import org.codehaus.groovy.ast.AnnotatedNode;
 import org.codehaus.groovy.ast.AnnotationNode;
-import org.codehaus.groovy.ast.ClassHelper;
 import org.codehaus.groovy.ast.ClassNode;
 import org.codehaus.groovy.ast.FieldNode;
 import org.codehaus.groovy.ast.MethodNode;
 import org.codehaus.groovy.ast.expr.Expression;
 import org.codehaus.groovy.ast.stmt.BlockStatement;
-import org.codehaus.groovy.transform.AbstractASTTransformation;
 import org.codehaus.groovy.transform.BuilderASTTransformation;
 
-import java.beans.BeanInfo;
-import java.beans.IntrospectionException;
-import java.beans.Introspector;
-import java.beans.PropertyDescriptor;
 import java.util.ArrayList;
 import java.util.List;
 
@@ -92,9 +86,6 @@ import static org.objectweb.asm.Opcodes.ACC_PRIVATE;
  *
  * The {@code @Builder} 'builderMethodName' and 'builderClassName' annotation attributes
aren't applicable for this strategy.
  * The {@code @Builder} 'useSetters' annotation attribute is ignored by this strategy which
always uses setters.
- *
- * @author Marcin Grzejszczak
- * @author Paul King
  */
 public class ExternalStrategy extends BuilderASTTransformation.AbstractBuilderStrategy {
     private static final Expression DEFAULT_INITIAL_VALUE = null;
@@ -119,13 +110,9 @@ public class ExternalStrategy extends BuilderASTTransformation.AbstractBuilderSt
         if (includes.size() == 1 && Undefined.isUndefined(includes.get(0))) includes
= null;
         if (unsupportedAttribute(transform, anno, "builderClassName")) return;
         if (unsupportedAttribute(transform, anno, "builderMethodName")) return;
-        List<PropertyInfo> props;
         boolean allNames = transform.memberHasValue(anno, "allNames", true);
-        if (buildee.getModule() == null) {
-            props = getPropertyInfoFromBeanInfo(buildee, includes, excludes, allNames);
-        } else {
-            props = getPropertyInfoFromClassNode(transform, anno, buildee, includes, excludes,
allNames);
-        }
+        boolean allProperties = !transform.memberHasValue(anno, "allProperties", false);
+        List<PropertyInfo> props = getPropertyInfos(transform, anno, buildee, excludes,
includes, allNames, allProperties);
         if (includes != null) {
             for (String name : includes) {
                 checkKnownProperty(transform, anno, name, props);
@@ -160,30 +147,6 @@ public class ExternalStrategy extends BuilderASTTransformation.AbstractBuilderSt
         return new FieldNode(propName.equals("class") ? "clazz" : propName, ACC_PRIVATE,
newClass(prop.getType()), builderClass, DEFAULT_INITIAL_VALUE);
     }
 
-    public static List<PropertyInfo> getPropertyInfoFromBeanInfo(ClassNode cNode, List<String>
includes, List<String> excludes, boolean allNames) {
-        final List<PropertyInfo> result = new ArrayList<PropertyInfo>();
-        try {
-            BeanInfo beanInfo = Introspector.getBeanInfo(cNode.getTypeClass());
-            for (PropertyDescriptor descriptor : beanInfo.getPropertyDescriptors()) {
-                if (AbstractASTTransformation.shouldSkipUndefinedAware(descriptor.getName(),
excludes, includes, allNames)) continue;
-                // skip hidden and read-only props
-                if (descriptor.isHidden() || descriptor.getWriteMethod() == null) continue;
-                result.add(new PropertyInfo(descriptor.getName(), ClassHelper.make(descriptor.getPropertyType())));
-            }
-        } catch (IntrospectionException ignore) {
-        }
-        return result;
-    }
-
-    private List<PropertyInfo> getPropertyInfoFromClassNode(BuilderASTTransformation
transform, AnnotationNode anno, ClassNode cNode, List<String> includes, List<String>
excludes, boolean allNames) {
-        List<PropertyInfo> props = new ArrayList<PropertyInfo>();
-        for (FieldNode fNode : getFields(transform, anno, cNode)) {
-            if (shouldSkip(fNode.getName(), excludes, includes, allNames)) continue;
-            props.add(new PropertyInfo(fNode.getName(), fNode.getType()));
-        }
-        return props;
-    }
-
     private static Expression initializeInstance(ClassNode sourceClass, List<PropertyInfo>
props, BlockStatement body) {
         Expression instance = varX("_the" + sourceClass.getNameWithoutPackage(), sourceClass);
         body.addStatement(declS(instance, ctorX(sourceClass)));

http://git-wip-us.apache.org/repos/asf/groovy/blob/bab4be2c/src/main/groovy/transform/builder/InitializerStrategy.java
----------------------------------------------------------------------
diff --git a/src/main/groovy/transform/builder/InitializerStrategy.java b/src/main/groovy/transform/builder/InitializerStrategy.java
index 37eddf2..4e0dd82 100644
--- a/src/main/groovy/transform/builder/InitializerStrategy.java
+++ b/src/main/groovy/transform/builder/InitializerStrategy.java
@@ -132,6 +132,7 @@ public class InitializerStrategy extends BuilderASTTransformation.AbstractBuilde
 
     public void build(BuilderASTTransformation transform, AnnotatedNode annotatedNode, AnnotationNode
anno) {
         if (unsupportedAttribute(transform, anno, "forClass")) return;
+        if (unsupportedAttribute(transform, anno, "allProperties")) return;
         boolean useSetters = transform.memberHasValue(anno, "useSetters", true);
         boolean allNames = transform.memberHasValue(anno, "allNames", true);
         if (annotatedNode instanceof ClassNode) {

http://git-wip-us.apache.org/repos/asf/groovy/blob/bab4be2c/src/main/groovy/transform/builder/SimpleStrategy.java
----------------------------------------------------------------------
diff --git a/src/main/groovy/transform/builder/SimpleStrategy.java b/src/main/groovy/transform/builder/SimpleStrategy.java
index 980728e..7956ac6 100644
--- a/src/main/groovy/transform/builder/SimpleStrategy.java
+++ b/src/main/groovy/transform/builder/SimpleStrategy.java
@@ -93,6 +93,7 @@ public class SimpleStrategy extends BuilderASTTransformation.AbstractBuilderStra
         if (unsupportedAttribute(transform, anno, "builderMethodName")) return;
         if (unsupportedAttribute(transform, anno, "forClass")) return;
         if (unsupportedAttribute(transform, anno, "includeSuperProperties")) return;
+        if (unsupportedAttribute(transform, anno, "allProperties")) return;
         boolean useSetters = transform.memberHasValue(anno, "useSetters", true);
         boolean allNames = transform.memberHasValue(anno, "allNames", true);
 

http://git-wip-us.apache.org/repos/asf/groovy/blob/bab4be2c/src/main/org/codehaus/groovy/transform/BuilderASTTransformation.java
----------------------------------------------------------------------
diff --git a/src/main/org/codehaus/groovy/transform/BuilderASTTransformation.java b/src/main/org/codehaus/groovy/transform/BuilderASTTransformation.java
index 255a895..3cc49a2 100644
--- a/src/main/org/codehaus/groovy/transform/BuilderASTTransformation.java
+++ b/src/main/org/codehaus/groovy/transform/BuilderASTTransformation.java
@@ -31,10 +31,16 @@ import org.codehaus.groovy.ast.ConstructorNode;
 import org.codehaus.groovy.ast.FieldNode;
 import org.codehaus.groovy.ast.MethodNode;
 import org.codehaus.groovy.ast.Parameter;
+import org.codehaus.groovy.ast.PropertyNode;
+import org.codehaus.groovy.ast.tools.BeanUtils;
 import org.codehaus.groovy.control.CompilationUnit;
 import org.codehaus.groovy.control.CompilePhase;
 import org.codehaus.groovy.control.SourceUnit;
 
+import java.beans.BeanInfo;
+import java.beans.IntrospectionException;
+import java.beans.Introspector;
+import java.beans.PropertyDescriptor;
 import java.util.ArrayList;
 import java.util.List;
 
@@ -90,6 +96,21 @@ public class BuilderASTTransformation extends AbstractASTTransformation
implemen
             return props;
         }
 
+        protected static List<PropertyInfo> getPropertyInfoFromBeanInfo(ClassNode cNode,
List<String> includes, List<String> excludes, boolean allNames) {
+            final List<PropertyInfo> result = new ArrayList<PropertyInfo>();
+            try {
+                BeanInfo beanInfo = Introspector.getBeanInfo(cNode.getTypeClass());
+                for (PropertyDescriptor descriptor : beanInfo.getPropertyDescriptors()) {
+                    if (shouldSkipUndefinedAware(descriptor.getName(), excludes, includes,
allNames)) continue;
+                    // skip hidden and read-only props
+                    if (descriptor.isHidden() || descriptor.getWriteMethod() == null) continue;
+                    result.add(new PropertyInfo(descriptor.getName(), ClassHelper.make(descriptor.getPropertyType())));
+                }
+            } catch (IntrospectionException ignore) {
+            }
+            return result;
+        }
+
         protected String getSetterName(String prefix, String fieldName) {
             return prefix.isEmpty() ? fieldName : prefix + Character.toUpperCase(fieldName.charAt(0))
+ fieldName.substring(1);
         }
@@ -166,6 +187,28 @@ public class BuilderASTTransformation extends AbstractASTTransformation
implemen
            return includeSuperProperties ? getSuperPropertyFields(buildee) : getInstancePropertyFields(buildee);
         }
 
+        protected List<PropertyInfo> getPropertyInfoFromClassNode(BuilderASTTransformation
transform, AnnotationNode anno, ClassNode cNode, List<String> includes, List<String>
excludes, boolean allNames, boolean allProperties) {
+            List<PropertyInfo> props = new ArrayList<PropertyInfo>();
+            List<String> seen = new ArrayList<String>();
+            for (PropertyNode pNode : BeanUtils.getAllProperties(cNode, false, false, allProperties))
{
+                if (shouldSkip(pNode.getName(), excludes, includes, allNames)) continue;
+                props.add(new PropertyInfo(pNode.getName(), pNode.getType()));
+                seen.add(pNode.getName());
+            }
+            for (FieldNode fNode : getFields(transform, anno, cNode)) {
+                if (seen.contains(fNode.getName()) || shouldSkip(fNode.getName(), excludes,
includes, allNames)) continue;
+                props.add(new PropertyInfo(fNode.getName(), fNode.getType()));
+            }
+            return props;
+        }
+
+        protected List<PropertyInfo> getPropertyInfos(BuilderASTTransformation transform,
AnnotationNode anno, ClassNode buildee, List<String> excludes, List<String> includes,
boolean allNames, boolean allProperties) {
+            if (buildee.getModule() == null) {
+                return getPropertyInfoFromBeanInfo(buildee, includes, excludes, allNames);
+            }
+            return getPropertyInfoFromClassNode(transform, anno, buildee, includes, excludes,
allNames, allProperties);
+        }
+
         protected static class PropertyInfo {
             public PropertyInfo(String name, ClassNode type) {
                 this.name = name;

http://git-wip-us.apache.org/repos/asf/groovy/blob/bab4be2c/src/test/org/codehaus/groovy/transform/BuilderTransformTest.groovy
----------------------------------------------------------------------
diff --git a/src/test/org/codehaus/groovy/transform/BuilderTransformTest.groovy b/src/test/org/codehaus/groovy/transform/BuilderTransformTest.groovy
index 1e51e0d..1febfbc 100644
--- a/src/test/org/codehaus/groovy/transform/BuilderTransformTest.groovy
+++ b/src/test/org/codehaus/groovy/transform/BuilderTransformTest.groovy
@@ -757,4 +757,23 @@ class BuilderTransformTest extends CompilableTestSupport {
          '''
     }
 
+    // GROOVY-8186
+    void testJavaBeanPropertiesAreProperlyProcessed() {
+        assertScript '''
+            import groovy.transform.builder.*
+
+            class Foo {
+              String getName() {
+                'John'
+              }
+              void setName(String ignore) {}
+            }
+
+            @Builder(builderStrategy=ExternalStrategy, forClass=Foo)
+            class FooBuilder { }
+
+            assert new FooBuilder().name('Mary').build().name == 'John'
+         '''
+    }
+
 }


Mime
View raw message