diff --git a/source/java/org/alfresco/repo/content/transform/DoubleMap.java b/source/java/org/alfresco/repo/content/transform/DoubleMap.java index d48e5c6d1f..6c12af9d77 100644 --- a/source/java/org/alfresco/repo/content/transform/DoubleMap.java +++ b/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 map.put(key2, t); } + + @Override + public String toString() + { + StringBuilder sb = new StringBuilder(); + for (Entry> outerEntry: mapMap.entrySet()) + { + for (Entry 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(); + } } diff --git a/source/java/org/alfresco/repo/content/transform/TransformerConfigLimits.java b/source/java/org/alfresco/repo/content/transform/TransformerConfigLimits.java index 0aa0246428..c14eae0e7b 100644 --- a/source/java/org/alfresco/repo/content/transform/TransformerConfigLimits.java +++ b/source/java/org/alfresco/repo/content/transform/TransformerConfigLimits.java @@ -27,10 +27,12 @@ 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; import org.alfresco.service.cmr.repository.TransformationOptionLimits; +import org.alfresco.service.cmr.repository.TransformationOptionPair; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; @@ -43,12 +45,14 @@ public class TransformerConfigLimits extends TransformerPropertyNameExtractor { private static Log logger = LogFactory.getLog(TransformerConfigLimits.class); + private static final String NOT_THROWN_MESSAGE = "Both max and limit should not be set."; + // Map using use, transformer, source mimetype and target mimetype to a set of limits. // Entries higher up the hierarchy are added so that values may be defaulted down. // Entries are added lower down the hierarchy when a search takes place. private Map>> 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 uses; public TransformerConfigLimits(TransformerProperties transformerProperties, MimetypeService mimetypeService) @@ -57,7 +61,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,27 +83,63 @@ 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 properties = getPropertiesForUse(use, allUseMap); + + // 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) + for (TransformerSourceTargetSuffixValue property1: properties) { - int origLevel = getLevel(property.transformerName, property.sourceMimetype); + int origLevel = getLevel(property1.transformerName, property1.sourceMimetype); if (pass == origLevel) { - String transformerName = (property.transformerName == null) - ? DEFAULT_TRANSFORMER : property.transformerName; + logger.debug(property1); + String transformerName = (property1.transformerName == null) + ? DEFAULT_TRANSFORMER : property1.transformerName; limits = getOrCreateTransformerOptionLimits(transformerName, - property.sourceMimetype, property.targetMimetype, use); - setTransformationLimitsFromProperties(limits, property.value, property.suffix); - debug("V", transformerName, property.sourceMimetype, property.targetMimetype, use, limits); + property1.sourceMimetype, property1.targetMimetype, use); + setTransformationLimitsFromProperties(limits, property1.value, property1.suffix); + debug("V", transformerName, property1.sourceMimetype, property1.targetMimetype, use, limits); } } } } + 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 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) + { + 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, + property.sourceMimetype, property.targetMimetype, use); + setTransformationLimitsFromProperties(limits, property.value, property.suffix); + debug("V", transformerName, property.sourceMimetype, property.targetMimetype, use, limits); + } + } + } } /** @@ -205,6 +245,29 @@ public class TransformerConfigLimits extends TransformerPropertyNameExtractor return limits; } + + @Override + public String toString() + { + StringBuilder sb = new StringBuilder(); + for (Entry>> useEntry: limitsMap.entrySet()) + { + for (Entry> 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. @@ -253,43 +316,50 @@ public class TransformerConfigLimits extends TransformerPropertyNameExtractor return level; } + /** + * Sets a TransformationOptionLimits value. This may be a size in K bytes, + * a time in ms or number of pages. It may also be a maximum or limit value. + * When the maximum and limit are both set and the same, the maximum value + * wins. + */ private void setTransformationLimitsFromProperties(TransformationOptionLimits limits, String value, String suffix) { - long l = Long.parseLong(value); - if (suffix == TransformerConfig.MAX_SOURCE_SIZE_K_BYTES) + long newValue = Long.parseLong(value); + TransformationOptionPair optionPair = + suffix == TransformerConfig.MAX_SOURCE_SIZE_K_BYTES || + suffix == TransformerConfig.READ_LIMIT_K_BYTES + ? limits.getKBytesPair() + : suffix == TransformerConfig.TIMEOUT_MS || + suffix == TransformerConfig.READ_LIMIT_TIME_MS + ? limits.getTimePair() + : limits.getPagesPair(); + + // If max rather than limit value + if (suffix == TransformerConfig.MAX_SOURCE_SIZE_K_BYTES || + suffix == TransformerConfig.TIMEOUT_MS || + suffix == TransformerConfig.MAX_PAGES) { - limits.setReadLimitKBytes(-1); - limits.setMaxSourceSizeKBytes(l); + long limit = optionPair.getLimit(); + if (limit < 0 || limit >= newValue) + { + optionPair.setLimit(-1, NOT_THROWN_MESSAGE); + optionPair.setMax(newValue, NOT_THROWN_MESSAGE); + } } - else if (suffix == TransformerConfig.TIMEOUT_MS) + else { - 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); + long max = optionPair.getMax(); + if (max < 0 || max > newValue) + { + optionPair.setMax(-1, NOT_THROWN_MESSAGE); + optionPair.setLimit(newValue, NOT_THROWN_MESSAGE); + } } } - 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 +382,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); } } diff --git a/source/java/org/alfresco/repo/content/transform/TransformerPropertyNameExtractor.java b/source/java/org/alfresco/repo/content/transform/TransformerPropertyNameExtractor.java index 7370fa37e4..7dd77d6b3e 100644 --- a/source/java/org/alfresco/repo/content/transform/TransformerPropertyNameExtractor.java +++ b/source/java/org/alfresco/repo/content/transform/TransformerPropertyNameExtractor.java @@ -197,10 +197,14 @@ public abstract class TransformerPropertyNameExtractor } /** - * Optionally adds a new TransformerSourceTargetValue. If the supplied value is constructed from - * from a mimetypes property a new value is always added and will replace any existing value. If - * from an extensions property the value is only added if there is not a current value. - * In other words properties that include mimetypes win out over those that use extensions. + * Optionally adds a new TransformerSourceTargetValue and discards some when + * there is another 'better' property.

+ * + * If there is a property for the max value and another for the limit the lower + * one wins. If equal, the max value wins.

+ * + * If the supplied value is constructed from a mimetypes value it wins over one + * constructed from an extensions property. */ private void addTransformerSourceTargetValue( Map transformerSourceTargetSuffixValues, @@ -213,7 +217,35 @@ public abstract class TransformerPropertyNameExtractor if (mimetypeProperty || !transformerSourceTargetSuffixValues.containsKey(key)) { - transformerSourceTargetSuffixValues.put(key, transformerSourceTargetSuffixValue); + TransformerSourceTargetSuffixKey siblingKey = transformerSourceTargetSuffixValue.getSiblingKey(); + TransformerSourceTargetSuffixValue sibling = transformerSourceTargetSuffixValues.get(siblingKey); + + boolean doAdd = true; + if (sibling != null) + { + doAdd = false; + long newValue = Long.parseLong(value); + if (newValue > 0) + { + long oldValue = Long.parseLong(sibling.value); + + // If max rather than limit value + boolean isMax = + suffix == TransformerConfig.MAX_SOURCE_SIZE_K_BYTES || + suffix == TransformerConfig.TIMEOUT_MS || + suffix == TransformerConfig.MAX_PAGES; + if (oldValue < 0 || (isMax && oldValue >= newValue) || (!isMax && oldValue > newValue)) + { + transformerSourceTargetSuffixValues.remove(siblingKey); + doAdd = true; + } + } + } + + if (doAdd) + { + transformerSourceTargetSuffixValues.put(key, transformerSourceTargetSuffixValue); + } } } @@ -348,7 +380,7 @@ class TransformerSourceTargetSuffixKey ? "" : TransformerConfig.EXTENSIONS+sourceExt+'.'+targetExt)+ suffix+ - (use == null ? "" : TransformerConfig.USE + use); + (use == null || ANY.equals(use) ? "" : TransformerConfig.USE + use); } @Override @@ -435,6 +467,30 @@ class TransformerSourceTargetSuffixValue extends TransformerSourceTargetSuffixKe return new TransformerSourceTargetSuffixKey(transformerName, sourceExt, targetExt, suffix, use); } + /** + * @return the key of the sibling property for the max value if this is the limit value and one + * for the limit value if the max value. + */ + public TransformerSourceTargetSuffixKey getSiblingKey() + { + String siblingSuffix = + suffix == TransformerConfig.MAX_SOURCE_SIZE_K_BYTES + ? TransformerConfig.READ_LIMIT_K_BYTES + : suffix == TransformerConfig.READ_LIMIT_K_BYTES + ? TransformerConfig.MAX_SOURCE_SIZE_K_BYTES + + : suffix == TransformerConfig.TIMEOUT_MS + ? TransformerConfig.READ_LIMIT_TIME_MS + : suffix == TransformerConfig.READ_LIMIT_TIME_MS + ? TransformerConfig.TIMEOUT_MS + + : suffix == TransformerConfig.MAX_PAGES + ? TransformerConfig.PAGE_LIMIT + : TransformerConfig.MAX_PAGES; + + return new TransformerSourceTargetSuffixKey(transformerName, sourceExt, targetExt, siblingSuffix, use); + } + public String toString() { return super.toString()+'='+value; diff --git a/source/java/org/alfresco/service/cmr/repository/TransformationOptionLimits.java b/source/java/org/alfresco/service/cmr/repository/TransformationOptionLimits.java index 4cdcef01cd..b16b5573e0 100644 --- a/source/java/org/alfresco/service/cmr/repository/TransformationOptionLimits.java +++ b/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(); } /** diff --git a/source/test-java/org/alfresco/repo/content/transform/TransformerConfigLimitsTest.java b/source/test-java/org/alfresco/repo/content/transform/TransformerConfigLimitsTest.java index d0be79092f..da0d57d4e2 100644 --- a/source/test-java/org/alfresco/repo/content/transform/TransformerConfigLimitsTest.java +++ b/source/test-java/org/alfresco/repo/content/transform/TransformerConfigLimitsTest.java @@ -187,6 +187,66 @@ 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() + { + mockProperties(transformerProperties, + "content.transformer.default.timeoutMs", "120000", + "content.transformer.default.readLimitTimeMs", "-1"); + + extractor = new TransformerConfigLimits(transformerProperties, mimetypeService); + TransformationOptionLimits limits = extractor.getLimits(transformer1, "text/plain", "image/png", null); + assertEquals(120000L, limits.getTimeoutMs()); + assertEquals(-1L, limits.getReadLimitTimeMs()); + } + + @Test + public void propertyOrderJava8or6Test() + { + mockProperties(transformerProperties, + "content.transformer.default.timeoutMs", "-1", + "content.transformer.default.readLimitTimeMs", "120000"); + + extractor = new TransformerConfigLimits(transformerProperties, mimetypeService); + TransformationOptionLimits limits = extractor.getLimits(transformer1, "text/plain", "image/png", null); + assertEquals(-1L, limits.getTimeoutMs()); + assertEquals(120000L, limits.getReadLimitTimeMs()); + } + + @Test + public void bothMaxAndLimitSetIgnoreLimitTest() + { + mockProperties(transformerProperties, + "content.transformer.default.readLimitTimeMs", "990000", + "content.transformer.default.timeoutMs", "120000"); + + extractor = new TransformerConfigLimits(transformerProperties, mimetypeService); + TransformationOptionLimits limits = extractor.getLimits(transformer1, "text/plain", "image/png", null); + assertEquals(120000L, limits.getTimeoutMs()); + assertEquals(-1L, limits.getReadLimitTimeMs()); + } + + @Test + public void bothMaxAndLimitSetIgnoreMaxTest() + { + mockProperties(transformerProperties, + "content.transformer.default.readLimitTimeMs", "120000", + "content.transformer.default.timeoutMs", "990000"); + + extractor = new TransformerConfigLimits(transformerProperties, mimetypeService); + TransformationOptionLimits limits = extractor.getLimits(transformer1, "text/plain", "image/png", null); + assertEquals(-1L, limits.getTimeoutMs()); + assertEquals(120000L, limits.getReadLimitTimeMs()); + } + // --------------------------------------- @Test