brooklyn-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From s...@apache.org
Subject [1/3] brooklyn-server git commit: BROOKLYN-513: fix coercion for jclouds reflective builder calls
Date Fri, 02 Jun 2017 10:50:42 GMT
Repository: brooklyn-server
Updated Branches:
  refs/heads/master ad5d43eb9 -> 97eaf5acd


BROOKLYN-513: fix coercion for jclouds reflective builder calls

Project: http://git-wip-us.apache.org/repos/asf/brooklyn-server/repo
Commit: http://git-wip-us.apache.org/repos/asf/brooklyn-server/commit/077fcf92
Tree: http://git-wip-us.apache.org/repos/asf/brooklyn-server/tree/077fcf92
Diff: http://git-wip-us.apache.org/repos/asf/brooklyn-server/diff/077fcf92

Branch: refs/heads/master
Commit: 077fcf92788d7ae90356d89dc6c4f9009c6c9fe2
Parents: ad5d43e
Author: Aled Sage <aled.sage@gmail.com>
Authored: Thu Jun 1 13:30:32 2017 +0100
Committer: Aled Sage <aled.sage@gmail.com>
Committed: Thu Jun 1 13:30:32 2017 +0100

----------------------------------------------------------------------
 .../util/core/flags/MethodCoercions.java        |   9 +-
 .../util/core/flags/MethodCoercionsTest.java    |  50 ++++++-
 .../JcloudsTypeCoercionsWithBuilderTest.java    |  58 ++++++++
 .../javalang/MethodAccessibleReflections.java   | 142 +++++++++++++++++++
 .../brooklyn/util/javalang/Reflections.java     |  17 +++
 .../brooklyn/util/javalang/ReflectionsTest.java |  44 ++++++
 6 files changed, 317 insertions(+), 3 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/077fcf92/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 b0afcdd..1934872 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,6 +22,7 @@ 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;
@@ -30,7 +31,9 @@ import javax.annotation.Nullable;
 
 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;
@@ -83,10 +86,11 @@ public class MethodCoercions {
         Optional<Method> matchingMethod = Iterables.tryFind(methods, matchSingleParameterMethod(methodName,
argument));
         if (matchingMethod.isPresent()) {
             Method method = matchingMethod.get();
+            Method accessibleMethod = Reflections.findAccessibleMethod(method);
             try {
                 Type paramType = method.getGenericParameterTypes()[0];
                 Object coercedArgument = TypeCoercions.coerce(argument, TypeToken.of(paramType));
-                return Maybe.of(method.invoke(instance, coercedArgument));
+                return Maybe.of(accessibleMethod.invoke(instance, coercedArgument));
             } catch (IllegalAccessException | InvocationTargetException e) {
                 throw Exceptions.propagate(e);
             }
@@ -166,6 +170,7 @@ public class MethodCoercions {
         Optional<Method> matchingMethod = Iterables.tryFind(methods, matchMultiParameterMethod(arguments));
         if (matchingMethod.isPresent()) {
             Method method = matchingMethod.get();
+            Method accessibleMethod = Reflections.findAccessibleMethod(method);
             try {
                 int numOptionParams = ((List<?>)arguments).size();
                 Object[] coercedArguments = new Object[numOptionParams];
@@ -174,7 +179,7 @@ public class MethodCoercions {
                     Type paramType = method.getGenericParameterTypes()[paramCount];
                     coercedArguments[paramCount] = TypeCoercions.coerce(argument, TypeToken.of(paramType));
                 }
-                return Maybe.of(method.invoke(instanceOrClazz, coercedArguments));
+                return Maybe.of(accessibleMethod.invoke(instanceOrClazz, coercedArguments));
             } catch (IllegalAccessException | InvocationTargetException e) {
                 throw Exceptions.propagate(e);
             }

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/077fcf92/core/src/test/java/org/apache/brooklyn/util/core/flags/MethodCoercionsTest.java
----------------------------------------------------------------------
diff --git a/core/src/test/java/org/apache/brooklyn/util/core/flags/MethodCoercionsTest.java
b/core/src/test/java/org/apache/brooklyn/util/core/flags/MethodCoercionsTest.java
index 57adb54..cc1d970 100644
--- a/core/src/test/java/org/apache/brooklyn/util/core/flags/MethodCoercionsTest.java
+++ b/core/src/test/java/org/apache/brooklyn/util/core/flags/MethodCoercionsTest.java
@@ -129,6 +129,18 @@ public class MethodCoercionsTest {
     }
 
     @Test
+    public void testTryFindAndInvokeBestMatchingMethodOnPrivateClassWithPublicSuper() throws
Exception {
+        PrivateClass instance = new PrivateClass();
+        Maybe<?> maybe = MethodCoercions.tryFindAndInvokeBestMatchingMethod(instance,
"methodOnSuperClass", "42");
+        assertTrue(maybe.isPresent());
+        assertTrue(instance.wasMethodOnSuperClassCalled());
+        
+        Maybe<?> maybe2 = MethodCoercions.tryFindAndInvokeBestMatchingMethod(instance,
"methodOnInterface", "42");
+        assertTrue(maybe2.isPresent());
+        assertTrue(instance.wasMethodOnInterfaceCalled());
+    }
+
+    @Test
     public void testTryFindAndInvokeSingleParameterMethod() throws Exception {
         TestClass instance = new TestClass();
         Maybe<?> maybe = MethodCoercions.tryFindAndInvokeSingleParameterMethod(instance,
"singleParameterMethod", "42");
@@ -188,4 +200,40 @@ public class MethodCoercionsTest {
             return singleCollectionParameterMethodCalled;
         }
     }
-}
\ No newline at end of file
+
+    public static abstract class PublicSuperClass {
+        public abstract PublicSuperClass methodOnSuperClass(int arg);
+    }
+        
+    public static interface PublicInterface {
+        public PublicInterface methodOnInterface(int arg);
+    }
+        
+    static class PrivateClass extends PublicSuperClass implements PublicInterface {
+
+        private boolean methodOnSuperClassCalled;
+        private boolean methodOnInterfaceCalled;
+
+        public PrivateClass() {}
+        
+        @Override
+        public PrivateClass methodOnSuperClass(int arg) {
+            methodOnSuperClassCalled = true;
+            return this;
+        }
+        
+        @Override
+        public PrivateClass methodOnInterface(int arg) {
+            methodOnInterfaceCalled = true;
+            return this;
+        }
+        
+        public boolean wasMethodOnSuperClassCalled() {
+            return methodOnSuperClassCalled;
+        }
+
+        public boolean wasMethodOnInterfaceCalled() {
+            return methodOnInterfaceCalled;
+        }
+    }
+}

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/077fcf92/locations/jclouds/src/test/java/org/apache/brooklyn/location/jclouds/JcloudsTypeCoercionsWithBuilderTest.java
----------------------------------------------------------------------
diff --git a/locations/jclouds/src/test/java/org/apache/brooklyn/location/jclouds/JcloudsTypeCoercionsWithBuilderTest.java
b/locations/jclouds/src/test/java/org/apache/brooklyn/location/jclouds/JcloudsTypeCoercionsWithBuilderTest.java
index f863102..815e646 100644
--- a/locations/jclouds/src/test/java/org/apache/brooklyn/location/jclouds/JcloudsTypeCoercionsWithBuilderTest.java
+++ b/locations/jclouds/src/test/java/org/apache/brooklyn/location/jclouds/JcloudsTypeCoercionsWithBuilderTest.java
@@ -22,6 +22,7 @@ import static org.apache.brooklyn.util.core.flags.TypeCoercions.coerce;
 import static org.testng.Assert.assertEquals;
 
 import org.apache.brooklyn.test.Asserts;
+import org.apache.brooklyn.util.core.flags.MethodCoercions;
 import org.apache.brooklyn.util.javalang.coerce.ClassCoercionException;
 import org.testng.annotations.Test;
 
@@ -97,6 +98,17 @@ public class JcloudsTypeCoercionsWithBuilderTest {
                 coerce(ImmutableMap.of("val",ImmutableMap.of("arg1", "val1", "arg2", "val2")),
MyCompositeClazz.class), 
                 MyCompositeClazz.builder().val((MyClazz.builder().arg1("val1").arg2("val2").build())).build());
     }
+    
+    @Test
+    public void testPrivateBuilderClass() throws Exception {
+        org.apache.brooklyn.location.jclouds.JcloudsTypeCoercionsWithBuilderTest.MyClazzWithBuilderReturningPrivateClass.Builder
builder = MyClazzWithBuilderReturningPrivateClass.builder();
+        MethodCoercions.tryFindAndInvokeSingleParameterMethod(builder, "arg1", "val1").get();
+
+        assertEquals(
+                coerce(ImmutableMap.of("arg1", "val1"), MyClazzWithBuilderReturningPrivateClass.class),

+                MyClazzWithBuilderReturningPrivateClass.builder().arg1("val1").build());
+        
+    }
 
     public static class MyClazz {
         private final String arg1;
@@ -247,4 +259,50 @@ public class JcloudsTypeCoercionsWithBuilderTest {
         private MyClazzWithNoNoargBuildMethod(String arg1) {
         }
     }
+    
+    public static class MyClazzWithBuilderReturningPrivateClass {
+        private final String arg1;
+        
+        public static abstract class Builder {
+            public abstract Builder arg1(String val);
+            public abstract MyClazzWithBuilderReturningPrivateClass build();
+        }
+
+        private static class PrivateBuilder extends Builder {
+            private String arg1;
+            
+            public Builder arg1(String val) {
+                this.arg1 = val;
+                return this;
+            }
+            public MyClazzWithBuilderReturningPrivateClass build() {
+                return new MyClazzWithBuilderReturningPrivateClass(arg1);
+            }
+        }
+
+        public static Builder builder() {
+            return new PrivateBuilder();
+        }
+        
+        private MyClazzWithBuilderReturningPrivateClass(String arg1) {
+            this.arg1 = arg1;
+        }
+        
+        @Override
+        public boolean equals(Object obj) {
+            if (obj == null || obj.getClass() != getClass()) return false;
+            MyClazzWithBuilderReturningPrivateClass o = (MyClazzWithBuilderReturningPrivateClass)
obj;
+            return Objects.equal(arg1, o.arg1);
+        }
+        
+        @Override
+        public int hashCode() {
+            return Objects.hashCode(arg1);
+        }
+        
+        @Override
+        public String toString() {
+            return MoreObjects.toStringHelper(this).add("arg1", arg1).toString();
+        }
+    }
 }

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/077fcf92/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
new file mode 100644
index 0000000..5e8b98a
--- /dev/null
+++ b/utils/common/src/main/java/org/apache/brooklyn/util/javalang/MethodAccessibleReflections.java
@@ -0,0 +1,142 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.brooklyn.util.javalang;
+
+import java.lang.reflect.Method;
+import java.lang.reflect.Modifier;
+import java.util.Set;
+
+import org.apache.brooklyn.util.guava.Maybe;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import com.google.common.collect.Sets;
+
+/**
+ * Use {@link Reflections} methods to access this. The methods are declared here (to this

+ * package-private class) so we can avoid having an ever-growing single Reflections class!
+ */
+class MethodAccessibleReflections {
+
+    private static final Logger LOG = LoggerFactory.getLogger(MethodAccessibleReflections.class);
+    
+    /**
+     * Contains method.toString() representations for methods we have logged about failing
to
+     * set accessible (or to find an alternative accessible version). Use this to ensure
we 
+     * log.warn just once per method, rather than risk flooding our log.
+     */
+    private static final Set<String> SET_ACCESSIBLE_FAILED_LOGGED_METHODS = Sets.newConcurrentHashSet();
+    
+    /**
+     * Contains method.toString() representations for methods we have logged about having
set
+     * accessible. Having to setAccessible is discouraged, so want a single log.warn once
per
+     * method.
+     */
+    private static final Set<String> SET_ACCESSIBLE_SUCCEEDED_LOGGED_METHODS = Sets.newConcurrentHashSet();
+    
+    static boolean trySetAccessible(Method method) {
+        try {
+            method.setAccessible(true);
+            if (SET_ACCESSIBLE_SUCCEEDED_LOGGED_METHODS.add(method.toString())) {
+                LOG.warn("Discouraged use of setAccessible, called for method " + method);
+            } else {
+                if (LOG.isTraceEnabled()) LOG.trace("Discouraged use of setAccessible, called
for method " + method);
+            }
+            return true;
+            
+        } catch (SecurityException e) {
+            boolean added = SET_ACCESSIBLE_FAILED_LOGGED_METHODS.add(method.toString());
+            if (added) {
+                LOG.warn("Problem setting accessible for method " + method, e);
+            } else {
+                if (LOG.isTraceEnabled()) LOG.trace("Problem setting accessible for method
" + method, e);
+            }
+            return false;
+        }
+    }
+
+    /**
+     * @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;
+            }
+        }
+        
+        return method;
+    }
+    
+    private static Maybe<Method> tryFindPublicEquivalent(Method method) {
+        if (Modifier.isStatic(method.getModifiers())) {
+            return Maybe.absent();
+        }
+        
+        Class<?> clazz = method.getDeclaringClass();
+        
+        for (Class<?> interf : clazz.getInterfaces()) {
+            Maybe<Method> altMethod = tryFindPublicMethod(interf, method.getName(),
method.getParameterTypes());
+            if (altMethod.isPresent()) {
+                return altMethod;
+            }
+        }
+        
+        Class<?> superClazz = clazz.getSuperclass();
+        while (superClazz != null) {
+            Maybe<Method> altMethod = tryFindPublicMethod(superClazz, method.getName(),
method.getParameterTypes());
+            if (altMethod.isPresent()) {
+                return altMethod;
+            }
+            superClazz = superClazz.getSuperclass();
+        }
+        
+        return Maybe.absent();
+    }
+
+    private static Maybe<Method> tryFindPublicMethod(Class<?> clazz, String methodName,
Class<?>... parameterTypes) {
+        if (!Modifier.isPublic(clazz.getModifiers())) {
+            return Maybe.absent();
+        }
+        
+        try {
+            Method altMethod = clazz.getMethod(methodName, parameterTypes);
+            if (Modifier.isPublic(altMethod.getModifiers()) && !Modifier.isStatic(altMethod.getModifiers()))
{
+                return Maybe.of(altMethod);
+            }
+        } catch (NoSuchMethodException | SecurityException e) {
+            // Not found; return absent
+        }
+        
+        return Maybe.absent();
+    }
+}

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/077fcf92/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 bedcc54..09992e8 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
@@ -1110,4 +1110,21 @@ public class Reflections {
         }
         return result;
     }
+    
+    /**
+     * 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.
+     * 
+     * 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).
+     * 
+     * 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!
+     */
+    public static Method findAccessibleMethod(Method method) {
+        return MethodAccessibleReflections.findAccessibleMethod(method);
+    }
 }

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/077fcf92/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 201cd86..a58c42e 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
@@ -19,6 +19,7 @@
 package org.apache.brooklyn.util.javalang;
 
 import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertFalse;
 import static org.testng.Assert.assertTrue;
 
 import java.lang.reflect.Field;
@@ -178,6 +179,30 @@ public class ReflectionsTest {
         assertEquals(Reflections.arrayToList((Object) new String[] {"a", "b"}), ImmutableList.of("a",
"b"));
     }
     
+    @Test
+    public void testFindAccessibleMethodFromSuperType() throws Exception {
+        Method objectHashCode = Object.class.getMethod("hashCode", new Class[0]);
+        Method methodOnSuperClass = PublicSuperClass.class.getMethod("methodOnSuperClass",
new Class[0]);
+        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]);
+        
+        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);
+    }
+    
+    @Test
+    public void testFindAccessibleMethodCallsSetAccessible() throws Exception {
+        Method inaccessibleOtherMethod = PrivateClass.class.getMethod("otherMethod", new
Class[0]);
+        
+        assertFalse(inaccessibleOtherMethod.isAccessible());
+        assertEquals(Reflections.findAccessibleMethod(inaccessibleOtherMethod), inaccessibleOtherMethod);
+        assertTrue(inaccessibleOtherMethod.isAccessible());
+    }
+    
     interface I { };
     interface J extends I { };
     interface K extends I, J { };
@@ -194,4 +219,23 @@ public class ReflectionsTest {
         Assert.assertEquals(Reflections.getAllInterfaces(C.class), ImmutableList.of(M.class,
K.class, I.class, J.class, L.class));
     }
 
+    public static abstract class PublicSuperClass {
+        public abstract void methodOnSuperClass();
+    }
+        
+    public static interface PublicInterface {
+        public void methodOnInterface();
+    }
+        
+    static class PrivateClass extends PublicSuperClass implements PublicInterface {
+        public PrivateClass() {}
+        
+        @Override
+        public void methodOnSuperClass() {}
+        
+        @Override
+        public void methodOnInterface() {}
+        
+        public void otherMethod() {}
+    }
 }


Mime
View raw message