From 2396f226e78a6d6e273d0a8a8068467d4c1f3b00 Mon Sep 17 00:00:00 2001 From: Valentin Aitken Date: Tue, 10 Jan 2017 21:29:25 +0200 Subject: [PATCH 1/4] Use stripped version of ScriptBytecodeAdapter.isCase - Optimize performance and avoid locks used in groovy dynamic invocation Should fix BROOKLYN-424 --- .../util/groovy/FromRunnableClosure.java | 4 +- .../util/groovy/GroovyJavaMethods.java | 23 ++++- .../util/groovy/GroovJavaMethodsTest.java | 89 +++++++++++++++++++ 3 files changed, 110 insertions(+), 6 deletions(-) create mode 100644 utils/groovy/src/test/java/org/apache/brooklyn/util/groovy/GroovJavaMethodsTest.java diff --git a/utils/groovy/src/main/java/org/apache/brooklyn/util/groovy/FromRunnableClosure.java b/utils/groovy/src/main/java/org/apache/brooklyn/util/groovy/FromRunnableClosure.java index 37fad878c7..3f60a3e9d4 100644 --- a/utils/groovy/src/main/java/org/apache/brooklyn/util/groovy/FromRunnableClosure.java +++ b/utils/groovy/src/main/java/org/apache/brooklyn/util/groovy/FromRunnableClosure.java @@ -22,7 +22,7 @@ import java.util.concurrent.Callable; -import org.codehaus.groovy.runtime.ScriptBytecodeAdapter; +import org.codehaus.groovy.runtime.DefaultGroovyMethods; public class FromRunnableClosure extends Closure { private static final long serialVersionUID = 1L; @@ -35,7 +35,7 @@ public FromRunnableClosure(Class owner, Runnable job) { @SuppressWarnings("unchecked") public T doCall() throws Throwable { - if (ScriptBytecodeAdapter.isCase(job, Callable.class)) { + if (DefaultGroovyMethods.isCase(job, Callable.class)) { return ((Callable)job).call(); } else { job.run(); diff --git a/utils/groovy/src/main/java/org/apache/brooklyn/util/groovy/GroovyJavaMethods.java b/utils/groovy/src/main/java/org/apache/brooklyn/util/groovy/GroovyJavaMethods.java index 7f019a91f8..ffd4c3b2e4 100644 --- a/utils/groovy/src/main/java/org/apache/brooklyn/util/groovy/GroovyJavaMethods.java +++ b/utils/groovy/src/main/java/org/apache/brooklyn/util/groovy/GroovyJavaMethods.java @@ -22,6 +22,7 @@ import org.apache.brooklyn.util.concurrent.CallableFromRunnable; import org.apache.brooklyn.util.exceptions.Exceptions; +import org.codehaus.groovy.runtime.DefaultGroovyMethods; import org.codehaus.groovy.runtime.ScriptBytecodeAdapter; import org.codehaus.groovy.runtime.callsite.CallSite; import org.codehaus.groovy.runtime.callsite.CallSiteArray; @@ -65,7 +66,7 @@ public static Callable callableFromClosure(final Closure job) { @SuppressWarnings("unchecked") public static Callable callableFromRunnable(final Runnable job) { try { - if (ScriptBytecodeAdapter.isCase(job, Callable.class)) { + if (safeGroovyIsCase(job, Callable.class)) { return (Callable)ScriptBytecodeAdapter.asType(job, Callable.class); } else { return CallableFromRunnable.newInstance(job, null); @@ -75,6 +76,20 @@ public static Callable callableFromRunnable(final Runnable job) { } } + /** + * Alternative implementation of {@link ScriptBytecodeAdapter#isCase(Object, Object)}
+ * Stripped down to work only for caseExpression of type java.lang.Class.
+ * It behaves the same way only for cases when caseExpression java.lang.Class does not implement isCase method.
+ * It goes directly to {@link DefaultGroovyMethods#isCase(Object, Object)} method instead of using Groovy dynamic invocation.
+ * This saves extra operations and avoids the locks used in Groovy dynamic invocation. See BROOKLYN-424. + */ + public static boolean safeGroovyIsCase(Object switchValue, Class caseExpression) { + if (caseExpression == null) { + return switchValue == null; + } + return DefaultGroovyMethods.isCase(caseExpression, switchValue); + } + public static Predicate predicateFromClosure(final Closure job) { return new Predicate() { @Override @@ -96,7 +111,7 @@ public T apply(F input) { @SuppressWarnings("unchecked") public static Predicate castToPredicate(Object o) { try { - if (ScriptBytecodeAdapter.isCase(o, Closure.class)) { + if (safeGroovyIsCase(o, Closure.class)) { return predicateFromClosure((Closure)o); } else { return (Predicate) o; @@ -111,7 +126,7 @@ public static Closure castToClosure(Object o) { try { if (ScriptBytecodeAdapter.compareEqual(o, null)) { return (Closure)ScriptBytecodeAdapter.castToType(o, Closure.class); - } else if (ScriptBytecodeAdapter.isCase(o, Closure.class)) { + } else if (safeGroovyIsCase(o, Closure.class)) { return (Closure)ScriptBytecodeAdapter.castToType(o, Closure.class); } else if (o instanceof Runnable) { return closureFromRunnable((Runnable)ScriptBytecodeAdapter.createPojoWrapper(ScriptBytecodeAdapter.castToType(o, Runnable.class), Runnable.class)); @@ -173,7 +188,7 @@ public static T elvis(Object... preferences) { @SuppressWarnings("unchecked") public static T fix(Object o) { try { - if (ScriptBytecodeAdapter.isCase(o, GString.class)) { + if (safeGroovyIsCase(o, GString.class)) { return (T)ScriptBytecodeAdapter.asType(o, String.class); } else { return (T)o; diff --git a/utils/groovy/src/test/java/org/apache/brooklyn/util/groovy/GroovJavaMethodsTest.java b/utils/groovy/src/test/java/org/apache/brooklyn/util/groovy/GroovJavaMethodsTest.java new file mode 100644 index 0000000000..f32e92ed85 --- /dev/null +++ b/utils/groovy/src/test/java/org/apache/brooklyn/util/groovy/GroovJavaMethodsTest.java @@ -0,0 +1,89 @@ +/* + * 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.groovy; + +import groovy.lang.Closure; +import groovy.lang.GString; +import org.testng.annotations.Test; + +import java.util.concurrent.Callable; + +import static org.apache.brooklyn.util.groovy.GroovyJavaMethods.truth; +import static org.testng.Assert.assertFalse; +import static org.testng.Assert.assertTrue; + +public class GroovJavaMethodsTest { + @Test + public void testTruth() { + assertTrue(truth("someString")); + assertTrue(truth(1)); + assertFalse(truth(false)); + assertFalse(truth(null)); + } + + @Test + public void testIsCase() throws Throwable { + assertFalse(callScriptBytecodeAdapter_isCase( + null, + GString.class)); + assertTrue( + callScriptBytecodeAdapter_isCase( + new GString(new String[]{"Hi"}) { + @Override public String[] getStrings() { + return new String[0]; + } + }, + GString.class)); + assertFalse( + callScriptBytecodeAdapter_isCase( + "exampleString", + GString.class)); + + assertTrue( + callScriptBytecodeAdapter_isCase( + new Callable() { + @Override public Void call() { + return null; + } + }, + Callable.class)); + assertFalse( + callScriptBytecodeAdapter_isCase( + "exampleString", + Callable.class)); + + assertTrue( + callScriptBytecodeAdapter_isCase( + new Closure(null) { + @Override public Void call() { + return null; + } + }, + Closure.class)); + assertFalse( + callScriptBytecodeAdapter_isCase( + "exampleString", + Closure.class)); + } + + private boolean callScriptBytecodeAdapter_isCase(Object switchValue, Class caseExpression) throws Throwable { +// return org.codehaus.groovy.runtime.ScriptBytecodeAdapter.isCase(switchValue, caseExpression); + return org.apache.brooklyn.util.groovy.GroovyJavaMethods.safeGroovyIsCase(switchValue, caseExpression); + } +} From 1e24e8271c30f514def0bacd578b44ceb8f371c3 Mon Sep 17 00:00:00 2001 From: Aled Sage Date: Wed, 11 Jan 2017 14:39:10 +0000 Subject: [PATCH 2/4] Remove groovy usage: use JavaGroovyEquivalents --- .../brooklyn/core/effector/MethodEffector.java | 4 ++-- .../brooklyn/core/entity/EntityFunctions.java | 2 +- .../core/mgmt/internal/EffectorUtils.java | 8 ++++---- .../core/objs/AbstractEntityAdjunct.java | 4 ++-- .../entity/group/DynamicFabricImpl.java | 9 ++++----- .../FixedListMachineProvisioningLocation.java | 12 ++++++------ .../LocalhostMachineProvisioningLocation.java | 8 ++++---- .../location/ssh/SshMachineLocation.java | 10 +++++----- .../brooklyn/util/core/flags/FlagUtils.java | 18 +++++++++--------- .../ssh/sshj/SshjClientConnection.java | 6 +++--- .../brooklyn/util/core/task/ScheduledTask.java | 10 +++++----- 11 files changed, 45 insertions(+), 46 deletions(-) diff --git a/core/src/main/java/org/apache/brooklyn/core/effector/MethodEffector.java b/core/src/main/java/org/apache/brooklyn/core/effector/MethodEffector.java index 0d5f909e80..351e86c990 100644 --- a/core/src/main/java/org/apache/brooklyn/core/effector/MethodEffector.java +++ b/core/src/main/java/org/apache/brooklyn/core/effector/MethodEffector.java @@ -30,9 +30,9 @@ import org.apache.brooklyn.core.annotation.EffectorParam; import org.apache.brooklyn.core.entity.AbstractEntity; import org.apache.brooklyn.core.mgmt.internal.EffectorUtils; +import org.apache.brooklyn.util.JavaGroovyEquivalents; import org.apache.brooklyn.util.core.flags.TypeCoercions; import org.apache.brooklyn.util.exceptions.Exceptions; -import org.apache.brooklyn.util.groovy.GroovyJavaMethods; import org.codehaus.groovy.runtime.MethodClosure; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -139,7 +139,7 @@ public MethodEffector(MethodClosure mc) { @SuppressWarnings("unchecked") protected MethodEffector(AnnotationsOnMethod anns, String description) { - super(anns.name, (Class)anns.returnType, anns.parameters, GroovyJavaMethods.elvis(description, anns.description)); + super(anns.name, (Class)anns.returnType, anns.parameters, JavaGroovyEquivalents.elvis(description, anns.description)); } @SuppressWarnings({ "rawtypes", "unchecked" }) diff --git a/core/src/main/java/org/apache/brooklyn/core/entity/EntityFunctions.java b/core/src/main/java/org/apache/brooklyn/core/entity/EntityFunctions.java index 505c0d67a2..75f20ae696 100644 --- a/core/src/main/java/org/apache/brooklyn/core/entity/EntityFunctions.java +++ b/core/src/main/java/org/apache/brooklyn/core/entity/EntityFunctions.java @@ -19,7 +19,7 @@ package org.apache.brooklyn.core.entity; import static com.google.common.base.Preconditions.checkNotNull; -import static org.apache.brooklyn.util.groovy.GroovyJavaMethods.elvis; +import static org.apache.brooklyn.util.JavaGroovyEquivalents.elvis; import java.util.Collection; import java.util.Map; diff --git a/core/src/main/java/org/apache/brooklyn/core/mgmt/internal/EffectorUtils.java b/core/src/main/java/org/apache/brooklyn/core/mgmt/internal/EffectorUtils.java index 1b298e3687..880a74b647 100644 --- a/core/src/main/java/org/apache/brooklyn/core/mgmt/internal/EffectorUtils.java +++ b/core/src/main/java/org/apache/brooklyn/core/mgmt/internal/EffectorUtils.java @@ -18,7 +18,7 @@ */ package org.apache.brooklyn.core.mgmt.internal; -import static org.apache.brooklyn.util.groovy.GroovyJavaMethods.truth; +import static org.apache.brooklyn.util.JavaGroovyEquivalents.groovyTruth; import java.lang.reflect.Method; import java.util.Arrays; @@ -156,7 +156,7 @@ private static Object[] prepareArgsForEffectorFromMap(Effector eff, Map m) { for (int index = 0; index < eff.getParameters().size(); index++) { ParameterType it = eff.getParameters().get(index); Object v; - if (truth(it.getName()) && m.containsKey(it.getName())) { + if (groovyTruth(it.getName()) && m.containsKey(it.getName())) { // argument is in the map v = m.remove(it.getName()); } else if (it instanceof BasicParameterType && ((BasicParameterType)it).hasDefaultValue()) { @@ -223,7 +223,7 @@ public static Object[] oldPrepareArgsForEffector(Effector eff, Object args) { if (l.size() >= newArgsNeeded) { //all supplied (unnamed) arguments must be used; ignore map newArgs.add(l.remove(0)); - } else if (truth(m) && truth(it.getName()) && m.containsKey(it.getName())) { + } else if (groovyTruth(m) && groovyTruth(it.getName()) && m.containsKey(it.getName())) { //some arguments were not supplied, and this one is in the map newArgs.add(m.remove(it.getName())); } else if (index == 0 && Map.class.isAssignableFrom(it.getParameterClass())) { @@ -249,7 +249,7 @@ public static Object[] oldPrepareArgsForEffector(Effector eff, Object args) { if (!l.isEmpty()) { throw new IllegalArgumentException("Invalid arguments ("+l.size()+" extra) for effector "+eff+": "+argsArray.length+" args"); } - if (truth(m) && !mapUsed) { + if (groovyTruth(m) && !mapUsed) { throw new IllegalArgumentException("Invalid arguments ("+m.size()+" extra named) for effector "+eff+": "+argsArray.length+" args"); } return newArgs.toArray(new Object[newArgs.size()]); diff --git a/core/src/main/java/org/apache/brooklyn/core/objs/AbstractEntityAdjunct.java b/core/src/main/java/org/apache/brooklyn/core/objs/AbstractEntityAdjunct.java index 4acf141b0d..636b7a6353 100644 --- a/core/src/main/java/org/apache/brooklyn/core/objs/AbstractEntityAdjunct.java +++ b/core/src/main/java/org/apache/brooklyn/core/objs/AbstractEntityAdjunct.java @@ -19,7 +19,7 @@ package org.apache.brooklyn.core.objs; import static com.google.common.base.Preconditions.checkState; -import static org.apache.brooklyn.util.groovy.GroovyJavaMethods.truth; +import static org.apache.brooklyn.util.JavaGroovyEquivalents.groovyTruth; import java.util.Collection; import java.util.Collections; @@ -156,7 +156,7 @@ public AbstractEntityAdjunct configure(Map flags) { FlagUtils.setAllConfigKeys(this, bag, false); leftoverProperties.putAll(bag.getUnusedConfig()); - if (!truth(name) && leftoverProperties.containsKey("displayName")) { + if (!groovyTruth(name) && leftoverProperties.containsKey("displayName")) { //TODO inconsistent with entity and location, where name is legacy and displayName is encouraged! //'displayName' is a legacy way to refer to a policy's name Preconditions.checkArgument(leftoverProperties.get("displayName") instanceof CharSequence, "'displayName' property should be a string"); diff --git a/core/src/main/java/org/apache/brooklyn/entity/group/DynamicFabricImpl.java b/core/src/main/java/org/apache/brooklyn/entity/group/DynamicFabricImpl.java index f95c38d566..3148a4e035 100644 --- a/core/src/main/java/org/apache/brooklyn/entity/group/DynamicFabricImpl.java +++ b/core/src/main/java/org/apache/brooklyn/entity/group/DynamicFabricImpl.java @@ -18,8 +18,8 @@ */ package org.apache.brooklyn.entity.group; -import static org.apache.brooklyn.util.groovy.GroovyJavaMethods.elvis; -import static org.apache.brooklyn.util.groovy.GroovyJavaMethods.truth; +import static org.apache.brooklyn.util.JavaGroovyEquivalents.elvis; +import static org.apache.brooklyn.util.JavaGroovyEquivalents.groovyTruth; import java.util.Arrays; import java.util.Collection; @@ -45,7 +45,6 @@ import org.apache.brooklyn.enricher.stock.Enrichers; import org.apache.brooklyn.util.collections.MutableList; import org.apache.brooklyn.util.exceptions.Exceptions; -import org.apache.brooklyn.util.groovy.GroovyJavaMethods; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -235,7 +234,7 @@ public void restart() { public Integer getFabricSize() { int result = 0; for (Entity child : getChildren()) { - result += GroovyJavaMethods.elvis(child.getAttribute(Changeable.GROUP_SIZE), 0); + result += elvis(child.getAttribute(Changeable.GROUP_SIZE), 0); } return result; } @@ -258,7 +257,7 @@ protected Entity addCluster(Location location) { String locationName = elvis(location.getDisplayName(), location.getDisplayName(), null); Map creation = Maps.newLinkedHashMap(); creation.putAll(getCustomChildFlags()); - if (truth(getDisplayNamePrefix()) || truth(getDisplayNameSuffix())) { + if (groovyTruth(getDisplayNamePrefix()) || groovyTruth(getDisplayNameSuffix())) { String displayName = "" + elvis(getDisplayNamePrefix(), "") + elvis(locationName, "unnamed") + elvis(getDisplayNameSuffix(),""); creation.put("displayName", displayName); } diff --git a/core/src/main/java/org/apache/brooklyn/location/byon/FixedListMachineProvisioningLocation.java b/core/src/main/java/org/apache/brooklyn/location/byon/FixedListMachineProvisioningLocation.java index cc4c2d15e0..c57a5b78b1 100644 --- a/core/src/main/java/org/apache/brooklyn/location/byon/FixedListMachineProvisioningLocation.java +++ b/core/src/main/java/org/apache/brooklyn/location/byon/FixedListMachineProvisioningLocation.java @@ -18,7 +18,7 @@ */ package org.apache.brooklyn.location.byon; -import static org.apache.brooklyn.util.groovy.GroovyJavaMethods.truth; +import static org.apache.brooklyn.util.JavaGroovyEquivalents.groovyTruth; import java.io.Closeable; import java.io.File; @@ -504,14 +504,14 @@ private Map makeConfig(String address) { address = address.substring(address.indexOf("@")+1); } Map config = MutableMap.of("address", address); - if (truth(user)) { + if (groovyTruth(user)) { config.put("user", user); config.put("sshconfig.user", user); } - if (truth(privateKeyPassphrase)) config.put("sshconfig.privateKeyPassphrase", privateKeyPassphrase); - if (truth(privateKeyFile)) config.put("sshconfig.privateKeyFile", privateKeyFile); - if (truth(privateKeyData)) config.put("sshconfig.privateKey", privateKeyData); - if (truth(localTempDir)) config.put("localTempDir", localTempDir); + if (groovyTruth(privateKeyPassphrase)) config.put("sshconfig.privateKeyPassphrase", privateKeyPassphrase); + if (groovyTruth(privateKeyFile)) config.put("sshconfig.privateKeyFile", privateKeyFile); + if (groovyTruth(privateKeyData)) config.put("sshconfig.privateKey", privateKeyData); + if (groovyTruth(localTempDir)) config.put("localTempDir", localTempDir); return config; } @SuppressWarnings("unchecked") diff --git a/core/src/main/java/org/apache/brooklyn/location/localhost/LocalhostMachineProvisioningLocation.java b/core/src/main/java/org/apache/brooklyn/location/localhost/LocalhostMachineProvisioningLocation.java index fbdb769761..3fb207f1c2 100644 --- a/core/src/main/java/org/apache/brooklyn/location/localhost/LocalhostMachineProvisioningLocation.java +++ b/core/src/main/java/org/apache/brooklyn/location/localhost/LocalhostMachineProvisioningLocation.java @@ -18,8 +18,8 @@ */ package org.apache.brooklyn.location.localhost; -import static org.apache.brooklyn.util.groovy.GroovyJavaMethods.elvis; -import static org.apache.brooklyn.util.groovy.GroovyJavaMethods.truth; +import static org.apache.brooklyn.util.JavaGroovyEquivalents.elvis; +import static org.apache.brooklyn.util.JavaGroovyEquivalents.groovyTruth; import java.io.File; import java.net.InetAddress; @@ -131,8 +131,8 @@ public static LocationSpec spec() { public LocalhostMachineProvisioningLocation configure(Map flags) { super.configure(flags); - if (!truth(getDisplayName())) { setDisplayName("localhost"); } - if (!truth(address)) address = getLocalhostInetAddress(); + if (!groovyTruth(getDisplayName())) { setDisplayName("localhost"); } + if (!groovyTruth(address)) address = getLocalhostInetAddress(); // TODO should try to confirm this machine is accessible on the given address ... but there's no // immediate convenience in java so early-trapping of that particular error is deferred diff --git a/core/src/main/java/org/apache/brooklyn/location/ssh/SshMachineLocation.java b/core/src/main/java/org/apache/brooklyn/location/ssh/SshMachineLocation.java index d19f08ea5d..7df55aafe9 100644 --- a/core/src/main/java/org/apache/brooklyn/location/ssh/SshMachineLocation.java +++ b/core/src/main/java/org/apache/brooklyn/location/ssh/SshMachineLocation.java @@ -18,7 +18,7 @@ */ package org.apache.brooklyn.location.ssh; -import static org.apache.brooklyn.util.groovy.GroovyJavaMethods.truth; +import static org.apache.brooklyn.util.JavaGroovyEquivalents.groovyTruth; import java.io.Closeable; import java.io.File; @@ -428,7 +428,7 @@ public SshMachineLocation configure(Map properties) { boolean deferConstructionChecks = (properties.containsKey("deferConstructionChecks") && TypeCoercions.coerce(properties.get("deferConstructionChecks"), Boolean.class)); if (!deferConstructionChecks) { if (getDisplayName() == null) { - setDisplayName((truth(user) ? user+"@" : "") + address.getHostName()); + setDisplayName((groovyTruth(user) ? user+"@" : "") + address.getHostName()); } } return this; @@ -547,7 +547,7 @@ public HostAndPort getSshHostAndPort() { } public String getUser() { - if (!truth(user)) { + if (!groovyTruth(user)) { if (config().getLocalRaw(SshTool.PROP_USER).isPresent()) { LOG.warn("User configuration for "+this+" set after deployment; deprecated behaviour may not be supported in future versions"); } @@ -582,7 +582,7 @@ protected T execSsh(final Map props, final Function LOG.trace("{} execSsh got pool: {}", this, pool); } - if (truth(props.get(CLOSE_CONNECTION.getName()))) { + if (groovyTruth(props.get(CLOSE_CONNECTION.getName()))) { Function close = new Function() { @Override public T apply(SshTool input) { @@ -608,7 +608,7 @@ protected SshTool connectSsh() { protected boolean previouslyConnected = false; protected SshTool connectSsh(Map props) { try { - if (!truth(user)) { + if (!groovyTruth(user)) { String newUser = getUser(); if (LOG.isTraceEnabled()) LOG.trace("For "+this+", setting user in connectSsh: oldUser="+user+"; newUser="+newUser); user = newUser; diff --git a/core/src/main/java/org/apache/brooklyn/util/core/flags/FlagUtils.java b/core/src/main/java/org/apache/brooklyn/util/core/flags/FlagUtils.java index 26f3e9d261..9a78ce5691 100644 --- a/core/src/main/java/org/apache/brooklyn/util/core/flags/FlagUtils.java +++ b/core/src/main/java/org/apache/brooklyn/util/core/flags/FlagUtils.java @@ -19,8 +19,8 @@ package org.apache.brooklyn.util.core.flags; import static com.google.common.base.Preconditions.checkNotNull; -import static org.apache.brooklyn.util.groovy.GroovyJavaMethods.elvis; -import static org.apache.brooklyn.util.groovy.GroovyJavaMethods.truth; +import static org.apache.brooklyn.util.JavaGroovyEquivalents.elvis; +import static org.apache.brooklyn.util.JavaGroovyEquivalents.groovyTruth; import java.lang.reflect.Field; import java.lang.reflect.Modifier; @@ -341,7 +341,7 @@ private static Map getFieldsWithFlagsInternal(Object o, Collecti SetFromFlag cf = f.getAnnotation(SetFromFlag.class); if (cf != null) { String flagName = elvis(cf.value(), f.getName()); - if (truth(flagName)) { + if (groovyTruth(flagName)) { result.put(flagName, getField(o, f)); } else { log.warn("Ignoring field {} of object {} as no flag name available", f, o); @@ -385,9 +385,9 @@ private static boolean setFieldFromFlagInternal(String flagName, Object fieldVal } private static void setFieldFromConfig(Object o, Field f, ConfigBag bag, SetFromFlag optionalAnnotation, boolean setDefaultVals) { - String flagName = optionalAnnotation==null ? null : (String)elvis(optionalAnnotation.value(), f.getName()); + String flagName = optionalAnnotation==null ? null : elvis(optionalAnnotation.value(), f.getName()); // prefer flag name, if present - if (truth(flagName) && bag.containsKey(flagName)) { + if (groovyTruth(flagName) && bag.containsKey(flagName)) { setField(o, f, bag.getStringKey(flagName), optionalAnnotation); return; } @@ -398,7 +398,7 @@ private static void setFieldFromConfig(Object o, Field f, ConfigBag bag, SetFrom setField(o, f, uncoercedValue, optionalAnnotation); return; } - if (setDefaultVals && optionalAnnotation!=null && truth(optionalAnnotation.defaultVal())) { + if (setDefaultVals && optionalAnnotation!=null && groovyTruth(optionalAnnotation.defaultVal())) { Object oldValue; try { f.setAccessible(true); @@ -522,7 +522,7 @@ public static Map getAnnotatedFields(Class type) { Map result = Maps.newLinkedHashMap(); for (Field f: getAllFields(type)) { SetFromFlag cf = f.getAnnotation(SetFromFlag.class); - if (truth(cf)) result.put(f, cf); + if (cf != null) result.put(f, cf); } return result; } @@ -556,7 +556,7 @@ public static Map getFieldsWithValues(Object o) { Field f = entry.getKey(); SetFromFlag cf = entry.getValue(); String flagName = elvis(cf.value(), f.getName()); - if (truth(flagName)) { + if (groovyTruth(flagName)) { if (!f.isAccessible()) f.setAccessible(true); result.put(flagName, f.get(o)); } @@ -584,7 +584,7 @@ public static void checkRequiredFields(Object o) { if (v==null) unsetFields.add(flagName); } } - if (truth(unsetFields)) { + if (groovyTruth(unsetFields)) { throw new IllegalStateException("Missing required "+(unsetFields.size()>1 ? "fields" : "field")+": "+unsetFields); } } catch (IllegalAccessException e) { diff --git a/core/src/main/java/org/apache/brooklyn/util/core/internal/ssh/sshj/SshjClientConnection.java b/core/src/main/java/org/apache/brooklyn/util/core/internal/ssh/sshj/SshjClientConnection.java index f4972fb91e..bd5943e4ed 100644 --- a/core/src/main/java/org/apache/brooklyn/util/core/internal/ssh/sshj/SshjClientConnection.java +++ b/core/src/main/java/org/apache/brooklyn/util/core/internal/ssh/sshj/SshjClientConnection.java @@ -29,8 +29,8 @@ import net.schmizz.sshj.userauth.keyprovider.OpenSSHKeyFile; import net.schmizz.sshj.userauth.password.PasswordUtils; +import org.apache.brooklyn.util.JavaGroovyEquivalents; import org.apache.brooklyn.util.core.internal.ssh.SshAbstractTool.SshAction; -import org.apache.brooklyn.util.groovy.GroovyJavaMethods; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -192,14 +192,14 @@ public SSHClient create() throws Exception { } else if (privateKeyData != null) { OpenSSHKeyFile key = new OpenSSHKeyFile(); key.init(privateKeyData, null, - GroovyJavaMethods.truth(privateKeyPassphrase) ? + JavaGroovyEquivalents.groovyTruth(privateKeyPassphrase) ? PasswordUtils.createOneOff(privateKeyPassphrase.toCharArray()) : null); ssh.authPublickey(username, key); } else if (privateKeyFile != null) { OpenSSHKeyFile key = new OpenSSHKeyFile(); key.init(privateKeyFile, - GroovyJavaMethods.truth(privateKeyPassphrase) ? + JavaGroovyEquivalents.groovyTruth(privateKeyPassphrase) ? PasswordUtils.createOneOff(privateKeyPassphrase.toCharArray()) : null); ssh.authPublickey(username, key); diff --git a/core/src/main/java/org/apache/brooklyn/util/core/task/ScheduledTask.java b/core/src/main/java/org/apache/brooklyn/util/core/task/ScheduledTask.java index 219f4f87e9..25f268c473 100644 --- a/core/src/main/java/org/apache/brooklyn/util/core/task/ScheduledTask.java +++ b/core/src/main/java/org/apache/brooklyn/util/core/task/ScheduledTask.java @@ -18,8 +18,8 @@ */ package org.apache.brooklyn.util.core.task; -import static org.apache.brooklyn.util.groovy.GroovyJavaMethods.elvis; -import static org.apache.brooklyn.util.groovy.GroovyJavaMethods.truth; +import static org.apache.brooklyn.util.JavaGroovyEquivalents.elvis; +import static org.apache.brooklyn.util.JavaGroovyEquivalents.groovyTruth; import java.util.Map; import java.util.concurrent.Callable; @@ -98,7 +98,7 @@ public ScheduledTask(Map flags, Callable> taskFactory) { delay = Duration.of(elvis(flags.remove("delay"), 0)); period = Duration.of(elvis(flags.remove("period"), null)); - maxIterations = elvis(flags.remove("maxIterations"), null); + maxIterations = (Integer) elvis(flags.remove("maxIterations"), null); Object cancelFlag = flags.remove("cancelOnException"); cancelOnException = cancelFlag == null || Boolean.TRUE.equals(cancelFlag); } @@ -152,7 +152,7 @@ protected String getActiveTaskStatusString(int verbosity) { Duration start = Duration.sinceUtc(recentRun.getStartTimeUtc()); rv.append(", last run ").append(start).append(" ago"); } - if (truth(getNextScheduled())) { + if (groovyTruth(getNextScheduled())) { Duration untilNext = Duration.millis(getNextScheduled().getDelay(TimeUnit.MILLISECONDS)); if (untilNext.isPositive()) rv.append(", next in ").append(untilNext); @@ -190,7 +190,7 @@ public void blockUntilEnded() { public Object get() throws InterruptedException, ExecutionException { blockUntilStarted(); blockUntilFirstScheduleStarted(); - return (truth(recentRun)) ? recentRun.get() : internalFuture.get(); + return (groovyTruth(recentRun)) ? recentRun.get() : internalFuture.get(); } @Override From 70c1bae18d2a7f5f912b2f63a47f58b07132665c Mon Sep 17 00:00:00 2001 From: Aled Sage Date: Wed, 11 Jan 2017 14:40:05 +0000 Subject: [PATCH 3/4] JavaGroovyEquivalents improvements/tests --- .../brooklyn/util/JavaGroovyEquivalents.java | 7 +- .../util/JavaGroovyEquivalentsTest.java | 83 +++++++++++++++++++ 2 files changed, 89 insertions(+), 1 deletion(-) create mode 100644 utils/common/src/test/java/org/apache/brooklyn/util/JavaGroovyEquivalentsTest.java diff --git a/utils/common/src/main/java/org/apache/brooklyn/util/JavaGroovyEquivalents.java b/utils/common/src/main/java/org/apache/brooklyn/util/JavaGroovyEquivalents.java index 6c25ea76ac..c6b805034b 100644 --- a/utils/common/src/main/java/org/apache/brooklyn/util/JavaGroovyEquivalents.java +++ b/utils/common/src/main/java/org/apache/brooklyn/util/JavaGroovyEquivalents.java @@ -66,8 +66,9 @@ public static String elvis(String preferred, String fallback) { public static String elvisString(Object preferred, Object fallback) { return elvis(asString(preferred), asString(fallback)); } + @SuppressWarnings("unchecked") public static T elvis(T preferred, T fallback) { - return groovyTruth(preferred) ? preferred : fallback; + return (T) fix(groovyTruth(preferred) ? preferred : fallback); } public static T elvis(Iterable preferences) { return elvis(Iterables.toArray(preferences, Object.class)); @@ -117,6 +118,10 @@ public static boolean groovyTruth(Object o) { return ((Iterator)o).hasNext(); } else if (o instanceof Enumeration) { return ((Enumeration)o).hasMoreElements(); + } else if (o instanceof Number) { + return ((Number)o).doubleValue() != 0d; + } else if (o instanceof CharSequence) { + return ((CharSequence)o).length() > 0; } else { return true; } diff --git a/utils/common/src/test/java/org/apache/brooklyn/util/JavaGroovyEquivalentsTest.java b/utils/common/src/test/java/org/apache/brooklyn/util/JavaGroovyEquivalentsTest.java new file mode 100644 index 0000000000..8871cce349 --- /dev/null +++ b/utils/common/src/test/java/org/apache/brooklyn/util/JavaGroovyEquivalentsTest.java @@ -0,0 +1,83 @@ +/* + * 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; + +import static org.apache.brooklyn.util.JavaGroovyEquivalents.elvis; +import static org.apache.brooklyn.util.JavaGroovyEquivalents.groovyTruth; +import static org.testng.Assert.assertEquals; +import static org.testng.Assert.assertFalse; +import static org.testng.Assert.assertTrue; + +import java.util.List; + +import org.codehaus.groovy.runtime.GStringImpl; +import org.testng.annotations.Test; + +import com.google.common.collect.ImmutableList; + +import groovy.lang.GString; + +public class JavaGroovyEquivalentsTest { + + private String gstringVal = "exampleGString"; + private GString gstring = new GStringImpl(new Object[0], new String[] {gstringVal}); + private GString emptyGstring = new GStringImpl(new Object[0], new String[] {""}); + + @Test + public void testTruth() { + assertFalse(groovyTruth((Object)null)); + assertTrue(groovyTruth("someString")); + assertFalse(groovyTruth("")); + assertTrue(groovyTruth(1)); + assertTrue(groovyTruth((byte)1)); + assertTrue(groovyTruth((short)1)); + assertTrue(groovyTruth(1L)); + assertTrue(groovyTruth(1F)); + assertTrue(groovyTruth(1D)); + assertFalse(groovyTruth(0)); + assertFalse(groovyTruth((byte)0)); + assertFalse(groovyTruth((short)0)); + assertFalse(groovyTruth(0L)); + assertFalse(groovyTruth(0F)); + assertFalse(groovyTruth(0D)); + assertTrue(groovyTruth(true)); + assertFalse(groovyTruth(false)); + assertTrue(groovyTruth(gstring)); + assertFalse(groovyTruth(emptyGstring)); + } + + @Test + public void testElvis() { + final List emptyList = ImmutableList.of(); + final List singletonList = ImmutableList.of("myVal"); + final List differentList = ImmutableList.of("differentVal"); + + assertEquals(elvis("", "string2"), "string2"); + assertEquals(elvis("string1", "string2"), "string1"); + assertEquals(elvis(null, "string2"), "string2"); + assertEquals(elvis("", "string2"), "string2"); + assertEquals(elvis(1, 2), Integer.valueOf(1)); + assertEquals(elvis(0, 2), Integer.valueOf(2)); + assertEquals(elvis(singletonList, differentList), singletonList); + assertEquals(elvis(emptyList, differentList), differentList); + assertEquals(elvis(gstring, "other"), gstringVal); + assertEquals(elvis(emptyGstring, "other"), "other"); + assertEquals(elvis(emptyGstring, gstring), gstringVal); + } +} From 9f3f2b5f2d705c374cdba188e7dae5c1f58b5501 Mon Sep 17 00:00:00 2001 From: Aled Sage Date: Wed, 11 Jan 2017 14:40:38 +0000 Subject: [PATCH 4/4] GroovyJavaMethodsTest improvements --- .../util/groovy/GroovJavaMethodsTest.java | 98 ++++++++++++++----- 1 file changed, 71 insertions(+), 27 deletions(-) diff --git a/utils/groovy/src/test/java/org/apache/brooklyn/util/groovy/GroovJavaMethodsTest.java b/utils/groovy/src/test/java/org/apache/brooklyn/util/groovy/GroovJavaMethodsTest.java index f32e92ed85..8db1af7599 100644 --- a/utils/groovy/src/test/java/org/apache/brooklyn/util/groovy/GroovJavaMethodsTest.java +++ b/utils/groovy/src/test/java/org/apache/brooklyn/util/groovy/GroovJavaMethodsTest.java @@ -18,45 +18,77 @@ */ package org.apache.brooklyn.util.groovy; -import groovy.lang.Closure; -import groovy.lang.GString; -import org.testng.annotations.Test; +import static org.apache.brooklyn.util.groovy.GroovyJavaMethods.elvis; +import static org.testng.Assert.assertEquals; +import static org.testng.Assert.assertFalse; +import static org.testng.Assert.assertTrue; +import java.util.List; import java.util.concurrent.Callable; -import static org.apache.brooklyn.util.groovy.GroovyJavaMethods.truth; -import static org.testng.Assert.assertFalse; -import static org.testng.Assert.assertTrue; +import org.codehaus.groovy.runtime.GStringImpl; +import org.testng.annotations.Test; + +import com.google.common.collect.ImmutableList; + +import groovy.lang.Closure; +import groovy.lang.GString; public class GroovJavaMethodsTest { + + private String gstringVal = "exampleGString"; + private GString gstring = new GStringImpl(new Object[0], new String[] {gstringVal}); + private GString emptyGstring = new GStringImpl(new Object[0], new String[] {""}); + + @Test + public void testTruth() throws Throwable { + assertFalse(groovyTruthInvocation(null)); + assertTrue(groovyTruthInvocation("someString")); + assertFalse(groovyTruthInvocation("")); + assertTrue(groovyTruthInvocation(1)); + assertFalse(groovyTruthInvocation(0)); + assertTrue(groovyTruthInvocation(true)); + assertFalse(groovyTruthInvocation(false)); + assertTrue(groovyTruthInvocation(gstring)); + assertFalse(groovyTruthInvocation(emptyGstring)); + } + @Test - public void testTruth() { - assertTrue(truth("someString")); - assertTrue(truth(1)); - assertFalse(truth(false)); - assertFalse(truth(null)); + public void testElvis() { + final List emptyList = ImmutableList.of(); + final List singletonList = ImmutableList.of("myVal"); + final List differentList = ImmutableList.of("differentVal"); + + assertEquals(elvis("", "string2"), "string2"); + assertEquals(elvis("string1", "string2"), "string1"); + assertEquals(elvis(null, "string2"), "string2"); + assertEquals(elvis("", "string2"), "string2"); + assertEquals(elvis(1, 2), 1); + assertEquals(elvis(0, 2), 2); + assertEquals(elvis(singletonList, differentList), singletonList); + assertEquals(elvis(emptyList, differentList), differentList); + assertEquals(elvis(gstring, "other"), gstringVal); + assertEquals(elvis(emptyGstring, "other"), "other"); + assertEquals(elvis(emptyGstring, gstring), gstringVal); } @Test + @SuppressWarnings("serial") public void testIsCase() throws Throwable { - assertFalse(callScriptBytecodeAdapter_isCase( + assertFalse(groovyIsCaseInvocation( null, GString.class)); assertTrue( - callScriptBytecodeAdapter_isCase( - new GString(new String[]{"Hi"}) { - @Override public String[] getStrings() { - return new String[0]; - } - }, + groovyIsCaseInvocation( + gstring, GString.class)); assertFalse( - callScriptBytecodeAdapter_isCase( + groovyIsCaseInvocation( "exampleString", GString.class)); assertTrue( - callScriptBytecodeAdapter_isCase( + groovyIsCaseInvocation( new Callable() { @Override public Void call() { return null; @@ -64,26 +96,38 @@ public void testIsCase() throws Throwable { }, Callable.class)); assertFalse( - callScriptBytecodeAdapter_isCase( + groovyIsCaseInvocation( "exampleString", Callable.class)); assertTrue( - callScriptBytecodeAdapter_isCase( - new Closure(null) { + groovyIsCaseInvocation( + new Closure(null) { @Override public Void call() { return null; } }, Closure.class)); assertFalse( - callScriptBytecodeAdapter_isCase( + groovyIsCaseInvocation( "exampleString", Closure.class)); } - private boolean callScriptBytecodeAdapter_isCase(Object switchValue, Class caseExpression) throws Throwable { -// return org.codehaus.groovy.runtime.ScriptBytecodeAdapter.isCase(switchValue, caseExpression); - return org.apache.brooklyn.util.groovy.GroovyJavaMethods.safeGroovyIsCase(switchValue, caseExpression); + private boolean groovyIsCaseInvocation(Object switchValue, Class caseExpression) throws Throwable { + // We expect this to be equivalent to: + // org.codehaus.groovy.runtime.ScriptBytecodeAdapter.isCase(switchValue, caseExpression); + boolean result = org.apache.brooklyn.util.groovy.GroovyJavaMethods.safeGroovyIsCase(switchValue, caseExpression); + boolean equiv = org.codehaus.groovy.runtime.ScriptBytecodeAdapter.isCase(switchValue, caseExpression); + assertEquals(result, equiv, "switchValue="+switchValue+"; caseExpression="+caseExpression); + return result; + } + + private boolean groovyTruthInvocation(T value) throws Throwable { + // We expect this to be equivalent to Groovy-Truth + boolean result = org.apache.brooklyn.util.JavaGroovyEquivalents.groovyTruth(value); + boolean groovyTruth = org.apache.brooklyn.util.groovy.GroovyJavaMethods.truth(value); + assertEquals(result, groovyTruth, "value="+value); + return result; } }