Tests for FlowRuleStore plus bugfixes

Change-Id: Ib5b0bd9d41fbbcac1cf09684e70446326887caf7

 package org.onlab.onos.net.flow;
 
+import static com.google.common.base.MoreObjects.toStringHelper;
+
 import org.onlab.onos.net.DeviceId;
 
 public class DefaultFlowEntry extends DefaultFlowRule implements FlowEntry {
@@ -63,7 +65,9 @@ public class DefaultFlowEntry extends DefaultFlowRule implements FlowEntry {
     public int hashCode() {
         final int prime = 31;
         int result = prime * this.deviceId().hashCode();
-        result = prime * result + Long.valueOf(this.created).hashCode();
+        result = prime * result + this.priority;
+        result = prime * result + this.selector().hashCode();
+        result = prime * result + this.treatment().hashCode();
         return result;
     }
 
@@ -77,7 +81,10 @@ public class DefaultFlowEntry extends DefaultFlowRule implements FlowEntry {
     public boolean equals(Object obj) {
         if (obj instanceof DefaultFlowEntry) {
             DefaultFlowEntry that = (DefaultFlowEntry) obj;
-            if (!this.id.equals(that.id())) {
+            if (!this.deviceId().equals(that.deviceId())) {
+                return false;
+            }
+            if (!(this.priority == that.priority)) {
                 return false;
             }
             return super.equals(obj);
@@ -85,4 +92,15 @@ public class DefaultFlowEntry extends DefaultFlowRule implements FlowEntry {
         return false;
     }
 
+    @Override
+    public String toString() {
+        return toStringHelper(this)
+                .add("id", id)
+                .add("deviceId", deviceId())
+                .add("priority", priority)
+                .add("selector", selector())
+                .add("treatment", treatment())
+                .toString();
+    }
+
 }

@@ -17,6 +17,7 @@ public class DefaultFlowRule implements FlowRule {
 
     @Override
     public int priority() {
+        // is this supposed to be 0?
         return 0;
     }
 
@@ -63,9 +64,10 @@ public class DefaultFlowRule implements FlowRule {
             if (!this.selector().equals(that.selector())) {
                 return false;
             }
-        }
             return true;
         }
+        return false;
+    }
 
 
 }

@@ -45,10 +45,10 @@ implements FlowRuleService, FlowRuleProviderRegistry {
     private final SimpleFlowRuleStore store = new SimpleFlowRuleStore();
 
     @Reference(cardinality = ReferenceCardinality.MANDATORY_UNARY)
-    private EventDeliveryService eventDispatcher;
+    protected EventDeliveryService eventDispatcher;
 
     @Reference(cardinality = ReferenceCardinality.MANDATORY_UNARY)
-    private DeviceService deviceService;
+    protected DeviceService deviceService;
 
     @Activate
     public void activate() {

@@ -53,17 +53,20 @@ public class SimpleFlowRuleStore {
     FlowRuleEvent addOrUpdateFlowRule(FlowRule rule) {
         DeviceId did = rule.deviceId();
 
+        FlowEntry entry = new DefaultFlowEntry(
+                did,
+                rule.selector(),
+                rule.treatment(),
+                rule.priority());
+
         // check if this new rule is an update to an existing entry
         for (FlowEntry fe : flowEntries.get(did)) {
-            if (rule.equals(fe)) {
+            if (entry.equals(fe)) {
                 // TODO update the stats on this flowEntry?
                 return null;
             }
         }
-
-        FlowEntry newfe = new DefaultFlowEntry(did,
-                rule.selector(), rule.treatment(), rule.priority());
-        flowEntries.put(did, newfe);
+        flowEntries.put(did, entry);
         return new FlowRuleEvent(RULE_ADDED, rule);
     }
 
@@ -73,8 +76,11 @@ public class SimpleFlowRuleStore {
      * @return flow_removed event, or null if nothing removed
      */
     FlowRuleEvent removeFlowRule(FlowRule rule) {
+
+        FlowEntry rem = new DefaultFlowEntry(rule.deviceId(),
+                rule.selector(), rule.treatment(), rule.priority());
         synchronized (this) {
-            if (flowEntries.remove(rule.deviceId(), rule)) {
+            if (flowEntries.remove(rem.deviceId(), rem)) {
                 return new FlowRuleEvent(RULE_REMOVED, rule);
             } else {
                 return null;

+package org.onlab.onos.net.trivial.flow.impl;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+import org.onlab.onos.event.impl.TestEventDispatcher;
+import org.onlab.onos.net.DefaultDevice;
+import org.onlab.onos.net.Device;
+import org.onlab.onos.net.Device.Type;
+import org.onlab.onos.net.DeviceId;
+import org.onlab.onos.net.MastershipRole;
+import org.onlab.onos.net.Port;
+import org.onlab.onos.net.PortNumber;
+import org.onlab.onos.net.device.DeviceListener;
+import org.onlab.onos.net.device.DeviceService;
+import org.onlab.onos.net.flow.DefaultFlowEntry;
+import org.onlab.onos.net.flow.DefaultFlowRule;
+import org.onlab.onos.net.flow.FlowEntry;
+import org.onlab.onos.net.flow.FlowRule;
+import org.onlab.onos.net.flow.FlowRuleEvent;
+import org.onlab.onos.net.flow.FlowRuleListener;
+import org.onlab.onos.net.flow.FlowRuleProvider;
+import org.onlab.onos.net.flow.FlowRuleProviderRegistry;
+import org.onlab.onos.net.flow.FlowRuleProviderService;
+import org.onlab.onos.net.flow.FlowRuleService;
+import org.onlab.onos.net.flow.TrafficSelector;
+import org.onlab.onos.net.flow.TrafficTreatment;
+import org.onlab.onos.net.flow.criteria.Criterion;
+import org.onlab.onos.net.flow.instructions.Instruction;
+import org.onlab.onos.net.provider.AbstractProvider;
+import org.onlab.onos.net.provider.ProviderId;
+
+import com.google.common.collect.Lists;
+import com.google.common.collect.Sets;
+
+import static org.onlab.onos.net.flow.FlowRuleEvent.Type.*;
+
+/**
+ * Test codifying the flow rule service & flow rule provider service contracts.
+ */
+public class SimpleFlowRuleManagerTest {
+
+    private static final ProviderId PID = new ProviderId("foo");
+    private static final DeviceId DID = DeviceId.deviceId("of:001");
+    private static final Device DEV = new DefaultDevice(
+            PID, DID, Type.SWITCH, "", "", "", "");
+
+    private SimpleFlowRuleManager mgr;
+
+    protected FlowRuleService service;
+    protected FlowRuleProviderRegistry registry;
+    protected FlowRuleProviderService providerSerivce;
+    protected TestProvider provider;
+    protected TestListener listener = new TestListener();
+
+    @Before
+    public void setUp() {
+        mgr = new SimpleFlowRuleManager();
+        mgr.eventDispatcher = new TestEventDispatcher();
+        mgr.deviceService = new TestDeviceService();
+        service = mgr;
+        registry = mgr;
+
+        mgr.activate();
+        mgr.addListener(listener);
+        provider = new TestProvider(PID);
+        providerSerivce = registry.register(provider);
+        assertTrue("provider should be registered",
+                registry.getProviders().contains(provider.id()));
+    }
+
+    @After
+    public void tearDown() {
+        registry.unregister(provider);
+        assertFalse("provider should not be registered",
+                    registry.getProviders().contains(provider.id()));
+        service.removeListener(listener);
+        mgr.deactivate();
+        mgr.eventDispatcher = null;
+        mgr.deviceService = null;
+    }
+
+    private FlowRule flowRule(int tsval, int trval) {
+        TestSelector ts = new TestSelector(tsval);
+        TestTreatment tr = new TestTreatment(trval);
+        return new DefaultFlowRule(DID, ts, tr);
+    }
+
+    private void addFlowRule(int hval) {
+        FlowRule rule = flowRule(hval, hval);
+        providerSerivce.flowAdded(rule);
+        assertNotNull("rule should be found", service.getFlowEntries(DID));
+    }
+
+    private void validateEvents(FlowRuleEvent.Type ... events) {
+        if (events == null) {
+            assertTrue("events generated", listener.events.isEmpty());
+        }
+
+        int i = 0;
+        for (FlowRuleEvent e : listener.events) {
+            assertTrue("unexpected event", e.type().equals(events[i]));
+            i++;
+        }
+
+        assertEquals("mispredicted number of events",
+                events.length, listener.events.size());
+
+        listener.events.clear();
+    }
+
+    private int flowCount() {
+        return Sets.newHashSet(service.getFlowEntries(DID)).size();
+    }
+    @Test
+    public void getFlowEntries() {
+        assertTrue("store should be empty",
+                Sets.newHashSet(service.getFlowEntries(DID)).isEmpty());
+        addFlowRule(1);
+        addFlowRule(2);
+        assertEquals("2 rules should exist", 2, flowCount());
+        validateEvents(RULE_ADDED, RULE_ADDED);
+
+        addFlowRule(1);
+        assertEquals("should still be 2 rules", 2, flowCount());
+        validateEvents();
+    }
+
+    @Test
+    public void applyFlowRules() {
+        TestSelector ts = new TestSelector(1);
+        FlowRule r1 = flowRule(1, 1);
+        FlowRule r2 = flowRule(1, 2);
+        FlowRule r3 = flowRule(1, 3);
+
+        //current FlowRules always return 0. FlowEntries inherit the value
+        FlowEntry e1 = new DefaultFlowEntry(DID, ts, r1.treatment(), 0);
+        FlowEntry e2 = new DefaultFlowEntry(DID, ts, r2.treatment(), 0);
+        FlowEntry e3 = new DefaultFlowEntry(DID, ts, r3.treatment(), 0);
+        List<FlowEntry> fel = Lists.newArrayList(e1, e2, e3);
+
+        assertTrue("store should be empty",
+                Sets.newHashSet(service.getFlowEntries(DID)).isEmpty());
+        List<FlowEntry> ret = mgr.applyFlowRules(r1, r2, r3);
+        assertEquals("3 rules should exist", 3, flowCount());
+        assertTrue("3 entries should result", fel.containsAll(ret));
+    }
+
+    @Test
+    public void removeFlowRules() {
+        addFlowRule(1);
+        addFlowRule(2);
+        addFlowRule(3);
+        assertEquals("3 rules should exist", 3, flowCount());
+        validateEvents(RULE_ADDED, RULE_ADDED, RULE_ADDED);
+
+        FlowRule rem1 = flowRule(1, 1);
+        FlowRule rem2 = flowRule(2, 2);
+        mgr.removeFlowRules(rem1, rem2);
+        //removing from north, so no events generated
+        validateEvents();
+        assertEquals("1 rule should exist", 1, flowCount());
+
+        mgr.removeFlowRules(rem1);
+        assertEquals("1 rule should still exist", 1, flowCount());
+    }
+
+    @Test
+    public void flowRemoved() {
+        addFlowRule(1);
+        addFlowRule(2);
+        FlowRule rem1 = flowRule(1, 1);
+        providerSerivce.flowRemoved(rem1);
+        validateEvents(RULE_ADDED, RULE_ADDED, RULE_REMOVED);
+
+        providerSerivce.flowRemoved(rem1);
+        validateEvents();
+    }
+
+    private static class TestListener implements FlowRuleListener {
+        final List<FlowRuleEvent> events = new ArrayList<>();
+
+        @Override
+        public void event(FlowRuleEvent event) {
+            events.add(event);
+        }
+    }
+
+    private static class TestDeviceService implements DeviceService {
+
+        @Override
+        public int getDeviceCount() {
+            return 0;
+        }
+
+        @Override
+        public Iterable<Device> getDevices() {
+            return null;
+        }
+
+        @Override
+        public Device getDevice(DeviceId deviceId) {
+            return DEV;
+        }
+
+        @Override
+        public MastershipRole getRole(DeviceId deviceId) {
+            return null;
+        }
+
+        @Override
+        public List<Port> getPorts(DeviceId deviceId) {
+            return null;
+        }
+
+        @Override
+        public Port getPort(DeviceId deviceId, PortNumber portNumber) {
+            return null;
+        }
+
+        @Override
+        public boolean isAvailable(DeviceId deviceId) {
+            return false;
+        }
+
+        @Override
+        public void addListener(DeviceListener listener) {
+        }
+
+        @Override
+        public void removeListener(DeviceListener listener) {
+        }
+
+    }
+
+    private class TestProvider extends AbstractProvider implements FlowRuleProvider {
+
+        protected TestProvider(ProviderId id) {
+            super(PID);
+        }
+
+        @Override
+        public void applyFlowRule(FlowRule... flowRules) {
+        }
+
+        @Override
+        public void removeFlowRule(FlowRule... flowRules) {
+        }
+
+        @Override
+        public Iterable<FlowEntry> getFlowMetrics(DeviceId deviceId) {
+            return null;
+        }
+
+    }
+
+    private class TestSelector implements TrafficSelector {
+
+        //for controlling hashcode uniqueness;
+        private int testval;
+
+        public TestSelector(int val) {
+            testval = val;
+        }
+
+        @Override
+        public List<Criterion> criteria() {
+            return null;
+        }
+
+        @Override
+        public int hashCode() {
+            return testval;
+        }
+
+        @Override
+        public boolean equals(Object o) {
+            if (o instanceof TestSelector) {
+                return this.testval == ((TestSelector) o).testval;
+            }
+            return false;
+        }
+    }
+
+    private class TestTreatment implements TrafficTreatment {
+
+        //for controlling hashcode uniqueness;
+        private int testval;
+
+        public TestTreatment(int val) {
+            testval = val;
+        }
+
+        @Override
+        public List<Instruction> instructions() {
+            return null;
+        }
+
+        @Override
+        public int hashCode() {
+            return testval;
+        }
+
+        @Override
+        public boolean equals(Object o) {
+            if (o instanceof TestTreatment) {
+                return this.testval == ((TestTreatment) o).testval;
+            }
+            return false;
+        }
+
+    }
+
+}

@@ -18,6 +18,7 @@ public final class IpAddress {
 
     //maximum CIDR value
     public static final int MAX_INET_MASK = 32;
+    //no mask (no network), e.g. a simple address
     public static final int DEFAULT_MASK = 0;
 
     /**
@@ -112,7 +113,7 @@ public final class IpAddress {
         final String [] parts = address.split("\\/");
         if (parts.length > 2) {
             throw new IllegalArgumentException("Malformed IP address string; "
-                    + "Addres must take form \"x.x.x.x\" or \"x.x.x.x/y\"");
+                    + "Address must take form \"x.x.x.x\" or \"x.x.x.x/y\"");
         }
 
         int mask = DEFAULT_MASK;
@@ -128,7 +129,7 @@ public final class IpAddress {
         final String [] net = parts[0].split("\\.");
         if (net.length != INET_LEN) {
             throw new IllegalArgumentException("Malformed IP address string; "
-                    + "Addres must have four decimal values separated by dots (.)");
+                    + "Address must have four decimal values separated by dots (.)");
         }
         final byte [] bytes = new byte[INET_LEN];
         for (int i = 0; i < INET_LEN; i++) {