launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:productrelease-addreleasefile-bytes into launchpad:master.

Commit message:
Make ProductRelease.addReleaseFile take content as bytes

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/379650

More explicit, and needed for Python 3.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:productrelease-addreleasefile-bytes into launchpad:master.

diff --git a/lib/lp/registry/browser/tests/milestone-views.txt b/lib/lp/registry/browser/tests/milestone-views.txt
index 67c6566..2613124 100644
--- a/lib/lp/registry/browser/tests/milestone-views.txt
+++ b/lib/lp/registry/browser/tests/milestone-views.txt
@@ -231,7 +231,7 @@ files. If there is no release, or no files, None is returned.
 If there are files, these files will be returned as a list.
 
     >>> release_file = release.addReleaseFile(
-    ...     'test.txt', 'test', 'text/plain', person,
+    ...     'test.txt', b'test', 'text/plain', person,
     ...     signature_filename='test.txt.asc', signature_content='123',
     ...     description="test file")
     >>> view = create_view(milestone, '+index')
@@ -678,7 +678,7 @@ milestone.
     >>> release = milestone.createProductRelease(
     ...     owner, datetime.now(UTC))
     >>> release_file = release.addReleaseFile(
-    ...     'test', 'test', 'text/plain', owner, description="test file")
+    ...     'test', b'test', 'text/plain', owner, description="test file")
     >>> specification = factory.makeSpecification(product=firefox)
     >>> specification.milestone = milestone
     >>> bug = factory.makeBug(target=firefox)
diff --git a/lib/lp/registry/doc/productrelease-file-download.txt b/lib/lp/registry/doc/productrelease-file-download.txt
index 3f47337..067e5a7 100644
--- a/lib/lp/registry/doc/productrelease-file-download.txt
+++ b/lib/lp/registry/doc/productrelease-file-download.txt
@@ -49,16 +49,16 @@ Add a file alias to the productrelease.
     >>> from lp.services.webapp.interfaces import ILaunchBag
 
     >>> login("foo.bar@xxxxxxxxxxxxx")
-    >>> from zope.security.proxy import removeSecurityProxy
-    >>> from StringIO import StringIO
     >>> from datetime import datetime
+    >>> from io import BytesIO
     >>> from pytz import UTC
+    >>> from zope.security.proxy import removeSecurityProxy
     >>> def add_release_file(release, file_content, name, description,
     ...                      date_uploaded=None):
     ...     user = getUtility(ILaunchBag).user
     ...     result = release.addReleaseFile(
     ...         filename=name,
-    ...         file_content=StringIO(file_content),
+    ...         file_content=BytesIO(file_content),
     ...         content_type='test/plain',
     ...         uploader=user,
     ...         description=description)
@@ -66,7 +66,7 @@ Add a file alias to the productrelease.
     ...         removeSecurityProxy(result).date_uploaded = date_uploaded
     ...     return result
     >>> product_release_file = add_release_file(
-    ...    rel, 'Some useful information.',
+    ...    rel, b'Some useful information.',
     ...    'foo.txt', 'Foo file')
     >>> print product_release_file.description
     Foo file
@@ -170,7 +170,7 @@ Let's add some release files to the releases for firefox.
     >>> from datetime import timedelta
     >>> now = datetime.now(UTC)
     >>> for i, release in enumerate(releases):
-    ...     content = "Content %d" % i
+    ...     content = b"Content %d" % i
     ...     name = "name%d" % i
     ...     description = "description%d" % i
     ...     upload_date = now + timedelta(days=i)
diff --git a/lib/lp/registry/doc/productrelease.txt b/lib/lp/registry/doc/productrelease.txt
index 41ea69c..132cfb7 100644
--- a/lib/lp/registry/doc/productrelease.txt
+++ b/lib/lp/registry/doc/productrelease.txt
@@ -55,7 +55,7 @@ deleted.
     >>> milestone = firefox_1_0.newMilestone('1.0.10')
     >>> firefox_1010 = milestone.createProductRelease(owner,
     ...                                               datetime.now(UTC))
-    >>> firefox_1010.addReleaseFile('test', 'test', 'text/plain', owner)
+    >>> firefox_1010.addReleaseFile('test', b'test', 'text/plain', owner)
     <ProductReleaseFile...
     >>> firefox_1010.destroySelf()
     Traceback (most recent call last):
diff --git a/lib/lp/registry/interfaces/productrelease.py b/lib/lp/registry/interfaces/productrelease.py
index 552b174..c5aef3e 100644
--- a/lib/lp/registry/interfaces/productrelease.py
+++ b/lib/lp/registry/interfaces/productrelease.py
@@ -246,11 +246,11 @@ class IProductReleaseEditRestricted(Interface):
         The signature file will also be added if available.
 
         :param filename: Name of the file being uploaded.
-        :param file_content: StringIO or file object.
+        :param file_content: `io.BytesIO` or binary file object.
         :param content_type: A MIME content type string.
         :param uploader: The person who uploaded the file.
         :param signature_filename: Name of the uploaded gpg signature file.
-        :param signature_content: StringIO or file object.
+        :param signature_content: `io.BytesIO` or binary file object.
         :param file_type: An `UpstreamFileType` enum value.
         :param description: Info about the file.
         :returns: `IProductReleaseFile` object.
diff --git a/lib/lp/registry/model/productrelease.py b/lib/lp/registry/model/productrelease.py
index 16a4e47..6dfc8b3 100644
--- a/lib/lp/registry/model/productrelease.py
+++ b/lib/lp/registry/model/productrelease.py
@@ -9,8 +9,11 @@ __all__ = [
     'productrelease_to_milestone',
     ]
 
+from io import (
+    BufferedIOBase,
+    BytesIO,
+    )
 import os
-from StringIO import StringIO
 
 from sqlobject import (
     ForeignKey,
@@ -126,14 +129,14 @@ class ProductRelease(SQLBase):
     def _getFileObjectAndSize(self, file_or_data):
         """Return an object and length for file_or_data.
 
-        :param file_or_data: A string or a file object or StringIO object.
-        :return: file object or StringIO object and size.
+        :param file_or_data: `bytes` or `io.BufferedIOBase`.
+        :return: binary file object or `io.BytesIO` object and size.
         """
-        if isinstance(file_or_data, basestring):
+        if isinstance(file_or_data, bytes):
             file_size = len(file_or_data)
-            file_obj = StringIO(file_or_data)
+            file_obj = BytesIO(file_or_data)
         else:
-            assert isinstance(file_or_data, (file, StringIO)), (
+            assert isinstance(file_or_data, BufferedIOBase), (
                 "file_or_data is not an expected type")
             file_obj = file_or_data
             start = file_obj.tell()
diff --git a/lib/lp/registry/tests/test_productrelease.py b/lib/lp/registry/tests/test_productrelease.py
index a93b9e9..26c7410 100644
--- a/lib/lp/registry/tests/test_productrelease.py
+++ b/lib/lp/registry/tests/test_productrelease.py
@@ -78,7 +78,7 @@ class ProductReleaseFileTestcase(TestCaseWithFactory):
         maintainer = release.milestone.product.owner
         with person_logged_in(maintainer):
             release_file = release.addReleaseFile(
-                'pting.txt', 'test', 'text/plain', maintainer,
+                'pting.txt', b'test', 'text/plain', maintainer,
                 file_type=UpstreamFileType.README, description='desc')
         self.assertEqual('desc', release_file.description)
         self.assertEqual(UpstreamFileType.README, release_file.filetype)
@@ -93,7 +93,7 @@ class ProductReleaseFileTestcase(TestCaseWithFactory):
         with person_logged_in(maintainer):
             self.assertRaises(
                 InvalidFilename, release.addReleaseFile,
-                library_file.filename, 'test', 'text/plain', maintainer)
+                library_file.filename, b'test', 'text/plain', maintainer)
 
     def test_addReleaseFile_only_works_on_public_products(self):
         owner = self.factory.makePerson()
@@ -104,4 +104,4 @@ class ProductReleaseFileTestcase(TestCaseWithFactory):
             self.assertFalse(release.can_have_release_files)
             self.assertRaises(
                 ProprietaryProduct, release.addReleaseFile,
-                'README', 'test', 'text/plain', owner)
+                'README', b'test', 'text/plain', owner)
diff --git a/lib/lp/testing/factory.py b/lib/lp/testing/factory.py
index bac9b17..c3e8a01 100644
--- a/lib/lp/testing/factory.py
+++ b/lib/lp/testing/factory.py
@@ -967,7 +967,7 @@ class BareLaunchpadObjectFactory(ObjectFactory):
                                               milestone=milestone)
         with person_logged_in(release.milestone.product.owner):
             release_file = release.addReleaseFile(
-                filename, 'test', 'text/plain',
+                filename, b'test', 'text/plain',
                 uploader=release.milestone.product.owner,
                 signature_filename=signature_filename,
                 signature_content=signature_content,