Improve network config validation errors to show which fields are invalid.
Previously, uploading invalid config results in a generic error message
which makes it difficult to figure out what is wrong with the config
Change-Id: I307d2fc0669679b067389c722556eef3aae098b9
diff --git a/core/api/src/main/java/org/onosproject/net/config/Config.java b/core/api/src/main/java/org/onosproject/net/config/Config.java
index b36b0e2..4aa3d8d 100644
--- a/core/api/src/main/java/org/onosproject/net/config/Config.java
+++ b/core/api/src/main/java/org/onosproject/net/config/Config.java
@@ -21,7 +21,6 @@
import com.fasterxml.jackson.databind.node.ObjectNode;
import com.google.common.annotations.Beta;
import com.google.common.collect.ImmutableSet;
-import com.google.common.collect.Iterators;
import com.google.common.collect.Lists;
import org.onlab.packet.IpAddress;
import org.onlab.packet.IpPrefix;
@@ -103,7 +102,7 @@
* Default implementation returns true.
* Subclasses are expected to override this with their own validation.
* Implementations are free to throw a RuntimeException if data is invalid.
- * * </p>
+ * </p>
*
* @return true if the data is valid; false otherwise
* @throws RuntimeException if configuration is invalid or completely foobar
@@ -114,11 +113,13 @@
// isString(path)
// isBoolean(path)
// isNumber(path, [min, max])
+ // isIntegralNumber(path, [min, max])
// isDecimal(path, [min, max])
// isMacAddress(path)
// isIpAddress(path)
// isIpPrefix(path)
// isConnectPoint(path)
+ // isTpPort(path)
return true;
}
@@ -153,8 +154,9 @@
/**
* Applies any configuration changes made via this configuration.
- *
+ * <p>
* Not effective for detached configs.
+ * </p>
*/
public void apply() {
checkState(delegate != null, "Cannot apply detached config");
@@ -414,7 +416,12 @@
*/
protected boolean hasOnlyFields(ObjectNode node, String... allowedFields) {
Set<String> fields = ImmutableSet.copyOf(allowedFields);
- return !Iterators.any(node.fieldNames(), f -> !fields.contains(f));
+ node.fieldNames().forEachRemaining(f -> {
+ if (!fields.contains(f)) {
+ throw new InvalidFieldException(f, "Field is not allowed");
+ }
+ });
+ return true;
}
/**
@@ -437,7 +444,12 @@
*/
protected boolean hasFields(ObjectNode node, String... mandatoryFields) {
Set<String> fields = ImmutableSet.copyOf(mandatoryFields);
- return Iterators.all(fields.iterator(), f -> !node.path(f).isMissingNode());
+ fields.forEach(f -> {
+ if (node.path(f).isMissingNode()) {
+ throw new InvalidFieldException(f, "Mandatory field is not present");
+ }
+ });
+ return true;
}
/**
@@ -446,12 +458,10 @@
* @param field JSON field name
* @param presence specifies if field is optional or mandatory
* @return true if valid; false otherwise
- * @throws IllegalArgumentException if field is present, but not valid MAC
+ * @throws InvalidFieldException if the field is present but not valid
*/
protected boolean isMacAddress(String field, FieldPresence presence) {
- JsonNode node = object.path(field);
- return isValid(node, presence, node.isTextual() &&
- MacAddress.valueOf(node.asText()) != null);
+ return isMacAddress(object, field, presence);
}
/**
@@ -462,12 +472,13 @@
* @param field JSON field name
* @param presence specifies if field is optional or mandatory
* @return true if valid; false otherwise
- * @throws IllegalArgumentException if field is present, but not valid MAC
+ * @throws InvalidFieldException if the field is present but not valid
*/
protected boolean isMacAddress(ObjectNode objectNode, String field, FieldPresence presence) {
- JsonNode node = objectNode.path(field);
- return isValid(node, presence, node.isTextual() &&
- MacAddress.valueOf(node.asText()) != null);
+ return isValid(objectNode, field, presence, n -> {
+ MacAddress.valueOf(n.asText());
+ return true;
+ });
}
/**
@@ -476,7 +487,7 @@
* @param field JSON field name
* @param presence specifies if field is optional or mandatory
* @return true if valid; false otherwise
- * @throws IllegalArgumentException if field is present, but not valid IP
+ * @throws InvalidFieldException if the field is present but not valid
*/
protected boolean isIpAddress(String field, FieldPresence presence) {
return isIpAddress(object, field, presence);
@@ -490,12 +501,13 @@
* @param field JSON field name
* @param presence specifies if field is optional or mandatory
* @return true if valid; false otherwise
- * @throws IllegalArgumentException if field is present, but not valid IP
+ * @throws InvalidFieldException if the field is present but not valid
*/
protected boolean isIpAddress(ObjectNode objectNode, String field, FieldPresence presence) {
- JsonNode node = objectNode.path(field);
- return isValid(node, presence, node.isTextual() &&
- IpAddress.valueOf(node.asText()) != null);
+ return isValid(objectNode, field, presence, n -> {
+ IpAddress.valueOf(n.asText());
+ return true;
+ });
}
/**
@@ -504,8 +516,7 @@
* @param field JSON field name
* @param presence specifies if field is optional or mandatory
* @return true if valid; false otherwise
- * @throws IllegalArgumentException if field is present, but not valid IP
- * prefix
+ * @throws InvalidFieldException if the field is present but not valid
*/
protected boolean isIpPrefix(String field, FieldPresence presence) {
return isIpPrefix(object, field, presence);
@@ -519,13 +530,13 @@
* @param field JSON field name
* @param presence specifies if field is optional or mandatory
* @return true if valid; false otherwise
- * @throws IllegalArgumentException if field is present, but not valid IP
- * prefix
+ * @throws InvalidFieldException if the field is present but not valid
*/
protected boolean isIpPrefix(ObjectNode objectNode, String field, FieldPresence presence) {
- JsonNode node = objectNode.path(field);
- return isValid(node, presence, node.isTextual() &&
- IpPrefix.valueOf(node.asText()) != null);
+ return isValid(objectNode, field, presence, n -> {
+ IpPrefix.valueOf(n.asText());
+ return true;
+ });
}
/**
@@ -534,7 +545,7 @@
* @param field JSON field name
* @param presence specifies if field is optional or mandatory
* @return true if valid; false otherwise
- * @throws IllegalArgumentException if field is present, but not valid value
+ * @throws InvalidFieldException if the field is present but not valid
*/
protected boolean isTpPort(String field, FieldPresence presence) {
return isTpPort(object, field, presence);
@@ -548,12 +559,13 @@
* @param field JSON field name
* @param presence specifies if field is optional or mandatory
* @return true if valid; false otherwise
- * @throws IllegalArgumentException if field is present, but not valid value
+ * @throws InvalidFieldException if the field is present but not valid
*/
protected boolean isTpPort(ObjectNode objectNode, String field, FieldPresence presence) {
- JsonNode node = objectNode.path(field);
- return isValid(node, presence, node.isNumber() &&
- TpPort.tpPort(node.asInt()) != null);
+ return isValid(objectNode, field, presence, n -> {
+ TpPort.tpPort(n.asInt());
+ return true;
+ });
}
/**
@@ -562,8 +574,7 @@
* @param field JSON field name
* @param presence specifies if field is optional or mandatory
* @return true if valid; false otherwise
- * @throws IllegalArgumentException if field is present, but not valid
- * connect point string representation
+ * @throws InvalidFieldException if the field is present but not valid
*/
protected boolean isConnectPoint(String field, FieldPresence presence) {
return isConnectPoint(object, field, presence);
@@ -577,13 +588,13 @@
* @param field JSON field name
* @param presence specifies if field is optional or mandatory
* @return true if valid; false otherwise
- * @throws IllegalArgumentException if field is present, but not valid
- * connect point string representation
+ * @throws InvalidFieldException if the field is present but not valid
*/
protected boolean isConnectPoint(ObjectNode objectNode, String field, FieldPresence presence) {
- JsonNode node = objectNode.path(field);
- return isValid(node, presence, node.isTextual() &&
- ConnectPoint.deviceConnectPoint(node.asText()) != null);
+ return isValid(objectNode, field, presence, n -> {
+ ConnectPoint.deviceConnectPoint(n.asText());
+ return true;
+ });
}
/**
@@ -593,7 +604,7 @@
* @param presence specifies if field is optional or mandatory
* @param pattern optional regex pattern
* @return true if valid; false otherwise
- * @throws IllegalArgumentException if field is present, but not valid string
+ * @throws InvalidFieldException if the field is present but not valid
*/
protected boolean isString(String field, FieldPresence presence, String... pattern) {
return isString(object, field, presence, pattern);
@@ -608,13 +619,17 @@
* @param presence specifies if field is optional or mandatory
* @param pattern optional regex pattern
* @return true if valid; false otherwise
- * @throws IllegalArgumentException if field is present, but not valid string
+ * @throws InvalidFieldException if the field is present but not valid
*/
protected boolean isString(ObjectNode objectNode, String field,
FieldPresence presence, String... pattern) {
- JsonNode node = objectNode.path(field);
- return isValid(node, presence, node.isTextual() &&
- (pattern.length > 0 && node.asText().matches(pattern[0]) || pattern.length < 1));
+ return isValid(objectNode, field, presence, (node) -> {
+ if (!(node.isTextual() &&
+ (pattern.length > 0 && node.asText().matches(pattern[0]) || pattern.length < 1))) {
+ fail("Invalid string value");
+ }
+ return true;
+ });
}
/**
@@ -624,28 +639,34 @@
* @param presence specifies if field is optional or mandatory
* @param minMax optional min/max values
* @return true if valid; false otherwise
- * @throws IllegalArgumentException if field is present, but not valid
+ * @throws InvalidFieldException if the field is present but not valid
*/
protected boolean isNumber(String field, FieldPresence presence, long... minMax) {
- JsonNode node = object.path(field);
- return isValid(node, presence, node.isNumber() &&
- (minMax.length > 0 && minMax[0] <= node.asLong() || minMax.length < 1) &&
- (minMax.length > 1 && minMax[1] > node.asLong() || minMax.length < 2));
+ return isNumber(object, field, presence, minMax);
}
+
/**
- * Indicates whether the specified field holds a valid number.
+ * Indicates whether the specified field of a particular node holds a
+ * valid number.
*
+ * @param objectNode JSON object
* @param field JSON field name
* @param presence specifies if field is optional or mandatory
* @param minMax optional min/max values
* @return true if valid; false otherwise
- * @throws IllegalArgumentException if field is present, but not valid
+ * @throws InvalidFieldException if the field is present but not valid
*/
- protected boolean isNumber(String field, FieldPresence presence, double... minMax) {
- JsonNode node = object.path(field);
- return isValid(node, presence, node.isNumber() &&
- (minMax.length > 0 && minMax[0] <= node.asDouble() || minMax.length < 1) &&
- (minMax.length > 1 && minMax[1] > node.asDouble() || minMax.length < 2));
+ protected boolean isNumber(ObjectNode objectNode, String field,
+ FieldPresence presence, long... minMax) {
+ return isValid(objectNode, field, presence, n -> {
+ long number = (n.isNumber()) ? n.asLong() : Long.parseLong(n.asText());
+ if (minMax.length > 1) {
+ verifyRange(number, minMax[0], minMax[1]);
+ } else if (minMax.length > 0) {
+ verifyRange(number, minMax[0]);
+ }
+ return true;
+ });
}
/**
@@ -655,7 +676,7 @@
* @param presence specifies if field is optional or mandatory
* @param minMax optional min/max values
* @return true if valid; false otherwise
- * @throws IllegalArgumentException if field is present, but not valid
+ * @throws InvalidFieldException if the field is present but not valid
*/
protected boolean isIntegralNumber(String field, FieldPresence presence, long... minMax) {
return isIntegralNumber(object, field, presence, minMax);
@@ -670,16 +691,18 @@
* @param presence specifies if field is optional or mandatory
* @param minMax optional min/max values
* @return true if valid; false otherwise
- * @throws IllegalArgumentException if field is present, but not valid
+ * @throws InvalidFieldException if the field is present but not valid
*/
protected boolean isIntegralNumber(ObjectNode objectNode, String field,
FieldPresence presence, long... minMax) {
- JsonNode node = objectNode.path(field);
-
- return isValid(node, presence, n -> {
- long number = (node.isIntegralNumber()) ? n.asLong() : Long.parseLong(n.asText());
- return (minMax.length > 0 && minMax[0] <= number || minMax.length < 1) &&
- (minMax.length > 1 && minMax[1] > number || minMax.length < 2);
+ return isValid(objectNode, field, presence, n -> {
+ long number = (n.isIntegralNumber()) ? n.asLong() : Long.parseLong(n.asText());
+ if (minMax.length > 1) {
+ verifyRange(number, minMax[0], minMax[1]);
+ } else if (minMax.length > 0) {
+ verifyRange(number, minMax[0]);
+ }
+ return true;
});
}
@@ -690,13 +713,34 @@
* @param presence specifies if field is optional or mandatory
* @param minMax optional min/max values
* @return true if valid; false otherwise
- * @throws IllegalArgumentException if field is present, but not valid
+ * @throws InvalidFieldException if the field is present but not valid
*/
protected boolean isDecimal(String field, FieldPresence presence, double... minMax) {
- JsonNode node = object.path(field);
- return isValid(node, presence, (node.isDouble() || node.isFloat()) &&
- (minMax.length > 0 && minMax[0] <= node.asDouble() || minMax.length < 1) &&
- (minMax.length > 1 && minMax[1] > node.asDouble() || minMax.length < 2));
+ return isDecimal(object, field, presence, minMax);
+ }
+
+ /**
+ * Indicates whether the specified field of a particular node holds a valid
+ * decimal number.
+ *
+ * @param objectNode JSON node
+ * @param field JSON field name
+ * @param presence specifies if field is optional or mandatory
+ * @param minMax optional min/max values
+ * @return true if valid; false otherwise
+ * @throws InvalidFieldException if the field is present but not valid
+ */
+ protected boolean isDecimal(ObjectNode objectNode, String field,
+ FieldPresence presence, double... minMax) {
+ return isValid(objectNode, field, presence, n -> {
+ double number = (n.isDouble()) ? n.asDouble() : Double.parseDouble(n.asText());
+ if (minMax.length > 1) {
+ verifyRange(number, minMax[0], minMax[1]);
+ } else if (minMax.length > 0) {
+ verifyRange(number, minMax[0]);
+ }
+ return true;
+ });
}
/**
@@ -705,6 +749,7 @@
* @param field JSON field name
* @param presence specifies if field is optional or mandatory
* @return true if valid; false otherwise
+ * @throws InvalidFieldException if the field is present but not valid
*/
protected boolean isBoolean(String field, FieldPresence presence) {
return isBoolean(object, field, presence);
@@ -718,11 +763,15 @@
* @param field JSON field name
* @param presence specifies if field is optional or mandatory
* @return true if valid; false otherwise
+ * @throws InvalidFieldException if the field is present but not valid
*/
protected boolean isBoolean(ObjectNode objectNode, String field, FieldPresence presence) {
- JsonNode node = objectNode.path(field);
- return isValid(node, presence, node.isBoolean() ||
- (node.isTextual() && isBooleanString(node.asText())));
+ return isValid(objectNode, field, presence, n -> {
+ if (!(n.isBoolean() || (n.isTextual() && isBooleanString(n.asText())))) {
+ fail("Field is not a boolean value");
+ }
+ return true;
+ });
}
/**
@@ -737,39 +786,56 @@
}
/**
- * Indicates whether the node is present and of correct value or not
- * mandatory and absent.
+ * Indicates whether a field in the node is present and of correct value or
+ * not mandatory and absent.
*
- * @param node JSON node
- * @param presence specifies if field is optional or mandatory
- * @param correctValue true if the value is correct
- * @return true if the field is as expected
- */
- private boolean isValid(JsonNode node, FieldPresence presence, boolean correctValue) {
- return isValid(node, presence, n -> correctValue);
- }
-
- /**
- * Indicates whether the node is present and of correct value or not
- * mandatory and absent.
- *
- * @param node JSON node
+ * @param objectNode JSON object node containing field to validate
+ * @param field name of field to validate
* @param presence specified if field is optional or mandatory
* @param validationFunction function which can be used to verify if the
* node has the correct value
* @return true if the field is as expected
+ * @throws InvalidFieldException if the field is present but not valid
*/
- private boolean isValid(JsonNode node, FieldPresence presence,
+ private boolean isValid(ObjectNode objectNode, String field, FieldPresence presence,
Function<JsonNode, Boolean> validationFunction) {
+ JsonNode node = objectNode.path(field);
boolean isMandatory = presence == FieldPresence.MANDATORY;
- if (isMandatory && validationFunction.apply(node)) {
- return true;
+ if (isMandatory && node.isMissingNode()) {
+ throw new InvalidFieldException(field, "Mandatory field not present");
}
if (!isMandatory && (node.isNull() || node.isMissingNode())) {
return true;
}
- return validationFunction.apply(node);
+ try {
+ if (validationFunction.apply(node)) {
+ return true;
+ } else {
+ throw new InvalidFieldException(field, "Validation error");
+ }
+ } catch (IllegalArgumentException e) {
+ throw new InvalidFieldException(field, e);
+ }
}
+
+ private static void fail(String message) {
+ throw new IllegalArgumentException(message);
+ }
+
+ private static <N extends Comparable> void verifyRange(N num, N min) {
+ if (num.compareTo(min) < 0) {
+ fail("Field must be greater than " + min);
+ }
+ }
+
+ private static <N extends Comparable> void verifyRange(N num, N min, N max) {
+ verifyRange(num, min);
+
+ if (num.compareTo(max) > 0) {
+ fail("Field must be less than " + max);
+ }
+ }
+
}
diff --git a/core/api/src/main/java/org/onosproject/net/config/InvalidConfigException.java b/core/api/src/main/java/org/onosproject/net/config/InvalidConfigException.java
new file mode 100644
index 0000000..c46ef65
--- /dev/null
+++ b/core/api/src/main/java/org/onosproject/net/config/InvalidConfigException.java
@@ -0,0 +1,89 @@
+/*
+ * Copyright 2016 Open Networking Laboratory
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.onosproject.net.config;
+
+/**
+ * Indicates an invalid configuration was supplied by the user.
+ */
+public class InvalidConfigException extends RuntimeException {
+
+ private final String subjectKey;
+ private final String subject;
+ private final String configKey;
+
+ /**
+ * Creates a new invalid config exception about a specific config.
+ *
+ * @param subjectKey config's subject key
+ * @param subject config's subject
+ * @param configKey config's config key
+ */
+ public InvalidConfigException(String subjectKey, String subject, String configKey) {
+ this(subjectKey, subject, configKey, null);
+ }
+
+ /**
+ * Creates a new invalid config exception about a specific config with an
+ * exception regarding the cause of the invalidity.
+ *
+ * @param subjectKey config's subject key
+ * @param subject config's subject
+ * @param configKey config's config key
+ * @param cause cause of the invalidity
+ */
+ public InvalidConfigException(String subjectKey, String subject, String configKey, Throwable cause) {
+ super(message(subjectKey, subject, configKey, cause), cause);
+ this.subjectKey = subjectKey;
+ this.subject = subject;
+ this.configKey = configKey;
+ }
+
+ /**
+ * Returns the subject key of the config.
+ *
+ * @return subject key
+ */
+ public String subjectKey() {
+ return subjectKey;
+ }
+
+ /**
+ * Returns the string representation of the subject of the config.
+ *
+ * @return subject
+ */
+ public String subject() {
+ return subject;
+ }
+
+ /**
+ * Returns the config key of the config.
+ *
+ * @return config key
+ */
+ public String configKey() {
+ return configKey;
+ }
+
+ private static String message(String subjectKey, String subject, String configKey, Throwable cause) {
+ String error = "Error parsing config " + subjectKey + "/" + subject + "/" + configKey;
+ if (cause != null) {
+ error = error + ": " + cause.getMessage();
+ }
+ return error;
+ }
+}
diff --git a/core/api/src/main/java/org/onosproject/net/config/InvalidFieldException.java b/core/api/src/main/java/org/onosproject/net/config/InvalidFieldException.java
new file mode 100644
index 0000000..f542dae
--- /dev/null
+++ b/core/api/src/main/java/org/onosproject/net/config/InvalidFieldException.java
@@ -0,0 +1,73 @@
+/*
+ * Copyright 2016-present Open Networking Laboratory
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.onosproject.net.config;
+
+/**
+ * Indicates a field of a configuration was invalid.
+ */
+public class InvalidFieldException extends RuntimeException {
+
+ private final String field;
+ private final String reason;
+
+ /**
+ * Creates a new invalid field exception about a given field.
+ *
+ * @param field field name
+ * @param reason reason the field is invalid
+ */
+ public InvalidFieldException(String field, String reason) {
+ super(message(field, reason));
+ this.field = field;
+ this.reason = reason;
+ }
+
+ /**
+ * Creates a new invalid field exception about a given field.
+ *
+ * @param field field name
+ * @param cause throwable that occurred while trying to validate field
+ */
+ public InvalidFieldException(String field, Throwable cause) {
+ super(message(field, cause.getMessage()));
+ this.field = field;
+ this.reason = cause.getMessage();
+ }
+
+ /**
+ * Returns the field name.
+ *
+ * @return field name
+ */
+ public String field() {
+ return field;
+ }
+
+ /**
+ * Returns the reason the field failed to validate.
+ *
+ * @return reason
+ */
+ public String reason() {
+ return reason;
+ }
+
+ private static String message(String field, String reason) {
+ return "Field \"" + field + "\" is invalid: " + reason;
+ }
+
+}
diff --git a/core/api/src/test/java/org/onosproject/net/config/ConfigTest.java b/core/api/src/test/java/org/onosproject/net/config/ConfigTest.java
index 84dbca7..956befe 100644
--- a/core/api/src/test/java/org/onosproject/net/config/ConfigTest.java
+++ b/core/api/src/test/java/org/onosproject/net/config/ConfigTest.java
@@ -21,7 +21,6 @@
import org.junit.Before;
import org.junit.Test;
-import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;
import static org.onosproject.net.config.Config.FieldPresence.MANDATORY;
import static org.onosproject.net.config.Config.FieldPresence.OPTIONAL;
@@ -37,8 +36,15 @@
private static final String TEXT = "text";
private static final String LONG = "long";
private static final String DOUBLE = "double";
+ private static final String BOOLEAN = "boolean";
private static final String MAC = "mac";
+ private static final String BAD_MAC = "badMac";
private static final String IP = "ip";
+ private static final String BAD_IP = "badIp";
+ private static final String PREFIX = "prefix";
+ private static final String BAD_PREFIX = "badPrefix";
+ private static final String CONNECT_POINT = "connectPoint";
+ private static final String BAD_CONNECT_POINT = "badConnectPoint";
private static final String TP_PORT = "tpPort";
private static final String BAD_TP_PORT = "badTpPort";
@@ -52,7 +58,12 @@
public void setUp() {
json = new ObjectMapper().createObjectNode()
.put(TEXT, "foo").put(LONG, 5).put(DOUBLE, 0.5)
- .put(MAC, "ab:cd:ef:ca:fe:ed").put(IP, "12.34.56.78")
+ .put(BOOLEAN, "true")
+ .put(MAC, "ab:cd:ef:ca:fe:ed").put(BAD_MAC, "ab:cd:ef:ca:fe.ed")
+ .put(IP, "12.34.56.78").put(BAD_IP, "12.34-56.78")
+ .put(PREFIX, "12.34.56.78/18").put(BAD_PREFIX, "12.34.56.78-18")
+ .put(CONNECT_POINT, "of:0000000000000001/1")
+ .put(BAD_CONNECT_POINT, "of:0000000000000001-1")
.put(TP_PORT, 65535).put(BAD_TP_PORT, 65536);
cfg = new TestConfig();
cfg.init(SUBJECT, KEY, json, mapper, delegate);
@@ -60,16 +71,20 @@
@Test
public void hasOnlyFields() {
- assertTrue("has unexpected fields", cfg.hasOnlyFields(TEXT, LONG, DOUBLE, MAC, IP,
- TP_PORT, BAD_TP_PORT));
- assertFalse("did not detect unexpected fields", cfg.hasOnlyFields(TEXT, LONG, DOUBLE, MAC));
- assertTrue("is not proper text", cfg.isString(TEXT, MANDATORY));
+ assertTrue("has unexpected fields",
+ cfg.hasOnlyFields(TEXT, LONG, DOUBLE, BOOLEAN, MAC, BAD_MAC,
+ IP, BAD_IP, PREFIX, BAD_PREFIX,
+ CONNECT_POINT, BAD_CONNECT_POINT, TP_PORT, BAD_TP_PORT));
+ assertTrue("did not detect unexpected fields",
+ expectInvalidField(() -> cfg.hasOnlyFields(TEXT, LONG, DOUBLE, MAC)));
}
@Test
public void hasFields() {
- assertTrue("does not have mandatory field", cfg.hasFields(TEXT, LONG, DOUBLE, MAC));
- assertFalse("did not detect missing field", cfg.hasFields("none"));
+ assertTrue("does not have mandatory field",
+ cfg.hasFields(TEXT, LONG, DOUBLE, MAC));
+ assertTrue("did not detect missing field",
+ expectInvalidField(() -> cfg.hasFields("none")));
}
@Test
@@ -79,7 +94,10 @@
assertTrue("is not proper text", cfg.isString(TEXT, OPTIONAL, "^f.*"));
assertTrue("is not proper text", cfg.isString(TEXT, OPTIONAL));
assertTrue("is not proper text", cfg.isString("none", OPTIONAL));
- assertFalse("did not detect missing field", cfg.isString("none", MANDATORY));
+ assertTrue("did not detect missing field",
+ expectInvalidField(() -> cfg.isString("none", MANDATORY)));
+ assertTrue("did not detect bad text",
+ expectInvalidField(() -> cfg.isString(TEXT, OPTIONAL, "^b.*")));
}
@Test
@@ -88,12 +106,41 @@
assertTrue("is not proper number", cfg.isNumber(LONG, MANDATORY, 0));
assertTrue("is not proper number", cfg.isNumber(LONG, MANDATORY, 0, 10));
assertTrue("is not proper number", cfg.isNumber(LONG, MANDATORY, 5, 6));
- assertFalse("is not in range", cfg.isNumber(LONG, MANDATORY, 6, 10));
- assertFalse("is not in range", cfg.isNumber(LONG, MANDATORY, 4, 5));
+ assertTrue("is not in range",
+ expectInvalidField(() -> cfg.isNumber(LONG, MANDATORY, 6, 10)));
+ assertTrue("is not in range", cfg.isNumber(LONG, MANDATORY, 4, 5));
assertTrue("is not proper number", cfg.isNumber(LONG, OPTIONAL, 0, 10));
assertTrue("is not proper number", cfg.isNumber(LONG, OPTIONAL));
assertTrue("is not proper number", cfg.isNumber("none", OPTIONAL));
- assertFalse("did not detect missing field", cfg.isNumber("none", MANDATORY));
+ assertTrue("did not detect missing field",
+ expectInvalidField(() -> cfg.isNumber("none", MANDATORY)));
+ assertTrue("is not proper number",
+ expectInvalidField(() -> cfg.isNumber(TEXT, MANDATORY)));
+
+ assertTrue("is not proper number", cfg.isNumber(DOUBLE, MANDATORY, 0, 1));
+ assertTrue("is not in range",
+ expectInvalidField(() -> cfg.isNumber(DOUBLE, MANDATORY, 1, 2)));
+ }
+
+ @Test
+ public void isIntegralNumber() {
+ assertTrue("is not proper number", cfg.isIntegralNumber(LONG, MANDATORY));
+ assertTrue("is not proper number", cfg.isIntegralNumber(LONG, MANDATORY, 0));
+ assertTrue("is not proper number", cfg.isIntegralNumber(LONG, MANDATORY, 0, 10));
+ assertTrue("is not proper number", cfg.isIntegralNumber(LONG, MANDATORY, 5, 6));
+ assertTrue("is not in range",
+ expectInvalidField(() -> cfg.isIntegralNumber(LONG, MANDATORY, 6, 10)));
+ assertTrue("is not in range", cfg.isIntegralNumber(LONG, MANDATORY, 4, 5));
+ assertTrue("is not proper number", cfg.isIntegralNumber(LONG, OPTIONAL, 0, 10));
+ assertTrue("is not proper number", cfg.isIntegralNumber(LONG, OPTIONAL));
+ assertTrue("is not proper number", cfg.isIntegralNumber("none", OPTIONAL));
+ assertTrue("did not detect missing field",
+ expectInvalidField(() -> cfg.isIntegralNumber("none", MANDATORY)));
+ assertTrue("is not proper number",
+ expectInvalidField(() -> cfg.isIntegralNumber(TEXT, MANDATORY)));
+
+ assertTrue("is not in range",
+ expectInvalidField(() -> cfg.isIntegralNumber(DOUBLE, MANDATORY, 0, 10)));
}
@Test
@@ -102,12 +149,26 @@
assertTrue("is not proper decimal", cfg.isDecimal(DOUBLE, MANDATORY, 0.0));
assertTrue("is not proper decimal", cfg.isDecimal(DOUBLE, MANDATORY, 0.0, 1.0));
assertTrue("is not proper decimal", cfg.isDecimal(DOUBLE, MANDATORY, 0.5, 0.6));
- assertFalse("is not in range", cfg.isDecimal(DOUBLE, MANDATORY, 0.6, 1.0));
- assertFalse("is not in range", cfg.isDecimal(DOUBLE, MANDATORY, 0.4, 0.5));
+ assertTrue("is not proper decimal", cfg.isDecimal(DOUBLE, MANDATORY, 0.4, 0.5));
+ assertTrue("is not in range",
+ expectInvalidField(() -> cfg.isDecimal(DOUBLE, MANDATORY, 0.6, 1.0)));
+ assertTrue("is not in range",
+ expectInvalidField(() -> cfg.isDecimal(DOUBLE, MANDATORY, 0.3, 0.4)));
assertTrue("is not proper decimal", cfg.isDecimal(DOUBLE, OPTIONAL, 0.0, 1.0));
assertTrue("is not proper decimal", cfg.isDecimal(DOUBLE, OPTIONAL));
assertTrue("is not proper decimal", cfg.isDecimal("none", OPTIONAL));
- assertFalse("did not detect missing field", cfg.isDecimal("none", MANDATORY));
+ assertTrue("did not detect missing field",
+ expectInvalidField(() -> cfg.isDecimal("none", MANDATORY)));
+ }
+
+ @Test
+ public void isBoolean() {
+ assertTrue("is not proper boolean", cfg.isBoolean(BOOLEAN, MANDATORY));
+ assertTrue("did not detect missing field",
+ expectInvalidField(() -> cfg.isBoolean("none", MANDATORY)));
+ assertTrue("is not proper boolean", cfg.isBoolean("none", OPTIONAL));
+ assertTrue("did not detect bad boolean",
+ expectInvalidField(() -> cfg.isBoolean(TEXT, MANDATORY)));
}
@Test
@@ -115,26 +176,43 @@
assertTrue("is not proper mac", cfg.isMacAddress(MAC, MANDATORY));
assertTrue("is not proper mac", cfg.isMacAddress(MAC, OPTIONAL));
assertTrue("is not proper mac", cfg.isMacAddress("none", OPTIONAL));
- assertFalse("did not detect missing field", cfg.isMacAddress("none", MANDATORY));
+ assertTrue("did not detect missing field",
+ expectInvalidField(() -> cfg.isMacAddress("none", MANDATORY)));
+ assertTrue("did not detect bad ip",
+ expectInvalidField(() -> cfg.isMacAddress(BAD_MAC, MANDATORY)));
}
- @Test(expected = IllegalArgumentException.class)
- public void badMacAddress() {
- assertTrue("is not proper mac", cfg.isMacAddress(TEXT, MANDATORY));
- }
-
-
@Test
public void isIpAddress() {
assertTrue("is not proper ip", cfg.isIpAddress(IP, MANDATORY));
assertTrue("is not proper ip", cfg.isIpAddress(IP, OPTIONAL));
assertTrue("is not proper ip", cfg.isIpAddress("none", OPTIONAL));
- assertFalse("did not detect missing field", cfg.isMacAddress("none", MANDATORY));
+ assertTrue("did not detect missing ip",
+ expectInvalidField(() -> cfg.isIpAddress("none", MANDATORY)));
+ assertTrue("did not detect bad ip",
+ expectInvalidField(() -> cfg.isIpAddress(BAD_IP, MANDATORY)));
}
- @Test(expected = IllegalArgumentException.class)
- public void badIpAddress() {
- assertTrue("is not proper ip", cfg.isIpAddress(TEXT, MANDATORY));
+ @Test
+ public void isIpPrefix() {
+ assertTrue("is not proper prefix", cfg.isIpPrefix(PREFIX, MANDATORY));
+ assertTrue("is not proper prefix", cfg.isIpPrefix(PREFIX, OPTIONAL));
+ assertTrue("is not proper prefix", cfg.isIpPrefix("none", OPTIONAL));
+ assertTrue("did not detect missing prefix",
+ expectInvalidField(() -> cfg.isIpPrefix("none", MANDATORY)));
+ assertTrue("did not detect bad prefix",
+ expectInvalidField(() -> cfg.isIpPrefix(BAD_PREFIX, MANDATORY)));
+ }
+
+ @Test
+ public void isConnectPoint() {
+ assertTrue("is not proper connectPoint", cfg.isConnectPoint(CONNECT_POINT, MANDATORY));
+ assertTrue("is not proper connectPoint", cfg.isConnectPoint(CONNECT_POINT, OPTIONAL));
+ assertTrue("is not proper connectPoint", cfg.isConnectPoint("none", OPTIONAL));
+ assertTrue("did not detect missing connectPoint",
+ expectInvalidField(() -> cfg.isConnectPoint("none", MANDATORY)));
+ assertTrue("did not detect bad connectPoint",
+ expectInvalidField(() -> cfg.isConnectPoint(BAD_CONNECT_POINT, MANDATORY)));
}
@Test
@@ -142,16 +220,28 @@
assertTrue("is not proper transport port", cfg.isTpPort(TP_PORT, MANDATORY));
assertTrue("is not proper transport port", cfg.isTpPort(TP_PORT, OPTIONAL));
assertTrue("is not proper transport port", cfg.isTpPort("none", OPTIONAL));
- assertFalse("did not detect missing field", cfg.isTpPort("none", MANDATORY));
+ assertTrue("did not detect missing field",
+ expectInvalidField(() -> cfg.isTpPort("none", MANDATORY)));
+ assertTrue("is not proper transport port",
+ expectInvalidField(() -> cfg.isTpPort(BAD_TP_PORT, MANDATORY)));
}
- @Test(expected = IllegalArgumentException.class)
- public void badTpPort() {
- assertTrue("is not proper transport port", cfg.isTpPort(BAD_TP_PORT, MANDATORY));
+ /**
+ * Expects an InvalidFieldException to be thrown when the given runnable is
+ * run.
+ *
+ * @param runnable runnable to run
+ * @return true if an InvalidFieldException was thrown, otherwise false
+ */
+ private boolean expectInvalidField(Runnable runnable) {
+ try {
+ runnable.run();
+ return false;
+ } catch (InvalidFieldException e) {
+ return true;
+ }
}
- // TODO: Add tests for other helper methods
-
private class TestConfig extends Config<String> {
}
@@ -160,4 +250,4 @@
public void onApply(Config config) {
}
}
-}
\ No newline at end of file
+}
diff --git a/core/store/dist/src/main/java/org/onosproject/store/config/impl/DistributedNetworkConfigStore.java b/core/store/dist/src/main/java/org/onosproject/store/config/impl/DistributedNetworkConfigStore.java
index 2d4fc4d..87148a8 100644
--- a/core/store/dist/src/main/java/org/onosproject/store/config/impl/DistributedNetworkConfigStore.java
+++ b/core/store/dist/src/main/java/org/onosproject/store/config/impl/DistributedNetworkConfigStore.java
@@ -39,6 +39,7 @@
import org.onosproject.net.config.Config;
import org.onosproject.net.config.ConfigApplyDelegate;
import org.onosproject.net.config.ConfigFactory;
+import org.onosproject.net.config.InvalidConfigException;
import org.onosproject.net.config.NetworkConfigEvent;
import org.onosproject.net.config.NetworkConfigStore;
import org.onosproject.net.config.NetworkConfigStoreDelegate;
@@ -248,7 +249,17 @@
public <S, C extends Config<S>> C applyConfig(S subject, Class<C> configClass, JsonNode json) {
// Create the configuration and validate it.
C config = createConfig(subject, configClass, json);
- checkArgument(config.isValid(), INVALID_CONFIG_JSON);
+
+ try {
+ checkArgument(config.isValid(), INVALID_CONFIG_JSON);
+ } catch (RuntimeException e) {
+ ConfigFactory<S, C> configFactory = getConfigFactory(configClass);
+ String subjectKey = configFactory.subjectFactory().subjectClassKey();
+ String subjectString = configFactory.subjectFactory().subjectKey(config.subject());
+ String configKey = config.key();
+
+ throw new InvalidConfigException(subjectKey, subjectString, configKey, e);
+ }
// Insert the validated configuration and get it back.
Versioned<JsonNode> versioned = configs.putAndGet(key(subject, configClass), json);
diff --git a/web/api/src/main/java/org/onosproject/rest/exceptions/InvalidConfigExceptionMapper.java b/web/api/src/main/java/org/onosproject/rest/exceptions/InvalidConfigExceptionMapper.java
new file mode 100644
index 0000000..ecbe0af
--- /dev/null
+++ b/web/api/src/main/java/org/onosproject/rest/exceptions/InvalidConfigExceptionMapper.java
@@ -0,0 +1,60 @@
+/*
+ * Copyright 2016-present Open Networking Laboratory
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.onosproject.rest.exceptions;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fasterxml.jackson.databind.node.ObjectNode;
+import org.onlab.rest.exceptions.AbstractMapper;
+import org.onosproject.net.config.InvalidConfigException;
+import org.onosproject.net.config.InvalidFieldException;
+
+import javax.ws.rs.core.Response;
+
+/**
+ * Maps InvalidConfigException to JSON output.
+ */
+public class InvalidConfigExceptionMapper extends AbstractMapper<InvalidConfigException> {
+
+ @Override
+ protected Response.Status responseStatus() {
+ return Response.Status.BAD_REQUEST;
+ }
+
+ @Override
+ protected Response.ResponseBuilder response(Response.Status status, Throwable exception) {
+ error = exception;
+
+ InvalidConfigException ex = (InvalidConfigException) exception;
+
+ ObjectMapper mapper = new ObjectMapper();
+ String message = messageFrom(exception);
+ ObjectNode result = mapper.createObjectNode()
+ .put("code", status.getStatusCode())
+ .put("message", message)
+ .put("subjectKey", ex.subjectKey())
+ .put("subject", ex.subject())
+ .put("configKey", ex.configKey());
+
+ if (ex.getCause() instanceof InvalidFieldException) {
+ InvalidFieldException fieldException = (InvalidFieldException) ex.getCause();
+ result.put("field", fieldException.field())
+ .put("reason", fieldException.reason());
+ }
+
+ return Response.status(status).entity(result.toString());
+ }
+}
diff --git a/web/api/src/main/java/org/onosproject/rest/exceptions/package-info.java b/web/api/src/main/java/org/onosproject/rest/exceptions/package-info.java
new file mode 100644
index 0000000..26c7f26
--- /dev/null
+++ b/web/api/src/main/java/org/onosproject/rest/exceptions/package-info.java
@@ -0,0 +1,20 @@
+/*
+ * Copyright 2016-present Open Networking Laboratory
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+/**
+ * REST exceptions.
+ */
+package org.onosproject.rest.exceptions;
diff --git a/web/api/src/main/java/org/onosproject/rest/resources/CoreWebApplication.java b/web/api/src/main/java/org/onosproject/rest/resources/CoreWebApplication.java
index 3a4170b..b8597fa 100644
--- a/web/api/src/main/java/org/onosproject/rest/resources/CoreWebApplication.java
+++ b/web/api/src/main/java/org/onosproject/rest/resources/CoreWebApplication.java
@@ -17,6 +17,7 @@
package org.onosproject.rest.resources;
import org.onlab.rest.AbstractWebApplication;
+import org.onosproject.rest.exceptions.InvalidConfigExceptionMapper;
import java.util.Set;
@@ -49,7 +50,8 @@
RegionsWebResource.class,
TenantWebResource.class,
VirtualNetworkWebResource.class,
- MastershipWebResource.class
+ MastershipWebResource.class,
+ InvalidConfigExceptionMapper.class
);
}
}