Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OGNL improve getter/setter detection algorithm: #75

Merged
merged 7 commits into from
Sep 12, 2019
Merged

OGNL improve getter/setter detection algorithm: #75

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
387db2b
OGNL improve getter/setter detection algorithm:
JCgH4164838Gh792C124B5 Jun 10, 2019
db6f0ff
Merge branch 'ognl-3-1-x' of https://github.com/jkuhnert/ognl into lo…
JCgH4164838Gh792C124B5 Jun 12, 2019
d5bf014
Updated commit:
JCgH4164838Gh792C124B5 Jun 13, 2019
a2c0c95
Update:
JCgH4164838Gh792C124B5 Jun 15, 2019
4f8dec7
Update:
JCgH4164838Gh792C124B5 Jul 17, 2019
0dea426
Merge branch 'ognl-3-1-x' of https://github.com/jkuhnert/ognl into lo…
JCgH4164838Gh792C124B5 Sep 9, 2019
da3eb4b
Update source/target from 1.5 to 1.6 (Travis CI build failing with 1.5)
JCgH4164838Gh792C124B5 Sep 9, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,11 +78,12 @@

<plugins>
<plugin>
<!-- Raised source/target levels to 1.6 as Travis CI now rejects 1.5 -->
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-compiler-plugin</artifactId>
<configuration>
<source>1.5</source>
<target>1.5</target>
<source>1.6</source>
<target>1.6</target>
</configuration>
<executions>
<execution>
Expand All @@ -92,8 +93,8 @@
<goal>testCompile</goal>
</goals>
<configuration>
<source>1.5</source>
<target>1.5</target>
<source>1.6</source>
<target>1.6</target>
</configuration>
</execution>
</executions>
Expand Down
154 changes: 149 additions & 5 deletions src/java/ognl/OgnlRuntime.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -311,6 +311,36 @@ public class OgnlRuntime {
_disableOgnlSecurityManagerOnInit = initialFlagState;
}

/**
* Allow users to revert to the old "first match" lookup for getters/setters by OGNL using the JVM options:
* -Dognl.UseFirstMatchGetSetLookup=true
* -Dognl.UseFirstMatchGetSetLookup=false
*
* Note: Using the "false" value has the same effect as omitting the option completely.
* The default behaviour is to use the "best match" lookup for getters/setters.
* Using the "true" value reverts to the older "first match" lookup for getters/setters
* (in the event the "best match" processing causes issues for existing applications).
*/
static final String USE_FIRSTMATCH_GETSET_LOOKUP = "ognl.UseFirstMatchGetSetLookup";

/**
* Hold environment flag state associated with USE_FIRSTMATCH_GETSET_LOOKUP.
* Default: false (if not set)
*/
private static final boolean _useFirstMatchGetSetLookup;
static {
boolean initialFlagState = false;
try {
final String propertyString = System.getProperty(USE_FIRSTMATCH_GETSET_LOOKUP);
if (propertyString != null && propertyString.length() > 0) {
initialFlagState = Boolean.parseBoolean(propertyString);
}
} catch (Exception ex) {
// Unavailable (SecurityException, etc.)
}
_useFirstMatchGetSetLookup = initialFlagState;
}

static final ClassCache _methodAccessors = new ClassCacheImpl();
static final ClassCache _propertyAccessors = new ClassCacheImpl();
static final ClassCache _elementsAccessors = new ClassCacheImpl();
Expand Down Expand Up @@ -2128,6 +2158,13 @@ private static void addMethodToResult(Map result, Method method)

/**
* Backport of java.lang.reflect.Method#isDefault()
*
* JDK8+ supports Default Methods for interfaces. Default Methods are defined as:
* public, non-abstract and declared within an interface (must also be non-static).
*
* @param method The Method to check against the requirements for a Default Method.
*
* @return true If the Method qualifies as a Default Method, false otherwise.
*/
private static boolean isDefaultMethod(Method method)
{
Expand All @@ -2136,6 +2173,24 @@ private static boolean isDefaultMethod(Method method)
&& method.getDeclaringClass().isInterface();
}

/**
* Determine if the provided Method is a non-Default public Interface method.
*
* Public non-Default Methods are defined as:
* public, abstract, non-static and declared within an interface.
*
* @param method The Method to check against the requirements for a non-Default Method.
*
* @return true If method qualifies as a non-Default public Interface method, false otherwise.
*
* @since 3.1.25
*/
private static boolean isNonDefaultPublicInterfaceMethod(Method method) {
return ((method.getModifiers()
& (Modifier.ABSTRACT | Modifier.PUBLIC | Modifier.STATIC)) == (Modifier.ABSTRACT | Modifier.PUBLIC))
&& method.getDeclaringClass().isInterface();
}

public static Map getAllMethods(Class targetClass, boolean staticMethods)
{
ClassCache cache = (staticMethods ? _staticMethodCache : _instanceMethodCache);
Expand Down Expand Up @@ -2498,7 +2553,7 @@ private static void collectAccessors(Class c, String baseName, List result, bool
final Method[] methods = c.getDeclaredMethods();
for (int i = 0; i < methods.length; i++) {
if (c.isInterface()) {
if (isDefaultMethod(methods[i])) {
if (isDefaultMethod(methods[i]) || isNonDefaultPublicInterfaceMethod(methods[i])) {
addIfAccessor(result, methods[i], baseName, findSets);
}
continue;
Expand Down Expand Up @@ -2571,6 +2626,26 @@ public static Method getGetMethod(OgnlContext context, Class targetClass, String
return method;
}

/**
* Returns a qualifying get (getter) method, if one is available for the given targetClass and propertyName.
*
* Note: From OGNL 3.1.25 onward, this method will attempt to find the first get getter method(s) that match:
* 1) First get (getter) method, whether public or not.
* 2) First public get (getter) method, provided the method's declaring class is also public.
* This may be the same as 1), if 1) is also public and its declaring class is also public.
* 3) First public non-Default interface get (getter) method, provided the method's declaring class is also public.
* The <b>order of preference (priority)<b> for the above matches will be <b>2</b> (1st public getter),
* <b>3</b> (1st public non-Default interface getter), <b>1</b> (1st getter of any kind).
* This updated methodology should help limit the need to modify method accessibility levels in some circumstances.
*
* @param context The current execution context.
* @param targetClass Class to search for a get method (getter).
* @param propertyName Name of the property for the get method (getter).
*
* @return
* @throws IntrospectionException
* @throws OgnlException
*/
private static Method _getGetMethod(OgnlContext context, Class targetClass, String propertyName)
throws IntrospectionException, OgnlException
{
Expand All @@ -2580,17 +2655,34 @@ private static Method _getGetMethod(OgnlContext context, Class targetClass, Stri

if (methods != null)
{
Method firstGetter = null;
Method firstPublicGetter = null;
Method firstNonDefaultPublicInterfaceGetter = null;
for (int i = 0, icount = methods.size(); i < icount; i++)
{
Method m = (Method) methods.get(i);
Class[] mParameterTypes = findParameterTypes(targetClass, m); //getParameterTypes(m);

if (mParameterTypes.length == 0)
{
result = m;
break;
boolean declaringClassIsPublic = Modifier.isPublic(m.getDeclaringClass().getModifiers());
if (firstGetter == null) {
firstGetter = m;
if (_useFirstMatchGetSetLookup) {
break; // Stop looking (emulate original logic, return 1st match)
}
}
if (firstPublicGetter == null && Modifier.isPublic(m.getModifiers()) && declaringClassIsPublic) {
firstPublicGetter = m;
break; // Stop looking (this is the best possible match)
}
if (firstNonDefaultPublicInterfaceGetter == null && isNonDefaultPublicInterfaceMethod(m) && declaringClassIsPublic) {
firstNonDefaultPublicInterfaceGetter = m;
}
}
}
result = (firstPublicGetter != null) ? firstPublicGetter :
(firstNonDefaultPublicInterfaceGetter != null) ? firstNonDefaultPublicInterfaceGetter : firstGetter;
}

return result;
Expand Down Expand Up @@ -2628,6 +2720,26 @@ public static Method getSetMethod(OgnlContext context, Class targetClass, String
return method;
}

/**
* Returns a qualifying set (setter) method, if one is available for the given targetClass and propertyName.
*
* Note: From OGNL 3.1.25 onward, this method will attempt to find the first set setter method(s) that match:
* 1) First set (setter) method, whether public or not.
* 2) First public set (setter) method, provided the method's declaring class is also public.
* This may be the same as 1), if 1) is also public and its declaring class is also public.
* 3) First public non-Default interface set (setter) method, provided the method's declaring class is also public.
* The <b>order of preference (priority)<b> for the above matches will be <b>2</b> (1st public setter),
* <b>3</b> (1st public non-Default interface setter), <b>1</b> (1st setter of any kind).
* This updated methodology should help limit the need to modify method accessibility levels in some circumstances.
*
* @param context The current execution context.
* @param targetClass Class to search for a set method (setter).
* @param propertyName Name of the property for the set method (setter).
*
* @return
* @throws IntrospectionException
* @throws OgnlException
*/
private static Method _getSetMethod(OgnlContext context, Class targetClass, String propertyName)
throws IntrospectionException, OgnlException
{
Expand All @@ -2637,16 +2749,34 @@ private static Method _getSetMethod(OgnlContext context, Class targetClass, Stri

if (methods != null)
{
Method firstSetter = null;
Method firstPublicSetter = null;
Method firstNonDefaultPublicInterfaceSetter = null;
for (int i = 0, icount = methods.size(); i < icount; i++)
{
Method m = (Method) methods.get(i);
Class[] mParameterTypes = findParameterTypes(targetClass, m); //getParameterTypes(m);

if (mParameterTypes.length == 1) {
result = m;
break;
boolean declaringClassIsPublic = Modifier.isPublic(m.getDeclaringClass().getModifiers());
if (firstSetter == null) {
firstSetter = m;
if (_useFirstMatchGetSetLookup) {
break; // Stop looking (emulate original logic, return 1st match)
}
}
if (firstPublicSetter == null && Modifier.isPublic(m.getModifiers()) && declaringClassIsPublic) {
firstPublicSetter = m;
break; // Stop looking (this is the best possible match)
}
if (firstNonDefaultPublicInterfaceSetter == null && isNonDefaultPublicInterfaceMethod(m) && declaringClassIsPublic) {
firstNonDefaultPublicInterfaceSetter = m;
}
}
}

result = (firstPublicSetter != null) ? firstPublicSetter :
(firstNonDefaultPublicInterfaceSetter != null) ? firstNonDefaultPublicInterfaceSetter : firstSetter;
}

return result;
Expand Down Expand Up @@ -3813,4 +3943,18 @@ public static boolean usingJDK9PlusAccessHandler() {
return (_jdk9Plus && _useJDK9PlusAccessHandler);
}

/**
* Returns the value of the flag indicating whether the old "first match" lookup for
* getters/setters is in effect or not.
*
* Note: Value is controlled by a Java option flag {@link OgnlRuntime#USE_FIRSTMATCH_GETSET_LOOKUP}.
*
* @return true if the old "first match" lookup is in effect, false otherwise.
*
* @since 3.1.25
*/
public static boolean getUseFirstMatchGetSetLookupValue() {
return _useFirstMatchGetSetLookup;
}

}
29 changes: 29 additions & 0 deletions src/test/java/ognl/OgnlRuntimeTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -471,4 +471,33 @@ public void testStricterInvocationMode() {
}
}

/**
* Test OgnlRuntime value for _useFirstMatchGetSetLookup based on the System property
* represented by {@link OgnlRuntime#USE_FIRSTMATCH_GETSET_LOOKUP}.
*/
@Test
public void testUseFirstMatchGetSetStateFlag() {
// Ensure no exceptions, basic ouput for test report and sanity check on flag state.
final boolean defaultValue = false; // Expected non-configured default
boolean optionDefinedInEnvironment = false; // Track if option defined in environment
boolean flagValueFromEnvironment = false; // Value result from environment retrieval
try {
final String propertyString = System.getProperty(OgnlRuntime.USE_FIRSTMATCH_GETSET_LOOKUP);
if (propertyString != null && propertyString.length() > 0) {
optionDefinedInEnvironment = true;
flagValueFromEnvironment = Boolean.parseBoolean(propertyString);
}
} catch (Exception ex) {
// Unavailable (SecurityException, etc.)
}
if (optionDefinedInEnvironment) {
System.out.println("System property " + OgnlRuntime.USE_FIRSTMATCH_GETSET_LOOKUP + " value: " + flagValueFromEnvironment);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO it would be better to use logging instead of sysout even for testing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi.
Given the nature of this library, having tests output to sysout seems sufficient for now (AFAIK it's the only mechanism that has been available up until now, and the results are clearly visible in the Maven test run output).
Maybe someone will introduce a testing-only logging replacement for the sysouts in the tests in a future PR. 😄

} else {
System.out.println("System property " + OgnlRuntime.USE_FIRSTMATCH_GETSET_LOOKUP + " not present. Default value should be: " + defaultValue);
}
System.out.println("Current OGNL value for Use First Match Get/Set State Flag: " + OgnlRuntime.getUseFirstMatchGetSetLookupValue());
Assert.assertEquals("Mismatch between system property (or default) and OgnlRuntime _useFirstMatchGetSetLookup flag state ?",
optionDefinedInEnvironment ? flagValueFromEnvironment : defaultValue, OgnlRuntime.getUseFirstMatchGetSetLookupValue());
}

}