Merge ~cjwatson/launchpad:project-download-queries into launchpad:master

[Colin Watson]

Preload more librarian references on Product:+download

The Product:+download page needs `ProductReleaseFile.signature` and `ProductReleaseFile.libraryfile.last_downloaded` for each file, and so needs to preload these to avoid excessive query counts. The former is easy; the latter requires some minor PostgreSQL gymnastics.

I took the opportunity to convert the corresponding tests away from doctests first.

[Ioana Lasc (ilasc)]

LGTM

review: Approve

diff --git a/lib/lp/registry/browser/product.py b/lib/lp/registry/browser/product.py
index 942b4b9..3f58926 100644
--- a/lib/lp/registry/browser/product.py
+++ b/lib/lp/registry/browser/product.py
@@ -208,6 +208,7 @@ from lp.services.fields import (
     PublicPersonChoice,
     URIField,
     )
+from lp.services.librarian.interfaces import ILibraryFileAliasSet
 from lp.services.propertycache import cachedproperty
 from lp.services.webapp import (
     ApplicationMenu,
@@ -848,8 +849,10 @@ class ReleaseWithFiles:
             # returns all releases sorted properly.
             product = self.parent.parent
             release_delegates = product.release_by_id.values()
-            files = getUtility(IProductReleaseSet).getFilesForReleases(
-                release_delegates)
+            files = list(getUtility(IProductReleaseSet).getFilesForReleases(
+                release_delegates))
+            getUtility(ILibraryFileAliasSet).preloadLastDownloaded(
+                {file.libraryfile for file in files})
             for release_delegate in release_delegates:
                 release_delegate._files = []
             for file in files:
@@ -1271,8 +1274,6 @@ class ProductDownloadFilesView(LaunchpadView,
                                ProductDownloadFileMixin):
     """View class for the product's file downloads page."""
 
-    batch_size = config.launchpad.download_batch_size
-
     @property
     def page_title(self):
         return "%s project files" % self.context.displayname
@@ -1304,7 +1305,7 @@ class ProductDownloadFilesView(LaunchpadView,
                     if pair not in series_and_releases:
                         series_and_releases.append(pair)
         batch = BatchNavigator(series_and_releases, self.request,
-                               size=self.batch_size)
+                               size=config.launchpad.download_batch_size)
         batch.setHeadings("release", "releases")
         return batch
 
diff --git a/lib/lp/registry/browser/tests/product-files-views.txt b/lib/lp/registry/browser/tests/product-files-views.txt
deleted file mode 100644
index c999548..0000000
--- a/lib/lp/registry/browser/tests/product-files-views.txt
+++ /dev/null
@@ -1,93 +0,0 @@
- Product Download Files Page
-=============================
-
-Test for the product/+download page.
-
-    >>> product = factory.makeProduct(name='alfajore')
-    >>> productseries = factory.makeProductSeries(
-    ...     product=product, name="sammy")
-    >>> milestone = factory.makeMilestone(productseries=productseries,
-    ...                                   name="apple")
-    >>> release_file = factory.makeProductReleaseFile(
-    ...     product=product, productseries=productseries, milestone=milestone)
-    >>> view = create_initialized_view(product, '+download')
-
-    >>> view.batch_size
-    4
-
-    >>> batch = view.series_and_releases_batch.currentBatch()
-    >>> print(len(list(batch)))
-    1
-
-    >>> def print_series_release(sr):
-    ...     print("%s from the %s series" % (sr.release.name_with_codename,
-    ...                                      sr.series.name))
-
-    >>> for sr in batch:
-    ...     print_series_release(sr)
-    apple from the sammy series
-
-    >>> product = factory.makeProduct(name='bombilla')
-    >>> for i in range(1,5):
-    ...     productseries = factory.makeProductSeries(
-    ...         product=product, name="s%d"%i)
-    ...     for j in range(1,4):
-    ...         milestone = factory.makeMilestone(productseries=productseries,
-    ...                                           name="%d.%d"%(i,j))
-    ...         release_file = factory.makeProductReleaseFile(
-    ...             product=product, productseries=productseries,
-    ...             milestone=milestone)
-    >>> view = create_initialized_view(product, '+download')
-    >>> batch = view.series_and_releases_batch.currentBatch()
-    >>> print(len(batch))
-    4
-    >>> for sr in batch:
-    ...     print_series_release(sr)
-    4.3 from the s4 series
-    4.2 from the s4 series
-    4.1 from the s4 series
-    3.3 from the s3 series
-
-    >>> batch = batch.nextBatch()
-    >>> for sr in batch:
-    ...     print_series_release(sr)
-    3.2 from the s3 series
-    3.1 from the s3 series
-    2.3 from the s2 series
-    2.2 from the s2 series
-
-For an administrator of the project, at the bottom of each batched
-page will be links to add new files for each series and release.
-
-    >>> from lp.testing.pages import (
-    ...     extract_text, find_tag_by_id)
-    >>> ignored = login_person(product.owner)
-    >>> view = create_initialized_view(product, '+download',
-    ...                                principal=product.owner)
-    >>> admin_links = find_tag_by_id(view.render(), 'admin-links')
-    >>> content = extract_text(admin_links)
-    >>> print(content)
-    Add download file to the s4 series for release: 4.3, 4.2, 4.1
-    Add download file to the s3 series for release: 3.3, 3.2, 3.1
-    Add download file to the s2 series for release: 2.3, 2.2, 2.1
-    Add download file to the s1 series for release: 1.3, 1.2, 1.1
-
-
-Product index
--------------
-
-The product index view shows the latest release for the project.
-
-    >>> view = create_initialized_view(product, name='+index')
-    >>> print(view.latest_release_with_download_files.version)
-    4.3
-
-Obsolete series are ignored.
-
-    >>> from lp.registry.interfaces.series import SeriesStatus
-
-    >>> obsolete_series = product.getSeries('s4')
-    >>> obsolete_series.status = SeriesStatus.OBSOLETE
-    >>> view = create_initialized_view(product, name='+index')
-    >>> print(view.latest_release_with_download_files.version)
-    3.3
diff --git a/lib/lp/registry/browser/tests/test_product.py b/lib/lp/registry/browser/tests/test_product.py
index 3e71422..1f571fe 100644
--- a/lib/lp/registry/browser/tests/test_product.py
+++ b/lib/lp/registry/browser/tests/test_product.py
@@ -1,4 +1,4 @@
-# Copyright 2010-2020 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2021 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for product views."""
@@ -6,6 +6,7 @@
 __all__ = ['make_product_form']
 
 import re
+from textwrap import dedent
 
 from lazr.restful.fields import Reference
 from lazr.restful.interfaces import (
@@ -55,6 +56,7 @@ from lp.registry.interfaces.product import (
     License,
     )
 from lp.registry.interfaces.productseries import IProductSeries
+from lp.registry.interfaces.series import SeriesStatus
 from lp.registry.model.product import Product
 from lp.services.config import config
 from lp.services.database.interfaces import IStore
@@ -69,6 +71,7 @@ from lp.testing import (
     login_celebrity,
     login_person,
     person_logged_in,
+    record_two_runs,
     StormStatementRecorder,
     TestCaseWithFactory,
     )
@@ -835,6 +838,89 @@ class TestProductEditView(BrowserTestCase):
             InformationType.PUBLIC, updated_product.information_type)
 
 
+class TestProductDownloadFilesView(TestCaseWithFactory):
+    """Test `ProductDownloadFilesView`."""
+
+    layer = LaunchpadFunctionalLayer
+
+    def makeProductAndReleases(self):
+        product = self.factory.makeProduct()
+        for i in range(1, 5):
+            productseries = self.factory.makeProductSeries(
+                product=product, name="s%d" % i)
+            for j in range(1, 4):
+                milestone = self.factory.makeMilestone(
+                    productseries=productseries, name="%d.%d" % (i, j))
+                self.factory.makeProductReleaseFile(
+                    product=product, productseries=productseries,
+                    milestone=milestone)
+        return product
+
+    def test_series_and_releases_batch(self):
+        self.pushConfig("launchpad", download_batch_size=4)
+        product = self.makeProductAndReleases()
+        view = create_initialized_view(product, "+download")
+        batch = view.series_and_releases_batch.currentBatch()
+        self.assertEqual(
+            [("4.3", "s4"), ("4.2", "s4"), ("4.1", "s4"), ("3.3", "s3")],
+            [(sr.release.name_with_codename, sr.series.name) for sr in batch])
+        batch = batch.nextBatch()
+        self.assertEqual(
+            [("3.2", "s3"), ("3.1", "s3"), ("2.3", "s2"), ("2.2", "s2")],
+            [(sr.release.name_with_codename, sr.series.name) for sr in batch])
+
+    def test_query_count(self):
+        self.pushConfig("launchpad", download_batch_size=20)
+        product = self.factory.makeProduct()
+
+        def create_series_and_releases():
+            productseries = self.factory.makeProductSeries(product=product)
+            for _ in range(3):
+                self.factory.makeProductReleaseFile(
+                    product=product, productseries=productseries)
+
+        def render_product():
+            create_initialized_view(product, "+download").render()
+
+        recorder1, recorder2 = record_two_runs(
+            render_product, create_series_and_releases, 2)
+        self.assertThat(recorder2, HasQueryCount.byEquality(recorder1))
+
+    def test_add_download_file_links(self):
+        # Project administrators have links at the bottom of each batched
+        # page to add new files for each series and release.
+        product = self.makeProductAndReleases()
+        login_person(product.owner)
+        view = create_initialized_view(
+            product, "+download", principal=product.owner)
+        admin_links = find_tag_by_id(view.render(), "admin-links")
+        expected_links = dedent("""
+            Add download file to the s4 series for release: 4.3, 4.2, 4.1
+            Add download file to the s3 series for release: 3.3, 3.2, 3.1
+            Add download file to the s2 series for release: 2.3, 2.2, 2.1
+            Add download file to the s1 series for release: 1.3, 1.2, 1.1
+            """)
+        self.assertTextMatchesExpressionIgnoreWhitespace(
+            expected_links, extract_text(admin_links))
+
+    def test_latest_release_on_index(self):
+        # The project index view shows the latest release for the project.
+        product = self.makeProductAndReleases()
+        view = create_initialized_view(product, "+index")
+        self.assertEqual(
+            "4.3", view.latest_release_with_download_files.version)
+
+    def test_latest_release_ignores_obsolete_series(self):
+        # The project index view ignores obsolete series for the purpose of
+        # showing the latest release.
+        product = self.makeProductAndReleases()
+        with person_logged_in(product.owner):
+            product.getSeries("s4").status = SeriesStatus.OBSOLETE
+        view = create_initialized_view(product, "+index")
+        self.assertEqual(
+            "3.3", view.latest_release_with_download_files.version)
+
+
 class ProductSetReviewLicensesViewTestCase(TestCaseWithFactory):
     """Tests the ProductSetReviewLicensesView."""
 
diff --git a/lib/lp/registry/browser/tests/test_views.py b/lib/lp/registry/browser/tests/test_views.py
index 02ac47f..35254f4 100644
--- a/lib/lp/registry/browser/tests/test_views.py
+++ b/lib/lp/registry/browser/tests/test_views.py
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2021 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """
@@ -34,7 +34,6 @@ special_test_layer = {
     'milestone-views.txt': LaunchpadFunctionalLayer,
     'person-views.txt': LaunchpadFunctionalLayer,
     'product-edit-people-view.txt': LaunchpadFunctionalLayer,
-    'product-files-views.txt': LaunchpadFunctionalLayer,
     'product-views.txt': LaunchpadFunctionalLayer,
     'productseries-views.txt': LaunchpadFunctionalLayer,
     'projectgroup-views.txt': LaunchpadFunctionalLayer,
diff --git a/lib/lp/registry/model/productrelease.py b/lib/lp/registry/model/productrelease.py
index 3b88d33..00c7914 100644
--- a/lib/lp/registry/model/productrelease.py
+++ b/lib/lp/registry/model/productrelease.py
@@ -1,4 +1,4 @@
-# Copyright 2009-2013 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2021 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __all__ = [
@@ -281,9 +281,14 @@ class ProductReleaseSet(object):
             return EmptyResultSet()
         return ProductReleaseFile.select(
             """ProductReleaseFile.productrelease IN %s""" % (
-            sqlvalues([release.id for release in releases])),
+                sqlvalues([release.id for release in releases])),
             orderBy='-date_uploaded',
-            prejoins=['libraryfile', 'libraryfile.content', 'productrelease'])
+            prejoins=[
+                'libraryfile',
+                'libraryfile.content',
+                'productrelease',
+                'signature',
+                ])
 
 
 def productrelease_to_milestone(productrelease):
diff --git a/lib/lp/services/librarian/interfaces/__init__.py b/lib/lp/services/librarian/interfaces/__init__.py
index 6b3646e..73b390b 100644
--- a/lib/lp/services/librarian/interfaces/__init__.py
+++ b/lib/lp/services/librarian/interfaces/__init__.py
@@ -1,4 +1,4 @@
-# Copyright 2009-2016 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2021 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Librarian interfaces."""
@@ -175,6 +175,9 @@ class ILibraryFileAliasSet(Interface):
         given sha256.
         """
 
+    def preloadLastDownloaded(self, lfas):
+        """Preload last_downloaded for a collection of `LibraryFileAlias`es."""
+
 
 class ILibraryFileDownloadCount(Interface):
     """Download count of a given file in a given day."""
diff --git a/lib/lp/services/librarian/model.py b/lib/lp/services/librarian/model.py
index 3e730eb..2c513d3 100644
--- a/lib/lp/services/librarian/model.py
+++ b/lib/lp/services/librarian/model.py
@@ -1,4 +1,4 @@
-# Copyright 2009-2016 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2021 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __all__ = [
@@ -40,7 +40,10 @@ from lp.services.database.constants import (
     UTC_NOW,
     )
 from lp.services.database.datetimecol import UtcDateTimeCol
-from lp.services.database.interfaces import IMasterStore
+from lp.services.database.interfaces import (
+    IMasterStore,
+    IStore,
+    )
 from lp.services.database.sqlbase import (
     session_store,
     SQLBase,
@@ -66,6 +69,10 @@ from lp.services.librarian.interfaces.client import (
     IRestrictedLibrarianClient,
     LIBRARIAN_SERVER_DEFAULT_TIMEOUT,
     )
+from lp.services.propertycache import (
+    cachedproperty,
+    get_property_cache,
+    )
 from lp.services.tokens import create_token
 
 
@@ -180,7 +187,7 @@ class LibraryFileAlias(SQLBase):
             self._datafile.close()
             self._datafile = None
 
-    @property
+    @cachedproperty
     def last_downloaded(self):
         """See `ILibraryFileAlias`."""
         store = Store.of(self)
@@ -271,6 +278,33 @@ class LibraryFileAliasSet(object):
             AND LibraryFileContent.sha256 = '%s'
             """ % sha256, clauseTables=['LibraryFileContent'])
 
+    def preloadLastDownloaded(self, lfas):
+        """See `ILibraryFileAliasSet`."""
+        store = IStore(LibraryFileAlias)
+        results = store.find(
+            (LibraryFileDownloadCount.libraryfilealias_id,
+             LibraryFileDownloadCount.day),
+            LibraryFileDownloadCount.libraryfilealias_id.is_in(
+                sorted(lfa.id for lfa in lfas)))
+        results.order_by(
+            # libraryfilealias doesn't need to be descending for
+            # correctness, but this allows the index on
+            # LibraryFileDownloadCount (libraryfilealias, day, country) to
+            # satisfy this query efficiently.
+            Desc(LibraryFileDownloadCount.libraryfilealias_id),
+            Desc(LibraryFileDownloadCount.day))
+        # Request the first row for each LFA, which corresponds to the most
+        # recent day due to the above ordering.
+        results.config(
+            distinct=(LibraryFileDownloadCount.libraryfilealias_id,))
+        now = datetime.now(pytz.utc).date()
+        lfas_by_id = {lfa.id: lfa for lfa in lfas}
+        for lfa_id, day in results:
+            get_property_cache(lfas_by_id[lfa_id]).last_downloaded = now - day
+            del lfas_by_id[lfa_id]
+        for lfa in lfas_by_id.values():
+            get_property_cache(lfa).last_downloaded = None
+
 
 @implementer(ILibraryFileDownloadCount)
 class LibraryFileDownloadCount(SQLBase):