You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@brooklyn.apache.org by sv...@apache.org on 2017/06/02 10:50:43 UTC
[2/3] brooklyn-server git commit: PR #714: incorporate review comments
PR #714: incorporate review comments
Project: http://git-wip-us.apache.org/repos/asf/brooklyn-server/repo
Commit: http://git-wip-us.apache.org/repos/asf/brooklyn-server/commit/4d13e80d
Tree: http://git-wip-us.apache.org/repos/asf/brooklyn-server/tree/4d13e80d
Diff: http://git-wip-us.apache.org/repos/asf/brooklyn-server/diff/4d13e80d
Branch: refs/heads/master
Commit: 4d13e80de0b2dfda4a119e0c433a9e832ba14001
Parents: 077fcf9
Author: Aled Sage <al...@gmail.com>
Authored: Fri Jun 2 09:59:49 2017 +0100
Committer: Aled Sage <al...@gmail.com>
Committed: Fri Jun 2 09:59:49 2017 +0100
----------------------------------------------------------------------
.../util/core/flags/MethodCoercions.java | 6 +-
.../javalang/MethodAccessibleReflections.java | 89 +++++++++++++-------
.../brooklyn/util/javalang/Reflections.java | 33 +++++---
.../brooklyn/util/javalang/ReflectionsTest.java | 32 +++++--
4 files changed, 108 insertions(+), 52 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/4d13e80d/core/src/main/java/org/apache/brooklyn/util/core/flags/MethodCoercions.java
----------------------------------------------------------------------
diff --git a/core/src/main/java/org/apache/brooklyn/util/core/flags/MethodCoercions.java b/core/src/main/java/org/apache/brooklyn/util/core/flags/MethodCoercions.java
index 1934872..a334fa7 100644
--- a/core/src/main/java/org/apache/brooklyn/util/core/flags/MethodCoercions.java
+++ b/core/src/main/java/org/apache/brooklyn/util/core/flags/MethodCoercions.java
@@ -22,7 +22,6 @@ import static com.google.common.base.Preconditions.checkNotNull;
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
-import java.lang.reflect.Modifier;
import java.lang.reflect.Type;
import java.util.Arrays;
import java.util.List;
@@ -33,7 +32,6 @@ import org.apache.brooklyn.util.exceptions.Exceptions;
import org.apache.brooklyn.util.guava.Maybe;
import org.apache.brooklyn.util.javalang.Reflections;
-import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Optional;
import com.google.common.base.Predicate;
import com.google.common.base.Predicates;
@@ -86,7 +84,7 @@ public class MethodCoercions {
Optional<Method> matchingMethod = Iterables.tryFind(methods, matchSingleParameterMethod(methodName, argument));
if (matchingMethod.isPresent()) {
Method method = matchingMethod.get();
- Method accessibleMethod = Reflections.findAccessibleMethod(method);
+ Method accessibleMethod = Reflections.findAccessibleMethod(method).get();
try {
Type paramType = method.getGenericParameterTypes()[0];
Object coercedArgument = TypeCoercions.coerce(argument, TypeToken.of(paramType));
@@ -170,7 +168,7 @@ public class MethodCoercions {
Optional<Method> matchingMethod = Iterables.tryFind(methods, matchMultiParameterMethod(arguments));
if (matchingMethod.isPresent()) {
Method method = matchingMethod.get();
- Method accessibleMethod = Reflections.findAccessibleMethod(method);
+ Method accessibleMethod = Reflections.findAccessibleMethod(method).get();
try {
int numOptionParams = ((List<?>)arguments).size();
Object[] coercedArguments = new Object[numOptionParams];
http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/4d13e80d/utils/common/src/main/java/org/apache/brooklyn/util/javalang/MethodAccessibleReflections.java
----------------------------------------------------------------------
diff --git a/utils/common/src/main/java/org/apache/brooklyn/util/javalang/MethodAccessibleReflections.java b/utils/common/src/main/java/org/apache/brooklyn/util/javalang/MethodAccessibleReflections.java
index 5e8b98a..b73085d 100644
--- a/utils/common/src/main/java/org/apache/brooklyn/util/javalang/MethodAccessibleReflections.java
+++ b/utils/common/src/main/java/org/apache/brooklyn/util/javalang/MethodAccessibleReflections.java
@@ -18,6 +18,7 @@
*/
package org.apache.brooklyn.util.javalang;
+import java.lang.reflect.Member;
import java.lang.reflect.Method;
import java.lang.reflect.Modifier;
import java.util.Set;
@@ -50,6 +51,12 @@ class MethodAccessibleReflections {
*/
private static final Set<String> SET_ACCESSIBLE_SUCCEEDED_LOGGED_METHODS = Sets.newConcurrentHashSet();
+ /**
+ * Contains method.toString() representations for methods for which {@link #findAccessibleMethod(Method)}
+ * failed to find a suitable publicly accessible method.
+ */
+ private static final Set<String> FIND_ACCESSIBLE_FAILED_LOGGED_METHODS = Sets.newConcurrentHashSet();
+
static boolean trySetAccessible(Method method) {
try {
method.setAccessible(true);
@@ -70,42 +77,66 @@ class MethodAccessibleReflections {
return false;
}
}
+
+ // Copy of org.apache.commons.lang3.reflect.MemberUtils, except not checking !m.isSynthetic()
+ static boolean isAccessible(Member m) {
+ return m != null && Modifier.isPublic(m.getModifiers());
+ }
+ static boolean isAccessible(Class<?> c) {
+ return c != null && Modifier.isPublic(c.getModifiers());
+ }
+
/**
* @see {@link Reflections#findAccessibleMethod(Method)}
*/
- static Method findAccessibleMethod(Method method) {
- if (!Modifier.isPublic(method.getModifiers())) {
- trySetAccessible(method);
- return method;
- }
- boolean declaringClassPublic = Modifier.isPublic(method.getDeclaringClass().getModifiers());
- if (!declaringClassPublic) {
- // reflectively calling a public method on a private class can fail, unless we first set it
- // call setAccessible. But first see if there is a public method on a public super-type
- // that we can call instead!
- Maybe<Method> publicMethod = tryFindPublicEquivalent(method);
- if (publicMethod.isPresent()) {
- LOG.debug("Switched method for publicly accessible equivalent: method={}; origMethod={}", publicMethod.get(), method);
- return publicMethod.get();
- } else {
- trySetAccessible(method);
- return method;
+ // Similar to org.apache.commons.lang3.reflect.MethodUtils.getAccessibleMethod
+ // We could delegate to that, but currently brooklyn-utils-common doesn't depend on commons-lang3; maybe it should?
+ static Maybe<Method> findAccessibleMethod(Method method) {
+ if (!isAccessible(method)) {
+ String err = "Method is not public, so not normally accessible for "+method;
+ if (FIND_ACCESSIBLE_FAILED_LOGGED_METHODS.add(method.toString())) {
+ LOG.warn(err+"; usage may subsequently fail");
}
+ return Maybe.absent(err);
}
- return method;
- }
-
- private static Maybe<Method> tryFindPublicEquivalent(Method method) {
+ boolean declaringClassAccessible = isAccessible(method.getDeclaringClass());
+ if (declaringClassAccessible) {
+ return Maybe.of(method);
+ }
+
+ // reflectively calling a public method on a private class can fail (unless one first
+ // calls setAccessible). Check if this overrides a public method on a public super-type
+ // that we can call instead!
+
if (Modifier.isStatic(method.getModifiers())) {
- return Maybe.absent();
+ String err = "Static method not declared on a public class, so not normally accessible for "+method;
+ if (FIND_ACCESSIBLE_FAILED_LOGGED_METHODS.add(method.toString())) {
+ LOG.warn(err+"; usage may subsequently fail");
+ }
+ return Maybe.absent(err);
}
+
+ Maybe<Method> altMethod = tryFindAccessibleEquivalent(method);
+ if (altMethod.isPresent()) {
+ LOG.debug("Switched method for publicly accessible equivalent: method={}; origMethod={}", altMethod.get(), method);
+ return altMethod;
+ } else {
+ String err = "No accessible (overridden) method found in public super-types for method " + method;
+ if (FIND_ACCESSIBLE_FAILED_LOGGED_METHODS.add(method.toString())) {
+ LOG.warn(err);
+ }
+ return Maybe.absent(err);
+ }
+ }
+
+ private static Maybe<Method> tryFindAccessibleEquivalent(Method method) {
Class<?> clazz = method.getDeclaringClass();
- for (Class<?> interf : clazz.getInterfaces()) {
- Maybe<Method> altMethod = tryFindPublicMethod(interf, method.getName(), method.getParameterTypes());
+ for (Class<?> interf : Reflections.getAllInterfaces(clazz)) {
+ Maybe<Method> altMethod = tryFindAccessibleMethod(interf, method.getName(), method.getParameterTypes());
if (altMethod.isPresent()) {
return altMethod;
}
@@ -113,7 +144,7 @@ class MethodAccessibleReflections {
Class<?> superClazz = clazz.getSuperclass();
while (superClazz != null) {
- Maybe<Method> altMethod = tryFindPublicMethod(superClazz, method.getName(), method.getParameterTypes());
+ Maybe<Method> altMethod = tryFindAccessibleMethod(superClazz, method.getName(), method.getParameterTypes());
if (altMethod.isPresent()) {
return altMethod;
}
@@ -123,18 +154,18 @@ class MethodAccessibleReflections {
return Maybe.absent();
}
- private static Maybe<Method> tryFindPublicMethod(Class<?> clazz, String methodName, Class<?>... parameterTypes) {
- if (!Modifier.isPublic(clazz.getModifiers())) {
+ private static Maybe<Method> tryFindAccessibleMethod(Class<?> clazz, String methodName, Class<?>... parameterTypes) {
+ if (!isAccessible(clazz)) {
return Maybe.absent();
}
try {
Method altMethod = clazz.getMethod(methodName, parameterTypes);
- if (Modifier.isPublic(altMethod.getModifiers()) && !Modifier.isStatic(altMethod.getModifiers())) {
+ if (isAccessible(altMethod) && !Modifier.isStatic(altMethod.getModifiers())) {
return Maybe.of(altMethod);
}
} catch (NoSuchMethodException | SecurityException e) {
- // Not found; return absent
+ // Not found; swallow, and return absent
}
return Maybe.absent();
http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/4d13e80d/utils/common/src/main/java/org/apache/brooklyn/util/javalang/Reflections.java
----------------------------------------------------------------------
diff --git a/utils/common/src/main/java/org/apache/brooklyn/util/javalang/Reflections.java b/utils/common/src/main/java/org/apache/brooklyn/util/javalang/Reflections.java
index 09992e8..be6e9a4 100644
--- a/utils/common/src/main/java/org/apache/brooklyn/util/javalang/Reflections.java
+++ b/utils/common/src/main/java/org/apache/brooklyn/util/javalang/Reflections.java
@@ -1112,19 +1112,32 @@ public class Reflections {
}
/**
- * Attempts to find an equivalent accessible method to be invoked, or failing that will call
- * {@link Method#setAccessible(boolean)} if either of the method or the declaring class are
- * not public.
+ * Attempts to find an equivalent accessible method to be invoked (or if the given method is
+ * already accessible, then return it). Otherwise return absent.
*
- * For example, if a {@code method} is declared on a private sub-class, but the method is also
- * declared on a public super-class, then this method will return the {@link Method} instance
- * for the public super-class (assuming the method is not static).
+ * "Accessible" means that it is a public method declared on a public type.
*
- * If the call to {@link Method#setAccessible(boolean)} fails, this method will return anyway.
- * It will log.warn once per method signature for which we fail to set it accessible. It will
- * also log.warn about succeeding (once per method signature) as this is discouraged!
+ * For example, if {@code method} is declared on a private sub-class, but that overides a
+ * method declared on a public super-class, then this method will return the {@link Method}
+ * instance for the public super-class (assuming the method is not static).
+ *
+ * If no better method could be found, it does a log.warn (once per method signature, per
+ * jvm-invocation), and then returns absent.
*/
- public static Method findAccessibleMethod(Method method) {
+ public static Maybe<Method> findAccessibleMethod(Method method) {
return MethodAccessibleReflections.findAccessibleMethod(method);
}
+
+ /**
+ * Calls {@link Method#setAccessible(boolean)} "safely", wrapping in a try-catch block so that
+ * the exception is never propagated.
+ * <p>
+ * It will log.warn once per method signature for which we fail to set it accessible. It will
+ * also log.warn if it succeeds (once per method signature) as this is discouraged!
+ *
+ * @return True if setAccessible succeeded; false otherwise
+ */
+ public static boolean trySetAccessible(Method method) {
+ return MethodAccessibleReflections.trySetAccessible(method);
+ }
}
http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/4d13e80d/utils/common/src/test/java/org/apache/brooklyn/util/javalang/ReflectionsTest.java
----------------------------------------------------------------------
diff --git a/utils/common/src/test/java/org/apache/brooklyn/util/javalang/ReflectionsTest.java b/utils/common/src/test/java/org/apache/brooklyn/util/javalang/ReflectionsTest.java
index a58c42e..3c628a9 100644
--- a/utils/common/src/test/java/org/apache/brooklyn/util/javalang/ReflectionsTest.java
+++ b/utils/common/src/test/java/org/apache/brooklyn/util/javalang/ReflectionsTest.java
@@ -186,20 +186,30 @@ public class ReflectionsTest {
Method subMethodOnSuperClass = PrivateClass.class.getMethod("methodOnSuperClass", new Class[0]);
Method methodOnInterface = PublicInterface.class.getMethod("methodOnInterface", new Class[0]);
Method subMethodOnInterface = PrivateClass.class.getMethod("methodOnInterface", new Class[0]);
+ Method inaccessibleStaticMethod = PrivateClass.class.getMethod("otherStaticMethod", new Class[0]);
+ Method inaccessiblePublicMethod = PrivateClass.class.getMethod("otherPublicMethod", new Class[0]);
+ Method inaccessibleProtectedMethod = PrivateClass.class.getDeclaredMethod("otherProtectedMethod", new Class[0]);
- assertEquals(Reflections.findAccessibleMethod(objectHashCode), objectHashCode);
- assertEquals(Reflections.findAccessibleMethod(methodOnSuperClass), methodOnSuperClass);
- assertEquals(Reflections.findAccessibleMethod(methodOnInterface), methodOnInterface);
- assertEquals(Reflections.findAccessibleMethod(subMethodOnSuperClass), methodOnSuperClass);
- assertEquals(Reflections.findAccessibleMethod(subMethodOnInterface), methodOnInterface);
+ assertEquals(Reflections.findAccessibleMethod(objectHashCode).get(), objectHashCode);
+ assertEquals(Reflections.findAccessibleMethod(methodOnSuperClass).get(), methodOnSuperClass);
+ assertEquals(Reflections.findAccessibleMethod(methodOnInterface).get(), methodOnInterface);
+ assertEquals(Reflections.findAccessibleMethod(subMethodOnSuperClass).get(), methodOnSuperClass);
+ assertEquals(Reflections.findAccessibleMethod(subMethodOnInterface).get(), methodOnInterface);
+
+ assertFalse(Reflections.findAccessibleMethod(inaccessibleStaticMethod).isPresent());
+ assertFalse(Reflections.findAccessibleMethod(inaccessiblePublicMethod).isPresent());
+ assertFalse(Reflections.findAccessibleMethod(inaccessibleProtectedMethod).isPresent());
}
+ // I wanted to use LogWatcher to confirm we only get log.warn once, but that is in
+ // brooklyn-test-support, which depends on brooklyn-utils-common (where this class is).
+ // Unfortunately that would create a circular dependency.
@Test
- public void testFindAccessibleMethodCallsSetAccessible() throws Exception {
- Method inaccessibleOtherMethod = PrivateClass.class.getMethod("otherMethod", new Class[0]);
+ public void testTrySetAccessible() throws Exception {
+ Method inaccessibleOtherMethod = PrivateClass.class.getMethod("otherPublicMethod", new Class[0]);
assertFalse(inaccessibleOtherMethod.isAccessible());
- assertEquals(Reflections.findAccessibleMethod(inaccessibleOtherMethod), inaccessibleOtherMethod);
+ Reflections.trySetAccessible(inaccessibleOtherMethod);
assertTrue(inaccessibleOtherMethod.isAccessible());
}
@@ -236,6 +246,10 @@ public class ReflectionsTest {
@Override
public void methodOnInterface() {}
- public void otherMethod() {}
+ public void otherPublicMethod() {}
+
+ protected void otherProtectedMethod() {}
+
+ public static void otherStaticMethod() {}
}
}