Opened 2 years ago

Closed 2 years ago

#6205 regression closed fixed (fixed)

No longer able to set header values to non-str

Reported by: radix Owned by: therve
Priority: normal Milestone: Twisted-12.3
Component: web Keywords:
Cc: jknight Branch: branches/non-bytes-headers-6205
(diff, github, buildbot, log)
Author: therve Launchpad Bug:

Description

We previously allowed setting header values to non-str objects, like ints. Since the Python 3 port of http.py, that's no longer possible.

Change History (11)

comment:1 Changed 2 years ago by DefaultCC Plugin

  • Cc jknight added

comment:2 Changed 2 years ago by glyph

  • Milestone set to Twisted-12.3

Adding to release milestone as this is a regression.

comment:3 Changed 2 years ago by exarkun

It would be helpful if you mentioned what API the change in behavior relates to.

comment:4 Changed 2 years ago by therve

  • Owner set to therve

comment:5 Changed 2 years ago by therve

  • Author set to therve
  • Branch set to branches/non-bytes-headers-6205

(In [36535]) Branching to 'non-bytes-headers-6205'

comment:6 Changed 2 years ago by therve

  • Keywords review added
  • Owner therve deleted

The change was in http.Request.write. We used to force cast to string, now we just pass the data to writeSequence directly. The branch tries to change it back, but it's a bit gross. Maybe we should deprecate it too.

comment:7 Changed 2 years ago by exarkun

  • Keywords review removed
  • Owner set to therve

Thanks.

  1. setRawHeaders never supported non-bytes arguments for the name argument (it required something with a lower method and if it were unicode, interpolating it in write later would produce unicode which would break the transport) and I don't think it should start now.
  2. The .encode('ascii') call in write is just what networkString is for.
  3. Also the comment in write is indented weirdly.
  4. I agree this should be deprecated. It was never intended as a feature.

Please merge after these are fixed and buildbot says it works everywhere.

comment:8 Changed 2 years ago by radix

I will let you judge for yourself, but I think there is a very uncommon incompatibility remaining.

Previously (before the py3k porting work), an object that had a str that returned non-ascii bytes would work (meaning those bytes would be sent directly over the wire as returned), and now, in your branch, it won't. You'll get an ascii decoding or encoding error, since you're converting to unicode and then back to bytes.

comment:9 Changed 2 years ago by therve

  • Keywords review added
  • Owner therve deleted

http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/non-bytes-headers-6205 with the build.

I couldn't use networkString as far I can tell. Maybe I'm missing something, so I put it back for a round of review. Everything else handled.

Radix: maybe you're right. Considering it was never explicitly supported and it's more work, I'd wait for someone to complain.

comment:10 Changed 2 years ago by exarkun

  • Keywords review removed
  • Owner set to therve
  1. You can use networkString. You may have just had the impression that it was going to be more useful than it really is. Instead of:
value = ('%s' % (value,)).encode('ascii')

I suggest:

value = networkString('%s' % (value,))

This just has the benefit of being more self-documented about why the value is being encoded to ascii - because it is a string that belongs on the network.

Something else I just noticed is that this doesn't support unicode values though (even ascii-only unicode). Of course, the old version didn't support unicode either, so that's just fine. It's kinda funny, since if I didn't know that this bug was filed because someone was passing an integer, I would have guessed the most likely incorrect type to be passed would be unicode.

  1. test_nonByteHeaderValue depends on dictionary iteration order, I think.

comment:11 Changed 2 years ago by therve

  • Resolution set to fixed
  • Status changed from new to closed

(In [36539]) Merge non-bytes-headers-6205

Author: therve
Reviewer: exarkun
Fixes: #6205

Re-allow usage of non-bytes header values in Request.write, as it was before
the py3 refactoring, but deprecate it.

Note: See TracTickets for help on using tickets.