[trac926] support direct addition of values to named set

and a few minor style fixes

src/bin/bindctl/bindcmd.py

@@ -659,10 +659,9 @@ class BindCmdInterpreter(Cmd):
                 data, default = self.config_data.get_value(identifier)
                 print(json.dumps(data))
         elif cmd.command == "add":
-            if 'value' in cmd.params:
-                self.config_data.add_value(identifier, cmd.params['value'])
-            else:
-                self.config_data.add_value(identifier)
+            self.config_data.add_value(identifier,
+                                       cmd.params.get('value_or_name'),
+                                       cmd.params.get('value_for_set'))
         elif cmd.command == "remove":
             if 'value' in cmd.params:
                 self.config_data.remove_value(identifier, cmd.params['value'])
@@ -686,7 +685,7 @@ class BindCmdInterpreter(Cmd):
         elif cmd.command == "commit":
             self.config_data.commit()
         elif cmd.command == "diff":
-            print(self.config_data.get_local_changes());
+            print(self.config_data.get_local_changes())
         elif cmd.command == "go":
             self.go(identifier)
 

src/bin/bindctl/bindctl_main.py.in

@@ -50,17 +50,28 @@ def prepare_config_commands(tool):
     cmd.add_param(param)
     module.add_command(cmd)
 
-    cmd = CommandInfo(name = "add", desc = "Add an entry to configuration list. If no value is given, a default value is added.")
+    cmd = CommandInfo(name = "add", desc =
+        "Add an entry to configuration list or a named set. "
+        "When adding to a list, the command has one optional argument, "
+        "a value to add to the list. The value must be in correct JSON "
+        "and complete. When adding to a named set, it has one "
+        "mandatory parameter (the name to add), and an optional "
+        "parameter value, similar to when adding to a list. "
+        "In either case, when no value is given, an entry will be "
+        "constructed with default values.")
     param = ParamInfo(name = "identifier", type = "string", optional=True, desc = DEFAULT_IDENTIFIER_DESC)
     cmd.add_param(param)
-    param = ParamInfo(name = "value", type = "string", optional=True, desc = "Specifies a value to add to the list. It must be in correct JSON format and complete.")
+    param = ParamInfo(name = "value_or_name", type = "string", optional=True, desc = "Specifies a value to add to the list, or the name when adding to a named set. It must be in correct JSON format and complete.")
+    cmd.add_param(param)
+    module.add_command(cmd)
+    param = ParamInfo(name = "value_for_set", type = "string", optional=True, desc = "Specifies an optional value to add to the named map. It must be in correct JSON format and complete.")
     cmd.add_param(param)
     module.add_command(cmd)
 
-    cmd = CommandInfo(name = "remove", desc = "Remove entry from configuration list.")
+    cmd = CommandInfo(name = "remove", desc = "Remove entry from configuration list or named set.")
     param = ParamInfo(name = "identifier", type = "string", optional=True, desc = DEFAULT_IDENTIFIER_DESC)
     cmd.add_param(param)
-    param = ParamInfo(name = "value", type = "string", optional=True, desc = "Specifies a value to remove from the list. It must be in correct JSON format and complete.")
+    param = ParamInfo(name = "value", type = "string", optional=True, desc = "When identifier is a list, specifies a value to remove from the list. It must be in correct JSON format and complete. When it is a named set, specifies the name to remove.")
     cmd.add_param(param)
     module.add_command(cmd)
 

src/lib/config/tests/module_spec_unittests.cc

@@ -217,7 +217,6 @@ TEST(ModuleSpec, NamedSetValidation) {
 
     ElementPtr errors = Element::createList();
     EXPECT_TRUE(dataTestWithErrors(dd, "data32_1.data", errors));
-    std::cout << "[XX] ERRORS: " << *errors << std::endl;
     EXPECT_FALSE(dataTest(dd, "data32_2.data"));
     EXPECT_FALSE(dataTest(dd, "data32_3.data"));
 }

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

@@ -312,7 +312,7 @@ class ModuleCCSession(ConfigData):
         module_spec = isc.config.module_spec_from_file(spec_file_name)
         module_cfg = ConfigData(module_spec)
         module_name = module_spec.get_module_name()
-        self._session.group_subscribe(module_name);
+        self._session.group_subscribe(module_name)
 
         # Get the current config for that module now
         seq = self._session.group_sendmsg(create_command(COMMAND_GET_CONFIG, { "module_name": module_name }), "ConfigManager")
@@ -327,7 +327,7 @@ class ModuleCCSession(ConfigData):
             rcode, value = parse_answer(answer)
             if rcode == 0:
                 if value != None and module_spec.validate_config(False, value):
-                    module_cfg.set_local_config(value);
+                    module_cfg.set_local_config(value)
                     if config_update_callback is not None:
                         config_update_callback(value, module_cfg)
 
@@ -377,7 +377,7 @@ class ModuleCCSession(ConfigData):
                         if self.get_module_spec().validate_config(False,
                                                                   value,
                                                                   errors):
-                            self.set_local_config(value);
+                            self.set_local_config(value)
                             if self._config_handler:
                                 self._config_handler(value)
                         else:
@@ -415,8 +415,8 @@ class UIModuleCCSession(MultiConfigData):
             self.set_specification(isc.config.ModuleSpec(specs[module]))
 
     def update_specs_and_config(self):
-        self.request_specifications();
-        self.request_current_config();
+        self.request_specifications()
+        self.request_current_config()
 
     def request_current_config(self):
         """Requests the current configuration from the configuration
@@ -436,19 +436,27 @@ class UIModuleCCSession(MultiConfigData):
                 value = module_spec["list_item_spec"]["item_default"]
 
         if value is None:
-            raise isc.cc.data.DataNotFoundError("No value given and no default for " + str(identifier))
+            raise isc.cc.data.DataNotFoundError(
+                "No value given and no default for " + str(identifier))
 
         if value not in cur_list:
             cur_list.append(value)
             self.set_value(identifier, cur_list)
         else:
-            raise isc.cc.data.DataAlreadyPresentError(value + " already in " + identifier)
+            raise isc.cc.data.DataAlreadyPresentError(value +
+                                                      " already in "
+                                                      + identifier)
 
     def _add_value_to_named_set(self, identifier, value, item_value):
-        if value is None:
-            raise isc.cc.data.DataNotFoundError("Need a name to add a new item to named_set " + str(identifier))
-        elif type(value) != str:
-            raise isc.cc.data.DataTypeError("Name for named_set " + identifier + " must be a string")
+        if type(value) != str:
+            raise isc.cc.data.DataTypeError("Name for named_set " +
+                                            identifier +
+                                            " must be a string")
+        # fail on both None and empty string
+        if not value:
+            raise isc.cc.data.DataNotFoundError(
+                    "Need a name to add a new item to named_set " +
+                    str(identifier))
         else:
             cur_map, status = self.get_value(identifier)
             if not cur_map:
@@ -457,9 +465,11 @@ class UIModuleCCSession(MultiConfigData):
                 cur_map[value] = item_value
                 self.set_value(identifier, cur_map)
             else:
-                raise isc.cc.data.DataAlreadyPresentError(value + " already in " + identifier)
+                raise isc.cc.data.DataAlreadyPresentError(value +
+                                                          " already in "
+                                                          + identifier)
 
-    def add_value(self, identifier, value_str = None):
+    def add_value(self, identifier, value_str = None, set_value_str = None):
         """Add a value to a configuration list. Raises a DataTypeError
            if the value does not conform to the list_item_spec field
            of the module config data specification. If value_str is
@@ -472,19 +482,29 @@ class UIModuleCCSession(MultiConfigData):
         if module_spec is None:
             raise isc.cc.data.DataNotFoundError("Unknown item " + str(identifier))
 
-        # Hmm. Do we need to check for duplicates?
-        value = None
-        if value_str is not None:
-            value = isc.cc.data.parse_value_str(value_str)
-
         # the specified element must be a list or a named_set
         if 'list_item_spec' in module_spec:
+            value = None
+            # in lists, we might get the value with spaces, making it
+            # the third argument. In that case we interpret both as
+            # one big string meant as the value
+            if value_str is not None:
+                if set_value_str is not None:
+                    value_str += set_value_str
+                value = isc.cc.data.parse_value_str(value_str)
             self._add_value_to_list(identifier, value)
         elif 'named_set_item_spec' in module_spec:
+            item_name = None
             item_value = None
-            if 'item_default' in module_spec['named_set_item_spec']:
-                item_value = module_spec['named_set_item_spec']['item_default']
-            self._add_value_to_named_set(identifier, value, item_value)
+            if value_str is not None:
+                item_name =  isc.cc.data.parse_value_str(value_str)
+            if set_value_str is not None:
+                item_value = isc.cc.data.parse_value_str(set_value_str)
+            else:
+                if 'item_default' in module_spec['named_set_item_spec']:
+                    item_value = module_spec['named_set_item_spec']['item_default']
+            self._add_value_to_named_set(identifier, item_name,
+                                         item_value)
         else:
             raise isc.cc.data.DataNotFoundError(str(identifier) + " is not a list or a named set")
 

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

@@ -261,7 +261,7 @@ class ConfigData:
 
     def get_local_config(self):
         """Returns the non-default config values in a dict"""
-        return self.data;
+        return self.data
 
     def get_item_list(self, identifier = None, recurse = False):
         """Returns a list of strings containing the full identifiers of

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

@@ -361,7 +361,7 @@ class TestMultiConfigData(unittest.TestCase):
 
     def test_get_current_config(self):
         cf = { 'module1': { 'item1': 2, 'item2': True } }
-        self.mcd._set_current_config(cf);
+        self.mcd._set_current_config(cf)
         self.assertEqual(cf, self.mcd.get_current_config())
 
     def test_get_local_changes(self):
@@ -411,7 +411,6 @@ class TestMultiConfigData(unittest.TestCase):
         self.assertEqual('a', value)
         value = self.mcd.get_default_value("Spec2/item5[1]")
         self.assertEqual('b', value)
-        self.assertRaises(self.mcd.get_default_value("Spec2/item5[2]"))
         value = self.mcd.get_default_value("Spec2/item5[5]")
         self.assertEqual(None, value)
         value = self.mcd.get_default_value("Spec2/item5[0][1]")