Skip to content

Commit f3f0c89

Browse files
JanKanisJanKanis
committed
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.
1 parent 94286c4 commit f3f0c89

File tree

2 files changed

+107
-35
lines changed

2 files changed

+107
-35
lines changed

‎grails-core/src/main/groovy/grails/util/GrailsClassUtils.java‎

Lines changed: 67 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -1016,11 +1016,12 @@ public static String getSetterName(String propertyName) {
10161016
}
10171017

10181018
/**
1019-
* Returns true if the name of the method specified and the number of arguments make it a javabean property
1019+
* Returns true if the name of the method specified and the number of arguments make it a javabean property getter.
1020+
* The name is assumed to be a valid Java method name, that is not verified.
10201021
*
1021-
* @param name True if its a Javabean property
1022+
* @param name The name of the method
10221023
* @param args The arguments
1023-
* @return true if it is a javabean property method
1024+
* @return true if it is a javabean property getter
10241025
*/
10251026
public static boolean isGetter(String name, Class<?>[] args) {
10261027
if (!StringUtils.hasText(name) || args == null)return false;
@@ -1049,90 +1050,121 @@ else if (name.startsWith("is")) {
10491050
* <li>Word</li>
10501051
* <li>aProperty</li>
10511052
* <li>S</li>
1053+
* <li>X567</li>
10521054
* </ul>
10531055
*
1054-
* Example sof suffixes that would not be considered property getters:
1056+
* Examples of suffixes that would not be considered property getters:
10551057
* <ul>
10561058
* <li>someProperty</li>
10571059
* <li>word</li>
10581060
* <li>s</li>
1061+
* <li>x567</li>
1062+
* <li>2other</li>
1063+
* <li>5</li>
10591064
* </ul>
1065+
*
1066+
* A suffix like <code>prop</code> from a method <code>getprop()</code> is
1067+
* not recognized as a valid suffix. However Groovy will recognize such a
1068+
* method as a property getter but only if a method <code>getProp()</code> or
1069+
* a property <code>prop</code> does not also exist. The Java Beans
1070+
* specification is unclear on how to treat such method names, it only says
1071+
* that "by default" the suffix will start with a capital letter because of
1072+
* the camel case style usually used. (See the JavaBeans API specification
1073+
* sections 8.3 and 8.8.)
1074+
*
1075+
* This method assumes that all characters in the name are valid Java identifier
1076+
* letters.
1077+
*
10601078
* @param suffix The suffix to inspect
10611079
* @return true if suffix indicates a property name
10621080
*/
10631081
protected static boolean isPropertyMethodSuffix(String suffix) {
1064-
if (suffix.length() > 0) {
1065-
if(suffix.length() == 1) {
1066-
if(Character.isUpperCase(suffix.charAt(0))) {
1067-
return true;
1068-
}
1069-
} else {
1070-
if(Character.isUpperCase(suffix.charAt(0)) ||
1071-
(Character.isUpperCase(suffix.charAt(1)) && Character.isLowerCase(suffix.charAt(0)))) {
1072-
return true;
1073-
}
1074-
}
1075-
}
1076-
return false;
1082+
if(suffix.length() == 0) return false;
1083+
if(!Character.isJavaIdentifierStart(suffix.charAt(0))) return false;
1084+
if(suffix.length() == 1) return Character.isUpperCase(suffix.charAt(0));
1085+
return Character.isUpperCase(suffix.charAt(0)) || Character.isUpperCase(suffix.charAt(1));
10771086
}
10781087

10791088
/**
1080-
* Returns a property name equivalent for the given getter name or null if it is not a getter
1089+
* Returns a property name equivalent for the given getter name or null if it is not a valid getter. If not null
1090+
* or empty the getter name is assumed to be a valid identifier.
10811091
*
10821092
* @param getterName The getter name
10831093
* @return The property name equivalent
10841094
*/
10851095
public static String getPropertyForGetter(String getterName) {
1086-
if (!StringUtils.hasText(getterName))return null;
1096+
if (getterName == null || getterName.length() == 0) return null;
10871097

10881098
if (getterName.startsWith("get")) {
10891099
String prop = getterName.substring(3);
1090-
return convertPropertyName(prop);
1100+
return convertValidPropertyMethodSuffix(prop);
10911101
}
10921102
if (getterName.startsWith("is")) {
10931103
String prop = getterName.substring(2);
1094-
return convertPropertyName(prop);
1104+
return convertValidPropertyMethodSuffix(prop);
10951105
}
10961106
return null;
10971107
}
10981108

1099-
private static String convertPropertyName(String prop) {
1100-
if (prop.length() == 1) {
1101-
return prop.toLowerCase();
1109+
/**
1110+
* This method functions the same as {@link #isPropertyMethodSuffix(String)},
1111+
* but in addition returns the property name, or null if not a valid property.
1112+
*
1113+
* @param suffix The suffix to inspect
1114+
* @return The property name or null
1115+
*/
1116+
private static String convertValidPropertyMethodSuffix(String suffix) {
1117+
if (suffix.length() == 0) return null;
1118+
1119+
// We assume all characters are Character.isJavaIdentifierPart, but the first one may not be a valid
1120+
// starting character.
1121+
if (!Character.isJavaIdentifierStart(suffix.charAt(0))) return null;
1122+
1123+
if (suffix.length() == 1) {
1124+
return Character.isUpperCase(suffix.charAt(0)) ? suffix.toLowerCase() : null;
11021125
}
1103-
if (Character.isUpperCase(prop.charAt(0)) && Character.isUpperCase(prop.charAt(1))) {
1104-
return prop;
1126+
if (Character.isUpperCase(suffix.charAt(1))) {
1127+
// "aProperty", "AProperty"
1128+
return suffix;
11051129
}
1106-
if (Character.isDigit(prop.charAt(0))) {
1107-
return prop;
1130+
if (Character.isUpperCase(suffix.charAt(0))) {
1131+
return Character.toLowerCase(suffix.charAt(0)) + suffix.substring(1);
11081132
}
1109-
return Character.toLowerCase(prop.charAt(0)) + prop.substring(1);
1133+
return null;
11101134
}
11111135

11121136
/**
1113-
* Returns a property name equivalent for the given setter name or null if it is not a getter
1137+
* Returns a property name equivalent for the given setter name or null if it is not a valid setter. If not null
1138+
* or empty the setter name is assumed to be a valid identifier.
11141139
*
1115-
* @param setterName The setter name
1140+
* @param setterName The setter name, must be null or empty or a valid identifier name
11161141
* @return The property name equivalent
11171142
*/
11181143
public static String getPropertyForSetter(String setterName) {
1119-
if (!StringUtils.hasText(setterName))return null;
1144+
if (setterName == null || setterName.length() == 0) return null;
11201145

11211146
if (setterName.startsWith("set")) {
11221147
String prop = setterName.substring(3);
1123-
return convertPropertyName(prop);
1148+
return convertValidPropertyMethodSuffix(prop);
11241149
}
11251150
return null;
11261151
}
11271152

1153+
/**
1154+
* Returns true if the name of the method specified and the number of arguments make it a javabean property setter.
1155+
* The name is assumed to be a valid Java method name, that is not verified.
1156+
*
1157+
* @param name The name of the method
1158+
* @param args The arguments
1159+
* @return true if it is a javabean property setter
1160+
*/
11281161
@SuppressWarnings("rawtypes")
11291162
public static boolean isSetter(String name, Class[] args) {
11301163
if (!StringUtils.hasText(name) || args == null)return false;
11311164

11321165
if (name.startsWith("set")) {
11331166
if (args.length != 1) return false;
1134-
name = name.substring(3);
1135-
if (name.length() > 0 && Character.isUpperCase(name.charAt(0))) return true;
1167+
return isPropertyMethodSuffix(name.substring(3));
11361168
}
11371169

11381170
return false;

‎grails-test-suite-uber/src/test/groovy/org/grails/commons/GrailsClassUtilsTests.java‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -113,8 +113,19 @@ public void testGetterNames() {
113113
public void testIsGetterOrSetter() {
114114
assertTrue(GrailsClassUtils.isSetter("setSomething", new Class[] { String.class }));
115115
assertTrue(GrailsClassUtils.isGetter("getSomething", new Class[0]));
116+
assertTrue(GrailsClassUtils.isGetter("isSomething", new Class[0]));
116117
assertTrue(GrailsClassUtils.isSetter("setURL", new Class[] { String.class }));
117118
assertTrue(GrailsClassUtils.isGetter("getURL", new Class[0]));
119+
assertTrue(GrailsClassUtils.isGetter("isURL", new Class[0]));
120+
assertTrue(GrailsClassUtils.isSetter("setaProp", new Class[] { String.class }));
121+
assertTrue(GrailsClassUtils.isGetter("getaProp", new Class[0]));
122+
assertTrue(GrailsClassUtils.isGetter("isaProp", new Class[0]));
123+
assertTrue(GrailsClassUtils.isSetter("setX", new Class[] { String.class }));
124+
assertTrue(GrailsClassUtils.isGetter("getX", new Class[0]));
125+
assertTrue(GrailsClassUtils.isGetter("isX", new Class[0]));
126+
assertTrue(GrailsClassUtils.isSetter("setX2", new Class[] { String.class }));
127+
assertTrue(GrailsClassUtils.isGetter("getX2", new Class[0]));
128+
assertTrue(GrailsClassUtils.isGetter("isX2", new Class[0]));
118129

119130
assertFalse(GrailsClassUtils.isGetter("something", new Class[] { String.class }));
120131
assertFalse(GrailsClassUtils.isGetter("get", new Class[0]));
@@ -123,6 +134,20 @@ public void testIsGetterOrSetter() {
123134
assertFalse(GrailsClassUtils.isSetter("setSomething", new Class[] { String.class, Object.class }));
124135
assertFalse(GrailsClassUtils.isGetter("getSomething", new Class[] { Object.class }));
125136

137+
assertFalse(GrailsClassUtils.isGetter("getsomething", new Class[0]));
138+
assertFalse(GrailsClassUtils.isGetter("issomething", new Class[0]));
139+
assertFalse(GrailsClassUtils.isSetter("setsomething", new Class[] { String.class }));
140+
assertFalse(GrailsClassUtils.isGetter("get0", new Class[0]));
141+
assertFalse(GrailsClassUtils.isSetter("set0", new Class[] { String.class }));
142+
assertFalse(GrailsClassUtils.isGetter("get2other", new Class[0]));
143+
assertFalse(GrailsClassUtils.isSetter("set2other", new Class[] { String.class }));
144+
assertFalse(GrailsClassUtils.isGetter("getq3", new Class[0]));
145+
assertFalse(GrailsClassUtils.isSetter("setq3", new Class[] { String.class }));
146+
assertFalse(GrailsClassUtils.isGetter("get5A", new Class[0]));
147+
assertFalse(GrailsClassUtils.isSetter("set5A", new Class[] { String.class }));
148+
assertFalse(GrailsClassUtils.isGetter("", new Class[0]));
149+
assertFalse(GrailsClassUtils.isSetter("", new Class[] { String.class }));
150+
126151
assertFalse(GrailsClassUtils.isGetter(null, new Class[] { Object.class }));
127152
assertFalse(GrailsClassUtils.isGetter("getSomething", null));
128153
assertFalse(GrailsClassUtils.isGetter(null, null));
@@ -132,6 +157,21 @@ public void testGetPropertyForGetter() {
132157
assertEquals("something", GrailsClassUtils.getPropertyForGetter("getSomething"));
133158
assertEquals("URL", GrailsClassUtils.getPropertyForGetter("getURL"));
134159
assertEquals("p", GrailsClassUtils.getPropertyForGetter("getP"));
160+
assertEquals("URL", GrailsClassUtils.getPropertyForGetter("isURL"));
161+
assertEquals("aProp", GrailsClassUtils.getPropertyForGetter("getaProp"));
162+
assertEquals("x2", GrailsClassUtils.getPropertyForGetter("getX2"));
163+
assertEquals("x2", GrailsClassUtils.getPropertyForGetter("isX2"));
164+
165+
assertNull(GrailsClassUtils.getPropertyForGetter(null));
166+
assertNull(GrailsClassUtils.getPropertyForGetter(""));
167+
assertNull(GrailsClassUtils.getPropertyForGetter("get0"));
168+
assertNull(GrailsClassUtils.getPropertyForGetter("get2other"));
169+
assertNull(GrailsClassUtils.getPropertyForGetter("getq3"));
170+
assertNull(GrailsClassUtils.getPropertyForGetter("get5A"));
171+
assertNull(GrailsClassUtils.getPropertyForGetter("setSomething"));
172+
assertNull(GrailsClassUtils.getPropertyForGetter("getit"));
173+
assertNull(GrailsClassUtils.getPropertyForGetter("geta"));
174+
assertNull(GrailsClassUtils.getPropertyForGetter("get0"));
135175
}
136176

137177
public void testGetStaticField() {

0 commit comments

Comments
 (0)