Fix Editor creating fields before client Grid can attach them (#16214)

This patch includes some race condition handling.

Change-Id: I6ab3cf15a67de722181b2718ab85b6b4a6bcb997

4 files changed, 80 insertions, 59 deletions

diff --git a/client/src/com/vaadin/client/connectors/GridConnector.java b/client/src/com/vaadin/client/connectors/GridConnector.java
index 93e2b0568d..488dac37ef 100644
--- a/client/src/com/vaadin/client/connectors/GridConnector.java
+++ b/client/src/com/vaadin/client/connectors/GridConnector.java
@@ -210,8 +210,13 @@ public class GridConnector extends AbstractHasComponentsConnector implements
 
                 @Override
                 public void bind(final int rowIndex) {
-                    serverInitiated = true;
-                    GridConnector.this.getWidget().editRow(rowIndex);
+                    // call this finally to avoid issues with editing on init
+                    Scheduler.get().scheduleFinally(new ScheduledCommand() {
+                        @Override
+                        public void execute() {
+                            GridConnector.this.getWidget().editRow(rowIndex);
+                        }
+                    });
                 }
 
                 @Override
@@ -223,7 +228,6 @@ public class GridConnector extends AbstractHasComponentsConnector implements
                 @Override
                 public void confirmBind(final boolean bindSucceeded) {
                     endRequest(bindSucceeded);
-
                 }
 
                 @Override
@@ -235,18 +239,14 @@ public class GridConnector extends AbstractHasComponentsConnector implements
 
         @Override
         public void bind(EditorRequest<JsonObject> request) {
-            if (!handleServerInitiated(request)) {
-                startRequest(request);
-                rpc.bind(request.getRowIndex());
-            }
+            startRequest(request);
+            rpc.bind(request.getRowIndex());
         }
 
         @Override
         public void save(EditorRequest<JsonObject> request) {
-            if (!handleServerInitiated(request)) {
-                startRequest(request);
-                rpc.save(request.getRowIndex());
-            }
+            startRequest(request);
+            rpc.save(request.getRowIndex());
         }
 
         @Override
@@ -296,11 +296,13 @@ public class GridConnector extends AbstractHasComponentsConnector implements
         }
 
         private void startRequest(EditorRequest<?> request) {
+            assert currentRequest == null : "Earlier request not yet finished";
+
             currentRequest = request;
         }
 
         private void endRequest(boolean succeeded) {
-            assert currentRequest != null;
+            assert currentRequest != null : "Current request was null";
             /*
              * Clear current request first to ensure the state is valid if
              * another request is made in the callback.
@@ -406,6 +408,7 @@ public class GridConnector extends AbstractHasComponentsConnector implements
     protected void init() {
         super.init();
 
+        // All scroll RPC calls are executed finally to avoid issues on init
         registerRpc(GridClientRpc.class, new GridClientRpc() {
             @Override
             public void scrollToStart() {
diff --git a/client/src/com/vaadin/client/widgets/Grid.java b/client/src/com/vaadin/client/widgets/Grid.java
index 01decd1386..980261c452 100644
--- a/client/src/com/vaadin/client/widgets/Grid.java
+++ b/client/src/com/vaadin/client/widgets/Grid.java
@@ -944,7 +944,7 @@ public class Grid<T> extends ResizeComposite implements
         public static final int KEYCODE_HIDE = KeyCodes.KEY_ESCAPE;
 
         protected enum State {
-            INACTIVE, ACTIVATING, ACTIVE, SAVING
+            INACTIVE, ACTIVATING, BINDING, ACTIVE, SAVING
         }
 
         private Grid<T> grid;
@@ -1018,7 +1018,7 @@ public class Grid<T> extends ResizeComposite implements
         private final RequestCallback<T> bindRequestCallback = new RequestCallback<T>() {
             @Override
             public void onSuccess(EditorRequest<T> request) {
-                if (state == State.ACTIVATING) {
+                if (state == State.BINDING) {
                     state = State.ACTIVE;
                     bindTimeout.cancel();
 
@@ -1029,7 +1029,7 @@ public class Grid<T> extends ResizeComposite implements
 
             @Override
             public void onError(EditorRequest<T> request) {
-                if (state == State.ACTIVATING) {
+                if (state == State.BINDING) {
                     state = State.INACTIVE;
                     bindTimeout.cancel();
 
@@ -1188,6 +1188,7 @@ public class Grid<T> extends ResizeComposite implements
 
         protected void show() {
             if (state == State.ACTIVATING) {
+                state = State.BINDING;
                 bindTimeout.schedule(BIND_TIMEOUT_MS);
                 EditorRequest<T> request = new EditorRequest<T>(grid, rowIndex,
                         bindRequestCallback);
diff --git a/server/src/com/vaadin/ui/Grid.java b/server/src/com/vaadin/ui/Grid.java
index 999d75e99a..d77c6411ef 100644
--- a/server/src/com/vaadin/ui/Grid.java
+++ b/server/src/com/vaadin/ui/Grid.java
@@ -2740,14 +2740,19 @@ public class Grid extends AbstractComponent implements SelectionNotifier,
 
             @Override
             public void bind(int rowIndex) {
-                boolean success;
+                boolean success = false;
                 try {
                     Object id = getContainerDataSource().getIdByIndex(rowIndex);
-                    doEditItem(id);
-                    success = true;
+                    if (editedItemId == null) {
+                        editedItemId = id;
+                    }
+
+                    if (editedItemId.equals(id)) {
+                        success = true;
+                        doEditItem();
+                    }
                 } catch (Exception e) {
                     handleError(e);
-                    success = false;
                 }
                 getEditorRpc().confirmBind(success);
             }
@@ -2764,13 +2769,12 @@ public class Grid extends AbstractComponent implements SelectionNotifier,
 
             @Override
             public void save(int rowIndex) {
-                boolean success;
+                boolean success = false;
                 try {
                     saveEditor();
                     success = true;
                 } catch (Exception e) {
                     handleError(e);
-                    success = false;
                 }
                 getEditorRpc().confirmSave(success);
             }
@@ -4474,31 +4478,31 @@ public class Grid extends AbstractComponent implements SelectionNotifier,
      * @param itemId
      *            the id of the item to edit
      * @throws IllegalStateException
-     *             if the editor is not enabled
+     *             if the editor is not enabled or already editing an item
      * @throws IllegalArgumentException
      *             if the {@code itemId} is not in the backing container
      * @see #setEditorEnabled(boolean)
      */
     public void editItem(Object itemId) throws IllegalStateException,
             IllegalArgumentException {
-        doEditItem(itemId);
-
-        getEditorRpc().bind(getContainerDataSource().indexOfId(itemId));
-    }
-
-    protected void doEditItem(Object itemId) {
         if (!isEditorEnabled()) {
             throw new IllegalStateException("Item editor is not enabled");
-        }
-
-        Item item = getContainerDataSource().getItem(itemId);
-        if (item == null) {
+        } else if (editedItemId != null) {
+            throw new IllegalStateException("Editing item + " + itemId
+                    + " failed. Item editor is already editing item "
+                    + editedItemId);
+        } else if (!getContainerDataSource().containsId(itemId)) {
             throw new IllegalArgumentException("Item with id " + itemId
                     + " not found in current container");
         }
+        editedItemId = itemId;
+        getEditorRpc().bind(getContainerDataSource().indexOfId(itemId));
+    }
+
+    protected void doEditItem() {
+        Item item = getContainerDataSource().getItem(editedItemId);
 
         editorFieldGroup.setItemDataSource(item);
-        editedItemId = itemId;
 
         for (Column column : getColumns()) {
             Object propertyId = column.getPropertyId();
diff --git a/server/tests/src/com/vaadin/tests/server/component/grid/GridEditorTest.java b/server/tests/src/com/vaadin/tests/server/component/grid/GridEditorTest.java
index 3e52314fbc..b247876d5d 100644
--- a/server/tests/src/com/vaadin/tests/server/component/grid/GridEditorTest.java
+++ b/server/tests/src/com/vaadin/tests/server/component/grid/GridEditorTest.java
@@ -22,6 +22,8 @@ import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertSame;
 import static org.junit.Assert.assertTrue;
 
+import java.lang.reflect.Method;
+
 import org.easymock.EasyMock;
 import org.junit.After;
 import org.junit.Assert;
@@ -48,14 +50,15 @@ public class GridEditorTest {
     private static final Integer DEFAULT_AGE = 25;
     private static final Object ITEM_ID = new Object();
 
-    private Grid grid;
-
     // Explicit field for the test session to save it from GC
     private VaadinSession session;
 
+    private final Grid grid = new Grid();
+    private Method doEditMethod;
+
     @Before
     @SuppressWarnings("unchecked")
-    public void setup() {
+    public void setup() throws SecurityException, NoSuchMethodException {
         IndexedContainer container = new IndexedContainer();
         container.addContainerProperty(PROPERTY_NAME, String.class, "[name]");
         container.addContainerProperty(PROPERTY_AGE, Integer.class,
@@ -64,8 +67,7 @@ public class GridEditorTest {
         Item item = container.addItem(ITEM_ID);
         item.getItemProperty(PROPERTY_NAME).setValue(DEFAULT_NAME);
         item.getItemProperty(PROPERTY_AGE).setValue(DEFAULT_AGE);
-
-        grid = new Grid(container);
+        grid.setContainerDataSource(container);
 
         // VaadinSession needed for ConverterFactory
         VaadinService mockService = EasyMock
@@ -73,6 +75,10 @@ public class GridEditorTest {
         session = new MockVaadinSession(mockService);
         VaadinSession.setCurrent(session);
         session.lock();
+
+        // Access to method for actual editing.
+        doEditMethod = Grid.class.getDeclaredMethod("doEditItem");
+        doEditMethod.setAccessible(true);
     }
 
     @After
@@ -83,21 +89,21 @@ public class GridEditorTest {
     }
 
     @Test
-    public void initAssumptions() throws Exception {
+    public void testInitAssumptions() throws Exception {
         assertFalse(grid.isEditorEnabled());
         assertNull(grid.getEditedItemId());
         assertNotNull(grid.getEditorFieldGroup());
     }
 
     @Test
-    public void setEnabled() throws Exception {
+    public void testSetEnabled() throws Exception {
         assertFalse(grid.isEditorEnabled());
         grid.setEditorEnabled(true);
         assertTrue(grid.isEditorEnabled());
     }
 
     @Test
-    public void setDisabled() throws Exception {
+    public void testSetDisabled() throws Exception {
         assertFalse(grid.isEditorEnabled());
         grid.setEditorEnabled(true);
         grid.setEditorEnabled(false);
@@ -105,7 +111,7 @@ public class GridEditorTest {
     }
 
     @Test
-    public void setReEnabled() throws Exception {
+    public void testSetReEnabled() throws Exception {
         assertFalse(grid.isEditorEnabled());
         grid.setEditorEnabled(true);
         grid.setEditorEnabled(false);
@@ -114,7 +120,7 @@ public class GridEditorTest {
     }
 
     @Test
-    public void detached() throws Exception {
+    public void testDetached() throws Exception {
         FieldGroup oldFieldGroup = grid.getEditorFieldGroup();
         grid.removeAllColumns();
         grid.setContainerDataSource(new IndexedContainer());
@@ -122,12 +128,12 @@ public class GridEditorTest {
     }
 
     @Test(expected = IllegalStateException.class)
-    public void disabledEditItem() throws Exception {
+    public void testDisabledEditItem() throws Exception {
         grid.editItem(ITEM_ID);
     }
 
     @Test
-    public void editItem() throws Exception {
+    public void testEditItem() throws Exception {
         startEdit();
         assertEquals(ITEM_ID, grid.getEditedItemId());
         assertEquals(getEditedItem(), grid.getEditorFieldGroup()
@@ -140,7 +146,7 @@ public class GridEditorTest {
     }
 
     @Test
-    public void saveEditor() throws Exception {
+    public void testSaveEditor() throws Exception {
         startEdit();
         TextField field = (TextField) grid.getEditorField(PROPERTY_NAME);
 
@@ -155,7 +161,7 @@ public class GridEditorTest {
     }
 
     @Test
-    public void saveEditorCommitFail() throws Exception {
+    public void testSaveEditorCommitFail() throws Exception {
         startEdit();
 
         ((TextField) grid.getEditorField(PROPERTY_AGE)).setValue("Invalid");
@@ -170,7 +176,7 @@ public class GridEditorTest {
     }
 
     @Test
-    public void cancelEditor() throws Exception {
+    public void testCancelEditor() throws Exception {
         startEdit();
         TextField field = (TextField) grid.getEditorField(PROPERTY_NAME);
         field.setValue("New Name");
@@ -184,26 +190,26 @@ public class GridEditorTest {
     }
 
     @Test(expected = IllegalArgumentException.class)
-    public void nonexistentEditItem() throws Exception {
+    public void testNonexistentEditItem() throws Exception {
         grid.setEditorEnabled(true);
         grid.editItem(new Object());
     }
 
     @Test
-    public void getField() throws Exception {
+    public void testGetField() throws Exception {
         startEdit();
 
         assertNotNull(grid.getEditorField(PROPERTY_NAME));
     }
 
     @Test
-    public void getFieldWithoutItem() throws Exception {
+    public void testGetFieldWithoutItem() throws Exception {
         grid.setEditorEnabled(true);
         assertNotNull(grid.getEditorField(PROPERTY_NAME));
     }
 
     @Test
-    public void customBinding() {
+    public void testCustomBinding() {
         TextField textField = new TextField();
         grid.setEditorField(PROPERTY_NAME, textField);
 
@@ -213,13 +219,13 @@ public class GridEditorTest {
     }
 
     @Test(expected = IllegalStateException.class)
-    public void disableWhileEditing() {
+    public void testDisableWhileEditing() {
         startEdit();
         grid.setEditorEnabled(false);
     }
 
     @Test
-    public void fieldIsNotReadonly() {
+    public void testFieldIsNotReadonly() {
         startEdit();
 
         Field<?> field = grid.getEditorField(PROPERTY_NAME);
@@ -227,7 +233,7 @@ public class GridEditorTest {
     }
 
     @Test
-    public void fieldIsReadonlyWhenFieldGroupIsReadonly() {
+    public void testFieldIsReadonlyWhenFieldGroupIsReadonly() {
         startEdit();
 
         grid.getEditorFieldGroup().setReadOnly(true);
@@ -236,18 +242,18 @@ public class GridEditorTest {
     }
 
     @Test
-    public void columnRemoved() {
+    public void testColumnRemoved() {
         Field<?> field = grid.getEditorField(PROPERTY_NAME);
 
-        assertSame("field should be attached to grid.", grid, field.getParent());
+        assertSame("field should be attached to ", grid, field.getParent());
 
         grid.removeColumn(PROPERTY_NAME);
 
-        assertNull("field should be detached from grid.", field.getParent());
+        assertNull("field should be detached from ", field.getParent());
     }
 
     @Test
-    public void setFieldAgain() {
+    public void testSetFieldAgain() {
         TextField field = new TextField();
         grid.setEditorField(PROPERTY_NAME, field);
 
@@ -261,6 +267,13 @@ public class GridEditorTest {
     private void startEdit() {
         grid.setEditorEnabled(true);
         grid.editItem(ITEM_ID);
+        // Simulate succesful client response to actually start the editing.
+        try {
+            doEditMethod.invoke(grid);
+        } catch (Exception e) {
+            Assert.fail("Editing item " + ITEM_ID + " failed. Cause: "
+                    + e.getCause().toString());
+        }
     }
 
     private Item getEditedItem() {