launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/do-not-snapshot-person-ppas into lp:launchpad.

Commit message:
Don't snapshot Person.ppas; in some cases it is very large.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1453786 in Launchpad itself: "Can't add another member to the team, ShortListTooBigError"
  https://bugs.launchpad.net/launchpad/+bug/1453786

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/do-not-snapshot-person-ppas/+merge/258765

Don't snapshot Person.ppas; in some cases it is very large.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/do-not-snapshot-person-ppas into lp:launchpad.

=== modified file 'lib/lp/registry/browser/tests/test_person_webservice.py'
--- lib/lp/registry/browser/tests/test_person_webservice.py	2015-03-10 11:38:21 +0000
+++ lib/lp/registry/browser/tests/test_person_webservice.py	2015-05-11 13:21:57 +0000
@@ -16,6 +16,7 @@
 from lp.registry.interfaces.teammembership import ITeamMembershipSet
 from lp.services.identity.interfaces.account import AccountStatus
 from lp.services.openid.model.openididentifier import OpenIdIdentifier
+from lp.services.webapp import snapshot
 from lp.services.webapp.interfaces import OAuthPermission
 from lp.testing import (
     admin_logged_in,
@@ -23,6 +24,7 @@
     launchpadlib_for,
     login,
     logout,
+    person_logged_in,
     record_two_runs,
     TestCaseWithFactory,
     )
@@ -179,6 +181,30 @@
             get_members, create_member, 2)
         self.assertThat(recorder2, HasQueryCount(Equals(recorder1.count)))
 
+    def test_many_ppas(self):
+        # POSTing to a person with many PPAs doesn't OOPS.
+        with admin_logged_in():
+            team = self.factory.makeTeam()
+            owner = team.teamowner
+        new_member = self.factory.makePerson()
+        ws = webservice_for_person(
+            owner, permission=OAuthPermission.WRITE_PUBLIC)
+        real_hard_limit_for_snapshot = snapshot.HARD_LIMIT_FOR_SNAPSHOT
+        snapshot.HARD_LIMIT_FOR_SNAPSHOT = 3
+        try:
+            with person_logged_in(owner):
+                for count in range(snapshot.HARD_LIMIT_FOR_SNAPSHOT + 1):
+                    self.factory.makeArchive(owner=team)
+                team_url = api_url(team)
+                new_member_url = api_url(new_member)
+            response = ws.named_post(
+                team_url, 'addMember', person=new_member_url,
+                status='Approved')
+            self.assertEqual(200, response.status)
+            self.assertEqual([True, 'Approved'], response.jsonBody())
+        finally:
+            snapshot.HARD_LIMIT_FOR_SNAPSHOT = real_hard_limit_for_snapshot
+
 
 class PersonSetWebServiceTests(TestCaseWithFactory):
 

=== modified file 'lib/lp/registry/interfaces/person.py'
--- lib/lp/registry/interfaces/person.py	2015-05-04 14:56:58 +0000
+++ lib/lp/registry/interfaces/person.py	2015-05-11 13:21:57 +0000
@@ -914,14 +914,14 @@
             # Really IArchive, see archive.py
             schema=Interface))
 
-    ppas = exported(
+    ppas = exported(doNotSnapshot(
         CollectionField(
             title=_("PPAs for this person."),
             description=_(
                 "PPAs owned by the context person ordered by name."),
             readonly=True, required=False,
             # Really IArchive, see archive.py
-            value_type=Reference(schema=Interface)))
+            value_type=Reference(schema=Interface))))
 
     structural_subscriptions = Attribute(
         "The structural subscriptions for this person.")

=== modified file 'lib/lp/registry/tests/test_person.py'
--- lib/lp/registry/tests/test_person.py	2015-02-26 11:34:47 +0000
+++ lib/lp/registry/tests/test_person.py	2015-05-11 13:21:57 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2013 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -962,7 +962,7 @@
             'api_all_members', 'approvedmembers',
             'deactivatedmembers', 'expiredmembers', 'inactivemembers',
             'invited_members', 'member_memberships', 'pendingmembers',
-            'proposedmembers', 'time_zone',
+            'ppas', 'proposedmembers', 'time_zone',
             )
         snap = Snapshot(self.myteam, providing=providedBy(self.myteam))
         for name in omitted: