REPO-5549 Fix: Local transformer names must exist and be unique docli… (#264)

* REPO-5549 Fix: Local transformer names must exist and be unique doclib Read from ... when overriding a Local transform.

* Problem was that there were two conflicting pieces of code in play in the LocalTransformRegistry.register method. One that checked for duplicate T-Engine names and the other that allowed transforms to be overridden if they had the same name.
* The code checking for duplicate names needed an extra clause to only look at T-Engines (they have a T-Engine url associated with them).
* The code that overrode transforms then worked, but still had issues as the supported source to target mimetypes, priorities and max sizes were not cleared.
* It turned out to be simpler to split the original LocalTransformRegistry.register method into two. Extracting a new method into CombinedConfig.removeOverriddenOrInvalidTransformers that discarded invalid or overridden config before the list of supported source to target mimetypes was created rather than try to fix them up later. This is why there appear to be quite a few changes.
* More extensive unit tests were also added.

data-model/src/main/java/org/alfresco/util/ConfigScheduler.java

@@ -2,7 +2,7 @@
  * #%L
  * Alfresco Repository
  * %%
- * Copyright (C) 2005 - 2020 Alfresco Software Limited
+ * Copyright (C) 2005 - 2021 Alfresco Software Limited
  * %%
  * This file is part of the Alfresco software.
  * If the software was purchased under a paid Alfresco license, the terms of
@@ -57,7 +57,10 @@ public abstract class ConfigScheduler<Data>
     public static class ConfigSchedulerJob implements Job
     {
         @Override
-        public void execute(JobExecutionContext context) throws JobExecutionException
+        // Synchronized has little effect in normal operation, but on laptops that are suspended, there can be a number
+        // of Threads calling execute concurrently without it, resulting in errors in the log. Theoretically possible in
+        // production but not very likely.
+        public synchronized void execute(JobExecutionContext context) throws JobExecutionException
         {
             JobDataMap dataMap = context.getJobDetail().getJobDataMap();
             ConfigScheduler configScheduler = (ConfigScheduler)dataMap.get(CONFIG_SCHEDULER);

repository/src/main/java/org/alfresco/repo/content/transform/LocalCombinedConfig.java

@@ -0,0 +1,143 @@
+/*
+ * #%L
+ * Alfresco Repository
+ * %%
+ * Copyright (C) 2019 - 2021 Alfresco Software Limited
+ * %%
+ * This file is part of the Alfresco software.
+ * If the software was purchased under a paid Alfresco license, the terms of
+ * the paid license agreement will prevail.  Otherwise, the software is
+ * provided under the following open source license terms:
+ *
+ * Alfresco is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Lesser General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * Alfresco is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with Alfresco. If not, see <http://www.gnu.org/licenses/>.
+ * #L%
+ */
+package org.alfresco.repo.content.transform;
+
+import org.alfresco.transform.client.model.config.Transformer;
+import org.alfresco.transform.client.registry.CombinedConfig;
+import org.alfresco.transform.client.registry.TransformServiceRegistryImpl;
+import org.apache.commons.logging.Log;
+
+import java.util.List;
+
+import static org.alfresco.repo.content.transform.LocalTransformServiceRegistry.LOCAL_TRANSFORMER;
+import static org.alfresco.repo.content.transform.LocalTransformServiceRegistry.URL;
+import static org.alfresco.util.EqualsHelper.nullSafeEquals;
+
+/**
+ * Extends the standard CombinedConfig to add in removal of overridden or invalid transforms.
+ *
+ * @author adavis
+ */
+public class LocalCombinedConfig extends CombinedConfig
+{
+    public LocalCombinedConfig(Log log)
+    {
+        super(log);
+    }
+
+    /**
+     * Discards a transformer that is invalid (e.g. T-Engines with the same name, baseUrl has not been specified on a
+     * T-Engine transform) or overridden an earlier transform with the same name). If the overridden transform is from
+     * a T-Engine and the overriding transform is not a pipeline or a failover, we also copy the {@code baseUrl} from
+     * the overridden transform so that the original T-Engine will still be called.
+     *
+     * @param i the current transform's index into combinedTransformers.
+     * @param combinedTransformers the full list of transformers in the order they were read.
+     * @param registry that wil hold the transforms.
+     * @param transformAndItsOrigin the current combinedTransformers element.
+     * @param transformer the current transformer.
+     * @param name the current transformer's name.
+     * @param readFrom where the current transformer was read from.
+     * @param isPipeline if the current transform is a pipeline.
+     * @param isFailover if the current transform is a failover.
+     *
+     * @returns the index of a transform to be removed. {@code -1} is returned if there should not be a remove.
+     * @throws IllegalArgumentException if the current transform has a problem and should be removed.
+     */
+    @Override
+    protected int removeInvalidTransformer(int i, List<TransformAndItsOrigin> combinedTransformers,
+                                               TransformServiceRegistryImpl registry,
+                                               TransformAndItsOrigin transformAndItsOrigin, Transformer transformer,
+                                               String name, String readFrom, boolean isPipeline, boolean isFailover)
+    {
+        int indexToRemove = -1;
+
+        if (name == null || "".equals(name.trim()))
+        {
+            throw new IllegalArgumentException("Local transformer names may not be null. Read from " + readFrom);
+        }
+
+        // Get the baseUrl - test code might change it
+        String baseUrl = transformAndItsOrigin.getBaseUrl();
+        String testBaseUrl = ((LocalTransformServiceRegistry)registry).getBaseUrlIfTesting(name, baseUrl);
+        if (!nullSafeEquals(baseUrl, testBaseUrl))
+        {
+            baseUrl = testBaseUrl;
+            transformAndItsOrigin = new TransformAndItsOrigin(transformer, baseUrl, readFrom);
+            combinedTransformers.set(i, transformAndItsOrigin);
+        }
+
+        boolean isOneStepTransform = !isPipeline && !isFailover && !name.equals(LocalPassThroughTransform.NAME);
+
+        // Check to see if the name has been used before.
+        int j = lastIndexOf(name, combinedTransformers, i);
+        if (j >= 0)
+        {
+            if (baseUrl != null) // If a T-Engine, else it is an override
+            {
+                throw new IllegalArgumentException("Local T-Engine transformer " + transformerName(name) +
+                        " must be a unique name. Read from " + readFrom);
+            }
+
+            if (isOneStepTransform)
+            {
+                // We need to set the baseUrl of the original transform in the one overriding,
+                // so we can talk to its T-Engine
+                TransformAndItsOrigin overriddenTransform = combinedTransformers.get(j);
+                String overriddenBaseUrl = overriddenTransform.getBaseUrl();
+                Transformer overriddenTransformTransform = transformAndItsOrigin.getTransformer();
+                TransformAndItsOrigin overridingTransform = new TransformAndItsOrigin(
+                        overriddenTransformTransform, overriddenBaseUrl, readFrom);
+                combinedTransformers.set(i, overridingTransform);
+            }
+            indexToRemove = j;
+        }
+        else if (isOneStepTransform && baseUrl == null)
+        {
+            throw new IllegalArgumentException("Local T-Engine transformer " + transformerName(name) +
+                    " must have its baseUrl set in " + LOCAL_TRANSFORMER + name + URL + " Read from " +
+                    readFrom);
+        }
+        return indexToRemove;
+    }
+
+    private static int lastIndexOf(String name, List<TransformAndItsOrigin> combinedTransformers, int toIndex)
+    {
+        // Lists are short (< 100) entries and this is not a frequent or time critical step, so walking the list
+        // should be okay.
+        for (int j = toIndex-1; j >=0; j--)
+        {
+            TransformAndItsOrigin transformAndItsOrigin = combinedTransformers.get(j);
+            Transformer transformer = transformAndItsOrigin.getTransformer();
+            String transformerName = transformer.getTransformerName();
+            if (name.equals(transformerName))
+            {
+                return j;
+            }
+        }
+        return -1;
+    }
+}

repository/src/main/java/org/alfresco/repo/content/transform/LocalTransformImpl.java

@@ -2,7 +2,7 @@
  * #%L
  * Alfresco Repository
  * %%
- * Copyright (C) 2005 - 2020 Alfresco Software Limited
+ * Copyright (C) 2005 - 2021 Alfresco Software Limited
  * %%
  * This file is part of the Alfresco software.
  * If the software was purchased under a paid Alfresco license, the terms of
@@ -70,7 +70,7 @@ public class LocalTransformImpl extends AbstractLocalTransform
         checkAvailability();
     }
 
-    private boolean remoteTransformerClientConfigured()
+    public boolean remoteTransformerClientConfigured()
     {
         return remoteTransformerClient.getBaseUrl() != null;
     }

repository/src/main/java/org/alfresco/repo/content/transform/LocalTransformServiceRegistry.java

@@ -2,7 +2,7 @@
  * #%L
  * Alfresco Repository
  * %%
- * Copyright (C) 2019 - 2020 Alfresco Software Limited
+ * Copyright (C) 2019 - 2021 Alfresco Software Limited
  * %%
  * This file is part of the Alfresco software.
  * If the software was purchased under a paid Alfresco license, the terms of
@@ -60,8 +60,8 @@ public class LocalTransformServiceRegistry extends TransformServiceRegistryImpl
 {
     private static final Log log = LogFactory.getLog(LocalTransformServiceRegistry.class);
 
-    private static final String LOCAL_TRANSFORMER = "localTransform.";
-    private static final String URL = ".url";
+    public static final String LOCAL_TRANSFORMER = "localTransform.";
+    public static final String URL = ".url";
     static final String STRICT_MIMETYPE_CHECK_WHITELIST_MIMETYPES = "transformer.strict.mimetype.check.whitelist.mimetypes";
 
     public class LocalData extends TransformServiceRegistryImpl.Data
@@ -133,7 +133,7 @@ public class LocalTransformServiceRegistry extends TransformServiceRegistryImpl
     @Override
     public boolean readConfig() throws IOException
     {
-        CombinedConfig combinedConfig = new CombinedConfig(getLog());
+        CombinedConfig combinedConfig = new LocalCombinedConfig(getLog());
         List<String> urls = getTEngineUrls();
         boolean successReadingConfig = combinedConfig.addRemoteConfig(urls, "T-Engine");
         successReadingConfig &= combinedConfig.addLocalConfig("alfresco/transforms");
@@ -167,11 +167,6 @@ public class LocalTransformServiceRegistry extends TransformServiceRegistryImpl
             String name = transformer.getTransformerName();
             LocalData data = getData();
             Map<String, LocalTransform> localTransforms = data.localTransforms;
-            if (name == null || localTransforms.get(name) != null)
-            {
-                throw new IllegalArgumentException("Local transformer names must exist and be unique (" + name + ")."+
-                        " Read from "+readFrom);
-            }
 
             Set<TransformOption> transformsTransformOptions = lookupTransformOptions(
                     transformer.getTransformOptions(), transformOptions, readFrom, this::logError);
@@ -187,20 +182,8 @@ public class LocalTransformServiceRegistry extends TransformServiceRegistryImpl
                         strictMimeTypeCheck, strictMimetypeExceptions, retryTransformOnDifferentMimeType,
                         transformsTransformOptions, this);
             }
-            else if (isPipeline && isFailover)
-            {
-                throw new IllegalArgumentException("Local transformer " + name +
-                        " cannot have pipeline and failover sections. Read from "+readFrom);
-            }
             else if (!isPipeline && !isFailover)
             {
-                baseUrl = getBaseUrlIfTesting(name, baseUrl);
-                if (baseUrl == null)
-                {
-                    throw new IllegalArgumentException("Local transformer " + name +
-                            " must have its baseUrl set in " + LOCAL_TRANSFORMER+name+URL+
-                            " Read from "+readFrom);
-                }
                 int startupRetryPeriodSeconds = getStartupRetryPeriodSeconds(name);
                 localTransform = new LocalTransformImpl(name, transformerDebug, mimetypeService,
                          strictMimeTypeCheck, strictMimetypeExceptions, retryTransformOnDifferentMimeType,
@@ -333,7 +316,7 @@ public class LocalTransformServiceRegistry extends TransformServiceRegistryImpl
     }
 
     // When testing, we need to be able to set the baseUrl when reading from a file.
-    protected String getBaseUrlIfTesting(String name, String baseUrl)
+    public String getBaseUrlIfTesting(String name, String baseUrl)
     {
         return baseUrl;
     }

repository/src/main/java/org/alfresco/transform/client/registry/CombinedConfig.java

@@ -2,7 +2,7 @@
  * #%L
  * Alfresco Repository
  * %%
- * Copyright (C) 2005 - 2020 Alfresco Software Limited
+ * Copyright (C) 2005 - 2021 Alfresco Software Limited
  * %%
  * This file is part of the Alfresco software.
  * If the software was purchased under a paid Alfresco license, the terms of
@@ -54,6 +54,7 @@ import java.util.List;
 import java.util.Map;
 import java.util.Set;
 import java.util.stream.Collectors;
+import java.util.stream.Stream;
 
 import static org.alfresco.repo.content.metadata.AsynchronousExtractor.isMetadataEmbedMimetype;
 import static org.alfresco.repo.content.metadata.AsynchronousExtractor.isMetadataExtractMimetype;
@@ -71,13 +72,13 @@ public class CombinedConfig
 {
     private final Log log;
 
-    static class TransformAndItsOrigin
+    public static class TransformAndItsOrigin
     {
         final Transformer transformer;
         final String baseUrl;
         final String readFrom;
 
-        TransformAndItsOrigin(Transformer transformer, String baseUrl, String readFrom)
+        public TransformAndItsOrigin(Transformer transformer, String baseUrl, String readFrom)
         {
             this.transformer = transformer;
             this.baseUrl = baseUrl;
@@ -88,6 +89,11 @@ public class CombinedConfig
         {
             return transformer;
         }
+
+        public String getBaseUrl()
+        {
+            return baseUrl;
+        }
     }
 
     Map<String, Set<TransformOption>> combinedTransformOptions = new HashMap<>();
@@ -267,8 +273,8 @@ public class CombinedConfig
         data.setTEngineCount(tEngineCount);
         data.setFileCount(configFileFinder.getFileCount());
 
-        combinedTransformers = sortTransformers(combinedTransformers);
-
+        combinedTransformers = removeInvalidTransformers(combinedTransformers, registry);
+        combinedTransformers = sortTransformers(combinedTransformers, registry);
         addWildcardSupportedSourceAndTarget(combinedTransformers);
 
         combinedTransformers.forEach(transformer ->
@@ -276,8 +282,77 @@ public class CombinedConfig
                     transformer.baseUrl, transformer.readFrom));
     }
 
+    /**
+     * Discards transformers that are invalid (e.g. transformers that have both pipeline and failover sections). Calls
+     * {@link #removeInvalidTransformer(int, List, TransformServiceRegistryImpl, TransformAndItsOrigin, Transformer,
+     * String, String, boolean, boolean)} for each transform, so that subclasses (LocalCombinedConfig), may also
+     * discard invalid transforms or overridden transforms.
+     *
+     * @param combinedTransformers the full list of transformers in the order they were read.
+     * @param registry that wil hold the transforms.
+     */
+    private List<TransformAndItsOrigin> removeInvalidTransformers(List<TransformAndItsOrigin> combinedTransformers,
+                                                                  TransformServiceRegistryImpl registry)
+    {
+        for (int i=0; i<combinedTransformers.size(); i++)
+        {
+            try
+            {
+                TransformAndItsOrigin transformAndItsOrigin = combinedTransformers.get(i);
+                Transformer transformer = transformAndItsOrigin.transformer;
+                String readFrom = transformAndItsOrigin.readFrom;
+                String name = transformer.getTransformerName();
+                List<TransformStep> pipeline = transformer.getTransformerPipeline();
+                List<String> failover = transformer.getTransformerFailover();
+                boolean isPipeline = pipeline != null && !pipeline.isEmpty();
+                boolean isFailover = failover != null && !failover.isEmpty();
+
+                if (isPipeline && isFailover)
+                {
+                    throw new IllegalArgumentException("Transformer " + transformerName(name) +
+                            " cannot have pipeline and failover sections. Read from " + readFrom);
+                }
+
+                // Local transforms may override each other or be invalid
+                int indexToRemove = removeInvalidTransformer(i, combinedTransformers, registry, transformAndItsOrigin,
+                        transformer, name, readFrom, isPipeline, isFailover);
+
+                // If required remove the requested transform
+                if (indexToRemove >= 0)
+                {
+                    combinedTransformers.remove(indexToRemove);
+                    // this may also require the current index i to be changed so we don't skip one.
+                    if (i <= indexToRemove)
+                    {
+                        i--;
+                    }
+                }
+            }
+            catch (IllegalArgumentException e)
+            {
+                String msg = e.getMessage();
+                registry.logError(msg);
+                combinedTransformers.remove(i--);
+            }
+        }
+        return combinedTransformers;
+    }
+
+    protected int removeInvalidTransformer(int i, List<TransformAndItsOrigin> combinedTransformers,
+                                           TransformServiceRegistryImpl registry,
+                                           TransformAndItsOrigin transformAndItsOrigin, Transformer transformer,
+                                           String name, String readFrom, boolean isPipeline, boolean isFailover)
+    {
+        return -1;
+    }
+
+    protected static String transformerName(String name)
+    {
+        return name == null ? " without a name" : "\"" + name + "\"";
+    }
+
     // Sort transformers so there are no forward references, if that is possible.
-    private static List<TransformAndItsOrigin> sortTransformers(List<TransformAndItsOrigin> original)
+    private List<TransformAndItsOrigin> sortTransformers(List<TransformAndItsOrigin> original, TransformServiceRegistryImpl registry)
     {
         List<TransformAndItsOrigin> transformers = new ArrayList<>(original.size());
         List<TransformAndItsOrigin> todo = new ArrayList<>(original.size());
@@ -334,6 +409,13 @@ public class CombinedConfig
 
         transformers.addAll(todo);
 
+        for (TransformAndItsOrigin transformAndItsOrigin : original)
+        {
+            String name = transformAndItsOrigin.getTransformer().getTransformerName();
+            registry.logError("Transformer " + transformerName(name) +
+                    " ignored as step transforms do not exist. Read from " + transformAndItsOrigin.readFrom);
+        }
+
         return transformers;
     }
 

repository/src/test/java/org/alfresco/transform/client/registry/LocalTransformServiceRegistryConfigTest.java

@@ -2,7 +2,7 @@
  * #%L
  * Alfresco Repository
  * %%
- * Copyright (C) 2005 - 2020 Alfresco Software Limited
+ * Copyright (C) 2005 - 2021 Alfresco Software Limited
  * %%
  * This file is part of the Alfresco software.
  * If the software was purchased under a paid Alfresco license, the terms of
@@ -28,7 +28,9 @@ package org.alfresco.transform.client.registry;
 import com.fasterxml.jackson.databind.ObjectMapper;
 import org.alfresco.repo.content.MimetypeMap;
 import org.alfresco.repo.content.transform.AbstractLocalTransform;
+import org.alfresco.repo.content.transform.LocalCombinedConfig;
 import org.alfresco.repo.content.transform.LocalPipelineTransform;
+import org.alfresco.repo.content.transform.LocalTransformImpl;
 import org.alfresco.repo.content.transform.LocalTransformServiceRegistry;
 import org.alfresco.repo.content.transform.TransformerDebug;
 import org.alfresco.transform.client.model.config.SupportedSourceAndTarget;
@@ -50,11 +52,15 @@ import java.util.List;
 import java.util.Map;
 import java.util.Properties;
 import java.util.Set;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNotEquals;
 import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
 
 /**
  * Extends the {@link TransformServiceRegistryConfigTest} (used to test the config received from the Transform Service)
@@ -63,10 +69,15 @@ import static org.junit.Assert.assertTrue;
  */
 public class LocalTransformServiceRegistryConfigTest extends TransformServiceRegistryConfigTest
 {
+    public static final String HARD_CODED_VALUE = "hard coded value";
+
     private class TestLocalTransformServiceRegistry extends LocalTransformServiceRegistry
     {
         private boolean mockSuccessReadingConfig = true;
         LocalData dummyData = new LocalData();
+        private List<String> errorsLogged = new ArrayList<>();
+        private boolean resetBaseUrl = true;
+        private int tEngineCount = 0;
 
         public synchronized boolean getMockSuccessReadingConfig()
         {
@@ -79,11 +90,25 @@ public class LocalTransformServiceRegistryConfigTest extends TransformServiceReg
             this.mockSuccessReadingConfig = mockSuccessReadingConfig;
         }
 
-        @Override
-        protected String getBaseUrlIfTesting(String name, String baseUrl)
+        public void setResetBaseUrl(boolean resetBaseUrl)
         {
-            return baseUrl == null
+            this.resetBaseUrl = resetBaseUrl;
+        }
+
+        @Override
+        // As we are mocking, baseUrl is always null, so this method sets it so we don't get errors.
+        // It looks for the alfresco global property or system property with a localTransform. prefix,
+        // just like the normal code.
+        // If setResetBaseUrl(false) is called, the baseUrl remains null
+        // If we are using transforms called "t-engine" only the first one has its baseUrl set - does not use properties.
+        public String getBaseUrlIfTesting(String name, String baseUrl)
+        {
+            boolean isTEngine = "t-engine".equals(name);
+            tEngineCount += isTEngine ? 1 : 0;
+            return baseUrl == null && resetBaseUrl && !isTEngine
                     ? getProperty(LOCAL_TRANSFORM +name+URL, null)
+                    : isTEngine && tEngineCount == 1
+                    ? HARD_CODED_VALUE
                     : baseUrl;
         }
 
@@ -103,6 +128,13 @@ public class LocalTransformServiceRegistryConfigTest extends TransformServiceReg
              return dummyData;
          }
 
+        @Override
+        protected void logError(String msg)
+        {
+            errorsLogged.add(msg);
+            super.logError(msg);
+        }
+
         public Data assertDataChanged(Data prevData, String msg)
         {
             // If the data changes, there has been a read
@@ -119,6 +151,20 @@ public class LocalTransformServiceRegistryConfigTest extends TransformServiceReg
             assertEquals("The configuration data should be the same: "+msg, getData(), data);
             return getData();
         }
+
+        public void assertErrorLogged(String pattern)
+        {
+            Pattern p = Pattern.compile(pattern);
+            for (String msg : errorsLogged)
+            {
+                Matcher matcher = p.matcher(msg);
+                if (matcher.matches())
+                {
+                    return;
+                }
+            }
+            fail("Did not find error message that matches "+pattern);
+        }
     }
 
     private static Log log = LogFactory.getLog(LocalTransformServiceRegistry.class);
@@ -209,11 +255,12 @@ public class LocalTransformServiceRegistryConfigTest extends TransformServiceReg
 
     /**
      * Loads localTransforms from the LOCAL_TRANSFORM_SERVICE_CONFIG config file.
+     * @param path
      */
-    private void retrieveLocalTransformList()
+    private void retrieveLocalTransformList(String path)
     {
-        CombinedConfig combinedConfig = new CombinedConfig(log);
-        combinedConfig.addLocalConfig(LOCAL_TRANSFORM_SERVICE_CONFIG);
+        CombinedConfig combinedConfig = new LocalCombinedConfig(log);
+        combinedConfig.addLocalConfig(path);
         combinedConfig.register(registry);
         mapOfTransformOptions = combinedConfig.combinedTransformOptions;
         transformerList = combinedConfig.combinedTransformers;
@@ -339,16 +386,10 @@ public class LocalTransformServiceRegistryConfigTest extends TransformServiceReg
         internalTestJsonConfig(64, 70);
     }
 
-    @Test
-    public void testReadWriteJson() throws IOException
-    {
-        // Override super method so it passes, as there is nothing more to be gained for LocalTransforms.
-    }
-
     @Test
     public void testReadJsonConfig()
     {
-        retrieveLocalTransformList();
+        retrieveLocalTransformList(LOCAL_TRANSFORM_SERVICE_CONFIG);
 
         // Assert expected size of the transformers.
         assertNotNull("Transformer list is null.", transformerList);
@@ -451,7 +492,7 @@ public class LocalTransformServiceRegistryConfigTest extends TransformServiceReg
     @Test
     public void testReadTransformProperties()
     {
-        retrieveLocalTransformList();
+        retrieveLocalTransformList(LOCAL_TRANSFORM_SERVICE_CONFIG);
 
         assertNotNull("Transformer list is null.", transformerList);
         for (CombinedConfig.TransformAndItsOrigin t : transformerList)
@@ -480,6 +521,7 @@ public class LocalTransformServiceRegistryConfigTest extends TransformServiceReg
     }
 
     @Test
+    // Simulates the reading of config which changes over time and sometimes T-Engines fail to reply.
     public void testAdditionAndRemovalOfTEngines() throws Exception
     {
         CronExpression origCronExpression = registry.getCronExpression();
@@ -543,9 +585,10 @@ public class LocalTransformServiceRegistryConfigTest extends TransformServiceReg
     }
 
     @Test
+    // Checks that only options expected by individual transforms in the pipeline are actually passed to the transform.
     public void testStripExtraOptions()
     {
-        retrieveLocalTransformList();
+        retrieveLocalTransformList(LOCAL_TRANSFORM_SERVICE_CONFIG);
 
         Map<String, String> actualOptions = Map.of(
                 "autoOrient", "true",
@@ -570,9 +613,10 @@ public class LocalTransformServiceRegistryConfigTest extends TransformServiceReg
     }
 
     @Test
+    // Checks that the correct transformer is selected based on priority
     public void testPriority()
     {
-        retrieveLocalTransformList();
+        retrieveLocalTransformList(LOCAL_TRANSFORM_SERVICE_CONFIG);
 
         assertEquals("pdfrenderer",
                 ((AbstractLocalTransform)registry.getLocalTransform("source", -1,
@@ -586,4 +630,122 @@ public class LocalTransformServiceRegistryConfigTest extends TransformServiceReg
                 ((AbstractLocalTransform)registry.getLocalTransform("source", -1,
                         "target3", Collections.emptyMap(), null)).getName());
     }
+
+    @Test
+    public void testNoName()
+    {
+        retrieveLocalTransformList("alfresco/local-transform-service-config-no-name-test.json");
+        registry.assertErrorLogged("Local transformer names may not be null.*no-name-test.*");
+    }
+
+    @Test
+    public void testPipelineAndFailover()
+    {
+        retrieveLocalTransformList("alfresco/local-transform-service-config-pipeline-and-failover-test.json");
+        registry.assertErrorLogged("Transformer .* cannot have pipeline and failover sections.*pipeline-and-failover.*");
+    }
+
+    @Test
+    public void testTEngineDuplicateNames()
+    {
+        retrieveLocalTransformList("alfresco/local-transform-service-config-dup-name-test.json");
+        registry.assertErrorLogged("Local T-Engine transformer .* must be a unique name.*dup-name.*");
+    }
+
+    @Test
+    public void testTEngineNoBaseUrls()
+    {
+        registry.setResetBaseUrl(false);
+        retrieveLocalTransformList("alfresco/local-transform-service-config-no-base-url-test.json");
+        registry.assertErrorLogged("Local T-Engine transformer .* must have its baseUrl set .*no-base-url.*");
+    }
+
+    @Test
+    public void testPipelineMissingStepTransform()
+    {
+        retrieveLocalTransformList("alfresco/transform-service-config-pipeline-missing-step-test.json");
+        registry.assertErrorLogged("Transformer .* ignored as step transforms do not exist.*pipeline-missing-step.*");
+    }
+
+    @Test
+    public void testFailoverMissingStepTransform()
+    {
+        retrieveLocalTransformList("alfresco/transform-service-config-failover-missing-step-test.json");
+        registry.assertErrorLogged("Transformer .* ignored as step transforms do not exist.*failover-missing-step.*");
+    }
+
+    @Test
+    public void testOverrideTEngine()
+    {
+        // This json file contains two transformers with the same name.
+        // * The second one should override the first which has different supported source mimetypes, target mimetypes
+        //   and max sizes. It also has different transform options.
+        // * There is special code in getBaseUrlIfTesting mocking up the baseUrl just for the first one. It should be
+        //   copied to the second one.
+        retrieveLocalTransformList("alfresco/local-transform-service-config-override-test.json");
+
+        assertNotNull("Should still be supported",
+                ((AbstractLocalTransform)registry.getLocalTransform("text/csv", 1000,
+                        "text/html", Collections.emptyMap(), null)));
+
+        assertNotNull("Increased max size be supported",
+                ((AbstractLocalTransform)registry.getLocalTransform("text/csv", 2000,
+                        "text/html", Collections.emptyMap(), null)));
+
+        assertNull("Increased max size is now 2000",
+                ((AbstractLocalTransform)registry.getLocalTransform("text/csv", 3000,
+                        "text/html", Collections.emptyMap(), null)));
+
+        assertNotNull("Should have been added",
+                ((AbstractLocalTransform)registry.getLocalTransform("text/csv", -1,
+                        "application/pdf", Collections.emptyMap(), null)));
+
+        assertNull("Should have been removed",
+                ((AbstractLocalTransform)registry.getLocalTransform("text/csv", -1,
+                        "text/tab-separated-values", Collections.emptyMap(), null)));
+
+        assertNotNull("options1 should still exist, even if not used", mapOfTransformOptions.get("options1"));
+
+        assertNotNull("options2 should exist", mapOfTransformOptions.get("options2"));
+
+        Map<String, String> actualOptions = Map.of(
+                "width", "100",
+                "height", "50");
+        assertNull("width from options1 is no longer used, so should find no transformer",
+                ((AbstractLocalTransform)registry.getLocalTransform("text/csv", -1,
+                        "application/pdf", actualOptions, null)));
+
+        actualOptions = Map.of(
+                "page", "100",
+                "height", "50");
+        assertNotNull("Both options are in options2, so should we should find the transformer",
+                ((AbstractLocalTransform)registry.getLocalTransform("text/csv", -1,
+                        "application/pdf", actualOptions, null)));
+
+        LocalTransformImpl localTransform = (LocalTransformImpl)
+                registry.getLocalTransform("text/csv", -1,
+                "application/pdf", Collections.emptyMap(), null);
+        assertEquals("Should only have 2 options", 2,
+                localTransform.getTransformsTransformOptionNames().size());
+        assertTrue("The baseUrl should have been copied", localTransform.remoteTransformerClientConfigured());
+    }
+
+    @Test
+    public void testOverrideTEngineWithPipeline()
+    {
+        // This json file contains two transformers with the same name, plus two others libreoffice and pdfrenderer
+        // * The first transform which should be overridden has a baseUrl as it talks to a T-Engine and has 5
+        //   supported source to target mimetypes.
+        // * The second one should override the first with a pipeline of libreoffice and pdfrenderer but only has 1
+        //   supported source to target mimetype. It should not have a baseUrl as it will not talk to a T-Engine.
+        //   THIS IS WHAT IS BASICALLY DIFFERENT TO testOverrideTEngine, resulting in a LocalPipelineTransform rather
+        //   rather than a LocalTransformImpl.
+        // * There is special code in getBaseUrlIfTesting mocking up the baseUrl just for the first one. It should be
+        //   copied to the second one.
+        retrieveLocalTransformList("alfresco/local-transform-service-config-override-with-pipeline-test.json");
+
+        LocalPipelineTransform pipelineTransform = (LocalPipelineTransform) registry.getLocalTransform("text/csv",
+                -1,"image/png", Collections.emptyMap(), null);
+        assertNotNull("Should supported csv to png", pipelineTransform);
+    }
 }

repository/src/test/resources/alfresco/local-transform-service-config-dup-name-test.json

@@ -0,0 +1,22 @@
+{
+  "transformOptions": {
+  },
+  "transformers": [
+    {
+      "transformerName": "pdfrenderer",
+      "supportedSourceAndTargetList": [
+        {"sourceMediaType": "image/jpeg", "targetMediaType": "image/bmp"}
+      ],
+      "transformOptions": [
+      ]
+    },
+    {
+      "transformerName": "pdfrenderer",
+      "supportedSourceAndTargetList": [
+        {"sourceMediaType": "image/jpeg", "targetMediaType": "image/bmp"}
+      ],
+      "transformOptions": [
+      ]
+    }
+  ]
+}

repository/src/test/resources/alfresco/local-transform-service-config-no-base-url-test.json

@@ -0,0 +1,14 @@
+{
+  "transformOptions": {
+  },
+  "transformers": [
+    {
+      "transformerName": "pdfrenderer",
+      "supportedSourceAndTargetList": [
+        {"sourceMediaType": "image/jpeg", "targetMediaType": "image/bmp"}
+      ],
+      "transformOptions": [
+      ]
+    }
+  ]
+}

repository/src/test/resources/alfresco/local-transform-service-config-no-name-test.json

@@ -0,0 +1,14 @@
+{
+  "transformOptions": {
+  },
+  "transformers": [
+    {
+      "transformerName": "",
+      "supportedSourceAndTargetList": [
+        {"sourceMediaType": "image/jpeg", "targetMediaType": "image/bmp"}
+      ],
+      "transformOptions": [
+      ]
+    }
+  ]
+}

repository/src/test/resources/alfresco/local-transform-service-config-override-test.json

@@ -0,0 +1,37 @@
+{
+  "transformOptions": {
+    "options1": [
+      {"value": {"name": "width"}},
+      {"value": {"name": "height"}}
+    ],
+    "options2": [
+      {"value": {"name": "page"}},
+      {"value": {"name": "height"}}
+    ]
+  },
+  "transformers": [
+    {
+      "transformerName": "t-engine",
+      "supportedSourceAndTargetList": [
+        {"sourceMediaType": "text/csv", "maxSourceSizeBytes": 1000, "targetMediaType": "text/html" },
+        {"sourceMediaType": "text/csv", "targetMediaType": "application/vnd.oasis.opendocument.spreadsheet" },
+        {"sourceMediaType": "text/csv", "targetMediaType": "application/vnd.oasis.opendocument.spreadsheet-template" },
+        {"sourceMediaType": "text/csv", "targetMediaType": "text/tab-separated-values" },
+        {"sourceMediaType": "text/csv", "priority": 45, "targetMediaType": "application/vnd.ms-excel" }
+      ],
+      "transformOptions": [
+        "options1"
+      ]
+    },
+    {
+      "transformerName": "t-engine",
+      "supportedSourceAndTargetList": [
+        {"sourceMediaType": "text/csv", "maxSourceSizeBytes": 2000, "targetMediaType": "text/html" },
+        {"sourceMediaType": "text/csv", "targetMediaType": "application/pdf" }
+      ],
+      "transformOptions": [
+        "options2"
+      ]
+    }
+  ]
+}

repository/src/test/resources/alfresco/local-transform-service-config-override-with-pipeline-test.json

@@ -0,0 +1,35 @@
+{
+  "transformOptions": {
+  },
+  "transformers": [
+    {
+      "transformerName": "t-engine",
+      "supportedSourceAndTargetList": [
+        {"sourceMediaType": "text/csv", "maxSourceSizeBytes": 1000, "targetMediaType": "text/html" },
+        {"sourceMediaType": "text/csv", "targetMediaType": "application/vnd.oasis.opendocument.spreadsheet" },
+        {"sourceMediaType": "text/csv", "targetMediaType": "application/vnd.oasis.opendocument.spreadsheet-template" },
+        {"sourceMediaType": "text/csv", "targetMediaType": "text/tab-separated-values" },
+        {"sourceMediaType": "text/csv", "priority": 45, "targetMediaType": "application/vnd.ms-excel" }
+      ]
+    },
+    {
+      "transformerName": "t-engine",
+      "transformerPipeline" : [
+        {"transformerName": "libreoffice", "targetMediaType": "application/pdf"},
+        {"transformerName": "pdfrenderer"}
+      ]
+    },
+    {
+      "transformerName": "libreoffice",
+      "supportedSourceAndTargetList": [
+        {"sourceMediaType": "text/csv", "targetMediaType": "application/pdf" }
+      ]
+    },
+    {
+      "transformerName": "pdfrenderer",
+      "supportedSourceAndTargetList": [
+        {"sourceMediaType": "application/pdf", "targetMediaType": "image/png" }
+      ]
+    }
+  ]
+}

repository/src/test/resources/alfresco/local-transform-service-config-pipeline-and-failover-test.json

@@ -0,0 +1,19 @@
+{
+  "transformOptions": {
+  },
+  "transformers": [
+    {
+      "transformerName": "ohDearHasBothPipelineAndFailover",
+      "transformerPipeline" : [
+        {"transformerName": "libreoffice", "targetMediaType": "application/pdf"},
+        {"transformerName": "pdfrenderer"}
+      ],
+      "transformerFailover" : [ "libreoffice", "pdfrenderer" ],
+      "supportedSourceAndTargetList": [
+        {"sourceMediaType": "image/jpeg", "targetMediaType": "image/bmp"}
+      ],
+      "transformOptions": [
+      ]
+    }
+  ]
+}

repository/src/test/resources/alfresco/transform-service-config-failover-missing-step-test.json

@@ -0,0 +1,34 @@
+{
+  "transformOptions": {
+  },
+  "transformers": [
+    {
+      "transformerName": "imagemagick",
+      "supportedSourceAndTargetList": [
+        {"sourceMediaType": "image/jpeg", "targetMediaType": "image/jpeg"},
+        {"sourceMediaType": "image/jpeg", "targetMediaType": "image/png"},
+        {"sourceMediaType": "image/jpeg", "targetMediaType": "image/bmp"}
+      ],
+      "transformOptions": [
+      ]
+    },
+    {
+      "transformerName": "libreoffice",
+      "supportedSourceAndTargetList": [
+        {"sourceMediaType": "application/vnd.ms-outlook",                                                                                "targetMediaType": "application/pdf"}
+      ]
+    },
+    {
+      "transformerName": "missingPdfrenderer",
+      "transformerFailover" : [ "libreoffice", "pdfrenderer" ],
+      "supportedSourceAndTargetList": [
+        {"sourceMediaType": "application/msword",  "targetMediaType": "image/gif" },
+        {"sourceMediaType": "application/msword",  "targetMediaType": "image/jpeg"},
+        {"sourceMediaType": "application/msword",  "targetMediaType": "image/png" },
+        {"sourceMediaType": "application/msword",  "targetMediaType": "image/tiff"}
+      ],
+      "transformOptions": [
+      ]
+    }
+  ]
+}

repository/src/test/resources/alfresco/transform-service-config-pipeline-missing-step-test.json

@@ -0,0 +1,38 @@
+{
+  "transformOptions": {
+  },
+  "transformers": [
+    {
+      "transformerName": "imagemagick",
+      "supportedSourceAndTargetList": [
+        {"sourceMediaType": "image/jpeg", "targetMediaType": "image/jpeg"},
+        {"sourceMediaType": "image/jpeg", "targetMediaType": "image/png"},
+        {"sourceMediaType": "image/jpeg", "targetMediaType": "image/bmp"}
+      ],
+      "transformOptions": [
+      ]
+    },
+    {
+      "transformerName": "libreoffice",
+      "supportedSourceAndTargetList": [
+        {"sourceMediaType": "application/vnd.ms-outlook",                                                                                "targetMediaType": "application/pdf"}
+      ]
+    },
+    {
+      "transformerName": "missingPdfrenderer",
+      "transformerPipeline" : [
+        {"transformerName": "libreoffice", "targetMediaType": "application/pdf"},
+        {"transformerName": "pdfrenderer", "targetMediaType": "image/png"},
+        {"transformerName": "imagemagick"}
+      ],
+      "supportedSourceAndTargetList": [
+        {"sourceMediaType": "application/msword",  "targetMediaType": "image/gif" },
+        {"sourceMediaType": "application/msword",  "targetMediaType": "image/jpeg"},
+        {"sourceMediaType": "application/msword",  "targetMediaType": "image/png" },
+        {"sourceMediaType": "application/msword",  "targetMediaType": "image/tiff"}
+      ],
+      "transformOptions": [
+      ]
+    }
+  ]
+}