Repository: twisted/treq

Issue:
treq will silently ignore invalid non-string header values
If a header value is something other than a string, bytes, or list, treq will silently drop the header. It should raise an error instead.

For example this request would not send `Some-Header`:

```py
treq.get('http://example.com', headers={'Some-Header': uuid.uuid4()})
```

This if statement needs a final `else` to raise an error.

https://github.com/twisted/treq/blob/7a8f45af471d172254176e4d71c8d2c0cbc5a7ae/src/treq/client.py#L171-L175

Discussion:

Patch:
diff --git a/changelog.d/294.removal.rst b/changelog.d/294.removal.rst
new file mode 100644
index 0000000..deac5df
--- /dev/null
+++ b/changelog.d/294.removal.rst
@@ -0,0 +1,1 @@
+Deprecate tolerance of non-string values when passing headers as a dict. They have historically been silently dropped, but will raise TypeError in the next treq release. Also deprecate passing headers other than :class:`dict`, :class:`~twisted.web.http_headers.Headers`, or ``None``. Historically falsy values like ``[]`` or ``()`` were accepted.
diff --git a/src/treq/client.py b/src/treq/client.py
index 113d6bb..ba4f0a1 100644
--- a/src/treq/client.py
+++ b/src/treq/client.py
@@ -171,27 +171,13 @@ class HTTPClient(object):
 
         url = parsed_url.to_uri().to_text().encode('ascii')
 
-        # Convert headers dictionary to
-        # twisted raw headers format.
-        headers = kwargs.pop('headers', None)
-        if headers:
-            if isinstance(headers, dict):
-                h = Headers({})
-                for k, v in headers.items():
-                    if isinstance(v, (bytes, six.text_type)):
-                        h.addRawHeader(k, v)
-                    elif isinstance(v, list):
-                        h.setRawHeaders(k, v)
-
-                headers = h
-        else:
-            headers = Headers({})
+        headers = self._request_headers(kwargs.pop('headers', None), stacklevel + 1)
 
         bodyProducer, contentType = self._request_body(
             data=kwargs.pop('data', None),
             files=kwargs.pop('files', None),
             json=kwargs.pop('json', _NOTHING),
-            stacklevel=stacklevel,
+            stacklevel=stacklevel + 1,
         )
         if contentType is not None:
             headers.setRawHeaders(b'Content-Type', [contentType])
@@ -252,6 +238,47 @@ class HTTPClient(object):
 
         return d.addCallback(_Response, cookies)
 
+    def _request_headers(self, headers, stacklevel):
+        """
+        Convert the *headers* argument to a :class:`Headers` instance
+
+        :returns:
+            :class:`twisted.web.http_headers.Headers`
+        """
+        if isinstance(headers, dict):
+            h = Headers({})
+            for k, v in headers.items():
+                if isinstance(v, (bytes, six.text_type)):
+                    h.addRawHeader(k, v)
+                elif isinstance(v, list):
+                    h.setRawHeaders(k, v)
+                else:
+                    warnings.warn(
+                        (
+                            "The value of headers key {!r} has non-string type {}"
+                            " and will be dropped."
+                            " This will raise TypeError in the next treq release."
+                        ).format(k, type(v)),
+                        DeprecationWarning,
+                        stacklevel=stacklevel,
+                    )
+            return h
+        if isinstance(headers, Headers):
+            return headers
+        if headers is None:
+            return Headers({})
+
+        warnings.warn(
+            (
+                "headers must be a dict, twisted.web.http_headers.Headers, or None,"
+                " but found {}, which will be ignored."
+                " This will raise TypeError in the next treq release."
+            ).format(type(headers)),
+            DeprecationWarning,
+            stacklevel=stacklevel,
+        )
+        return Headers({})
+
     def _request_body(self, data, files, json, stacklevel):
         """
         Here we choose a right producer based on the parameters passed in.
@@ -287,7 +314,7 @@ class HTTPClient(object):
                     " This will raise TypeError in the next treq release."
                 ).format("data" if data else "files"),
                 DeprecationWarning,
-                stacklevel=stacklevel + 1,
+                stacklevel=stacklevel,
             )
 
         if files:


Test patch:
diff --git a/src/treq/test/test_client.py b/src/treq/test/test_client.py
index 51a316d..a4dc957 100644
--- a/src/treq/test/test_client.py
+++ b/src/treq/test/test_client.py
@@ -1,4 +1,5 @@
 # -*- encoding: utf-8 -*-
+from collections import OrderedDict
 from io import BytesIO
 
 import mock
@@ -517,6 +518,43 @@ class HTTPClientTests(TestCase):
                      b'Accept': [b'application/json', b'text/plain']}),
             None)
 
+    def test_request_headers_invalid_type(self):
+        """
+        `HTTPClient.request()` warns that headers of an unexpected type are
+        invalid and that this behavior is deprecated.
+        """
+        self.client.request('GET', 'http://example.com', headers=[])
+
+        [w] = self.flushWarnings([self.test_request_headers_invalid_type])
+        self.assertEqual(DeprecationWarning, w['category'])
+        self.assertIn(
+            "headers must be a dict, twisted.web.http_headers.Headers, or None,",
+            w['message'],
+        )
+
+    def test_request_dict_headers_invalid_values(self):
+        """
+        `HTTPClient.request()` warns that non-string header values are dropped
+        and that this behavior is deprecated.
+        """
+        self.client.request('GET', 'http://example.com', headers=OrderedDict([
+            ('none', None),
+            ('one', 1),
+            ('ok', 'string'),
+        ]))
+
+        [w1, w2] = self.flushWarnings([self.test_request_dict_headers_invalid_values])
+        self.assertEqual(DeprecationWarning, w1['category'])
+        self.assertEqual(DeprecationWarning, w2['category'])
+        self.assertIn(
+            "The value of headers key 'none' has non-string type",
+            w1['message'],
+        )
+        self.assertIn(
+            "The value of headers key 'one' has non-string type",
+            w2['message'],
+        )
+
     def test_request_invalid_param(self):
         """
         `HTTPClient.request()` warns that invalid parameters are ignored and