From 1c33205dba004aff66365e2752ceb4b17c4e0bb3 Mon Sep 17 00:00:00 2001 From: Christoph Langguth Date: Fri, 17 Jul 2026 16:11:07 +0200 Subject: [PATCH 1/4] SED-4816 Introduce AttachmentStorage --- .../StepStreamingResourceManager.java | 85 ++++++++++++++++- .../StreamingResourcesControllerPlugin.java | 10 +- .../java/step/attachments/FileResolver.java | 18 ++-- .../java/step/core/AbstractStepContext.java | 40 +++++++- .../step/resources/AttachmentStorage.java | 14 +++ .../resources/LocalAttachmentStorage.java | 93 +++++++++++++++++++ .../StreamingResourceContentProvider.java | 10 -- .../artefacts/handlers/ExportHandler.java | 44 +++++---- .../junitxml/JUnitXmlReportBuilder.java | 39 ++++---- .../step/reporting/JUnit4ReportWriter.java | 25 +++-- .../step/reporting/Junit4ReportConfig.java | 20 ++-- .../artefacts/handlers/ExportHandlerTest.java | 14 ++- .../core/execution/ExecutionEngineRunner.java | 15 ++- .../ReportNodeAttachmentManager.java | 74 +++++---------- .../core/plans/runner/PlanRunnerResult.java | 60 ++++++------ 15 files changed, 374 insertions(+), 187 deletions(-) create mode 100644 step-core/src/main/java/step/resources/AttachmentStorage.java create mode 100644 step-core/src/main/java/step/resources/LocalAttachmentStorage.java delete mode 100644 step-core/src/main/java/step/resources/StreamingResourceContentProvider.java diff --git a/step-controller/step-controller-base-plugins/src/main/java/step/plugins/streaming/StepStreamingResourceManager.java b/step-controller/step-controller-base-plugins/src/main/java/step/plugins/streaming/StepStreamingResourceManager.java index b660ef052..cfc6981ae 100644 --- a/step-controller/step-controller-base-plugins/src/main/java/step/plugins/streaming/StepStreamingResourceManager.java +++ b/step-controller/step-controller-base-plugins/src/main/java/step/plugins/streaming/StepStreamingResourceManager.java @@ -1,24 +1,36 @@ package step.plugins.streaming; +import org.bson.types.ObjectId; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import step.attachments.AttachmentMeta; +import step.attachments.SkippedAttachmentMeta; +import step.attachments.StreamingAttachmentMeta; import step.constants.LiveReportingConstants; import step.core.GlobalContext; import step.core.access.User; import step.core.deployment.AuthorizationException; +import step.core.execution.ExecutionContext; +import step.core.objectenricher.ObjectEnricher; import step.core.objectenricher.ObjectHookRegistry; import step.framework.server.Session; import step.framework.server.access.AuthorizationManager; -import step.resources.StreamingResourceContentProvider; -import step.streaming.common.*; +import step.resources.AttachmentStorage; +import step.streaming.common.QuotaExceededException; +import step.streaming.common.StreamingResourceMetadata; +import step.streaming.common.StreamingResourceReference; +import step.streaming.common.StreamingResourceUploadContext; +import step.streaming.common.StreamingResourceUploadContexts; import step.streaming.server.DefaultStreamingResourceManager; import step.streaming.server.StreamingResourcesStorageBackend; +import java.io.ByteArrayInputStream; import java.io.IOException; import java.io.InputStream; +import java.util.Objects; import java.util.function.Function; -public class StepStreamingResourceManager extends DefaultStreamingResourceManager implements StreamingResourceContentProvider { +public class StepStreamingResourceManager extends DefaultStreamingResourceManager implements AttachmentStorage { private static final Logger logger = LoggerFactory.getLogger(StepStreamingResourceManager.class); static final String ATTRIBUTE_STEP_SESSION = "stepSession"; @@ -139,8 +151,73 @@ public void checkDownloadPermission(StreamingResource resource, Session st } } + /* The following methods implement the AttachmentStorage interface */ + @Override - public InputStream getResourceContentStream(String resourceId) throws Exception { + public InputStream getAttachmentStream(String resourceId) throws Exception { return openStream(resourceId, 0, getStatus(resourceId).getCurrentSize()); } + + @Override + public AttachmentMeta saveAttachment(Object executionContext, byte[] content, String filename, String mimeType) { + ExecutionContext context = (ExecutionContext) Objects.requireNonNull(executionContext); + StreamingResourceUploadContexts uploadContexts = context.get(StreamingResourceUploadContexts.class); + + if (uploadContexts == null) { + return new SkippedAttachmentMeta(filename, mimeType, "UNEXPECTED: no StreamingResourceUploadContexts found in execution context"); + } + + // We don't need to react to any events, but we need an UploadContext to convey relevant contextual information about the attachment. + StreamingResourceUploadContext uploadContext = new StreamingResourceUploadContext(); + ObjectEnricher enricher = context.getObjectEnricher(); + if (enricher != null) { + uploadContext.getAttributes().put(LiveReportingConstants.ACCESSCONTROL_ENRICHER, enricher); + } + uploadContext.getAttributes().put(LiveReportingConstants.CONTEXT_EXECUTION_ID, context.getExecutionId()); + uploadContext.getAttributes().put(LiveReportingConstants.CONTEXT_VARIABLES_MANAGER, context.getVariablesManager()); + uploadContext.getAttributes().put(LiveReportingConstants.CONTEXT_REPORT_NODE, context.getCurrentReportNode()); + uploadContexts.registerContext(uploadContext); + + try { + return createAttachmentFromContent(content, filename, mimeType, uploadContext.contextId); + } catch (Exception e) { + return new SkippedAttachmentMeta(filename, mimeType, e.getMessage()); + } finally { + uploadContexts.unregisterContext(uploadContext); + } + } + + public AttachmentMeta createAttachmentFromContent(byte[] content, String filename, String mimeType, String uploadContextId) throws QuotaExceededException, IOException { + mimeType = sanitizeMimeType(mimeType); + boolean supportsLineAccess = isLineAccessSupported(mimeType); + StreamingResourceMetadata metadata = new StreamingResourceMetadata(filename, mimeType, supportsLineAccess); + String resourceId = registerNewResource(metadata, uploadContextId); + StreamingAttachmentMeta meta = new StreamingAttachmentMeta(new ObjectId(resourceId), filename, mimeType); + // Saving will be completely finished, in a single call, because all data is already present from the beginning. + writeChunk(resourceId, new ByteArrayInputStream(content), true); + var status = markCompleted(resourceId); + meta.setCurrentNumberOfLines(status.getNumberOfLines()); + meta.setCurrentSize(status.getCurrentSize()); + meta.setStatus(StreamingAttachmentMeta.Status.COMPLETED); + return meta; + } + + private static String sanitizeMimeType(String mimeType) { + if (mimeType == null) { + // fallback if no mimeType was given + return "application/octet-stream"; + } + mimeType = mimeType.trim(); // just in case + if (!mimeType.matches("^[a-zA-Z0-9!#$&^_.+-]+/[a-zA-Z0-9!#$&^_.+-]+$")) { + logger.warn("Invalid mime type \"{}\", replacing with generic application/octet-stream", mimeType); + return "application/octet-stream"; + } + return mimeType; + } + + private static boolean isLineAccessSupported(String mimeType) { + // Very simple heuristic for now, should catch > 90% of the useful cases + // TODO: we can add a few other well-known textual formats if needed + return mimeType.startsWith("text/"); + } } diff --git a/step-controller/step-controller-base-plugins/src/main/java/step/plugins/streaming/StreamingResourcesControllerPlugin.java b/step-controller/step-controller-base-plugins/src/main/java/step/plugins/streaming/StreamingResourcesControllerPlugin.java index c707ff738..83a71b7ce 100644 --- a/step-controller/step-controller-base-plugins/src/main/java/step/plugins/streaming/StreamingResourcesControllerPlugin.java +++ b/step-controller/step-controller-base-plugins/src/main/java/step/plugins/streaming/StreamingResourcesControllerPlugin.java @@ -1,6 +1,5 @@ package step.plugins.streaming; -import ch.exense.commons.app.Configuration; import jakarta.servlet.http.HttpSession; import jakarta.websocket.Endpoint; import jakarta.websocket.HandshakeResponse; @@ -10,7 +9,6 @@ import org.slf4j.LoggerFactory; import step.constants.LiveReportingConstants; import step.core.GlobalContext; -import step.core.controller.StepControllerPlugin; import step.core.deployment.ObjectHookControllerPlugin; import step.core.execution.AbstractExecutionEngineContext; import step.core.execution.ExecutionContext; @@ -20,7 +18,6 @@ import step.engine.plugins.AbstractExecutionEnginePlugin; import step.engine.plugins.ExecutionEnginePlugin; import step.resources.ResourceManagerControllerPlugin; -import step.resources.StreamingResourceContentProvider; import step.streaming.common.StreamingResourceUploadContexts; import step.streaming.server.FilesystemStreamingResourcesStorageBackend; import step.streaming.server.StreamingResourceManager; @@ -66,6 +63,7 @@ public void serverStart(GlobalContext context) throws Exception { manager = new StepStreamingResourceManager(context, catalog, storage, referenceProducer, uploadContexts); context.put(StepStreamingResourceManager.class, manager); + context.setAttachmentStorage(manager); context.getServiceRegistrationCallback().registerService(StreamingResourceServices.class); context.getServiceRegistrationCallback().registerWebsocketEndpoint(makeUploadConfig(manager)); @@ -115,8 +113,8 @@ public ExecutionEnginePlugin getExecutionEnginePlugin() { return new AbstractExecutionEnginePlugin() { @Override public void initializeExecutionEngineContext(AbstractExecutionEngineContext parentContext, ExecutionEngineContext executionEngineContext) { - // required by AP reporting for fetching attachments - executionEngineContext.put(StreamingResourceContentProvider.class, manager); + // required by AP reporting for fetching attachments; FIXME -- this is now redundant + executionEngineContext.setAttachmentStorage(manager); } @Override @@ -126,6 +124,8 @@ public void initializeExecutionContext(ExecutionEngineContext executionEngineCon executionContext.put(LiveReportingConstants.STREAMING_WEBSOCKET_UPLOAD_PATH, UPLOAD_PATH); // Note: this URL is also used by the measures streaming to determine the hostname. executionContext.put(LiveReportingConstants.LIVEREPORTING_CONTROLLER_URL, controllerUrl); + // FIXME: This is now also redundant... + executionContext.setAttachmentStorage(manager); } diff --git a/step-core/src/main/java/step/attachments/FileResolver.java b/step-core/src/main/java/step/attachments/FileResolver.java index 8b2ff6632..0bcf2fa7c 100644 --- a/step-core/src/main/java/step/attachments/FileResolver.java +++ b/step-core/src/main/java/step/attachments/FileResolver.java @@ -18,18 +18,18 @@ ******************************************************************************/ package step.attachments; -import java.io.Closeable; -import java.io.File; -import java.io.IOException; - import org.bson.types.ObjectId; import step.resources.Resource; import step.resources.ResourceManager; import step.resources.ResourceRevisionFileHandle; +import java.io.Closeable; +import java.io.File; +import java.io.IOException; + public class FileResolver { - public static final String ATTACHMENT_PREFIX = "attachment:"; + public static final String ATTACHMENT_PREFIX_OBSOLETE = "attachment:"; public static final String RESOURCE_PREFIX = "resource:"; public static final String RESOURCE_PATH_SEPARATOR = ":"; @@ -46,9 +46,9 @@ public ResourceManager getResourceManager() { public File resolve(String path) { File file; - if (path.startsWith(ATTACHMENT_PREFIX)) { - throw new RuntimeException("Attachments have been migrated to the ResourceManager. The reference " + path + - " isn't valid anymore. Your attachment should be migrated to the ResourceManager."); + if (path.startsWith(ATTACHMENT_PREFIX_OBSOLETE)) { + throw new RuntimeException("Attachments have been migrated to use the AttachmentStorage. The reference " + path + + " isn't valid anymore. Please update your code to use the AttachmentStorage instead."); } else if (path.startsWith(RESOURCE_PREFIX)) { file = getResourceRevisionFileHandleForPath(path).getResourceFile(); } else { @@ -114,7 +114,7 @@ protected static String extractResourceSubPath(String path) { public FileHandle resolveFileHandle(String path) { File file; ResourceRevisionFileHandle resourceRevisionFileHandle; - if (path.startsWith(ATTACHMENT_PREFIX)) { + if (path.startsWith(ATTACHMENT_PREFIX_OBSOLETE)) { throw new RuntimeException("Attachments have been migrated to the ResourceManager. The reference " + path + " isn't valid anymore. Your attachment should be migrated to the ResourceManager."); } else if (path.startsWith(RESOURCE_PREFIX)) { diff --git a/step-core/src/main/java/step/core/AbstractStepContext.java b/step-core/src/main/java/step/core/AbstractStepContext.java index 9f6724aa5..e9d50da93 100644 --- a/step-core/src/main/java/step/core/AbstractStepContext.java +++ b/step-core/src/main/java/step/core/AbstractStepContext.java @@ -25,7 +25,13 @@ import step.core.dynamicbeans.DynamicBeanResolver; import step.core.dynamicbeans.DynamicValueResolver; import step.expressions.ExpressionHandler; -import step.resources.*; +import step.resources.AttachmentStorage; +import step.resources.InMemoryResourceAccessor; +import step.resources.InMemoryResourceRevisionAccessor; +import step.resources.LayeredResourceManager; +import step.resources.LocalAttachmentStorage; +import step.resources.LocalResourceManagerImpl; +import step.resources.ResourceManager; import java.io.File; import java.io.IOException; @@ -42,25 +48,31 @@ public abstract class AbstractStepContext extends AbstractContext { private LoadingCache fileResolverCache; // Keep track of the default resource manager created at initialization of the context private LocalResourceManagerImpl localResourceManager; + // Similar to the resource manager, attachmentStorage can be shared by multiple context, but only one of them owns it. + private AttachmentStorage attachmentStorage; + private AttachmentStorage ownedAttachmentStorage; protected void setDefaultAttributes() { expressionHandler = new ExpressionHandler(); dynamicBeanResolver = new DynamicBeanResolver(new DynamicValueResolver(expressionHandler)); // Create a local resource manager in a dedicated folder per default - localResourceManager = new LocalResourceManagerImpl(getContextFolderAsFile(), new InMemoryResourceAccessor(), new InMemoryResourceRevisionAccessor()); + localResourceManager = new LocalResourceManagerImpl(getContextFolderAsFile("resources"), new InMemoryResourceAccessor(), new InMemoryResourceRevisionAccessor()); setResourceManager(localResourceManager); + ownedAttachmentStorage = new LocalAttachmentStorage(getContextFolderAsFile("attachments")); + setAttachmentStorage(ownedAttachmentStorage); } - private File getContextFolderAsFile() { + private File getContextFolderAsFile(String suffix) { String tmpPath = System.getProperty("java.io.tmpdir"); tmpPath = (tmpPath.endsWith(File.separator)) ? tmpPath : tmpPath + File.separator; - String dirName = tmpPath + "stepContext_" + getClass().getSimpleName() + "_" + contextId; + String dirName = tmpPath + "stepContext_" + getClass().getSimpleName() + "_" + contextId + "_" + suffix; return new File(dirName); } protected void useAllAttributesFromParentContext(AbstractStepContext parentContext) { ResourceManager resourceManager = new LayeredResourceManager(parentContext.getResourceManager(), true); setResourceManager(resourceManager); + setAttachmentStorage(parentContext.getAttachmentStorage()); expressionHandler = parentContext.getExpressionHandler(); dynamicBeanResolver = parentContext.getDynamicBeanResolver(); } @@ -90,6 +102,21 @@ public void setResourceManager(ResourceManager resourceManager) { updateFileResolver(); } + public AttachmentStorage getAttachmentStorage() { + return attachmentStorage; + } + + public void setAttachmentStorage(AttachmentStorage attachmentStorage) { + if (attachmentStorage == this.attachmentStorage) { + System.err.println("FIXME: redundant setting of attachment storage in " + this.getClass().getSimpleName()); + return; + } + if (this.attachmentStorage != null) { + throw new IllegalStateException("BUG: attachmentStorage must not be set more than once"); + } + this.attachmentStorage = attachmentStorage; + } + public FileResolver getFileResolver() { return fileResolver; } @@ -115,10 +142,13 @@ public void close() throws IOException { try { super.close(); } finally { - // Cleanup the default resource manager + // Cleanup the default resource manager, and attachment storage if (localResourceManager != null) { localResourceManager.cleanup(); } + if (ownedAttachmentStorage != null) { + ownedAttachmentStorage.cleanupIfNeeded(); + } } } } diff --git a/step-core/src/main/java/step/resources/AttachmentStorage.java b/step-core/src/main/java/step/resources/AttachmentStorage.java new file mode 100644 index 000000000..bdf7901c7 --- /dev/null +++ b/step-core/src/main/java/step/resources/AttachmentStorage.java @@ -0,0 +1,14 @@ +package step.resources; + +import step.attachments.AttachmentMeta; + +import java.io.InputStream; + +public interface AttachmentStorage { + InputStream getAttachmentStream(String attachmentId) throws Exception; + + AttachmentMeta saveAttachment(Object executionContext, byte[] content, String filename, String mimeType); + + default void cleanupIfNeeded() { + } +} diff --git a/step-core/src/main/java/step/resources/LocalAttachmentStorage.java b/step-core/src/main/java/step/resources/LocalAttachmentStorage.java new file mode 100644 index 000000000..e85f75e22 --- /dev/null +++ b/step-core/src/main/java/step/resources/LocalAttachmentStorage.java @@ -0,0 +1,93 @@ +package step.resources; + +import org.apache.commons.io.FileUtils; +import org.bson.types.ObjectId; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import step.attachments.AttachmentMeta; +import step.attachments.SkippedAttachmentMeta; + +import java.io.File; +import java.io.FileNotFoundException; +import java.io.IOException; +import java.io.InputStream; +import java.nio.file.DirectoryStream; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.Objects; + +/** + * Local (i.e. "non-Step-infrastructure") implementation of Attachment Storage, + * which is used for instance when running Unit Tests, Step JUnit runner, or local + * executions using the Step CLI. + *

+ * It's a really simple implementation that stores files in the given root directory. + * In theory, we could use only the IDs, but we append the original filename to remain + * at least somewhat user-friendly. Another option would be to create a subdirectory + * per ID, then put a single file in each directory. + */ +public class LocalAttachmentStorage implements AttachmentStorage { + private static final Logger logger = LoggerFactory.getLogger(LocalAttachmentStorage.class); + private final File rootDirectory; + + public LocalAttachmentStorage(File rootDirectory) { + this.rootDirectory = Objects.requireNonNull(rootDirectory); + if (rootDirectory.exists()) { + throw new IllegalArgumentException("Attachments storage root directory already exists: " + rootDirectory.getAbsolutePath()); + } + if (!rootDirectory.mkdirs()) { + throw new RuntimeException("Cannot create attachments storage directory: " + rootDirectory.getAbsolutePath()); + } + logger.info("Attachment storage initialized, directory={}", rootDirectory.getAbsolutePath()); + } + + @Override + public InputStream getAttachmentStream(String attachmentId) throws Exception { + Path rootPath = rootDirectory.toPath(); + String searchPattern = attachmentId + "_*"; + + // Open a directory stream filtering only for files that start with our ID + try (DirectoryStream stream = Files.newDirectoryStream(rootPath, searchPattern)) { + for (Path entry : stream) { + // Since IDs are guaranteed unique, we can safely return an InputStream + // for the very first match we find. + return Files.newInputStream(entry); + } + } + + // If the loop finishes without returning, the file doesn't exist + throw new FileNotFoundException("Could not find attachment with ID: " + attachmentId); + } + + @Override + public AttachmentMeta saveAttachment(Object executionContext, byte[] content, String filename, String mimeType) { + // executionContext is not required nor used in this implementation. + ObjectId id = new ObjectId(); + // This stores to ${ID}_${FILENAME}. + // We could also use an intermediate directory, ie. ${ID}/${FILENAME}, which will + // somewhat facilitate lookup but requires twice the number of inodes. + String storedFilename = id.toHexString() + "_" + filename; + Path targetPath = rootDirectory.toPath().resolve(storedFilename); + try { + // Resolve the path and write the byte array directly + Files.write(targetPath, content); + logger.debug("Wrote {} bytes to {}", content.length, targetPath); + AttachmentMeta meta = new AttachmentMeta(id); + meta.setName(filename); + meta.setMimeType(mimeType); + return meta; + } catch (IOException e) { + logger.error("Failed to save attachment {} to {}", targetPath, e); + return new SkippedAttachmentMeta(filename, mimeType, e.getMessage()); + } + } + + public void cleanupIfNeeded() { + try { + logger.info("Cleanup attachment storage: {}", rootDirectory.getAbsolutePath()); + FileUtils.deleteDirectory(rootDirectory); + } catch (Exception e) { + logger.error("Error while cleaning up directory {}", rootDirectory.getAbsolutePath(), e); + } + } +} diff --git a/step-core/src/main/java/step/resources/StreamingResourceContentProvider.java b/step-core/src/main/java/step/resources/StreamingResourceContentProvider.java deleted file mode 100644 index bd47a55b0..000000000 --- a/step-core/src/main/java/step/resources/StreamingResourceContentProvider.java +++ /dev/null @@ -1,10 +0,0 @@ -package step.resources; - -import java.io.InputStream; - -/** - * This class serves as a simple and targeted abstraction layer to get access to the content of streamed resources. - */ -public interface StreamingResourceContentProvider { - InputStream getResourceContentStream(String resourceId) throws Exception; -} diff --git a/step-plans/step-plans-base-artefacts/src/main/java/step/artefacts/handlers/ExportHandler.java b/step-plans/step-plans-base-artefacts/src/main/java/step/artefacts/handlers/ExportHandler.java index c6cf648dc..c134a5275 100644 --- a/step-plans/step-plans-base-artefacts/src/main/java/step/artefacts/handlers/ExportHandler.java +++ b/step-plans/step-plans-base-artefacts/src/main/java/step/artefacts/handlers/ExportHandler.java @@ -18,20 +18,20 @@ ******************************************************************************/ package step.artefacts.handlers; -import java.io.File; -import java.io.IOException; -import java.util.List; -import java.util.regex.Pattern; - -import com.google.common.io.Files; - import step.artefacts.Export; -import step.artefacts.reports.EchoReportNode; import step.attachments.AttachmentMeta; +import step.attachments.SkippedAttachmentMeta; import step.core.artefacts.handlers.ArtefactHandler; import step.core.artefacts.reports.ReportNode; import step.core.artefacts.reports.ReportNodeStatus; -import step.resources.ResourceManager; +import step.resources.AttachmentStorage; + +import java.io.File; +import java.io.FileOutputStream; +import java.io.InputStream; +import java.io.OutputStream; +import java.util.List; +import java.util.regex.Pattern; public class ExportHandler extends ArtefactHandler { @@ -70,18 +70,24 @@ private void export(ReportNode node, Export testArtefact) { if (file.isDirectory()) { List list = (List) value; for (Object object : list) { - if (object instanceof AttachmentMeta) { - AttachmentMeta attachmentMeta = (AttachmentMeta) object; - ResourceManager resourceManager = context.getResourceManager(); - File fileToCopy = resourceManager.getResourceFile(attachmentMeta.getId().toString()).getResourceFile(); + if (object instanceof AttachmentMeta attachmentMeta) { + if (attachmentMeta instanceof SkippedAttachmentMeta) { + continue; + } // Export only if the file name matches the defined filter - if (filter == null || filter.matcher(fileToCopy.getName()).matches()) { + String fileName = attachmentMeta.getName(); + if (filter == null || filter.matcher(fileName).matches()) { + AttachmentStorage storage = context.getAttachmentStorage(); + if (storage == null) { + throw new RuntimeException("Attachment storage is null"); + } String filenamePrefix = testArtefact.getPrefix() != null ? testArtefact.getPrefix().get() : ""; - File target = new File(file + "/" + filenamePrefix + fileToCopy.getName()); - try { - Files.copy(fileToCopy, target); - } catch (IOException e) { - throw new RuntimeException("Error while copying file " + fileToCopy.getName() + " to " + target.getAbsolutePath()); + File target = new File(file + "/" + filenamePrefix + fileName); + try (InputStream in = storage.getAttachmentStream(attachmentMeta.getId().toString()); + OutputStream out = new FileOutputStream(target)) { + in.transferTo(out); + } catch (Exception e) { + throw new RuntimeException("Error while saving " + fileName + " to " + target.getAbsolutePath()); } } } diff --git a/step-plans/step-plans-base-artefacts/src/main/java/step/core/artefacts/reports/junitxml/JUnitXmlReportBuilder.java b/step-plans/step-plans-base-artefacts/src/main/java/step/core/artefacts/reports/junitxml/JUnitXmlReportBuilder.java index 9bc122959..2edaaaa93 100644 --- a/step-plans/step-plans-base-artefacts/src/main/java/step/core/artefacts/reports/junitxml/JUnitXmlReportBuilder.java +++ b/step-plans/step-plans-base-artefacts/src/main/java/step/core/artefacts/reports/junitxml/JUnitXmlReportBuilder.java @@ -26,20 +26,23 @@ import org.slf4j.LoggerFactory; import step.attachments.AttachmentMeta; import step.attachments.SkippedAttachmentMeta; -import step.attachments.StreamingAttachmentMeta; import step.core.artefacts.reports.ReportNodeAccessor; import step.core.execution.ExecutionEngineContext; -import step.reporting.Junit4ReportConfig; import step.reporting.JUnit4ReportWriter; import step.reporting.JUnitReport; +import step.reporting.Junit4ReportConfig; import step.reporting.ReportMetadata; -import step.resources.ResourceManager; -import step.resources.ResourceRevisionContent; -import step.resources.StreamingResourceContentProvider; - -import java.io.*; +import step.resources.AttachmentStorage; + +import java.io.ByteArrayOutputStream; +import java.io.File; +import java.io.FileOutputStream; +import java.io.IOException; +import java.io.InputStream; +import java.io.OutputStreamWriter; import java.util.List; import java.util.Map; +import java.util.Objects; public class JUnitXmlReportBuilder { @@ -48,15 +51,13 @@ public class JUnitXmlReportBuilder { public static final String DEFAULT_ATTACHMETS_SUBFOLDER = "attachments"; private final ReportNodeAccessor reportNodeAccessor; - private final ResourceManager attachmentsResourceManager; - private final StreamingResourceContentProvider streamingResourceContentProvider; + private final AttachmentStorage attachmentStorage; private final Configuration configuration; public JUnitXmlReportBuilder(ExecutionEngineContext executionEngineContext) { - this.reportNodeAccessor = executionEngineContext.getReportNodeAccessor(); - this.attachmentsResourceManager = executionEngineContext.getResourceManager(); - this.streamingResourceContentProvider = executionEngineContext.get(StreamingResourceContentProvider.class); - this.configuration = executionEngineContext.getConfiguration(); + this.reportNodeAccessor = Objects.requireNonNull(executionEngineContext.getReportNodeAccessor()); + this.attachmentStorage = Objects.requireNonNull(executionEngineContext.getAttachmentStorage()); + this.configuration = Objects.requireNonNull(executionEngineContext.getConfiguration()); } /** @@ -91,7 +92,7 @@ public JUnitReport buildJunitZipReport(List executionIds, Boolean includ .setAddAttachments(includeAttachments != null ? includeAttachments : false) .setAttachmentSubfolder(attachmentsSubfolder) .setAttachmentRootFolder(attachmentsRootFolder) - .setAttachmentResourceManager(attachmentsResourceManager) + .setAttachmentStorage(attachmentStorage) .setAddLinksToStepFrontend(true) .setServerConfiguration(configuration) .createConfig(); @@ -132,15 +133,9 @@ public JUnitReport buildJunitZipReport(List executionIds, Boolean includ ObjectId attachmentId = attachment.getId(); InputStream resourceStream; try { - if (attachment instanceof StreamingAttachmentMeta) { - // streaming attachment - resourceStream = streamingResourceContentProvider.getResourceContentStream(attachmentId.toString()); - } else { - // "regular" attachment - resourceStream = attachmentsResourceManager.getResourceContent(attachmentId.toString()).getResourceStream(); - } + resourceStream = attachmentStorage.getAttachmentStream(attachmentId.toString()); } catch (Exception e) { - log.warn("Unable to obtain attachment content for attachment '{}' (id={}), not adding to report", attachment.getName(), attachmentId); + log.warn("Unable to obtain attachment content for attachment '{}' (id={}), not adding to report", attachment.getName(), attachmentId, e); continue; } File attachmentOutFile = new File(testCaseSubdir, attachment.getName()); diff --git a/step-plans/step-plans-base-artefacts/src/main/java/step/reporting/JUnit4ReportWriter.java b/step-plans/step-plans-base-artefacts/src/main/java/step/reporting/JUnit4ReportWriter.java index fa4a1c51e..79ebbf279 100644 --- a/step-plans/step-plans-base-artefacts/src/main/java/step/reporting/JUnit4ReportWriter.java +++ b/step-plans/step-plans-base-artefacts/src/main/java/step/reporting/JUnit4ReportWriter.java @@ -24,15 +24,21 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; import step.artefacts.TestSet; -import step.attachments.SkippedAttachmentMeta; -import step.attachments.StreamingAttachmentMeta; -import step.reports.CustomReportType; import step.attachments.AttachmentMeta; -import step.core.artefacts.reports.*; +import step.attachments.SkippedAttachmentMeta; +import step.core.artefacts.reports.ReportNode; +import step.core.artefacts.reports.ReportNodeStatus; +import step.core.artefacts.reports.ReportNodeVisitorEventHandler; +import step.core.artefacts.reports.ReportTreeAccessor; +import step.core.artefacts.reports.ReportTreeVisitor; import step.core.artefacts.reports.ReportTreeVisitor.ReportNodeEvent; -import step.resources.ResourceRevisionFileHandle; +import step.reports.CustomReportType; -import java.io.*; +import java.io.BufferedWriter; +import java.io.File; +import java.io.FileWriter; +import java.io.IOException; +import java.io.Writer; import java.net.InetAddress; import java.net.UnknownHostException; import java.text.DecimalFormat; @@ -41,7 +47,10 @@ import java.time.LocalDateTime; import java.time.ZoneId; import java.time.format.DateTimeFormatter; -import java.util.*; +import java.util.ArrayList; +import java.util.List; +import java.util.Locale; +import java.util.Stack; import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicLong; import java.util.concurrent.atomic.AtomicReference; @@ -344,7 +353,7 @@ private String generateLinkToStep(String executionId) { } private void writeAttachmentTag(String testCaseId, AttachmentMeta attachment, Writer writer, String reportFileName) { - if (junit4ReportConfig.isAddAttachments() && junit4ReportConfig.getAttachmentResourceManager() != null) { + if (junit4ReportConfig.isAddAttachments() && junit4ReportConfig.getAttachmentStorage() != null) { // Writes tags similar to the following example to the report: // [[ATTACHMENT|screenshots/dashboard.png]] if (attachment instanceof SkippedAttachmentMeta) { diff --git a/step-plans/step-plans-base-artefacts/src/main/java/step/reporting/Junit4ReportConfig.java b/step-plans/step-plans-base-artefacts/src/main/java/step/reporting/Junit4ReportConfig.java index d4bdd2439..4d8465260 100644 --- a/step-plans/step-plans-base-artefacts/src/main/java/step/reporting/Junit4ReportConfig.java +++ b/step-plans/step-plans-base-artefacts/src/main/java/step/reporting/Junit4ReportConfig.java @@ -19,7 +19,7 @@ package step.reporting; import ch.exense.commons.app.Configuration; -import step.resources.ResourceManager; +import step.resources.AttachmentStorage; public class Junit4ReportConfig { @@ -27,16 +27,16 @@ public class Junit4ReportConfig { private final boolean addLinksToStepFrontend; private String attachmentsSubfolder; private String attachmentsRootFolder; - private ResourceManager attachmentResourceManager; + private AttachmentStorage attachmentStorage; private Configuration serverConfiguration; private Junit4ReportConfig(String attachmentsSubfolder, String attachmentsRootFolder, - ResourceManager attachmentResourceManager, + AttachmentStorage attachmentStorage, boolean addAttachments, boolean addLinksToStepFrontend, Configuration serverConfiguration) { this.attachmentsSubfolder = attachmentsSubfolder; this.attachmentsRootFolder = attachmentsRootFolder; - this.attachmentResourceManager = attachmentResourceManager; + this.attachmentStorage = attachmentStorage; this.addAttachments = addAttachments; this.addLinksToStepFrontend = addLinksToStepFrontend; this.serverConfiguration = serverConfiguration; @@ -50,8 +50,8 @@ public String getAttachmentsRootFolder() { return attachmentsRootFolder; } - public ResourceManager getAttachmentResourceManager() { - return attachmentResourceManager; + public AttachmentStorage getAttachmentStorage() { + return attachmentStorage; } public boolean isAddAttachments() { @@ -71,7 +71,7 @@ public static class Builder { private boolean addLinksToStepFrontend = true; private String attachmentSubfolder; private String attachmentRootFolder; - private ResourceManager attachmentResourceManager; + private AttachmentStorage attachmentStorage; private Configuration serverConfiguration; public Builder setAttachmentSubfolder(String attachmentSubfolder) { @@ -84,8 +84,8 @@ public Builder setAttachmentRootFolder(String attachmentRootFolder) { return this; } - public Builder setAttachmentResourceManager(ResourceManager attachmentResourceManager) { - this.attachmentResourceManager = attachmentResourceManager; + public Builder setAttachmentStorage(AttachmentStorage attachmentStorage) { + this.attachmentStorage = attachmentStorage; return this; } @@ -106,7 +106,7 @@ public Builder setServerConfiguration(Configuration serverConfiguration) { public Junit4ReportConfig createConfig() { return new Junit4ReportConfig( - attachmentSubfolder, attachmentRootFolder, attachmentResourceManager, + attachmentSubfolder, attachmentRootFolder, attachmentStorage, addAttachments, addLinksToStepFrontend, serverConfiguration ); } diff --git a/step-plans/step-plans-base-artefacts/src/test/java/step/artefacts/handlers/ExportHandlerTest.java b/step-plans/step-plans-base-artefacts/src/test/java/step/artefacts/handlers/ExportHandlerTest.java index 459370ac0..36db450ca 100644 --- a/step-plans/step-plans-base-artefacts/src/test/java/step/artefacts/handlers/ExportHandlerTest.java +++ b/step-plans/step-plans-base-artefacts/src/test/java/step/artefacts/handlers/ExportHandlerTest.java @@ -18,16 +18,10 @@ ******************************************************************************/ package step.artefacts.handlers; -import java.io.File; -import java.io.IOException; -import java.nio.charset.Charset; - -import org.junit.Test; - -import com.google.common.io.Files; - import ch.exense.commons.io.FileHelper; +import com.google.common.io.Files; import org.junit.Assert; +import org.junit.Test; import step.artefacts.BaseArtefactPlugin; import step.artefacts.Export; import step.artefacts.Sequence; @@ -37,6 +31,10 @@ import step.core.plans.Plan; import step.core.plans.builder.PlanBuilder; +import java.io.File; +import java.io.IOException; +import java.nio.charset.Charset; + public class ExportHandlerTest { @Test diff --git a/step-plans/step-plans-core/src/main/java/step/core/execution/ExecutionEngineRunner.java b/step-plans/step-plans-core/src/main/java/step/core/execution/ExecutionEngineRunner.java index 1f1be9fd1..edf99563c 100644 --- a/step-plans/step-plans-core/src/main/java/step/core/execution/ExecutionEngineRunner.java +++ b/step-plans/step-plans-core/src/main/java/step/core/execution/ExecutionEngineRunner.java @@ -24,13 +24,16 @@ import step.core.accessors.AbstractOrganizableObject; import step.core.artefacts.AbstractArtefact; import step.core.artefacts.handlers.ArtefactHandlerManager; -import step.core.artefacts.reports.ReportNode; import step.core.artefacts.reports.ParentSource; +import step.core.artefacts.reports.ReportNode; import step.core.artefacts.reports.ReportNodeStatus; import step.core.artefacts.reports.aggregated.ReportNodeTimeSeries; import step.core.artefacts.reports.resolvedplan.ResolvedPlanBuilder; import step.core.artefacts.reports.resolvedplan.ResolvedPlanNode; -import step.core.execution.model.*; +import step.core.execution.model.Execution; +import step.core.execution.model.ExecutionParameters; +import step.core.execution.model.ExecutionStatus; +import step.core.execution.model.ReportExport; import step.core.plans.Plan; import step.core.plans.PlanAccessor; import step.core.plans.runner.PlanRunnerResult; @@ -45,7 +48,11 @@ import step.functions.Function; import step.functions.accessor.FunctionAccessor; -import java.util.*; +import java.util.ArrayList; +import java.util.Collection; +import java.util.List; +import java.util.Objects; +import java.util.Optional; import java.util.function.Consumer; import java.util.stream.Collectors; @@ -262,7 +269,7 @@ private ReportNode execute(Plan plan, ReportNode rootReportNode) throws Provisio private PlanRunnerResult result(String executionId) { return new PlanRunnerResult(executionId, executionContext.getExecutionAccessor(), executionContext.getReportNodeAccessor(), - executionContext.getResourceManager()); + executionContext.getAttachmentStorage()); } private ImportResult importPlan(ExecutionContext context) { diff --git a/step-plans/step-plans-core/src/main/java/step/core/miscellaneous/ReportNodeAttachmentManager.java b/step-plans/step-plans-core/src/main/java/step/core/miscellaneous/ReportNodeAttachmentManager.java index 27252708b..5deac14e8 100644 --- a/step-plans/step-plans-core/src/main/java/step/core/miscellaneous/ReportNodeAttachmentManager.java +++ b/step-plans/step-plans-core/src/main/java/step/core/miscellaneous/ReportNodeAttachmentManager.java @@ -18,31 +18,28 @@ ******************************************************************************/ package step.core.miscellaneous; -import java.io.BufferedOutputStream; -import java.io.IOException; -import java.io.PrintWriter; -import java.io.StringWriter; - -import org.bson.types.ObjectId; import org.slf4j.Logger; import org.slf4j.LoggerFactory; - import step.attachments.AttachmentMeta; import step.attachments.SkippedAttachmentMeta; import step.core.artefacts.reports.ReportNode; import step.core.execution.ExecutionContext; -import step.core.variables.UndefinedVariableException; -import step.core.variables.VariablesManager; -import step.resources.*; +import step.resources.AttachmentStorage; +import step.resources.InvalidResourceFormatException; +import step.resources.LayeredResourceManager; +import step.resources.Resource; +import step.resources.ResourceManager; +import step.resources.ResourceRevisionContainer; + +import java.io.BufferedOutputStream; +import java.io.IOException; +import java.io.PrintWriter; +import java.io.StringWriter; // TODO refactor this class to remove the ExecutionContext dependency public class ReportNodeAttachmentManager { - public static String QUOTA_COUNT_VARNAME = "tec.quota.attachments.count"; - - public static String QUOTA_VARNAME = "tec.quota.attachments"; - private static final Logger logger = LoggerFactory.getLogger(ReportNodeAttachmentManager.class); private ResourceManager resourceManager; @@ -74,29 +71,6 @@ public ReportNodeAttachmentManager(ResourceManager resourceManager) { this.resourceManager = resourceManager; } - private boolean checkAndUpateAttachmentQuota() { - synchronized (context) { - VariablesManager varManager = context.getVariablesManager(); - - Integer count; - try { - count = varManager.getVariableAsInteger(QUOTA_COUNT_VARNAME) + 1; - varManager.updateVariable(QUOTA_COUNT_VARNAME, count); - } catch (UndefinedVariableException e) { - count = 1; - varManager.putVariable(context.getReport(), QUOTA_COUNT_VARNAME, count); - } - - Integer quota = varManager.getVariableAsInteger(QUOTA_VARNAME, 100); - - if (quota == count) { - logger.info(context.getExecutionId().toString() + ". Maximum number of attachment (" + quota + ") reached. Next attachments will be skipped."); - } - - return quota >= count; - } - } - private static byte[] exceptionToAttachment(Throwable e) { StringWriter w = new StringWriter(); e.printStackTrace(new PrintWriter(w)); @@ -112,28 +86,22 @@ public void attach(byte[] content, String filename, String mimeType, ReportNode } public AttachmentMeta createAttachment(byte[] content, String filename, String mimeType) { - if (checkAndUpateAttachmentQuota()) { - return createResourceWithoutQuotaCheck(ResourceManager.RESOURCE_TYPE_ATTACHMENT, content, filename, mimeType); - } else { - String message = String.format("The attachment %s has been skipped because the execution generated more than" + - " the maximum number of attachments permitted. This quota can be changed by setting the variable %s with an higher value.", filename, QUOTA_VARNAME); - if (logger.isDebugEnabled()) { - logger.debug("Execution {} - {}", context.getExecutionId(), message); - } - return new SkippedAttachmentMeta(filename, mimeType, message); + AttachmentStorage attachmentStorage = context.getAttachmentStorage(); + if (attachmentStorage == null) { + logger.error("{}: Unable to create attachment {} because attachmentStorage is not set", context, filename); + return new SkippedAttachmentMeta(filename, mimeType, "attachmentStorage not defined"); } + return attachmentStorage.saveAttachment(context, content, filename, mimeType); } + // By now, this method is only used for creating resources of type RESOURCE_TYPE_TEMP. public AttachmentMeta createResourceWithoutQuotaCheck(String resourceType, byte[] content, String filename, String mimeType) { + if (!ResourceManager.RESOURCE_TYPE_TEMP.equals(resourceType)) { + throw new IllegalArgumentException("The only supported resource type for this operation is: " + ResourceManager.RESOURCE_TYPE_TEMP); + } ResourceRevisionContainer container; try { - if (ResourceManager.RESOURCE_TYPE_ATTACHMENT.equals(resourceType) && context == null) { - throw new IllegalStateException("No context present, unable to create attachment"); - } container = resourceManager.createResourceContainer(resourceType, filename, null); - if (ResourceManager.RESOURCE_TYPE_ATTACHMENT.equals(resourceType)) { - container.getResource().setExecutionId(context.getExecutionId()); - } try { BufferedOutputStream bos = new BufferedOutputStream(container.getOutputStream()); bos.write(content); @@ -158,7 +126,7 @@ public AttachmentMeta createResourceWithoutQuotaCheck(String resourceType, byte[ attachmentMeta.setMimeType(mimeType); return attachmentMeta; } catch (IOException e1) { - throw new RuntimeException("Error while createing resource container", e1); + throw new RuntimeException("Error while creating resource container", e1); } } diff --git a/step-plans/step-plans-core/src/main/java/step/core/plans/runner/PlanRunnerResult.java b/step-plans/step-plans-core/src/main/java/step/core/plans/runner/PlanRunnerResult.java index 0bf3da4eb..171137f8b 100644 --- a/step-plans/step-plans-core/src/main/java/step/core/plans/runner/PlanRunnerResult.java +++ b/step-plans/step-plans-core/src/main/java/step/core/plans/runner/PlanRunnerResult.java @@ -18,9 +18,27 @@ ******************************************************************************/ package step.core.plans.runner; +import org.apache.commons.io.IOUtils; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import step.attachments.AttachmentMeta; +import step.attachments.SkippedAttachmentMeta; +import step.core.artefacts.reports.ParentSource; +import step.core.artefacts.reports.ReportNode; +import step.core.artefacts.reports.ReportNodeStatus; +import step.core.artefacts.reports.ReportTreeAccessor; +import step.core.artefacts.reports.ReportTreeVisitor; +import step.core.artefacts.reports.ReportTreeVisitor.ReportNodeEvent; +import step.core.execution.model.Execution; +import step.core.execution.model.ExecutionProvider; +import step.core.reports.Error; +import step.reporting.ReportWriter; +import step.resources.AttachmentStorage; + import java.io.BufferedWriter; import java.io.File; import java.io.IOException; +import java.io.InputStream; import java.io.OutputStreamWriter; import java.io.StringWriter; import java.io.Writer; @@ -36,21 +54,6 @@ import java.util.stream.Collectors; import java.util.stream.Stream; -import org.apache.commons.io.IOUtils; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; - -import step.attachments.AttachmentMeta; -import step.core.artefacts.reports.*; -import step.core.artefacts.reports.ReportTreeVisitor.ReportNodeEvent; -import step.core.execution.model.Execution; -import step.core.execution.model.ExecutionProvider; -import step.core.reports.Error; -import step.reporting.ReportWriter; -import step.resources.Resource; -import step.resources.ResourceManager; -import step.resources.ResourceRevisionContent; - /** * This class provides an API for the manipulation of plan executions * @@ -63,18 +66,18 @@ public class PlanRunnerResult { protected final ExecutionProvider executionProvider; protected final ReportTreeAccessor reportTreeAccessor; - protected final ResourceManager resourceManager; + protected final AttachmentStorage attachmentStorage; public PlanRunnerResult(String executionId, ExecutionProvider executionProvider, ReportTreeAccessor reportTreeAccessor) { this(executionId, executionProvider, reportTreeAccessor, null); } - public PlanRunnerResult(String executionId, ExecutionProvider executionProvider, ReportTreeAccessor reportTreeAccessor, ResourceManager resourceManager) { + public PlanRunnerResult(String executionId, ExecutionProvider executionProvider, ReportTreeAccessor reportTreeAccessor, AttachmentStorage attachmentStorage) { super(); this.executionId = executionId; this.executionProvider = executionProvider; this.reportTreeAccessor = reportTreeAccessor; - this.resourceManager = resourceManager; + this.attachmentStorage = attachmentStorage; } public ReportNodeStatus getResult() { @@ -274,20 +277,17 @@ public PlanRunnerResult printTree(Writer writer, boolean printNodeDetails, boole if (printAttachments) { List attachments = node.getAttachments(); if (attachments != null) { - attachments.forEach(a -> { - Resource resource = resourceManager.getResource(a.getId().toString()); - // TODO create constant for value "exception.log" - if (resource.getResourceName().equals("exception.log")) { - try { - bWriter.write("Stacktrace: \n"); - ResourceRevisionContent resourceContent = resourceManager.getResourceContent(a.getId().toString()); + attachments.forEach(att -> { + if (!(att instanceof SkippedAttachmentMeta)) { + if (att.getName().equals("exception.log")) { try { - IOUtils.copy(resourceContent.getResourceStream(), bWriter, StandardCharsets.UTF_8); - } finally { - resourceContent.close(); + bWriter.write("Stacktrace: \n"); + try (InputStream data = attachmentStorage.getAttachmentStream(att.getId().toString())) { + IOUtils.copy(data, bWriter, StandardCharsets.UTF_8); + } + } catch (Exception e) { + logger.error("Error while writing attachment {}", att.getName(), e); } - } catch (IOException e) { - logger.error("Error while writing attachment", e); } } }); From cea617bb59bd93c3e154417afd4ddb274a996362 Mon Sep 17 00:00:00 2001 From: Christoph Langguth Date: Fri, 17 Jul 2026 16:40:58 +0200 Subject: [PATCH 2/4] SED-4816 Cleanup --- .../streaming/StreamingResourcesControllerPlugin.java | 9 --------- .../src/main/java/step/core/AbstractStepContext.java | 7 ------- 2 files changed, 16 deletions(-) diff --git a/step-controller/step-controller-base-plugins/src/main/java/step/plugins/streaming/StreamingResourcesControllerPlugin.java b/step-controller/step-controller-base-plugins/src/main/java/step/plugins/streaming/StreamingResourcesControllerPlugin.java index 83a71b7ce..f63fb1352 100644 --- a/step-controller/step-controller-base-plugins/src/main/java/step/plugins/streaming/StreamingResourcesControllerPlugin.java +++ b/step-controller/step-controller-base-plugins/src/main/java/step/plugins/streaming/StreamingResourcesControllerPlugin.java @@ -10,7 +10,6 @@ import step.constants.LiveReportingConstants; import step.core.GlobalContext; import step.core.deployment.ObjectHookControllerPlugin; -import step.core.execution.AbstractExecutionEngineContext; import step.core.execution.ExecutionContext; import step.core.execution.ExecutionEngineContext; import step.core.plugins.AbstractControllerPlugin; @@ -111,12 +110,6 @@ private ServerEndpointConfig makeDownloadConfig(StepStreamingResourceManager man @Override public ExecutionEnginePlugin getExecutionEnginePlugin() { return new AbstractExecutionEnginePlugin() { - @Override - public void initializeExecutionEngineContext(AbstractExecutionEngineContext parentContext, ExecutionEngineContext executionEngineContext) { - // required by AP reporting for fetching attachments; FIXME -- this is now redundant - executionEngineContext.setAttachmentStorage(manager); - } - @Override public void initializeExecutionContext(ExecutionEngineContext executionEngineContext, ExecutionContext executionContext) { // Makes streaming available to the execution @@ -124,8 +117,6 @@ public void initializeExecutionContext(ExecutionEngineContext executionEngineCon executionContext.put(LiveReportingConstants.STREAMING_WEBSOCKET_UPLOAD_PATH, UPLOAD_PATH); // Note: this URL is also used by the measures streaming to determine the hostname. executionContext.put(LiveReportingConstants.LIVEREPORTING_CONTROLLER_URL, controllerUrl); - // FIXME: This is now also redundant... - executionContext.setAttachmentStorage(manager); } diff --git a/step-core/src/main/java/step/core/AbstractStepContext.java b/step-core/src/main/java/step/core/AbstractStepContext.java index e9d50da93..094a55aac 100644 --- a/step-core/src/main/java/step/core/AbstractStepContext.java +++ b/step-core/src/main/java/step/core/AbstractStepContext.java @@ -107,13 +107,6 @@ public AttachmentStorage getAttachmentStorage() { } public void setAttachmentStorage(AttachmentStorage attachmentStorage) { - if (attachmentStorage == this.attachmentStorage) { - System.err.println("FIXME: redundant setting of attachment storage in " + this.getClass().getSimpleName()); - return; - } - if (this.attachmentStorage != null) { - throw new IllegalStateException("BUG: attachmentStorage must not be set more than once"); - } this.attachmentStorage = attachmentStorage; } From da01686f3059503f6ce7aaf70fb3d85dfabb5a6e Mon Sep 17 00:00:00 2001 From: Christoph Langguth Date: Tue, 21 Jul 2026 11:10:52 +0200 Subject: [PATCH 3/4] SED-4816 Robot PR feedback --- .../StepStreamingResourceManager.java | 8 ++++++- .../step/resources/AttachmentStorage.java | 11 ++++++++++ .../resources/LocalAttachmentStorage.java | 22 +++++++++++++++---- .../artefacts/handlers/ExportHandler.java | 2 +- .../core/plans/runner/PlanRunnerResult.java | 5 ++++- 5 files changed, 41 insertions(+), 7 deletions(-) diff --git a/step-controller/step-controller-base-plugins/src/main/java/step/plugins/streaming/StepStreamingResourceManager.java b/step-controller/step-controller-base-plugins/src/main/java/step/plugins/streaming/StepStreamingResourceManager.java index cfc6981ae..90b2b22c7 100644 --- a/step-controller/step-controller-base-plugins/src/main/java/step/plugins/streaming/StepStreamingResourceManager.java +++ b/step-controller/step-controller-base-plugins/src/main/java/step/plugins/streaming/StepStreamingResourceManager.java @@ -29,10 +29,12 @@ import java.io.InputStream; import java.util.Objects; import java.util.function.Function; +import java.util.regex.Pattern; public class StepStreamingResourceManager extends DefaultStreamingResourceManager implements AttachmentStorage { private static final Logger logger = LoggerFactory.getLogger(StepStreamingResourceManager.class); static final String ATTRIBUTE_STEP_SESSION = "stepSession"; + private static final Pattern MIME_TYPE_PATTERN = Pattern.compile("^[a-zA-Z0-9!#$&^_.+-]+/[a-zA-Z0-9!#$&^_.+-]+$"); private final AuthorizationManager> authorizationManager; @@ -160,6 +162,10 @@ public InputStream getAttachmentStream(String resourceId) throws Exception { @Override public AttachmentMeta saveAttachment(Object executionContext, byte[] content, String filename, String mimeType) { + if (!(executionContext instanceof ExecutionContext)) { + String className = executionContext == null ? "null" : executionContext.getClass().getName(); + return new SkippedAttachmentMeta(filename, mimeType, "UNEXPECTED: Invalid execution context type of class " + className); + } ExecutionContext context = (ExecutionContext) Objects.requireNonNull(executionContext); StreamingResourceUploadContexts uploadContexts = context.get(StreamingResourceUploadContexts.class); @@ -208,7 +214,7 @@ private static String sanitizeMimeType(String mimeType) { return "application/octet-stream"; } mimeType = mimeType.trim(); // just in case - if (!mimeType.matches("^[a-zA-Z0-9!#$&^_.+-]+/[a-zA-Z0-9!#$&^_.+-]+$")) { + if (!MIME_TYPE_PATTERN.matcher(mimeType).matches()) { logger.warn("Invalid mime type \"{}\", replacing with generic application/octet-stream", mimeType); return "application/octet-stream"; } diff --git a/step-core/src/main/java/step/resources/AttachmentStorage.java b/step-core/src/main/java/step/resources/AttachmentStorage.java index bdf7901c7..4d1aa8ff7 100644 --- a/step-core/src/main/java/step/resources/AttachmentStorage.java +++ b/step-core/src/main/java/step/resources/AttachmentStorage.java @@ -7,6 +7,17 @@ public interface AttachmentStorage { InputStream getAttachmentStream(String attachmentId) throws Exception; + /** + * Saves an attachment in the given executionContent, with the given metadata. + * Implementation note: the executionContext will be a step.core.execution.ExecutionContext, but that class + * is out of scope here, hence the use of a generic Object. + * + * @param executionContext execution context + * @param content content of the attachment, as a byte array + * @param filename file name of the attachment + * @param mimeType MIME type, may be null + * @return AttachmentMeta object corresponding to the attachment + */ AttachmentMeta saveAttachment(Object executionContext, byte[] content, String filename, String mimeType); default void cleanupIfNeeded() { diff --git a/step-core/src/main/java/step/resources/LocalAttachmentStorage.java b/step-core/src/main/java/step/resources/LocalAttachmentStorage.java index e85f75e22..cc45b744e 100644 --- a/step-core/src/main/java/step/resources/LocalAttachmentStorage.java +++ b/step-core/src/main/java/step/resources/LocalAttachmentStorage.java @@ -33,9 +33,13 @@ public class LocalAttachmentStorage implements AttachmentStorage { public LocalAttachmentStorage(File rootDirectory) { this.rootDirectory = Objects.requireNonNull(rootDirectory); if (rootDirectory.exists()) { - throw new IllegalArgumentException("Attachments storage root directory already exists: " + rootDirectory.getAbsolutePath()); - } - if (!rootDirectory.mkdirs()) { + if (!rootDirectory.isDirectory()) { + throw new IllegalArgumentException("Attachments storage root path exists and is not a directory: " + rootDirectory.getAbsolutePath()); + } else { + // We normally expect our own unique (usually temporary) path. Allow instantiation with existing path nevertheless, but complain. + logger.warn("Attachments storage root directory already exists, reusing directory:{}", rootDirectory.getAbsolutePath()); + } + } else if (!rootDirectory.mkdirs()) { throw new RuntimeException("Cannot create attachments storage directory: " + rootDirectory.getAbsolutePath()); } logger.info("Attachment storage initialized, directory={}", rootDirectory.getAbsolutePath()); @@ -43,6 +47,9 @@ public LocalAttachmentStorage(File rootDirectory) { @Override public InputStream getAttachmentStream(String attachmentId) throws Exception { + if (attachmentId == null || !attachmentId.matches("^[a-fA-F0-9]+$")) { + throw new IllegalArgumentException("Unexpected attachment ID format: " + attachmentId); + } Path rootPath = rootDirectory.toPath(); String searchPattern = attachmentId + "_*"; @@ -66,7 +73,7 @@ public AttachmentMeta saveAttachment(Object executionContext, byte[] content, St // This stores to ${ID}_${FILENAME}. // We could also use an intermediate directory, ie. ${ID}/${FILENAME}, which will // somewhat facilitate lookup but requires twice the number of inodes. - String storedFilename = id.toHexString() + "_" + filename; + String storedFilename = id.toHexString() + "_" + escapeFilename(filename); Path targetPath = rootDirectory.toPath().resolve(storedFilename); try { // Resolve the path and write the byte array directly @@ -82,6 +89,13 @@ public AttachmentMeta saveAttachment(Object executionContext, byte[] content, St } } + private static String escapeFilename(String filename) { + if (filename == null || filename.isBlank()) { + return filename; + } + return filename.replaceAll("[^a-zA-Z0-9.-]+", "_"); + } + public void cleanupIfNeeded() { try { logger.info("Cleanup attachment storage: {}", rootDirectory.getAbsolutePath()); diff --git a/step-plans/step-plans-base-artefacts/src/main/java/step/artefacts/handlers/ExportHandler.java b/step-plans/step-plans-base-artefacts/src/main/java/step/artefacts/handlers/ExportHandler.java index c134a5275..10ec46408 100644 --- a/step-plans/step-plans-base-artefacts/src/main/java/step/artefacts/handlers/ExportHandler.java +++ b/step-plans/step-plans-base-artefacts/src/main/java/step/artefacts/handlers/ExportHandler.java @@ -82,7 +82,7 @@ private void export(ReportNode node, Export testArtefact) { throw new RuntimeException("Attachment storage is null"); } String filenamePrefix = testArtefact.getPrefix() != null ? testArtefact.getPrefix().get() : ""; - File target = new File(file + "/" + filenamePrefix + fileName); + File target = new File(file, filenamePrefix + fileName); try (InputStream in = storage.getAttachmentStream(attachmentMeta.getId().toString()); OutputStream out = new FileOutputStream(target)) { in.transferTo(out); diff --git a/step-plans/step-plans-core/src/main/java/step/core/plans/runner/PlanRunnerResult.java b/step-plans/step-plans-core/src/main/java/step/core/plans/runner/PlanRunnerResult.java index 171137f8b..627dc844d 100644 --- a/step-plans/step-plans-core/src/main/java/step/core/plans/runner/PlanRunnerResult.java +++ b/step-plans/step-plans-core/src/main/java/step/core/plans/runner/PlanRunnerResult.java @@ -276,7 +276,10 @@ public PlanRunnerResult printTree(Writer writer, boolean printNodeDetails, boole if (printAttachments) { List attachments = node.getAttachments(); - if (attachments != null) { + if (attachmentStorage == null) { + // This should not normally happen, but better safe than sorry + logger.warn("Unable to evaluate exception attachments because attachmentStorage is null"); + } else if (attachments != null) { attachments.forEach(att -> { if (!(att instanceof SkippedAttachmentMeta)) { if (att.getName().equals("exception.log")) { From d86326465bf955f37e6e1bbbfe3cdff597ea22cf Mon Sep 17 00:00:00 2001 From: Christoph Langguth Date: Tue, 21 Jul 2026 14:24:44 +0200 Subject: [PATCH 4/4] SED-4816 Minor enhancements --- .../test/java/step/core/GlobalContextBuilder.java | 15 ++++++++++++--- .../step/resources/LocalAttachmentStorage.java | 5 +++++ 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/step-controller/step-controller-server/src/test/java/step/core/GlobalContextBuilder.java b/step-controller/step-controller-server/src/test/java/step/core/GlobalContextBuilder.java index 60226da11..f947014f5 100644 --- a/step-controller/step-controller-server/src/test/java/step/core/GlobalContextBuilder.java +++ b/step-controller/step-controller-server/src/test/java/step/core/GlobalContextBuilder.java @@ -60,7 +60,14 @@ import step.functions.accessor.FunctionAccessor; import step.functions.accessor.FunctionEntity; import step.functions.accessor.InMemoryFunctionAccessorImpl; -import step.resources.*; +import step.resources.InMemoryResourceAccessor; +import step.resources.InMemoryResourceRevisionAccessor; +import step.resources.LocalResourceManagerImpl; +import step.resources.ResourceAccessor; +import step.resources.ResourceEntity; +import step.resources.ResourceManager; +import step.resources.ResourceRevision; +import step.resources.ResourceRevisionAccessor; import java.io.File; import java.io.IOException; @@ -102,8 +109,10 @@ public static GlobalContext createGlobalContext() throws CircularDependencyExcep ResourceAccessor resourceAccessor = new InMemoryResourceAccessor(); InMemoryResourceRevisionAccessor resourceRevisionAccessor = new InMemoryResourceRevisionAccessor(); try { - File rootFolder = FileHelper.createTempFolder(); - ResourceManager resourceManager = new ResourceManagerImpl(rootFolder, resourceAccessor, resourceRevisionAccessor); + // WARNING: The following folder is leaked unless it's cleaned up + // in the referencing unit tests (e.g. using ResourceManager#cleanup) - tracked in SED-4849 + File resourceRootFolder = FileHelper.createTempFolder("step-resourceManager-"); + ResourceManager resourceManager = new LocalResourceManagerImpl(resourceRootFolder, resourceAccessor, resourceRevisionAccessor); context.setResourceManager(resourceManager); } catch (IOException e) { logger.error("Unable to create temp folder for the resource manager", e); diff --git a/step-core/src/main/java/step/resources/LocalAttachmentStorage.java b/step-core/src/main/java/step/resources/LocalAttachmentStorage.java index cc45b744e..3c51ea938 100644 --- a/step-core/src/main/java/step/resources/LocalAttachmentStorage.java +++ b/step-core/src/main/java/step/resources/LocalAttachmentStorage.java @@ -104,4 +104,9 @@ public void cleanupIfNeeded() { logger.error("Error while cleaning up directory {}", rootDirectory.getAbsolutePath(), e); } } + + @Override + public String toString() { + return "LocalAttachmentStorage [rootDirectory=" + rootDirectory + "]"; + } }