Merge lp:~thisfred/u1db/indexing-deleted-docs into lp:u1db

[Eric Casteleijn]

Adding a new index to a database that contains deleted documents no longer breaks.

Adding a new index to a database that contains deleted documents no longer breaks.

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 4/23/2012 10:09 PM, Eric Casteleijn wrote:
> Eric Casteleijn has proposed merging
> lp:~thisfred/u1db/indexing-deleted-docs into lp:u1db.
>
> Requested reviews: Ubuntu One hackers (ubuntuone-hackers)
>
> For more details, see:
> https://code.launchpad.net/~thisfred/u1db/indexing-deleted-docs/+merge/103165
>
> Adding a new index to a database that contains deleted documents
> no longer breaks.

Nice catch on this. Though I'm not sure about your solution.

+ if (context.content == NULL)
+ {
+ // Invalid JSON in the database, for now we just continue?
+ status = sqlite3_step(statement);
+ continue;
+ }

I think this should instead be:

if (context.content == NULL) {
  // Deleted document, just skip it and onto the next
  continue;
}

And this one:

         if (context.obj == NULL
                 || !json_object_is_type(context.obj, json_type_object))
         {
             // Invalid JSON in the database, for now we just continue?
+ status = sqlite3_step(statement);
             continue;
         }

Should probably actually be an error, rather than just continuing. I
think I was just cutting corners because we didn't need it for
compatibility with the test suite.

So I think your change is ok here, but we need a test for this case. I
suppose it is arguable. Should we skip (because creating an index
isn't a great time to just fail because you have bad data), or should
we fail (so that you know early your content is incorrect.)

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk+WlX0ACgkQJdeBCYSNAAMgfACfbWefqMJBiDvuvAKjUmkdRYM1
/YYAn232RCIBxvGLpDh2o0BUVpH9bkdu
=WeJf
-----END PGP SIGNATURE-----

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 4/23/2012 10:09 PM, Eric Casteleijn wrote:
> Eric Casteleijn has proposed merging
> lp:~thisfred/u1db/indexing-deleted-docs into lp:u1db.
>
> Requested reviews: Ubuntu One hackers (ubuntuone-hackers)
>
> For more details, see:
> https://code.launchpad.net/~thisfred/u1db/indexing-deleted-docs/+merge/103165
>
> Adding a new index to a database that contains deleted documents
> no longer breaks.

 review: approve

With the small tweak of fixing the comment on why we are continuing.

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk+WlZYACgkQJdeBCYSNAAMsewCgydv24JAuttJVcURcpWzG6k5Z
FB0An3dd/nPYp+VlxhdP4jQVnbLHksKo
=X70+
-----END PGP SIGNATURE-----

review: Approve

[Eric Casteleijn (thisfred)]

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 4/23/2012 10:09 PM, Eric Casteleijn wrote:
> > Eric Casteleijn has proposed merging
> > lp:~thisfred/u1db/indexing-deleted-docs into lp:u1db.
> >
> > Requested reviews: Ubuntu One hackers (ubuntuone-hackers)
> >
> > For more details, see:
> > https://code.launchpad.net/~thisfred/u1db/indexing-deleted-
> docs/+merge/103165
> >
> > Adding a new index to a database that contains deleted documents
> > no longer breaks.
>
> Nice catch on this. Though I'm not sure about your solution.
>
> + if (context.content == NULL)
> + {
> + // Invalid JSON in the database, for now we just continue?
> + status = sqlite3_step(statement);
> + continue;
> + }
>
> I think this should instead be:
>
> if (context.content == NULL) {
> // Deleted document, just skip it and onto the next
> continue;
> }

I'm not sure I understand:

The while loop will never advance to the next table row if we just do continue without the

status = sqlite3_step(statement);

unless we advance the row in the while statement itself, this would result in an infinite loop? Maybe I'm reading the code wrong.

> And this one:
>
> if (context.obj == NULL
> || !json_object_is_type(context.obj, json_type_object))
> {
> // Invalid JSON in the database, for now we just continue?
> + status = sqlite3_step(statement);
> continue;
> }
>
> Should probably actually be an error, rather than just continuing. I
> think I was just cutting corners because we didn't need it for
> compatibility with the test suite.

Yes, now that we catch the case of the deleted document, this should probably be an error.

> So I think your change is ok here, but we need a test for this case. I
> suppose it is arguable. Should we skip (because creating an index
> isn't a great time to just fail because you have bad data), or should
> we fail (so that you know early your content is incorrect.)

I'll file a bug and think about a good test case, and how to handle the case of corrupted content in general. Since it's not easy (hopefully) to get invalid JSON into the db using our API, I think we should probably make it break as early and explicitly as possible.

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

...

>> I think this should instead be:
>>
>> if (context.content == NULL) { // Deleted document, just skip it
>> and onto the next continue; }
>
>
> I'm not sure I understand:
>
> The while loop will never advance to the next table row if we just
> do continue without the
>
> status = sqlite3_step(statement);
>
> unless we advance the row in the while statement itself, this would
> result in an infinite loop? Maybe I'm reading the code wrong.
>

Sorry, I'm not saying you shouldn't step, just fix the comment.

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk+WpBQACgkQJdeBCYSNAAOm1wCgiXxGVWtwWGLwwMjP1Aty/ncL
KncAn1HZEXxVqXK8w7I33EuExyouvJeN
=fb/Y
-----END PGP SIGNATURE-----

[lp:~thisfred/u1db/indexing-deleted-docs]

254. By Eric Casteleijn on 2012-04-24

comments improved.

=== modified file 'src/u1db_query.c'
--- src/u1db_query.c	2012-04-17 14:26:03 +0000
+++ src/u1db_query.c	2012-04-24 13:05:24 +0000
@@ -796,15 +796,23 @@
     while (status == SQLITE_ROW) {
         context.doc_id = (const char*)sqlite3_column_text(statement, 0);
         context.content = (const char*)sqlite3_column_text(statement, 1);
+        if (context.content == NULL)
+        {
+            // This document is deleted so does not need to be indexed.
+            status = sqlite3_step(statement);
+            continue;
+        }
         context.obj = json_tokener_parse(context.content);
         if (context.obj == NULL
                 || !json_object_is_type(context.obj, json_type_object))
         {
             // Invalid JSON in the database, for now we just continue?
+            // TODO: Raise an error here.
+            status = sqlite3_step(statement);
             continue;
         }
         for (i = 0; i < n_expressions; ++i) {
-            status = evaluate_index_and_insert_into_db(&context, 
+            status = evaluate_index_and_insert_into_db(&context,
                     expressions[i]);
             if (status != U1DB_OK)
                 goto finish;

=== modified file 'u1db/backends/inmemory.py'
--- u1db/backends/inmemory.py	2012-02-01 13:36:52 +0000
+++ u1db/backends/inmemory.py	2012-04-24 13:05:24 +0000
@@ -168,7 +168,8 @@
     def create_index(self, index_name, index_expression):
         index = InMemoryIndex(index_name, index_expression)
         for doc_id, (doc_rev, doc) in self._docs.iteritems():
-            index.add_json(doc_id, doc)
+            if doc is not None:
+                index.add_json(doc_id, doc)
         self._indexes[index_name] = index
 
     def delete_index(self, index_name):

=== modified file 'u1db/backends/sqlite_backend.py'
--- u1db/backends/sqlite_backend.py	2012-02-23 09:46:58 +0000
+++ u1db/backends/sqlite_backend.py	2012-04-24 13:05:24 +0000
@@ -669,6 +669,8 @@
                    for field in new_fields]
         c = self._db_handle.cursor()
         for doc_id, doc in self._iter_all_docs():
+            if doc is None:
+                continue
             raw_doc = simplejson.loads(doc)
             self._update_indexes(doc_id, raw_doc, getters, c)
 

=== modified file 'u1db/tests/test_backends.py'
--- u1db/tests/test_backends.py	2012-04-16 16:15:56 +0000
+++ u1db/tests/test_backends.py	2012-04-24 13:05:24 +0000
@@ -42,7 +42,7 @@
 
 def http_create_database(test, replica_uid, path='test'):
     test.startServer()
-    db = test.request_state._create_database(replica_uid)
+    test.request_state._create_database(replica_uid)
     return http_database.HTTPDatabase(test.getURL(path))
 
 
@@ -593,6 +593,14 @@
         self.assertEqual([doc],
                          self.db.get_from_index('test-idx', [('value',)]))
 
+    def test_create_index_after_deleting_document(self):
+        doc = self.db.create_doc(simple_doc)
+        doc2 = self.db.create_doc(simple_doc)
+        self.db.delete_doc(doc2)
+        self.db.create_index('test-idx', ['key'])
+        self.assertEqual([doc],
+                         self.db.get_from_index('test-idx', [('value',)]))
+
     def test_delete_index(self):
         self.db.create_index('test-idx', ['key'])
         self.assertEqual([('test-idx', ['key'])], self.db.list_indexes())
@@ -606,9 +614,10 @@
             self.db.get_from_index('test-idx', [('value',)]))
 
     def test_get_from_index_unmatched(self):
-        doc = self.db.create_doc(simple_doc)
+        self.db.create_doc(simple_doc)
         self.db.create_index('test-idx', ['key'])
-        self.assertEqual([], self.db.get_from_index('test-idx', [('novalue',)]))
+        self.assertEqual(
+            [], self.db.get_from_index('test-idx', [('novalue',)]))
 
     def test_create_index_multiple_exact_matches(self):
         doc = self.db.create_doc(simple_doc)