launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #02792
[Merge] lp:~lifeless/launchpad/bug-722956 into lp:launchpad
Robert Collins has proposed merging lp:~lifeless/launchpad/bug-722956 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
#722956 BranchSet:CollectionResource#branches timeouts
https://bugs.launchpad.net/bugs/722956
For more details, see:
https://code.launchpad.net/~lifeless/launchpad/bug-722956/+merge/51481
The API for /branches is nearly guaranteed to have branches from many different contexts, and we were doing late evaluation on many attributes trying to render the collection. This fixes 2 of those cases and lays the ground work to fix more when the page becomes an issue again.
--
https://code.launchpad.net/~lifeless/launchpad/bug-722956/+merge/51481
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~lifeless/launchpad/bug-722956 into lp:launchpad.
=== modified file 'lib/lp/app/widgets/suggestion.py'
--- lib/lp/app/widgets/suggestion.py 2011-02-08 15:43:30 +0000
+++ lib/lp/app/widgets/suggestion.py 2011-02-28 00:51:16 +0000
@@ -231,7 +231,9 @@
collection = branch.target.collection.targetedBy(logged_in_user,
since)
collection = collection.visibleByUser(logged_in_user)
- branches = collection.getBranches().config(distinct=True)
+ # May actually need some eager loading, but the API isn't fine grained
+ # yet.
+ branches = collection.getBranches(eager_load=False).config(distinct=True)
target_branches = list(branches.config(limit=5))
# If there is a development focus branch, make sure it is always
# shown, and as the first item.
=== modified file 'lib/lp/code/browser/branchlisting.py'
--- lib/lp/code/browser/branchlisting.py 2011-02-11 15:09:53 +0000
+++ lib/lp/code/browser/branchlisting.py 2011-02-28 00:51:16 +0000
@@ -650,7 +650,7 @@
if lifecycle_status is not None:
collection = collection.withLifecycleStatus(*lifecycle_status)
collection = collection.visibleByUser(self.user)
- return collection.getBranches().order_by(
+ return collection.getBranches(eager_load=False).order_by(
self._listingSortToOrderBy(self.sort_by))
@property
@@ -809,7 +809,7 @@
if lifecycle_status is not None:
collection = collection.withLifecycleStatus(*lifecycle_status)
collection = collection.visibleByUser(self.user)
- return collection.getBranches().order_by(
+ return collection.getBranches(eager_load=False).order_by(
self._branch_order)
@@ -1501,7 +1501,7 @@
# We're only interested in active branches.
collection = self.getBranchCollection().withLifecycleStatus(
*DEFAULT_BRANCH_STATUS_IN_LISTING)
- for branch in collection.getBranches():
+ for branch in collection.getBranches(eager_load=False):
branches.setdefault(branch.distroseries, []).append(branch)
return branches
=== modified file 'lib/lp/code/feed/branch.py'
--- lib/lp/code/feed/branch.py 2010-08-24 10:45:57 +0000
+++ lib/lp/code/feed/branch.py 2011-02-28 00:51:16 +0000
@@ -163,7 +163,7 @@
from lp.code.model.branch import Branch
collection = self._getCollection().visibleByUser(
None).withLifecycleStatus(*DEFAULT_BRANCH_STATUS_IN_LISTING)
- branches = collection.getBranches()
+ branches = collection.getBranches(eager_load=False)
branches.order_by(
Desc(Branch.date_last_modified),
Asc(Branch.target_suffix),
=== modified file 'lib/lp/code/interfaces/branch.py'
--- lib/lp/code/interfaces/branch.py 2011-02-04 16:34:12 +0000
+++ lib/lp/code/interfaces/branch.py 2011-02-28 00:51:16 +0000
@@ -1278,8 +1278,13 @@
"""
@collection_default_content()
- def getBranches(limit=50):
- """Return a collection of branches."""
+ def getBranches(limit=50, eager_load=True):
+ """Return a collection of branches.
+
+ :param eager_load: If True (the default because this is used in the
+ web service and it needs the related objects to create links) eager
+ load related objects (products, code imports etc).
+ """
class IBranchListingQueryOptimiser(Interface):
=== modified file 'lib/lp/code/interfaces/branchcollection.py'
--- lib/lp/code/interfaces/branchcollection.py 2011-01-20 00:06:17 +0000
+++ lib/lp/code/interfaces/branchcollection.py 2011-02-28 00:51:16 +0000
@@ -57,13 +57,16 @@
of individuals and teams that own branches in this collection.
"""
- def getBranches():
+ def getBranches(eager_load=False):
"""Return a result set of all branches in this collection.
The returned result set will also join across the specified tables as
defined by the arguments to this function. These extra tables are
joined specificly to allow the caller to sort on values not in the
Branch table itself.
+
+ :param eager_load: If True trigger eager loading of all the related
+ objects in the collection.
"""
def getMergeProposals(statuses=None, for_branches=None,
=== modified file 'lib/lp/code/interfaces/branchnamespace.py'
--- lib/lp/code/interfaces/branchnamespace.py 2010-08-20 20:31:18 +0000
+++ lib/lp/code/interfaces/branchnamespace.py 2011-02-28 00:51:16 +0000
@@ -52,8 +52,11 @@
a given prefix, use createBranchWithPrefix.
"""
- def getBranches():
- """Return the branches in this namespace."""
+ def getBranches(eager_load=False):
+ """Return the branches in this namespace.
+
+ :param eager_load: If True eager load related data for the branches.
+ """
def getBranchName(name):
"""Get the potential unique name for a branch called 'name'.
=== modified file 'lib/lp/code/interfaces/hasbranches.py'
--- lib/lp/code/interfaces/hasbranches.py 2010-10-27 07:15:02 +0000
+++ lib/lp/code/interfaces/hasbranches.py 2011-02-28 00:51:16 +0000
@@ -60,13 +60,15 @@
@operation_returns_collection_of(Interface) # Really IBranch.
@export_read_operation()
def getBranches(status=None, visible_by_user=None,
- modified_since=None):
+ modified_since=None, eager_load=False):
"""Returns all branches with the given lifecycle status.
:param status: A list of statuses to filter with.
:param visible_by_user: Normally the user who is asking.
:param modified_since: If set, filters the branches being returned
to those that have been modified since the specified date/time.
+ :param eager_load: If True load related objects for the whole
+ collection.
:returns: A list of `IBranch`.
"""
=== modified file 'lib/lp/code/interfaces/seriessourcepackagebranch.py'
--- lib/lp/code/interfaces/seriessourcepackagebranch.py 2010-08-20 20:31:18 +0000
+++ lib/lp/code/interfaces/seriessourcepackagebranch.py 2011-02-28 00:51:16 +0000
@@ -65,6 +65,13 @@
:return: An `IResultSet` of `ISeriesSourcePackageBranch` objects.
"""
+ def findForBranches(branches):
+ """Get the links to source packages from a branch.
+
+ :param branches: A an iterable of `IBranch`.
+ :return: An `IResultSet` of `ISeriesSourcePackageBranch` objects.
+ """
+
def findForSourcePackage(sourcepackage):
"""Get the links to branches from a source package.
=== modified file 'lib/lp/code/model/branch.py'
--- lib/lp/code/model/branch.py 2011-02-23 01:24:09 +0000
+++ lib/lp/code/model/branch.py 2011-02-28 00:51:16 +0000
@@ -137,6 +137,7 @@
from lp.services.job.interfaces.job import JobStatus
from lp.services.job.model.job import Job
from lp.services.mail.notificationrecipientset import NotificationRecipientSet
+from lp.services.propertycache import cachedproperty
class Branch(SQLBase, BzrIdentityMixin):
@@ -704,13 +705,19 @@
DeleteCodeImport(self.code_import)()
Store.of(self).flush()
+ @cachedproperty
+ def _associatedProductSeries(self):
+ """Helper for eager loading associatedProductSeries."""
+ # This is eager loaded by BranchCollection.getBranches.
+ # Imported here to avoid circular import.
+ from lp.registry.model.productseries import ProductSeries
+ return Store.of(self).find(
+ ProductSeries,
+ ProductSeries.branch == self)
+
def associatedProductSeries(self):
"""See `IBranch`."""
- # Imported here to avoid circular import.
- from lp.registry.model.productseries import ProductSeries
- return Store.of(self).find(
- ProductSeries,
- ProductSeries.branch == self)
+ return self._associatedProductSeries
def getProductSeriesPushingTranslations(self):
"""See `IBranch`."""
@@ -720,14 +727,20 @@
ProductSeries,
ProductSeries.translations_branch == self)
- def associatedSuiteSourcePackages(self):
- """See `IBranch`."""
+ @cachedproperty
+ def _associatedSuiteSourcePackages(self):
+ """Helper for associatedSuiteSourcePackages."""
+ # This is eager loaded by BranchCollection.getBranches.
series_set = getUtility(IFindOfficialBranchLinks)
# Order by the pocket to get the release one first.
links = series_set.findForBranch(self).order_by(
SeriesSourcePackageBranch.pocket)
return [link.suite_sourcepackage for link in links]
+ def associatedSuiteSourcePackages(self):
+ """See `IBranch`."""
+ return self._associatedSuiteSourcePackages
+
# subscriptions
def subscribe(self, person, notification_level, max_diff_lines,
code_review_level, subscribed_by):
@@ -1291,7 +1304,8 @@
branches = all_branches.visibleByUser(
visible_by_user).withLifecycleStatus(*lifecycle_statuses)
branches = branches.withBranchType(
- BranchType.HOSTED, BranchType.MIRRORED).scanned().getBranches()
+ BranchType.HOSTED, BranchType.MIRRORED).scanned().getBranches(
+ eager_load=False)
branches.order_by(
Desc(Branch.date_last_modified), Desc(Branch.id))
if branch_count is not None:
@@ -1307,7 +1321,7 @@
branches = all_branches.visibleByUser(
visible_by_user).withLifecycleStatus(*lifecycle_statuses)
branches = branches.withBranchType(
- BranchType.IMPORTED).scanned().getBranches()
+ BranchType.IMPORTED).scanned().getBranches(eager_load=False)
branches.order_by(
Desc(Branch.date_last_modified), Desc(Branch.id))
if branch_count is not None:
@@ -1321,7 +1335,8 @@
"""See `IBranchSet`."""
all_branches = getUtility(IAllBranches)
branches = all_branches.withLifecycleStatus(
- *lifecycle_statuses).visibleByUser(visible_by_user).getBranches()
+ *lifecycle_statuses).visibleByUser(visible_by_user).getBranches(
+ eager_load=False)
branches.order_by(
Desc(Branch.date_created), Desc(Branch.id))
if branch_count is not None:
@@ -1340,10 +1355,10 @@
"""See `IBranchSet`."""
return getUtility(IBranchLookup).getByUrls(urls)
- def getBranches(self, limit=50):
+ def getBranches(self, limit=50, eager_load=True):
"""See `IBranchSet`."""
anon_branches = getUtility(IAllBranches).visibleByUser(None)
- branches = anon_branches.scanned().getBranches()
+ branches = anon_branches.scanned().getBranches(eager_load=eager_load)
branches.order_by(
Desc(Branch.date_last_modified), Desc(Branch.id))
branches.config(limit=limit)
=== modified file 'lib/lp/code/model/branchcollection.py'
--- lib/lp/code/model/branchcollection.py 2011-01-27 14:02:38 +0000
+++ lib/lp/code/model/branchcollection.py 2011-02-28 00:51:16 +0000
@@ -23,20 +23,26 @@
from zope.component import getUtility
from zope.interface import implements
+from canonical.launchpad.components.decoratedresultset import (
+ DecoratedResultSet,
+ )
from canonical.launchpad.webapp.interfaces import (
DEFAULT_FLAVOR,
IStoreSelector,
MAIN_STORE,
)
from canonical.launchpad.webapp.vocabulary import CountableIterator
+from canonical.lazr.utils import safe_hasattr
+from lp.bugs.model.bug import Bug
+from lp.bugs.model.bugbranch import BugBranch
from lp.code.interfaces.branch import user_has_special_branch_access
from lp.code.interfaces.branchcollection import (
IBranchCollection,
InvalidFilter,
)
-
-from lp.bugs.model.bug import Bug
-from lp.bugs.model.bugbranch import BugBranch
+from lp.code.interfaces.seriessourcepackagebranch import (
+ IFindOfficialBranchLinks,
+ )
from lp.code.enums import BranchMergeProposalStatus
from lp.code.interfaces.branchlookup import IBranchLookup
from lp.code.interfaces.codehosting import LAUNCHPAD_SERVICES
@@ -55,6 +61,7 @@
from lp.registry.model.product import Product
from lp.registry.model.sourcepackagename import SourcePackageName
from lp.registry.model.teammembership import TeamParticipation
+from lp.services.propertycache import get_property_cache
class GenericBranchCollection:
@@ -89,7 +96,7 @@
def count(self):
"""See `IBranchCollection`."""
- return self.getBranches().count()
+ return self.getBranches(eager_load=False).count()
def ownerCounts(self):
"""See `IBranchCollection`."""
@@ -136,7 +143,7 @@
def _getBranchIdQuery(self):
"""Return a Storm 'Select' for the branch IDs in this collection."""
- select = self.getBranches()._get_select()
+ select = self.getBranches(eager_load=False)._get_select()
select.columns = (Branch.id,)
return select
@@ -144,11 +151,42 @@
"""Return the where expressions for this collection."""
return self._branch_filter_expressions
- def getBranches(self):
+ def getBranches(self, eager_load=False):
"""See `IBranchCollection`."""
tables = [Branch] + self._tables.values()
expressions = self._getBranchExpressions()
- return self.store.using(*tables).find(Branch, *expressions)
+ resultset = self.store.using(*tables).find(Branch, *expressions)
+ if not eager_load:
+ return resultset
+ def do_eager_load(rows):
+ branch_ids = set(branch.id for branch in rows)
+ if not branch_ids:
+ return
+ branches = dict((branch.id, branch) for branch in rows)
+ caches = dict((branch.id, get_property_cache(branch))
+ for branch in rows)
+ for cache in caches.values():
+ if not safe_hasattr(cache, '_associatedProductSeries'):
+ cache._associatedProductSeries = []
+ if not safe_hasattr(cache, '_associatedSuiteSourcePackages'):
+ cache._associatedSuiteSourcePackages = []
+ # associatedProductSeries
+ # Imported here to avoid circular import.
+ from lp.registry.model.productseries import ProductSeries
+ for productseries in self.store.find(
+ ProductSeries,
+ ProductSeries.branchID.is_in(branch_ids)):
+ cache = caches[productseries.branchID]
+ cache._associatedProductSeries.append(productseries)
+ # associatedSuiteSourcePackages
+ series_set = getUtility(IFindOfficialBranchLinks)
+ # Order by the pocket to get the release one first.
+ links = series_set.findForBranches(rows).order_by(
+ SeriesSourcePackageBranch.pocket)
+ for link in links:
+ cache = caches[link.branchID]
+ cache._associatedSuiteSourcePackages.append(link)
+ return DecoratedResultSet(resultset, pre_iter_hook=do_eager_load)
def getMergeProposals(self, statuses=None, for_branches=None,
target_branch=None, merged_revnos=None):
@@ -364,7 +402,7 @@
# of the unique name and sort based on relevance.
branch = self._getExactMatch(search_term)
if branch is not None:
- if branch in self.getBranches():
+ if branch in self.getBranches(eager_load=False):
return CountableIterator(1, [branch])
else:
return CountableIterator(0, [])
@@ -397,7 +435,8 @@
# Get the results.
collection = self._filterBy([Branch.id.is_in(Union(*queries))])
- results = collection.getBranches().order_by(Branch.name, Branch.id)
+ results = collection.getBranches(eager_load=False).order_by(
+ Branch.name, Branch.id)
return CountableIterator(results.count(), results)
def scanned(self):
=== modified file 'lib/lp/code/model/branchnamespace.py'
--- lib/lp/code/model/branchnamespace.py 2010-10-18 02:37:53 +0000
+++ lib/lp/code/model/branchnamespace.py 2011-02-28 00:51:16 +0000
@@ -218,7 +218,7 @@
name = "%s-%s" % (prefix, count)
return name
- def getBranches(self):
+ def getBranches(self, eager_load=False):
"""See `IBranchNamespace`."""
store = getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR)
return store.find(Branch, self._getBranchesClause())
=== modified file 'lib/lp/code/model/hasbranches.py'
--- lib/lp/code/model/hasbranches.py 2010-10-27 07:15:02 +0000
+++ lib/lp/code/model/hasbranches.py 2011-02-28 00:51:16 +0000
@@ -26,7 +26,7 @@
"""A mixin implementation for `IHasBranches`."""
def getBranches(self, status=None, visible_by_user=None,
- modified_since=None):
+ modified_since=None, eager_load=False):
"""See `IHasBranches`."""
if status is None:
status = DEFAULT_BRANCH_STATUS_IN_LISTING
@@ -35,7 +35,7 @@
collection = collection.withLifecycleStatus(*status)
if modified_since is not None:
collection = collection.modifiedSince(modified_since)
- return collection.getBranches()
+ return collection.getBranches(eager_load=eager_load)
class HasMergeProposalsMixin:
=== modified file 'lib/lp/code/model/seriessourcepackagebranch.py'
--- lib/lp/code/model/seriessourcepackagebranch.py 2010-08-20 20:31:18 +0000
+++ lib/lp/code/model/seriessourcepackagebranch.py 2011-02-28 00:51:16 +0000
@@ -101,10 +101,15 @@
def findForBranch(self, branch):
"""See `IFindOfficialBranchLinks`."""
+ return self.findForBranches([branch])
+
+ def findForBranches(self, branches):
+ """See `IFindOfficialBranchLinks`."""
+ branch_ids = set(branch.id for branch in branches)
store = getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR)
return store.find(
SeriesSourcePackageBranch,
- SeriesSourcePackageBranch.branch == branch.id)
+ SeriesSourcePackageBranch.branchID.is_in(branch_ids))
def findForSourcePackage(self, sourcepackage):
"""See `IFindOfficialBranchLinks`."""
=== modified file 'lib/lp/code/model/tests/test_branchset.py'
--- lib/lp/code/model/tests/test_branchset.py 2010-10-04 19:50:45 +0000
+++ lib/lp/code/model/tests/test_branchset.py 2011-02-28 00:51:16 +0000
@@ -5,32 +5,34 @@
__metaclass__ = type
-from unittest import TestLoader
+from testtools.matchers import LessThan
+from canonical.launchpad.testing.pages import LaunchpadWebServiceCaller
from canonical.testing.layers import DatabaseFunctionalLayer
from lp.code.interfaces.branch import IBranchSet
from lp.code.model.branch import BranchSet
-from lp.testing import TestCaseWithFactory
+from lp.testing import (
+ logout,
+ TestCaseWithFactory,
+ )
+from lp.testing._webservice import QueryCollector
+from lp.testing.matchers import HasQueryCount
class TestBranchSet(TestCaseWithFactory):
layer = DatabaseFunctionalLayer
- def setUp(self):
- TestCaseWithFactory.setUp(self)
- self.branch_set = BranchSet()
-
def test_provides_IBranchSet(self):
# BranchSet instances provide IBranchSet.
- self.assertProvides(self.branch_set, IBranchSet)
+ self.assertProvides(BranchSet(), IBranchSet)
def test_getByUrls(self):
# getByUrls returns a list of branches matching the list of URLs that
# it's given.
a = self.factory.makeAnyBranch()
b = self.factory.makeAnyBranch()
- branches = self.branch_set.getByUrls(
+ branches = BranchSet().getByUrls(
[a.bzr_identity, b.bzr_identity])
self.assertEqual({a.bzr_identity: a, b.bzr_identity: b}, branches)
@@ -38,9 +40,21 @@
# If a branch cannot be found for a URL, then None appears in the list
# in place of the branch.
url = 'http://example.com/doesntexist'
- branches = self.branch_set.getByUrls([url])
+ branches = BranchSet().getByUrls([url])
self.assertEqual({url: None}, branches)
-
-def test_suite():
- return TestLoader().loadTestsFromName(__name__)
+ def test_api_branches_query_count(self):
+ webservice = LaunchpadWebServiceCaller()
+ collector = QueryCollector()
+ collector.register()
+ self.addCleanup(collector.unregister)
+ # Get 'all' of the 50 branches this collection is limited to - rather
+ # than the default in-test-suite pagination size of 5.
+ url = "/branches?ws.size=50"
+ logout()
+ response = webservice.get(url,
+ headers={'User-Agent': 'AnonNeedsThis'})
+ self.assertEqual(response.status, 200,
+ "Got %d for url %r with response %r" % (
+ response.status, url, response.body))
+ self.assertThat(collector, HasQueryCount(LessThan(13)))
=== modified file 'lib/lp/code/model/tests/test_branchtarget.py'
--- lib/lp/code/model/tests/test_branchtarget.py 2011-01-31 02:12:30 +0000
+++ lib/lp/code/model/tests/test_branchtarget.py 2011-02-28 00:51:16 +0000
@@ -53,7 +53,7 @@
# branches related to the branch target.
self.assertEqual(self.target.collection.getBranches().count(), 0)
branch = self.makeBranchForTarget()
- branches = self.target.collection.getBranches()
+ branches = self.target.collection.getBranches(eager_load=False)
self.assertEqual([branch], list(branches))
def test_retargetBranch_packageBranch(self):
=== modified file 'lib/lp/code/tests/test_helpers.py'
--- lib/lp/code/tests/test_helpers.py 2010-10-04 19:50:45 +0000
+++ lib/lp/code/tests/test_helpers.py 2011-02-28 00:51:16 +0000
@@ -35,6 +35,7 @@
self.assertIsNot(None, fooix)
# There should be one branch with one commit.
[branch] = list(
- getUtility(IAllBranches).inProduct(fooix).getBranches())
+ getUtility(IAllBranches).inProduct(fooix).getBranches(
+ eager_load=False))
self.assertEqual(1, branch.revision_count)
self.assertEqual(commit_time, branch.getTipRevision().revision_date)
=== modified file 'lib/lp/code/vocabularies/branch.py'
--- lib/lp/code/vocabularies/branch.py 2010-12-01 18:58:44 +0000
+++ lib/lp/code/vocabularies/branch.py 2011-02-28 00:51:16 +0000
@@ -69,7 +69,7 @@
logged_in_user = getUtility(ILaunchBag).user
collection = self._getCollection().visibleByUser(logged_in_user)
if query is None:
- branches = collection.getBranches()
+ branches = collection.getBranches(eager_load=False)
else:
branches = collection.search(query)
return CountableIterator(branches.count(), branches, self.toTerm)
=== modified file 'lib/lp/codehosting/branchdistro.py'
--- lib/lp/codehosting/branchdistro.py 2010-11-05 14:17:11 +0000
+++ lib/lp/codehosting/branchdistro.py 2011-02-28 00:51:16 +0000
@@ -146,7 +146,8 @@
"""
branches = getUtility(IAllBranches)
distroseries_branches = branches.inDistroSeries(self.old_distroseries)
- return distroseries_branches.officialBranches().getBranches()
+ return distroseries_branches.officialBranches().getBranches(
+ eager_load=False)
def checkConsistentOfficialPackageBranch(self, db_branch):
"""Check that `db_branch` is a consistent official package branch.
=== modified file 'lib/lp/codehosting/scanner/mergedetection.py'
--- lib/lp/codehosting/scanner/mergedetection.py 2010-10-08 20:25:27 +0000
+++ lib/lp/codehosting/scanner/mergedetection.py 2011-02-28 00:51:16 +0000
@@ -97,7 +97,7 @@
BranchLifecycleStatus.DEVELOPMENT,
BranchLifecycleStatus.EXPERIMENTAL,
BranchLifecycleStatus.MATURE,
- BranchLifecycleStatus.ABANDONED).getBranches()
+ BranchLifecycleStatus.ABANDONED).getBranches(eager_load=False)
for branch in branches:
last_scanned = branch.last_scanned_id
# If the branch doesn't have any revisions, not any point setting
