Firestore: return non existent snapshot if document not found instead…

… of raising NotFound exception (#5007)

firestore/google/cloud/firestore_v1beta1/client.py

@@ -582,8 +582,7 @@ def _parse_batch_get(get_doc_response, reference_map, client):
             a document factory.
 
     Returns:
-        Optional[.DocumentSnapshot]: The retrieved snapshot. If the
-        snapshot is :data:`None`, that means the document is ``missing``.
+       [.DocumentSnapshot]: The retrieved snapshot.
 
     Raises:
         ValueError: If the response has a ``result`` field (a oneof) other
@@ -601,13 +600,19 @@ def _parse_batch_get(get_doc_response, reference_map, client):
             read_time=get_doc_response.read_time,
             create_time=get_doc_response.found.create_time,
             update_time=get_doc_response.found.update_time)
-        return snapshot
     elif result_type == 'missing':
-        return None
+        snapshot = DocumentSnapshot(
+            None,
+            None,
+            exists=False,
+            read_time=get_doc_response.read_time,
+            create_time=None,
+            update_time=None)
     else:
         raise ValueError(
             '`BatchGetDocumentsResponse.result` (a oneof) had a field other '
             'than `found` or `missing` set, or was unset')
+    return snapshot
 
 
 def _get_doc_mask(field_paths):

firestore/google/cloud/firestore_v1beta1/document.py

@@ -17,7 +17,6 @@
 
 import copy
 
-from google.cloud import exceptions
 from google.cloud.firestore_v1beta1 import _helpers
 
 
@@ -419,18 +418,14 @@ def get(self, field_paths=None, transaction=None):
 
         Returns:
             ~.firestore_v1beta1.document.DocumentSnapshot: A snapshot of
-            the current document.
-
-        Raises:
-            ~google.cloud.exceptions.NotFound: If the document does not exist.
+                the current document. If the document does not exist at
+                the time of `snapshot`, the snapshot `reference`, `data`,
+                `update_time`, and `create_time` attributes will all be
+                `None` and `exists` will be `False`.
         """
         snapshot_generator = self._client.get_all(
             [self], field_paths=field_paths, transaction=transaction)
-        snapshot = _consume_single_get(snapshot_generator)
-        if snapshot is None:
-            raise exceptions.NotFound(self._document_path)
-        else:
-            return snapshot
+        return _consume_single_get(snapshot_generator)
 
 
 class DocumentSnapshot(object):
@@ -566,12 +561,16 @@ def get(self, field_path):
                 field names).
 
         Returns:
-            Any: (A copy of) the value stored for the ``field_path``.
+            Any or None:
+                (A copy of) the value stored for the ``field_path`` or
+                None if snapshot document does not exist.
 
         Raises:
             KeyError: If the ``field_path`` does not match nested data
                 in the snapshot.
         """
+        if not self._exists:
+            return None
         nested_data = _helpers.get_nested_value(field_path, self._data)
         return copy.deepcopy(nested_data)
 
@@ -582,8 +581,12 @@ def to_dict(self):
         but the data stored in the snapshot must remain immutable.
 
         Returns:
-            Dict[str, Any]: The data in the snapshot.
+            Dict[str, Any] or None:
+                The data in the snapshot.  Returns None if reference
+                does not exist.
         """
+        if not self._exists:
+            return None
         return copy.deepcopy(self._data)
 
 

firestore/tests/system.py

@@ -129,6 +129,16 @@ def assert_timestamp_less(timestamp_pb1, timestamp_pb2):
     assert dt_val1 < dt_val2
 
 
+def test_no_document(client, cleanup):
+    document_id = 'no_document' + unique_resource_id('-')
+    document = client.document('abcde', document_id)
+    option0 = client.write_option(create_if_missing=False)
+    with pytest.raises(NotFound):
+        document.set({'no': 'way'}, option=option0)
+    snapshot = document.get()
+    assert snapshot.to_dict() is None
+
+
 def test_document_set(client, cleanup):
     document_id = 'for-set' + unique_resource_id('-')
     document = client.document('i-did-it', document_id)
@@ -313,10 +323,7 @@ def test_document_get(client, cleanup):
     cleanup(document)
 
     # First make sure it doesn't exist.
-    with pytest.raises(NotFound) as exc_info:
-        document.get()
-
-    assert exc_info.value.message == document._document_path
+    assert not document.get().exists
 
     ref_doc = client.document('top', 'middle1', 'middle2', 'bottom')
     data = {
@@ -631,8 +638,10 @@ def test_get_all(client, cleanup):
     snapshots = list(client.get_all(
         [document1, document2, document3]))
 
-    assert snapshots.count(None) == 1
-    snapshots.remove(None)
+    assert snapshots[0].exists
+    assert snapshots[1].exists
+    assert not snapshots[2].exists
+    snapshots = [snapshot for snapshot in snapshots if snapshot.exists]
     id_attr = operator.attrgetter('id')
     snapshots.sort(key=id_attr)
 
@@ -716,5 +725,4 @@ def test_batch(client, cleanup):
     assert_timestamp_less(snapshot2.create_time, write_result2.update_time)
     assert snapshot2.update_time == write_result2.update_time
 
-    with pytest.raises(NotFound):
-        document3.get()
+    assert not document3.get().exists

firestore/tests/unit/test_client.py

@@ -378,7 +378,7 @@ def test_get_all_wrong_order(self):
         self.assertIs(snapshot2._reference, document1)
         self.assertEqual(snapshot2._data, data1)
 
-        self.assertIsNone(snapshots[2])
+        self.assertFalse(snapshots[2].exists)
 
         # Verify the call to the mock.
         doc_paths = [
@@ -669,7 +669,7 @@ def test_missing(self):
         response_pb = _make_batch_response(missing=ref_string)
 
         snapshot = self._call_fut(response_pb, {})
-        self.assertIsNone(snapshot)
+        self.assertFalse(snapshot.exists)
 
     def test_unset_result_type(self):
         response_pb = _make_batch_response()

firestore/tests/unit/test_document.py

@@ -445,10 +445,12 @@ def test_get_with_transaction(self):
             [document], field_paths=None, transaction=transaction)
 
     def test_get_not_found(self):
-        from google.cloud.exceptions import NotFound
+        from google.cloud.firestore_v1beta1.document import DocumentSnapshot
 
         # Create a minimal fake client with a dummy response.
-        response_iterator = iter([None])
+        read_time = 123
+        expected = DocumentSnapshot(None, None, False, read_time, None, None)
+        response_iterator = iter([expected])
         client = mock.Mock(
             _database_string='sprinklez',
             spec=['_database_string', 'get_all'])
@@ -457,8 +459,13 @@ def test_get_not_found(self):
         # Actually make a document and call get().
         document = self._make_one('house', 'cowse', client=client)
         field_paths = ['x.y', 'x.z', 't']
-        with self.assertRaises(NotFound):
-            document.get(field_paths=field_paths)
+        snapshot = document.get(field_paths=field_paths)
+        self.assertIsNone(snapshot.reference)
+        self.assertIsNone(snapshot._data)
+        self.assertFalse(snapshot.exists)
+        self.assertEqual(snapshot.read_time, expected.read_time)
+        self.assertIsNone(snapshot.create_time)
+        self.assertIsNone(snapshot.update_time)
 
         # Verify the response and the mocks.
         client.get_all.assert_called_once_with(
@@ -538,6 +545,10 @@ def test_get(self):
         with self.assertRaises(KeyError):
             snapshot.get('two')
 
+    def test_nonexistent_snapshot(self):
+        snapshot = self._make_one(None, None, False, None, None, None)
+        self.assertIsNone(snapshot.get('one'))
+
     def test_to_dict(self):
         data = {
             'a': 10,
@@ -553,6 +564,11 @@ def test_to_dict(self):
         self.assertEqual(data, snapshot.to_dict())
         self.assertNotEqual(data, as_dict)
 
+    def test_non_existent(self):
+        snapshot = self._make_one(None, None, False, None, None, None)
+        as_dict = snapshot.to_dict()
+        self.assertIsNone(as_dict)
+
 
 class Test__get_document_path(unittest.TestCase):