#9145 Reviewed and committed Petri's patch with minor cleanup and clarifications

svn changeset:24196/svn branch:6.8

4 files changed, 247 insertions, 41 deletions

diff --git a/src/com/vaadin/data/util/sqlcontainer/ColumnProperty.java b/src/com/vaadin/data/util/sqlcontainer/ColumnProperty.java
index 85ff60d5d9..81c0568f15 100644
--- a/src/com/vaadin/data/util/sqlcontainer/ColumnProperty.java
+++ b/src/com/vaadin/data/util/sqlcontainer/ColumnProperty.java
@@ -36,6 +36,7 @@ final public class ColumnProperty implements Property {
     private boolean modified;
 
     private boolean versionColumn;
+    private boolean primaryKey = false;
 
     /**
      * Prevent instantiation without required parameters.
@@ -44,9 +45,50 @@ final public class ColumnProperty implements Property {
     private ColumnProperty() {
     }
 
+    /**
+     * Deprecated constructor for ColumnProperty. If this is used the primary
+     * keys are not identified correctly in some cases for some databases (i.e.
+     * Oracle). See http://dev.vaadin.com/ticket/9145.
+     * 
+     * @param propertyId
+     * @param readOnly
+     * @param allowReadOnlyChange
+     * @param nullable
+     * @param value
+     * @param type
+     * 
+     * @deprecated
+     */
+    @Deprecated
     public ColumnProperty(String propertyId, boolean readOnly,
             boolean allowReadOnlyChange, boolean nullable, Object value,
             Class<?> type) {
+        this(propertyId, readOnly, allowReadOnlyChange, nullable, false, value,
+                type);
+    }
+
+    /**
+     * Creates a new ColumnProperty instance.
+     * 
+     * @param propertyId
+     *            The ID of this property.
+     * @param readOnly
+     *            Whether this property is read-only.
+     * @param allowReadOnlyChange
+     *            Whether the read-only status of this property can be changed.
+     * @param nullable
+     *            Whether this property accepts null values.
+     * @param primaryKey
+     *            Whether this property corresponds to a database primary key.
+     * @param value
+     *            The value of this property.
+     * @param type
+     *            The type of this property.
+     */
+    public ColumnProperty(String propertyId, boolean readOnly,
+            boolean allowReadOnlyChange, boolean nullable, boolean primaryKey,
+            Object value, Class<?> type) {
+
         if (propertyId == null) {
             throw new IllegalArgumentException("Properties must be named.");
         }
@@ -60,8 +102,15 @@ final public class ColumnProperty implements Property {
         this.allowReadOnlyChange = allowReadOnlyChange;
         this.nullable = nullable;
         this.readOnly = readOnly;
+        this.primaryKey = primaryKey;
     }
 
+    /**
+     * Returns the current value for this property. To get the previous value
+     * (if one exists) for a modified property use {@link #getOldValue()}.
+     * 
+     * @return
+     */
     public Object getValue() {
         if (isModified()) {
             return changedValue;
@@ -69,6 +118,17 @@ final public class ColumnProperty implements Property {
         return value;
     }
 
+    /**
+     * Returns the original non-modified value of this property if it has been
+     * modified.
+     * 
+     * @return The original value if <code>isModified()</code> is true,
+     *         <code>getValue()</code> otherwise.
+     */
+    public Object getOldValue() {
+        return value;
+    }
+
     public void setValue(Object newValue) throws ReadOnlyException,
             ConversionException {
         if (newValue == null && !nullable) {
@@ -154,6 +214,17 @@ final public class ColumnProperty implements Property {
         return readOnly;
     }
 
+    /**
+     * Returns whether the read-only status of this property can be changed
+     * using {@link #setReadOnly(boolean)}.
+     * <p>
+     * Used to prevent setting to read/write mode a property that is not allowed
+     * to be written by the underlying database. Also used for values like
+     * VERSION and AUTO_INCREMENT fields that might be set to read-only by the
+     * container but the database still allows writes.
+     * 
+     * @return true if the read-only status can be changed, false otherwise.
+     */
     public boolean isReadOnlyChangeAllowed() {
         return allowReadOnlyChange;
     }
@@ -164,6 +235,10 @@ final public class ColumnProperty implements Property {
         }
     }
 
+    public boolean isPrimaryKey() {
+        return primaryKey;
+    }
+
     public String getPropertyId() {
         return propertyId;
     }
@@ -205,6 +280,32 @@ final public class ColumnProperty implements Property {
     }
 
     /**
+     * Return whether the value of this property should be persisted to the
+     * database.
+     * 
+     * @return true if the value should be written to the database, false
+     *         otherwise.
+     */
+    public boolean isPersistent() {
+        if (isVersionColumn()) {
+            return false;
+        } else if (isReadOnlyChangeAllowed() && !isReadOnly()) {
+            return true;
+        } else {
+            return false;
+        }
+    }
+
+    /**
+     * Returns whether or not this property is used as a row identifier.
+     * 
+     * @return true if the property is a row identifier, false otherwise.
+     */
+    public boolean isRowIdentifier() {
+        return isPrimaryKey() || isVersionColumn();
+    }
+
+    /**
      * An exception that signals that a <code>null</code> value was passed to
      * the <code>setValue</code> method, but the value of this property can not
      * be set to <code>null</code>.
diff --git a/src/com/vaadin/data/util/sqlcontainer/SQLContainer.java b/src/com/vaadin/data/util/sqlcontainer/SQLContainer.java
index 32afc337a8..71b260fa21 100644
--- a/src/com/vaadin/data/util/sqlcontainer/SQLContainer.java
+++ b/src/com/vaadin/data/util/sqlcontainer/SQLContainer.java
@@ -56,7 +56,9 @@ public class SQLContainer implements Container, Container.Filterable,
     private final List<String> propertyIds = new ArrayList<String>();
     private final Map<String, Class<?>> propertyTypes = new HashMap<String, Class<?>>();
     private final Map<String, Boolean> propertyReadOnly = new HashMap<String, Boolean>();
+    private final Map<String, Boolean> propertyPersistable = new HashMap<String, Boolean>();
     private final Map<String, Boolean> propertyNullable = new HashMap<String, Boolean>();
+    private final Map<String, Boolean> propertyPrimaryKey = new HashMap<String, Boolean>();
 
     /** Filters (WHERE) and sorters (ORDER BY) */
     private final List<Filter> filters = new ArrayList<Filter>();
@@ -136,11 +138,14 @@ public class SQLContainer implements Container, Container.Filterable,
         List<ColumnProperty> itemProperties = new ArrayList<ColumnProperty>();
         for (String propertyId : propertyIds) {
             /* Default settings for new item properties. */
-            itemProperties
-                    .add(new ColumnProperty(propertyId, propertyReadOnly
-                            .get(propertyId),
-                            !propertyReadOnly.get(propertyId), propertyNullable
-                                    .get(propertyId), null, getType(propertyId)));
+            ColumnProperty cp = new ColumnProperty(propertyId,
+                    propertyReadOnly.get(propertyId),
+                    propertyPersistable.get(propertyId),
+                    propertyNullable.get(propertyId),
+                    propertyPrimaryKey.get(propertyId), null,
+                    getType(propertyId));
+
+            itemProperties.add(cp);
         }
         RowItem newRowItem = new RowItem(this, itemId, itemProperties);
 
@@ -1085,14 +1090,22 @@ public class SQLContainer implements Container, Container.Filterable,
                  */
                 boolean readOnly = rsmd.isAutoIncrement(i)
                         || rsmd.isReadOnly(i);
-                if (delegate instanceof TableQuery
-                        && rsmd.getColumnLabel(i).equals(
-                                ((TableQuery) delegate).getVersionColumn())) {
-                    readOnly = true;
+
+                boolean persistable = !rsmd.isReadOnly(i);
+
+                if (delegate instanceof TableQuery) {
+                    if (rsmd.getColumnLabel(i).equals(
+                            ((TableQuery) delegate).getVersionColumn())) {
+                        readOnly = true;
+                    }
                 }
+
                 propertyReadOnly.put(colName, readOnly);
+                propertyPersistable.put(colName, persistable);
                 propertyNullable.put(colName,
                         rsmd.isNullable(i) == ResultSetMetaData.columnNullable);
+                propertyPrimaryKey.put(colName, delegate.getPrimaryKeyColumns()
+                        .contains(rsmd.getColumnLabel(i)));
                 propertyTypes.put(colName, type);
             }
             rs.getStatement().close();
@@ -1192,10 +1205,13 @@ public class SQLContainer implements Container, Container.Filterable,
                          * column.
                          */
                         if (propertiesToAdd.contains(colName)) {
+
                             cp = new ColumnProperty(colName,
                                     propertyReadOnly.get(colName),
-                                    !propertyReadOnly.get(colName),
-                                    propertyNullable.get(colName), value, type);
+                                    propertyPersistable.get(colName),
+                                    propertyNullable.get(colName),
+                                    propertyPrimaryKey.get(colName), value,
+                                    type);
                             itemProperties.add(cp);
                             propertiesToAdd.remove(colName);
                         }
diff --git a/src/com/vaadin/data/util/sqlcontainer/query/generator/DefaultSQLGenerator.java b/src/com/vaadin/data/util/sqlcontainer/query/generator/DefaultSQLGenerator.java
index d1700fc0d2..130a72605f 100644
--- a/src/com/vaadin/data/util/sqlcontainer/query/generator/DefaultSQLGenerator.java
+++ b/src/com/vaadin/data/util/sqlcontainer/query/generator/DefaultSQLGenerator.java
@@ -311,10 +311,8 @@ public class DefaultSQLGenerator implements SQLGenerator {
                     && cp.getPropertyId().equalsIgnoreCase("rownum")) {
                 continue;
             }
-            Object value = cp.getValue() == null ? null : cp.getValue();
-            /* Only include properties whose read-only status can be altered */
-            if (cp.isReadOnlyChangeAllowed() && !cp.isVersionColumn()) {
-                columnToValueMap.put(cp.getPropertyId(), value);
+            if (cp.isPersistent()) {
+                columnToValueMap.put(cp.getPropertyId(), cp.getValue());
             }
         }
         return columnToValueMap;
@@ -329,8 +327,16 @@ public class DefaultSQLGenerator implements SQLGenerator {
                     && cp.getPropertyId().equalsIgnoreCase("rownum")) {
                 continue;
             }
-            Object value = cp.getValue() == null ? null : cp.getValue();
-            if (!cp.isReadOnlyChangeAllowed() || cp.isVersionColumn()) {
+
+            if (cp.isRowIdentifier()) {
+                Object value;
+                if (cp.isPrimaryKey()) {
+                    // If the value of a primary key has changed, its old value
+                    // should be used to identify the row (#9145)
+                    value = cp.getOldValue();
+                } else {
+                    value = cp.getValue();
+                }
                 rowIdentifiers.put(cp.getPropertyId(), value);
             }
         }
diff --git a/tests/server-side/com/vaadin/data/util/sqlcontainer/ColumnPropertyTest.java b/tests/server-side/com/vaadin/data/util/sqlcontainer/ColumnPropertyTest.java
index b9621d518a..09f620cc2a 100644
--- a/tests/server-side/com/vaadin/data/util/sqlcontainer/ColumnPropertyTest.java
+++ b/tests/server-side/com/vaadin/data/util/sqlcontainer/ColumnPropertyTest.java
@@ -4,46 +4,48 @@ import java.sql.ResultSet;
 import java.sql.ResultSetMetaData;
 import java.sql.SQLException;
 import java.sql.Statement;
+import java.util.ArrayList;
 import java.util.Arrays;
 
-import com.vaadin.data.Property.ReadOnlyException;
-import com.vaadin.data.util.sqlcontainer.ColumnProperty.NotNullableException;
-import com.vaadin.data.util.sqlcontainer.query.QueryDelegate;
-
 import org.easymock.EasyMock;
 import org.junit.Assert;
 import org.junit.Test;
 
+import com.vaadin.data.Property.ReadOnlyException;
+import com.vaadin.data.util.sqlcontainer.ColumnProperty.NotNullableException;
+import com.vaadin.data.util.sqlcontainer.query.QueryDelegate;
+
 public class ColumnPropertyTest {
 
     @Test
     public void constructor_legalParameters_shouldSucceed() {
         ColumnProperty cp = new ColumnProperty("NAME", false, true, true,
-                "Ville", String.class);
+                false, "Ville", String.class);
         Assert.assertNotNull(cp);
     }
 
     @Test(expected = IllegalArgumentException.class)
     public void constructor_missingPropertyId_shouldFail() {
-        new ColumnProperty(null, false, true, true, "Ville", String.class);
+        new ColumnProperty(null, false, true, true, false, "Ville",
+                String.class);
     }
 
     @Test(expected = IllegalArgumentException.class)
     public void constructor_missingType_shouldFail() {
-        new ColumnProperty("NAME", false, true, true, "Ville", null);
+        new ColumnProperty("NAME", false, true, true, false, "Ville", null);
     }
 
     @Test
     public void getValue_defaultValue_returnsVille() {
         ColumnProperty cp = new ColumnProperty("NAME", false, true, true,
-                "Ville", String.class);
+                false, "Ville", String.class);
         Assert.assertEquals("Ville", cp.getValue());
     }
 
     @Test
     public void setValue_readWriteNullable_returnsKalle() {
         ColumnProperty cp = new ColumnProperty("NAME", false, true, true,
-                "Ville", String.class);
+                false, "Ville", String.class);
         SQLContainer container = EasyMock.createMock(SQLContainer.class);
         RowItem owner = new RowItem(container, new RowId(new Object[] { 1 }),
                 Arrays.asList(cp));
@@ -57,7 +59,7 @@ public class ColumnPropertyTest {
     @Test(expected = ReadOnlyException.class)
     public void setValue_readOnlyNullable_shouldFail() {
         ColumnProperty cp = new ColumnProperty("NAME", true, true, true,
-                "Ville", String.class);
+                false, "Ville", String.class);
         SQLContainer container = EasyMock.createMock(SQLContainer.class);
         new RowItem(container, new RowId(new Object[] { 1 }), Arrays.asList(cp));
         EasyMock.replay(container);
@@ -68,7 +70,7 @@ public class ColumnPropertyTest {
     @Test
     public void setValue_readWriteNullable_nullShouldWork() {
         ColumnProperty cp = new ColumnProperty("NAME", false, true, true,
-                "Ville", String.class);
+                false, "Ville", String.class);
         SQLContainer container = EasyMock.createMock(SQLContainer.class);
         RowItem owner = new RowItem(container, new RowId(new Object[] { 1 }),
                 Arrays.asList(cp));
@@ -82,7 +84,7 @@ public class ColumnPropertyTest {
     @Test(expected = NotNullableException.class)
     public void setValue_readWriteNotNullable_nullShouldFail() {
         ColumnProperty cp = new ColumnProperty("NAME", false, true, false,
-                "Ville", String.class);
+                false, "Ville", String.class);
         SQLContainer container = EasyMock.createMock(SQLContainer.class);
         RowItem owner = new RowItem(container, new RowId(new Object[] { 1 }),
                 Arrays.asList(cp));
@@ -96,28 +98,28 @@ public class ColumnPropertyTest {
     @Test
     public void getType_normal_returnsStringClass() {
         ColumnProperty cp = new ColumnProperty("NAME", false, true, true,
-                "Ville", String.class);
+                false, "Ville", String.class);
         Assert.assertSame(String.class, cp.getType());
     }
 
     @Test
     public void isReadOnly_readWriteNullable_returnsTrue() {
         ColumnProperty cp = new ColumnProperty("NAME", false, true, true,
-                "Ville", String.class);
+                false, "Ville", String.class);
         Assert.assertFalse(cp.isReadOnly());
     }
 
     @Test
     public void isReadOnly_readOnlyNullable_returnsTrue() {
         ColumnProperty cp = new ColumnProperty("NAME", true, true, true,
-                "Ville", String.class);
+                false, "Ville", String.class);
         Assert.assertTrue(cp.isReadOnly());
     }
 
     @Test
     public void setReadOnly_readOnlyChangeAllowed_shouldSucceed() {
         ColumnProperty cp = new ColumnProperty("NAME", false, true, true,
-                "Ville", String.class);
+                false, "Ville", String.class);
         cp.setReadOnly(true);
         Assert.assertTrue(cp.isReadOnly());
     }
@@ -125,7 +127,7 @@ public class ColumnPropertyTest {
     @Test
     public void setReadOnly_readOnlyChangeDisallowed_shouldFail() {
         ColumnProperty cp = new ColumnProperty("NAME", false, false, true,
-                "Ville", String.class);
+                false, "Ville", String.class);
         cp.setReadOnly(true);
         Assert.assertFalse(cp.isReadOnly());
     }
@@ -133,14 +135,14 @@ public class ColumnPropertyTest {
     @Test
     public void getPropertyId_normal_returnsNAME() {
         ColumnProperty cp = new ColumnProperty("NAME", false, false, true,
-                "Ville", String.class);
+                false, "Ville", String.class);
         Assert.assertEquals("NAME", cp.getPropertyId());
     }
 
     @Test
     public void isModified_valueModified_returnsTrue() {
         ColumnProperty cp = new ColumnProperty("NAME", false, true, true,
-                "Ville", String.class);
+                false, "Ville", String.class);
         SQLContainer container = EasyMock.createMock(SQLContainer.class);
         RowItem owner = new RowItem(container, new RowId(new Object[] { 1 }),
                 Arrays.asList(cp));
@@ -155,14 +157,14 @@ public class ColumnPropertyTest {
     @Test
     public void isModified_valueNotModified_returnsFalse() {
         ColumnProperty cp = new ColumnProperty("NAME", false, false, true,
-                "Ville", String.class);
+                false, "Ville", String.class);
         Assert.assertFalse(cp.isModified());
     }
 
     @Test
     public void setValue_nullOnNullable_shouldWork() {
         ColumnProperty cp = new ColumnProperty("NAME", false, true, true,
-                "asdf", String.class);
+                false, "asdf", String.class);
         SQLContainer container = EasyMock.createMock(SQLContainer.class);
         new RowItem(container, new RowId(new Object[] { 1 }), Arrays.asList(cp));
         cp.setValue(null);
@@ -171,8 +173,8 @@ public class ColumnPropertyTest {
 
     @Test
     public void setValue_resetTonullOnNullable_shouldWork() {
-        ColumnProperty cp = new ColumnProperty("NAME", false, true, true, null,
-                String.class);
+        ColumnProperty cp = new ColumnProperty("NAME", false, true, true, false,
+                null, String.class);
         SQLContainer container = EasyMock.createMock(SQLContainer.class);
         new RowItem(container, new RowId(new Object[] { 1 }), Arrays.asList(cp));
         cp.setValue("asdf");
@@ -202,7 +204,7 @@ public class ColumnPropertyTest {
         }
 
         ColumnProperty property = new ColumnProperty("NAME", false, true, true,
-                "Ville", String.class);
+                false, "Ville", String.class);
 
         Statement statement = EasyMock.createNiceMock(Statement.class);
         EasyMock.replay(statement);
@@ -229,4 +231,85 @@ public class ColumnPropertyTest {
         Assert.assertEquals("Kalle", container.value);
         Assert.assertTrue(container.modified);
     }
+
+    @Test
+    public void versionColumnsShouldNotBeInValueMap_shouldReturnFalse() {
+        ColumnProperty property = new ColumnProperty("NAME", false, true, true,
+                false, "Ville", String.class);
+        property.setVersionColumn(true);
+
+        Assert.assertFalse(property.isPersistent());
+    }
+
+    @Test
+    public void neverWritableColumnsShouldNotBeInValueMap_shouldReturnFalse() {
+        ColumnProperty property = new ColumnProperty("NAME", true, false, true,
+                false, "Ville", String.class);
+
+        Assert.assertFalse(property.isPersistent());
+    }
+
+    @Test
+    public void writableColumnsShouldBeInValueMap_shouldReturnTrue() {
+        ColumnProperty property = new ColumnProperty("NAME", false, true, true,
+                false, "Ville", String.class);
+
+        Assert.assertTrue(property.isPersistent());
+    }
+
+    @Test
+    public void writableButReadOnlyColumnsShouldNotBeInValueMap_shouldReturnFalse() {
+        ColumnProperty property = new ColumnProperty("NAME", true, true, true,
+                false, "Ville", String.class);
+
+        Assert.assertFalse(property.isPersistent());
+    }
+
+    @Test
+    public void primKeysShouldBeRowIdentifiers_shouldReturnTrue() {
+        ColumnProperty property = new ColumnProperty("NAME", false, true, true,
+                true, "Ville", String.class);
+
+        Assert.assertTrue(property.isRowIdentifier());
+    }
+
+    @Test
+    public void versionColumnsShouldBeRowIdentifiers_shouldReturnTrue() {
+        ColumnProperty property = new ColumnProperty("NAME", false, true, true,
+                false, "Ville", String.class);
+        property.setVersionColumn(true);
+
+        Assert.assertTrue(property.isRowIdentifier());
+    }
+
+    @Test
+    public void nonPrimKeyOrVersionColumnsShouldBeNotRowIdentifiers_shouldReturnFalse() {
+        ColumnProperty property = new ColumnProperty("NAME", false, true, true,
+                false, "Ville", String.class);
+
+        Assert.assertFalse(property.isRowIdentifier());
+    }
+
+    @Test
+    public void getOldValueShouldReturnPreviousValue_shouldReturnVille() {
+        ColumnProperty property = new ColumnProperty("NAME", false, true, true,
+                false, "Ville", String.class);
+
+        // Here we really don't care about the container management, but in
+        // order to set the value for a column the owner (RowItem) must be set
+        // and to create the owner we must have a container...
+        ArrayList<ColumnProperty> properties = new ArrayList<ColumnProperty>();
+        properties.add(property);
+
+        SQLContainer container = EasyMock.createNiceMock(SQLContainer.class);
+        RowItem rowItem = new RowItem(container, new RowId(new Object[] { 1 }),
+                Arrays.asList(property));
+
+        property.setValue("Kalle");
+        // Just check that the new value was actually set...
+        Assert.assertEquals("Kalle", property.getValue());
+        // Assert that old value is the original value...
+        Assert.assertEquals("Ville", property.getOldValue());
+    }
+
 }