Prevent upload if no file is selected. (#11939)

Fixes #10419

client/src/main/java/com/vaadin/client/ui/VUpload.java

@@ -58,8 +58,7 @@ public class VUpload extends SimplePanel {
         public void onBrowserEvent(Event event) {
             super.onBrowserEvent(event);
             if (event.getTypeInt() == Event.ONCHANGE) {
-                if (isImmediateMode() && fu.getFilename() != null
-                        && !fu.getFilename().isEmpty()) {
+                if (isImmediateMode() && hasFilename()) {
                     submit();
                 }
             } else if (BrowserInfo.get().isIE()
@@ -122,6 +121,15 @@ public class VUpload extends SimplePanel {
 
     private boolean immediateMode;
 
+    /**
+     * Just-in-case option to override the default assumption that if no file
+     * has been selected, no upload attempt should happen. Not part of public
+     * API so can be removed without a warning -- if you have an actual need for
+     * this feature (which is currently only accessible through violator
+     * pattern), let us know.
+     */
+    private boolean allowUploadWithoutFilename = false;
+
     private String acceptMimeTypes;
 
     private Hidden maxfilesize = new Hidden();
@@ -144,6 +152,19 @@ public class VUpload extends SimplePanel {
         setWidget(panel);
         panel.add(maxfilesize);
         panel.add(fu);
+        fu.addChangeHandler(event -> {
+            if (!isImmediateMode()) {
+                updateEnabledForSubmitButton();
+            }
+            if (client != null) {
+                UploadConnector connector = ((UploadConnector) ConnectorMap
+                        .get(client).getConnector(VUpload.this));
+                if (connector.hasEventListener(EventId.CHANGE)) {
+                    connector.getRpcProxy(UploadServerRpc.class)
+                            .change(fu.getFilename());
+                }
+            }
+        });
         submitButton = new VButton();
         submitButton.addClickHandler(event -> {
             if (isImmediateMode()) {
@@ -180,6 +201,7 @@ public class VUpload extends SimplePanel {
                 fu.unsinkEvents(Event.ONCHANGE);
                 fu.unsinkEvents(Event.ONFOCUS);
             }
+            updateEnabledForSubmitButton();
         }
         setStyleName(getElement(), CLASSNAME + "-immediate", immediateMode);
     }
@@ -205,20 +227,20 @@ public class VUpload extends SimplePanel {
 
     /** For internal use only. May be removed or replaced in the future. */
     public void disableUpload() {
-        setEnabledForSubmitButton(false);
         if (!submitted) {
             // Cannot disable the fileupload while submitting or the file won't
             // be submitted at all
             fu.getElement().setPropertyBoolean("disabled", true);
         }
         enabled = false;
+        updateEnabledForSubmitButton();
     }
 
     /** For internal use only. May be removed or replaced in the future. */
     public void enableUpload() {
-        setEnabledForSubmitButton(true);
         fu.getElement().setPropertyBoolean("disabled", false);
         enabled = true;
+        updateEnabledForSubmitButton();
         if (submitted) {
             /*
              * An old request is still in progress (most likely cancelled),
@@ -242,9 +264,27 @@ public class VUpload extends SimplePanel {
         }
     }
 
-    private void setEnabledForSubmitButton(boolean enabled) {
-        submitButton.setEnabled(enabled);
-        submitButton.setStyleName(StyleConstants.DISABLED, !enabled);
+    /**
+     * Updates the enabled status for submit button. If the widget itself is
+     * disabled, so is the submit button. It must also follow overall enabled
+     * status in immediate mode, otherwise you cannot select a file at all. In
+     * non-immediate mode there is another button for selecting the file, so the
+     * submit button should be disabled until a file has been selected, unless
+     * upload without selection has been specifically allowed.
+     */
+    private void updateEnabledForSubmitButton() {
+        if (enabled && (isImmediateMode() || hasFilename()
+                || allowUploadWithoutFilename)) {
+            submitButton.setEnabled(true);
+            submitButton.setStyleName(StyleConstants.DISABLED, false);
+        } else {
+            submitButton.setEnabled(false);
+            submitButton.setStyleName(StyleConstants.DISABLED, true);
+        }
+    }
+
+    private boolean hasFilename() {
+        return fu.getFilename() != null && !fu.getFilename().isEmpty();
     }
 
     /**
@@ -266,6 +306,9 @@ public class VUpload extends SimplePanel {
             fu.sinkEvents(Event.ONCHANGE);
         }
         fu.addChangeHandler(event -> {
+            if (!isImmediateMode()) {
+                updateEnabledForSubmitButton();
+            }
             if (client != null) {
                 UploadConnector connector = ((UploadConnector) ConnectorMap
                         .get(client).getConnector(VUpload.this));
@@ -350,7 +393,10 @@ public class VUpload extends SimplePanel {
                     .info("Submit cancelled (disabled or already submitted)");
             return;
         }
-        if (fu.getFilename().isEmpty()) {
+        if (!hasFilename()) {
+            if (!allowUploadWithoutFilename) {
+                return;
+            }
             getLogger().info("Submitting empty selection (no file)");
         }
         // flush possibly pending variable changes, so they will be handled

client/src/main/java/com/vaadin/client/ui/upload/UploadConnector.java

@@ -22,10 +22,8 @@ import com.vaadin.client.UIDL;
 import com.vaadin.client.communication.StateChangeEvent;
 import com.vaadin.client.ui.AbstractComponentConnector;
 import com.vaadin.client.ui.VUpload;
-import com.vaadin.shared.EventId;
 import com.vaadin.shared.ui.Connect;
 import com.vaadin.shared.ui.upload.UploadClientRpc;
-import com.vaadin.shared.ui.upload.UploadServerRpc;
 import com.vaadin.shared.ui.upload.UploadState;
 import com.vaadin.ui.Upload;
 
@@ -37,18 +35,6 @@ public class UploadConnector extends AbstractComponentConnector
         registerRpc(UploadClientRpc.class, () -> getWidget().submit());
     }
 
-    @Override
-    protected void init() {
-        super.init();
-
-        getWidget().fu.addChangeHandler(event -> {
-            if (hasEventListener(EventId.CHANGE)) {
-                getRpcProxy(UploadServerRpc.class)
-                        .change(getWidget().fu.getFilename());
-            }
-        });
-    }
-
     @Override
     public void updateFromUIDL(UIDL uidl, ApplicationConnection client) {
         if (!isRealUpdate(uidl)) {

server/src/main/java/com/vaadin/ui/Upload.java

@@ -1038,21 +1038,20 @@ public class Upload extends AbstractComponent
     }
 
     /**
-     * Forces the upload the send selected file to the server.
+     * Instructs the upload component to send selected file to the server.
      * <p>
      * In case developer wants to use this feature, he/she will most probably
-     * want to hide the uploads internal submit button by setting its caption to
-     * null with {@link #setButtonCaption(String)} method.
+     * want to hide the upload component's internal submit button by setting its
+     * caption to null with {@link #setButtonCaption(String)} method.
      * <p>
-     * Note, that the upload runs asynchronous. Developer should use normal
-     * upload listeners to trac the process of upload. If the field is empty
-     * uploaded the file name will be empty string and file length 0 in the
-     * upload finished event.
+     * Note that the upload runs asynchronously. Developer should use normal
+     * upload listeners to track the process of upload. If the file name field
+     * is empty, no upload will be triggered.
      * <p>
-     * Also note, that the developer should not remove or modify the upload in
-     * the same user transaction where the upload submit is requested. The
-     * upload may safely be hidden or removed once the upload started event is
-     * fired.
+     * Also note that the developer should not remove or modify the upload
+     * component in the same user transaction where the upload submit is
+     * requested. The upload component can be safely hidden or removed once the
+     * upload started event has been fired.
      */
     public void submitUpload() {
         markAsDirty();

shared/src/main/java/com/vaadin/shared/ui/upload/UploadClientRpc.java

@@ -17,10 +17,13 @@ package com.vaadin.shared.ui.upload;
 
 import com.vaadin.shared.communication.ClientRpc;
 
+/**
+ * Server-to-client RPC interface for Upload.
+ */
 public interface UploadClientRpc extends ClientRpc {
 
     /**
-     * Forces the upload the send selected file to the server.
+     * Instructs the upload component to send the selected file to the server.
      */
     void submitUpload();
 }

shared/src/main/java/com/vaadin/shared/ui/upload/UploadServerRpc.java

@@ -17,6 +17,9 @@ package com.vaadin.shared.ui.upload;
 
 import com.vaadin.shared.communication.ServerRpc;
 
+/**
+ * Client-to-server RPC interface for Upload.
+ */
 public interface UploadServerRpc extends ServerRpc {
 
     /**

uitest/src/main/java/com/vaadin/tests/components/ui/MultiFileUploadTest.java

@@ -68,6 +68,7 @@ public class MultiFileUploadTest extends AbstractTestUIWithLog {
 
     private Upload createUpload() {
         Upload upload = new Upload();
+        upload.setImmediateMode(false);
         upload.addChangeListener(changeListener);
         return upload;
     }

uitest/src/main/java/com/vaadin/tests/components/upload/UploadNoSelection.java

@@ -3,11 +3,16 @@ package com.vaadin.tests.components.upload;
 import java.io.ByteArrayOutputStream;
 import java.io.OutputStream;
 
+import com.vaadin.annotations.Widgetset;
 import com.vaadin.server.VaadinRequest;
 import com.vaadin.tests.components.AbstractTestUIWithLog;
+import com.vaadin.tests.widgetset.TestingWidgetSet;
+import com.vaadin.tests.widgetset.server.upload.AllowUploadWithoutFilenameExtension;
+import com.vaadin.ui.Button;
 import com.vaadin.ui.Upload;
 import com.vaadin.ui.Upload.Receiver;
 
+@Widgetset(TestingWidgetSet.NAME)
 public class UploadNoSelection extends AbstractTestUIWithLog
         implements Receiver {
 
@@ -27,7 +32,10 @@ public class UploadNoSelection extends AbstractTestUIWithLog
 
     @Override
     protected String getTestDescription() {
-        return "Uploading an empty selection (no file) will trigger FinishedEvent with 0-length file size and empty filename.";
+        return "Uploading an empty selection (no file) should not be possible by "
+                + "default. If the default behavior is overridden with a custom "
+                + "extension the upload attempt will trigger FinishedEvent with "
+                + "0-length file size and empty filename.";
     }
 
     @Override
@@ -49,6 +57,16 @@ public class UploadNoSelection extends AbstractTestUIWithLog
             log(FILE_LENGTH_PREFIX + " " + event.getLength());
             log(FILE_NAME_PREFIX + " " + event.getFilename());
         });
+
+        Button progButton = new Button("Upload programmatically",
+                e -> u.submitUpload());
+        progButton.setId("programmatic");
+        addComponent(progButton);
+
+        Button extButton = new Button("Allow upload without filename",
+                e -> AllowUploadWithoutFilenameExtension.wrap(u));
+        extButton.setId("extend");
+        addComponent(extButton);
     }
 
     @Override

uitest/src/main/java/com/vaadin/tests/widgetset/client/upload/AllowUploadWithoutFilenameConnector.java

@@ -0,0 +1,25 @@
+package com.vaadin.tests.widgetset.client.upload;
+
+import com.vaadin.client.ServerConnector;
+import com.vaadin.client.extensions.AbstractExtensionConnector;
+import com.vaadin.client.ui.VUpload;
+import com.vaadin.client.ui.upload.UploadConnector;
+import com.vaadin.shared.ui.Connect;
+import com.vaadin.tests.widgetset.server.upload.AllowUploadWithoutFilenameExtension;
+
+@Connect(AllowUploadWithoutFilenameExtension.class)
+public class AllowUploadWithoutFilenameConnector
+        extends AbstractExtensionConnector {
+
+    @Override
+    protected void extend(ServerConnector target) {
+        UploadConnector connector = ((UploadConnector) target);
+        allowUploadWithoutFilename(connector.getWidget());
+    }
+
+    private native void allowUploadWithoutFilename(VUpload upload)
+    /*-{
+        upload.@com.vaadin.client.ui.VUpload::allowUploadWithoutFilename = true;
+    }-*/;
+
+}

uitest/src/main/java/com/vaadin/tests/widgetset/server/upload/AllowUploadWithoutFilenameExtension.java

@@ -0,0 +1,13 @@
+package com.vaadin.tests.widgetset.server.upload;
+
+import com.vaadin.server.AbstractExtension;
+import com.vaadin.ui.Upload;
+
+public class AllowUploadWithoutFilenameExtension extends AbstractExtension {
+
+    public static AllowUploadWithoutFilenameExtension wrap(Upload upload) {
+        AllowUploadWithoutFilenameExtension extension = new AllowUploadWithoutFilenameExtension();
+        extension.extend(upload);
+        return extension;
+    }
+}

uitest/src/test/java/com/vaadin/tests/components/ui/MultiFileUploadTestTest.java

@@ -0,0 +1,102 @@
+package com.vaadin.tests.components.ui;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
+
+import java.io.BufferedWriter;
+import java.io.File;
+import java.io.FileWriter;
+import java.io.IOException;
+
+import org.junit.Test;
+import org.openqa.selenium.By;
+import org.openqa.selenium.WebElement;
+import org.openqa.selenium.internal.WrapsElement;
+import org.openqa.selenium.remote.LocalFileDetector;
+import org.openqa.selenium.remote.RemoteWebElement;
+
+import com.vaadin.testbench.elements.ButtonElement;
+import com.vaadin.testbench.elements.UploadElement;
+import com.vaadin.tests.tb3.MultiBrowserTest;
+
+public class MultiFileUploadTestTest extends MultiBrowserTest {
+
+    @Test
+    public void changeListenerWorksAfterFirstUpload() throws IOException {
+        openTestURL();
+        ButtonElement upload = $(ButtonElement.class).first();
+
+        File tempFile = createTempFile();
+        fillPathToUploadInput(tempFile.getPath(),
+                $(UploadElement.class).last());
+        tempFile = createTempFile();
+        fillPathToUploadInput(tempFile.getPath(),
+                $(UploadElement.class).last());
+        tempFile = createTempFile();
+        fillPathToUploadInput(tempFile.getPath(),
+                $(UploadElement.class).last());
+
+        assertEquals("Unexpected amount of Upload components.", 4,
+                $(UploadElement.class).all().size());
+
+        upload.click();
+
+        // Last one doesn't have a file selected, and shouldn't trigger an
+        // event.
+        String logRow = getLogRow(0);
+        assertTrue("Unexpected upload log: " + logRow,
+                logRow.startsWith("3. Upload of ")
+                        && logRow.endsWith(" complete"));
+    }
+
+    /**
+     * @return The generated temp file handle
+     * @throws IOException
+     */
+    private File createTempFile() throws IOException {
+        File tempFile = File.createTempFile("TestFileUpload", ".txt");
+        BufferedWriter writer = new BufferedWriter(new FileWriter(tempFile));
+        writer.write(getTempFileContents());
+        writer.close();
+        tempFile.deleteOnExit();
+        return tempFile;
+    }
+
+    private String getTempFileContents() {
+        StringBuilder sb = new StringBuilder("This is a small test file.");
+        sb.append("\n");
+        sb.append("Very small.");
+        return sb.toString();
+    }
+
+    private void fillPathToUploadInput(String tempFileName,
+            UploadElement uploadElement) {
+        // create a valid path in upload input element. Instead of selecting a
+        // file by some file browsing dialog, we use the local path directly.
+        WebElement input = getInput(uploadElement);
+        setLocalFileDetector(input);
+        input.sendKeys(tempFileName);
+    }
+
+    private WebElement getInput(UploadElement uploadElement) {
+        return uploadElement.findElement(By.className("gwt-FileUpload"));
+    }
+
+    private void setLocalFileDetector(WebElement element) {
+        if (getRunLocallyBrowser() != null) {
+            return;
+        }
+
+        if (element instanceof WrapsElement) {
+            element = ((WrapsElement) element).getWrappedElement();
+        }
+        if (element instanceof RemoteWebElement) {
+            ((RemoteWebElement) element)
+                    .setFileDetector(new LocalFileDetector());
+        } else {
+            throw new IllegalArgumentException(
+                    "Expected argument of type RemoteWebElement, received "
+                            + element.getClass().getName());
+        }
+    }
+}

uitest/src/test/java/com/vaadin/tests/components/upload/UploadNoSelectionTest.java

@@ -1,11 +1,13 @@
 package com.vaadin.tests.components.upload;
 
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
 
 import org.junit.Test;
 import org.openqa.selenium.By;
 import org.openqa.selenium.WebElement;
 
+import com.vaadin.testbench.elements.ButtonElement;
 import com.vaadin.tests.tb3.MultiBrowserTest;
 
 public class UploadNoSelectionTest extends MultiBrowserTest {
@@ -17,7 +19,24 @@ public class UploadNoSelectionTest extends MultiBrowserTest {
         // empty content is populated by com.vaadin.tests.util.Log
         assertEquals(" ", getLogRow(0));
 
-        getSubmitButton().click();
+        WebElement submitButton = getSubmitButton();
+        assertTrue("Upload button should be disabled when no selection.",
+                hasCssClass(submitButton, "v-disabled"));
+
+        submitButton.click();
+
+        // clicking the disabled default button doesn't do a thing
+        assertEquals(" ", getLogRow(0));
+
+        $(ButtonElement.class).id("programmatic").click();
+
+        // neither does triggering upload programmatically
+        assertEquals(" ", getLogRow(0));
+
+        // add an extension that allows upload without filename
+        $(ButtonElement.class).id("extend").click();
+
+        submitButton.click();
 
         // expecting empty file name
         assertLogRow(0, 4, UploadNoSelection.FILE_NAME_PREFIX);
@@ -25,6 +44,17 @@ public class UploadNoSelectionTest extends MultiBrowserTest {
         assertLogRow(1, 3, UploadNoSelection.FILE_LENGTH_PREFIX + " " + 0);
         assertLogRow(2, 2, UploadNoSelection.UPLOAD_FINISHED);
         assertLogRow(3, 1, UploadNoSelection.RECEIVING_UPLOAD);
+
+        // and the same programmatically
+        $(ButtonElement.class).id("programmatic").click();
+
+        // expecting empty file name
+        assertLogRow(0, 8, UploadNoSelection.FILE_NAME_PREFIX);
+        // expecting 0-length file
+        assertLogRow(1, 7, UploadNoSelection.FILE_LENGTH_PREFIX + " " + 0);
+        assertLogRow(2, 6, UploadNoSelection.UPLOAD_FINISHED);
+        assertLogRow(3, 5, UploadNoSelection.RECEIVING_UPLOAD);
+
     }
 
     private WebElement getSubmitButton() {