launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/snap-find-by-url-prefixes into lp:launchpad.

Commit message:
Add SnapSet.findByURLPrefixes.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/snap-find-by-url-prefixes/+merge/323035

I have my reservations about this from the point of view of ever-growing API surface, and I probably made a mistake when adding SnapSet.findByURLPrefix before, but I think we need something like this for build.snapcraft.io in order to support organisations.  We're searching for snaps relevant to a user by URL prefix, so to avoid needing to make O(organisations the user belongs to) webservice queries, we need a bulk version.

(What we actually want is "all snaps for GitHub repositories where the user signed into BSI has write access"; but we have no reasonable way to get at that information from LP at the moment.)
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/snap-find-by-url-prefixes into lp:launchpad.

=== modified file 'lib/lp/snappy/interfaces/snap.py'
--- lib/lp/snappy/interfaces/snap.py	2017-03-30 14:08:55 +0000
+++ lib/lp/snappy/interfaces/snap.py	2017-04-24 13:50:42 +0000
@@ -708,6 +708,28 @@
             this user; otherwise, only return publicly-visible packages.
         """
 
+    @operation_parameters(
+        url_prefixes=List(
+            title=_("The URL prefixes to search for."), value_type=TextLine()),
+        owner=Reference(IPerson, title=_("Owner"), required=False))
+    @call_with(visible_by_user=REQUEST_USER)
+    @operation_returns_collection_of(ISnap)
+    @export_read_operation()
+    @operation_for_version("devel")
+    def findByURLPrefixes(url_prefixes, owner=None, visible_by_user=None):
+        """Return all snap packages that build from a URL with any of these
+        prefixes.
+
+        This currently only works for packages that build directly from a
+        URL, rather than being linked to a Bazaar branch or Git repository
+        hosted in Launchpad.
+
+        :param url_prefixes: A list of URL prefixes.
+        :param owner: Only return packages owned by this user.
+        :param visible_by_user: If not None, only return packages visible by
+            this user; otherwise, only return publicly-visible packages.
+        """
+
     def preloadDataForSnaps(snaps, user):
         """Load the data related to a list of snap packages."""
 

=== modified file 'lib/lp/snappy/model/snap.py'
--- lib/lp/snappy/model/snap.py	2017-03-28 16:19:36 +0000
+++ lib/lp/snappy/model/snap.py	2017-04-24 13:50:42 +0000
@@ -845,7 +845,16 @@
 
     def findByURLPrefix(self, url_prefix, owner=None, visible_by_user=None):
         """See `ISnapSet`."""
-        clauses = [Snap.git_repository_url.startswith(url_prefix)]
+        return self.findByURLPrefixes(
+            [url_prefix], owner=owner, visible_by_user=visible_by_user)
+
+    def findByURLPrefixes(self, url_prefixes, owner=None,
+                          visible_by_user=None):
+        """See `ISnapSet`."""
+        prefix_clauses = [
+            Snap.git_repository_url.startswith(url_prefix)
+            for url_prefix in url_prefixes]
+        clauses = [Or(*prefix_clauses)]
         if owner is not None:
             clauses.append(Snap.owner == owner)
         clauses.append(self._findByURLVisibilityClause(visible_by_user))

=== modified file 'lib/lp/snappy/tests/test_snap.py'
--- lib/lp/snappy/tests/test_snap.py	2017-03-28 16:19:36 +0000
+++ lib/lp/snappy/tests/test_snap.py	2017-04-24 13:50:42 +0000
@@ -882,6 +882,35 @@
             [snaps[0], snaps[2]],
             getUtility(ISnapSet).findByURLPrefix(prefix, owner=owners[0]))
 
+    def test_findByURLPrefixes(self):
+        # ISnapSet.findByURLPrefixes returns visible Snaps with any of the
+        # given URL prefixes.
+        urls = [
+            u"https://git.example.org/foo/a";,
+            u"https://git.example.org/foo/b";,
+            u"https://git.example.org/bar/a";,
+            u"https://git.example.org/bar/b";,
+            u"https://git.example.org/baz";,
+            ]
+        owners = [self.factory.makePerson() for i in range(2)]
+        snaps = []
+        for url in urls:
+            for owner in owners:
+                snaps.append(self.factory.makeSnap(
+                    registrant=owner, owner=owner,
+                    git_ref=self.factory.makeGitRefRemote(repository_url=url)))
+        snaps.append(
+            self.factory.makeSnap(branch=self.factory.makeAnyBranch()))
+        snaps.append(
+            self.factory.makeSnap(git_ref=self.factory.makeGitRefs()[0]))
+        prefixes = [
+            u"https://git.example.org/foo/";, u"https://git.example.org/bar/";]
+        self.assertContentEqual(
+            snaps[:8], getUtility(ISnapSet).findByURLPrefixes(prefixes))
+        self.assertContentEqual(
+            [snaps[0], snaps[2], snaps[4], snaps[6]],
+            getUtility(ISnapSet).findByURLPrefixes(prefixes, owner=owners[0]))
+
     def test__findStaleSnaps(self):
         # Stale; not built automatically.
         self.factory.makeSnap(is_stale=True)
@@ -1502,6 +1531,88 @@
             [ws_snaps[i] for i in (0, 1, 4, 5)],
             [entry["self_link"] for entry in response.jsonBody()["entries"]])
 
+    def test_findByURLPrefixes(self):
+        # lp.snaps.findByURLPrefixes returns visible Snaps with any of the
+        # given URL prefixes.
+        self.pushConfig("launchpad", default_batch_size=20)
+        persons = [self.factory.makePerson(), self.factory.makePerson()]
+        urls = [
+            u"https://git.example.org/foo/a";,
+            u"https://git.example.org/foo/b";,
+            u"https://git.example.org/bar/a";,
+            u"https://git.example.org/bar/b";,
+            u"https://git.example.org/baz";,
+            ]
+        snaps = []
+        for url in urls:
+            for person in persons:
+                for private in (False, True):
+                    ref = self.factory.makeGitRefRemote(repository_url=url)
+                    snaps.append(self.factory.makeSnap(
+                        registrant=person, owner=person, git_ref=ref,
+                        private=private))
+        with admin_logged_in():
+            person_urls = [api_url(person) for person in persons]
+            ws_snaps = [
+                self.webservice.getAbsoluteUrl(api_url(snap))
+                for snap in snaps]
+        commercial_admin = (
+            getUtility(ILaunchpadCelebrities).commercial_admin.teamowner)
+        logout()
+        prefixes = [
+            u"https://git.example.org/foo/";, u"https://git.example.org/bar/";]
+        # Anonymous requests can only see public snaps.
+        anon_webservice = LaunchpadWebServiceCaller("test", "")
+        response = anon_webservice.named_get(
+            "/+snaps", "findByURLPrefixes", url_prefixes=prefixes,
+            api_version="devel")
+        self.assertEqual(200, response.status)
+        self.assertContentEqual(
+            [ws_snaps[i] for i in (0, 2, 4, 6, 8, 10, 12, 14)],
+            [entry["self_link"] for entry in response.jsonBody()["entries"]])
+        response = anon_webservice.named_get(
+            "/+snaps", "findByURLPrefixes", url_prefixes=prefixes,
+            owner=person_urls[0], api_version="devel")
+        self.assertEqual(200, response.status)
+        self.assertContentEqual(
+            [ws_snaps[i] for i in (0, 4, 8, 12)],
+            [entry["self_link"] for entry in response.jsonBody()["entries"]])
+        # persons[0] can see all public snaps with any of these URL
+        # prefixes, as well as their own matching private snaps.
+        webservice = webservice_for_person(
+            persons[0], permission=OAuthPermission.READ_PRIVATE)
+        response = webservice.named_get(
+            "/+snaps", "findByURLPrefixes", url_prefixes=prefixes,
+            api_version="devel")
+        self.assertEqual(200, response.status)
+        self.assertContentEqual(
+            [ws_snaps[i] for i in (0, 1, 2, 4, 5, 6, 8, 9, 10, 12, 13, 14)],
+            [entry["self_link"] for entry in response.jsonBody()["entries"]])
+        response = webservice.named_get(
+            "/+snaps", "findByURLPrefixes", url_prefixes=prefixes,
+            owner=person_urls[0], api_version="devel")
+        self.assertEqual(200, response.status)
+        self.assertContentEqual(
+            [ws_snaps[i] for i in (0, 1, 4, 5, 8, 9, 12, 13)],
+            [entry["self_link"] for entry in response.jsonBody()["entries"]])
+        # Admins can see all snaps with any of these URL prefixes.
+        commercial_admin_webservice = webservice_for_person(
+            commercial_admin, permission=OAuthPermission.READ_PRIVATE)
+        response = commercial_admin_webservice.named_get(
+            "/+snaps", "findByURLPrefixes", url_prefixes=prefixes,
+            api_version="devel")
+        self.assertEqual(200, response.status)
+        self.assertContentEqual(
+            ws_snaps[:16],
+            [entry["self_link"] for entry in response.jsonBody()["entries"]])
+        response = commercial_admin_webservice.named_get(
+            "/+snaps", "findByURLPrefixes", url_prefixes=prefixes,
+            owner=person_urls[0], api_version="devel")
+        self.assertEqual(200, response.status)
+        self.assertContentEqual(
+            [ws_snaps[i] for i in (0, 1, 4, 5, 8, 9, 12, 13)],
+            [entry["self_link"] for entry in response.jsonBody()["entries"]])
+
     def setProcessors(self, user, snap, names):
         ws = webservice_for_person(
             user, permission=OAuthPermission.WRITE_PUBLIC)