PYTHON-4786 - Fix UpdateResult.did_upsert TypeError #1878
Conversation
pymongo/results.py
Outdated
| assert self.__raw_result is not None | ||
| return len(self.__raw_result.get("upserted", {})) > 0 | ||
| result = self.__raw_result.get("upserted") | ||
| return result is not None and len(str(result)) > 0 |
There was a problem hiding this comment.
This doesn't handle the None case correctly.
There was a problem hiding this comment.
We need a few more tests. For both client.bulk_write and collection.update_one() that tests upserting a document with "_id": None.
There was a problem hiding this comment.
Do we expect something besides False if the result is None?
There was a problem hiding this comment.
Do we allow upserting a document's id to be None?
|
For example: >>> res = client.t.t.update_one({"_id": None}, {'$set': {'x': 1}}, upsert=True)
>>> res
UpdateResult({'n': 1, 'upserted': None, 'nModified': 0, 'ok': 1.0, 'updatedExisting': False}, acknowledged=True)
>>> res.upserted_id
>>> res.upserted_id is None
True
>>> res.did_upsert
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/Users/shane/git/mongo-python-driver/pymongo/results.py", line 176, in did_upsert
return len(self.__raw_result.get("upserted", {})) > 0
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
TypeError: object of type 'NoneType' has no len() |
I can't reproduce this locally, I get this result instead: >>> from pymongo import MongoClient
>>> client = MongoClient()
>>> res = client.t.t.update_one({"_id": None}, {'$set': {'x': 1}}, upsert=True)
>>> res
UpdateResult({'n': 1, 'nModified': 0, 'ok': 1.0, 'updatedExisting': True}, acknowledged=True)
>>> res.upserted_id
>>> res.upserted_id is None
True
>>> res.did_upsert
FalseThe current change to |
|
The problem with your repro attempt is that it did not perform an upsert (see Here's an example using the update command directly: >>> client.t4.test.delete_many({})
DeleteResult({'n': 1, 'ok': 1.0}, acknowledged=True)
>>> client.t4.command({'update': 'test', 'updates': [{'q': {'_id': None}, 'u': {'$set': {'foo': 'bar'}}, 'upsert': True, 'multi': False}]})
{'n': 1, 'upserted': [{'index': 0, '_id': None}], 'nModified': 0, 'ok': 1.0}
>>> client.t4.command({'update': 'test', 'updates': [{'q': {'_id': None}, 'u': {'$set': {'foo': 'bar'}}, 'upsert': True, 'multi': False}]})
{'n': 1, 'nModified': 0, 'ok': 1.0} |
Good catch--I tried again with a fresh DB and successfully reproduced an incorrect result without an error: >>> from pymongo import MongoClient
>>> client = MongoClient()
>>> res = client.t.t.update_one({"_id": None}, {'$set': {'x': 1}}, upsert=True)
>>> res
UpdateResult({'n': 1, 'upserted': None, 'nModified': 0, 'ok': 1.0, 'updatedExisting': False}, acknowledged=True)
>>> res.did_upsert
FalseI'll add a test to catch this case with a fix. |
|
drivers-pr-bot please backport to v4.9 |
test/test_client_bulk_write.py
Outdated
| @@ -550,6 +550,47 @@ def test_returns_error_if_auto_encryption_configured(self): | |||
| "bulk_write does not currently support automatic encryption", context.exception._message | |||
| ) | |||
|
|
|||
| @client_context.require_version_min(8, 0, 0, -24) | |||
| @client_context.require_no_serverless | |||
| @unittest.skipUnless(_HAVE_PYMONGOCRYPT, "pymongocrypt is not installed") | |||
There was a problem hiding this comment.
Why require _HAVE_PYMONGOCRYPT?
There was a problem hiding this comment.
Whoops, artifact of copy-and-paste.
pymongo/results.py
Outdated
| @@ -171,9 +171,9 @@ def upserted_id(self) -> Any: | |||
|
|
|||
| @property | |||
| def did_upsert(self) -> bool: | |||
| """Whether or not an upsert took place.""" | |||
| """Whether an upsert took place.""" | |||
There was a problem hiding this comment.
Please add .. versionadded:: 4.9
|
drivers-pr-bot please backport to v4.9 |
(cherry picked from commit 7848feb)
No description provided.