6763051: MXBean: Incorrect type names for parametrized dealing with arrays (openType)
6713777: developer diagnosability of errors in uncompliant mxbean interfaces
Reviewed-by: dfuchs
--- a/jdk/src/share/classes/com/sun/jmx/mbeanserver/DefaultMXBeanMappingFactory.java Tue Oct 28 16:14:51 2008 +0000
+++ b/jdk/src/share/classes/com/sun/jmx/mbeanserver/DefaultMXBeanMappingFactory.java Tue Oct 28 18:21:36 2008 +0100
@@ -26,7 +26,8 @@
package com.sun.jmx.mbeanserver;
import static com.sun.jmx.mbeanserver.Util.*;
-import java.lang.annotation.ElementType;
+import static com.sun.jmx.mbeanserver.MXBeanIntrospector.typeName;
+
import javax.management.openmbean.MXBeanMappingClass;
import static javax.management.openmbean.SimpleType.*;
@@ -247,8 +248,10 @@
public synchronized MXBeanMapping mappingForType(Type objType,
MXBeanMappingFactory factory)
throws OpenDataException {
- if (inProgress.containsKey(objType))
- throw new OpenDataException("Recursive data structure");
+ if (inProgress.containsKey(objType)) {
+ throw new OpenDataException(
+ "Recursive data structure, including " + typeName(objType));
+ }
MXBeanMapping mapping;
@@ -259,6 +262,8 @@
inProgress.put(objType, objType);
try {
mapping = makeMapping(objType, factory);
+ } catch (OpenDataException e) {
+ throw openDataException("Cannot convert type: " + typeName(objType), e);
} finally {
inProgress.remove(objType);
}
@@ -411,7 +416,7 @@
MXBeanMappingFactory factory)
throws OpenDataException {
- final String objTypeName = objType.toString();
+ final String objTypeName = typeName(objType);
final MXBeanMapping keyMapping = factory.mappingForType(keyType, factory);
final MXBeanMapping valueMapping = factory.mappingForType(valueType, factory);
final OpenType<?> keyOpenType = keyMapping.getOpenType();
@@ -926,6 +931,7 @@
concatenating each Builder's explanation of why it
isn't applicable. */
final StringBuilder whyNots = new StringBuilder();
+ Throwable possibleCause = null;
find:
for (CompositeBuilder[] relatedBuilders : builders) {
for (int i = 0; i < relatedBuilders.length; i++) {
@@ -935,6 +941,9 @@
foundBuilder = builder;
break find;
}
+ Throwable cause = builder.possibleCause();
+ if (cause != null)
+ possibleCause = cause;
if (whyNot.length() > 0) {
if (whyNots.length() > 0)
whyNots.append("; ");
@@ -945,10 +954,12 @@
}
}
if (foundBuilder == null) {
- final String msg =
+ String msg =
"Do not know how to make a " + targetClass.getName() +
" from a CompositeData: " + whyNots;
- throw new InvalidObjectException(msg);
+ if (possibleCause != null)
+ msg += ". Remaining exceptions show a POSSIBLE cause.";
+ throw invalidObjectException(msg, possibleCause);
}
compositeBuilder = foundBuilder;
}
@@ -996,6 +1007,16 @@
abstract String applicable(Method[] getters)
throws InvalidObjectException;
+ /** If the subclass returns an explanation of why it is not applicable,
+ it can additionally indicate an exception with details. This is
+ potentially confusing, because the real problem could be that one
+ of the other subclasses is supposed to be applicable but isn't.
+ But the advantage of less information loss probably outweighs the
+ disadvantage of possible confusion. */
+ Throwable possibleCause() {
+ return null;
+ }
+
abstract Object fromCompositeData(CompositeData cd,
String[] itemNames,
MXBeanMapping[] converters)
@@ -1031,8 +1052,8 @@
if (fromMethod.getReturnType() != getTargetClass()) {
final String msg =
"Method from(CompositeData) returns " +
- fromMethod.getReturnType().getName() +
- " not " + targetClass.getName();
+ typeName(fromMethod.getReturnType()) +
+ " not " + typeName(targetClass);
throw new InvalidObjectException(msg);
}
@@ -1083,6 +1104,7 @@
try {
getterConverters[i].checkReconstructible();
} catch (InvalidObjectException e) {
+ possibleCause = e;
return "method " + getters[i].getName() + " returns type " +
"that cannot be mapped back from OpenData";
}
@@ -1090,6 +1112,11 @@
return "";
}
+ @Override
+ Throwable possibleCause() {
+ return possibleCause;
+ }
+
final Object fromCompositeData(CompositeData cd,
String[] itemNames,
MXBeanMapping[] converters) {
@@ -1097,6 +1124,7 @@
}
private final MXBeanMapping[] getterConverters;
+ private Throwable possibleCause;
}
/** Builder for when the target class has a setter for every getter. */
@@ -1227,10 +1255,16 @@
for (int i = 0; i < propertyNames.length; i++) {
String propertyName = propertyNames[i];
if (!getterMap.containsKey(propertyName)) {
- final String msg =
+ String msg =
"@ConstructorProperties includes name " + propertyName +
- " which does not correspond to a property: " +
- constr;
+ " which does not correspond to a property";
+ for (String getterName : getterMap.keySet()) {
+ if (getterName.equalsIgnoreCase(propertyName)) {
+ msg += " (differs only in case from property " +
+ getterName + ")";
+ }
+ }
+ msg += ": " + constr;
throw new InvalidObjectException(msg);
}
int getterIndex = getterMap.get(propertyName);
--- a/jdk/src/share/classes/com/sun/jmx/mbeanserver/MXBeanIntrospector.java Tue Oct 28 16:14:51 2008 +0000
+++ b/jdk/src/share/classes/com/sun/jmx/mbeanserver/MXBeanIntrospector.java Tue Oct 28 18:21:36 2008 +0100
@@ -391,26 +391,26 @@
if (type instanceof Class<?>)
return ((Class<?>) type).getName();
else
- return genericTypeString(type);
+ return typeName(type);
}
- private static String genericTypeString(Type type) {
+ static String typeName(Type type) {
if (type instanceof Class<?>) {
Class<?> c = (Class<?>) type;
if (c.isArray())
- return genericTypeString(c.getComponentType()) + "[]";
+ return typeName(c.getComponentType()) + "[]";
else
return c.getName();
} else if (type instanceof GenericArrayType) {
GenericArrayType gat = (GenericArrayType) type;
- return genericTypeString(gat.getGenericComponentType()) + "[]";
+ return typeName(gat.getGenericComponentType()) + "[]";
} else if (type instanceof ParameterizedType) {
ParameterizedType pt = (ParameterizedType) type;
StringBuilder sb = new StringBuilder();
- sb.append(genericTypeString(pt.getRawType())).append("<");
+ sb.append(typeName(pt.getRawType())).append("<");
String sep = "";
for (Type t : pt.getActualTypeArguments()) {
- sb.append(sep).append(genericTypeString(t));
+ sb.append(sep).append(typeName(t));
sep = ", ";
}
return sb.append(">").toString();
--- a/jdk/src/share/classes/javax/management/MBeanServerInvocationHandler.java Tue Oct 28 16:14:51 2008 +0000
+++ b/jdk/src/share/classes/javax/management/MBeanServerInvocationHandler.java Tue Oct 28 18:21:36 2008 +0100
@@ -361,7 +361,13 @@
if (p != null)
return p;
}
- p = new MXBeanProxy(mxbeanInterface, mappingFactory);
+ try {
+ p = new MXBeanProxy(mxbeanInterface, mappingFactory);
+ } catch (IllegalArgumentException e) {
+ String msg = "Cannot make MXBean proxy for " +
+ mxbeanInterface.getName() + ": " + e.getMessage();
+ throw new IllegalArgumentException(msg, e.getCause());
+ }
classToProxy.put(mxbeanInterface, new WeakReference<MXBeanProxy>(p));
return p;
}
--- /dev/null Thu Jan 01 00:00:00 1970 +0000
+++ b/jdk/test/javax/management/mxbean/ExceptionDiagnosisTest.java Tue Oct 28 18:21:36 2008 +0100
@@ -0,0 +1,237 @@
+/*
+ * @test
+ * @bug 6713777
+ * @summary Test that exception messages include all relevant information
+ * @author Eamonn McManus
+ */
+
+import java.beans.ConstructorProperties;
+import java.io.File;
+import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
+import java.lang.reflect.Type;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import javax.management.JMX;
+import javax.management.MBeanServer;
+import javax.management.MBeanServerFactory;
+import javax.management.NotCompliantMBeanException;
+import javax.management.ObjectName;
+
+public class ExceptionDiagnosisTest {
+ private static volatile String failure;
+
+ // ------ Illegal MXBeans ------
+
+ // Test that all of BdelloidMXBean, Rotifer, and File appear in the
+ // exception messages. File is not an allowed type because of recursive
+ // getters like "File getParentFile()".
+ public static interface BdelloidMXBean {
+ public Rotifer getRotifer();
+ }
+
+ public static class Bdelloid implements BdelloidMXBean {
+ public Rotifer getRotifer() {
+ return null;
+ }
+ }
+
+ public static class Rotifer {
+ public File getFile() {
+ return null;
+ }
+ }
+
+ // Test that all of IndirectHashMapMXBean, HashMapContainer, and
+ // HashMap<String,String> appear in the exception messages.
+ // HashMap<String,String> is not an allowed type because only the
+ // java.util interface such as Map are allowed with generic parameters,
+ // not their concrete implementations like HashMap.
+ public static interface IndirectHashMapMXBean {
+ public HashMapContainer getContainer();
+ }
+
+ public static class IndirectHashMap implements IndirectHashMapMXBean {
+ public HashMapContainer getContainer() {
+ return null;
+ }
+ }
+
+ public static class HashMapContainer {
+ public HashMap<String, String> getHashMap() {return null;}
+ }
+
+ // ------ MXBeans that are legal but where proxies are not ------
+
+ // Test that all of BlimMXBean, BlimContainer, Blim, and Blam appear
+ // in the exception messages for a proxy for this MXBean. Blam is
+ // legal in MXBeans but is not reconstructible so you cannot make
+ // a proxy for BlimMXBean.
+ public static interface BlimMXBean {
+ public BlimContainer getBlimContainer();
+ }
+
+ public static class BlimImpl implements BlimMXBean {
+ public BlimContainer getBlimContainer() {
+ return null;
+ }
+ }
+
+ public static class BlimContainer {
+ public Blim getBlim() {return null;}
+ public void setBlim(Blim blim) {}
+ }
+
+ public static class Blim {
+ public Blam getBlam() {return null;}
+ public void setBlam(Blam blam) {}
+ }
+
+ public static class Blam {
+ public Blam(int x) {}
+
+ public int getX() {return 0;}
+ }
+
+
+ // ------ Property name differing only in case ------
+
+ public static interface CaseProbMXBean {
+ public CaseProb getCaseProb();
+ }
+
+ public static class CaseProbImpl implements CaseProbMXBean {
+ public CaseProb getCaseProb() {return null;}
+ }
+
+ public static class CaseProb {
+ @ConstructorProperties({"urlPath"})
+ public CaseProb(String urlPath) {}
+
+ public String getURLPath() {return null;}
+ }
+
+
+ public static void main(String[] args) throws Exception {
+ testMXBeans(new Bdelloid(), BdelloidMXBean.class, Rotifer.class, File.class);
+ testMXBeans(new IndirectHashMap(),
+ IndirectHashMapMXBean.class, HashMapContainer.class,
+ HashMapContainer.class.getMethod("getHashMap").getGenericReturnType());
+
+ testProxies(new BlimImpl(), BlimMXBean.class, BlimMXBean.class,
+ BlimContainer.class, Blim.class, Blam.class);
+
+ testCaseProb();
+
+ if (failure == null)
+ System.out.println("TEST PASSED");
+ else
+ throw new Exception("TEST FAILED: " + failure);
+ }
+
+ private static void testMXBeans(Object mbean, Type... expectedTypes)
+ throws Exception {
+ try {
+ MBeanServer mbs = MBeanServerFactory.newMBeanServer();
+ ObjectName name = new ObjectName("a:b=c");
+ mbs.registerMBean(mbean, name);
+ fail("No exception from registerMBean for " + mbean);
+ } catch (NotCompliantMBeanException e) {
+ checkExceptionChain("MBean " + mbean, e, expectedTypes);
+ }
+ }
+
+ private static <T> void testProxies(
+ Object mbean, Class<T> mxbeanClass, Type... expectedTypes)
+ throws Exception {
+ MBeanServer mbs = MBeanServerFactory.newMBeanServer();
+ ObjectName name = new ObjectName("a:b=c");
+ mbs.registerMBean(mbean, name);
+ T proxy = JMX.newMXBeanProxy(mbs, name, mxbeanClass);
+ List<Method> methods = new ArrayList<Method>();
+ for (Method m : mxbeanClass.getMethods()) {
+ if (m.getDeclaringClass() == mxbeanClass)
+ methods.add(m);
+ }
+ if (methods.size() != 1) {
+ fail("TEST BUG: expected to find exactly one method in " +
+ mxbeanClass.getName() + ": " + methods);
+ }
+ Method getter = methods.get(0);
+ try {
+ try {
+ getter.invoke(proxy);
+ fail("No exception from proxy method " + getter.getName() +
+ " in " + mxbeanClass.getName());
+ } catch (InvocationTargetException e) {
+ Throwable cause = e.getCause();
+ if (cause instanceof Exception)
+ throw (Exception) cause;
+ else
+ throw (Error) cause;
+ }
+ } catch (IllegalArgumentException e) {
+ checkExceptionChain(
+ "Proxy for " + mxbeanClass.getName(), e, expectedTypes);
+ }
+ }
+
+ private static void testCaseProb() throws Exception {
+ MBeanServer mbs = MBeanServerFactory.newMBeanServer();
+ ObjectName name = new ObjectName("a:b=c");
+ Object mbean = new CaseProbImpl();
+ mbs.registerMBean(new CaseProbImpl(), name);
+ CaseProbMXBean proxy = JMX.newMXBeanProxy(mbs, name, CaseProbMXBean.class);
+ try {
+ CaseProb prob = proxy.getCaseProb();
+ fail("No exception from proxy method getCaseProb");
+ } catch (IllegalArgumentException e) {
+ String messageChain = messageChain(e);
+ if (messageChain.contains("URLPath")) {
+ System.out.println("Message chain contains URLPath as required: "
+ + messageChain);
+ } else {
+ fail("Exception chain for CaseProb does not mention property" +
+ " URLPath differing only in case");
+ System.out.println("Full stack trace:");
+ e.printStackTrace(System.out);
+ }
+ }
+ }
+
+ private static void checkExceptionChain(
+ String what, Throwable e, Type[] expectedTypes) {
+ System.out.println("Exceptions in chain for " + what + ":");
+ for (Throwable t = e; t != null; t = t.getCause())
+ System.out.println(".." + t);
+
+ String messageChain = messageChain(e);
+
+ // Now check that each of the classes is mentioned in those messages
+ for (Type type : expectedTypes) {
+ String name = (type instanceof Class) ?
+ ((Class<?>) type).getName() : type.toString();
+ if (!messageChain.contains(name)) {
+ fail("Exception chain for " + what + " does not mention " +
+ name);
+ System.out.println("Full stack trace:");
+ e.printStackTrace(System.out);
+ }
+ }
+
+ System.out.println();
+ }
+
+ private static String messageChain(Throwable t) {
+ String msg = "//";
+ for ( ; t != null; t = t.getCause())
+ msg += " " + t.getMessage() + " //";
+ return msg;
+ }
+
+ private static void fail(String why) {
+ failure = why;
+ System.out.println("FAIL: " + why);
+ }
+}
--- a/jdk/test/javax/management/mxbean/TypeNameTest.java Tue Oct 28 16:14:51 2008 +0000
+++ b/jdk/test/javax/management/mxbean/TypeNameTest.java Tue Oct 28 18:21:36 2008 +0100
@@ -40,6 +40,8 @@
import javax.management.MBeanServerFactory;
import javax.management.ObjectName;
import javax.management.StandardMBean;
+import javax.management.openmbean.TabularData;
+import javax.management.openmbean.TabularType;
public class TypeNameTest {
public static interface TestMXBean {
@@ -63,7 +65,7 @@
}
};
- static String failure;
+ static volatile String failure;
public static void main(String[] args) throws Exception {
TestMXBean testImpl = (TestMXBean) Proxy.newProxyInstance(
@@ -74,24 +76,46 @@
mbs.registerMBean(mxbean, name);
MBeanInfo mbi = mbs.getMBeanInfo(name);
MBeanAttributeInfo[] mbais = mbi.getAttributes();
+ boolean sawTabular = false;
for (MBeanAttributeInfo mbai : mbais) {
String attrName = mbai.getName();
String attrTypeName = (String) mbai.getDescriptor().getFieldValue("originalType");
String fieldName = attrName + "Name";
Field nameField = TestMXBean.class.getField(fieldName);
String expectedTypeName = (String) nameField.get(null);
+
if (expectedTypeName.equals(attrTypeName)) {
System.out.println("OK: " + attrName + ": " + attrTypeName);
} else {
- failure = "For attribute " + attrName + " expected type name \"" +
+ fail("For attribute " + attrName + " expected type name \"" +
expectedTypeName + "\", found type name \"" + attrTypeName +
- "\"";
- System.out.println("FAIL: " + failure);
+ "\"");
+ }
+
+ if (mbai.getType().equals(TabularData.class.getName())) {
+ sawTabular = true;
+ TabularType tt = (TabularType) mbai.getDescriptor().getFieldValue("openType");
+ if (tt.getTypeName().equals(attrTypeName)) {
+ System.out.println("OK: TabularType name for " + attrName);
+ } else {
+ fail("For attribute " + attrName + " expected TabularType " +
+ "name \"" + attrTypeName + "\", found \"" +
+ tt.getTypeName());
+ }
}
}
+
+ if (!sawTabular)
+ fail("Test bug: did not test TabularType name");
+
if (failure == null)
System.out.println("TEST PASSED");
else
throw new Exception("TEST FAILED: " + failure);
}
+
+ private static void fail(String why) {
+ System.out.println("FAIL: " + why);
+ failure = why;
+ }
}