Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow overriding of the default Content-Type in POST and PUT #301

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions changelog.d/289.feature.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
treq request functions and methods like :func:`treq.put()` and :meth:`HTTPClient.post()` now accept Content-Type overrides through the headers kwarg
Content-Types will still default to the previous default values depending on the `data`, `files`, and `json` parameters.
49 changes: 44 additions & 5 deletions src/treq/client.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@


_NOTHING = object()
CONTENT_TYPE_HEADER_NAME = b'Content-Type'
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a personal design decision to reduce typos. Please let me know if it runs counter to the project's intended design.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having a constant isn't my personal preference — tests should catch any typos, and in this case the constant name is longer than the actual value and no more readable — but I'm okay with it. However, the constant should definitely be a private to the module, so please rename it to _CONTENT_TYPE_HEADER_NAME.



def urlencode(query, doseq):
Expand Down Expand Up @@ -192,9 +193,13 @@ def request(self, method, url, **kwargs):
files=kwargs.pop('files', None),
json=kwargs.pop('json', _NOTHING),
stacklevel=stacklevel,
headers=headers,
)
if contentType is not None:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since headers is now passed into _request_body I think that the header setting logic should move there too. I originally extracted this to simplify the logic of that function, but that doesn't hold anymore. You could make _get_content_type_from_header do the mutation instead (it would also need to be renamed in that case).

headers.setRawHeaders(b'Content-Type', [contentType])
headers.setRawHeaders(
CONTENT_TYPE_HEADER_NAME,
[contentType]
)

cookies = kwargs.pop('cookies', {})

Expand Down Expand Up @@ -252,7 +257,7 @@ def gotResult(result):

return d.addCallback(_Response, cookies)

def _request_body(self, data, files, json, stacklevel):
def _request_body(self, data, files, json, stacklevel, headers):
"""
Here we choose a right producer based on the parameters passed in.

Expand Down Expand Up @@ -301,17 +306,25 @@ def _request_body(self, data, files, json, stacklevel):
else:
data = []

content_type = _get_content_type_from_header(
headers,
b'multipart/form-data; boundary=' + boundary
)
return (
multipart.MultiPartProducer(data + files, boundary=boundary),
b'multipart/form-data; boundary=' + boundary,
content_type,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't setting the content type in this case make the request body unparseable for lack of the boundary parameter?

)

# Otherwise stick to x-www-form-urlencoded format
# as it's generally faster for smaller requests.
if isinstance(data, (dict, list, tuple)):
content_type = _get_content_type_from_header(
headers,
b'application/x-www-form-urlencoded'
)
return (
self._data_to_body_producer(urlencode(data, doseq=True)),
b'application/x-www-form-urlencoded',
content_type,
)
elif data:
return (
Expand All @@ -320,11 +333,15 @@ def _request_body(self, data, files, json, stacklevel):
)

if json is not _NOTHING:
content_type = _get_content_type_from_header(
headers,
b'application/json; charset=UTF-8'
)
return (
self._data_to_body_producer(
json_dumps(json, separators=(u',', u':')).encode('utf-8'),
),
b'application/json; charset=UTF-8',
content_type,
)

return None, None
Expand Down Expand Up @@ -431,6 +448,28 @@ def _guess_content_type(filename):
return guessed or 'application/octet-stream'


def _get_content_type_from_header(headers, default=None):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like this should be called _get_content_type_from_header*s*? But _get_content_type would be fine.

"""
Retrieve Content-Type header from twisted.web.http_headers.Headers object
or return supplied default

:param headers:
A twisted.web.http_headers.Headers object

:param default:
The default Content-Type to return, encoded as a byte-string

:returns:
byte-string containing a Content-Type value
:rtype:
bytes
"""
if not headers:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At this point headers is always a Headers object:

treq/src/treq/client.py

Lines 174 to 188 in d45cad2

# 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({})

This clause is dead code and should be removed (at which point this function doesn't do much...)

return default
return headers.getRawHeaders(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that this function should raise an exception when there are multiple content-type headers (since that's not valid). I think it's okay to break messily in this case, so you could unpack like:

[content_type] = headers.getRawHeaders(CONTENT_TYPE_HEADER_NAME, [default])
return content_type

CONTENT_TYPE_HEADER_NAME, [default]
)[0]

registerAdapter(_from_bytes, bytes, IBodyProducer)
registerAdapter(_from_file, io.BytesIO, IBodyProducer)

Expand Down
22 changes: 22 additions & 0 deletions src/treq/test/test_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -281,6 +281,19 @@ def test_request_data_file(self):

self.assertBody(b'hello')

def test_request_data_dict_content_type_override(self):
self.client.request('PUT', 'http://example.com/',
data={'foo': ['bar', 'baz']},
headers={'Content-Type': 'application/json'})

self.agent.request.assert_called_once_with(
b'PUT', b'http://example.com/',
Headers({b'Content-Type': [b'application/json'],
b'accept-encoding': [b'gzip']}),
self.FileBodyProducer.return_value)

self.assertBody(b'foo=bar&foo=baz')

def test_request_json_dict(self):
self.client.request('POST', 'http://example.com/', json={'foo': 'bar'})
self.agent.request.assert_called_once_with(
Expand Down Expand Up @@ -335,6 +348,15 @@ def test_request_json_none(self):
self.FileBodyProducer.return_value)
self.assertBody(b'null')

def test_request_json_header_override(self):
self.client.request('POST', 'http://example.com/', json={'foo': 'bar'}, headers={'Content-Type': 'multipart/form-data; boundary='})
self.agent.request.assert_called_once_with(
b'POST', b'http://example.com/',
Headers({b'Content-Type': [b'multipart/form-data; boundary='],
b'accept-encoding': [b'gzip']}),
self.FileBodyProducer.return_value)
self.assertBody(b'{"foo":"bar"}')

@mock.patch('treq.client.uuid.uuid4', mock.Mock(return_value="heyDavid"))
def test_request_no_name_attachment(self):

Expand Down