PYTHON-5449 - Do not attach invalid document in exception message #2539

NoahStapp · 2025-09-16T18:39:37Z

This PR removes the full document from the error message and stores it in the document property on the error instead.

ShaneHarvey · 2025-09-16T18:53:10Z

bson/errors.py

+
+    @property
+    def document(self) -> Any:
+        """The invalid document that caused the error."""


Please add a versionadded and changelog reference.

ShaneHarvey · 2025-09-16T18:53:28Z

bson/errors.py

+        return self._document
+
+    @document.setter
+    def document(self, value: Any) -> None:


Does this need to be public?

I can see this being a useful behavior for users, so making this public seems reasonable.

Sorry, I was suggesting we make it private, not public.

What's the reasoning for making it private? Security concerns if users misuse it? I find having the full document very useful for debugging and I imagine users do too.

I agree the getter should be public but when would we expect a user to set the document? We try to be conservative with public apis and only add ones that have a strong justification.

Oh whoops sorry I misread which method this was about. Totally agree, fixing.

ShaneHarvey · 2025-09-16T18:55:53Z

bson/_cbsonmodule.c

                goto cleanup;
            }
-            PyObject *new_msg = PyUnicode_FromFormat("Invalid document %s | %s", dict_str_utf8, msg_utf8);
+            PyObject *new_msg = PyUnicode_FromFormat("Invalid document: %s", msg_utf8);


For an invalid field in a nested doc, does this yield errors like "Invalid document: Invalid document: Invalid document: Invalid document: Invalid document: ...."?

No, it works as expected:

encode({"t": {"f": Wrapper(1)}}) # Produces Invalid document: cannot encode object: 1, of type: <class 'test.test_bson.TestBSON.test_doc_in_invalid_document_error_as_property.<locals>.Wrapper'>

ShaneHarvey · 2025-09-16T18:59:31Z

bson/__init__.py

                    elements.append(_element_to_bson(key, value, check_keys, opts))
                except InvalidDocument as err:
-                    raise InvalidDocument(f"Invalid document {doc} | {err}") from err
+                    raise InvalidDocument(f"Invalid document: {err}", doc) from err


Shouldn't this InvalidDocument except clause include the encoding of _id too?

That would be a pre-existing bug, no? Happy to fix if we identify why the code currently looks like this. Maybe we assume that _id will be valid if it reaches this far?

Correct it would be a pre-existing bug. Could you open a ticket for it?

>>>> encode({'_id': set()}) Traceback (most recent call last): File "<python-input-6>", line 1, in <module> encode({'_id': set()}) File "/Users/shane/git/mongo-python-driver/bson/__init__.py", line 1053, in encode return _dict_to_bson(document, check_keys, codec_options) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/Users/shane/git/mongo-python-driver/bson/__init__.py", line 1006, in _dict_to_bson elements.append(_name_value_to_bson(b"_id\x00", doc["_id"], check_keys, opts)) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/Users/shane/git/mongo-python-driver/bson/__init__.py", line 980, in _name_value_to_bson raise InvalidDocument(f"cannot encode object: {value!r}, of type: {type(value)!r}") bson.errors.InvalidDocument: cannot encode object: set(), of type: <class 'set'>

vs the expected behavior:

>>>> encode({'_id': 1, 'foo': set()}) Traceback (most recent call last): File "/Users/shane/git/mongo-python-driver/bson/__init__.py", line 1010, in _dict_to_bson elements.append(_element_to_bson(key, value, check_keys, opts)) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/Users/shane/git/mongo-python-driver/bson/__init__.py", line 994, in _element_to_bson return _name_value_to_bson(name, value, check_keys, opts) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/Users/shane/git/mongo-python-driver/bson/__init__.py", line 980, in _name_value_to_bson raise InvalidDocument(f"cannot encode object: {value!r}, of type: {type(value)!r}") bson.errors.InvalidDocument: cannot encode object: set(), of type: <class 'set'> The above exception was the direct cause of the following exception: Traceback (most recent call last): File "<python-input-7>", line 1, in <module> encode({'_id': 1, 'foo': set()}) File "/Users/shane/git/mongo-python-driver/bson/__init__.py", line 1053, in encode return _dict_to_bson(document, check_keys, codec_options) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/Users/shane/git/mongo-python-driver/bson/__init__.py", line 1012, in _dict_to_bson raise InvalidDocument(f"Invalid document {doc} | {err}") from err bson.errors.InvalidDocument: Invalid document {'_id': 1, 'foo': set()} | cannot encode object: set(), of type: <class 'set'>

Opened PYTHON-5560 to track.

ShaneHarvey · 2025-09-17T19:27:44Z

bson/_cbsonmodule.c

-                goto cleanup;
-            }
+            // Add doc to the error class as a property.
+            PyObject_SetAttrString(InvalidDocument, "document", dict);


Does this need to be "_document" now?

This also highlights that this PR needs to add a test to ensure .document is the expected dict.

Interestingly the tests fail if this is _document.

Are the two modified tests in this PR not sufficient to ensure that .document behaves as expected?

This line is setting a class attribute on the InvalidDocument class so the logic is incorrect, eg:

InvalidDocument.document = dict

Ah sorry I misunderstood what you meant. You're right that _error returns a class, not an instance.

Fixed. Had to do some C API delving to figure this out 😅

ShaneHarvey · 2025-09-18T18:32:44Z

bson/_cbsonmodule.c

            }
            PyObject *new_msg = PyUnicode_FromFormat("Invalid document: %s", msg_utf8);
+            // Add doc to the error instance as a property.
+            PyObject *new_evalue = PyObject_CallFunctionObjArgs(InvalidDocument, new_msg, dict, NULL);


Need to check the new_msg == NULL case before we call PyObject_CallFunctionObjArgs here.

PYTHON-5449 - Do not attach invalid document in exception message

296372e

NoahStapp requested review from ShaneHarvey and sleepyStick September 16, 2025 18:39

NoahStapp requested a review from a team as a code owner September 16, 2025 18:39

Merge branch 'master' into PYTHON-5449

cc07a59

ShaneHarvey reviewed Sep 16, 2025

View reviewed changes

Address review

4fb872e

NoahStapp requested a review from ShaneHarvey September 16, 2025 19:19

Make document setter private

ca9f09f

ShaneHarvey requested changes Sep 17, 2025

View reviewed changes

NoahStapp requested a review from ShaneHarvey September 17, 2025 19:39

sleepyStick previously approved these changes Sep 17, 2025

View reviewed changes

Fix C bson code

2319a43

NoahStapp dismissed sleepyStick’s stale review via 2319a43 September 18, 2025 14:13

NoahStapp requested a review from sleepyStick September 18, 2025 14:13

ShaneHarvey requested changes Sep 18, 2025

View reviewed changes

Check for NULL

6c73d1e

NoahStapp requested a review from ShaneHarvey September 18, 2025 19:03

PYTHON-5449 - Do not attach invalid document in exception message #2539

Are you sure you want to change the base?

PYTHON-5449 - Do not attach invalid document in exception message #2539

Uh oh!

Conversation

NoahStapp commented Sep 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NoahStapp Sep 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ShaneHarvey Sep 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NoahStapp Sep 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

NoahStapp commented Sep 16, 2025 •

edited

Loading

NoahStapp Sep 17, 2025 •

edited

Loading

ShaneHarvey Sep 17, 2025 •

edited

Loading

NoahStapp Sep 17, 2025 •

edited

Loading