Merge lp:~lifeless/launchpad/milestones into lp:launchpad

[Robert Collins]

Convert BugTaskSet.search to return a storm resultset and cache the user queried with as a viewer on returned Bug objects.

Hopefully fix the current instantiation of https://bugs.edge.launchpad.net/launchpad-registry/+bug/447418.

Private bug access checking is rather slow: https://bugs.edge.launchpad.net/malone/+bug/619039.

Solution - a cached attribute recording people that we've calculated should have view access, prepopulated by BugTaskSet.search().

Added tests for the low level behaviour and that it fixes the observed scaling problem in a simple milestone view.

\o/

[Robert Collins (lifeless)]

Deryck, Curtis, would appreciate sanity checks from you :)

[Graham Binns (gmb)]

Hi Rob,

Couple of nitpicks, but otherwise okay. Marking it needs fixing because
of the over-long test (see below) but if we can't split that up I'm
happy for it to land as-is.

> === modified file 'lib/lp/bugs/tests/test_bugtask.py'
> --- lib/lp/bugs/tests/test_bugtask.py 2010-07-14 14:11:15 +0000
> +++ lib/lp/bugs/tests/test_bugtask.py 2010-08-17 09:41:19 +0000
> @@ -792,6 +796,28 @@
> result = target.searchTasks(None, modified_since=date)
> self.assertEqual([task2], list(result))
>
> + def test_private_bug_view_permissions_cached(self):
> + """private bugs from a search know the user can see the bugs."""

Captialise at the start of the sentence ;).

> + target = self.makeBugTarget()
> + person = self.login()
> + self.factory.makeBug(product=target, private=True, owner=person)
> + self.factory.makeBug(product=target, private=True, owner=person)
> + # Search style and parameters taken from the milestone index view where
> + # the issue was discovered.
> + login_person(person)
> + tasks =target.searchTasks(BugTaskSearchParams(person, omit_dupes=True,

Needs a space after the =.

> + orderby=['status', '-importance', 'id']))

> === modified file 'lib/lp/registry/browser/tests/test_milestone.py'
> --- lib/lp/registry/browser/tests/test_milestone.py 2010-08-16 18:39:11 +0000
> +++ lib/lp/registry/browser/tests/test_milestone.py 2010-08-17 09:41:19 +0000
> @@ -105,5 +116,61 @@
> self.assertEqual(0, product.development_focus.all_bugtasks.count())
>
>
> -def test_suite():
> - return unittest.TestLoader().loadTestsFromName(__name__)
> +class TestMilestoneIndex(TestCaseWithFactory):
> +
> + layer = DatabaseFunctionalLayer
> +
> + def test_more_private_bugs_query_count_is_constant(self):

The test should have a comment at the start of it to save me having to
parse the method name.

> + product = self.factory.makeProduct()
> + login_person(product.owner)
> + milestone = self.factory.makeMilestone(
> + productseries=product.development_focus)
> + bug1 = self.factory.makeBug(product=product, private=True,
> + owner=product.owner)
> + bug1.bugtasks[0].transitionToMilestone(milestone, product.owner)
> + # We look at the page as someone who is a member of a team and the team
> + # is subscribed to the bugs, so that we don't get trivial shortcuts
> + # avoiding queries : test the worst case.
> + subscribed_team = self.factory.makeTeam()
> + viewer = self.factory.makePerson(password="test")
> + with person_logged_in(subscribed_team.teamowner):
> + subscribed_team.addMember(viewer, subscribed_team.teamowner)
> + bug1.subscribe(subscribed_team, product.owner)
> + bug1_url = canonical_url(bug1)
> + milestone_url = canonical_url(milestone)
> + browser = self.getUserBrowser(user=viewer)
> + # Seed the cookie cache and any other cross-request state we may gain
> + # in future. See canonical.launchpad.webapp.serssion: _get_secret.
> + browser.open(milestone_url)
> + collector = QueryC...

review: Needs Fixing (code)

[Robert Collins (lifeless)]

I'll do the tweaks; I don't see a good way (for now) of shrinking the
test; two separate tests would require a magic constant to be shared
between them rather than being derived on the fly; that would be more
fragile.

-Rob

[Graham Binns (gmb)]

On 17 August 2010 11:48, Robert Collins <email address hidden> wrote:
> I'll do the tweaks; I don't see a good way (for now) of shrinking the
> test; two separate tests would require a magic constant to be shared
> between them rather than being derived on the fly; that would be more
> fragile.
>

Right, that's pretty much what I figured.

[Graham Binns (gmb)]

review: Approve (code)

[Curtis Hovey (sinzui)]

MilestoneView._bugtasks uses precache_permission_for_objects() on the bugtasks and the conjoined bugtasks. Does this change allow us to remove precache_permission_for_objects()? Will your test fail if I remove precache_permission_for_objects() from the view?

MilestoneView._bugtasks calls getConjoinedMaster() with a prepared list (1 query) of expected ids. I think we need to keep behaviour; this branch just lowers the access cost of getConjoinedMaster()

review: Needs Information

[Robert Collins (lifeless)]

On Wed, Aug 18, 2010 at 2:18 AM, Curtis Hovey
<email address hidden> wrote:
> Review: Needs Information
> MilestoneView._bugtasks uses precache_permission_for_objects() on the bugtasks and the conjoined bugtasks. Does this change allow us to remove precache_permission_for_objects()? Will your test fail if I remove precache_permission_for_objects() from the view?

For objects queries via that search path, the precaching be removed. I
do not know if getConjoinedMaster accesses the related objects via
that code path - and I suspect in fact that it does not.

> MilestoneView._bugtasks calls getConjoinedMaster() with a prepared list (1 query) of expected ids. I think we need to keep behaviour; this branch just lowers the access cost of getConjoinedMaster()

We can refactor the conjoined master stuff too, but its a separate issue IMO.

-Rob

[Curtis Hovey (sinzui)]

I do not understand the first part of your answer. I see this in MilestoneView._bugtasks:
        ...
        for task in tasks:
            if task.getConjoinedMaster(bugs_and_tasks[task.bug]) is None:
                non_conjoined_slaves.append(task)
        # Checking bug permissions is expensive. We know from the query that
        # the user has at least launchpad.View on the bugtasks and their bugs.
        precache_permission_for_objects(
            self.request, 'launchpad.View', non_conjoined_slaves)
        precache_permission_for_objects(
            self.request, 'launchpad.View',
            [task.bug for task in non_conjoined_slaves])
        return non_conjoined_slaves

Since you modified the permission rules on bugtask, I wonder if the first precache_permission_for_objects() is not needed. I wonder if permission checking is fast enough.

We stopped adding enahncements to the milestone page when the preformance problems started, and we cannot entertain allowing users to edit the bugtasks unless we know the user has launchpad.Edit. I wonder if this permission change allows us to consider enhancements.

[Robert Collins (lifeless)]

I certainly think you can make enchancements, and yes we should be
able to remove the precache permissions for nonconjoinedslaves.

I'd like to do that separately from this branch, as a mitigation : I
know what I've researched works, I'm not sure that there aren't other
gremlins in there.

[Curtis Hovey (sinzui)]

Yes. I agree that the removal of precache_permissions is a separate branch. Thanks for answering my questions.

review: Approve (code)

[Deryck Hodge (deryck)]

Looks good to me, Robert. Thanks for doing this work!

Cheers,
deryck

review: Approve (code)

[Robert Collins (lifeless)]

I hate to ask for a rereview, but I'll ask anyhow. This branch ran into incompatibilities with SQLObjectResultSet so it now converts buildQuery and thing things on it to be less SQLObject and more Storm. This involved finding addressing a bunch of differences; it still has literal SQL and so on, but at least at the surface its now sensible. I'm running it through EC2 test now.

[Curtis Hovey (sinzui)]

Hi Robert.

Thanks for this refactoring.

Can you place a comment before ``if False and True:`` in BugtaskSet.search() to explain the dagnostic output? I had to think about the old and new guard.

I think you can remove the naked taskset in BugTaskSet.searchBugIds(). I do not think it is being used.

review: Approve (code)

[Robert Collins (lifeless)]

Thank you curtis; I've deleted the naked taskset, and deleted the
print statements I had accidentally left in which had that confusing
if statement.

This is the only failing test ec2 told me about

FAIL: productseries-views_txt
----------------------------------------------------------------------
Text attachment: traceback
------------
Traceback (most recent call last):
  File "/usr/lib/python2.6/unittest.py", line 279, in run
    testMethod()
  File "/usr/lib/python2.6/doctest.py", line 2152, in runTest
    raise self.failureException(self.format_failure(new.getvalue()))
AssertionError: Failed doctest test for productseries-views.txt
  File "lib/lp/registry/browser/tests/productseries-views.txt", line 0

----------------------------------------------------------------------
File "lib/lp/registry/browser/tests/productseries-views.txt", line 80,
in productseries-views.txt
Failed example:
    script = find_tag_by_id(view.render(), 'milestone-script')
Differences (ndiff with -expected +actual):
    + WARNING:root:Memcache set failed for
pt:testrunner:lp/bugs/templates/bugtarget-portlet-latestbugs.pt,10721:16:1,0:1,http_//127.0.0.1?6RKVP3n9fovtv8CxVi8ydF
----------------------------------------------------------------------
File "lib/lp/registry/browser/tests/productseries-views.txt", line
102, in productseries-views.txt
Failed example:
    content = view.render()
Differences (ndiff with -expected +actual):
    + WARNING:root:Memcache set failed for
pt:testrunner:lp/bugs/templates/bugtarget-portlet-latestbugs.pt,10721:243656:1,0:1,http_//127.0.0.1?4Ophjjtmt5aXGqxEwNgYvP
----------------------------------------------------------------------
File "lib/lp/registry/browser/tests/productseries-views.txt", line
117, in productseries-views.txt
Failed example:
    table = find_tag_by_id(view.render(), 'series-simple')
Differences (ndiff with -expected +actual):
    + WARNING:root:Memcache set failed for
pt:testrunner:lp/bugs/templates/bugtarget-portlet-latestbugs.pt,10721:243652:1,0:1,http_//127.0.0.1?IXiDEgHZPmgHvZKrJmNvh

If you have any ideas about it, I'd love to know.

[Curtis Hovey (sinzui)]

productseries-views.txt is now rendering a template that uses the memcache tales directive. We need to run it on a layer that has the memcached server up.

Add this to special_test_layer in lp/registry/browser/tests/test_views.py
    'productseries-views.txt': LaunchpadFunctionalLayer,

=== modified file 'lib/canonical/launchpad/interfaces/validation.py'
--- lib/canonical/launchpad/interfaces/validation.py	2010-08-20 20:31:18 +0000
+++ lib/canonical/launchpad/interfaces/validation.py	2010-08-22 18:34:46 +0000
@@ -312,7 +312,7 @@
     errors = []
     user = getUtility(ILaunchBag).user
     params = BugTaskSearchParams(user, bug=bug)
-    if product.searchTasks(params):
+    if not product.searchTasks(params).is_empty():
         errors.append(LaunchpadValidationError(_(
             'A fix for this bug has already been requested for ${product}',
             mapping={'product': product.displayname})))

=== modified file 'lib/canonical/launchpad/security.py'
--- lib/canonical/launchpad/security.py	2010-08-20 20:31:18 +0000
+++ lib/canonical/launchpad/security.py	2010-08-22 18:34:46 +0000
@@ -988,24 +988,8 @@
     usedfor = IHasBug
 
     def checkAuthenticated(self, user):
-
-        if user.in_admin:
-            # Admins can always edit bugtasks, whether they're reported on a
-            # private bug or not.
-            return True
-
-        if not self.obj.bug.private:
-            # This is a public bug, so anyone can edit it.
-            return True
-        else:
-            # This is a private bug, and we know the user isn't an admin, so
-            # we'll only allow editing if the user is explicitly subscribed to
-            # this bug.
-            for subscription in self.obj.bug.subscriptions:
-                if user.inTeam(subscription.person):
-                    return True
-
-            return False
+        # Delegated entirely to the bug.
+        return self.obj.bug.userCanView(user)
 
 
 class PublicToAllOrPrivateToExplicitSubscribersForBugTask(AuthorizationBase):
@@ -1027,21 +1011,10 @@
 
     def checkAuthenticated(self, user):
         """Allow any logged in user to edit a public bug, and only
-        explicit subscribers to edit private bugs.
+        explicit subscribers to edit private bugs. Any bug that can be seen can
+        be edited.
         """
-        if not self.obj.private:
-            # This is a public bug.
-            return True
-        elif user.in_admin:
-            # Admins can edit all bugs.
-            return True
-        else:
-            # This is a private bug. Only explicit subscribers may edit it.
-            for subscription in self.obj.subscriptions:
-                if user.inTeam(subscription.person):
-                    return True
-
-        return False
+        return self.obj.userCanView(user)
 
     def checkUnauthenticated(self):
         """Never allow unauthenticated users to edit a bug."""

=== modified file 'lib/canonical/launchpad/utilities/personroles.py'
--- lib/canonical/launchpad/utilities/personroles.py	2010-08-20 20:31:18 +0000
+++ lib/canonical/launchpad/utilities/personroles.py	2010-08-22 18:34:46 +0000
@@ -41,6 +41,10 @@
         except AttributeError:
             raise AttributeError(errortext)
 
+    @property
+    def id(self):
+        return self.person.id
+
     def isOwner(self, obj):
         """See IPersonRoles."""
         return self.person.inTeam(obj.owner)

=== modified file 'lib/lp/bugs/browser/bugtarget.py'
--- lib/lp/bugs/browser/bugtarget.py	2010-08-20 20:31:18 +0000
+++ lib/lp/bugs/browser/bugtarget.py	2010-08-22 18:34:46 +0000
@@ -966,13 +966,8 @@
 
         matching_bugtasks = getUtility(IBugTaskSet).findSimilar(
             self.user, title, **context_params)
-        # Remove all the prejoins, since we won't use them and they slow
-        # down the query significantly.
-        matching_bugtasks = matching_bugtasks.prejoin([])
-
         matching_bugs = getUtility(IBugSet).getDistinctBugsForBugTasks(
             matching_bugtasks, self.user, self._MATCHING_BUGS_LIMIT)
-
         return matching_bugs
 
     @property

=== modified file 'lib/lp/bugs/browser/bugtask.py'
--- lib/lp/bugs/browser/bugtask.py	2010-08-20 20:31:18 +0000
+++ lib/lp/bugs/browser/bugtask.py	2010-08-22 18:34:46 +0000
@@ -3197,25 +3197,8 @@
         else:
             raise AssertionError('Uknown context type: %s' % self.context)
 
-        # XXX flacoste 2008/04/25 bug=221947 This should be moved to an
-        # IBugTaskSet.findBugIds(search_params) method.
-        # buildQuery() is part of the internal API.
-        taskset = removeSecurityProxy(getUtility(IBugTaskSet))
-        query, clauseTables, orderBy = taskset.buildQuery(search_params)
-
-        # Convert list of SQLObject order by spec into SQL.
-        order_by_clause = []
-        for field in orderBy:
-            if field[0] == '-':
-                field = "%s DESC" % field[1:]
-            order_by_clause.append(field)
-
-        sql = 'SELECT BugTask.bug FROM %s WHERE %s ORDER BY %s' % (
-            ', '.join(clauseTables), query, ', '.join(order_by_clause))
-
-        cur = cursor()
-        cur.execute(sql)
-        return u"".join("%d\n" % row[0] for row in cur.fetchall())
+        return u"".join("%d\n" % bug_id for bug_id in 
+            getUtility(IBugTaskSet).searchBugIds(search_params))
 
 
 def _by_targetname(bugtask):

=== modified file 'lib/lp/bugs/browser/tests/bugtask-target-link-titles.txt'
--- lib/lp/bugs/browser/tests/bugtask-target-link-titles.txt	2009-06-12 16:36:02 +0000
+++ lib/lp/bugs/browser/tests/bugtask-target-link-titles.txt	2010-08-22 18:34:46 +0000
@@ -73,7 +73,7 @@
     >>> published_distro_series = factory.makeDistroRelease(
     ...     published_distro, name='published-distro-series')
     >>> publisher.setUpDefaultDistroSeries(published_distro_series)
-    <DistroSeries at ...>
+    <DistroSeries u'published-distro-series'>
     >>> print publisher.person.unique_displayname
     Foo Bar (name16)
     >>> no_priv = getUtility(IPersonSet).getByName('no-priv')

=== modified file 'lib/lp/bugs/browser/tests/test_bugtarget_filebug.py'
--- lib/lp/bugs/browser/tests/test_bugtarget_filebug.py	2010-08-20 20:31:18 +0000
+++ lib/lp/bugs/browser/tests/test_bugtarget_filebug.py	2010-08-22 18:34:46 +0000
@@ -212,7 +212,7 @@
 
     def test_getAcknowledgementMessage_dsp_custom_distro_message(self):
         # If a distribution has a customized conformatom message, it
-        # is used for bugs filed on DsitributionSourcePackages.
+        # is used for bugs filed on DistributionSourcePackages.
         dsp = self.factory.makeDistributionSourcePackage()
         dsp.distribution.bug_reported_acknowledgement = (
             u"Thank you for filing a bug in our distribution")

=== modified file 'lib/lp/bugs/interfaces/bugtask.py'
--- lib/lp/bugs/interfaces/bugtask.py	2010-08-20 20:31:18 +0000
+++ lib/lp/bugs/interfaces/bugtask.py	2010-08-22 18:34:46 +0000
@@ -1398,6 +1398,14 @@
         orderby specified in the first BugTaskSearchParams object.
         """
 
+    def searchBugIds(params):
+        """Search bug ids.
+
+        This is a variation on IBugTaskSet.search that returns only the bug ids.
+
+        :param params: the BugTaskSearchParams to search on.
+        """
+
     def getAssignedMilestonesFromSearch(search_results):
         """Returns distinct milestones for the given tasks.
 

=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py	2010-08-20 20:31:18 +0000
+++ lib/lp/bugs/model/bug.py	2010-08-22 18:34:46 +0000
@@ -70,6 +70,10 @@
     providedBy,
     )
 
+from canonical.cachedproperty import (
+    cachedproperty,
+    clear_property,
+    )
 from canonical.config import config
 from canonical.database.constants import UTC_NOW
 from canonical.database.datetimecol import UtcDateTimeCol
@@ -85,7 +89,10 @@
     MessageSet,
     )
 from canonical.launchpad.helpers import shortlist
-from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
+from canonical.launchpad.interfaces.launchpad import (
+    ILaunchpadCelebrities,
+    IPersonRoles,
+    )
 from canonical.launchpad.interfaces.librarian import ILibraryFileAliasSet
 from canonical.launchpad.interfaces.lpstorm import IStore
 from canonical.launchpad.interfaces.message import (
@@ -626,6 +633,7 @@
                 # disabled see the change.
                 store.flush()
                 self.updateHeat()
+                clear_property(self, '_cached_viewers')
                 return
 
     def unsubscribeFromDupes(self, person, unsubscribed_by):
@@ -1618,21 +1626,31 @@
             self, self.messages[comment_number])
         bug_message.visible = visible
 
+    @cachedproperty('_cached_viewers')
+    def _known_viewers(self):
+        """A dict of of known persons able to view this bug."""
+        return set()
+
     def userCanView(self, user):
-        """See `IBug`."""
-        admins = getUtility(ILaunchpadCelebrities).admin
+        """See `IBug`.
+        
+        Note that Editing is also controlled by this check,
+        because we permit editing of any bug one can see.
+        """
+        if user.id in self._known_viewers:
+            return True
         if not self.private:
             # This is a public bug.
             return True
-        elif user.inTeam(admins):
+        elif IPersonRoles(user).in_admin:
             # Admins can view all bugs.
             return True
         else:
             # This is a private bug. Only explicit subscribers may view it.
             for subscription in self.subscriptions:
                 if user.inTeam(subscription.person):
+                    self._known_viewers.add(user.id)
                     return True
-
         return False
 
     def linkHWSubmission(self, submission):
@@ -1706,6 +1724,7 @@
 
         self.heat = SQL("calculate_bug_heat(%s)" % sqlvalues(self))
         self.heat_last_updated = UTC_NOW
+        store.flush()
 
     @property
     def attachments(self):
@@ -1942,6 +1961,10 @@
         #      Transaction.iterSelect() will try to listify the results.
         #      This can be fixed by selecting from Bugs directly, but
         #      that's non-trivial.
+        # ---: Robert Collins 20100818: if bug_tasks implements IResultSset
+        #      then it should be very possible to improve on it, though
+        #      DecoratedResultSets would need careful handling (e.g. type
+        #      driven callbacks on columns)
         # We select more than :limit: since if a bug affects more than
         # one source package, it will be returned more than one time. 4
         # is an arbitrary number that should be large enough.

=== modified file 'lib/lp/bugs/model/bugtarget.py'
--- lib/lp/bugs/model/bugtarget.py	2010-08-20 20:31:18 +0000
+++ lib/lp/bugs/model/bugtarget.py	2010-08-22 18:34:46 +0000
@@ -207,13 +207,7 @@
     @property
     def has_bugtasks(self):
         """See `IHasBugs`."""
-        # Check efficiently if any bugtasks exist. We should avoid
-        # expensive calls like all_bugtasks.count(). all_bugtasks
-        # returns a storm.SQLObjectResultSet instance, and this
-        # class does not provide methods like is_empty(). But we can
-        # indirectly call SQLObjectResultSet._result_set.is_empty()
-        # by converting all_bugtasks into a boolean object.
-        return bool(self.all_bugtasks)
+        return not self.all_bugtasks.is_empty()
 
     def getBugCounts(self, user, statuses=None):
         """See `IHasBugs`."""

=== modified file 'lib/lp/bugs/model/bugtask.py'
--- lib/lp/bugs/model/bugtask.py	2010-08-20 20:31:18 +0000
+++ lib/lp/bugs/model/bugtask.py	2010-08-22 18:34:46 +0000
@@ -21,7 +21,7 @@
 
 
 import datetime
-from operator import attrgetter
+from operator import attrgetter, itemgetter
 
 from lazr.enum import DBItem
 import pytz
@@ -35,6 +35,7 @@
     Alias,
     And,
     AutoTables,
+    Desc,
     In,
     Join,
     LeftJoin,
@@ -58,6 +59,7 @@
     removeSecurityProxy,
     )
 
+from canonical.cachedproperty import cache_property
 from canonical.config import config
 from canonical.database.constants import UTC_NOW
 from canonical.database.datetimecol import UtcDateTimeCol
@@ -72,6 +74,7 @@
     SQLBase,
     sqlvalues,
     )
+from canonical.launchpad.components.decoratedresultset import DecoratedResultSet
 from canonical.launchpad.helpers import shortlist
 from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
 from canonical.launchpad.interfaces.lpstorm import IStore
@@ -118,6 +121,7 @@
     UserCannotEditBugTaskMilestone,
     UserCannotEditBugTaskStatus,
     )
+from lp.bugs.model.bugnomination import BugNomination
 from lp.bugs.model.bugsubscription import BugSubscription
 from lp.registry.interfaces.distribution import (
     IDistribution,
@@ -148,7 +152,10 @@
 from lp.registry.interfaces.sourcepackage import ISourcePackage
 from lp.registry.interfaces.sourcepackagename import ISourcePackageNameSet
 from lp.registry.model.pillar import pillar_sort_key
+from lp.registry.model.sourcepackagename import SourcePackageName
 from lp.soyuz.interfaces.publishing import PackagePublishingStatus
+from lp.soyuz.model.publishing import SourcePackagePublishingHistory
+from lp.soyuz.model.sourcepackagerelease import SourcePackageRelease
 
 
 debbugsseveritymap = {None:        BugTaskImportance.UNDECIDED,
@@ -673,9 +680,6 @@
 
         matching_bugtasks = getUtility(IBugTaskSet).findSimilar(
             user, self.bug.title, **context_params)
-        # Remove all the prejoins, since we won't use them and they slow
-        # down the query significantly.
-        matching_bugtasks = matching_bugtasks.prejoin([])
 
         # Make sure to exclude the current BugTask from the list of
         # matching tasks. We use 4*limit as an arbitrary value here to
@@ -1302,24 +1306,54 @@
 
 def get_bug_privacy_filter(user):
     """An SQL filter for search results that adds privacy-awareness."""
+    return get_bug_privacy_filter_with_decorator(user)[0]
+
+
+def _nocache_bug_decorator(obj):
+    """A pass through decorator for consistency.
+    
+    :seealso: get_bug_privacy_filter_with_decorator
+    """
+    return obj
+
+
+def _make_cache_user_can_view_bug(user):
+    """Curry a decorator for bugtask queries to cache permissions.
+
+    :seealso: get_bug_privacy_filter_with_decorator
+    """
+    userid = user.id
+    def cache_user_can_view_bug(bugtask):
+        cache_property(bugtask.bug, '_cached_viewers', set([userid]))
+        return bugtask
+    return cache_user_can_view_bug
+
+
+def get_bug_privacy_filter_with_decorator(user):
+    """Return a SQL filter to limit returned bug tasks.
+
+    :return: A SQL filter, a decorator to cache visibility in a resultset that
+        returns BugTask objects.
+    """
     if user is None:
-        return "Bug.private = FALSE"
+        return "Bug.private = FALSE", _nocache_bug_decorator
     admin_team = getUtility(ILaunchpadCelebrities).admin
     if user.inTeam(admin_team):
-        return ""
+        return "", _nocache_bug_decorator
     # A subselect is used here because joining through
     # TeamParticipation is only relevant to the "user-aware"
     # part of the WHERE condition (i.e. the bit below.) The
     # other half of this condition (see code above) does not
     # use TeamParticipation at all.
-    return """
+    return ("""
         (Bug.private = FALSE OR EXISTS (
              SELECT BugSubscription.bug
              FROM BugSubscription, TeamParticipation
              WHERE TeamParticipation.person = %(personid)s AND
                    BugSubscription.person = TeamParticipation.team AND
                    BugSubscription.bug = Bug.id))
-                     """ % sqlvalues(personid=user.id)
+                     """ % sqlvalues(personid=user.id),
+        _make_cache_user_can_view_bug(user))
 
 
 def build_tag_set_query(joiner, tags):
@@ -1408,24 +1442,7 @@
     """See `IBugTaskSet`."""
     implements(IBugTaskSet)
 
-    _ORDERBY_COLUMN = {
-        "id": "BugTask.bug",
-        "importance": "BugTask.importance",
-        "assignee": "BugTask.assignee",
-        "targetname": "BugTask.targetnamecache",
-        "status": "BugTask.status",
-        "title": "Bug.title",
-        "milestone": "BugTask.milestone",
-        "dateassigned": "BugTask.dateassigned",
-        "datecreated": "BugTask.datecreated",
-        "date_last_updated": "Bug.date_last_updated",
-        "date_closed": "BugTask.date_closed",
-        "number_of_duplicates": "Bug.number_of_duplicates",
-        "message_count": "Bug.message_count",
-        "users_affected_count": "Bug.users_affected_count",
-        "heat": "Bug.heat",
-        "latest_patch_uploaded": "Bug.latest_patch_uploaded",
-        }
+    _ORDERBY_COLUMN = None
 
     _open_resolved_upstream = """
                 EXISTS (
@@ -1544,7 +1561,7 @@
 
         search_params.fast_searchtext = nl_phrase_search(
             summary, Bug, ' AND '.join(constraint_clauses), ['BugTask'])
-        return self.search(search_params)
+        return self.search(search_params, _noprejoins=True)
 
     def _buildStatusClause(self, status):
         """Return the SQL query fragment for search by status.
@@ -1591,11 +1608,15 @@
         """Build and return an SQL query with the given parameters.
 
         Also return the clauseTables and orderBy for the generated query.
+
+        :return: A query, the tables to query, ordering expression and a
+            decorator to call on each returned row.
         """
         assert isinstance(params, BugTaskSearchParams)
-
+        from lp.bugs.model.bug import Bug
         extra_clauses = ['Bug.id = BugTask.bug']
-        clauseTables = ['BugTask', 'Bug']
+        clauseTables = [BugTask, Bug]
+        decorators = []
 
         # These arguments can be processed in a loop without any other
         # special handling.
@@ -1651,7 +1672,9 @@
             extra_clauses.append("BugTask.milestone %s" % where_cond)
 
         if params.project:
-            clauseTables.append("Product")
+            # Circular.
+            from lp.registry.model.product import Product
+            clauseTables.append(Product)
             extra_clauses.append("BugTask.product = Product.id")
             if isinstance(params.project, any):
                 extra_clauses.append("Product.project IN (%s)" % ",".join(
@@ -1694,7 +1717,7 @@
             extra_clauses.append(self._buildFastSearchTextClause(params))
 
         if params.subscriber is not None:
-            clauseTables.append('BugSubscription')
+            clauseTables.append(BugSubscription)
             extra_clauses.append("""Bug.id = BugSubscription.bug AND
                     BugSubscription.person = %(personid)s""" %
                     sqlvalues(personid=params.subscriber.id))
@@ -1742,8 +1765,8 @@
             extra_clauses.append(structural_subscriber_clause)
 
         if params.component:
-            clauseTables += ["SourcePackagePublishingHistory",
-                             "SourcePackageRelease"]
+            clauseTables += [SourcePackagePublishingHistory,
+                             SourcePackageRelease]
             distroseries = None
             if params.distribution:
                 distroseries = params.distribution.currentseries
@@ -1866,11 +1889,12 @@
                 BugNomination.status = %(nomination_status)s
                 """ % mappings
             extra_clauses.append(nominated_for_clause)
-            clauseTables.append('BugNomination')
+            clauseTables.append(BugNomination)
 
-        clause = get_bug_privacy_filter(params.user)
+        clause, decorator = get_bug_privacy_filter_with_decorator(params.user)
         if clause:
             extra_clauses.append(clause)
+            decorators.append(decorator)
 
         hw_clause = self._buildHardwareRelatedClause(params)
         if hw_clause is not None:
@@ -1899,7 +1923,15 @@
         orderby_arg = self._processOrderBy(params)
 
         query = " AND ".join(extra_clauses)
-        return query, clauseTables, orderby_arg
+
+        if not decorators:
+            decorator = lambda x:x
+        else:
+            def decorator(obj):
+                for decor in decorators:
+                    obj = decor(obj)
+                return obj
+        return query, clauseTables, orderby_arg, decorator
 
     def _buildUpstreamClause(self, params):
         """Return an clause for returning upstream data if the data exists.
@@ -2127,34 +2159,79 @@
             ', '.join(tables), ' AND '.join(clauses))
         return clause
 
-    def search(self, params, *args):
-        """See `IBugTaskSet`."""
-        store_selector = getUtility(IStoreSelector)
-        store = store_selector.get(MAIN_STORE, SLAVE_FLAVOR)
-        query, clauseTables, orderby = self.buildQuery(params)
+    def search(self, params, *args, **kwargs):
+        """See `IBugTaskSet`.
+        
+        :param _noprejoins: Private internal parameter to BugTaskSet which
+            disables all use of prejoins : consolidated from code paths that
+            claim they were inefficient and unwanted.
+        """
+        # Circular.
+        from lp.registry.model.product import Product
+        from lp.bugs.model.bug import Bug
+        _noprejoins = kwargs.get('_noprejoins', False)
+        store = IStore(BugTask)
+        query, clauseTables, orderby, bugtask_decorator = self.buildQuery(params)
         if len(args) == 0:
-            # Do normal prejoins, if we don't have to do any UNION
-            # queries.  Prejoins don't work well with UNION, and the way
-            # we prefetch objects without prejoins cause problems with
-            # COUNT(*) queries, which get inefficient.
-            return BugTask.select(
-                query, clauseTables=clauseTables, orderBy=orderby,
-                prejoins=['product', 'sourcepackagename'],
-                prejoinClauseTables=['Bug'])
+            if _noprejoins:
+                resultset = store.find(BugTask,
+                    AutoTables(SQL("1=1"), clauseTables),
+                    query)
+                decorator = bugtask_decorator
+            else:
+                tables = clauseTables + [Product, SourcePackageName]
+                origin = [
+                    BugTask,
+                    LeftJoin(Bug, BugTask.bug == Bug.id),
+                    LeftJoin(Product, BugTask.product == Product.id),
+                    LeftJoin(
+                        SourcePackageName,
+                        BugTask.sourcepackagename == SourcePackageName.id),
+                    ]
+                # NB: these may work with AutoTables, but its hard to tell,
+                # this way is known to work.
+                if BugNomination in tables:
+                    # The relation is already in query.
+                    origin.append(BugNomination)
+                if BugSubscription in tables:
+                    # The relation is already in query.
+                    origin.append(BugSubscription)
+                if SourcePackageRelease in tables:
+                    origin.append(SourcePackageRelease)
+                if SourcePackagePublishingHistory in tables:
+                    origin.append(SourcePackagePublishingHistory)
+                resultset = store.using(*origin).find(
+                    (BugTask, Product, SourcePackageName, Bug),
+                    AutoTables(SQL("1=1"), tables),
+                    query)
+                decorator=lambda row:bugtask_decorator(row[0])
+            resultset.order_by(orderby)
+            return DecoratedResultSet(resultset, result_decorator=decorator)
 
         bugtask_fti = SQL('BugTask.fti')
         result = store.find((BugTask, bugtask_fti), query,
                             AutoTables(SQL("1=1"), clauseTables))
+        decorators = [bugtask_decorator]
         for arg in args:
-            query, clauseTables, dummy = self.buildQuery(arg)
+            query, clauseTables, dummy, decorator = self.buildQuery(arg)
             result = result.union(
                 store.find((BugTask, bugtask_fti), query,
                            AutoTables(SQL("1=1"), clauseTables)))
+            # NB: assumes the decorators are all compatible.
+            # This may need revisiting if e.g. searches on behalf of different
+            # users are combined.
+            decorators.append(decorator)
+        def decorator(row):
+            bugtask = row[0]
+            for decorator in decorators:
+                bugtask = decorator(bugtask)
+            return bugtask
 
-        # Build up the joins
-        from lp.bugs.model.bug import Bug
-        from lp.registry.model.product import Product
-        from lp.registry.model.sourcepackagename import SourcePackageName
+        # Build up the joins.
+        # TODO: implement _noprejoins for this code path: as of 20100818 it has
+        # been silently disabled because clients of the API were setting
+        # prejoins=[] which had no effect; this TODO simply notes the reality
+        # already existing when it was added.
         joins = Alias(result._get_select(), "BugTask")
         joins = Join(joins, Bug, BugTask.bug == Bug.id)
         joins = LeftJoin(joins, Product, BugTask.product == Product.id)
@@ -2163,9 +2240,18 @@
 
         result = store.using(joins).find(
             (BugTask, Bug, Product, SourcePackageName))
-        bugtasks = SQLObjectResultSet(BugTask, orderBy=orderby,
-                                      prepared_result_set=result)
-        return bugtasks
+        result.order_by(orderby)
+        return DecoratedResultSet(result, result_decorator=decorator)
+
+    def searchBugIds(self, params):
+        """See `IBugTaskSet`."""
+        query, clauseTables, orderby, decorator = self.buildQuery(
+            params)
+        store = IStore(BugTask)
+        resultset = store.find(BugTask.bugID,
+            AutoTables(SQL("1=1"), clauseTables), query)
+        resultset.order_by(orderby)
+        return resultset
 
     def getAssignedMilestonesFromSearch(self, search_results):
         """See `IBugTaskSet`."""
@@ -2193,7 +2279,14 @@
             # Import here because of cyclic references.
             from lp.registry.model.milestone import (
                 Milestone, milestone_sort_key)
-            milestones = search_results._store.find(
+            # We need the store that was used, we have no objects to key off of
+            # other than the search result, and Store.of() doesn't currently
+            # work on result sets. Additionally it may be a DecoratedResultSet.
+            if zope_isinstance(search_results, DecoratedResultSet):
+                store = removeSecurityProxy(search_results).result_set._store
+            else:
+                store = search_results._store
+            milestones = store.find(
                 Milestone, In(Milestone.id, milestone_ids))
             return sorted(milestones, key=milestone_sort_key, reverse=True)
 
@@ -2514,14 +2607,40 @@
 
     def getOrderByColumnDBName(self, col_name):
         """See `IBugTaskSet`."""
-        return self._ORDERBY_COLUMN[col_name]
+        if BugTaskSet._ORDERBY_COLUMN is None:
+            # Local import of Bug to avoid import loop.
+            from lp.bugs.model.bug import Bug
+            BugTaskSet._ORDERBY_COLUMN = {
+                "id": BugTask.bugID,
+                "importance": BugTask.importance,
+                # TODO: sort by their name?
+                "assignee": BugTask.assigneeID,
+                "targetname": BugTask.targetnamecache,
+                "status": BugTask.status,
+                "title": Bug.title,
+                "milestone": BugTask.milestoneID,
+                "dateassigned": BugTask.date_assigned,
+                "datecreated": BugTask.datecreated,
+                "date_last_updated": Bug.date_last_updated,
+                "date_closed": BugTask.date_closed,
+                "number_of_duplicates": Bug.number_of_duplicates,
+                "message_count": Bug.message_count,
+                "users_affected_count": Bug.users_affected_count,
+                "heat": Bug.heat,
+                "latest_patch_uploaded": Bug.latest_patch_uploaded,
+                }
+        return BugTaskSet._ORDERBY_COLUMN[col_name]
 
     def _processOrderBy(self, params):
         """Process the orderby parameter supplied to search().
 
         This method ensures the sort order will be stable, and converting
         the string supplied to actual column names.
+
+        :return: A Storm order_by tuple.
         """
+        # Local import of Bug to avoid import loop.
+        from lp.bugs.model.bug import Bug
         orderby = params.orderby
         if orderby is None:
             orderby = []
@@ -2536,10 +2655,10 @@
         # columns to make the sort consistent over runs -- which is good
         # for the user and essential for the test suite.
         unambiguous_cols = set([
-            "BugTask.dateassigned",
-            "BugTask.datecreated",
-            "Bug.datecreated",
-            "Bug.date_last_updated"])
+            BugTask.date_assigned,
+            BugTask.datecreated,
+            Bug.datecreated,
+            Bug.date_last_updated])
         # Bug ID is unique within bugs on a product or source package.
         if (params.product or
             (params.distribution and params.sourcepackagename) or
@@ -2549,7 +2668,7 @@
             in_unique_context = False
 
         if in_unique_context:
-            unambiguous_cols.add("BugTask.bug")
+            unambiguous_cols.add(BugTask.bug)
 
         # Translate orderby keys into corresponding Table.attribute
         # strings.
@@ -2559,22 +2678,22 @@
                 orderby_arg.append(orderby_col)
                 continue
             if orderby_col.startswith("-"):
-                col_name = self.getOrderByColumnDBName(orderby_col[1:])
-                order_clause = "-" + col_name
+                col = self.getOrderByColumnDBName(orderby_col[1:])
+                order_clause = Desc(col)
             else:
-                col_name = self.getOrderByColumnDBName(orderby_col)
-                order_clause = col_name
-            if col_name in unambiguous_cols:
+                col = self.getOrderByColumnDBName(orderby_col)
+                order_clause = col
+            if col in unambiguous_cols:
                 ambiguous = False
             orderby_arg.append(order_clause)
 
         if ambiguous:
             if in_unique_context:
-                orderby_arg.append('BugTask.bug')
+                orderby_arg.append(BugTask.bugID)
             else:
-                orderby_arg.append('BugTask.id')
+                orderby_arg.append(BugTask.id)
 
-        return orderby_arg
+        return tuple(orderby_arg)
 
     def dangerousGetAllTasks(self):
         """DO NOT USE THIS METHOD. For details, see `IBugTaskSet`"""

=== modified file 'lib/lp/bugs/stories/bug-privacy/40-unsubscribe-from-private-bug.txt'
--- lib/lp/bugs/stories/bug-privacy/40-unsubscribe-from-private-bug.txt	2009-08-31 15:41:15 +0000
+++ lib/lp/bugs/stories/bug-privacy/40-unsubscribe-from-private-bug.txt	2010-08-22 18:34:46 +0000
@@ -11,10 +11,10 @@
     >>> launchbag = getUtility(ILaunchBag)
     >>> evo = getUtility(ISourcePackageNameSet).queryByName("evolution")
     >>> params = BugTaskSearchParams(user=launchbag.user,
-    ...     sourcepackagename=evo, orderby="id")
+    ...     sourcepackagename=evo, orderby="-id")
 
     >>> ubuntu = getUtility(IDistributionSet).getByName("ubuntu")
-    >>> latest_evo_task = ubuntu.searchTasks(params)[-1]
+    >>> latest_evo_task = ubuntu.searchTasks(params)[0]
     >>> latest_evo_bug = latest_evo_task.bug.id
     >>> logout()
 

=== modified file 'lib/lp/bugs/tests/test_bugtask.py'
--- lib/lp/bugs/tests/test_bugtask.py	2010-08-20 20:31:18 +0000
+++ lib/lp/bugs/tests/test_bugtask.py	2010-08-22 18:34:46 +0000
@@ -25,6 +25,7 @@
     BugTaskImportance,
     BugTaskSearchParams,
     BugTaskStatus,
+    IBugTaskSet,
     )
 from lp.bugs.model.bugtask import build_tag_search_clause
 from lp.hardwaredb.interfaces.hwdb import (
@@ -32,7 +33,7 @@
     IHWDeviceSet,
     )
 from lp.registry.interfaces.distribution import IDistributionSet
-from lp.registry.interfaces.person import IPersonSet
+from lp.registry.interfaces.person import IPerson, IPersonSet
 from lp.testing import (
     ANONYMOUS,
     login,
@@ -807,6 +808,28 @@
         result = target.searchTasks(None, modified_since=date)
         self.assertEqual([task2], list(result))
 
+    def test_private_bug_view_permissions_cached(self):
+        """Private bugs from a search know the user can see the bugs."""
+        target = self.makeBugTarget()
+        person = self.login()
+        self.factory.makeBug(product=target, private=True, owner=person)
+        self.factory.makeBug(product=target, private=True, owner=person)
+        # Search style and parameters taken from the milestone index view where
+        # the issue was discovered.
+        login_person(person)
+        tasks = target.searchTasks(BugTaskSearchParams(person, omit_dupes=True,
+            orderby=['status', '-importance', 'id']))
+        # We must be finding the bugs.
+        self.assertEqual(2, tasks.count())
+        # Cache in the storm cache the account->person lookup so its not
+        # distorting what we're testing.
+        _ = IPerson(person.account, None)
+        # One query and only one should be issued to get the tasks, bugs and
+        # allow access to getConjoinedMaster attribute - an attribute that
+        # triggers a permission check (nb: id does not trigger such a check)
+        self.assertStatementCount(1,
+            lambda:[task.getConjoinedMaster for task in tasks])
+
 
 def test_suite():
     suite = unittest.TestSuite()

=== modified file 'lib/lp/registry/browser/tests/test_milestone.py'
--- lib/lp/registry/browser/tests/test_milestone.py	2010-08-20 20:31:18 +0000
+++ lib/lp/registry/browser/tests/test_milestone.py	2010-08-22 18:34:46 +0000
@@ -1,24 +1,30 @@
 # Copyright 2010 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
+from __future__ import with_statement
+
 """Test milestone views."""
 
 __metaclass__ = type
 
-import unittest
-
+from testtools.matchers import LessThan
 from zope.component import getUtility
 
+from canonical.launchpad.webapp import canonical_url
 from canonical.testing.layers import DatabaseFunctionalLayer
 from lp.bugs.interfaces.bugtask import IBugTaskSet
 from lp.testing import (
     ANONYMOUS,
     login,
     login_person,
+    logout,
+    person_logged_in, 
     TestCaseWithFactory,
     )
+from lp.testing.matchers import HasQueryCount
 from lp.testing.memcache import MemcacheTestCase
 from lp.testing.views import create_initialized_view
+from lp.testing._webservice import QueryCollector
 
 
 class TestMilestoneViews(TestCaseWithFactory):
@@ -158,5 +164,67 @@
         self.assertEqual(0, product.development_focus.all_bugtasks.count())
 
 
-def test_suite():
-    return unittest.TestLoader().loadTestsFromName(__name__)
+class TestMilestoneIndex(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def test_more_private_bugs_query_count_is_constant(self):
+        # This test tests that as we add more private bugs to a milestone index
+        # page, the number of queries issued by the page does not change.
+        # It also sets a cap on the queries for this page: if the baseline 
+        # were to increase, the test would fail. As the baseline is very large
+        # already, if the test fails due to such a change, please cut some more
+        # of the existing fat out of it rather than increasing the cap.
+        product = self.factory.makeProduct()
+        login_person(product.owner)
+        milestone = self.factory.makeMilestone(
+            productseries=product.development_focus)
+        bug1 = self.factory.makeBug(product=product, private=True,
+            owner=product.owner)
+        bug1.bugtasks[0].transitionToMilestone(milestone, product.owner)
+        # We look at the page as someone who is a member of a team and the team
+        # is subscribed to the bugs, so that we don't get trivial shortcuts
+        # avoiding queries : test the worst case.
+        subscribed_team = self.factory.makeTeam()
+        viewer = self.factory.makePerson(password="test")
+        with person_logged_in(subscribed_team.teamowner):
+            subscribed_team.addMember(viewer, subscribed_team.teamowner)
+        bug1.subscribe(subscribed_team, product.owner)
+        bug1_url = canonical_url(bug1)
+        milestone_url = canonical_url(milestone)
+        browser = self.getUserBrowser(user=viewer)
+        # Seed the cookie cache and any other cross-request state we may gain
+        # in future.  See canonical.launchpad.webapp.serssion: _get_secret.
+        browser.open(milestone_url)
+        collector = QueryCollector()
+        collector.register()
+        self.addCleanup(collector.unregister)
+        # This page is rather high in queries : 46 as a baseline. 20100817
+        page_query_limit = 46
+        browser.open(milestone_url)
+        # Check that the test found the bug
+        self.assertTrue(bug1_url in browser.contents)
+        self.assertThat(collector, HasQueryCount(LessThan(page_query_limit)))
+        with_1_private_bug = collector.count
+        with_1_queries = ["%s: %s" % (pos, stmt[3]) for (pos, stmt) in 
+            enumerate(collector.queries)]
+        login_person(product.owner)
+        bug2 = self.factory.makeBug(product=product, private=True,
+            owner=product.owner)
+        bug2.bugtasks[0].transitionToMilestone(milestone, product.owner)
+        bug2.subscribe(subscribed_team, product.owner)
+        bug2_url = canonical_url(bug2)
+        bug3 = self.factory.makeBug(product=product, private=True,
+            owner=product.owner)
+        bug3.bugtasks[0].transitionToMilestone(milestone, product.owner)
+        bug3.subscribe(subscribed_team, product.owner)
+        logout()
+        browser.open(milestone_url)
+        self.assertTrue(bug2_url in browser.contents)
+        self.assertThat(collector, HasQueryCount(LessThan(page_query_limit)))
+        with_3_private_bugs = collector.count
+        with_3_queries = ["%s: %s" % (pos, stmt[3]) for (pos, stmt) in 
+            enumerate(collector.queries)]
+        self.assertEqual(with_1_private_bug, with_3_private_bugs,
+            "different query count: \n%s\n******************\n%s\n" % (
+            '\n'.join(with_1_queries), '\n'.join(with_3_queries)))

=== modified file 'lib/lp/registry/browser/tests/test_views.py'
--- lib/lp/registry/browser/tests/test_views.py	2010-08-20 20:31:18 +0000
+++ lib/lp/registry/browser/tests/test_views.py	2010-08-22 18:34:46 +0000
@@ -35,6 +35,7 @@
     'product-edit-people-view.txt': LaunchpadFunctionalLayer,
     'product-files-views.txt': LaunchpadFunctionalLayer,
     'product-views.txt': LaunchpadFunctionalLayer,
+    'productseries-views.txt': LaunchpadFunctionalLayer,
     'projectgroup-views.txt': LaunchpadFunctionalLayer,
     'user-to-user-views.txt': LaunchpadFunctionalLayer,
 }

=== modified file 'lib/lp/registry/doc/distribution.txt'
--- lib/lp/registry/doc/distribution.txt	2010-08-06 14:49:35 +0000
+++ lib/lp/registry/doc/distribution.txt	2010-08-22 18:34:46 +0000
@@ -585,7 +585,7 @@
     ...     name='impossible', dateexpected=datetime(2028, 10, 1))
     Traceback (most recent call last):
     ...
-    Unauthorized: (<DistroSeries at ...>, 'newMilestone', 'launchpad.Edit')
+    Unauthorized: (<DistroSeries u'woody'>, 'newMilestone', 'launchpad.Edit')
     >>> login('[e-mail]')
     >>> debian_milestone = woody.newMilestone(
     ...     name='woody-rc1', dateexpected=datetime(2028, 10, 1))

=== modified file 'lib/lp/registry/doc/sourcepackage.txt'
--- lib/lp/registry/doc/sourcepackage.txt	2010-07-02 20:32:58 +0000
+++ lib/lp/registry/doc/sourcepackage.txt	2010-08-22 18:34:46 +0000
@@ -71,7 +71,7 @@
     >>> firefox_warty_package = SourcePackage(sourcepackagename=firefox,
     ...     distroseries=ubuntu_warty)
     >>> firefox_warty_package
-    <SourcePackage ubuntu/warty/mozilla-firefox>
+    <SourcePackage ...'Ubuntu'...'warty'...'mozilla-firefox'...>
 
 An instance is commonly retrieved from a distroseries.
 

=== modified file 'lib/lp/registry/model/distroseries.py'
--- lib/lp/registry/model/distroseries.py	2010-08-20 20:31:18 +0000
+++ lib/lp/registry/model/distroseries.py	2010-08-22 18:34:46 +0000
@@ -270,6 +270,9 @@
         """See `IDistroSeries`."""
         return self.getDistroArchSeries(archtag)
 
+    def __repr__(self):
+        return '<%s %r>' % (self.__class__.__name__, self.name)
+
     def __str__(self):
         return '%s %s' % (self.distribution.name, self.name)
 

=== modified file 'lib/lp/registry/model/sourcepackage.py'
--- lib/lp/registry/model/sourcepackage.py	2010-08-20 20:31:18 +0000
+++ lib/lp/registry/model/sourcepackage.py	2010-08-22 18:34:46 +0000
@@ -213,7 +213,8 @@
         return cls(sourcepackagename, distroseries)
 
     def __repr__(self):
-        return '<%s %s>' % (self.__class__.__name__, self.path)
+        return '<%s %r %r %r>' % (self.__class__.__name__,
+            self.distribution, self.distroseries, self.sourcepackagename)
 
     def _get_ubuntu(self):
         # XXX: kiko 2006-03-20: Ideally, it would be possible to just do

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2010-08-20 20:31:18 +0000
+++ lib/lp/testing/factory.py	2010-08-22 18:34:46 +0000
@@ -7,7 +7,7 @@
 
 """Testing infrastructure for the Launchpad application.
 
-This module should not have any actual tests.
+This module should not contain tests (but it should be tested).
 """
 
 __metaclass__ = type