From 51982a77e3f8d76128e140a0f5982e19260e0bb7 Mon Sep 17 00:00:00 2001 From: Alexander Dobler <50330299+dobleralex@users.noreply.github.com> Date: Fri, 8 Nov 2019 11:11:32 +0100 Subject: [PATCH] Fixed error message not shown when exceeding filesize (#906) * Fixed message not showing when exceeding filesize Signed-off-by: Alexander Dobler * Also use fixed max filesize approach during drag and drop Signed-off-by: Alexander Dobler * Moved log messages and adjusted upload log levels Signed-off-by: Alexander Dobler * Further adjustments of logging Signed-off-by: Alexander Dobler --- .../upload/AbstractFileTransferHandler.java | 67 +++++++++---------- .../FileTransferHandlerStreamVariable.java | 21 +++--- .../FileTransferHandlerVaadinUpload.java | 24 +++---- .../src/main/resources/messages.properties | 1 - 4 files changed, 57 insertions(+), 56 deletions(-) diff --git a/hawkbit-ui/src/main/java/org/eclipse/hawkbit/ui/artifacts/upload/AbstractFileTransferHandler.java b/hawkbit-ui/src/main/java/org/eclipse/hawkbit/ui/artifacts/upload/AbstractFileTransferHandler.java index 255ea33af..d9e35018e 100644 --- a/hawkbit-ui/src/main/java/org/eclipse/hawkbit/ui/artifacts/upload/AbstractFileTransferHandler.java +++ b/hawkbit-ui/src/main/java/org/eclipse/hawkbit/ui/artifacts/upload/AbstractFileTransferHandler.java @@ -12,7 +12,6 @@ import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; import java.io.Serializable; -import java.util.Optional; import java.util.concurrent.ExecutorService; import java.util.concurrent.locks.Lock; @@ -21,10 +20,10 @@ import org.eclipse.hawkbit.repository.ArtifactManagement; import org.eclipse.hawkbit.repository.RegexCharacterCollection; import org.eclipse.hawkbit.repository.RegexCharacterCollection.RegexChar; import org.eclipse.hawkbit.repository.exception.ArtifactUploadFailedException; +import org.eclipse.hawkbit.repository.exception.AssignmentQuotaExceededException; import org.eclipse.hawkbit.repository.exception.FileSizeQuotaExceededException; import org.eclipse.hawkbit.repository.exception.InvalidMD5HashException; import org.eclipse.hawkbit.repository.exception.InvalidSHA1HashException; -import org.eclipse.hawkbit.repository.exception.AssignmentQuotaExceededException; import org.eclipse.hawkbit.repository.exception.StorageQuotaExceededException; import org.eclipse.hawkbit.repository.model.Artifact; import org.eclipse.hawkbit.repository.model.ArtifactUpload; @@ -40,6 +39,7 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.vaadin.spring.events.EventBus; import org.vaadin.spring.events.EventBus.UIEventBus; + import com.vaadin.ui.UI; /** @@ -72,7 +72,8 @@ public abstract class AbstractFileTransferHandler implements Serializable { protected static final RegexCharacterCollection ILLEGAL_FILENAME_CHARACTERS = new RegexCharacterCollection( RegexChar.GREATER_THAN, RegexChar.LESS_THAN, RegexChar.SLASHES); - AbstractFileTransferHandler(final ArtifactManagement artifactManagement, final VaadinMessageSource i18n, final Lock uploadLock) { + AbstractFileTransferHandler(final ArtifactManagement artifactManagement, final VaadinMessageSource i18n, + final Lock uploadLock) { this.artifactManagement = artifactManagement; this.i18n = i18n; this.eventBus = SpringContextHelper.getBean(EventBus.UIEventBus.class); @@ -100,8 +101,9 @@ public abstract class AbstractFileTransferHandler implements Serializable { protected void startTransferToRepositoryThread(final InputStream inputStream, final FileUploadId fileUploadId, final String mimeType) { - SpringContextHelper.getBean("asyncExecutor", ExecutorService.class).execute( - new TransferArtifactToRepositoryRunnable(inputStream, fileUploadId, mimeType, UI.getCurrent(), uploadLock)); + SpringContextHelper.getBean("asyncExecutor", ExecutorService.class) + .execute(new TransferArtifactToRepositoryRunnable(inputStream, fileUploadId, mimeType, UI.getCurrent(), + uploadLock)); } private void interruptUploadAndSetReason(final String failureReason) { @@ -129,10 +131,6 @@ public abstract class AbstractFileTransferHandler implements Serializable { interruptUploadAndSetReason(i18n.getMessage("message.no.duplicateFiles")); } - protected void interruptUploadDueToFileSizeExceeded(final long maxSize) { - interruptUploadAndSetReason(i18n.getMessage("message.uploadedfile.size.exceeded", maxSize)); - } - protected void interruptUploadDueToIllegalFilename() { interruptUploadAndSetReason(i18n.getMessage("message.uploadedfile.illegalFilename")); } @@ -170,7 +168,7 @@ public abstract class AbstractFileTransferHandler implements Serializable { } protected void publishUploadFinishedEvent(final FileUploadId fileUploadId) { - LOG.info("Upload finished for file {}", fileUploadId); + LOG.debug("Upload finished for file {}", fileUploadId); final FileUploadProgress fileUploadProgress = new FileUploadProgress(fileUploadId, FileUploadStatus.UPLOAD_FINISHED); eventBus.publish(this, fileUploadProgress); @@ -184,21 +182,22 @@ public abstract class AbstractFileTransferHandler implements Serializable { eventBus.publish(this, fileUploadProgress); } - protected void publishArtifactsChanged(final FileUploadId fileUploadId) { - eventBus.publish(this, - new SoftwareModuleEvent(SoftwareModuleEventType.ARTIFACTS_CHANGED, fileUploadId.getSoftwareModuleId())); - } - - protected void publishUploadFailedAndFinishedEvent(final FileUploadId fileUploadId, - final Exception uploadException) { - LOG.info("Upload failed for file {} due to reason: {}, exception: {}", fileUploadId, failureReason, - uploadException.getMessage()); - + protected void publishUploadFailedEvent(final FileUploadId fileUploadId) { + LOG.info("Upload failed for file {} due to reason: {}", fileUploadId, failureReason); final FileUploadProgress fileUploadProgress = new FileUploadProgress(fileUploadId, FileUploadStatus.UPLOAD_FAILED, StringUtils.isBlank(failureReason) ? i18n.getMessage(MESSAGE_UPLOAD_FAILED) : failureReason); artifactUploadState.updateFileUploadProgress(fileUploadId, fileUploadProgress); eventBus.publish(this, fileUploadProgress); + } + + protected void publishArtifactsChanged(final FileUploadId fileUploadId) { + eventBus.publish(this, + new SoftwareModuleEvent(SoftwareModuleEventType.ARTIFACTS_CHANGED, fileUploadId.getSoftwareModuleId())); + } + + protected void publishUploadFailedAndFinishedEvent(final FileUploadId fileUploadId) { + publishUploadFailedEvent(fileUploadId); publishUploadFinishedEvent(fileUploadId); } @@ -214,7 +213,7 @@ public abstract class AbstractFileTransferHandler implements Serializable { try { outputStream.close(); } catch (final IOException e1) { - LOG.error("Closing output stream caused an exception {}", e1); + LOG.warn("Closing output stream caused an exception {}", e1); } } @@ -225,7 +224,7 @@ public abstract class AbstractFileTransferHandler implements Serializable { try { inputStream.close(); } catch (final IOException e1) { - LOG.error("Closing input stream caused an exception {}", e1); + LOG.warn("Closing input stream caused an exception {}", e1); } } } @@ -248,9 +247,10 @@ public abstract class AbstractFileTransferHandler implements Serializable { /** * a lock object is used here that is propagated down from - * {@link org.eclipse.hawkbit.ui.artifacts.UploadArtifactView}. It ensures that from within the same UI instance - * all uploads are executed sequentially to avoid issues that occur when multiple files are processed at the - * same time (e.g. regarding quota checks) + * {@link org.eclipse.hawkbit.ui.artifacts.UploadArtifactView}. It + * ensures that from within the same UI instance all uploads are + * executed sequentially to avoid issues that occur when multiple files + * are processed at the same time (e.g. regarding quota checks) */ @Override public void run() { @@ -269,8 +269,8 @@ public abstract class AbstractFileTransferHandler implements Serializable { LOG.debug("Upload failed due to assignment quota exceeded:", e); } catch (final RuntimeException e) { interruptUploadDueToUploadFailed(); - publishUploadFailedAndFinishedEvent(fileUploadId, e); - LOG.error("Failed to transfer file to repository", e); + publishUploadFailedAndFinishedEvent(fileUploadId); + LOG.warn("Failed to transfer file to repository", e); } finally { tryToCloseIOStream(inputStream); uploadLock.unlock(); @@ -282,8 +282,8 @@ public abstract class AbstractFileTransferHandler implements Serializable { throw new ArtifactUploadFailedException(); } final String filename = fileUploadId.getFilename(); - LOG.info("Transfering file {} directly to repository", filename); - final Artifact artifact = uploadArtifact(filename).orElseThrow(ArtifactUploadFailedException::new); + LOG.debug("Transfering file {} directly to repository", filename); + final Artifact artifact = uploadArtifact(filename); if (isUploadInterrupted()) { handleUploadFailure(artifact); publishUploadFinishedEvent(fileUploadId); @@ -294,13 +294,12 @@ public abstract class AbstractFileTransferHandler implements Serializable { publishArtifactsChanged(fileUploadId); } - private Optional uploadArtifact(final String filename) { + private Artifact uploadArtifact(final String filename) { try { - return Optional.ofNullable(artifactManagement.create(new ArtifactUpload(inputStream, - fileUploadId.getSoftwareModuleId(), filename, null, null, true, mimeType, -1))); + return artifactManagement.create(new ArtifactUpload(inputStream, fileUploadId.getSoftwareModuleId(), + filename, null, null, true, mimeType, -1)); } catch (final ArtifactUploadFailedException | InvalidSHA1HashException | InvalidMD5HashException e) { - LOG.error("Failed to transfer file to repository", e); - return Optional.empty(); + throw new ArtifactUploadFailedException(e); } } diff --git a/hawkbit-ui/src/main/java/org/eclipse/hawkbit/ui/artifacts/upload/FileTransferHandlerStreamVariable.java b/hawkbit-ui/src/main/java/org/eclipse/hawkbit/ui/artifacts/upload/FileTransferHandlerStreamVariable.java index f2a9d0a94..878ba82f0 100644 --- a/hawkbit-ui/src/main/java/org/eclipse/hawkbit/ui/artifacts/upload/FileTransferHandlerStreamVariable.java +++ b/hawkbit-ui/src/main/java/org/eclipse/hawkbit/ui/artifacts/upload/FileTransferHandlerStreamVariable.java @@ -16,6 +16,7 @@ import java.util.concurrent.locks.Lock; import org.eclipse.hawkbit.repository.ArtifactManagement; import org.eclipse.hawkbit.repository.RegexCharacterCollection; +import org.eclipse.hawkbit.repository.SizeConversionHelper; import org.eclipse.hawkbit.repository.model.SoftwareModule; import org.eclipse.hawkbit.ui.utils.VaadinMessageSource; import org.slf4j.Logger; @@ -65,10 +66,10 @@ public class FileTransferHandlerStreamVariable extends AbstractFileTransferHandl assertStateConsistency(fileUploadId, event.getFileName()); if (RegexCharacterCollection.stringContainsCharacter(event.getFileName(), ILLEGAL_FILENAME_CHARACTERS)) { - LOG.info("Filename contains illegal characters {} for upload {}", fileUploadId.getFilename(), fileUploadId); + LOG.debug("Filename contains illegal characters {} for upload {}", fileUploadId.getFilename(), fileUploadId); interruptUploadDueToIllegalFilename(); } else if (isFileAlreadyContainedInSoftwareModule(fileUploadId, selectedSoftwareModule)) { - LOG.info("File {} already contained in Software Module {}", fileUploadId.getFilename(), + LOG.debug("File {} already contained in Software Module {}", fileUploadId.getFilename(), selectedSoftwareModule); interruptUploadDueToDuplicateFile(); } @@ -89,11 +90,11 @@ public class FileTransferHandlerStreamVariable extends AbstractFileTransferHandl publishUploadProgressEvent(fileUploadId, 0, fileSize); startTransferToRepositoryThread(inputStream, fileUploadId, mimeType); } catch (final IOException e) { - LOG.error("Creating piped Stream failed {}.", e); + LOG.warn("Creating piped Stream failed {}.", e); tryToCloseIOStream(outputStream); tryToCloseIOStream(inputStream); interruptUploadDueToUploadFailed(); - publishUploadFailedAndFinishedEvent(fileUploadId, e); + publishUploadFailedAndFinishedEvent(fileUploadId); return ByteStreams.nullOutputStream(); } return outputStream; @@ -118,12 +119,13 @@ public class FileTransferHandlerStreamVariable extends AbstractFileTransferHandl public void onProgress(final StreamingProgressEvent event) { assertStateConsistency(fileUploadId, event.getFileName()); - if (event.getBytesReceived() > maxSize || event.getContentLength() > maxSize) { - LOG.error("User tried to upload more than was allowed ({}).", maxSize); - interruptUploadDueToFileSizeExceeded(maxSize); + if (isUploadInterrupted()) { + publishUploadFailedAndFinishedEvent(fileUploadId); return; } - if (isUploadInterrupted()) { + + if (event.getBytesReceived() > maxSize || event.getContentLength() > maxSize) { + interruptUploadDueToFileSizeQuotaExceeded(SizeConversionHelper.byteValueToReadableString(maxSize)); return; } @@ -155,7 +157,8 @@ public class FileTransferHandlerStreamVariable extends AbstractFileTransferHandl if (!isUploadInterrupted()) { interruptUploadDueToUploadFailed(); } - publishUploadFailedAndFinishedEvent(fileUploadId, event.getException()); + LOG.debug("Streaming of file {} failed due to following exception: {}", fileUploadId, event.getException()); + publishUploadFailedAndFinishedEvent(fileUploadId); } @Override diff --git a/hawkbit-ui/src/main/java/org/eclipse/hawkbit/ui/artifacts/upload/FileTransferHandlerVaadinUpload.java b/hawkbit-ui/src/main/java/org/eclipse/hawkbit/ui/artifacts/upload/FileTransferHandlerVaadinUpload.java index 81701aed1..f933ec16e 100644 --- a/hawkbit-ui/src/main/java/org/eclipse/hawkbit/ui/artifacts/upload/FileTransferHandlerVaadinUpload.java +++ b/hawkbit-ui/src/main/java/org/eclipse/hawkbit/ui/artifacts/upload/FileTransferHandlerVaadinUpload.java @@ -16,6 +16,7 @@ import java.util.concurrent.locks.Lock; import org.eclipse.hawkbit.repository.ArtifactManagement; import org.eclipse.hawkbit.repository.RegexCharacterCollection; +import org.eclipse.hawkbit.repository.SizeConversionHelper; import org.eclipse.hawkbit.repository.SoftwareModuleManagement; import org.eclipse.hawkbit.repository.model.SoftwareModule; import org.eclipse.hawkbit.ui.utils.VaadinMessageSource; @@ -82,16 +83,15 @@ public class FileTransferHandlerVaadinUpload extends AbstractFileTransferHandler interruptUploadDueToDuplicateFile(); event.getUpload().interruptUpload(); } else { - LOG.info("Uploading file {}", fileUploadId); publishUploadStarted(fileUploadId); if (RegexCharacterCollection.stringContainsCharacter(event.getFilename(), ILLEGAL_FILENAME_CHARACTERS)) { - LOG.info("Filename contains illegal characters {} for upload {}", fileUploadId.getFilename(), + LOG.debug("Filename contains illegal characters {} for upload {}", fileUploadId.getFilename(), fileUploadId); interruptUploadDueToIllegalFilename(); event.getUpload().interruptUpload(); } else if (isFileAlreadyContainedInSoftwareModule(fileUploadId, softwareModule)) { - LOG.info("File {} already contained in Software Module {}", fileUploadId.getFilename(), softwareModule); + LOG.debug("File {} already contained in Software Module {}", fileUploadId.getFilename(), softwareModule); interruptUploadDueToDuplicateFile(); event.getUpload().interruptUpload(); } @@ -130,11 +130,11 @@ public class FileTransferHandlerVaadinUpload extends AbstractFileTransferHandler publishUploadProgressEvent(fileUploadId, 0, 0); startTransferToRepositoryThread(inputStream, fileUploadId, mimeType); } catch (final IOException e) { - LOG.error("Creating piped Stream failed {}.", e); + LOG.warn("Creating piped Stream failed {}.", e); tryToCloseIOStream(outputStream); tryToCloseIOStream(inputStream); interruptUploadDueToUploadFailed(); - publishUploadFailedAndFinishedEvent(fileUploadId, e); + publishUploadFailedAndFinishedEvent(fileUploadId); return ByteStreams.nullOutputStream(); } return outputStream; @@ -147,14 +147,13 @@ public class FileTransferHandlerVaadinUpload extends AbstractFileTransferHandler */ @Override public void updateProgress(final long readBytes, final long contentLength) { - if (readBytes > maxSize || contentLength > maxSize) { - LOG.error("User tried to upload more than was allowed ({}).", maxSize); - interruptUploadDueToFileSizeExceeded(maxSize); + if (isUploadInterrupted()) { + publishUploadFailedAndFinishedEvent(fileUploadId); return; } - if (isUploadInterrupted()) { - // Upload interruption is delayed maybe another event is fired - // before + + if (readBytes > maxSize || contentLength > maxSize) { + interruptUploadDueToFileSizeQuotaExceeded(SizeConversionHelper.byteValueToReadableString(maxSize)); return; } @@ -200,7 +199,8 @@ public class FileTransferHandlerVaadinUpload extends AbstractFileTransferHandler if (!isUploadInterrupted()) { interruptUploadDueToUploadFailed(); } - publishUploadFailedAndFinishedEvent(fileUploadId, event.getReason()); + LOG.debug("Upload of file {} failed due to following exception: {}", fileUploadId, event.getReason()); + publishUploadFailedAndFinishedEvent(fileUploadId); } @Override diff --git a/hawkbit-ui/src/main/resources/messages.properties b/hawkbit-ui/src/main/resources/messages.properties index 3948ecf94..e23c9d148 100644 --- a/hawkbit-ui/src/main/resources/messages.properties +++ b/hawkbit-ui/src/main/resources/messages.properties @@ -469,7 +469,6 @@ message.upload.failed = Upload failed message.upload.assignmentQuota = Max assignments per software module exceeded message.upload.fileSizeQuota = Maximum artifact size ({0}) exceeded message.upload.storageQuota = Storage quota exceeded, {0} left -message.uploadedfile.size.exceeded = File size exceeded. Allowed size {0} bytes message.uploadedfile.illegalFilename = Filename contains illegal characters message.artifact.deleted = Artifact with file {0} deleted successfully