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

[Colin Watson]

Access recipe parsing via interfaces.

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.

[William Grant (wgrant)]

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

review: Approve (code)

[Colin Watson (cjwatson)]

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.

=== modified file 'lib/lp/code/browser/sourcepackagerecipe.py'
--- lib/lp/code/browser/sourcepackagerecipe.py	2015-07-08 16:05:11 +0000
+++ lib/lp/code/browser/sourcepackagerecipe.py	2016-01-11 19:38:23 +0000
@@ -1,4 +1,4 @@
-# Copyright 2010-2013 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2016 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """SourcePackageRecipe views."""
@@ -20,7 +20,6 @@
 from bzrlib.plugins.builder.recipe import (
     ForbiddenInstructionError,
     RecipeParseError,
-    RecipeParser,
     )
 from lazr.lifecycle.event import ObjectModifiedEvent
 from lazr.lifecycle.snapshot import Snapshot
@@ -92,6 +91,7 @@
     )
 from lp.code.interfaces.branchtarget import IBranchTarget
 from lp.code.interfaces.sourcepackagerecipe import (
+    IRecipeBranchSource,
     ISourcePackageRecipe,
     ISourcePackageRecipeSource,
     MINIMAL_RECIPE_TEXT,
@@ -611,10 +611,11 @@
                     'distroseries',
                     'You must specify at least one series for daily builds.')
         try:
-            parser = RecipeParser(data['recipe_text'])
-            parser.parse()
-        except RecipeParseError as error:
-            self.setFieldError('recipe_text', str(error))
+            self.error_handler(
+                getUtility(IRecipeBranchSource).getParsedRecipe,
+                data['recipe_text'])
+        except ErrorHandled:
+            pass
 
     def error_handler(self, callable, *args, **kwargs):
         try:
@@ -631,7 +632,7 @@
         except NoSuchBranch as e:
             self.setFieldError(
                 'recipe_text', '%s is not a branch on Launchpad.' % e.name)
-        except PrivateBranchRecipe as e:
+        except (RecipeParseError, PrivateBranchRecipe) as e:
             self.setFieldError('recipe_text', str(e))
         raise ErrorHandled()
 
@@ -872,14 +873,14 @@
             self.context, providing=providedBy(self.context))
 
         recipe_text = data.pop('recipe_text')
-        parser = RecipeParser(recipe_text)
-        recipe = parser.parse()
-        if self.context.builder_recipe != recipe:
-            try:
+        try:
+            recipe = self.error_handler(
+                getUtility(IRecipeBranchSource).getParsedRecipe, recipe_text)
+            if self.context.builder_recipe != recipe:
                 self.error_handler(self.context.setRecipeText, recipe_text)
                 changed = True
-            except ErrorHandled:
-                return
+        except ErrorHandled:
+            return
 
         distros = data.pop('distroseries')
         if distros != self.context.distroseries:
@@ -927,7 +928,7 @@
     label = title
 
     class schema(Interface):
-        """Schema for deleting a branch."""
+        """Schema for deleting a recipe."""
 
     @property
     def cancel_url(self):
 
=== modified file 'lib/lp/code/configure.zcml'
--- lib/lp/code/configure.zcml	2015-10-09 16:56:45 +0000
+++ lib/lp/code/configure.zcml	2016-01-11 19:38:23 +0000
@@ -1,4 +1,4 @@
-<!-- Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
+<!-- Copyright 2009-2016 Canonical Ltd.  This software is licensed under the
      GNU Affero General Public License version 3 (see the file LICENSE).
 -->
 
@@ -1053,6 +1053,15 @@
     <require permission="launchpad.View"
     interface="lp.code.interfaces.sourcepackagerecipe.ISourcePackageRecipeData"/>
   </class>
+  <utility
+    component="lp.code.model.sourcepackagerecipedata.SourcePackageRecipeData"
+    provides="lp.code.interfaces.sourcepackagerecipe.IRecipeBranchSource">
+  </utility>
+  <securedutility
+      component="lp.code.model.sourcepackagerecipedata.SourcePackageRecipeData"
+      provides="lp.code.interfaces.sourcepackagerecipe.ISourcePackageRecipeDataSource">
+    <allow interface="lp.code.interfaces.sourcepackagerecipe.ISourcePackageRecipeDataSource"/>
+  </securedutility>
   <!-- SourcePackageRecipe -->
   <class
      class="lp.code.model.sourcepackagerecipe.SourcePackageRecipe">
 
=== modified file 'lib/lp/code/interfaces/sourcepackagerecipe.py'
--- lib/lp/code/interfaces/sourcepackagerecipe.py	2015-04-30 01:45:30 +0000
+++ lib/lp/code/interfaces/sourcepackagerecipe.py	2016-01-11 19:38:23 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2016 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Interface of the `SourcePackageRecipe` content type."""
@@ -8,8 +8,10 @@
 
 
 __all__ = [
+    'IRecipeBranchSource',
     'ISourcePackageRecipe',
     'ISourcePackageRecipeData',
+    'ISourcePackageRecipeDataSource',
     'ISourcePackageRecipeSource',
     'MINIMAL_RECIPE_TEXT',
     ]
@@ -88,6 +90,28 @@
         """An iterator of the branches referenced by this recipe."""
 
 
+class IRecipeBranchSource(Interface):
+
+    def getParsedRecipe(recipe_text):
+        """Parse recipe text into recipe data.
+
+        :param recipe_text: Recipe text as a string.
+        :return: a `RecipeBranch` representing the recipe.
+        """
+
+
+class ISourcePackageRecipeDataSource(Interface):
+
+    def createManifestFromText(text, sourcepackage_recipe_build):
+        """Create a manifest for the specified build.
+
+        :param text: The text of the recipe to create a manifest for.
+        :param sourcepackage_recipe_build: The build to associate the manifest
+            with.
+        :return: an instance of `SourcePackageRecipeData`.
+        """
+
+
 class ISourcePackageRecipeView(Interface):
     """IBranch attributes that require launchpad.View permission."""
 
 
=== modified file 'lib/lp/code/model/sourcepackagerecipe.py'
--- lib/lp/code/model/sourcepackagerecipe.py	2015-09-28 17:38:45 +0000
+++ lib/lp/code/model/sourcepackagerecipe.py	2016-01-11 19:38:23 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2016 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Implementation of the `SourcePackageRecipe` content type."""
@@ -42,6 +42,7 @@
     BuildNotAllowedForDistro,
     )
 from lp.code.interfaces.sourcepackagerecipe import (
+    IRecipeBranchSource,
     ISourcePackageRecipe,
     ISourcePackageRecipeData,
     ISourcePackageRecipeSource,
@@ -167,7 +168,7 @@
             owner_ids, need_validity=True))
 
     def setRecipeText(self, recipe_text):
-        parsed = SourcePackageRecipeData.getParsedRecipe(recipe_text)
+        parsed = getUtility(IRecipeBranchSource).getParsedRecipe(recipe_text)
         self._recipe_data.setRecipe(parsed)
 
     @property
@@ -187,7 +188,8 @@
         """See `ISourcePackageRecipeSource.new`."""
         store = IMasterStore(SourcePackageRecipe)
         sprecipe = SourcePackageRecipe()
-        builder_recipe = SourcePackageRecipeData.getParsedRecipe(recipe)
+        builder_recipe = getUtility(IRecipeBranchSource).getParsedRecipe(
+            recipe)
         SourcePackageRecipeData(builder_recipe, sprecipe)
         sprecipe.registrant = registrant
         sprecipe.owner = owner
 
=== modified file 'lib/lp/code/model/sourcepackagerecipebuild.py'
--- lib/lp/code/model/sourcepackagerecipebuild.py	2015-09-11 15:11:34 +0000
+++ lib/lp/code/model/sourcepackagerecipebuild.py	2016-01-11 19:38:23 +0000
@@ -1,4 +1,4 @@
-# Copyright 2010-2013 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2016 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Implementation code for source package builds."""
@@ -46,6 +46,10 @@
     BuildAlreadyPending,
     BuildNotAllowedForDistro,
     )
+from lp.code.interfaces.sourcepackagerecipe import (
+    IRecipeBranchSource,
+    ISourcePackageRecipeDataSource,
+    )
 from lp.code.interfaces.sourcepackagerecipebuild import (
     ISourcePackageRecipeBuild,
     ISourcePackageRecipeBuildSource,
@@ -53,7 +57,6 @@
 from lp.code.mail.sourcepackagerecipebuild import (
     SourcePackageRecipeBuildMailer,
     )
-from lp.code.model.sourcepackagerecipedata import SourcePackageRecipeData
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.registry.model.person import Person
 from lp.services.database.bulk import load_related
@@ -158,10 +161,11 @@
             if self.manifest is not None:
                 IStore(self.manifest).remove(self.manifest)
         elif self.manifest is None:
-            SourcePackageRecipeData.createManifestFromText(text, self)
+            getUtility(ISourcePackageRecipeDataSource).createManifestFromText(
+                text, self)
         else:
-            from bzrlib.plugins.builder.recipe import RecipeParser
-            self.manifest.setRecipe(RecipeParser(text).parse())
+            self.manifest.setRecipe(
+                getUtility(IRecipeBranchSource).getParsedRecipe(text))
 
     def getManifestText(self):
         if self.manifest is None:
 
=== modified file 'lib/lp/code/model/sourcepackagerecipedata.py'
--- lib/lp/code/model/sourcepackagerecipedata.py	2015-02-25 11:21:43 +0000
+++ lib/lp/code/model/sourcepackagerecipedata.py	2016-01-11 19:38:23 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2016 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Implementation of the recipe storage.
@@ -38,6 +38,10 @@
     Unicode,
     )
 from zope.component import getUtility
+from zope.interface import (
+    implementer,
+    provider,
+    )
 
 from lp.code.errors import (
     NoSuchBranch,
@@ -45,6 +49,11 @@
     TooNewRecipeFormat,
     )
 from lp.code.interfaces.branchlookup import IBranchLookup
+from lp.code.interfaces.sourcepackagerecipe import (
+    IRecipeBranchSource,
+    ISourcePackageRecipeData,
+    ISourcePackageRecipeDataSource,
+    )
 from lp.code.model.branch import Branch
 from lp.services.database.bulk import (
     load_referencing,
@@ -142,6 +151,8 @@
 MAX_RECIPE_FORMAT = 0.4
 
 
+@implementer(ISourcePackageRecipeData)
+@provider(IRecipeBranchSource, ISourcePackageRecipeDataSource)
 class SourcePackageRecipeData(Storm):
     """The database representation of a BaseRecipeBranch from bzr-builder.
 
@@ -177,6 +188,7 @@
 
     @staticmethod
     def getParsedRecipe(recipe_text):
+        """See `IRecipeBranchSource`."""
         parser = RecipeParser(recipe_text)
         return parser.parse(permitted_instructions=SAFE_INSTRUCTIONS)
 
@@ -202,13 +214,7 @@
 
     @classmethod
     def createManifestFromText(cls, text, sourcepackage_recipe_build):
-        """Create a manifest for the specified build.
-
-        :param text: The text of the recipe to create a manifest for.
-        :param sourcepackage_recipe_build: The build to associate the manifest
-            with.
-        :return: an instance of SourcePackageRecipeData.
-        """
+        """See `ISourcePackageRecipeDataSource`."""
         parsed = cls.getParsedRecipe(text)
         return cls(
             parsed, sourcepackage_recipe_build=sourcepackage_recipe_build)