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

[John A Meinel]

This changes SQLiteDatabase so that it defaults to creating a unique replica_uid when it initializes the database.

Some comments:

1) We never set a uniqueness constraint on the u1db_config table, so we could call _set_replica_uid as much as we wanted, but it wouldn't actually change it, since it just added more data to the db.

2) I went with: uuid.uuid4().hex. Which is 32 bytes long (vs the default 36 bytes with the extra '-' chars).
This should make it reasonably short, since it will end up in all the doc revs, etc.
We've talked about including something like hostname, to help in debugging issues. For now, this was simple and works, we can extend it later, I think.

3) We use 'index_storage' as a flag to indicate the db has been set up, move it to after _extra_schema_init and _set_replica_uid.

[Samuele Pedroni (pedronis)]

from my understarding while with sqlite3

with conn:
   A
   with conn:
     B
   C

is permitted, it doesn't do what one would expect, it will result in a commit after B and one after C (or more depending on the isolation level)

this means _initialize shouldn't call a helper with a with conn: of its own (_set_replica_uid), so things need to be factored a bit differently

review: Needs Fixing

[Samuele Pedroni (pedronis)]

with conn just emits a commit or rollback on __exit__. BEGIN is emitted by sqlite3 in a lazy, opportunistic way.

[Samuele Pedroni (pedronis)]

this kind of test shows the problem, a similar test but hooking on _extra_schema_init passes on trunk:

=== modified file 'u1db/tests/test_sqlite_backend.py'
--- u1db/tests/test_sqlite_backend.py 2011-11-30 14:05:19 +0000
+++ u1db/tests/test_sqlite_backend.py 2011-11-30 17:09:40 +0000
@@ -214,6 +214,25 @@
                           (doc1.doc_id, "sub.doc", "underneath"),
                          ], c.fetchall())

+ def test__ensure_schema_rollback(self):
+ temp_dir = self.createTempDir(prefix='u1db-test-')
+ path = temp_dir + '/rollback.db'
+ class SQLitePartialExpandDbTesting(
+ sqlite_backend.SQLitePartialExpandDatabase):
+ def _set_replica_uid(self, uid):
+ super(SQLitePartialExpandDbTesting, self)._set_replica_uid(uid)
+ if fail:
+ raise Exception()
+ db = SQLitePartialExpandDbTesting.__new__(SQLitePartialExpandDbTesting)
+ db._db_handle = dbapi2.connect(path) # db is there but not yet init-ed
+ fail = True
+ self.assertRaises(Exception, db._ensure_schema)
+ fail = False
+ db._ensure_schema()
+ x, err = db._which_index_storage(db._db_handle.cursor())
+ self.assertIsNotNone(x)
+ self.assertIsNone(err)
+
     def test__open_database(self):
         temp_dir = self.createTempDir(prefix='u1db-test-')
         path = temp_dir + '/test.sqlite'

[Samuele Pedroni (pedronis)]

this variant is simpler, has a clearer failure mode:

+ def test__ensure_schema_rollback(self):
+ temp_dir = self.createTempDir(prefix='u1db-test-')
+ path = temp_dir + '/rollback.db'
+ class SQLitePartialExpandDbTesting(
+ sqlite_backend.SQLitePartialExpandDatabase):
+ def _set_replica_uid(self, uid):
+ super(SQLitePartialExpandDbTesting, self)._set_replica_uid(uid)
+ if fail:
+ raise Exception()
+ db = SQLitePartialExpandDbTesting.__new__(SQLitePartialExpandDbTesting)
+ db._db_handle = dbapi2.connect(path) # db is there but not yet init-ed
+ fail = True
+ self.assertRaises(Exception, db._ensure_schema)
+ fail = False
+ db._initialize(db._db_handle.cursor())
+

[lp:~jameinel/u1db/uuid_databases]

133. By John A Meinel on 2011-12-01

Bring in Samuele's test case and a fix for the transaction isolation logic.

[John A Meinel (jameinel)]

Updated to use a helper function, and now includes your test case (modified to override the new helper function that is being called.)

[Samuele Pedroni (pedronis)]

+1, looks good, notice that it seems there will be a conflict when merging with trunk

review: Approve

=== modified file 'u1db/backends/sqlite_backend.py'
--- u1db/backends/sqlite_backend.py	2011-11-30 13:09:21 +0000
+++ u1db/backends/sqlite_backend.py	2011-12-01 14:02:25 +0000
@@ -18,6 +18,7 @@
 import simplejson
 from sqlite3 import dbapi2
 import time
+import uuid
 
 from u1db.backends import CommonBackend, CommonSyncTarget
 from u1db import Document, errors
@@ -158,11 +159,19 @@
                   " field TEXT,"
                   " CONSTRAINT index_definitions_pkey"
                   " PRIMARY KEY (name, offset))")
-        c.execute("CREATE TABLE u1db_config (name TEXT, value TEXT)")
+        c.execute("CREATE TABLE u1db_config ("
+                  " name TEXT PRIMARY KEY,"
+                  " value TEXT)")
         c.execute("INSERT INTO u1db_config VALUES ('sql_schema', '0')")
+        self._extra_schema_init(c)
+        # A unique identifier should be set for this replica. Implementations
+        # don't have to strictly use uuid here, but we do want the uid to be
+        # unique amongst all databases that will sync with each other.
+        # We might extend this to using something with hostname for easier
+        # debugging.
+        self._set_replica_uid_in_transaction(uuid.uuid4().hex)
         c.execute("INSERT INTO u1db_config VALUES" " ('index_storage', ?)",
                   (self._index_storage_value,))
-        self._extra_schema_init(c)
 
     def _ensure_schema(self):
         """Ensure that the database schema has been created."""
@@ -188,9 +197,14 @@
     def _set_replica_uid(self, replica_uid):
         """Force the replica_uid to be set."""
         with self._db_handle:
-            c = self._db_handle.cursor()
-            c.execute("INSERT INTO u1db_config VALUES ('replica_uid', ?)",
-                      (replica_uid,))
+            self._set_replica_uid_in_transaction(replica_uid)
+
+    def _set_replica_uid_in_transaction(self, replica_uid):
+        """Set the replica_uid. A transaction should already be held."""
+        c = self._db_handle.cursor()
+        c.execute("INSERT OR REPLACE INTO u1db_config"
+                  " VALUES ('replica_uid', ?)",
+                  (replica_uid,))
         self._real_replica_uid = replica_uid
 
     def _get_replica_uid(self):

=== modified file 'u1db/commandline/client.py'
--- u1db/commandline/client.py	2011-11-30 13:09:38 +0000
+++ u1db/commandline/client.py	2011-12-01 14:02:25 +0000
@@ -92,12 +92,18 @@
     @classmethod
     def _populate_subparser(cls, parser):
         parser.add_argument('database', help='The database to create')
-        parser.add_argument('replica_uid',
+        parser.add_argument('--replica-uid', default=None,
             help='The unique identifier for this database')
 
     def run(self, database, replica_uid):
+<<<<<<< TREE
         db = u1db_open(database, create=True)
         db._set_replica_uid(replica_uid)
+=======
+        db = sqlite_backend.SQLiteDatabase.open_database(database)
+        if replica_uid is not None:
+            db._set_replica_uid(replica_uid)
+>>>>>>> MERGE-SOURCE
 
 client_commands.register(CmdInitDB)
 

=== modified file 'u1db/tests/commandline/test_client.py'
--- u1db/tests/commandline/test_client.py	2011-11-23 12:39:27 +0000
+++ u1db/tests/commandline/test_client.py	2011-12-01 14:02:25 +0000
@@ -73,11 +73,18 @@
         self.assertEqual(sys.stdout, args.outfile)
 
     def test_init_db(self):
-        args = self.parse_args(['init-db', 'test.db', 'replica-uid'])
+        args = self.parse_args(
+            ['init-db', 'test.db', '--replica-uid=replica-uid'])
         self.assertEqual(client.CmdInitDB, args.subcommand)
         self.assertEqual('test.db', args.database)
         self.assertEqual('replica-uid', args.replica_uid)
 
+    def test_init_db_no_replica(self):
+        args = self.parse_args(['init-db', 'test.db'])
+        self.assertEqual(client.CmdInitDB, args.subcommand)
+        self.assertEqual('test.db', args.database)
+        self.assertIs(None, args.replica_uid)
+
     def test_put(self):
         args = self.parse_args(['put', 'test.db', 'doc-id', 'old-doc-rev'])
         self.assertEqual(client.CmdPut, args.subcommand)
@@ -149,6 +156,14 @@
         db = sqlite_backend.SQLiteDatabase.open_database(path, create=False)
         self.assertEqual('test-uid', db._replica_uid)
 
+    def test_init_no_uid(self):
+        path = self.working_dir + '/test2.db'
+        cmd = self.make_command(client.CmdInitDB)
+        cmd.run(path, None)
+        self.assertTrue(os.path.exists(path))
+        db = sqlite_backend.SQLiteDatabase.open_database(path, create=False)
+        self.assertIsNot(None, db._replica_uid)
+
 
 class TestCmdPut(TestCaseWithDB):
 
@@ -261,7 +276,7 @@
 
     def test_init_db(self):
         path = self.working_dir + '/test2.db'
-        ret, stdout, stderr = self.run_main(['init-db', path, 'uid'])
+        ret, stdout, stderr = self.run_main(['init-db', path])
         db2 = sqlite_backend.SQLiteDatabase.open_database(path, create=False)
 
     def test_put(self):

=== modified file 'u1db/tests/test_sqlite_backend.py'
--- u1db/tests/test_sqlite_backend.py	2011-11-30 14:19:48 +0000
+++ u1db/tests/test_sqlite_backend.py	2011-12-01 14:02:25 +0000
@@ -87,6 +87,12 @@
         raw_db = self.db._get_sqlite_handle()
         self.assertNotEqual(None, raw_db)
 
+    def test_default_replica_uid(self):
+        self.db = sqlite_backend.SQLitePartialExpandDatabase(':memory:')
+        self.assertIsNot(None, self.db._replica_uid)
+        self.assertEqual(32, len(self.db._replica_uid))
+        val = int(self.db._replica_uid, 16)
+
     def test__close_sqlite_handle(self):
         raw_db = self.db._get_sqlite_handle()
         self.db._close_sqlite_handle()
@@ -112,8 +118,8 @@
     def test__set_replica_uid(self):
         # Start from scratch, so that replica_uid isn't set.
         self.db = sqlite_backend.SQLitePartialExpandDatabase(':memory:')
-        self.assertEqual(None, self.db._real_replica_uid)
-        self.assertEqual(None, self.db._replica_uid)
+        self.assertIsNot(None, self.db._real_replica_uid)
+        self.assertIsNot(None, self.db._replica_uid)
         self.db._set_replica_uid('foo')
         c = self.db._get_sqlite_handle().cursor()
         c.execute("SELECT value FROM u1db_config WHERE name='replica_uid'")
@@ -124,7 +130,6 @@
         self.assertEqual('foo', self.db._replica_uid)
 
     def test__get_generation(self):
-        self.db._set_replica_uid('foo')
         self.assertEqual(0, self.db._get_generation())
 
     def test__allocate_doc_id(self):
@@ -209,6 +214,23 @@
                           (doc1.doc_id, "sub.doc", "underneath"),
                          ], c.fetchall())
 
+    def test__ensure_schema_rollback(self):
+        temp_dir = self.createTempDir(prefix='u1db-test-')
+        path = temp_dir + '/rollback.db'
+        class SQLitePartialExpandDbTesting(
+            sqlite_backend.SQLitePartialExpandDatabase):
+            def _set_replica_uid_in_transaction(self, uid):
+                super(SQLitePartialExpandDbTesting,
+                    self)._set_replica_uid_in_transaction(uid)
+                if fail:
+                    raise Exception()
+        db = SQLitePartialExpandDbTesting.__new__(SQLitePartialExpandDbTesting)
+        db._db_handle = dbapi2.connect(path) # db is there but not yet init-ed
+        fail = True
+        self.assertRaises(Exception, db._ensure_schema)
+        fail = False
+        db._initialize(db._db_handle.cursor())
+
     def test__open_database(self):
         temp_dir = self.createTempDir(prefix='u1db-test-')
         path = temp_dir + '/test.sqlite'