[PATCH 3/9] Re: Experimental LocalStatus stored in SQLite database

[Nicolas Sebrecht]

On Tue, Apr 26, 2011 at 12:31:34PM +0200, Sebastian Spaeth wrote:
> 
> Based on patches by Stewart Smith, updated by Rob Browning.
> This adds the file without using it yet.
> 
> Signed-off-by: Sebastian Spaeth <Sebastian at SSpaeth.de>
> ---
>  offlineimap/folder/LocalStatusSQLite.py |  202 +++++++++++++++++++++++++++++++
>  1 files changed, 202 insertions(+), 0 deletions(-)
>  create mode 100644 offlineimap/folder/LocalStatusSQLite.py
> 
> diff --git a/offlineimap/folder/LocalStatusSQLite.py b/offlineimap/folder/LocalStatusSQLite.py
> new file mode 100644
> index 0000000..974baf4
> --- /dev/null
> +++ b/offlineimap/folder/LocalStatusSQLite.py
> @@ -0,0 +1,202 @@
> +# Local status cache virtual folder: SQLite backend
> +# Copyright (C) 2009-2011 Stewart Smith and contributors
> +#
> +#    This program is free software; you can redistribute it and/or modify
> +#    it under the terms of the GNU General Public License as published by
> +#    the Free Software Foundation; either version 2 of the License, or
> +#    (at your option) any later version.
> +#
> +#    This program is distributed in the hope that it will be useful,
> +#    but WITHOUT ANY WARRANTY; without even the implied warranty of
> +#    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +#    GNU General Public License for more details.
> +#
> +#    You should have received a copy of the GNU General Public License
> +#    along with this program; if not, write to the Free Software
> +#    Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
> +
> +from Base import BaseFolder
> +import os, threading

Don't we want PEP8 (one import per line)? I remember you sending patches
like this. :-)

> +

Why a blank line?

> +from pysqlite2 import dbapi2 as sqlite
> +
> +magicline = "OFFLINEIMAP LocalStatus CACHE DATA - DO NOT MODIFY - FORMAT 1"
> +newmagicline = "OFFLINEIMAP LocalStatus NOW IN SQLITE, DO NOT MODIFY"
> +
> +class LocalStatusFolder(BaseFolder):
> +    """LocalStatus backend implemented with an SQLite database"""
> +    def __deinit__(self):
> +        self.save()
> +        self.cursor.close()
> +        self.connection.close()
> +
> +    def __init__(self, root, name, repository, accountname, config):
> +        self.name = name
> +        self.root = root
> +        self.sep = '.'
> +        self.config = config
> +        self.dofsync = config.getdefaultboolean("general", "fsync", True)
> +        self.filename = repository.getfolderfilename(name)
> +        self.messagelist = {}
> +        self.repository = repository
> +        self.savelock = threading.Lock()
> +        self.doautosave = 1
> +        self.accountname = accountname
> +        BaseFolder.__init__(self)
> +	self.dbfilename = self.filename + '.sqlite'

Is it the right place for this statement? No newline before?

> +
> +	# MIGRATE

I don't think we want such migration plans here. Handling database
format here could end with lot of migration code we don't usually care
about.

I think migration code should be done here _against a "TARGET_FORMAT"_.
Migration processes to go to this format would run all the missing
migration procedures.

Say we're at v6.3.3:1 (:1 for the database format release). Then, comes
v6.4.0:2 and later v6.5.0:3... Yeah, you got my point: what shoud we do
for people going from :1 to :3?  And we will get things worse than that
having SQL optionnal at the Account level.

Migrate from LocalStatus to LocalStatusSQLite _IS_ a migration like
above. A bit particular due to the creation of the database but it's
really the same.

IOW, we need something a bit more advanced NOW to force further updates
the right way.

> +	if os.path.exists(self.filename):
> +		self.connection = sqlite.connect(self.dbfilename)
> +		self.cursor = self.connection.cursor()
> +		self.cursor.execute('CREATE TABLE status (id INTEGER PRIMARY KEY, flags VARCHAR(50))')
> +		if self.isnewfolder():
> +                    self.messagelist = {}
> +	            return

Indentation looks ugly.

> +	        file = open(self.filename, "rt")
> +	        self.messagelist = {}

Repeated statement?

> +	        line = file.readline().strip()
> +	        assert(line == magicline)

I would add a comment here or implement a dummy (no-op) migration
process depending on the migration implementation.

> +	        for line in file.xreadlines():
> +	            line = line.strip()
> +	            uid, flags = line.split(':')
> +	            uid = long(uid)
> +	            flags = [x for x in flags]
> +		    flags.sort()
> +	            flags = ''.join(flags)
> +	            self.cursor.execute('INSERT INTO status (id,flags) VALUES (?,?)',
> +				(uid,flags))
> +	        file.close()

Indentation mistakes?

> +		self.connection.commit()
> +		os.rename(self.filename, self.filename + ".old")
> +		self.cursor.close()
> +		self.connection.close()
> +
> +	# create new

Notice: a creation is a migration. :-)

> +	if not os.path.exists(self.dbfilename):
> +		self.connection = sqlite.connect(self.dbfilename)
> +		self.cursor = self.connection.cursor()
> +		self.cursor.execute('CREATE TABLE status (id INTEGER PRIMARY KEY, flags VARCHAR(50))')
> +	else:
> +		self.connection = sqlite.connect(self.dbfilename)
> +		self.cursor = self.connection.cursor()

-- 
Nicolas Sebrecht