From 540b83b6b19d1615a1a56503aec3707104cb5b37 Mon Sep 17 00:00:00 2001 From: Eamonn McManus Date: Tue, 28 Oct 2008 18:21:36 +0100 Subject: [PATCH] 6763051: MXBean: Incorrect type names for parametrized dealing with arrays (openType) 6713777: developer diagnosability of errors in uncompliant mxbean interfaces Reviewed-by: dfuchs --- .../DefaultMXBeanMappingFactory.java | 56 ++++- .../jmx/mbeanserver/MXBeanIntrospector.java | 12 +- .../MBeanServerInvocationHandler.java | 8 +- .../mxbean/ExceptionDiagnosisTest.java | 237 ++++++++++++++++++ .../javax/management/mxbean/TypeNameTest.java | 32 ++- 5 files changed, 323 insertions(+), 22 deletions(-) create mode 100644 jdk/test/javax/management/mxbean/ExceptionDiagnosisTest.java diff --git a/jdk/src/share/classes/com/sun/jmx/mbeanserver/DefaultMXBeanMappingFactory.java b/jdk/src/share/classes/com/sun/jmx/mbeanserver/DefaultMXBeanMappingFactory.java index d7ca5b0f629..1b653c28eaf 100644 --- a/jdk/src/share/classes/com/sun/jmx/mbeanserver/DefaultMXBeanMappingFactory.java +++ b/jdk/src/share/classes/com/sun/jmx/mbeanserver/DefaultMXBeanMappingFactory.java @@ -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 class DefaultMXBeanMappingFactory extends MXBeanMappingFactory { 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 @@ public class DefaultMXBeanMappingFactory extends MXBeanMappingFactory { 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 @@ public class DefaultMXBeanMappingFactory extends MXBeanMappingFactory { 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 @@ public class DefaultMXBeanMappingFactory extends MXBeanMappingFactory { 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 @@ public class DefaultMXBeanMappingFactory extends MXBeanMappingFactory { 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 @@ public class DefaultMXBeanMappingFactory extends MXBeanMappingFactory { } } 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 @@ public class DefaultMXBeanMappingFactory extends MXBeanMappingFactory { 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 @@ public class DefaultMXBeanMappingFactory extends MXBeanMappingFactory { 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 @@ public class DefaultMXBeanMappingFactory extends MXBeanMappingFactory { 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 @@ public class DefaultMXBeanMappingFactory extends MXBeanMappingFactory { return ""; } + @Override + Throwable possibleCause() { + return possibleCause; + } + final Object fromCompositeData(CompositeData cd, String[] itemNames, MXBeanMapping[] converters) { @@ -1097,6 +1124,7 @@ public class DefaultMXBeanMappingFactory extends MXBeanMappingFactory { } private final MXBeanMapping[] getterConverters; + private Throwable possibleCause; } /** Builder for when the target class has a setter for every getter. */ @@ -1227,10 +1255,16 @@ public class DefaultMXBeanMappingFactory extends MXBeanMappingFactory { 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); diff --git a/jdk/src/share/classes/com/sun/jmx/mbeanserver/MXBeanIntrospector.java b/jdk/src/share/classes/com/sun/jmx/mbeanserver/MXBeanIntrospector.java index bcb6f869c20..af5420c38f3 100644 --- a/jdk/src/share/classes/com/sun/jmx/mbeanserver/MXBeanIntrospector.java +++ b/jdk/src/share/classes/com/sun/jmx/mbeanserver/MXBeanIntrospector.java @@ -391,26 +391,26 @@ class MXBeanIntrospector extends MBeanIntrospector { 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(); diff --git a/jdk/src/share/classes/javax/management/MBeanServerInvocationHandler.java b/jdk/src/share/classes/javax/management/MBeanServerInvocationHandler.java index 936b03ce489..20c934f15c6 100644 --- a/jdk/src/share/classes/javax/management/MBeanServerInvocationHandler.java +++ b/jdk/src/share/classes/javax/management/MBeanServerInvocationHandler.java @@ -361,7 +361,13 @@ public class MBeanServerInvocationHandler implements InvocationHandler { 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(p)); return p; } diff --git a/jdk/test/javax/management/mxbean/ExceptionDiagnosisTest.java b/jdk/test/javax/management/mxbean/ExceptionDiagnosisTest.java new file mode 100644 index 00000000000..d44e289efd8 --- /dev/null +++ b/jdk/test/javax/management/mxbean/ExceptionDiagnosisTest.java @@ -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 appear in the exception messages. + // HashMap 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 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 void testProxies( + Object mbean, Class 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 methods = new ArrayList(); + 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); + } +} diff --git a/jdk/test/javax/management/mxbean/TypeNameTest.java b/jdk/test/javax/management/mxbean/TypeNameTest.java index f5f48bccacb..ede7fdc69bf 100644 --- a/jdk/test/javax/management/mxbean/TypeNameTest.java +++ b/jdk/test/javax/management/mxbean/TypeNameTest.java @@ -40,6 +40,8 @@ import javax.management.MBeanServer; 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 @@ public class TypeNameTest { } }; - static String failure; + static volatile String failure; public static void main(String[] args) throws Exception { TestMXBean testImpl = (TestMXBean) Proxy.newProxyInstance( @@ -74,24 +76,46 @@ public class TypeNameTest { 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; + } }