mirror of
https://github.com/Alfresco/alfresco-transform-core.git
synced 2025-07-31 17:38:33 +00:00
Resolve some review comments
This commit is contained in:
@@ -3,7 +3,7 @@
|
||||
|
||||
<modelVersion>4.0.0</modelVersion>
|
||||
<artifactId>alfresco-transform-core-aio-boot</artifactId>
|
||||
<name>Alfresco Core All in One Transformer Spring Boot</name>
|
||||
<name>Alfresco Core All-In-One Transformer Spring Boot</name>
|
||||
<packaging>jar</packaging>
|
||||
|
||||
<parent>
|
||||
@@ -14,7 +14,7 @@
|
||||
</parent>
|
||||
|
||||
<properties>
|
||||
<image.name>alfresco/alfresco-transform-aio</image.name>
|
||||
<image.name>alfresco/alfresco-transform-core-aio</image.name>
|
||||
<image.registry>quay.io</image.registry>
|
||||
<env.project_artifactId>${project.artifactId}</env.project_artifactId>
|
||||
</properties>
|
||||
@@ -38,12 +38,6 @@
|
||||
<type>test-jar</type>
|
||||
<scope>test</scope>
|
||||
</dependency>
|
||||
<!-- TODO add transform-aio dependency
|
||||
<dependency>
|
||||
<groupId>org.alfresco</groupId>
|
||||
<artifactId>alfresco-transform-aio</artifactId>
|
||||
<version>${project.version}</version>
|
||||
</dependency> -->
|
||||
|
||||
<dependency>
|
||||
<groupId>org.springframework.boot</groupId>
|
||||
|
@@ -32,19 +32,20 @@ import static org.alfresco.transformer.fs.FileManager.createAttachment;
|
||||
import static org.alfresco.transformer.fs.FileManager.createSourceFile;
|
||||
import static org.alfresco.transformer.fs.FileManager.createTargetFile;
|
||||
import static org.alfresco.transformer.fs.FileManager.createTargetFileName;
|
||||
import static org.springframework.http.HttpStatus.BAD_REQUEST;
|
||||
import static org.springframework.http.HttpStatus.INTERNAL_SERVER_ERROR;
|
||||
import static org.springframework.http.HttpStatus.OK;
|
||||
import static org.springframework.http.MediaType.MULTIPART_FORM_DATA_VALUE;
|
||||
|
||||
import java.io.File;
|
||||
import java.util.List;
|
||||
import java.util.Map;
|
||||
import java.util.ArrayList;
|
||||
import java.util.Arrays;
|
||||
import java.util.Collection;
|
||||
import java.util.HashMap;
|
||||
import java.util.Iterator;
|
||||
|
||||
import javax.servlet.http.HttpServletRequest;
|
||||
|
||||
import org.alfresco.transform.exceptions.TransformException;
|
||||
import org.alfresco.transformer.logging.LogEntry;
|
||||
import org.alfresco.transformer.probes.ProbeTestTransform;
|
||||
import org.alfresco.transformer.transformers.AllInOneTransformer;
|
||||
@@ -63,19 +64,12 @@ public class AIOController extends AbstractTransformerController
|
||||
{
|
||||
private static final Logger logger = LoggerFactory.getLogger(AIOController.class);
|
||||
|
||||
//TODO Should these be moved to the AbstractTransformerController or are they present in the transform.client? They are used by most controllers...
|
||||
private static final String SOURCE_ENCODING = "sourceEncoding";
|
||||
private static final String SOURCE_EXTENSION = "sourceExtension";
|
||||
private static final String TARGET_EXTENSION = "targetExtension";
|
||||
private static final String TARGET_MIMETYPE = "targetMimetype";
|
||||
private static final String SOURCE_MIMETYPE = "sourceMimetype";
|
||||
private static final String TEST_DELAY = "testDelay";
|
||||
private static final String[] UNWANTED_OPTIONS = {SOURCE_EXTENSION,
|
||||
TARGET_EXTENSION,
|
||||
TARGET_MIMETYPE,
|
||||
SOURCE_MIMETYPE,
|
||||
TEST_DELAY
|
||||
};
|
||||
private static final String TEST_DELAY = "testDelay";
|
||||
|
||||
@Autowired
|
||||
private AllInOneTransformer transformer;
|
||||
@@ -96,18 +90,23 @@ public class AIOController extends AbstractTransformerController
|
||||
public void processTransform(File sourceFile, File targetFile, String sourceMimetype, String targetMimetype,
|
||||
Map<String, String> transformOptions, Long timeout)
|
||||
{
|
||||
debugLogTransform("Processing transform", sourceMimetype, targetMimetype, transformOptions);
|
||||
logger.debug("Processing request with: sourceFile '{}', targetFile '{}', transformOptions" +
|
||||
" '{}', timeout {} ms", sourceFile, targetFile, transformOptions, timeout);
|
||||
|
||||
final String transform = getTransformerName(sourceFile, sourceMimetype, targetMimetype, transformOptions);
|
||||
transformOptions.put(AllInOneTransformer.TRANSFORM_NAME_PARAMETER, transform);
|
||||
|
||||
|
||||
try
|
||||
{
|
||||
transformer.transform(sourceFile, targetFile, sourceMimetype, targetMimetype, transformOptions);
|
||||
}
|
||||
catch (Exception e)
|
||||
}
|
||||
catch (IllegalArgumentException e)
|
||||
{
|
||||
logger.error(e.getMessage(), e);
|
||||
throw new TransformException(BAD_REQUEST.value(), e.getMessage(), e);
|
||||
}
|
||||
catch (Exception e)
|
||||
{
|
||||
throw new TransformException(INTERNAL_SERVER_ERROR.value(), e.getMessage(), e);
|
||||
}
|
||||
|
||||
|
||||
@@ -136,7 +135,7 @@ public class AIOController extends AbstractTransformerController
|
||||
}
|
||||
catch(Exception e)
|
||||
{
|
||||
logger.error(e.getMessage(), e);
|
||||
throw new TransformException(INTERNAL_SERVER_ERROR.value(), e.getMessage(), e);
|
||||
}
|
||||
|
||||
}
|
||||
@@ -146,19 +145,22 @@ public class AIOController extends AbstractTransformerController
|
||||
@PostMapping(value = "/transform", consumes = MULTIPART_FORM_DATA_VALUE)
|
||||
public ResponseEntity<Resource> transform(HttpServletRequest request,
|
||||
@RequestParam("file") MultipartFile sourceMultipartFile,
|
||||
@RequestParam(TARGET_EXTENSION) String targetExtension,
|
||||
@RequestParam(TARGET_MIMETYPE) String targetMimetype,
|
||||
@RequestParam(SOURCE_MIMETYPE) String sourceMimetype,
|
||||
@RequestParam Map<String, String> transformOptions,
|
||||
@RequestParam(TARGET_MIMETYPE) String targetMimetype,
|
||||
@RequestParam(TARGET_EXTENSION) String targetExtension,
|
||||
@RequestParam Map<String, String> requestParameters,
|
||||
@RequestParam (value = TEST_DELAY, required = false) Long testDelay)
|
||||
{
|
||||
|
||||
// TODO - remove this logginng
|
||||
debugLogTransform("Entering request with: ", sourceMimetype, targetMimetype, transformOptions);
|
||||
debugLogTransform("Request parameters: ", sourceMimetype, targetMimetype, targetExtension, requestParameters);
|
||||
|
||||
//Remove all required parameters from request parameters to get the list of options
|
||||
List<String> optionsToFilter = Arrays.asList(SOURCE_EXTENSION, TARGET_EXTENSION, TARGET_MIMETYPE,
|
||||
SOURCE_MIMETYPE, TEST_DELAY);
|
||||
Map<String, String> transformOptions = new HashMap<>(requestParameters);
|
||||
transformOptions.keySet().removeAll(optionsToFilter);
|
||||
transformOptions.values().removeIf(v -> v.isEmpty());
|
||||
|
||||
//Using @RequestParam Map<String, String> will gather all text params, including those specified seperately above.
|
||||
removeUnwantedOptions(transformOptions, UNWANTED_OPTIONS, true);
|
||||
|
||||
final String targetFilename = createTargetFileName(
|
||||
sourceMultipartFile.getOriginalFilename(), targetExtension);
|
||||
@@ -166,24 +168,25 @@ public class AIOController extends AbstractTransformerController
|
||||
final File sourceFile = createSourceFile(request, sourceMultipartFile);
|
||||
final File targetFile = createTargetFile(request, targetFilename);
|
||||
|
||||
// TODO Currently sourceMimetype and targetMimetype could be an empty string how does this affect getting the name?
|
||||
// not all controllers take these from the request? Do requests intended for these transforms provide these?
|
||||
final String transform = getTransformerName(sourceFile, sourceMimetype, targetMimetype, transformOptions);
|
||||
|
||||
final String transform = getTransformerName(sourceFile, sourceMimetype, targetMimetype, transformOptions);
|
||||
transformOptions.put(AllInOneTransformer.TRANSFORM_NAME_PARAMETER, transform);
|
||||
|
||||
// TODO - remove this logginng
|
||||
debugLogTransform("After filtering props request with: ", sourceMimetype, targetMimetype, transformOptions);
|
||||
debugLogTransform("Performing transform with parameters: ", sourceMimetype, targetMimetype,
|
||||
targetExtension, requestParameters);
|
||||
|
||||
try
|
||||
{
|
||||
transformer.transform(sourceFile, targetFile, sourceMimetype, targetMimetype, transformOptions);
|
||||
}
|
||||
catch (Exception e)
|
||||
catch (IllegalArgumentException e)
|
||||
{
|
||||
logger.error(e.getMessage(), e);
|
||||
throw new TransformException(BAD_REQUEST.value(), e.getMessage(), e);
|
||||
}
|
||||
catch (Exception e)
|
||||
{
|
||||
throw new TransformException(INTERNAL_SERVER_ERROR.value(), e.getMessage(), e);
|
||||
}
|
||||
|
||||
|
||||
final ResponseEntity<Resource> body = createAttachment(targetFilename, targetFile);
|
||||
LogEntry.setTargetSize(targetFile.length());
|
||||
@@ -193,39 +196,13 @@ public class AIOController extends AbstractTransformerController
|
||||
return body;
|
||||
}
|
||||
|
||||
private void debugLogTransform(String message, String sourceMimetype, String targetMimetype, Map<String, String> transformOptions) {
|
||||
private void debugLogTransform(String message, String sourceMimetype, String targetMimetype, String targetExtension,
|
||||
Map<String, String> transformOptions) {
|
||||
if (logger.isDebugEnabled())
|
||||
{
|
||||
logger.debug(
|
||||
"{} : sourceMimetype: '{}', targetMimetype: '{}', transformOptions: '{}'",
|
||||
message, sourceMimetype, targetMimetype, transformOptions);
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Removes entries from transformOptions that have keys that match a value
|
||||
* contained in unwantedStrings.
|
||||
* Entries that contain empty strings can optionally be removed.
|
||||
*
|
||||
* @param transformOptions
|
||||
* @param unwantedStrings
|
||||
* @param emptyStrings
|
||||
*/
|
||||
private void removeUnwantedOptions(Map<String, String> transformOptions, String[] unwantedStrings, boolean emptyStrings)
|
||||
{
|
||||
for (Iterator<Map.Entry<String, String>> iter = transformOptions.entrySet().iterator();iter.hasNext();)
|
||||
{
|
||||
Map.Entry<String, String> entry = iter.next();
|
||||
if (entry.getValue().isEmpty() || Arrays.asList(unwantedStrings).contains(entry.getKey()))
|
||||
{
|
||||
iter.remove();
|
||||
if (logger.isDebugEnabled())
|
||||
{
|
||||
logger.debug("Key={} has been removed from the provided RequestParameters and it was passed value={}",
|
||||
entry.getKey(), entry.getValue()
|
||||
);
|
||||
}
|
||||
}
|
||||
"{} : sourceMimetype: '{}', targetMimetype: '{}', targetExtension: '{}', transformOptions: '{}'",
|
||||
message, sourceMimetype, targetMimetype, targetExtension, transformOptions);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
@@ -72,9 +72,6 @@ public abstract class AbstractTransformer implements Transformer
|
||||
return transformConfig;
|
||||
}
|
||||
|
||||
/*
|
||||
* TODO - Override default config name by a configurable location defined by a property
|
||||
*/
|
||||
private TransformConfig loadTransformConfig() throws Exception
|
||||
{
|
||||
String configFileName = getTransformerConfigPrefix() + TRANSFORMER_CONFIG_SUFFIX;
|
||||
|
@@ -35,11 +35,6 @@ import java.util.Map;
|
||||
|
||||
/**
|
||||
* Interface for transformers which can perform transformations and specify their own supported configuration.
|
||||
*
|
||||
* TODO - This could be implemented by each individual Transform engine in its own module
|
||||
* and used by controllers for simplicity and clarity. Controllers could be made generic
|
||||
*
|
||||
* @author eknizat
|
||||
*/
|
||||
public interface Transformer
|
||||
{
|
||||
@@ -51,10 +46,6 @@ public interface Transformer
|
||||
/**
|
||||
* Implementation of the actual transformation.
|
||||
*
|
||||
*
|
||||
* TODO - Do we really need the sourceMimetype and targetMimetype as separate arguments?
|
||||
* they could be passed in parameters with predefined keys like TRANSFORM_NAME_PARAMETER
|
||||
*
|
||||
* @param sourceFile
|
||||
* @param targetFile
|
||||
* @param transformOptions
|
||||
@@ -67,8 +58,6 @@ public interface Transformer
|
||||
/**
|
||||
* @return Supported config for the transformer implementation.
|
||||
*
|
||||
* TODO - maybe this does not have to be part of the common transform interface?
|
||||
*
|
||||
*/
|
||||
TransformConfig getTransformConfig();
|
||||
|
||||
|
@@ -80,7 +80,6 @@ public class AllInOneTransformerTest
|
||||
return new String(Files.readAllBytes(file.toPath()), encoding);
|
||||
}
|
||||
|
||||
// TODO - add more thorough test
|
||||
@Test
|
||||
public void testConfigAggregation() throws Exception
|
||||
{
|
||||
|
@@ -1,7 +1,7 @@
|
||||
<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
|
||||
<modelVersion>4.0.0</modelVersion>
|
||||
<artifactId>alfresco-transform-imagemagick-boot</artifactId>
|
||||
<name>Alfresco ImageMagick Transformer SpringBoot</name>
|
||||
<name>Alfresco ImageMagick Transformer Spring Boot</name>
|
||||
<packaging>jar</packaging>
|
||||
|
||||
<parent>
|
||||
|
@@ -3,7 +3,7 @@
|
||||
|
||||
<modelVersion>4.0.0</modelVersion>
|
||||
<artifactId>alfresco-transform-misc-boot</artifactId>
|
||||
<name>Alfresco Docker Miscellaneous Transformers</name>
|
||||
<name>Alfresco Miscellaneous Transformer Spring Boot</name>
|
||||
<packaging>jar</packaging>
|
||||
|
||||
<parent>
|
||||
|
@@ -3,7 +3,7 @@
|
||||
|
||||
<modelVersion>4.0.0</modelVersion>
|
||||
<artifactId>alfresco-transform-misc</artifactId>
|
||||
<name>Alfresco Miscellaneous Transformers</name>
|
||||
<name>Alfresco Miscellaneous Transformer</name>
|
||||
<packaging>jar</packaging>
|
||||
|
||||
<parent>
|
||||
|
@@ -40,10 +40,12 @@ import org.alfresco.transform.client.registry.TransformCache;
|
||||
import org.alfresco.transform.exceptions.TransformException;
|
||||
import org.slf4j.Logger;
|
||||
import org.slf4j.LoggerFactory;
|
||||
import org.springframework.beans.factory.annotation.Autowired;
|
||||
import org.springframework.beans.factory.annotation.Value;
|
||||
import org.springframework.core.io.Resource;
|
||||
|
||||
import com.fasterxml.jackson.databind.ObjectMapper;
|
||||
import org.springframework.core.io.ResourceLoader;
|
||||
|
||||
/**
|
||||
* Used by clients to work out if a transformation is supported based on the engine_config.json.
|
||||
@@ -52,13 +54,22 @@ public class TransformRegistryImpl extends AbstractTransformRegistry
|
||||
{
|
||||
private static final Logger log = LoggerFactory.getLogger(TransformRegistryImpl.class);
|
||||
|
||||
// TODO - do we really need this string?
|
||||
@Autowired
|
||||
ResourceLoader resourceLoader;
|
||||
|
||||
@Value("${transform.config.location:classpath:engine_config.json}")
|
||||
private String locationFromProperty;
|
||||
|
||||
@Value("${transform.config.location:classpath:engine_config.json}")
|
||||
private Resource engineConfig;
|
||||
|
||||
@PostConstruct
|
||||
public void afterPropertiesSet()
|
||||
{
|
||||
engineConfig = resourceLoader.getResource(locationFromProperty);
|
||||
TransformConfig transformConfig = getTransformConfig();
|
||||
registerAll(transformConfig, null, locationFromProperty);
|
||||
}
|
||||
|
||||
// Holds the structures used by AbstractTransformRegistry to look up what is supported.
|
||||
// Unlike other sub classes this class does not extend Data or replace it at run time.
|
||||
private TransformCache data = new TransformCache();
|
||||
@@ -74,17 +85,10 @@ public class TransformRegistryImpl extends AbstractTransformRegistry
|
||||
catch (IOException e)
|
||||
{
|
||||
throw new TransformException(INTERNAL_SERVER_ERROR.value(),
|
||||
"Could not read " + engineConfig.getDescription(), e);
|
||||
"Could not read " + locationFromProperty, e);
|
||||
}
|
||||
}
|
||||
|
||||
@PostConstruct
|
||||
public void afterPropertiesSet()
|
||||
{
|
||||
TransformConfig transformConfig = getTransformConfig();
|
||||
registerAll(transformConfig, null, locationFromProperty);
|
||||
}
|
||||
|
||||
@Override
|
||||
public TransformCache getData()
|
||||
{
|
||||
|
Reference in New Issue
Block a user