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

mattwwarren · 2020-10-11T14:35:15Z

No description provided.

mattwwarren · 2020-10-11T14:36:13Z

src/treq/client.py

@@ -39,6 +39,7 @@


 _NOTHING = object()
+CONTENT_TYPE_HEADER_NAME = b'Content-Type'


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

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.

mattwwarren · 2020-10-11T14:38:54Z

I noticed #297 was merged and wanted to get this up for review before a release with that change, since it would break our current code. Thanks in advance for the review, this is my first PR here so please don't hold back!

twm · 2020-10-14T06:33:02Z

src/treq/client.py

            return (
                multipart.MultiPartProducer(data + files, boundary=boundary),
-                b'multipart/form-data; boundary=' + boundary,
+                content_type,


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

twm · 2020-10-14T06:44:41Z

Hi @mattwwarren, thank you for diving in here! I may not get to a full review before the weekend but I did a quick skim. Apart from the boundary thing I noted above, tests should have docstrings (yeah, not all of the ones there do...). https://jml.io/pages/test-docstrings.html is a good guide to this.

twm

Hi @mattwwarren! Thank you for this. I've gone through and noted a few issues that would need to be addressed before landing, but the central issue is handling of the application/form-data.

Looking at #289 again I think that I may have misinterpreted what you were after. Did you want to set the Content-Type header of an application/x-www-form-urlencoded request to application/form-data? That doesn't make sense, as the server will fail to parse it due to the content-type mismatch, won't it? Or did you want to generate an application/form-data request without actual files? The latter is actually supported — you can do treq.post(..., files=[('field', 'value'), ...]) (or even pass a dict to files). You could also accomplish this by passing a MultiPartProducer() instance as the data parameter, along with the matching Content-Type header.

I'm left wondering if this is actually a documentation issue (of which, admittedly, there are many). The ability to override the Content-Type of a JSON request still seems useful to me, but it's a smaller feature than what you've implemented here.

Again, thank you for the PR! I'd love to work with you to make treq work for your use case.

twm · 2020-10-19T05:09:08Z

src/treq/client.py

@@ -39,6 +39,7 @@


 _NOTHING = object()
+CONTENT_TYPE_HEADER_NAME = b'Content-Type'


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.

twm · 2020-10-19T05:46:05Z

src/treq/client.py

@@ -431,6 +448,29 @@ def _guess_content_type(filename):
    return guessed or 'application/octet-stream'


+def _get_content_type_from_header(headers, default=None):


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

twm · 2020-10-19T05:48:46Z

src/treq/client.py

@@ -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:


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).

twm · 2020-10-19T06:09:47Z

src/treq/client.py

+    """
+    if not headers:
+        return default
+    return headers.getRawHeaders(


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

twm · 2020-10-19T06:12:01Z

src/treq/client.py

+    :rtype:
+        bytes
+    """
+    if not headers:


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...)

glyph · 2021-06-29T03:48:19Z

Hi @mattwwarren ! Just bumping this to see if you've still got interest. I know it's been a heck of a year but I think we're still interested in integrating this PR.

glyph · 2022-03-15T06:38:14Z

Closing this to clean up the review queue. If anyone is interested in picking up the work, feel free to reopen.

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

e12bace

mattwwarren commented Oct 11, 2020

View reviewed changes

mattwwarren added 2 commits October 11, 2020 10:54

test fixes

a586f4c

test fix

533ee14

twm reviewed Oct 14, 2020

View reviewed changes

twm requested changes Oct 19, 2020

View reviewed changes

twm added the abandoned This PR has code, but the author doesn't have time to address feedback. Newcomers, pick these up! label Feb 5, 2022

glyph closed this Mar 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

mattwwarren commented Oct 11, 2020

mattwwarren Oct 11, 2020

twm Oct 19, 2020

mattwwarren commented Oct 11, 2020

twm Oct 14, 2020

twm commented Oct 14, 2020

twm left a comment

twm Oct 19, 2020

twm Oct 19, 2020

twm Oct 19, 2020

twm Oct 19, 2020

twm Oct 19, 2020

glyph commented Jun 29, 2021

glyph commented Mar 15, 2022

		@@ -39,6 +39,7 @@


		_NOTHING = object()
		CONTENT_TYPE_HEADER_NAME = b'Content-Type'

		@@ -431,6 +448,29 @@ def _guess_content_type(filename):
		return guessed or 'application/octet-stream'


		def _get_content_type_from_header(headers, default=None):

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

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

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

Conversation

mattwwarren commented Oct 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattwwarren commented Oct 11, 2020

Choose a reason for hiding this comment

twm commented Oct 14, 2020

twm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glyph commented Jun 29, 2021

glyph commented Mar 15, 2022