6763051: MXBean: Incorrect type names for parametrized dealing with arrays (openType)

6713777: developer diagnosability of errors in uncompliant mxbean interfaces
Reviewed-by: dfuchs

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);

src/share/classes/com/sun/jmx/mbeanserver/MXBeanIntrospector.java

@@ -391,26 +391,26 @@ class MXBeanIntrospector extends MBeanIntrospector<ConvertingMethod> {
         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();

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<MXBeanProxy>(p));
             return p;
         }

test/javax/management/mxbean/ExceptionDiagnosisTest.java

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

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;
+    }
 }