launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/bug-1578205 into lp:launchpad.

Commit message:
Sort GitRepositorySet.getRepositories API results to make batching reliable.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1578205 in Launchpad itself: "API returns git repository list with missing items and duplicates"
  https://bugs.launchpad.net/launchpad/+bug/1578205

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/bug-1578205/+merge/293857

Sort GitRepositorySet.getRepositories API results to make batching reliable.

Also add indexes for this new sort order, and while we're at it recreate the target/owner_default indexes to be unique and correctly partial.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/bug-1578205 into lp:launchpad.

=== added file 'database/schema/patch-2209-77-1.sql'
--- database/schema/patch-2209-77-1.sql	1970-01-01 00:00:00 +0000
+++ database/schema/patch-2209-77-1.sql	2016-05-05 07:56:03 +0000
@@ -0,0 +1,42 @@
+-- Copyright 2016 Canonical Ltd.  This software is licensed under the
+-- GNU Affero General Public License version 3 (see the file LICENSE).
+
+SET client_min_messages=ERROR;
+
+-- Add ID sort indexes.
+CREATE INDEX gitrepository__distribution__spn__id__idx
+    ON gitrepository(distribution, sourcepackagename, id)
+    WHERE distribution IS NOT NULL;
+CREATE INDEX gitrepository__owner__distribution__spn__id__idx
+    ON gitrepository(owner, distribution, sourcepackagename, id)
+    WHERE distribution IS NOT NULL;
+CREATE INDEX gitrepository__project__id__idx
+    ON gitrepository(project, id)
+    WHERE project IS NOT NULL;
+CREATE INDEX gitrepository__owner__project__id__idx
+    ON gitrepository(owner, project, id)
+    WHERE project IS NOT NULL;
+CREATE INDEX gitrepository__owner__id__idx
+    ON gitrepository(owner, id);
+
+-- Replace owner/target_default indexes with unique and partial ones.
+CREATE UNIQUE INDEX gitrepository__distribution__spn__target_default__key
+    ON gitrepository(distribution, sourcepackagename)
+    WHERE distribution IS NOT NULL AND target_default;
+DROP INDEX gitrepository__distribution__spn__target_default__idx;
+CREATE UNIQUE INDEX gitrepository__owner__distribution__spn__owner_default__key
+    ON gitrepository(owner, distribution, sourcepackagename)
+    WHERE distribution IS NOT NULL AND owner_default;
+DROP INDEX gitrepository__owner__distribution__spn__owner_default__idx;
+
+CREATE UNIQUE INDEX gitrepository__project__target_default__key
+    ON gitrepository(project)
+    WHERE project IS NOT NULL AND target_default;
+DROP INDEX gitrepository__project__target_default__idx;
+CREATE UNIQUE INDEX gitrepository__owner__project__owner_default__key
+    ON gitrepository(owner, project)
+    WHERE project IS NOT NULL AND owner_default;
+DROP INDEX gitrepository__owner__project__owner_default__idx;
+
+
+INSERT INTO LaunchpadDatabaseRevision VALUES (2209, 77, 1);

=== modified file 'lib/lp/code/interfaces/gitcollection.py'
--- lib/lp/code/interfaces/gitcollection.py	2015-10-12 12:58:32 +0000
+++ lib/lp/code/interfaces/gitcollection.py	2016-05-05 07:56:03 +0000
@@ -1,4 +1,4 @@
-# Copyright 2015 Canonical Ltd.  This software is licensed under the
+# Copyright 2015-2016 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """A collection of Git repositories.
@@ -54,7 +54,8 @@
             collection.
         """
 
-    def getRepositories(eager_load=False, order_by_date=False):
+    def getRepositories(eager_load=False, order_by_date=False,
+                        order_by_id=False):
         """Return a result set of all repositories in this collection.
 
         The returned result set will also join across the specified tables
@@ -66,6 +67,7 @@
             objects in the collection.
         :param order_by_date: If True, order results by descending
             modification date.
+        :param order_by_id: If True, order results by ascending ID.
         """
 
     def getRepositoryIds():

=== modified file 'lib/lp/code/model/gitcollection.py'
--- lib/lp/code/model/gitcollection.py	2015-10-12 12:58:32 +0000
+++ lib/lp/code/model/gitcollection.py	2016-05-05 07:56:03 +0000
@@ -1,4 +1,4 @@
-# Copyright 2015 Canonical Ltd.  This software is licensed under the
+# Copyright 2015-2016 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Implementations of `IGitCollection`."""
@@ -199,15 +199,18 @@
         load_related(Product, repositories, ['project_id'])
 
     def getRepositories(self, find_expr=GitRepository, eager_load=False,
-                        order_by_date=False):
+                        order_by_date=False, order_by_id=False):
         """See `IGitCollection`."""
         all_tables = set(
             self._tables.values() + self._asymmetric_tables.values())
         tables = [GitRepository] + list(all_tables)
         expressions = self._getRepositoryExpressions()
         resultset = self.store.using(*tables).find(find_expr, *expressions)
+        assert not order_by_date or not order_by_id
         if order_by_date:
             resultset.order_by(Desc(GitRepository.date_last_modified))
+        elif order_by_id:
+            resultset.order_by(GitRepository.id)
 
         def do_eager_load(rows):
             repository_ids = set(repository.id for repository in rows)

=== modified file 'lib/lp/code/model/gitrepository.py'
--- lib/lp/code/model/gitrepository.py	2016-04-29 15:17:20 +0000
+++ lib/lp/code/model/gitrepository.py	2016-05-05 07:56:03 +0000
@@ -1221,7 +1221,7 @@
     def getRepositories(self, user, target):
         """See `IGitRepositorySet`."""
         collection = IGitCollection(target).visibleByUser(user)
-        return collection.getRepositories(eager_load=True)
+        return collection.getRepositories(eager_load=True, order_by_id=True)
 
     def getRepositoryVisibilityInfo(self, user, person, repository_names):
         """See `IGitRepositorySet`."""

=== modified file 'lib/lp/code/model/tests/test_gitrepository.py'
--- lib/lp/code/model/tests/test_gitrepository.py	2016-05-03 15:12:46 +0000
+++ lib/lp/code/model/tests/test_gitrepository.py	2016-05-05 07:56:03 +0000
@@ -102,6 +102,7 @@
     IAccessPolicyArtifactSource,
     IAccessPolicySource,
     )
+from lp.registry.interfaces.person import IPerson
 from lp.registry.interfaces.persondistributionsourcepackage import (
     IPersonDistributionSourcePackageFactory,
     )
@@ -125,6 +126,7 @@
     celebrity_logged_in,
     login_person,
     person_logged_in,
+    record_two_runs,
     TestCaseWithFactory,
     verifyObject,
     )
@@ -138,7 +140,10 @@
     ZopelessDatabaseLayer,
     )
 from lp.testing.mail_helpers import pop_notifications
-from lp.testing.matchers import DoesNotSnapshot
+from lp.testing.matchers import (
+    DoesNotSnapshot,
+    HasQueryCount,
+    )
 from lp.testing.pages import webservice_for_person
 
 
@@ -2443,56 +2448,51 @@
             "git+ssh://git.launchpad.dev/~person/project/+git/repository",
             repository["git_ssh_url"])
 
+    def assertGetRepositoriesWorks(self, target_db):
+        if IPerson.providedBy(target_db):
+            owner_db = target_db
+        else:
+            owner_db = self.factory.makePerson()
+        owner_url = api_url(owner_db)
+        target_url = api_url(target_db)
+
+        repos_db = []
+        repos_url = []
+
+        webservice = webservice_for_person(
+            owner_db, permission=OAuthPermission.WRITE_PUBLIC)
+        webservice.default_api_version = "devel"
+
+        def create_repository():
+            with admin_logged_in():
+                repo = self.factory.makeGitRepository(
+                    target=target_db, owner=owner_db)
+                repos_db.append(repo)
+                repos_url.append(api_url(repo))
+
+        def verify_getRepositories():
+            response = webservice.named_get(
+                "/+git", "getRepositories", user=owner_url, target=target_url)
+            self.assertEqual(200, response.status)
+            self.assertEqual(
+                [webservice.getAbsoluteUrl(url) for url in repos_url],
+                [entry["self_link"]
+                 for entry in response.jsonBody()["entries"]])
+
+        verify_getRepositories()
+        recorder1, recorder2 = record_two_runs(
+            verify_getRepositories, create_repository, 2)
+        self.assertThat(recorder2, HasQueryCount.byEquality(recorder1))
+
     def test_getRepositories_project(self):
-        project_db = self.factory.makeProduct()
-        repository_db = self.factory.makeGitRepository(target=project_db)
-        webservice = webservice_for_person(
-            repository_db.owner, permission=OAuthPermission.WRITE_PUBLIC)
-        webservice.default_api_version = "devel"
-        with person_logged_in(ANONYMOUS):
-            repository_url = api_url(repository_db)
-            owner_url = api_url(repository_db.owner)
-            project_url = api_url(project_db)
-        response = webservice.named_get(
-            "/+git", "getRepositories", user=owner_url, target=project_url)
-        self.assertEqual(200, response.status)
-        self.assertEqual(
-            [webservice.getAbsoluteUrl(repository_url)],
-            [entry["self_link"] for entry in response.jsonBody()["entries"]])
+        self.assertGetRepositoriesWorks(self.factory.makeProduct())
 
     def test_getRepositories_package(self):
-        dsp_db = self.factory.makeDistributionSourcePackage()
-        repository_db = self.factory.makeGitRepository(target=dsp_db)
-        webservice = webservice_for_person(
-            repository_db.owner, permission=OAuthPermission.WRITE_PUBLIC)
-        webservice.default_api_version = "devel"
-        with person_logged_in(ANONYMOUS):
-            repository_url = api_url(repository_db)
-            owner_url = api_url(repository_db.owner)
-            dsp_url = api_url(dsp_db)
-        response = webservice.named_get(
-            "/+git", "getRepositories", user=owner_url, target=dsp_url)
-        self.assertEqual(200, response.status)
-        self.assertEqual(
-            [webservice.getAbsoluteUrl(repository_url)],
-            [entry["self_link"] for entry in response.jsonBody()["entries"]])
+        self.assertGetRepositoriesWorks(
+            self.factory.makeDistributionSourcePackage())
 
     def test_getRepositories_personal(self):
-        owner_db = self.factory.makePerson()
-        repository_db = self.factory.makeGitRepository(
-            owner=owner_db, target=owner_db)
-        webservice = webservice_for_person(
-            owner_db, permission=OAuthPermission.WRITE_PUBLIC)
-        webservice.default_api_version = "devel"
-        with person_logged_in(ANONYMOUS):
-            repository_url = api_url(repository_db)
-            owner_url = api_url(owner_db)
-        response = webservice.named_get(
-            "/+git", "getRepositories", user=owner_url, target=owner_url)
-        self.assertEqual(200, response.status)
-        self.assertEqual(
-            [webservice.getAbsoluteUrl(repository_url)],
-            [entry["self_link"] for entry in response.jsonBody()["entries"]])
+        self.assertGetRepositoriesWorks(self.factory.makePerson())
 
     def test_set_information_type(self):
         # The repository owner can change the information type.