From 94286c4f3f8f5950130eaf8105f83379eebd4d8f Mon Sep 17 00:00:00 2001 From: Jan Kanis Date: Mon, 16 Jan 2017 18:09:57 +0100 Subject: [PATCH 1/3] Recognize properties with the second letter capitalized Make ClassPropertyFetcher also recognize properties of the form "xAge" that have the second letter capitalized. Fixes #10403 --- .../org/grails/core/util/ClassPropertyFetcher.java | 6 ++++-- .../grails/core/util/ClassPropertyFetcherSpec.groovy | 11 +++++++++++ 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/grails-core/src/main/groovy/org/grails/core/util/ClassPropertyFetcher.java b/grails-core/src/main/groovy/org/grails/core/util/ClassPropertyFetcher.java index 43e9d072eff..200480cf4e4 100644 --- a/grails-core/src/main/groovy/org/grails/core/util/ClassPropertyFetcher.java +++ b/grails-core/src/main/groovy/org/grails/core/util/ClassPropertyFetcher.java @@ -130,8 +130,10 @@ public void doWith(CachedMethod method) throws IllegalArgumentException, if (method.getParameterTypes().length == 0) { String name = method.getName(); if (name.indexOf('$') == -1) { - if (name.length() > 3 && name.startsWith("get") - && Character.isUpperCase(name.charAt(3))) { + if (name.length() > 3 && name.startsWith("get") && ( + Character.isUpperCase(name.charAt(3)) + || (name.length() > 4 && Character.isUpperCase(name.charAt(4))) + )) { name = name.substring(3); } else if (name.length() > 2 && name.startsWith("is") diff --git a/grails-core/src/test/groovy/org/grails/core/util/ClassPropertyFetcherSpec.groovy b/grails-core/src/test/groovy/org/grails/core/util/ClassPropertyFetcherSpec.groovy index c76678b6f13..a82dab0d959 100644 --- a/grails-core/src/test/groovy/org/grails/core/util/ClassPropertyFetcherSpec.groovy +++ b/grails-core/src/test/groovy/org/grails/core/util/ClassPropertyFetcherSpec.groovy @@ -15,9 +15,20 @@ class ClassPropertyFetcherSpec extends Specification { cpf.getPropertyValue(new Author(name: "Fred"),"name") == "Fred" cpf.getPropertyValue(new Author(name: "Fred", books: ["test"]),"books").contains "test" } + + void "test properties that have the fifth letter of their getter capitalized instead of the fourth"() { + when:"A class property fetcher is created" + def cpf = ClassPropertyFetcher.forClass(Person) + + then:"all properties are correct" + def person = new Person(name: "Fred", xAge: 30) + person.getxAge() == 30 + cpf.getPropertyValue(person, "xAge") == 30 + } } class Person { String name + Integer xAge } class Author extends Person { Set books From f3f0c8966d2248f32c1a785ac8b4cca74314475b Mon Sep 17 00:00:00 2001 From: Jan Kanis Date: Tue, 17 Jan 2017 15:14:15 +0100 Subject: [PATCH 2/3] Improve property detection GrailsClassUtils.getPropertyForGetter/Setter would previously return property names for some method names that were not valid getters or setters, although the documentation says they return null in those cases. This change improves the detection and makes sure only valid getters/setters result in a property name. This commit includes updates in the documentation of related methods to more clearly indicate what is accepted and what not, and what assumptions are made. This also includes a bunch of new test cases. --- .../groovy/grails/util/GrailsClassUtils.java | 102 ++++++++++++------ .../grails/commons/GrailsClassUtilsTests.java | 40 +++++++ 2 files changed, 107 insertions(+), 35 deletions(-) diff --git a/grails-core/src/main/groovy/grails/util/GrailsClassUtils.java b/grails-core/src/main/groovy/grails/util/GrailsClassUtils.java index b39ce091745..18da480535d 100644 --- a/grails-core/src/main/groovy/grails/util/GrailsClassUtils.java +++ b/grails-core/src/main/groovy/grails/util/GrailsClassUtils.java @@ -1016,11 +1016,12 @@ public static String getSetterName(String propertyName) { } /** - * Returns true if the name of the method specified and the number of arguments make it a javabean property + * Returns true if the name of the method specified and the number of arguments make it a javabean property getter. + * The name is assumed to be a valid Java method name, that is not verified. * - * @param name True if its a Javabean property + * @param name The name of the method * @param args The arguments - * @return true if it is a javabean property method + * @return true if it is a javabean property getter */ public static boolean isGetter(String name, Class[] args) { if (!StringUtils.hasText(name) || args == null)return false; @@ -1049,90 +1050,121 @@ else if (name.startsWith("is")) { *
  • Word
    • *
  • aProperty
    • *
  • S
    • + *
  • X567
    • * * - * Example sof suffixes that would not be considered property getters: + * Examples of suffixes that would not be considered property getters: * + * + * A suffix like prop from a method getprop() is + * not recognized as a valid suffix. However Groovy will recognize such a + * method as a property getter but only if a method getProp() or + * a property prop does not also exist. The Java Beans + * specification is unclear on how to treat such method names, it only says + * that "by default" the suffix will start with a capital letter because of + * the camel case style usually used. (See the JavaBeans API specification + * sections 8.3 and 8.8.) + * + * This method assumes that all characters in the name are valid Java identifier + * letters. + * * @param suffix The suffix to inspect * @return true if suffix indicates a property name */ protected static boolean isPropertyMethodSuffix(String suffix) { - if (suffix.length() > 0) { - if(suffix.length() == 1) { - if(Character.isUpperCase(suffix.charAt(0))) { - return true; - } - } else { - if(Character.isUpperCase(suffix.charAt(0)) || - (Character.isUpperCase(suffix.charAt(1)) && Character.isLowerCase(suffix.charAt(0)))) { - return true; - } - } - } - return false; + if(suffix.length() == 0) return false; + if(!Character.isJavaIdentifierStart(suffix.charAt(0))) return false; + if(suffix.length() == 1) return Character.isUpperCase(suffix.charAt(0)); + return Character.isUpperCase(suffix.charAt(0)) || Character.isUpperCase(suffix.charAt(1)); } /** - * Returns a property name equivalent for the given getter name or null if it is not a getter + * Returns a property name equivalent for the given getter name or null if it is not a valid getter. If not null + * or empty the getter name is assumed to be a valid identifier. * * @param getterName The getter name * @return The property name equivalent */ public static String getPropertyForGetter(String getterName) { - if (!StringUtils.hasText(getterName))return null; + if (getterName == null || getterName.length() == 0) return null; if (getterName.startsWith("get")) { String prop = getterName.substring(3); - return convertPropertyName(prop); + return convertValidPropertyMethodSuffix(prop); } if (getterName.startsWith("is")) { String prop = getterName.substring(2); - return convertPropertyName(prop); + return convertValidPropertyMethodSuffix(prop); } return null; } - private static String convertPropertyName(String prop) { - if (prop.length() == 1) { - return prop.toLowerCase(); + /** + * This method functions the same as {@link #isPropertyMethodSuffix(String)}, + * but in addition returns the property name, or null if not a valid property. + * + * @param suffix The suffix to inspect + * @return The property name or null + */ + private static String convertValidPropertyMethodSuffix(String suffix) { + if (suffix.length() == 0) return null; + + // We assume all characters are Character.isJavaIdentifierPart, but the first one may not be a valid + // starting character. + if (!Character.isJavaIdentifierStart(suffix.charAt(0))) return null; + + if (suffix.length() == 1) { + return Character.isUpperCase(suffix.charAt(0)) ? suffix.toLowerCase() : null; } - if (Character.isUpperCase(prop.charAt(0)) && Character.isUpperCase(prop.charAt(1))) { - return prop; + if (Character.isUpperCase(suffix.charAt(1))) { + // "aProperty", "AProperty" + return suffix; } - if (Character.isDigit(prop.charAt(0))) { - return prop; + if (Character.isUpperCase(suffix.charAt(0))) { + return Character.toLowerCase(suffix.charAt(0)) + suffix.substring(1); } - return Character.toLowerCase(prop.charAt(0)) + prop.substring(1); + return null; } /** - * Returns a property name equivalent for the given setter name or null if it is not a getter + * Returns a property name equivalent for the given setter name or null if it is not a valid setter. If not null + * or empty the setter name is assumed to be a valid identifier. * - * @param setterName The setter name + * @param setterName The setter name, must be null or empty or a valid identifier name * @return The property name equivalent */ public static String getPropertyForSetter(String setterName) { - if (!StringUtils.hasText(setterName))return null; + if (setterName == null || setterName.length() == 0) return null; if (setterName.startsWith("set")) { String prop = setterName.substring(3); - return convertPropertyName(prop); + return convertValidPropertyMethodSuffix(prop); } return null; } + /** + * Returns true if the name of the method specified and the number of arguments make it a javabean property setter. + * The name is assumed to be a valid Java method name, that is not verified. + * + * @param name The name of the method + * @param args The arguments + * @return true if it is a javabean property setter + */ @SuppressWarnings("rawtypes") public static boolean isSetter(String name, Class[] args) { if (!StringUtils.hasText(name) || args == null)return false; if (name.startsWith("set")) { if (args.length != 1) return false; - name = name.substring(3); - if (name.length() > 0 && Character.isUpperCase(name.charAt(0))) return true; + return isPropertyMethodSuffix(name.substring(3)); } return false; diff --git a/grails-test-suite-uber/src/test/groovy/org/grails/commons/GrailsClassUtilsTests.java b/grails-test-suite-uber/src/test/groovy/org/grails/commons/GrailsClassUtilsTests.java index 7bb92d5b23f..944c6545270 100644 --- a/grails-test-suite-uber/src/test/groovy/org/grails/commons/GrailsClassUtilsTests.java +++ b/grails-test-suite-uber/src/test/groovy/org/grails/commons/GrailsClassUtilsTests.java @@ -113,8 +113,19 @@ public void testGetterNames() { public void testIsGetterOrSetter() { assertTrue(GrailsClassUtils.isSetter("setSomething", new Class[] { String.class })); assertTrue(GrailsClassUtils.isGetter("getSomething", new Class[0])); + assertTrue(GrailsClassUtils.isGetter("isSomething", new Class[0])); assertTrue(GrailsClassUtils.isSetter("setURL", new Class[] { String.class })); assertTrue(GrailsClassUtils.isGetter("getURL", new Class[0])); + assertTrue(GrailsClassUtils.isGetter("isURL", new Class[0])); + assertTrue(GrailsClassUtils.isSetter("setaProp", new Class[] { String.class })); + assertTrue(GrailsClassUtils.isGetter("getaProp", new Class[0])); + assertTrue(GrailsClassUtils.isGetter("isaProp", new Class[0])); + assertTrue(GrailsClassUtils.isSetter("setX", new Class[] { String.class })); + assertTrue(GrailsClassUtils.isGetter("getX", new Class[0])); + assertTrue(GrailsClassUtils.isGetter("isX", new Class[0])); + assertTrue(GrailsClassUtils.isSetter("setX2", new Class[] { String.class })); + assertTrue(GrailsClassUtils.isGetter("getX2", new Class[0])); + assertTrue(GrailsClassUtils.isGetter("isX2", new Class[0])); assertFalse(GrailsClassUtils.isGetter("something", new Class[] { String.class })); assertFalse(GrailsClassUtils.isGetter("get", new Class[0])); @@ -123,6 +134,20 @@ public void testIsGetterOrSetter() { assertFalse(GrailsClassUtils.isSetter("setSomething", new Class[] { String.class, Object.class })); assertFalse(GrailsClassUtils.isGetter("getSomething", new Class[] { Object.class })); + assertFalse(GrailsClassUtils.isGetter("getsomething", new Class[0])); + assertFalse(GrailsClassUtils.isGetter("issomething", new Class[0])); + assertFalse(GrailsClassUtils.isSetter("setsomething", new Class[] { String.class })); + assertFalse(GrailsClassUtils.isGetter("get0", new Class[0])); + assertFalse(GrailsClassUtils.isSetter("set0", new Class[] { String.class })); + assertFalse(GrailsClassUtils.isGetter("get2other", new Class[0])); + assertFalse(GrailsClassUtils.isSetter("set2other", new Class[] { String.class })); + assertFalse(GrailsClassUtils.isGetter("getq3", new Class[0])); + assertFalse(GrailsClassUtils.isSetter("setq3", new Class[] { String.class })); + assertFalse(GrailsClassUtils.isGetter("get5A", new Class[0])); + assertFalse(GrailsClassUtils.isSetter("set5A", new Class[] { String.class })); + assertFalse(GrailsClassUtils.isGetter("", new Class[0])); + assertFalse(GrailsClassUtils.isSetter("", new Class[] { String.class })); + assertFalse(GrailsClassUtils.isGetter(null, new Class[] { Object.class })); assertFalse(GrailsClassUtils.isGetter("getSomething", null)); assertFalse(GrailsClassUtils.isGetter(null, null)); @@ -132,6 +157,21 @@ public void testGetPropertyForGetter() { assertEquals("something", GrailsClassUtils.getPropertyForGetter("getSomething")); assertEquals("URL", GrailsClassUtils.getPropertyForGetter("getURL")); assertEquals("p", GrailsClassUtils.getPropertyForGetter("getP")); + assertEquals("URL", GrailsClassUtils.getPropertyForGetter("isURL")); + assertEquals("aProp", GrailsClassUtils.getPropertyForGetter("getaProp")); + assertEquals("x2", GrailsClassUtils.getPropertyForGetter("getX2")); + assertEquals("x2", GrailsClassUtils.getPropertyForGetter("isX2")); + + assertNull(GrailsClassUtils.getPropertyForGetter(null)); + assertNull(GrailsClassUtils.getPropertyForGetter("")); + assertNull(GrailsClassUtils.getPropertyForGetter("get0")); + assertNull(GrailsClassUtils.getPropertyForGetter("get2other")); + assertNull(GrailsClassUtils.getPropertyForGetter("getq3")); + assertNull(GrailsClassUtils.getPropertyForGetter("get5A")); + assertNull(GrailsClassUtils.getPropertyForGetter("setSomething")); + assertNull(GrailsClassUtils.getPropertyForGetter("getit")); + assertNull(GrailsClassUtils.getPropertyForGetter("geta")); + assertNull(GrailsClassUtils.getPropertyForGetter("get0")); } public void testGetStaticField() { From 4a112088b1f44fe67d54cf4709da9f68ead64297 Mon Sep 17 00:00:00 2001 From: Jan Kanis Date: Tue, 17 Jan 2017 15:33:44 +0100 Subject: [PATCH 3/3] Re-use GrailsNameUtils property logic instead of separate logic Re-use GrailsNameUtils.getPropertyForGetter from ClassPropertyFetcher instead of having separate (and wrong) logic. This commit has a small behavioral change in that the old code would register both "Property" and "property" for a static property with a getter "getProperty" in ClassPropertyFetcher. With improved property name derivation that should not be necessary anymore. --- .../core/util/ClassPropertyFetcher.java | 45 ++++++++----------- 1 file changed, 18 insertions(+), 27 deletions(-) diff --git a/grails-core/src/main/groovy/org/grails/core/util/ClassPropertyFetcher.java b/grails-core/src/main/groovy/org/grails/core/util/ClassPropertyFetcher.java index 200480cf4e4..c73c8eb768a 100644 --- a/grails-core/src/main/groovy/org/grails/core/util/ClassPropertyFetcher.java +++ b/grails-core/src/main/groovy/org/grails/core/util/ClassPropertyFetcher.java @@ -126,33 +126,24 @@ public void doWith(CachedMethod method) throws IllegalArgumentException, if (!method.isPublic()) { return; } - if (returnType != Void.class && returnType != void.class) { - if (method.getParameterTypes().length == 0) { - String name = method.getName(); - if (name.indexOf('$') == -1) { - if (name.length() > 3 && name.startsWith("get") && ( - Character.isUpperCase(name.charAt(3)) - || (name.length() > 4 && Character.isUpperCase(name.charAt(4))) - )) { - name = name.substring(3); - } else if (name.length() > 2 - && name.startsWith("is") - && Character.isUpperCase(name.charAt(2)) - && (returnType == Boolean.class || - returnType == boolean.class)) { - name = name.substring(2); - } - if (method.isStatic()) { - GetterPropertyFetcher fetcher = new GetterPropertyFetcher(method, true); - staticFetchers.put(name, fetcher); - staticFetchers.put(StringUtils.uncapitalize(name), - fetcher); - } else { - instanceFetchers.put(StringUtils.uncapitalize(name), - new GetterPropertyFetcher(method, false)); - } - } - } + if (returnType == Void.class || returnType == void.class || method.getParameterTypes().length != 0) { + return; + } + + String propertyName = GrailsClassUtils.getPropertyForGetter(method.getName()); + if(propertyName == null || propertyName.indexOf('$') != -1) { + return; + } + + if (method.getName().startsWith("is") && + !(returnType == Boolean.class || returnType == boolean.class)) { + return; + } + + if (method.isStatic()) { + staticFetchers.put(propertyName, new GetterPropertyFetcher(method, true)); + } else { + instanceFetchers.put(propertyName, new GetterPropertyFetcher(method, false)); } } };