Merge lp:~jameinel/u1db/put_and_update into lp:u1db

[John A Meinel]

This updates some internal code (_put_and_update_indexes) to use Document objects.

I think this fixes a bug Samuele observed with put after delete. I added a test for it, but I added the test after I had done the refactoring, so I'm not positive his original bug is fixed.

[lp:~jameinel/u1db/put_and_update]

132. By John A Meinel on 2011-11-29

Merge trunk and resolve a small conflict.

133. By John A Meinel on 2011-11-29

Remove the now-unnecessary pdb trace call.

[Samuele Pedroni (pedronis)]

+1

review: Approve

=== modified file 'u1db/backends/__init__.py'
--- u1db/backends/__init__.py	2011-11-23 12:17:00 +0000
+++ u1db/backends/__init__.py	2011-11-29 15:19:27 +0000
@@ -77,13 +77,10 @@
         doc_vcr = VectorClockRev(doc.rev)
         if cur_doc is None:
             cur_vcr = VectorClockRev(None)
-            cur_content = None
         else:
             cur_vcr = VectorClockRev(cur_doc.rev)
-            cur_content = cur_doc.content
         if doc_vcr.is_newer(cur_vcr):
-            self._put_and_update_indexes(doc.doc_id, cur_content, doc.rev,
-                                         doc.content)
+            self._put_and_update_indexes(cur_doc, doc)
             return 'inserted'
         elif doc.rev == cur_doc.rev:
             # magical convergence

=== modified file 'u1db/backends/inmemory.py'
--- u1db/backends/inmemory.py	2011-11-28 17:51:34 +0000
+++ u1db/backends/inmemory.py	2011-11-29 15:19:27 +0000
@@ -61,30 +61,28 @@
     def put_doc(self, doc):
         if doc.doc_id is None:
             raise errors.InvalidDocId()
-        old_content = None
-        if doc.doc_id in self._docs:
-            if doc.doc_id in self._conflicts:
-                raise errors.ConflictedDoc()
-            old_rev, old_content = self._docs[doc.doc_id]
-            if old_rev != doc.rev:
+        if self._has_conflicts(doc.doc_id):
+            raise errors.ConflictedDoc()
+        old_doc = self._get_doc(doc.doc_id)
+        if old_doc is not None:
+            if old_doc.rev != doc.rev:
                 raise errors.RevisionConflict()
         else:
             if doc.rev is not None:
                 raise errors.RevisionConflict()
         new_rev = self._allocate_doc_rev(doc.rev)
-        self._put_and_update_indexes(doc.doc_id, old_content, new_rev,
-                                     doc.content)
         doc.rev = new_rev
+        self._put_and_update_indexes(old_doc, doc)
         return new_rev
 
-    def _put_and_update_indexes(self, doc_id, old_content, new_rev, content):
+    def _put_and_update_indexes(self, old_doc, doc):
         for index in self._indexes.itervalues():
-            if old_content is not None:
-                index.remove_json(doc_id, old_content)
-            if content not in (None, 'null'):
-                index.add_json(doc_id, content)
-        self._docs[doc_id] = (new_rev, content)
-        self._transaction_log.append(doc_id)
+            if old_doc is not None and old_doc.content is not None:
+                index.remove_json(old_doc.doc_id, old_doc.content)
+            if doc.content is not None:
+                index.add_json(doc.doc_id, doc.content)
+        self._docs[doc.doc_id] = (doc.rev, doc.content)
+        self._transaction_log.append(doc.doc_id)
 
     def _get_doc(self, doc_id):
         try:
@@ -111,7 +109,11 @@
         return result
 
     def resolve_doc(self, doc, conflicted_doc_revs):
-        cur_rev, cur_content = self._docs[doc.doc_id]
+        cur_doc = self._get_doc(doc.doc_id)
+        if cur_doc is None:
+            cur_rev = None
+        else:
+            cur_rev = cur_doc.rev
         new_rev = self._ensure_maximal_rev(cur_rev, conflicted_doc_revs)
         superseded_revs = set(conflicted_doc_revs)
         remaining_conflicts = []
@@ -120,16 +122,15 @@
             if c_rev in superseded_revs:
                 continue
             remaining_conflicts.append((c_rev, c_doc))
+        doc.rev = new_rev
         if cur_rev in superseded_revs:
-            self._put_and_update_indexes(doc.doc_id, cur_content, new_rev,
-                                         doc.content)
+            self._put_and_update_indexes(cur_doc, doc)
         else:
             remaining_conflicts.append((new_rev, doc.content))
         if not remaining_conflicts:
             del self._conflicts[doc.doc_id]
         else:
             self._conflicts[doc.doc_id] = remaining_conflicts
-        doc.rev = new_rev
         doc.has_conflicts = bool(remaining_conflicts)
 
     def delete_doc(self, doc):
@@ -169,12 +170,11 @@
                 set(self._transaction_log[old_generation:]))
 
     def force_doc_sync_conflict(self, doc):
-        my_doc_rev, my_content = self._docs[doc.doc_id]
+        my_doc = self._get_doc(doc.doc_id)
         self._conflicts.setdefault(doc.doc_id, []).append(
-            (my_doc_rev, my_content))
+            (my_doc.rev, my_doc.content))
         doc.has_conflicts = True
-        self._put_and_update_indexes(doc.doc_id, my_content, doc.rev,
-                                     doc.content)
+        self._put_and_update_indexes(my_doc, doc)
 
 
 class InMemoryIndex(object):

=== modified file 'u1db/backends/sqlite_backend.py'
--- u1db/backends/sqlite_backend.py	2011-11-28 17:51:34 +0000
+++ u1db/backends/sqlite_backend.py	2011-11-29 15:19:27 +0000
@@ -253,22 +253,19 @@
     def put_doc(self, doc):
         if doc.doc_id is None:
             raise errors.InvalidDocId()
-        old_content = None
         with self._db_handle:
             if self._has_conflicts(doc.doc_id):
                 raise errors.ConflictedDoc()
             old_doc = self._get_doc(doc.doc_id)
             if old_doc is not None:
-                old_content = old_doc.content
                 if old_doc.rev != doc.rev:
                     raise errors.RevisionConflict()
             else:
                 if doc.rev is not None:
                     raise errors.RevisionConflict()
             new_rev = self._allocate_doc_rev(doc.rev)
-            self._put_and_update_indexes(doc.doc_id, old_content, new_rev,
-                                         doc.content)
             doc.rev = new_rev
+            self._put_and_update_indexes(old_doc, doc)
         return new_rev
 
     def _expand_to_fields(self, doc_id, base_field, raw_doc, save_none):
@@ -300,7 +297,7 @@
                     values.append((doc_id, subfield_name, val, len(values)))
         return values
 
-    def _put_and_update_indexes(self, doc_id, old_doc, new_rev, doc):
+    def _put_and_update_indexes(self, old_doc, doc):
         """Actually insert a document into the database.
 
         This both updates the existing documents content, and any indexes that
@@ -333,10 +330,9 @@
             if self._has_conflicts(doc.doc_id):
                 raise errors.ConflictedDoc()
             new_rev = self._allocate_doc_rev(doc.rev)
-            self._put_and_update_indexes(doc.doc_id,
-                old_doc.content, new_rev, None)
             doc.rev = new_rev
             doc.content = None
+            self._put_and_update_indexes(old_doc, doc)
         return new_rev
 
     def _get_conflicts(self, doc_id):
@@ -385,8 +381,7 @@
             c = self._db_handle.cursor()
             self._add_conflict(c, doc.doc_id, my_doc.rev, my_doc.content)
             doc.has_conflicts = True
-            self._put_and_update_indexes(doc.doc_id, my_doc.content, doc.rev,
-                                         doc.content)
+            self._put_and_update_indexes(my_doc, doc)
 
     def resolve_doc(self, doc, conflicted_doc_revs):
         with self._db_handle:
@@ -395,15 +390,14 @@
             superseded_revs = set(conflicted_doc_revs)
             cur_conflicts = self._get_conflicts(doc.doc_id)
             c = self._db_handle.cursor()
+            doc.rev = new_rev
             if cur_doc.rev in superseded_revs:
-                self._put_and_update_indexes(doc.doc_id, cur_doc.content,
-                                             new_rev, doc.content)
+                self._put_and_update_indexes(cur_doc, doc)
             else:
                 self._add_conflict(c, doc.doc_id, new_rev, doc.content)
             deleting = [(doc.doc_id, c_rev) for c_rev in superseded_revs]
             c.executemany("DELETE FROM conflicts"
                           " WHERE doc_id=? AND doc_rev=?", deleting)
-            doc.rev = new_rev
             doc.has_conflicts = self._has_conflicts(doc.doc_id)
 
     def create_index(self, index_name, index_expression):
@@ -559,28 +553,28 @@
             db_cursor.executemany(
                 "INSERT INTO document_fields VALUES (?, ?, ?)", values)
 
-    def _put_and_update_indexes(self, doc_id, old_doc, new_rev, doc):
+    def _put_and_update_indexes(self, old_doc, doc):
         c = self._db_handle.cursor()
-        if doc:
-            raw_doc = simplejson.loads(doc)
+        if doc and doc.content:
+            raw_doc = simplejson.loads(doc.content)
         else:
             raw_doc = {}
-        if old_doc:
+        if old_doc is not None:
             c.execute("UPDATE document SET doc_rev=?, doc=? WHERE doc_id = ?",
-                      (new_rev, doc, doc_id))
+                      (doc.rev, doc.content, doc.doc_id))
             c.execute("DELETE FROM document_fields WHERE doc_id = ?",
-                      (doc_id,))
+                      (doc.doc_id,))
         else:
             c.execute("INSERT INTO document VALUES (?, ?, ?)",
-                      (doc_id, new_rev, doc))
+                      (doc.doc_id, doc.rev, doc.content))
         indexed_fields = self._get_indexed_fields()
         if indexed_fields:
             # It is expected that len(indexed_fields) is shorter than
             # len(raw_doc)
             # TODO: Handle nested indexed fields.
-            self._update_indexes(doc_id, raw_doc, indexed_fields, c)
+            self._update_indexes(doc.doc_id, raw_doc, indexed_fields, c)
         c.execute("INSERT INTO transaction_log(doc_id) VALUES (?)",
-                  (doc_id,))
+                  (doc.doc_id,))
 
     def create_index(self, index_name, index_expression):
         with self._db_handle:

=== modified file 'u1db/tests/test_backends.py'
--- u1db/tests/test_backends.py	2011-11-28 17:51:34 +0000
+++ u1db/tests/test_backends.py	2011-11-29 15:19:27 +0000
@@ -241,6 +241,30 @@
                                    [(doc1.rev, simple_doc),
                                     (doc2.rev, nested_doc)])
 
+    def test_resolve_doc_with_delete_conflict(self):
+        doc1 = self.db.create_doc(simple_doc)
+        self.db.delete_doc(doc1)
+        doc2 = Document(doc1.doc_id, 'alternate:1', nested_doc)
+        self.db.force_doc_sync_conflict(doc2)
+        self.assertGetDocConflicts(self.db, doc1.doc_id,
+                                   [(doc2.rev, nested_doc),
+                                    (doc1.rev, None)])
+        self.db.resolve_doc(doc2, [doc1.rev, doc2.rev])
+        self.assertGetDocConflicts(self.db, doc1.doc_id, [])
+        self.assertGetDoc(self.db, doc2.doc_id, doc2.rev, nested_doc, False)
+
+    def test_resolve_doc_with_delete_to_delete(self):
+        doc1 = self.db.create_doc(simple_doc)
+        self.db.delete_doc(doc1)
+        doc2 = Document(doc1.doc_id, 'alternate:1', nested_doc)
+        self.db.force_doc_sync_conflict(doc2)
+        self.assertGetDocConflicts(self.db, doc1.doc_id,
+                                   [(doc2.rev, nested_doc),
+                                    (doc1.rev, None)])
+        self.db.resolve_doc(doc1, [doc1.rev, doc2.rev])
+        self.assertGetDocConflicts(self.db, doc1.doc_id, [])
+        self.assertGetDoc(self.db, doc1.doc_id, doc1.rev, None, False)
+
     def test_get_docs_empty_list(self):
         self.assertEqual([], self.db.get_docs([]))
 
@@ -255,7 +279,8 @@
         doc1 = self.db.create_doc(orig_doc)
         doc1_rev1 = doc1.rev
         doc1.content = simple_doc
-        doc1_rev2 = self.db.put_doc(doc1)
+        self.db.put_doc(doc1)
+        doc1_rev2 = doc1.rev
         # Nothing is inserted, because the document is already superseded
         doc = Document(doc1.doc_id, doc1_rev1, orig_doc)
         state = self.db.put_doc_if_newer(doc)
@@ -287,6 +312,17 @@
                           (doc1.rev, simple_doc)],
                          self.db.get_doc_conflicts(doc1.doc_id))
 
+    def test_force_doc_sync_conflict_was_deleted(self):
+        doc1 = self.db.create_doc(simple_doc)
+        self.db.delete_doc(doc1)
+        doc2 = Document(doc1.doc_id, 'alternate:1', nested_doc)
+        self.db.force_doc_sync_conflict(doc2)
+        self.assertTrue(doc2.has_conflicts)
+        self.assertGetDoc(self.db, doc1.doc_id, 'alternate:1', nested_doc, True)
+        self.assertEqual([('alternate:1', nested_doc),
+                          (doc1.rev, None)],
+                         self.db.get_doc_conflicts(doc1.doc_id))
+
     def test_get_sync_generation(self):
         self.assertEqual(0, self.db.get_sync_generation('other-db'))
         self.db.set_sync_generation('other-db', 2)
@@ -307,6 +343,14 @@
         self.db.delete_doc(doc)
         self.assertEqual((2, set([doc.doc_id])), self.db.whats_changed(db_gen))
 
+    def test_delete_then_put(self):
+        doc = self.db.create_doc(simple_doc)
+        self.db.delete_doc(doc)
+        self.assertGetDoc(self.db, doc.doc_id, doc.rev, None, False)
+        doc.content = nested_doc
+        self.db.put_doc(doc)
+        self.assertGetDoc(self.db, doc.doc_id, doc.rev, nested_doc, False)
+
     def test_whats_changed_initial_database(self):
         self.assertEqual((0, set()), self.db.whats_changed())
 

=== modified file 'u1db/tests/test_sync.py'
--- u1db/tests/test_sync.py	2011-11-28 14:58:52 +0000
+++ u1db/tests/test_sync.py	2011-11-29 15:19:27 +0000
@@ -338,7 +338,8 @@
         content1 = '{"key": "localval"}'
         content2 = '{"key": "altval"}'
         doc.content = content2
-        doc2_rev2 = self.db2.put_doc(doc)
+        self.db2.put_doc(doc)
+        doc2_rev2 = doc.rev
         # Monkey patch so that after the local client has determined recent
         # changes, we get another one, before sync finishes.
         orig_wc = self.db1.whats_changed