Apply IDE suggestions, code-formating, tests, ...

Add test for DefaultTempFileCreationStrategy
Adjust comments, add test, improve error message

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

poi/src/main/java/org/apache/poi/hssf/record/RecordFactory.java

@@ -34,8 +34,8 @@ Licensed to the Apache Software Foundation (ASF) under one or more
 public final class RecordFactory {
     private static final int NUM_RECORDS = 512;
 
-    // how many records we read at max by default (can be adjusted via IOUtils)
-    //increased to 5 million due to https://bz.apache.org/bugzilla/show_bug.cgi?id=65887
+    // how many records we read at max by default (can be adjusted via the static setters)
+    // increased to 5 million due to https://bz.apache.org/bugzilla/show_bug.cgi?id=65887
     private static final int DEFAULT_MAX_NUMBER_OF_RECORDS = 5_000_000;
     private static int MAX_NUMBER_OF_RECORDS = DEFAULT_MAX_NUMBER_OF_RECORDS;
 

poi/src/main/java/org/apache/poi/hssf/record/RecordInputStream.java

@@ -216,7 +216,8 @@ public void nextRecord() throws RecordFormatException {
         _currentDataLength = _bhi.readDataSize();
         if (_currentDataLength > MAX_RECORD_DATA_SIZE) {
             throw new RecordFormatException("The content of an excel record cannot exceed "
-                    + MAX_RECORD_DATA_SIZE + " bytes");
+                    + MAX_RECORD_DATA_SIZE + " bytes, but had: " + _currentDataLength +
+                    " for record with sid: " + _currentSid);
         }
     }
 

poi/src/main/java/org/apache/poi/ss/formula/functions/TextFunction.java

@@ -108,12 +108,14 @@ protected ValueEval evaluate(String arg) {
             return new NumberEval(arg.length());
         }
     };
+
     public static final Function LOWER = new SingleArgTextFunc() {
         @Override
         protected ValueEval evaluate(String arg) {
             return new StringEval(arg.toLowerCase(Locale.ROOT));
         }
     };
+
     public static final Function UPPER = new SingleArgTextFunc() {
         @Override
         protected ValueEval evaluate(String arg) {
@@ -246,13 +248,16 @@ public ValueEval evaluate(int srcRowIndex, int srcColumnIndex, ValueEval arg0,
     private static final class LeftRight extends Var1or2ArgFunction {
         private static final ValueEval DEFAULT_ARG1 = new NumberEval(1.0);
         private final boolean _isLeft;
+
         protected LeftRight(boolean isLeft) {
             _isLeft = isLeft;
         }
+
         @Override
         public ValueEval evaluate(int srcRowIndex, int srcColumnIndex, ValueEval arg0) {
             return evaluate(srcRowIndex, srcColumnIndex, arg0, DEFAULT_ARG1);
         }
+
         @Override
         public ValueEval evaluate(int srcRowIndex, int srcColumnIndex, ValueEval arg0,
                 ValueEval arg1) {
@@ -369,7 +374,6 @@ public ValueEval evaluate(int srcRowIndex, int srcColumnIndex, ValueEval arg0, V
                             try {
                                 valueDouble = DateUtil.parseDateTime(evaluated);
                             } catch (Exception ignored) {
-                                valueDouble = null;
                             }
                         }
                     }
@@ -393,7 +397,7 @@ public ValueEval evaluate(int srcRowIndex, int srcColumnIndex, ValueEval arg0, V
          * Using it instead of {@link OperandResolver#coerceValueToString(ValueEval)} in order to handle booleans differently.
          */
         private String formatPatternValueEval2String(ValueEval ve) {
-            String format = null;
+            final String format;
             if (!(ve instanceof BoolEval) && (ve instanceof StringValueEval)) {
                 StringValueEval sve = (StringValueEval) ve;
                 format = sve.getStringValue();
@@ -414,6 +418,7 @@ private static final class SearchFind extends Var2or3ArgFunction {
         public SearchFind(boolean isCaseSensitive) {
             _isCaseSensitive = isCaseSensitive;
         }
+
         @Override
         public ValueEval evaluate(int srcRowIndex, int srcColumnIndex, ValueEval arg0, ValueEval arg1) {
             try {
@@ -424,6 +429,7 @@ public ValueEval evaluate(int srcRowIndex, int srcColumnIndex, ValueEval arg0, V
                 return e.getErrorEval();
             }
         }
+
         @Override
         public ValueEval evaluate(int srcRowIndex, int srcColumnIndex, ValueEval arg0, ValueEval arg1,
                 ValueEval arg2) {
@@ -440,6 +446,7 @@ public ValueEval evaluate(int srcRowIndex, int srcColumnIndex, ValueEval arg0, V
                 return e.getErrorEval();
             }
         }
+
         private ValueEval eval(String haystack, String needle, int startIndex) {
             int result;
             if (_isCaseSensitive) {
@@ -454,6 +461,7 @@ private ValueEval eval(String haystack, String needle, int startIndex) {
             return new NumberEval(result + 1.);
         }
     }
+
     /**
      * Implementation of the FIND() function.<p>
      *
@@ -468,6 +476,7 @@ private ValueEval eval(String haystack, String needle, int startIndex) {
      * Author: Torstein Tauno Svendsen ([email protected])
      */
     public static final Function FIND = new SearchFind(true);
+
     /**
      * Implementation of the FIND() function.<p>
      *

poi/src/main/java/org/apache/poi/ss/usermodel/DataFormatter.java

@@ -380,7 +380,7 @@ private Format getFormat(double cellValue, int formatIndex, String formatStrIn,
         // String formatStr = (i < formatBits.length) ? formatBits[i] : formatBits[0];
 
         // this replace is done to fix https://bz.apache.org/bugzilla/show_bug.cgi?id=63211
-        String formatStr = formatStrIn.replace("\\%", "\'%\'");
+        String formatStr = formatStrIn.replace("\\%", "'%'");
 
         // Excel supports 2+ part conditional data formats, eg positive/negative/zero,
         //  or (>1000),(>0),(0),(negative). As Java doesn't handle these kinds
@@ -700,7 +700,7 @@ else if (c == 'd' || c == 'D') {
 
     private String cleanFormatForNumber(String formatStrIn) {
         // this replace is done to fix https://bz.apache.org/bugzilla/show_bug.cgi?id=63211
-        String formatStr = formatStrIn.replace("\\%", "\'%\'");
+        String formatStr = formatStrIn.replace("\\%", "'%'");
 
         StringBuilder sb = new StringBuilder(formatStr);
 

poi/src/test/java/org/apache/poi/hssf/record/TestRecordFactory.java

@@ -19,16 +19,19 @@ Licensed to the Apache Software Foundation (ASF) under one or more
 
 import static org.junit.jupiter.api.Assertions.assertArrayEquals;
 import static org.junit.jupiter.api.Assertions.assertEquals;
-import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.junit.jupiter.api.Assertions.assertInstanceOf;
+import static org.junit.jupiter.api.Assertions.assertThrows;
 
 import java.io.ByteArrayInputStream;
 import java.io.IOException;
 import java.io.InputStream;
 import java.util.List;
 
 import org.apache.commons.io.output.UnsynchronizedByteArrayOutputStream;
+import org.apache.poi.hssf.HSSFTestDataSamples;
 import org.apache.poi.poifs.filesystem.POIFSFileSystem;
 import org.apache.poi.util.HexRead;
+import org.apache.poi.util.RecordFormatException;
 import org.junit.jupiter.api.Test;
 
 /**
@@ -178,11 +181,11 @@ void testMixedContinue() throws IOException {
 
         List<org.apache.poi.hssf.record.Record> records = RecordFactory.createRecords(new ByteArrayInputStream(data));
         assertEquals(5, records.size());
-        assertTrue(records.get(0) instanceof ObjRecord);
-        assertTrue(records.get(1) instanceof DrawingRecord);
-        assertTrue(records.get(2) instanceof TextObjectRecord);
-        assertTrue(records.get(3) instanceof ContinueRecord);
-        assertTrue(records.get(4) instanceof ObjRecord);
+        assertInstanceOf(ObjRecord.class, records.get(0));
+        assertInstanceOf(DrawingRecord.class, records.get(1));
+        assertInstanceOf(TextObjectRecord.class, records.get(2));
+        assertInstanceOf(ContinueRecord.class, records.get(3));
+        assertInstanceOf(ObjRecord.class, records.get(4));
 
         //serialize and verify that the serialized data is the same as the original
         UnsynchronizedByteArrayOutputStream out = UnsynchronizedByteArrayOutputStream.builder().get();
@@ -231,4 +234,21 @@ void testNonZeroPadding_bug46987() throws IOException {
             assertEquals(5, outRecs.size());
         }
     }
+
+    @Test
+    void testMaxNumberOfRecords() {
+        int prev = RecordFactory.getMaxNumberOfRecords();
+
+        try {
+            // check setter/getter
+            RecordFactory.setMaxNumberOfRecords(0);
+            assertEquals(0, RecordFactory.getMaxNumberOfRecords());
+
+            // check exception when exceeding the limit
+            assertThrows(RecordFormatException.class,
+                    () -> HSSFTestDataSamples.openSampleWorkbook("SampleSS.xls"));
+        } finally {
+            RecordFactory.setMaxNumberOfRecords(prev);
+        }
+    }
 }

poi/src/test/java/org/apache/poi/util/DefaultTempFileCreationStrategyTest.java

@@ -0,0 +1,149 @@
+/* ====================================================================
+   Licensed to the Apache Software Foundation (ASF) under one or more
+   contributor license agreements.  See the NOTICE file distributed with
+   this work for additional information regarding copyright ownership.
+   The ASF licenses this file to You under the Apache License, Version 2.0
+   (the "License"); you may not use this file except in compliance with
+   the License.  You may obtain a copy of the License at
+
+       http://www.apache.org/licenses/LICENSE-2.0
+
+   Unless required by applicable law or agreed to in writing, software
+   distributed under the License is distributed on an "AS IS" BASIS,
+   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+   See the License for the specific language governing permissions and
+   limitations under the License.
+==================================================================== */
+
+package org.apache.poi.util;
+
+import static org.apache.poi.util.DefaultTempFileCreationStrategy.DELETE_FILES_ON_EXIT;
+import static org.apache.poi.util.DefaultTempFileCreationStrategy.POIFILES;
+import static org.apache.poi.util.TempFile.JAVA_IO_TMPDIR;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.io.File;
+import java.io.IOException;
+
+import org.apache.commons.io.FileUtils;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+class DefaultTempFileCreationStrategyTest {
+
+    private String propBefore;
+    private String tmpBefore;
+
+    @BeforeEach
+    void before() {
+        propBefore = System.getProperty(DELETE_FILES_ON_EXIT);
+        tmpBefore = System.getProperty(JAVA_IO_TMPDIR);
+    }
+
+    @AfterEach
+    void after() {
+        if (propBefore == null) {
+            System.clearProperty(DELETE_FILES_ON_EXIT);
+        } else {
+            System.setProperty(DELETE_FILES_ON_EXIT, propBefore);
+        }
+
+        System.setProperty(JAVA_IO_TMPDIR, tmpBefore);
+    }
+
+    @Test
+    void testDefaultFile() throws IOException {
+        DefaultTempFileCreationStrategy strategy = new DefaultTempFileCreationStrategy();
+        checkGetFile(strategy);
+    }
+
+    private static void checkGetFile(DefaultTempFileCreationStrategy strategy) throws IOException {
+        File file = strategy.createTempFile("POITest", ".tmp");
+        try {
+            assertTrue(file.getParentFile().exists(),
+                    "Failed for " + file.getParentFile());
+
+            assertTrue(file.exists(),
+                    "Failed for " + file);
+        } finally {
+            assertTrue(file.delete());
+        }
+    }
+
+    @Test
+    void testDefaultDir() throws IOException {
+        DefaultTempFileCreationStrategy strategy = new DefaultTempFileCreationStrategy();
+        File dir = strategy.createTempDirectory("POITest");
+        try {
+            assertTrue(dir.getParentFile().exists(),
+                    "Failed for " + dir.getParentFile());
+
+            assertTrue(dir.exists(),
+                    "Failed for " + dir);
+        } finally {
+            assertTrue(dir.delete());
+        }
+    }
+
+    @Test
+    void testWithProperty() throws IOException {
+        System.setProperty(DELETE_FILES_ON_EXIT, "true");
+
+        // we can set the property, but not easily check if it works
+        // so let's just call the main method
+        testDefaultFile();
+    }
+
+    @Test
+    void testEmptyTempDir() {
+        System.clearProperty(JAVA_IO_TMPDIR);
+
+        DefaultTempFileCreationStrategy strategy = new DefaultTempFileCreationStrategy();
+        assertThrows(IOException.class,
+                () -> strategy.createTempDirectory("POITest"));
+    }
+
+    @Test
+    void testCustomDir() throws IOException {
+        File dirTest = File.createTempFile("POITest", ".dir");
+        try {
+            assertTrue(dirTest.delete());
+
+            DefaultTempFileCreationStrategy strategy = new DefaultTempFileCreationStrategy(dirTest);
+            checkGetFile(strategy);
+        } finally {
+            FileUtils.deleteDirectory(dirTest);
+        }
+    }
+
+    @Test
+    void testCustomDirExists() throws IOException {
+        File dirTest = File.createTempFile("POITest", ".dir");
+        try {
+            assertTrue(dirTest.delete());
+            assertTrue(dirTest.mkdir());
+
+            DefaultTempFileCreationStrategy strategy = new DefaultTempFileCreationStrategy(dirTest);
+            checkGetFile(strategy);
+        } finally {
+            FileUtils.deleteDirectory(dirTest);
+        }
+    }
+
+    @Test
+    void testCustomDirAndPoiFilesExists() throws IOException {
+        File dirTest = File.createTempFile("POITest", ".dir");
+        try {
+            assertTrue(dirTest.delete());
+            assertTrue(dirTest.mkdir());
+            assertTrue(new File(dirTest, POIFILES).mkdir());
+
+            DefaultTempFileCreationStrategy strategy = new DefaultTempFileCreationStrategy(dirTest);
+            checkGetFile(strategy);
+        } finally {
+            FileUtils.deleteDirectory(dirTest);
+        }
+    }
+}