classification
Title: set_payload does not handle binary payloads correctly
Type: behavior Stage: resolved
Components: email Versions: Python 3.4, Python 3.3
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: barry, python-dev, r.david.murray, vajrasky
Priority: normal Keywords: easy, patch

Created on 2013-06-28 19:16 by r.david.murray, last changed 2013-08-22 01:18 by r.david.murray. This issue is now closed.

Files
File name Uploaded Description Edit
set_qp_payload_test.patch r.david.murray, 2013-06-28 19:16 review
set_payload_handles_binary_correctly.txt vajrasky, 2013-07-10 15:58 Makes the set_payload handles binary correctly review
set_payload_binary.txt vajrasky, 2013-07-21 14:55 review
set_payload_binary_v2.txt vajrasky, 2013-07-22 03:13 review
set_payload_binary_v3.txt vajrasky, 2013-07-27 05:09 review
Messages (13)
msg192012 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2013-06-28 19:16
In order to maintain model consistency without exposing the need for 'surrogateescape' to library users, it should be possible to pass binary data to set_payload and have it do the correct conversion to the expected storage format for the model.  Currently, this does not work.  The attached patch provides one example test out of a class of tests that should be written and made to pass.
msg192823 - (view) Author: Vajrasky Kok (vajrasky) * Date: 2013-07-10 15:58
Here is the preliminary patch for email module to pass the test.
msg192826 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2013-07-10 16:07
Thanks, but the patch is incorrect.  The model consistently stores its data as surrogateescaped strings, and this assumption is baked in to other parts of the code.  So the correct fix is to do the surrogateescape encoding at the time the payload is set.

It might in fact be better to store a binary payload as binary, but making that kind of change to the model requires much more extensive review and testing.
msg192828 - (view) Author: Vajrasky Kok (vajrasky) * Date: 2013-07-10 16:31
I see. Thanks for the explanation. I'll do this patch if nobody is interested.
msg192829 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2013-07-10 16:42
If you want to work on it that would be great.  Note that one of the things that is needed is a bunch more tests of setting various *kinds* of binary payload, including ones containing non-ascii data, and making sure the right thing happens when the payload is later fetched/serialized.
msg193454 - (view) Author: Vajrasky Kok (vajrasky) * Date: 2013-07-21 14:52
Here is the patch for this ticket.

David Murray, am I on the right path? If yes, I'll put more robust tests, such as the ones with Asian encodings and unusual encodings.
msg193455 - (view) Author: Vajrasky Kok (vajrasky) * Date: 2013-07-21 14:55
Sorry, got typo for the last patch.
msg193456 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2013-07-21 15:38
It looks like you are still patching get_payload.  This should be a really simple patch against set_payload.

It occurs to me that there could be a backward compatibility concern if passing binary to set_payload currently actually works in some cases, so we definitely needs a bunch of test that do that to make sure they all fail before we fix the bug :)
msg193490 - (view) Author: Vajrasky Kok (vajrasky) * Date: 2013-07-22 03:13
"It looks like you are still patching get_payload.  This should be a really simple patch against set_payload."

Okay, do I get it right at this time?

About your second point, I need more time to think about it.
msg193493 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2013-07-22 03:48
Yes, that's what I had in mind.
msg193771 - (view) Author: Vajrasky Kok (vajrasky) * Date: 2013-07-27 05:09
Here is the third version of the patch.

I am not sure what to do with the invalid data for base64 and uuencode. I decided to raise exception instead of converting it to None silently.
msg195850 - (view) Author: Roundup Robot (python-dev) Date: 2013-08-22 01:14
New changeset 64e004737837 by R David Murray in branch '3.3':
#18324: set_payload now correctly handles binary input.
http://hg.python.org/cpython/rev/64e004737837

New changeset a4afcf93ef7b by R David Murray in branch 'default':
Merge #18324: set_payload now correctly handles binary input.
http://hg.python.org/cpython/rev/a4afcf93ef7b
msg195853 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2013-08-22 01:18
Thanks, Vajrasky. The v2 patch was almost correct.  What you couldn't know without being as deeply enmeshed in this code as I am is that the test failures from the encoders module were actually invalid.  We'd previously "fixed" them, but the fixes were incorrectly compensating for this bug in set_payload.  Once this bug was fixed, those "fixes" just needed to be backed out, a 'decode=True' added to their get_payload calls, and all the tests pass.

I *think* this is the last inconsistency in the model.  I hope.
History
Date User Action Args
2013-08-22 01:18:56r.david.murraysetstatus: open -> closed
resolution: fixed
messages: + msg195853

stage: test needed -> resolved
2013-08-22 01:14:28python-devsetnosy: + python-dev
messages: + msg195850
2013-07-27 05:09:09vajraskysetfiles: + set_payload_binary_v3.txt

messages: + msg193771
2013-07-22 03:48:19r.david.murraysetmessages: + msg193493
2013-07-22 03:13:27vajraskysetfiles: + set_payload_binary_v2.txt

messages: + msg193490
2013-07-21 15:38:47r.david.murraysetmessages: + msg193456
2013-07-21 14:55:27vajraskysetfiles: + set_payload_binary.txt

messages: + msg193455
2013-07-21 14:54:27vajraskysetfiles: - set_payload_binary.txt
2013-07-21 14:52:43vajraskysetfiles: + set_payload_binary.txt

messages: + msg193454
2013-07-10 16:42:01r.david.murraysetmessages: + msg192829
2013-07-10 16:31:16vajraskysetmessages: + msg192828
2013-07-10 16:07:05r.david.murraysetmessages: + msg192826
2013-07-10 15:58:19vajraskysetfiles: + set_payload_handles_binary_correctly.txt
nosy: + vajrasky
messages: + msg192823

2013-06-28 19:16:22r.david.murraycreate