Repository: mercadona/rele

Issue:
Improve sub decorator
The `sub` decorator can get some ease of use improvements

1. Use [functools.wraps](https://docs.python.org/3/library/functools.html#functools.wraps) to preserve `__name__` and `__doc__` of the original function
2. [Inspect](https://docs.python.org/3/library/functools.html#functools.wraps) callback function signature and raise an exception if the signature is not compatible (ie. missing `data`)
3. Log a warning message if the function cannot be discovered (ie. no `sub` in the path)

Slightly related to the second point: `@sub("topic", filter_by=42)` will raise a `TypeError` at runtime (because filters are used as callables but they are not checked)

Discussion:

Patch:
diff --git a/.travis.yml b/.travis.yml
index a9fabde..5ad6106 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -13,6 +13,7 @@ python:
   - "3.6"
   - "3.7"
   - "3.8"
+  - "3.9-dev"
 
 install:
   - make install-requirements
diff --git a/rele/client.py b/rele/client.py
index 8ba1be1..bcb3af5 100644
--- a/rele/client.py
+++ b/rele/client.py
@@ -167,6 +167,9 @@ class Publisher:
             if raise_exception:
                 raise e
         else:
+            run_middleware_hook("post_publish_success", topic)
+
+            # DEPRECATED
             run_middleware_hook("post_publish", topic)
 
         return future
diff --git a/rele/config.py b/rele/config.py
index 67f97df..da3ae8e 100644
--- a/rele/config.py
+++ b/rele/config.py
@@ -87,7 +87,7 @@ def subscription_from_attribute(attribute):
 
 def load_subscriptions_from_paths(sub_module_paths, sub_prefix=None, filter_by=None):
 
-    subscriptions = []
+    subscriptions = {}
     for sub_module_path in sub_module_paths:
         sub_module = importlib.import_module(sub_module_path)
         for attr_name in dir(sub_module):
@@ -102,5 +102,13 @@ def load_subscriptions_from_paths(sub_module_paths, sub_prefix=None, filter_by=N
             if filter_by and not subscription.filter_by:
                 subscription.set_filters(filter_by)
 
-            subscriptions.append(subscription)
-    return subscriptions
+            if subscription.name in subscriptions:
+                found_subscription = subscriptions[subscription.name]
+                raise RuntimeError(
+                    f"Duplicate subscription name found: {subscription.name}. Subs "
+                    f"{subscription._func.__module__}.{subscription._func.__name__} and "
+                    f"{found_subscription._func.__module__}.{found_subscription._func.__name__} collide."
+                )
+
+            subscriptions[subscription.name] = subscription
+    return list(subscriptions.values())
diff --git a/rele/middleware.py b/rele/middleware.py
index 84fb8da..dceb84d 100644
--- a/rele/middleware.py
+++ b/rele/middleware.py
@@ -1,4 +1,5 @@
 import importlib
+import warnings
 
 _middlewares = []
 
@@ -24,7 +25,20 @@ def run_middleware_hook(hook_name, *args, **kwargs):
         getattr(middleware, hook_name)(*args, **kwargs)
 
 
-class BaseMiddleware:
+class WarnDeprecatedHooks(type):
+    def __new__(cls, *args, **kwargs):
+        x = super().__new__(cls, *args, **kwargs)
+        if hasattr(x, "post_publish"):
+            warnings.warn(
+                "The post_publish hook in the middleware is deprecated "
+                "and will be removed in future versions. Please substitute it with "
+                "the post_publish_success hook instead.",
+                DeprecationWarning,
+            )
+        return x
+
+
+class BaseMiddleware(metaclass=WarnDeprecatedHooks):
     """Base class for middleware.  The default implementations
     for all hooks are no-ops and subclasses may implement whatever
     subset of hooks they like.
@@ -43,6 +57,11 @@ class BaseMiddleware:
         """
 
     def post_publish(self, topic):
+        """DEPRECATED: Called after Publisher sends message.
+        :param topic:
+        """
+
+    def post_publish_success(self, topic):
         """Called after Publisher sends message.
         :param topic:
         """
diff --git a/rele/publishing.py b/rele/publishing.py
index e2bf1d4..3368548 100644
--- a/rele/publishing.py
+++ b/rele/publishing.py
@@ -1,3 +1,5 @@
+from rele import config, discover
+
 from .client import Publisher
 
 _publisher = None
@@ -41,5 +43,10 @@ def publish(topic, data, **kwargs):
     :return: None
     """
     if not _publisher:
-        raise ValueError("init_global_publisher must be called first.")
+        settings, _ = discover.sub_modules()
+        if not hasattr(settings, "RELE"):
+            raise ValueError("Config setup not called and settings module not found.")
+
+        config.setup(settings.RELE)
+
     _publisher.publish(topic, data, **kwargs)
diff --git a/rele/subscription.py b/rele/subscription.py
index 47ead4d..1819d54 100644
--- a/rele/subscription.py
+++ b/rele/subscription.py
@@ -1,6 +1,8 @@
 import json
 import logging
 import time
+from collections.abc import Iterable
+from inspect import getfullargspec, getmodule, signature
 
 from .middleware import run_middleware_hook
 
@@ -40,7 +42,16 @@ class Subscription:
         self._filters = self._init_filters(filter_by)
 
     def _init_filters(self, filter_by):
-        if hasattr(filter_by, "__iter__"):
+        if filter_by and not (
+            callable(filter_by)
+            or (
+                isinstance(filter_by, Iterable)
+                and all(callable(filter) for filter in filter_by)
+            )
+        ):
+            raise ValueError("Filter_by must be a callable or a list of callables.")
+
+        if isinstance(filter_by, Iterable):
             return filter_by
         elif filter_by:
             return [filter_by]
@@ -180,6 +191,19 @@ def sub(topic, prefix=None, suffix=None, filter_by=None):
     """
 
     def decorator(func):
+        args_spec = getfullargspec(func)
+        if len(args_spec.args) != 1 or not args_spec.varkw:
+            raise RuntimeError(
+                f"Subscription function {func.__module__}.{func.__name__} is not valid. "
+                "The function must have one argument and accept keyword arguments."
+            )
+
+        if getmodule(func).__name__.split(".")[-1] != "subs":
+            logger.warning(
+                f"Subscription function {func.__module__}.{func.__name__} is "
+                "outside a subs module that will not be discovered."
+            )
+
         return Subscription(
             func=func,
             topic=topic,


Test patch:
diff --git a/tests/test_config.py b/tests/test_config.py
index 00612f0..7a26be4 100644
--- a/tests/test_config.py
+++ b/tests/test_config.py
@@ -15,6 +15,11 @@ def sub_stub(data, **kwargs):
     return data["id"]
 
 
+@sub(topic="another-cool-topic", prefix="rele")
+def another_sub_stub(data, **kwargs):
+    return data["id"]
+
+
 class TestLoadSubscriptions:
     @pytest.fixture
     def subscriptions(self):
@@ -25,8 +30,8 @@ class TestLoadSubscriptions:
         )
 
     def test_load_subscriptions_in_a_module(self, subscriptions):
-        assert len(subscriptions) == 1
-        func_sub = subscriptions[0]
+        assert len(subscriptions) == 2
+        func_sub = subscriptions[-1]
         assert isinstance(func_sub, Subscription)
         assert func_sub.name == "rele-test-topic"
         assert func_sub({"id": 4}, lang="en") == 4
@@ -44,6 +49,16 @@ class TestLoadSubscriptions:
         assert klass_sub.name == "test-alternative-cool-topic"
         assert klass_sub({"id": 4}, lang="en") == 4
 
+    def test_raises_error_when_subscription_is_duplicated(self):
+        with pytest.raises(RuntimeError) as excinfo:
+            load_subscriptions_from_paths(["tests.test_config", "tests.more_subs.subs"])
+
+        assert (
+            str(excinfo.value)
+            == "Duplicate subscription name found: rele-another-cool-topic. Subs "
+            "tests.more_subs.subs.another_sub_stub and tests.test_config.another_sub_stub collide."
+        )
+
     def test_returns_sub_value_when_filtered_value_applied(self, subscriptions):
 
         assert subscriptions[-1]({"id": 4}, lang="en") == 4
diff --git a/tests/test_middleware.py b/tests/test_middleware.py
index b13364d..57037bc 100644
--- a/tests/test_middleware.py
+++ b/tests/test_middleware.py
@@ -3,6 +3,7 @@ from unittest.mock import patch
 import pytest
 
 import rele
+from rele.middleware import BaseMiddleware
 
 
 class TestMiddleware:
@@ -23,3 +24,11 @@ class TestMiddleware:
         rele.setup(settings, foo="bar")
         assert mock_middleware_setup.called
         assert mock_middleware_setup.call_args_list[0][-1] == {"foo": "bar"}
+
+    def test_warns_about_deprecated_hooks(self):
+
+        with pytest.warns(DeprecationWarning):
+
+            class TestMiddleware(BaseMiddleware):
+                def post_publish(self, topic):
+                    pass
diff --git a/tests/test_publishing.py b/tests/test_publishing.py
index b8bc32f..4c7d4f8 100644
--- a/tests/test_publishing.py
+++ b/tests/test_publishing.py
@@ -1,12 +1,28 @@
 from unittest.mock import MagicMock, patch
 
 import pytest
+from tests import settings
 
 from rele import Publisher, publishing
 
 
 class TestPublish:
-    def test_raises_when_global_publisher_does_not_exist(self):
+    @patch("rele.publishing.Publisher", autospec=True)
+    def test_instantiates_publisher_and_publishes_when_does_not_exist(
+        self, mock_publisher
+    ):
+        with patch("rele.publishing.discover") as mock_discover:
+            mock_discover.sub_modules.return_value = settings, []
+
+            message = {"foo": "bar"}
+            publishing.publish(topic="order-cancelled", data=message, myattr="hello")
+
+            mock_publisher.return_value.publish.assert_called_with(
+                "order-cancelled", {"foo": "bar"}, myattr="hello"
+            )
+
+    def test_raises_error_when_publisher_does_not_exists_and_settings_not_found(self):
+        publishing._publisher = None
         message = {"foo": "bar"}
 
         with pytest.raises(ValueError):
@@ -18,6 +34,7 @@ class TestInitGlobalPublisher:
     def test_creates_global_publisher_when_published_called(
         self, mock_publisher, config
     ):
+        publishing._publisher = None
         mock_publisher.return_value = MagicMock(spec=Publisher)
         publishing.init_global_publisher(config)
         message = {"foo": "bar"}
diff --git a/tests/test_subscription.py b/tests/test_subscription.py
index ab9c816..1689a27 100644
--- a/tests/test_subscription.py
+++ b/tests/test_subscription.py
@@ -111,6 +111,25 @@ class TestSubscription:
 
         assert response is None
 
+    def test_raises_error_when_filter_by_is_not_valid(self, caplog):
+        Subscription(
+            func=lambda x: None, topic="topic", prefix="rele", filter_by=lambda x: True
+        )
+        Subscription(
+            func=lambda x: None,
+            topic="topic",
+            prefix="rele",
+            filter_by=((lambda x: True),),
+        )
+
+        with pytest.raises(ValueError):
+            Subscription(func=lambda x: None, topic="topic", prefix="rele", filter_by=1)
+
+        with pytest.raises(ValueError):
+            Subscription(
+                func=lambda x: None, topic="topic", prefix="rele", filter_by=(1,)
+            )
+
 
 class TestCallback:
     @pytest.fixture(autouse=True)
@@ -308,3 +327,26 @@ class TestCallback:
 
         assert res == 123
         assert mock_close_old_connections.call_count == 2
+
+
+class TestDecorator:
+    def test_returns_subscription_when_callback_valid(self):
+        subscription = sub(topic="topic", prefix="rele")(lambda data, **kwargs: None)
+        assert isinstance(subscription, Subscription)
+
+    def test_raises_error_when_function_signature_is_not_valid(self):
+        with pytest.raises(RuntimeError):
+            sub(topic="topic", prefix="rele")(lambda: None)
+
+        with pytest.raises(RuntimeError):
+            sub(topic="topic", prefix="rele")(lambda data: None)
+
+        with pytest.raises(RuntimeError):
+            sub(topic="topic", prefix="rele")(lambda data, value=None: None)
+
+    def test_logs_warning_when_function_not_in_subs_module(self, caplog):
+        sub(topic="topic", prefix="rele")(lambda data, **kwargs: None)
+        assert (
+            "Subscription function tests.test_subscription.<lambda> is outside a subs "
+            "module that will not be discovered." in caplog.text
+        )