Skip to content

Commit

Permalink
Closes #714
Browse files Browse the repository at this point in the history
BROOKLYN-513: fix coercion for jclouds reflective builder calls
  • Loading branch information
neykov committed Jun 2, 2017
2 parents ad5d43e + 4d13e80 commit 97eaf5a
Show file tree
Hide file tree
Showing 6 changed files with 373 additions and 3 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@

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.base.Optional;
import com.google.common.base.Predicate;
Expand Down Expand Up @@ -83,10 +84,11 @@ public static Maybe<?> tryFindAndInvokeSingleParameterMethod(final Object instan
Optional<Method> matchingMethod = Iterables.tryFind(methods, matchSingleParameterMethod(methodName, argument));
if (matchingMethod.isPresent()) {
Method method = matchingMethod.get();
Method accessibleMethod = Reflections.findAccessibleMethod(method).get();
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);
}
Expand Down Expand Up @@ -166,6 +168,7 @@ public static Maybe<?> tryFindAndInvokeMultiParameterMethod(Object instanceOrCla
Optional<Method> matchingMethod = Iterables.tryFind(methods, matchMultiParameterMethod(arguments));
if (matchingMethod.isPresent()) {
Method method = matchingMethod.get();
Method accessibleMethod = Reflections.findAccessibleMethod(method).get();
try {
int numOptionParams = ((List<?>)arguments).size();
Object[] coercedArguments = new Object[numOptionParams];
Expand All @@ -174,7 +177,7 @@ public static Maybe<?> tryFindAndInvokeMultiParameterMethod(Object instanceOrCla
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);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,18 @@ public void testTryFindAndInvokeBestMatchingMethod() throws Exception {
assertTrue(instance.wasSingleCollectionParameterMethodCalled());
}

@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();
Expand Down Expand Up @@ -188,4 +200,40 @@ public boolean wasSingleCollectionParameterMethodCalled() {
return singleCollectionParameterMethodCalled;
}
}
}

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;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
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;

Expand Down Expand Up @@ -97,6 +98,17 @@ public void testCompositeOfCreatedObjs() {
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;
Expand Down Expand Up @@ -247,4 +259,50 @@ public static Builder builder() {
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();
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,173 @@
/*
* 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.Member;
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();

/**
* 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);
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;
}
}

// 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)}
*/
// 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);
}

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())) {
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 : Reflections.getAllInterfaces(clazz)) {
Maybe<Method> altMethod = tryFindAccessibleMethod(interf, method.getName(), method.getParameterTypes());
if (altMethod.isPresent()) {
return altMethod;
}
}

Class<?> superClazz = clazz.getSuperclass();
while (superClazz != null) {
Maybe<Method> altMethod = tryFindAccessibleMethod(superClazz, method.getName(), method.getParameterTypes());
if (altMethod.isPresent()) {
return altMethod;
}
superClazz = superClazz.getSuperclass();
}

return Maybe.absent();
}

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 (isAccessible(altMethod) && !Modifier.isStatic(altMethod.getModifiers())) {
return Maybe.of(altMethod);
}
} catch (NoSuchMethodException | SecurityException e) {
// Not found; swallow, and return absent
}

return Maybe.absent();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -1110,4 +1110,34 @@ public static List<?> arrayToList(Object input) {
}
return result;
}

/**
* Attempts to find an equivalent accessible method to be invoked (or if the given method is
* already accessible, then return it). Otherwise return absent.
*
* "Accessible" means that it is a public method declared on a public type.
*
* 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 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);
}
}
Loading

0 comments on commit 97eaf5a

Please sign in to comment.