Merge lp:~jplacerda/zeitgeist/660307 into lp:zeitgeist/0.1

[J.P. Lacerda]

[lp:~jplacerda/zeitgeist/660307]

1763. By J.P. Lacerda on 2011-05-17

remove unecessary code block and add tests

[Siegfried Gevatter (rainct)]

First of all, good work and thank you for working on this! :)

Some comments though:

1)
> for i in xrange(old_version, new_version):
> _set_schema_version(cursor, schema_name, i+1)
> _do_schema_backup()

The _do_schema_backup() call should be before the loop, otherwise the first upgrade isn't protected at all (also, in case of going through several version upgrades, you're needlesly backing up the database several times, the backup scripts don't create any new non-recoverable data, so just having the initial backup is enough).

2)

> # FIXME: This can return True, we just need a decent testing
> # framework first
> return True, cursor

As I said in bug #784011, this isn't true, since upgrade scripts rely on the code being run to do stuff such as updating the event view, creating new indices, etc. I also don't think we ought to change this, since copying the event view, indices, etc. into all upgrade scripts is a pain.

3)

Maybe I'm missing something but it looks like this will go into an endless loop if upgrading the schema fails (for some reason other than Zeitgeist being killed).

review: Needs Fixing

[lp:~jplacerda/zeitgeist/660307]

1764. By J.P. Lacerda on 2011-05-17

optimize backup policy

1765. By J.P. Lacerda on 2011-05-17

reflect backup policy changes on schema upgrade and tests

=== modified file '_zeitgeist/engine/__init__.py'
--- _zeitgeist/engine/__init__.py	2011-02-09 15:07:17 +0000
+++ _zeitgeist/engine/__init__.py	2011-05-17 18:40:17 +0000
@@ -48,6 +48,8 @@
 		BaseDirectory.save_data_path("zeitgeist"))
 	DATABASE_FILE = os.environ.get("ZEITGEIST_DATABASE_PATH",
 		os.path.join(DATA_PATH, "activity.sqlite"))
+	DATABASE_FILE_BACKUP = os.environ.get("ZEITGEIST_DATABASE_PATH",
+		os.path.join(DATA_PATH, "activity.sqlite.bck"))
 	DEFAULT_LOG_PATH = os.path.join(BaseDirectory.xdg_cache_home,
 		"zeitgeist", "daemon.log")
 	

=== modified file '_zeitgeist/engine/sql.py'
--- _zeitgeist/engine/sql.py	2011-04-27 18:25:02 +0000
+++ _zeitgeist/engine/sql.py	2011-05-17 18:40:17 +0000
@@ -6,6 +6,7 @@
 # Copyright © 2009 Mikkel Kamstrup Erlandsen <[e-mail]>
 # Copyright © 2009-2011 Markus Korn <[e-mail]>
 # Copyright © 2009 Seif Lotfy <[e-mail]>
+# Copyright © 2011 J.P. Lacerda <[e-mail]>
 # Copyright © 2011 Collabora Ltd.
 #             By Siegfried-Angel Gevatter Pujals <[e-mail]>
 #
@@ -26,6 +27,7 @@
 import logging
 import time
 import os
+import shutil
 
 from _zeitgeist.engine import constants
 
@@ -112,8 +114,10 @@
 	named '_zeitgeist.engine.upgrades.$schema_name_$(i)_$(i+1)' and executing 
 	the run(cursor) method of those modules until new_version is reached
 	"""
+	_do_schema_backup()
 	for i in xrange(old_version, new_version):
-		# Fire of the right upgrade module
+		_set_schema_version(cursor, schema_name, -1)
+		# Fire off the right upgrade module
 		log.info("Upgrading database '%s' from version %s to %s. This may take a while" %
 		         (schema_name, i, i+1))
 		upgrader_name = "%s_%s_%s" % (schema_name, i, i+1)
@@ -122,41 +126,65 @@
 		
 		# Update the schema version
 		_set_schema_version(cursor, schema_name, i+1)
+
 	log.info("Upgrade succesful")
 
 def _check_core_schema_upgrade (cursor):
 	"""Return True if the schema is good and no setup needs to be run"""
 	# See if we have the right schema version, and try an upgrade if needed
 	core_schema_version = _get_schema_version(cursor, constants.CORE_SCHEMA)
-	if core_schema_version is not None:
-		if core_schema_version >= constants.CORE_SCHEMA_VERSION:
-			return True
-		else:
-			try:
-				_do_schema_upgrade (cursor,
-				                    constants.CORE_SCHEMA,
-				                    core_schema_version,
-				                    constants.CORE_SCHEMA_VERSION)
-				# Don't return here. The upgrade process might depend on the
-				# tables, indexes, and views being set up (to avoid code dup)
-				log.info("Running post upgrade setup")
-				return False
-			except Exception, e:
-				log.exception("Failed to upgrade database '%s' from version %s to %s: %s" %
-				          (constants.CORE_SCHEMA, core_schema_version, constants.CORE_SCHEMA_VERSION, e))
-				raise SystemExit(27)
+	if core_schema_version >= constants.CORE_SCHEMA_VERSION:
+		return True, cursor
 	else:
-		return False
-
+		try:
+			if core_schema_version <= -1:
+				cursor.connection.commit()
+				cursor.connection.close()
+				_do_schema_restore()
+				cursor = _connect_to_db(constants.DATABASE_FILE)
+				core_schema_version = _get_schema_version(cursor, constants.CORE_SCHEMA)
+				log.exception("Database corrupted at upgrade -- upgrading from version %s" % core_schema_version)
+
+			_do_schema_upgrade (cursor,
+			                    constants.CORE_SCHEMA,
+			                    core_schema_version,
+			                    constants.CORE_SCHEMA_VERSION)
+
+			# Don't return here. The upgrade process might depend on the
+			# tables, indexes, and views being set up (to avoid code dup)
+			log.info("Running post upgrade setup")
+			return False, cursor
+		except sqlite3.OperationalError:
+			# Something went wrong while applying the upgrade -- this is
+			# probably due to a non existing table (this occurs when 
+			# applying core_3_4, for example). We just need to fall through
+			# the rest of create_db to fix this...
+			log.exception("Database corrupted -- proceeding")
+			return False, cursor
+		except Exception, e:
+			print e
+			log.exception("Failed to upgrade database '%s' from version %s to %s: %s" %
+			          (constants.CORE_SCHEMA, core_schema_version, constants.CORE_SCHEMA_VERSION, e))
+			raise SystemExit(27)
+
+def _do_schema_backup ():
+	shutil.copyfile(constants.DATABASE_FILE, constants.DATABASE_FILE_BACKUP)
+
+def _do_schema_restore ():
+	shutil.move(constants.DATABASE_FILE_BACKUP, constants.DATABASE_FILE)
+
+def _connect_to_db(file_path):
+	conn = sqlite3.connect(file_path)
+	conn.row_factory = sqlite3.Row
+	cursor = conn.cursor(UnicodeCursor)
+	return cursor
 
 def create_db(file_path):
 	"""Create the database and return a default cursor for it"""
 	start = time.time()
 	log.info("Using database: %s" % file_path)
 	new_database = not os.path.exists(file_path)
-	conn = sqlite3.connect(file_path)
-	conn.row_factory = sqlite3.Row
-	cursor = conn.cursor(UnicodeCursor)
+	cursor = _connect_to_db(file_path)
 
 	# Seif: as result of the optimization story (LP: #639737) we are setting
 	# journal_mode to WAL if possible, this change is irreversible but
@@ -179,8 +207,9 @@
 	cursor.execute("CREATE TEMP TABLE _fix_cache (table_name VARCHAR, id INTEGER)")
 	
 	# Always assume that temporary memory backed DBs have good schemas
-	if constants.DATABASE_FILE != ":memory:":
-		if not new_database and _check_core_schema_upgrade(cursor):
+	if constants.DATABASE_FILE != ":memory:" and not new_database:
+		do_upgrade, cursor = _check_core_schema_upgrade(cursor)
+		if do_upgrade:
 			_time = (time.time() - start)*1000
 			log.debug("Core schema is good. DB loaded in %sms" % _time)
 			return cursor

=== modified file 'test/sql-test.py'
--- test/sql-test.py	2011-04-27 21:53:04 +0000
+++ test/sql-test.py	2011-05-17 18:40:17 +0000
@@ -19,12 +19,14 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 #
 
-import sys, os
+import sys, os, shutil
 sys.path.insert(0, os.path.join(os.path.dirname(__file__), ".."))
 
 import unittest
 WhereClause = None
 
+from _zeitgeist.engine import constants, sql
+
 class SQLTest (unittest.TestCase):
 	
 	def setUp(self):
@@ -109,7 +111,51 @@
 		self.assertEquals(where.sql.replace("?", "%s") % tuple(where.arguments),
 			"(actor NOT IN (SELECT id FROM actor " \
 			"WHERE (value >= bar AND value < bas)) OR actor IS NULL)")
-		
+
+	def testUpgradeCorruption(self):
+		# Set up the testing environment:
+		DATABASE_TEST_FILE = os.environ.get("ZEITGEIST_DATABASE_PATH",
+			os.path.join(constants.DATA_PATH, "activity.test.sqlite"))
+		DATABASE_TEST_FILE_BACKUP = os.environ.get("ZEITGEIST_DATABASE_PATH",
+			os.path.join(constants.DATA_PATH, "activity.test.sqlite.bck"))
+		if os.path.exists(constants.DATABASE_FILE): 
+			shutil.move(constants.DATABASE_FILE, DATABASE_TEST_FILE)
+		if os.path.exists(constants.DATABASE_FILE_BACKUP):
+			shutil.move(constants.DATABASE_FILE_BACKUP, DATABASE_TEST_FILE_BACKUP)
+
+		# Ensure we are at version 0:
+		cursor = sql._connect_to_db(constants.DATABASE_FILE)
+		self.assertEqual(0, sql._get_schema_version(cursor, constants.CORE_SCHEMA))
+
+		# Run through a successful upgrade (core_0_1):
+		sql._do_schema_upgrade(cursor, constants.CORE_SCHEMA, 0, 1)
+		self.assertEquals(1, sql._get_schema_version(cursor, constants.CORE_SCHEMA))
+		sql._set_schema_version(cursor, constants.CORE_SCHEMA, 1)
+		sql._do_schema_backup()
+		self.assertTrue(os.path.exists(constants.DATABASE_FILE_BACKUP))
+
+		# Simulate a failed upgrade:
+		sql._set_schema_version(cursor, constants.CORE_SCHEMA, -1)
+
+		# ... and then try to fix it:
+		do_upgrade, cursor = sql._check_core_schema_upgrade(cursor)
+
+		# ... and fail again, as a table is missing inbetween core_2_3 and core_3_4:
+		self.assertFalse(do_upgrade)
+		self.assertEqual(-1, sql._get_schema_version(cursor, constants.CORE_SCHEMA))
+
+		# ... we then let the database structure run through create_db:
+		cursor = sql.create_db(constants.DATABASE_FILE)
+
+		# ... it is now well-structured:
+		self.assertEquals(constants.CORE_SCHEMA_VERSION,
+			sql._get_schema_version(cursor, constants.CORE_SCHEMA))
+
+		# Clean-up after ourselves:
+		if os.path.exists(DATABASE_TEST_FILE):
+			shutil.move(DATABASE_TEST_FILE, constants.DATABASE_FILE)
+		if os.path.exists(DATABASE_TEST_FILE_BACKUP):
+			shutil.move(DATABASE_TEST_FILE_BACKUP, constants.DATABASE_FILE_BACKUP)
 
 if __name__ == "__main__":
 	unittest.main()