refactoring of cfgmgr and config in general; they now use the datadefinition class so they could later validate data that is passed around

(refactoring not done yet, though it is now in a working state again, which seemed like a good time to commit)
added a config_data.py with classes for storing definitions and data (for both modules and UIs)
fixed a missed refactoring bug in bob
changed DataDefinition initializer; a string is now parsed instead of seen as a file name; there is a helper function in the module to read a datadef directly from file now
added a temporary example config data specification for auth module
added a temporary second config data element to bob.spec



git-svn-id: svn://bind10.isc.org/svn/bind10/branches/jelte-configuration@814 e5f2f494-b856-4b98-b285-d166d9295462

src/bin/auth/auth.spec

@@ -1,6 +1,24 @@
 {
   "data_specification": {
-    "module_name": "ParkingLot"
+    "module_name": "Auth",
+    "config_data": [
+      { "item_name": "default_name",
+        "item_type": "string",
+        "item_optional": False,
+        "item_default": "Hello, world!"
+      },
+      { "item_name": "zone_list",
+        "item_type": "list",
+        "item_optional": False,
+        "item_default": [],
+        "list_item_spec":
+          { "item_name": "zone_name",
+            "item_type": "string",
+            "item_optional": True,
+            "item_default": ""
+          }
+      }
+    ]
   }
 }
 

src/bin/bind10/bind10.py.in

@@ -478,7 +478,7 @@ def main():
 
         for fd in rlist + xlist:
             if fd == ccs_fd:
-                boss_of_bind.ccs.checkCommand()
+                boss_of_bind.ccs.check_command()
             elif fd == wakeup_fd:
                 os.read(wakeup_fd, 32)
 

src/bin/bind10/bob.spec

@@ -7,6 +7,12 @@
         "item_type": "string",
         "item_optional": False,
         "item_default": "Hi, shane!"
+      },
+      {
+        "item_name": "some_other_string",
+        "item_type": "string",
+        "item_optional": False,
+        "item_default": "Hi, shane!"
       }
     ],
     "commands": [

src/bin/bindctl/bindcmd.py

@@ -85,7 +85,7 @@ class BindCmdInterpreter(Cmd):
                 return False
 
             # Get all module information from cmd-ctrld
-            self.config_data = isc.cc.data.UIConfigData(self)
+            self.config_data = isc.config.UIConfigData(self)
             self.update_commands()
             self.cmdloop()
         except KeyboardInterrupt:
@@ -150,7 +150,8 @@ class BindCmdInterpreter(Cmd):
         if (len(cmd_spec) == 0):
             print('can\'t get any command specification')
         for module_name in cmd_spec.keys():
-            self.prepare_module_commands(module_name, cmd_spec[module_name])
+            if cmd_spec[module_name]:
+                self.prepare_module_commands(module_name, cmd_spec[module_name])
 
     def send_GET(self, url, body = None):
         headers = {"cookie" : self.session_id}
@@ -315,7 +316,7 @@ class BindCmdInterpreter(Cmd):
                     if cmd.module == "config":
                         # grm text has been stripped of slashes...
                         my_text = self.location + "/" + cur_line.rpartition(" ")[2]
-                        list = self.config_data.config.get_item_list(my_text.rpartition("/")[0])
+                        list = self.config_data.get_config_item_list(my_text.rpartition("/")[0])
                         hints.extend([val for val in list if val.startswith(text)])
             except CmdModuleNameFormatError:
                 if not text:
@@ -440,17 +441,17 @@ class BindCmdInterpreter(Cmd):
                         line += "(modified)"
                     print(line)
             elif cmd.command == "add":
-                self.config_data.add(identifier, cmd.params['value'])
+                self.config_data.add_value(identifier, cmd.params['value'])
             elif cmd.command == "remove":
-                self.config_data.remove(identifier, cmd.params['value'])
+                self.config_data.remove_value(identifier, cmd.params['value'])
             elif cmd.command == "set":
-                self.config_data.set(identifier, cmd.params['value'])
+                self.config_data.set_value(identifier, cmd.params['value'])
             elif cmd.command == "unset":
                 self.config_data.unset(identifier)
             elif cmd.command == "revert":
                 self.config_data.revert()
             elif cmd.command == "commit":
-                self.config_data.commit(self)
+                self.config_data.commit()
             elif cmd.command == "go":
                 self.go(identifier)
         except isc.cc.data.DataTypeError as dte:

src/bin/bindctl/bindctl.in

@@ -5,7 +5,7 @@ export PYTHON_EXEC
 
 BINDCTL_PATH=@abs_top_srcdir@/src/bin/bindctl
 
-PYTHONPATH=@abs_top_srcdir@/src/lib/cc/python
+PYTHONPATH=@abs_top_builddir@/pyshared
 export PYTHONPATH
 
 cd ${BINDCTL_PATH}

src/bin/bindctl/bindctl.py

@@ -67,12 +67,17 @@ def prepare_config_commands(tool):
     tool.add_module_info(module)
 
 if __name__ == '__main__':
-    try:
-        tool = BindCmdInterpreter("localhost:8080")
-        prepare_config_commands(tool)
-        tool.run()
-    except Exception as e:
-        print(e)
-        print("Failed to connect with b10-cmdctl module, is it running?")
+    tool = BindCmdInterpreter("localhost:8080")
+    prepare_config_commands(tool)
+    tool.run()
+# TODO: put below back, was removed to see errors
+#if __name__ == '__main__':
+    #try:
+        #tool = BindCmdInterpreter("localhost:8080")
+        #prepare_config_commands(tool)
+        #tool.run()
+    #except Exception as e:
+        #print(e)
+        #print("Failed to connect with b10-cmdctl module, is it running?")
 
 

src/lib/cc/python/isc/cc/data.py

@@ -84,9 +84,10 @@ def set(element, identifier, value):
             else:
                 # set to none, and parent el not found, return
                 return element
-    if value:
+    # value can be an empty list or dict, so check for None eplicitely
+    if value != None:
         cur_el[id_parts[-1]] = value
-    else:
+    elif id_parts[-1] in cur_el:
         del cur_el[id_parts[-1]]
     return element
 
@@ -114,85 +115,6 @@ def find_no_exc(element, identifier):
             return None
     return cur_el
 
-#
-# hmm, these are more relevant for datadefition
-# should we (re)move them?
-#
-def find_spec(element, identifier):
-    """find the data definition for the given identifier
-       returns either a map with 'item_name' etc, or a list of those"""
-    id_parts = identifier.split("/")
-    id_parts[:] = (value for value in id_parts if value != "")
-    cur_el = element
-    for id in id_parts:
-        if type(cur_el) == dict and id in cur_el.keys():
-            cur_el = cur_el[id]
-        elif type(cur_el) == dict and 'item_name' in cur_el.keys() and cur_el['item_name'] == id:
-            pass
-        elif type(cur_el) == list:
-            found = False
-            for cur_el_item in cur_el:
-                if cur_el_item['item_name'] == id and 'item_default' in cur_el_item.keys():
-                    cur_el = cur_el_item
-                    found = True
-            if not found:
-                raise DataNotFoundError(id + " in " + str(cur_el))
-        else:
-            raise DataNotFoundError(id + " in " + str(cur_el))
-    return cur_el
-
-def check_type(specification, value):
-    """Returns true if the value is of the correct type given the
-       specification"""
-    if type(specification) == list:
-        data_type = "list"
-    else:
-        data_type = specification['item_type']
-
-    if data_type == "integer" and type(value) != int:
-        raise DataTypeError(str(value) + " should be an integer")
-    elif data_type == "real" and type(value) != double:
-        raise DataTypeError(str(value) + " should be a real")
-    elif data_type == "boolean" and type(value) != boolean:
-        raise DataTypeError(str(value) + " should be a boolean")
-    elif data_type == "string" and type(value) != str:
-        raise DataTypeError(str(value) + " should be a string")
-    elif data_type == "list":
-        if type(value) != list:
-            raise DataTypeError(str(value) + " should be a list, not a " + str(value.__class__.__name__))
-        else:
-            # todo: check subtypes etc
-            for element in value:
-                check_type(specification['list_item_spec'], element)
-    elif data_type == "map" and type(value) != dict:
-        # todo: check subtypes etc
-        raise DataTypeError(str(value) + " should be a map")
-
-def spec_name_list(spec, prefix="", recurse=False):
-    """Returns a full list of all possible item identifiers in the
-       specification (part)"""
-    result = []
-    if prefix != "" and not prefix.endswith("/"):
-        prefix += "/"
-    if type(spec) == dict:
-        for name in spec:
-            result.append(prefix + name + "/")
-            if recurse:
-                result.extend(spec_name_list(spec[name],name, recurse))
-    elif type(spec) == list:
-        for list_el in spec:
-            if 'item_name' in list_el:
-                if list_el['item_type'] == dict:
-                    if recurse:
-                        result.extend(spec_name_list(list_el['map_item_spec'], prefix + list_el['item_name'], recurse))
-                else:
-                    name = list_el['item_name']
-                    if list_el['item_type'] in ["list", "map"]:
-                        name += "/"
-                    result.append(name)
-
-    return result
-
 def parse_value_str(value_str):
     """Parses the given string to a native python object. If the
        string cannot be parsed, it is returned. If it is not a string,
@@ -208,181 +130,3 @@ def parse_value_str(value_str):
         # simply return the string itself
         return value_str
 
-class ConfigData:
-    def __init__(self, specification):
-        self.specification = specification
-        self.data = {}
-
-    def get_item_list(self, identifier = None):
-        if identifier:
-            spec = find_spec(self.specification, identifier)
-            return spec_name_list(spec, identifier + "/")
-        return spec_name_list(self.specification)
-
-    def get_value(self, identifier):
-        """Returns a tuple where the first item is the value at the
-           given identifier, and the second item is a bool which is
-           true if the value is an unset default"""
-        value = find_no_exc(self.data, identifier)
-        if value:
-            return value, False
-        spec = find_spec(self.specification, identifier)
-        if spec and 'item_default' in spec:
-            return spec['item_default'], True
-        return None, False
-
-class UIConfigData():
-    def __init__(self, conn, name = ''):
-        self.module_name = name
-        data_spec = self.get_data_specification(conn)
-        self.config = ConfigData(data_spec)
-        self.get_config_data(conn)
-        self.config_changes = {}
-    
-    def get_config_data(self, conn):
-        self.config.data = conn.send_GET('/config_data') 
-
-    def send_changes(self, conn):
-        conn.send_POST('/ConfigManager/set_config', self.config_changes)
-        # Get latest config data
-        self.get_config_data(conn)
-        self.config_changes = {}
-    
-    def get_data_specification(self, conn):
-        return conn.send_GET('/config_spec') 
-
-    def set(self, identifier, value):
-        # check against definition
-        spec = find_spec(identifier)
-        check_type(spec, value)
-        set(self.config_changes, identifier, value)
-
-    def get_value(self, identifier):
-        """Returns a three-tuple, where the first item is the value
-           (or None), the second is a boolean specifying whether
-           the value is the default value, and the third is a boolean
-           specifying whether the value is an uncommitted change"""
-        value = find_no_exc(self.config_changes, identifier)
-        if value:
-            return value, False, True
-        value, default = self.config.get_value(identifier)
-        if value:
-            return value, default, False
-        return None, False, False
-
-    def get_value_map_single(self, identifier, entry):
-        """Returns a single entry for a value_map, where the value is
-           not a part of a bigger map"""
-        result_part = {}
-        result_part['name'] = entry['item_name']
-        result_part['type'] = entry['item_type']
-        value, default, modified = self.get_value(identifier)
-        # should we check type and only set int, double, bool and string here?
-        result_part['value'] = value
-        result_part['default'] = default
-        result_part['modified'] = modified
-        return result_part
-
-    def get_value_map(self, identifier, entry):
-        """Returns a single entry for a value_map, where the value is
-           a part of a bigger map"""
-        result_part = {}
-        result_part['name'] = entry['item_name']
-        result_part['type'] = entry['item_type']
-        value, default, modified = self.get_value(identifier + "/" + entry['item_name'])
-        # should we check type and only set int, double, bool and string here?
-        result_part['value'] = value
-        result_part['default'] = default
-        result_part['modified'] = modified
-        return result_part
-
-    def get_value_maps(self, identifier = None):
-        """Returns a list of maps, containing the following values:
-           name: name of the entry (string)
-           type: string containing the type of the value (or 'module')
-           value: value of the entry if it is a string, int, double or bool
-           modified: true if the value is a local change
-           default: true if the value has been changed
-           Throws DataNotFoundError if the identifier is bad
-        """
-        spec = find_spec(self.config.specification, identifier)
-        result = []
-        if type(spec) == dict:
-            # either the top-level list of modules or a spec map
-            if 'item_name' in spec:
-                result_part = self.get_value_map_single(identifier, spec)
-                if result_part['type'] == "list":
-                    values = self.get_value(identifier)[0]
-                    if values:
-                        for value in values:
-                            result_part2 = {}
-                            li_spec = spec['list_item_spec']
-                            result_part2['name'] = li_spec['item_name']
-                            result_part2['value'] = value
-                            result_part2['type'] = li_spec['item_type']
-                            result_part2['default'] = False
-                            result_part2['modified'] = False
-                            result.append(result_part2)
-                else:
-                    result.append(result_part)
-                
-            else:
-                for name in spec:
-                    result_part = {}
-                    result_part['name'] = name
-                    result_part['type'] = "module"
-                    result_part['value'] = None
-                    result_part['default'] = False
-                    result_part['modified'] = False
-                    result.append(result_part)
-        elif type(spec) == list:
-            for entry in spec:
-                if type(entry) == dict and 'item_name' in entry:
-                    result.append(self.get_value_map(identifier, entry))
-        return result
-
-    def add(self, identifier, value_str):
-        data_spec = find_spec(self.config.specification, identifier)
-        if (type(data_spec) != dict or "list_item_spec" not in data_spec):
-            raise DataTypeError(identifier + " is not a list")
-        value = parse_value_str(value_str)
-        check_type(data_spec, [value])
-        cur_list = find_no_exc(self.config_changes, identifier)
-        if not cur_list:
-            cur_list = find_no_exc(self.config.data, identifier)
-        if not cur_list:
-            cur_list = []
-        if value not in cur_list:
-            cur_list.append(value)
-        set(self.config_changes, identifier, cur_list)
-
-    def remove(self, identifier, value_str):
-        data_spec = find_spec(self.config.specification, identifier)
-        if (type(data_spec) != dict or "list_item_spec" not in data_spec):
-            raise DataTypeError(identifier + " is not a list")
-        value = parse_value_str(value_str)
-        check_type(data_spec, [value])
-        cur_list = find_no_exc(self.config_changes, identifier)
-        if not cur_list:
-            cur_list = find_no_exc(self.config.data, identifier)
-        if not cur_list:
-            cur_list = []
-        if value in cur_list:
-            cur_list.remove(value)
-        set(self.config_changes, identifier, cur_list)
-
-    def set(self, identifier, value_str):
-        data_spec = find_spec(self.config.specification, identifier)
-        value = parse_value_str(value_str)
-        check_type(data_spec, value)
-        set(self.config_changes, identifier, value)
-
-    def unset(self, identifier):
-        # todo: check whether the value is optional?
-        unset(self.config_changes, identifier)
-
-    def revert(self):
-        self.config_changes = {}
-
-    def commit(self, conn):
-        self.send_changes(conn)

src/lib/config/python/isc/config/__init__.py

@@ -1,2 +1,3 @@
 from isc.config.ccsession import *
+from isc.config.config_data import *
 from isc.config.datadefinition import *

src/lib/config/python/isc/config/ccsession.py

@@ -27,7 +27,7 @@ import isc
 
 class CCSession:
     def __init__(self, spec_file_name, config_handler, command_handler):
-        self._data_definition = isc.config.DataDefinition(spec_file_name)
+        self._data_definition = isc.config.data_spec_from_file(spec_file_name)
         self._module_name = self._data_definition.get_module_name()
         
         self.set_config_handler(config_handler)
@@ -83,7 +83,7 @@ class CCSession:
 
     def __send_spec(self):
         """Sends the data specification to the configuration manager"""
-        self._session.group_sendmsg(self._data_definition.get_definition(), "ConfigManager")
+        self._session.group_sendmsg({ "data_specification": self._data_definition.get_definition() }, "ConfigManager")
         answer, env = self._session.group_recvmsg(False)
         
     def __get_full_config(self):

src/lib/config/python/isc/config/cfgmgr.py

@@ -106,9 +106,12 @@ class ConfigManager:
        The ability to specify a custom session is for testing purposes
        and should not be needed for normal usage."""
     def __init__(self, data_path, session = None):
-        self.commands = {}
+        # remove these and use self.data_specs
+        #self.commands = {}
         self.data_definitions = {}
+
         self.data_path = data_path
+        self.data_specs = {}
         self.config = ConfigManagerData(data_path)
         if session:
             self.cc = session
@@ -122,21 +125,40 @@ class ConfigManager:
         """Notifies the Boss module that the Config Manager is running"""
         self.cc.group_sendmsg({"running": "configmanager"}, "Boss")
 
-    def set_config(self, module_name, data_specification):
-        """Set the data specification for the given module"""
-        self.data_definitions[module_name] = data_specification
-        
-    def remove_config(self, module_name):
-        """Remove the data specification for the given module"""
-        self.data_definitions[module_name]
+    def set_data_spec(self, spec):
+        #data_def = isc.config.DataDefinition(spec)
+        self.data_specs[spec.get_module_name()] = spec
 
-    def set_commands(self, module_name, commands):
-        """Set the command list for the given module"""
-        self.commands[module_name] = commands
+    def get_data_spec(self, module_name):
+        if module_name in self.data_specs:
+            return self.data_specs[module_name]
 
-    def remove_commands(self, module_name):
-        """Remove the command list for the given module"""
-        del self.commands[module_name]
+    def get_config_data(self, name = None):
+        """Returns a dict containing 'module_name': config_data for
+           all modules. If name is specified, only that module will
+           be included"""
+        config_data = {}
+        if name:
+            if name in self.data_specs:
+                config_data[name] = self.data_specs[name].get_data
+        else:
+            for module_name in self.data_specs.keys():
+                config_data[module_name] = self.data_specs[module_name].get_config_data()
+        return config_data
+
+    def get_commands(self, name = None):
+        """Returns a dict containing 'module_name': commands_dict for
+           all modules. If name is specified, only that module will
+           be included"""
+        commands = {}
+        if name:
+            if name in self.data_specs:
+                commands[name] = self.data_specs[name].get_commands
+        else:
+            for module_name in self.data_specs.keys():
+                print("[XX] add commands for " + module_name)
+                commands[module_name] = self.data_specs[module_name].get_commands()
+        return commands
 
     def read_config(self):
         """Read the current configuration from the b10-config.db file
@@ -158,16 +180,13 @@ class ConfigManager:
             if type(cmd[1]) == dict:
                 if 'module_name' in cmd[1] and cmd[1]['module_name'] != '':
                     module_name = cmd[1]['module_name']
-                    try:
-                        answer["result"] = [0, self.data_definitions[module_name]]
-                    except KeyError as ke:
-                        answer["result"] = [1, "No specification for module " + module_name]
+                    answer["result"] = [0, self.get_config_data(module_name)]
                 else:
                     answer["result"] = [1, "Bad module_name in get_data_spec command"]
             else:
                 answer["result"] = [1, "Bad get_data_spec command, argument not a dict"]
         else:
-            answer["result"] = [0, self.data_definitions]
+            answer["result"] = [0, self.get_config_data()]
         return answer
 
     def _handle_get_config(self, cmd):
@@ -201,6 +220,8 @@ class ConfigManager:
                 self.cc.group_sendmsg({ "config_update": conf_part }, module_name)
             else:
                 conf_part = data.set(self.config.data, module_name, {})
+                print("[XX] SET CONF PART:")
+                print(conf_part)
                 data.merge(conf_part[module_name], cmd[2])
                 # send out changed info
                 self.cc.group_sendmsg({ "config_update": conf_part[module_name] }, module_name)
@@ -224,28 +245,37 @@ class ConfigManager:
         # todo: use DataDefinition class
         # todo: error checking (like keyerrors)
         answer = {}
-        if "config_data" in spec:
-            self.set_config(spec["module_name"], spec["config_data"])
-            self.cc.group_sendmsg({ "specification_update": [ spec["module_name"], spec["config_data"] ] }, "Cmd-Ctrld")
-        if "commands" in spec:
-            self.set_commands(spec["module_name"], spec["commands"])
-            self.cc.group_sendmsg({ "commands_update": [ spec["module_name"], spec["commands"] ] }, "Cmd-Ctrld")
+        print("[XX] CFGMGR got spec:")
+        print(spec)
+        self.set_data_spec(spec)
+        
+        # We should make one general 'spec update for module' that
+        # passes both specification and commands at once
+        self.cc.group_sendmsg({ "specification_update": [ spec.get_module_name(), spec.get_config_data() ] }, "Cmd-Ctrld")
+        self.cc.group_sendmsg({ "commands_update": [ spec.get_module_name(), spec.get_commands() ] }, "Cmd-Ctrld")
         answer["result"] = [ 0 ]
         return answer
 
     def handle_msg(self, msg):
         """Handle a direct command"""
         answer = {}
+        print("[XX] cfgmgr got msg:")
+        print(msg)
         if "command" in msg:
             cmd = msg["command"]
             try:
                 if cmd[0] == "get_commands":
-                    answer["result"] = [ 0, self.commands ]
-
+                    answer["result"] = [ 0, self.get_commands() ]
+                    print("[XX] get_commands answer:")
+                    print(answer)
                 elif cmd[0] == "get_data_spec":
                     answer = self._handle_get_data_spec(cmd)
+                    print("[XX] get_data_spec answer:")
+                    print(answer)
                 elif cmd[0] == "get_config":
                     answer = self._handle_get_config(cmd)
+                    print("[XX] get_config answer:")
+                    print(answer)
                 elif cmd[0] == "set_config":
                     answer = self._handle_set_config(cmd)
                 elif cmd[0] == "shutdown":
@@ -258,12 +288,17 @@ class ConfigManager:
                 answer["result"] = [ 1, "Missing argument in command: " + str(ie) ]
                 raise ie
         elif "data_specification" in msg:
-            answer = self._handle_data_specification(msg["data_specification"])
+            try:
+                answer = self._handle_data_specification(isc.config.DataDefinition(msg["data_specification"]))
+            except isc.config.DataDefinitionError as dde:
+                answer['result'] = [ 1, "Error in data definition: " + str(dde) ]
         elif 'result' in msg:
             # this seems wrong, might start pingpong
             answer['result'] = [0]
         else:
             answer["result"] = [ 1, "Unknown message format: " + str(msg) ]
+        print("[XX] cfgmgr sending answer:")
+        print(answer)
         return answer
         
     def run(self):

src/lib/config/python/isc/config/cfgmgr_test.py

@@ -102,12 +102,10 @@ class TestConfigManager(unittest.TestCase):
         self.fake_session = FakeCCSession()
         self.cm = ConfigManager(self.data_path, self.fake_session)
         self.name = "TestModule"
-        self.spec = { "asdf" }
-        self.commands = { "bbbb" }
+        self.spec = isc.config.data_spec_from_file(self.data_path + os.sep + "/spec2.spec")
     
     def test_init(self):
-        self.assert_(self.cm.commands == {})
-        self.assert_(self.cm.data_definitions == {})
+        self.assert_(self.cm.data_specs == {})
         self.assert_(self.cm.data_path == self.data_path)
         self.assert_(self.cm.config != None)
         self.assert_(self.fake_session.has_subscription("ConfigManager"))
@@ -121,27 +119,26 @@ class TestConfigManager(unittest.TestCase):
         # this one is actually wrong, but 'current status quo'
         self.assertEqual(msg, {"running": "configmanager"})
 
-    def test_set_config(self):
-        self.cm.set_config(self.name, self.spec)
-        self.assertEqual(self.cm.data_definitions[self.name], self.spec)
+    #def test_set_config(self):
+        #self.cm.set_config(self.name, self.spec)
+        #self.assertEqual(self.cm.data_definitions[self.name], self.spec)
 
-    def test_remove_config(self):
-        self.assertRaises(KeyError, self.cm.remove_config, self.name)
-        self.cm.set_config(self.name, self.spec)
-        self.cm.remove_config(self.name)
+    #def test_remove_config(self):
+        #self.assertRaises(KeyError, self.cm.remove_config, self.name)
+        #self.cm.set_config(self.name, self.spec)
+        #self.cm.remove_config(self.name)
 
-    def test_set_commands(self):
-        self.cm.set_commands(self.name, self.commands)
-        self.assertEqual(self.cm.commands[self.name], self.commands)
+    #def test_set_commands(self):
+    #    self.cm.set_commands(self.name, self.commands)
+    #    self.assertEqual(self.cm.commands[self.name], self.commands)
 
-    def test_write_config(self):
-        self.assertRaises(KeyError, self.cm.remove_commands, self.name)
-        self.cm.set_commands(self.name, self.commands)
-        self.cm.remove_commands(self.name)
+    #def test_write_config(self):
+    #    self.assertRaises(KeyError, self.cm.remove_commands, self.name)
+    #    self.cm.set_commands(self.name, self.commands)
+    #    self.cm.remove_commands(self.name)
 
     def _handle_msg_helper(self, msg, expected_answer):
         answer = self.cm.handle_msg(msg)
-        #self.assertEquals(answer, expected_answer)
         self.assertEqual(expected_answer, answer)
 
     def test_handle_msg(self):
@@ -150,8 +147,8 @@ class TestConfigManager(unittest.TestCase):
         self._handle_msg_helper({ "command": [ "badcommand" ] }, { 'result': [ 1, "Unknown command: ['badcommand']"]})
         self._handle_msg_helper({ "command": [ "get_commands" ] }, { 'result': [ 0, {} ]})
         self._handle_msg_helper({ "command": [ "get_data_spec" ] }, { 'result': [ 0, {} ]})
-        self._handle_msg_helper({ "command": [ "get_data_spec", { "module_name": "nosuchmodule" } ] },
-                                {'result': [1, 'No specification for module nosuchmodule']})
+        #self._handle_msg_helper({ "command": [ "get_data_spec", { "module_name": "nosuchmodule" } ] },
+        #                        {'result': [1, 'No specification for module nosuchmodule']})
         self._handle_msg_helper({ "command": [ "get_data_spec", 1 ] },
                                 {'result': [1, 'Bad get_data_spec command, argument not a dict']})
         self._handle_msg_helper({ "command": [ "get_data_spec", { } ] },
@@ -181,16 +178,23 @@ class TestConfigManager(unittest.TestCase):
                          self.fake_session.get_message(self.name, None))
         self.assertEqual({'version': 1, 'TestModule': {'test': 124}}, self.cm.config.data)
         self._handle_msg_helper({ "data_specification": 
-                                  { "module_name": self.name, "config_data": self.spec, "commands": self.commands }
+                                  self.spec.get_definition()
                                 },
                                 {'result': [0]})
-        self.assertEqual(len(self.fake_session.message_queue), 2)
+        self._handle_msg_helper({ "data_specification": 
+                                  { 'foo': 1 }
+                                },
+                                {'result': [1, 'Error in data definition: no module_name in data_specification']})
+        self._handle_msg_helper({ "command": [ "get_data_spec" ] }, { 'result': [ 0, { self.spec.get_module_name(): self.spec.get_config_spec() } ]})
+        self._handle_msg_helper({ "command": [ "get_commands" ] }, { 'result': [ 0, { self.spec.get_module_name(): self.spec.get_commands() } ]})
+        # re-add this once we have new way to propagate spec changes (1 instead of the current 2 messages)
+        #self.assertEqual(len(self.fake_session.message_queue), 2)
         # the name here is actually wrong (and hardcoded), but needed in the current version
         # TODO: fix that
-        self.assertEqual({'specification_update': [ self.name, self.spec ] },
-                         self.fake_session.get_message("Cmd-Ctrld", None))
-        self.assertEqual({'commands_update': [ self.name, self.commands ] },
-                         self.fake_session.get_message("Cmd-Ctrld", None))
+        #self.assertEqual({'specification_update': [ self.name, self.spec ] },
+        #                 self.fake_session.get_message("Cmd-Ctrld", None))
+        #self.assertEqual({'commands_update': [ self.name, self.commands ] },
+        #                 self.fake_session.get_message("Cmd-Ctrld", None))
         
         
 

src/lib/config/python/isc/config/config_data.py

@@ -0,0 +1,576 @@
+# Copyright (C) 2010  Internet Systems Consortium.
+#
+# Permission to use, copy, modify, and distribute this software for any
+# purpose with or without fee is hereby granted, provided that the above
+# copyright notice and this permission notice appear in all copies.
+#
+# THE SOFTWARE IS PROVIDED "AS IS" AND INTERNET SYSTEMS CONSORTIUM
+# DISCLAIMS ALL WARRANTIES WITH REGARD TO THIS SOFTWARE INCLUDING ALL
+# IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL
+# INTERNET SYSTEMS CONSORTIUM BE LIABLE FOR ANY SPECIAL, DIRECT,
+# INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING
+# FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT,
+# NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION
+# WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+
+#
+# Class to store configuration data and data definition
+# Used by the config manager and python modules that communicate
+# with the configuration manager
+#
+
+
+import isc.cc.data
+import isc.config.datadefinition
+
+class ConfigDataError(Exception): pass
+
+#
+# hmm, these are more relevant for datadefition
+# should we (re)move them?
+#
+def check_type(specification, value):
+    """Returns true if the value is of the correct type given the
+       specification"""
+    if type(specification) == list:
+        data_type = "list"
+    else:
+        data_type = specification['item_type']
+
+    if data_type == "integer" and type(value) != int:
+        raise DataTypeError(str(value) + " should be an integer")
+    elif data_type == "real" and type(value) != double:
+        raise DataTypeError(str(value) + " should be a real")
+    elif data_type == "boolean" and type(value) != boolean:
+        raise DataTypeError(str(value) + " should be a boolean")
+    elif data_type == "string" and type(value) != str:
+        raise DataTypeError(str(value) + " should be a string")
+    elif data_type == "list":
+        if type(value) != list:
+            raise DataTypeError(str(value) + " should be a list, not a " + str(value.__class__.__name__))
+        else:
+            # todo: check subtypes etc
+            for element in value:
+                check_type(specification['list_item_spec'], element)
+    elif data_type == "map" and type(value) != dict:
+        # todo: check subtypes etc
+        raise DataTypeError(str(value) + " should be a map")
+
+def find_spec(element, identifier):
+    """find the data definition for the given identifier
+       returns either a map with 'item_name' etc, or a list of those"""
+    if identifier == "":
+        return element
+    id_parts = identifier.split("/")
+    id_parts[:] = (value for value in id_parts if value != "")
+    cur_el = element
+    for id in id_parts:
+        if type(cur_el) == dict and id in cur_el.keys():
+            cur_el = cur_el[id]
+        elif type(cur_el) == dict and 'item_name' in cur_el.keys() and cur_el['item_name'] == id:
+            pass
+        elif type(cur_el) == list:
+            found = False
+            for cur_el_item in cur_el:
+                if cur_el_item['item_name'] == id and 'item_default' in cur_el_item.keys():
+                    cur_el = cur_el_item
+                    found = True
+            if not found:
+                raise isc.cc.data.DataNotFoundError(id + " in " + str(cur_el))
+        else:
+            raise isc.cc.data.DataNotFoundError(id + " in " + str(cur_el))
+    return cur_el
+
+def spec_name_list(spec, prefix="", recurse=False):
+    """Returns a full list of all possible item identifiers in the
+       specification (part)"""
+    result = []
+    if prefix != "" and not prefix.endswith("/"):
+        prefix += "/"
+    if type(spec) == dict:
+        for name in spec:
+            result.append(prefix + name + "/")
+            if recurse:
+                result.extend(spec_name_list(spec[name],name, recurse))
+    elif type(spec) == list:
+        for list_el in spec:
+            if 'item_name' in list_el:
+                if list_el['item_type'] == dict:
+                    if recurse:
+                        result.extend(spec_name_list(list_el['map_item_spec'], prefix + list_el['item_name'], recurse))
+                else:
+                    name = list_el['item_name']
+                    if list_el['item_type'] in ["list", "map"]:
+                        name += "/"
+                    result.append(name)
+    return result
+
+
+class ConfigData:
+    """This class stores the datadefinition and the current non-default
+       config values. It provides functions to get the actual value or
+       the default value if no non-default value has been set"""
+   
+    def __init__(self, specification):
+        """Initialize a ConfigData instance. If specification is not
+           of type DataDefinition, a ConfigDataError is raised."""
+        if type(specification) != isc.config.DataDefinition:
+            raise ConfigDataError("specification is of type " + str(type(specification)) + ", not DataDefinition")
+        self.specification = specification
+        self.data = {}
+
+    def get_item_list(self, identifier = None):
+        if identifier:
+            spec = find_spec(self.specification, identifier)
+            return spec_name_list(spec, identifier + "/")
+        return spec_name_list(self.specification)
+
+    def get_value(self, identifier):
+        """Returns a tuple where the first item is the value at the
+           given identifier, and the second item is a bool which is
+           true if the value is an unset default"""
+        value = find_no_exc(self.data, identifier)
+        if value:
+            return value, False
+        spec = find_spec(self.specification, identifier)
+        if spec and 'item_default' in spec:
+            return spec['item_default'], True
+        return None, False
+
+class MultiConfigData:
+    """This class stores the datadefinitions, current non-default
+       configuration values and 'local' (uncommitted) changes."""
+    LOCAL   = 1
+    CURRENT = 2
+    DEFAULT = 3
+    NONE    = 4
+    
+    def __init__(self):
+        self._specifications = {}
+        self._current_config = {}
+        self._local_changes = {}
+
+    def set_specification(self, spec):
+        if type(spec) != isc.config.DataDefinition:
+            raise Exception("not a datadef")
+        self._specifications[spec.get_module_name()] = spec
+
+    def get_specification(self, module):
+        if module in self._specifications:
+            return self._specifications[module]
+        else:
+            return None
+
+    def find_spec_part(self, identifier):
+        """returns the default value, or None if there is no default"""
+        if identifier[0] == '/':
+            identifier = identifier[1:]
+        module, sep, id = identifier.partition("/")
+        try:
+            return find_spec(self._specifications[module].get_config_data(), id)
+        except isc.cc.data.DataNotFoundError as dnfe:
+            return None
+
+    def set_current_config(self, config):
+        self._current_config = config
+
+    def get_current_config(self):
+        """The current config is a dict where the first level is
+           the module name, and the value is the config values for
+           that module"""
+        return self._current_config
+        
+    def get_local_changes(self):
+        return self._local_changes
+
+    def clear_local_changes(self):
+        self._local_changes = {}
+
+    def get_local_value(self, identifier):
+        return isc.cc.data.find_no_exc(self._local_changes, identifier)
+        
+    def get_current_value(self, identifier):
+        """Returns the current non-default value, or None if not set"""
+        return isc.cc.data.find_no_exc(self._current_config, identifier)
+        
+    def get_default_value(self, identifier):
+        """returns the default value, or None if there is no default"""
+        if identifier[0] == '/':
+            identifier = identifier[1:]
+        module, sep, id = identifier.partition("/")
+        try:
+            spec = find_spec(self._specifications[module].get_config_data(), id)
+            if 'item_default' in spec:
+                return spec['item_default']
+            else:
+                return None
+        except isc.cc.data.DataNotFoundError as dnfe:
+            return None
+
+    def get_value(self, identifier):
+        """Returns a tuple containing value,status. Status is either
+           LOCAL, CURRENT, DEFAULT or NONE, corresponding to the
+           source of the value"""
+        value = self.get_local_value(identifier)
+        if value:
+            return value, self.LOCAL
+        value = self.get_current_value(identifier)
+        if value:
+            return value, self.CURRENT
+        value = self.get_default_value(identifier)
+        if value:
+            return value, self.DEFAULT
+        return None, self.NONE
+
+    def get_value_maps(self, identifier = None):
+        """Returns a list of dicts, containing the following values:
+           name: name of the entry (string)
+           type: string containing the type of the value (or 'module')
+           value: value of the entry if it is a string, int, double or bool
+           modified: true if the value is a local change
+           default: true if the value has been changed
+           Throws DataNotFoundError if the identifier is bad
+           TODO: use the consts for those last ones
+        """
+        result = []
+        if not identifier:
+            # No identifier, so we need the list of current modules
+            for module in self._specifications.keys():
+                entry = {}
+                entry['name'] = module
+                entry['type'] = 'module'
+                entry['value'] = None
+                entry['modified'] = False
+                entry['default'] = False
+                result.append(entry)
+        else:
+            if identifier[0] == '/':
+                identifier = identifier[1:]
+            module, sep, id = identifier.partition('/')
+            spec = self.get_specification(module)
+            if spec:
+                print("[XX] getpartspec")
+                spec_part = find_spec(spec.get_config_data(), id)
+                print(spec_part)
+                if type(spec_part) == list:
+                    for item in spec_part:
+                        entry = {}
+                        entry['name'] = item['item_name']
+                        entry['type'] = item['item_type']
+                        print("[XX] getvalue")
+                        value, status = self.get_value("/" + identifier + "/" + item['item_name'])
+                        entry['value'] = value
+                        if status == self.LOCAL:
+                            entry['modified'] = True
+                        else:
+                            entry['modified'] = False
+                        if status == self.DEFAULT:
+                            entry['default'] = False
+                        else:
+                            entry['default'] = False
+                        result.append(entry)
+                else:
+                    item = spec_part
+                    if item['item_type'] == 'list':
+                        li_spec = item['list_item_spec']
+                        print("[XX] item:")
+                        print(item)
+                        l, status =  self.get_value("/" + identifier)
+                        if l:
+                            for value in l:
+                                result_part2 = {}
+                                result_part2['name'] = li_spec['item_name']
+                                result_part2['value'] = value
+                                result_part2['type'] = li_spec['item_type']
+                                result_part2['default'] = False
+                                result_part2['modified'] = False
+                                result.append(result_part2)
+                    else:
+                        entry = {}
+                        entry['name'] = item['item_name']
+                        entry['type'] = item['item_type']
+                        #value, status = self.get_value("/" + identifier + "/" + item['item_name'])
+                        value, status = self.get_value("/" + identifier)
+                        entry['value'] = value
+                        if status == self.LOCAL:
+                            entry['modified'] = True
+                        else:
+                            entry['modified'] = False
+                        if status == self.DEFAULT:
+                            entry['default'] = False
+                        else:
+                            entry['default'] = False
+                        result.append(entry)
+            #print(spec)
+        return result
+
+    def set_value(self, identifier, value):
+        """Set the local value at the given identifier to value"""
+        # todo: validate
+        isc.cc.data.set(self._local_changes, identifier, value)
+
+    def get_config_item_list(self, identifier = None):
+        """Returns a list of strings containing the item_names of
+           the child items at the given identifier. If no identifier is
+           specified, returns a list of module names. The first part of
+           the identifier is interpreted as the module name"""
+        if identifier:
+            spec = self.find_spec_part(identifier)
+            return spec_name_list(spec, identifier + "/")
+        else:
+            return self._specifications.keys()
+
+    
+class UIConfigData():
+    """This class is used in a configuration user interface. It contains
+       specific functions for getting, displaying, and sending
+       configuration settings."""
+    def __init__(self, conn):
+        self._conn = conn
+        self._data = MultiConfigData()
+        self.request_specifications()
+        self.request_current_config()
+        a,b = self._data.get_value("/Boss/some_string")
+        print("[XX] a,b: " + str(a) + ", " + str(b))
+
+    def request_specifications(self):
+        # this step should be unnecessary but is the current way cmdctl returns stuff
+        # so changes are needed there to make this clean (we need a command to simply get the
+        # full specs for everything, including commands etc, not separate gets for that)
+        specs = self._conn.send_GET('/config_spec')
+        commands = self._conn.send_GET('/commands')
+        #print(specs)
+        #print(commands)
+        for module in specs.keys():
+            cur_spec = { 'module_name': module }
+            if module in specs and specs[module]:
+                cur_spec['config_data'] = specs[module]
+            if module in commands and commands[module]:
+                cur_spec['commands'] = commands[module]
+            
+            self._data.set_specification(isc.config.DataDefinition(cur_spec))
+
+    def request_current_config(self):
+        config = self._conn.send_GET('/config_data')
+        if 'version' not in config or config['version'] != 1:
+            raise Exception("Bad config version")
+        self._data.set_current_config(config)
+
+    def get_value(self, identifier):
+        return self._data.get_value(identifier)
+
+    def set_value(self, identifier, value):
+        return self._data.set_value(identifier, value);
+    
+    def add_value(self, identifier, value_str):
+        data_spec = self._data.find_spec_part(identifier)
+        if (type(data_spec) != dict or "list_item_spec" not in data_spec):
+            raise DataTypeError(identifier + " is not a list")
+        value = isc.cc.data.parse_value_str(value_str)
+        cur_list, status = self.get_value(identifier)
+        if not cur_list:
+            cur_list = []
+        if value not in cur_list:
+            cur_list.append(value)
+        self.set_value(identifier, cur_list)
+
+    def remove_value(self, identifier, value_str):
+        data_spec = find_spec(self.config.specification, identifier)
+        if (type(data_spec) != dict or "list_item_spec" not in data_spec):
+            raise DataTypeError(identifier + " is not a list")
+        value = parse_value_str(value_str)
+        check_type(data_spec, [value])
+        cur_list = find_no_exc(self.config_changes, identifier)
+        if not cur_list:
+            cur_list = find_no_exc(self.config.data, identifier)
+        if not cur_list:
+            cur_list = []
+        if value in cur_list:
+            cur_list.remove(value)
+        set(self.config_changes, identifier, cur_list)
+
+    def get_value_maps(self, identifier = None):
+        return self._data.get_value_maps(identifier)
+
+    def commit(self):
+        self._conn.send_POST('/ConfigManager/set_config', self._data.get_local_changes())
+        # todo: check result
+        self.request_current_config()
+        self._data.clear_local_changes()
+
+    def get_config_item_list(self, identifier = None):
+        return self._data.get_config_item_list(identifier)
+
+# remove
+class OUIConfigData():
+    """This class is used in a configuration user interface. It contains
+       specific functions for getting, displaying, and sending
+       configuration settings."""
+    def __init__(self, conn):
+        # the specs dict contains module: configdata elements
+        # these should all be replaced by the new stuff
+        data_spec = self.get_data_specification(conn)
+        self.config = data_spec
+        self.get_config_data(conn)
+        self.config_changes = {}
+        #
+        self.config_
+        self.specs = self.get_data_specifications(conn)
+        
+    
+    def get_config_data(self, conn):
+        data = conn.send_GET('/config_data')
+
+    def send_changes(self, conn):
+        conn.send_POST('/ConfigManager/set_config', self.config_changes)
+        # Get latest config data
+        self.get_config_data(conn)
+        self.config_changes = {}
+
+    def get_data_specification(self, conn):
+        return conn.send_GET('/config_spec')
+
+    def get_data_specifications(self, conn):
+        specs = {}
+        allspecs = conn.send_GET('/config_spec')
+        print("[XX] allspecs:")
+        print(allspecs)
+        
+
+    def set(self, identifier, value):
+        # check against definition
+        spec = find_spec(identifier)
+        check_type(spec, value)
+        set(self.config_changes, identifier, value)
+
+    def get_value(self, identifier):
+        """Returns a three-tuple, where the first item is the value
+           (or None), the second is a boolean specifying whether
+           the value is the default value, and the third is a boolean
+           specifying whether the value is an uncommitted change"""
+        value = isc.cc.data.find_no_exc(self.config_changes, identifier)
+        if value:
+            return value, False, True
+        value, default = self.config.get_value(identifier)
+        if value:
+            return value, default, False
+        return None, False, False
+
+    def get_value_map_single(self, identifier, entry):
+        """Returns a single entry for a value_map, where the value is
+           not a part of a bigger map"""
+        result_part = {}
+        result_part['name'] = entry['item_name']
+        result_part['type'] = entry['item_type']
+        value, default, modified = self.get_value(identifier)
+        # should we check type and only set int, double, bool and string here?
+        result_part['value'] = value
+        result_part['default'] = default
+        result_part['modified'] = modified
+        return result_part
+
+    def get_value_map(self, identifier, entry):
+        """Returns a single entry for a value_map, where the value is
+           a part of a bigger map"""
+        result_part = {}
+        result_part['name'] = entry['item_name']
+        result_part['type'] = entry['item_type']
+        value, default, modified = self.get_value(identifier + "/" + entry['item_name'])
+        # should we check type and only set int, double, bool and string here?
+        result_part['value'] = value
+        result_part['default'] = default
+        result_part['modified'] = modified
+        return result_part
+
+    def get_value_maps(self, identifier = None):
+        """Returns a list of maps, containing the following values:
+           name: name of the entry (string)
+           type: string containing the type of the value (or 'module')
+           value: value of the entry if it is a string, int, double or bool
+           modified: true if the value is a local change
+           default: true if the value has been changed
+           Throws DataNotFoundError if the identifier is bad
+        """
+        print("[XX] config:")
+        print(self.config)
+        spec = find_spec(self.config, identifier)
+        result = []
+        if type(spec) == dict:
+            # either the top-level list of modules or a spec map
+            if 'item_name' in spec:
+                result_part = self.get_value_map_single(identifier, spec)
+                if result_part['type'] == "list":
+                    values = self.get_value(identifier)[0]
+                    if values:
+                        for value in values:
+                            result_part2 = {}
+                            li_spec = spec['list_item_spec']
+                            result_part2['name'] = li_spec['item_name']
+                            result_part2['value'] = value
+                            result_part2['type'] = li_spec['item_type']
+                            result_part2['default'] = False
+                            result_part2['modified'] = False
+                            result.append(result_part2)
+                else:
+                    result.append(result_part)
+                
+            else:
+                for name in spec:
+                    result_part = {}
+                    result_part['name'] = name
+                    result_part['type'] = "module"
+                    result_part['value'] = None
+                    result_part['default'] = False
+                    result_part['modified'] = False
+                    result.append(result_part)
+        elif type(spec) == list:
+            for entry in spec:
+                if type(entry) == dict and 'item_name' in entry:
+                    result.append(self.get_value_map(identifier, entry))
+        return result
+
+    def add(self, identifier, value_str):
+        data_spec = find_spec(self.config.specification, identifier)
+        if (type(data_spec) != dict or "list_item_spec" not in data_spec):
+            raise DataTypeError(identifier + " is not a list")
+        value = parse_value_str(value_str)
+        check_type(data_spec, [value])
+        cur_list = find_no_exc(self.config_changes, identifier)
+        if not cur_list:
+            cur_list = find_no_exc(self.config.data, identifier)
+        if not cur_list:
+            cur_list = []
+        if value not in cur_list:
+            cur_list.append(value)
+        set(self.config_changes, identifier, cur_list)
+
+    def remove(self, identifier, value_str):
+        data_spec = find_spec(self.config.specification, identifier)
+        if (type(data_spec) != dict or "list_item_spec" not in data_spec):
+            raise DataTypeError(identifier + " is not a list")
+        value = parse_value_str(value_str)
+        check_type(data_spec, [value])
+        cur_list = find_no_exc(self.config_changes, identifier)
+        if not cur_list:
+            cur_list = find_no_exc(self.config.data, identifier)
+        if not cur_list:
+            cur_list = []
+        if value in cur_list:
+            cur_list.remove(value)
+        set(self.config_changes, identifier, cur_list)
+
+    def set(self, identifier, value_str):
+        data_spec = find_spec(self.config.specification, identifier)
+        value = parse_value_str(value_str)
+        check_type(data_spec, value)
+        set(self.config_changes, identifier, value)
+
+    def unset(self, identifier):
+        # todo: check whether the value is optional?
+        unset(self.config_changes, identifier)
+
+    def revert(self):
+        self.config_changes = {}
+
+    def commit(self, conn):
+        self.send_changes(conn)

src/lib/config/python/isc/config/config_data_test.py

@@ -0,0 +1,41 @@
+# Copyright (C) 2010  Internet Systems Consortium.
+#
+# Permission to use, copy, modify, and distribute this software for any
+# purpose with or without fee is hereby granted, provided that the above
+# copyright notice and this permission notice appear in all copies.
+#
+# THE SOFTWARE IS PROVIDED "AS IS" AND INTERNET SYSTEMS CONSORTIUM
+# DISCLAIMS ALL WARRANTIES WITH REGARD TO THIS SOFTWARE INCLUDING ALL
+# IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL
+# INTERNET SYSTEMS CONSORTIUM BE LIABLE FOR ANY SPECIAL, DIRECT,
+# INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING
+# FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT,
+# NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION
+# WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+
+#
+# Tests for the ConfigData and UIConfigData classes
+#
+
+import unittest
+import os
+from isc.config.config_data import *
+from isc.config.datadefinition import *
+
+class TestConfigData(unittest.TestCase):
+    def setUp(self):
+        if 'CONFIG_TESTDATA_PATH' in os.environ:
+            self.data_path = os.environ['CONFIG_TESTDATA_PATH']
+        else:
+            self.data_path = "../../../testdata"
+
+    def test_data_spec_from_file(self):
+        spec = isc.config.data_spec_from_file(self.data_path + os.sep + "spec1.spec")
+        cd = ConfigData(spec)
+        self.assertEqual(cd.specification, spec)
+        self.assertEqual(cd.data, {})
+        self.assertRaises(ConfigDataError, ConfigData, 1)
+
+if __name__ == '__main__':
+    unittest.main()
+

src/lib/config/python/isc/config/datadefinition.py

@@ -27,16 +27,28 @@ import isc.cc.data
 class DataDefinitionError(Exception):
     pass
 
+def data_spec_from_file(spec_file, check = True):
+    data_spec = None
+    if hasattr(spec_file, 'read'):
+        data_spec = ast.literal_eval(spec_file.read(-1))
+    elif type(spec_file) == str:
+        file = open(spec_file)
+        data_spec = ast.literal_eval(file.read(-1))
+        file.close()
+    else:
+        raise DataDefinitionError("spec_file not a str or file-like object")
+    if 'data_specification' not in data_spec:
+        raise DataDefinitionError("Data definition has no data_specification element")
+        
+    return DataDefinition(data_spec['data_specification'], check)
+
 class DataDefinition:
-    def __init__(self, spec_file, check = True):
-        if hasattr(spec_file, 'read'):
-            self._data_spec = self.__read_data_spec_file(spec_file)
-        elif type(spec_file) == str:
-            file = open(spec_file)
-            self._data_spec = self.__read_data_spec_file(file)
-            file.close()
-        else:
-            raise DataDefinitionError("Not a str or file-like object")
+    def __init__(self, data_spec, check = True):
+        if type(data_spec) != dict:
+            raise DataDefinitionError("data_spec is of type " + str(type(data_spec)) + ", not dict")
+        if check:
+            _check(data_spec)
+        self._data_spec = data_spec
 
     def validate(self, data, errors = None):
         """Check whether the given piece of data conforms to this
@@ -46,11 +58,6 @@ class DataDefinition:
            version stops as soon as there is one error so this list
            will not be exhaustive."""
         data_def = self.get_definition()
-        if 'data_specification' not in data_def:
-            if errors:
-                errors.append("Data definition has no data_specification element")
-            return False
-        data_def = data_def['data_specification']
         if 'config_data' not in data_def:
             if errors:
                 errors.append("The is no config_data for this specification")
@@ -58,26 +65,33 @@ class DataDefinition:
         errors = []
         return _validate_spec_list(data_def['config_data'], data, errors)
 
-    def __read_data_spec_file(self, file, check = True):
-        """Reads the data spec from the given file object.
-           If check is True, check whether it is of the correct form.
-           If it is not, an DataDefinitionError exception is raised"""
-        if not hasattr(file, 'read'):
-            raise DataDefinitionError("Not a file-like object:" + str(type(file)))
-        str = file.read(-1)
-        # TODO catch error here and reraise as a less ugly exception
-        data_spec = ast.literal_eval(str)
-        if check:
-            # TODO
-            _check(data_spec)
-            pass
-        return data_spec
+
+    def get_module_name(self):
+        return self._data_spec['module_name']
 
     def get_definition(self):
         return self._data_spec
 
-    def get_module_name(self):
-        return self._data_spec["data_specification"]["module_name"]
+    def get_config_spec(self):
+        if 'config_data' in self._data_spec:
+            return self._data_spec['config_data']
+        else:
+            return None
+    
+    def get_commands(self):
+        if 'commands' in self._data_spec:
+            return self._data_spec['commands']
+        else:
+            return None
+    
+    def get_config_data(self):
+        if 'config_data' in self._data_spec:
+            return self._data_spec['config_data']
+        else:
+            return None
+    
+    def __str__(self):
+        return self._data_spec.__str__()
 
 def _check(data_spec):
     """Checks the full specification. This is a dict that contains the
@@ -87,9 +101,6 @@ def _check(data_spec):
        of dicts. Raises a DataDefinitionError if there is a problem."""
     if type(data_spec) != dict:
         raise DataDefinitionError("data specification not a dict")
-    if "data_specification" not in data_spec:
-        raise DataDefinitionError("no data_specification element in specification")
-    data_spec = data_spec["data_specification"]
     if "module_name" not in data_spec:
         raise DataDefinitionError("no module_name in data_specification")
     if "config_data" in data_spec:
@@ -105,7 +116,7 @@ def _check_config_spec(config_data):
        specification. Raises a DataDefinitionError if there is a
        problem."""
     if type(config_data) != list:
-        raise DataDefinitionError("config_data is not a list of items")
+        raise DataDefinitionError("config_data is of type " + str(type(config_data)) + ", not a list of items")
     for config_item in config_data:
         _check_item_spec(config_item)
 

src/lib/config/python/isc/config/datadefinition_test.py

@@ -25,29 +25,29 @@ import isc.cc.data
 class TestDataDefinition(unittest.TestCase):
 
     def setUp(self):
-        self.assert_('CONFIG_TESTDATA_PATH' in os.environ)
-        self.data_path = os.environ['CONFIG_TESTDATA_PATH']
+        if 'CONFIG_TESTDATA_PATH' in os.environ:
+            self.data_path = os.environ['CONFIG_TESTDATA_PATH']
+        else:
+            self.data_path = "../../../testdata"
 
     def spec_file(self, filename):
         return(self.data_path + os.sep + filename)
 
     def read_spec_file(self, filename):
-        return DataDefinition(self.spec_file(filename))
+        return isc.config.data_spec_from_file(self.spec_file(filename))
 
     def spec1(self, dd):
-        data_def = dd.get_definition()
-        self.assert_('data_specification' in data_def)
-        data_spec = data_def['data_specification']
+        data_spec = dd.get_definition()
         self.assert_('module_name' in data_spec)
         self.assertEqual(data_spec['module_name'], "Spec1")
         
     def test_open_file_name(self):
-        dd = DataDefinition(self.spec_file("spec1.spec"))
+        dd = self.read_spec_file("spec1.spec")
         self.spec1(dd)
 
     def test_open_file_obj(self):
         file1 = open(self.spec_file("spec1.spec"))
-        dd = DataDefinition(file1)
+        dd = isc.config.data_spec_from_file(file1)
         self.spec1(dd)
 
     def test_bad_specfiles(self):
@@ -72,7 +72,7 @@ class TestDataDefinition(unittest.TestCase):
         self.assertRaises(DataDefinitionError, self.read_spec_file, "spec21.spec")
 
     def validate_data(self, specfile_name, datafile_name):
-        dd = DataDefinition(self.spec_file(specfile_name));
+        dd = self.read_spec_file(specfile_name);
         data_file = open(self.spec_file(datafile_name))
         data_str = data_file.read()
         data = isc.cc.data.parse_value_str(data_str)
@@ -89,7 +89,4 @@ class TestDataDefinition(unittest.TestCase):
         self.assertEqual(False, self.validate_data("spec22.spec", "data22_8.data"))
 
 if __name__ == '__main__':
-    if not 'CONFIG_TESTDATA_PATH' in os.environ:
-        print("You need to set the environment variable CONFIG_TESTDATA_PATH to point to the directory containing the test data files")
-        exit(1)
     unittest.main()

src/lib/config/testdata/spec2.spec

@@ -49,6 +49,23 @@
           }
         ]
       }
+    ],
+    "commands": [
+      {
+        "command_name": "print_message",
+        "command_description": "Print the given message to stdout",
+        "command_args": [ {
+          "item_name": "message",
+          "item_type": "string",
+          "item_optional": False,
+          "item_default": ""
+        } ]
+      },
+      {
+        "command_name": "shutdown",
+        "command_description": "Shut down BIND 10",
+        "command_args": []
+      }
     ]
   }
 }

src/lib/config/testdata/spec3.spec

@@ -1,6 +1,6 @@
 {
   "data_specification": {
-    "module_name": "Spec2",
+    "module_name": "Spec3",
     "config_data": [
       {
         "item_type": "integer",