Coverage and SONAR improvements for Objectives classes

- cleaned up constructors to take a builder rather
  than a long list of parameters
- improved coverage of unit tests
- added missing APIs to builder interfaces

Change-Id: I4c4eac302d41f785d401f21e9935bc659ca5f892

@@ -47,39 +47,19 @@ public final class DefaultFilteringObjective implements FilteringObjective {
     private final Operation op;
     private final Optional<ObjectiveContext> context;
 
-    private DefaultFilteringObjective(Type type, boolean permanent, int timeout,
-                                      ApplicationId appId, int priority, Criterion key,
-                                      List<Criterion> conditions, Operation op) {
-        this.key = key;
-        this.type = type;
-        this.permanent = permanent;
-        this.timeout = timeout;
-        this.appId = appId;
-        this.priority = priority;
-        this.conditions = conditions;
-        this.op = op;
-        this.context = Optional.empty();
+    private DefaultFilteringObjective(Builder builder) {
+        this.key = builder.key;
+        this.type = builder.type;
+        this.permanent = builder.permanent;
+        this.timeout = builder.timeout;
+        this.appId = builder.appId;
+        this.priority = builder.priority;
+        this.conditions = builder.conditions;
+        this.op = builder.op;
+        this.context = Optional.ofNullable(builder.context);
 
         this.id = Objects.hash(type, key, conditions, permanent,
-                               timeout, appId, priority);
-    }
-
-    public DefaultFilteringObjective(Type type, boolean permanent, int timeout,
-                                     ApplicationId appId, int priority, Criterion key,
-                                     List<Criterion> conditions,
-                                     ObjectiveContext context, Operation op) {
-        this.key = key;
-        this.type = type;
-        this.permanent = permanent;
-        this.timeout = timeout;
-        this.appId = appId;
-        this.priority = priority;
-        this.conditions = conditions;
-        this.op = op;
-        this.context = Optional.ofNullable(context);
-
-        this.id = Objects.hash(type, key, conditions, permanent,
-                               timeout, appId, priority);
+                timeout, appId, priority);
     }
 
     @Override
@@ -152,6 +132,9 @@ public final class DefaultFilteringObjective implements FilteringObjective {
         private ApplicationId appId;
         private int priority = DEFAULT_PRIORITY;
         private Criterion key = Criteria.dummy();
+        private List<Criterion> conditions;
+        private Operation op;
+        private ObjectiveContext context;
 
         @Override
         public Builder withKey(Criterion key) {
@@ -204,54 +187,50 @@ public final class DefaultFilteringObjective implements FilteringObjective {
 
         @Override
         public FilteringObjective add() {
-            List<Criterion> conditions = listBuilder.build();
+            conditions = listBuilder.build();
+            op = Operation.ADD;
             checkNotNull(type, "Must have a type.");
             checkArgument(!conditions.isEmpty(), "Must have at least one condition.");
             checkNotNull(appId, "Must supply an application id");
 
-            return new DefaultFilteringObjective(type, permanent, timeout,
-                                                appId, priority, key, conditions,
-                                                Operation.ADD);
+            return new DefaultFilteringObjective(this);
 
         }
 
         @Override
         public FilteringObjective remove() {
-            List<Criterion> conditions = listBuilder.build();
+            conditions = listBuilder.build();
             checkNotNull(type, "Must have a type.");
             checkArgument(!conditions.isEmpty(), "Must have at least one condition.");
             checkNotNull(appId, "Must supply an application id");
+            op = Operation.REMOVE;
 
-
-            return new DefaultFilteringObjective(type, permanent, timeout,
-                                                 appId, priority, key, conditions,
-                                                 Operation.REMOVE);
+            return new DefaultFilteringObjective(this);
 
         }
 
         @Override
         public FilteringObjective add(ObjectiveContext context) {
-            List<Criterion> conditions = listBuilder.build();
+            conditions = listBuilder.build();
             checkNotNull(type, "Must have a type.");
             checkArgument(!conditions.isEmpty(), "Must have at least one condition.");
             checkNotNull(appId, "Must supply an application id");
+            op = Operation.ADD;
+            this.context = context;
 
-            return new DefaultFilteringObjective(type, permanent, timeout,
-                                                 appId, priority, key, conditions,
-                                                 context, Operation.ADD);
+            return new DefaultFilteringObjective(this);
         }
 
         @Override
         public FilteringObjective remove(ObjectiveContext context) {
-            List<Criterion> conditions = listBuilder.build();
+            conditions = listBuilder.build();
             checkNotNull(type, "Must have a type.");
             checkArgument(!conditions.isEmpty(), "Must have at least one condition.");
             checkNotNull(appId, "Must supply an application id");
+            op = Operation.REMOVE;
+            this.context = context;
 
-
-            return new DefaultFilteringObjective(type, permanent, timeout,
-                                                 appId, priority, key, conditions,
-                                                 context, Operation.REMOVE);
+            return new DefaultFilteringObjective(this);
         }
 
 

@@ -45,47 +45,21 @@ public final class DefaultForwardingObjective implements ForwardingObjective {
 
     private final int id;
 
-    private DefaultForwardingObjective(TrafficSelector selector,
-                                      Flag flag, boolean permanent,
-                                      int timeout, ApplicationId appId,
-                                      int priority, Integer nextId,
-                                      TrafficTreatment treatment, Operation op) {
-        this.selector = selector;
-        this.flag = flag;
-        this.permanent = permanent;
-        this.timeout = timeout;
-        this.appId = appId;
-        this.priority = priority;
-        this.nextId = nextId;
-        this.treatment = treatment;
-        this.op = op;
-        this.context = Optional.empty();
+    private DefaultForwardingObjective(Builder builder) {
+        this.selector = builder.selector;
+        this.flag = builder.flag;
+        this.permanent = builder.permanent;
+        this.timeout = builder.timeout;
+        this.appId = builder.appId;
+        this.priority = builder.priority;
+        this.nextId = builder.nextId;
+        this.treatment = builder.treatment;
+        this.op = builder.op;
+        this.context = Optional.ofNullable(builder.context);
 
         this.id = Objects.hash(selector, flag, permanent,
-                               timeout, appId, priority, nextId,
-                               treatment, op);
-    }
-
-    private DefaultForwardingObjective(TrafficSelector selector,
-                                       Flag flag, boolean permanent,
-                                       int timeout, ApplicationId appId,
-                                       int priority, Integer nextId,
-                                       TrafficTreatment treatment,
-                                       ObjectiveContext context, Operation op) {
-        this.selector = selector;
-        this.flag = flag;
-        this.permanent = permanent;
-        this.timeout = timeout;
-        this.appId = appId;
-        this.priority = priority;
-        this.nextId = nextId;
-        this.treatment = treatment;
-        this.op = op;
-        this.context = Optional.ofNullable(context);
-
-        this.id = Objects.hash(selector, flag, permanent,
-                               timeout, appId, priority, nextId,
-                               treatment, op);
+                timeout, appId, priority, nextId,
+                treatment, op);
     }
 
 
@@ -164,6 +138,8 @@ public final class DefaultForwardingObjective implements ForwardingObjective {
         private ApplicationId appId;
         private Integer nextId;
         private TrafficTreatment treatment;
+        private Operation op;
+        private ObjectiveContext context;
 
         @Override
         public Builder withSelector(TrafficSelector selector) {
@@ -221,9 +197,8 @@ public final class DefaultForwardingObjective implements ForwardingObjective {
             checkArgument(nextId != null || treatment != null, "Must supply at " +
                     "least a treatment and/or a nextId");
             checkNotNull(appId, "Must supply an application id");
-            return new DefaultForwardingObjective(selector, flag, permanent,
-                                                  timeout, appId, priority,
-                                                  nextId, treatment, Operation.ADD);
+            op = Operation.ADD;
+            return new DefaultForwardingObjective(this);
         }
 
         @Override
@@ -233,9 +208,8 @@ public final class DefaultForwardingObjective implements ForwardingObjective {
             checkArgument(nextId != null || treatment != null, "Must supply at " +
                     "least a treatment and/or a nextId");
             checkNotNull(appId, "Must supply an application id");
-            return new DefaultForwardingObjective(selector, flag, permanent,
-                                                   timeout, appId, priority,
-                                                   nextId, treatment, Operation.REMOVE);
+            op = Operation.REMOVE;
+            return new DefaultForwardingObjective(this);
         }
 
         @Override
@@ -245,10 +219,10 @@ public final class DefaultForwardingObjective implements ForwardingObjective {
             checkArgument(nextId != null || treatment != null, "Must supply at " +
                     "least a treatment and/or a nextId");
             checkNotNull(appId, "Must supply an application id");
-            return new DefaultForwardingObjective(selector, flag, permanent,
-                                                  timeout, appId, priority,
-                                                  nextId, treatment,
-                                                  context, Operation.ADD);
+            op = Operation.ADD;
+            this.context = context;
+
+            return new DefaultForwardingObjective(this);
         }
 
         @Override
@@ -258,10 +232,10 @@ public final class DefaultForwardingObjective implements ForwardingObjective {
             checkArgument(nextId != null || treatment != null, "Must supply at " +
                     "least a treatment and/or a nextId");
             checkNotNull(appId, "Must supply an application id");
-            return new DefaultForwardingObjective(selector, flag, permanent,
-                                                  timeout, appId, priority,
-                                                  nextId, treatment,
-                                                  context, Operation.REMOVE);
+            op = Operation.REMOVE;
+            this.context = context;
+
+            return new DefaultForwardingObjective(this);
         }
     }
 }

@@ -40,25 +40,13 @@ public final class DefaultNextObjective implements NextObjective {
     private final Operation op;
     private final Optional<ObjectiveContext> context;
 
-    private DefaultNextObjective(Integer id, List<TrafficTreatment> treatments,
-                                ApplicationId appId, Type type, Operation op) {
-        this.treatments = treatments;
-        this.appId = appId;
-        this.type = type;
-        this.id = id;
-        this.op = op;
-        this.context = Optional.empty();
-    }
-
-    private DefaultNextObjective(Integer id, List<TrafficTreatment> treatments,
-                                 ApplicationId appId, ObjectiveContext context,
-                                 Type type, Operation op) {
-        this.treatments = treatments;
-        this.appId = appId;
-        this.type = type;
-        this.id = id;
-        this.op = op;
-        this.context = Optional.ofNullable(context);
+    private DefaultNextObjective(Builder builder) {
+        this.treatments = builder.treatments;
+        this.appId = builder.appId;
+        this.type = builder.type;
+        this.id = builder.id;
+        this.op = builder.op;
+        this.context = Optional.ofNullable(builder.context);
     }
 
     @Override
@@ -120,12 +108,15 @@ public final class DefaultNextObjective implements NextObjective {
         private ApplicationId appId;
         private Type type;
         private Integer id;
+        private List<TrafficTreatment> treatments;
+        private Operation op;
+        private ObjectiveContext context;
 
         private final ImmutableList.Builder<TrafficTreatment> listBuilder
                 = ImmutableList.builder();
 
         @Override
-        public NextObjective.Builder withId(int nextId) {
+        public Builder withId(int nextId) {
             this.id = nextId;
             return this;
         }
@@ -164,7 +155,7 @@ public final class DefaultNextObjective implements NextObjective {
         }
 
         @Override
-        public NextObjective.Builder fromApp(ApplicationId appId) {
+        public Builder fromApp(ApplicationId appId) {
             this.appId = appId;
             return this;
         }
@@ -182,46 +173,50 @@ public final class DefaultNextObjective implements NextObjective {
 
         @Override
         public NextObjective add() {
-            List<TrafficTreatment> treatments = listBuilder.build();
+            treatments = listBuilder.build();
+            op = Operation.ADD;
             checkNotNull(appId, "Must supply an application id");
             checkNotNull(id, "id cannot be null");
             checkNotNull(type, "The type cannot be null");
             checkArgument(!treatments.isEmpty(), "Must have at least one treatment");
 
-            return new DefaultNextObjective(id, treatments, appId, type, Operation.ADD);
+            return new DefaultNextObjective(this);
         }
 
         @Override
         public NextObjective remove() {
-            List<TrafficTreatment> treatments = listBuilder.build();
+            treatments = listBuilder.build();
+            op = Operation.REMOVE;
             checkNotNull(appId, "Must supply an application id");
             checkNotNull(id, "id cannot be null");
             checkNotNull(type, "The type cannot be null");
 
-            return new DefaultNextObjective(id, treatments, appId, type, Operation.REMOVE);
+            return new DefaultNextObjective(this);
         }
 
         @Override
         public NextObjective add(ObjectiveContext context) {
-            List<TrafficTreatment> treatments = listBuilder.build();
+            treatments = listBuilder.build();
+            op = Operation.ADD;
+            this.context = context;
             checkNotNull(appId, "Must supply an application id");
             checkNotNull(id, "id cannot be null");
             checkNotNull(type, "The type cannot be null");
             checkArgument(!treatments.isEmpty(), "Must have at least one treatment");
 
-            return new DefaultNextObjective(id, treatments, appId,
-                                            context, type, Operation.ADD);
+            return new DefaultNextObjective(this);
         }
 
         @Override
         public NextObjective remove(ObjectiveContext context) {
-            List<TrafficTreatment> treatments = listBuilder.build();
+            treatments = listBuilder.build();
+            op = Operation.REMOVE;
+            this.context = context;
             checkNotNull(appId, "Must supply an application id");
             checkNotNull(id, "id cannot be null");
             checkNotNull(type, "The type cannot be null");
 
-            return new DefaultNextObjective(id, treatments, appId,
-                                            context, type, Operation.REMOVE);
+            return new DefaultNextObjective(this);
         }
     }
 }

@@ -112,10 +112,25 @@ public interface NextObjective extends Objective {
          */
         Builder addTreatment(TrafficTreatment treatment);
 
+        /**
+         * Specifies the application which applied the filter.
+         *
+         * @param appId an application id
+         * @return an objective builder
+         */
         @Override
         Builder fromApp(ApplicationId appId);
 
         /**
+         * Sets the priority for this objective.
+         *
+         * @param priority an integer
+         * @return an objective builder
+         */
+        @Override
+        Builder withPriority(int priority);
+
+        /**
          * Builds the next objective that will be added.
          *
          * @return a next objective

@@ -25,12 +25,16 @@ import org.onosproject.net.flow.criteria.Criterion;
 
 import static org.hamcrest.CoreMatchers.hasItem;
 import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.CoreMatchers.nullValue;
 import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.not;
 import static org.onlab.junit.ImmutableClassChecker.assertThatClassIsImmutable;
 import static org.onosproject.net.NetTestTools.APP_ID;
 import static org.onosproject.net.flowobjective.FilteringObjective.Type.DENY;
 import static org.onosproject.net.flowobjective.ForwardingObjective.Flag.SPECIFIC;
 import static org.onosproject.net.flowobjective.NextObjective.Type.HASHED;
+import static org.onosproject.net.flowobjective.Objective.Operation.ADD;
+import static org.onosproject.net.flowobjective.Objective.Operation.REMOVE;
 
 /**
  * Unit tests for forwarding objective class.
@@ -82,6 +86,8 @@ public class ObjectiveTest {
                 .withTreatment(treatment)
                 .withFlag(SPECIFIC)
                 .fromApp(APP_ID)
+                .withPriority(22)
+                .makeTemporary(5)
                 .nextStep(33);
     }
 
@@ -90,12 +96,24 @@ public class ObjectiveTest {
      *
      * @param objective forwarding objective to check
      */
-    private void checkForwardingBase(ForwardingObjective objective) {
+    private void checkForwardingBase(ForwardingObjective objective,
+                                     Objective.Operation op,
+                                     ObjectiveContext expectedContext) {
+        assertThat(objective.permanent(), is(false));
+        assertThat(objective.timeout(), is(5));
         assertThat(objective.selector(), is(selector));
         assertThat(objective.treatment(), is(treatment));
         assertThat(objective.flag(), is(SPECIFIC));
         assertThat(objective.appId(), is(APP_ID));
         assertThat(objective.nextId(), is(33));
+        assertThat(objective.id(), is(not(0)));
+        assertThat(objective.priority(), is(22));
+        assertThat(objective.op(), is(op));
+        if (objective.context().isPresent()) {
+            assertThat(objective.context().get(), is(expectedContext));
+        } else {
+            assertThat(expectedContext, nullValue());
+        }
     }
 
     /**
@@ -104,7 +122,7 @@ public class ObjectiveTest {
      */
     @Test
     public void testForwardingAdd() {
-        checkForwardingBase(baseForwardingBuilder().add());
+        checkForwardingBase(baseForwardingBuilder().add(), ADD, null);
     }
 
     /**
@@ -114,7 +132,7 @@ public class ObjectiveTest {
     @Test
     public void testForwardingAddWithContext() {
         ObjectiveContext context = new MockObjectiveContext();
-        checkForwardingBase(baseForwardingBuilder().add(context));
+        checkForwardingBase(baseForwardingBuilder().add(context), ADD, context);
     }
 
     /**
@@ -123,7 +141,7 @@ public class ObjectiveTest {
      */
     @Test
     public void testForwardingRemove() {
-        checkForwardingBase(baseForwardingBuilder().remove());
+        checkForwardingBase(baseForwardingBuilder().remove(), REMOVE, null);
     }
 
     /**
@@ -133,7 +151,7 @@ public class ObjectiveTest {
     @Test
     public void testForwardingRemoveWithContext() {
         ObjectiveContext context = new MockObjectiveContext();
-        checkForwardingBase(baseForwardingBuilder().remove(context));
+        checkForwardingBase(baseForwardingBuilder().remove(context), REMOVE, context);
     }
 
     // Filtering objectives
@@ -158,7 +176,9 @@ public class ObjectiveTest {
      *
      * @param objective filtering objective to check
      */
-    private void checkFilteringBase(FilteringObjective objective) {
+    private void checkFilteringBase(FilteringObjective objective,
+                                    Objective.Operation op,
+                                    ObjectiveContext expectedContext) {
         assertThat(objective.key(), is(key));
         assertThat(objective.conditions(), hasItem(criterion));
         assertThat(objective.permanent(), is(false));
@@ -166,6 +186,13 @@ public class ObjectiveTest {
         assertThat(objective.priority(), is(5));
         assertThat(objective.appId(), is(APP_ID));
         assertThat(objective.type(), is(DENY));
+        assertThat(objective.id(), is(not(0)));
+        assertThat(objective.op(), is(op));
+        if (objective.context().isPresent()) {
+            assertThat(objective.context().get(), is(expectedContext));
+        } else {
+            assertThat(expectedContext, nullValue());
+        }
     }
 
     /**
@@ -174,7 +201,7 @@ public class ObjectiveTest {
      */
     @Test
     public void testFilteringAdd() {
-        checkFilteringBase(baseFilteringBuilder().add());
+        checkFilteringBase(baseFilteringBuilder().add(), ADD, null);
     }
 
     /**
@@ -184,7 +211,7 @@ public class ObjectiveTest {
     @Test
     public void testFilteringAddWithContext() {
         ObjectiveContext context = new MockObjectiveContext();
-        checkFilteringBase(baseFilteringBuilder().add(context));
+        checkFilteringBase(baseFilteringBuilder().add(context), ADD, context);
     }
 
     /**
@@ -193,7 +220,7 @@ public class ObjectiveTest {
      */
     @Test
     public void testFilteringRemove() {
-        checkFilteringBase(baseFilteringBuilder().remove());
+        checkFilteringBase(baseFilteringBuilder().remove(), REMOVE, null);
     }
 
     /**
@@ -203,7 +230,7 @@ public class ObjectiveTest {
     @Test
     public void testFilteringRemoveWithContext() {
         ObjectiveContext context = new MockObjectiveContext();
-        checkFilteringBase(baseFilteringBuilder().remove(context));
+        checkFilteringBase(baseFilteringBuilder().remove(context), REMOVE, context);
     }
 
     // Next objectives
@@ -218,6 +245,8 @@ public class ObjectiveTest {
                 .addTreatment(treatment)
                 .withId(12)
                 .withType(HASHED)
+                .makeTemporary(777)
+                .withPriority(33)
                 .fromApp(APP_ID);
     }
 
@@ -226,11 +255,22 @@ public class ObjectiveTest {
      *
      * @param objective next objective to check
      */
-    private void checkNextBase(NextObjective objective) {
+    private void checkNextBase(NextObjective objective,
+                               Objective.Operation op,
+                               ObjectiveContext expectedContext) {
         assertThat(objective.id(), is(12));
         assertThat(objective.appId(), is(APP_ID));
         assertThat(objective.type(), is(HASHED));
         assertThat(objective.next(), hasItem(treatment));
+        assertThat(objective.permanent(), is(false));
+        assertThat(objective.timeout(), is(0));
+        assertThat(objective.priority(), is(0));
+        assertThat(objective.op(), is(op));
+        if (objective.context().isPresent()) {
+            assertThat(objective.context().get(), is(expectedContext));
+        } else {
+            assertThat(expectedContext, nullValue());
+        }
     }
 
     /**
@@ -239,7 +279,7 @@ public class ObjectiveTest {
      */
     @Test
     public void testNextAdd() {
-        checkNextBase(baseNextBuilder().add());
+        checkNextBase(baseNextBuilder().add(), ADD, null);
     }
 
     /**
@@ -249,7 +289,7 @@ public class ObjectiveTest {
     @Test
     public void testNextAddWithContext() {
         ObjectiveContext context = new MockObjectiveContext();
-        checkNextBase(baseNextBuilder().add(context));
+        checkNextBase(baseNextBuilder().add(context), ADD, context);
     }
 
     /**
@@ -258,7 +298,7 @@ public class ObjectiveTest {
      */
     @Test
     public void testNextRemove() {
-        checkNextBase(baseNextBuilder().remove());
+        checkNextBase(baseNextBuilder().remove(), REMOVE, null);
     }
 
     /**
@@ -268,6 +308,6 @@ public class ObjectiveTest {
     @Test
     public void testNextRemoveWithContext() {
         ObjectiveContext context = new MockObjectiveContext();
-        checkNextBase(baseNextBuilder().remove(context));
+        checkNextBase(baseNextBuilder().remove(context), REMOVE, context);
     }
 }