From a0ac31dacc8e33ab3a21bd7218a8b1954925ce42 Mon Sep 17 00:00:00 2001 From: Tom Most Date: Tue, 29 Sep 2020 21:34:24 -0700 Subject: [PATCH 1/4] Extract _request_headers method --- src/treq/client.py | 32 +++++++++++++++++++------------- 1 file changed, 19 insertions(+), 13 deletions(-) diff --git a/src/treq/client.py b/src/treq/client.py index 113d6bb6..b63f9347 100644 --- a/src/treq/client.py +++ b/src/treq/client.py @@ -173,19 +173,7 @@ def request(self, method, url, **kwargs): # 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) bodyProducer, contentType = self._request_body( data=kwargs.pop('data', None), @@ -252,6 +240,24 @@ def gotResult(result): return d.addCallback(_Response, cookies) + def _request_headers(self, headers, stacklevel): + """ + Convert the *headers* argument to a :class:`Headers` instance + """ + 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({}) + return headers + def _request_body(self, data, files, json, stacklevel): """ Here we choose a right producer based on the parameters passed in. From c5a65bf73de02cb128a9c9e8bc20dab636986ed5 Mon Sep 17 00:00:00 2001 From: Tom Most Date: Tue, 29 Sep 2020 22:01:35 -0700 Subject: [PATCH 2/4] Deprecate dropping non-string header values --- src/treq/client.py | 57 ++++++++++++++++++++++++------------ src/treq/test/test_client.py | 49 +++++++++++++++++++++++++++++++ 2 files changed, 88 insertions(+), 18 deletions(-) diff --git a/src/treq/client.py b/src/treq/client.py index b63f9347..ba4f0a1c 100644 --- a/src/treq/client.py +++ b/src/treq/client.py @@ -171,15 +171,13 @@ def request(self, method, url, **kwargs): url = parsed_url.to_uri().to_text().encode('ascii') - # Convert headers dictionary to - # twisted raw headers format. - headers = self._request_headers(kwargs.pop('headers', None), stacklevel) + 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]) @@ -243,20 +241,43 @@ def gotResult(result): def _request_headers(self, headers, stacklevel): """ Convert the *headers* argument to a :class:`Headers` instance + + :returns: + :class:`twisted.web.http_headers.Headers` """ - 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({}) - return 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): """ @@ -293,7 +314,7 @@ def _request_body(self, data, files, json, stacklevel): " This will raise TypeError in the next treq release." ).format("data" if data else "files"), DeprecationWarning, - stacklevel=stacklevel + 1, + stacklevel=stacklevel, ) if files: diff --git a/src/treq/test/test_client.py b/src/treq/test/test_client.py index 51a316d2..8a40bcd2 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,54 @@ def test_request_dict_headers(self): 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.assertEqual( + ( + "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." + ), + 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), + ])) + + [w1, w2] = self.flushWarnings([self.test_request_dict_headers_invalid_values]) + self.assertEqual(DeprecationWarning, w1['category']) + self.assertEqual(DeprecationWarning, w2['category']) + self.assertEqual( + ( + "The value of headers key 'none' has non-string type" + " and will be dropped. This will raise TypeError" + " in the next treq release." + ), + w1['message'], + ) + self.assertEqual( + ( + "The value of headers key 'one' has non-string type" + " and will be dropped. This will raise TypeError" + " in the next treq release." + ), + w2['message'], + ) + def test_request_invalid_param(self): """ `HTTPClient.request()` warns that invalid parameters are ignored and From 2b7c65e38581b5bf8ecc0b0fd81ba87a3776086d Mon Sep 17 00:00:00 2001 From: Tom Most Date: Tue, 29 Sep 2020 22:04:56 -0700 Subject: [PATCH 3/4] Update changelog --- changelog.d/294.removal.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/294.removal.rst diff --git a/changelog.d/294.removal.rst b/changelog.d/294.removal.rst new file mode 100644 index 00000000..deac5dfb --- /dev/null +++ b/changelog.d/294.removal.rst @@ -0,0 +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. From 216d9cddf2ed0318e701c5c0c476f68dfd2ada24 Mon Sep 17 00:00:00 2001 From: Tom Most Date: Fri, 9 Oct 2020 20:23:26 -0700 Subject: [PATCH 4/4] vs. --- src/treq/test/test_client.py | 25 +++++++------------------ 1 file changed, 7 insertions(+), 18 deletions(-) diff --git a/src/treq/test/test_client.py b/src/treq/test/test_client.py index 8a40bcd2..a4dc9577 100644 --- a/src/treq/test/test_client.py +++ b/src/treq/test/test_client.py @@ -527,12 +527,8 @@ def test_request_headers_invalid_type(self): [w] = self.flushWarnings([self.test_request_headers_invalid_type]) self.assertEqual(DeprecationWarning, w['category']) - self.assertEqual( - ( - "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." - ), + self.assertIn( + "headers must be a dict, twisted.web.http_headers.Headers, or None,", w['message'], ) @@ -544,25 +540,18 @@ def test_request_dict_headers_invalid_values(self): 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.assertEqual( - ( - "The value of headers key 'none' has non-string type" - " and will be dropped. This will raise TypeError" - " in the next treq release." - ), + self.assertIn( + "The value of headers key 'none' has non-string type", w1['message'], ) - self.assertEqual( - ( - "The value of headers key 'one' has non-string type" - " and will be dropped. This will raise TypeError" - " in the next treq release." - ), + self.assertIn( + "The value of headers key 'one' has non-string type", w2['message'], )