From 8e4ce65f723556a3bec8c3799d39e0bce14e6398 Mon Sep 17 00:00:00 2001
From: Mike McLean <mikem@redhat.com>
Date: Jun 15 2023 03:59:27 +0000
Subject: [PATCH 1/5] cli: streamline python/json options in call command


---

diff --git a/cli/koji_cli/commands.py b/cli/koji_cli/commands.py
index fc78c6d..59a2594 100644
--- a/cli/koji_cli/commands.py
+++ b/cli/koji_cli/commands.py
@@ -966,36 +966,70 @@ def handle_call(goptions, session, args):
         Note, that you can use global option --noauth for anonymous calls here"""
     usage = textwrap.dedent(usage)
     parser = OptionParser(usage=get_usage_str(usage))
-    parser.add_option("--python", action="store_true",
+    parser.add_option("-p", "--python", action="store_true",
                       help="Use python syntax for RPC parameter values")
     parser.add_option("--kwargs",
-                      help="Specify keyword arguments as a dictionary (implies --python)")
+                      help="Specify keyword arguments as a dictionary (implies --python or "
+                           "--json-input)")
+    parser.add_option("-j", "--json", action="store_true",
+                      help="Use JSON syntax for input and output")
+    parser.add_option("--json-input", action="store_true", help="Use JSON syntax for input")
     parser.add_option("--json-output", action="store_true", help="Use JSON syntax for output")
+    parser.add_option("-b", "--bare-strings", action="store_true",
+                      help="Treat invalid json/python as bare strings")
     (options, args) = parser.parse_args(args)
     if len(args) < 1:
         parser.error("Please specify the name of the XML-RPC method")
-    if options.kwargs:
+    if options.json:
+        options.json_input = True
+        options.json_output = True
+    if options.python and options.json_input:
+        parser.error('The --python option conflicts with using --json-input')
+    if options.kwargs and not options.json_input:
+        # for backwards compatibility, --python is implied
         options.python = True
     if options.python and ast is None:
         parser.error("The ast module is required to read python syntax")
-    if options.json_output and json is None:
-        parser.error("The json module is required to output JSON syntax")
-    activate_session(session, goptions)
+    if (options.json_output or options.json_input) and json is None:
+        parser.error("The json module is required to use JSON syntax")
+
+    def parse_arg(arg):
+        try:
+            if options.python:
+                return ast.literal_eval(arg)
+            elif options.json_input:
+                return json.loads(arg)
+            else:
+                return arg_filter(arg)
+        except ValueError as e:
+            if options.bare_strings:
+                return arg
+            else:
+                parser.error(f"Invalid value: {arg!r}")
+
+    # the method to call
     name = args[0]
-    non_kw = []
+
+    # base kw args
+    # we update with name=value args later
     kw = {}
-    if options.python:
-        non_kw = [ast.literal_eval(a) for a in args[1:]]
-        if options.kwargs:
-            kw = ast.literal_eval(options.kwargs)
-    else:
-        for arg in args[1:]:
-            if arg.find('=') != -1:
-                key, value = arg.split('=', 1)
-                kw[key] = arg_filter(value)
-            else:
-                non_kw.append(arg_filter(arg))
+    if options.kwargs:
+        kw = parse_arg(options.kwargs)
+
+    # read the args
+    non_kw = []
+    for arg in args[1:]:
+        if arg.find('=') != -1:
+            key, value = arg.split('=', 1)
+            kw[key] = parse_arg(value)
+        else:
+            non_kw.append(parse_arg(arg))
+
+    # make the call
+    activate_session(session, goptions)
     response = getattr(session, name).__call__(*non_kw, **kw)
+
+    # print the result
     if options.json_output:
         print(json.dumps(response, indent=2, separators=(',', ': '), cls=DatetimeJSONEncoder))
     else:

From ab8ddbd1b09bc375c54e6cd583c56952c0b69b60 Mon Sep 17 00:00:00 2001
From: Mike McLean <mikem@redhat.com>
Date: Jun 15 2023 03:59:27 +0000
Subject: [PATCH 2/5] unit tests


---

diff --git a/tests/test_cli/test_call.py b/tests/test_cli/test_call.py
index 52c2ea4..9c89b04 100644
--- a/tests/test_cli/test_call.py
+++ b/tests/test_cli/test_call.py
@@ -97,7 +97,7 @@ Note, that you can use global option --noauth for anonymous calls here
 
         module = {
             'ast': "The ast module is required to read python syntax",
-            'json': "The json module is required to output JSON syntax",
+            'json': "The json module is required to use JSON syntax",
         }
 
         for mod, msg in module.items():
@@ -119,11 +119,14 @@ Note, that you can use global option --noauth for anonymous calls here
 (Specify the --help global option for a list of other help options)
 
 Options:
-  -h, --help       show this help message and exit
-  --python         Use python syntax for RPC parameter values
-  --kwargs=KWARGS  Specify keyword arguments as a dictionary (implies
-                   --python)
-  --json-output    Use JSON syntax for output
+  -h, --help          show this help message and exit
+  -p, --python        Use python syntax for RPC parameter values
+  --kwargs=KWARGS     Specify keyword arguments as a dictionary (implies
+                      --python or --json-input)
+  -j, --json          Use JSON syntax for input and output
+  --json-input        Use JSON syntax for input
+  --json-output       Use JSON syntax for output
+  -b, --bare-strings  Treat invalid json/python as bare strings
 """ % self.progname)
 
 

From 9121dd97a84933bd3796634f7195086ccee5428b Mon Sep 17 00:00:00 2001
From: Mike McLean <mikem@redhat.com>
Date: Jun 15 2023 03:59:27 +0000
Subject: [PATCH 3/5] additional unit tests


---

diff --git a/tests/test_cli/test_call.py b/tests/test_cli/test_call.py
index 9c89b04..aca2ca3 100644
--- a/tests/test_cli/test_call.py
+++ b/tests/test_cli/test_call.py
@@ -58,6 +58,49 @@ Note, that you can use global option --noauth for anonymous calls here
         self.assert_console_message(stdout, "'%s'\n" % response[1])
 
     @mock.patch('sys.stdout', new_callable=six.StringIO)
+    def test_handle_call_json_syntax(self, stdout):
+        """Test handle_call with json input syntax"""
+        response = ["SUCCESS", "FAKE-RESPONSE"]
+        self.session.ssl_login.return_value = response[1]
+
+        # Invalid json syntax
+        arguments = ['--json-input', 'ssl_login', 'cert=/etc/pki/cert']
+        self.assert_system_exit(
+            handle_call,
+            self.options, self.session, arguments,
+            stderr=self.format_error_message("Invalid value: '/etc/pki/cert'"),
+            activate_session=None)
+        self.activate_session_mock.assert_not_called()
+
+        # Incompatible opts
+        arguments = ['--json', '--python', 'ssl_login', 'cert=/etc/pki/cert']
+        self.assert_system_exit(
+            handle_call,
+            self.options, self.session, arguments,
+            stderr=self.format_error_message("The --python option conflicts with using --json-input"),
+            activate_session=None)
+        self.activate_session_mock.assert_not_called()
+
+        arguments = ['--json-input', 'ssl_login', '--kwargs', '{"cert":"/etc/pki/cert"}']
+        handle_call(self.options, self.session, arguments)
+        self.activate_session_mock.assert_called_with(self.session, self.options)
+        self.session.ssl_login.assert_called_with(cert='/etc/pki/cert')
+        self.assert_console_message(stdout, "'%s'\n" % response[1])
+
+    @mock.patch('sys.stdout', new_callable=six.StringIO)
+    def test_handle_call_bare_strings(self, stdout):
+        """Test handle_call with bare string fallback"""
+        response = ["SUCCESS", "FAKE-RESPONSE"]
+        self.session.ssl_login.return_value = response[1]
+
+        # Invalid json syntax, but with bare-string fallback
+        arguments = ['--json-input', '--bare-strings', 'ssl_login', 'cert=/etc/pki/cert']
+        handle_call(self.options, self.session, arguments)
+        self.activate_session_mock.assert_called_with(self.session, self.options)
+        self.session.ssl_login.assert_called_with(cert='/etc/pki/cert')
+        self.assert_console_message(stdout, "'%s'\n" % response[1])
+
+    @mock.patch('sys.stdout', new_callable=six.StringIO)
     def test_handle_call_json_output(self, stdout):
         """Test handle_call with json output"""
         arguments = ['ssl_login', 'cert=/etc/pki/cert', '--json-output']

From 1e71e0e0a2cd67e6aa1c3f4d95b2810d3c35fd82 Mon Sep 17 00:00:00 2001
From: Mike McLean <mikem@redhat.com>
Date: Jun 15 2023 14:16:45 +0000
Subject: [PATCH 4/5] be pickier about keyword args and add a note about them in usage


---

diff --git a/cli/koji_cli/commands.py b/cli/koji_cli/commands.py
index 59a2594..7604460 100644
--- a/cli/koji_cli/commands.py
+++ b/cli/koji_cli/commands.py
@@ -963,6 +963,7 @@ def handle_call(goptions, session, args):
     usage = """\
         usage: %prog call [options] <name> [<arg> ...]
 
+        <arg> values of the form NAME=VALUE are treated as keyword arguments
         Note, that you can use global option --noauth for anonymous calls here"""
     usage = textwrap.dedent(usage)
     parser = OptionParser(usage=get_usage_str(usage))
@@ -1016,11 +1017,14 @@ def handle_call(goptions, session, args):
     if options.kwargs:
         kw = parse_arg(options.kwargs)
 
+    kw_pat = re.compile(r'^([^\W0-9]\w*)=(.*)$')
+
     # read the args
     non_kw = []
     for arg in args[1:]:
-        if arg.find('=') != -1:
-            key, value = arg.split('=', 1)
+        m = kw_pat.match(arg)
+        if m:
+            key, value = m.groups()
             kw[key] = parse_arg(value)
         else:
             non_kw.append(parse_arg(arg))
diff --git a/tests/test_cli/test_call.py b/tests/test_cli/test_call.py
index aca2ca3..727fefc 100644
--- a/tests/test_cli/test_call.py
+++ b/tests/test_cli/test_call.py
@@ -22,6 +22,7 @@ class TestCall(utils.CliTestCase):
         self.activate_session_mock = mock.patch('koji_cli.commands.activate_session').start()
         self.error_format = """Usage: %s call [options] <name> [<arg> ...]
 
+<arg> values of the form NAME=VALUE are treated as keyword arguments
 Note, that you can use global option --noauth for anonymous calls here
 (Specify the --help global option for a list of other help options)
 
@@ -158,6 +159,7 @@ Note, that you can use global option --noauth for anonymous calls here
             handle_call,
             """Usage: %s call [options] <name> [<arg> ...]
 
+<arg> values of the form NAME=VALUE are treated as keyword arguments
 Note, that you can use global option --noauth for anonymous calls here
 (Specify the --help global option for a list of other help options)
 

From 766973ab3e101cefd7afe1da4506a871706064c6 Mon Sep 17 00:00:00 2001
From: Mike McLean <mikem@redhat.com>
Date: Jun 26 2023 14:37:50 +0000
Subject: [PATCH 5/5] avoid using fstrings in client code


---

diff --git a/cli/koji_cli/commands.py b/cli/koji_cli/commands.py
index 7604460..cc694e4 100644
--- a/cli/koji_cli/commands.py
+++ b/cli/koji_cli/commands.py
@@ -1006,7 +1006,7 @@ def handle_call(goptions, session, args):
             if options.bare_strings:
                 return arg
             else:
-                parser.error(f"Invalid value: {arg!r}")
+                parser.error("Invalid value: %r" % arg)
 
     # the method to call
     name = args[0]

