Merge lp:~wgrant/launchpad/bug-1578205 into lp:launchpad

[William Grant]

Sort GitRepositorySet.getRepositories API results to make batching reliable.

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.

[Colin Watson (cjwatson)]

review: Approve

=== 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.