-
-
Notifications
You must be signed in to change notification settings - Fork 35.1k
gh-62040: Raise ValueError on invalid *errors* argument in several codecs
#136611
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
cb8197a
f7d77c4
289c590
3b73dd6
59a62aa
a254fec
5d92e64
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,14 +8,20 @@ | |
| import codecs | ||
| import base64 | ||
|
|
||
| ### Codec Helpers | ||
|
|
||
| def _assert_strict(errors): | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This function is duplicated in six modules. Since all of them already import
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed; this helper should live in codecs.py. |
||
| if errors != 'strict': | ||
| raise ValueError(f'Unsupported error handling mode: "{errors}" - must be "strict"') | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "error handling mode" does not occur anywhere in the code or documentation. Only "error handling"/"error handler".
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "error handling parameter" would be clearer. |
||
|
|
||
| ### Codec APIs | ||
|
|
||
| def base64_encode(input, errors='strict'): | ||
| assert errors == 'strict' | ||
| _assert_strict(errors) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, now that I see where you are using the helper, I'm not so sure that this is the right approach anymore. "assert" only works when Python is run without -O, so the helper does change semantics. I'm also worried that the extra function call will make things slower. The change in semantics should be documented and I would prefer to have the checks inlined in all these cases. Yes, it's duplication, but those are only two lines of code and the error message could additionally include information about which encoding caused the problem. |
||
| return (base64.encodebytes(input), len(input)) | ||
|
|
||
| def base64_decode(input, errors='strict'): | ||
| assert errors == 'strict' | ||
| _assert_strict(errors) | ||
| return (base64.decodebytes(input), len(input)) | ||
|
|
||
| class Codec(codecs.Codec): | ||
|
|
@@ -26,12 +32,12 @@ def decode(self, input, errors='strict'): | |
|
|
||
| class IncrementalEncoder(codecs.IncrementalEncoder): | ||
| def encode(self, input, final=False): | ||
| assert self.errors == 'strict' | ||
| _assert_strict(self.errors) | ||
| return base64.encodebytes(input) | ||
|
|
||
| class IncrementalDecoder(codecs.IncrementalDecoder): | ||
| def decode(self, input, final=False): | ||
| assert self.errors == 'strict' | ||
| _assert_strict(self.errors) | ||
| return base64.decodebytes(input) | ||
|
|
||
| class StreamWriter(Codec, codecs.StreamWriter): | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3129,6 +3129,20 @@ def test_uu_invalid(self): | |
| # Missing "begin" line | ||
| self.assertRaises(ValueError, codecs.decode, b"", "uu-codec") | ||
|
|
||
| def test_invalid_error_input(self): | ||
| for encoding in bytes_transform_encodings: | ||
| with self.subTest(encoding=encoding): | ||
| encoder = codecs.getencoder(encoding) | ||
| decoder = codecs.getdecoder(encoding) | ||
|
|
||
| self.assertRaises(ValueError, encoder, 'in', errors='notstrict') | ||
| self.assertRaises(ValueError, decoder, 'in', errors='notstrict') | ||
|
Comment on lines
+3138
to
+3139
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Input should be bytes, not str. |
||
|
|
||
| incdev = codecs.getincrementaldecoder(encoding) | ||
| if encoding not in ('base64_codec', 'uu_codec', 'quopri_codec', 'hex_codec'): | ||
|
StanFromIreland marked this conversation as resolved.
|
||
| self.assertRaises(ValueError, incdev, errors='notstrict') | ||
|
|
||
|
|
||
|
|
||
| # The codec system tries to add notes to exceptions in order to ensure | ||
| # the error mentions the operation being performed and the codec involved. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| The ``base64_codec``, ``uu_codec``, ``quopri_codec``, ``hex_codec``, | ||
| ``zlib_codec`` and ``bz2_codec`` now raise a :exc:`ValueError` when their | ||
| decoder/encoder is provided an *errors* parameter that is not equal to | ||
| ``'strict'``. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe
_check_strictfrom the original patch would be better?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer
assert_strictsince it is closer to what was there before, and it is more obvious, to me at least, that it will fail iferrors != 'strict', whereascheck_strict, seems like something that will enable more verbose errors or the like.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, it's not fully correct to name method
assert_somethingand don't make anyassertchecks or raising anAssertionError.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@malemburg which do you prefer?
_check_strictor_assert_strictThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a preference. Both are fine.