PYTHON-3075 bulk_write does not apply CodecOptions to upserted_ids result #840
Conversation
|
LGTM! |
pymongo/bulk.py
Outdated
| _id = doc['_id'] | ||
| if rep == UuidRepresentation.UNSPECIFIED: | ||
| if isinstance(_id, uuid.UUID): | ||
| _id = Binary.from_uuid(_id) |
There was a problem hiding this comment.
I couldn't figure out how to trigger this condition
There was a problem hiding this comment.
This condition will only happen on pymongo 3.
pymongo/bulk.py
Outdated
| _id = doc['_id'] | ||
| if rep == UuidRepresentation.UNSPECIFIED: | ||
| if isinstance(_id, uuid.UUID): | ||
| _id = Binary.from_uuid(_id) |
There was a problem hiding this comment.
This condition will only happen on pymongo 3.
pymongo/bulk.py
Outdated
| @@ -317,10 +329,10 @@ def _execute_command(self, generator, write_concern, session, | |||
| # Synthesize the full bulk result without modifying the | |||
| # current one because this write operation may be retried. | |||
| full = copy.deepcopy(full_result) | |||
| _merge_command(run, full, run.idx_offset, result) | |||
| _merge_command(run, full, run.idx_offset, result, codec_options) | |||
There was a problem hiding this comment.
One problem with this approach is that it doesn't handle _id that are subdocuments, eg: _id={'f': Binary(), 'f2': ...}. Could you add a test for this as well?
Instead of converting the result after the fact, we should ensure that BulkWriteContext.execute() decodes the command response with the correct uuid_representation/CodecOptions to begin with. This will involve passing the _Bulk.collection or _Bulk.collection.codec_options to the BulkWriteContext to use for decoding.
Edit: I see that we already pass the _Bulk.collection.codec_options to the BulkWriteContext. The issue is that BulkWriteContext.write_command() does not utilize it when paring the command response.
There was a problem hiding this comment.
Cool, fixed the decoding in the latest commit. I'll address the tests in the morning.
There was a problem hiding this comment.
Added the subdocument test
| @@ -376,6 +379,27 @@ def test_client_generated_upsert_id(self): | |||
| {'index': 2, '_id': 2}]}, | |||
| result.bulk_api_result) | |||
|
|
|||
| def test_upsert_uuid_standard(self): | |||
| options = CodecOptions(uuid_representation=UuidRepresentation.STANDARD) | |||
There was a problem hiding this comment.
Nice. Could you add a similar test to ensure that UuidRepresentation.UNSPECIFIED results in Binary instances?
There was a problem hiding this comment.
As far as I can tell we can't do that because it raises a ValueError.
There was a problem hiding this comment.
Yes the test will need to convert the uuids to Binary manually before inserting them:
uuids = [Binary.from_uuid(uuid.uuid4()) for _ in range(3)]
| @@ -798,7 +798,7 @@ def write_command(self, cmd, request_id, msg, docs): | |||
| self._start(cmd, request_id, docs) | |||
| start = datetime.datetime.now() | |||
| try: | |||
| reply = self.sock_info.write_command(request_id, msg) | |||
| reply = self.sock_info.write_command(request_id, msg, self.codec) | |||
There was a problem hiding this comment.
I suspect we need a similar change in _EncryptedBulkWriteContext.execute().
There was a problem hiding this comment.
I wasn't sure how to test that locally, waiting for evergreen
No description provided.