Patch from Josh from bug #44609 - Handle leading spaces in formulas, such as '= 4'

git-svn-id: https://svn.apache.org/repos/asf/poi/trunk@637601 13f79535-47bb-0310-9956-ffa450edef68

src/documentation/content/xdocs/changes.xml

@@ -36,6 +36,7 @@
 
 		<!-- Don't forget to update status.xml too! -->
         <release version="3.1-beta1" date="2008-??-??">
+           <action dev="POI-DEVELOPERS" type="fix">44609 - Handle leading spaces in formulas, such as '= 4'</action>
            <action dev="POI-DEVELOPERS" type="add">44608 - Support for PercentPtg in the formula evaluator</action>
            <action dev="POI-DEVELOPERS" type="fix">44606 - Support calculated string values for evaluated formulas</action>
            <action dev="POI-DEVELOPERS" type="add">Add accessors to horizontal and vertical alignment in HSSFTextbox</action>

src/documentation/content/xdocs/status.xml

@@ -33,6 +33,7 @@
 	<!-- Don't forget to update changes.xml too! -->
     <changes>
         <release version="3.1-beta1" date="2008-??-??">
+           <action dev="POI-DEVELOPERS" type="fix">44609 - Handle leading spaces in formulas, such as '= 4'</action>
            <action dev="POI-DEVELOPERS" type="add">44608 - Support for PercentPtg in the formula evaluator</action>
            <action dev="POI-DEVELOPERS" type="fix">44606 - Support calculated string values for evaluated formulas</action>
            <action dev="POI-DEVELOPERS" type="add">Add accessors to horizontal and vertical alignment in HSSFTextbox</action>

src/java/org/apache/poi/hssf/model/FormulaParser.java

@@ -943,23 +943,7 @@ end;
         }
         Stack stack = new Stack();
 
-           // Excel allows to have AttrPtg at position 0 (such as Blanks) which
+        for (int i=0 ; i < ptgs.length; i++) {
-           // do not have any operands. Skip them.
-        int i;
-        if(ptgs[0] instanceof AttrPtg) {
-            AttrPtg attrPtg0 = (AttrPtg) ptgs[0];
-            if(attrPtg0.isSemiVolatile()) {
-                // no visible formula for semi-volatile
-            } else {
-                // TODO -this requirement is unclear and is not addressed by any junits
-                stack.push(ptgs[0].toFormulaString(book));
-            }
-            i=1;
-        } else {
-            i=0;
-        }
-
-        for ( ; i < ptgs.length; i++) {
             Ptg ptg = ptgs[i];
             // TODO - what about MemNoMemPtg?
             if(ptg instanceof MemAreaPtg || ptg instanceof MemFuncPtg || ptg instanceof MemErrPtg) {
@@ -973,21 +957,30 @@ end;
                 continue;
             }
 
-            if (ptg instanceof AttrPtg && ((AttrPtg) ptg).isOptimizedIf()) {
+            if (ptg instanceof AttrPtg) {
-                continue;
+                AttrPtg attrPtg = ((AttrPtg) ptg);
+                if (attrPtg.isOptimizedIf()) {
+                    continue;
+                }
+                if (attrPtg.isSpace()) {
+                    // POI currently doesn't render spaces in formulas
+                    continue;
+                    // but if it ever did, care must be taken:
+                    // tAttrSpace comes *before* the operand it applies to, which may be consistent
+                    // with how the formula text appears but is against the RPN ordering assumed here 
+                }
             }
 
             final OperationPtg o = (OperationPtg) ptg;
             int nOperands = o.getNumberOfOperands();
             final String[] operands = new String[nOperands];
 
-            for (int j = nOperands-1; j >= 0; j--) {
+            for (int j = nOperands-1; j >= 0; j--) { // reverse iteration because args were pushed in-order
                 if(stack.isEmpty()) {
-                    //TODO: write junit to prove this works
                    String msg = "Too few arguments suppled to operation token ("
                         + o.getClass().getName() + "). Expected (" + nOperands
-                        + " but got " + (nOperands - j + 1);
+                        + ") operands but got (" + (nOperands - j + 1) + ")";
-                    throw new FormulaParseException(msg);
+                    throw new IllegalStateException(msg);
                 }
                 operands[j] = (String) stack.pop();
             }

src/java/org/apache/poi/hssf/record/formula/AttrPtg.java

@@ -33,20 +33,42 @@ import org.apache.poi.util.BitFieldFactory;
  * @author Jason Height (jheight at chariot dot net dot au)
  */
 
-public class AttrPtg
+public final class AttrPtg extends OperationPtg {
-    extends OperationPtg
-{
     public final static byte sid  = 0x19;
     private final static int  SIZE = 4;
     private byte              field_1_options;
     private short             field_2_data;
-    private BitField          semiVolatile = BitFieldFactory.getInstance(0x01);
+    
-    private BitField          optiIf       = BitFieldFactory.getInstance(0x02);
+    // flags 'volatile' and 'space', can be combined.  
-    private BitField          optiChoose   = BitFieldFactory.getInstance(0x04);
+    // OOO spec says other combinations are theoretically possible but not likely to occur.
-    private BitField          optGoto      = BitFieldFactory.getInstance(0x08);
+    private static final BitField semiVolatile = BitFieldFactory.getInstance(0x01);
-    private BitField          sum          = BitFieldFactory.getInstance(0x10);
+    private static final BitField optiIf       = BitFieldFactory.getInstance(0x02);
-    private BitField          baxcel       = BitFieldFactory.getInstance(0x20);
+    private static final BitField optiChoose   = BitFieldFactory.getInstance(0x04);
-    private BitField          space        = BitFieldFactory.getInstance(0x40);
+    private static final BitField optGoto      = BitFieldFactory.getInstance(0x08); // skip
+    private static final BitField sum          = BitFieldFactory.getInstance(0x10);
+    private static final BitField baxcel       = BitFieldFactory.getInstance(0x20); // 'assignment-style formula in a macro sheet'
+    private static final BitField space        = BitFieldFactory.getInstance(0x40);
+    
+    public static final class SpaceType {
+        private SpaceType() {
+            // no instances of this class
+        }
+        
+        /** 00H = Spaces before the next token (not allowed before tParen token) */
+        public static final int SPACE_BEFORE = 0x00;
+        /** 01H = Carriage returns before the next token (not allowed before tParen token) */
+        public static final int CR_BEFORE = 0x01;
+        /** 02H = Spaces before opening parenthesis (only allowed before tParen token) */
+        public static final int SPACE_BEFORE_OPEN_PAREN = 0x02;
+        /** 03H = Carriage returns before opening parenthesis (only allowed before tParen token) */
+        public static final int CR_BEFORE_OPEN_PAREN = 0x03;
+        /** 04H = Spaces before closing parenthesis (only allowed before tParen, tFunc, and tFuncVar tokens) */
+        public static final int SPACE_BEFORE_CLOSE_PAERN = 0x04;
+        /** 05H = Carriage returns before closing parenthesis (only allowed before tParen, tFunc, and tFuncVar tokens) */
+        public static final int CR_BEFORE_CLOSE_PAREN = 0x05;
+        /** 06H = Spaces following the equality sign (only in macro sheets) */
+        public static final int SPACE_AFTER_EQUALITY = 0x06;
+    }
 
     public AttrPtg() {
     }
@@ -56,6 +78,19 @@ public class AttrPtg
         field_1_options = in.readByte();
         field_2_data    = in.readShort();
     }
+    private AttrPtg(int options, int data) {
+        field_1_options = (byte) options;
+        field_2_data = (short) data;
+    }
+    
+    /**
+     * @param type a constant from <tt>SpaceType</tt>
+     * @param count the number of space characters
+     */
+    public static AttrPtg createSpace(int type, int count) {
+        int data = type & 0x00FF | (count << 8) & 0x00FFFF;
+        return new AttrPtg(space.set(0), data);
+    }
 
     public void setOptions(byte options)
     {
@@ -131,21 +166,31 @@ public class AttrPtg
         return field_2_data;
     }
 
-    public String toString()
+    public String toString() {
-    {
+        StringBuffer sb = new StringBuffer(64);
-        StringBuffer buffer = new StringBuffer();
+        sb.append(getClass().getName()).append(" [");
 
-        buffer.append("AttrPtg\n");
+        if(isSemiVolatile()) {
-        buffer.append("options=").append(field_1_options).append("\n");
+            sb.append("volatile ");
-        buffer.append("data   =").append(field_2_data).append("\n");
+        }
-        buffer.append("semi   =").append(isSemiVolatile()).append("\n");
+        if(isSpace()) {
-        buffer.append("optimif=").append(isOptimizedIf()).append("\n");
+            sb.append("space count=").append((field_2_data >> 8) & 0x00FF);
-        buffer.append("optchos=").append(isOptimizedChoose()).append("\n");
+            sb.append(" type=").append(field_2_data & 0x00FF).append(" ");
-        buffer.append("isGoto =").append(isGoto()).append("\n");
+        }
-        buffer.append("isSum  =").append(isSum()).append("\n");
+        // the rest seem to be mutually exclusive
-        buffer.append("isBaxce=").append(isBaxcel()).append("\n");
+        if(isOptimizedIf()) {
-        buffer.append("isSpace=").append(isSpace()).append("\n");
+            sb.append("if dist=").append(getData());
-        return buffer.toString();
+        } else if(isOptimizedChoose()) {
+            sb.append("choose dist=").append(getData());
+        } else if(isGoto()) {
+            sb.append("skip dist=").append(getData()); 
+        } else if(isSum()) {
+            sb.append("sum ");
+        } else if(isBaxcel()) {
+            sb.append("assign ");
+        }
+        sb.append("]");
+        return sb.toString();
     }
 
     public void writeBytes(byte [] array, int offset)

src/testcases/org/apache/poi/hssf/model/TestFormulaParser.java

@@ -844,4 +844,48 @@ public final class TestFormulaParser extends TestCase {
         }
         assertEquals("SUM(A32769:A32770)", cell.getCellFormula());
     }
+
+    public void testSpaceAtStartOfFormula() {
+        // Simulating cell formula of "= 4" (note space)
+        // The same Ptg array can be observed if an excel file is saved with that exact formula
+
+        AttrPtg spacePtg = AttrPtg.createSpace(AttrPtg.SpaceType.SPACE_BEFORE, 1);
+        Ptg[] ptgs = { spacePtg, new IntPtg(4), };
+        String formulaString;
+        try {
+            formulaString = FormulaParser.toFormulaString(null, ptgs);
+        } catch (IllegalStateException e) {
+            if(e.getMessage().equalsIgnoreCase("too much stuff left on the stack")) {
+                throw new AssertionFailedError("Identified bug 44609");
+            }
+            // else some unexpected error
+            throw e;
+        }
+        // FormulaParser strips spaces anyway
+        assertEquals("4", formulaString); 
+
+        ptgs = new Ptg[] { new IntPtg(3), spacePtg, new IntPtg(4), spacePtg, new AddPtg()};
+        formulaString = FormulaParser.toFormulaString(null, ptgs);
+        assertEquals("3+4", formulaString); 
+    }
+
+    /**
+     * Checks some internal error detecting logic ('stack underflow error' in toFormulaString)
+     */
+    public void testTooFewOperandArgs() {
+        // Simulating badly encoded cell formula of "=/1"
+        // Not sure if Excel could ever produce this
+        Ptg[] ptgs = { 
+                // Excel would probably have put tMissArg here
+                new IntPtg(1),
+                new DividePtg(),
+        };
+        try {
+            FormulaParser.toFormulaString(null, ptgs);
+            fail("Expected exception was not thrown");
+        } catch (IllegalStateException e) {
+            // expected during successful test
+            assertTrue(e.getMessage().startsWith("Too few arguments suppled to operation token"));
+        }
+    }
 }