Merge lp:~michael.nelson/launchpad/432152-orig-file-counts into lp:launchpad

[Michael Nelson]

[Michael Nelson (michael.nelson)]

= Summary =

This is a fix for bug 432152, where tarball uploads to a PPA for
different distroseries was resulting in the tarball size being counted
once for each upload - even though we (can) only store one copy of the
tarball in the pool (for eg. the tarball
freeswitch_1.0.4+repack3.orig.tar.gz at
http://ppa.launchpad.net/pbxbuntu-drivers/ppa/ubuntu/pool/main/f/freeswitch/
is used for all the distroseries builds.)

== Proposed fix ==

== Pre-implementation notes ==

I had a pre-imp. with Celso who recommended that we not put the
responsibility of checking for previous uploads of the same librarian
file in the DSCFile, but rather, just updating the sources_size property
on the archive, to ensure that it only counts unique filenames in the pool.

== Implementation details ==

== Tests ==

bin/test -vv -t TestArchiveRepositorySize -t doc/archive.txt -t
archive-views.txt

== Demo and Q/A ==

No demo. Q/A we can do on dogfood, or check on production when William
uploads his next update.

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/lp/soyuz/tests/test_archive.py
  lib/lp/soyuz/model/archive.py

== Pylint notices ==

lib/lp/soyuz/model/archive.py
    14: [F0401] Unable to import 'lazr.lifecycle.event' (No module named
lifecycle)

--
Michael

[Gavin Panella (allenap)]

As you suggested on IRC, it's a good idea to use the sha1 in the query, if only to make it clear that the query will DTRT, and so that future maintainers of this code don't wonder what's happening. It seems that a good understanding of the data model is required to appreciate why the query is correct as it stands.

One other thing. I think you can safely avoid the extra query for the result.count() by using result.values(...):

{{{
--- lib/lp/soyuz/model/archive.py 2009-09-30 09:10:16 +0000
+++ lib/lp/soyuz/model/archive.py 2009-09-30 09:52:30 +0000
@@ -468,10 +468,7 @@
         result = result.config(distinct=True)

         # Storm's result.sum() throws errors when the result is empty.
- if result.count() == 0:
- return 0
- else:
- return result.sum(LibraryFileContent.filesize)
+ return sum(result.values(LibraryFileContent.filesize))

     def _getBinaryPublishingBaseClauses (
         self, name=None, version=None, status=None, distroarchseries=None,
}}}

I doubt this will be measurably slower than using result.sum(), but it should be quicker than running the query twice. Also, do you know if there's a bug filed for this issue in Storm, or is it reasonable behaviour?

Thanks, Gavin.

review: Approve

=== modified file 'lib/lp/soyuz/model/archive.py'
--- lib/lp/soyuz/model/archive.py	2009-09-23 07:41:13 +0000
+++ lib/lp/soyuz/model/archive.py	2009-09-30 15:06:17 +0000
@@ -444,8 +444,10 @@
     def sources_size(self):
         """See `IArchive`."""
         store = getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR)
-        result = store.find(
-            (LibraryFileContent),
+        result = store.find((
+            LibraryFileAlias.filename,
+            LibraryFileContent.sha1,
+            LibraryFileContent.filesize,),
             SourcePackagePublishingHistory.archive == self.id,
             SourcePackagePublishingHistory.dateremoved == None,
             SourcePackagePublishingHistory.sourcepackagereleaseID ==
@@ -453,15 +455,22 @@
             SourcePackageReleaseFile.libraryfileID == LibraryFileAlias.id,
             LibraryFileAlias.contentID == LibraryFileContent.id)
 
-        # We need to select distinct `LibraryFileContent`s because that how
-        # they end up published in the archive disk. Duplications may happen
-        # because of the publishing records join, the same `LibraryFileAlias`
-        # gets logically re-published in several locations and the fact that
-        # the same `LibraryFileContent` can be shared by multiple
-        # `LibraryFileAlias.` (librarian-gc).
+        # Note: we can't use the LFC.sha1 instead of LFA.filename above
+        # because some archives publish the same file content with different
+        # names in the archive, so although the duplication will be removed
+        # in the librarian by the librarian-gc, we do not (yet) remove
+        # this duplication in the pool when the filenames are different.
+
+        # We need to select distinct on the (filename, filesize) result
+        # so that we only count duplicate files (with the same filename)
+        # once (ie. the same tarball used for different distroseries) as
+        # we do avoid this duplication in the pool when the names are
+        # the same.
         result = result.config(distinct=True)
-        size = sum([lfc.filesize for lfc in result])
-        return size
+
+        # Using result.sum(LibraryFileContent.filesize) throws errors when
+        # the result is empty, so instead:
+        return sum(result.values(LibraryFileContent.filesize))
 
     def _getBinaryPublishingBaseClauses (
         self, name=None, version=None, status=None, distroarchseries=None,

=== modified file 'lib/lp/soyuz/tests/test_archive.py'
--- lib/lp/soyuz/tests/test_archive.py	2009-09-17 07:12:09 +0000
+++ lib/lp/soyuz/tests/test_archive.py	2009-09-30 15:06:17 +0000
@@ -168,6 +168,43 @@
             previous_size + 1,
             self.publisher.ubuntutest.main_archive.binaries_size)
 
+    def test_sources_size_on_empty_archive(self):
+        # Zero is returned for an archive without sources.
+        self.assertEquals(
+            0, self.ppa.sources_size,
+            'Zero should be returned for an archive without sources.')
+
+    def test_sources_size_does_not_count_duplicated_files(self):
+        # If there are multiple copies of the same file name/size
+        # only one will be counted.
+        pub_1 = self.publisher.getPubSource(
+            filecontent='22', version='0.5.11~ppa1', archive=self.ppa)
+
+        pub_2 = self.publisher.getPubSource(
+            filecontent='333', version='0.5.11~ppa2', archive=self.ppa)
+
+        self.assertEquals(5, self.ppa.sources_size)
+
+        shared_tarball = self.publisher.addMockFile(
+            filename='foo_0.5.11.tar.gz', filecontent='1')
+
+        # After adding a the shared tarball to the ppa1 version,
+        # the sources_size updates to reflect the change.
+        pub_1.sourcepackagerelease.addFile(shared_tarball)
+        self.assertEquals(
+            6, self.ppa.sources_size,
+            'The sources_size should update after a file is added.')
+
+        # But after adding a copy of the shared tarball to the ppa2 version,
+        # the sources_size is unchanged.
+        shared_tarball_copy = self.publisher.addMockFile(
+            filename='foo_0.5.11.tar.gz', filecontent='1')
+
+        pub_2.sourcepackagerelease.addFile(shared_tarball_copy)
+        self.assertEquals(
+            6, self.ppa.sources_size,
+            'The sources_size should change after adding a duplicate file.')
+
 
 class TestSeriesWithSources(TestCaseWithFactory):
     """Create some sources in different series."""