From 1840818bdcc8aae7a520ace73b15ab3521d5c870 Mon Sep 17 00:00:00 2001 From: Artur Signell Date: Thu, 6 Nov 2025 22:37:50 +0200 Subject: [PATCH 1/2] feat: add upload rejection API with optional messages Add ability to reject file uploads during processing with optional rejection messages. The rejection status is tracked per-file and communicated back to the client via appropriate HTTP status codes: - 200 OK: all files accepted - 422 Unprocessable Entity: all files rejected (with JSON body) - 207 Multi-Status: mixed results (with JSON body) Key changes: - Add reject() and reject(String) methods to UploadEvent - Extend UploadResult record with acceptedFiles/rejectedFiles tracking - Add UploadResult.Builder for incremental result construction - Move JSON response handling into UploadHandler.responseHandled() - Add rejected() callback to FileUploadCallback and InMemoryUploadCallback --- .../server/communication/TransferUtil.java | 59 +++-- .../server/streams/FileUploadCallback.java | 23 ++ .../streams/InMemoryUploadCallback.java | 23 ++ .../flow/server/streams/UploadEvent.java | 57 +++++ .../flow/server/streams/UploadHandler.java | 77 ++++++- .../flow/server/streams/UploadResult.java | 155 ++++++++++++- .../communication/UploadHandlerTest.java | 208 ++++++++++++++++++ .../flow/server/streams/UploadEventTest.java | 172 +++++++++++++++ 8 files changed, 748 insertions(+), 26 deletions(-) create mode 100644 flow-server/src/test/java/com/vaadin/flow/server/streams/UploadEventTest.java diff --git a/flow-server/src/main/java/com/vaadin/flow/server/communication/TransferUtil.java b/flow-server/src/main/java/com/vaadin/flow/server/communication/TransferUtil.java index 289136adaec..8a83c9362fd 100644 --- a/flow-server/src/main/java/com/vaadin/flow/server/communication/TransferUtil.java +++ b/flow-server/src/main/java/com/vaadin/flow/server/communication/TransferUtil.java @@ -29,6 +29,7 @@ import java.util.Map; import java.util.Objects; +import org.slf4j.Logger; import org.slf4j.LoggerFactory; import com.vaadin.flow.component.Component; @@ -52,6 +53,9 @@ */ public final class TransferUtil { + private static final Logger logger = LoggerFactory + .getLogger(TransferUtil.class); + /** * Default buffer size for reading data from the input stream. *

@@ -60,6 +64,10 @@ public final class TransferUtil { */ public static int DEFAULT_BUFFER_SIZE = 16384; + private static Logger getLogger() { + return logger; + } + /** * Transfers data from the given input stream to the output stream while * notifying the progress to the given listeners. @@ -146,6 +154,7 @@ public static void handleUpload(UploadHandler handler, VaadinRequest request, VaadinResponse response, VaadinSession session, Element owner) { boolean isMultipartUpload = isMultipartContent(request); + UploadResult.Builder resultBuilder = new UploadResult.Builder(response); try { if (isMultipartUpload) { Collection parts = Collections.EMPTY_LIST; @@ -164,9 +173,10 @@ public static void handleUpload(UploadHandler handler, session, part.getSubmittedFileName(), part.getSize(), part.getContentType(), owner, part); - handleUploadRequest(handler, event); + + handleUploadRequest(handler, event, resultBuilder); } - handler.responseHandled(new UploadResult(true, response)); + handler.responseHandled(resultBuilder.build()); } else { LoggerFactory.getLogger(UploadHandler.class) .warn("Multipart request has no parts"); @@ -180,8 +190,8 @@ public static void handleUpload(UploadHandler handler, fileName, request.getContentLengthLong(), contentType, owner, null); - handleUploadRequest(handler, event); - handler.responseHandled(new UploadResult(true, response)); + handleUploadRequest(handler, event, resultBuilder); + handler.responseHandled(resultBuilder.build()); } } catch (UploadSizeLimitExceededException | UploadFileSizeLimitExceededException @@ -190,23 +200,24 @@ public static void handleUpload(UploadHandler handler, + "extend StreamRequestHandler, override {} method for " + "UploadHandler and provide a higher limit."; if (e instanceof UploadSizeLimitExceededException) { - LoggerFactory.getLogger(UploadHandler.class).warn(limitInfoStr, - "Request size", "getRequestSizeMax"); + getLogger().warn(limitInfoStr, "Request size", + "getRequestSizeMax"); } else if (e instanceof UploadFileSizeLimitExceededException fileSizeException) { - LoggerFactory.getLogger(UploadHandler.class).warn( - limitInfoStr + " File: {}", "File size", + getLogger().warn(limitInfoStr + " File: {}", "File size", "getFileSizeMax", fileSizeException.getFileName()); } else if (e instanceof UploadFileCountLimitExceededException) { - LoggerFactory.getLogger(UploadHandler.class).warn(limitInfoStr, - "File count", "getFileCountMax"); + getLogger().warn(limitInfoStr, "File count", + "getFileCountMax"); } LoggerFactory.getLogger(UploadHandler.class) .warn("File upload failed.", e); - handler.responseHandled(new UploadResult(false, response, e)); + handler.responseHandled( + resultBuilder.withException(e).build()); } catch (Exception e) { LoggerFactory.getLogger(UploadHandler.class) .error("Exception during upload", e); - handler.responseHandled(new UploadResult(false, response, e)); + handler.responseHandled( + resultBuilder.withException(e).build()); } } @@ -324,8 +335,21 @@ private static void validateUploadLimits(UploadHandler handler, } } + /** + * Handles an upload request and checks for rejection. + * + * @param handler + * the upload handler + * @param event + * the upload event + * @param resultBuilder + * the upload result builder to update with acceptance/rejection + * @throws IOException + * if an I/O error occurs + */ private static void handleUploadRequest(UploadHandler handler, - UploadEvent event) throws IOException { + UploadEvent event, UploadResult.Builder resultBuilder) + throws IOException { Component owner = event.getOwningComponent(); try { ComponentUtil.fireEvent(owner, new UploadStartEvent(owner)); @@ -333,5 +357,14 @@ private static void handleUploadRequest(UploadHandler handler, } finally { ComponentUtil.fireEvent(owner, new UploadCompleteEvent(owner)); } + + if (event.isRejected()) { + getLogger().debug("File rejected: {} - {}", event.getFileName(), + event.getRejectionMessage()); + resultBuilder.addRejected(event.getFileName(), + event.getRejectionMessage()); + } else { + resultBuilder.addAccepted(event.getFileName()); + } } } diff --git a/flow-server/src/main/java/com/vaadin/flow/server/streams/FileUploadCallback.java b/flow-server/src/main/java/com/vaadin/flow/server/streams/FileUploadCallback.java index 6f1d15af6b0..0833b7ffd15 100644 --- a/flow-server/src/main/java/com/vaadin/flow/server/streams/FileUploadCallback.java +++ b/flow-server/src/main/java/com/vaadin/flow/server/streams/FileUploadCallback.java @@ -46,4 +46,27 @@ public interface FileUploadCallback extends Serializable { * if an I/O error occurs in the callback */ void complete(UploadMetadata metadata, File file) throws IOException; + + /** + * Called when a file upload is rejected. + *

+ * This method is invoked when {@link UploadEvent#reject()} or + * {@link UploadEvent#reject(String)} is called, allowing the handler to + * perform cleanup or logging for rejected uploads. + *

+ * The default implementation does nothing. Override this method to handle + * rejections. + * + * @param metadata + * the upload metadata containing relevant information about the + * rejected upload + * @param reason + * the reason for rejection + * @throws IOException + * if an I/O error occurs in the callback + */ + default void rejected(UploadMetadata metadata, String reason) + throws IOException { + // Default implementation does nothing + } } diff --git a/flow-server/src/main/java/com/vaadin/flow/server/streams/InMemoryUploadCallback.java b/flow-server/src/main/java/com/vaadin/flow/server/streams/InMemoryUploadCallback.java index db52099d98d..0925861e936 100644 --- a/flow-server/src/main/java/com/vaadin/flow/server/streams/InMemoryUploadCallback.java +++ b/flow-server/src/main/java/com/vaadin/flow/server/streams/InMemoryUploadCallback.java @@ -42,4 +42,27 @@ public interface InMemoryUploadCallback extends Serializable { * if an I/O error occurs in the callback */ void complete(UploadMetadata metadata, byte[] data) throws IOException; + + /** + * Called when a file upload is rejected. + *

+ * This method is invoked when {@link UploadEvent#reject()} or + * {@link UploadEvent#reject(String)} is called, allowing the handler to + * perform cleanup or logging for rejected uploads. + *

+ * The default implementation does nothing. Override this method to handle + * rejections. + * + * @param metadata + * the upload metadata containing relevant information about the + * rejected upload + * @param reason + * the reason for rejection + * @throws IOException + * if an I/O error occurs in the callback + */ + default void rejected(UploadMetadata metadata, String reason) + throws IOException { + // Default implementation does nothing + } } diff --git a/flow-server/src/main/java/com/vaadin/flow/server/streams/UploadEvent.java b/flow-server/src/main/java/com/vaadin/flow/server/streams/UploadEvent.java index 7ce0ccb0b32..8ac71f9f74f 100644 --- a/flow-server/src/main/java/com/vaadin/flow/server/streams/UploadEvent.java +++ b/flow-server/src/main/java/com/vaadin/flow/server/streams/UploadEvent.java @@ -52,6 +52,9 @@ public class UploadEvent { private final Part part; + private boolean rejected = false; + private String rejectionMessage; + /** * Create a new download event with required data. * @@ -90,8 +93,15 @@ public UploadEvent(VaadinRequest request, VaadinResponse response, * * @return the input stream from which the contents of the request can be * read + * @throws IllegalStateException + * if the upload has been rejected */ public InputStream getInputStream() { + if (rejected) { + throw new IllegalStateException( + "Cannot access input stream of rejected upload: " + + rejectionMessage); + } try { if (part != null) { return part.getInputStream(); @@ -201,4 +211,51 @@ private UI getUiFromSession(Component value) { session.unlock(); } } + + /** + * Rejects this upload with a default message. + *

+ * When called, the file will not be processed (or will be cleaned up if + * already processed) and the rejection will be communicated to the client. + * The default rejection message "File rejected" will be used. + * + * @see #reject(String) + */ + public void reject() { + reject("File rejected"); + } + + /** + * Rejects this upload with a custom message. + *

+ * When called, the file will not be processed (or will be cleaned up if + * already processed) and the rejection will be communicated to the client + * with the provided message. + * + * @param message + * the rejection message to send to the client + */ + public void reject(String message) { + this.rejected = true; + this.rejectionMessage = message; + } + + /** + * Checks whether this upload has been rejected. + * + * @return {@code true} if the upload has been rejected, {@code false} + * otherwise + */ + public boolean isRejected() { + return rejected; + } + + /** + * Gets the rejection message if this upload has been rejected. + * + * @return the rejection message, or {@code null} if not rejected + */ + public String getRejectionMessage() { + return rejectionMessage; + } } diff --git a/flow-server/src/main/java/com/vaadin/flow/server/streams/UploadHandler.java b/flow-server/src/main/java/com/vaadin/flow/server/streams/UploadHandler.java index 878e2bd40f4..538b0b4c879 100644 --- a/flow-server/src/main/java/com/vaadin/flow/server/streams/UploadHandler.java +++ b/flow-server/src/main/java/com/vaadin/flow/server/streams/UploadHandler.java @@ -16,8 +16,16 @@ package com.vaadin.flow.server.streams; import java.io.IOException; +import java.io.PrintWriter; +import java.util.List; + +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import tools.jackson.core.JacksonException; +import tools.jackson.databind.ObjectMapper; import com.vaadin.flow.dom.Element; +import com.vaadin.flow.internal.JacksonUtils; import com.vaadin.flow.server.HttpStatusCode; import com.vaadin.flow.server.VaadinRequest; import com.vaadin.flow.server.VaadinResponse; @@ -111,25 +119,78 @@ public interface UploadHandler extends ElementRequestHandler { * {@link UploadHandler#handleUploadRequest(UploadEvent)} methods have been * called for all files. *

- * This method sets the http response return codes according to internal - * exception handling in the framework. + * This method sets the HTTP response return codes and writes JSON responses + * for rejected files: + *

*

* If you want custom exception handling and to set the return code, * implement this method and overwrite the default functionality. * * @param result * the result of the upload operation containing success status, - * response object, and any exception that occurred + * response object, any exception that occurred, and lists of + * accepted/rejected files */ default void responseHandled(UploadResult result) { - if (result.success()) { - result.response().setStatus(HttpStatusCode.OK.getCode()); - } else { - result.response() - .setStatus(HttpStatusCode.INTERNAL_SERVER_ERROR.getCode()); + VaadinResponse response = result.response(); + try { + if (result.exception() != null) { + response.setStatus( + HttpStatusCode.INTERNAL_SERVER_ERROR.getCode()); + } else if (result.allRejected()) { + response.setStatus(422); // Unprocessable Entity + response.setContentType("application/json"); + writeJsonResponse(response, + new RejectedFilesResponse(result.rejectedFiles())); + } else if (result.hasMixed()) { + response.setStatus(207); // Multi-Status + response.setContentType("application/json"); + writeJsonResponse(response, new MixedUploadResponse( + result.acceptedFiles(), result.rejectedFiles())); + } else { + response.setStatus(HttpStatusCode.OK.getCode()); + } + } catch (IOException e) { + LoggerFactory.getLogger(UploadHandler.class) + .error("Error writing upload response", e); + response.setStatus(HttpStatusCode.INTERNAL_SERVER_ERROR.getCode()); + } + } + + private static void writeJsonResponse(VaadinResponse response, + Object responseObject) throws IOException { + ObjectMapper mapper = JacksonUtils.getMapper(); + try { + String json = mapper.writeValueAsString(responseObject); + PrintWriter writer = response.getWriter(); + writer.write(json); + } catch (JacksonException e) { + throw new IOException("Failed to serialize response to JSON", e); } } + /** + * JSON response structure for rejected files. + */ + record RejectedFilesResponse( + List rejected) implements + java.io.Serializable { + } + + /** + * JSON response structure for mixed upload results. + */ + record MixedUploadResponse(List accepted, + List rejected) implements + java.io.Serializable { + } + default void handleRequest(VaadinRequest request, VaadinResponse response, VaadinSession session, Element owner) throws IOException { TransferUtil.handleUpload(this, request, response, session, owner); diff --git a/flow-server/src/main/java/com/vaadin/flow/server/streams/UploadResult.java b/flow-server/src/main/java/com/vaadin/flow/server/streams/UploadResult.java index 3f0674eb875..f4c1cf800e4 100644 --- a/flow-server/src/main/java/com/vaadin/flow/server/streams/UploadResult.java +++ b/flow-server/src/main/java/com/vaadin/flow/server/streams/UploadResult.java @@ -16,6 +16,9 @@ package com.vaadin.flow.server.streams; import java.io.Serializable; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; import com.vaadin.flow.server.VaadinResponse; @@ -23,8 +26,8 @@ * Represents the result of an upload operation. *

* This record encapsulates the outcome of processing an upload request, - * including whether it was successful, the response object, and any exception - * that may have occurred. + * including whether it was successful, the response object, any exception that + * may have occurred, and lists of accepted and rejected files. * * @param success * {@code true} if the upload was successful, {@code false} otherwise @@ -33,12 +36,29 @@ * @param exception * the exception that caused the failure, or {@code null} if * successful or no exception available + * @param acceptedFiles + * list of file names that were accepted + * @param rejectedFiles + * list of rejected files with their rejection reasons */ public record UploadResult(boolean success, VaadinResponse response, - Exception exception) implements Serializable { + Exception exception, List acceptedFiles, + List rejectedFiles) implements Serializable { /** - * Creates an upload result without an exception. + * Represents a rejected file with its rejection reason. + * + * @param fileName + * the name of the rejected file + * @param reason + * the reason for rejection + */ + public record RejectedFile(String fileName, String reason) + implements Serializable { + } + + /** + * Creates an upload result without an exception or file tracking. * * @param success * {@code true} if the upload was successful, {@code false} @@ -47,6 +67,131 @@ public record UploadResult(boolean success, VaadinResponse response, * the response object for the upload request */ public UploadResult(boolean success, VaadinResponse response) { - this(success, response, null); + this(success, response, null, Collections.emptyList(), + Collections.emptyList()); + } + + /** + * Creates an upload result with an exception but no file tracking. + * + * @param success + * {@code true} if the upload was successful, {@code false} + * otherwise + * @param response + * the response object for the upload request + * @param exception + * the exception that caused the failure + */ + public UploadResult(boolean success, VaadinResponse response, + Exception exception) { + this(success, response, exception, Collections.emptyList(), + Collections.emptyList()); + } + + /** + * Checks if all files were accepted. + * + * @return {@code true} if there are accepted files and no rejected files + */ + public boolean allAccepted() { + return !acceptedFiles.isEmpty() && rejectedFiles.isEmpty(); + } + + /** + * Checks if all files were rejected. + * + * @return {@code true} if there are rejected files and no accepted files + */ + public boolean allRejected() { + return !rejectedFiles.isEmpty() && acceptedFiles.isEmpty(); + } + + /** + * Checks if there is a mix of accepted and rejected files. + * + * @return {@code true} if there are both accepted and rejected files + */ + public boolean hasMixed() { + return !acceptedFiles.isEmpty() && !rejectedFiles.isEmpty(); + } + + /** + * Checks if any files were processed. + * + * @return {@code true} if there are any accepted or rejected files + */ + public boolean hasFiles() { + return !acceptedFiles.isEmpty() || !rejectedFiles.isEmpty(); + } + + /** + * Builder class for constructing UploadResult instances with file tracking. + */ + public static class Builder { + private final VaadinResponse response; + private final List acceptedFiles = new ArrayList<>(); + private final List rejectedFiles = new ArrayList<>(); + private Exception exception; + + /** + * Creates a new builder for the given response. + * + * @param response + * the response object + */ + public Builder(VaadinResponse response) { + this.response = response; + } + + /** + * Adds an accepted file. + * + * @param fileName + * the name of the accepted file + * @return this builder + */ + public Builder addAccepted(String fileName) { + acceptedFiles.add(fileName); + return this; + } + + /** + * Adds a rejected file. + * + * @param fileName + * the name of the rejected file + * @param reason + * the reason for rejection + * @return this builder + */ + public Builder addRejected(String fileName, String reason) { + rejectedFiles.add(new RejectedFile(fileName, reason)); + return this; + } + + /** + * Sets an exception that occurred during upload. + * + * @param exception + * the exception + * @return this builder + */ + public Builder withException(Exception exception) { + this.exception = exception; + return this; + } + + /** + * Builds the UploadResult. + * + * @return the constructed UploadResult + */ + public UploadResult build() { + boolean success = !acceptedFiles.isEmpty() || rejectedFiles.isEmpty(); + return new UploadResult(success, response, exception, + Collections.unmodifiableList(new ArrayList<>(acceptedFiles)), + Collections.unmodifiableList( + new ArrayList<>(rejectedFiles))); + } } } diff --git a/flow-server/src/test/java/com/vaadin/flow/server/communication/UploadHandlerTest.java b/flow-server/src/test/java/com/vaadin/flow/server/communication/UploadHandlerTest.java index 22bc1d74f42..c03609d9560 100644 --- a/flow-server/src/test/java/com/vaadin/flow/server/communication/UploadHandlerTest.java +++ b/flow-server/src/test/java/com/vaadin/flow/server/communication/UploadHandlerTest.java @@ -756,6 +756,214 @@ public void inmemoryUploadCallback_doesNotRequireCatch() { }); } + @Test + public void xhrUpload_earlyRejection_returns422WithJson() + throws IOException { + UploadHandler handler = (event) -> { + if (!event.getFileName().endsWith(".png")) { + event.reject("Only PNG files are accepted"); + } + }; + + Mockito.when(request.getHeader("X-Filename")).thenReturn("test.zip"); + Mockito.when(response.getWriter()) + .thenReturn(Mockito.mock(java.io.PrintWriter.class)); + + handler.handleRequest(request, response, session, element); + + Mockito.verify(response).setStatus(422); + Mockito.verify(response).setContentType("application/json"); + } + + @Test + public void xhrUpload_noRejection_returns200() + throws IOException { + UploadHandler handler = (event) -> { + // Accept the file + }; + + Mockito.when(request.getHeader("X-Filename")).thenReturn("test.png"); + + handler.handleRequest(request, response, session, element); + + Mockito.verify(response).setStatus(200); + } + + @Test + public void xhrUpload_rejectionWithDefaultMessage_usesDefaultMessage() + throws IOException { + AtomicBoolean rejected = new AtomicBoolean(false); + + UploadHandler handler = (event) -> { + if (event.getFileName().endsWith(".zip")) { + event.reject(); + rejected.set(true); + } + }; + + Mockito.when(request.getHeader("X-Filename")).thenReturn("test.zip"); + Mockito.when(response.getWriter()) + .thenReturn(Mockito.mock(java.io.PrintWriter.class)); + + handler.handleRequest(request, response, session, element); + + Assert.assertTrue("File should have been rejected", rejected.get()); + Mockito.verify(response).setStatus(422); + } + + @Test + public void multipartUpload_mixedAcceptReject_returns207WithJson() + throws IOException, ServletException { + List parts = new ArrayList<>(); + parts.add(createPart(createInputStream("one"), MULTIPART_CONTENT_TYPE, + "file1.png", 3)); + parts.add(createPart(createInputStream("two"), MULTIPART_CONTENT_TYPE, + "file2.zip", 3)); + parts.add(createPart(createInputStream("three"), + MULTIPART_CONTENT_TYPE, "file3.png", 5)); + + Mockito.when(request.getParts()).thenReturn(parts); + + List processedFiles = new ArrayList<>(); + + UploadHandler uploadHandler = (event) -> { + if (event.getFileName().endsWith(".zip")) { + event.reject("ZIP files are not allowed"); + } else { + processedFiles.add(event.getFileName()); + } + }; + + StreamRegistration streamRegistration = streamResourceRegistry + .registerResource(uploadHandler); + AbstractStreamResource res = streamRegistration.getResource(); + + mockRequest(res, "testContent"); + Mockito.when(request.getContentType()) + .thenReturn(MULTIPART_CONTENT_TYPE); + Mockito.when(response.getWriter()) + .thenReturn(Mockito.mock(java.io.PrintWriter.class)); + + handler.handleRequest(session, request, response); + + // Should have processed 2 PNG files + Assert.assertEquals("Two files should have been accepted", 2, + processedFiles.size()); + Assert.assertTrue("file1.png should be in processed files", + processedFiles.contains("file1.png")); + Assert.assertTrue("file3.png should be in processed files", + processedFiles.contains("file3.png")); + + // Should return 207 Multi-Status for mixed results + Mockito.verify(response).setStatus(207); + Mockito.verify(response).setContentType("application/json"); + } + + @Test + public void multipartUpload_allRejected_returns422() + throws IOException, ServletException { + List parts = new ArrayList<>(); + parts.add(createPart(createInputStream("one"), MULTIPART_CONTENT_TYPE, + "file1.zip", 3)); + parts.add(createPart(createInputStream("two"), MULTIPART_CONTENT_TYPE, + "file2.exe", 3)); + + Mockito.when(request.getParts()).thenReturn(parts); + + UploadHandler uploadHandler = (event) -> { + event.reject("File type not allowed"); + }; + + StreamRegistration streamRegistration = streamResourceRegistry + .registerResource(uploadHandler); + AbstractStreamResource res = streamRegistration.getResource(); + + mockRequest(res, "testContent"); + Mockito.when(request.getContentType()) + .thenReturn(MULTIPART_CONTENT_TYPE); + Mockito.when(response.getWriter()) + .thenReturn(Mockito.mock(java.io.PrintWriter.class)); + + handler.handleRequest(session, request, response); + + // Should return 422 for all rejected + Mockito.verify(response).setStatus(422); + Mockito.verify(response).setContentType("application/json"); + } + + @Test + public void multipartUpload_allAccepted_returns200() + throws IOException, ServletException { + List parts = new ArrayList<>(); + parts.add(createPart(createInputStream("one"), MULTIPART_CONTENT_TYPE, + "file1.png", 3)); + parts.add(createPart(createInputStream("two"), MULTIPART_CONTENT_TYPE, + "file2.png", 3)); + + Mockito.when(request.getParts()).thenReturn(parts); + + UploadHandler uploadHandler = (event) -> { + // Accept all files + }; + + StreamRegistration streamRegistration = streamResourceRegistry + .registerResource(uploadHandler); + AbstractStreamResource res = streamRegistration.getResource(); + + mockRequest(res, "testContent"); + Mockito.when(request.getContentType()) + .thenReturn(MULTIPART_CONTENT_TYPE); + + handler.handleRequest(session, request, response); + + // Should return 200 for all accepted + Mockito.verify(response).setStatus(200); + } + + @Test + public void multipartUpload_earlyRejection_fileNotProcessed() + throws IOException, ServletException { + List parts = new ArrayList<>(); + Part rejectedPart = createPart(createInputStream("content"), + MULTIPART_CONTENT_TYPE, "file.zip", 7); + parts.add(rejectedPart); + + Mockito.when(request.getParts()).thenReturn(parts); + + AtomicBoolean inputStreamAccessed = new AtomicBoolean(false); + + UploadHandler uploadHandler = (event) -> { + if (event.getFileName().endsWith(".zip")) { + event.reject("ZIP files not allowed"); + } else { + // This should not be reached for rejected files + try { + event.getInputStream().read(); + inputStreamAccessed.set(true); + } catch (IOException e) { + throw new RuntimeException(e); + } + } + }; + + StreamRegistration streamRegistration = streamResourceRegistry + .registerResource(uploadHandler); + AbstractStreamResource res = streamRegistration.getResource(); + + mockRequest(res, "testContent"); + Mockito.when(request.getContentType()) + .thenReturn(MULTIPART_CONTENT_TYPE); + Mockito.when(response.getWriter()) + .thenReturn(Mockito.mock(java.io.PrintWriter.class)); + + handler.handleRequest(session, request, response); + + Assert.assertFalse( + "Input stream should not be accessed for rejected file", + inputStreamAccessed.get()); + Mockito.verify(response).setStatus(422); + } + private Part createPart(InputStream inputStream, String contentType, String name, long size) throws IOException { Part part = mock(Part.class); diff --git a/flow-server/src/test/java/com/vaadin/flow/server/streams/UploadEventTest.java b/flow-server/src/test/java/com/vaadin/flow/server/streams/UploadEventTest.java new file mode 100644 index 00000000000..53dfdb534d5 --- /dev/null +++ b/flow-server/src/test/java/com/vaadin/flow/server/streams/UploadEventTest.java @@ -0,0 +1,172 @@ +/* + * Copyright 2000-2025 Vaadin Ltd. + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not + * use this file except in compliance with the License. You may obtain a copy of + * the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations under + * the License. + */ +package com.vaadin.flow.server.streams; + +import java.io.ByteArrayInputStream; +import java.io.IOException; + +import org.junit.Assert; +import org.junit.Before; +import org.junit.Test; +import org.mockito.Mockito; + +import com.vaadin.flow.dom.Element; +import com.vaadin.flow.server.VaadinRequest; +import com.vaadin.flow.server.VaadinResponse; +import com.vaadin.flow.server.VaadinSession; + +/** + * Unit tests for {@link UploadEvent} rejection functionality. + */ +public class UploadEventTest { + + private VaadinRequest request; + private VaadinResponse response; + private VaadinSession session; + private Element owner; + + @Before + public void setUp() throws IOException { + request = Mockito.mock(VaadinRequest.class); + response = Mockito.mock(VaadinResponse.class); + session = Mockito.mock(VaadinSession.class); + owner = Mockito.mock(Element.class); + + Mockito.when(request.getInputStream()) + .thenReturn(new ByteArrayInputStream(new byte[0])); + } + + @Test + public void testInitialState_notRejected() { + UploadEvent event = new UploadEvent(request, response, session, + "test.txt", 100L, "text/plain", owner, null); + + Assert.assertFalse("Event should not be rejected initially", + event.isRejected()); + Assert.assertNull("Rejection message should be null initially", + event.getRejectionMessage()); + } + + @Test + public void testReject_withDefaultMessage() { + UploadEvent event = new UploadEvent(request, response, session, + "test.txt", 100L, "text/plain", owner, null); + + event.reject(); + + Assert.assertTrue("Event should be marked as rejected", + event.isRejected()); + Assert.assertEquals("Default rejection message should be set", + "File rejected", event.getRejectionMessage()); + } + + @Test + public void testReject_withCustomMessage() { + UploadEvent event = new UploadEvent(request, response, session, + "test.zip", 100L, "application/zip", owner, null); + + String customMessage = "Only PNG files are accepted"; + event.reject(customMessage); + + Assert.assertTrue("Event should be marked as rejected", + event.isRejected()); + Assert.assertEquals("Custom rejection message should be set", + customMessage, event.getRejectionMessage()); + } + + @Test + public void testReject_canBeCalledMultipleTimes() { + UploadEvent event = new UploadEvent(request, response, session, + "test.txt", 100L, "text/plain", owner, null); + + event.reject("First reason"); + Assert.assertTrue("Event should be marked as rejected", + event.isRejected()); + Assert.assertEquals("First rejection message should be set", + "First reason", event.getRejectionMessage()); + + // Call reject again with different message + event.reject("Second reason"); + Assert.assertTrue("Event should still be marked as rejected", + event.isRejected()); + Assert.assertEquals("Rejection message should be updated", + "Second reason", event.getRejectionMessage()); + } + + @Test + public void testGetFileName_returnsCorrectFileName() { + String fileName = "document.pdf"; + UploadEvent event = new UploadEvent(request, response, session, + fileName, 1000L, "application/pdf", owner, null); + + Assert.assertEquals("File name should match", fileName, + event.getFileName()); + } + + @Test + public void testReject_withNullMessage_setsNullMessage() { + UploadEvent event = new UploadEvent(request, response, session, + "test.txt", 100L, "text/plain", owner, null); + + event.reject(null); + + Assert.assertTrue("Event should be marked as rejected", + event.isRejected()); + Assert.assertNull("Rejection message should be null", + event.getRejectionMessage()); + } + + @Test + public void testReject_withEmptyMessage_setsEmptyMessage() { + UploadEvent event = new UploadEvent(request, response, session, + "test.txt", 100L, "text/plain", owner, null); + + event.reject(""); + + Assert.assertTrue("Event should be marked as rejected", + event.isRejected()); + Assert.assertEquals("Rejection message should be empty", "", + event.getRejectionMessage()); + } + + @Test + public void testGetInputStream_rejectedUpload_throwsException() { + UploadEvent event = new UploadEvent(request, response, session, + "test.txt", 100L, "text/plain", owner, null); + + event.reject("Not allowed"); + + try { + event.getInputStream(); + Assert.fail( + "Expected IllegalStateException when accessing rejected upload stream"); + } catch (IllegalStateException e) { + Assert.assertTrue("Exception should mention rejection", + e.getMessage().contains("rejected")); + Assert.assertTrue("Exception should include rejection reason", + e.getMessage().contains("Not allowed")); + } + } + + @Test + public void testGetInputStream_beforeRejection_works() { + UploadEvent event = new UploadEvent(request, response, session, + "test.txt", 100L, "text/plain", owner, null); + + Assert.assertNotNull("Should be able to get input stream", + event.getInputStream()); + } +} From bc1544eaeeb6014b1d13d3a6e9a485e11997f4aa Mon Sep 17 00:00:00 2001 From: Artur Signell Date: Fri, 28 Nov 2025 15:33:38 +0200 Subject: [PATCH 2/2] refactor: simplify upload result handling by removing Builder pattern Remove unnecessary UploadResult.Builder class and use direct list accumulation in TransferUtil. This simplifies the code flow by eliminating mutable state passed as method parameters, making the upload handling logic clearer and more maintainable. --- .../server/communication/TransferUtil.java | 62 ++++++++------- .../server/streams/FileUploadCallback.java | 23 ------ .../streams/InMemoryUploadCallback.java | 23 ------ .../flow/server/streams/UploadHandler.java | 12 +-- .../flow/server/streams/UploadResult.java | 76 +------------------ .../communication/UploadHandlerTest.java | 7 +- 6 files changed, 44 insertions(+), 159 deletions(-) diff --git a/flow-server/src/main/java/com/vaadin/flow/server/communication/TransferUtil.java b/flow-server/src/main/java/com/vaadin/flow/server/communication/TransferUtil.java index 8a83c9362fd..a63185b0261 100644 --- a/flow-server/src/main/java/com/vaadin/flow/server/communication/TransferUtil.java +++ b/flow-server/src/main/java/com/vaadin/flow/server/communication/TransferUtil.java @@ -23,9 +23,11 @@ import java.io.InputStream; import java.io.OutputStream; import java.io.UncheckedIOException; +import java.util.ArrayList; import java.util.Collection; import java.util.Collections; import java.util.HashMap; +import java.util.List; import java.util.Map; import java.util.Objects; @@ -53,9 +55,6 @@ */ public final class TransferUtil { - private static final Logger logger = LoggerFactory - .getLogger(TransferUtil.class); - /** * Default buffer size for reading data from the input stream. *

@@ -65,7 +64,7 @@ public final class TransferUtil { public static int DEFAULT_BUFFER_SIZE = 16384; private static Logger getLogger() { - return logger; + return LoggerFactory.getLogger(TransferUtil.class); } /** @@ -154,7 +153,8 @@ public static void handleUpload(UploadHandler handler, VaadinRequest request, VaadinResponse response, VaadinSession session, Element owner) { boolean isMultipartUpload = isMultipartContent(request); - UploadResult.Builder resultBuilder = new UploadResult.Builder(response); + List acceptedFiles = new ArrayList<>(); + List rejectedFiles = new ArrayList<>(); try { if (isMultipartUpload) { Collection parts = Collections.EMPTY_LIST; @@ -174,9 +174,18 @@ public static void handleUpload(UploadHandler handler, part.getSize(), part.getContentType(), owner, part); - handleUploadRequest(handler, event, resultBuilder); + handleUploadRequest(handler, event); + + if (event.isRejected()) { + rejectedFiles.add(new UploadResult.RejectedFile( + event.getFileName(), + event.getRejectionMessage())); + } else { + acceptedFiles.add(event.getFileName()); + } } - handler.responseHandled(resultBuilder.build()); + handler.responseHandled(new UploadResult(true, response, + null, acceptedFiles, rejectedFiles)); } else { LoggerFactory.getLogger(UploadHandler.class) .warn("Multipart request has no parts"); @@ -190,8 +199,16 @@ public static void handleUpload(UploadHandler handler, fileName, request.getContentLengthLong(), contentType, owner, null); - handleUploadRequest(handler, event, resultBuilder); - handler.responseHandled(resultBuilder.build()); + handleUploadRequest(handler, event); + + if (event.isRejected()) { + rejectedFiles.add(new UploadResult.RejectedFile( + event.getFileName(), event.getRejectionMessage())); + } else { + acceptedFiles.add(event.getFileName()); + } + handler.responseHandled(new UploadResult(true, response, null, + acceptedFiles, rejectedFiles)); } } catch (UploadSizeLimitExceededException | UploadFileSizeLimitExceededException @@ -206,18 +223,17 @@ public static void handleUpload(UploadHandler handler, getLogger().warn(limitInfoStr + " File: {}", "File size", "getFileSizeMax", fileSizeException.getFileName()); } else if (e instanceof UploadFileCountLimitExceededException) { - getLogger().warn(limitInfoStr, "File count", - "getFileCountMax"); + getLogger().warn(limitInfoStr, "File count", "getFileCountMax"); } LoggerFactory.getLogger(UploadHandler.class) .warn("File upload failed.", e); - handler.responseHandled( - resultBuilder.withException(e).build()); + handler.responseHandled(new UploadResult(false, response, e, + acceptedFiles, rejectedFiles)); } catch (Exception e) { LoggerFactory.getLogger(UploadHandler.class) .error("Exception during upload", e); - handler.responseHandled( - resultBuilder.withException(e).build()); + handler.responseHandled(new UploadResult(false, response, e, + acceptedFiles, rejectedFiles)); } } @@ -336,20 +352,17 @@ private static void validateUploadLimits(UploadHandler handler, } /** - * Handles an upload request and checks for rejection. + * Handles an upload request. * * @param handler * the upload handler * @param event * the upload event - * @param resultBuilder - * the upload result builder to update with acceptance/rejection * @throws IOException * if an I/O error occurs */ private static void handleUploadRequest(UploadHandler handler, - UploadEvent event, UploadResult.Builder resultBuilder) - throws IOException { + UploadEvent event) throws IOException { Component owner = event.getOwningComponent(); try { ComponentUtil.fireEvent(owner, new UploadStartEvent(owner)); @@ -357,14 +370,5 @@ private static void handleUploadRequest(UploadHandler handler, } finally { ComponentUtil.fireEvent(owner, new UploadCompleteEvent(owner)); } - - if (event.isRejected()) { - getLogger().debug("File rejected: {} - {}", event.getFileName(), - event.getRejectionMessage()); - resultBuilder.addRejected(event.getFileName(), - event.getRejectionMessage()); - } else { - resultBuilder.addAccepted(event.getFileName()); - } } } diff --git a/flow-server/src/main/java/com/vaadin/flow/server/streams/FileUploadCallback.java b/flow-server/src/main/java/com/vaadin/flow/server/streams/FileUploadCallback.java index 0833b7ffd15..6f1d15af6b0 100644 --- a/flow-server/src/main/java/com/vaadin/flow/server/streams/FileUploadCallback.java +++ b/flow-server/src/main/java/com/vaadin/flow/server/streams/FileUploadCallback.java @@ -46,27 +46,4 @@ public interface FileUploadCallback extends Serializable { * if an I/O error occurs in the callback */ void complete(UploadMetadata metadata, File file) throws IOException; - - /** - * Called when a file upload is rejected. - *

- * This method is invoked when {@link UploadEvent#reject()} or - * {@link UploadEvent#reject(String)} is called, allowing the handler to - * perform cleanup or logging for rejected uploads. - *

- * The default implementation does nothing. Override this method to handle - * rejections. - * - * @param metadata - * the upload metadata containing relevant information about the - * rejected upload - * @param reason - * the reason for rejection - * @throws IOException - * if an I/O error occurs in the callback - */ - default void rejected(UploadMetadata metadata, String reason) - throws IOException { - // Default implementation does nothing - } } diff --git a/flow-server/src/main/java/com/vaadin/flow/server/streams/InMemoryUploadCallback.java b/flow-server/src/main/java/com/vaadin/flow/server/streams/InMemoryUploadCallback.java index 0925861e936..db52099d98d 100644 --- a/flow-server/src/main/java/com/vaadin/flow/server/streams/InMemoryUploadCallback.java +++ b/flow-server/src/main/java/com/vaadin/flow/server/streams/InMemoryUploadCallback.java @@ -42,27 +42,4 @@ public interface InMemoryUploadCallback extends Serializable { * if an I/O error occurs in the callback */ void complete(UploadMetadata metadata, byte[] data) throws IOException; - - /** - * Called when a file upload is rejected. - *

- * This method is invoked when {@link UploadEvent#reject()} or - * {@link UploadEvent#reject(String)} is called, allowing the handler to - * perform cleanup or logging for rejected uploads. - *

- * The default implementation does nothing. Override this method to handle - * rejections. - * - * @param metadata - * the upload metadata containing relevant information about the - * rejected upload - * @param reason - * the reason for rejection - * @throws IOException - * if an I/O error occurs in the callback - */ - default void rejected(UploadMetadata metadata, String reason) - throws IOException { - // Default implementation does nothing - } } diff --git a/flow-server/src/main/java/com/vaadin/flow/server/streams/UploadHandler.java b/flow-server/src/main/java/com/vaadin/flow/server/streams/UploadHandler.java index 538b0b4c879..07020838fe8 100644 --- a/flow-server/src/main/java/com/vaadin/flow/server/streams/UploadHandler.java +++ b/flow-server/src/main/java/com/vaadin/flow/server/streams/UploadHandler.java @@ -19,7 +19,6 @@ import java.io.PrintWriter; import java.util.List; -import org.slf4j.Logger; import org.slf4j.LoggerFactory; import tools.jackson.core.JacksonException; import tools.jackson.databind.ObjectMapper; @@ -178,17 +177,18 @@ private static void writeJsonResponse(VaadinResponse response, /** * JSON response structure for rejected files. */ - record RejectedFilesResponse( - List rejected) implements - java.io.Serializable { + record RejectedFilesResponse(List rejected) + implements + java.io.Serializable { } /** * JSON response structure for mixed upload results. */ record MixedUploadResponse(List accepted, - List rejected) implements - java.io.Serializable { + List rejected) + implements + java.io.Serializable { } default void handleRequest(VaadinRequest request, VaadinResponse response, diff --git a/flow-server/src/main/java/com/vaadin/flow/server/streams/UploadResult.java b/flow-server/src/main/java/com/vaadin/flow/server/streams/UploadResult.java index f4c1cf800e4..7b473e0c8b9 100644 --- a/flow-server/src/main/java/com/vaadin/flow/server/streams/UploadResult.java +++ b/flow-server/src/main/java/com/vaadin/flow/server/streams/UploadResult.java @@ -16,7 +16,6 @@ package com.vaadin.flow.server.streams; import java.io.Serializable; -import java.util.ArrayList; import java.util.Collections; import java.util.List; @@ -53,8 +52,8 @@ public record UploadResult(boolean success, VaadinResponse response, * @param reason * the reason for rejection */ - public record RejectedFile(String fileName, String reason) - implements Serializable { + public record RejectedFile(String fileName, + String reason) implements Serializable { } /** @@ -123,75 +122,4 @@ public boolean hasMixed() { public boolean hasFiles() { return !acceptedFiles.isEmpty() || !rejectedFiles.isEmpty(); } - - /** - * Builder class for constructing UploadResult instances with file tracking. - */ - public static class Builder { - private final VaadinResponse response; - private final List acceptedFiles = new ArrayList<>(); - private final List rejectedFiles = new ArrayList<>(); - private Exception exception; - - /** - * Creates a new builder for the given response. - * - * @param response - * the response object - */ - public Builder(VaadinResponse response) { - this.response = response; - } - - /** - * Adds an accepted file. - * - * @param fileName - * the name of the accepted file - * @return this builder - */ - public Builder addAccepted(String fileName) { - acceptedFiles.add(fileName); - return this; - } - - /** - * Adds a rejected file. - * - * @param fileName - * the name of the rejected file - * @param reason - * the reason for rejection - * @return this builder - */ - public Builder addRejected(String fileName, String reason) { - rejectedFiles.add(new RejectedFile(fileName, reason)); - return this; - } - - /** - * Sets an exception that occurred during upload. - * - * @param exception - * the exception - * @return this builder - */ - public Builder withException(Exception exception) { - this.exception = exception; - return this; - } - - /** - * Builds the UploadResult. - * - * @return the constructed UploadResult - */ - public UploadResult build() { - boolean success = !acceptedFiles.isEmpty() || rejectedFiles.isEmpty(); - return new UploadResult(success, response, exception, - Collections.unmodifiableList(new ArrayList<>(acceptedFiles)), - Collections.unmodifiableList( - new ArrayList<>(rejectedFiles))); - } - } } diff --git a/flow-server/src/test/java/com/vaadin/flow/server/communication/UploadHandlerTest.java b/flow-server/src/test/java/com/vaadin/flow/server/communication/UploadHandlerTest.java index c03609d9560..6524f0c0d9b 100644 --- a/flow-server/src/test/java/com/vaadin/flow/server/communication/UploadHandlerTest.java +++ b/flow-server/src/test/java/com/vaadin/flow/server/communication/UploadHandlerTest.java @@ -776,8 +776,7 @@ public void xhrUpload_earlyRejection_returns422WithJson() } @Test - public void xhrUpload_noRejection_returns200() - throws IOException { + public void xhrUpload_noRejection_returns200() throws IOException { UploadHandler handler = (event) -> { // Accept the file }; @@ -819,8 +818,8 @@ public void multipartUpload_mixedAcceptReject_returns207WithJson() "file1.png", 3)); parts.add(createPart(createInputStream("two"), MULTIPART_CONTENT_TYPE, "file2.zip", 3)); - parts.add(createPart(createInputStream("three"), - MULTIPART_CONTENT_TYPE, "file3.png", 5)); + parts.add(createPart(createInputStream("three"), MULTIPART_CONTENT_TYPE, + "file3.png", 5)); Mockito.when(request.getParts()).thenReturn(parts);