Bugfix: OpticalPortOperator should be able to overwrite port number.

- bug introduced in ONOS-3503.
- added exact equality comparison method to PortNumber
- removed null PortNumber case handling test

Change-Id: I6d1f191b5a64b79426de9d80cffadd6c9de96d56

@@ -15,6 +15,7 @@
  */
 package org.onosproject.net;
 
+import com.google.common.base.Objects;
 import com.google.common.base.Supplier;
 import com.google.common.base.Suppliers;
 import com.google.common.collect.ImmutableMap;
@@ -273,4 +274,21 @@ public final class PortNumber {
         }
         return false;
     }
+
+    /**
+     * Indicates whether some other PortNumber object is equal to this one
+     * including it's name.
+     *
+     * @param that other {@link PortNumber} instance to compare
+     * @return true if equal, false otherwise
+     */
+    public boolean exactlyEquals(PortNumber that) {
+        if (this == that) {
+            return true;
+        }
+
+        return this.equals(that) &&
+               this.hasName == that.hasName &&
+               Objects.equal(this.name, that.name);
+    }
 }

@@ -20,6 +20,7 @@ import org.onosproject.net.AbstractDescription;
 import org.onosproject.net.PortNumber;
 import org.onosproject.net.SparseAnnotations;
 
+import static com.google.common.base.Preconditions.checkNotNull;
 import static org.onosproject.net.Port.Type;
 import com.google.common.base.Objects;
 
@@ -61,7 +62,7 @@ public class DefaultPortDescription extends AbstractDescription
                                   Type type, long portSpeed,
                                   SparseAnnotations...annotations) {
         super(annotations);
-        this.number = number;
+        this.number = checkNotNull(number);
         this.isEnabled = isEnabled;
         this.type = type;
         this.portSpeed = portSpeed;

@@ -21,7 +21,7 @@ import org.junit.Test;
 import org.onosproject.net.PortNumber.Logical;
 
 import static java.util.stream.Collectors.toList;
-import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.*;
 import static org.onosproject.net.PortNumber.portNumber;
 
 import java.util.List;
@@ -77,4 +77,16 @@ public class PortNumberTest {
         ps.forEach(p -> assertEquals(p, PortNumber.fromString(p.toString())));
     }
 
+    @Test
+    public void exactlyEquals() {
+        assertTrue(portNumber(0).exactlyEquals(portNumber(0)));
+        assertTrue(portNumber(0, "foo").exactlyEquals(portNumber(0, "foo")));
+
+        assertFalse(portNumber(0, "foo").exactlyEquals(portNumber(0, "bar")));
+        assertFalse(portNumber(0, "foo").exactlyEquals(portNumber(0)));
+        assertFalse(portNumber(0, "foo").exactlyEquals(portNumber(1, "foo")));
+
+        assertFalse(portNumber(123).exactlyEquals(portNumber(123, "123")));
+    }
+
 }

@@ -102,50 +102,64 @@ public final class OpticalPortOperator implements ConfigOperator {
     }
 
     // updates a port description whose port type has not changed.
-    private static PortDescription updateDescription(
-            PortNumber port, SparseAnnotations sa, PortDescription descr) {
+    /**
+     * Updates {@link PortDescription} using specified number and annotations.
+     *
+     * @param port {@link PortNumber} to use in updated description
+     * @param sa   annotations to use in updated description
+     * @param descr base {@link PortDescription}
+     * @return updated {@link PortDescription}
+     */
+    private static PortDescription updateDescription(PortNumber port,
+                                                     SparseAnnotations sa,
+                                                     PortDescription descr) {
+
+        // TODO This switch can go away once deprecation is complete.
         switch (descr.type()) {
             case OMS:
                 if (descr instanceof OmsPortDescription) {
-                    // TODO This block can go away once deprecation is complete.
                     OmsPortDescription oms = (OmsPortDescription) descr;
                     return omsPortDescription(port, oms.isEnabled(), oms.minFrequency(),
                                                   oms.maxFrequency(), oms.grid(), sa);
                 }
-                return descr;
+                break;
             case OCH:
                 // We might need to update lambda below with STATIC_LAMBDA.
                 if (descr instanceof OchPortDescription) {
-                    // TODO This block can go away once deprecation is complete.
                     OchPortDescription och = (OchPortDescription) descr;
                     return ochPortDescription(port, och.isEnabled(), och.signalType(),
                             och.isTunable(), och.lambda(), sa);
                 }
-                return descr;
+                break;
             case ODUCLT:
                 if (descr instanceof OduCltPortDescription) {
-                    // TODO This block can go away once deprecation is complete.
                     OduCltPortDescription odu = (OduCltPortDescription) descr;
                     return oduCltPortDescription(port, odu.isEnabled(), odu.signalType(), sa);
                 }
-                return descr;
+                break;
             case PACKET:
             case FIBER:
             case COPPER:
-                // TODO: it should be safe to just return descr. confirm and fix
-                return new DefaultPortDescription(port, descr.isEnabled(), descr.type(),
-                        descr.portSpeed(), sa);
+                break;
             case OTU:
                 if (descr instanceof OtuPortDescription) {
-                    // TODO This block can go away once deprecation is complete.
                     OtuPortDescription otu = (OtuPortDescription) descr;
                     return otuPortDescription(port, otu.isEnabled(), otu.signalType(), sa);
                 }
-                return descr;
+                break;
             default:
                 log.warn("Unsupported optical port type {} - can't update", descr.type());
                 return descr;
         }
+        if (port.exactlyEquals(descr.portNumber()) && sa.equals(descr.annotations())) {
+            // result is no-op
+            return descr;
+        }
+        return new DefaultPortDescription(port,
+                                          descr.isEnabled(),
+                                          descr.type(),
+                                          descr.portSpeed(),
+                                          sa);
     }
 
     /**

@@ -28,62 +28,94 @@ import org.onosproject.net.DeviceId;
 import org.onosproject.net.Port;
 import org.onosproject.net.PortNumber;
 import org.onosproject.net.SparseAnnotations;
-import org.onosproject.net.device.OduCltPortDescription;
-
+import org.onosproject.net.device.PortDescription;
 import com.fasterxml.jackson.databind.ObjectMapper;
 import com.fasterxml.jackson.databind.node.JsonNodeFactory;
 
 import static org.junit.Assert.assertEquals;
+import static org.onosproject.net.optical.device.OduCltPortHelper.oduCltPortDescription;
 
 public class OpticalPortOperatorTest {
     private static final DeviceId DID = DeviceId.deviceId("op-test");
-    private static final String TPNAME = "test-port-100";
-    private static final String SPNAME = "out-port-200";
-    private static final String CFGNAME = "cfg-name";
+    private static final long PORT_NUMBER = 100;
+    private static final String CFG_KEY = "optical";
+
+    private static final String CFG_PORT_NAME = "cfg-name";
+    private static final long CFG_STATIC_LAMBDA = 300L;
 
-    private static final PortNumber NAMED = PortNumber.portNumber(100, TPNAME);
-    private static final PortNumber UNNAMED = PortNumber.portNumber(101);
-    private static final ConnectPoint NCP = new ConnectPoint(DID, UNNAMED);
+    private static final String DESC_PORT_NAME = "test-port-100";
+    private static final PortNumber NAMED = PortNumber.portNumber(PORT_NUMBER, DESC_PORT_NAME);
+    private static final PortNumber UNNAMED = PortNumber.portNumber(PORT_NUMBER);
 
+    private static final String DESC_STATIC_PORT = "out-port-200";
     private static final SparseAnnotations SA = DefaultAnnotations.builder()
-                                                    .set(AnnotationKeys.STATIC_PORT, SPNAME)
+                                                    .set(AnnotationKeys.STATIC_PORT, DESC_STATIC_PORT)
                                                     .build();
 
-    private static final OduCltPortDescription N_DESC = new OduCltPortDescription(
+    private static final PortDescription N_DESC = oduCltPortDescription(
             NAMED, true, CltSignalType.CLT_100GBE, SA);
-    private static final OduCltPortDescription FAULTY = new OduCltPortDescription(
-            null, true, CltSignalType.CLT_100GBE);
+    private static final PortDescription U_DESC = oduCltPortDescription(
+            UNNAMED, true, CltSignalType.CLT_100GBE, SA);
 
     private final ConfigApplyDelegate delegate = new MockCfgDelegate();
     private final ObjectMapper mapper = new ObjectMapper();
 
-    private static final OpticalPortConfig N_OPC = new OpticalPortConfig();
-    private static final OpticalPortConfig UNN_OPC = new OpticalPortConfig();
+    private static final ConnectPoint CP = new ConnectPoint(DID, UNNAMED);
+
+    private static final OpticalPortConfig OPC = new OpticalPortConfig();
 
     @Before
     public void setUp() {
-        N_OPC.init(NCP, TPNAME, JsonNodeFactory.instance.objectNode(), mapper, delegate);
-        UNN_OPC.init(NCP, TPNAME, JsonNodeFactory.instance.objectNode(), mapper, delegate);
-
-        N_OPC.portName(CFGNAME).portNumberName(101L).portType(Port.Type.ODUCLT).staticLambda(300L);
-        UNN_OPC.portType(Port.Type.ODUCLT);
+        OPC.init(CP, CFG_KEY, JsonNodeFactory.instance.objectNode(), mapper, delegate);
     }
 
-    @Test(expected = RuntimeException.class)
-    public void testDescOps() {
-        // port-null desc + opc with port number name
-        OduCltPortDescription res = (OduCltPortDescription) OpticalPortOperator.combine(N_OPC, FAULTY);
-        assertEquals(CFGNAME, res.portNumber().name());
+    @Test
+    public void testConfigPortName() {
+        OPC.portType(Port.Type.ODUCLT)
+            .portNumberName(PORT_NUMBER)
+            .portName(CFG_PORT_NAME);
+
+        PortDescription res;
         // full desc + opc with name
-        assertEquals(TPNAME, N_DESC.portNumber().name());
-        res = (OduCltPortDescription) OpticalPortOperator.combine(N_OPC, N_DESC);
+        res = OpticalPortOperator.combine(OPC, N_DESC);
+        assertEquals("Configured port name expected",
+                     CFG_PORT_NAME, res.portNumber().name());
+        assertEquals(DESC_STATIC_PORT, res.annotations().value(AnnotationKeys.STATIC_PORT));
+
+        res = OpticalPortOperator.combine(OPC, U_DESC);
+        assertEquals("Configured port name expected",
+                     CFG_PORT_NAME, res.portNumber().name());
+        assertEquals(DESC_STATIC_PORT, res.annotations().value(AnnotationKeys.STATIC_PORT));
+    }
+
+    @Test
+    public void testConfigAddStaticLambda() {
+        OPC.portType(Port.Type.ODUCLT)
+            .portNumberName(PORT_NUMBER)
+            .staticLambda(CFG_STATIC_LAMBDA);
+
+        PortDescription res;
+        res = OpticalPortOperator.combine(OPC, N_DESC);
+        assertEquals("Original port name expected",
+                     DESC_PORT_NAME, res.portNumber().name());
+        assertEquals(DESC_STATIC_PORT, res.annotations().value(AnnotationKeys.STATIC_PORT));
         long sl = Long.valueOf(res.annotations().value(AnnotationKeys.STATIC_LAMBDA));
-        assertEquals(CFGNAME, res.portNumber().name());
-        assertEquals(300L, sl);
-        // port-null desc + opc without port number name - throws RE
-        res = (OduCltPortDescription) OpticalPortOperator.combine(UNN_OPC, FAULTY);
+        assertEquals(CFG_STATIC_LAMBDA, sl);
+    }
+
+    @Test
+    public void testEmptyConfig() {
+        OPC.portType(Port.Type.ODUCLT)
+            .portNumberName(PORT_NUMBER);
+
+        PortDescription res;
+        res = OpticalPortOperator.combine(OPC, N_DESC);
+        assertEquals("Configured port name expected",
+                     DESC_PORT_NAME, res.portNumber().name());
+        assertEquals(DESC_STATIC_PORT, res.annotations().value(AnnotationKeys.STATIC_PORT));
     }
 
+
     private class MockCfgDelegate implements ConfigApplyDelegate {
 
         @Override