webwork/src/main/webwork/util ValueStack.java,1.58,1.59

[email protected] Mon, 31 May 2004 16:14:22 -0700
Newsgroups gmane.comp.java.open-symphony.cvs
Message-ID <[email protected]>
Update of /cvsroot/opensymphony/webwork/src/main/webwork/util
In directory sc8-pr-cvs1.sourceforge.net:/tmp/cvs-serv26496

Modified Files:
	ValueStack.java 
Log Message:
Fixed bug where stack continued searching when it found a null value
(issue WW-382 and WW-464).
Improved performance when accessing collections data.
Fixed bug where primitive arrays could not be accessed (issue WW-542).
Fixed bug where stack would try to call a method even though the
number of parameters did not even match.


Index: ValueStack.java
===================================================================
RCS file: /cvsroot/opensymphony/webwork/src/main/webwork/util/ValueStack.java,v
retrieving revision 1.58
retrieving revision 1.59
diff -u -d -r1.58 -r1.59
--- ValueStack.java	14 Apr 2004 00:13:03 -0000	1.58
+++ ValueStack.java	31 May 2004 23:14:19 -0000	1.59
@@ -14,6 +14,7 @@
 import java.lang.reflect.AccessibleObject;
 import java.lang.reflect.Method;
 import java.lang.reflect.Modifier;
+import java.lang.reflect.Array;
 import java.util.*;
 
 import org.apache.commons.logging.Log;
@@ -67,9 +68,14 @@
 
    /**
     * Peek at the object that is at the top of the stack.
+    * The object is returned unmodified, so if it would
+    * be a ValueHolder it is not unwrapped
     */
    public Object peek() {
-      return valueList.get(valueList.size()-1);
+      int size = valueList.size();
+      if (size < 1)
+         return null;
+      return valueList.get(size-1);
    }
 
    /**
@@ -78,18 +84,11 @@
     * @return  the popped value
     */
    public Object popValue() {
-      if (valueList == Collections.EMPTY_LIST)
-          return null;
 
-     try {
-         return valueList.remove(valueList.size() - 1);
-      } catch (IndexOutOfBoundsException e) {
+      int size = valueList.size();
+      if (size < 1)
          return null;
-      } finally {
-//         log.debug("Pop from value stack:");
-//         log.debug(toString());
-//         new Throwable().printStackTrace();
-      }
+      return valueList.remove(size-1);
    }
 
    public Iterator iterator() {
@@ -146,7 +145,7 @@
       return answer;
    }
 
-    /** 
+    /**
      * Find a value by id. This method can be overridden by subclasses to
      * have some context concept to evaluate @identified expressions.
      */
@@ -154,6 +153,12 @@
     	return null;
     }
 
+    protected Object unwrap(Object value) {
+       if (value instanceof ValueHolder)
+          return ((ValueHolder) value).getValue();
+       return value;
+    }
+
    /**
     * Find a value for a given name.
     *
@@ -186,11 +191,12 @@
 
       // The current stack pointer, and the current object,
       int stackIdx = 0;
+      int size;
       Object value;
-      
+
       /////////////////////////////////////////////////////////////////////////
       // evaluate the first element of the expression to see where to
-      // get the requested value.  These should be quick and easy objects
+      // get the requested value. These should be quick and easy objects
       // to find or create.
       /////////////////////////////////////////////////////////////////////////
       switch (segment.getType()) {
@@ -198,19 +204,18 @@
          // get the top value off of the stack
          case QuerySegment.CURRENT:
 
-            if (valueList.size() < 1)
+            size = valueList.size();
+            if (size < 1)
                return null;
             // set up the stack, pointer, and current value
-            value = valueList.get(valueList.size()-1);
-            if (value instanceof ValueHolder)
-               value = ((ValueHolder) value).getValue();
+            value = valueList.get(size-1);
 
             // always have the next segment ready to go
             segment = segments[segmentIdx++];
 
             // if we don't need to get other values then return the value
             if (segment == null)
-               return value;
+               return unwrap(value);
 
             // The stackIdx is already zero so we will not try to search the stack
             break;
@@ -227,9 +232,7 @@
          case QuerySegment.ATTRIBUTE:
 
             // get the attribute
-	     // SK
-	     value=findInContext(segment.getId());
-	     // /SK
+            value=findInContext(segment.getId());
 
             if (value == null) {
 //               if (log.isDebugEnabled()) {
@@ -240,16 +243,13 @@
 
                //throw new IllegalArgumentException("No such attribute: " + token.image);
             }
-            // get the real value
-            if (value instanceof ValueHolder)
-               value = ((ValueHolder) value).getValue();
 
             // always have the next segment ready to go
             segment = segments[segmentIdx++];
 
             // if we don't need to search through this attribute simply return it
             if (segment == null)
-               return value;
+               return unwrap(value);
 
             // The stackIdx is already zero so we will not try to search the stack
 
@@ -257,9 +257,8 @@
 
             // return the http request parameter
          case QuerySegment.PARAMETER:
-	     // SK
             return getParameter(segment.getId());
-	     // /SK
+
             // the reserved keyword "true"
          case QuerySegment.TRUE:
             return Boolean.TRUE;
@@ -299,50 +298,42 @@
             // if we don't need to search through the stack then return the value
             // this is very unlikely to happen, but we have account for it anyway
             if (segment == null)
-               return value;
+               return unwrap(value);
 
             break;
 
          default:
-
-            if (valueList.size() < 1)
+            size = valueList.size();
+            if (size < 1)
                return null;
-
             // set up the stack pointer, and current value
-            stackIdx = valueList.size() - 1;
-
-            try {
-               value = valueList.get(stackIdx);
-            } catch (IndexOutOfBoundsException e) {
-               return null;
-               //throw new IllegalArgumentException("Illegal valuestack query:"+q);
-            }
-
+            stackIdx = size - 1;
+            value = valueList.get(stackIdx);
             break;
       }
 
       //log.debug( "first segment id: '" + segment.getId() + "' segment type: '" + segment.getType() + "'" );
 
       /////////////////////////////////////////////////////////////////////////
-      // Now that stack has been set up and the context set (valueIdx) we will
+      // Now that stack has been set up and the context set (stackIdx) we will
       // begin parsing the rest of the expression and drilling down through
       // the object properties, collection elements, and method calls.
       /////////////////////////////////////////////////////////////////////////
       int saveSegmentIdx = segmentIdx;
-
+      boolean gotResult = false;
       while (true) {
          //log.debug("beginning valuestack search at level: '" + stackIdx);
 
          int workStackIdx = stackIdx;
          if (value != null) {
             objectWalk: do {
+               gotResult = false;
                switch (segment.getType()) {
 
                   // access a classes property
                   case QuerySegment.PROPERTY:
                      // get the real value
-                     if (value instanceof ValueHolder)
-                        value = ((ValueHolder) value).getValue();
+                     value = unwrap(value);
                      // If the value is null we break out now since it
                      // would only result in an exception anyway
                      if (value == null)
@@ -353,15 +344,14 @@
                         MethodInfo[] methods = getMethod(value.getClass(), segment.getId());
                         if (methods == null) {
                            //log.debug( "PROPERTY: method not found: '" + segment.getId() + "' current value: '" + value + "' (" + value.getClass() + ")");
-                           value = null;
                            break objectWalk;
                         } else {
                            value = methods[0].getMethod().invoke(value, null);
+                           gotResult = true;
                            //log.debug( "PROPERTY: found property value: " + value  + " (" + value.getClass() + ")");
                         }
                      } catch (Exception e) {
                         //log.debug( "PROPERTY: method called failed: " + e.getMessage() );
-                        value = null;
                         break objectWalk;
                      }
                      break;
@@ -370,8 +360,7 @@
                   case QuerySegment.METHOD:
 
                      // get the real value
-                     if (value instanceof ValueHolder)
-                        value = ((ValueHolder) value).getValue();
+                     value = unwrap(value);
                      // If the value is null we break out now since it
                      // would only result in an exception anyway
                      if (value == null)
@@ -387,7 +376,6 @@
 
                         if (methods == null) {
                            //log.debug( "METHOD: "  + segment.getId() + " was not found." );
-                           value = null;
                            break objectWalk;
                         }
 
@@ -404,7 +392,6 @@
                         if(target==null)
                         {
                           log.error("No method found for " + segment.getId() + " with parameters " + Arrays.asList(getParameterClasses(params)) + " in class " + value.getClass());
-                          value = null;
                           break objectWalk;
                         }
                         // Convert if necessary
@@ -424,7 +411,6 @@
                                  // If the parameterClass is primitive then null is not an acceptable argument
                                  if (parameterClass.isPrimitive())
                                  {
-                                    value = null;
                                     break objectWalk;
                                  }
                               }
@@ -441,13 +427,13 @@
                            }
                         }
                         value = target.getMethod().invoke(value, params);
+                        gotResult = true;
                      } catch (IllegalArgumentException e) {
                         log.error("Illegal parameters invoking " + value.getClass() + "." + target.getMethod().getName() + "(" +Arrays.asList(getParameterClasses(params)) + ")");
+                        break objectWalk;
                      } catch (Exception e) {
                         log.error("METHOD: \"" + segment.getId() + "\", exception: ", e);
-                        value = null;
                         break objectWalk;
-
                         //throw new IllegalArgumentException("Invalid single method access. " +
                         //                                   "Error accessing method \"" + token.image +
                         //                                   "\" using parameter: \"" + param + "\"");
@@ -456,10 +442,8 @@
 
                      // access the current value as a collection
                   case QuerySegment.COLLECTION:
-
                      // get the real value
-                     if (value instanceof ValueHolder)
-                        value = ((ValueHolder) value).getValue();
+                     value = unwrap(value);
                      // If the value is null we break out now since it
                      // would result in an uncaught exception otherwise
                      if (value == null)
@@ -467,69 +451,67 @@
 
                      Object key = findValue(segment.getQuery());
                      if (key == null) {
-                        value = null;
                         break objectWalk;
                      }
 
                      // Map
-                     if (Map.class.isAssignableFrom(value.getClass())) {
+                     if (value instanceof Map) {
                         value = ((Map) value).get(key);
+                        gotResult = true;
                         break;
                      }
-                     // Resource Bundle
-                     else if (ResourceBundle.class.isAssignableFrom(value.getClass())) {
-                        value = ((ResourceBundle) value).getObject(key.toString());
+                     // List
+                     else if (value instanceof List) {
+                        value = ((List) value).get(((Integer) key).intValue());
+                        gotResult = true;
                         break;
                      }
                      // Array
                      else if (value.getClass().isArray()) {
-                        value = ((Object[]) value)[((Integer) key).intValue()];
+                        value = Array.get(value,((Integer) key).intValue());
+                        gotResult = true;
                         break;
                      }
-                     // List
-                     else if (List.class.isAssignableFrom(value.getClass())) {
-                        value = ((List) value).get(((Integer) key).intValue());
+                     // Resource Bundle
+                     else if (value instanceof ResourceBundle) {
+                        value = ((ResourceBundle) value).getObject(key.toString());
+                        gotResult = true;
                         break;
                      }
                      // Collection
-                     else if (Collection.class.isAssignableFrom(value.getClass())) {
+                     else if (value instanceof Collection) {
                         // Not very efficient, but at least it works
                         value = ((Collection) value).toArray()[((Integer) key).intValue()];
+                        gotResult = true;
                         break;
                      }
 
                      // fail if the user tries to access something other than a Collection
-                     value = null;
                      break objectWalk;
 
                      // access the parent by going up one level on the stack
                   case QuerySegment.PARENT:
                      workStackIdx--;
-
                      if (workStackIdx < 0) {
-                        value = null;
                         break objectWalk;
                         //throw new IllegalArgumentException("Parent object not available.");
                      } else {
                         value = valueList.get(workStackIdx);
+                        gotResult = true;
                      }
                      break;
 
-
                   case QuerySegment.EXPAND:
                      //log.debug( "EXPAND: going after: " + segment.getId() );
                      try {
 
                         Object methodName = findValue(segment.getQuery());
                         if (methodName == null) {
-                           value = null;
                            break objectWalk;
                         }
-
                         value = findValue(methodName.toString());
-
+                        gotResult = true;
                      } catch (Exception e) {
-                        value = null;
                         break objectWalk;
                      }
                      break;
@@ -542,7 +524,7 @@
 
          // if we didn't find the value, then move one down the stack and
          // try again.
-         if (value == null && stackIdx > 0) {
+         if (!gotResult && stackIdx > 0) {
             stackIdx--;
             value = valueList.get(stackIdx);
 
@@ -554,13 +536,14 @@
          }
       }
 
-      // get the real value
-      if (value instanceof ValueHolder)
-         value = ((ValueHolder) value).getValue();
-
+      // If no result was found then return null
+      if (!gotResult)
+         return null;
 //      if (value == null && log.isDebugEnabled())
 //         log.debug("value for [" + q + "] is null.");
-      return value;
+
+      // get the real value
+      return unwrap(value);
    }
 
   private Object[] getParameterClasses(Object[] params) {
@@ -714,7 +697,11 @@
 
    protected MethodInfo findMethod(MethodInfo[] m, Object[] params) {
       if (m.length == 1)
-         return m[0];
+      {
+         // Check if the method with the matching name also expects
+         // the same number of parameters
+         return (m[0].getNrOfParameters() == params.length ? m[0] : null);
+      }
 
       MethodInfo oneMatch = null;
       List match = null;



-------------------------------------------------------
This SF.Net email is sponsored by: Oracle 10g
Get certified on the hottest thing ever to hit the market... Oracle 10g. 
Take an Oracle 10g class now, and we'll give you the exam FREE.
http://ads.osdn.com/?ad_id=3149&alloc_id=8166&op=click