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

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
1=== modified file 'lib/canonical/launchpad/pagetests/webservice/xx-branches.txt'
2--- lib/canonical/launchpad/pagetests/webservice/xx-branches.txt 2009-07-04 16:34:20 +0000
3+++ lib/canonical/launchpad/pagetests/webservice/xx-branches.txt 2009-11-20 16:01:17 +0000
4@@ -17,6 +17,9 @@
5 >>> branch = factory.makeAnyBranch(branch_type=BranchType.MIRRORED)
6 >>> branch_url = branch.url
7 >>> branch_unique_name = branch.unique_name
8+ >>> branch2 = factory.makeAnyBranch()
9+ >>> branch2_url = branch2.bzr_identity
10+ >>> branch2_unique_name = branch2.unique_name
11 >>> logout()
12
13
14@@ -26,3 +29,33 @@
15 ... '/branches?ws.op=getByUrl&url=%s' % branch_url).jsonBody()
16 >>> found_branch['unique_name'] == branch_unique_name
17 True
18+
19+
20+=== Getting many branches by URL ===
21+
22+The branches collection has a helper to get a lot of branches at once. This
23+saves roundtrips and provides potential performance improvements.
24+
25+ >>> doesnt_exist = 'http://example.com/doesntexist'
26+ >>> branches = webservice.get(
27+ ... '/branches?ws.op=getByUrls&urls=%s&urls=%s&urls=%s'
28+ ... % (branch_url, doesnt_exist, branch2_url))
29+ >>> branches = branches.jsonBody()
30+
31+We gave three URLs, so we get back a dict with three branches.
32+
33+ >>> print len(branches)
34+ 3
35+
36+The URL that refers to a branch that doesn't exist maps to None.
37+
38+ >>> print branches[doesnt_exist]
39+ None
40+
41+The URLs that refer to real, honest-to-goodness existing branches map to those
42+branches:
43+
44+ >>> print branches[branch_url]['unique_name'] == branch_unique_name
45+ True
46+ >>> print branches[branch2_url]['unique_name'] == branch2_unique_name
47+ True
48
49=== modified file 'lib/lp/code/interfaces/branch.py'
50--- lib/lp/code/interfaces/branch.py 2009-11-09 17:59:18 +0000
51+++ lib/lp/code/interfaces/branch.py 2009-11-20 16:01:17 +0000
52@@ -50,7 +50,8 @@
53 call_with, collection_default_content, export_as_webservice_collection,
54 export_as_webservice_entry, export_factory_operation,
55 export_operation_as, export_read_operation, export_write_operation,
56- exported, operation_parameters, operation_returns_entry, REQUEST_USER)
57+ exported, operation_parameters, operation_returns_collection_of,
58+ operation_returns_entry, REQUEST_USER)
59
60 from canonical.config import config
61
62@@ -1169,9 +1170,38 @@
63 Either from the external specified in Branch.url, from the URL on
64 http://bazaar.launchpad.net/ or the lp: URL.
65
66+ This is a frontend shim to `IBranchLookup.getByUrl` to allow it to be
67+ exported over the API. If you want to call this from within the
68+ Launchpad app, use the `IBranchLookup` version instead.
69+
70 Return None if no match was found.
71 """
72
73+ @operation_parameters(
74+ urls=List(
75+ title=u'A list of URLs of branches',
76+ description=(
77+ u'These can be URLs external to '
78+ u'Launchpad, lp: URLs, or http://bazaar.launchpad.net/ URLs, '
79+ u'or any mix of all these different kinds.'),
80+ value_type=TextLine(),
81+ required=True))
82+ @export_read_operation()
83+ def getByUrls(urls):
84+ """Finds branches by URL.
85+
86+ Either from the external specified in Branch.url, from the URL on
87+ http://bazaar.launchpad.net/, or from the lp: URL.
88+
89+ This is a frontend shim to `IBranchLookup.getByUrls` to allow it to be
90+ exported over the API. If you want to call this from within the
91+ Launchpad app, use the `IBranchLookup` version instead.
92+
93+ :param urls: An iterable of URLs expressed as strings.
94+ :return: A dictionary mapping URLs to branches. If the URL has no
95+ associated branch, the URL will map to `None`.
96+ """
97+
98 @collection_default_content()
99 def getBranches(limit=50):
100 """Return a collection of branches."""
101
102=== modified file 'lib/lp/code/interfaces/branchlookup.py'
103--- lib/lp/code/interfaces/branchlookup.py 2009-10-20 01:46:10 +0000
104+++ lib/lp/code/interfaces/branchlookup.py 2009-11-20 16:01:17 +0000
105@@ -100,6 +100,14 @@
106 Return None if no match was found.
107 """
108
109+ def getByUrls(urls):
110+ """Find branches by URL.
111+
112+ :param urls: A list of URLs expressed as strings.
113+ :return: A dictionary mapping those URLs to `IBranch` objects. If
114+ there is no branch for a URL, the URL is mapped to `None` instead.
115+ """
116+
117 def getByLPPath(path):
118 """Find the branch associated with an lp: path.
119
120
121=== modified file 'lib/lp/code/model/branch.py'
122--- lib/lp/code/model/branch.py 2009-11-05 20:26:48 +0000
123+++ lib/lp/code/model/branch.py 2009-11-20 16:01:17 +0000
124@@ -1184,6 +1184,10 @@
125 """See `IBranchSet`."""
126 return getUtility(IBranchLookup).getByUrl(url)
127
128+ def getByUrls(self, urls):
129+ """See `IBranchSet`."""
130+ return getUtility(IBranchLookup).getByUrls(urls)
131+
132 def getBranches(self, limit=50):
133 """See `IBranchSet`."""
134 anon_branches = getUtility(IAllBranches).visibleByUser(None)
135
136=== modified file 'lib/lp/code/model/branchlookup.py'
137--- lib/lp/code/model/branchlookup.py 2009-10-28 22:55:08 +0000
138+++ lib/lp/code/model/branchlookup.py 2009-11-20 16:01:17 +0000
139@@ -239,6 +239,10 @@
140
141 return Branch.selectOneBy(url=url)
142
143+ def getByUrls(self, urls):
144+ """See `IBranchLookup`."""
145+ return dict((url, self.getByUrl(url)) for url in set(urls))
146+
147 def getByUniqueName(self, unique_name):
148 """Find a branch by its unique name.
149
150
151=== modified file 'lib/lp/code/model/tests/test_branchlookup.py'
152--- lib/lp/code/model/tests/test_branchlookup.py 2009-10-29 02:41:18 +0000
153+++ lib/lp/code/model/tests/test_branchlookup.py 2009-11-20 16:01:17 +0000
154@@ -309,6 +309,19 @@
155 branch2 = branch_set.getByUrl('lp://edge/~aa/b/c')
156 self.assertEqual(branch, branch2)
157
158+ def test_getByUrls(self):
159+ # getByUrls returns a dictionary mapping branches to URLs.
160+ branch1 = self.factory.makeAnyBranch()
161+ branch2 = self.factory.makeAnyBranch()
162+ url3 = 'http://example.com/%s' % self.factory.getUniqueString()
163+ branch_set = getUtility(IBranchLookup)
164+ branches = branch_set.getByUrls(
165+ [branch1.bzr_identity, branch2.bzr_identity, url3])
166+ self.assertEqual(
167+ {branch1.bzr_identity: branch1,
168+ branch2.bzr_identity: branch2,
169+ url3: None}, branches)
170+
171 def test_uriToUniqueName(self):
172 """Ensure uriToUniqueName works.
173
174
175=== modified file 'lib/lp/code/model/tests/test_branchset.py'
176--- lib/lp/code/model/tests/test_branchset.py 2009-06-25 04:06:00 +0000
177+++ lib/lp/code/model/tests/test_branchset.py 2009-11-20 16:01:17 +0000
178@@ -5,32 +5,27 @@
179
180 __metaclass__ = type
181
182-from unittest import TestCase, TestLoader
183+from unittest import TestLoader
184
185-from canonical.launchpad.ftests import login, logout, ANONYMOUS, syncUpdate
186 from lp.code.model.branch import BranchSet
187 from lp.code.enums import BranchLifecycleStatus
188 from lp.registry.interfaces.product import IProductSet
189+from lp.testing import TestCaseWithFactory
190 from canonical.testing import DatabaseFunctionalLayer
191
192 from zope.component import getUtility
193 from zope.security.proxy import removeSecurityProxy
194
195
196-class TestBranchSet(TestCase):
197+class TestBranchSet(TestCaseWithFactory):
198
199 layer = DatabaseFunctionalLayer
200
201 def setUp(self):
202- TestCase.setUp(self)
203- login(ANONYMOUS)
204+ TestCaseWithFactory.setUp(self)
205 self.product = getUtility(IProductSet).getByName('firefox')
206 self.branch_set = BranchSet()
207
208- def tearDown(self):
209- logout()
210- TestCase.tearDown(self)
211-
212 def test_limitedByQuantity(self):
213 """When getting the latest branches for a product, we can specify the
214 maximum number of branches we want to know about.
215@@ -64,11 +59,26 @@
216 # change it.
217 branch = removeSecurityProxy(branch)
218 branch.lifecycle_status = BranchLifecycleStatus.ABANDONED
219- syncUpdate(branch)
220 latest_branches = list(
221 self.branch_set.getLatestBranchesForProduct(self.product, 5))
222 self.assertEqual(original_branches[1:], latest_branches)
223
224+ def test_getByUrls(self):
225+ # getByUrls returns a list of branches matching the list of URLs that
226+ # it's given.
227+ a = self.factory.makeAnyBranch()
228+ b = self.factory.makeAnyBranch()
229+ branches = self.branch_set.getByUrls(
230+ [a.bzr_identity, b.bzr_identity])
231+ self.assertEqual({a.bzr_identity: a, b.bzr_identity: b}, branches)
232+
233+ def test_getByUrls_cant_find_url(self):
234+ # If a branch cannot be found for a URL, then None appears in the list
235+ # in place of the branch.
236+ url = 'http://example.com/doesntexist'
237+ branches = self.branch_set.getByUrls([url])
238+ self.assertEqual({url: None}, branches)
239+
240
241 def test_suite():
242 return TestLoader().loadTestsFromName(__name__)