Merged 5.0.N (5.0.4) to 5.1.N (5.1.1)

123386 adavis: MNT-14295 TransformerConfigLimits Behavior Differs on Java 7 vs 8
      - Fix TransformerConfigLimits.setTransformationLimitsFromProperties(...)
        so that it does not clear the limit (e.g. readLimitTimeMs) when setting
        the max value (e.g. timeoutMs) or the other way around. It is normal to
        have both values defined for the default.transformer. 
      - This has been a problem since 4.2.
      - The problem only came to light with the switch to Java 8, as the order in which
        transformer properties are loaded changed. They were being loaded in MapEntry order.
        The problem did not show up in the unit tests, because they too were impacted by
        the MapEntry order and ironically loaded the properties in the same order.
      - Added unit tests that are not impacted by MapEntry order.
      - Enhanced the debug and toString methods in order to trace the problem.


git-svn-id: https://svn.alfresco.com/repos/alfresco-enterprise/alfresco/BRANCHES/DEV/5.1.N/root@123402 c4b6b30b-aa2e-2d43-bbcb-ca4b014f7261

source/java/org/alfresco/repo/content/transform/DoubleMap.java

@@ -19,6 +19,7 @@
 package org.alfresco.repo.content.transform;
 
 import java.util.Map;
+import java.util.Map.Entry;
 import java.util.concurrent.ConcurrentHashMap;
 
 /**
@@ -138,4 +139,26 @@ public class DoubleMap<K1, K2, V>
         
         map.put(key2, t);
     }
+    
+    @Override
+    public String toString()
+    {
+        StringBuilder sb = new StringBuilder();
+        for (Entry<K1, Map<K2, V>> outerEntry: mapMap.entrySet())
+        {
+            for (Entry<K2, V> innerEntry: outerEntry.getValue().entrySet())
+            {
+                if (sb.length() > 0)
+                {
+                    sb.append("\n");
+                }
+                sb.append(outerEntry.getKey()).
+                    append(", ").
+                    append(innerEntry.getKey()).
+                    append(" = ").
+                    append(innerEntry.getValue());
+            }
+        }
+        return sb.toString();
+    }
 }

source/java/org/alfresco/repo/content/transform/TransformerConfigLimits.java

@@ -27,6 +27,7 @@ import java.util.Collection;
 import java.util.HashSet;
 import java.util.Map;
 import java.util.Set;
+import java.util.Map.Entry;
 import java.util.concurrent.ConcurrentHashMap;
 
 import org.alfresco.service.cmr.repository.MimetypeService;
@@ -48,7 +49,7 @@ public class TransformerConfigLimits extends TransformerPropertyNameExtractor
     // Entries are added lower down the hierarchy when a search takes place.
     private Map<String, Map<String, DoubleMap<String, String, TransformationOptionLimits>>> limitsMap;
 
-    // The 'use' value that had properties defined, including null for the default.
+    // The 'use' value that had properties defined, including ANY for the default.
     private Set<String> uses;
 
     public TransformerConfigLimits(TransformerProperties transformerProperties, MimetypeService mimetypeService)
@@ -57,7 +58,7 @@ public class TransformerConfigLimits extends TransformerPropertyNameExtractor
     }
 
     /**
-     * Sets the transformer limits created from system properties.  
+     * Sets the transformer limits created from properties.  
      */
     private void setLimits(TransformerProperties transformerProperties, MimetypeService mimetypeService)
     {
@@ -79,10 +80,25 @@ public class TransformerConfigLimits extends TransformerPropertyNameExtractor
         // Populate the limitsMap for each 'use'.
         for (String use: uses)
         {
-            // Add the system wide default just in case it is not included, as we always need this one
-            TransformationOptionLimits limits = getOrCreateTransformerOptionLimits(DEFAULT_TRANSFORMER, ANY, ANY, use);
-
             Collection<TransformerSourceTargetSuffixValue> properties = getPropertiesForUse(use, allUseMap);
+            setLimits(use, properties);
+        }
+        logger.debug(this);
+    }
+
+    /**
+     * Sets the transformer limits for a single use from properties. Method extracted so that
+     * it is possible to write a simpler unit test, that changes to the order of the
+     * properties. The order changed between Java 6, 7 and 8, resulting in MNT-14295. The original
+     * outer method cannot be used as it creates the list from a map (allUseMap) that it also
+     * creates and the order of values from that map cannot be controlled from a test.
+     */
+    void setLimits(String use, Collection<TransformerSourceTargetSuffixValue> properties)
+    {
+        // Add the system wide default just in case it is not included, as we always need this one
+        getOrCreateTransformerOptionLimits(DEFAULT_TRANSFORMER, ANY, ANY, use);
+
+        TransformationOptionLimits limits;
         for (int pass=0; pass<=3; pass++)
         {
             for (TransformerSourceTargetSuffixValue property: properties)
@@ -90,6 +106,7 @@ public class TransformerConfigLimits extends TransformerPropertyNameExtractor
                 int origLevel = getLevel(property.transformerName, property.sourceMimetype);
                 if (pass == origLevel)
                 {
+                    logger.debug(property);
                     String transformerName = (property.transformerName == null)
                             ? DEFAULT_TRANSFORMER : property.transformerName;
                     limits = getOrCreateTransformerOptionLimits(transformerName,
@@ -100,7 +117,6 @@ public class TransformerConfigLimits extends TransformerPropertyNameExtractor
             }
         }
     }
-    }
 
     /**
      * Returns the 'effective' properties for the given 'use'.
@@ -205,6 +221,29 @@ public class TransformerConfigLimits extends TransformerPropertyNameExtractor
         return limits;
     }
     
+    
+    @Override
+    public String toString()
+    {
+        StringBuilder sb = new StringBuilder();
+        for (Entry<String, Map<String, DoubleMap<String, String, TransformationOptionLimits>>> useEntry: limitsMap.entrySet())
+        {
+            for (Entry<String, DoubleMap<String, String, TransformationOptionLimits>> transformerEntry: useEntry.getValue().entrySet())
+            {
+                if (sb.length() > 0)
+                {
+                    sb.append("\n");
+                }
+                sb.append(useEntry.getKey()).
+                    append(", ").
+                    append(transformerEntry.getKey()).
+                    append(" =>\n").
+                    append(transformerEntry.getValue());
+            }
+        }
+        return sb.toString();
+    }
+
     /**
      * Creates a new TransformationOptionLimits for the use, transformer and mimetype combination,
      * defaulting values from lower levels.
@@ -259,37 +298,32 @@ public class TransformerConfigLimits extends TransformerPropertyNameExtractor
         long l = Long.parseLong(value);
         if (suffix == TransformerConfig.MAX_SOURCE_SIZE_K_BYTES)
         {
-            limits.setReadLimitKBytes(-1);
             limits.setMaxSourceSizeKBytes(l);
         }
         else if (suffix == TransformerConfig.TIMEOUT_MS)
         {
-            limits.setReadLimitTimeMs(-1);
             limits.setTimeoutMs(l);
         }
         else if (suffix == TransformerConfig.MAX_PAGES)
         {
-            limits.setPageLimit(-1);
             limits.setMaxPages((int)l);
         }
         else if (suffix == TransformerConfig.READ_LIMIT_K_BYTES)
         {
-            limits.setMaxSourceSizeKBytes(-1);
             limits.setReadLimitKBytes(l);
         }
         else if (suffix == TransformerConfig.READ_LIMIT_TIME_MS)
         {
-            limits.setTimeoutMs(-1);
             limits.setReadLimitTimeMs(l);
         }
         else // if (suffix == TransformerConfig.PAGE_LIMIT)
         {
-            limits.setMaxPages(-1);
             limits.setPageLimit((int)l);
         }
     }
     
-    private void debug(String msg, String transformerName, String sourceMimetype, String targetMimetype, String use, TransformationOptionLimits limits)
+    private void debug(String msg, String transformerName, String sourceMimetype,
+        String targetMimetype, String use, TransformationOptionLimits limits)
     {
         if (logger.isDebugEnabled())
         {
@@ -312,10 +346,9 @@ public class TransformerConfigLimits extends TransformerPropertyNameExtractor
                 sb.append('.');
                 sb.append(use);
                 sb.append('=');
-                sb.append(limits.getMaxSourceSizeKBytes());
+                sb.append(limits);
             }
             String line = sb.toString();
-//          System.err.println(line);
             logger.debug(line);
         }
     }

source/java/org/alfresco/repo/content/transform/TransformerPropertyNameExtractor.java

@@ -348,7 +348,7 @@ class TransformerSourceTargetSuffixKey
                 ? ""
                 : TransformerConfig.EXTENSIONS+sourceExt+'.'+targetExt)+
                 suffix+
-                (use == null ? "" : TransformerConfig.USE + use);
+                (use == null || ANY.equals(use) ? "" : TransformerConfig.USE + use);
     }
 
     @Override

source/java/org/alfresco/service/cmr/repository/TransformationOptionLimits.java

@@ -221,7 +221,7 @@ public class TransformationOptionLimits implements Serializable
         kbytes.append(sb, OPT_MAX_SOURCE_SIZE_K_BYTES, OPT_READ_LIMIT_K_BYTES);
         pages.append(sb, OPT_MAX_PAGES, OPT_PAGE_LIMIT);
         sb.append("}");
-        return sb.length() == 2 ? "" : sb.toString();
+        return sb.length() == 2 ? "no limits" : sb.toString();
     }
 
     /**

source/test-java/org/alfresco/repo/content/transform/TransformerConfigLimitsTest.java

@@ -18,10 +18,15 @@
  */
 package org.alfresco.repo.content.transform;
 
+import static org.alfresco.repo.content.transform.TransformerConfig.ANY;
 import static org.alfresco.repo.content.transform.TransformerPropertyNameExtractorTest.mockMimetypes;
 import static org.alfresco.repo.content.transform.TransformerPropertyNameExtractorTest.mockProperties;
 import static org.junit.Assert.assertEquals;
 
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.List;
+
 import org.alfresco.service.cmr.repository.MimetypeService;
 import org.alfresco.service.cmr.repository.TransformationOptionLimits;
 import org.junit.Before;
@@ -61,6 +66,44 @@ public class TransformerConfigLimitsTest
                 "text/plain",       "txt");
     }
 
+    private TransformerConfigLimits newTransformerConfigLimits(String[][] data)
+    {
+        // Mock up an extractor with no properties
+        mockProperties(transformerProperties);
+        TransformerConfigLimits extractor = new TransformerConfigLimits(transformerProperties, mimetypeService);
+
+        // Work out the uses, but keep the order
+        List<String> uses = new ArrayList<>();
+        for (int i=0; i < data.length; i++)
+        {
+            if (!uses.contains(data[i][4]))
+            {
+                uses.add(data[i][4]);
+            }
+        }
+        
+        // Load data for each use - keep the order
+        for (String use: uses)
+        {
+            Collection<TransformerSourceTargetSuffixValue> properties = new ArrayList<>();
+            for (int i=0; i < data.length; i++)
+            {
+                if (use.equals(data[i][4]))
+                {
+                    properties.add(new TransformerSourceTargetSuffixValue(
+                        data[i][0], data[i][1], data[i][2], data[i][3], data[i][4], data[i][5],
+                        mimetypeService));
+                }
+            }
+            if (properties.size() > 0)
+            {
+                extractor.setLimits(use, properties);
+            }
+        }
+        
+        return extractor;
+    }
+    
     @Test
     // A value is specified for a transformer and mimetypes
     public void transformerMimetypesTest()
@@ -187,6 +230,58 @@ public class TransformerConfigLimitsTest
         assertEquals(1, txtToPngLimits.getPageLimit());
     }
     
+    // MNT-14295 With Java 7 the order in which properties were supplied changed from
+    // what happen with Java 6 and happens with 8. When combined with a bug to do with
+    // always clearing the max value when setting a limit or the limit when setting
+    // the max value, the initial map of TransformerConfigLimits would be different.
+    // Java 7 was used as the runtime for 4.2 and the 5.0 but Java 8 became the default
+    // from 5.0.1.
+    // None of the other unit tests in this class failed as a none of them provided
+    // both max and limit values.
+    @Test
+    public void propertyOrderJava7Test()
+    {
+        // Load extractor with properties in the order seen when running Java 7
+        //     "content.transformer.default.timeoutMs", "120000",
+        //     "content.transformer.default.readLimitTimeMs", "-1");
+        extractor = newTransformerConfigLimits(new String[][]
+        {
+            {"transformer.default", ANY, ANY, ".timeoutMs",       ANY, "120000"},
+            {"transformer.default", ANY, ANY, ".readLimitTimeMs", ANY, "-1"}
+        });
+
+        TransformationOptionLimits limits = extractor.getLimits(transformer1, "text/plain", "image/png", null);
+        assertEquals(120000L, limits.getTimeoutMs());
+        assertEquals(-1L, limits.getReadLimitTimeMs());
+    }
+
+    @Test
+    public void propertyOrderJava8or6Test()
+    {
+        // Load extractor with properties in the order seen when running Java 8
+        //     "content.transformer.default.timeoutMs", "120000",
+        //     "content.transformer.default.readLimitTimeMs", "-1");
+        extractor = newTransformerConfigLimits(new String[][]
+        {
+            {"transformer.default", ANY, ANY, ".readLimitTimeMs", ANY, "-1"},
+            {"transformer.default", ANY, ANY, ".timeoutMs",       ANY, "120000"}
+        });
+
+        TransformationOptionLimits limits = extractor.getLimits(transformer1, "text/plain", "image/png", null);
+        assertEquals(120000L, limits.getTimeoutMs());
+        assertEquals(-1L, limits.getReadLimitTimeMs());
+    }
+
+    @Test(expected=IllegalArgumentException.class)
+    public void bothMaxAndLimitSetTest()
+    {
+        mockProperties(transformerProperties,
+            "content.transformer.default.readLimitTimeMs", "990000",
+            "content.transformer.default.timeoutMs", "120000");
+        
+        extractor = new TransformerConfigLimits(transformerProperties, mimetypeService);
+    }
+    
     // ---------------------------------------
 
     @Test