launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #27812
[Merge] ~cjwatson/launchpad:project-download-queries into launchpad:master
Colin Watson has proposed merging ~cjwatson/launchpad:project-download-queries into launchpad:master.
Commit message:
Preload more librarian references on Product:+download
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #1952958 in Launchpad itself: "Timeout error on Product:+download"
https://bugs.launchpad.net/launchpad/+bug/1952958
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/412706
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.
--
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:project-download-queries into launchpad:master.
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):
