Fix misnamed variable in SwiftReader.

actual_md5 was being used instead of _actual_md5 only when assigning
the md5 object. This resulted in all checks and updates of the md5
to be skipped.

When fixed it revealed another bug that the md5 of the read content
is always checked even if there is no _expected_etag.

Added tests

Change-Id: Iaf1e21324ce592049d02cd5be123604b99833e86

2 files changed, 129 insertions, 17 deletions

diff --git a/swiftclient/service.py b/swiftclient/service.py
index 281947e..a964d4d 100644
--- a/swiftclient/service.py
+++ b/swiftclient/service.py
@@ -313,36 +313,37 @@ class _SwiftReader(object):
         self._actual_md5 = None
         self._expected_etag = headers.get('etag')
 
-        if 'x-object-manifest' not in headers and \
-                'x-static-large-object' not in headers:
-            self.actual_md5 = md5()
+        if ('x-object-manifest' not in headers
+                and 'x-static-large-object' not in headers):
+            self._actual_md5 = md5()
 
         if 'content-length' in headers:
-            self._content_length = int(headers.get('content-length'))
+            try:
+                self._content_length = int(headers.get('content-length'))
+            except ValueError:
+                raise SwiftError('content-length header must be an integer')
 
     def __enter__(self):
         return self
 
     def __exit__(self, exc_type, exc_val, exc_tb):
-        if self._actual_md5 is not None:
+        if self._actual_md5 and self._expected_etag:
             etag = self._actual_md5.hexdigest()
             if etag != self._expected_etag:
-                raise SwiftError(
-                    'Error downloading %s: md5sum != etag, %s != %s' %
-                    (self._path, etag, self._expected_etag)
-                )
+                raise SwiftError('Error downloading {0}: md5sum != etag, '
+                                 '{1} != {2}'.format(
+                                     self._path, etag, self._expected_etag))
 
-        if self._content_length is not None and \
-                self._actual_read != self._content_length:
-            raise SwiftError(
-                'Error downloading %s: read_length != content_length, '
-                '%d != %d' % (self._path, self._actual_read,
-                              self._content_length)
-            )
+        if (self._content_length is not None
+                and self._actual_read != self._content_length):
+            raise SwiftError('Error downloading {0}: read_length != '
+                             'content_length, {1:d} != {2:d}'.format(
+                                 self._path, self._actual_read,
+                                 self._content_length))
 
     def buffer(self):
         for chunk in self._body:
-            if self._actual_md5 is not None:
+            if self._actual_md5:
                 self._actual_md5.update(chunk)
             self._actual_read += len(chunk)
             yield chunk
diff --git a/tests/unit/test_service.py b/tests/unit/test_service.py
index 53867ae..5211456 100644
--- a/tests/unit/test_service.py
+++ b/tests/unit/test_service.py
@@ -14,8 +14,119 @@
 # limitations under the License.
 
 import testtools
+from hashlib import md5
 
 from swiftclient.service import SwiftService, SwiftError
+import swiftclient
+
+
+class TestSwiftPostObject(testtools.TestCase):
+
+    def setUp(self):
+        self.spo = swiftclient.service.SwiftPostObject
+        super(TestSwiftPostObject, self).setUp()
+
+    def test_create(self):
+        spo = self.spo('obj_name')
+
+        self.assertEqual(spo.object_name, 'obj_name')
+        self.assertEqual(spo.options, None)
+
+    def test_create_with_invalid_name(self):
+        # empty strings are not allowed as names
+        self.assertRaises(SwiftError, self.spo, '')
+
+        # names cannot be anything but strings
+        self.assertRaises(SwiftError, self.spo, 1)
+
+
+class TestSwiftReader(testtools.TestCase):
+
+    def setUp(self):
+        self.sr = swiftclient.service._SwiftReader
+        super(TestSwiftReader, self).setUp()
+        self.md5_type = type(md5())
+
+    def test_create(self):
+        sr = self.sr('path', 'body', {})
+
+        self.assertEqual(sr._path, 'path')
+        self.assertEqual(sr._body, 'body')
+        self.assertEqual(sr._content_length, None)
+        self.assertEqual(sr._expected_etag, None)
+
+        self.assertNotEqual(sr._actual_md5, None)
+        self.assertTrue(isinstance(sr._actual_md5, self.md5_type))
+
+    def test_create_with_large_object_headers(self):
+        # md5 should not be initalized if large object headers are present
+        sr = self.sr('path', 'body', {'x-object-manifest': 'test'})
+        self.assertEqual(sr._path, 'path')
+        self.assertEqual(sr._body, 'body')
+        self.assertEqual(sr._content_length, None)
+        self.assertEqual(sr._expected_etag, None)
+        self.assertEqual(sr._actual_md5, None)
+
+        sr = self.sr('path', 'body', {'x-static-large-object': 'test'})
+        self.assertEqual(sr._path, 'path')
+        self.assertEqual(sr._body, 'body')
+        self.assertEqual(sr._content_length, None)
+        self.assertEqual(sr._expected_etag, None)
+        self.assertEqual(sr._actual_md5, None)
+
+    def test_create_with_content_length(self):
+        sr = self.sr('path', 'body', {'content-length': 5})
+
+        self.assertEqual(sr._path, 'path')
+        self.assertEqual(sr._body, 'body')
+        self.assertEqual(sr._content_length, 5)
+        self.assertEqual(sr._expected_etag, None)
+
+        self.assertNotEqual(sr._actual_md5, None)
+        self.assertTrue(isinstance(sr._actual_md5, self.md5_type))
+
+        # Check Contentlength raises error if it isnt an integer
+        self.assertRaises(SwiftError, self.sr, 'path', 'body',
+                          {'content-length': 'notanint'})
+
+    def test_context_usage(self):
+        def _context(sr):
+            with sr:
+                pass
+
+        sr = self.sr('path', 'body', {})
+        _context(sr)
+
+        # Check error is raised if expected etag doesnt match calculated md5.
+        # md5 for a SwiftReader that has done nothing is
+        # d41d8cd98f00b204e9800998ecf8427e  i.e md5 of nothing
+        sr = self.sr('path', 'body', {'etag': 'doesntmatch'})
+        self.assertRaises(SwiftError, _context, sr)
+
+        sr = self.sr('path', 'body',
+                     {'etag': 'd41d8cd98f00b204e9800998ecf8427e'})
+        _context(sr)
+
+        # Check error is raised if SwiftReader doesnt read the same length
+        # as the content length it is created with
+        sr = self.sr('path', 'body', {'content-length': 5})
+        self.assertRaises(SwiftError, _context, sr)
+
+        sr = self.sr('path', 'body', {'content-length': 5})
+        sr._actual_read = 5
+        _context(sr)
+
+    def test_buffer(self):
+        # md5 = 97ac82a5b825239e782d0339e2d7b910
+        mock_buffer_content = ['abc'.encode()] * 3
+
+        sr = self.sr('path', mock_buffer_content, {})
+        for x in sr.buffer():
+            pass
+
+        self.assertEqual(sr._actual_read, 9)
+        self.assertEqual(sr._actual_md5.hexdigest(),
+                         '97ac82a5b825239e782d0339e2d7b910')
 
 
 class TestService(testtools.TestCase):