Bug 495789 - Catch errors during update for lists and setsI20191027-1800

Handle exceptions during the update of lists and
sets in the same way as exceptions are handled for
values: Catch and set validation result.

For errors during the actual updates this is not a
change in behaviour. For errors during conversion
and misc errors at other points this is a change.

Change-Id: Icc08aa9f1a1e4e9c55ddf3f48c4d4fd12990949b
Signed-off-by: Jens Lidestrom <jens@lidestrom.se>

6 files changed, 92 insertions, 8 deletions

diff --git a/bundles/org.eclipse.core.databinding/src/org/eclipse/core/databinding/ListBinding.java b/bundles/org.eclipse.core.databinding/src/org/eclipse/core/databinding/ListBinding.java
index 665bef2f80b..8aed6fc3364 100644
--- a/bundles/org.eclipse.core.databinding/src/org/eclipse/core/databinding/ListBinding.java
+++ b/bundles/org.eclipse.core.databinding/src/org/eclipse/core/databinding/ListBinding.java
@@ -25,6 +25,7 @@ import org.eclipse.core.databinding.observable.list.ListDiff;
 import org.eclipse.core.databinding.observable.list.ListDiffVisitor;
 import org.eclipse.core.databinding.observable.value.IObservableValue;
 import org.eclipse.core.databinding.observable.value.WritableValue;
+import org.eclipse.core.databinding.util.Policy;
 import org.eclipse.core.internal.databinding.BindingStatus;
 import org.eclipse.core.runtime.IStatus;
 import org.eclipse.core.runtime.MultiStatus;
@@ -222,6 +223,9 @@ public class ListBinding<M, T> extends Binding {
 				});
 				// TODO - at this point, the two lists will be out
 				// of sync if an error occurred...
+			} catch (Exception ex) {
+				String message = ex.getMessage() != null ? ex.getMessage() : ""; //$NON-NLS-1$
+				multiStatus.add(new Status(IStatus.ERROR, Policy.JFACE_DATABINDING, IStatus.ERROR, message, ex));
 			} finally {
 				setValidationStatus(multiStatus);
 
diff --git a/bundles/org.eclipse.core.databinding/src/org/eclipse/core/databinding/SetBinding.java b/bundles/org.eclipse.core.databinding/src/org/eclipse/core/databinding/SetBinding.java
index e75f6e770c7..cd85e923fa9 100644
--- a/bundles/org.eclipse.core.databinding/src/org/eclipse/core/databinding/SetBinding.java
+++ b/bundles/org.eclipse.core.databinding/src/org/eclipse/core/databinding/SetBinding.java
@@ -25,6 +25,7 @@ import org.eclipse.core.databinding.observable.set.ISetChangeListener;
 import org.eclipse.core.databinding.observable.set.SetDiff;
 import org.eclipse.core.databinding.observable.value.IObservableValue;
 import org.eclipse.core.databinding.observable.value.WritableValue;
+import org.eclipse.core.databinding.util.Policy;
 import org.eclipse.core.internal.databinding.BindingStatus;
 import org.eclipse.core.runtime.IStatus;
 import org.eclipse.core.runtime.MultiStatus;
@@ -194,6 +195,9 @@ public class SetBinding<M, T> extends Binding {
 					mergeStatus(multiStatus, setterStatus2);
 					// TODO: At this point, the two sets will be out of sync if an error occurred.
 				}
+			} catch (Exception ex) {
+				String message = ex.getMessage() != null ? ex.getMessage() : ""; //$NON-NLS-1$
+				multiStatus.add(new Status(IStatus.ERROR, Policy.JFACE_DATABINDING, IStatus.ERROR, message, ex));
 			} finally {
 				setValidationStatus(multiStatus);
 
diff --git a/bundles/org.eclipse.core.databinding/src/org/eclipse/core/databinding/UpdateListStrategy.java b/bundles/org.eclipse.core.databinding/src/org/eclipse/core/databinding/UpdateListStrategy.java
index 04c8b6b1f70..ab585f3bdfb 100644
--- a/bundles/org.eclipse.core.databinding/src/org/eclipse/core/databinding/UpdateListStrategy.java
+++ b/bundles/org.eclipse.core.databinding/src/org/eclipse/core/databinding/UpdateListStrategy.java
@@ -170,6 +170,9 @@ public class UpdateListStrategy<S, D> extends UpdateStrategy<S, D> {
 	/**
 	 * Sets the converter to be invoked when converting added elements from the
 	 * source element type to the destination element type.
+	 * <p>
+	 * If the converter throws any exceptions they are reported as validation
+	 * errors, using the exception message.
 	 *
 	 * @param converter
 	 * @return the receiver, to enable method call chaining
diff --git a/bundles/org.eclipse.core.databinding/src/org/eclipse/core/databinding/UpdateSetStrategy.java b/bundles/org.eclipse.core.databinding/src/org/eclipse/core/databinding/UpdateSetStrategy.java
index 8fa03a5e81f..aa378e86bd8 100644
--- a/bundles/org.eclipse.core.databinding/src/org/eclipse/core/databinding/UpdateSetStrategy.java
+++ b/bundles/org.eclipse.core.databinding/src/org/eclipse/core/databinding/UpdateSetStrategy.java
@@ -170,6 +170,9 @@ public class UpdateSetStrategy<S, D> extends UpdateStrategy<S, D> {
 	/**
 	 * Sets the converter to be invoked when converting added elements from the
 	 * source element type to the destination element type.
+	 * <p>
+	 * If the converter throws any exceptions they are reported as validation
+	 * errors, using the exception message.
 	 *
 	 * @param converter
 	 * @return the receiver, to enable method call chaining
diff --git a/tests/org.eclipse.jface.tests.databinding/src/org/eclipse/core/tests/databinding/ListBindingTest.java b/tests/org.eclipse.jface.tests.databinding/src/org/eclipse/core/tests/databinding/ListBindingTest.java
index 136a0322996..bbbc084150a 100755
--- a/tests/org.eclipse.jface.tests.databinding/src/org/eclipse/core/tests/databinding/ListBindingTest.java
+++ b/tests/org.eclipse.jface.tests.databinding/src/org/eclipse/core/tests/databinding/ListBindingTest.java
@@ -220,13 +220,13 @@ public class ListBindingTest extends AbstractDefaultRealmTestCase {
 	 * class would need much more stubbing and mocking to test it.
 	 */
 	@Test
-	public void testErrorDuringConversionIsLogged() {
+	public void testErrorDuringConversion() {
 		UpdateListStrategy<String, String> modelToTarget = new UpdateListStrategy<>();
 		modelToTarget.setConverter(IConverter.create(String.class, String.class, fromObject -> {
 			throw new IllegalArgumentException();
 		}));
 
-		dbc.bindList(target, model, new UpdateListStrategy<>(), modelToTarget);
+		Binding binding = dbc.bindList(target, model, new UpdateListStrategy<>(), modelToTarget);
 		CountDownLatch latch = new CountDownLatch(1);
 
 		Policy.setLog(status -> {
@@ -237,7 +237,9 @@ public class ListBindingTest extends AbstractDefaultRealmTestCase {
 
 		model.add("first");
 
+		assertTrue("Target not changed on conversion error", target.isEmpty());
 		assertEquals(0, latch.getCount());
+		assertEquals(IStatus.ERROR, binding.getValidationStatus().getValue().getCode());
 
 		Policy.setLog(null);
 	}
@@ -247,14 +249,15 @@ public class ListBindingTest extends AbstractDefaultRealmTestCase {
 	 * class would need much more stubbing and mocking to test it.
 	 */
 	@Test
-	public void testErrorDuringRemoveIsLogged() {
+	public void testErrorDuringRemove() {
 		IObservableList<String> target = new WritableList<String>() {
 			@Override
 			public String remove(int index) {
 				throw new IllegalArgumentException();
 			}
 		};
-		dbc.bindList(target, model, new UpdateListStrategy<>(), new UpdateListStrategy<>());
+
+		Binding binding = dbc.bindList(target, model, new UpdateListStrategy<>(), new UpdateListStrategy<>());
 		CountDownLatch latch = new CountDownLatch(1);
 
 		Policy.setLog(status -> {
@@ -266,7 +269,9 @@ public class ListBindingTest extends AbstractDefaultRealmTestCase {
 		model.add("first");
 		model.remove("first");
 
+		assertEquals("Target not changed on conversion error", Arrays.asList("first"), target);
 		assertEquals(0, latch.getCount());
+		assertEquals(IStatus.ERROR, binding.getValidationStatus().getValue().getSeverity());
 	}
 
 	/**
@@ -274,14 +279,14 @@ public class ListBindingTest extends AbstractDefaultRealmTestCase {
 	 * class would need much more stubbing and mocking to test it.
 	 */
 	@Test
-	public void testErrorDuringMoveIsLogged() {
+	public void testErrorDuringMove() {
 		IObservableList<String> target = new WritableList<String>() {
 			@Override
 			public String move(int index, int index2) {
 				throw new IllegalArgumentException();
 			}
 		};
-		dbc.bindList(target, model, new UpdateListStrategy<>(), new UpdateListStrategy<>());
+		Binding binding = dbc.bindList(target, model, new UpdateListStrategy<>(), new UpdateListStrategy<>());
 		CountDownLatch latch = new CountDownLatch(1);
 
 		Policy.setLog(status -> {
@@ -294,7 +299,9 @@ public class ListBindingTest extends AbstractDefaultRealmTestCase {
 		model.add("second");
 		model.move(0, 1);
 
+		assertEquals("Target not changed on conversion error", Arrays.asList("first", "second"), target);
 		assertEquals(0, latch.getCount());
+		assertEquals(IStatus.ERROR, binding.getValidationStatus().getValue().getSeverity());
 	}
 
 	/**
@@ -302,14 +309,15 @@ public class ListBindingTest extends AbstractDefaultRealmTestCase {
 	 * class would need much more stubbing and mocking to test it.
 	 */
 	@Test
-	public void testErrorDuringReplaceIsLogged() {
+	public void testErrorDuringReplace() {
 		IObservableList<String> target = new WritableList<String>() {
 			@Override
 			public String set(int index, String element) {
 				throw new IllegalArgumentException();
 			}
 		};
-		dbc.bindList(target, model, new UpdateListStrategy<>(), new UpdateListStrategy<>());
+
+		Binding binding = dbc.bindList(target, model, new UpdateListStrategy<>(), new UpdateListStrategy<>());
 		CountDownLatch latch = new CountDownLatch(1);
 
 		Policy.setLog(status -> {
@@ -321,7 +329,9 @@ public class ListBindingTest extends AbstractDefaultRealmTestCase {
 		model.add("first");
 		model.set(0, "second");
 
+		assertEquals("Element not changed on conversion error", "first", target.get(0));
 		assertEquals(0, latch.getCount());
+		assertEquals(IStatus.ERROR, binding.getValidationStatus().getValue().getSeverity());
 	}
 
 	/**
diff --git a/tests/org.eclipse.jface.tests.databinding/src/org/eclipse/core/tests/databinding/SetBindingTest.java b/tests/org.eclipse.jface.tests.databinding/src/org/eclipse/core/tests/databinding/SetBindingTest.java
index bebd51beb40..d915689a905 100644
--- a/tests/org.eclipse.jface.tests.databinding/src/org/eclipse/core/tests/databinding/SetBindingTest.java
+++ b/tests/org.eclipse.jface.tests.databinding/src/org/eclipse/core/tests/databinding/SetBindingTest.java
@@ -17,13 +17,19 @@ package org.eclipse.core.tests.databinding;
 import static org.eclipse.core.databinding.UpdateSetStrategy.POLICY_NEVER;
 import static org.eclipse.core.databinding.UpdateSetStrategy.POLICY_UPDATE;
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
 
+import java.util.concurrent.CountDownLatch;
+
+import org.eclipse.core.databinding.Binding;
 import org.eclipse.core.databinding.DataBindingContext;
 import org.eclipse.core.databinding.SetBinding;
 import org.eclipse.core.databinding.UpdateSetStrategy;
+import org.eclipse.core.databinding.conversion.IConverter;
 import org.eclipse.core.databinding.observable.set.IObservableSet;
 import org.eclipse.core.databinding.observable.set.WritableSet;
 import org.eclipse.core.databinding.util.Policy;
+import org.eclipse.core.runtime.IStatus;
 import org.eclipse.jface.tests.databinding.AbstractDefaultRealmTestCase;
 import org.junit.After;
 import org.junit.Assert;
@@ -97,4 +103,58 @@ public class SetBindingTest extends AbstractDefaultRealmTestCase {
 		assertEquals(model.size(), target.size());
 	}
 
+	@Test
+	public void testErrorDuringConversion() {
+		UpdateSetStrategy<String, String> modelToTarget = new UpdateSetStrategy<>();
+		modelToTarget.setConverter(IConverter.create(String.class, String.class, fromObject -> {
+			throw new IllegalArgumentException();
+		}));
+
+		Binding binding = dbc.bindSet(target, model, new UpdateSetStrategy<>(), modelToTarget);
+		CountDownLatch latch = new CountDownLatch(1);
+
+		Policy.setLog(status -> {
+			latch.countDown();
+			assertEquals(IStatus.ERROR, status.getSeverity());
+			assertTrue(status.getException() instanceof IllegalArgumentException);
+		});
+
+		model.add("first");
+
+		assertTrue("Target not changed on conversion error", target.isEmpty());
+		assertEquals(0, latch.getCount());
+		assertEquals(IStatus.ERROR, binding.getValidationStatus().getValue().getSeverity());
+
+		Policy.setLog(null);
+	}
+
+	/**
+	 * We test common functionality from UpdateStrategy here, because that base
+	 * class would need much more stubbing and mocking to test it.
+	 */
+	@Test
+	public void testErrorDuringRemoveIsLogged() {
+		IObservableSet<String> target = new WritableSet<String>() {
+			@Override
+			public boolean remove(Object elem) {
+				throw new IllegalArgumentException();
+			}
+		};
+
+		Binding binding = dbc.bindSet(target, model, new UpdateSetStrategy<>(), new UpdateSetStrategy<>());
+		CountDownLatch latch = new CountDownLatch(1);
+
+		Policy.setLog(status -> {
+			latch.countDown();
+			assertEquals(IStatus.ERROR, status.getSeverity());
+			assertTrue(status.getException() instanceof IllegalArgumentException);
+		});
+
+		model.add("first");
+		model.remove("first");
+
+		assertEquals(0, latch.getCount());
+		assertEquals(IStatus.ERROR, binding.getValidationStatus().getValue().getSeverity());
+	}
+
 }