Merge lp:~cjwatson/launchpad/custom-upload-parsing into lp:launchpad

[Colin Watson]

Redesign the CustomUpload interface to allow pushing semi-duplicated parsing code down from CustomUploadsCopier.

== Summary ==

In bug 827973, Jeroen noted that the custom uploads copier has code that is rather similar to the filename parsing in custom upload implementations, and that these should be unified.

== Proposed fix ==

Redesign the custom upload interface to support both its previous uses and the custom upload copier.

Note that this should make a subsequent fix for bug 827941 trivial.

== Implementation details ==

The custom upload constructor now takes no parameters, with these being passed in later. This makes the new filename parsing methods implemented by each custom upload type (setTargetDirectory and getSeriesKey) make somewhat more sense, and avoids inelegant passing of None in tests.

I discarded the business where extractNameFields uses a default architecture of "all". Firstly, this should now be handled by custom upload implementations if at all. But secondly and more importantly, I know of no real-world case where it matters. The copier only handles debian-installer and dist-upgrader uploads right now. debian-installer uploads always have an architecture (e.g. https://launchpadlibrarian.net/106301545/debian-installer_20101020ubuntu144_i386.changes), and dist-upgrader uploads always have "all" (e.g. https://launchpadlibrarian.net/103130380/update-manager_0.156.14.1_i386.changes). When we add support for ddtp-tarball uploads, they will simply return a different key (the component) rather than worrying about a default architecture.

I pushed the check for conflicts in the filesystem down into CustomUpload. The ddtp-tarball implementation overrides this, since it doesn't care.

== LOC Rationale ==

+25 (my first attempt was +119, but its interface was the wrong shape). However, this is part of an arc of work building on r15312, which was -199, so I'd like to claim credit against that.

== Tests ==

bin/test -vvct test_customupload -t test_debian_installer -t test_dist_upgrader -t test_ddtp_tarball -t test_distroseriesqueue -t test_custom_uploads_copier

== Demo and Q/A ==

None needed; this is refactoring.

[William Grant (wgrant)]

Looks reasonable to me, although I'd personally prefer to turn getSeriesKey into a classmethod and merge setTargetDirectory back into __init__.

review: Approve (code)

[Colin Watson (cjwatson)]

I've made the classmethod change, thanks. I prefer to keep setTargetDirectory separate, though, in order that all processing failures happen during process() rather than some of them happening during the constructor instead; this makes more sense to me, and it makes it easier to consolidate the previously-duplicated *AlreadyExists code too.

=== modified file 'lib/lp/archivepublisher/customupload.py'
--- lib/lp/archivepublisher/customupload.py	2012-01-06 11:08:30 +0000
+++ lib/lp/archivepublisher/customupload.py	2012-05-30 10:30:27 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Infrastructure for handling custom uploads.
@@ -71,7 +71,7 @@
 
 class CustomUploadTarballBadFile(CustomUploadError):
     """A file was found which resolves outside the immediate tree.
-    
+
     This can happen if someone embeds ../file in the tar, for example.
     """
     def __init__(self, tarfile_path, file_name):
@@ -80,30 +80,64 @@
         CustomUploadError.__init__(self, message)
 
 
+class CustomUploadAlreadyExists(CustomUploadError):
+    """A build for this type, architecture, and version already exists."""
+    def __init__(self, custom_type, arch, version):
+        message = ('%s build %s for architecture %s already exists' %
+                   (custom_type, arch, version))
+        CustomUploadError.__init__(self, message)
+
+
 class CustomUpload:
     """Base class for custom upload handlers"""
 
-    # The following should be overridden by subclasses, probably in
-    # their __init__
-    targetdir = None
-    version = None
+    # This should be set as a class property on each subclass.
+    custom_type = None
 
-    def __init__(self, archive_root, tarfile_path, distroseries):
-        self.archive_root = archive_root
-        self.tarfile_path = tarfile_path
-        self.distroseries = distroseries
+    def __init__(self):
+        self.targetdir = None
+        self.version = None
+        self.arch = None
 
         self.tmpdir = None
 
-    def process(self):
+    def process(self, archive_root, tarfile_path, distroseries):
         """Process the upload and install it into the archive."""
+        self.tarfile_path = tarfile_path
         try:
+            self.setTargetDirectory(archive_root, tarfile_path, distroseries)
+            self.checkForConflicts()
             self.extract()
             self.installFiles()
             self.fixCurrentSymlink()
         finally:
             self.cleanup()
 
+    def setTargetDirectory(self, archive_root, tarfile_path, distroseries):
+        """Set self.targetdir based on parameters.
+
+        This should also set self.version and self.arch (if applicable) as a
+        side-effect.
+        """
+        raise NotImplementedError
+
+    @classmethod
+    def getSeriesKey(cls, tarfile_path):
+        """Get a unique key for instances of this custom upload type.
+
+        The key should differ for any uploads that may be published
+        simultaneously, but should be identical for (e.g.) different
+        versions of the same type of upload on the same architecture in the
+        same series.  Returns None on failure to parse tarfile_path.
+        """
+        raise NotImplementedError
+
+    def checkForConflicts(self):
+        """Check for conflicts with existing publications in the archive."""
+        if os.path.exists(os.path.join(self.targetdir, self.version)):
+            raise CustomUploadAlreadyExists(
+                self.custom_type, self.arch, self.version)
+
     def verifyBeforeExtracting(self, tar):
         """Verify the tarball before extracting it.
 

=== modified file 'lib/lp/archivepublisher/ddtp_tarball.py'
--- lib/lp/archivepublisher/ddtp_tarball.py	2012-04-16 23:02:44 +0000
+++ lib/lp/archivepublisher/ddtp_tarball.py	2012-05-30 10:30:27 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """The processing of translated packages descriptions (ddtp) tarballs.
@@ -42,14 +42,20 @@
 
     Old contents will be preserved.
     """
-    def __init__(self, archive_root, tarfile_path, distroseries):
-        CustomUpload.__init__(self, archive_root, tarfile_path, distroseries)
+    custom_type = "ddtp-tarball"
 
+    def setTargetDirectory(self, archive_root, tarfile_path, distroseries):
         tarfile_base = os.path.basename(tarfile_path)
         name, component, self.version = tarfile_base.split('_')
+        self.arch = None
+
         self.targetdir = os.path.join(archive_root, 'dists',
                                       distroseries, component)
 
+    def checkForConflicts(self):
+        # We just overwrite older files, so no conflicts are possible.
+        pass
+
     def shouldInstall(self, filename):
         # Ignore files outside of the i18n subdirectory
         return filename.startswith('i18n/')
@@ -66,6 +72,5 @@
     Raises CustomUploadError (or some subclass thereof) if
     anything goes wrong.
     """
-    upload = DdtpTarballUpload(archive_root, tarfile_path, distroseries)
-    upload.process()
-
+    upload = DdtpTarballUpload()
+    upload.process(archive_root, tarfile_path, distroseries)

=== modified file 'lib/lp/archivepublisher/debian_installer.py'
--- lib/lp/archivepublisher/debian_installer.py	2012-05-23 23:02:08 +0000
+++ lib/lp/archivepublisher/debian_installer.py	2012-05-30 10:30:27 +0000
@@ -8,23 +8,15 @@
 
 __metaclass__ = type
 
-__all__ = ['process_debian_installer']
+__all__ = [
+    'DebianInstallerUpload',
+    'process_debian_installer',
+    ]
 
 import os
 import shutil
 
-from lp.archivepublisher.customupload import (
-    CustomUpload,
-    CustomUploadError,
-    )
-
-
-class DebianInstallerAlreadyExists(CustomUploadError):
-    """A build for this type, architecture, and version already exists."""
-    def __init__(self, arch, version):
-        message = ('installer build %s for architecture %s already exists' %
-                   (arch, version))
-        CustomUploadError.__init__(self, message)
+from lp.archivepublisher.customupload import CustomUpload
 
 
 class DebianInstallerUpload(CustomUpload):
@@ -46,20 +38,24 @@
 
     A 'current' symbolic link points to the most recent version.
     """
-    def __init__(self, archive_root, tarfile_path, distroseries):
-        CustomUpload.__init__(self, archive_root, tarfile_path, distroseries)
+    custom_type = "installer"
 
+    def setTargetDirectory(self, archive_root, tarfile_path, distroseries):
         tarfile_base = os.path.basename(tarfile_path)
-        components = tarfile_base.split('_')
-        self.version = components[1]
-        self.arch = components[2].split('.')[0]
+        _, self.version, self.arch = tarfile_base.split("_")
+        self.arch = self.arch.split(".")[0]
 
         self.targetdir = os.path.join(
             archive_root, 'dists', distroseries, 'main',
             'installer-%s' % self.arch)
 
-        if os.path.exists(os.path.join(self.targetdir, self.version)):
-            raise DebianInstallerAlreadyExists(self.arch, self.version)
+    @classmethod
+    def getSeriesKey(cls, tarfile_path):
+        try:
+            _, _, arch = os.path.basename(tarfile_path).split("_")
+            return arch.split(".")[0]
+        except ValueError:
+            return None
 
     def extract(self):
         CustomUpload.extract(self)
@@ -81,5 +77,5 @@
     Raises CustomUploadError (or some subclass thereof) if anything goes
     wrong.
     """
-    upload = DebianInstallerUpload(archive_root, tarfile_path, distroseries)
-    upload.process()
+    upload = DebianInstallerUpload()
+    upload.process(archive_root, tarfile_path, distroseries)

=== modified file 'lib/lp/archivepublisher/dist_upgrader.py'
--- lib/lp/archivepublisher/dist_upgrader.py	2010-08-20 20:31:18 +0000
+++ lib/lp/archivepublisher/dist_upgrader.py	2012-05-30 10:30:27 +0000
@@ -1,11 +1,14 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """The processing of dist-upgrader tarballs."""
 
 __metaclass__ = type
 
-__all__ = ['process_dist_upgrader']
+__all__ = [
+    'DistUpgraderUpload',
+    'process_dist_upgrader',
+    ]
 
 import os
 
@@ -19,14 +22,6 @@
     )
 
 
-class DistUpgraderAlreadyExists(CustomUploadError):
-    """A build for this type, version already exists."""
-    def __init__(self, arch, version):
-        message = ('dist-upgrader build %s for architecture %s already exists'%
-                   (arch, version))
-        CustomUploadError.__init__(self, message)
-
-
 class DistUpgraderBadVersion(CustomUploadError):
     def __init__(self, tarfile_path, exc):
         message = "bad version found in '%s': %s" % (tarfile_path, str(exc))
@@ -60,20 +55,23 @@
 
     A 'current' symbolic link points to the most recent version.
     """
-    def __init__(self, archive_root, tarfile_path, distroseries):
-        CustomUpload.__init__(self, archive_root, tarfile_path, distroseries)
+    custom_type = "dist-upgrader"
 
+    def setTargetDirectory(self, archive_root, tarfile_path, distroseries):
         tarfile_base = os.path.basename(tarfile_path)
-        name, self.version, arch = tarfile_base.split('_')
-        arch = arch.split('.')[0]
+        name, self.version, self.arch = tarfile_base.split("_")
+        self.arch = self.arch.split(".")[0]
 
         self.targetdir = os.path.join(archive_root, 'dists', distroseries,
-                                      'main', 'dist-upgrader-%s' % arch)
+                                      'main', 'dist-upgrader-%s' % self.arch)
 
-        # Make sure the target version doesn't already exist. If it does, raise
-        # DistUpgraderAlreadyExists.
-        if os.path.exists(os.path.join(self.targetdir, self.version)):
-            raise DistUpgraderAlreadyExists(arch, self.version)
+    @classmethod
+    def getSeriesKey(cls, tarfile_path):
+        try:
+            _, _, arch = os.path.basename(tarfile_path).split("_")
+            return arch.split(".")[0]
+        except ValueError:
+            return None
 
     def shouldInstall(self, filename):
         """ Install files from a dist-upgrader tarball.
@@ -104,5 +102,5 @@
     Raises CustomUploadError (or some subclass thereof) if anything goes
     wrong.
     """
-    upload = DistUpgraderUpload(archive_root, tarfile_path, distroseries)
-    upload.process()
+    upload = DistUpgraderUpload()
+    upload.process(archive_root, tarfile_path, distroseries)

=== modified file 'lib/lp/archivepublisher/tests/test_customupload.py'
--- lib/lp/archivepublisher/tests/test_customupload.py	2012-02-21 22:46:28 +0000
+++ lib/lp/archivepublisher/tests/test_customupload.py	2012-05-30 10:30:27 +0000
@@ -46,7 +46,7 @@
         """
         # Setup a bogus `CustomUpload` object with the 'targetdir' pointing
         # to the directory created for the test.
-        custom_processor = CustomUpload(None, None, None)
+        custom_processor = CustomUpload()
         custom_processor.targetdir = self.test_dir
 
         # Let's create 4 entries named as valid versions.
@@ -79,10 +79,10 @@
     def setUp(self):
         TestCase.setUp(self)
         self.tarfile_path = "/tmp/_verify_extract"
-        self.tarfile_name = os.path.join(
-            self.tarfile_path, "test_tarfile.tar")
-        self.custom_processor = CustomUpload(None, self.tarfile_name, None)
+        self.tarfile_name = os.path.join(self.tarfile_path, "test_tarfile.tar")
+        self.custom_processor = CustomUpload()
         self.custom_processor.tmpdir = self.makeTemporaryDirectory()
+        self.custom_processor.tarfile_path = self.tarfile_name
 
     def createTarfile(self):
         self.tar_fileobj = cStringIO.StringIO()

=== modified file 'lib/lp/archivepublisher/tests/test_debian_installer.py'
--- lib/lp/archivepublisher/tests/test_debian_installer.py	2012-05-25 13:26:08 +0000
+++ lib/lp/archivepublisher/tests/test_debian_installer.py	2012-05-30 10:30:27 +0000
@@ -9,9 +9,12 @@
 
 import os
 
-from lp.archivepublisher.customupload import CustomUploadBadUmask
+from lp.archivepublisher.customupload import (
+    CustomUploadAlreadyExists,
+    CustomUploadBadUmask,
+    )
 from lp.archivepublisher.debian_installer import (
-    DebianInstallerAlreadyExists,
+    DebianInstallerUpload,
     process_debian_installer,
     )
 from lp.services.tarfile_helpers import LaunchpadWriteTarFile
@@ -69,7 +72,7 @@
         # If the target directory already exists, processing fails.
         self.openArchive()
         os.makedirs(self.getInstallerPath("."))
-        self.assertRaises(DebianInstallerAlreadyExists, self.process)
+        self.assertRaises(CustomUploadAlreadyExists, self.process)
 
     def test_bad_umask(self):
         # The umask must be 022 to avoid incorrect permissions.
@@ -145,3 +148,21 @@
             0644, os.stat(self.getInstallerPath(filename)).st_mode & 0777)
         self.assertEqual(
             0755, os.stat(self.getInstallerPath(directory)).st_mode & 0777)
+
+    def test_getSeriesKey_extracts_architecture(self):
+        # getSeriesKey extracts the architecture from an upload's filename.
+        self.openArchive()
+        self.assertEqual(
+            self.arch, DebianInstallerUpload.getSeriesKey(self.path))
+
+    def test_getSeriesKey_returns_None_on_mismatch(self):
+        # getSeriesKey returns None if the filename does not match the
+        # expected pattern.
+        self.assertIsNone(DebianInstallerUpload.getSeriesKey("argh_1.0.jpg"))
+
+    def test_getSeriesKey_refuses_names_with_wrong_number_of_fields(self):
+        # getSeriesKey requires exactly three fields.
+        self.assertIsNone(DebianInstallerUpload.getSeriesKey(
+            "package_1.0.tar.gz"))
+        self.assertIsNone(DebianInstallerUpload.getSeriesKey(
+            "one_two_three_four_5.tar.gz"))

=== modified file 'lib/lp/archivepublisher/tests/test_dist_upgrader.py'
--- lib/lp/archivepublisher/tests/test_dist_upgrader.py	2012-05-25 13:27:41 +0000
+++ lib/lp/archivepublisher/tests/test_dist_upgrader.py	2012-05-30 10:30:27 +0000
@@ -9,10 +9,13 @@
 
 import os
 
-from lp.archivepublisher.customupload import CustomUploadBadUmask
+from lp.archivepublisher.customupload import (
+    CustomUploadAlreadyExists,
+    CustomUploadBadUmask,
+    )
 from lp.archivepublisher.dist_upgrader import (
-    DistUpgraderAlreadyExists,
     DistUpgraderBadVersion,
+    DistUpgraderUpload,
     process_dist_upgrader,
     )
 from lp.services.tarfile_helpers import LaunchpadWriteTarFile
@@ -55,7 +58,7 @@
         self.openArchive("20060302.0120")
         self.archive.add_file("20060302.0120/hello", "world")
         os.makedirs(os.path.join(self.getUpgraderPath(), "20060302.0120"))
-        self.assertRaises(DistUpgraderAlreadyExists, self.process)
+        self.assertRaises(CustomUploadAlreadyExists, self.process)
 
     def test_bad_umask(self):
         # The umask must be 022 to avoid incorrect permissions.
@@ -84,3 +87,20 @@
         self.openArchive("20070219.1234")
         self.archive.add_file("foobar/foobar/dapper.tar.gz", "")
         self.assertRaises(DistUpgraderBadVersion, self.process)
+
+    def test_getSeriesKey_extracts_architecture(self):
+        # getSeriesKey extracts the architecture from an upload's filename.
+        self.openArchive("20060302.0120")
+        self.assertEqual("all", DistUpgraderUpload.getSeriesKey(self.path))
+
+    def test_getSeriesKey_returns_None_on_mismatch(self):
+        # getSeriesKey returns None if the filename does not match the
+        # expected pattern.
+        self.assertIsNone(DistUpgraderUpload.getSeriesKey("argh_1.0.jpg"))
+
+    def test_getSeriesKey_refuses_names_with_wrong_number_of_fields(self):
+        # getSeriesKey requires exactly three fields.
+        self.assertIsNone(DistUpgraderUpload.getSeriesKey(
+            "package_1.0.tar.gz"))
+        self.assertIsNone(DistUpgraderUpload.getSeriesKey(
+            "one_two_three_four_5.tar.gz"))

=== modified file 'lib/lp/archiveuploader/nascentuploadfile.py'
--- lib/lp/archiveuploader/nascentuploadfile.py	2012-04-27 11:50:45 +0000
+++ lib/lp/archiveuploader/nascentuploadfile.py	2012-05-30 10:30:27 +0000
@@ -254,7 +254,8 @@
     results in new archive files.
     """
 
-    # This is a marker as per the comment in dbschema.py: ##CUSTOMFORMAT##
+    # This is a marker as per the comment in lib/lp/soyuz/enums.py:
+    ##CUSTOMFORMAT##
     # Essentially if you change anything to do with custom formats, grep for
     # the marker in the codebase and make sure the same changes are made
     # everywhere which needs them.

=== modified file 'lib/lp/soyuz/enums.py'
--- lib/lp/soyuz/enums.py	2012-04-24 17:24:32 +0000
+++ lib/lp/soyuz/enums.py	2012-05-30 10:30:27 +0000
@@ -1,4 +1,4 @@
-# Copyright 2010-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2012 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Enumerations used in the lp/soyuz modules."""
@@ -423,8 +423,8 @@
 
 
 # If you change this (add items, change the meaning, whatever) search for
-# the token ##CUSTOMFORMAT## e.g. database/queue.py or nascentupload.py and
-# update the stuff marked with it.
+# the token ##CUSTOMFORMAT## e.g. queue.py or nascentupload.py and update
+# the stuff marked with it.
 class PackageUploadCustomFormat(DBEnumeratedType):
     """Custom formats valid for the upload queue
 

=== modified file 'lib/lp/soyuz/model/queue.py'
--- lib/lp/soyuz/model/queue.py	2012-05-14 03:12:44 +0000
+++ lib/lp/soyuz/model/queue.py	2012-05-30 10:30:27 +0000
@@ -1238,7 +1238,7 @@
 
     def publish(self, logger=None):
         """See `IPackageUploadCustom`."""
-        # This is a marker as per the comment in dbschema.py.
+        # This is a marker as per the comment in lib/lp/soyuz/enums.py:
         ##CUSTOMFORMAT##
         # Essentially, if you alter anything to do with what custom formats
         # are, what their tags are, or anything along those lines, you should

=== modified file 'lib/lp/soyuz/scripts/custom_uploads_copier.py'
--- lib/lp/soyuz/scripts/custom_uploads_copier.py	2011-08-17 12:24:47 +0000
+++ lib/lp/soyuz/scripts/custom_uploads_copier.py	2012-05-30 10:30:27 +0000
@@ -1,4 +1,4 @@
-# Copyright 2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2011-2012 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Copy latest custom uploads into a distribution release series.
@@ -13,10 +13,11 @@
     ]
 
 from operator import attrgetter
-import re
 
 from zope.component import getUtility
 
+from lp.archivepublisher.debian_installer import DebianInstallerUpload
+from lp.archivepublisher.dist_upgrader import DistUpgraderUpload
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.services.database.bulk import load_referencing
 from lp.soyuz.enums import PackageUploadCustomFormat
@@ -30,10 +31,16 @@
 class CustomUploadsCopier:
     """Copy `PackageUploadCustom` objects into a new `DistroSeries`."""
 
-    copyable_types = [
-        PackageUploadCustomFormat.DEBIAN_INSTALLER,
-        PackageUploadCustomFormat.DIST_UPGRADER,
-        ]
+    # This is a marker as per the comment in lib/lp/soyuz/enums.py:
+    ##CUSTOMFORMAT##
+    # Essentially, if you alter anything to do with what custom formats are,
+    # what their tags are, or anything along those lines, you should grep
+    # for the marker in the source tree and fix it up in every place so
+    # marked.
+    copyable_types = {
+        PackageUploadCustomFormat.DEBIAN_INSTALLER: DebianInstallerUpload,
+        PackageUploadCustomFormat.DIST_UPGRADER: DistUpgraderUpload,
+        }
 
     def __init__(self, target_series):
         self.target_series = target_series
@@ -45,46 +52,16 @@
     def getCandidateUploads(self, source_series):
         """Find custom uploads that may need copying."""
         uploads = source_series.getPackageUploads(
-            custom_type=self.copyable_types)
+            custom_type=self.copyable_types.keys())
         load_referencing(PackageUploadCustom, uploads, ['packageuploadID'])
         customs = sum([list(upload.customfiles) for upload in uploads], [])
         customs = filter(self.isCopyable, customs)
         customs.sort(key=attrgetter('id'), reverse=True)
         return customs
 
-    def extractNameFields(self, filename):
-        """Get the relevant fields out of `filename`.
-
-        Scans filenames of any of these forms:
-
-            <package>_<version>_<architecture>.tar.<compression_suffix>
-            <package>_<version>.tar[.<compression_suffix>]
-
-        Versions may contain dots, dashes etc. but no underscores.
-
-        :return: A tuple of (<architecture>, version); or None if the
-            filename does not match the expected pattern.  If no
-            architecture is found in the filename, it defaults to 'all'.
-        """
-        # XXX JeroenVemreulen 2011-08-17, bug=827973: Push this down
-        # into the CustomUpload-derived classes, and share it with their
-        # constructors.
-        regex_parts = {
-            'package': "[^_]+",
-            'version': "[^_]+",
-            'arch': "[^._]+",
-        }
-        filename_regex = (
-            "%(package)s_(%(version)s)(?:_(%(arch)s))?.tar" % regex_parts)
-        match = re.match(filename_regex, filename)
-        if match is None:
-            return None
-        default_arch = 'all'
-        fields = match.groups(default_arch)
-        if len(fields) != 2:
-            return None
-        version, architecture = fields
-        return (architecture, version)
+    def extractSeriesKey(self, custom_type, filename):
+        """Get the relevant fields out of `filename` for `custom_type`."""
+        return custom_type.getSeriesKey(filename)
 
     def getKey(self, upload):
         """Get an indexing key for `upload`."""
@@ -92,12 +69,13 @@
         # translations tarballs, we'll have to include the component
         # name as well.
         custom_format = upload.customformat
-        name_fields = self.extractNameFields(upload.libraryfilealias.filename)
-        if name_fields is None:
+        series_key = self.extractSeriesKey(
+            self.copyable_types[custom_format],
+            upload.libraryfilealias.filename)
+        if series_key is None:
             return None
         else:
-            arch, version = name_fields
-            return (custom_format, arch)
+            return (custom_format, series_key)
 
     def getLatestUploads(self, source_series):
         """Find the latest uploads.

=== modified file 'lib/lp/soyuz/scripts/tests/test_custom_uploads_copier.py'
--- lib/lp/soyuz/scripts/tests/test_custom_uploads_copier.py	2012-01-01 02:58:52 +0000
+++ lib/lp/soyuz/scripts/tests/test_custom_uploads_copier.py	2012-05-30 10:30:27 +0000
@@ -1,4 +1,4 @@
-# Copyright 2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2011-2012 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Test copying of custom package uploads for a new `DistroSeries`."""
@@ -64,12 +64,8 @@
         # isCopyable checks a custom upload's customformat field to
         # determine whether the upload is a candidate for copying.  It
         # approves only those whose customformats are in copyable_types.
-        class FakePackageUploadCustom:
-            def __init__(self, customformat):
-                self.customformat = customformat
-
         uploads = [
-            FakePackageUploadCustom(custom_type)
+            FakeUpload(custom_type, None)
             for custom_type in PackageUploadCustomFormat.items]
 
         copier = CustomUploadsCopier(FakeDistroSeries())
@@ -78,48 +74,27 @@
             CustomUploadsCopier.copyable_types,
             [upload.customformat for upload in copied_uploads])
 
-    def test_extractNameFields_extracts_architecture_and_version(self):
-        # extractNameFields picks up the architecture and version out
-        # of an upload's filename field.
-        # XXX JeroenVermeulen 2011-08-17, bug=827941: For ddtp
-        # translations tarballs, we'll have to include the component
-        # name as well.
-        package_name = self.factory.getUniqueString('package')
-        version = self.makeVersion()
-        architecture = self.factory.getUniqueString('arch')
-        filename = '%s_%s_%s.tar.gz' % (package_name, version, architecture)
-        copier = CustomUploadsCopier(FakeDistroSeries())
-        self.assertEqual(
-            (architecture, version), copier.extractNameFields(filename))
-
-    def test_extractNameFields_does_not_require_architecture(self):
-        # When extractNameFields does not see an architecture, it
-        # defaults to 'all'.
-        package_name = self.factory.getUniqueString('package')
-        version = self.makeVersion()
-        filename = '%s_%s.tar.gz' % (package_name, version)
-        copier = CustomUploadsCopier(FakeDistroSeries())
-        self.assertEqual(
-            ('all', version), copier.extractNameFields(filename))
-
-    def test_extractNameFields_returns_None_on_mismatch(self):
-        # If the filename does not match the expected pattern,
-        # extractNameFields returns None.
-        copier = CustomUploadsCopier(FakeDistroSeries())
-        self.assertIs(None, copier.extractNameFields('argh_1.0.jpg'))
-
-    def test_extractNameFields_ignores_names_with_too_many_fields(self):
-        # As one particularly nasty case that might break
-        # extractNameFields, a name with more underscore-seprated fields
-        # than the search pattern allows for is sensibly rejected.
-        copier = CustomUploadsCopier(FakeDistroSeries())
-        self.assertIs(
-            None, copier.extractNameFields('one_two_three_four_5.tar.gz'))
+    def test_getKey_calls_correct_custom_upload_method(self):
+        # getKey calls the getSeriesKey method on the correct custom upload.
+        class FakeCustomUpload:
+            @classmethod
+            def getSeriesKey(cls, tarfile_path):
+                return "dummy"
+
+        copier = CustomUploadsCopier(FakeDistroSeries())
+        copier.copyable_types = {
+            PackageUploadCustomFormat.DEBIAN_INSTALLER: FakeCustomUpload,
+            }
+        custom_format, series_key = copier.getKey(
+            FakeUpload(PackageUploadCustomFormat.DEBIAN_INSTALLER, "anything"))
+        self.assertEqual(
+            PackageUploadCustomFormat.DEBIAN_INSTALLER, custom_format)
+        self.assertEqual("dummy", series_key)
 
     def test_getKey_returns_None_on_name_mismatch(self):
-        # If extractNameFields returns None, getKey also returns None.
+        # If extractSeriesKey returns None, getKey also returns None.
         copier = CustomUploadsCopier(FakeDistroSeries())
-        copier.extractNameFields = FakeMethod()
+        copier.extractSeriesKey = FakeMethod()
         self.assertIs(
             None,
             copier.getKey(FakeUpload(
@@ -142,8 +117,9 @@
         package_name = self.factory.getUniqueString("package")
         if version is None:
             version = self.makeVersion()
-        filename = "%s.tar.gz" % '_'.join(
-            filter(None, [package_name, version, arch]))
+        if arch is None:
+            arch = self.factory.getUniqueString()
+        filename = "%s.tar.gz" % '_'.join([package_name, version, arch])
         package_upload = self.factory.makeCustomPackageUpload(
             distroseries=distroseries, custom_type=custom_type,
             filename=filename)

=== modified file 'lib/lp/soyuz/tests/test_distroseriesqueue_debian_installer.py'
--- lib/lp/soyuz/tests/test_distroseriesqueue_debian_installer.py	2012-05-25 13:26:08 +0000
+++ lib/lp/soyuz/tests/test_distroseriesqueue_debian_installer.py	2012-05-30 10:30:27 +0000
@@ -11,7 +11,7 @@
 
 import transaction
 
-from lp.archivepublisher.debian_installer import DebianInstallerAlreadyExists
+from lp.archivepublisher.customupload import CustomUploadAlreadyExists
 from lp.archiveuploader.nascentupload import NascentUpload
 from lp.archiveuploader.tests import (
     datadir,
@@ -69,6 +69,6 @@
             "main", "installer-i386", "20070214ubuntu1"))
         self.assertFalse(upload.queue_root.realiseUpload(self.logger))
         self.assertRaises(
-            DebianInstallerAlreadyExists,
+            CustomUploadAlreadyExists,
             upload.queue_root.customfiles[0].publish, self.logger)
         self.assertEqual("ACCEPTED", upload.queue_root.status.name)