Repository: groovy Updated Branches: refs/heads/master f8fb6e734 -> 7ed686f2f
GROOVY-8453: @TupleConstructor ignoring JavaBean properties (including inherited properties) (closes #657) Project: http://git-wip-us.apache.org/repos/asf/groovy/repo Commit: http://git-wip-us.apache.org/repos/asf/groovy/commit/7ed686f2 Tree: http://git-wip-us.apache.org/repos/asf/groovy/tree/7ed686f2 Diff: http://git-wip-us.apache.org/repos/asf/groovy/diff/7ed686f2 Branch: refs/heads/master Commit: 7ed686f2f80b8ea8fd3301458206d3775cecc039 Parents: f8fb6e7 Author: paulk <[email protected]> Authored: Mon Jan 22 14:47:57 2018 +1000 Committer: paulk <[email protected]> Committed: Wed Jan 24 00:17:03 2018 +1000 ---------------------------------------------------------------------- .../groovy/transform/TupleConstructor.java | 13 ++- .../codehaus/groovy/ast/tools/BeanUtils.java | 110 ++++++++++++------- .../codehaus/groovy/ast/tools/GeneralUtils.java | 31 ++++-- .../TupleConstructorASTTransformation.java | 39 ++++--- .../TupleConstructorTransformTest.groovy | 27 ++++- 5 files changed, 152 insertions(+), 68 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/groovy/blob/7ed686f2/src/main/groovy/groovy/transform/TupleConstructor.java ---------------------------------------------------------------------- diff --git a/src/main/groovy/groovy/transform/TupleConstructor.java b/src/main/groovy/groovy/transform/TupleConstructor.java index 2cd2be7..12e0cb6 100644 --- a/src/main/groovy/groovy/transform/TupleConstructor.java +++ b/src/main/groovy/groovy/transform/TupleConstructor.java @@ -46,7 +46,9 @@ import java.lang.annotation.Target; * AST transformation which adds the necessary constructor method to your class. * <p> * A tuple constructor is created with a parameter for each property (and optionally field and - * super properties). + * super properties). The default order is properties, pseudo/JavaBean properties and then fields + * for parent classes first (if includeSuperXxx annotation attributes are used). + * * A default value is provided (using Java's default values) for all parameters in the constructor. * Groovy's normal conventions then allows any number of parameters to be left off the end of the parameter list * including all of the parameters - giving a no-arg constructor which can be used with the map-style naming conventions. @@ -262,6 +264,15 @@ public @interface TupleConstructor { boolean allNames() default false; /** + * Whether to include all properties (as per the JavaBean spec) in the generated constructor. + * When true, Groovy treats any explicitly created setXxx() methods as property setters as per the JavaBean + * specification. + * + * @since 2.5.0 + */ + boolean allProperties() default false; + + /** * A Closure containing statements which will be prepended to the generated constructor. The first statement * within the Closure may be {@code super(someArgs)} in which case the no-arg super constructor won't be called. * http://git-wip-us.apache.org/repos/asf/groovy/blob/7ed686f2/src/main/java/org/codehaus/groovy/ast/tools/BeanUtils.java ---------------------------------------------------------------------- diff --git a/src/main/java/org/codehaus/groovy/ast/tools/BeanUtils.java b/src/main/java/org/codehaus/groovy/ast/tools/BeanUtils.java index 32d4a81..5869d4e 100644 --- a/src/main/java/org/codehaus/groovy/ast/tools/BeanUtils.java +++ b/src/main/java/org/codehaus/groovy/ast/tools/BeanUtils.java @@ -22,6 +22,7 @@ import org.codehaus.groovy.ast.ClassHelper; import org.codehaus.groovy.ast.ClassNode; import org.codehaus.groovy.ast.MethodNode; import org.codehaus.groovy.ast.PropertyNode; +import org.codehaus.groovy.ast.stmt.Statement; import java.util.ArrayList; import java.util.HashSet; @@ -32,6 +33,7 @@ import static java.beans.Introspector.decapitalize; public class BeanUtils { static final String GET_PREFIX = "get"; + static final String SET_PREFIX = "set"; static final String IS_PREFIX = "is"; /** @@ -44,17 +46,38 @@ public class BeanUtils { * @return the list of found property nodes */ public static List<PropertyNode> getAllProperties(ClassNode type, boolean includeSuperProperties, boolean includeStatic, boolean includePseudoGetters) { + return getAllProperties(type, includeSuperProperties, includeStatic, includePseudoGetters, false, false); + } + + /** + * Get all properties including JavaBean pseudo properties matching JavaBean getter or setter conventions. + * + * @param type the ClassNode + * @param includeSuperProperties whether to include super properties + * @param includeStatic whether to include static properties + * @param includePseudoGetters whether to include JavaBean pseudo (getXXX/isYYY) properties with no corresponding field + * @param includePseudoSetters whether to include JavaBean pseudo (setXXX) properties with no corresponding field + * @param superFirst are properties gathered first from parent classes + * @return the list of found property nodes + */ + public static List<PropertyNode> getAllProperties(ClassNode type, boolean includeSuperProperties, boolean includeStatic, boolean includePseudoGetters, boolean includePseudoSetters, boolean superFirst) { + return getAllProperties(type, type, new HashSet<String>(), includeSuperProperties, includeStatic, includePseudoGetters, includePseudoSetters, superFirst); + } + + private static List<PropertyNode> getAllProperties(ClassNode origType, ClassNode type, Set<String> names, boolean includeSuperProperties, boolean includeStatic, boolean includePseudoGetters, boolean includePseudoSetters, boolean superFirst) { // TODO add generics support so this can be used for @EAHC - // TODO add an includePseudoSetters so this can be used for @TupleConstructor - ClassNode node = type; + if (type == null) { + return new ArrayList<PropertyNode>(); + } List<PropertyNode> result = new ArrayList<PropertyNode>(); - Set<String> names = new HashSet<String>(); - while (node != null) { - addExplicitProperties(node, result, names, includeStatic); - if (!includeSuperProperties) break; - node = node.getSuperClass(); + if (superFirst && includeSuperProperties) { + result.addAll(getAllProperties(origType, type.getSuperClass(), names, includeSuperProperties, includeStatic, includePseudoGetters, includePseudoSetters, superFirst)); + } + addExplicitProperties(type, result, names, includeStatic); + addPseudoProperties(origType, type, result, names, includeStatic, includePseudoGetters, includePseudoSetters); + if (!superFirst && includeSuperProperties) { + result.addAll(getAllProperties(origType, type.getSuperClass(), names, includeSuperProperties, includeStatic, includePseudoGetters, includePseudoSetters, superFirst)); } - addPseudoProperties(type, result, names, includeStatic, includePseudoGetters, includeSuperProperties); return result; } @@ -69,20 +92,10 @@ public class BeanUtils { } } - private static void addPseudoProperties(ClassNode cNode, List<PropertyNode> result, Set<String> names, boolean includeStatic, boolean includePseudoGetters, boolean includeSuperProperties) { - if (!includePseudoGetters) return; + public static void addPseudoProperties(ClassNode origType, ClassNode cNode, List<PropertyNode> result, Set<String> names, boolean includeStatic, boolean includePseudoGetters, boolean includePseudoSetters) { + if (!includePseudoGetters && !includePseudoSetters) return; List<MethodNode> methods = cNode.getAllDeclaredMethods(); ClassNode node = cNode.getSuperClass(); - if (includeSuperProperties) { - while (node != null) { - for (MethodNode next : node.getAllDeclaredMethods()) { - if (!next.isPrivate()) { - methods.add(next); - } - } - node = node.getSuperClass(); - } - } for (MethodNode mNode : methods) { if (!includeStatic && mNode.isStatic()) continue; String name = mNode.getName(); @@ -90,31 +103,54 @@ public class BeanUtils { // Optimization: skip invalid propertyNames continue; } - if (mNode.getDeclaringClass() != cNode && mNode.isPrivate()) { + if (mNode.getDeclaringClass() != origType && mNode.isPrivate()) { // skip private super methods continue; } int paramCount = mNode.getParameters().length; - ClassNode returnType = mNode.getReturnType(); + ClassNode paramType = mNode.getReturnType(); + String propName = null; + Statement getter = null; + Statement setter = null; if (paramCount == 0) { - if (name.startsWith(GET_PREFIX)) { + if (includePseudoGetters && name.startsWith(GET_PREFIX)) { // Simple getter - String propName = decapitalize(name.substring(3)); - if (!names.contains(propName)) { - result.add(new PropertyNode(propName, mNode.getModifiers(), returnType, cNode, null, mNode.getCode(), null)); - names.add(propName); - } - } else { - if (name.startsWith(IS_PREFIX) && returnType.equals(ClassHelper.boolean_TYPE)) { - // boolean getter - String propName = decapitalize(name.substring(2)); - if (!names.contains(propName)) { - names.add(propName); - result.add(new PropertyNode(propName, mNode.getModifiers(), returnType, cNode, null, mNode.getCode(), null)); - } - } + propName = decapitalize(name.substring(3)); + getter = mNode.getCode(); + } else if (includePseudoGetters && name.startsWith(IS_PREFIX) && paramType.equals(ClassHelper.boolean_TYPE)) { + // boolean getter + propName = decapitalize(name.substring(2)); + getter = mNode.getCode(); + } + } else if (paramCount == 1) { + if (includePseudoSetters && name.startsWith(SET_PREFIX)) { + // Simple setter + propName = decapitalize(name.substring(3)); + setter = mNode.getCode(); + paramType = mNode.getParameters()[0].getType(); + + } + } + if (propName != null) { + addIfMissing(cNode, result, names, mNode, paramType, propName, getter, setter); + } + } + } + + private static void addIfMissing(ClassNode cNode, List<PropertyNode> result, Set<String> names, MethodNode mNode, ClassNode returnType, String propName, Statement getter, Statement setter) { + if (cNode.getProperty(propName) != null) return; + if (names.contains(propName)) { + for (PropertyNode pn : result) { + if (pn.getName().equals(propName) && getter != null && pn.getGetterBlock() == null) { + pn.setGetterBlock(getter); + } + if (pn.getName().equals(propName) && setter != null && pn.getSetterBlock() == null) { + pn.setSetterBlock(setter); } } + } else { + result.add(new PropertyNode(propName, mNode.getModifiers(), returnType, cNode, null, getter, setter)); + names.add(propName); } } } http://git-wip-us.apache.org/repos/asf/groovy/blob/7ed686f2/src/main/java/org/codehaus/groovy/ast/tools/GeneralUtils.java ---------------------------------------------------------------------- diff --git a/src/main/java/org/codehaus/groovy/ast/tools/GeneralUtils.java b/src/main/java/org/codehaus/groovy/ast/tools/GeneralUtils.java index fa99ad4..b61ae64 100644 --- a/src/main/java/org/codehaus/groovy/ast/tools/GeneralUtils.java +++ b/src/main/java/org/codehaus/groovy/ast/tools/GeneralUtils.java @@ -466,24 +466,33 @@ public class GeneralUtils { return result; } - public static List<FieldNode> getAllFields(ClassNode cNode, boolean includeSuperProperties, boolean includeSuperFields) { - final List<FieldNode> result; - if (cNode == ClassHelper.OBJECT_TYPE) { - result = new ArrayList<FieldNode>(); + public static List<PropertyNode> getAllFields(Set<String> names, ClassNode cNode, boolean includeProperties, boolean includeFields, boolean allProperties, boolean traverseSuperClasses) { + return getAllFields(names, cNode, cNode, includeProperties, includeFields, allProperties, traverseSuperClasses); + } + + private static List<PropertyNode> getAllFields(Set<String> names, ClassNode origType, ClassNode cNode, boolean includeProperties, boolean includeFields, boolean allProperties, boolean traverseSuperClasses) { + final List<PropertyNode> result; + if (cNode == ClassHelper.OBJECT_TYPE || !traverseSuperClasses) { + result = new ArrayList<PropertyNode>(); } else { - result = getAllFields(cNode.getSuperClass(), includeSuperProperties, includeSuperFields); + result = getAllFields(names, origType, cNode.getSuperClass(), includeProperties, includeFields, allProperties, true); } - if (includeSuperProperties) { + if (includeProperties) { for (PropertyNode pNode : cNode.getProperties()) { - if (!pNode.isStatic()) { - result.add(pNode.getField()); + if (!pNode.isStatic() && !names.contains(pNode.getName())) { + result.add(pNode); + names.add(pNode.getName()); } } + if (allProperties) { + BeanUtils.addPseudoProperties(origType, cNode, result, names, false, false, true); + } } - if (includeSuperFields) { + if (includeFields) { for (FieldNode fNode : cNode.getFields()) { - if (!fNode.isStatic() && cNode.getProperty(fNode.getName()) == null) { - result.add(fNode); + if (!fNode.isStatic() && cNode.getProperty(fNode.getName()) == null && !names.contains(fNode.getName())) { + result.add(new PropertyNode(fNode, fNode.getModifiers(), null, null)); + names.add(fNode.getName()); } } } http://git-wip-us.apache.org/repos/asf/groovy/blob/7ed686f2/src/main/java/org/codehaus/groovy/transform/TupleConstructorASTTransformation.java ---------------------------------------------------------------------- diff --git a/src/main/java/org/codehaus/groovy/transform/TupleConstructorASTTransformation.java b/src/main/java/org/codehaus/groovy/transform/TupleConstructorASTTransformation.java index e469597..ce2e25d 100644 --- a/src/main/java/org/codehaus/groovy/transform/TupleConstructorASTTransformation.java +++ b/src/main/java/org/codehaus/groovy/transform/TupleConstructorASTTransformation.java @@ -43,9 +43,11 @@ import java.util.ArrayList; import java.util.Collections; import java.util.Comparator; import java.util.HashMap; +import java.util.HashSet; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; +import java.util.Set; import static org.codehaus.groovy.ast.ClassHelper.make; import static org.codehaus.groovy.ast.ClassHelper.makeWithoutCaching; @@ -115,6 +117,7 @@ public class TupleConstructorASTTransformation extends AbstractASTTransformation boolean force = memberHasValue(anno, "force", true); boolean defaults = !memberHasValue(anno, "defaults", false); boolean useSetters = memberHasValue(anno, "useSetters", true); + boolean allProperties = memberHasValue(anno, "allProperties", true); List<String> excludes = getMemberStringList(anno, "excludes"); List<String> includes = getMemberStringList(anno, "includes"); boolean allNames = memberHasValue(anno, "allNames", true); @@ -134,7 +137,7 @@ public class TupleConstructorASTTransformation extends AbstractASTTransformation return; } createConstructor(this, cNode, includeFields, includeProperties, includeSuperFields, includeSuperProperties, - callSuper, force, excludes, includes, useSetters, defaults, allNames, sourceUnit, + callSuper, force, excludes, includes, useSetters, defaults, allNames, allProperties, sourceUnit, (ClosureExpression) pre, (ClosureExpression) post); if (pre != null) { anno.setMember("pre", new ClosureExpression(new Parameter[0], EmptyStatement.INSTANCE)); @@ -164,21 +167,27 @@ public class TupleConstructorASTTransformation extends AbstractASTTransformation List<String> excludes, final List<String> includes, boolean useSetters, boolean defaults, boolean allNames, SourceUnit sourceUnit, ClosureExpression pre, ClosureExpression post) { + createConstructor(xform, cNode, includeFields, includeProperties, includeSuperFields, includeSuperProperties, callSuper, force, excludes, includes, useSetters, defaults, allNames, false, sourceUnit, pre, post); + } + + public static void createConstructor(AbstractASTTransformation xform, ClassNode cNode, boolean includeFields, + boolean includeProperties, boolean includeSuperFields, boolean + includeSuperProperties, boolean callSuper, boolean force, + List<String> excludes, final List<String> includes, boolean useSetters, boolean + defaults, boolean allNames, boolean allProperties, SourceUnit sourceUnit, ClosureExpression + pre, ClosureExpression post) { // no processing if existing constructors found if (!cNode.getDeclaredConstructors().isEmpty() && !force) return; - List<FieldNode> superList = new ArrayList<FieldNode>(); + Set<String> names = new HashSet<String>(); + List<PropertyNode> superList; if (includeSuperProperties || includeSuperFields) { - superList.addAll(getAllFields(cNode.getSuperClass(), includeSuperProperties, includeSuperFields)); + superList = getAllFields(names, cNode.getSuperClass(), includeSuperProperties, includeSuperFields, allProperties, true); + } else { + superList = new ArrayList<PropertyNode>(); } - List<FieldNode> list = new ArrayList<FieldNode>(); - if (includeProperties) { - list.addAll(getInstancePropertyFields(cNode)); - } - if (includeFields) { - list.addAll(getInstanceNonPropertyFields(cNode)); - } + List<PropertyNode> list = getAllFields(names, cNode, true, includeFields, allProperties, false); final List<Parameter> params = new ArrayList<Parameter>(); final List<Expression> superParams = new ArrayList<Expression>(); @@ -192,8 +201,9 @@ public class TupleConstructorASTTransformation extends AbstractASTTransformation } } final BlockStatement body = new BlockStatement(); - for (FieldNode fNode : superList) { - String name = fNode.getName(); + for (PropertyNode pNode : superList) { + FieldNode fNode = pNode.getField(); + String name = pNode.getName(); if (shouldSkipUndefinedAware(name, excludes, includes, allNames)) continue; params.add(createParam(fNode, name, defaults, xform)); boolean hasSetter = cNode.getProperty(name) != null && !fNode.isFinal(); @@ -213,8 +223,9 @@ public class TupleConstructorASTTransformation extends AbstractASTTransformation if (!preBody.isEmpty()) { body.addStatements(preBody.getStatements()); } - for (FieldNode fNode : list) { - String name = fNode.getName(); + for (PropertyNode pNode : list) { + String name = pNode.getName(); + FieldNode fNode = pNode.getField(); if (shouldSkipUndefinedAware(name, excludes, includes, allNames)) continue; Parameter nextParam = createParam(fNode, name, defaults, xform); params.add(nextParam); http://git-wip-us.apache.org/repos/asf/groovy/blob/7ed686f2/src/test/org/codehaus/groovy/transform/TupleConstructorTransformTest.groovy ---------------------------------------------------------------------- diff --git a/src/test/org/codehaus/groovy/transform/TupleConstructorTransformTest.groovy b/src/test/org/codehaus/groovy/transform/TupleConstructorTransformTest.groovy index 933b592..991cdb4 100644 --- a/src/test/org/codehaus/groovy/transform/TupleConstructorTransformTest.groovy +++ b/src/test/org/codehaus/groovy/transform/TupleConstructorTransformTest.groovy @@ -214,37 +214,54 @@ class TupleConstructorTransformTest extends GroovyShellTestCase { ''' } - void testSuperPropertyAndSuperFieldOrder_groovy8455() { + // GROOVY-8455, GROOVY-8453 + void testPropPsuedoPropAndFieldOrderIncludingInheritedMembers() { assertScript ''' - import groovy.transform.* + import groovy.transform.TupleConstructor + + class Basepubf{} + class Basep{} + class Basepp{} + class Base { + Basep basep + public Basepubf basepubf + protected Byte baseProtField + void setBasePseudoProp(Basepp bpp) {} + } class Foopubf{} class Foop{} - class Foo { + class Foopp{} + class Foo extends Base { Foop foop public Foopubf foopubf protected Short fooProtField + Foopp getFooPseudoProp() { null } } class Barpubf{} class Barp{} + class Barpp{} class Bar extends Foo { Barp barp public Barpubf barpubf protected Integer barProtField + void setBarPseudoProp(Barpp bpp) { } } class Bazpubf{} class Bazp{} - @TupleConstructor(includeSuperProperties=true, includeFields=true, includeSuperFields=true) + class Bazpp{} + @TupleConstructor(includeSuperProperties=true, includeFields=true, includeSuperFields=true, allProperties=true) class Baz extends Bar { Bazp bazp public Bazpubf bazpubf protected Long bazProtField + void setBazPseudoProp(Bazpp bpp) { } } assert Baz.constructors.max{ it.parameterTypes.size() }.toString() == - 'public Baz(Foop,Foopubf,java.lang.Short,Barp,Barpubf,java.lang.Integer,Bazp,Bazpubf,java.lang.Long)' + 'public Baz(Basep,Basepp,Basepubf,java.lang.Byte,Foop,Foopubf,java.lang.Short,Barp,Barpp,Barpubf,java.lang.Integer,Bazp,Bazpp,Bazpubf,java.lang.Long)' ''' }
