Launchpad itself

Merge lp:~jml/launchpad/get-by-urls into lp:launchpad

  1. get-by-urls
  2. Merge into devel
Proposed by Jonathan Lange
Status: Merged
Merged at revision: not available
Proposed branch: lp:~jml/launchpad/get-by-urls
Merge into: lp:launchpad
Diff against target: 242 lines (+113/-11)
7 files modified
lib/canonical/launchpad/pagetests/webservice/xx-branches.txt (+33/-0)
lib/lp/code/interfaces/branch.py (+31/-1)
lib/lp/code/interfaces/branchlookup.py (+8/-0)
lib/lp/code/model/branch.py (+4/-0)
lib/lp/code/model/branchlookup.py (+4/-0)
lib/lp/code/model/tests/test_branchlookup.py (+13/-0)
lib/lp/code/model/tests/test_branchset.py (+20/-10)
To merge this branch: bzr merge lp:~jml/launchpad/get-by-urls
Reviewer Review Type Date Requested Status
Jeroen T. Vermeulen (community) code Approve
Review via email: mp+14773@code.launchpad.net

Commit message

Add a getByUrls method to IBranchSet and IBranchLookup so that API users can query for multiple branches in a single roundtrip.

To post a comment you must log in.
Revision history for this message
Jonathan Lange (jml) wrote :

This branch exposes a convenience API method for getting Bazaar branches based on their URL.

Generally, when you are trying to get the Launchpad object for a Bazaar branch, you cannot be sure which of the many URLs associated with a branch is the correct rule to try. This means you generally want to try multiple URLs. However, this means lots of roundtrips, which means slow code.

The new method getByUrls allows you to query multiple URLs. It returns a list of branch objects in an order that matches the list of URLs. If the branch is not found, None is put in its place.

In an ideal world, it would return a dict mapping URLs to branches, but the API system doesn't support that right now.

Revision history for this message
Jonathan Lange (jml) wrote :

The original comment is out of date. I've since been told of a way to return a dictionary from an exposed API method.

Revision history for this message
Jeroen T. Vermeulen (jtv) wrote :

Looks fine overall, and with a few nice drive-by improvements. A few smaller things:

Docstring, line 86 of the diff:

+ Either from the external specified in Branch.url, from the URL on
+ http://bazaar.launchpad.net/ or the lp: URL.

I know it's ugly after a URL, but consider replacing the
    " or "
with
    ", or from ".

Also, if there's any real cost associated with a lookup, you might want to do the lookup for set(urls) instead of urls in branchlookup.py (line 146 of the diff), just in case you get a lot of insane requests with duplicated URLs.

Finally, in line 130 of the diff:

+ # XXX: Move this to IBranchLookup

Does that still make sense? AFAICS you already have. If so, remove the comment; if not, file a bug and include its number in the comment.

Jeroen

review: Approve (code)
Revision history for this message
Jonathan Lange (jml) wrote :

On Thu, Nov 19, 2009 at 11:19 PM, Jeroen T. Vermeulen <email address hidden> wrote:
> Review: Approve code
> Looks fine overall, and with a few nice drive-by improvements.  A few smaller things:
>

Thanks.

> Docstring, line 86 of the diff:
>
> + Either from the external specified in Branch.url, from the URL on
> + http://bazaar.launchpad.net/ or the lp: URL.
>
> I know it's ugly after a URL, but consider replacing the
>    " or "
> with
>    ", or from ".
>

Changed.

> Also, if there's any real cost associated with a lookup, you might want to do the lookup for set(urls) instead of urls in branchlookup.py (line 146 of the diff), just in case you get a lot of insane requests with duplicated URLs.
>

Good idea. Done.

> Finally, in line 130 of the diff:
>
> + # XXX: Move this to IBranchLookup
>
> Does that still make sense?  AFAICS you already have.  If so, remove the comment; if not, file a bug and include its number in the comment.
>

Removed.

jml

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'lib/canonical/launchpad/pagetests/webservice/xx-branches.txt'
--- lib/canonical/launchpad/pagetests/webservice/xx-branches.txt 2009-07-04 16:34:20 +0000
+++ lib/canonical/launchpad/pagetests/webservice/xx-branches.txt 2009-11-20 16:01:17 +0000
@@ -17,6 +17,9 @@
17 >>> branch = factory.makeAnyBranch(branch_type=BranchType.MIRRORED)17 >>> branch = factory.makeAnyBranch(branch_type=BranchType.MIRRORED)
18 >>> branch_url = branch.url18 >>> branch_url = branch.url
19 >>> branch_unique_name = branch.unique_name19 >>> branch_unique_name = branch.unique_name
20 >>> branch2 = factory.makeAnyBranch()
21 >>> branch2_url = branch2.bzr_identity
22 >>> branch2_unique_name = branch2.unique_name
20 >>> logout()23 >>> logout()
2124
2225
@@ -26,3 +29,33 @@
26 ... '/branches?ws.op=getByUrl&url=%s' % branch_url).jsonBody()29 ... '/branches?ws.op=getByUrl&url=%s' % branch_url).jsonBody()
27 >>> found_branch['unique_name'] == branch_unique_name30 >>> found_branch['unique_name'] == branch_unique_name
28 True31 True
32
33
34=== Getting many branches by URL ===
35
36The branches collection has a helper to get a lot of branches at once. This
37saves roundtrips and provides potential performance improvements.
38
39 >>> doesnt_exist = 'http://example.com/doesntexist'
40 >>> branches = webservice.get(
41 ... '/branches?ws.op=getByUrls&urls=%s&urls=%s&urls=%s'
42 ... % (branch_url, doesnt_exist, branch2_url))
43 >>> branches = branches.jsonBody()
44
45We gave three URLs, so we get back a dict with three branches.
46
47 >>> print len(branches)
48 3
49
50The URL that refers to a branch that doesn't exist maps to None.
51
52 >>> print branches[doesnt_exist]
53 None
54
55The URLs that refer to real, honest-to-goodness existing branches map to those
56branches:
57
58 >>> print branches[branch_url]['unique_name'] == branch_unique_name
59 True
60 >>> print branches[branch2_url]['unique_name'] == branch2_unique_name
61 True
2962
=== modified file 'lib/lp/code/interfaces/branch.py'
--- lib/lp/code/interfaces/branch.py 2009-11-09 17:59:18 +0000
+++ lib/lp/code/interfaces/branch.py 2009-11-20 16:01:17 +0000
@@ -50,7 +50,8 @@
50 call_with, collection_default_content, export_as_webservice_collection,50 call_with, collection_default_content, export_as_webservice_collection,
51 export_as_webservice_entry, export_factory_operation,51 export_as_webservice_entry, export_factory_operation,
52 export_operation_as, export_read_operation, export_write_operation,52 export_operation_as, export_read_operation, export_write_operation,
53 exported, operation_parameters, operation_returns_entry, REQUEST_USER)53 exported, operation_parameters, operation_returns_collection_of,
54 operation_returns_entry, REQUEST_USER)
5455
55from canonical.config import config56from canonical.config import config
5657
@@ -1169,9 +1170,38 @@
1169 Either from the external specified in Branch.url, from the URL on1170 Either from the external specified in Branch.url, from the URL on
1170 http://bazaar.launchpad.net/ or the lp: URL.1171 http://bazaar.launchpad.net/ or the lp: URL.
11711172
1173 This is a frontend shim to `IBranchLookup.getByUrl` to allow it to be
1174 exported over the API. If you want to call this from within the
1175 Launchpad app, use the `IBranchLookup` version instead.
1176
1172 Return None if no match was found.1177 Return None if no match was found.
1173 """1178 """
11741179
1180 @operation_parameters(
1181 urls=List(
1182 title=u'A list of URLs of branches',
1183 description=(
1184 u'These can be URLs external to '
1185 u'Launchpad, lp: URLs, or http://bazaar.launchpad.net/ URLs, '
1186 u'or any mix of all these different kinds.'),
1187 value_type=TextLine(),
1188 required=True))
1189 @export_read_operation()
1190 def getByUrls(urls):
1191 """Finds branches by URL.
1192
1193 Either from the external specified in Branch.url, from the URL on
1194 http://bazaar.launchpad.net/, or from the lp: URL.
1195
1196 This is a frontend shim to `IBranchLookup.getByUrls` to allow it to be
1197 exported over the API. If you want to call this from within the
1198 Launchpad app, use the `IBranchLookup` version instead.
1199
1200 :param urls: An iterable of URLs expressed as strings.
1201 :return: A dictionary mapping URLs to branches. If the URL has no
1202 associated branch, the URL will map to `None`.
1203 """
1204
1175 @collection_default_content()1205 @collection_default_content()
1176 def getBranches(limit=50):1206 def getBranches(limit=50):
1177 """Return a collection of branches."""1207 """Return a collection of branches."""
11781208
=== modified file 'lib/lp/code/interfaces/branchlookup.py'
--- lib/lp/code/interfaces/branchlookup.py 2009-10-20 01:46:10 +0000
+++ lib/lp/code/interfaces/branchlookup.py 2009-11-20 16:01:17 +0000
@@ -100,6 +100,14 @@
100 Return None if no match was found.100 Return None if no match was found.
101 """101 """
102102
103 def getByUrls(urls):
104 """Find branches by URL.
105
106 :param urls: A list of URLs expressed as strings.
107 :return: A dictionary mapping those URLs to `IBranch` objects. If
108 there is no branch for a URL, the URL is mapped to `None` instead.
109 """
110
103 def getByLPPath(path):111 def getByLPPath(path):
104 """Find the branch associated with an lp: path.112 """Find the branch associated with an lp: path.
105113
106114
=== modified file 'lib/lp/code/model/branch.py'
--- lib/lp/code/model/branch.py 2009-11-05 20:26:48 +0000
+++ lib/lp/code/model/branch.py 2009-11-20 16:01:17 +0000
@@ -1184,6 +1184,10 @@
1184 """See `IBranchSet`."""1184 """See `IBranchSet`."""
1185 return getUtility(IBranchLookup).getByUrl(url)1185 return getUtility(IBranchLookup).getByUrl(url)
11861186
1187 def getByUrls(self, urls):
1188 """See `IBranchSet`."""
1189 return getUtility(IBranchLookup).getByUrls(urls)
1190
1187 def getBranches(self, limit=50):1191 def getBranches(self, limit=50):
1188 """See `IBranchSet`."""1192 """See `IBranchSet`."""
1189 anon_branches = getUtility(IAllBranches).visibleByUser(None)1193 anon_branches = getUtility(IAllBranches).visibleByUser(None)
11901194
=== modified file 'lib/lp/code/model/branchlookup.py'
--- lib/lp/code/model/branchlookup.py 2009-10-28 22:55:08 +0000
+++ lib/lp/code/model/branchlookup.py 2009-11-20 16:01:17 +0000
@@ -239,6 +239,10 @@
239239
240 return Branch.selectOneBy(url=url)240 return Branch.selectOneBy(url=url)
241241
242 def getByUrls(self, urls):
243 """See `IBranchLookup`."""
244 return dict((url, self.getByUrl(url)) for url in set(urls))
245
242 def getByUniqueName(self, unique_name):246 def getByUniqueName(self, unique_name):
243 """Find a branch by its unique name.247 """Find a branch by its unique name.
244248
245249
=== modified file 'lib/lp/code/model/tests/test_branchlookup.py'
--- lib/lp/code/model/tests/test_branchlookup.py 2009-10-29 02:41:18 +0000
+++ lib/lp/code/model/tests/test_branchlookup.py 2009-11-20 16:01:17 +0000
@@ -309,6 +309,19 @@
309 branch2 = branch_set.getByUrl('lp://edge/~aa/b/c')309 branch2 = branch_set.getByUrl('lp://edge/~aa/b/c')
310 self.assertEqual(branch, branch2)310 self.assertEqual(branch, branch2)
311311
312 def test_getByUrls(self):
313 # getByUrls returns a dictionary mapping branches to URLs.
314 branch1 = self.factory.makeAnyBranch()
315 branch2 = self.factory.makeAnyBranch()
316 url3 = 'http://example.com/%s' % self.factory.getUniqueString()
317 branch_set = getUtility(IBranchLookup)
318 branches = branch_set.getByUrls(
319 [branch1.bzr_identity, branch2.bzr_identity, url3])
320 self.assertEqual(
321 {branch1.bzr_identity: branch1,
322 branch2.bzr_identity: branch2,
323 url3: None}, branches)
324
312 def test_uriToUniqueName(self):325 def test_uriToUniqueName(self):
313 """Ensure uriToUniqueName works.326 """Ensure uriToUniqueName works.
314327
315328
=== modified file 'lib/lp/code/model/tests/test_branchset.py'
--- lib/lp/code/model/tests/test_branchset.py 2009-06-25 04:06:00 +0000
+++ lib/lp/code/model/tests/test_branchset.py 2009-11-20 16:01:17 +0000
@@ -5,32 +5,27 @@
55
6__metaclass__ = type6__metaclass__ = type
77
8from unittest import TestCase, TestLoader8from unittest import TestLoader
99
10from canonical.launchpad.ftests import login, logout, ANONYMOUS, syncUpdate
11from lp.code.model.branch import BranchSet10from lp.code.model.branch import BranchSet
12from lp.code.enums import BranchLifecycleStatus11from lp.code.enums import BranchLifecycleStatus
13from lp.registry.interfaces.product import IProductSet12from lp.registry.interfaces.product import IProductSet
13from lp.testing import TestCaseWithFactory
14from canonical.testing import DatabaseFunctionalLayer14from canonical.testing import DatabaseFunctionalLayer
1515
16from zope.component import getUtility16from zope.component import getUtility
17from zope.security.proxy import removeSecurityProxy17from zope.security.proxy import removeSecurityProxy
1818
1919
20class TestBranchSet(TestCase):20class TestBranchSet(TestCaseWithFactory):
2121
22 layer = DatabaseFunctionalLayer22 layer = DatabaseFunctionalLayer
2323
24 def setUp(self):24 def setUp(self):
25 TestCase.setUp(self)25 TestCaseWithFactory.setUp(self)
26 login(ANONYMOUS)
27 self.product = getUtility(IProductSet).getByName('firefox')26 self.product = getUtility(IProductSet).getByName('firefox')
28 self.branch_set = BranchSet()27 self.branch_set = BranchSet()
2928
30 def tearDown(self):
31 logout()
32 TestCase.tearDown(self)
33
34 def test_limitedByQuantity(self):29 def test_limitedByQuantity(self):
35 """When getting the latest branches for a product, we can specify the30 """When getting the latest branches for a product, we can specify the
36 maximum number of branches we want to know about.31 maximum number of branches we want to know about.
@@ -64,11 +59,26 @@
64 # change it.59 # change it.
65 branch = removeSecurityProxy(branch)60 branch = removeSecurityProxy(branch)
66 branch.lifecycle_status = BranchLifecycleStatus.ABANDONED61 branch.lifecycle_status = BranchLifecycleStatus.ABANDONED
67 syncUpdate(branch)
68 latest_branches = list(62 latest_branches = list(
69 self.branch_set.getLatestBranchesForProduct(self.product, 5))63 self.branch_set.getLatestBranchesForProduct(self.product, 5))
70 self.assertEqual(original_branches[1:], latest_branches)64 self.assertEqual(original_branches[1:], latest_branches)
7165
66 def test_getByUrls(self):
67 # getByUrls returns a list of branches matching the list of URLs that
68 # it's given.
69 a = self.factory.makeAnyBranch()
70 b = self.factory.makeAnyBranch()
71 branches = self.branch_set.getByUrls(
72 [a.bzr_identity, b.bzr_identity])
73 self.assertEqual({a.bzr_identity: a, b.bzr_identity: b}, branches)
74
75 def test_getByUrls_cant_find_url(self):
76 # If a branch cannot be found for a URL, then None appears in the list
77 # in place of the branch.
78 url = 'http://example.com/doesntexist'
79 branches = self.branch_set.getByUrls([url])
80 self.assertEqual({url: None}, branches)
81
7282
73def test_suite():83def test_suite():
74 return TestLoader().loadTestsFromName(__name__)84 return TestLoader().loadTestsFromName(__name__)