Return-Path: Delivered-To: apmail-velocity-commits-archive@locus.apache.org Received: (qmail 1353 invoked from network); 18 May 2007 05:47:45 -0000 Received: from hermes.apache.org (HELO mail.apache.org) (140.211.11.2) by minotaur.apache.org with SMTP; 18 May 2007 05:47:45 -0000 Received: (qmail 98042 invoked by uid 500); 18 May 2007 05:47:51 -0000 Delivered-To: apmail-velocity-commits-archive@velocity.apache.org Received: (qmail 98015 invoked by uid 500); 18 May 2007 05:47:51 -0000 Mailing-List: contact commits-help@velocity.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@velocity.apache.org Delivered-To: mailing list commits@velocity.apache.org Received: (qmail 97990 invoked by uid 99); 18 May 2007 05:47:51 -0000 Received: from herse.apache.org (HELO herse.apache.org) (140.211.11.133) by apache.org (qpsmtpd/0.29) with ESMTP; Thu, 17 May 2007 22:47:51 -0700 X-ASF-Spam-Status: No, hits=-99.5 required=10.0 tests=ALL_TRUSTED,NO_REAL_NAME X-Spam-Check-By: apache.org Received: from [140.211.11.3] (HELO eris.apache.org) (140.211.11.3) by apache.org (qpsmtpd/0.29) with ESMTP; Thu, 17 May 2007 22:47:44 -0700 Received: by eris.apache.org (Postfix, from userid 65534) id 6FA391A981A; Thu, 17 May 2007 22:47:24 -0700 (PDT) Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: svn commit: r539268 - in /velocity/tools/branches/2.x/src/main/java/org/apache/velocity/tools: ToolInfo.java config/ToolConfiguration.java Date: Fri, 18 May 2007 05:47:24 -0000 To: commits@velocity.apache.org From: nbubna@apache.org X-Mailer: svnmailer-1.1.0 Message-Id: <20070518054724.6FA391A981A@eris.apache.org> X-Virus-Checked: Checked by ClamAV on apache.org Author: nbubna Date: Thu May 17 22:47:23 2007 New Revision: 539268 URL: http://svn.apache.org/viewvc?view=rev&rev=539268 Log: refactor and enhance tool validation to catch more problems earlier and with better error reporting Modified: velocity/tools/branches/2.x/src/main/java/org/apache/velocity/tools/ToolInfo.java velocity/tools/branches/2.x/src/main/java/org/apache/velocity/tools/config/ToolConfiguration.java Modified: velocity/tools/branches/2.x/src/main/java/org/apache/velocity/tools/ToolInfo.java URL: http://svn.apache.org/viewvc/velocity/tools/branches/2.x/src/main/java/org/apache/velocity/tools/ToolInfo.java?view=diff&rev=539268&r1=539267&r2=539268 ============================================================================== --- velocity/tools/branches/2.x/src/main/java/org/apache/velocity/tools/ToolInfo.java (original) +++ velocity/tools/branches/2.x/src/main/java/org/apache/velocity/tools/ToolInfo.java Thu May 17 22:47:23 2007 @@ -76,19 +76,17 @@ */ public void setClass(Class clazz) { - try + if (clazz == null) { - // make sure we can get an instance - // make sure we can get an instance - clazz.newInstance(); - - // ok, we'll accept this one - this.clazz = clazz; - } - catch (Throwable t) - { - throw new IllegalArgumentException("Could not create an instance of "+clazz, t); + throw new NullPointerException("Tool class must not be null"); } + this.clazz = clazz; + + //NOTE: we used to check here that we could get an instance of + // the tool class, but that's been moved to ToolConfiguration + // in order to fail as earlier as possible. most people won't + // manually create ToolInfo. if they do and we can't get an + // instance, they should be capable of figuring out what happened // search for a configure(Map params) method in the class try @@ -304,15 +302,13 @@ } catch (IllegalAccessException iae) { - String msg = "Unable to invoke " + - method + " on " + tool; + String msg = "Unable to invoke " + method + " on " + tool; // restricting access to this method by this class ist verboten throw new IllegalStateException(msg, iae); } catch (InvocationTargetException ite) { - String msg = "Exception when invoking " + - method + " on " + tool; + String msg = "Exception when invoking " + method + " on " + tool; // convert to a runtime exception, and re-throw throw new RuntimeException(msg, ite.getCause()); } Modified: velocity/tools/branches/2.x/src/main/java/org/apache/velocity/tools/config/ToolConfiguration.java URL: http://svn.apache.org/viewvc/velocity/tools/branches/2.x/src/main/java/org/apache/velocity/tools/config/ToolConfiguration.java?view=diff&rev=539268&r1=539267&r2=539268 ============================================================================== --- velocity/tools/branches/2.x/src/main/java/org/apache/velocity/tools/config/ToolConfiguration.java (original) +++ velocity/tools/branches/2.x/src/main/java/org/apache/velocity/tools/config/ToolConfiguration.java Thu May 17 22:47:23 2007 @@ -33,9 +33,15 @@ */ public class ToolConfiguration extends Configuration { + private enum Status { + VALID, OLD, NONE, MISSING, UNSUPPORTED, UNINSTANTIABLE; + } + private String key; private String classname; private String restrictTo; + private Status status; + private Throwable problem; public void setKey(String key) { @@ -48,12 +54,13 @@ */ public void setClass(Class clazz) { - this.classname = clazz.getName(); + setClassname(clazz.getName()); } public void setClassname(String classname) { this.classname = classname; + this.status = null; } public void setRestrictTo(String path) @@ -125,45 +132,81 @@ } } - private enum Status { - VALID, OLD, NONE, MISSING, UNSUPPORTED; - } - private final Status getStatus() { - if (getClassname() == null) + if (this.status == null) { - return Status.NONE; - } + if (getClassname() == null) + { + this.status = Status.NONE; + } - // check for mere presence of init() or configure() - try - { - Class clazz = Utils.getClass(getClassname()); + // check for mere presence of init() or configure() + try + { + // make sure the classname resolves to a class we have + Class clazz = Utils.getClass(getClassname()); + + // try hard to ensure we have all necessary supporting classes + digForDependencies(clazz); - Method init = clazz.getMethod("init", new Class[]{ Object.class }); + // create an instance to make sure we can do that + clazz.newInstance(); - // if init is deprecated, then we'll consider it a - // new tool with BC support, not an old tool - Deprecated bc = init.getAnnotation(Deprecated.class); - if (bc == null) + // check for an init method + Method init = + clazz.getMethod("init", new Class[]{ Object.class }); + + // if init is deprecated, then we'll consider it a + // new tool with BC support, not an old tool + Deprecated bc = init.getAnnotation(Deprecated.class); + if (bc == null) + { + this.status = Status.OLD; + this.problem = null; + } + else + { + this.status = Status.VALID; + this.problem = null; + } + } + catch (NoSuchMethodException nsme) { - return Status.OLD; + // ignore this + this.status = Status.VALID; + this.problem = null; + } + catch (ClassNotFoundException cnfe) + { + this.status = Status.MISSING; + this.problem = cnfe; + } + catch (NoClassDefFoundError ncdfe) + { + this.status = Status.UNSUPPORTED; + this.problem = ncdfe; + } + catch (Throwable t) + { + // for all other problems... + this.status = Status.UNINSTANTIABLE; + this.problem = t; } } - catch (NoSuchMethodException nsme) - { - // ignore this - } - catch (ClassNotFoundException cnfe) - { - return Status.MISSING; - } - catch (NoClassDefFoundError ncdfe) + return this.status; + } + + private void digForDependencies(Class clazz) + { + clazz.getDeclaredMethods(); + clazz.getDeclaredFields(); + + Class superClass = clazz.getSuperclass(); + if (superClass != null) { - return Status.UNSUPPORTED; + digForDependencies(superClass); } - return Status.VALID; } public String getRestrictTo() @@ -201,9 +244,14 @@ case NONE: return "No classname set for: "+this; case MISSING: - return "Couldn't find tool class in the classpath for: "+this; + return "Couldn't find tool class in the classpath for: "+this+ + "("+this.problem+")"; case UNSUPPORTED: - return "Couldn't find necessary supporting classes for: "+this; + return "Couldn't find necessary supporting classes for: "+this+ + "("+this.problem+")"; + case UNINSTANTIABLE: + return "Couldn't instantiate instance of tool for: "+this+ + "("+this.problem+")"; default: return ""; } @@ -254,6 +302,10 @@ break; case UNSUPPORTED: out.append("Unsupported "); + break; + case UNINSTANTIABLE: + out.append("Unusable "); + break; } out.append("Tool '"); out.append(getKey());