Add methods for getting invalid fields from a FieldGroup (#13775)

* Method for retrieving all failing fields exceptions from a
CommitException
* Methods for handling commit errors in Grid (#16515)
* Show editor row validation errors only on the fields (#16509)

Change-Id: Iabef662579e4ccae3803a513205e46542c41cce2

Conflicts:
	server/src/com/vaadin/ui/Grid.java

server/src/com/vaadin/data/fieldgroup/FieldGroup.java

@@ -23,6 +23,7 @@ import java.util.Collections;
 import java.util.HashMap;
 import java.util.LinkedHashMap;
 import java.util.List;
+import java.util.Map;
 
 import com.vaadin.data.Item;
 import com.vaadin.data.Property;
@@ -465,46 +466,74 @@ public class FieldGroup implements Serializable {
         try {
             firePreCommitEvent();
 
-            List<InvalidValueException> invalidValueExceptions = commitFields();
+            Map<Field<?>, InvalidValueException> invalidValueExceptions = commitFields();
 
             if (invalidValueExceptions.isEmpty()) {
                 firePostCommitEvent();
                 commitTransactions();
             } else {
-                throwInvalidValueException(invalidValueExceptions);
+                throw new FieldGroupInvalidValueException(
+                        invalidValueExceptions);
             }
         } catch (Exception e) {
             rollbackTransactions();
-
-            throw new CommitException("Commit failed", e);
+            throw new CommitException("Commit failed", this, e);
         }
 
     }
 
-    private List<InvalidValueException> commitFields() {
-        List<InvalidValueException> invalidValueExceptions = new ArrayList<InvalidValueException>();
+    /**
+     * Tries to commit all bound fields one by one and gathers any validation
+     * exceptions in a map, which is returned to the caller
+     * 
+     * @return a propertyId to validation exception map which is empty if all
+     *         commits succeeded
+     */
+    private Map<Field<?>, InvalidValueException> commitFields() {
+        Map<Field<?>, InvalidValueException> invalidValueExceptions = new HashMap<Field<?>, InvalidValueException>();
 
         for (Field<?> f : fieldToPropertyId.keySet()) {
             try {
                 f.commit();
             } catch (InvalidValueException e) {
-                invalidValueExceptions.add(e);
+                invalidValueExceptions.put(f, e);
             }
         }
 
         return invalidValueExceptions;
     }
 
-    private void throwInvalidValueException(
-            List<InvalidValueException> invalidValueExceptions) {
-        if (invalidValueExceptions.size() == 1) {
-            throw invalidValueExceptions.get(0);
-        } else {
-            InvalidValueException[] causes = invalidValueExceptions
-                    .toArray(new InvalidValueException[invalidValueExceptions
-                            .size()]);
+    /**
+     * Exception which wraps InvalidValueExceptions from all invalid fields in a
+     * FieldGroup
+     * 
+     * @since 7.4
+     */
+    public static class FieldGroupInvalidValueException extends
+            InvalidValueException {
+        private Map<Field<?>, InvalidValueException> invalidValueExceptions;
 
-            throw new InvalidValueException(null, causes);
+        /**
+         * Constructs a new exception with the specified validation exceptions.
+         * 
+         * @param invalidValueExceptions
+         *            a property id to exception map
+         */
+        public FieldGroupInvalidValueException(
+                Map<Field<?>, InvalidValueException> invalidValueExceptions) {
+            super(null, invalidValueExceptions.values().toArray(
+                    new InvalidValueException[invalidValueExceptions.size()]));
+            this.invalidValueExceptions = invalidValueExceptions;
+        }
+
+        /**
+         * Returns a map containing fields which failed validation and the
+         * exceptions the corresponding validators threw.
+         *
+         * @return a map with all the invalid value exceptions
+         */
+        public Map<Field<?>, InvalidValueException> getInvalidFields() {
+            return invalidValueExceptions;
         }
     }
 
@@ -1006,26 +1035,64 @@ public class FieldGroup implements Serializable {
         return fieldName.toLowerCase().replace("_", "");
     }
 
+    /**
+     * Exception thrown by a FieldGroup when the commit operation fails.
+     * 
+     * Provides information about validation errors through
+     * {@link #getInvalidFields()} if the cause of the failure is that all bound
+     * fields did not pass validation
+     * 
+     */
     public static class CommitException extends Exception {
 
+        private FieldGroup fieldGroup;
+
         public CommitException() {
             super();
-            // TODO Auto-generated constructor stub
+        }
+
+        public CommitException(String message, FieldGroup fieldGroup,
+                Throwable cause) {
+            super(message, cause);
+            this.fieldGroup = fieldGroup;
         }
 
         public CommitException(String message, Throwable cause) {
             super(message, cause);
-            // TODO Auto-generated constructor stub
         }
 
         public CommitException(String message) {
             super(message);
-            // TODO Auto-generated constructor stub
         }
 
         public CommitException(Throwable cause) {
             super(cause);
-            // TODO Auto-generated constructor stub
+        }
+
+        /**
+         * Returns a map containing the fields which failed validation and the
+         * exceptions the corresponding validators threw.
+         *
+         * @since 7.4
+         * @return a map with all the invalid value exceptions. Can be empty but
+         *         not null
+         */
+        public Map<Field<?>, InvalidValueException> getInvalidFields() {
+            if (getCause() instanceof FieldGroupInvalidValueException) {
+                return ((FieldGroupInvalidValueException) getCause())
+                        .getInvalidFields();
+            }
+            return new HashMap<Field<?>, InvalidValueException>();
+        }
+
+        /**
+         * Returns the field group where the exception occurred
+         * 
+         * @since 7.4
+         * @return the field group
+         */
+        public FieldGroup getFieldGroup() {
+            return fieldGroup;
         }
 
     }

server/src/com/vaadin/ui/Grid.java

@@ -48,6 +48,7 @@ import com.vaadin.data.Item;
 import com.vaadin.data.Property;
 import com.vaadin.data.RpcDataProviderExtension;
 import com.vaadin.data.RpcDataProviderExtension.DataProviderKeyMapper;
+import com.vaadin.data.Validator.InvalidValueException;
 import com.vaadin.data.fieldgroup.DefaultFieldGroupFieldFactory;
 import com.vaadin.data.fieldgroup.FieldGroup;
 import com.vaadin.data.fieldgroup.FieldGroup.BindException;
@@ -90,6 +91,7 @@ import com.vaadin.shared.ui.grid.GridStaticSectionState.RowState;
 import com.vaadin.shared.ui.grid.HeightMode;
 import com.vaadin.shared.ui.grid.ScrollDestination;
 import com.vaadin.shared.util.SharedUtil;
+import com.vaadin.ui.Notification.Type;
 import com.vaadin.ui.renderer.Renderer;
 import com.vaadin.ui.renderer.TextRenderer;
 import com.vaadin.util.ReflectTools;
@@ -238,6 +240,102 @@ public class Grid extends AbstractComponent implements SelectionNotifier,
         }
     }
 
+    /**
+     * Error handler for the editor
+     */
+    public interface EditorErrorHandler extends Serializable {
+
+        /**
+         * Called when an exception occurs while the editor row is being saved
+         * 
+         * @param event
+         *            An event providing more information about the error
+         */
+        void commitError(CommitErrorEvent event);
+    }
+
+    /**
+     * An event which is fired when saving the editor fails
+     */
+    public static class CommitErrorEvent extends Component.Event {
+
+        private CommitException cause;
+
+        public CommitErrorEvent(Grid grid, CommitException cause) {
+            super(grid);
+            this.cause = cause;
+        }
+
+        /**
+         * Retrieves the cause of the failure
+         * 
+         * @return the cause of the failure
+         */
+        public CommitException getCause() {
+            return cause;
+        }
+
+        @Override
+        public Grid getComponent() {
+            return (Grid) super.getComponent();
+        }
+
+        /**
+         * Checks if validation exceptions caused this error
+         * 
+         * @return true if the problem was caused by a validation error
+         */
+        public boolean isValidationFailure() {
+            return cause.getCause() instanceof InvalidValueException;
+        }
+
+    }
+
+    /**
+     * Default error handler for the editor
+     * 
+     */
+    public class DefaultEditorErrorHandler implements EditorErrorHandler {
+
+        @Override
+        public void commitError(CommitErrorEvent event) {
+            Map<Field<?>, InvalidValueException> invalidFields = event
+                    .getCause().getInvalidFields();
+
+            if (!invalidFields.isEmpty()) {
+                // Validation error, show first failure as
+                // "<Column header>: <message>"
+                FieldGroup fieldGroup = event.getCause().getFieldGroup();
+                Object propertyId = getFirstPropertyId(fieldGroup,
+                        invalidFields.keySet());
+                Field<?> field = fieldGroup.getField(propertyId);
+                String caption = getColumn(propertyId).getHeaderCaption();
+                // TODO This should be shown in the editor component once
+                // there is a place for that. Optionally, all errors should be
+                // shown
+                Notification.show(caption + ": "
+                        + invalidFields.get(field).getLocalizedMessage(),
+                        Type.ERROR_MESSAGE);
+
+            } else {
+                com.vaadin.server.ErrorEvent.findErrorHandler(Grid.this).error(
+                        new ConnectorErrorEvent(Grid.this, event.getCause()));
+            }
+        }
+
+        private Object getFirstPropertyId(FieldGroup fieldGroup,
+                Set<Field<?>> keySet) {
+            for (Column c : getColumns()) {
+                Object propertyId = c.getPropertyId();
+                Field<?> f = fieldGroup.getField(propertyId);
+                if (keySet.contains(f)) {
+                    return propertyId;
+                }
+            }
+            return null;
+        }
+    }
+
     /**
      * Selection modes representing built-in {@link SelectionModel
      * SelectionModels} that come bundled with {@link Grid}.
@@ -2604,6 +2702,8 @@ public class Grid extends AbstractComponent implements SelectionNotifier,
      */
     private boolean defaultContainer = true;
 
+    private EditorErrorHandler editorErrorHandler = new DefaultEditorErrorHandler();
+
     private static final Method SELECTION_CHANGE_METHOD = ReflectTools
             .findMethod(SelectionListener.class, "select", SelectionEvent.class);
 
@@ -2843,6 +2943,15 @@ public class Grid extends AbstractComponent implements SelectionNotifier,
                 try {
                     saveEditor();
                     success = true;
+                } catch (CommitException e) {
+                    try {
+                        getEditorErrorHandler().commitError(
+                                new CommitErrorEvent(Grid.this, e));
+                    } catch (Exception ee) {
+                        // A badly written error handler can throw an exception,
+                        // which would lock up the Grid
+                        handleError(ee);
+                    }
                 } catch (Exception e) {
                     handleError(e);
                 }
@@ -4695,6 +4804,35 @@ public class Grid extends AbstractComponent implements SelectionNotifier,
         editorFieldGroup.setFieldFactory(fieldFactory);
     }
 
+    /**
+     * Sets the error handler for the editor.
+     * 
+     * The error handler is called whenever there is an exception in the editor.
+     * 
+     * @param editorErrorHandler
+     *            The editor error handler to use
+     * @throws IllegalArgumentException
+     *             if the error handler is null
+     */
+    public void setEditorErrorHandler(EditorErrorHandler editorErrorHandler)
+            throws IllegalArgumentException {
+        if (editorErrorHandler == null) {
+            throw new IllegalArgumentException(
+                    "The error handler cannot be null");
+        }
+        this.editorErrorHandler = editorErrorHandler;
+    }
+
+    /**
+     * Gets the error handler used for the editor
+     * 
+     * @see #setErrorHandler(com.vaadin.server.ErrorHandler)
+     * @return the editor error handler, never null
+     */
+    public EditorErrorHandler getEditorErrorHandler() {
+        return editorErrorHandler;
+    }
+
     /**
      * Gets the field factory for the {@link FieldGroup}. The field factory is
      * only used when {@link FieldGroup} creates a new field.

server/tests/src/com/vaadin/tests/server/component/fieldgroup/FieldGroupTest.java

@@ -15,10 +15,23 @@
  */
 package com.vaadin.tests.server.component.fieldgroup;
 
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Map.Entry;
+import java.util.Set;
+
 import org.junit.Assert;
 import org.junit.Test;
 
+import com.vaadin.data.Item;
+import com.vaadin.data.Property;
+import com.vaadin.data.Validator.InvalidValueException;
 import com.vaadin.data.fieldgroup.FieldGroup;
+import com.vaadin.data.fieldgroup.FieldGroup.CommitException;
+import com.vaadin.data.util.AbstractProperty;
+import com.vaadin.ui.Field;
 import com.vaadin.ui.TextField;
 
 /**
@@ -52,4 +65,85 @@ public class FieldGroupTest {
 
         Assert.assertFalse("Field is not writable", field.isReadOnly());
     }
+
+    @Test
+    public void commit_validationFailed_allValidationFailuresAvailable()
+            throws CommitException {
+        FieldGroup fieldGroup = new FieldGroup();
+
+        fieldGroup.setItemDataSource(new TestItem());
+
+        TextField field1 = new TextField();
+        field1.setRequired(true);
+        fieldGroup.bind(field1, "prop1");
+
+        TextField field2 = new TextField();
+        field2.setRequired(true);
+        fieldGroup.bind(field2, "prop2");
+
+        Set<TextField> set = new HashSet<TextField>(Arrays.asList(field1,
+                field2));
+
+        try {
+            fieldGroup.commit();
+            Assert.fail("No commit exception is thrown");
+        } catch (CommitException exception) {
+            Map<Field<?>, ? extends InvalidValueException> invalidFields = exception
+                    .getInvalidFields();
+            for (Entry<Field<?>, ? extends InvalidValueException> entry : invalidFields
+                    .entrySet()) {
+                set.remove(entry.getKey());
+            }
+            Assert.assertEquals(
+                    "Some fields are not found in the invalid fields map", 0,
+                    set.size());
+            Assert.assertEquals(
+                    "Invalid value exception should be thrown for each field",
+                    2, invalidFields.size());
+        }
+    }
+
+    private static class TestItem implements Item {
+
+        @Override
+        public Property<String> getItemProperty(Object id) {
+            return new StringProperty();
+        }
+
+        @Override
+        public Collection<?> getItemPropertyIds() {
+            return Arrays.asList("prop1", "prop2");
+        }
+
+        @Override
+        public boolean addItemProperty(Object id, Property property)
+                throws UnsupportedOperationException {
+            return false;
+        }
+
+        @Override
+        public boolean removeItemProperty(Object id)
+                throws UnsupportedOperationException {
+            return false;
+        }
+
+    }
+
+    private static class StringProperty extends AbstractProperty<String> {
+
+        @Override
+        public String getValue() {
+            return null;
+        }
+
+        @Override
+        public void setValue(String newValue) throws Property.ReadOnlyException {
+        }
+
+        @Override
+        public Class<? extends String> getType() {
+            return String.class;
+        }
+    }
+
 }

uitest/src/com/vaadin/tests/components/grid/basicfeatures/server/GridEditorTest.java

@@ -187,9 +187,10 @@ public class GridEditorTest extends GridBasicFeaturesTest {
         intField.clear();
         intField.sendKeys("banana phone");
         editor.save();
-        assertTrue(
-                "No exception on invalid value.",
-                logContainsText("Exception occured, com.vaadin.data.fieldgroup.FieldGroup$CommitException: Commit failed"));
+        WebElement n = $(NotificationElement.class).first();
+        assertEquals("Column 7: Could not convert value to Integer",
+                n.getText());
+        n.click();
         editor.cancel();
 
         selectMenuPath(EDIT_ITEM_100);

uitest/src/com/vaadin/tests/fieldgroup/AbstractBasicCrud.java

@@ -16,16 +16,20 @@
 package com.vaadin.tests.fieldgroup;
 
 import java.util.Iterator;
+import java.util.Map;
 
 import com.vaadin.annotations.Theme;
 import com.vaadin.data.Property.ValueChangeEvent;
 import com.vaadin.data.Property.ValueChangeListener;
+import com.vaadin.data.Validator.InvalidValueException;
 import com.vaadin.data.fieldgroup.BeanFieldGroup;
 import com.vaadin.data.fieldgroup.FieldGroup.CommitException;
 import com.vaadin.data.fieldgroup.PropertyId;
 import com.vaadin.data.util.BeanItem;
 import com.vaadin.data.util.BeanItemContainer;
+import com.vaadin.data.validator.IntegerRangeValidator;
 import com.vaadin.server.VaadinRequest;
+import com.vaadin.shared.ui.label.ContentMode;
 import com.vaadin.shared.util.SharedUtil;
 import com.vaadin.tests.components.AbstractTestUIWithLog;
 import com.vaadin.ui.Alignment;
@@ -34,10 +38,15 @@ import com.vaadin.ui.Button.ClickEvent;
 import com.vaadin.ui.Button.ClickListener;
 import com.vaadin.ui.CheckBox;
 import com.vaadin.ui.ComboBox;
+import com.vaadin.ui.Component;
 import com.vaadin.ui.Field;
 import com.vaadin.ui.GridLayout;
 import com.vaadin.ui.HorizontalLayout;
+import com.vaadin.ui.Label;
+import com.vaadin.ui.Notification;
+import com.vaadin.ui.Notification.Type;
 import com.vaadin.ui.TextField;
+import com.vaadin.ui.themes.ValoTheme;
 
 @Theme("valo")
 public abstract class AbstractBasicCrud extends AbstractTestUIWithLog {
@@ -100,6 +109,7 @@ public abstract class AbstractBasicCrud extends AbstractTestUIWithLog {
         private TextField birthDate = new TextField("Birth date");
         private TextField age = new TextField("Age");
         private CheckBox alive = new CheckBox("Alive");
+        private Label errorLabel = new Label((String) null, ContentMode.HTML);
 
         @PropertyId("address.streetAddress")
         private TextField address_streetAddress = new TextField(
@@ -120,17 +130,51 @@ public abstract class AbstractBasicCrud extends AbstractTestUIWithLog {
             address_country.setNullRepresentation("");
             birthDate.setNullRepresentation("");
 
+            age.addValidator(new IntegerRangeValidator(
+                    "Must be between 0 and 100", 0, 100));
+
             setDefaultComponentAlignment(Alignment.MIDDLE_LEFT);
             addComponents(firstName, lastName, gender, birthDate, age, alive,
                     address_streetAddress, address_postalCode, address_city,
                     address_country);
 
+            errorLabel.addStyleName(ValoTheme.LABEL_COLORED);
+            setRows(3);
+            addComponent(errorLabel, 0, 2, getColumns() - 1, 2);
+
             HorizontalLayout hl = new HorizontalLayout(save, cancel);
             hl.setSpacing(true);
             addComponent(hl);
 
         }
 
+        @Override
+        protected void handleCommitException(CommitException e) {
+            String message = "";
+            // Produce error message in the order in which the fields are in the
+            // layout
+            for (Component c : this) {
+                if (!(c instanceof Field)) {
+                    continue;
+                }
+                Field<?> f = (Field<?>) c;
+                Map<Field<?>, InvalidValueException> exceptions = e
+                        .getInvalidFields();
+                if (exceptions.containsKey(f)) {
+                    message += f.getCaption() + ": "
+                            + exceptions.get(f).getLocalizedMessage()
+                            + "<br/>\n";
+                }
+            }
+
+            errorLabel.setValue(message);
+        }
+
+        @Override
+        protected void discard() {
+            super.discard();
+            errorLabel.setValue(null);
+        }
     }
 
     protected abstract void deselectAll();
@@ -153,6 +197,7 @@ public abstract class AbstractBasicCrud extends AbstractTestUIWithLog {
                         fieldGroup.commit();
                         log("Saved " + fieldGroup.getItemDataSource());
                     } catch (CommitException e) {
+                        handleCommitException(e);
                         log("Commit failed: " + e.getMessage());
                     }
                 }
@@ -161,11 +206,28 @@ public abstract class AbstractBasicCrud extends AbstractTestUIWithLog {
                 @Override
                 public void buttonClick(ClickEvent event) {
                     log("Discarded " + fieldGroup.getItemDataSource());
-                    deselectAll();
+                    discard();
                 }
             });
         }
 
+        protected void discard() {
+            deselectAll();
+        }
+
+        protected void handleCommitException(CommitException e) {
+            String message = "";
+            for (Object propertyId : e.getInvalidFields().keySet()) {
+                Field<?> f = e.getFieldGroup().getField(propertyId);
+                message += f.getCaption() + ": "
+                        + e.getInvalidFields().get(propertyId);
+            }
+
+            if (!message.isEmpty()) {
+                Notification.show(message, Type.ERROR_MESSAGE);
+            }
+        }
+
         public void edit(BeanItem<ComplexPerson> item) {
             fieldGroup.setItemDataSource(item);
         }

uitest/src/com/vaadin/tests/fieldgroup/BasicCrudGridEditorRowTest.java

@@ -15,25 +15,61 @@
  */
 package com.vaadin.tests.fieldgroup;
 
+import org.junit.Assert;
+import org.junit.Before;
 import org.junit.Test;
+import org.openqa.selenium.By;
+import org.openqa.selenium.Keys;
+import org.openqa.selenium.WebElement;
 import org.openqa.selenium.interactions.Actions;
 
+import com.vaadin.testbench.elements.DateFieldElement;
 import com.vaadin.testbench.elements.GridElement;
 import com.vaadin.testbench.elements.GridElement.GridCellElement;
+import com.vaadin.testbench.elements.GridElement.GridEditorElement;
+import com.vaadin.tests.annotations.TestCategory;
 import com.vaadin.tests.tb3.MultiBrowserTest;
 
+@TestCategory("grid")
 public class BasicCrudGridEditorRowTest extends MultiBrowserTest {
+    private GridElement grid;
+
+    @Before
+    public void openTest() {
+        openTestURL();
+        grid = $(GridElement.class).first();
+
+    }
 
     @Test
     public void lookAndFeel() throws Exception {
-        openTestURL();
-        GridElement grid = $(GridElement.class).first();
         GridCellElement ritaBirthdate = grid.getCell(2, 3);
         compareScreen("grid");
 
         // Open editor row
         new Actions(getDriver()).doubleClick(ritaBirthdate).perform();
         compareScreen("editorrow");
+    }
+
+    @Test
+    public void editorRowOneInvalidValue() throws Exception {
+        GridCellElement ritaBirthdate = grid.getCell(2, 3);
+        // Open editor row
+        new Actions(getDriver()).doubleClick(ritaBirthdate).perform();
 
+        GridEditorElement editor = grid.getEditor();
+        DateFieldElement dateField = editor.$(DateFieldElement.class).first();
+        WebElement input = dateField.findElement(By.xpath("input"));
+        // input.click();
+        input.sendKeys("Invalid", Keys.TAB);
+        editor.save();
+
+        Assert.assertTrue("Editor wasn't displayed.", editor.isDisplayed());
+        Assert.assertTrue("DateField wasn't displayed.",
+                dateField.isDisplayed());
+
+        Assert.assertTrue("DateField didn't have 'v-invalid' css class.",
+                hasCssClass(dateField, "v-datefield-error"));
     }
+
 }