Fix handling of safe=False PYTHON-358

Author: rozza

pymongo/collection.py

@@ -186,7 +186,7 @@ def __set_uuid_subtype(self, subtype):
                             a UUID used for this collection.""")
 
     def save(self, to_save, manipulate=True,
-             safe=False, check_keys=True, **kwargs):
+             safe=None, check_keys=True, **kwargs):
         """Save a document in this collection.
 
         If `to_save` already has an ``"_id"`` then an :meth:`update`
@@ -239,7 +239,7 @@ def save(self, to_save, manipulate=True,
             return to_save.get("_id", None)
 
     def insert(self, doc_or_docs, manipulate=True,
-               safe=False, check_keys=True, continue_on_error=False, **kwargs):
+               safe=None, check_keys=True, continue_on_error=False, **kwargs):
         """Insert a document(s) into this collection.
 
         If `manipulate` is ``True``, the document(s) are manipulated using
@@ -302,21 +302,17 @@ def insert(self, doc_or_docs, manipulate=True,
         if manipulate:
             docs = [self.__database._fix_incoming(doc, self) for doc in docs]
 
-        if self.safe or kwargs:
-            safe = True
-            if not kwargs:
-                kwargs.update(self.get_lasterror_options())
-
+        safe, options = self._get_safe_and_lasterror_options(safe, **kwargs)
         self.__database.connection._send_message(
             message.insert(self.__full_name, docs,
-                           check_keys, safe, kwargs,
+                           check_keys, safe, options,
                            continue_on_error, self.__uuid_subtype), safe)
 
         ids = [doc.get("_id", None) for doc in docs]
         return return_one and ids[0] or ids
 
     def update(self, spec, document, upsert=False, manipulate=False,
-               safe=False, multi=False, _check_keys=False, **kwargs):
+               safe=None, multi=False, _check_keys=False, **kwargs):
         """Update a document(s) in this collection.
 
         Raises :class:`TypeError` if either `spec` or `document` is
@@ -398,17 +394,14 @@ def update(self, spec, document, upsert=False, manipulate=False,
         if manipulate:
             document = self.__database._fix_incoming(document, self)
 
-        if self.safe or kwargs:
-            safe = True
-            if not kwargs:
-                kwargs.update(self.get_lasterror_options())
+        safe, options = self._get_safe_and_lasterror_options(safe, **kwargs)
 
         # _check_keys is used by save() so we don't upsert pre-existing
         # documents after adding an invalid key like 'a.b'. It can't really
         # be used for any other update operations.
         return self.__database.connection._send_message(
             message.update(self.__full_name, upsert, multi,
-                           spec, document, safe, kwargs,
+                           spec, document, safe, options,
                            _check_keys, self.__uuid_subtype), safe)
 
     def drop(self):
@@ -423,7 +416,7 @@ def drop(self):
         """
         self.__database.drop_collection(self.__name)
 
-    def remove(self, spec_or_id=None, safe=False, **kwargs):
+    def remove(self, spec_or_id=None, safe=None, **kwargs):
         """Remove a document(s) from this collection.
 
         .. warning:: Calls to :meth:`remove` should be performed with
@@ -478,14 +471,10 @@ def remove(self, spec_or_id=None, safe=False, **kwargs):
         if not isinstance(spec_or_id, dict):
             spec_or_id = {"_id": spec_or_id}
 
-        if self.safe or kwargs:
-            safe = True
-            if not kwargs:
-                kwargs.update(self.get_lasterror_options())
-
+        safe, options = self._get_safe_and_lasterror_options(safe, **kwargs)
         return self.__database.connection._send_message(
-            message.delete(self.__full_name, spec_or_id,
-                           safe, kwargs, self.__uuid_subtype), safe)
+            message.delete(self.__full_name, spec_or_id, safe,
+                           options, self.__uuid_subtype), safe)
 
     def find_one(self, spec_or_id=None, *args, **kwargs):
         """Get a single document from the database.

pymongo/common.py

@@ -98,7 +98,7 @@ def validate_positive_float(option, value):
 
     return value
 
-    
+
 def validate_timeout_or_none(option, value):
     """Validates a timeout specified in milliseconds returning
     a value in floating point seconds.
@@ -195,7 +195,7 @@ def __init__(self, **options):
         self.__read_pref = ReadPreference.PRIMARY
         self.__tag_sets = [{}]
         self.__secondary_acceptable_latency_ms = 15
-        self.__safe = False
+        self.__safe = None
         self.__safe_opts = {}
         self.__set_options(options)
         if (self.__read_pref == ReadPreference.PRIMARY
@@ -204,6 +204,14 @@ def __init__(self, **options):
             raise ConfigurationError(
                 "ReadPreference PRIMARY cannot be combined with tags")
 
+        # If safe hasn't been implicitly set by write concerns then set it.
+        if self.__safe is None:
+            self.__safe = validate_boolean('safe', options.get("safe", False))
+        if self.__safe and not options.get("safe", True):
+            warnings.warn("Conflicting write concerns.  Safe set as False "
+                          "but write concerns have been set making safe True. "
+                          "Please set safe to True.", UserWarning)
+
     def __set_safe_option(self, option, value, check=False):
         """Validates and sets getlasterror options for this
         object (Connection, Database, Collection, etc.)
@@ -231,8 +239,6 @@ def __set_options(self, options):
             ):
                 self.__secondary_acceptable_latency_ms = \
                     validate_positive_float(option, value)
-            elif option == 'safe':
-                self.__safe = validate_boolean(option, value)
             elif option in SAFE_OPTIONS:
                 if option == 'journal':
                     self.__set_safe_option('j', value)
@@ -272,7 +278,7 @@ def __set_read_pref(self, value):
         self.__read_pref = validate_read_preference('read_preference', value)
 
     read_preference = property(__get_read_pref, __set_read_pref)
-    
+
     def __get_acceptable_latency(self):
         """Any replica-set member whose ping time is within
            secondary_acceptable_latency_ms of the nearest member may accept
@@ -366,3 +372,25 @@ def unset_lasterror_options(self, *options):
                 self.__safe_opts.pop(option, None)
         else:
             self.__safe_opts = {}
+
+    def _get_safe_and_lasterror_options(self, safe=None, **options):
+        """Get the current safe mode and any getLastError options.
+
+        Determines if the current write is safe or not based on the
+        passed in or inherited safe value.  Passing any write concerns
+        automatically sets safe to True.
+
+        :Parameters:
+            - `safe`: check that the operation succeeded?
+            - `**options`: overriding getLastError options
+
+        .. versionadded:: 2.2.1+
+        """
+        if safe is None:
+            safe = self.safe
+        safe = validate_boolean('safe', safe)
+        if safe or options:
+            safe = True
+            if not options:
+                options.update(self.get_lasterror_options())
+        return safe, options

test/test_collection.py

@@ -696,6 +696,7 @@ def test_insert_multiple(self):
     def test_insert_multiple_with_duplicate(self):
         db = self.db
         db.drop_collection("test")
+        db.safe = True
         db.test.ensure_index([('i', ASCENDING)], unique=True)
 
         # No error
@@ -706,11 +707,38 @@ def test_insert_multiple_with_duplicate(self):
         db.test.insert([{'i': 1}] * 2, safe=False)
         self.assertEqual(1, db.test.count())
 
+        self.assertRaises(
+            DuplicateKeyError,
+            lambda: db.test.insert([{'i': 2}] * 2),
+        )
+
+        # Test based on default setting as False
+        db.drop_collection("test")
+        db.safe = False
+        db.test.ensure_index([('i', ASCENDING)], unique=True)
+
+        # No error
+        db.test.insert([{'i': 1}] * 2)
+        self.assertEqual(1, db.test.count())
+
+        # Implied safe
+        self.assertRaises(
+            DuplicateKeyError,
+            lambda: db.test.insert([{'i': 2}] * 2, w=1),
+        )
+
+        # Explicit safe
         self.assertRaises(
             DuplicateKeyError,
             lambda: db.test.insert([{'i': 2}] * 2, safe=True),
         )
 
+        # Misconfigured value for safe
+        self.assertRaises(
+            TypeError,
+            lambda: db.test.insert([{'i': 2}] * 2, safe=1),
+        )
+
     def test_insert_iterables(self):
         db = self.db
 

test/test_common.py

@@ -32,13 +32,32 @@ class TestCommon(unittest.TestCase):
 
     def test_baseobject(self):
 
+        # In Python 2.6+ we could use the catch_warnings context
+        # manager to test this warning nicely. As we can't do that
+        # we must test raising errors before the ignore filter is applied.
+        warnings.simplefilter("error", UserWarning)
+        self.assertRaises(UserWarning, lambda:
+                Connection(wtimeout=1000, safe=False))
+        try:
+            Connection(wtimeout=1000, safe=True)
+        except UserWarning:
+            self.assertFalse(True)
+
+        try:
+            Connection(wtimeout=1000)
+        except UserWarning:
+            self.assertFalse(True)
+
+        warnings.resetwarnings()
+
         warnings.simplefilter("ignore")
 
         c = Connection(pair)
         self.assertFalse(c.slave_okay)
         self.assertFalse(c.safe)
         self.assertEqual({}, c.get_lasterror_options())
         db = c.test
+        db.drop_collection("test")
         self.assertFalse(db.slave_okay)
         self.assertFalse(db.safe)
         self.assertEqual({}, db.get_lasterror_options())
@@ -51,6 +70,9 @@ def test_baseobject(self):
         cursor = coll.find(slave_okay=True)
         self.assertTrue(cursor._Cursor__slave_okay)
 
+        # Setting any safe operations overrides explicit safe
+        self.assertTrue(Connection(wtimeout=1000, safe=False).safe)
+
         c = Connection(pair, slaveok=True, w='majority',
                                      wtimeout=300, fsync=True, j=True)
         self.assertTrue(c.slave_okay)
@@ -154,6 +176,7 @@ def test_baseobject(self):
         # Succeeds since we override the lasterror settings per query.
         self.assertTrue(coll.insert({'foo': 'bar'}, fsync=True))
         drop_collections(db)
+
         warnings.resetwarnings()