Save developers from wasting hours on wrong bucket type

Enforce bucket type to be consistent with group type
Update unit tests accordingly

Change-Id: Ia5f56ca4b5445268ebee09e321c34ed3f3f39827
diff --git a/core/api/src/main/java/org/onosproject/net/group/DefaultGroupDescription.java b/core/api/src/main/java/org/onosproject/net/group/DefaultGroupDescription.java
index e047062..4322482 100644
--- a/core/api/src/main/java/org/onosproject/net/group/DefaultGroupDescription.java
+++ b/core/api/src/main/java/org/onosproject/net/group/DefaultGroupDescription.java
@@ -65,7 +65,8 @@
         if (this.type == GroupDescription.Type.INDIRECT) {
             checkArgument(buckets.buckets().size() == 1, "Indirect group " +
                     "should have only one action bucket");
-       }
+        }
+        checkArgument(buckets.buckets().stream().allMatch(b -> b.type() == type), "Inconsistent bucket type");
         this.appCookie = appCookie;
         this.givenGroupId = groupId;
         this.appId = appId;
diff --git a/core/api/src/test/java/org/onosproject/net/group/DefaultGroupDescriptionTest.java b/core/api/src/test/java/org/onosproject/net/group/DefaultGroupDescriptionTest.java
index 0a0a093..3d2d368 100644
--- a/core/api/src/test/java/org/onosproject/net/group/DefaultGroupDescriptionTest.java
+++ b/core/api/src/test/java/org/onosproject/net/group/DefaultGroupDescriptionTest.java
@@ -16,6 +16,8 @@
 package org.onosproject.net.group;
 
 import org.junit.Test;
+import org.onosproject.core.GroupId;
+import org.onosproject.net.PortNumber;
 import org.onosproject.net.flow.DefaultTrafficTreatment;
 import org.onosproject.net.flow.TrafficTreatment;
 
@@ -32,28 +34,31 @@
  * Default group description unit tests.
  */
 public class DefaultGroupDescriptionTest {
-    byte[] keyData = "abcdefg".getBytes();
+    private final byte[] keyData = "abcdefg".getBytes();
     private final GroupKey key = new DefaultGroupKey(keyData);
-    private final TrafficTreatment treatment =
-            DefaultTrafficTreatment.emptyTreatment();
-    private final GroupBucket bucket =
-            DefaultGroupBucket.createSelectGroupBucket(treatment);
-    private final GroupBuckets groupBuckets =
-            new GroupBuckets(ImmutableList.of(bucket));
+    private final GroupId groupId1 = new GroupId(1);
+    private final TrafficTreatment treatment = DefaultTrafficTreatment.emptyTreatment();
+
+    private final GroupBucket failoverGroupBucket =
+            DefaultGroupBucket.createFailoverGroupBucket(treatment, PortNumber.IN_PORT, groupId1);
+    private final GroupBuckets failoverGroupBuckets = new GroupBuckets(ImmutableList.of(failoverGroupBucket));
+    private final GroupBucket indirectGroupBucket =
+            DefaultGroupBucket.createIndirectGroupBucket(treatment);
+    private final GroupBuckets indirectGroupBuckets = new GroupBuckets(ImmutableList.of(indirectGroupBucket));
+
     private final DefaultGroupDescription d1 =
             new DefaultGroupDescription(did("2"),
                     GroupDescription.Type.FAILOVER,
-                    groupBuckets);
-    private final DefaultGroupDescription sameAsD1 =
-            new DefaultGroupDescription(d1);
+                    failoverGroupBuckets);
+    private final DefaultGroupDescription sameAsD1 = new DefaultGroupDescription(d1);
     private final DefaultGroupDescription d2 =
             new DefaultGroupDescription(did("2"),
                     GroupDescription.Type.INDIRECT,
-                    groupBuckets);
+                    indirectGroupBuckets);
     private final DefaultGroupDescription d3 =
             new DefaultGroupDescription(did("3"),
                     GroupDescription.Type.FAILOVER,
-                    groupBuckets,
+                    failoverGroupBuckets,
                     key,
                     711,
                     APP_ID);
@@ -86,7 +91,7 @@
     public void testConstruction() {
         assertThat(d3.deviceId(), is(did("3")));
         assertThat(d3.type(), is(GroupDescription.Type.FAILOVER));
-        assertThat(d3.buckets(), is(groupBuckets));
+        assertThat(d3.buckets(), is(failoverGroupBuckets));
         assertThat(d3.appId(), is(APP_ID));
         assertThat(d3.givenGroupId(), is(711));
         assertThat(key.key(), is(keyData));
diff --git a/core/api/src/test/java/org/onosproject/net/group/DefaultGroupTest.java b/core/api/src/test/java/org/onosproject/net/group/DefaultGroupTest.java
index e8fb7b3..c84ac87 100644
--- a/core/api/src/test/java/org/onosproject/net/group/DefaultGroupTest.java
+++ b/core/api/src/test/java/org/onosproject/net/group/DefaultGroupTest.java
@@ -18,6 +18,7 @@
 import org.junit.Test;
 import org.onosproject.core.GroupId;
 import org.onosproject.net.NetTestTools;
+import org.onosproject.net.PortNumber;
 import org.onosproject.net.flow.DefaultTrafficTreatment;
 
 import com.google.common.collect.ImmutableList;
@@ -35,24 +36,20 @@
     private final GroupId id2 = new GroupId(7);
     private final GroupId id3 = new GroupId(1234);
 
-    private final GroupBucket bucket =
-            DefaultGroupBucket.createSelectGroupBucket(
-                    DefaultTrafficTreatment.emptyTreatment());
-    private final GroupBuckets groupBuckets =
-            new GroupBuckets(ImmutableList.of(bucket));
-    private final GroupDescription groupDesc1 =
-            new DefaultGroupDescription(did("1"),
-                    GroupDescription.Type.FAILOVER,
-                    groupBuckets);
-    private final GroupDescription groupDesc2 =
-            new DefaultGroupDescription(did("2"),
-                    GroupDescription.Type.FAILOVER,
-                    groupBuckets);
+    private final GroupBucket failoverBucket = DefaultGroupBucket.createFailoverGroupBucket(
+            DefaultTrafficTreatment.emptyTreatment(), PortNumber.IN_PORT, id1);
+    private final GroupBuckets failoverGroupBuckets = new GroupBuckets(ImmutableList.of(failoverBucket));
 
-     private final GroupDescription groupDesc3 =
-            new DefaultGroupDescription(did("3"),
-                    GroupDescription.Type.INDIRECT,
-                    groupBuckets);
+    private final GroupBucket indirectBucket =
+            DefaultGroupBucket.createIndirectGroupBucket(DefaultTrafficTreatment.emptyTreatment());
+    private final GroupBuckets indirectGroupBuckets = new GroupBuckets(ImmutableList.of(indirectBucket));
+
+    private final GroupDescription groupDesc1 =
+            new DefaultGroupDescription(did("1"), GroupDescription.Type.FAILOVER, failoverGroupBuckets);
+    private final GroupDescription groupDesc2 =
+            new DefaultGroupDescription(did("2"), GroupDescription.Type.FAILOVER, failoverGroupBuckets);
+    private final GroupDescription groupDesc3 =
+            new DefaultGroupDescription(did("3"), GroupDescription.Type.INDIRECT, indirectGroupBuckets);
 
     DefaultGroup group1 = new DefaultGroup(id1, groupDesc1);
     DefaultGroup sameAsGroup1 = new DefaultGroup(id1, groupDesc1);
@@ -83,7 +80,7 @@
         assertThat(group1.life(), is(0L));
         assertThat(group1.packets(), is(0L));
         assertThat(group1.referenceCount(), is(0L));
-        assertThat(group1.buckets(), is(groupBuckets));
+        assertThat(group1.buckets(), is(failoverGroupBuckets));
         assertThat(group1.state(), is(Group.GroupState.PENDING_ADD));
         assertThat(group1.failedRetryCount(), is(0));
     }
@@ -94,14 +91,14 @@
     @Test
     public void checkConstructionWithDid() {
         DefaultGroup group = new DefaultGroup(id2, NetTestTools.did("1"),
-                GroupDescription.Type.ALL, groupBuckets);
+                GroupDescription.Type.FAILOVER, failoverGroupBuckets);
         assertThat(group.id(), is(id2));
         assertThat(group.bytes(), is(0L));
         assertThat(group.life(), is(0L));
         assertThat(group.packets(), is(0L));
         assertThat(group.referenceCount(), is(0L));
         assertThat(group.deviceId(), is(NetTestTools.did("1")));
-        assertThat(group.buckets(), is(groupBuckets));
+        assertThat(group.buckets(), is(failoverGroupBuckets));
         assertThat(group.failedRetryCount(), is(0));
     }
 
diff --git a/core/common/src/test/java/org/onosproject/codec/impl/GroupCodecTest.java b/core/common/src/test/java/org/onosproject/codec/impl/GroupCodecTest.java
index 0867213..19c9671 100644
--- a/core/common/src/test/java/org/onosproject/codec/impl/GroupCodecTest.java
+++ b/core/common/src/test/java/org/onosproject/codec/impl/GroupCodecTest.java
@@ -76,23 +76,22 @@
 
     @Test
     public void codecEncodeTest() {
-        GroupBucket bucket1 = DefaultGroupBucket
-                .createSelectGroupBucket(DefaultTrafficTreatment.emptyTreatment());
-        GroupBucket bucket2 = DefaultGroupBucket
-                .createIndirectGroupBucket(DefaultTrafficTreatment.emptyTreatment());
-        GroupBuckets buckets = new GroupBuckets(ImmutableList.of(bucket1, bucket2));
-        GroupBuckets bucketsIndirect = new GroupBuckets(ImmutableList.of(bucket2));
+        GroupBucket bucket1 = DefaultGroupBucket.createAllGroupBucket(DefaultTrafficTreatment.emptyTreatment());
+        GroupBucket bucket2 = DefaultGroupBucket.createAllGroupBucket(DefaultTrafficTreatment.emptyTreatment());
+        GroupBucket bucket3 = DefaultGroupBucket.createIndirectGroupBucket(DefaultTrafficTreatment.emptyTreatment());
+        GroupBuckets allBuckets = new GroupBuckets(ImmutableList.of(bucket1, bucket2));
+        GroupBuckets indirectBuckets = new GroupBuckets(ImmutableList.of(bucket3));
 
         DefaultGroup group = new DefaultGroup(
                 new GroupId(1),
                 NetTestTools.did("d1"),
                 ALL,
-                buckets);
+                allBuckets);
         DefaultGroup group1 = new DefaultGroup(
                 new GroupId(2),
                 NetTestTools.did("d2"),
                 INDIRECT,
-                bucketsIndirect);
+                indirectBuckets);
 
         MockCodecContext context = new MockCodecContext();
         GroupCodec codec = new GroupCodec();
diff --git a/core/common/src/test/java/org/onosproject/utils/ComparatorsTest.java b/core/common/src/test/java/org/onosproject/utils/ComparatorsTest.java
index 6047719..d616e71 100644
--- a/core/common/src/test/java/org/onosproject/utils/ComparatorsTest.java
+++ b/core/common/src/test/java/org/onosproject/utils/ComparatorsTest.java
@@ -129,8 +129,7 @@
     private final ConnectPoint cp =
             new ConnectPoint(DeviceId.deviceId("of:00001"), PortNumber.portNumber(100));
     private final GroupBucket testBucket =
-            DefaultGroupBucket.createSelectGroupBucket(
-                    DefaultTrafficTreatment.emptyTreatment());
+            DefaultGroupBucket.createAllGroupBucket(DefaultTrafficTreatment.emptyTreatment());
     private final GroupBuckets groupBuckets =
             new GroupBuckets(ImmutableList.of(testBucket));
     private final GroupDescription groupDesc1 =
diff --git a/core/store/dist/src/test/java/org/onosproject/store/group/impl/DistributedGroupStoreTest.java b/core/store/dist/src/test/java/org/onosproject/store/group/impl/DistributedGroupStoreTest.java
index 0a44359..087bd80 100644
--- a/core/store/dist/src/test/java/org/onosproject/store/group/impl/DistributedGroupStoreTest.java
+++ b/core/store/dist/src/test/java/org/onosproject/store/group/impl/DistributedGroupStoreTest.java
@@ -61,11 +61,11 @@
 import static org.hamcrest.Matchers.notNullValue;
 import static org.hamcrest.Matchers.nullValue;
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
 import static org.onosproject.net.NetTestTools.APP_ID;
 import static org.onosproject.net.NetTestTools.did;
 import static org.onosproject.net.group.GroupDescription.Type.ALL;
 import static org.onosproject.net.group.GroupDescription.Type.INDIRECT;
-import static org.onosproject.net.group.GroupDescription.Type.SELECT;
 import static org.onosproject.net.group.GroupStore.UpdateType.ADD;
 import static org.onosproject.net.group.GroupStore.UpdateType.SET;
 /**
@@ -73,49 +73,49 @@
  */
 public class DistributedGroupStoreTest {
 
-    DeviceId deviceId1 = did("dev1");
-    DeviceId deviceId2 = did("dev2");
-    GroupId groupId1 = new GroupId(1);
-    GroupId groupId2 = new GroupId(2);
-    GroupId groupId3 = new GroupId(3);
-    GroupKey groupKey1 = new DefaultGroupKey("abc".getBytes());
-    GroupKey groupKey2 = new DefaultGroupKey("def".getBytes());
-    GroupKey groupKey3 = new DefaultGroupKey("ghi".getBytes());
+    private final DeviceId deviceId1 = did("dev1");
+    private final DeviceId deviceId2 = did("dev2");
+    private final GroupId groupId1 = new GroupId(1);
+    private final GroupId groupId2 = new GroupId(2);
+    private final GroupId groupId3 = new GroupId(3);
+    private final GroupKey groupKey1 = new DefaultGroupKey("abc".getBytes());
+    private final GroupKey groupKey2 = new DefaultGroupKey("def".getBytes());
+    private final GroupKey groupKey3 = new DefaultGroupKey("ghi".getBytes());
 
-    TrafficTreatment treatment =
-            DefaultTrafficTreatment.emptyTreatment();
-    GroupBucket selectGroupBucket =
-            DefaultGroupBucket.createSelectGroupBucket(treatment);
-    GroupBucket failoverGroupBucket =
-            DefaultGroupBucket.createFailoverGroupBucket(treatment,
-                    PortNumber.IN_PORT, groupId1);
+    private final TrafficTreatment treatment = DefaultTrafficTreatment.emptyTreatment();
+    private final TrafficTreatment treatment2 = DefaultTrafficTreatment.builder()
+            .setOutput(PortNumber.portNumber(2)).build();
+    private final GroupBucket allGroupBucket = DefaultGroupBucket.createAllGroupBucket(treatment);
+    private final GroupBucket allGroupBucket2 = DefaultGroupBucket.createAllGroupBucket(treatment2);
+    private final GroupBuckets allGroupBuckets = new GroupBuckets(ImmutableList.of(allGroupBucket));
+    private final GroupBucket indirectGroupBucket = DefaultGroupBucket.createIndirectGroupBucket(treatment);
+    private final GroupBuckets indirectGroupBuckets = new GroupBuckets(ImmutableList.of(indirectGroupBucket));
 
-    GroupBuckets buckets = new GroupBuckets(ImmutableList.of(selectGroupBucket));
-    GroupDescription groupDescription1 = new DefaultGroupDescription(
+    private final GroupDescription groupDescription1 = new DefaultGroupDescription(
             deviceId1,
             ALL,
-            buckets,
+            allGroupBuckets,
             groupKey1,
             groupId1.id(),
             APP_ID);
-    GroupDescription groupDescription2 = new DefaultGroupDescription(
+    private final GroupDescription groupDescription2 = new DefaultGroupDescription(
             deviceId2,
             INDIRECT,
-            buckets,
+            indirectGroupBuckets,
             groupKey2,
             groupId2.id(),
             APP_ID);
-    GroupDescription groupDescription3 = new DefaultGroupDescription(
+    private final GroupDescription groupDescription3 = new DefaultGroupDescription(
             deviceId2,
             INDIRECT,
-            buckets,
+            indirectGroupBuckets,
             groupKey3,
             groupId3.id(),
             APP_ID);
 
-    DistributedGroupStore groupStoreImpl;
-    GroupStore groupStore;
-    ConsistentMap auditPendingReqQueue;
+    private DistributedGroupStore groupStoreImpl;
+    private GroupStore groupStore;
+    private ConsistentMap auditPendingReqQueue;
 
     static class MasterOfAll extends MastershipServiceAdapter {
         @Override
@@ -285,8 +285,8 @@
 
         GroupDescription groupDescription3 = new DefaultGroupDescription(
                 deviceId1,
-                SELECT,
-                buckets,
+                ALL,
+                allGroupBuckets,
                 new DefaultGroupKey("aaa".getBytes()),
                 null,
                 APP_ID);
@@ -347,7 +347,7 @@
         GroupOperation opAdd =
                 GroupOperation.createAddGroupOperation(groupId1,
                         INDIRECT,
-                        buckets);
+                        indirectGroupBuckets);
         groupStore.groupOperationFailed(deviceId1, opAdd);
 
         List<GroupEvent> eventsAfterAddFailed = delegate.eventsSeen();
@@ -361,7 +361,7 @@
         GroupOperation opModify =
                 GroupOperation.createModifyGroupOperation(groupId2,
                         INDIRECT,
-                        buckets);
+                        indirectGroupBuckets);
         groupStore.groupOperationFailed(deviceId2, opModify);
         List<GroupEvent> eventsAfterModifyFailed = delegate.eventsSeen();
         assertThat(eventsAfterModifyFailed, hasSize(1));
@@ -400,7 +400,7 @@
 
         // test group exists
         GroupOperation opAdd = GroupOperation
-                .createAddGroupOperation(groupId1, INDIRECT, buckets);
+                .createAddGroupOperation(groupId1, ALL, allGroupBuckets);
         GroupOperation addFailedExists = GroupOperation
                 .createFailedGroupOperation(opAdd, GroupMsgErrorCode.GROUP_EXISTS);
         groupStore.groupOperationFailed(deviceId1, addFailedExists);
@@ -420,7 +420,7 @@
         assertEquals(0, g2.failedRetryCount());
         assertEquals(GroupState.PENDING_ADD, g2.state());
         GroupOperation opAdd1 = GroupOperation
-                .createAddGroupOperation(groupId2, INDIRECT, buckets);
+                .createAddGroupOperation(groupId2, INDIRECT, indirectGroupBuckets);
         GroupOperation addFailedInvalid = GroupOperation
                 .createFailedGroupOperation(opAdd1, GroupMsgErrorCode.INVALID_GROUP);
 
@@ -487,9 +487,7 @@
      */
     @Test
     public void testUpdateGroupDescription() {
-
-        GroupBuckets buckets =
-                new GroupBuckets(ImmutableList.of(failoverGroupBucket, selectGroupBucket));
+        GroupBuckets buckets = new GroupBuckets(ImmutableList.of(allGroupBucket2));
 
         groupStore.deviceInitialAuditCompleted(deviceId1, true);
         groupStore.storeGroupDescription(groupDescription1);
@@ -504,40 +502,31 @@
         assertThat(group1.appCookie(), is(newKey));
         assertThat(group1.buckets().buckets(), hasSize(2));
 
-        short weight = 5;
-        GroupBucket selectGroupBucketWithWeight =
-                DefaultGroupBucket.createSelectGroupBucket(treatment, weight);
-        buckets = new GroupBuckets(ImmutableList.of(failoverGroupBucket,
-                selectGroupBucketWithWeight));
-
+        buckets = new GroupBuckets(ImmutableList.of(allGroupBucket, allGroupBucket2));
         groupStore.updateGroupDescription(deviceId1,
                 newKey,
                 ADD,
                 buckets,
                 newKey);
-
         group1 = groupStore.getGroup(deviceId1, groupId1);
         assertThat(group1.appCookie(), is(newKey));
         assertThat(group1.buckets().buckets(), hasSize(2));
         for (GroupBucket bucket : group1.buckets().buckets()) {
-            if (bucket.type() == SELECT) {
-                assertEquals(weight, bucket.weight());
-            }
+            assertTrue(bucket.treatment().equals(treatment) ||
+                    bucket.treatment().equals(treatment2));
         }
 
-        buckets = new GroupBuckets(ImmutableList.of(selectGroupBucketWithWeight));
-
+        buckets = new GroupBuckets(ImmutableList.of(allGroupBucket2));
         groupStore.updateGroupDescription(deviceId1,
                 newKey,
                 SET,
                 buckets,
                 newKey);
-
         group1 = groupStore.getGroup(deviceId1, groupId1);
         assertThat(group1.appCookie(), is(newKey));
         assertThat(group1.buckets().buckets(), hasSize(1));
         GroupBucket onlyBucket = group1.buckets().buckets().iterator().next();
-        assertEquals(weight, onlyBucket.weight());
+        assertEquals(treatment2, onlyBucket.treatment());
     }
 
     @Test