diff options
author | adam <adam@vaadin.com> | 2016-10-18 12:00:55 +0300 |
---|---|---|
committer | Vaadin Code Review <review@vaadin.com> | 2016-11-14 12:10:06 +0000 |
commit | 7535fbf6338be95f79910e4575d1e39796e5c701 (patch) | |
tree | 0217d4f5bdb53fbe3ba86b4488dbb22b9b519a90 | |
parent | 5d2f6a372e0c31c471ecfbe7b7483f32e0a00f1d (diff) | |
download | vaadin-framework-7535fbf6338be95f79910e4575d1e39796e5c701.tar.gz vaadin-framework-7535fbf6338be95f79910e4575d1e39796e5c701.zip |
Fix NPE in case some items don't contain all properties of Grid.
This could occur in when parent is a different entity than its children
in hierarchical data.
Change-Id: Icd53b5b5e5544a3680d0cd99702ab78224b2dc08
5 files changed, 605 insertions, 19 deletions
diff --git a/server/src/main/java/com/vaadin/data/fieldgroup/FieldGroup.java b/server/src/main/java/com/vaadin/data/fieldgroup/FieldGroup.java index 85db78f50e..fadb05cd8c 100644 --- a/server/src/main/java/com/vaadin/data/fieldgroup/FieldGroup.java +++ b/server/src/main/java/com/vaadin/data/fieldgroup/FieldGroup.java @@ -101,6 +101,13 @@ public class FieldGroup implements Serializable { public void setItemDataSource(Item itemDataSource) { this.itemDataSource = itemDataSource; + bindFields(); + } + + /** + * Binds all fields to the properties in the item in use. + */ + protected void bindFields() { for (Field<?> f : fieldToPropertyId.keySet()) { bind(f, fieldToPropertyId.get(f)); } @@ -256,20 +263,7 @@ public class FieldGroup implements Serializable { fieldToPropertyId.put(field, propertyId); propertyIdToField.put(propertyId, field); if (itemDataSource == null) { - // Clear any possible existing binding to clear the field - field.setPropertyDataSource(null); - boolean fieldReadOnly = field.isReadOnly(); - if (!fieldReadOnly) { - field.clear(); - } else { - // Temporarily make the field read-write so we can clear the - // value. Needed because setPropertyDataSource(null) does not - // currently clear the field - // (https://dev.vaadin.com/ticket/14733) - field.setReadOnly(false); - field.clear(); - field.setReadOnly(true); - } + clearField(field); // Will be bound when data source is set return; @@ -281,6 +275,29 @@ public class FieldGroup implements Serializable { } /** + * Clears field and any possible existing binding. + * + * @param field + * The field to be cleared + */ + protected void clearField(Field<?> field) { + // Clear any possible existing binding to clear the field + field.setPropertyDataSource(null); + boolean fieldReadOnly = field.isReadOnly(); + if (!fieldReadOnly) { + field.clear(); + } else { + // Temporarily make the field read-write so we can clear the + // value. Needed because setPropertyDataSource(null) does not + // currently clear the field + // (https://dev.vaadin.com/ticket/14733) + field.setReadOnly(false); + field.clear(); + field.setReadOnly(true); + } + } + + /** * Wrap property to transactional property. */ protected <T> Property.Transactional<T> wrapInTransactionalProperty( diff --git a/server/src/main/java/com/vaadin/ui/Grid.java b/server/src/main/java/com/vaadin/ui/Grid.java index 3bce7fcc39..b8becd1c33 100644 --- a/server/src/main/java/com/vaadin/ui/Grid.java +++ b/server/src/main/java/com/vaadin/ui/Grid.java @@ -104,7 +104,6 @@ import com.vaadin.shared.ui.grid.selection.MultiSelectionModelState; import com.vaadin.shared.ui.grid.selection.SingleSelectionModelServerRpc; import com.vaadin.shared.ui.grid.selection.SingleSelectionModelState; import com.vaadin.shared.util.SharedUtil; -import com.vaadin.ui.Grid.SelectionModel; import com.vaadin.ui.declarative.DesignAttributeHandler; import com.vaadin.ui.declarative.DesignContext; import com.vaadin.ui.declarative.DesignException; @@ -548,6 +547,36 @@ public class Grid extends AbstractFocusable implements SelectionNotifier, } return field; } + + @Override + protected void bindFields() { + List<Field<?>> fields = new ArrayList<Field<?>>(getFields()); + Item itemDataSource = getItemDataSource(); + + if (itemDataSource == null) { + unbindFields(fields); + } else { + bindFields(fields, itemDataSource); + } + } + + private void unbindFields(List<Field<?>> fields) { + for (Field<?> field : fields) { + clearField(field); + unbind(field); + field.setParent(null); + } + } + + private void bindFields(List<Field<?>> fields, + Item itemDataSource) { + for (Field<?> field : fields) { + if (itemDataSource.getItemProperty(getPropertyId(field)) + != null) { + bind(field, getPropertyId(field)); + } + } + } } /** @@ -2197,8 +2226,9 @@ public class Grid extends AbstractFocusable implements SelectionNotifier, Renderer<?> renderer = column.getRenderer(); Item item = cell.getItem(); - Object modelValue = item.getItemProperty(cell.getPropertyId()) - .getValue(); + Property itemProperty = item.getItemProperty(cell.getPropertyId()); + Object modelValue = + itemProperty == null ? null : itemProperty.getValue(); data.put(columnKeys.key(cell.getPropertyId()), AbstractRenderer .encodeValue(modelValue, renderer, converter, getLocale())); @@ -4521,6 +4551,12 @@ public class Grid extends AbstractFocusable implements SelectionNotifier, private boolean editorSaving = false; private FieldGroup editorFieldGroup = new CustomFieldGroup(); + /** + * Poperty ID to Field mapping that stores editor fields set by {@link + * #setEditorField(Object, Field)}. + */ + private Map<Object, Field<?>> editorFields = new HashMap<Object, Field<?>>(); + private CellStyleGenerator cellStyleGenerator; private RowStyleGenerator rowStyleGenerator; @@ -6805,6 +6841,15 @@ public class Grid extends AbstractFocusable implements SelectionNotifier, Field<?> editor = editorFieldGroup.getField(propertyId); + // If field group has no field for this property, see if we have it stored + if (editor == null) { + editor = editorFields.get(propertyId); + if (editor != null) { + editorFieldGroup.bind(editor, propertyId); + } + } + + // Otherwise try to build one try { if (editor == null) { editor = editorFieldGroup.buildAndBind(propertyId); @@ -6860,8 +6905,9 @@ public class Grid extends AbstractFocusable implements SelectionNotifier, editorFieldGroup.setItemDataSource(item); for (Column column : getColumns()) { - column.getState().editorConnector = getEditorField( - column.getPropertyId()); + column.getState().editorConnector = + item.getItemProperty(column.getPropertyId()) == null + ? null : getEditorField(column.getPropertyId()); } editorActive = true; @@ -6890,6 +6936,9 @@ public class Grid extends AbstractFocusable implements SelectionNotifier, field.setParent(this); editorFieldGroup.bind(field, propertyId); } + + // Store field for this property for future reference + editorFields.put(propertyId, field); } /** diff --git a/server/src/test/java/com/vaadin/tests/server/component/grid/GridEditorMissingPropertyTest.java b/server/src/test/java/com/vaadin/tests/server/component/grid/GridEditorMissingPropertyTest.java new file mode 100644 index 0000000000..31afc4276b --- /dev/null +++ b/server/src/test/java/com/vaadin/tests/server/component/grid/GridEditorMissingPropertyTest.java @@ -0,0 +1,318 @@ +package com.vaadin.tests.server.component.grid; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collection; +import java.util.List; + +import org.junit.Before; +import org.junit.Test; + +import com.vaadin.data.Item; +import com.vaadin.data.Property; +import com.vaadin.data.fieldgroup.FieldGroup; +import com.vaadin.data.util.AbstractInMemoryContainer; +import com.vaadin.data.util.BeanItem; +import com.vaadin.ui.Field; +import com.vaadin.ui.PasswordField; +import com.vaadin.ui.TextField; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; + +public class GridEditorMissingPropertyTest { + + private static final String PROPERTY_NAME = "name"; + private static final String PROPERTY_SIZE = "size"; + + private static final String FOLDER_NAME_BEFORE = "Folder name"; + private static final String FOLDER_NAME_AFTER = "Modified folder name"; + private static final String FILE_NAME_BEFORE = "File name"; + private static final String FILE_NAME_AFTER = "Modified file name"; + private static final String FILE_SIZE_BEFORE = "10kB"; + private static final String FILE_SIZE_AFTER = "20MB"; + + private final Grid grid = new Grid(); + + // Test items + private final Folder folder = new Folder(FOLDER_NAME_BEFORE); + private final File file = new File(FILE_NAME_BEFORE, FILE_SIZE_BEFORE); + + @Before + public void setup() throws SecurityException, NoSuchMethodException { + final BeanItem<Entry> folderItem = new BeanItem<Entry>(folder); + final BeanItem<Entry> childItem = new BeanItem<Entry>(file); + + @SuppressWarnings("unchecked") + TestContainer container = new TestContainer( + Arrays.asList(folderItem, childItem), + Arrays.asList(PROPERTY_NAME, PROPERTY_SIZE)); + + grid.setContainerDataSource(container); + grid.setSelectionMode(Grid.SelectionMode.SINGLE); + grid.setEditorEnabled(true); + } + + @Test + public void testBindFields() { + FieldGroup fieldGroup = grid.getEditorFieldGroup(); + + // Item with incomplete property set + fieldGroup.setItemDataSource( + grid.getContainerDataSource().getItem(folder)); + grid.getColumn(PROPERTY_NAME) + .getEditorField(); // called in grid.doEditItem + assertTrue("Properties in item should be bound", + fieldGroup.getBoundPropertyIds().contains(PROPERTY_NAME)); + assertFalse("Properties not present in item should not be bound", + fieldGroup.getBoundPropertyIds().contains(PROPERTY_SIZE)); + assertTrue("All of item's properties should be bound", + fieldGroup.getUnboundPropertyIds().isEmpty()); + + // Unbind all fields + fieldGroup.setItemDataSource(null); + assertTrue("No properties should be bound", + fieldGroup.getBoundPropertyIds().isEmpty()); + assertTrue("No unbound properties should exist", + fieldGroup.getUnboundPropertyIds().isEmpty()); + + // Item with complete property set + fieldGroup + .setItemDataSource(grid.getContainerDataSource().getItem(file)); + grid.getColumn(PROPERTY_NAME).getEditorField(); + grid.getColumn(PROPERTY_SIZE).getEditorField(); + assertTrue("Properties in item should be bound", + fieldGroup.getBoundPropertyIds().contains(PROPERTY_NAME)); + assertTrue("Properties in item should be bound", + fieldGroup.getBoundPropertyIds().contains(PROPERTY_SIZE)); + assertTrue("All of item's properties should be bound", + fieldGroup.getUnboundPropertyIds().isEmpty()); + + // Unbind all fields + fieldGroup.setItemDataSource(null); + assertTrue("No properties should be bound", + fieldGroup.getBoundPropertyIds().isEmpty()); + assertTrue("No unbound properties should exist", + fieldGroup.getUnboundPropertyIds().isEmpty()); + } + + @Test + public void testSetEditorField() { + FieldGroup fieldGroup = grid.getEditorFieldGroup(); + Field editorField = new PasswordField(); + + // Explicitly set editor field + fieldGroup.setItemDataSource( + grid.getContainerDataSource().getItem(folder)); + grid.getColumn(PROPERTY_NAME).setEditorField(editorField); + assertTrue("Editor field should be the one that was previously set", + grid.getColumn(PROPERTY_NAME).getEditorField() == editorField); + + // Reset item + fieldGroup.setItemDataSource(null); + fieldGroup + .setItemDataSource(grid.getContainerDataSource().getItem(file)); + assertTrue("Editor field should be the one that was previously set", + grid.getColumn(PROPERTY_NAME).getEditorField() == editorField); + } + + @Test + public void testEditCell() { + // Row with missing property + startEdit(folder); + assertEquals(folder, grid.getEditedItemId()); + assertEquals(getEditedItem(), + grid.getEditorFieldGroup().getItemDataSource()); + + assertEquals(FOLDER_NAME_BEFORE, + grid.getColumn(PROPERTY_NAME).getEditorField().getValue()); + try { + grid.getColumn(PROPERTY_SIZE).getEditorField(); + fail("Grid.editorFieldGroup should throw BindException by default"); + } catch (FieldGroup.BindException e) { + // BindException is thrown using the default FieldGroup + } + grid.cancelEditor(); + + // Row with all properties + startEdit(file); + assertEquals(file, grid.getEditedItemId()); + assertEquals(getEditedItem(), + grid.getEditorFieldGroup().getItemDataSource()); + + assertEquals(FILE_NAME_BEFORE, + grid.getColumn(PROPERTY_NAME).getEditorField().getValue()); + assertEquals(FILE_SIZE_BEFORE, + grid.getColumn(PROPERTY_SIZE).getEditorField().getValue()); + grid.cancelEditor(); + } + + @Test + public void testCancelEditor() { + // Row with all properties + testCancel(file, PROPERTY_NAME, FILE_NAME_BEFORE, FILE_NAME_AFTER); + testCancel(file, PROPERTY_SIZE, FILE_SIZE_BEFORE, FILE_SIZE_AFTER); + + // Row with missing property + testCancel(folder, PROPERTY_NAME, FOLDER_NAME_BEFORE, FOLDER_NAME_AFTER); + } + + private void testCancel(Object itemId, String propertyId, + String valueBefore, String valueAfter) { + startEdit(itemId); + + TextField field = (TextField) grid.getColumn(propertyId) + .getEditorField(); + field.setValue(valueAfter); + + Property<?> datasource = field.getPropertyDataSource(); + + grid.cancelEditor(); + assertFalse(grid.isEditorActive()); + assertNull(grid.getEditedItemId()); + assertFalse(field.isModified()); + assertEquals("", field.getValue()); + assertEquals(valueBefore, datasource.getValue()); + assertNull(field.getPropertyDataSource()); + assertNull(grid.getEditorFieldGroup().getItemDataSource()); + } + + @Test + public void testSaveEditor() throws Exception { + // Row with all properties + testSave(file, PROPERTY_SIZE, FILE_SIZE_BEFORE, FILE_SIZE_AFTER); + + // Row with missing property + testSave(folder, PROPERTY_NAME, FOLDER_NAME_BEFORE, FOLDER_NAME_AFTER); + } + + private void testSave(Object itemId, String propertyId, String valueBefore, + String valueAfter) throws Exception { + startEdit(itemId); + TextField field = (TextField) grid.getColumn(propertyId) + .getEditorField(); + + field.setValue(valueAfter); + assertEquals(valueBefore, field.getPropertyDataSource().getValue()); + + grid.saveEditor(); + assertTrue(grid.isEditorActive()); + assertFalse(field.isModified()); + assertEquals(valueAfter, field.getValue()); + assertEquals(valueAfter, getEditedProperty(propertyId).getValue()); + grid.cancelEditor(); + } + + private Item getEditedItem() { + assertNotNull(grid.getEditedItemId()); + return grid.getContainerDataSource().getItem(grid.getEditedItemId()); + } + + private Property<?> getEditedProperty(Object propertyId) { + return getEditedItem().getItemProperty(propertyId); + } + + private void startEdit(Object itemId) { + grid.setEditorEnabled(true); + grid.editItem(itemId); + // Simulate succesful client response to actually start the editing. + grid.doEditItem(); + } + + private class TestContainer extends + AbstractInMemoryContainer<Object, String, BeanItem> { + + private final List<BeanItem<Entry>> items; + private final List<String> pids; + + public TestContainer(List<BeanItem<Entry>> items, List<String> pids) { + this.items = items; + this.pids = pids; + } + + @Override + protected List<Object> getAllItemIds() { + List<Object> ids = new ArrayList<Object>(); + for (BeanItem<Entry> item : items) { + ids.add(item.getBean()); + } + return ids; + } + + @Override + protected BeanItem<Entry> getUnfilteredItem(Object itemId) { + for (BeanItem<Entry> item : items) { + if (item.getBean().equals(itemId)) { + return item; + } + } + return null; + } + + @Override + public Collection<?> getContainerPropertyIds() { + return pids; + } + + @Override + public Property getContainerProperty(Object itemId, Object propertyId) { + return getItem(itemId).getItemProperty(propertyId); + } + + @Override + public Class<?> getType(Object propertyId) { + return String.class; + } + } + + public class Entry { + private String name; + + public String getName() { + return name; + } + + public void setName(String name) { + this.name = name; + } + + public Entry(String name) { + this.name = name; + } + } + + public class Folder extends Entry { + + public Folder(String name) { + super(name); + } + } + + public class File extends Entry { + private String size; + + public File(String name, String size) { + super(name); + this.size = size; + } + + public String getSize() { + return size; + } + + public void setSize(String size) { + this.size = size; + } + } + + private class Grid extends com.vaadin.ui.Grid { + @Override + protected void doEditItem() { + super.doEditItem(); + } + } +} diff --git a/uitest/src/main/java/com/vaadin/tests/components/grid/GridMissingProperty.java b/uitest/src/main/java/com/vaadin/tests/components/grid/GridMissingProperty.java new file mode 100644 index 0000000000..d6a74ff5b7 --- /dev/null +++ b/uitest/src/main/java/com/vaadin/tests/components/grid/GridMissingProperty.java @@ -0,0 +1,129 @@ +package com.vaadin.tests.components.grid; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collection; +import java.util.List; + +import com.vaadin.data.Property; +import com.vaadin.data.util.AbstractInMemoryContainer; +import com.vaadin.data.util.BeanItem; +import com.vaadin.server.VaadinRequest; +import com.vaadin.tests.components.AbstractTestUI; +import com.vaadin.ui.Grid; + +public class GridMissingProperty extends AbstractTestUI { + + @Override + protected void setup(VaadinRequest request) { + final Grid grid = new Grid(); + + final Folder folder = new Folder("Folder name"); + final BeanItem<Entry> folderItem = new BeanItem<Entry>(folder); + + final File file = new File("File name", "10kB"); + final BeanItem<Entry> fileItem = new BeanItem<Entry>(file); + + @SuppressWarnings("unchecked") + TestContainer container = new TestContainer( + Arrays.asList(folderItem, fileItem), + Arrays.asList("name", "size")); + + grid.setContainerDataSource(container); + grid.setSelectionMode(Grid.SelectionMode.SINGLE); + grid.setEditorEnabled(true); + + addComponent(grid); + } + + @Override + protected String getTestDescription() { + return "Grid Editor should not throw exception even when items are missing properties."; + } + + private class TestContainer extends + AbstractInMemoryContainer<Object, String, BeanItem> { + + private final List<BeanItem<Entry>> items; + private final List<String> pids; + + public TestContainer(List<BeanItem<Entry>> items, List<String> pids) { + this.items = items; + this.pids = pids; + } + + @Override + protected List<Object> getAllItemIds() { + List<Object> ids = new ArrayList<Object>(); + for (BeanItem<Entry> item : items) { + ids.add(item.getBean()); + } + return ids; + } + + @Override + protected BeanItem<Entry> getUnfilteredItem(Object itemId) { + for (BeanItem<Entry> item : items) { + if (item.getBean().equals(itemId)) { + return item; + } + } + return null; + } + + @Override + public Collection<?> getContainerPropertyIds() { + return pids; + } + + @Override + public Property getContainerProperty(Object itemId, Object propertyId) { + return getItem(itemId).getItemProperty(propertyId); + } + + @Override + public Class<?> getType(Object propertyId) { + return String.class; + } + } + + public class Entry { + private String name; + + public String getName() { + return name; + } + + public void setName(String name) { + this.name = name; + } + + public Entry(String name) { + this.name = name; + } + } + + public class Folder extends Entry { + + public Folder(String name) { + super(name); + } + } + + public class File extends Entry { + private String size; + + public File(String name, String size) { + super(name); + this.size = size; + } + + public String getSize() { + return size; + } + + public void setSize(String size) { + this.size = size; + } + } +} diff --git a/uitest/src/test/java/com/vaadin/tests/components/grid/GridMissingPropertyTest.java b/uitest/src/test/java/com/vaadin/tests/components/grid/GridMissingPropertyTest.java new file mode 100644 index 0000000000..b3ebc00c2a --- /dev/null +++ b/uitest/src/test/java/com/vaadin/tests/components/grid/GridMissingPropertyTest.java @@ -0,0 +1,73 @@ +package com.vaadin.tests.components.grid; + +import org.junit.Test; + +import com.vaadin.testbench.elements.GridElement; +import com.vaadin.testbench.elements.GridElement.GridEditorElement; +import com.vaadin.testbench.elements.TextFieldElement; +import com.vaadin.tests.tb3.SingleBrowserTest; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + +public class GridMissingPropertyTest extends SingleBrowserTest { + + @Test + public void testCellEditable() { + openTestURL(); + GridElement grid = $(GridElement.class).first(); + + // Row with missing property + grid.getCell(0, 0).doubleClick(); + GridEditorElement editor = grid.getEditor(); + + assertTrue("Cell with property should be editable", + editor.isEditable(0)); + assertFalse("Cell without property should not be editable", + editor.isEditable(1)); + + editor.cancel(); + + // Row with all properties + grid.getCell(1, 0).doubleClick(); + editor = grid.getEditor(); + + assertTrue("Cell with property should be editable", + editor.isEditable(0)); + assertTrue("Cell with property should be editable", + editor.isEditable(1)); + + editor.cancel(); + } + + @Test + public void testEditCell() { + openTestURL(); + GridElement grid = $(GridElement.class).first(); + + GridEditorElement editor; + TextFieldElement editorField; + + grid.getCell(0, 0).doubleClick(); + editor = grid.getEditor(); + editorField = editor.getField(0).wrap(TextFieldElement.class); + editorField.setValue("New Folder Name"); + editor.save(); + assertEquals("New Folder Name", grid.getCell(0, 0).getText()); + + grid.getCell(1, 0).doubleClick(); + editor = grid.getEditor(); + editorField = editor.getField(1).wrap(TextFieldElement.class); + editorField.setValue("10 MB"); + editor.save(); + assertEquals("10 MB", grid.getCell(1, 1).getText()); + + grid.getCell(1, 0).doubleClick(); + editor = grid.getEditor(); + editorField = editor.getField(0).wrap(TextFieldElement.class); + editorField.setValue("New File Name"); + editor.save(); + assertEquals("New File Name", grid.getCell(1, 0).getText()); + } +} |