Merge a36aedd into 933b634

CHANGELOG.md

@@ -18,6 +18,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com)
 
 ### Changed
 - #1539 - Removed unused references to the QueryBuilder API.
+- #1765 - Strings in spreadsheet input are no longer automatically assumed to be strings -- Fixes to spreadsheet and variant for handling data types, especially dates, as well as unit test coverage for data importer.
 - #1774 - Upgraded oakpal dependency to 1.2.0 to support execution in an AEM OSGi runtime.
 - #1786 - Shade embedded libraries and produce dependency-reduced pom to avoid downstream effects of embedded dependencies.
 - #1823 - Upgraded oakpal plugin to 1.2.1 to for json serialization fix.

bundle/src/main/java/com/adobe/acs/commons/data/CompositeVariant.java

@@ -19,11 +19,11 @@
  */
 package com.adobe.acs.commons.data;
 
-import org.osgi.annotation.versioning.ProviderType;
 import java.lang.reflect.Array;
 import java.util.ArrayList;
 import java.util.List;
 import java.util.stream.Collectors;
+import org.osgi.annotation.versioning.ProviderType;
 
 /**
  * Represents a value which could be either a list of variants or a single variant. The idea is that this supports a
@@ -40,13 +40,17 @@ public final class CompositeVariant<T> {
     /**
      * Create a variant either as a preferred type (set value later with addValue) or
      * with an initial value and the preferred type is assumed by the value provided.
-     * @param initial 
+     * @param initial
      */
     public CompositeVariant(T initial) {
         if (initial instanceof Class) {
             this.type = (Class) initial;
         } else {
-            this.type = initial.getClass();
+            if (initial instanceof Variant) {
+                this.type = ((Variant) initial).getBaseType();
+            } else {
+                this.type = initial.getClass();
+            }
             addValue(initial);
         }
     }
@@ -90,7 +94,7 @@ public List<T> getValues() {
     public <U> List<U> getValuesAs(Class<U> otherType) {
         return values.stream().map(v -> getValueAsType(v, otherType)).collect(Collectors.toList());
     }
-    
+
     private <U> U getValueAsType(Variant v, Class<U> type) {
         // This shouldn't be necessary but it helps disambiguate a runtime lambda issue
         return v.asType(type);

bundle/src/main/java/com/adobe/acs/commons/data/Spreadsheet.java

@@ -24,8 +24,8 @@
 import java.lang.reflect.Array;
 import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.Calendar;
 import java.util.Collections;
-import java.util.Date;
 import java.util.HashMap;
 import java.util.Iterator;
 import java.util.LinkedHashMap;
@@ -59,7 +59,7 @@ public class Spreadsheet {
     private int rowCount;
     private transient List<Map<String, CompositeVariant>> dataRows;
     private final List<String> requiredColumns;
-    private Map<String, Class> headerTypes;
+    private Map<String, Optional<Class>> headerTypes;
     private List<String> headerRow;
     private final Map<String, String> delimiters;
     private boolean enableHeaderNameConversion = true;
@@ -70,12 +70,12 @@ public class Spreadsheet {
      * Simple constructor used for unit testing purposes
      *
      * @param convertHeaderNames If true, header names are converted
-     * @param headerArray List of strings for header columns
+     * @param headerArray        List of strings for header columns
      */
     public Spreadsheet(boolean convertHeaderNames, String... headerArray) {
         this.enableHeaderNameConversion = convertHeaderNames;
         headerTypes = Arrays.stream(headerArray)
-                            .collect(Collectors.toMap(this::convertHeaderName, this::detectTypeFromName));
+                .collect(Collectors.toMap(this::convertHeaderName, this::detectTypeFromName));
         headerRow = Arrays.asList(headerArray);
         requiredColumns = Collections.EMPTY_LIST;
         dataRows = new ArrayList<>();
@@ -85,9 +85,9 @@ public Spreadsheet(boolean convertHeaderNames, String... headerArray) {
     /**
      * Simple constructor used for unit testing purposes
      *
-     * @param convertHeaderNames If true, header names are converted
+     * @param convertHeaderNames     If true, header names are converted
      * @param caseInsensitiveHeaders Header names that will be ignored during conversion
-     * @param headerArray List of strings for header columns
+     * @param headerArray            List of strings for header columns
      */
     public Spreadsheet(boolean convertHeaderNames, List<String> caseInsensitiveHeaders, String... headerArray) {
         this(convertHeaderNames, headerArray);
@@ -101,8 +101,8 @@ public Spreadsheet(boolean convertHeaderNames, InputStream file, String... requi
             requiredColumns = Collections.EMPTY_LIST;
         } else {
             requiredColumns = Arrays.stream(required)
-                                    .map(this::convertHeaderName)
-                                    .collect(Collectors.toList());
+                    .map(this::convertHeaderName)
+                    .collect(Collectors.toList());
         }
         this.inputStream = file;
     }
@@ -186,9 +186,16 @@ private Optional<Map<String, CompositeVariant>> buildRow(Row row) {
             if (colName != null && data.get(i) != null && !data.get(i).isEmpty()) {
                 empty = false;
                 if (!out.containsKey(colName)) {
-                    out.put(colName, new CompositeVariant(headerTypes.get(colName)));
+                    Class type = headerTypes.get(colName).orElse(data.get(i).getBaseType());
+                    if (type == Object.class) {
+                        type = data.get(i).getBaseType();
+                    } else if (type == Object[].class) {
+                        type = getArrayType(Optional.of(data.get(i).getBaseType())).get();
+                    }
+                    out.put(colName, new CompositeVariant(type));
                 }
-                if (headerTypes.get(colName).isArray()) {
+                Optional<Class> type = headerTypes.get(colName);
+                if (type.isPresent() && type.get().isArray()) {
                     String[] values = data.get(i).toString().split(Pattern.quote(delimiters.getOrDefault(colName, DEFAULT_DELIMITER)));
                     for (String value : values) {
                         if (value != null && !value.isEmpty()) {
@@ -257,6 +264,9 @@ public String convertHeaderName(String str) {
         } else {
             name = str;
         }
+        if (name.contains("[")) {
+            name = StringUtils.substringBefore(name, "[");
+        }
         if (enableHeaderNameConversion && isHeaderCaseInsensitive(name)) {
             name = String.valueOf(name).toLowerCase().replaceAll("[^0-9a-zA-Z:\\-]+", "_");
         }
@@ -287,26 +297,28 @@ private boolean isHeaderCaseInsensitive(String name) {
      * @param name
      * @return
      */
-    private Class detectTypeFromName(String name) {
+    private Optional<Class> detectTypeFromName(String name) {
         boolean isArray = false;
-        Class detectedClass = String.class;
+        Class detectedClass = Object.class;
         if (name.contains("@")) {
             String typeStr = StringUtils.substringAfter(name, "@");
-            if (name.endsWith("]")) {
-                String colName = convertHeaderName(name);
-                isArray = true;
-                String delimiter = StringUtils.substringBetween(name, "[", "]");
-                typeStr = StringUtils.substringBefore("[", delimiter);
-                if (!StringUtils.isEmpty(delimiter)) {
-                    delimiters.put(colName, delimiter);
-                }
+            if (typeStr.contains("[")) {
+                typeStr = StringUtils.substringBefore(typeStr, "[");
             }
             detectedClass = getClassFromName(typeStr);
         }
+        if (name.endsWith("]")) {
+            isArray = true;
+            String delimiter = StringUtils.substringBetween(name, "[", "]");
+            if (!StringUtils.isEmpty(delimiter)) {
+                String colName = convertHeaderName(name);
+                delimiters.put(colName, delimiter);
+            }
+        }
         if (isArray) {
-            return getArrayType(detectedClass);
+            return getArrayType(Optional.of(detectedClass));
         } else {
-            return detectedClass;
+            return Optional.of(detectedClass);
         }
     }
 
@@ -324,7 +336,7 @@ private Class getClassFromName(String typeStr) {
             case "calendar":
             case "cal":
             case "time":
-                return Date.class;
+                return Calendar.class;
             case "boolean":
             case "bool":
                 return Boolean.TYPE;
@@ -345,25 +357,27 @@ private Class getClassFromName(String typeStr) {
      * @param b
      * @return
      */
-    private Class upgradeToArray(Class a, Class b) {
-        if (a == null) {
+    private Optional<Class> upgradeToArray(Optional<Class> a, Optional<Class> b) {
+        if (!a.isPresent()) {
             return b;
         }
-        if (b == null) {
+        if (!b.isPresent()) {
             return a;
         }
-        if (a.equals(b) || b == String.class) {
+        if (a.get().equals(b.get()) || b.get() == Object.class) {
             return getArrayType(a);
         } else {
             return getArrayType(b);
         }
     }
 
-    private static Class getArrayType(Class clazz) {
-        if (clazz.isArray()) {
+    private static Optional<Class> getArrayType(Optional<Class> clazz) {
+        if (!clazz.isPresent()) {
+            return Optional.empty();
+        } else if (clazz.get().isArray()) {
             return clazz;
         } else {
-            return Array.newInstance(clazz, 0).getClass();
+            return Optional.of(Array.newInstance(clazz.get(), 0).getClass());
         }
     }
 }

bundle/src/main/java/com/adobe/acs/commons/data/Variant.java

@@ -19,28 +19,29 @@
  */
 package com.adobe.acs.commons.data;
 
-import org.osgi.annotation.versioning.ProviderType;
+import java.text.ParseException;
 import java.time.Instant;
 import java.util.Calendar;
 import java.util.Date;
 import java.util.Optional;
-import java.text.ParseException;
 import java.util.function.Function;
 import org.apache.commons.lang.StringUtils;
 import org.apache.commons.lang3.time.FastDateFormat;
 import org.apache.poi.ss.usermodel.Cell;
 import org.apache.poi.ss.usermodel.CellStyle;
 import org.apache.poi.ss.usermodel.DataFormatter;
 import org.apache.poi.ss.usermodel.DateUtil;
+import org.osgi.annotation.versioning.ProviderType;
 
 /**
- * Used to represent values that might be provided as one type but used as
+ * Used to represent values that might be provided as one baseType but used as
  * another. Avoids glue code and switch statements in other parts of the code
  * especially dealing with data from spreadsheets.
  */
 @ProviderType
 public final class Variant {
 
+    private Class baseType = null;
     private static final FastDateFormat STANDARD_DATE_FORMAT = FastDateFormat.getDateTimeInstance(FastDateFormat.SHORT, FastDateFormat.SHORT);
     private Optional<Long> longVal = Optional.empty();
     private Optional<Double> doubleVal = Optional.empty();
@@ -49,14 +50,17 @@ public final class Variant {
     private Optional<Date> dateVal = Optional.empty();
 
     private static final FastDateFormat[] DATE_FORMATS = {
-        FastDateFormat.getDateInstance(FastDateFormat.SHORT),
-        FastDateFormat.getDateInstance(FastDateFormat.LONG),
-        FastDateFormat.getTimeInstance(FastDateFormat.SHORT),
-        FastDateFormat.getTimeInstance(FastDateFormat.LONG),
-        STANDARD_DATE_FORMAT,
-        FastDateFormat.getDateTimeInstance(FastDateFormat.LONG, FastDateFormat.SHORT),
-        FastDateFormat.getDateTimeInstance(FastDateFormat.SHORT, FastDateFormat.LONG),
-        FastDateFormat.getDateTimeInstance(FastDateFormat.LONG, FastDateFormat.LONG),};
+            FastDateFormat.getDateInstance(FastDateFormat.SHORT),
+            FastDateFormat.getDateInstance(FastDateFormat.LONG),
+            FastDateFormat.getTimeInstance(FastDateFormat.SHORT),
+            FastDateFormat.getTimeInstance(FastDateFormat.LONG),
+            STANDARD_DATE_FORMAT,
+            FastDateFormat.getDateTimeInstance(FastDateFormat.LONG, FastDateFormat.SHORT),
+            FastDateFormat.getDateTimeInstance(FastDateFormat.SHORT, FastDateFormat.LONG),
+            FastDateFormat.getDateTimeInstance(FastDateFormat.LONG, FastDateFormat.LONG),
+            FastDateFormat.getDateTimeInstance(FastDateFormat.FULL, FastDateFormat.FULL),
+            FastDateFormat.getInstance("yyyy-MM-dd'T'HH:mm:ss.SSSXXX")
+    };
 
     public Variant() {
     }
@@ -103,6 +107,7 @@ private void setValue(Cell cell) {
                 }
                 if (DateUtil.isCellDateFormatted(cell)) {
                     setValue(cell.getDateCellValue());
+                    baseType = Calendar.class;
                 }
                 DataFormatter dataFormatter = new DataFormatter();
                 if (cellType == Cell.CELL_TYPE_FORMULA) {
@@ -131,38 +136,65 @@ public final <T> void setValue(T val) {
         if (val == null) {
             return;
         }
-        Class type = val.getClass();
-        if (type == Variant.class) {
+        Class valueType = val.getClass();
+        if (valueType == Variant.class) {
             Variant v = (Variant) val;
             longVal = v.longVal;
             doubleVal = v.doubleVal;
             stringVal = v.stringVal;
             booleanVal = v.booleanVal;
             dateVal = v.dateVal;
-        } else if (type == Byte.TYPE || type == Byte.class) {
+            this.baseType = v.baseType;
+        } else if (valueType == Byte.TYPE || valueType == Byte.class) {
             setLongVal(((Byte) val).longValue());
-        } else if (type == Integer.TYPE || type == Integer.class) {
+            if (baseType == null || baseType == String.class) {
+                baseType = Long.TYPE;
+            }
+        } else if (valueType == Integer.TYPE || valueType == Integer.class) {
             setLongVal(((Integer) val).longValue());
-        } else if (type == Long.TYPE || type == Long.class) {
+            if (baseType == null || baseType == String.class) {
+                baseType = Long.TYPE;
+            }
+        } else if (valueType == Long.TYPE || valueType == Long.class) {
             setLongVal((Long) val);
-        } else if (type == Short.TYPE || type == Short.class) {
+            if (baseType == null || baseType == String.class) {
+                baseType = Long.TYPE;
+            }
+        } else if (valueType == Short.TYPE || valueType == Short.class) {
             setLongVal(((Short) val).longValue());
-        } else if (type == Float.TYPE || type == Float.class) {
+            if (baseType == null || baseType == String.class) {
+                baseType = Long.TYPE;
+            }
+        } else if (valueType == Float.TYPE || valueType == Float.class
+                || valueType == Double.TYPE || valueType == Double.class) {
             setDoubleVal((Double) val);
-        } else if (type == Double.TYPE || type == Double.class) {
-            setDoubleVal((Double) val);
-        } else if (type == Boolean.TYPE || type == Boolean.class) {
+            if (baseType == null || baseType == String.class) {
+                baseType = Double.TYPE;
+            }
+        } else if (valueType == Boolean.TYPE || valueType == Boolean.class) {
             setBooleanVal((Boolean) val);
-        } else if (type == String.class) {
+            if (baseType == null || baseType == String.class) {
+                baseType = Boolean.TYPE;
+            }
+        } else if (valueType == String.class) {
             setStringVal((String) val);
-        } else if (type == Date.class) {
+            if (baseType == null) {
+                baseType = String.class;
+            }
+        } else if (valueType == Date.class) {
             setDateVal((Date) val);
-        } else if (type == Instant.class) {
+            baseType = Calendar.class;
+        } else if (valueType == Instant.class) {
             setDateVal(new Date(((Instant) val).toEpochMilli()));
-        } else if (type == Calendar.class) {
+            baseType = Calendar.class;
+        } else if (valueType == Calendar.class) {
             setDateVal(((Calendar) val).getTime());
+            baseType = Calendar.class;
         } else {
             setStringVal(String.valueOf(val));
+            if (baseType == null) {
+                baseType = String.class;
+            }
         }
     }
 
@@ -215,6 +247,7 @@ public Double toDouble() {
                         })));
     }
 
+    @Override
     public String toString() {
         return stringVal.orElse(dateVal.map(STANDARD_DATE_FORMAT::format)
                 .orElse(doubleVal.map(String::valueOf)
@@ -301,4 +334,8 @@ public static <S, D> D convert(S val, Class<D> destType) {
         Variant v = new Variant(val);
         return v.asType(destType);
     }
+
+    Class getBaseType() {
+        return baseType;
+    }
 }