- issue2551223 - fix timestamp truncation in mysql and postgresql

The data types used to represent timestamps in pg and mysql for
ephemeral tables: sessions and otks don't have enough signifcant
digits to work. As a result the timestamps are rounduped (up/down)
rsuling in the stored timestamp being 2 minutes (pg) or 2-3
hours(mysql) off from what it should be.

Modify db schema to use a numeric type that preserves more significant
figures. Implement schema upgrade. Document need for upgrade in
upgrading.txt.

Write tests for schema upgrade.

Implement test for updateTimestamp method on BasicDatabase that showed
this issue in the first place. Write overrides for test for
anydbm/memorydb which store timestamp properly or not at all.

Author: rouilj

CHANGES.txt

@@ -22,6 +22,10 @@ Fixed:
   sqlite. New databases are created for session data (db-session)
   and one time key data (db-otk). The data is ephemeral so no need to
   migrate.
+- issue2551223 - Timestamps are truncated in mysql and postgresql for
+  session and otk database tables. Modify db schema to use a numeric
+  type that preserves more significant figures. See upgrading.txt for
+  required steps.
 
 Features:
 

doc/upgrading.txt

@@ -35,6 +35,38 @@ Contents:
 Migrating from 2.2.0 to 2.3.0
 =============================
 
+Rdbms version change from 7 to 8 (required)
+-------------------------------------------
+
+This release includes a change that requires updates to the
+database schema.
+
+Sessions and one time key (otks) tables in the Mysql and
+PostgreSQL database use a numeric type that
+truncates/rounds expiration timestamps. This results in
+entries being purged early or late (depending on whether
+it rounds up or down). The discrepancy is a couple of
+days for Mysql or a couple of minutes for PostgreSQL.
+  
+Session keys stay for a week or more and CSRF keys are
+two weeks by default. As a result, this isn't usually a
+visible issue. This migration updates the numeric types
+to ones that supports more significant figures.
+
+You should backup your instance and run the
+``roundup-admin -i <tracker_home> migrate``
+command for all your trackers once you've
+installed the latest code base.
+
+Do this before you use the web, command-line or mail
+interface and before any users access the tracker.
+
+If successful, this command will respond with either
+"Tracker updated" (if you've not previously run it on an
+RDBMS backend) or "No migration action required" (if you
+have run it, or have used another interface to the tracker,
+or are using anydbm).
+
 Session/OTK data storage for SQLite backend changed
 ---------------------------------------------------
 

roundup/backends/back_mysql.py

@@ -219,13 +219,13 @@ def save_dbschema(self):
     def create_version_2_tables(self):
         # OTK store
         self.sql('''CREATE TABLE otks (otk_key VARCHAR(255),
-            otk_value TEXT, otk_time FLOAT(20))
+            otk_value TEXT, otk_time DOUBLE)
             ENGINE=%s'''%self.mysql_backend)
         self.sql('CREATE INDEX otks_key_idx ON otks(otk_key)')
 
         # Sessions store
         self.sql('''CREATE TABLE sessions (session_key VARCHAR(255),
-            session_time FLOAT(20), session_value TEXT)
+            session_time DOUBLE, session_value TEXT)
             ENGINE=%s'''%self.mysql_backend)
         self.sql('''CREATE INDEX sessions_key_idx ON
             sessions(session_key)''')
@@ -421,6 +421,13 @@ def fix_version_6_tables(self):
         #   column length and maxlength.
         c.execute(sql, (self.indexer.maxlength + 5,))
 
+    def fix_version_7_tables(self):
+        # Modify type for session.session_time/otk.otk_time column.
+        sql = "alter table sessions modify session_time double"
+        self.sql(sql)
+        sql = "alter table otks modify otk_time double"
+        self.sql(sql)
+
     def __repr__(self):
         return '<myroundsql 0x%x>'%id(self)
 

roundup/backends/back_postgresql.py

@@ -234,12 +234,12 @@ def restore_connection_on_error(self, savepoint="importing"):
     def create_version_2_tables(self):
         # OTK store
         self.sql('''CREATE TABLE otks (otk_key VARCHAR(255),
-            otk_value TEXT, otk_time REAL)''')
+            otk_value TEXT, otk_time float)''')
         self.sql('CREATE INDEX otks_key_idx ON otks(otk_key)')
 
         # Sessions store
         self.sql('''CREATE TABLE sessions (
-            session_key VARCHAR(255), session_time REAL,
+            session_key VARCHAR(255), session_time float,
             session_value TEXT)''')
         self.sql('''CREATE INDEX sessions_key_idx ON
             sessions(session_key)''')
@@ -296,6 +296,14 @@ def fix_version_6_tables(self):
 
         self._add_fts_table()
 
+    def fix_version_7_tables(self):
+        # Modify type for session.session_time/otk.otk_time column.
+        # float is double precision 15 signifcant digits
+        sql = 'alter table sessions alter column session_time type float'
+        self.sql(sql)
+        sql = 'alter table otks alter column otk_time type float'
+        self.sql(sql)
+
     def add_actor_column(self):
         # update existing tables to have the new actor column
         tables = self.database_schema['tables']

roundup/backends/rdbms_common.py

@@ -331,7 +331,7 @@ def post_init(self):
 
     # update this number when we need to make changes to the SQL structure
     # of the backend database
-    current_db_version = 7
+    current_db_version = 8
     db_version_updated = False
 
     def upgrade_db(self):
@@ -381,6 +381,10 @@ def upgrade_db(self):
             self.log_info('upgrade to version 7')
             self.fix_version_6_tables()
 
+        if version < 8:
+            self.log_info('upgrade to version 8')
+            self.fix_version_7_tables()
+
         self.database_schema['version'] = self.current_db_version
         self.db_version_updated = True
         return 1
@@ -422,6 +426,12 @@ def fix_version_6_tables(self):
         # You would think ALTER commands would be the same but nooo.
         pass
 
+    def fix_version_7_tables(self):
+        # Default (used by sqlite): NOOP
+        # Each backend mysql, postgres overrides this
+        # You would think ALTER commands would be the same but nooo.
+        pass
+
     def _convert_journal_tables(self):
         """Get current journal table contents, drop the table and re-create"""
         c = self.cursor

test/session_common.py

@@ -59,3 +59,32 @@ def testUpdateSession(self):
         self.sessions.set('random_key', text='nope')
         self.assertEqual(self.sessions.get('random_key', 'text'), 'nope')
 
+    # overridden in dbm and memory backends
+    def testUpdateTimestamp(self):
+        def get_ts_via_sql(self):
+            sql = '''select %(name)s_time from %(name)ss
+                 where %(name)s_key = '%(session)s';'''% \
+                     {'name': self.sessions.name,
+                      'session': 'random_session'}
+
+            self.sessions.cursor.execute(sql)
+            db_tstamp = self.sessions.cursor.fetchone()
+            return db_tstamp
+
+        # make sure timestamp is older than one minute so update will apply
+        timestamp = time.time() - 62
+        self.sessions.set('random_session', text='hello, world!',
+                          __timestamp=timestamp)
+
+        self.sessions.updateTimestamp('random_session')
+        # this doesn't work as the rdbms backends have a
+        # session_time, otk_time column and the timestamp in the
+        # session marshalled payload isn't updated. The dbm
+        # backend does update the __timestamp value so it works
+        # for dbm.
+        #self.assertNotEqual (self.sessions.get('random_session',
+        #                                       '__timestamp'),
+        #                     timestamp)
+
+        # use 61 to allow a fudge factor
+        self.assertGreater(get_ts_via_sql(self)[0] - timestamp, 61)

test/test_anydbm.py

@@ -55,6 +55,19 @@ class anydbmHTMLItemTest(HTMLItemTest, unittest.TestCase):
 class anydbmSessionTest(anydbmOpener, SessionTest, unittest.TestCase):
     s2b = lambda x,y: strings.s2b(y)
 
+    # this only works for dbm. other backends don't change the __timestamp
+    # value, they have a separate column for the timestamp so they can
+    # update it with SQL.
+    def testUpdateTimestamp(self):
+        # make sure timestamp is older than one minute so update will work
+        timestamp = time.time() - 62
+        self.sessions.set('random_session', text='hello, world!',
+                          __timestamp=timestamp)
+        self.sessions.updateTimestamp('random_session')
+        self.assertNotEqual (self.sessions.get('random_session',
+                                               '__timestamp'),
+                             timestamp)
+
 class anydbmSpecialActionTestCase(anydbmOpener, SpecialActionTest,
                                   unittest.TestCase):
     backend = 'anydbm'

test/test_memorydb.py

@@ -63,5 +63,9 @@ def setUp(self):
         setupSchema(self.db, 1, self.module)
         self.sessions = self.db.sessions
 
+    # doesn't work for memory as it uses a mock for session db.
+    def testUpdateTimestamp(self):
+        pass
+
 # vim: set filetype=python ts=4 sw=4 et si
 

test/test_mysql.py

@@ -71,7 +71,19 @@ def testUpgrade_6_to_7(self):
         self.db.issue.create(title="flebble frooz")
         self.db.commit()
 
-        if self.db.database_schema['version'] != 7:
+        if self.db.database_schema['version'] > 7:
+            # make testUpgrades run the downgrade code only.
+            if hasattr(self, "downgrade_only"):
+                # we are being called by an earlier test
+                self.testUpgrade_7_to_8()
+                self.assertEqual(self.db.database_schema['version'], 7)
+            else:
+                # we are being called directly
+                self.downgrade_only = True
+                self.testUpgrade_7_to_8()
+                self.assertEqual(self.db.database_schema['version'], 7)
+                del(self.downgrade_only)
+        elif self.db.database_schema['version'] != 7:
             self.skipTest("This test only runs for database version 7")
 
 
@@ -111,6 +123,65 @@ def testUpgrade_6_to_7(self):
         self.assertEqual(self.db.database_schema['version'],
                          self.db.current_db_version)
 
+    def testUpgrade_7_to_8(self):
+        # load the database
+        self.db.issue.create(title="flebble frooz")
+        self.db.commit()
+
+        if self.db.database_schema['version'] != 8:
+            self.skipTest("This test only runs for database version 8")
+
+        # change otk and session db's _time value to their original types
+        sql = "alter table sessions modify session_time float;"
+        self.db.sql(sql)
+        sql = "alter table otks modify otk_time float;"
+        self.db.sql(sql)
+
+        # verify they truncate long ints.
+        test_double =  1658718284.7616878
+        for tablename in ['otk', 'session']:
+            self.db.sql(
+              'insert %(name)ss(%(name)s_key, %(name)s_time, %(name)s_value) '
+              'values("foo", %(double)s, "value");'%{'name': tablename,
+                                                     'double': test_double}
+            )
+
+            self.db.cursor.execute('select %(name)s_time from %(name)ss '
+                            'where %(name)s_key = "foo"'%{'name': tablename})
+
+            self.assertNotAlmostEqual(self.db.cursor.fetchone()[0],
+                                      test_double, -1)
+
+            # cleanup or else the inserts after the upgrade will not
+            # work.
+            self.db.sql("delete from %(name)ss where %(name)s_key='foo'"%{
+                'name': tablename} )
+
+        self.db.database_schema['version'] = 7
+
+        if hasattr(self,"downgrade_only"):
+            return
+
+        # test upgrade
+        self.db.post_init()
+
+        # verify they keep all signifcant digits before the decimal point
+        for tablename in ['otk', 'session']:
+            self.db.sql(
+              'insert %(name)ss(%(name)s_key, %(name)s_time, %(name)s_value) '
+              'values("foo", %(double)s, "value");'%{'name': tablename,
+                                                     'double': test_double}
+            )
+
+            self.db.cursor.execute('select %(name)s_time from %(name)ss '
+                            'where %(name)s_key = "foo"'%{'name': tablename})
+
+            # -1 compares just the integer part. No fractions.
+            self.assertAlmostEqual(self.db.cursor.fetchone()[0],
+                                  test_double, -1)
+
+        self.assertEqual(self.db.database_schema['version'], 8)
+
 @skip_mysql
 class mysqlROTest(mysqlOpener, ROTest, unittest.TestCase):
     def setUp(self):

test/test_postgresql.py

@@ -84,9 +84,19 @@ def testUpgrade_6_to_7(self):
         self.db.issue.create(title="flebble frooz")
         self.db.commit()
 
-        if self.db.database_schema['version'] != 7:
-            # consider calling next testUpgrade script to roll back
-            # schema to version 7.
+        if self.db.database_schema['version'] > 7:
+            # make testUpgrades run the downgrade code only.
+            if hasattr(self, "downgrade_only"):
+                # we are being called by an earlier test
+                self.testUpgrade_7_to_8()
+                self.assertEqual(self.db.database_schema['version'], 7)
+            else:
+                # we are being called directly
+                self.downgrade_only = True
+                self.testUpgrade_7_to_8()
+                self.assertEqual(self.db.database_schema['version'], 7)
+                del(self.downgrade_only)
+        elif self.db.database_schema['version'] != 7:
             self.skipTest("This test only runs for database version 7")
 
         # remove __fts table/index; shrink length of  __words._words
@@ -140,6 +150,65 @@ def testUpgrade_6_to_7(self):
         self.assertEqual(self.db.database_schema['version'],
                          self.db.current_db_version)
 
+    def testUpgrade_7_to_8(self):
+        """ change _time fields in BasicDatabases to double """
+        # load the database
+        self.db.issue.create(title="flebble frooz")
+        self.db.commit()
+
+        if self.db.database_schema['version'] != 8:
+            self.skipTest("This test only runs for database version 8")
+
+        # change otk and session db's _time value to their original types
+        sql = "alter table sessions alter column session_time type REAL;"
+        self.db.sql(sql)
+        sql = "alter table otks alter column otk_time type REAL;"
+        self.db.sql(sql)
+
+        # verify they truncate long ints.
+        test_double =  1658718284.7616878
+        for tablename in ['otk', 'session']:
+            self.db.sql(
+              'insert into %(name)ss(%(name)s_key, %(name)s_time, %(name)s_value) '
+              "values ('foo', %(double)s, 'value');"%{'name': tablename,
+                                                     'double': test_double}
+            )
+
+            self.db.cursor.execute('select %(name)s_time from %(name)ss '
+                            "where %(name)s_key = 'foo'"%{'name': tablename})
+
+            self.assertNotAlmostEqual(self.db.cursor.fetchone()[0],
+                                      test_double, -1)
+
+            # cleanup or else the inserts after the upgrade will not
+            # work.
+            self.db.sql("delete from %(name)ss where %(name)s_key='foo'"%{
+                'name': tablename} )
+
+        self.db.database_schema['version'] = 7
+
+        if hasattr(self,"downgrade_only"):
+            return
+
+        # test upgrade altering row
+        self.db.post_init()
+
+        # verify they keep all signifcant digits before the decimal point
+        for tablename in ['otk', 'session']:
+            self.db.sql(
+              'insert into %(name)ss(%(name)s_key, %(name)s_time, %(name)s_value) '
+              "values ('foo', %(double)s, 'value');"%{'name': tablename,
+                                                     'double': test_double}
+            )
+
+            self.db.cursor.execute('select %(name)s_time from %(name)ss '
+                            "where %(name)s_key = 'foo'"%{'name': tablename})
+
+            self.assertAlmostEqual(self.db.cursor.fetchone()[0],
+                                      test_double, -1)
+
+        self.assertEqual(self.db.database_schema['version'], 8)
+
 @skip_postgresql
 class postgresqlROTest(postgresqlOpener, ROTest, unittest.TestCase):
     def setUp(self):