Fixed annotation handling.

- Store as SparseAnnotations internally and convert to Annotations
  when merging multiple provider supplied annotations.

Change-Id: I82fe159b536b3e7344a33e09792f6a3473fb3500

@@ -73,31 +73,63 @@ public final class DefaultAnnotations implements SparseAnnotations {
     }
 
     /**
-     * Convert Annotations to DefaultAnnotations if needed and merges.
+     * Creates the union of two given SparseAnnotations.
+     * Unlike the {@link #merge(DefaultAnnotations, SparseAnnotations)} method,
+     * result will be {@link SparseAnnotations} instead of {@link Annotations}.
      *
-     * @see #merge(DefaultAnnotations, SparseAnnotations)
+     * A key tagged for removal will remain in the output SparseAnnotations,
+     * if the counterpart of the input does not contain the same key.
      *
      * @param annotations       base annotations
      * @param sparseAnnotations additional sparse annotations
      * @return combined annotations or the original base annotations if there
      * are not additional annotations
      */
-    public static DefaultAnnotations merge(Annotations annotations,
-                                    SparseAnnotations sparseAnnotations) {
+    public static SparseAnnotations union(SparseAnnotations annotations,
+                                          SparseAnnotations sparseAnnotations) {
+
+        if (sparseAnnotations == null || sparseAnnotations.keys().isEmpty()) {
+            return annotations;
+        }
+
+        final HashMap<String, String> newMap;
         if (annotations instanceof DefaultAnnotations) {
-            return merge((DefaultAnnotations) annotations, sparseAnnotations);
+            newMap = copy(((DefaultAnnotations) annotations).map);
+        } else {
+            newMap = new HashMap<>(annotations.keys().size() +
+                                   sparseAnnotations.keys().size());
+            putAllSparseAnnotations(newMap, annotations);
         }
 
-        DefaultAnnotations.Builder builder = DefaultAnnotations.builder();
-        for (String key : annotations.keys()) {
-            builder.set(key, annotations.value(key));
+        putAllSparseAnnotations(newMap, sparseAnnotations);
+        return new DefaultAnnotations(newMap);
+    }
+
+    // adds the key-values contained in sparseAnnotations to
+    // newMap, if sparseAnnotations had a key tagged for removal,
+    // and corresponding key exist in newMap, entry will be removed.
+    // if corresponding key does not exist, removal tag will be added to
+    // the newMap.
+    private static void putAllSparseAnnotations(
+                            final HashMap<String, String> newMap,
+                            SparseAnnotations sparseAnnotations) {
+
+        for (String key : sparseAnnotations.keys()) {
+            if (sparseAnnotations.isRemoved(key)) {
+                if (newMap.containsKey(key)) {
+                    newMap.remove(key);
+                } else {
+                    newMap.put(key, Builder.REMOVED);
+                }
+            } else {
+                String value = sparseAnnotations.value(key);
+                newMap.put(key, value);
+            }
         }
-        return merge(builder.build(), sparseAnnotations);
     }
 
     @Override
     public Set<String> keys() {
-        // TODO: unmodifiable to be removed after switching to ImmutableMap;
         return Collections.unmodifiableSet(map.keySet());
     }
 
@@ -115,7 +147,7 @@ public final class DefaultAnnotations implements SparseAnnotations {
     @SuppressWarnings("unchecked")
     private static HashMap<String, String> copy(Map<String, String> original) {
         if (original instanceof HashMap) {
-            return (HashMap) ((HashMap) original).clone();
+            return (HashMap<String, String>) ((HashMap<?, ?>) original).clone();
         }
         throw new IllegalArgumentException("Expecting HashMap instance");
     }

@@ -33,4 +33,4 @@ public class ConnectPointTest {
                 .addEqualityGroup(new ConnectPoint(DID2, P1), new ConnectPoint(DID2, P1))
                 .testEquals();
     }
-}
\ No newline at end of file
+}

@@ -36,6 +36,23 @@ public class DefaultAnnotationsTest {
     }
 
     @Test
+    public void union() {
+        annotations = builder().set("foo", "1").set("bar", "2").remove("buz").build();
+        assertEquals("incorrect keys", of("foo", "bar", "buz"), annotations.keys());
+
+        SparseAnnotations updates = builder().remove("foo").set("bar", "3").set("goo", "4").remove("fuzz").build();
+
+        SparseAnnotations result = DefaultAnnotations.union(annotations, updates);
+
+        assertTrue("remove instruction in original remains", result.isRemoved("buz"));
+        assertTrue("remove instruction in update remains", result.isRemoved("fuzz"));
+        assertEquals("incorrect keys", of("buz", "goo", "bar", "fuzz"), result.keys());
+        assertNull("incorrect value", result.value("foo"));
+        assertEquals("incorrect value", "3", result.value("bar"));
+        assertEquals("incorrect value", "4", result.value("goo"));
+    }
+
+    @Test
     public void merge() {
         annotations = builder().set("foo", "1").set("bar", "2").build();
         assertEquals("incorrect keys", of("foo", "bar"), annotations.keys());
@@ -65,4 +82,4 @@ public class DefaultAnnotationsTest {
         DefaultAnnotations.merge(null, null);
     }
 
-}
\ No newline at end of file
+}

@@ -58,7 +58,5 @@ public class DefaultDeviceTest {
         assertEquals("incorrect hw", HW, device.hwVersion());
         assertEquals("incorrect sw", SW, device.swVersion());
         assertEquals("incorrect serial", SN1, device.serialNumber());
-        assertEquals("incorrect serial", SN1, device.serialNumber());
     }
-
-}
\ No newline at end of file
+}

@@ -37,5 +37,4 @@ public class DefaultGraphDescriptionTest {
         new DefaultGraphDescription(4321L, ImmutableSet.of(DEV1, DEV3),
                                     ImmutableSet.of(L1, L2));
     }
-
-}
\ No newline at end of file
+}

@@ -50,5 +50,4 @@ public class DefaultTopologyEdgeTest {
                                   new DefaultTopologyEdge(V2, V1, L2))
                 .testEquals();
     }
-
-}
\ No newline at end of file
+}

@@ -26,5 +26,4 @@ public class DefaultTopologyVertexTest {
                 .addEqualityGroup(new DefaultTopologyVertex(D2),
                                   new DefaultTopologyVertex(D2)).testEquals();
     }
-
-}
\ No newline at end of file
+}

@@ -56,6 +56,7 @@ import static org.onlab.onos.net.device.DeviceEvent.Type.*;
 import static org.slf4j.LoggerFactory.getLogger;
 import static org.apache.commons.lang3.concurrent.ConcurrentUtils.createIfAbsentUnchecked;
 import static org.onlab.onos.net.DefaultAnnotations.merge;
+import static org.onlab.onos.net.DefaultAnnotations.union;
 import static com.google.common.base.Verify.verify;
 
 // TODO: implement remove event handling and call *Internal
@@ -603,7 +604,7 @@ public class GossipDeviceStore
             Timestamped<DeviceDescription> oldOne = deviceDesc.get();
             Timestamped<DeviceDescription> newOne = newDesc;
             if (oldOne != null) {
-                SparseAnnotations merged = merge(oldOne.value().annotations(),
+                SparseAnnotations merged = union(oldOne.value().annotations(),
                                                  newDesc.value().annotations());
                 newOne = new Timestamped<DeviceDescription>(
                         new DefaultDeviceDescription(newDesc.value(), merged),
@@ -622,7 +623,7 @@ public class GossipDeviceStore
             Timestamped<PortDescription> oldOne = portDescs.get(newDesc.value().portNumber());
             Timestamped<PortDescription> newOne = newDesc;
             if (oldOne != null) {
-                SparseAnnotations merged = merge(oldOne.value().annotations(),
+                SparseAnnotations merged = union(oldOne.value().annotations(),
                                                  newDesc.value().annotations());
                 newOne = new Timestamped<PortDescription>(
                         new DefaultPortDescription(newDesc.value(), merged),

@@ -51,6 +51,7 @@ import static com.google.common.base.Predicates.notNull;
 import static org.onlab.onos.net.device.DeviceEvent.Type.*;
 import static org.slf4j.LoggerFactory.getLogger;
 import static org.apache.commons.lang3.concurrent.ConcurrentUtils.createIfAbsentUnchecked;
+import static org.onlab.onos.net.DefaultAnnotations.union;
 import static org.onlab.onos.net.DefaultAnnotations.merge;
 
 /**
@@ -488,7 +489,7 @@ public class SimpleDeviceStore
             DeviceDescription oldOne = deviceDesc.get();
             DeviceDescription newOne = newDesc;
             if (oldOne != null) {
-                SparseAnnotations merged = merge(oldOne.annotations(),
+                SparseAnnotations merged = union(oldOne.annotations(),
                                                  newDesc.annotations());
                 newOne = new DefaultDeviceDescription(newOne, merged);
             }
@@ -505,7 +506,7 @@ public class SimpleDeviceStore
             PortDescription oldOne = portDescs.get(newDesc.portNumber());
             PortDescription newOne = newDesc;
             if (oldOne != null) {
-                SparseAnnotations merged = merge(oldOne.annotations(),
+                SparseAnnotations merged = union(oldOne.annotations(),
                                                  newDesc.annotations());
                 newOne = new DefaultPortDescription(newOne, merged);
             }

@@ -11,7 +11,6 @@ import org.apache.felix.scr.annotations.Activate;
 import org.apache.felix.scr.annotations.Component;
 import org.apache.felix.scr.annotations.Deactivate;
 import org.apache.felix.scr.annotations.Service;
-import org.onlab.onos.net.Annotations;
 import org.onlab.onos.net.AnnotationsUtil;
 import org.onlab.onos.net.ConnectPoint;
 import org.onlab.onos.net.DefaultAnnotations;
@@ -39,6 +38,7 @@ import java.util.Map.Entry;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.ConcurrentMap;
 
+import static org.onlab.onos.net.DefaultAnnotations.union;
 import static org.onlab.onos.net.DefaultAnnotations.merge;
 import static org.onlab.onos.net.Link.Type.DIRECT;
 import static org.onlab.onos.net.Link.Type.INDIRECT;
@@ -173,7 +173,7 @@ public class SimpleLinkStore
         LinkDescription oldDesc = descs.get(providerId);
         LinkDescription newDesc = linkDescription;
         if (oldDesc != null) {
-            SparseAnnotations merged = merge(oldDesc.annotations(),
+            SparseAnnotations merged = union(oldDesc.annotations(),
                     linkDescription.annotations());
             newDesc = new DefaultLinkDescription(
                         linkDescription.src(),
@@ -268,7 +268,7 @@ public class SimpleLinkStore
         ConnectPoint src = base.src();
         ConnectPoint dst = base.dst();
         Type type = base.type();
-        Annotations annotations = DefaultAnnotations.builder().build();
+        DefaultAnnotations annotations = DefaultAnnotations.builder().build();
         annotations = merge(annotations, base.annotations());
 
         for (Entry<ProviderId, LinkDescription> e : descs.entrySet()) {