base64_codec uses assert for runtime validity checks

[ncoghlan]

BPO	17840
Nosy	@malemburg, @ncoghlan, @vstinner, @ezio-melotti, @berkerpeksag, @vadmium
Files	issue17840.patch: Removes error-mode assertions from encodings/*_codec.py issue17840.patch: New patch raising ValueError

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = None
created_at = <Date 2013-04-25.07:40:50.545>
labels = ['type-bug', 'library', '3.11']
title = 'base64_codec uses assert for runtime validity checks'
updated_at = <Date 2021-12-12.00:45:46.721>
user = 'https://github.com/ncoghlan'

bugs.python.org fields:

activity = <Date 2021-12-12.00:45:46.721>
actor = 'iritkatriel'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Library (Lib)']
creation = <Date 2013-04-25.07:40:50.545>
creator = 'ncoghlan'
dependencies = []
files = ['30814', '30902']
hgrepos = []
issue_num = 17840
keywords = ['patch']
message_count = 11.0
messages = ['187762', '192434', '192561', '192638', '192710', '192951', '234303', '234306', '234307', '234613', '348628']
nosy_count = 7.0
nosy_names = ['lemburg', 'ncoghlan', 'vstinner', 'ezio.melotti', 'berker.peksag', 'martin.panter', 'alex.henderson']
pr_nums = []
priority = 'normal'
resolution = None
stage = 'needs patch'
status = 'open'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue17840'
versions = ['Python 3.11']

[ncoghlan]

encodings.base64_codec currently uses "assert errors=='strict'" in a few places, since it doesn't actually support any of the Unicode specific error handling modes.

This should either be discarded entirely (and document that the error handling mode is irrelevant for this codec), or else turned into a real check that raises ValueError if an unsupported error mode is passed in.

I have a slight preference for just ignoring the error mode as irrelevant (since this isn't a text encoding in the normal Unicode serialisation-as-bytes sense).

[alexhenderson]

I see that there is identical usage of "assert errors=='strict'" in a number of similar encodings modules:

base64_codec.py
bz2_codec.py
hex_codec.py
quopri_codec.py
uu_codec.py
zlib_codec.py

The error handling mode is irrelevant for all these codecs, so the attached patch addresses them all (choosing to ignore the error mode and documenting this).

[alexhenderson]

Having discussed this with Ezio, I think the better option might be to raise ValueError instead - if someone is expecting to be able to silently recover from errors they won't be able to, and should find out about this sooner rather than later.
I'll upload an updated patch shortly.

[malemburg]

On 07.07.2013 16:31, Alex Henderson wrote:

Having discussed this with Ezio, I think the better option might be to raise ValueError instead - if someone is expecting to be able to silently recover from errors they won't be able to, and should find out about this sooner rather than later.
I'll upload an updated patch shortly.

+1

[ncoghlan]

ValueError works for me.

[alexhenderson]

OK, now raises ValueError on passing anything other than 'strict'.

Note that for the incremental classes I've put checking in __init__ so that ValueError is raised when non-'strict' values are passed to the constructor, not when the incremental encode/decode methods are subsequently called.

[vadmium]

Is this patch likely to go ahead? It has been sitting around a while and would conflict with patches I am working on.

If so, I reckon it would be good to factor out some of the new bits of code (_check_strict, _StrictErrors) into a common place, like the “codecs” module.

[berkerpeksag]

Thanks for the patch. I left a couple of comments on Rietveld.

[vadmium]

Would also be good to document that errors='ignored' is not allowed. Currently the documentation says

The following string values are defined and implemented by all standard Python codecs:

• 'strict'
• 'ignore'

[ncoghlan]

I'd be slightly more inclined to file the fact that "ignore" isn't supported by the base64 codec as a potential bug, with changing the docs as one possible resolution.

It shouldn't hold up switching from the assertions to proper runtime checks.

[vstinner]

This issue is 6 years old and has patches: it is no newcomer friendly, I remove the "easy" keyword.