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
diff --git a/core/api/src/main/java/org/onosproject/net/flowobjective/DefaultFilteringObjective.java b/core/api/src/main/java/org/onosproject/net/flowobjective/DefaultFilteringObjective.java
index e5589b4..7b5924f 100644
--- a/core/api/src/main/java/org/onosproject/net/flowobjective/DefaultFilteringObjective.java
+++ b/core/api/src/main/java/org/onosproject/net/flowobjective/DefaultFilteringObjective.java
@@ -47,39 +47,19 @@
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 @@
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 @@
@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);
}
diff --git a/core/api/src/main/java/org/onosproject/net/flowobjective/DefaultForwardingObjective.java b/core/api/src/main/java/org/onosproject/net/flowobjective/DefaultForwardingObjective.java
index ae47de1..0abf5ab 100644
--- a/core/api/src/main/java/org/onosproject/net/flowobjective/DefaultForwardingObjective.java
+++ b/core/api/src/main/java/org/onosproject/net/flowobjective/DefaultForwardingObjective.java
@@ -45,47 +45,21 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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);
}
}
}
diff --git a/core/api/src/main/java/org/onosproject/net/flowobjective/DefaultNextObjective.java b/core/api/src/main/java/org/onosproject/net/flowobjective/DefaultNextObjective.java
index 245fd57..20e8929 100644
--- a/core/api/src/main/java/org/onosproject/net/flowobjective/DefaultNextObjective.java
+++ b/core/api/src/main/java/org/onosproject/net/flowobjective/DefaultNextObjective.java
@@ -40,25 +40,13 @@
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 @@
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 @@
}
@Override
- public NextObjective.Builder fromApp(ApplicationId appId) {
+ public Builder fromApp(ApplicationId appId) {
this.appId = appId;
return this;
}
@@ -182,46 +173,50 @@
@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);
}
}
}
diff --git a/core/api/src/main/java/org/onosproject/net/flowobjective/NextObjective.java b/core/api/src/main/java/org/onosproject/net/flowobjective/NextObjective.java
index 9a90cff..1350d7a 100644
--- a/core/api/src/main/java/org/onosproject/net/flowobjective/NextObjective.java
+++ b/core/api/src/main/java/org/onosproject/net/flowobjective/NextObjective.java
@@ -112,10 +112,25 @@
*/
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
diff --git a/core/api/src/test/java/org/onosproject/net/flowobjective/ObjectiveTest.java b/core/api/src/test/java/org/onosproject/net/flowobjective/ObjectiveTest.java
index b0919f6..850582b 100644
--- a/core/api/src/test/java/org/onosproject/net/flowobjective/ObjectiveTest.java
+++ b/core/api/src/test/java/org/onosproject/net/flowobjective/ObjectiveTest.java
@@ -25,12 +25,16 @@
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 @@
.withTreatment(treatment)
.withFlag(SPECIFIC)
.fromApp(APP_ID)
+ .withPriority(22)
+ .makeTemporary(5)
.nextStep(33);
}
@@ -90,12 +96,24 @@
*
* @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 @@
*/
@Test
public void testForwardingAdd() {
- checkForwardingBase(baseForwardingBuilder().add());
+ checkForwardingBase(baseForwardingBuilder().add(), ADD, null);
}
/**
@@ -114,7 +132,7 @@
@Test
public void testForwardingAddWithContext() {
ObjectiveContext context = new MockObjectiveContext();
- checkForwardingBase(baseForwardingBuilder().add(context));
+ checkForwardingBase(baseForwardingBuilder().add(context), ADD, context);
}
/**
@@ -123,7 +141,7 @@
*/
@Test
public void testForwardingRemove() {
- checkForwardingBase(baseForwardingBuilder().remove());
+ checkForwardingBase(baseForwardingBuilder().remove(), REMOVE, null);
}
/**
@@ -133,7 +151,7 @@
@Test
public void testForwardingRemoveWithContext() {
ObjectiveContext context = new MockObjectiveContext();
- checkForwardingBase(baseForwardingBuilder().remove(context));
+ checkForwardingBase(baseForwardingBuilder().remove(context), REMOVE, context);
}
// Filtering objectives
@@ -158,7 +176,9 @@
*
* @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 @@
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 @@
*/
@Test
public void testFilteringAdd() {
- checkFilteringBase(baseFilteringBuilder().add());
+ checkFilteringBase(baseFilteringBuilder().add(), ADD, null);
}
/**
@@ -184,7 +211,7 @@
@Test
public void testFilteringAddWithContext() {
ObjectiveContext context = new MockObjectiveContext();
- checkFilteringBase(baseFilteringBuilder().add(context));
+ checkFilteringBase(baseFilteringBuilder().add(context), ADD, context);
}
/**
@@ -193,7 +220,7 @@
*/
@Test
public void testFilteringRemove() {
- checkFilteringBase(baseFilteringBuilder().remove());
+ checkFilteringBase(baseFilteringBuilder().remove(), REMOVE, null);
}
/**
@@ -203,7 +230,7 @@
@Test
public void testFilteringRemoveWithContext() {
ObjectiveContext context = new MockObjectiveContext();
- checkFilteringBase(baseFilteringBuilder().remove(context));
+ checkFilteringBase(baseFilteringBuilder().remove(context), REMOVE, context);
}
// Next objectives
@@ -218,6 +245,8 @@
.addTreatment(treatment)
.withId(12)
.withType(HASHED)
+ .makeTemporary(777)
+ .withPriority(33)
.fromApp(APP_ID);
}
@@ -226,11 +255,22 @@
*
* @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 @@
*/
@Test
public void testNextAdd() {
- checkNextBase(baseNextBuilder().add());
+ checkNextBase(baseNextBuilder().add(), ADD, null);
}
/**
@@ -249,7 +289,7 @@
@Test
public void testNextAddWithContext() {
ObjectiveContext context = new MockObjectiveContext();
- checkNextBase(baseNextBuilder().add(context));
+ checkNextBase(baseNextBuilder().add(context), ADD, context);
}
/**
@@ -258,7 +298,7 @@
*/
@Test
public void testNextRemove() {
- checkNextBase(baseNextBuilder().remove());
+ checkNextBase(baseNextBuilder().remove(), REMOVE, null);
}
/**
@@ -268,6 +308,6 @@
@Test
public void testNextRemoveWithContext() {
ObjectiveContext context = new MockObjectiveContext();
- checkNextBase(baseNextBuilder().remove(context));
+ checkNextBase(baseNextBuilder().remove(context), REMOVE, context);
}
}