TombstoneReclaimer: quote string timestamp in sql query

Reclaim age and sync "timestamp" args passed to
DatabaseBroker.reclaim() and TombstoneReclaimer.__init__() are
expected to be floats. This is adhered to in code, but many tests pass
a string representation of a Timestamp instance. The string has been
tolerated because it has so far always been identical to a
str(float).

However, if a Timestamp instance has an offset then its string
representation is not equivalent to str(float), and
TombstoneReclaimer.reclaim() would raise an error because the reclaim
age value in the SQL query is not quoted and does not appear to be any
recognised type.

This patch quotes the reclaim age in the TombstoneReclaimer SQL
query. This makes it tolerant of being passed string representations
of Timestamps with offsets. This is not yet expected, but,
nevertheless, text in SQL queries should be quoted.

Change-Id: Ic70689db1e4ddc6f526c283bf0cbb5862b16fd90
Signed-off-by: Alistair Coles <alistairncoles@gmail.com>
Co-Authored-By: Alistair Coles <alistairncoles@gmail.com>

swift/common/db.py

@@ -252,7 +252,7 @@ class TombstoneReclaimer(object):
         ''' % self.broker.db_contains_type
         self.clean_batch_query = '''
             DELETE FROM %s WHERE deleted = 1
-            AND name >= ? AND %s < %s
+            AND name >= ? AND %s < '%s'
         ''' % (self.broker.db_contains_type, self.broker.db_reclaim_timestamp,
                self.age_timestamp)
 
@@ -1053,8 +1053,10 @@ class DatabaseBroker(object):
 
         Subclasses may reclaim other items by overriding :meth:`_reclaim`.
 
-        :param age_timestamp: max created_at timestamp of object rows to delete
-        :param sync_timestamp: max update_at timestamp of sync rows to delete
+        :param age_timestamp: (float) the max created_at timestamp of object
+            rows to delete
+        :param sync_timestamp: (float) the max update_at timestamp of sync rows
+            to delete
         """
         if not self._skip_commit_puts():
             with lock_parent_directory(self.pending_file,
@@ -1072,11 +1074,22 @@ class DatabaseBroker(object):
         """
         This is only called once at the end of reclaim after tombstone reclaim
         has been completed.
+
+        :param conn: db connection
+        :param age_timestamp: (float) the max created_at timestamp of object
+            rows to delete
+        :param sync_timestamp: (float) the max update_at timestamp of sync rows
+            to delete
         """
         self._reclaim_sync(conn, sync_timestamp)
         self._reclaim_metadata(conn, age_timestamp)
 
     def _reclaim_sync(self, conn, sync_timestamp):
+        """
+        :param conn: db connection
+        :param sync_timestamp: (float) the max update_at timestamp of sync rows
+            to delete
+        """
         try:
             conn.execute('''
                 DELETE FROM outgoing_sync WHERE updated_at < ?
@@ -1098,7 +1111,7 @@ class DatabaseBroker(object):
         from other related functions.
 
         :param conn: Database connection to reclaim metadata within.
-        :param timestamp: Empty metadata items last updated before this
+        :param timestamp: (float) Empty metadata items last updated before this
                           timestamp will be removed.
         :returns: True if conn.commit() should be called
         """

swift/container/backend.py

@@ -1662,6 +1662,16 @@ class ContainerBroker(DatabaseBroker):
         ''' % SHARD_RANGE_TABLE)
 
     def _reclaim_other_stuff(self, conn, age_timestamp, sync_timestamp):
+        """
+        This is only called once at the end of reclaim after tombstone reclaim
+        has been completed.
+
+        :param conn: db connection
+        :param age_timestamp: (float) the max created_at timestamp of object
+            rows to delete
+        :param sync_timestamp: (float) the max update_at timestamp of sync rows
+            to delete
+        """
         super(ContainerBroker, self)._reclaim_other_stuff(
             conn, age_timestamp, sync_timestamp)
         # populate instance cache, but use existing conn to avoid deadlock

test/unit/common/test_db.py

@@ -1676,7 +1676,7 @@ class TestTombstoneReclaimer(TestDbBase):
                 Timestamp(top_of_the_minute - (i * 60)), True)
             self._make_object(
                 broker, 'a_%3d' % (559 - i if reverse_names else i + 1),
-                Timestamp(top_of_the_minute - ((i + 1) * 60)), False)
+                Timestamp(top_of_the_minute - ((i + 1) * 60), offset=1), False)
             self._make_object(
                 broker, 'b_%3d' % (560 - i if reverse_names else i),
                 Timestamp(top_of_the_minute - ((i + 2) * 60)), True)
@@ -1792,6 +1792,22 @@ class TestTombstoneReclaimer(TestDbBase):
         self.assertEqual(0, self._get_reclaimable(broker, reclaim_age))
         self.assertEqual(140, actual)
 
+    def test_reclaim_with_timestamp_with_offset(self):
+        broker, totm, reclaim_age = self._setup_tombstones()
+
+        age_timestamp = Timestamp(reclaim_age, offset=0xabcd)
+        reclaimer = TombstoneReclaimer(broker, age_timestamp.internal)
+        reclaimer.reclaim()
+
+        self.assertEqual(0, self._get_reclaimable(broker, reclaim_age))
+        tombstones = self._get_reclaimable(broker, totm + 1)
+        self.assertEqual(140, tombstones)
+        # in this scenario the reclaim phase all tombstones (140)
+        self.assertEqual(0, reclaimer.remaining_tombstones)
+        # get_tombstone_count finds the rest
+        actual = reclaimer.get_tombstone_count()
+        self.assertEqual(140, actual)
+
 
 if __name__ == '__main__':
     unittest.main()