Repository: huge-success/sanic
Pull request #1736 (2019-12-22 06:03:33)

Allow empty body without Content-Type; Introduce response.empty()
Sometimes, a 204 response is just enough to signal the client what the backend wants to do. Currently, this is possible but has some awkwardness.
First, an empty response has no `Content-Type`, however all responses require Content-Type header.

This PR allows `content_type` param in `HTTPResponse` to be `None`. It is the simplest solution, however I have to say I am not a big fan.
A big disadvantage is the fact that the content type is now located in two locations: `self.headers` and `self.content_type` and they COULD be out of sync, since `headers` have a higher precedence.
This means that if one passes `text/plain` to `content_type` but `Content-Type: application/json` in `headers`, the final result will be an inconsistant response object with one field saying one thing and the other another.

Since this behavior exists before this PR, I did not set out to fix it in this enhancement PR. I personally like to keep PRs atomic, and therefor will contribute a refactor if requested in a sequetial PR.

Fixes https://github.com/huge-success/sanic/issues/1735

codecov[bot]: # [Codecov](https://codecov.io/gh/huge-success/sanic/pull/1736?src=pr&el=h1) Report
> Merging [#1736](https://codecov.io/gh/huge-success/sanic/pull/1736?src=pr&el=desc) into [master](https://codecov.io/gh/huge-success/sanic/commit/3f6a978328a1976a1c3d75cb8bda039ce3d1c11c?src=pr&el=desc) will **increase** coverage by `0.01%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/huge-success/sanic/pull/1736/graphs/tree.svg?width=650&token=NqLbJHUqoR&height=150&src=pr)](https://codecov.io/gh/huge-success/sanic/pull/1736?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master    #1736      +/-   ##
==========================================
+ Coverage   92.17%   92.18%   +0.01%     
==========================================
  Files          22       22              
  Lines        2235     2238       +3     
  Branches      418      419       +1     
==========================================
+ Hits         2060     2063       +3     
  Misses        136      136              
  Partials       39       39
```


| [Impacted Files](https://codecov.io/gh/huge-success/sanic/pull/1736?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [sanic/response.py](https://codecov.io/gh/huge-success/sanic/pull/1736/diff?src=pr&el=tree#diff-c2FuaWMvcmVzcG9uc2UucHk=) | `100% <100%> (ø)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/huge-success/sanic/pull/1736?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/huge-success/sanic/pull/1736?src=pr&el=footer). Last update [3f6a978...e8fabac](https://codecov.io/gh/huge-success/sanic/pull/1736?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

ahopkins: Looks good to me. I also like the atomic approach, and would welcome a second PR on that point.
Tronic: Looks good, excellent reasoning and I absolutely agree on a second PR that might also get rid of `body`/`body_bytes` duplication as well as the `content_type` argument (but taking compatibility concerns into account).
LiraNuna: How  would you address compatibility concerns with a removal of a parameter? It is a breaking change to interface. 

I will gladly merge the `body`/`body_bytes` param in addition to eliminating `content_type` property (I was planning on keeping it in headers and make a getter for compatibility)
ahopkins: @LiraNuna Let's open this discussion in a new conversation. I have some thoughts to share.

@huge-success/sanic-release-managers this one is good to go.

Patch:
diff --git a/sanic/response.py b/sanic/response.py
index 91fb25f4..c75af62e 100644
--- a/sanic/response.py
+++ b/sanic/response.py
@@ -153,7 +153,7 @@ class HTTPResponse(BaseHTTPResponse):
         body=None,
         status=200,
         headers=None,
-        content_type="text/plain",
+        content_type=None,
         body_bytes=b"",
     ):
         self.content_type = content_type
@@ -181,9 +181,9 @@ class HTTPResponse(BaseHTTPResponse):
                 "Content-Length", len(self.body)
             )
 
-        self.headers["Content-Type"] = self.headers.get(
-            "Content-Type", self.content_type
-        )
+        # self.headers get priority over content_type
+        if self.content_type and "Content-Type" not in self.headers:
+            self.headers["Content-Type"] = self.content_type
 
         if self.status in (304, 412):
             self.headers = remove_entity_headers(self.headers)
@@ -214,6 +214,18 @@ class HTTPResponse(BaseHTTPResponse):
         return self._cookies
 
 
+def empty(
+    status=204, headers=None,
+):
+    """
+    Returns an empty response to the client.
+
+    :param status Response code.
+    :param headers Custom Headers.
+    """
+    return HTTPResponse(body_bytes=b"", status=status, headers=headers,)
+
+
 def json(
     body,
     status=200,


Test patch:
diff --git a/tests/test_response.py b/tests/test_response.py
index 8feadb06..87bda1bf 100644
--- a/tests/test_response.py
+++ b/tests/test_response.py
@@ -21,6 +21,7 @@ from sanic.response import (
     raw,
     stream,
 )
+from sanic.response import empty
 from sanic.server import HttpProtocol
 from sanic.testing import HOST, PORT
 
@@ -591,3 +592,13 @@ def test_raw_response(app):
     request, response = app.test_client.get("/test")
     assert response.content_type == "application/octet-stream"
     assert response.body == b"raw_response"
+
+
+def test_empty_response(app):
+    @app.get("/test")
+    def handler(request):
+        return empty()
+
+    request, response = app.test_client.get("/test")
+    assert response.content_type is None
+    assert response.body == b""