Commit 3a69c8a7 authored by Jelte Jansen's avatar Jelte Jansen

added error feedback to config_validate() and option to validate partial...

added error feedback to config_validate() and option to validate partial configurations in cpp version
fixed a bug in Mapelement->str() (it can now print empty map elements)
added tests for python config_data module and fixed a few bugs there


git-svn-id: svn://bind10.isc.org/svn/bind10/trunk@911 e5f2f494-b856-4b98-b285-d166d9295462
parent 633e231b
...@@ -122,7 +122,11 @@ AuthSrv::updateConfig(isc::data::ElementPtr config) { ...@@ -122,7 +122,11 @@ AuthSrv::updateConfig(isc::data::ElementPtr config) {
// todo: what to do with port change. restart automatically? // todo: what to do with port change. restart automatically?
// ignore atm // ignore atm
//} //}
std::cout << "[XX] auth: new config " << config << std::endl; if (config) {
std::cout << "[XX] auth: new config " << config << std::endl;
} else {
std::cout << "[XX] auth: new config empty" << std::endl;
}
return isc::config::createAnswer(0); return isc::config::createAnswer(0);
} }
...@@ -487,7 +487,11 @@ MapElement::str() ...@@ -487,7 +487,11 @@ MapElement::str()
ss << ", "; ss << ", ";
} }
ss << "\"" << (*it).first << "\": "; ss << "\"" << (*it).first << "\": ";
ss << (*it).second->str(); if ((*it).second) {
ss << (*it).second->str();
} else {
ss << "None";
}
} }
ss << "}"; ss << "}";
return ss.str(); return ss.str();
......
...@@ -409,7 +409,7 @@ public: ...@@ -409,7 +409,7 @@ public:
using Element::setValue; using Element::setValue;
bool setValue(std::map<std::string, ElementPtr>& v) { m = v; return true; }; bool setValue(std::map<std::string, ElementPtr>& v) { m = v; return true; };
using Element::get; using Element::get;
ElementPtr get(const std::string& s) { return m[s]; }; ElementPtr get(const std::string& s) { if (contains(s)) { return m[s]; } else { return ElementPtr();} };
using Element::set; using Element::set;
void set(const std::string& s, ElementPtr p) { m[s] = p; }; void set(const std::string& s, ElementPtr p) { m[s] = p; };
using Element::remove; using Element::remove;
......
...@@ -172,12 +172,20 @@ ElementPtr ...@@ -172,12 +172,20 @@ ElementPtr
ModuleCCSession::handleConfigUpdate(ElementPtr new_config) ModuleCCSession::handleConfigUpdate(ElementPtr new_config)
{ {
ElementPtr answer; ElementPtr answer;
ElementPtr errors = Element::createFromString("[]");
std::cout << "handleConfigUpdate " << new_config << std::endl;
if (!config_handler_) { if (!config_handler_) {
answer = createAnswer(1, module_name_ + " does not have a config handler"); answer = createAnswer(1, module_name_ + " does not have a config handler");
} else if (!module_specification_.validate_config(new_config)) { } else if (!module_specification_.validate_config(new_config, false, errors)) {
answer = createAnswer(2, "Error in config validation"); std::stringstream ss;
ss << "Error in config validation: ";
BOOST_FOREACH(ElementPtr error, errors->listValue()) {
ss << error->stringValue();
}
answer = createAnswer(2, ss.str());
} else { } else {
// handle config update // handle config update
std::cout << "handleConfigUpdate " << new_config << std::endl;
answer = config_handler_(new_config); answer = config_handler_(new_config);
int rcode; int rcode;
parseAnswer(rcode, answer); parseAnswer(rcode, answer);
...@@ -185,6 +193,7 @@ ModuleCCSession::handleConfigUpdate(ElementPtr new_config) ...@@ -185,6 +193,7 @@ ModuleCCSession::handleConfigUpdate(ElementPtr new_config)
config_ = new_config; config_ = new_config;
} }
} }
std::cout << "end handleConfigUpdate " << new_config << std::endl;
return answer; return answer;
} }
......
...@@ -209,10 +209,17 @@ ModuleSpec::getModuleName() ...@@ -209,10 +209,17 @@ ModuleSpec::getModuleName()
} }
bool bool
ModuleSpec::validate_config(const ElementPtr data) ModuleSpec::validate_config(const ElementPtr data, const bool full)
{ {
ElementPtr spec = module_specification->find("module_spec/config_data"); ElementPtr spec = module_specification->find("module_spec/config_data");
return validate_spec_list(spec, data); return validate_spec_list(spec, data, full, ElementPtr());
}
bool
ModuleSpec::validate_config(const ElementPtr data, const bool full, ElementPtr errors)
{
ElementPtr spec = module_specification->find("module_spec/config_data");
return validate_spec_list(spec, data, full, errors);
} }
ModuleSpec ModuleSpec
...@@ -279,28 +286,34 @@ check_type(ElementPtr spec, ElementPtr element) ...@@ -279,28 +286,34 @@ check_type(ElementPtr spec, ElementPtr element)
} }
bool bool
ModuleSpec::validate_item(const ElementPtr spec, const ElementPtr data) { ModuleSpec::validate_item(const ElementPtr spec, const ElementPtr data, const bool full, ElementPtr errors) {
if (!check_type(spec, data)) { if (!check_type(spec, data)) {
// we should do some proper error feedback here // we should do some proper error feedback here
// std::cout << "type mismatch; not " << spec->get("item_type") << ": " << data << std::endl; // std::cout << "type mismatch; not " << spec->get("item_type") << ": " << data << std::endl;
// std::cout << spec << std::endl; // std::cout << spec << std::endl;
if (errors) {
errors->add(Element::create("Type mismatch"));
}
return false; return false;
} }
if (data->getType() == Element::list) { if (data->getType() == Element::list) {
ElementPtr list_spec = spec->get("list_item_spec"); ElementPtr list_spec = spec->get("list_item_spec");
BOOST_FOREACH(ElementPtr list_el, data->listValue()) { BOOST_FOREACH(ElementPtr list_el, data->listValue()) {
if (!check_type(list_spec, list_el)) { if (!check_type(list_spec, list_el)) {
if (errors) {
errors->add(Element::create("Type mismatch"));
}
return false; return false;
} }
if (list_spec->get("item_type")->stringValue() == "map") { if (list_spec->get("item_type")->stringValue() == "map") {
if (!validate_item(list_spec, list_el)) { if (!validate_item(list_spec, list_el, full, errors)) {
return false; return false;
} }
} }
} }
} }
if (data->getType() == Element::map) { if (data->getType() == Element::map) {
if (!validate_spec_list(spec->get("map_item_spec"), data)) { if (!validate_spec_list(spec->get("map_item_spec"), data, full, errors)) {
return false; return false;
} }
} }
...@@ -309,18 +322,21 @@ ModuleSpec::validate_item(const ElementPtr spec, const ElementPtr data) { ...@@ -309,18 +322,21 @@ ModuleSpec::validate_item(const ElementPtr spec, const ElementPtr data) {
// spec is a map with item_name etc, data is a map // spec is a map with item_name etc, data is a map
bool bool
ModuleSpec::validate_spec(const ElementPtr spec, const ElementPtr data) { ModuleSpec::validate_spec(const ElementPtr spec, const ElementPtr data, const bool full, ElementPtr errors) {
std::string item_name = spec->get("item_name")->stringValue(); std::string item_name = spec->get("item_name")->stringValue();
bool optional = spec->get("item_optional")->boolValue(); bool optional = spec->get("item_optional")->boolValue();
ElementPtr data_el; ElementPtr data_el;
data_el = data->get(item_name); data_el = data->get(item_name);
if (data_el) { if (data_el) {
if (!validate_item(spec, data_el)) { if (!validate_item(spec, data_el, full, errors)) {
return false; return false;
} }
} else { } else {
if (!optional) { if (!optional && full) {
if (errors) {
errors->add(Element::create("Non-optional value missing"));
}
return false; return false;
} }
} }
...@@ -329,11 +345,11 @@ ModuleSpec::validate_spec(const ElementPtr spec, const ElementPtr data) { ...@@ -329,11 +345,11 @@ ModuleSpec::validate_spec(const ElementPtr spec, const ElementPtr data) {
// spec is a list of maps, data is a map // spec is a list of maps, data is a map
bool bool
ModuleSpec::validate_spec_list(const ElementPtr spec, const ElementPtr data) { ModuleSpec::validate_spec_list(const ElementPtr spec, const ElementPtr data, const bool full, ElementPtr errors) {
ElementPtr cur_data_el; ElementPtr cur_data_el;
std::string cur_item_name; std::string cur_item_name;
BOOST_FOREACH(ElementPtr cur_spec_el, spec->listValue()) { BOOST_FOREACH(ElementPtr cur_spec_el, spec->listValue()) {
if (!validate_spec(cur_spec_el, data)) { if (!validate_spec(cur_spec_el, data, full, errors)) {
return false; return false;
} }
} }
......
...@@ -83,12 +83,15 @@ namespace isc { namespace config { ...@@ -83,12 +83,15 @@ namespace isc { namespace config {
/// \param data The base \c Element of the data to check /// \param data The base \c Element of the data to check
/// \return true if the data conforms to the specification, /// \return true if the data conforms to the specification,
/// false otherwise. /// false otherwise.
bool validate_config(const ElementPtr data); bool validate_config(const ElementPtr data, const bool full = false);
/// errors must be of type ListElement
bool validate_config(const ElementPtr data, const bool full, ElementPtr errors);
private: private:
bool validate_item(const ElementPtr spec, const ElementPtr data); bool validate_item(const ElementPtr spec, const ElementPtr data, const bool full, ElementPtr errors);
bool validate_spec(const ElementPtr spec, const ElementPtr data); bool validate_spec(const ElementPtr spec, const ElementPtr data, const bool full, ElementPtr errors);
bool validate_spec_list(const ElementPtr spec, const ElementPtr data); bool validate_spec_list(const ElementPtr spec, const ElementPtr data, const bool full, ElementPtr errors);
ElementPtr module_specification; ElementPtr module_specification;
}; };
......
...@@ -230,6 +230,8 @@ class ConfigManager: ...@@ -230,6 +230,8 @@ class ConfigManager:
# todo: use api (and check the data against the definition?) # todo: use api (and check the data against the definition?)
module_name = cmd[1] module_name = cmd[1]
conf_part = data.find_no_exc(self.config.data, module_name) conf_part = data.find_no_exc(self.config.data, module_name)
print("[XX] cfgmgr conf part:")
print(conf_part)
if conf_part: if conf_part:
data.merge(conf_part, cmd[2]) data.merge(conf_part, cmd[2])
self.cc.group_sendmsg({ "config_update": conf_part }, module_name) self.cc.group_sendmsg({ "config_update": conf_part }, module_name)
...@@ -246,6 +248,7 @@ class ConfigManager: ...@@ -246,6 +248,7 @@ class ConfigManager:
self.write_config() self.write_config()
elif len(cmd) == 2: elif len(cmd) == 2:
# todo: use api (and check the data against the definition?) # todo: use api (and check the data against the definition?)
old_data = self.config.data.copy()
data.merge(self.config.data, cmd[1]) data.merge(self.config.data, cmd[1])
# send out changed info # send out changed info
got_error = False got_error = False
...@@ -262,8 +265,8 @@ class ConfigManager: ...@@ -262,8 +265,8 @@ class ConfigManager:
self.write_config() self.write_config()
answer = isc.config.ccsession.create_answer(0) answer = isc.config.ccsession.create_answer(0)
else: else:
# TODO rollback changes that did get through? # TODO rollback changes that did get through, should we re-send update?
# feed back *all* errors? self.config.data = old_data
answer = isc.config.ccsession.create_answer(1, " ".join(err_list)) answer = isc.config.ccsession.create_answer(1, " ".join(err_list))
else: else:
answer = isc.config.ccsession.create_answer(1, "Wrong number of arguments") answer = isc.config.ccsession.create_answer(1, "Wrong number of arguments")
......
...@@ -26,9 +26,10 @@ import isc.config.module_spec ...@@ -26,9 +26,10 @@ import isc.config.module_spec
class ConfigDataError(Exception): pass class ConfigDataError(Exception): pass
def check_type(spec_part, value): def check_type(spec_part, value):
"""Returns true if the value is of the correct type given the """Does nothing if the value is of the correct type given the
specification part relevant for the value. spec_part can be specification part relevant for the value. Raises an
retrieved with find_spec()""" isc.cc.data.DataTypeError exception if not. spec_part can be
retrieved with find_spec_part()"""
if type(spec_part) == list: if type(spec_part) == list:
data_type = "list" data_type = "list"
else: else:
...@@ -36,9 +37,9 @@ def check_type(spec_part, value): ...@@ -36,9 +37,9 @@ def check_type(spec_part, value):
if data_type == "integer" and type(value) != int: if data_type == "integer" and type(value) != int:
raise isc.cc.data.DataTypeError(str(value) + " is not an integer") raise isc.cc.data.DataTypeError(str(value) + " is not an integer")
elif data_type == "real" and type(value) != double: elif data_type == "real" and type(value) != float:
raise isc.cc.data.DataTypeError(str(value) + " is not a real") raise isc.cc.data.DataTypeError(str(value) + " is not a real")
elif data_type == "boolean" and type(value) != boolean: elif data_type == "boolean" and type(value) != bool:
raise isc.cc.data.DataTypeError(str(value) + " is not a boolean") raise isc.cc.data.DataTypeError(str(value) + " is not a boolean")
elif data_type == "string" and type(value) != str: elif data_type == "string" and type(value) != str:
raise isc.cc.data.DataTypeError(str(value) + " is not a string") raise isc.cc.data.DataTypeError(str(value) + " is not a string")
...@@ -52,7 +53,7 @@ def check_type(spec_part, value): ...@@ -52,7 +53,7 @@ def check_type(spec_part, value):
# todo: check types of map contents too # todo: check types of map contents too
raise isc.cc.data.DataTypeError(str(value) + " is not a map") raise isc.cc.data.DataTypeError(str(value) + " is not a map")
def find_spec(element, identifier): def find_spec_part(element, identifier):
"""find the data definition for the given identifier """find the data definition for the given identifier
returns either a map with 'item_name' etc, or a list of those""" returns either a map with 'item_name' etc, or a list of those"""
if identifier == "": if identifier == "":
...@@ -65,6 +66,14 @@ def find_spec(element, identifier): ...@@ -65,6 +66,14 @@ def find_spec(element, identifier):
cur_el = cur_el[id] cur_el = cur_el[id]
elif type(cur_el) == dict and 'item_name' in cur_el.keys() and cur_el['item_name'] == id: elif type(cur_el) == dict and 'item_name' in cur_el.keys() and cur_el['item_name'] == id:
pass pass
elif type(cur_el) == dict and 'map_item_spec' in cur_el.keys():
found = False
for cur_el_item in cur_el['map_item_spec']:
if cur_el_item['item_name'] == id:
cur_el = cur_el_item
found = True
if not found:
raise isc.cc.data.DataNotFoundError(id + " in " + str(cur_el))
elif type(cur_el) == list: elif type(cur_el) == list:
found = False found = False
for cur_el_item in cur_el: for cur_el_item in cur_el:
...@@ -84,24 +93,30 @@ def spec_name_list(spec, prefix="", recurse=False): ...@@ -84,24 +93,30 @@ def spec_name_list(spec, prefix="", recurse=False):
if prefix != "" and not prefix.endswith("/"): if prefix != "" and not prefix.endswith("/"):
prefix += "/" prefix += "/"
if type(spec) == dict: if type(spec) == dict:
for name in spec: if 'map_item_spec' in spec:
result.append(prefix + name + "/") for map_el in spec['map_item_spec']:
if recurse: name = map_el['item_name']
result.extend(spec_name_list(spec[name],name, recurse)) if map_el['item_type'] == 'map':
name += "/"
result.append(prefix + name)
else:
for name in spec:
result.append(prefix + name + "/")
if recurse:
print("[XX] recurse1")
result.extend(spec_name_list(spec[name],name, recurse))
elif type(spec) == list: elif type(spec) == list:
for list_el in spec: for list_el in spec:
if 'item_name' in list_el: if 'item_name' in list_el:
if list_el['item_type'] == dict: if list_el['item_type'] == "map" and recurse:
if recurse: result.extend(spec_name_list(list_el['map_item_spec'], prefix + list_el['item_name'], recurse))
result.extend(spec_name_list(list_el['map_item_spec'], prefix + list_el['item_name'], recurse))
else: else:
name = list_el['item_name'] name = list_el['item_name']
if list_el['item_type'] in ["list", "map"]: if list_el['item_type'] in ["list", "map"]:
name += "/" name += "/"
result.append(name) result.append(prefix + name)
return result return result
class ConfigData: class ConfigData:
"""This class stores the module specs and the current non-default """This class stores the module specs and the current non-default
config values. It provides functions to get the actual value or config values. It provides functions to get the actual value or
...@@ -118,11 +133,12 @@ class ConfigData: ...@@ -118,11 +133,12 @@ class ConfigData:
def get_value(self, identifier): def get_value(self, identifier):
"""Returns a tuple where the first item is the value at the """Returns a tuple where the first item is the value at the
given identifier, and the second item is a bool which is given identifier, and the second item is a bool which is
true if the value is an unset default""" true if the value is an unset default. Raises an
isc.cc.data.DataNotFoundError if the identifier is bad"""
value = isc.cc.data.find_no_exc(self.data, identifier) value = isc.cc.data.find_no_exc(self.data, identifier)
if value: if value != None:
return value, False return value, False
spec = find_spec(self.specification.get_config_spec(), identifier) spec = find_spec_part(self.specification.get_config_spec(), identifier)
if spec and 'item_default' in spec: if spec and 'item_default' in spec:
return spec['item_default'], True return spec['item_default'], True
return None, False return None, False
...@@ -144,7 +160,7 @@ class ConfigData: ...@@ -144,7 +160,7 @@ class ConfigData:
all 'sub'options at the given identifier. If recurse is True, all 'sub'options at the given identifier. If recurse is True,
it will also add all identifiers of all children, if any""" it will also add all identifiers of all children, if any"""
if identifier: if identifier:
spec = find_spec(self.specification.get_config_spec(), identifier, recurse) spec = find_spec_part(self.specification.get_config_spec(), identifier)
return spec_name_list(spec, identifier + "/") return spec_name_list(spec, identifier + "/")
return spec_name_list(self.specification.get_config_spec(), "", recurse) return spec_name_list(self.specification.get_config_spec(), "", recurse)
...@@ -183,7 +199,8 @@ class MultiConfigData: ...@@ -183,7 +199,8 @@ class MultiConfigData:
self._specifications[spec.get_module_name()] = spec self._specifications[spec.get_module_name()] = spec
def get_module_spec(self, module): def get_module_spec(self, module):
"""Returns the ModuleSpec for the module with the given name""" """Returns the ModuleSpec for the module with the given name.
If there is no such module, it returns None"""
if module in self._specifications: if module in self._specifications:
return self._specifications[module] return self._specifications[module]
else: else:
...@@ -193,14 +210,16 @@ class MultiConfigData: ...@@ -193,14 +210,16 @@ class MultiConfigData:
"""Returns the specification for the item at the given """Returns the specification for the item at the given
identifier, or None if not found. The first part of the identifier, or None if not found. The first part of the
identifier (up to the first /) is interpreted as the module identifier (up to the first /) is interpreted as the module
name.""" name. Returns None if not found."""
if identifier[0] == '/': if identifier[0] == '/':
identifier = identifier[1:] identifier = identifier[1:]
module, sep, id = identifier.partition("/") module, sep, id = identifier.partition("/")
try: try:
return find_spec(self._specifications[module].get_config_spec(), id) return find_spec_part(self._specifications[module].get_config_spec(), id)
except isc.cc.data.DataNotFoundError as dnfe: except isc.cc.data.DataNotFoundError as dnfe:
return None return None
except KeyError as ke:
return None
# this function should only be called by __request_config # this function should only be called by __request_config
def _set_current_config(self, config): def _set_current_config(self, config):
...@@ -252,7 +271,7 @@ class MultiConfigData: ...@@ -252,7 +271,7 @@ class MultiConfigData:
identifier = identifier[1:] identifier = identifier[1:]
module, sep, id = identifier.partition("/") module, sep, id = identifier.partition("/")
try: try:
spec = find_spec(self._specifications[module].get_config_spec(), id) spec = find_spec_part(self._specifications[module].get_config_spec(), id)
if 'item_default' in spec: if 'item_default' in spec:
return spec['item_default'] return spec['item_default']
else: else:
...@@ -268,13 +287,13 @@ class MultiConfigData: ...@@ -268,13 +287,13 @@ class MultiConfigData:
(local change, current setting, default as specified by the (local change, current setting, default as specified by the
specification, or not found at all).""" specification, or not found at all)."""
value = self.get_local_value(identifier) value = self.get_local_value(identifier)
if value: if value != None:
return value, self.LOCAL return value, self.LOCAL
value = self.get_current_value(identifier) value = self.get_current_value(identifier)
if value: if value != None:
return value, self.CURRENT return value, self.CURRENT
value = self.get_default_value(identifier) value = self.get_default_value(identifier)
if value: if value != None:
return value, self.DEFAULT return value, self.DEFAULT
return None, self.NONE return None, self.NONE
...@@ -305,7 +324,7 @@ class MultiConfigData: ...@@ -305,7 +324,7 @@ class MultiConfigData:
module, sep, id = identifier.partition('/') module, sep, id = identifier.partition('/')
spec = self.get_module_spec(module) spec = self.get_module_spec(module)
if spec: if spec:
spec_part = find_spec(spec.get_config_spec(), id) spec_part = find_spec_part(spec.get_config_spec(), id)
print(spec_part) print(spec_part)
if type(spec_part) == list: if type(spec_part) == list:
for item in spec_part: for item in spec_part:
...@@ -357,12 +376,15 @@ class MultiConfigData: ...@@ -357,12 +376,15 @@ class MultiConfigData:
return result return result
def set_value(self, identifier, value): def set_value(self, identifier, value):
"""Set the local value at the given identifier to value""" """Set the local value at the given identifier to value. If
there is a specification for the given identifier, the type
is checked."""
spec_part = self.find_spec_part(identifier) spec_part = self.find_spec_part(identifier)
check_type(spec_part, value) if spec_part != None:
check_type(spec_part, value)
isc.cc.data.set(self._local_changes, identifier, value) isc.cc.data.set(self._local_changes, identifier, value)
def get_config_item_list(self, identifier = None): def get_config_item_list(self, identifier = None, recurse = False):
"""Returns a list of strings containing the item_names of """Returns a list of strings containing the item_names of
the child items at the given identifier. If no identifier is the child items at the given identifier. If no identifier is
specified, returns a list of module names. The first part of specified, returns a list of module names. The first part of
...@@ -370,6 +392,12 @@ class MultiConfigData: ...@@ -370,6 +392,12 @@ class MultiConfigData:
module name""" module name"""
if identifier: if identifier:
spec = self.find_spec_part(identifier) spec = self.find_spec_part(identifier)
return spec_name_list(spec, identifier + "/") return spec_name_list(spec, identifier + "/", recurse)
else: else:
return self._specifications.keys() if recurse:
id_list = []
for module in self._specifications:
id_list.extend(spec_name_list(self._specifications[module], module, recurse))
return id_list
else:
return list(self._specifications.keys())
...@@ -28,13 +28,261 @@ class TestConfigData(unittest.TestCase): ...@@ -28,13 +28,261 @@ class TestConfigData(unittest.TestCase):
self.data_path = os.environ['CONFIG_TESTDATA_PATH'] self.data_path = os.environ['CONFIG_TESTDATA_PATH']
else: else:
self.data_path = "../../../testdata" self.data_path = "../../../testdata"
spec = isc.config.module_spec_from_file(self.data_path + os.sep + "spec2.spec")
self.cd = ConfigData(spec)
def test_module_spec_from_file(self): #def test_module_spec_from_file(self):
spec = isc.config.module_spec_from_file(self.data_path + os.sep + "spec1.spec") # spec = isc.config.module_spec_from_file(self.data_path + os.sep + "spec1.spec")
cd = ConfigData(spec) # cd = ConfigData(spec)
self.assertEqual(cd.specification, spec) # self.assertEqual(cd.specification, spec)
self.assertEqual(cd.data, {}) # self.assertEqual(cd.data, {})