classification
Title: base64 module should use memoryview
Type: enhancement Stage: resolved
Components: Library (Lib) Versions: Python 3.4
process
Status: closed Resolution: fixed
Dependencies: 17842 Superseder:
Assigned To: ncoghlan Nosy List: barry, ezio.melotti, haypo, kushal.das, ncoghlan, pitrou, python-dev, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2013-04-25 07:32 by ncoghlan, last changed 2013-11-13 14:25 by python-dev. This issue is now closed.

Files
File name Uploaded Description Edit
issue17839_v1.patch kushal.das, 2013-04-26 07:26 Code update and test case update review
base64_buffer_input.patch serhiy.storchaka, 2013-04-28 15:06 review
base64_buffer_input_2.patch serhiy.storchaka, 2013-05-22 06:53 review
issue17839_base64_buffer_input_3.patch ncoghlan, 2013-05-23 11:33 Now with memoryview support for the codec interface review
Messages (20)
msg187760 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2013-04-25 07:32
The base64 module is currently restricted specifically to bytes and bytearray objects. By using memoryview, it could effectively decode any input type that provides an 8-bit C contiguous view of the underlying data.
msg187778 - (view) Author: Kushal Das (kushal.das) * (Python committer) Date: 2013-04-25 11:52
Working on this.
msg187837 - (view) Author: Kushal Das (kushal.das) * (Python committer) Date: 2013-04-26 07:26
A patch with tests update to use memoryview in base64 module.
msg187860 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-04-26 14:52
Not only memoryview should be supported, but any class which supports the buffer protocol and is C-contiguous (i.e. array.array).
msg187897 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-04-27 11:28
Yes, this should probably be done the other way around: wrap the base64 argument in a memoryview, and encode/decode from it.
msg187911 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2013-04-27 15:27
As Serhiy and Antoine noted, what I had in mind here was to try to wrap the input in a memoryview if it wasn't an instance of str, bytes or bytearray.

An existing memoryview will be passed back unmodified, while something like array.array will provide a view into its raw data for encoding or decoding. The tests would then cover both memoryview and array.array to ensure it was all working as expected.
msg187919 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-04-27 18:07
Note that some functions use bytes/bytearray methods and an argument should converted to bytes for them. Other functions use C functions which supports the buffer protocol and do not required any conversion.
msg187984 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-04-28 15:06
Here is a patch which allows bytes-like arguments in the base64 module. I just removed type checking if underlying function raises an exception with an appropriate message. I'm not sure about b32encode(), perhaps we can left an exception from memoryview().
msg189537 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-05-18 18:16
Nick, what you say about the patch?
msg189580 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2013-05-19 11:39
Oops, my review comments don't actually make sense because I looked at the patch in isolation, rather than checking the full context in the module. Sorry about that.

We have 2 different cases to deal with, only one of which currently has a helper function.

I suggest renaming _bytes_from_decode_data to "_bytes_for_decoding" and adding "_bytes_for_encoding". The difference between them is the implicit encoding of pure ASCII strings to bytes in the decoding case and the details of the error message thrown.

The encoding and decoding functions should then use the appropriate coercion helper for both the input data and for altchars.
msg189793 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-05-22 06:53
Thank you Ezio and Nick for your comments.

> I suggest renaming _bytes_from_decode_data to "_bytes_for_decoding" and adding "_bytes_for_encoding".

I rather think a TypeError exception raised by `memoryview(s).tobytes()` is good enough and we don't need a special wrapper.
msg189858 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2013-05-23 10:28
We now also need to update the new footnote that I added for issue 17844 in http://hg.python.org/cpython/rev/801567d6302c
msg189860 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2013-05-23 11:33
The codec uses the old API that breaks the encoded output up into multiple lines.

The new patch:

1. Also changes the behaviour of the older de/encodebytes API
2. Checks that all the defined binary transform codecs actually support memoryview as input for both encoding and decoding (and that the data roundtrips correctly)
msg189861 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2013-05-23 11:36
Just adding a note for easier cross-referencing: the old behaviour of these functions was one of the culprits that led to the removal of the codec aliases as discussed in issue 7475
msg189900 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-05-24 07:01
It works only if input also supports slicing and this slicing is corresponded to bytes-slicing. It perhaps doesn't catch a non C-continuous buffers.

Actually we need something like (unlikely cast() doesn't work with empty buffers):

    s = memoryview(s)
    if not s:
        return b''
    s = s.cast('B')
    ...
msg198832 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2013-10-02 12:54
binascii already only supports simple C contiguous buffers, expanding it and the base64 module to handle anything else should be a separate RFE.
msg198835 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2013-10-02 13:05
However, _input_type_check should enforce that (as binascii does), so I'll add that before committing.
msg198843 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2013-10-02 14:48
After working through this, I found that the modern base64 API just relies on the checks in binascii. All that checks for is:

1. Can the data by exported using PyBUF_SIMPLE?
2. Is it C contiguous?

It completely ignores the number of dimensions and the format information. I added tests to at least capture this behaviour, even though it seems a little dubious to me.

For the legacy API, I didn't relax the input checks that far - the legacy API will still complain if there is more than 1 dimension and if the format code isn't one of 'c', 'b' or 'B'. That's already substantially more permissive than what it supported in previous versions.

Just running the full test suite now, will push after that finishes.
msg198844 - (view) Author: Roundup Robot (python-dev) Date: 2013-10-02 15:03
New changeset d90f25e1a705 by Nick Coghlan in branch 'default':
Close #17839: support bytes-like objects in base64 module
http://hg.python.org/cpython/rev/d90f25e1a705
msg202752 - (view) Author: Roundup Robot (python-dev) Date: 2013-11-13 14:25
New changeset e53984133740 by Nick Coghlan in branch 'default':
Issue #17839: mention base64 change in What's New
http://hg.python.org/cpython/rev/e53984133740
History
Date User Action Args
2013-11-13 14:25:23python-devsetmessages: + msg202752
2013-10-02 15:03:44python-devsetstatus: open -> closed

nosy: + python-dev
messages: + msg198844

resolution: fixed
stage: patch review -> resolved
2013-10-02 14:48:52ncoghlansetmessages: + msg198843
2013-10-02 13:05:17ncoghlansetassignee: ncoghlan
messages: + msg198835
2013-10-02 12:54:56ncoghlansetdependencies: - Allow memoryview.cast() for empty views
messages: + msg198832
2013-10-01 21:48:18serhiy.storchakasetdependencies: + Allow memoryview.cast() for empty views
2013-05-24 07:01:59serhiy.storchakasetmessages: + msg189900
2013-05-23 11:36:55ncoghlansetmessages: + msg189861
2013-05-23 11:33:40ncoghlansetfiles: + issue17839_base64_buffer_input_3.patch

messages: + msg189860
2013-05-23 10:28:34ncoghlansetmessages: + msg189858
2013-05-22 06:53:48serhiy.storchakasetfiles: + base64_buffer_input_2.patch

messages: + msg189793
2013-05-19 11:39:03ncoghlansetmessages: + msg189580
2013-05-18 18:16:22serhiy.storchakasetmessages: + msg189537
2013-04-29 14:33:59hayposetnosy: + haypo
2013-04-28 15:06:05serhiy.storchakasetfiles: + base64_buffer_input.patch

messages: + msg187984
2013-04-27 18:07:21serhiy.storchakasetmessages: + msg187919
2013-04-27 15:27:12ncoghlansetmessages: + msg187911
2013-04-27 11:28:42pitrousetnosy: + pitrou
messages: + msg187897
2013-04-26 14:52:47serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg187860
2013-04-26 11:46:44ezio.melottisetnosy: + ezio.melotti

stage: needs patch -> patch review
2013-04-26 07:26:50kushal.dassetfiles: + issue17839_v1.patch
keywords: + patch
messages: + msg187837
2013-04-25 13:55:00barrysetnosy: + barry
2013-04-25 11:52:37kushal.dassetnosy: + kushal.das
messages: + msg187778
2013-04-25 10:51:23serhiy.storchakasetdependencies: + Add base64 module tests for a bytearray argument
2013-04-25 07:49:12ncoghlanlinkissue7475 dependencies
2013-04-25 07:32:34ncoghlancreate