PYTHON-1982 Update Invalid Document error message to include doc #1854
Conversation
navjots18
changed the title
|
Thanks @navjots18! It looks like there is a segmentation fault in the C implementation. Are you able to reproduce that failure locally? I can take a closer look on Monday, but https://dnmtechs.com/handling-segmentation-fault-errors-in-python-3-programming/ is a good resource. |
|
Hi @blink1073 , it was happening because i was assigning some variables to eType and eValue and then using Py_DECREF on the variables. Changed this which fixed the segmentation error on my local |
bson/_cbsonmodule.c
Outdated
| PyErr_Fetch(&etype, &evalue, &etrace); | ||
| PyObject *InvalidDocument = _error("InvalidDocument"); | ||
|
|
||
| if (PyErr_GivenExceptionMatches(etype, InvalidDocument)) { |
There was a problem hiding this comment.
Can you please reduce the nesting by using if (InvalidDocument && PyErr_GivenExceptionMatches(etype, InvalidDocument))?
| try: | ||
| elements.append(_element_to_bson(key, value, check_keys, opts)) | ||
| except InvalidDocument as err: | ||
| raise InvalidDocument(f"Invalid document {doc} | {err}") from err |
There was a problem hiding this comment.
I'm not sure this is the pattern we want, in the C version of this code we recursively call write_dict() for subdocuments which means if we end up erroring in a nested field the error will be something like:
Invalid document {"a": {"b": {"c": ...}}} | Invalid document {"b": {"c": ...}} | Invalid document {"c": ...} | ...
Is that the intended behavior?
There was a problem hiding this comment.
@ShaneHarvey You're right, we can check the top_level param to prevent this from happening
| if (PyErr_Occurred()) { | ||
| PyObject *etype, *evalue, *etrace; | ||
| PyErr_Fetch(&etype, &evalue, &etrace); | ||
| PyObject *InvalidDocument = _error("InvalidDocument"); |
There was a problem hiding this comment.
There's a lot of missing error/NULL checks in this code. I'm not comfortable merging this for 4.9 so it will need to wait for the next release.
There was a problem hiding this comment.
@blink1073 @ShaneHarvey I took context from code written in the same file, would appreciate if can you point out some existing code from where i can see what all error/Nulls check to put here?
There was a problem hiding this comment.
Hi @navjots18, apologies for the delay. I think all we're missing in this block is initializing the etype, evalue, and etrace to NULL as we do here. Otherwise I agree this matches the rest of the existing code block.
There was a problem hiding this comment.
Hey @blink1073 fixed these issues, can you review again?
blink1073
changed the title
blink1073
changed the title
Updated bson and cbson code to add the doc in the Invalid Document error message and added Unit test cases.
Note: Make sure to run to
python _setup.py build_ext --inplaceto install C file changes