-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
PYTHON-1982 Update Invalid Document error message to include doc #1854
Changes from all commits
5c6fc58
65f9beb
73d38c8
50d9130
491b2b4
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 |
---|---|---|
|
@@ -1743,6 +1743,41 @@ int write_dict(PyObject* self, buffer_t buffer, | |
while (PyDict_Next(dict, &pos, &key, &value)) { | ||
if (!decode_and_write_pair(self, buffer, key, value, | ||
check_keys, options, top_level)) { | ||
if (PyErr_Occurred()) { | ||
PyObject *etype = NULL, *evalue = NULL, *etrace = NULL; | ||
PyErr_Fetch(&etype, &evalue, &etrace); | ||
PyObject *InvalidDocument = _error("InvalidDocument"); | ||
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. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi @navjots18, apologies for the delay. I think all we're missing in this block is initializing the 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. Hey @blink1073 fixed these issues, can you review again? |
||
|
||
if (top_level && InvalidDocument && PyErr_GivenExceptionMatches(etype, InvalidDocument)) { | ||
|
||
Py_DECREF(etype); | ||
etype = InvalidDocument; | ||
|
||
if (evalue) { | ||
PyObject *msg = PyObject_Str(evalue); | ||
Py_DECREF(evalue); | ||
|
||
if (msg) { | ||
// Prepend doc to the existing message | ||
PyObject *dict_str = PyObject_Str(dict); | ||
PyObject *new_msg = PyUnicode_FromFormat("Invalid document %s | %s", PyUnicode_AsUTF8(dict_str), PyUnicode_AsUTF8(msg)); | ||
Py_DECREF(dict_str); | ||
|
||
if (new_msg) { | ||
evalue = new_msg; | ||
} | ||
else { | ||
evalue = msg; | ||
} | ||
} | ||
} | ||
PyErr_NormalizeException(&etype, &evalue, &etrace); | ||
} | ||
else { | ||
Py_DECREF(InvalidDocument); | ||
} | ||
PyErr_Restore(etype, evalue, etrace); | ||
} | ||
return 0; | ||
} | ||
} | ||
|
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'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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ShaneHarvey You're right, we can check the top_level param to prevent this from happening