Add a "--callback-file" option

Previously, the callback needed to reside on the Python Path, which is
convenient if you're already making a Python package. Plenty of folks
aren't, though, so allow users to provide a file that we'll boldly exec
and load a callback from.

Fixes fedora-infra#159

Signed-off-by: Jeremy Cline <jcline@redhat.com>

fedora_messaging/cli.py

@@ -51,6 +51,13 @@
     "and should point to either a function or a class. Consult the API "
     "documentation for the interface required for these objects."
 )
+_callback_file_help = (
+    "The path of a Python file that contains the callable object to"
+    " execute when the message arrives. This should be in the format "
+    '"my/script.py:object_in_file". Providing this overrides the callback set '
+    'by the "--callback" argument or configuration file. It executes the entire '
+    "Python file to load the callable object."
+)
 _routing_key_help = (
     "The AMQP routing key to use with the queue. This controls what messages are "
     "delivered to the consumer. Can be specified multiple times; any message "
@@ -91,11 +98,12 @@ def cli(conf):
 
 @cli.command()
 @click.option("--app-name", help=_app_name_help)
+@click.option("--callback-file", help=_callback_file_help)
 @click.option("--callback", help=_callback_help)
 @click.option("--routing-key", help=_routing_key_help, multiple=True)
 @click.option("--queue-name", help=_queue_name_help)
 @click.option("--exchange", help=_exchange_help)
-def consume(exchange, queue_name, routing_key, callback, app_name):
+def consume(exchange, queue_name, routing_key, callback, callback_file, app_name):
     """Consume messages from an AMQP queue using a Python callback."""
     # The configuration validates these are not null and contain all required keys
     # when it is loaded.
@@ -123,6 +131,94 @@ def consume(exchange, queue_name, routing_key, callback, app_name):
         for binding in bindings:
             binding["routing_keys"] = routing_key
 
+    if callback_file:
+        callback = _callback_from_filesystem(callback_file)
+    else:
+        callback = _callback_from_python_path(callback)
+
+    if app_name:
+        config.conf["client_properties"]["app"] = app_name
+
+    try:
+        deferred_consumers = api.twisted_consume(
+            callback, bindings=bindings, queues=queues
+        )
+        deferred_consumers.addCallback(_consume_callback)
+        deferred_consumers.addErrback(_consume_errback)
+    except ValueError as e:
+        click_version = pkg_resources.get_distribution("click").parsed_version
+        if click_version < pkg_resources.parse_version("7.0"):
+            raise click.exceptions.BadOptionUsage(str(e))
+        else:
+            raise click.exceptions.BadOptionUsage("callback", str(e))
+
+    reactor.run()
+    sys.exit(_exit_code)
+
+
+def _callback_from_filesystem(callback_file):
+    """
+    Load a callable from a Python script on the file system.
+
+    Args:
+        callback_file (str): The callback as a filesystem path and callable name
+            separated by a ":". For example, "my/python/file.py:printer".
+
+    Raises:
+        click.ClickException: If the object cannot be loaded.
+
+    Returns:
+        callable: The callable object.
+    """
+    try:
+        file_path, callable_name = callback_file.strip().split(":")
+    except ValueError:
+        raise click.ClickException(
+            "Unable to parse the '--callback-file' option; the "
+            'expected format is "path/to/file.py:callable_object" where '
+            '"callable_object" is the name of the function or class in the '
+            "Python file"
+        )
+
+    try:
+        file_namespace = {}
+        with open(file_path, "rb") as fd:
+            try:
+                # Using "exec" is generally a Bad Idea (TM), so bandit is upset at
+                # us. In this case, it seems like a Good Idea (TM), but I might be
+                # wrong. Sorry.
+                exec(compile(fd.read(), file_path, "exec"), file_namespace)  # nosec
+            except Exception as e:
+                raise click.ClickException(
+                    "The {} file raised the following exception during execution:"
+                    " {}".format(file_path, str(e))
+                )
+
+        if callable_name not in file_namespace:
+            err = "The '{}' object was not found in the '{}' file.".format(
+                callable_name, file_path
+            )
+            raise click.ClickException(err)
+        else:
+            return file_namespace[callable_name]
+    except IOError as e:
+        raise click.ClickException("An IO error occurred: {}".format(str(e)))
+
+
+def _callback_from_python_path(callback):
+    """
+    Load a callable from a Python path.
+
+    Args:
+        callback_file (str): The callback as a Python path and callable name
+            separated by a ":". For example, "my_package.my_module:printer".
+
+    Raises:
+        click.ClickException: If the object cannot be loaded.
+
+    Returns:
+        callable: The callable object.
+    """
     callback_path = callback or config.conf["callback"]
     if not callback_path:
         raise click.ClickException(
@@ -148,34 +244,16 @@ def consume(exchange, queue_name, routing_key, callback, app_name):
         )
 
     try:
-        callback = getattr(module, cls)
+        callback_object = getattr(module, cls)
     except AttributeError as e:
         raise click.ClickException(
             "Unable to import {} ({}); is the package installed? The python path should "
             'be in the format "my_package.module:callable_object"'.format(
                 callback_path, str(e)
             )
         )
-
-    if app_name:
-        config.conf["client_properties"]["app"] = app_name
-
     _log.info("Starting consumer with %s callback", callback_path)
-    try:
-        deferred_consumers = api.twisted_consume(
-            callback, bindings=bindings, queues=queues
-        )
-        deferred_consumers.addCallback(_consume_callback)
-        deferred_consumers.addErrback(_consume_errback)
-    except ValueError as e:
-        click_version = pkg_resources.get_distribution("click").parsed_version
-        if click_version < pkg_resources.parse_version("7.0"):
-            raise click.exceptions.BadOptionUsage(str(e))
-        else:
-            raise click.exceptions.BadOptionUsage("callback", str(e))
-
-    reactor.run()
-    sys.exit(_exit_code)
+    return callback_object
 
 
 def _consume_errback(failure):

fedora_messaging/tests/fixtures/bad_cb

@@ -0,0 +1 @@
+not a valid python file

fedora_messaging/tests/fixtures/callback.py

@@ -0,0 +1,2 @@
+def rand(message):
+    return 4

fedora_messaging/tests/unit/test_cli.py

@@ -23,6 +23,7 @@
 from click.testing import CliRunner
 from twisted.internet import error
 from twisted.python import failure
+import click
 import mock
 
 from fedora_messaging import cli, config, exceptions
@@ -444,3 +445,65 @@ def test_errback_general_exception(self, mock_reactor):
         cli._consume_errback(f)
 
         self.assertEqual(11, cli._exit_code)
+
+
+class CallbackFromFilesytem(unittest.TestCase):
+    """Unit tests for :func:`fedora_messaging.cli._callback_from_filesystem`."""
+
+    def test_good_callback(self):
+        """Assert loading a callback from a file works."""
+        cb = cli._callback_from_filesystem(
+            os.path.join(FIXTURES_DIR, "callback.py") + ":rand"
+        )
+        self.assertEqual(4, cb(None))
+
+    def test_bad_format(self):
+        """Assert an exception is raised if the format is bad."""
+        with self.assertRaises(click.ClickException) as cm:
+            cli._callback_from_filesystem("file/with/no/function.py")
+
+        self.assertEqual(
+            "Unable to parse the '--callback-file' option; the "
+            'expected format is "path/to/file.py:callable_object" where '
+            '"callable_object" is the name of the function or class in the '
+            "Python file",
+            cm.exception.message,
+        )
+
+    def test_invalid_file(self):
+        """Assert an exception is raised if the Python file can't be executed."""
+        with self.assertRaises(click.ClickException) as cm:
+            cli._callback_from_filesystem(
+                os.path.join(FIXTURES_DIR, "bad_cb") + ":missing"
+            )
+
+        self.assertEqual(
+            "The {} file raised the following exception during execution: "
+            "invalid syntax (bad_cb, line 1)".format(
+                os.path.join(FIXTURES_DIR, "bad_cb")
+            ),
+            cm.exception.message,
+        )
+
+    def test_callable_does_not_exist(self):
+        """Assert an exception is raised if the callable is missing."""
+        with self.assertRaises(click.ClickException) as cm:
+            cli._callback_from_filesystem(
+                os.path.join(FIXTURES_DIR, "callback.py") + ":missing"
+            )
+
+        self.assertEqual(
+            "The 'missing' object was not found in the '{}' file."
+            "".format(os.path.join(FIXTURES_DIR, "callback.py")),
+            cm.exception.message,
+        )
+
+    def test_file_does_not_exist(self):
+        """Assert an exception is raised if the file doesn't exist."""
+        with self.assertRaises(click.ClickException) as cm:
+            cli._callback_from_filesystem("file/that/is/missing.py:callable")
+
+        self.assertEqual(
+            "An IO error occurred: [Errno 2] No such file or directory: 'file/that/is/missing.py'",
+            cm.exception.message,
+        )

news/159.feature

@@ -0,0 +1,3 @@
+"fedora-messaging consume" now accepts a "--callback-file" argument which will
+load a callback function from an arbitrary Python file. Previously, it was
+required that the callback be in the Python path.