bpo-31505: Fix an assertion failure in json, in case _json.make_encoder() received a bad encoder() argument #3643
Conversation
Lib/test/test_json/test_speedups.py
Outdated
| None) | ||
|
|
||
| @test.support.cpython_only | ||
| def test_issue31505(self): |
There was a problem hiding this comment.
I think this can be named test_bad_str_encoder.
Lib/test/test_json/test_speedups.py
Outdated
| b"\xCD\x7D\x3D\x4E\x12\x4C\xF9\x79\xD7\x52\xBA\x82\xF2\x27\x4A\x7D\xA0\xCA\x75", | ||
| None) | ||
|
|
||
| @test.support.cpython_only |
There was a problem hiding this comment.
I think this test should be passed on other implementations that have compatible c_make_encoder.
There was a problem hiding this comment.
i thought about using test.support.get_attribute() to check whether c_make_encoder() is available, but test_make_encoder() just uses it, and would have failed if it hadn't existed.
maybe we can assume that every other C implementations implements test_make_encoder()?
There was a problem hiding this comment.
I think that if other C implementation doesn't implement test_make_encoder() we should get a request for making this test optional.
| def test_make_encoder(self): | ||
| self.assertRaises(TypeError, self.json.encoder.c_make_encoder, | ||
| (True, False), | ||
| b"\xCD\x7D\x3D\x4E\x12\x4C\xF9\x79\xD7\x52\xBA\x82\xF2\x27\x4A\x7D\xA0\xCA\x75", |
There was a problem hiding this comment.
This isn't related to this issue, but it seems to me that TypeError is raised for different cause than was originally in this test. This test doesn't work as intended.
There was a problem hiding this comment.
IIUC, Victor wrote this test to verify that the interpreter doesn't crash (as part of https://bugs.python.org/issue6986, as originally PyArg_ParseTupleAndKeywords() was given actual fields of the new PyEncoderObject, instead of temp vars), so raising a TypeError is the wanted outcome here.
maybe we can just change the name of the test to 'test_issue6986' (as it tests only this specific issue)?
and maybe do the same for test_make_scanner()?
anyway, I guess any such change should be in a PR referring to bpo-6986..
There was a problem hiding this comment.
Seems your are right. Thank you for a reference.
Lib/test/test_json/test_speedups.py
Outdated
| # receives a bad encoder() argument. | ||
| def _bad_encoder1(*args): | ||
| return None | ||
| enc = self.json.encoder.c_make_encoder(None, None, _bad_encoder1, None, |
There was a problem hiding this comment.
Use more meaningful arguments. Otherwise TypeError could be raised for different reason (for example if default is not a callable).
c_make_encoder(None, default, _bad_encoder1, None, ': ', ', ', False, False, False)
Lib/test/test_json/test_speedups.py
Outdated
| def test_issue31505(self): | ||
| # There shouldn't be an assertion failure in case c_make_encoder() | ||
| # receives a bad encoder() argument. | ||
| def _bad_encoder1(*args): |
There was a problem hiding this comment.
No need to use starting underscore. This is a local name.
Lib/test/test_json/test_speedups.py
Outdated
| enc = self.json.encoder.c_make_encoder(None, None, _bad_encoder1, None, | ||
| 'foo', 'bar', None, None, None) | ||
| with self.assertRaises(TypeError): | ||
| enc(obj='spam', _current_indent_level=4) |
There was a problem hiding this comment.
Why keyword arguments are used? In the code an encoder is called with positional arguments.
| return s->fast_encode(NULL, obj); | ||
| else | ||
| return PyObject_CallFunctionObjArgs(s->encoder, obj, NULL); | ||
| encoded = PyObject_CallFunctionObjArgs(s->encoder, obj, NULL); |
There was a problem hiding this comment.
Add braces around return above.
serhiy-storchaka
left a comment
serhiy-storchaka
left a comment
There was a problem hiding this comment.
LGTM.
I prefer different form for assertions, but will merge the PR in any case.
| bad_encoder1, None, ': ', ', ', | ||
| False, False, False) | ||
| with self.assertRaises(TypeError): | ||
| enc('spam', 4) |
There was a problem hiding this comment.
This can be written without context manager:
self.assertRaises(TypeError, enc, 'spam', 4)
I prefer this form for simple function calls.
serhiy-storchaka
added
needs backport to 2.7
type-bug
|
Thanks @orenmn for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 2.7, 3.6. |
|
Thanks @orenmn for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 2.7, 3.6. |
|
Thanks @orenmn for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 2.7, 3.6. |
|
Thanks @orenmn for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 2.7, 3.6. |
|
Sorry @orenmn and @serhiy-storchaka, I had trouble checking out the |
|
GH-3777 is a backport of this pull request to the 3.6 branch. |
…_encoder() received a bad encoder() argument. (pythonGH-3643) (cherry picked from commit 2b382dd)
6 participants
_json.c: add a check whetherencoder()hasn't returned a string.test_json/test_speedups.py: add a test to verify that the assertion failure is no more, and that the patch doesn't break anything whenencoder()fails.https://bugs.python.org/issue31505