Fixed bugs with internal client and object expirer

These bug fixes are lumped together because they all caused problems
with the object expirer doing its job.

There was a bug with the internal client doing listings that happened
to run across a Unicode object name for use as a marker.

There was a bug with the object expirer not utf8 encoding object
names it got from json listings, causing deletes to fail.

There was a bug with the object expirer url quoting object names when
calling the internal client's make_request, when make_request already
handles that.

Change-Id: I29fdd351fd60c8e63874b44d604c5fdff35169d4

swift/common/internal_client.py

@@ -214,7 +214,10 @@ class InternalClient(object):
         :raises Exception: Exception is raised when code fails in an
                            unexpected way.
         """
-
+        if isinstance(marker, unicode):
+            marker = marker.encode('utf8')
+        if isinstance(end_marker, unicode):
+            end_marker = end_marker.encode('utf8')
         while True:
             resp = self.make_request(
                 'GET', '%s?format=json&marker=%s&end_marker=%s' %
@@ -227,7 +230,7 @@ class InternalClient(object):
                 break
             for item in data:
                 yield item
-            marker = data[-1]['name']
+            marker = data[-1]['name'].encode('utf8')
 
     def make_path(self, account, container=None, obj=None):
         """

swift/obj/expirer.py

@@ -15,7 +15,6 @@
 
 from random import random
 from time import time
-from urllib import quote
 from os.path import join
 
 from eventlet import sleep, Timeout
@@ -26,11 +25,6 @@ from swift.common.utils import get_logger, dump_recon_cache
 from swift.common.http import HTTP_NOT_FOUND, HTTP_CONFLICT, \
     HTTP_PRECONDITION_FAILED
 
-try:
-    import simplejson as json
-except ImportError:
-    import json
-
 
 class ObjectExpirer(Daemon):
     """
@@ -104,7 +98,7 @@ class ObjectExpirer(Daemon):
                     break
                 for o in self.swift.iter_objects(self.expiring_objects_account,
                                                  container):
-                    obj = o['name']
+                    obj = o['name'].encode('utf8')
                     timestamp, actual_obj = obj.split('-', 1)
                     timestamp = int(timestamp)
                     if timestamp > int(time()):
@@ -168,6 +162,6 @@ class ObjectExpirer(Daemon):
         :param timestamp: The timestamp the X-Delete-At value must match to
                           perform the actual delete.
         """
-        self.swift.make_request('DELETE', '/v1/%s' % (quote(actual_obj),),
+        self.swift.make_request('DELETE', '/v1/%s' % actual_obj.lstrip('/'),
                                 {'X-If-Delete-At': str(timestamp)},
                                 (2, HTTP_NOT_FOUND, HTTP_PRECONDITION_FAILED))

test/unit/common/test_internal_client.py

@@ -484,12 +484,12 @@ class TestInternalClient(unittest.TestCase):
 
         paths = [
             '/?format=json&marker=start&end_marker=end',
-            '/?format=json&marker=one&end_marker=end',
+            '/?format=json&marker=one%C3%A9&end_marker=end',
             '/?format=json&marker=two&end_marker=end',
         ]
 
         responses = [
-            Response(200, json.dumps([{'name': 'one'}, ])),
+            Response(200, json.dumps([{'name': 'one\xc3\xa9'}, ])),
             Response(200, json.dumps([{'name': 'two'}, ])),
             Response(204, ''),
         ]
@@ -497,9 +497,9 @@ class TestInternalClient(unittest.TestCase):
         items = []
         client = InternalClient(self, paths, responses)
         for item in client._iter_items('/', marker='start', end_marker='end'):
-            items.append(item['name'])
+            items.append(item['name'].encode('utf8'))
 
-        self.assertEquals('one two'.split(), items)
+        self.assertEquals('one\xc3\xa9 two'.split(), items)
 
     def test_set_metadata(self):
         class InternalClient(internal_client.InternalClient):

test/unit/obj/test_expirer.py

@@ -285,6 +285,47 @@ class TestObjectExpirer(TestCase):
                '2 possible objects',), {}),
              (('Pass completed in 0s; 1 objects expired',), {})])
 
+    def test_delete_actual_object_does_not_get_unicode(self):
+        class InternalClient(object):
+            def __init__(self, containers, objects):
+                self.containers = containers
+                self.objects = objects
+
+            def get_account_info(*a, **kw):
+                return 1, 2
+
+            def iter_containers(self, *a, **kw):
+                return self.containers
+
+            def delete_container(*a, **kw):
+                pass
+
+            def delete_object(*a, **kw):
+                pass
+
+            def iter_objects(self, *a, **kw):
+                return self.objects
+
+        got_unicode = [False]
+
+        def delete_actual_object_test_for_unicode(actual_obj, timestamp):
+            if isinstance(actual_obj, unicode):
+                got_unicode[0] = True
+
+        x = expirer.ObjectExpirer({})
+        x.logger = FakeLogger()
+        x.delete_actual_object = delete_actual_object_test_for_unicode
+        self.assertEquals(x.report_objects, 0)
+        x.swift = InternalClient([{'name': str(int(time() - 86400))}],
+            [{'name': u'%d-actual-obj' % int(time() - 86400)}])
+        x.run_once()
+        self.assertEquals(x.report_objects, 1)
+        self.assertEquals(x.logger.log_dict['info'],
+            [(('Pass beginning; 1 possible containers; '
+               '2 possible objects',), {}),
+             (('Pass completed in 0s; 1 objects expired',), {})])
+        self.assertFalse(got_unicode[0])
+
     def test_failed_delete_continues_on(self):
         class InternalClient(object):
             def __init__(self, containers, objects):
@@ -416,6 +457,24 @@ class TestObjectExpirer(TestCase):
         x.delete_actual_object('/path/to/object', ts)
         self.assertEquals(got_env[0]['HTTP_X_IF_DELETE_AT'], ts)
 
+    def test_delete_actual_object_nourlquoting(self):
+        # delete_actual_object should not do its own url quoting because
+        # internal client's make_request handles that.
+        got_env = [None]
+
+        def fake_app(env, start_response):
+            got_env[0] = env
+            start_response('204 No Content', [('Content-Length', '0')])
+            return []
+
+        internal_client.loadapp = lambda x: fake_app
+
+        x = expirer.ObjectExpirer({})
+        ts = '1234'
+        x.delete_actual_object('/path/to/object name', ts)
+        self.assertEquals(got_env[0]['HTTP_X_IF_DELETE_AT'], ts)
+        self.assertEquals(got_env[0]['PATH_INFO'], '/v1/path/to/object name')
+
     def test_delete_actual_object_handles_404(self):
 
         def fake_app(env, start_response):