Merge lp:~cjwatson/launchpad/sprd-utilities into lp:launchpad

Proposed by Colin Watson on 2016-01-11
Status: Merged
Merged at revision: 17887
Proposed branch: lp:~cjwatson/launchpad/sprd-utilities
Merge into: lp:launchpad
Diff against target: 302 lines (+78/-32)
6 files modified
lib/lp/code/browser/sourcepackagerecipe.py (+15/-14)
lib/lp/code/configure.zcml (+10/-1)
lib/lp/code/interfaces/sourcepackagerecipe.py (+25/-1)
lib/lp/code/model/sourcepackagerecipe.py (+5/-3)
lib/lp/code/model/sourcepackagerecipebuild.py (+9/-5)
lib/lp/code/model/sourcepackagerecipedata.py (+14/-8)
To merge this branch: bzr merge lp:~cjwatson/launchpad/sprd-utilities
Reviewer Review Type Date Requested Status
William Grant code 2016-01-11 Approve on 2016-01-12
Review via email: mp+282218@code.launchpad.net

Commit message

Access recipe parsing via interfaces.

Description of the change

Access recipe parsing via interfaces.

This is a bit of refactoring before starting on Git recipe support; for that, we're going to want to push recipe text through a small amount of munging before passing it to RecipeParser, so it makes sense for it all to go through common code, and that ought to use an interface so that it can be used correctly from browser code.

To post a comment you must log in.
William Grant (wgrant) wrote :

I don't understand why there are two interfaces and utilities, but otherwise good.

review: Approve (code)
Colin Watson (cjwatson) wrote :

Mainly because the method that returns a RecipeBranch can't reasonably be a securedutility (there's no Zope interface declared for that, so returning proxied objects is unhelpful), while the method that returns a SourcePackageRecipeData should be. Also the naming feels a little more natural this way.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/code/browser/sourcepackagerecipe.py'
2--- lib/lp/code/browser/sourcepackagerecipe.py 2015-07-08 16:05:11 +0000
3+++ lib/lp/code/browser/sourcepackagerecipe.py 2016-01-11 19:38:23 +0000
4@@ -1,4 +1,4 @@
5-# Copyright 2010-2013 Canonical Ltd. This software is licensed under the
6+# Copyright 2010-2016 Canonical Ltd. This software is licensed under the
7 # GNU Affero General Public License version 3 (see the file LICENSE).
8
9 """SourcePackageRecipe views."""
10@@ -20,7 +20,6 @@
11 from bzrlib.plugins.builder.recipe import (
12 ForbiddenInstructionError,
13 RecipeParseError,
14- RecipeParser,
15 )
16 from lazr.lifecycle.event import ObjectModifiedEvent
17 from lazr.lifecycle.snapshot import Snapshot
18@@ -92,6 +91,7 @@
19 )
20 from lp.code.interfaces.branchtarget import IBranchTarget
21 from lp.code.interfaces.sourcepackagerecipe import (
22+ IRecipeBranchSource,
23 ISourcePackageRecipe,
24 ISourcePackageRecipeSource,
25 MINIMAL_RECIPE_TEXT,
26@@ -611,10 +611,11 @@
27 'distroseries',
28 'You must specify at least one series for daily builds.')
29 try:
30- parser = RecipeParser(data['recipe_text'])
31- parser.parse()
32- except RecipeParseError as error:
33- self.setFieldError('recipe_text', str(error))
34+ self.error_handler(
35+ getUtility(IRecipeBranchSource).getParsedRecipe,
36+ data['recipe_text'])
37+ except ErrorHandled:
38+ pass
39
40 def error_handler(self, callable, *args, **kwargs):
41 try:
42@@ -631,7 +632,7 @@
43 except NoSuchBranch as e:
44 self.setFieldError(
45 'recipe_text', '%s is not a branch on Launchpad.' % e.name)
46- except PrivateBranchRecipe as e:
47+ except (RecipeParseError, PrivateBranchRecipe) as e:
48 self.setFieldError('recipe_text', str(e))
49 raise ErrorHandled()
50
51@@ -872,14 +873,14 @@
52 self.context, providing=providedBy(self.context))
53
54 recipe_text = data.pop('recipe_text')
55- parser = RecipeParser(recipe_text)
56- recipe = parser.parse()
57- if self.context.builder_recipe != recipe:
58- try:
59+ try:
60+ recipe = self.error_handler(
61+ getUtility(IRecipeBranchSource).getParsedRecipe, recipe_text)
62+ if self.context.builder_recipe != recipe:
63 self.error_handler(self.context.setRecipeText, recipe_text)
64 changed = True
65- except ErrorHandled:
66- return
67+ except ErrorHandled:
68+ return
69
70 distros = data.pop('distroseries')
71 if distros != self.context.distroseries:
72@@ -927,7 +928,7 @@
73 label = title
74
75 class schema(Interface):
76- """Schema for deleting a branch."""
77+ """Schema for deleting a recipe."""
78
79 @property
80 def cancel_url(self):
81
82=== modified file 'lib/lp/code/configure.zcml'
83--- lib/lp/code/configure.zcml 2015-10-09 16:56:45 +0000
84+++ lib/lp/code/configure.zcml 2016-01-11 19:38:23 +0000
85@@ -1,4 +1,4 @@
86-<!-- Copyright 2009-2015 Canonical Ltd. This software is licensed under the
87+<!-- Copyright 2009-2016 Canonical Ltd. This software is licensed under the
88 GNU Affero General Public License version 3 (see the file LICENSE).
89 -->
90
91@@ -1053,6 +1053,15 @@
92 <require permission="launchpad.View"
93 interface="lp.code.interfaces.sourcepackagerecipe.ISourcePackageRecipeData"/>
94 </class>
95+ <utility
96+ component="lp.code.model.sourcepackagerecipedata.SourcePackageRecipeData"
97+ provides="lp.code.interfaces.sourcepackagerecipe.IRecipeBranchSource">
98+ </utility>
99+ <securedutility
100+ component="lp.code.model.sourcepackagerecipedata.SourcePackageRecipeData"
101+ provides="lp.code.interfaces.sourcepackagerecipe.ISourcePackageRecipeDataSource">
102+ <allow interface="lp.code.interfaces.sourcepackagerecipe.ISourcePackageRecipeDataSource"/>
103+ </securedutility>
104 <!-- SourcePackageRecipe -->
105 <class
106 class="lp.code.model.sourcepackagerecipe.SourcePackageRecipe">
107
108=== modified file 'lib/lp/code/interfaces/sourcepackagerecipe.py'
109--- lib/lp/code/interfaces/sourcepackagerecipe.py 2015-04-30 01:45:30 +0000
110+++ lib/lp/code/interfaces/sourcepackagerecipe.py 2016-01-11 19:38:23 +0000
111@@ -1,4 +1,4 @@
112-# Copyright 2009-2012 Canonical Ltd. This software is licensed under the
113+# Copyright 2009-2016 Canonical Ltd. This software is licensed under the
114 # GNU Affero General Public License version 3 (see the file LICENSE).
115
116 """Interface of the `SourcePackageRecipe` content type."""
117@@ -8,8 +8,10 @@
118
119
120 __all__ = [
121+ 'IRecipeBranchSource',
122 'ISourcePackageRecipe',
123 'ISourcePackageRecipeData',
124+ 'ISourcePackageRecipeDataSource',
125 'ISourcePackageRecipeSource',
126 'MINIMAL_RECIPE_TEXT',
127 ]
128@@ -88,6 +90,28 @@
129 """An iterator of the branches referenced by this recipe."""
130
131
132+class IRecipeBranchSource(Interface):
133+
134+ def getParsedRecipe(recipe_text):
135+ """Parse recipe text into recipe data.
136+
137+ :param recipe_text: Recipe text as a string.
138+ :return: a `RecipeBranch` representing the recipe.
139+ """
140+
141+
142+class ISourcePackageRecipeDataSource(Interface):
143+
144+ def createManifestFromText(text, sourcepackage_recipe_build):
145+ """Create a manifest for the specified build.
146+
147+ :param text: The text of the recipe to create a manifest for.
148+ :param sourcepackage_recipe_build: The build to associate the manifest
149+ with.
150+ :return: an instance of `SourcePackageRecipeData`.
151+ """
152+
153+
154 class ISourcePackageRecipeView(Interface):
155 """IBranch attributes that require launchpad.View permission."""
156
157
158=== modified file 'lib/lp/code/model/sourcepackagerecipe.py'
159--- lib/lp/code/model/sourcepackagerecipe.py 2015-09-28 17:38:45 +0000
160+++ lib/lp/code/model/sourcepackagerecipe.py 2016-01-11 19:38:23 +0000
161@@ -1,4 +1,4 @@
162-# Copyright 2009-2015 Canonical Ltd. This software is licensed under the
163+# Copyright 2009-2016 Canonical Ltd. This software is licensed under the
164 # GNU Affero General Public License version 3 (see the file LICENSE).
165
166 """Implementation of the `SourcePackageRecipe` content type."""
167@@ -42,6 +42,7 @@
168 BuildNotAllowedForDistro,
169 )
170 from lp.code.interfaces.sourcepackagerecipe import (
171+ IRecipeBranchSource,
172 ISourcePackageRecipe,
173 ISourcePackageRecipeData,
174 ISourcePackageRecipeSource,
175@@ -167,7 +168,7 @@
176 owner_ids, need_validity=True))
177
178 def setRecipeText(self, recipe_text):
179- parsed = SourcePackageRecipeData.getParsedRecipe(recipe_text)
180+ parsed = getUtility(IRecipeBranchSource).getParsedRecipe(recipe_text)
181 self._recipe_data.setRecipe(parsed)
182
183 @property
184@@ -187,7 +188,8 @@
185 """See `ISourcePackageRecipeSource.new`."""
186 store = IMasterStore(SourcePackageRecipe)
187 sprecipe = SourcePackageRecipe()
188- builder_recipe = SourcePackageRecipeData.getParsedRecipe(recipe)
189+ builder_recipe = getUtility(IRecipeBranchSource).getParsedRecipe(
190+ recipe)
191 SourcePackageRecipeData(builder_recipe, sprecipe)
192 sprecipe.registrant = registrant
193 sprecipe.owner = owner
194
195=== modified file 'lib/lp/code/model/sourcepackagerecipebuild.py'
196--- lib/lp/code/model/sourcepackagerecipebuild.py 2015-09-11 15:11:34 +0000
197+++ lib/lp/code/model/sourcepackagerecipebuild.py 2016-01-11 19:38:23 +0000
198@@ -1,4 +1,4 @@
199-# Copyright 2010-2013 Canonical Ltd. This software is licensed under the
200+# Copyright 2010-2016 Canonical Ltd. This software is licensed under the
201 # GNU Affero General Public License version 3 (see the file LICENSE).
202
203 """Implementation code for source package builds."""
204@@ -46,6 +46,10 @@
205 BuildAlreadyPending,
206 BuildNotAllowedForDistro,
207 )
208+from lp.code.interfaces.sourcepackagerecipe import (
209+ IRecipeBranchSource,
210+ ISourcePackageRecipeDataSource,
211+ )
212 from lp.code.interfaces.sourcepackagerecipebuild import (
213 ISourcePackageRecipeBuild,
214 ISourcePackageRecipeBuildSource,
215@@ -53,7 +57,6 @@
216 from lp.code.mail.sourcepackagerecipebuild import (
217 SourcePackageRecipeBuildMailer,
218 )
219-from lp.code.model.sourcepackagerecipedata import SourcePackageRecipeData
220 from lp.registry.interfaces.pocket import PackagePublishingPocket
221 from lp.registry.model.person import Person
222 from lp.services.database.bulk import load_related
223@@ -158,10 +161,11 @@
224 if self.manifest is not None:
225 IStore(self.manifest).remove(self.manifest)
226 elif self.manifest is None:
227- SourcePackageRecipeData.createManifestFromText(text, self)
228+ getUtility(ISourcePackageRecipeDataSource).createManifestFromText(
229+ text, self)
230 else:
231- from bzrlib.plugins.builder.recipe import RecipeParser
232- self.manifest.setRecipe(RecipeParser(text).parse())
233+ self.manifest.setRecipe(
234+ getUtility(IRecipeBranchSource).getParsedRecipe(text))
235
236 def getManifestText(self):
237 if self.manifest is None:
238
239=== modified file 'lib/lp/code/model/sourcepackagerecipedata.py'
240--- lib/lp/code/model/sourcepackagerecipedata.py 2015-02-25 11:21:43 +0000
241+++ lib/lp/code/model/sourcepackagerecipedata.py 2016-01-11 19:38:23 +0000
242@@ -1,4 +1,4 @@
243-# Copyright 2009-2015 Canonical Ltd. This software is licensed under the
244+# Copyright 2009-2016 Canonical Ltd. This software is licensed under the
245 # GNU Affero General Public License version 3 (see the file LICENSE).
246
247 """Implementation of the recipe storage.
248@@ -38,6 +38,10 @@
249 Unicode,
250 )
251 from zope.component import getUtility
252+from zope.interface import (
253+ implementer,
254+ provider,
255+ )
256
257 from lp.code.errors import (
258 NoSuchBranch,
259@@ -45,6 +49,11 @@
260 TooNewRecipeFormat,
261 )
262 from lp.code.interfaces.branchlookup import IBranchLookup
263+from lp.code.interfaces.sourcepackagerecipe import (
264+ IRecipeBranchSource,
265+ ISourcePackageRecipeData,
266+ ISourcePackageRecipeDataSource,
267+ )
268 from lp.code.model.branch import Branch
269 from lp.services.database.bulk import (
270 load_referencing,
271@@ -142,6 +151,8 @@
272 MAX_RECIPE_FORMAT = 0.4
273
274
275+@implementer(ISourcePackageRecipeData)
276+@provider(IRecipeBranchSource, ISourcePackageRecipeDataSource)
277 class SourcePackageRecipeData(Storm):
278 """The database representation of a BaseRecipeBranch from bzr-builder.
279
280@@ -177,6 +188,7 @@
281
282 @staticmethod
283 def getParsedRecipe(recipe_text):
284+ """See `IRecipeBranchSource`."""
285 parser = RecipeParser(recipe_text)
286 return parser.parse(permitted_instructions=SAFE_INSTRUCTIONS)
287
288@@ -202,13 +214,7 @@
289
290 @classmethod
291 def createManifestFromText(cls, text, sourcepackage_recipe_build):
292- """Create a manifest for the specified build.
293-
294- :param text: The text of the recipe to create a manifest for.
295- :param sourcepackage_recipe_build: The build to associate the manifest
296- with.
297- :return: an instance of SourcePackageRecipeData.
298- """
299+ """See `ISourcePackageRecipeDataSource`."""
300 parsed = cls.getParsedRecipe(text)
301 return cls(
302 parsed, sourcepackage_recipe_build=sourcepackage_recipe_build)