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