Issue #3713 - Emit warning when invoking deprecated method in Jetty XML.

Added warnings for invocation of deprecated constructors and methods.
Moved methods exclusively used by XmlConfiguration from TypeUtil.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

jetty-util/src/main/java/org/eclipse/jetty/util/TypeUtil.java

@@ -18,13 +18,10 @@
 
 package org.eclipse.jetty.util;
 
-import java.io.File;
 import java.io.IOException;
-import java.lang.annotation.Annotation;
 import java.lang.reflect.Constructor;
 import java.lang.reflect.InvocationTargetException;
 import java.lang.reflect.Method;
-import java.lang.reflect.Modifier;
 import java.net.URI;
 import java.net.URISyntaxException;
 import java.net.URL;
@@ -35,9 +32,7 @@ import java.util.Collections;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
-import java.util.Objects;
 
-import org.eclipse.jetty.util.annotation.Name;
 import org.eclipse.jetty.util.log.Log;
 import org.eclipse.jetty.util.log.Logger;
 
@@ -501,175 +496,22 @@ public class TypeUtil
         }
     }
 
-
+    @Deprecated
-    public static Object call(Class<?> oClass, String methodName, Object obj, Object[] arg)
+    public static Object call(Class<?> oClass, String methodName, Object obj, Object[] arg) throws InvocationTargetException, NoSuchMethodException
-       throws InvocationTargetException, NoSuchMethodException
     {
-        Objects.requireNonNull(oClass,"Class cannot be null");
+        throw new UnsupportedOperationException();
-        Objects.requireNonNull(methodName,"Method name cannot be null");
-        if (StringUtil.isBlank(methodName))
-        {
-            throw new IllegalArgumentException("Method name cannot be blank");
-        }
-        
-        // Lets just try all methods for now
-        for (Method method : oClass.getMethods())
-        {
-            if (!method.getName().equals(methodName))
-                continue;            
-            if (method.getParameterCount() != arg.length)
-                continue;
-            if (Modifier.isStatic(method.getModifiers()) != (obj == null))
-                continue;
-            if ((obj == null) && method.getDeclaringClass() != oClass)
-                continue;
-
-            try
-            {
-                return method.invoke(obj, arg);
-            }
-            catch (IllegalAccessException | IllegalArgumentException e)
-            {
-                LOG.ignore(e);
-            }
-        }
-        
-        // Lets look for a method with optional arguments
-        Object[] args_with_opts=null;
-        
-        for (Method method : oClass.getMethods())
-        {
-            if (!method.getName().equals(methodName))
-                continue;            
-            if (method.getParameterCount() != arg.length+1)
-                continue;
-            if (!method.getParameterTypes()[arg.length].isArray())
-                continue;
-            if (Modifier.isStatic(method.getModifiers()) != (obj == null))
-                continue;
-            if ((obj == null) && method.getDeclaringClass() != oClass)
-                continue;
-
-            if (args_with_opts==null)
-                args_with_opts=ArrayUtil.addToArray(arg,new Object[]{},Object.class);
-            try
-            {
-                return method.invoke(obj, args_with_opts);
-            }
-            catch (IllegalAccessException | IllegalArgumentException e)
-            {
-                LOG.ignore(e);
-            }
-        }
-        
-        
-        throw new NoSuchMethodException(methodName);
     }
 
+    @Deprecated
     public static Object construct(Class<?> klass, Object[] arguments) throws InvocationTargetException, NoSuchMethodException
     {
-        Objects.requireNonNull(klass,"Class cannot be null");
+        throw new UnsupportedOperationException();
-        
-        for (Constructor<?> constructor : klass.getConstructors())
-        {
-            if (arguments == null)
-            {
-                // null arguments in .newInstance() is allowed
-                if (constructor.getParameterCount() != 0)
-                    continue;
-            }
-            else if (constructor.getParameterCount() != arguments.length)
-                continue;
-
-            try
-            {
-                return constructor.newInstance(arguments);
-            }
-            catch (InstantiationException | IllegalAccessException | IllegalArgumentException e)
-            {
-                LOG.ignore(e);
-            }
-        }
-        throw new NoSuchMethodException("<init>");
     }
 
+    @Deprecated
     public static Object construct(Class<?> klass, Object[] arguments, Map<String, Object> namedArgMap) throws InvocationTargetException, NoSuchMethodException
     {
-        Objects.requireNonNull(klass,"Class cannot be null");
+        throw new UnsupportedOperationException();
-        Objects.requireNonNull(namedArgMap,"Named Argument Map cannot be null");
-        
-        for (Constructor<?> constructor : klass.getConstructors())
-        {
-            if (arguments == null)
-            {
-                // null arguments in .newInstance() is allowed
-                if (constructor.getParameterCount() != 0)
-                    continue;
-            }
-            else if (constructor.getParameterCount() != arguments.length)
-                continue;
-
-            try
-            {
-                Annotation[][] parameterAnnotations = constructor.getParameterAnnotations();
-                
-                if (arguments == null || arguments.length == 0)
-                {
-                    if (LOG.isDebugEnabled())
-                        LOG.debug("Constructor has no arguments");
-                    return constructor.newInstance(arguments);
-                }
-                else if (parameterAnnotations == null || parameterAnnotations.length == 0)
-                {
-                    if (LOG.isDebugEnabled())
-                        LOG.debug("Constructor has no parameter annotations");
-                    return constructor.newInstance(arguments);
-                }
-                else
-                {
-                   Object[] swizzled = new Object[arguments.length];
-                   
-                   int count = 0;
-                   for ( Annotation[] annotations : parameterAnnotations )
-                   {
-                       for ( Annotation annotation : annotations)
-                       {
-                           if ( annotation instanceof Name )
-                           {
-                               Name param = (Name)annotation;
-                               
-                               if (namedArgMap.containsKey(param.value()))
-                               {
-                                   if (LOG.isDebugEnabled())
-                                       LOG.debug("placing named {} in position {}", param.value(), count);
-                                   swizzled[count] = namedArgMap.get(param.value());
-                               }
-                               else
-                               {
-                                   if (LOG.isDebugEnabled())
-                                       LOG.debug("placing {} in position {}", arguments[count], count);
-                                   swizzled[count] = arguments[count];
-                               }
-                               ++count;
-                           }
-                           else
-                           {
-                               if (LOG.isDebugEnabled())
-                                   LOG.debug("passing on annotation {}", annotation);
-                           }
-                       }
-                   }
-                   
-                   return constructor.newInstance(swizzled);
-                }
-                
-            }
-            catch (InstantiationException | IllegalAccessException | IllegalArgumentException e)
-            {
-                LOG.ignore(e);
-            }
-        }
-        throw new NoSuchMethodException("<init>");
     }
 
     /* ------------------------------------------------------------ */

jetty-xml/src/main/java/org/eclipse/jetty/xml/XmlConfiguration.java

@@ -21,6 +21,7 @@ package org.eclipse.jetty.xml;
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.StringReader;
+import java.lang.annotation.Annotation;
 import java.lang.reflect.Array;
 import java.lang.reflect.Constructor;
 import java.lang.reflect.Field;
@@ -44,16 +45,19 @@ import java.util.LinkedList;
 import java.util.List;
 import java.util.Locale;
 import java.util.Map;
+import java.util.Objects;
 import java.util.Properties;
 import java.util.Queue;
 import java.util.ServiceLoader;
 import java.util.Set;
 
+import org.eclipse.jetty.util.ArrayUtil;
 import org.eclipse.jetty.util.LazyList;
 import org.eclipse.jetty.util.Loader;
 import org.eclipse.jetty.util.MultiException;
 import org.eclipse.jetty.util.StringUtil;
 import org.eclipse.jetty.util.TypeUtil;
+import org.eclipse.jetty.util.annotation.Name;
 import org.eclipse.jetty.util.component.LifeCycle;
 import org.eclipse.jetty.util.log.Log;
 import org.eclipse.jetty.util.log.Logger;
@@ -396,10 +400,7 @@ public class XmlConfiguration
 
                 try
                 {
-                    if (namedArgMap.size() > 0)
+                    obj = construct(oClass, arguments.toArray(), namedArgMap);
-                        obj = TypeUtil.construct(oClass, arguments.toArray(), namedArgMap);
-                    else
-                        obj = TypeUtil.construct(oClass, arguments.toArray());
                 }
                 catch (NoSuchMethodException x)
                 {
@@ -546,7 +547,7 @@ public class XmlConfiguration
             try
             {
                 Method set = oClass.getMethod(name, vClass);
-                set.invoke(obj, arg);
+                invokeMethod(set, obj, arg);
                 return;
             }
             catch (IllegalArgumentException | IllegalAccessException | NoSuchMethodException e)
@@ -561,7 +562,7 @@ public class XmlConfiguration
                 Field type = vClass[0].getField("TYPE");
                 vClass[0] = (Class<?>)type.get(null);
                 Method set = oClass.getMethod(name, vClass);
-                set.invoke(obj, arg);
+                invokeMethod(set, obj, arg);
                 return;
             }
             catch (NoSuchFieldException | IllegalArgumentException | IllegalAccessException | NoSuchMethodException e)
@@ -602,7 +603,7 @@ public class XmlConfiguration
                     try
                     {
                         set = setter;
-                        setter.invoke(obj, arg);
+                        invokeMethod(set, obj, arg);
                         return;
                     }
                     catch (IllegalArgumentException | IllegalAccessException e)
@@ -617,7 +618,7 @@ public class XmlConfiguration
                         {
                             if (paramTypes[0].isAssignableFrom(c))
                             {
-                                setter.invoke(obj, convertArrayToCollection(value, c));
+                                invokeMethod(setter, obj, convertArrayToCollection(value, c));
                                 return;
                             }
                         }
@@ -650,7 +651,7 @@ public class XmlConfiguration
                     Constructor<?> cons = sClass.getConstructor(vClass);
                     arg[0] = cons.newInstance(arg);
                     _configuration.initializeDefaults(arg[0]);
-                    set.invoke(obj, arg);
+                    invokeMethod(set, obj, arg);
                     return;
                 }
                 catch (NoSuchMethodException | IllegalAccessException | InstantiationException e)
@@ -672,6 +673,22 @@ public class XmlConfiguration
             throw failure;
         }
 
+        private Object invokeConstructor(Constructor<?> constructor, Object... args) throws IllegalAccessException, InvocationTargetException, InstantiationException
+        {
+            Object result = constructor.newInstance(args);
+            if (constructor.getAnnotation(Deprecated.class) != null)
+                LOG.warn("Deprecated {} in {}", constructor, _url);
+            return result;
+        }
+
+        private Object invokeMethod(Method method, Object obj, Object... args) throws IllegalAccessException, InvocationTargetException
+        {
+            Object result = method.invoke(obj, args);
+            if (method.getAnnotation(Deprecated.class) != null)
+                LOG.warn("Deprecated {} in {}", method, _url);
+            return result;
+        }
+
         /**
          * @param array the array to convert
          * @param collectionType the desired collection type
@@ -758,7 +775,7 @@ public class XmlConfiguration
                 {
                     // Try calling a getXxx method.
                     Method method = oClass.getMethod("get" + name.substring(0, 1).toUpperCase(Locale.ENGLISH) + name.substring(1));
-                    obj = method.invoke(obj);
+                    obj = invokeMethod(method, obj);
                 }
                 if (id != null)
                     _configuration.getIdMap().put(id, obj);
@@ -819,7 +836,7 @@ public class XmlConfiguration
 
             try
             {
-                Object nobj = TypeUtil.call(oClass, name, obj, args.toArray(new Object[0]));
+                Object nobj = call(oClass, name, obj, args.toArray(new Object[0]));
                 if (id != null)
                     _configuration.getIdMap().put(id, nobj);
                 configure(nobj, node, aoeNode.getNext());
@@ -831,6 +848,65 @@ public class XmlConfiguration
             }
         }
 
+        private Object call(Class<?> oClass, String methodName, Object obj, Object[] arg) throws InvocationTargetException, NoSuchMethodException
+        {
+            Objects.requireNonNull(oClass, "Class cannot be null");
+            Objects.requireNonNull(methodName, "Method name cannot be null");
+            if (StringUtil.isBlank(methodName))
+                throw new IllegalArgumentException("Method name cannot be blank");
+
+            // Lets just try all methods for now
+            for (Method method : oClass.getMethods())
+            {
+                if (!method.getName().equals(methodName))
+                    continue;
+                if (method.getParameterCount() != arg.length)
+                    continue;
+                if (Modifier.isStatic(method.getModifiers()) != (obj == null))
+                    continue;
+                if ((obj == null) && method.getDeclaringClass() != oClass)
+                    continue;
+
+                try
+                {
+                    return invokeMethod(method, obj, arg);
+                }
+                catch (IllegalAccessException | IllegalArgumentException e)
+                {
+                    LOG.ignore(e);
+                }
+            }
+
+            // Lets look for a method with varargs arguments
+            Object[] argsWithVarargs = null;
+            for (Method method : oClass.getMethods())
+            {
+                if (!method.getName().equals(methodName))
+                    continue;
+                if (method.getParameterCount() != arg.length + 1)
+                    continue;
+                if (!method.getParameterTypes()[arg.length].isArray())
+                    continue;
+                if (Modifier.isStatic(method.getModifiers()) != (obj == null))
+                    continue;
+                if ((obj == null) && method.getDeclaringClass() != oClass)
+                    continue;
+
+                if (argsWithVarargs == null)
+                    argsWithVarargs = ArrayUtil.addToArray(arg, new Object[0], Object.class);
+                try
+                {
+                    invokeMethod(method, obj, argsWithVarargs);
+                }
+                catch (IllegalAccessException | IllegalArgumentException e)
+                {
+                    LOG.ignore(e);
+                }
+            }
+
+            throw new NoSuchMethodException(methodName);
+        }
+
         /**
          * <p>Creates a new value object.</p>
          *
@@ -869,16 +945,7 @@ public class XmlConfiguration
             Object nobj;
             try
             {
-                if (namedArgMap.size() > 0)
+                nobj = construct(oClass, arguments.toArray(), namedArgMap);
-                {
-                    LOG.debug("using named mapping");
-                    nobj = TypeUtil.construct(oClass, arguments.toArray(), namedArgMap);
-                }
-                else
-                {
-                    LOG.debug("using normal mapping");
-                    nobj = TypeUtil.construct(oClass, arguments.toArray());
-                }
             }
             catch (NoSuchMethodException e)
             {
@@ -893,6 +960,87 @@ public class XmlConfiguration
             return nobj;
         }
 
+        private Object construct(Class<?> klass, Object[] arguments, Map<String, Object> namedArgMap) throws InvocationTargetException, NoSuchMethodException
+        {
+            Objects.requireNonNull(klass, "Class cannot be null");
+            Objects.requireNonNull(namedArgMap, "Named Argument Map cannot be null");
+
+            for (Constructor<?> constructor : klass.getConstructors())
+            {
+                if (arguments == null)
+                {
+                    // null arguments in .newInstance() is allowed
+                    if (constructor.getParameterCount() != 0)
+                        continue;
+                }
+                else if (constructor.getParameterCount() != arguments.length)
+                    continue;
+
+                try
+                {
+                    if (arguments == null || arguments.length == 0)
+                    {
+                        if (LOG.isDebugEnabled())
+                            LOG.debug("Invoking constructor, no arguments");
+                        return invokeConstructor(constructor);
+                    }
+
+                    if (namedArgMap.isEmpty())
+                    {
+                        if (LOG.isDebugEnabled())
+                            LOG.debug("Invoking constructor, no XML parameter mapping");
+                        return invokeConstructor(constructor, arguments);
+                    }
+
+                    Annotation[][] parameterAnnotations = constructor.getParameterAnnotations();
+                    if (parameterAnnotations == null || parameterAnnotations.length == 0)
+                    {
+                        if (LOG.isDebugEnabled())
+                            LOG.debug("Invoking constructor, no parameter annotations");
+                        return invokeConstructor(constructor, arguments);
+                    }
+
+                    int count = 0;
+                    Object[] swizzled = new Object[arguments.length];
+                    for (Annotation[] annotations : parameterAnnotations)
+                    {
+                        for (Annotation annotation : annotations)
+                        {
+                            if (annotation instanceof Name)
+                            {
+                                Name param = (Name)annotation;
+                                if (namedArgMap.containsKey(param.value()))
+                                {
+                                    if (LOG.isDebugEnabled())
+                                        LOG.debug("Mapping named parameter {} in position {}", param.value(), count);
+                                    swizzled[count] = namedArgMap.get(param.value());
+                                }
+                                else
+                                {
+                                    if (LOG.isDebugEnabled())
+                                        LOG.debug("Mapping argument {} in position {}", arguments[count], count);
+                                    swizzled[count] = arguments[count];
+                                }
+                                ++count;
+                            }
+                            else
+                            {
+                                if (LOG.isDebugEnabled())
+                                    LOG.debug("Skipping parameter annotated with {}", annotation);
+                            }
+                        }
+                    }
+
+                    return invokeConstructor(constructor, swizzled);
+                }
+                catch (InstantiationException | IllegalAccessException | IllegalArgumentException e)
+                {
+                    LOG.ignore(e);
+                }
+            }
+            throw new NoSuchMethodException("<init>");
+        }
+
         /**
          * <p>Returns a reference object mapped to an id.</p>
          *

jetty-xml/src/test/java/org/eclipse/jetty/xml/AnnotatedTestConfiguration.java

@@ -25,8 +25,14 @@ public class AnnotatedTestConfiguration
     private String first;
     private String second;
     private String third;
+    private String deprecated;
+    private AnnotatedTestConfiguration nested;
 
-    AnnotatedTestConfiguration nested;
+    // Do not remove deprecation, used in tests.
+    @Deprecated
+    public AnnotatedTestConfiguration()
+    {
+    }
 
     public AnnotatedTestConfiguration(@Name("first") String first, @Name("second") String second, @Name("third") String third)
     {
@@ -75,4 +81,17 @@ public class AnnotatedTestConfiguration
         this.nested = nested;
     }
 
+    // Do not remove deprecation, used in tests.
+    @Deprecated
+    public void setDeprecated(String value)
+    {
+        this.deprecated = value;
+    }
+
+    // Do not remove deprecation, used in tests.
+    @Deprecated
+    public String getDeprecated()
+    {
+        return deprecated;
+    }
 }

jetty-xml/src/test/java/org/eclipse/jetty/xml/XmlConfigurationTest.java

@@ -981,4 +981,17 @@ public class XmlConfigurationTest
             assertThat(propName, tc.getTestString(), startsWith("file:"));
         }
     }
+
+    @Test
+    public void testDeprecated() throws Exception
+    {
+        XmlConfiguration xmlConfiguration = new XmlConfiguration("" +
+            "<Configure class=\"org.eclipse.jetty.xml.AnnotatedTestConfiguration\">" +
+            "  <Set name=\"deprecated\">foo</Set>" +
+            "  <Call name=\"setDeprecated\"><Arg><Get name=\"deprecated\" /></Arg></Call>" +
+            "</Configure>");
+
+        xmlConfiguration.configure();
+        // Cannot test that the warnings are logged.
+    }
 }