From 480a61d21498e55b48b5f5ab38384c68e04c291d Mon Sep 17 00:00:00 2001 From: Shruti Sridhar Date: Tue, 13 Aug 2024 14:30:06 -0700 Subject: [PATCH 1/4] PYTHON-4660 Fix AttributeError when accessing error.details in client.bulk_write --- pymongo/asynchronous/client_bulk.py | 3 ++- pymongo/synchronous/client_bulk.py | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/pymongo/asynchronous/client_bulk.py b/pymongo/asynchronous/client_bulk.py index 671d989c25..1f3cca2f6c 100644 --- a/pymongo/asynchronous/client_bulk.py +++ b/pymongo/asynchronous/client_bulk.py @@ -550,7 +550,8 @@ async def _execute_command( if result.get("error"): error = result["error"] retryable_top_level_error = ( - isinstance(error.details, dict) + hasattr(error, "details") + and isinstance(error.details, dict) and error.details.get("code", 0) in _RETRYABLE_ERROR_CODES ) retryable_network_error = isinstance( diff --git a/pymongo/synchronous/client_bulk.py b/pymongo/synchronous/client_bulk.py index 229abd4330..5f969804d5 100644 --- a/pymongo/synchronous/client_bulk.py +++ b/pymongo/synchronous/client_bulk.py @@ -548,7 +548,8 @@ def _execute_command( if result.get("error"): error = result["error"] retryable_top_level_error = ( - isinstance(error.details, dict) + hasattr(error, "details") + and isinstance(error.details, dict) and error.details.get("code", 0) in _RETRYABLE_ERROR_CODES ) retryable_network_error = isinstance( From 335843ed335e2be740d18c9cba8b59d49c13c829 Mon Sep 17 00:00:00 2001 From: Shruti Sridhar Date: Wed, 14 Aug 2024 15:56:37 -0700 Subject: [PATCH 2/4] Add test case for handling non-PyMongoError --- pymongo/asynchronous/mongo_client.py | 4 ++- pymongo/synchronous/mongo_client.py | 4 ++- test/asynchronous/pymongo_mocks.py | 16 +++++++++++- test/asynchronous/test_client_bulk_write.py | 27 ++++++++++++++++++++- test/pymongo_mocks.py | 16 +++++++++++- test/test_client_bulk_write.py | 27 ++++++++++++++++++++- 6 files changed, 88 insertions(+), 6 deletions(-) diff --git a/pymongo/asynchronous/mongo_client.py b/pymongo/asynchronous/mongo_client.py index fbbd9a4eed..8848fa4fd5 100644 --- a/pymongo/asynchronous/mongo_client.py +++ b/pymongo/asynchronous/mongo_client.py @@ -2562,7 +2562,9 @@ async def run(self) -> T: if not self._retryable: raise if isinstance(exc, ClientBulkWriteException) and exc.error: - retryable_write_error_exc = exc.error.has_error_label("RetryableWriteError") + retryable_write_error_exc = isinstance( + exc.error, PyMongoError + ) and exc.error.has_error_label("RetryableWriteError") else: retryable_write_error_exc = exc.has_error_label("RetryableWriteError") if retryable_write_error_exc: diff --git a/pymongo/synchronous/mongo_client.py b/pymongo/synchronous/mongo_client.py index 1863165625..4aff3b5eed 100644 --- a/pymongo/synchronous/mongo_client.py +++ b/pymongo/synchronous/mongo_client.py @@ -2549,7 +2549,9 @@ def run(self) -> T: if not self._retryable: raise if isinstance(exc, ClientBulkWriteException) and exc.error: - retryable_write_error_exc = exc.error.has_error_label("RetryableWriteError") + retryable_write_error_exc = isinstance( + exc.error, PyMongoError + ) and exc.error.has_error_label("RetryableWriteError") else: retryable_write_error_exc = exc.has_error_label("RetryableWriteError") if retryable_write_error_exc: diff --git a/test/asynchronous/pymongo_mocks.py b/test/asynchronous/pymongo_mocks.py index ed2395bc98..e96a090fb3 100644 --- a/test/asynchronous/pymongo_mocks.py +++ b/test/asynchronous/pymongo_mocks.py @@ -22,11 +22,14 @@ from test.asynchronous import async_client_context from pymongo import AsyncMongoClient, common +from pymongo.asynchronous.client_bulk import _AsyncClientBulk from pymongo.asynchronous.monitor import Monitor from pymongo.asynchronous.pool import Pool -from pymongo.errors import AutoReconnect, NetworkTimeout +from pymongo.errors import AutoReconnect, ClientBulkWriteException, NetworkTimeout from pymongo.hello import Hello, HelloCompat +from pymongo.operations import _Op from pymongo.server_description import ServerDescription +from pymongo.write_concern import WriteConcern _IS_SYNC = False @@ -246,7 +249,18 @@ def mock_hello(self, host): return response, rtt + async def mock_client_bulk_write(self, models): + blk = _AsyncMockClientBulk(self, write_concern=WriteConcern(w=1)) + for model in models: + model._add_to_client_bulk(blk) + return await blk.execute(None, _Op.BULK_WRITE) + def _process_periodic_tasks(self): # Avoid the background thread causing races, e.g. a surprising # reconnect while we're trying to test a disconnected client. pass + + +class _AsyncMockClientBulk(_AsyncClientBulk): + async def write_command(self, bwc, cmd, request_id, msg, to_send_ops, to_send_ns, client): + return {"error": TypeError("mock type error")} diff --git a/test/asynchronous/test_client_bulk_write.py b/test/asynchronous/test_client_bulk_write.py index eea0b4e8e0..8b255c3c82 100644 --- a/test/asynchronous/test_client_bulk_write.py +++ b/test/asynchronous/test_client_bulk_write.py @@ -19,7 +19,13 @@ sys.path[0:0] = [""] -from test.asynchronous import AsyncIntegrationTest, async_client_context, unittest +from test.asynchronous import ( + AsyncIntegrationTest, + AsyncMockClientTest, + async_client_context, + unittest, +) +from test.asynchronous.pymongo_mocks import AsyncMockClient from test.utils import ( OvertCommandListener, async_rs_or_single_client, @@ -577,3 +583,22 @@ async def test_timeout_in_multi_batch_bulk_write(self): if event.command_name == "bulkWrite": bulk_write_events.append(event) self.assertEqual(len(bulk_write_events), 2) + + +class TestClientBulkWriteMock(AsyncMockClientTest): + @async_client_context.require_version_min(8, 0, 0, -24) + async def test_handles_non_pymongo_error(self): + mock_client = await AsyncMockClient.get_async_mock_client( + standalones=[], + members=["a:1", "b:2", "c:3"], + mongoses=[], + host="b:2", # Pass a secondary. + replicaSet="rs", + heartbeatFrequencyMS=500, + ) + self.addAsyncCleanup(mock_client.aclose) + models = [InsertOne(namespace="db.coll", document={"a": "b"})] + with self.assertRaises(ClientBulkWriteException) as context: + await mock_client.mock_client_bulk_write(models=models) + self.assertIsInstance(context.exception.error, TypeError) + self.assertFalse(hasattr(context.exception.error, "details")) diff --git a/test/pymongo_mocks.py b/test/pymongo_mocks.py index 7662dc9682..5f49f34a8e 100644 --- a/test/pymongo_mocks.py +++ b/test/pymongo_mocks.py @@ -21,11 +21,14 @@ from test import client_context from pymongo import MongoClient, common -from pymongo.errors import AutoReconnect, NetworkTimeout +from pymongo.errors import AutoReconnect, ClientBulkWriteException, NetworkTimeout from pymongo.hello import Hello, HelloCompat +from pymongo.operations import _Op from pymongo.server_description import ServerDescription +from pymongo.synchronous.client_bulk import _ClientBulk from pymongo.synchronous.monitor import Monitor from pymongo.synchronous.pool import Pool +from pymongo.write_concern import WriteConcern _IS_SYNC = True @@ -245,7 +248,18 @@ def mock_hello(self, host): return response, rtt + def mock_client_bulk_write(self, models): + blk = _MockClientBulk(self, write_concern=WriteConcern(w=1)) + for model in models: + model._add_to_client_bulk(blk) + return blk.execute(None, _Op.BULK_WRITE) + def _process_periodic_tasks(self): # Avoid the background thread causing races, e.g. a surprising # reconnect while we're trying to test a disconnected client. pass + + +class _MockClientBulk(_ClientBulk): + def write_command(self, bwc, cmd, request_id, msg, to_send_ops, to_send_ns, client): + return {"error": TypeError("mock type error")} diff --git a/test/test_client_bulk_write.py b/test/test_client_bulk_write.py index 8f6aad0cfa..3e8c04fa5d 100644 --- a/test/test_client_bulk_write.py +++ b/test/test_client_bulk_write.py @@ -19,7 +19,13 @@ sys.path[0:0] = [""] -from test import IntegrationTest, client_context, unittest +from test import ( + IntegrationTest, + MockClientTest, + client_context, + unittest, +) +from test.pymongo_mocks import MockClient from test.utils import ( OvertCommandListener, rs_or_single_client, @@ -577,3 +583,22 @@ def test_timeout_in_multi_batch_bulk_write(self): if event.command_name == "bulkWrite": bulk_write_events.append(event) self.assertEqual(len(bulk_write_events), 2) + + +class TestClientBulkWriteMock(MockClientTest): + @client_context.require_version_min(8, 0, 0, -24) + def test_handles_non_pymongo_error(self): + mock_client = MockClient.get_mock_client( + standalones=[], + members=["a:1", "b:2", "c:3"], + mongoses=[], + host="b:2", # Pass a secondary. + replicaSet="rs", + heartbeatFrequencyMS=500, + ) + self.addCleanup(mock_client.close) + models = [InsertOne(namespace="db.coll", document={"a": "b"})] + with self.assertRaises(ClientBulkWriteException) as context: + mock_client.mock_client_bulk_write(models=models) + self.assertIsInstance(context.exception.error, TypeError) + self.assertFalse(hasattr(context.exception.error, "details")) From a84430cb68f59096d4cf1cabc7c31af037299b63 Mon Sep 17 00:00:00 2001 From: Shruti Sridhar Date: Wed, 14 Aug 2024 16:05:10 -0700 Subject: [PATCH 3/4] rebase --- test/asynchronous/test_client_bulk_write.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/asynchronous/test_client_bulk_write.py b/test/asynchronous/test_client_bulk_write.py index 8b255c3c82..e60e80d1cb 100644 --- a/test/asynchronous/test_client_bulk_write.py +++ b/test/asynchronous/test_client_bulk_write.py @@ -596,7 +596,7 @@ async def test_handles_non_pymongo_error(self): replicaSet="rs", heartbeatFrequencyMS=500, ) - self.addAsyncCleanup(mock_client.aclose) + self.addAsyncCleanup(mock_client.close) models = [InsertOne(namespace="db.coll", document={"a": "b"})] with self.assertRaises(ClientBulkWriteException) as context: await mock_client.mock_client_bulk_write(models=models) From f4853a99cc54d35da96815970859cec98eaf46be Mon Sep 17 00:00:00 2001 From: Shruti Sridhar Date: Thu, 15 Aug 2024 13:20:31 -0700 Subject: [PATCH 4/4] Use unittest.mock.patch for the test --- test/asynchronous/pymongo_mocks.py | 16 +-------- test/asynchronous/test_client_bulk_write.py | 37 +++++++++------------ test/pymongo_mocks.py | 16 +-------- test/test_client_bulk_write.py | 37 +++++++++------------ 4 files changed, 34 insertions(+), 72 deletions(-) diff --git a/test/asynchronous/pymongo_mocks.py b/test/asynchronous/pymongo_mocks.py index e96a090fb3..ed2395bc98 100644 --- a/test/asynchronous/pymongo_mocks.py +++ b/test/asynchronous/pymongo_mocks.py @@ -22,14 +22,11 @@ from test.asynchronous import async_client_context from pymongo import AsyncMongoClient, common -from pymongo.asynchronous.client_bulk import _AsyncClientBulk from pymongo.asynchronous.monitor import Monitor from pymongo.asynchronous.pool import Pool -from pymongo.errors import AutoReconnect, ClientBulkWriteException, NetworkTimeout +from pymongo.errors import AutoReconnect, NetworkTimeout from pymongo.hello import Hello, HelloCompat -from pymongo.operations import _Op from pymongo.server_description import ServerDescription -from pymongo.write_concern import WriteConcern _IS_SYNC = False @@ -249,18 +246,7 @@ def mock_hello(self, host): return response, rtt - async def mock_client_bulk_write(self, models): - blk = _AsyncMockClientBulk(self, write_concern=WriteConcern(w=1)) - for model in models: - model._add_to_client_bulk(blk) - return await blk.execute(None, _Op.BULK_WRITE) - def _process_periodic_tasks(self): # Avoid the background thread causing races, e.g. a surprising # reconnect while we're trying to test a disconnected client. pass - - -class _AsyncMockClientBulk(_AsyncClientBulk): - async def write_command(self, bwc, cmd, request_id, msg, to_send_ops, to_send_ns, client): - return {"error": TypeError("mock type error")} diff --git a/test/asynchronous/test_client_bulk_write.py b/test/asynchronous/test_client_bulk_write.py index e60e80d1cb..20e6ab7c95 100644 --- a/test/asynchronous/test_client_bulk_write.py +++ b/test/asynchronous/test_client_bulk_write.py @@ -21,16 +21,16 @@ from test.asynchronous import ( AsyncIntegrationTest, - AsyncMockClientTest, async_client_context, unittest, ) -from test.asynchronous.pymongo_mocks import AsyncMockClient from test.utils import ( OvertCommandListener, async_rs_or_single_client, ) +from unittest.mock import patch +from pymongo.asynchronous.client_bulk import _AsyncClientBulk from pymongo.encryption_options import _HAVE_PYMONGOCRYPT, AutoEncryptionOpts from pymongo.errors import ( ClientBulkWriteException, @@ -59,6 +59,20 @@ async def test_returns_error_if_no_namespace_provided(self): context.exception._message, ) + @async_client_context.require_version_min(8, 0, 0, -24) + async def test_handles_non_pymongo_error(self): + with patch.object( + _AsyncClientBulk, "write_command", return_value={"error": TypeError("mock type error")} + ): + client = await async_rs_or_single_client() + self.addAsyncCleanup(client.close) + + models = [InsertOne(namespace="db.coll", document={"a": "b"})] + with self.assertRaises(ClientBulkWriteException) as context: + await client.bulk_write(models=models) + self.assertIsInstance(context.exception.error, TypeError) + self.assertFalse(hasattr(context.exception.error, "details")) + # https://github.com/mongodb/specifications/tree/master/source/crud/tests class TestClientBulkWriteCRUD(AsyncIntegrationTest): @@ -583,22 +597,3 @@ async def test_timeout_in_multi_batch_bulk_write(self): if event.command_name == "bulkWrite": bulk_write_events.append(event) self.assertEqual(len(bulk_write_events), 2) - - -class TestClientBulkWriteMock(AsyncMockClientTest): - @async_client_context.require_version_min(8, 0, 0, -24) - async def test_handles_non_pymongo_error(self): - mock_client = await AsyncMockClient.get_async_mock_client( - standalones=[], - members=["a:1", "b:2", "c:3"], - mongoses=[], - host="b:2", # Pass a secondary. - replicaSet="rs", - heartbeatFrequencyMS=500, - ) - self.addAsyncCleanup(mock_client.close) - models = [InsertOne(namespace="db.coll", document={"a": "b"})] - with self.assertRaises(ClientBulkWriteException) as context: - await mock_client.mock_client_bulk_write(models=models) - self.assertIsInstance(context.exception.error, TypeError) - self.assertFalse(hasattr(context.exception.error, "details")) diff --git a/test/pymongo_mocks.py b/test/pymongo_mocks.py index 5f49f34a8e..7662dc9682 100644 --- a/test/pymongo_mocks.py +++ b/test/pymongo_mocks.py @@ -21,14 +21,11 @@ from test import client_context from pymongo import MongoClient, common -from pymongo.errors import AutoReconnect, ClientBulkWriteException, NetworkTimeout +from pymongo.errors import AutoReconnect, NetworkTimeout from pymongo.hello import Hello, HelloCompat -from pymongo.operations import _Op from pymongo.server_description import ServerDescription -from pymongo.synchronous.client_bulk import _ClientBulk from pymongo.synchronous.monitor import Monitor from pymongo.synchronous.pool import Pool -from pymongo.write_concern import WriteConcern _IS_SYNC = True @@ -248,18 +245,7 @@ def mock_hello(self, host): return response, rtt - def mock_client_bulk_write(self, models): - blk = _MockClientBulk(self, write_concern=WriteConcern(w=1)) - for model in models: - model._add_to_client_bulk(blk) - return blk.execute(None, _Op.BULK_WRITE) - def _process_periodic_tasks(self): # Avoid the background thread causing races, e.g. a surprising # reconnect while we're trying to test a disconnected client. pass - - -class _MockClientBulk(_ClientBulk): - def write_command(self, bwc, cmd, request_id, msg, to_send_ops, to_send_ns, client): - return {"error": TypeError("mock type error")} diff --git a/test/test_client_bulk_write.py b/test/test_client_bulk_write.py index 3e8c04fa5d..686b60642a 100644 --- a/test/test_client_bulk_write.py +++ b/test/test_client_bulk_write.py @@ -21,15 +21,14 @@ from test import ( IntegrationTest, - MockClientTest, client_context, unittest, ) -from test.pymongo_mocks import MockClient from test.utils import ( OvertCommandListener, rs_or_single_client, ) +from unittest.mock import patch from pymongo.encryption_options import _HAVE_PYMONGOCRYPT, AutoEncryptionOpts from pymongo.errors import ( @@ -40,6 +39,7 @@ ) from pymongo.monitoring import * from pymongo.operations import * +from pymongo.synchronous.client_bulk import _ClientBulk from pymongo.write_concern import WriteConcern _IS_SYNC = True @@ -59,6 +59,20 @@ def test_returns_error_if_no_namespace_provided(self): context.exception._message, ) + @client_context.require_version_min(8, 0, 0, -24) + def test_handles_non_pymongo_error(self): + with patch.object( + _ClientBulk, "write_command", return_value={"error": TypeError("mock type error")} + ): + client = rs_or_single_client() + self.addCleanup(client.close) + + models = [InsertOne(namespace="db.coll", document={"a": "b"})] + with self.assertRaises(ClientBulkWriteException) as context: + client.bulk_write(models=models) + self.assertIsInstance(context.exception.error, TypeError) + self.assertFalse(hasattr(context.exception.error, "details")) + # https://github.com/mongodb/specifications/tree/master/source/crud/tests class TestClientBulkWriteCRUD(IntegrationTest): @@ -583,22 +597,3 @@ def test_timeout_in_multi_batch_bulk_write(self): if event.command_name == "bulkWrite": bulk_write_events.append(event) self.assertEqual(len(bulk_write_events), 2) - - -class TestClientBulkWriteMock(MockClientTest): - @client_context.require_version_min(8, 0, 0, -24) - def test_handles_non_pymongo_error(self): - mock_client = MockClient.get_mock_client( - standalones=[], - members=["a:1", "b:2", "c:3"], - mongoses=[], - host="b:2", # Pass a secondary. - replicaSet="rs", - heartbeatFrequencyMS=500, - ) - self.addCleanup(mock_client.close) - models = [InsertOne(namespace="db.coll", document={"a": "b"})] - with self.assertRaises(ClientBulkWriteException) as context: - mock_client.mock_client_bulk_write(models=models) - self.assertIsInstance(context.exception.error, TypeError) - self.assertFalse(hasattr(context.exception.error, "details"))