launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/ttb-from-disk into lp:launchpad.

Commit message:
Allow TranslationImportQueue to import entries from file objects rather than having to read arbitrarily-large files into memory.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #674575 in Launchpad itself: "addOrUpdateEntriesFromTarball expects the tarball in memory, this Is Bad"
  https://bugs.launchpad.net/launchpad/+bug/674575

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/ttb-from-disk/+merge/322759
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/ttb-from-disk into lp:launchpad.

=== modified file 'lib/lp/translations/doc/poexport-language-pack.txt'
--- lib/lp/translations/doc/poexport-language-pack.txt	2016-02-05 16:51:12 +0000
+++ lib/lp/translations/doc/poexport-language-pack.txt	2017-04-19 11:20:19 +0000
@@ -153,11 +153,11 @@
 
 Attach the en-US.xpi (the template) file.
 
-    >>> en_US_xpi =  get_en_US_xpi_file_to_import('en-US')
+    >>> en_US_xpi = get_en_US_xpi_file_to_import('en-US')
     >>> translation_import_queue = getUtility(ITranslationImportQueue)
     >>> by_maintainer = True
     >>> template_entry = translation_import_queue.addOrUpdateEntry(
-    ...     firefox_template.path, en_US_xpi.read(), by_maintainer,
+    ...     firefox_template.path, en_US_xpi, by_maintainer,
     ...     mark, distroseries=series, sourcepackagename=spn,
     ...     potemplate=firefox_template)
 
@@ -166,7 +166,7 @@
     >>> es_xpi = get_en_US_xpi_file_to_import('en-US')
     >>> firefox_es_translation = firefox_template.newPOFile('es')
     >>> translation_entry = translation_import_queue.addOrUpdateEntry(
-    ...     'es.xpi', es_xpi.read(), by_maintainer,
+    ...     'es.xpi', es_xpi, by_maintainer,
     ...     mark, distroseries=series, sourcepackagename=spn,
     ...     potemplate=firefox_template,
     ...     pofile=firefox_es_translation)

=== modified file 'lib/lp/translations/interfaces/translationimporter.py'
--- lib/lp/translations/interfaces/translationimporter.py	2013-01-07 02:40:55 +0000
+++ lib/lp/translations/interfaces/translationimporter.py	2017-04-19 11:20:19 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2017 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Interfaces to handle translation files imports."""
@@ -141,7 +141,7 @@
         """Return the translation file format for the given file extension.
 
         :param file_extension: File extension including the dot.
-        :param file_contents: File contents.
+        :param file_contents: File contents (a seekable file object).
         :return: A `TranslationFileFormat` for the given file extension
             and file contents or None if it's not supported format.
         """
@@ -182,10 +182,11 @@
     def getFormat(file_contents):
         """The file format of the import.
 
-        :param file_contents: A unicode string with the contents of the file
-            being imported.  A returned format may sometimes be different
-            from the base format of the `ITranslationFormatImporter`, and
-            that is determined based on the `contents`.
+        :param file_contents: A seekable file object with the contents of
+            the file being imported.  A returned format may sometimes be
+            different from the base format of the
+            `ITranslationFormatImporter`, and that is determined based on
+            the `contents`.
         :return: A `TranslationFileFormat` value.
         """
 

=== modified file 'lib/lp/translations/interfaces/translationimportqueue.py'
--- lib/lp/translations/interfaces/translationimportqueue.py	2014-08-20 06:59:52 +0000
+++ lib/lp/translations/interfaces/translationimportqueue.py	2017-04-19 11:20:19 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2017 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 from datetime import timedelta
@@ -313,7 +313,8 @@
         """Return a new or updated entry of the import queue.
 
         :arg path: is the path, with the filename, of the uploaded file.
-        :arg content: is the file content.
+        :arg content: is the file content, or a seekable file object open on
+            the file content.
         :arg by_maintainer: indicates if the file was uploaded by the
             maintainer of the project or package.
         :arg importer: is the person that did the import.
@@ -336,7 +337,8 @@
         only_templates=False):
         """Add all .po or .pot files from the tarball at :content:.
 
-        :arg content: is a tarball stream.
+        :arg content: is the tarball content, or a seekable file object open
+            on the tarball.
         :arg by_maintainer: indicates if the file was uploaded by the
             maintainer of the project or package.
         :arg importer: is the person that did the import.

=== modified file 'lib/lp/translations/model/translationimportqueue.py'
--- lib/lp/translations/model/translationimportqueue.py	2015-07-08 16:05:11 +0000
+++ lib/lp/translations/model/translationimportqueue.py	2017-04-19 11:20:19 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2017 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -8,8 +8,8 @@
     'TranslationImportQueue',
     ]
 
-from cStringIO import StringIO
 import datetime
+from io import BytesIO
 import logging
 from operator import attrgetter
 import os.path
@@ -1030,6 +1030,20 @@
         return (
             format, translation_importer.getTranslationFormatImporter(format))
 
+    def _getFileObjectAndSize(self, file_or_data):
+        """Get the size of a seekable file object."""
+        if (isinstance(file_or_data, bytes) or
+                isinstance(file_or_data, unicode)):
+            file_obj = BytesIO(file_or_data)
+            file_size = len(file_or_data)
+        else:
+            file_obj = file_or_data
+            start = file_obj.tell()
+            file_obj.seek(0, os.SEEK_END)
+            file_size = file_obj.tell()
+            file_obj.seek(start)
+        return file_obj, file_size
+
     def addOrUpdateEntry(self, path, content, by_maintainer, importer,
                          sourcepackagename=None, distroseries=None,
                          productseries=None, potemplate=None, pofile=None,
@@ -1040,16 +1054,18 @@
             "This one has either neither or both.")
         assert productseries is None or sourcepackagename is None, (
             "Can't upload to a sourcepackagename in a productseries.")
-        assert content is not None and content != '', "Upload has no content."
+        assert content is not None, "Upload has no content."
         assert path is not None and path != '', "Upload has no path."
 
+        file, size = self._getFileObjectAndSize(content)
+        assert size is not None and size != 0, "Upload has empty content."
+
         filename = os.path.basename(path)
         format, format_importer = self._getFormatAndImporter(
-            filename, content, format=format)
+            filename, file, format=format)
+        file.seek(0)
 
         # Upload the file into librarian.
-        size = len(content)
-        file = StringIO(content)
         client = getUtility(ILibrarianClient)
         alias = client.addFile(
             name=filename, size=size, file=file,
@@ -1144,9 +1160,9 @@
         num_files = 0
         conflict_files = []
 
-        tarball_io = StringIO(content)
+        tarball_io, _ = self._getFileObjectAndSize(content)
         try:
-            tarball = tarfile.open('', 'r|*', tarball_io)
+            tarball = tarfile.open('', 'r:*', tarball_io)
         except (tarfile.CompressionError, tarfile.ReadError):
             # If something went wrong with the tarfile, assume it's
             # busted and let the user deal with it.
@@ -1158,7 +1174,6 @@
             path = self._makePath(name, filename_filter)
             if self._isTranslationFile(path, only_templates):
                 upload_files[name] = path
-        tarball.close()
 
         if approver_factory is None:
             approver_factory = TranslationNullApprover
@@ -1167,14 +1182,10 @@
             productseries=productseries,
             distroseries=distroseries, sourcepackagename=sourcepackagename)
 
-        # Re-opening because we are using sequential access ("r|*") which is
-        # so much faster.
-        tarball_io.seek(0)
-        tarball = tarfile.open('', 'r|*', tarball_io)
         for tarinfo in tarball:
             if tarinfo.name not in upload_files:
                 continue
-            file_content = tarball.extractfile(tarinfo).read()
+            file_content = tarball.extractfile(tarinfo)
 
             path = upload_files[tarinfo.name]
             entry = approver.approve(self.addOrUpdateEntry(

=== modified file 'lib/lp/translations/model/translationtemplatesbuildbehaviour.py'
--- lib/lp/translations/model/translationtemplatesbuildbehaviour.py	2016-03-31 14:58:46 +0000
+++ lib/lp/translations/model/translationtemplatesbuildbehaviour.py	2017-04-19 11:20:19 +0000
@@ -1,4 +1,4 @@
-# Copyright 2010-2014 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2017 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """An `IBuildFarmJobBehaviour` for `TranslationTemplatesBuild`.
@@ -110,26 +110,17 @@
         filename = yield self._readTarball(
             self.build.buildqueue_record, filemap, logger)
 
-        # XXX 2010-11-12 bug=674575
-        # Please make addOrUpdateEntriesFromTarball() take files on
-        # disk; reading arbitrarily sized files into memory is
-        # dangerous.
         if filename is None:
             logger.error("Build produced no tarball.")
         else:
             tarball_file = open(filename)
             try:
-                tarball = tarball_file.read()
-                if tarball is None:
-                    logger.error("Build produced empty tarball.")
-                else:
-                    logger.debug(
-                        "Uploading translation templates tarball.")
-                    self._uploadTarball(
-                        self.build.buildqueue_record.specific_build.branch,
-                        tarball, logger)
-                    transaction.commit()
-                    logger.debug("Upload complete.")
+                logger.debug("Uploading translation templates tarball.")
+                self._uploadTarball(
+                    self.build.buildqueue_record.specific_build.branch,
+                    tarball_file, logger)
+                transaction.commit()
+                logger.debug("Upload complete.")
             finally:
                 tarball_file.close()
                 os.remove(filename)

=== modified file 'lib/lp/translations/scripts/upload_translations.py'
--- lib/lp/translations/scripts/upload_translations.py	2015-10-14 16:23:18 +0000
+++ lib/lp/translations/scripts/upload_translations.py	2017-04-19 11:20:19 +0000
@@ -1,4 +1,4 @@
-# Copyright 2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2011-2017 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -55,11 +55,11 @@
                 raise LaunchpadScriptFailure(
                     "File not readable: %s" % filename)
             self.logger.info("Uploading: %s." % filename)
-            content = open(filename).read()
-            queue.addOrUpdateEntry(
-                filename, content, True, rosetta_team,
-                sourcepackagename=self.sourcepackagename,
-                distroseries=self.distroseries)
+            with open(filename) as content:
+                queue.addOrUpdateEntry(
+                    filename, content, True, rosetta_team,
+                    sourcepackagename=self.sourcepackagename,
+                    distroseries=self.distroseries)
             self._commit()
 
         self.logger.info("Done.")

=== modified file 'lib/lp/translations/tests/test_translationimportqueue.py'
--- lib/lp/translations/tests/test_translationimportqueue.py	2013-07-26 12:48:37 +0000
+++ lib/lp/translations/tests/test_translationimportqueue.py	2017-04-19 11:20:19 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2017 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -409,7 +409,7 @@
             self._makeFile('po'),
             self._makeFile('xpi'),
             ))
-        tarfile_content = LaunchpadWriteTarFile.files_to_string(files)
+        tarfile_content = LaunchpadWriteTarFile.files_to_stream(files)
         self.import_queue.addOrUpdateEntriesFromTarball(
             tarfile_content, True, self.importer,
             productseries=self.productseries)
@@ -420,7 +420,7 @@
         files = dict((
             self._makeFile(),
             ))
-        tarfile_content = LaunchpadWriteTarFile.files_to_string(files)
+        tarfile_content = LaunchpadWriteTarFile.files_to_stream(files)
         self.import_queue.addOrUpdateEntriesFromTarball(
             tarfile_content, True, self.importer,
             productseries=self.productseries)
@@ -431,7 +431,7 @@
         files = dict((
             self._makeFile('pot', 'directory'),
             ))
-        tarfile_content = LaunchpadWriteTarFile.files_to_string(files)
+        tarfile_content = LaunchpadWriteTarFile.files_to_stream(files)
         self.import_queue.addOrUpdateEntriesFromTarball(
             tarfile_content, True, self.importer,
             productseries=self.productseries)
@@ -441,7 +441,7 @@
         # Leading slashes are stripped from path names.
         path, content = self._makeFile('pot', '/directory')
         files = dict(((path, content),))
-        tarfile_content = LaunchpadWriteTarFile.files_to_string(files)
+        tarfile_content = LaunchpadWriteTarFile.files_to_stream(files)
         self.import_queue.addOrUpdateEntriesFromTarball(
             tarfile_content, True, self.importer,
             productseries=self.productseries)

=== modified file 'lib/lp/translations/utilities/kde_po_importer.py'
--- lib/lp/translations/utilities/kde_po_importer.py	2015-07-08 16:05:11 +0000
+++ lib/lp/translations/utilities/kde_po_importer.py	2017-04-19 11:20:19 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2017 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Import module for legacy KDE .po files.
@@ -41,11 +41,17 @@
         # and with extremely big PO files, this will be too slow).  Thus,
         # a heuristic verified to be correct on all PO files from
         # Ubuntu language packs.
-        if ('msgid "_n: ' in file_contents or
-            'msgid ""\n"_n: ' in file_contents or
-            'msgid "_: ' in file_contents or
-            'msgid ""\n"_: ' in file_contents):
-            return TranslationFileFormat.KDEPO
+        msgid_start = False
+        for line in file_contents:
+            if line == b'msgid ""\n':
+                msgid_start = True
+            elif (line.startswith(b'msgid "_n: ') or
+                  (msgid_start and line.startswith(b'"_n: ')) or
+                  line.startswith(b'msgid "_: ') or
+                  (msgid_start and line.startswith(b'"_: '))):
+                return TranslationFileFormat.KDEPO
+            else:
+                msgid_start = False
         else:
             return TranslationFileFormat.PO
 

=== modified file 'lib/lp/translations/utilities/tests/helpers.py'
--- lib/lp/translations/utilities/tests/helpers.py	2011-09-18 08:39:01 +0000
+++ lib/lp/translations/utilities/tests/helpers.py	2017-04-19 11:20:19 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2017 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Helper module reused in different tests."""
@@ -25,7 +25,8 @@
     pofile=None, potemplate=None, by_maintainer=True):
     """Import a `POFile` or `POTemplate` from the given string.
 
-    :param file_contents: text of "file" to import.
+    :param file_contents: text of "file" to import, or a seekable file
+        object containing the text.
     :param person: party requesting the import.
     :param pofile: if uploading a `POFile`, file to import to; None otherwise.
     :param potemplate: if uploading a `POTemplate`, file to import to; None

=== modified file 'lib/lp/translations/utilities/tests/test_gettext_po_importer.py'
--- lib/lp/translations/utilities/tests/test_gettext_po_importer.py	2012-01-01 02:58:52 +0000
+++ lib/lp/translations/utilities/tests/test_gettext_po_importer.py	2017-04-19 11:20:19 +0000
@@ -1,10 +1,11 @@
-# Copyright 2009-2010 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2017 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Gettext PO importer tests."""
 
 __metaclass__ = type
 
+from io import BytesIO
 import unittest
 
 import transaction
@@ -89,7 +90,7 @@
 
     def testFormat(self):
         # GettextPOImporter reports that it handles the PO file format.
-        format = self.template_importer.getFormat(test_template)
+        format = self.template_importer.getFormat(BytesIO(test_template))
         self.failUnless(
             format == TranslationFileFormat.PO,
             'GettextPOImporter format expected PO but got %s' % format.name)

=== modified file 'lib/lp/translations/utilities/tests/test_kde_po_importer.py'
--- lib/lp/translations/utilities/tests/test_kde_po_importer.py	2012-01-01 02:58:52 +0000
+++ lib/lp/translations/utilities/tests/test_kde_po_importer.py	2017-04-19 11:20:19 +0000
@@ -1,10 +1,11 @@
-# Copyright 2009-2010 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2017 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """KDE PO importer tests."""
 
 __metaclass__ = type
 
+from io import BytesIO
 import unittest
 
 import transaction
@@ -114,7 +115,7 @@
 
     def testFormat(self):
         """Check whether KdePOImporter can handle the KDEPO file format."""
-        format = self.template_importer.getFormat(test_kde_template)
+        format = self.template_importer.getFormat(BytesIO(test_kde_template))
         self.failUnless(
             format == TranslationFileFormat.KDEPO,
             'KdePOImporter format expected KDEPO but got %s' % format.name)

=== modified file 'lib/lp/translations/utilities/tests/test_mozilla_xpi_importer.py'
--- lib/lp/translations/utilities/tests/test_mozilla_xpi_importer.py	2011-12-28 17:03:06 +0000
+++ lib/lp/translations/utilities/tests/test_mozilla_xpi_importer.py	2017-04-19 11:20:19 +0000
@@ -1,10 +1,11 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2017 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Mozilla XPI importer tests."""
 
 __metaclass__ = type
 
+from io import BytesIO
 import unittest
 
 from zope.interface.verify import verifyObject
@@ -34,7 +35,7 @@
 
     def testFormat(self):
         """Check that MozillaXpiImporter handles the XPI file format."""
-        format = self.importer.getFormat(u'')
+        format = self.importer.getFormat(BytesIO(b''))
         self.failUnless(
             format == TranslationFileFormat.XPI,
             'MozillaXpiImporter format expected XPI but got %s' % format.name)

=== modified file 'lib/lp/translations/utilities/tests/test_translation_importer.py'
--- lib/lp/translations/utilities/tests/test_translation_importer.py	2012-01-01 02:58:52 +0000
+++ lib/lp/translations/utilities/tests/test_translation_importer.py	2017-04-19 11:20:19 +0000
@@ -1,10 +1,12 @@
-# Copyright 2009-2010 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2017 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Translation Importer tests."""
 
 __metaclass__ = type
 
+from io import BytesIO
+
 import transaction
 
 from lp.services.log.logger import DevNullLogger
@@ -84,17 +86,17 @@
         self.assertEqual(
             TranslationFileFormat.PO,
             importer.getTranslationFileFormat(
-                ".po", u'msgid "message"\nmsgstr ""'))
+                ".po", BytesIO(b'msgid "message"\nmsgstr ""')))
 
         # And PO file with KDE-style messages is recognised as KDEPO file.
         self.assertEqual(
             TranslationFileFormat.KDEPO,
             importer.getTranslationFileFormat(
-                ".po", u'msgid "_: kde context\nmessage"\nmsgstr ""'))
+                ".po", BytesIO(b'msgid "_: kde context\nmessage"\nmsgstr ""')))
 
         self.assertEqual(
             TranslationFileFormat.XPI,
-            importer.getTranslationFileFormat(".xpi", u""))
+            importer.getTranslationFileFormat(".xpi", BytesIO(b"")))
 
     def testNoConflictingPriorities(self):
         """Check that no two importers for the same file extension have

=== modified file 'lib/lp/translations/utilities/tests/test_xpi_import.py'
--- lib/lp/translations/utilities/tests/test_xpi_import.py	2012-01-01 02:58:52 +0000
+++ lib/lp/translations/utilities/tests/test_xpi_import.py	2017-04-19 11:20:19 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2010 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2017 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Functional tests for XPI file format"""
@@ -61,7 +61,7 @@
         # Get the file to import.
         en_US_xpi = get_en_US_xpi_file_to_import(subdir)
         return import_pofile_or_potemplate(
-            file_contents=en_US_xpi.read(),
+            file_contents=en_US_xpi,
             person=self.importer,
             potemplate=self.firefox_template)
 
@@ -74,7 +74,7 @@
         # just use the same template file like a translation one.
         es_xpi = get_en_US_xpi_file_to_import(subdir)
         return import_pofile_or_potemplate(
-            file_contents=es_xpi.read(),
+            file_contents=es_xpi,
             person=self.importer,
             pofile=self.spanish_firefox,
             by_maintainer=True)

=== modified file 'lib/lp/translations/utilities/tests/test_xpi_po_exporter.py'
--- lib/lp/translations/utilities/tests/test_xpi_po_exporter.py	2012-03-27 13:41:38 +0000
+++ lib/lp/translations/utilities/tests/test_xpi_po_exporter.py	2017-04-19 11:20:19 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2017 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -82,7 +82,7 @@
         translation_import_queue = getUtility(ITranslationImportQueue)
         by_maintainer = True
         entry = translation_import_queue.addOrUpdateEntry(
-            self.firefox_template.path, en_US_xpi.read(), by_maintainer,
+            self.firefox_template.path, en_US_xpi, by_maintainer,
             self.importer, productseries=self.firefox_template.productseries,
             potemplate=self.firefox_template)
 

=== modified file 'lib/lp/translations/utilities/tests/test_xpi_search.py'
--- lib/lp/translations/utilities/tests/test_xpi_search.py	2012-01-01 02:58:52 +0000
+++ lib/lp/translations/utilities/tests/test_xpi_search.py	2017-04-19 11:20:19 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2017 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Functional tests for searching through XPI POTemplates"""
@@ -51,7 +51,7 @@
         # Get the file to import.
         en_US_xpi = get_en_US_xpi_file_to_import(subdir)
         return import_pofile_or_potemplate(
-            file_contents=en_US_xpi.read(),
+            file_contents=en_US_xpi,
             person=self.importer,
             potemplate=self.firefox_template)