Fix slackapi#31 by adding app.error handler (slackapi#43)

jlcjohns · Aug 26, 2020 · dd855e1 · dd855e1
1 parent 059d375
commit dd855e1
Show file tree

Hide file tree

Showing 14 changed files with 592 additions and 37 deletions.
diff --git a/.travis.yml b/.travis.yml
@@ -20,4 +20,4 @@ script:
   # run all tests just in case
   - travis_retry python setup.py test
   # Run pytype only for Python 3.8
-  - if [ ${TRAVIS_PYTHON_VERSION:0:3} == "3.8" ]; then pip install -e ".[adapter]"; pytype slack_bolt/; fi
+  - if [ ${TRAVIS_PYTHON_VERSION:0:3} == "3.8" ]; then pip install -e ".[adapter]" && pytype slack_bolt/; fi
diff --git a/samples/app.py b/samples/app.py
@@ -26,6 +26,12 @@ def event_test(payload, say, logger):
     say("What's up?")
 
 
+@app.error
+def global_error_handler(error, payload, logger):
+    logger.exception(error)
+    logger.info(payload)
+
+
 if __name__ == "__main__":
     app.start(3000)
 

diff --git a/scripts/install_all_and_run_tests.sh b/scripts/install_all_and_run_tests.sh
@@ -0,0 +1,21 @@
+#!/bin/bash
+# Run all the tests or a single test
+# all: ./scripts/install_all_and_run_tests.sh
+# single: ./scripts/install_all_and_run_tests.sh tests/scenario_tests/test_app.py
+
+script_dir=`dirname $0`
+cd ${script_dir}/..
+
+test_target="$1"
+
+if [[ $test_target != "" ]]
+then
+  pip install -e ".[testing]" && \
+    black slack_bolt/ tests/ && \
+    pytest $1
+else
+  pip install -e ".[testing]" && \
+    black slack_bolt/ tests/ && \
+    pytest && \
+    pytype slack_bolt/
+fi
diff --git a/scripts/run_pytype.sh b/scripts/run_pytype.sh
@@ -0,0 +1,6 @@
+#!/bin/bash
+# ./scripts/run_pytype.sh
+
+script_dir=$(dirname $0)
+cd ${script_dir}/..
+pip install -e ".[adapter]" && pytype slack_bolt/
diff --git a/scripts/run_tests.sh b/scripts/run_tests.sh
@@ -10,12 +10,10 @@ test_target="$1"
 
 if [[ $test_target != "" ]]
 then
-  pip install -e ".[testing]" && \
-    black slack_bolt/ tests/ && \
+  black slack_bolt/ tests/ && \
     pytest $1
 else
-  pip install -e ".[testing]" && \
-    black slack_bolt/ tests/ && \
+  black slack_bolt/ tests/ && \
     pytest && \
     pytype slack_bolt/
 fi
diff --git a/slack_bolt/app/app.py b/slack_bolt/app/app.py
@@ -15,6 +15,11 @@
 from slack_bolt.error import BoltError
 from slack_bolt.listener.custom_listener import CustomListener
 from slack_bolt.listener.listener import Listener
+from slack_bolt.listener.listener_error_handler import (
+    ListenerErrorHandler,
+    DefaultListenerErrorHandler,
+    CustomListenerErrorHandler,
+)
 from slack_bolt.listener_matcher import CustomListenerMatcher
 from slack_bolt.listener_matcher import builtins as builtin_matchers
 from slack_bolt.listener_matcher.listener_matcher import ListenerMatcher
@@ -180,6 +185,9 @@ def __init__(
         self._middleware_list: List[Union[Callable, Middleware]] = []
         self._listeners: List[Listener] = []
         self._listener_executor = ThreadPoolExecutor(max_workers=5)  # TODO: shutdown
+        self._listener_error_handler = DefaultListenerErrorHandler(
+            logger=self._framework_logger
+        )
         self._process_before_response = process_before_response
 
         self._init_middleware_list_done = False
@@ -238,6 +246,10 @@ def installation_store(self) -> Optional[InstallationStore]:
     def oauth_state_store(self) -> Optional[OAuthStateStore]:
         return self._oauth_state_store
 
+    @property
+    def listener_error_handler(self) -> ListenerErrorHandler:
+        return self._listener_error_handler
+
     # -------------------------
     # standalone server
 
@@ -301,13 +313,24 @@ def run_listener(
         ack = request.context.ack
         starting_time = time.time()
         if self._process_before_response:
-            returned_value = listener.run_ack_function(
-                request=request, response=response
-            )
-            if isinstance(returned_value, BoltResponse):
-                response = returned_value
-            if ack.response is None and listener.auto_acknowledgement:
-                ack()  # automatic ack() call if the call is not yet done
+            try:
+                returned_value = listener.run_ack_function(
+                    request=request, response=response
+                )
+                if isinstance(returned_value, BoltResponse):
+                    response = returned_value
+                if ack.response is None and listener.auto_acknowledgement:
+                    ack()  # automatic ack() call if the call is not yet done
+            except Exception as e:
+                # The default response status code is 500 in this case.
+                # You can customize this by passing your own error handler.
+                if response is None:
+                    response = BoltResponse(status=500)
+                response.status = 500
+                self._listener_error_handler.handle(
+                    error=e, request=request, response=response,
+                )
+                ack.response = response
 
             if response is not None:
                 self._debug_log_completion(starting_time, response)
@@ -318,13 +341,22 @@ def run_listener(
         else:
             # start the listener function asynchronously
             def run_ack_function_asynchronously():
+                nonlocal ack, request, response
                 try:
                     listener.run_ack_function(request=request, response=response)
                 except Exception as e:
-                    # TODO: error handler
-                    self._framework_logger.exception(
-                        f"Failed to run listener function (error: {e})"
+                    # The default response status code is 500 in this case.
+                    # You can customize this by passing your own error handler.
+                    if response is None:
+                        response = BoltResponse(status=500)
+                    response.status = 500
+                    if ack.response is not None:  # already acknowledged
+                        response = None
+
+                    self._listener_error_handler.handle(
+                        error=e, request=request, response=response,
                     )
+                    ack.response = response
 
             self._listener_executor.submit(run_ack_function_asynchronously)
 
@@ -336,12 +368,17 @@ def run_ack_function_asynchronously():
                 while ack.response is None and time.time() - starting_time <= 3:
                     time.sleep(0.01)
 
+            if response is None and ack.response is None:
+                self._framework_logger.warning(f"{listener_name} didn't call ack()")
+                return None
+
             if response is None and ack.response is not None:
                 response = ack.response
                 self._debug_log_completion(starting_time, response)
                 return response
-            else:
-                self._framework_logger.warning(f"{listener_name} didn't call ack()")
+
+            if response is not None:
+                return response
 
         # None for both means no ack() in the listener
         return None
@@ -367,6 +404,16 @@ def middleware(self, *args):
                 CustomMiddleware(app_name=self.name, func=func)
             )
 
+    # -------------------------
+    # global error handler
+
+    def error(self, *args):
+        if len(args) > 0:
+            func = args[0]
+            self._listener_error_handler = CustomListenerErrorHandler(
+                logger=self._framework_logger, func=func,
+            )
+
     # -------------------------
     # events
 

diff --git a/slack_bolt/app/async_app.py b/slack_bolt/app/async_app.py
@@ -15,8 +15,14 @@
 from slack_sdk.oauth.state_store.async_state_store import AsyncOAuthStateStore
 from slack_sdk.web.async_client import AsyncWebClient
 
+from slack_bolt.context.ack.async_ack import AsyncAck
 from slack_bolt.error import BoltError
 from slack_bolt.listener.async_listener import AsyncListener, AsyncCustomListener
+from slack_bolt.listener.async_listener_error_handler import (
+    AsyncDefaultListenerErrorHandler,
+    AsyncListenerErrorHandler,
+    AsyncCustomListenerErrorHandler,
+)
 from slack_bolt.listener_matcher import builtins as builtin_matchers
 from slack_bolt.listener_matcher.async_listener_matcher import (
     AsyncListenerMatcher,
@@ -199,6 +205,9 @@ def __init__(
 
         self._async_middleware_list: List[Union[Callable, AsyncMiddleware]] = []
         self._async_listeners: List[AsyncListener] = []
+        self._async_listener_error_handler = AsyncDefaultListenerErrorHandler(
+            logger=self._framework_logger
+        )
         self._process_before_response = process_before_response
 
         self._init_middleware_list_done = False
@@ -256,6 +265,10 @@ def installation_store(self) -> Optional[AsyncInstallationStore]:
     def oauth_state_store(self) -> Optional[AsyncOAuthStateStore]:
         return self._async_oauth_state_store
 
+    @property
+    def listener_error_handler(self) -> AsyncListenerErrorHandler:
+        return self._async_listener_error_handler
+
     # -------------------------
     # standalone server
 
@@ -325,13 +338,24 @@ async def run_async_listener(
         ack = request.context.ack
         starting_time = time.time()
         if self._process_before_response:
-            returned_value = await async_listener.run_ack_function(
-                request=request, response=response
-            )
-            if isinstance(returned_value, BoltResponse):
-                response = returned_value
-            if ack.response is None and async_listener.auto_acknowledgement:
-                await ack()  # automatic ack() call if the call is not yet done
+            try:
+                returned_value = await async_listener.run_ack_function(
+                    request=request, response=response
+                )
+                if isinstance(returned_value, BoltResponse):
+                    response = returned_value
+                if ack.response is None and async_listener.auto_acknowledgement:
+                    await ack()  # automatic ack() call if the call is not yet done
+            except Exception as e:
+                # The default response status code is 500 in this case.
+                # You can customize this by passing your own error handler.
+                if response is None:
+                    response = BoltResponse(status=500)
+                response.status = 500
+                await self._async_listener_error_handler.handle(
+                    error=e, request=request, response=response,
+                )
+                ack.response = response
 
             if response is not None:
                 self._debug_log_completion(starting_time, response)
@@ -347,33 +371,46 @@ async def run_async_listener(
             # start the listener function asynchronously
             # NOTE: intentionally
             async def run_ack_function_asynchronously(
-                request: AsyncBoltRequest, response: BoltResponse,
+                ack: AsyncAck, request: AsyncBoltRequest, response: BoltResponse,
             ):
                 try:
                     await async_listener.run_ack_function(
                         request=request, response=response
                     )
                 except Exception as e:
-                    # TODO: error handler
-                    self._framework_logger.exception(
-                        f"Failed to run listener function (error: {e})"
+                    # The default response status code is 500 in this case.
+                    # You can customize this by passing your own error handler.
+                    if response is None:
+                        response = BoltResponse(status=500)
+                    response.status = 500
+                    if ack.response is not None:  # already acknowledged
+                        response = None
+
+                    await self._async_listener_error_handler.handle(
+                        error=e, request=request, response=response,
                     )
+                    ack.response = response
 
             _f: Future = asyncio.ensure_future(
-                run_ack_function_asynchronously(request, response)
+                run_ack_function_asynchronously(ack, request, response)
             )
             self._framework_logger.debug(f"Async listener: {listener_name} started..")
 
             # await for the completion of ack() in the async listener execution
             while ack.response is None and time.time() - starting_time <= 3:
                 await asyncio.sleep(0.01)
 
-            if ack.response is not None:
+            if response is None and ack.response is None:
+                self._framework_logger.warning(f"{listener_name} didn't call ack()")
+                return None
+
+            if response is None and ack.response is not None:
                 response = ack.response
                 self._debug_log_completion(starting_time, response)
                 return response
-            else:
-                self._framework_logger.warning(f"{listener_name} didn't call ack()")
+
+            if response is not None:
+                return response
 
         # None for both means no ack() in the listener
         return None
@@ -399,6 +436,16 @@ def middleware(self, *args):
                 AsyncCustomMiddleware(app_name=self.name, func=func)
             )
 
+    # -------------------------
+    # global error handler
+
+    def error(self, *args):
+        if len(args) > 0:
+            func = args[0]
+            self._async_listener_error_handler = AsyncCustomListenerErrorHandler(
+                logger=self._framework_logger, func=func,
+            )
+
     # -------------------------
     # events
 

diff --git a/slack_bolt/context/async_context.py b/slack_bolt/context/async_context.py
@@ -1,12 +1,18 @@
 from typing import Optional
 
+from slack_sdk.web.async_client import AsyncWebClient
+
 from slack_bolt.context.ack.async_ack import AsyncAck
 from slack_bolt.context.base_context import BaseContext
 from slack_bolt.context.respond.async_respond import AsyncRespond
 from slack_bolt.context.say.async_say import AsyncSay
 
 
 class AsyncBoltContext(BaseContext):
+    @property
+    def client(self) -> Optional[AsyncWebClient]:
+        return self.get("client", None)
+
     @property
     def ack(self) -> AsyncAck:
         if "ack" not in self:

diff --git a/slack_bolt/context/base_context.py b/slack_bolt/context/base_context.py
@@ -2,7 +2,6 @@
 from typing import Optional, Tuple
 
 from slack_bolt.auth import AuthorizationResult
-from slack_sdk import WebClient
 
 
 class BaseContext(dict):
@@ -18,10 +17,6 @@ def logger(self) -> Logger:
     def token(self) -> Optional[str]:
         return self.get("token", None)
 
-    @property
-    def client(self) -> Optional[WebClient]:
-        return self.get("client", None)
-
     @property
     def enterprise_id(self) -> Optional[str]:
         return self.get("enterprise_id", None)

diff --git a/slack_bolt/context/context.py b/slack_bolt/context/context.py
@@ -1,13 +1,19 @@
 # pytype: skip-file
 from typing import Optional
 
+from slack_sdk import WebClient
+
 from slack_bolt.context.ack import Ack
 from slack_bolt.context.base_context import BaseContext
 from slack_bolt.context.respond import Respond
 from slack_bolt.context.say import Say
 
 
 class BoltContext(BaseContext):
+    @property
+    def client(self) -> Optional[WebClient]:
+        return self.get("client", None)
+
     @property
     def ack(self) -> Ack:
         if "ack" not in self: