Opened 5 years ago

Closed 18 months ago

#6082 enhancement closed fixed (fixed)

Make test_updateWithKeywords work on Python 3

Reported by: Jean-Paul Calderone Owned by: Unit03
Priority: normal Milestone: Python-3.x
Component: web Keywords:
Cc: jknight Branch: branches/http-headers-update-keywords-6082
branch-diff, diff-cov, branch-cov, buildbot
Author: adiroiban

Description

twisted.web.test.test_http_headers._DictHeader.update works like dict.update, accepting keyword arguments. Since header names should be bytes, though, some extra support is needed to make it correct on Python 3.

Skipped porting this functionality as part of #6080 since it's somewhat obscure and slightly more work, but eventually it should be supported.

Attachments (2)

headers-keyword-update-6082-1.patch (1.2 KB) - added by Unit03 19 months ago.
headers-keyword-update-6082-2.patch (1.6 KB) - added by Unit03 18 months ago.

Download all attachments as: .zip

Change History (12)

comment:1 Changed 5 years ago by DefaultCC Plugin

Cc: jknight added

Changed 19 months ago by Unit03

comment:2 Changed 19 months ago by Unit03

Keywords: review added

This is seemingly more trivial than expected, but maybe I'm missing something.

t.w.http_headers._DictHeaders stores headers with keys as bytes, but for keyword-style update of the inner dict, keyword names - which are unicode/str - must first be encoded in __setitem__. I wasn't sure about which encoding to use.

Does the patch look reasonable?

comment:3 Changed 19 months ago by Adi Roiban

Author: adiroiban
Branch: branches/http-headers-update-keywords-6082

(In [46240]) Branching to http-headers-update-keywords-6082.

comment:4 Changed 19 months ago by Adi Roiban

Keywords: review removed
Owner: set to Unit03

Many thanks for your patch!

Is 'ascii' the right encoding for headers? From what I understand from RFC 2616 I think that latin1 characters are also allowed as part of the headers.

Looking forward for your comments.

I have applied your latest patch and sent it to our builbot.

In case latin1 is not supported, please add an explicit test and update the documentation to let user know that even if RFC 2616 allows latin1, Twisted does not yet support that.

Thanks again!

comment:5 Changed 18 months ago by Unit03

Owner: changed from Unit03 to Adi Roiban

Then I guess it should be latin1. But

In case latin1 is not supported (...)

Not supported where?

comment:6 Changed 18 months ago by Adi Roiban

Not supported by Twisted.


I think that ASCII is fine. I was confused ... but now I think that latin-1 was only supported as header value, not as header name

https://tools.ietf.org/html/rfc7230#section-3.2.4

Historically, HTTP has allowed field content with text in the ISO-8859-1 charset [ISO-8859-1], supporting other charsets only through use of [RFC2047] encoding. In practice, most HTTP header field values use only a subset of the US-ASCII charset [USASCII]. Newly defined header fields SHOULD limit their field values to US-ASCII octets. A recipient SHOULD treat other octets in field content (obs-text) as opaque data.


So. Your patch is great.

It still needs a news file fragment so that we will have a note in the release note and people will know that this bug is fixed.

Here is some more info about how we manage the news file: http://twistedmatrix.com/trac/wiki/ReviewProcess#Newsfiles

For this case, feel fee to just the release notes as a comment and I will take care of creating the right file.

Thanks!

comment:7 Changed 18 months ago by Adi Roiban

Owner: changed from Adi Roiban to Unit03

Please provide a text for the release notes associated with this patch.

Thanks!

Changed 18 months ago by Unit03

comment:8 Changed 18 months ago by Unit03

Keywords: review added
Owner: changed from Unit03 to Adi Roiban

Great, thanks. :)

Sorry about lack of news file, I wrongly assumed I'm not supposed to write it. I'm attaching updated patch with news entry.

Feel free to reword it if necessary, though (I wasn't sure about few things, e.g. whether to include that t.w.http_headers is now fully ported to 3 - it wasn't mentioned in NEWS previously).

comment:9 Changed 18 months ago by Adi Roiban

Keywords: review removed
Owner: changed from Adi Roiban to Unit03

Thanks.

Changes applied.

Will wait for test result and if all is ok will merge.

Many thanks again for your help!

comment:10 Changed 18 months ago by Adi Roiban

Resolution: fixed
Status: newclosed

(In [46255]) Merge http-headers-update-keywords-6082: Fix updating twisted.web.http_headers._DictHeaders using keywords in Python 3.

Author: Unit03 Reviewer: adiroiban Fixes: #6082

Note: See TracTickets for help on using tickets.