Repository: RasaHQ/rasa
Pull request: Fix callback resp
**Proposed changes**:
- fix https://github.com/RasaHQ/rasa/issues/5320
- remove unused `return_method` kwarg
- gracefully handle response types other than json instead of raising an error

**Status (please check what you already did)**:
- [x] added some tests for the functionality
- [ ] updated the documentation
- [x] updated the changelog (please check [changelog](https://github.com/RasaHQ/rasa/tree/master/changelog) for instructions)
- [x] reformat files using `black` (please check [Readme](https://github.com/RasaHQ/rasa#code-style) for instructions)


Problem statement:
callback post only expects json response, fails on others
If you use a callback channel to post to a slack incoming webhook, you get a traceback:
```
2020-02-07 10:12:19 DEBUG    rasa.server  - Traceback (most recent call last):
  File "/build/lib/python3.6/site-packages/rasa/server.py", line 663, in trigger_intent
    tracker=tracker,
  File "/usr/local/lib/python3.6/asyncio/coroutines.py", line 110, in __next__
    return self.gen.send(None)
  File "/build/lib/python3.6/site-packages/rasa/core/agent.py", line 535, in trigger_intent
    intent_name, entities, tracker, output_channel,
  File "/usr/local/lib/python3.6/asyncio/coroutines.py", line 110, in __next__
    return self.gen.send(None)
  File "/build/lib/python3.6/site-packages/rasa/core/processor.py", line 378, in trigger_external_user_uttered
    await self._predict_and_execute_next_action(output_channel, tracker)
  File "/usr/local/lib/python3.6/asyncio/coroutines.py", line 110, in __next__
    return self.gen.send(None)
  File "/build/lib/python3.6/site-packages/rasa/core/processor.py", line 509, in _predict_and_execute_next_action
    action, tracker, output_channel, self.nlg, policy, confidence
  File "/usr/local/lib/python3.6/asyncio/coroutines.py", line 110, in __next__
    return self.gen.send(None)
  File "/build/lib/python3.6/site-packages/rasa/core/processor.py", line 620, in _run_action
    await self._send_bot_messages(events, tracker, output_channel)
  File "/usr/local/lib/python3.6/asyncio/coroutines.py", line 110, in __next__
    return self.gen.send(None)
  File "/build/lib/python3.6/site-packages/rasa/core/processor.py", line 549, in _send_bot_messages
    await output_channel.send_response(tracker.sender_id, e.message())
  File "/usr/local/lib/python3.6/asyncio/coroutines.py", line 110, in __next__
    return self.gen.send(None)
  File "/build/lib/python3.6/site-packages/rasa/core/channels/channel.py", line 186, in send_response
    await self.send_text_message(recipient_id, message.pop("text"), **message)
  File "/usr/local/lib/python3.6/asyncio/coroutines.py", line 110, in __next__
    return self.gen.send(None)
  File "/build/lib/python3.6/site-packages/rasa/core/channels/channel.py", line 328, in send_text_message
    await self._persist_message(self._message(recipient_id, text=message_part))
  File "/usr/local/lib/python3.6/asyncio/coroutines.py", line 110, in __next__
    return self.gen.send(None)
  File "/build/lib/python3.6/site-packages/rasa/core/channels/callback.py", line 34, in _persist_message
    "post", content_type="application/json", json=message
  File "/usr/local/lib/python3.6/asyncio/coroutines.py", line 110, in __next__
    return self.gen.send(None)
  File "/build/lib/python3.6/site-packages/rasa/utils/endpoints.py", line 152, in request
    return await getattr(resp, return_method)()
  File "/usr/local/lib/python3.6/asyncio/coroutines.py", line 110, in __next__
    return self.gen.send(None)
  File "/build/lib/python3.6/site-packages/aiohttp/client_reqrep.py", line 1027, in json
    headers=self.headers)
aiohttp.client_exceptions.ContentTypeError: 0, message='Attempt to decode JSON with unexpected mimetype: text/html'
```
which leads to a "could not forward message" 500 error even when the message was correctly forwarded.

Hints from issue discussion:

Commit message: Merge branch 'master' into fix-callback-resp

Patch to review:
diff --git a/changelog/5230.bugfix.rst b/changelog/5230.bugfix.rst
new file mode 100644
index 000000000000..627ba67bcd71
--- /dev/null
+++ b/changelog/5230.bugfix.rst
@@ -0,0 +1 @@
+Fixed issue where posting to certain callback channel URLs would return a 500 error on successful posts due to invalid response format
diff --git a/rasa/utils/endpoints.py b/rasa/utils/endpoints.py
index 244534c2f395..7bceddb50a55 100644
--- a/rasa/utils/endpoints.py
+++ b/rasa/utils/endpoints.py
@@ -2,6 +2,7 @@
 import os
 
 import aiohttp
+from aiohttp.client_exceptions import ContentTypeError
 from typing import Any, Optional, Text, Dict
 
 from sanic.request import Request
@@ -119,10 +120,9 @@ async def request(
         method: Text = "post",
         subpath: Optional[Text] = None,
         content_type: Optional[Text] = "application/json",
-        return_method: Text = "json",
         **kwargs: Any,
-    ):
-        """Send a HTTP request to the endpoint.
+    ) -> Optional[Any]:
+        """Send a HTTP request to the endpoint. Return json response, if available.
 
         All additional arguments will get passed through
         to aiohttp's `session.request`."""
@@ -144,12 +144,15 @@ async def request(
                 headers=headers,
                 params=self.combine_parameters(kwargs),
                 **kwargs,
-            ) as resp:
-                if resp.status >= 400:
+            ) as response:
+                if response.status >= 400:
                     raise ClientResponseError(
-                        resp.status, resp.reason, await resp.content.read()
+                        response.status, response.reason, await response.content.read()
                     )
-                return await getattr(resp, return_method)()
+                try:
+                    return response.json
+                except ContentTypeError:
+                    return None
 
     @classmethod
     def from_dict(cls, data) -> "EndpointConfig":
diff --git a/tests/utils/test_endpoints.py b/tests/utils/test_endpoints.py
index 115e1ae51ca9..d4d9b859ce96 100644
--- a/tests/utils/test_endpoints.py
+++ b/tests/utils/test_endpoints.py
@@ -94,3 +94,19 @@ def test_endpoint_config_custom_token_name():
     actual = endpoint_utils.EndpointConfig.from_dict(test_data)
 
     assert actual.token_name == "test_token"
+
+
+async def test_request_non_json_response():
+    with aioresponses() as mocked:
+        endpoint = endpoint_utils.EndpointConfig("https://example.com/")
+
+        mocked.post(
+            "https://example.com/test",
+            payload="ok",
+            content_type="application/text",
+            status=200,
+        )
+
+        response = await endpoint.request("post", subpath="test",)
+
+        assert response


Code review comments:
diff_hunk: @@ -94,3 +94,19 @@ def test_endpoint_config_custom_token_name():
     actual = endpoint_utils.EndpointConfig.from_dict(test_data)
 
     assert actual.token_name == "test_token"
+
+
+async def test_request_non_json_response():
+    with aioresponses() as mocked:
+        endpoint = endpoint_utils.EndpointConfig("https://example.com/")
+
+        mocked.post(
+            "https://example.com/test",
+            payload="ok",
+            content_type="application/text",
+            status=200,
+        )
+
+        response = await endpoint.request("post", subpath="test",)
+
+        assert response
path: tests/utils/test_endpoints.py
text: @user1:
I guess that should be `assert not response`.
We're returning None in case the response doesn't have json.

@author:
Yeah, realized this after I had changed the implementation 😃wasn't super concerned about the tests before i got feedback on what you thought about it as a patch.

diff_hunk: @@ -144,12 +144,15 @@ def combine_parameters(
                 headers=headers,
                 params=self.combine_parameters(kwargs),
                 **kwargs,
-            ) as resp:
-                if resp.status >= 400:
+            ) as response:
+                if response.status >= 400:
                     raise ClientResponseError(
-                        resp.status, resp.reason, await resp.content.read()
+                        response.status, response.reason, await response.content.read()
                     )
-                return await getattr(resp, return_method)()
+                try:
+                    return response.json
path: rasa/utils/endpoints.py
text: @user1:
shouldn't it be `await response.json()`?

@author:
I tried this but it actually yells and doesn't work, while this does 🤔 i'll grab the error and see if you make anything of it

@author:
nope, now it's working. don't know why that was weird before.