sis-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From desruisse...@apache.org
Subject svn commit: r1478760 - in /sis/branches/JDK7/sis-metadata/src: main/java/org/apache/sis/metadata/ test/java/org/apache/sis/metadata/
Date Fri, 03 May 2013 12:44:09 GMT
Author: desruisseaux
Date: Fri May  3 12:44:09 2013
New Revision: 1478760

URL: http://svn.apache.org/r1478760
Log:
MetadataTreeChildren.add(...) shall only add, not overwrite previous values.

Modified:
    sis/branches/JDK7/sis-metadata/src/main/java/org/apache/sis/metadata/MetadataTreeChildren.java
    sis/branches/JDK7/sis-metadata/src/main/java/org/apache/sis/metadata/PropertyAccessor.java
    sis/branches/JDK7/sis-metadata/src/test/java/org/apache/sis/metadata/MetadataTreeChildrenTest.java
    sis/branches/JDK7/sis-metadata/src/test/java/org/apache/sis/metadata/PropertyAccessorTest.java

Modified: sis/branches/JDK7/sis-metadata/src/main/java/org/apache/sis/metadata/MetadataTreeChildren.java
URL: http://svn.apache.org/viewvc/sis/branches/JDK7/sis-metadata/src/main/java/org/apache/sis/metadata/MetadataTreeChildren.java?rev=1478760&r1=1478759&r2=1478760&view=diff
==============================================================================
--- sis/branches/JDK7/sis-metadata/src/main/java/org/apache/sis/metadata/MetadataTreeChildren.java
[UTF-8] (original)
+++ sis/branches/JDK7/sis-metadata/src/main/java/org/apache/sis/metadata/MetadataTreeChildren.java
[UTF-8] Fri May  3 12:44:09 2013
@@ -471,7 +471,7 @@ final class MetadataTreeChildren extends
      * are ignored.
      *
      * <p>If the identified property is a collection, then this method adds the value
to that collection.
-     * Otherwise the new value will overwrite the value that existed prior this method call.</p>
+     * Otherwise the new value will be set only if the previous value is null or empty.</p>
      *
      * <p>This method does not iterate explicitly through the children list, because
adding a metadata
      * object implicitly adds all its children.</p>
@@ -480,6 +480,7 @@ final class MetadataTreeChildren extends
      * @return {@code true} if the metadata changed as a result of this method call.
      * @throws NullPointerException if the given node is null.
      * @throws IllegalArgumentException if this list does not have a property for the node
identifier.
+     * @throws IllegalStateException if a value already exists and no more value can be added
for the node identifier.
      * @throws UnmodifiableMetadataException if the property for the node identifier is read-only.
      * @throws ClassCastException if the node value is not of the expected type.
      * @throws BackingStoreException if the metadata implementation threw a checked exception.
@@ -496,11 +497,14 @@ final class MetadataTreeChildren extends
             return false;
         }
         final int index = accessor.indexOf(identifier, true);
-        if ((Boolean) accessor.set(index, metadata, value, PropertyAccessor.RETURN_CHANGED))
{
+        final Boolean changed = (Boolean) accessor.set(index, metadata, value, PropertyAccessor.APPEND);
+        if (changed == null) {
+            throw new IllegalStateException(Errors.format(Errors.Keys.ValueAlreadyDefined_1,
identifier));
+        }
+        if (changed) {
             modCount++;
-            return true;
         }
-        return false;
+        return changed;
     }
 
     /**

Modified: sis/branches/JDK7/sis-metadata/src/main/java/org/apache/sis/metadata/PropertyAccessor.java
URL: http://svn.apache.org/viewvc/sis/branches/JDK7/sis-metadata/src/main/java/org/apache/sis/metadata/PropertyAccessor.java?rev=1478760&r1=1478759&r2=1478760&view=diff
==============================================================================
--- sis/branches/JDK7/sis-metadata/src/main/java/org/apache/sis/metadata/PropertyAccessor.java
[UTF-8] (original)
+++ sis/branches/JDK7/sis-metadata/src/main/java/org/apache/sis/metadata/PropertyAccessor.java
[UTF-8] Fri May  3 12:44:09 2013
@@ -107,7 +107,7 @@ final class PropertyAccessor {
      * Enumeration constants for the {@code mode} argument
      * in the {@link #set(int, Object, Object, int)} method.
      */
-    static final int RETURN_NULL=0, RETURN_CHANGED=1, RETURN_PREVIOUS=2;
+    static final int RETURN_NULL=0, RETURN_PREVIOUS=1, APPEND=2;
 
     /**
      * Additional getter to declare in every list of getter methods that do not already provide
@@ -670,21 +670,22 @@ final class PropertyAccessor {
 
     /**
      * Sets a value for the specified metadata and returns the old value if {@code mode}
is
-     * {@link #RETURN_PREVIOUS}. The {@code mode} argument can be one of the following, from
-     * cheapest to more expensive mode:
+     * {@link #RETURN_PREVIOUS}. The {@code mode} argument can be one of the following:
      *
      * <ul>
      *   <li>RETURN_NULL:     Set the value and returns {@code null}.</li>
-     *   <li>RETURN_CHANGED:  Set the value and returns {@link Boolean#TRUE} if the
metadata changed
-     *                        as a result of this method call, or {@code Boolean#FALSE} otherwise.</li>
      *   <li>RETURN_PREVIOUS: Set the value and returns the previous value. If the
previous value was a
      *                        collection or a map, then that value is copied in a new collection
or map
      *                        before the new value is set because the setter methods typically
copy the
      *                        new collection in their existing instance.</li>
+     *   <li>APPEND:          Set the value only if it doesn't overwrite an existing
value, then returns
+     *                        {@link Boolean#TRUE} if the metadata changed as a result of
this method call,
+     *                        {@code Boolean#FALSE} if the metadata didn't changed or {@code
null} if the
+     *                        value can not be set because an other value already exists.</li>
      * </ul>
      *
-     * <p>The {@code RETURN_CHANGED} mode has an additional side effect: it sets the
{@code append} argument
-     * to {@code true} in the call to the {@link #convert(Method, Object, Object, Object[],
Class, boolean)}
+     * <p>The {@code APPEND} mode has an additional side effect: it sets the {@code
append} argument to
+     * {@code true} in the call to the {@link #convert(Method, Object, Object, Object[],
Class, boolean)}
      * method. See the {@code convert} javadoc for more information.</p>
      *
      * <p>If the given index is out of bounds, then this method does nothing and return
{@code null}.
@@ -696,7 +697,7 @@ final class PropertyAccessor {
      * @param  index       The index of the property to set.
      * @param  metadata    The metadata object on which to set the value.
      * @param  value       The new value.
-     * @param  mode Whether this method should first fetches the old value,
+     * @param  mode        Whether this method should first fetches the old value,
      *                     as one of the {@code RETURN_*} constants.
      * @return The old value, or {@code null} if {@code returnValue} was {@code RETURN_NULL}.
      * @throws UnmodifiableMetadataException if the property for the given key is read-only.
@@ -713,33 +714,58 @@ final class PropertyAccessor {
             final Method getter = getters[index];
             final Method setter = setters[index];
             if (setter != null) {
-                Object old  = null;
-                Object copy = null;
-                if (mode != RETURN_NULL) {
-                    old  = get(getter, metadata);
-                    copy = old;
-                    if (mode == RETURN_PREVIOUS) {
-                        if (old instanceof Collection<?>) {
-                            if (old instanceof List<?>) {
-                                copy = snapshot((List<?>) old);
+                final Object oldValue;
+                final Object snapshot; // Copy of oldValue before modification.
+                switch (mode) {
+                    case RETURN_NULL: {
+                        oldValue = null;
+                        snapshot = null;
+                        break;
+                    }
+                    case APPEND: {
+                        oldValue = get(getter, metadata);
+                        snapshot = null;
+                        break;
+                    }
+                    case RETURN_PREVIOUS: {
+                        oldValue = get(getter, metadata);
+                        if (oldValue instanceof Collection<?>) {
+                            if (oldValue instanceof List<?>) {
+                                snapshot = snapshot((List<?>) oldValue);
                             } else {
-                                copy = modifiableCopy((Collection<?>) old);
+                                snapshot = modifiableCopy((Collection<?>) oldValue);
                             }
-                        } else if (old instanceof Map<?,?>) {
-                            copy = modifiableCopy((Map<?,?>) old);
+                        } else if (oldValue instanceof Map<?,?>) {
+                            snapshot = modifiableCopy((Map<?,?>) oldValue);
+                        } else {
+                            snapshot = oldValue;
                         }
+                        break;
                     }
+                    default: throw new AssertionError(mode);
                 }
+                /*
+                 * Converts the new value to a type acceptable for the setter method (if
possible).
+                 * If the new value is a singleton while the expected type is a collection,
then the 'convert'
+                 * method added the singleton in the existing collection, which may result
in no change if the
+                 * collection is a Set and the new value already exists in that Set. If we
detect that there is
+                 * no change, then we don't need to invoke the setter method. Note that we
conservatively assume
+                 * that there is always a change in RETURN_NULL mode since we don't know
the previous value.
+                 */
                 final Object[] newValues = new Object[] {value};
-                Boolean changed = convert(getter, metadata, old, newValues, elementTypes[index],
mode == RETURN_CHANGED);
-                set(setter, metadata, newValues);
-                if (mode == RETURN_CHANGED) {
-                    if (changed == null) {
-                        changed = (newValues[0] != old);
+                Boolean changed = convert(getter, metadata, oldValue, newValues, elementTypes[index],
mode == APPEND);
+                if (changed == null) {
+                    changed = (mode == RETURN_NULL) || (newValues[0] != oldValue);
+                    if (changed && mode == APPEND && !ValueExistencePolicy.isNullOrEmpty(oldValue))
{
+                        // If 'convert' did not added the value in a collection and if a
value already
+                        // exists, do not modify the existing value. Exit now with "no change"
status.
+                        return null;
                     }
-                    return changed;
                 }
-                return copy;
+                if (changed) {
+                    set(setter, metadata, newValues);
+                }
+                return (mode == APPEND) ? changed : snapshot;
             }
         }
         throw new UnmodifiableMetadataException(Errors.format(Errors.Keys.CanNotSetPropertyValue_1,
names[index]));

Modified: sis/branches/JDK7/sis-metadata/src/test/java/org/apache/sis/metadata/MetadataTreeChildrenTest.java
URL: http://svn.apache.org/viewvc/sis/branches/JDK7/sis-metadata/src/test/java/org/apache/sis/metadata/MetadataTreeChildrenTest.java?rev=1478760&r1=1478759&r2=1478760&view=diff
==============================================================================
--- sis/branches/JDK7/sis-metadata/src/test/java/org/apache/sis/metadata/MetadataTreeChildrenTest.java
[UTF-8] (original)
+++ sis/branches/JDK7/sis-metadata/src/test/java/org/apache/sis/metadata/MetadataTreeChildrenTest.java
[UTF-8] Fri May  3 12:44:09 2013
@@ -209,7 +209,14 @@ public final strictfp class MetadataTree
         toAdd.setValue(TableColumn.VALUE, citation.getEdition());
         assertFalse("Adding the same value shall be a no-op.", children.add(toAdd));
         toAdd.setValue(TableColumn.VALUE, "New edition");
-        assertTrue("Setting a different value shall be a change.", children.add(toAdd));
+        try {
+            children.add(toAdd);
+            fail("Setting a different value shall be refused.");
+        } catch (IllegalStateException e) {
+            assertTrue(e.getMessage().contains("edition"));
+        }
+        citation.setEdition(null); // Clears so we are allowed to add.
+        assertTrue("Setting a new value shall be a change.", children.add(toAdd));
 
         toAdd.setValue(TableColumn.IDENTIFIER, "presentationForm");
         toAdd.setValue(TableColumn.VALUE, PresentationForm.MAP_DIGITAL);

Modified: sis/branches/JDK7/sis-metadata/src/test/java/org/apache/sis/metadata/PropertyAccessorTest.java
URL: http://svn.apache.org/viewvc/sis/branches/JDK7/sis-metadata/src/test/java/org/apache/sis/metadata/PropertyAccessorTest.java?rev=1478760&r1=1478759&r2=1478760&view=diff
==============================================================================
--- sis/branches/JDK7/sis-metadata/src/test/java/org/apache/sis/metadata/PropertyAccessorTest.java
[UTF-8] (original)
+++ sis/branches/JDK7/sis-metadata/src/test/java/org/apache/sis/metadata/PropertyAccessorTest.java
[UTF-8] Fri May  3 12:44:09 2013
@@ -53,7 +53,8 @@ import org.junit.Test;
 import static java.util.Collections.singleton;
 import static org.opengis.test.Assert.*;
 import static org.apache.sis.test.TestUtilities.getSingleton;
-import static org.apache.sis.metadata.PropertyAccessor.RETURN_CHANGED;
+import static org.apache.sis.metadata.PropertyAccessor.APPEND;
+import static org.apache.sis.metadata.PropertyAccessor.RETURN_NULL;
 import static org.apache.sis.metadata.PropertyAccessor.RETURN_PREVIOUS;
 
 
@@ -279,6 +280,33 @@ public final strictfp class PropertyAcce
     }
 
     /**
+     * Tests the {@link PropertyAccessor#set(int, Object, Object, int)} method with a {@code
null} value.
+     * Setting a property to {@code null} is equivalent to removing that property value.
+     * The metadata object used by this test (before removal) is:
+     *
+     * {@preformat text
+     *   DefaultCitation
+     *     └─Title………………………… Some title
+     * }
+     */
+    @Test
+    @DependsOnMethod("testSet")
+    public void testSetNull() {
+        final DefaultCitation  instance = new DefaultCitation("Some title");
+        final PropertyAccessor accessor = createPropertyAccessor();
+        final InternationalString title = instance.getTitle();
+        final int index = accessor.indexOf("title", true);
+
+        assertEquals("Some title", title.toString()); // Sanity check before to continue.
+        assertNull("title", accessor.set(index, instance, null, RETURN_NULL));
+        assertNull("title", instance.getTitle());
+
+        instance.setTitle(title);
+        assertSame("title", title, accessor.set(index, instance, null, RETURN_PREVIOUS));
+        assertNull("title", instance.getTitle());
+    }
+
+    /**
      * Tests the {@link PropertyAccessor#set(int, Object, Object, int)} method
      * with a value that will need to be converted. The conversion will be from
      * {@link String} to {@link InternationalString}. The created metadata object is:
@@ -414,7 +442,7 @@ public final strictfp class PropertyAcce
 
     /**
      * Tests the {@link PropertyAccessor#set(int, Object, Object, int)} method in
-     * {@link PropertyAccessor#RETURN_CHANGED} mode. In this mode, new collections
+     * {@link PropertyAccessor#APPEND} mode. In this mode, new collections
      * are added into existing collections instead than replacing them.
      * The metadata object created by this test after the merge is:
      *
@@ -444,23 +472,29 @@ public final strictfp class PropertyAcce
         instance.setAlternateTitles(oldTitles);
         final PropertyAccessor accessor = createPropertyAccessor();
         final int titleIndex = accessor.indexOf("title", true);
-        Object titleChanged = accessor.set(titleIndex, instance, "Added title", RETURN_CHANGED);
+        Object titleChanged = accessor.set(titleIndex, instance, "Added title", APPEND);
 
         // Set the alternate titles.
         final int    index    = accessor.indexOf("alternateTitles", true);
-        final Object changed  = accessor.set(index, instance, newTitles, RETURN_CHANGED);
+        final Object changed  = accessor.set(index, instance, newTitles, APPEND);
         final Object newValue = accessor.get(index, instance);
 
         // Verify the values.
-        assertEquals("set(…, RETURN_CHANGED)", Boolean.TRUE, titleChanged);
-        assertEquals("set(…, RETURN_CHANGED)", Boolean.TRUE, changed);
-        assertEquals("get(…)",                 merged, newValue);
-        assertSame  ("alternateTitles",        newValue, instance.getAlternateTitles());
+        assertEquals("set(…, APPEND)",  Boolean.TRUE, titleChanged);
+        assertEquals("set(…, APPEND)",  Boolean.TRUE, changed);
+        assertEquals("get(…)",          merged, newValue);
+        assertSame  ("alternateTitles", newValue, instance.getAlternateTitles());
         assertEquals("title", "Added title", instance.getTitle().toString());
 
         // Test setting again the title to the same value.
-        titleChanged = accessor.set(titleIndex, instance, "Added title", RETURN_CHANGED);
-        assertEquals("set(…, RETURN_CHANGED)", Boolean.FALSE, titleChanged);
+        titleChanged = accessor.set(titleIndex, instance, "Added title", APPEND);
+        assertEquals("set(…, APPEND)", Boolean.FALSE, titleChanged);
+        assertEquals("title", "Added title", instance.getTitle().toString());
+
+        // Test setting the title to a different value.
+        titleChanged = accessor.set(titleIndex, instance, "Different title", APPEND);
+        assertNull("set(…, APPEND)", titleChanged); // Operation shall be refused.
+        assertEquals("title", "Added title", instance.getTitle().toString());
     }
 
     /**



Mime
View raw message