From abe1fae16aeb33faf381999a9fe043e54c6101e5 Mon Sep 17 00:00:00 2001 From: alandavis Date: Wed, 3 Aug 2022 16:25:27 +0100 Subject: [PATCH] Tidy up TransformHandler --- .../transform/base/TransformController.java | 11 +- .../base/transform/TransformHandler.java | 110 ++++-------------- .../base/transform/TransformProcess.java | 93 +++++++++++++-- .../transform/base/AbstractBaseTest.java | 2 +- .../base/TransformControllerAllInOneTest.java | 7 +- .../base/TransformControllerTest.java | 15 +-- .../transform/TransformStreamHandlerTest.java | 2 +- 7 files changed, 124 insertions(+), 116 deletions(-) diff --git a/engines/base/src/main/java/org/alfresco/transform/base/TransformController.java b/engines/base/src/main/java/org/alfresco/transform/base/TransformController.java index 013c6e85..743e6eff 100644 --- a/engines/base/src/main/java/org/alfresco/transform/base/TransformController.java +++ b/engines/base/src/main/java/org/alfresco/transform/base/TransformController.java @@ -104,13 +104,11 @@ public class TransformController private String coreVersion; TransformEngine transformEngine; - ProbeTransform probeTransform; @PostConstruct private void init() { transformEngine = transformHandler.getTransformEngine(); - probeTransform = transformHandler.getProbeTransform(); } @EventListener(ApplicationReadyEvent.class) @@ -186,7 +184,7 @@ public class TransformController @ResponseBody public String ready(HttpServletRequest request) { - return probeTransform.doTransformOrNothing(request, false, transformHandler); + return transformHandler.probe(request, false); } /** @@ -196,7 +194,7 @@ public class TransformController @ResponseBody public String live(HttpServletRequest request) { - return probeTransform.doTransformOrNothing(request, true, transformHandler); + return transformHandler.probe(request, true); } @GetMapping(value = ENDPOINT_TRANSFORM_CONFIG) @@ -298,4 +296,9 @@ public class TransformController mav.setViewName("error"); // display error.html return mav; } + + void resetForTesting() + { + + } } diff --git a/engines/base/src/main/java/org/alfresco/transform/base/transform/TransformHandler.java b/engines/base/src/main/java/org/alfresco/transform/base/transform/TransformHandler.java index ba841405..91c0a42f 100644 --- a/engines/base/src/main/java/org/alfresco/transform/base/transform/TransformHandler.java +++ b/engines/base/src/main/java/org/alfresco/transform/base/transform/TransformHandler.java @@ -65,14 +65,12 @@ import java.io.FileOutputStream; import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; -import java.util.Arrays; import java.util.Comparator; import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicReference; -import java.util.stream.Collectors; import static java.util.stream.Collectors.joining; import static org.alfresco.transform.base.fs.FileManager.createAttachment; @@ -80,11 +78,6 @@ import static org.alfresco.transform.base.fs.FileManager.createTargetFile; import static org.alfresco.transform.base.fs.FileManager.getDirectAccessUrlInputStream; import static org.alfresco.transform.base.fs.FileManager.getMultipartFileInputStream; import static org.alfresco.transform.common.RequestParamMap.DIRECT_ACCESS_URL; -import static org.alfresco.transform.common.RequestParamMap.SOURCE_ENCODING; -import static org.alfresco.transform.common.RequestParamMap.SOURCE_EXTENSION; -import static org.alfresco.transform.common.RequestParamMap.SOURCE_MIMETYPE; -import static org.alfresco.transform.common.RequestParamMap.TARGET_EXTENSION; -import static org.alfresco.transform.common.RequestParamMap.TARGET_MIMETYPE; import static org.springframework.http.HttpStatus.BAD_REQUEST; import static org.springframework.http.HttpStatus.CREATED; import static org.springframework.http.HttpStatus.INTERNAL_SERVER_ERROR; @@ -96,8 +89,6 @@ import static org.springframework.http.HttpStatus.INTERNAL_SERVER_ERROR; public class TransformHandler { private static final Logger logger = LoggerFactory.getLogger(TransformHandler.class); - private static final List NON_TRANSFORM_OPTION_REQUEST_PARAMETERS = Arrays.asList(SOURCE_EXTENSION, - TARGET_EXTENSION, TARGET_MIMETYPE, SOURCE_MIMETYPE, DIRECT_ACCESS_URL); @Autowired(required = false) private List transformEngines; @@ -116,14 +107,12 @@ public class TransformHandler private final AtomicInteger httpRequestCount = new AtomicInteger(1); private TransformEngine transformEngine; - private ProbeTransform probeTransform; private final Map customTransformersByName = new HashMap<>(); @PostConstruct private void init() { initTransformEngine(); - initProbeTestTransform(); initCustomTransformersByName(); } @@ -131,14 +120,7 @@ public class TransformHandler { if (transformEngines != null) { - // Normally there is just one TransformEngine per t-engine, but we also want to be able to amalgamate the - // CustomTransform code from many t-engines into a single t-engine. In this case, there should be a wrapper - // TransformEngine (it has no TransformConfig of its own). - transformEngine = transformEngines.stream() - .filter(transformEngine -> transformEngine.getTransformConfig() == null) - .findFirst() - .orElse(transformEngines.get(0)); - + transformEngine = getTransformEngine(); logger.info("TransformEngine: " + transformEngine.getTransformEngineName()); transformEngines.stream() .filter(te -> te != transformEngine) @@ -147,14 +129,6 @@ public class TransformHandler } } - private void initProbeTestTransform() - { - if (transformEngine != null) - { - probeTransform = transformEngine.getProbeTransform(); - } - } - private void initCustomTransformersByName() { if (customTransformers != null) @@ -171,17 +145,18 @@ public class TransformHandler public TransformEngine getTransformEngine() { - return transformEngine; + // Normally there is just one TransformEngine per t-engine, but we also want to be able to amalgamate the + // CustomTransform code from many t-engines into a single t-engine. In this case, there should be a wrapper + // TransformEngine (it has no TransformConfig of its own). + return transformEngines.stream() + .filter(transformEngine -> transformEngine.getTransformConfig() == null) + .findFirst() + .orElse(transformEngines.get(0)); } public ProbeTransform getProbeTransform() { - return probeTransform; - } - - public TransformerDebug getTransformerDebug() - { - return transformerDebug; + return transformEngine.getProbeTransform(); } public ResponseEntity handleHttpRequest(HttpServletRequest request, @@ -190,8 +165,9 @@ public class TransformHandler { AtomicReference> responseEntity = new AtomicReference<>(); - new TransformProcess(this, sourceMimetype, targetMimetype, requestParameters, - "e" + httpRequestCount.getAndIncrement()) + new TransformProcess(sourceMimetype, targetMimetype, requestParameters, + "e" + httpRequestCount.getAndIncrement(), transformRegistry, + transformerDebug, getProbeTransform(), customTransformersByName) { @Override protected void init() throws IOException @@ -233,8 +209,9 @@ public class TransformHandler public void handleProbRequest(String sourceMimetype, String targetMimetype, Map transformOptions, File sourceFile, File targetFile) { - new TransformProcess(this, sourceMimetype, targetMimetype, transformOptions, - "p" + httpRequestCount.getAndIncrement()) + new TransformProcess(sourceMimetype, targetMimetype, transformOptions, + "p" + httpRequestCount.getAndIncrement(), transformRegistry, + transformerDebug, getProbeTransform(), customTransformersByName) { @Override protected void init() throws IOException @@ -268,8 +245,9 @@ public class TransformHandler public TransformReply handleMessageRequest(TransformRequest request, Long timeout, Destination replyToQueue) { TransformReply reply = createBasicTransformReply(request); - new TransformProcess(this, request.getSourceMediaType(), request.getTargetMediaType(), - request.getTransformRequestOptions(), "unset") + new TransformProcess(request.getSourceMediaType(), request.getTargetMediaType(), + request.getTransformRequestOptions(),"unset", transformRegistry, + transformerDebug, getProbeTransform(), customTransformersByName) { @Override protected void init() throws IOException @@ -320,6 +298,11 @@ public class TransformHandler return reply; } + public String probe(HttpServletRequest request, boolean isLiveProbe) + { + return getProbeTransform().doTransformOrNothing(request, isLiveProbe, this); + } + private void sendSuccessfulResponse(Long timeout, TransformReply reply, Destination replyToQueue) { logger.trace("Sending successful {}, timeout {} ms", reply, timeout); @@ -383,14 +366,6 @@ public class TransformHandler request.setInternalContext(InternalContext.initialise(request.getInternalContext())); } - public Map cleanTransformOptions(Map requestParameters) - { - Map transformOptions = new HashMap<>(requestParameters); - NON_TRANSFORM_OPTION_REQUEST_PARAMETERS.forEach(transformOptions.keySet()::remove); - transformOptions.values().removeIf(String::isEmpty); - return transformOptions; - } - private InputStream getSharedFileStoreInputStream(String sourceReference) { ResponseEntity responseEntity = alfrescoSharedFileStoreClient.retrieveFile(sourceReference); @@ -500,43 +475,4 @@ public class TransformHandler return sb.toString(); } - - public String getTransformerName(final String sourceMimetype, long sourceSizeInBytes, final String targetMimetype, - final Map transformOptions) - { - // The transformOptions always contains sourceEncoding when sent to a T-Engine, even though it should not be - // used to select a transformer. Similar to source and target mimetypes and extensions, but these are not - // passed in transformOptions. - String sourceEncoding = transformOptions.remove(SOURCE_ENCODING); - try - { - final String transformerName = transformRegistry.findTransformerName(sourceMimetype, - sourceSizeInBytes, targetMimetype, transformOptions, null); - if (transformerName == null) - { - throw new TransformException(BAD_REQUEST, "No transforms for: "+ - sourceMimetype+" -> "+targetMimetype+transformOptions.entrySet().stream() - .map(entry -> entry.getKey()+"="+entry.getValue()) - .collect(Collectors.joining(", ", " ", ""))); - } - return transformerName; - } - finally - { - if (sourceEncoding != null) - { - transformOptions.put(SOURCE_ENCODING, sourceEncoding); - } - } - } - - public CustomTransformer getCustomTransformer(String transformName) - { - CustomTransformer customTransformer = customTransformersByName.get(transformName); - if (customTransformer == null) - { - throw new TransformException(INTERNAL_SERVER_ERROR, "Custom Transformer "+transformName+" not found"); - } - return customTransformer; - } } diff --git a/engines/base/src/main/java/org/alfresco/transform/base/transform/TransformProcess.java b/engines/base/src/main/java/org/alfresco/transform/base/transform/TransformProcess.java index dbfb3c5a..ef6509ce 100644 --- a/engines/base/src/main/java/org/alfresco/transform/base/transform/TransformProcess.java +++ b/engines/base/src/main/java/org/alfresco/transform/base/transform/TransformProcess.java @@ -29,17 +29,30 @@ package org.alfresco.transform.base.transform; import org.alfresco.transform.base.CustomTransformer; import org.alfresco.transform.base.TransformController; import org.alfresco.transform.base.logging.LogEntry; +import org.alfresco.transform.base.probes.ProbeTransform; import org.alfresco.transform.client.model.TransformRequest; import org.alfresco.transform.common.TransformException; import org.alfresco.transform.common.TransformerDebug; +import org.alfresco.transform.registry.TransformServiceRegistry; import org.springframework.http.HttpStatus; import org.springframework.web.multipart.MultipartFile; import javax.jms.Destination; import javax.servlet.http.HttpServletRequest; import java.io.File; +import java.util.Arrays; +import java.util.HashMap; +import java.util.List; import java.util.Map; +import java.util.stream.Collectors; +import static org.alfresco.transform.common.RequestParamMap.DIRECT_ACCESS_URL; +import static org.alfresco.transform.common.RequestParamMap.SOURCE_ENCODING; +import static org.alfresco.transform.common.RequestParamMap.SOURCE_EXTENSION; +import static org.alfresco.transform.common.RequestParamMap.SOURCE_MIMETYPE; +import static org.alfresco.transform.common.RequestParamMap.TARGET_EXTENSION; +import static org.alfresco.transform.common.RequestParamMap.TARGET_MIMETYPE; +import static org.springframework.http.HttpStatus.BAD_REQUEST; import static org.springframework.http.HttpStatus.INTERNAL_SERVER_ERROR; import static org.springframework.http.HttpStatus.OK; @@ -52,36 +65,53 @@ import static org.springframework.http.HttpStatus.OK; */ abstract class TransformProcess extends TransformStreamHandler { - private final TransformHandler transformHandler; - private final TransformerDebug transformerDebug; + private static final List NON_TRANSFORM_OPTION_REQUEST_PARAMETERS = Arrays.asList(SOURCE_EXTENSION, + TARGET_EXTENSION, TARGET_MIMETYPE, SOURCE_MIMETYPE, DIRECT_ACCESS_URL); + protected final String sourceMimetype; protected final String targetMimetype; private final Map transformOptions; protected String reference; + private final TransformServiceRegistry transformRegistry; + private final TransformerDebug transformerDebug; + private final ProbeTransform probeTransform; + private final Map customTransformersByName; - TransformProcess(TransformHandler transformHandler, String sourceMimetype, String targetMimetype, - Map transformOptions, String reference) + TransformProcess(String sourceMimetype, String targetMimetype, Map transformOptions, + String reference, TransformServiceRegistry transformRegistry, TransformerDebug transformerDebug, + ProbeTransform probeTransform, Map customTransformersByName) { - LogEntry.start(); - this.transformHandler = transformHandler; this.sourceMimetype = sourceMimetype; this.targetMimetype = targetMimetype; - this.transformOptions = transformHandler.cleanTransformOptions(transformOptions); + this.transformOptions = cleanTransformOptions(transformOptions); this.reference = reference; - this.transformerDebug = transformHandler.getTransformerDebug(); - transformHandler.getProbeTransform().incrementTransformerCount(); + + this.transformRegistry = transformRegistry; + this.transformerDebug = transformerDebug; + this.probeTransform = probeTransform; + this.customTransformersByName = customTransformersByName; + } + + private static Map cleanTransformOptions(Map requestParameters) + { + Map transformOptions = new HashMap<>(requestParameters); + NON_TRANSFORM_OPTION_REQUEST_PARAMETERS.forEach(transformOptions.keySet()::remove); + transformOptions.values().removeIf(String::isEmpty); + return transformOptions; } public void handleTransformRequest() { + LogEntry.start(); transformManager.setSourceMimetype(sourceMimetype); transformManager.setTargetMimetype(targetMimetype); + probeTransform.incrementTransformerCount(); try { init(); long sourceSizeInBytes = getSourceSize(); - String transformName = transformHandler.getTransformerName(sourceMimetype, sourceSizeInBytes, targetMimetype, transformOptions); - CustomTransformer customTransformer = transformHandler.getCustomTransformer(transformName); + String transformName = getTransformerName(sourceMimetype, sourceSizeInBytes, targetMimetype, transformOptions); + CustomTransformer customTransformer = getCustomTransformer(transformName); transformerDebug.pushTransform(reference, sourceMimetype, targetMimetype, sourceSizeInBytes, transformName); transformerDebug.logOptions(reference, transformOptions); handleTransform(customTransformer); @@ -101,7 +131,7 @@ abstract class TransformProcess extends TransformStreamHandler finally { long time = LogEntry.getTransformDuration(); - transformHandler.getProbeTransform().recordTransformTime(time); + probeTransform.recordTransformTime(time); transformerDebug.popTransform(reference, time); LogEntry.complete(); } @@ -137,4 +167,43 @@ abstract class TransformProcess extends TransformStreamHandler { throw new TransformException(INTERNAL_SERVER_ERROR, e.getMessage(), e); } + + private String getTransformerName(final String sourceMimetype, long sourceSizeInBytes, final String targetMimetype, + final Map transformOptions) + { + // The transformOptions always contains sourceEncoding when sent to a T-Engine, even though it should not be + // used to select a transformer. Similar to source and target mimetypes and extensions, but these are not + // passed in transformOptions. + String sourceEncoding = transformOptions.remove(SOURCE_ENCODING); + try + { + final String transformerName = transformRegistry.findTransformerName(sourceMimetype, + sourceSizeInBytes, targetMimetype, transformOptions, null); + if (transformerName == null) + { + throw new TransformException(BAD_REQUEST, "No transforms for: "+ + sourceMimetype+" -> "+targetMimetype+transformOptions.entrySet().stream() + .map(entry -> entry.getKey()+"="+entry.getValue()) + .collect(Collectors.joining(", ", " ", ""))); + } + return transformerName; + } + finally + { + if (sourceEncoding != null) + { + transformOptions.put(SOURCE_ENCODING, sourceEncoding); + } + } + } + + private CustomTransformer getCustomTransformer(String transformName) + { + CustomTransformer customTransformer = customTransformersByName.get(transformName); + if (customTransformer == null) + { + throw new TransformException(INTERNAL_SERVER_ERROR, "Custom Transformer "+transformName+" not found"); + } + return customTransformer; + } } diff --git a/engines/base/src/test/java/org/alfresco/transform/base/AbstractBaseTest.java b/engines/base/src/test/java/org/alfresco/transform/base/AbstractBaseTest.java index ea8fd24f..b6a15942 100644 --- a/engines/base/src/test/java/org/alfresco/transform/base/AbstractBaseTest.java +++ b/engines/base/src/test/java/org/alfresco/transform/base/AbstractBaseTest.java @@ -327,7 +327,7 @@ public abstract class AbstractBaseTest @Test public void calculateMaxTime() throws Exception { - ProbeTransform probeTransform = controller.probeTransform; + ProbeTransform probeTransform = controller.transformHandler.getProbeTransform(); probeTransform.resetForTesting(); probeTransform.setLivenessPercent(110); diff --git a/engines/base/src/test/java/org/alfresco/transform/base/TransformControllerAllInOneTest.java b/engines/base/src/test/java/org/alfresco/transform/base/TransformControllerAllInOneTest.java index 10d5c1ab..538afaeb 100644 --- a/engines/base/src/test/java/org/alfresco/transform/base/TransformControllerAllInOneTest.java +++ b/engines/base/src/test/java/org/alfresco/transform/base/TransformControllerAllInOneTest.java @@ -47,6 +47,7 @@ import java.util.StringJoiner; import static org.alfresco.transform.base.TransformControllerTest.assertConfig; import static org.alfresco.transform.base.TransformControllerTest.getLogMessagesFor; +import static org.alfresco.transform.base.TransformControllerTest.resetProbeForTesting; import static org.alfresco.transform.common.Mimetype.MIMETYPE_IMAGE_JPEG; import static org.alfresco.transform.common.Mimetype.MIMETYPE_PDF; import static org.alfresco.transform.common.Mimetype.MIMETYPE_TEXT_PLAIN; @@ -64,7 +65,6 @@ import static org.alfresco.transform.common.RequestParamMap.SOURCE_MIMETYPE; import static org.alfresco.transform.common.RequestParamMap.TARGET_MIMETYPE; import static org.hamcrest.Matchers.containsString; import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.content; import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.header; import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; @@ -100,7 +100,6 @@ public class TransformControllerAllInOneTest { assertEquals(FakeTransformEngineWithAllInOne.class.getSimpleName(), transformController.transformEngine.getClass().getSimpleName()); - assertNotNull(transformController.probeTransform); } @Test @@ -161,7 +160,7 @@ public class TransformControllerAllInOneTest @Test public void testReadyEndpointReturnsSuccessful() throws Exception { - transformController.probeTransform.resetForTesting(); + resetProbeForTesting(transformController); mockMvc.perform(MockMvcRequestBuilders.get(ENDPOINT_READY)) .andExpect(status().isOk()) .andExpect(content().string(containsString("Success - "))); @@ -170,7 +169,7 @@ public class TransformControllerAllInOneTest @Test public void testLiveEndpointReturnsSuccessful() throws Exception { - transformController.probeTransform.resetForTesting(); + resetProbeForTesting(transformController); mockMvc.perform(MockMvcRequestBuilders.get(ENDPOINT_LIVE)) .andExpect(status().isOk()) .andExpect(content().string(containsString("Success - "))); diff --git a/engines/base/src/test/java/org/alfresco/transform/base/TransformControllerTest.java b/engines/base/src/test/java/org/alfresco/transform/base/TransformControllerTest.java index 18faf9a5..2e015488 100644 --- a/engines/base/src/test/java/org/alfresco/transform/base/TransformControllerTest.java +++ b/engines/base/src/test/java/org/alfresco/transform/base/TransformControllerTest.java @@ -90,7 +90,6 @@ import static org.alfresco.transform.common.RequestParamMap.SOURCE_MIMETYPE; import static org.alfresco.transform.common.RequestParamMap.TARGET_MIMETYPE; import static org.hamcrest.Matchers.containsString; import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.spy; @@ -152,12 +151,17 @@ public class TransformControllerTest .body((Resource) new UrlResource(sfsRef2File.get(invocation.getArguments()[0]).toURI()))); } + + static void resetProbeForTesting(TransformController transformController) + { + transformController.transformHandler.getProbeTransform().resetForTesting(); + } + @Test public void testInitEngine() throws Exception { assertEquals(FakeTransformEngineWithTwoCustomTransformers.class.getSimpleName(), transformController.transformEngine.getClass().getSimpleName()); - assertNotNull(transformController.probeTransform); } @Test @@ -232,7 +236,7 @@ public class TransformControllerTest @Test public void testReadyEndpointReturnsSuccessful() throws Exception { - transformController.probeTransform.resetForTesting(); + resetProbeForTesting(transformController); mockMvc.perform(MockMvcRequestBuilders.get(ENDPOINT_READY)) .andExpect(status().isOk()) .andExpect(content().string(containsString("Success - "))); @@ -241,7 +245,7 @@ public class TransformControllerTest @Test public void testLiveEndpointReturnsSuccessful() throws Exception { - transformController.probeTransform.resetForTesting(); + resetProbeForTesting(transformController); mockMvc.perform(MockMvcRequestBuilders.get(ENDPOINT_LIVE)) .andExpect(status().isOk()) .andExpect(content().string(containsString("Success - "))); @@ -373,9 +377,6 @@ public class TransformControllerTest .param("name2", PAGE_REQUEST_PARAM).param("value2", "1") .param("name3", SOURCE_ENCODING).param("value3", "UTF-8")); - // Do the dispatch, just in case not doing it leaves it in a strange state. -// mockMvc.perform(asyncDispatch(mvcResult)); - verify(spy).handleHttpRequest(any(), any(), eq(MIMETYPE_TEXT_PLAIN), eq(MIMETYPE_PDF), eq(ImmutableMap.of( SOURCE_MIMETYPE, MIMETYPE_TEXT_PLAIN, diff --git a/engines/base/src/test/java/org/alfresco/transform/base/transform/TransformStreamHandlerTest.java b/engines/base/src/test/java/org/alfresco/transform/base/transform/TransformStreamHandlerTest.java index 67130e85..107a8893 100644 --- a/engines/base/src/test/java/org/alfresco/transform/base/transform/TransformStreamHandlerTest.java +++ b/engines/base/src/test/java/org/alfresco/transform/base/transform/TransformStreamHandlerTest.java @@ -48,7 +48,7 @@ import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertTrue; /** - * Tests {@link TransformStreamHandler} and {@link TransformManagerImpl#createSourceFile()} and + * Tests {@link TransformStreamHandler}, {@link TransformManagerImpl#createSourceFile()} and * {@link TransformManagerImpl#createTargetFile()} methods. */ public class TransformStreamHandlerTest