launchpad-reviewers team mailing list archive

[Colin Watson <mp+417963@xxxxxxxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:diskpool-refactor-addFile into launchpad:master.

Commit message:
Refactor DiskPool.addFile to take an IPackageReleaseFile

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

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

For publishing to Artifactory, `addFile` will need a little more context than it currently has: as well as the file contents and hash, it'll need a reference to the originating `SourcePackageRelease` or `BinaryPackageRelease` in order to be able to calculate initial properties for the new artifact.

In support of this, refactor `ISourcePackageReleaseFile` and `IBinaryPackageFile` to inherit from a common `IPackageReleaseFile` interface, and refactor `DiskPool.addFile` to take an object providing that interface rather than taking the file contents and hash separately.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:diskpool-refactor-addFile into launchpad:master.

diff --git a/lib/lp/archivepublisher/diskpool.py b/lib/lp/archivepublisher/diskpool.py
index 528da2a..064d171 100644
--- a/lib/lp/archivepublisher/diskpool.py
+++ b/lib/lp/archivepublisher/diskpool.py
@@ -12,6 +12,7 @@ from lp.services.librarian.utils import (
     sha1_from_path,
     )
 from lp.services.propertycache import cachedproperty
+from lp.soyuz.interfaces.files import IPackageReleaseFile
 from lp.soyuz.interfaces.publishing import (
     MissingSymlinkInPool,
     NotInPool,
@@ -181,16 +182,18 @@ class DiskPoolEntry:
         targetpath = self.pathFor(self.file_component)
         return sha1_from_path(targetpath)
 
-    def addFile(self, component, sha1, contents):
+    def addFile(self, component, pub_file: IPackageReleaseFile):
         """See DiskPool.addFile."""
         assert component in HARDCODED_COMPONENT_ORDER
 
         targetpath = self.pathFor(component)
         if not os.path.exists(os.path.dirname(targetpath)):
             os.makedirs(os.path.dirname(targetpath))
+        lfa = pub_file.libraryfile
 
         if self.file_component:
             # There's something on disk. Check hash.
+            sha1 = lfa.content.sha1
             if sha1 != self.file_hash:
                 raise PoolFileOverwriteError('%s != %s for %s' %
                     (sha1, self.file_hash,
@@ -219,8 +222,8 @@ class DiskPoolEntry:
 
         file_to_write = _diskpool_atomicfile(
             targetpath, "wb", rootpath=self.temppath)
-        contents.open()
-        copy_and_close(contents, file_to_write)
+        lfa.open()
+        copy_and_close(lfa, file_to_write)
         self.file_component = component
         return FileAddActionEnum.FILE_ADDED
 
@@ -407,17 +410,16 @@ class DiskPool:
             return os.path.join(path, file)
         return path
 
-    def addFile(self, component, sourcename, filename, sha1, contents):
+    def addFile(self, component, sourcename, filename,
+                pub_file: IPackageReleaseFile):
         """Add a file with the given contents to the pool.
 
         Component, sourcename and filename are used to calculate the
         on-disk location.
 
-        sha1 is used to compare with the existing file's checksum, if
-        a file already exists for any component.
-
-        contents is a file-like object containing the contents we want
-        to write.
+        pub_file is an `IPackageReleaseFile` providing the file's contents
+        and SHA-1 hash.  The SHA-1 hash is used to compare the given file
+        with an existing file, if one exists for any component.
 
         There are four possible outcomes:
         - If the file doesn't exist in the pool for any component, it will
@@ -425,22 +427,22 @@ class DiskPool:
         returned.
 
         - If the file already exists in the pool, in this or any other
-        component, the checksum of the file on disk will be calculated and
-        compared with the checksum provided. If they fail to match,
+        component, the hash of the file on disk will be calculated and
+        compared with the hash provided. If they fail to match,
         PoolFileOverwriteError will be raised.
 
         - If the file already exists but not in this component, and the
-        checksum test above passes, a symlink will be added, and
+        hash test above passes, a symlink will be added, and
         results.SYMLINK_ADDED will be returned. Also, the symlinks will be
         checked and sanitised, to ensure the real copy of the file is in the
         most preferred component, according to HARDCODED_COMPONENT_ORDER.
 
         - If the file already exists and is already in this component,
-        either as a file or a symlink, and the checksum check passes,
+        either as a file or a symlink, and the hash check passes,
         results.NONE will be returned and nothing will be done.
         """
         entry = self._getEntry(sourcename, filename)
-        return entry.addFile(component, sha1, contents)
+        return entry.addFile(component, pub_file)
 
     def removeFile(self, component, sourcename, filename):
         """Remove the specified file from the pool.
diff --git a/lib/lp/archivepublisher/tests/test_pool.py b/lib/lp/archivepublisher/tests/test_pool.py
index 1387697..e6bc01c 100644
--- a/lib/lp/archivepublisher/tests/test_pool.py
+++ b/lib/lp/archivepublisher/tests/test_pool.py
@@ -16,9 +16,16 @@ from lp.archivepublisher.diskpool import (
 from lp.services.log.logger import BufferLogger
 
 
-class MockFile:
+class FakeLibraryFileContent:
 
     def __init__(self, contents):
+        self.sha1 = hashlib.sha1(contents).hexdigest()
+
+
+class FakeLibraryFileAlias:
+
+    def __init__(self, contents):
+        self.content = FakeLibraryFileContent(contents)
         self.contents = contents
 
     def open(self):
@@ -34,6 +41,12 @@ class MockFile:
         pass
 
 
+class FakePackageReleaseFile:
+
+    def __init__(self, contents):
+        self.libraryfile = FakeLibraryFileAlias(contents)
+
+
 class PoolTestingFile:
 
     def __init__(self, pool, sourcename, filename):
@@ -45,7 +58,7 @@ class PoolTestingFile:
     def addToPool(self, component):
         return self.pool.addFile(
             component, self.sourcename, self.filename,
-            hashlib.sha1(self.contents).hexdigest(), MockFile(self.contents))
+            FakePackageReleaseFile(self.contents))
 
     def removeFromPool(self, component):
         return self.pool.removeFile(component, self.sourcename, self.filename)
diff --git a/lib/lp/soyuz/interfaces/files.py b/lib/lp/soyuz/interfaces/files.py
index 755150f..c7bc8d3 100644
--- a/lib/lp/soyuz/interfaces/files.py
+++ b/lib/lp/soyuz/interfaces/files.py
@@ -5,6 +5,7 @@
 
 __all__ = [
     'IBinaryPackageFile',
+    'IPackageReleaseFile',
     'ISourcePackageReleaseFile',
     ]
 
@@ -20,37 +21,35 @@ from lp.services.librarian.interfaces import ILibraryFileAlias
 from lp.soyuz.interfaces.sourcepackagerelease import ISourcePackageRelease
 
 
-class IBinaryPackageFile(Interface):
-    """A binary package to librarian link record."""
+class IPackageReleaseFile(Interface):
+    """A link between a source/binary package release and the librarian."""
 
-    id = Int(
-            title=_('ID'), required=True, readonly=True,
-            )
-    binarypackagerelease = Int(
-            title=_('The binarypackagerelease being published'),
-            required=True, readonly=False,
-            )
+    id = Int(title=_('ID'), required=True, readonly=True)
 
     libraryfileID = Int(
-            title=_("The LibraryFileAlias id for this .deb"), required=True,
-            readonly=True)
+        title=_("The LibraryFileAlias id for this file"), required=True,
+        readonly=True)
 
     libraryfile = Reference(
-        ILibraryFileAlias, title=_("The library file alias for this .deb"),
+        ILibraryFileAlias, title=_("The library file alias for this file"),
         required=True, readonly=False)
 
     filetype = Int(
-            title=_('The type of this file'), required=True, readonly=False,
-            )
+        title=_('The type of this file'), required=True, readonly=False)
 
 
-class ISourcePackageReleaseFile(Interface):
-    """A source package release to librarian link record."""
+class IBinaryPackageFile(IPackageReleaseFile):
+    """A binary package to librarian link record."""
 
-    id = Int(
-            title=_('ID'), required=True, readonly=True,
+    binarypackagerelease = Int(
+            title=_('The binarypackagerelease being published'),
+            required=True, readonly=False,
             )
 
+
+class ISourcePackageReleaseFile(IPackageReleaseFile):
+    """A source package release to librarian link record."""
+
     sourcepackagerelease = Reference(
         ISourcePackageRelease,
         title=_("The source package release being published"),
@@ -61,15 +60,6 @@ class ISourcePackageReleaseFile(Interface):
             required=True,
             readonly=False)
 
-    libraryfile = Int(
-            title=_('The library file alias for this file'), required=True,
-            readonly=False,
-            )
-
-    filetype = Int(
-            title=_('The type of this file'), required=True, readonly=False,
-            )
-
     is_orig = Bool(
             title=_('Whether this file is an original tarball'),
             required=True, readonly=False,
diff --git a/lib/lp/soyuz/model/publishing.py b/lib/lp/soyuz/model/publishing.py
index ea0b012..810475d 100644
--- a/lib/lp/soyuz/model/publishing.py
+++ b/lib/lp/soyuz/model/publishing.py
@@ -174,12 +174,10 @@ class ArchivePublisherBase:
                 source = self.source_package_name
                 component = self.component.name
                 filename = pub_file.libraryfile.filename
-                filealias = pub_file.libraryfile
-                sha1 = filealias.content.sha1
                 path = diskpool.pathFor(component, source, filename)
 
                 action = diskpool.addFile(
-                    component, source, filename, sha1, filealias)
+                    component, source, filename, pub_file)
                 if action == diskpool.results.FILE_ADDED:
                     log.debug("Added %s from library" % path)
                 elif action == diskpool.results.SYMLINK_ADDED: