[2254] refactor tab-completion for identifiers

removed all word-boundary values except whitespace (so that we no longer have to to magic to replace things around the / character, and other possible characters used in identifiers)

Pulled out the identifier-hints collection (and added tests for those)

src/bin/bindctl/bindcmd.py

@@ -48,20 +48,18 @@ except ImportError:
 # if we have readline support, use that, otherwise use normal stdio
 try:
     import readline
-    # This is a fix for the problem described in
-    # http://bind10.isc.org/ticket/1345
-    # If '-' is seen as a word-boundary, the final completion-step
-    # (as handled by the cmd module, and hence outside our reach) can
-    # mistakenly add data twice, resulting in wrong completion results
-    # The solution is to remove it.
-    delims = readline.get_completer_delims()
-    delims = delims.replace('-', '')
-    readline.set_completer_delims(delims)
+    # Only consider whitespace as word boundaries
+    readline.set_completer_delims(' \t\n')
 
     my_readline = readline.get_line_buffer
 except ImportError:
     my_readline = sys.stdin.readline
 
+# Used for tab-completion of 'identifiers' (i.e. config values)
+# If a command parameter has this name, the tab completion hints
+# are derived from config data
+IDENTIFIER_PARAM = 'identifier'
+
 CSV_FILE_NAME = 'default_user.csv'
 CONFIG_MODULE_NAME = 'config'
 CONST_BINDCTL_HELP = """
@@ -463,20 +461,24 @@ class BindCmdInterpreter(Cmd):
 
         Cmd.onecmd(self, line)
 
-    def remove_prefix(self, list, prefix):
-        """Removes the prefix already entered, and all elements from the
-           list that don't match it"""
-        if prefix.startswith('/'):
-            prefix = prefix[1:]
-
-        new_list = []
-        for val in list:
-            if val.startswith(prefix):
-                new_val = val[len(prefix):]
-                if new_val.startswith("/"):
-                    new_val = new_val[1:]
-                new_list.append(new_val)
-        return new_list
+    def _get_identifier_startswith(self, id_text):
+        """Return the tab-completion hints for identifiers starting with
+           id_text"""
+        # First get all items from the given module (up to the first /)
+        list = self.config_data.get_config_item_list(
+                        id_text.rpartition("/")[0], True)
+        hints = [val for val in list if val.startswith(id_text[1:])]
+        return hints
+
+    def _cmd_has_identifier_param(self, cmd):
+        """
+        Returns True if the given (parsed) command is known and has a
+        parameter which points to a config data identifier
+        """
+        if cmd.module not in self.modules:
+            return False
+        command = self.modules[cmd.module].get_command_with_name(cmd.command)
+        return command.has_param_with_name(IDENTIFIER_PARAM)
 
     def complete(self, text, state):
         if 0 == state:
@@ -491,17 +493,12 @@ class BindCmdInterpreter(Cmd):
                 else:
                     hints = self._get_param_startswith(cmd.module, cmd.command,
                                                        text)
-                    if cmd.module == CONFIG_MODULE_NAME:
-                        # grm text has been stripped of slashes...
-                        my_text = self.location + "/" + cur_line.rpartition(" ")[2]
-                        list = self.config_data.get_config_item_list(my_text.rpartition("/")[0], True)
-                        hints.extend([val for val in list if val.startswith(my_text[1:])])
-                        # remove the common prefix from the hints so we don't get it twice
-                        prefix, _, rest = my_text.rpartition("/")
-                        hints = self.remove_prefix(hints, prefix)
-                        # And prevent 'double addition' by also removing final
-                        # part matches
-                        hints = [ h for h in hints if h != rest ]
+                    if self._cmd_has_identifier_param(cmd):
+                        # For tab-completion of identifiers, replace hardcoded
+                        # hints with hints derived from the config data
+                        id_text = self.location + "/" + cur_line.rpartition(" ")[2]
+                        hints = self._get_identifier_startswith(id_text)
+
             except CmdModuleNameFormatError:
                 if not text:
                     hints = self.get_module_names()
@@ -522,13 +519,8 @@ class BindCmdInterpreter(Cmd):
             except BindCtlException:
                 hints = []
 
-            # there are a couple of 'standard' names that are usable, but
-            # should not be included in direct tab-completion
-            hints = [ h for h in hints if h not in [ 'argument', 'identifier']]
-
             self.hint = hints
 
-
         if state < len(self.hint):
             return self.hint[state]
         else:

src/bin/bindctl/bindctl_main.py.in

@@ -42,12 +42,12 @@ def prepare_config_commands(tool):
     cmd = CommandInfo(name = "show", desc = "Show configuration.")
     param = ParamInfo(name = "argument", type = "string", optional=True, desc = "If you specify the argument 'all' (before the identifier), recursively show all child elements for the given identifier.")
     cmd.add_param(param)
-    param = ParamInfo(name = "identifier", type = "string", optional=True, desc = DEFAULT_IDENTIFIER_DESC)
+    param = ParamInfo(name = IDENTIFIER_PARAM, type = "string", optional=True, desc = DEFAULT_IDENTIFIER_DESC)
     cmd.add_param(param)
     module.add_command(cmd)
 
     cmd = CommandInfo(name = "show_json", desc = "Show full configuration in JSON format.")
-    param = ParamInfo(name = "identifier", type = "string", optional=True, desc = DEFAULT_IDENTIFIER_DESC)
+    param = ParamInfo(name = IDENTIFIER_PARAM, type = "string", optional=True, desc = DEFAULT_IDENTIFIER_DESC)
     cmd.add_param(param)
     module.add_command(cmd)
 
@@ -60,7 +60,7 @@ def prepare_config_commands(tool):
         "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)
+    param = ParamInfo(name = IDENTIFIER_PARAM, type = "string", optional=True, desc = DEFAULT_IDENTIFIER_DESC)
     cmd.add_param(param)
     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)
@@ -70,21 +70,21 @@ def prepare_config_commands(tool):
     module.add_command(cmd)
 
     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)
+    param = ParamInfo(name = IDENTIFIER_PARAM, type = "string", optional=True, desc = DEFAULT_IDENTIFIER_DESC)
     cmd.add_param(param)
     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)
 
     cmd = CommandInfo(name = "set", desc = "Set a configuration value.")
-    param = ParamInfo(name = "identifier", type = "string", optional=True, desc = DEFAULT_IDENTIFIER_DESC)
+    param = ParamInfo(name = IDENTIFIER_PARAM, type = "string", optional=True, desc = DEFAULT_IDENTIFIER_DESC)
     cmd.add_param(param)
     param = ParamInfo(name = "value", type = "string", optional=False, desc = "Specifies a value to set. It must be in correct JSON format and complete.")
     cmd.add_param(param)
     module.add_command(cmd)
 
     cmd = CommandInfo(name = "unset", desc = "Unset a configuration value (i.e. revert to the default, if any).")
-    param = ParamInfo(name = "identifier", type = "string", optional=False, desc = DEFAULT_IDENTIFIER_DESC)
+    param = ParamInfo(name = IDENTIFIER_PARAM, type = "string", optional=False, desc = DEFAULT_IDENTIFIER_DESC)
     cmd.add_param(param)
     module.add_command(cmd)
 
@@ -98,7 +98,7 @@ def prepare_config_commands(tool):
     module.add_command(cmd)
 
     cmd = CommandInfo(name = "go", desc = "Go to a specific configuration part.")
-    param = ParamInfo(name = "identifier", type="string", optional=False, desc = DEFAULT_IDENTIFIER_DESC)
+    param = ParamInfo(name = IDENTIFIER_PARAM, type="string", optional=False, desc = DEFAULT_IDENTIFIER_DESC)
     cmd.add_param(param)
     module.add_command(cmd)
 

src/bin/bindctl/tests/bindctl_test.py

@@ -419,6 +419,40 @@ class TestConfigCommands(unittest.TestCase):
     def tearDown(self):
         sys.stdout = self.stdout_backup
 
+    def test_cmd_has_identifier_param(self):
+        module = ModuleInfo(name = "test_module")
+
+        cmd = CommandInfo(name = "command_with_identifier")
+        param = ParamInfo(name = bindcmd.IDENTIFIER_PARAM)
+        cmd.add_param(param)
+        module.add_command(cmd)
+
+        cmd = CommandInfo(name = "command_without_identifier")
+        param = ParamInfo(name = "some_argument")
+        cmd.add_param(param)
+        module.add_command(cmd)
+
+        self.tool.add_module_info(module)
+
+        cmd = cmdparse.BindCmdParse("test_module command_with_identifier")
+        self.assertTrue(self.tool._cmd_has_identifier_param(cmd))
+
+        cmd = cmdparse.BindCmdParse("test_module command_without_identifier")
+        self.assertFalse(self.tool._cmd_has_identifier_param(cmd))
+
+        cmd = cmdparse.BindCmdParse("badmodule command_without_identifier")
+        self.assertFalse(self.tool._cmd_has_identifier_param(cmd))
+
+    def test_get_identifier_startswith(self):
+        hints = self.tool._get_identifier_startswith("/")
+        self.assertEqual([ 'foo/an_int', 'foo/a_list'], hints)
+
+        hints = self.tool._get_identifier_startswith("/foo/an")
+        self.assertEqual(['foo/an_int'], hints)
+
+        hints = self.tool._get_identifier_startswith("/bar")
+        self.assertEqual([], hints)
+
 class FakeBindCmdInterpreter(bindcmd.BindCmdInterpreter):
     def __init__(self):
         pass

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

@@ -807,13 +807,13 @@ class MultiConfigData:
                spec_part['named_set_item_spec']['item_type'] == 'named_set':
                 subslash = "/"
             values, status = self.get_value(item_name)
-            if len(values) > 0:
+            if values is not None and len(values) > 0:
                 return [ item_name + "/" + v + subslash for v in values.keys() ]
             else:
                 return [ item_name ]
         elif spec_part_is_list(spec_part):
             values, status = self.get_value(item_name)
-            if len(values) > 0:
+            if values is not None and len(values) > 0:
                 result = []
                 for i in range(len(values)):
                     name = item_name + '[%d]' % i